Don't wrap EventChord around rhythmic events by default.

Don't wrap EventChord around rhythmic events by default.

This changes quite a number of things and required quite a few code
changes.  It is obvious that there is a lot of code duplication in
Lilypond.

A number of regtest differences show up: most of them actually
demonstrate bugs in the preexisting code base that fails to typeset
things like <c-.> with the same carefulness as c-. is typeset.  Since
c-. is no longer treated like <c>-. this becomes obvious in a number
of cases.

The part combiner has several problems, the tablature code has a few,
relying on string numbers getting lost by default does no longer work,
the fingering engraver is affected.

Displaying music obviously is no longer working; this is not a defect
of the old code but rather needs work to match the new realities.

All in all, things could be worse.

This is merely a first sketch and should at least provide for
interesting experiments.

[MikeSol, 2011-12-02 19:50:03]

Hey David,

This patch is way over my head, so I can't really comment on the details of the
implementation, but I just wanted to voice one concern for me (and possibly
other) algorithmic composers.

I have a lot of algorithms written that comb through music streams and finagle
with the elements.  EventChord is the bread and butter unit that most of these
algorithms search for and then slice & dice.

What I'd ask is that lilypond could have an option
(-dwrap-single-rhythmic-events-in-event-chord) that allows for this type of
behavior to continue.  It'll give people in my position time to migrate code w/o
having it completely break on us.  I'm not sure if this is feasible given the
current design of the patch (again, it's over my head), but around line 765,
perhaps keep the old syntax but use a the results from the
-dwarp-single-rhythmic-events-in-event-chord flag in the if statement instead of
!is_event?

Cheers,
MS

[dak, 2011-12-03 08:27:14]

On 2011/12/02 19:50:03, MikeSol wrote:
> Hey David,
> 
> This patch is way over my head, so I can't really comment on the details of
the
> implementation, but I just wanted to voice one concern for me (and possibly
> other) algorithmic composers.
> 
> I have a lot of algorithms written that comb through music streams and finagle
> with the elements.  EventChord is the bread and butter unit that most of these
> algorithms search for and then slice & dice.
> 
> What I'd ask is that lilypond could have an option
> (-dwrap-single-rhythmic-events-in-event-chord) that allows for this type of
> behavior to continue.  It'll give people in my position time to migrate code
w/o
> having it completely break on us.  I'm not sure if this is feasible given the
> current design of the patch (again, it's over my head), but around line 765,
> perhaps keep the old syntax but use a the results from the
> -dwarp-single-rhythmic-events-in-event-chord flag in the if statement instead
of
> !is_event?

Not feasible.  The main motivation for the patch was to be able to have sensible
semantics for music functions inside of chords.  That's incompatible with
putting EventChord around everything.

The place for a compatibility hook would likely be when stuff is collected into
a (sequential or simultanous) list.  Few people complained or even noticed when
#{ ... #} did not make a sequential list from single events.

However, displaying music expressions will then produce chorded expressions, and
c-. will then turn into <c-.> rather than <c>-. so it is not all that clear that
this kind of mode will take you all the way.

One of the problems for any change currently simply is that Lilypond has no
sensible programmer APIs to process music.  It's all hand-coded manipulation of
raw music, without any consistency or code reuse even between different parts of
Lilypond itself.

[dak, 2011-12-22 12:09:06]

Ok, I need some pointers here.  In order to make this work compatibly at the
lowest level, articulations need to behave differently depending on whether they
are on note events that are children of an EventChord (which all of them are
currently) or not.

Ok, so now since I don't actually have a clue, the following are basically
buzzwords that I imagine could be in some relation to reality.

So articulations basically need to announce themselves as two event types, and
an EventChord installs a listener for the first type and routes those events to
per-note/notehead engravers.  And if no EventChord actually is listening, they
announce themselves as music events (i.e., not tied to a particular note) and
get typeset in that way.

I have no clue about what I need to install where and how to get this working,
and where I would find information for doing that.  But it sounds like this is
the sort of level where making the distinction between articulations on single
notes inside of a chord, and articulations on a chord (or note) as a whole
should really be done if one wants a non-tricky logically consistent behavior
for making c-. and <c>-. equivalent and different from <c-.>.

And I am pretty much stuck here at the moment.

[dak, 2012-01-20 12:53:12]

Ok, this latest iteration is quite close to what I want to end up committing. 
All the work of picking apart articulations and events have gone into the
rhythmic event iterator.  What it does is to broadcast all articulations that
have a listener, and keep the rest as articulations.

One side effect will be that things like c\3 will exhibit a string number
(previously, only <c\3> did).  Another side effect is that tweaks can work on
single notes outside of chords even when those have non-articulation postevents.

You get an EventChord iff you write < ... >.  A note getting postevents always
receives them in 'articulations, whether it is part of an EventChord or not.

Postevents on a chord get appended to the EventChord members as previously.

This is total straightforward.  I have _removed_ the compatibility option since
it
a) considerably complicated code, grammar and semantics
b) was not able to a reasonably reliable job

Instead, it will make sense to write a music function that tacks EventChord
around naked rhythmic events (unwrapping the articulations in the process) and
use that manually
for converting music expressions to the old style when one does not want to
adjust one's internals.

In spite of the remaining problems with spanners and trills (slated to be
removed in the next days), this should be good for testing how hand-written
analysis code fares.  There should be no difference to the way synthesized code
(in the old style) gets typeset.

So give this a testing now or after I shook out the last bugs.  I am really
planning to get this into 2.16.

[md5i, 2012-01-20 13:55:56]

http://codereview.appspot.com/5440084/diff/7001/lily/parser.yy
File lily/parser.yy (right):

http://codereview.appspot.com/5440084/diff/7001/lily/parser.yy#newcode432
lily/parser.yy:432: %type <scm> list_music
I must *strongly* recommend that the name of either music_list or list_music be
changed.  Even if the names make distinct sense, it is far to easy to transpose
identifiers like this when reading or writing code.  (I have made this mistake
in my own code many times in the past.)  Given the existence of other _list
types, I suggest that the name of list_music be changed.  Maybe "wrapped_music"
or "music_chord"...

[MikeSol, 2012-01-20 14:08:18]

http://codereview.appspot.com/5440084/diff/11001/lily/rhythmic-music-iterator.cc
File lily/rhythmic-music-iterator.cc (right):

http://codereview.appspot.com/5440084/diff/11001/lily/rhythmic-music-iterator...
lily/rhythmic-music-iterator.cc:62: if (scm_is_true
ly_lily_module_constant should only be called for things that don't exist in
C++.  Otherwise, it's better to use the C++ function.  In this case:
ly_is_listened_event_class
(in translator.cc)

[dak, 2012-01-20 14:52:09]

http://codereview.appspot.com/5440084/diff/7001/lily/parser.yy
File lily/parser.yy (right):

http://codereview.appspot.com/5440084/diff/7001/lily/parser.yy#newcode432
lily/parser.yy:432: %type <scm> list_music
On 2012/01/20 13:55:56, md5i wrote:
> I must *strongly* recommend that the name of either music_list or list_music
be
> changed.  Even if the names make distinct sense, it is far to easy to
transpose
> identifiers like this when reading or writing code.  (I have made this mistake
> in my own code many times in the past.)  Given the existence of other _list
> types, I suggest that the name of list_music be changed.  Maybe
"wrapped_music"
> or "music_chord"...

Or nothing at all.  This nonterminal is not in the current patch.  It was part
of -devent-chord-wrapper which was not reliable enough to be worth the trouble.

[dak, 2012-01-20 14:55:38]

On 2012/01/20 14:08:18, MikeSol wrote:
>
http://codereview.appspot.com/5440084/diff/11001/lily/rhythmic-music-iterator.cc
> File lily/rhythmic-music-iterator.cc (right):
> 
>
http://codereview.appspot.com/5440084/diff/11001/lily/rhythmic-music-iterator...
> lily/rhythmic-music-iterator.cc:62: if (scm_is_true
> ly_lily_module_constant should only be called for things that don't exist in
> C++.

I've seen that elsewhere.  Maybe to support redefining the function before it is
first memoized.

> Otherwise, it's better to use the C++ function.  In this case:
> ly_is_listened_event_class
> (in translator.cc)

Will do.

[dak, 2012-01-20 15:33:07]

On 2012/01/20 14:55:38, dak wrote:
> On 2012/01/20 14:08:18, MikeSol wrote:
> 
> > Otherwise, it's better to use the C++ function.  In this case:
> > ly_is_listened_event_class
> > (in translator.cc)
> 
> Will do.

Very funny.  After putting the (required) prototype into
translator.hh, it takes hours to recompile on my slow machine.

Rest is shaping up.  For your programmatic uses of LilyPond, consider
using "extract-named-music" and "extract-typed-music" (yes, you'll get
to see _that_ only after I got through with !@#@! recompilation but
then I might fork it out into a separate commit) since they get rather
transparently at material without bothering about NoteEvent layers in
between.

Should I be forking the rhythmic-music-iterator stuff into the
separate review (it's dormant code elsewhere, so Patchy is not all too
informative once it compiles) or is it visible enough in here?  It is,
after all, pretty well-separated and requires the Issue 2233 patch as
basis (currently also contained in this review but going to leave once
I commit and it percolates to master).

[MikeSol, 2012-01-20 15:37:13]

On 2012/01/20 15:33:07, dak wrote:

> Should I be forking the rhythmic-music-iterator stuff into the
> separate review

I'd say leave it with this - it's easy to forget that two patches need to be
tested in concert.

[Neil Puttock, 2012-01-20 16:32:12]

Hi David,

Should I wait for a new patch or can I test using the latest one here?

I've tried it out briefly on a real music example, and have a problem with
identifiers:

foo = \mark \default

\relative c' {
 \foo
  c1
}

-> error: syntax error, unexpected EVENT_IDENTIFIER
 
 \foo

If I add a note before the identifier, it gets erroneously added to the
articulations list of the first NoteEvent, and is typeset in the wrong place (at
the start of the system).

Cheers,
Neil

[dak, 2012-01-20 17:33:44]

n.puttock@gmail.com writes:

> Hi David,
>
> Should I wait for a new patch or can I test using the latest one here?
>
> I've tried it out briefly on a real music example, and have a problem
> with identifiers:
>
> foo = \mark \default
>
> \relative c' {
>  \foo
>   c1
> }

You have the newest.  The problem is the definition of ly:event? (for
recognizing postevents).  It is currently (in lily/music-scheme.cc)
defined as the equivalent of
(define (ly:event? m)
        (and (ly:music? m)
             (ly:is-music-of-type? m 'event)
             (not (ly:is-music-of-type? m 'rhythmic-event))))

That seemed to be more or less right.  Apparently you found a "less
right" case.

Suggestions?

-- 
David Kastrup

[dak, 2012-01-20 17:58:13]

On 2012/01/20 17:33:44, dak wrote:
> mailto:n.puttock@gmail.com writes:
> 
> > Hi David,
> >
> > Should I wait for a new patch or can I test using the latest one here?
> >
> > I've tried it out briefly on a real music example, and have a problem
> > with identifiers:
> >
> > foo = \mark \default
> >
> > \relative c' {
> >  \foo
> >   c1
> > }
> 
> You have the newest.  The problem is the definition of ly:event? (for
> recognizing postevents).  It is currently (in lily/music-scheme.cc)
> defined as the equivalent of
> (define (ly:event? m)
>         (and (ly:music? m)
>              (ly:is-music-of-type? m 'event)
>              (not (ly:is-music-of-type? m 'rhythmic-event))))
> 
> That seemed to be more or less right.  Apparently you found a "less
> right" case.
> 
> Suggestions?

define-music-display-methods has a rather crude post-event? definition but I am
not all too sure whether it is early enough in the load-order.

I think I will go the tiresome way of explicitly putting post-event into the
types of each suitable music event.  That puts the information to an obvious
place, and it does not look like the current event types discriminate correctly.

And either post-event? or ly:event? should eventually be eradicated.

[Neil Puttock, 2012-01-20 17:59:40]

On 20 January 2012 17:32, David Kastrup <dak@gnu.org> wrote:
> n.puttock@gmail.com writes:
>
>> Hi David,
>>
>> Should I wait for a new patch or can I test using the latest one here?
>>
>> I've tried it out briefly on a real music example, and have a problem
>> with identifiers:
>>
>> foo = \mark \default
>>
>> \relative c' {
>>  \foo
>>   c1
>> }
>
> You have the newest.  The problem is the definition of ly:event? (for
> recognizing postevents).  It is currently (in lily/music-scheme.cc)
> defined as the equivalent of
> (define (ly:event? m)
>        (and (ly:music? m)
>             (ly:is-music-of-type? m 'event)
>             (not (ly:is-music-of-type? m 'rhythmic-event))))
>
> That seemed to be more or less right.  Apparently you found a "less
> right" case.
>
> Suggestions?

I can't think of anything better than adding another type such as
`command-event' to things like TempoChangeEvent and MarkEvent and
filtering that out too.

Cheers,
Neil

[dak, 2012-01-20 22:33:16]

On 2012/01/20 17:59:40, Neil Puttock wrote:
 
> I can't think of anything better than adding another type such as
> `command-event' to things like TempoChangeEvent and MarkEvent and
> filtering that out too.

Emacs stands for "Editor Macros".

;; Keyboard Macro Editor.  Press C-c C-c to finish; press C-x k RET to cancel.
;; Original keys: ESC @ ESC w C-x b RET C-s C-y RET C-s general-music RET SPC
post-event C-x b RET C-s ' RET

Command: last-kbd-macro
Key: none

Macro:

ESC @			;; mark-word
ESC w			;; kill-ring-save
C-x b			;; switch-to-buffer
RET			;; newline
C-s			;; isearch-forward
C-y			;; yank
RET			;; newline
C-s			;; isearch-forward
general-music		;; self-insert-command * 13
RET			;; newline
SPC			;; self-insert-command
post-event		;; self-insert-command * 10
C-x b			;; switch-to-buffer
RET			;; newline
C-s			;; isearch-forward
'			;; self-insert-command
RET			;; newline

And lo and behold, we have transferred the list of post events from
define-music-display-methods.scm into define-music-types.scm.

And have discovered in that way when regtesting that
\LaissezVibrerEvent and \BreakDynamicSpanEvent must have wrongly been
formatted as command events (since the post-event? function did not
recognize them).

Let's see whether we get more surprises.

[Carl, 2012-01-20 23:02:20]

Wow -- what a lot of work.  And it really seems to be cleaning things up!

Thanks!

http://codereview.appspot.com/5440084/diff/11001/scm/modal-transforms.scm
File scm/modal-transforms.scm (right):

http://codereview.appspot.com/5440084/diff/11001/scm/modal-transforms.scm#new...
scm/modal-transforms.scm:129: (define (make-scale music)
I'm a bit hesitant to name this make-scale instead of extract-pitches.

make-scale is a particular use, extract-pitches is a general use.

But I don't feel really strongly about it.

[dak, 2012-01-21 07:27:24]

http://codereview.appspot.com/5440084/diff/11001/scm/modal-transforms.scm
File scm/modal-transforms.scm (right):

http://codereview.appspot.com/5440084/diff/11001/scm/modal-transforms.scm#new...
scm/modal-transforms.scm:129: (define (make-scale music)
On 2012/01/20 23:02:21, Carl wrote:
> I'm a bit hesitant to name this make-scale instead of extract-pitches.
> 
> make-scale is a particular use, extract-pitches is a general use.
> 
> But I don't feel really strongly about it.

extract-pitch-sequence [sic!] was only used inside of make-scale and returned a
different, hardly useful value.  I decided to keep its only caller make-scale
instead.

It is not like extract-pitch-sequence was the equivalent of a published API:
almost every file has its own version of it.

[Neil Puttock, 2012-01-24 22:47:31]

Great work David.  I like how \harmonic works properly for single notes now. :)

http://codereview.appspot.com/5440084/diff/14005/lily/music-scheme.cc
File lily/music-scheme.cc (right):

http://codereview.appspot.com/5440084/diff/14005/lily/music-scheme.cc#newcode79
lily/music-scheme.cc:79: "Is @var{obj} a proper (non-rhythmic) event object?")
I'd be much happier if there were a separate ly:post-event? predicate.  If I see
ly:event? I'd expect it to return true for all events.

http://codereview.appspot.com/5440084/diff/14005/lily/music.cc
File lily/music.cc (right):

http://codereview.appspot.com/5440084/diff/14005/lily/music.cc#newcode164
lily/music.cc:164: (void) music_list_to_relative (get_property
("articulations"), last, true);
Since this is only used in TrillSpanEvent, wouldn't it be better to have a
callback specific to rhythmic music which does this instead of checking it every
time?

Ideally, we'd have a separate event for the trill pitch, then we wouldn't need
to do this at all if it were kept out of 'articulations (and it would have the
added benefit of correct origin info).

http://codereview.appspot.com/5440084/diff/14005/scm/define-music-display-met...
File scm/define-music-display-methods.scm (right):

http://codereview.appspot.com/5440084/diff/14005/scm/define-music-display-met...
scm/define-music-display-methods.scm:141: (define (post-event? m)
This just duplicates the code for the exported predicate.

[dak, 2012-01-24 23:19:10]

On 2012/01/24 22:47:31, Neil Puttock wrote:
> Great work David.  I like how \harmonic works properly for single notes now.
:)

As opposed to an earlier version of the patch or to LilyPond's previous
behavior?

All of your code comments are quite spot on and touch some sore spots that I
decided would make more sense to tackle independently in order not to introduce
too many incompatibilities in one go.

ly:event? definitely has to go one way or the other.  Its original definition at
one time was equivalent to mus-is-of-type? 'event, but while it was used for
recognizing post-events from a smaller set than what the predicate is used for
testing now, that was never really correct.  It is used in many places though,
and it seems weird to check for ly:music? but only for post-event? even though
the latter does not really require C++ to define.

[dak, 2012-01-24 23:19:25]

http://codereview.appspot.com/5440084/diff/14005/lily/music-scheme.cc
File lily/music-scheme.cc (right):

http://codereview.appspot.com/5440084/diff/14005/lily/music-scheme.cc#newcode79
lily/music-scheme.cc:79: "Is @var{obj} a proper (non-rhythmic) event object?")
On 2012/01/24 22:47:32, Neil Puttock wrote:
> I'd be much happier if there were a separate ly:post-event? predicate.  If I
see
> ly:event? I'd expect it to return true for all events.

I did not change DOC string (which has stopped being even closely accurate some
time ago) or name at this stage.  The music display code contains a separate
post-event? function that has been folded into this.

One should eventually narrow down on a single definition and change docs (and
convert-ly) rules accordingly.  The post-event? stuff has already been committed
separately.

I think that ly:post-event? would likely make most sense, even though it is
neither of the preexisting definitions.

http://codereview.appspot.com/5440084/diff/14005/lily/music.cc
File lily/music.cc (right):

http://codereview.appspot.com/5440084/diff/14005/lily/music.cc#newcode164
lily/music.cc:164: (void) music_list_to_relative (get_property
("articulations"), last, true);
On 2012/01/24 22:47:32, Neil Puttock wrote:
> Since this is only used in TrillSpanEvent, wouldn't it be better to have a
> callback specific to rhythmic music which does this instead of checking it
every
> time?

Not sure about that.

Articulations tend not to be all that many.

> Ideally, we'd have a separate event for the trill pitch, then we wouldn't need
> to do this at all if it were kept out of 'articulations (and it would have the
> added benefit of correct origin info).

"Separate event" is not necessarily fun.  In the first iteration, I don't want
to tackle all too many complex changes and intricacies but rather keep things
working as simple as possible.  That does not preclude patches to better tune
the behavior.

In any way, if a trill afternote is a post-event, it ends up as an articulation
on single notes.  Simple rules, simple effects.  Why it is a post-event, I don't
know, but it is not the topic of this patch to change that.  And I don't know
whether it is really the only pitched articulation around.

http://codereview.appspot.com/5440084/diff/14005/scm/define-music-display-met...
File scm/define-music-display-methods.scm (right):

http://codereview.appspot.com/5440084/diff/14005/scm/define-music-display-met...
scm/define-music-display-methods.scm:141: (define (post-event? m)
On 2012/01/24 22:47:32, Neil Puttock wrote:
> This just duplicates the code for the exported predicate.

Correct.  You'd rather have it refer to there instead?  I've not yet decided
which of post-event? and ly:event? has to go, and such a decision would need a
convert-ly rule.  So I prefer postponing it.

parser.yy also calls is_mus_type ("post-event") directly. Another duplication. 
But the calling indirection does not really seem worth the gain.

[Keith, 2012-08-14 02:45:39]

Needs a tweak for \endSpanners

http://codereview.appspot.com/5440084/diff/14005/ly/music-functions-init.ly
File ly/music-functions-init.ly (right):

http://codereview.appspot.com/5440084/diff/14005/ly/music-functions-init.ly#n...
ly/music-functions-init.ly:296: (if (memq (ly:music-property music 'name)
'(EventChord NoteEvent))
and   RestEvent SkipEvent
{ \endSpanners r4\cresc b b b }

[dak, 2012-08-14 05:33:49]

On 2012/08/14 02:45:39, Keith wrote:
> Needs a tweak for \endSpanners
> 
> http://codereview.appspot.com/5440084/diff/14005/ly/music-functions-init.ly
> File ly/music-functions-init.ly (right):
> 
>
http://codereview.appspot.com/5440084/diff/14005/ly/music-functions-init.ly#n...
> ly/music-functions-init.ly:296: (if (memq (ly:music-property music 'name)
> '(EventChord NoteEvent))
> and   RestEvent SkipEvent
> { \endSpanners r4\cresc b b b }

I lean towards removing the check altogether.  Extract-typed-music is quite more
robust here than the original code was, and the worst that can happen is that
one finishes spanners that are both started and ended already in complex music a
second time.  But that would generate a separate error/warning anyway, and a
warning much more likely to make sense than the one we are generating here.