Re: Don't wrap EventChord around rhythmic events by default. (issue 5440084)
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#newcode296
ly/music-functions-init.ly:296: (if (memq (ly:music-property music
'name) '(EventChord NoteEvent))
and RestEvent SkipEvent
{ \endSpanners r4\cresc b b b }
http://codereview.appspot.com/5440084/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Don't wrap EventChord around rhythmic events by default. (issue 5440084)
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-methods.scm
File scm/define-music-display-methods.scm (right):
http://codereview.appspot.com/5440084/diff/14005/scm/define-music-display-methods.scm#newcode141
scm/define-music-display-methods.scm:141: (define (post-event? m)
This just duplicates the code for the exported predicate.
http://codereview.appspot.com/5440084/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Don't wrap EventChord around rhythmic events by default. (issue 5440084)
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. http://codereview.appspot.com/5440084/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Don't wrap EventChord around rhythmic events by default. (issue 5440084)
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-methods.scm
File scm/define-music-display-methods.scm (right):
http://codereview.appspot.com/5440084/diff/14005/scm/define-music-display-methods.scm#newcode141
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.
http://codereview.appspot.com/5440084/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Don't wrap EventChord around rhythmic events by default. (issue 5440084)
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. http://codereview.appspot.com/5440084/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Don't wrap EventChord around rhythmic events by default. (issue 5440084)
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... http://codereview.appspot.com/5440084/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Don't wrap EventChord around rhythmic events by default. (issue 5440084)
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.cc#newcode62 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) http://codereview.appspot.com/5440084/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Don't wrap EventChord around rhythmic events by default. (issue 5440084)
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. http://codereview.appspot.com/5440084/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Don't wrap EventChord around rhythmic events by default. (issue 5440084)
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.cc#newcode62 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. http://codereview.appspot.com/5440084/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Don't wrap EventChord around rhythmic events by default. (issue 5440084)
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). http://codereview.appspot.com/5440084/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Don't wrap EventChord around rhythmic events by default. (issue 5440084)
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. http://codereview.appspot.com/5440084/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Don't wrap EventChord around rhythmic events by default. (issue 5440084)
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
http://codereview.appspot.com/5440084/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Don't wrap EventChord around rhythmic events by default. (issue 5440084)
n.putt...@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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Don't wrap EventChord around rhythmic events by default. (issue 5440084)
On 2012/01/20 17:33:44, dak wrote:
mailto:n.putt...@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.
http://codereview.appspot.com/5440084/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Don't wrap EventChord around rhythmic events by default. (issue 5440084)
On 20 January 2012 17:32, David Kastrup d...@gnu.org wrote:
n.putt...@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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Don't wrap EventChord around rhythmic events by default. (issue 5440084)
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. http://codereview.appspot.com/5440084/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Don't wrap EventChord around rhythmic events by default. (issue 5440084)
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#newcode129 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. http://codereview.appspot.com/5440084/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Don't wrap EventChord around rhythmic events by default. (issue 5440084)
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#newcode129 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. http://codereview.appspot.com/5440084/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Don't wrap EventChord around rhythmic events by default. (issue 5440084)
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. http://codereview.appspot.com/5440084/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Don't wrap EventChord around rhythmic events by default. (issue 5440084)
Reviewers: MikeSol,
Message:
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.
Description:
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.
Please review this at http://codereview.appspot.com/5440084/
Affected files:
M input/regression/music-map.ly
M lily/music-scheme.cc
M lily/parser.yy
M ly/music-functions-init.ly
M scm/define-music-display-methods.scm
M scm/define-music-types.scm
M scm/modal-transforms.scm
M scm/music-functions.scm
M scm/song.scm
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
