Menu

#2070 Patch: Don't wrap EventChord around rhythmic events by default.

Duplicate
nobody
None
Enhancement
2012-01-21
2011-12-02
Anonymous
No

Originally created by: *anonymous

Originally created by: dak@gnu.org
Originally owned by: dak@gnu.org

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.

http://codereview.appspot.com/5440084

Discussion

  • Google Importer

    Google Importer - 2011-12-03

    Originally posted by: dak@gnu.org

    (No comment was entered for this change.)

    Labels: -Patch-new Patch-needs_work
    Owner: dak@gnu.org
    Status: Started

     
  • Google Importer

    Google Importer - 2011-12-05

    Originally posted by: lilypond...@gmail.com

    Patchy the autobot says: I'm sure that David already knows this, but the pathc adds a bunch of ***Song Warning*** to song-*.ly and some misc other warnings. Only graphical change is moving 8va higher in tablature-harmonic-function.ly

    Labels: Patch-needs_work

     
  • Google Importer

    Google Importer - 2011-12-06

    Originally posted by: dak@gnu.org

    Actually, as I move the patch to behave more like Lilypond works by default (only keeping articulations in "articulations" inside of chords) the number of graphical differences go down significantly.

    Which is not necessarily an improvement in all respects since a considerable number of them indicates bugs in the handling of articulations on chord members as compared to that of whole chords.

    I'll try to remember doing a few reports before they get hidden away again.

     
  • Google Importer

    Google Importer - 2011-12-09

    Originally posted by: lilypond...@gmail.com

    Patchy the autobot says: balloon.log: not an articulation 8

    Labels: Patch-needs_work

     
  • Google Importer

    Google Importer - 2011-12-10

    Originally posted by: lilypond...@gmail.com

    Patchy the autobot says: LGTM.  I can accept this, but the 8va in tablature-harmonic-functions.ly moves suspiciously higher. No other changes.

    Labels: Patch-review

     
  • Google Importer

    Google Importer - 2011-12-10

    Originally posted by: dak@gnu.org

    The appearance in tablature-harmonic-functions.ly is simply wrong (all of them are too high even before the change) because there are string numbers underneath that have been made transparent, see issue 2085, and there is one additional non-chord event for which a "transparent" string number messes up the spacing like it already happens with the chord event string numbers.

    The actual change here is that string numbers on single notes don't just disappear, and the suspicious move you observe is a transparent string number.  This difference gets much more obvious when you do -dno-event-chord-wrapper, but there are some more differences that bypass the event chord wrapper, like when you explicitly call make-sequential-music yourself instead of using { ... } or #{ ... #}.

    This is what happens in the function creating harmonics.  Also some grace functions are affected.  I'll be changing most of those since they mostly are cases of unnecessary complex or imprudent programming, but of course this does not help users that have written similar constructs in their code.

    I won't be pushing this patch before getting considerably useful feedback from some of the more heavy hitters regarding their private document base.

    See also issue 2085 for more problems triggered by this patch (depending on whether you use -dno-event-chord-wrapper or not).  There is not really much of a shortage for them.

     
  • Google Importer

    Google Importer - 2011-12-13

    Originally posted by: ColinPKC...@gmail.com

    @David: responding to your comment above, I'll keep this off the countdown schedule until I hear from you that you've had the feedback you're looking for.

     
  • Google Importer

    Google Importer - 2011-12-14

    Originally posted by: dak@gnu.org

    That's ok.  I will likely edge a few changes into master at the meantime that make the transition smoother: while the default of -devent-chord-wrapper makes for a smoother transition, it can't cover things like music functions that call make-music on their own.

    So it is a compatibility option that causes _fewer_ things to break, but not in a manner where one could actually _depend_ on it.  For that reason, I think that the patch should go in when the default can, in reasonably good conscience, be -dno-event-chord-wrapper.

     
  • Google Importer

    Google Importer - 2011-12-14

    Originally posted by: Carl.D.S...@gmail.com

    I will take a look at the tablature stuff when I get done grading finals (probably about a week out).  I think the proposed changes make sense.

    Thanks,

    Carl

     
  • Google Importer

    Google Importer - 2011-12-16

    Originally posted by: Carl.D.S...@gmail.com

    A patch for issue 2085 has been posted.  It now looks to see if the StringNumber stencil is #f, and if it is, it avoids creating a StringNumber grob.

    I guess there are still likely to be issues to work out concerning the differences between string-number articulations (inside of chord constructs) and string-number events (outside of chord constructs).  Presumably the new code will be consistent, i.e. the difference between the articulations and the events will go away?

     
  • Google Importer

    Google Importer - 2011-12-29

    Originally posted by: ColinPKC...@gmail.com

    Marking as "needs work" to re3flect David's comment @17.

    Labels: -Patch-review Patch-needs_work

     
  • Google Importer

    Google Importer - 2012-01-16

    Originally posted by: Carl.D.S...@gmail.com

    OK, I'm trying to understand the implications of things and respond to comments 16 and 17.  I'm not sure I'm clear about what's going on, so let me ask a few questions and make a few comments.

    In the Notation Reference, section 1.5.1 Single voice under Chorded notes, it describes the state of articulations on notes in chords and articulations on chords themselves.

    In that documentation, it indicates that articulations can apply to chords (which seems to make the most sense for what I would consider musical articulations, such as staccato, prall, etc.)  It also indicates that notes can have articulations within a chord (which seems to make somewhat less sense -- how do we indicate that one note of a chord is staccato while another is legato?).

    The examples I see in this section just stack the articulations above the chord, rather than adding them to individual noteheads, so it seems that there is not really a need to have *musical* articulations attached to noteheads.

    Currently dynamics are ignored if they are inside chord constructs.

    Currently string numbers, fingerings, and right-hand fingerings that occur inside of a chord construct are stored as articulations.  If they are outside a chord, they are stored as events.

    My knowledge of the parser and lexer is miniscule compared to yours, so I may be making a completely infeasible recommendation here.

    But could we define "articulations" as things that apply to notes not in chords or to chords as a whole?  For articulations, then, c-? would be equivalent to  <c>-? and <c-?> would be a syntax error.

    Similarly, for dynamics, c\p would be equivalent to <c>\p and <c\p> would be a syntax error.

    For string numbers and fingerings, c\5-2 would be equivalent to <c\5-2> and <c>\5-2 would be a syntax error.

    Currently, we handle fingerings outside a chord by assuming that they go to the note heads inside the chord with the order of the note heads inside the chord matching the order of the fingerings outside the chord.  This seems to be a non-robust usage.

    A logical structure that says articulations and dynamics apply to a chord, and fingerings and string numbers apply to notes within a chord allows one to do the right thing if there is no explicit chord, it seems to me.

    But I don't know if this is really possible or not.

    Thanks,

    Carl

     
  • Google Importer

    Google Importer - 2012-01-17

    Originally posted by: m...@mikesolomon.org

    Just a pedantic terminological thing: there isn't an EventChord engraver, just an EventChord iterator.  Iterators do different things than engravers - iterators comb through music events to figure out how to send events to engravers, and engravers receive events and do things with them.

    If you want c-. to behave like <c>-. but not <c-.> in a context where rhythmic events are not necessarily wrapped by event chords, after line 49 of event-chord-iterator.cc, you can do something like mus->set_property("containing-event-chord", get_music ()->self_scm()).  Make sure to define the property in define-music-properties.scm.  Then, when you get to the new-fingering-engraver, you can test note_ev to see if it has something stashed in the property "containing-event-chord" defined.  If not, that means that the note must be solo, in which case you can treat the articulation differently.

    Another solution would be to create an ArticulationEvent in the simple-music-iterator if an event has an articulation array.  When an event hits this iterator, check to see if it has an articulation array.  If it does and does not have something set for "containing-event-chord", report an ArticulationEvent via report_event (you can construct this event through a Scheme callback).

    The second option seems better than the first, as it won't require messing with the engravers - they'll receive the same information they did before.  It's just that the iterator will create a new event.

    I don't have time to test the patch this week, but hopefully that gives you food for thought for ways to move forward with it.

     
  • Google Importer

    Google Importer - 2012-01-17

    Originally posted by: dak@gnu.org

    @mike: of course I'll get the terminology wrong: my amount of knowledge is just sufficient for buzzword juggling.  Second option sounds better.  I would prefer if I did not need to meddle with the music by tagging on "contained-in-event-chord" (which, if we want to be pedantic, would be the right kind of operation).  Is there another way in which the EventChord can report an ArticulationEvent and have the NoteEvent iterator (?) know that it does not need to cater for the ArticulationEvent any more?  Perhaps it should just iterate itself through its contained events and call a different iterator, or the same iterator with some "hidden argument"?  It would be possible to create a copy of the NoteEvent with all articulations already treated by the EventChord iterator removed, but I would prefer avoiding this kind of additional copying.  Just handing the NoteEvent engraver an additional list of ArticulationEvents it should just ignore sounds like a nicer option.  It would likely not be a noticeable performance problem if this list was created per-EventChord rather than per-NoteEvent.

     
  • Google Importer

    Google Importer - 2012-01-21

    Originally posted by: lilypond...@gmail.com

    Patchy the autobot says: based on current master, this removes the pitched grace note from tie-pitched-trill.ly. If it requires current staging, then put the patch back to patch-new so a later run will test it again.

    Labels: Patch-needs_work

     
  • Google Importer

    Google Importer - 2012-01-21

    Originally posted by: dak@gnu.org

    The last upload is based on current master, the current work is based on staging.  Since staging has not been bounced to master for a few days, I am not really sure what the next upload should look like.  Indeed, there are a _number_ of incremental commits with separate focus piling up in the feature branch.

    Is there a chance to get Patchy feedback for a branch tip (dev/eventchord or whatever) rather than a "patch" for which the base to which is supposed to apply is a moving target?

     
  • Google Importer

    Google Importer - 2012-01-21

    Originally posted by: gra...@percival-music.ca

    staging is vaguely in limbo: I've disabled the crontab on my desktop since I want to control when it gets bogged down with lots of processing, and also since James is trying to set it up on his computer.  So I generally run it once a day, unless I forget.  I'm running it right now.

    Patchy feedback for a branch tip is certainly technologically feasible.  I'm happy to give your github account direct pushing ability to lilypond-extra, or merge a pull request if you don't want direct push for whatever reason.  Alternately, recruit somebody else to add this feature to Patchy.

     
  • Google Importer

    Google Importer - 2012-01-21

    Originally posted by: lilypond...@gmail.com

    Patchy the autobot says: this loses the second word FAT in text-spanner-attachment-alignment.ly

    Labels: Patch-needs_work

     
  • Google Importer

    Google Importer - 2012-01-21

    Originally posted by: dak@gnu.org

    Not when tested on the current brand-new master (unless I made a mistake when uploading).  It _would_ lose it without

    commit [re8f544af29c93145d122efa8dcfc0548d9b84f0b]
    Author: David Kastrup <dak@gnu.org>
    Date:   Fri Jan 20 12:01:03 2012 +0100

        Make inherited stream event properties immutable.

    which is the reason I created that patch in the first place.

     
  • Google Importer

    Google Importer - 2012-01-21

    Originally posted by: dak@gnu.org

    If you are not working with song/festival or the XML output, this is pretty much the final candidate.  If you are doing Scheme analysis/manipulation of music, check out extract-named-music and extract-typed-music in the music functions: those pick out things (possibly for manipulation) without being fazed at what depth they are hidden in the music.  They might help with adapting your code.

    http://codereview.appspot.com/5440084

    Blockedon: -2229

     
  • Google Importer

    Google Importer - 2012-01-21

    Originally posted by: pkx1...@gmail.com

    David,

    Do we need any ew @warnings added anywhere in the NR that you can think of off the top of your head - in case this is going to result in changed behaviour from previous releases?

    I don't have anything in mind but am just asking as a 'general user'.

    james

     
  • Google Importer

    Google Importer - 2012-01-21

    Originally posted by: dak@gnu.org

    I don't think NR should be affected.  The extending/programming reference will likely need a few touchups, but not all that much.  It is mostly vague and a guide for experimenting around yourself.  The results of those experiments will differ and likely better match expectations.

     
  • Google Importer

    Google Importer - 2012-01-21

    Originally posted by: gra...@percival-music.ca

    oh, it's probably that problem where google code only returns the first 20 or 25 comments when you ask it for all comments.

    Blocked on issue 2239.  The workaround is sufficient to unblock this issue, although it's a fairly silly workaround.

    Blockedon: 2239
    Labels: -Patch-new Patch-needs_work

     
  • Google Importer

    Google Importer - 2012-01-21

    Originally posted by: dak@gnu.org

    lilypond@googlecode.com writes:

    Hm.  I deleted many comments now.  If it is a total number problem, this
    might help.  If it is a "broken sequence" problem (maybe I deleted one
    previously), it won't.  Neither if it is a "highest number" problem.

     
  • Google Importer

    Google Importer - 2012-01-21

    Originally posted by: dak@gnu.org

    Merging this into a new issue.  That's ridiculous, but may help.

    Blockedon: -2239
    Labels: -Patch-needs_work
    Mergedinto: 2240
    Status: Duplicate