Issue 117050043: Simplify the NullVoice context
DescriptionLet NullVoice work completely correctly when inside a Staff:
Users have begun to use NullVoice in ways that require it to live within a Staff (for example to print solfege above the staff)
Note-head interface: add missing property
Patch Set 1 #Patch Set 2 : Accidentals, engraved by staff, need hidden #Patch Set 3 : and AccidentalSuggestion #
Total comments: 7
Patch Set 4 : NullVoice a child of Score #Patch Set 5 : NullVoice a child of Score, but robust enough to move to Staff #Patch Set 6 : back to Staff #
Total comments: 1
Patch Set 7 : paper over a load of problems #Patch Set 8 : supress accidentals correctly, using a property of NullVoice #
MessagesTotal messages: 14
Hi Keith, sorry for writing so late - i didn't notice this patch until yesterday and i didn't have time for reviewing it today... The changes looks promising! I'll give it a closer look tomorrow, hopefully in the morning. best, Janek
Sign in to reply to this message.
Hi Keith, your changes look good overall, but i think that NullVoice could be simplified even further - see below. thanks for your work! https://codereview.appspot.com/117050043/diff/40001/input/regression/lyric-co... File input/regression/lyric-combine-nullvoice.ly (right): https://codereview.appspot.com/117050043/diff/40001/input/regression/lyric-co... input/regression/lyric-combine-nullvoice.ly:12: \autoBeamOff c4 r16 b,16~ b,8 c8[ e8 g8 e8] | g4( f4) g2 } It seems that the closing slur is one note too early. https://codereview.appspot.com/117050043/diff/40001/input/regression/lyric-co... input/regression/lyric-combine-nullvoice.ly:14: \new Lyrics \lyricsto "nv" { free a -- lign -- ment } Shouldn't there be a lyric extender at the end? https://codereview.appspot.com/117050043/diff/40001/input/regression/lyric-co... input/regression/lyric-combine-nullvoice.ly:15: >> } This regtest is nice, but as far as i can see it doesn't check whether issue 3825 has been fixed - i.e. the output looks the same before and after the change in the code. I think we should add the example from issue 3825 verbatim (as another regtest), and have a regtest corresponding to issue 3834. https://codereview.appspot.com/117050043/diff/40001/ly/engraver-init.ly File ly/engraver-init.ly (right): https://codereview.appspot.com/117050043/diff/40001/ly/engraver-init.ly#newco... ly/engraver-init.ly:787: \override NoteHead.meta.interfaces = #(delete 'rhythmic-head-interface What about making NullVoice \accepted not by \Staff, but by \Score (like Devnull)? From my test it seems that it would work and we wouldn't have to care about Rhythmic_column_engraver. https://codereview.appspot.com/117050043/diff/40001/ly/engraver-init.ly#newco... ly/engraver-init.ly:794: squashedPosition = 0 This should also become unneeded if we "move" NullVoice from Staff to Score. https://codereview.appspot.com/117050043/diff/40001/ly/engraver-init.ly#newco... ly/engraver-init.ly:798: \omit AccidentalSuggestion I think these are unneeded anyway.
Sign in to reply to this message.
https://codereview.appspot.com/117050043/diff/40001/ly/engraver-init.ly File ly/engraver-init.ly (right): https://codereview.appspot.com/117050043/diff/40001/ly/engraver-init.ly#newco... ly/engraver-init.ly:787: \override NoteHead.meta.interfaces = #(delete 'rhythmic-head-interface On 2014/07/27 11:25:41, janek wrote: > What about making NullVoice \accepted not by \Staff, but by \Score (like > Devnull)? From my test it seems that it would work and we wouldn't have to care > about Rhythmic_column_engraver. Ugh. This looks incredibly ugly. No code elsewhere messes with the basic underlying grob properties, and overriding a nested property to boot is a recipe for trouble. We really, really should not be doing something like that. It would make more sense to make rhythmic-head-interface get along without Rhythmic_column_engraver or otherwise fix things to work properly when a number of engravers are being removed. If we really, really cannot make do otherwise, we could design substitute engravers that mostly do the same job but without producing output. I mean, Midi has to produce the same timing for Lyrics and manages with a vastly simplified set of performers. It's even conceivable to tie the lyrics and other stuff together with translators that are basically decoupled from the engravers/performers doing the actual grobs or sobs (or what we call the sound equivalent of a grob). We could use those for every output then. For example, if we have some separate translator producing artificial tie STOP events (and catering for tieWaitForNote and similar), we'd run less of a danger that Midi and PDF get out of synch. And custom engravers making use of that info would become simpler. That way we don't need to reimplement the synhronization yet another time if we add, say, MusicXML output.
Sign in to reply to this message.
On 2014/07/27 11:25:41, janek wrote: > https://codereview.appspot.com/117050043/diff/40001/input/regression/lyric-co... > input/regression/lyric-combine-nullvoice.ly:15: >> } > This regtest is nice, but as far as i can see it doesn't check whether issue > 3825 has been fixed - i.e. the output looks the same before and after the change > in the code. Oops. Putting the tie into the rhythm covered up the problem of issue 3825. I have extended the test a little so that it demonstrates each of the reported issues. > https://codereview.appspot.com/117050043/diff/40001/ly/engraver-init.ly#newco... > ly/engraver-init.ly:787: \override NoteHead.meta.interfaces = #(delete > 'rhythmic-head-interface > What about making NullVoice \accepted not by \Staff, but by \Score (like > Devnull)? From my test it seems that it would work and we wouldn't have to care > about Rhythmic_column_engraver. That works, unless people setting Lyrics move engravers around in ways that I haven't considered. It seems we need to make NoteHead objects with the proper extent so they are fully equivalent to notes for Lyrics alignment, but invisible and non-interacting with the rest of the layout. Putting these NoteHeads in the Score, where no other layout is looking for them, is simpler than having the NoteHeads tell every other engraver "I am not the kind of NoteHead that you are looking for." > https://codereview.appspot.com/117050043/diff/40001/ly/engraver-init.ly#newco... > ly/engraver-init.ly:798: \omit AccidentalSuggestion > I think these are unneeded anyway. So long as Accidental_engraver stays out of \Score. We could keep some of these overrides just in the engravers are moved around, but ket's test and review it without the defensive \overrides.
Sign in to reply to this message.
On 2014/07/27 11:50:17, dak wrote: > https://codereview.appspot.com/117050043/diff/40001/ly/engraver-init.ly#newco... > ly/engraver-init.ly:787: \override NoteHead.meta.interfaces = #(delete > 'rhythmic-head-interface > Ugh. This looks incredibly ugly. I guess I lack a sense of aesthetics for Scheme code; I thought it was rather pretty. > It would make more sense to make rhythmic-head-interface get along without Rhythmic_column_engraver It is easy to make the rest of the code accept an object with rhythmic-head-interface that is unattached to any Rhythmic_Column (just removing a test and warning in rest-collision.cc as I recall). The thing I didn't like about that was changing completely unrelated code to accommodate the weirdness of NullVoice. > If we really, really cannot make do otherwise, we could design substitute > engravers that mostly do the same job but without producing output. I mean, > Midi has to produce the same timing for Lyrics and manages with a vastly > simplified set of performers. My first inclination was to make an engraver that merely sets melismaBusy and lets the Lyrics attach to PaperColumns, but then we would need to synchronize that engraver with what Tie_engraver, etc., do if there is ever a change, and the current gives NoteHeads with real extents to which the Lyrics can line up exactly as if there was a printed note.
Sign in to reply to this message.
On 2014/07/27 21:31:52, Keith wrote: > On 2014/07/27 11:50:17, dak wrote: > > > https://codereview.appspot.com/117050043/diff/40001/ly/engraver-init.ly#newco... > > ly/engraver-init.ly:787: \override NoteHead.meta.interfaces = #(delete > > 'rhythmic-head-interface > > > Ugh. This looks incredibly ugly. > > I guess I lack a sense of aesthetics for Scheme code; I thought it was rather > pretty. The ugliness has nothing to do with the programming language involved. The ugliness is with messing up the whole grob definition of a NoteHead (which is fixed to the degree of being documented in the Internals Reference) inside of a particular context. > > It would make more sense to make rhythmic-head-interface get along without > Rhythmic_column_engraver > > It is easy to make the rest of the code accept an object with > rhythmic-head-interface that is unattached to any Rhythmic_Column (just removing > a test and warning in rest-collision.cc as I recall). The thing I didn't like > about that was changing completely unrelated code to accommodate the weirdness > of NullVoice. I don't see anything wrong with letting code work in more combinations than originally envisioned. Where is the point in having separate building blocks if you can only assemble them in a single way? > > If we really, really cannot make do otherwise, we could design substitute > > engravers that mostly do the same job but without producing output. I mean, > > Midi has to produce the same timing for Lyrics and manages with a vastly > > simplified set of performers. > > My first inclination was to make an engraver that merely sets melismaBusy and > lets the Lyrics attach to PaperColumns, > but then we would need to synchronize that engraver with what Tie_engraver, > etc., do if there is ever a change, The point would be to stop the Tie_engraver from bothering about melismaBusy at all. > and the current gives NoteHeads with real extents to which the Lyrics can line > up exactly as if there was a printed note. But isn't NullVoice for faking lyrics to a synthetic voice that is _not_ actually being printed? That would imply that we want to rather align to the existing NoteHeads/NoteColumns rather than the NullVoice one. And even if there are no actual notes (like in a chant situation), we'd rather align on a well-spaced pattern rather than one based on imaginary noteheads, optical stem adjustments and what not.
Sign in to reply to this message.
On Sun, 27 Jul 2014 23:07:58 -0700, <dak@gnu.org> wrote: > The ugliness is with messing up the whole grob definition of a NoteHead > (which is fixed to the degree of being documented in the Internals > Reference) inside of a particular context. > I thought that was pretty, as well. We override the defaults of grobs all the time to suit particular contexts (\omit NoteHead for example). And this seemed a clear way of saying "all the default interfaces of a NoteHead except rhythmic-head". I see that we usually don't change interfaces, though. >> It is easy to make the rest of the code accept an object with >> rhythmic-head-interface that is unattached to any Rhythmic_Column >> (just removing a test and warning in rest-collision.cc as I recall). >> The thing I didn't like >> about that was changing completely unrelated code to accommodate the >> weirdness of NullVoice. > > I don't see anything wrong with letting code work in more combinations > than originally envisioned. Where is the point in having separate > building blocks if you can only assemble them in a single way? > Well, the code worked, but gave a warning. We might want to warn that someone is assembling blocks in a dangerous way. Looking at the history, this warning does not seem helpful. I can remove that warning, and restore the \omit Accidental* so that someone can have Staff accept NullVoice > The point would be to stop the Tie_engraver from bothering about > melismaBusy at all. > Splitting the melismaBusy function from Tie_engraver, etc., and putting it in a new engraver does seem sensible. > But isn't NullVoice for faking lyrics to a synthetic voice that is _not_ > actually being printed? That would imply that we want to rather align > to the existing NoteHeads/NoteColumns rather than the NullVoice one. > And even if there are no actual notes (like in a chant situation), we'd > rather align on a well-spaced pattern rather than one based on imaginary > noteheads, optical stem adjustments and what not. (We don't get stem-corrections without stems.) My first inclination was to produce no NoteHead grobs. LyricExtenders and I think LyricHyphens needed work to help them find the NoteColums within their parent PaperColumns (like Janek made happen for LyricText). With all that, NullVoice can be just Devnull plus a melisma_signal_engraver.
Sign in to reply to this message.
Patchset 4 LGTM :) I agree that it would be nice to separate melisma stuff from ties, but - as i've already written on the tracker - that's a topic for another patch.
Sign in to reply to this message.
https://codereview.appspot.com/117050043/diff/90001/ly/engraver-init.ly File ly/engraver-init.ly (right): https://codereview.appspot.com/117050043/diff/90001/ly/engraver-init.ly#newco... ly/engraver-init.ly:788: \override NoteHead.meta.interfaces = #(delete 'rhythmic-head-interface This again messes around with internals in a context definition. No, definitely no. We might want a mechanism to reroute grob announcements for named interfaces, possibly in connection with additional contexts/listeners: for example, we have piano scores with multiple logical voices (as witnessed by slurring) but a common stem or beam. In that case a sort-of intermediate and/or dormant context that can take over listening to interfaces temporarily might be nice to have. It's definitely worth thinking about something like that. But messing up the basic grob definition by fetching internals and meddling through them locally: no. Most definitely no. That's a recipe for inscrutable behavior and hard to find problems because the changes it causes are unexpected when looking anywhere else but this hacky passage.
Sign in to reply to this message.
On Sun, 07 Sep 2014 01:21:58 -0700, <dak@gnu.org> wrote: > But messing up the basic grob definition by fetching internals and > meddling through them locally: no. Well, you say "messing up the basic grob definition" I say "reading the basic definition, and making a local modified copy" We can do in LilyPond code with ly:grob-interfaces, making a copy with one interface removed, and storing the shorter list in the NullVoice context definition with an \override, so the changes to apply to grobs made in this context. LilyPond tries to be robust to grobs different interfaces, by checking has-interface() before using the properties associated with that interface. It turns out that there is a hierarchy in define-event-classes that says all note-events are musical-events and all musical-events are rhythmic-events. Maybe we have assumed that the correspondingly-named interfaces have a corresponding hierarchy. I'll try some other way of telling Accidental_engraver to ignore NullVoice. I'm trying to let people move NullVoice to Staff without reintroducing the known issue of the cancellation accidentals in \new Staff << \new NullVoice {cis dis es fis} \new Voice {r4 c d e } >>
Sign in to reply to this message.
"Keith OHara" <k-ohara5a5a@oco.net> writes: > On Sun, 07 Sep 2014 01:21:58 -0700, <dak@gnu.org> wrote: > >> But messing up the basic grob definition by fetching internals and >> meddling through them locally: no. > > Well, you say "messing up the basic grob definition" > I say "reading the basic definition, and making a local modified copy" Which is cute but does not take into account that the definition has been assembled by specific functions in a manner ensuring that it is self-consistent, and that has dependencies with regard to the consistency of other grob elements. That's the "You need another door? Just smash a window and climb through" approach. Whether or not a particular window might indeed have been the best place for a patio door, that's not a "solution" that has a place in our code base. > I'll try some other way of telling Accidental_engraver to ignore NullVoice. I'm trying to let people move NullVoice to Staff without reintroducing the known issue of the cancellation accidentals in > \new Staff << > \new NullVoice {cis dis es fis} > \new Voice {r4 c d e } >> Again: I would strongly suggest that you backpedal and consider the question "what was my first approach which did not work, for reasons?". And then we see what it would take to address the reasons. Even if we assume that messing up our codebase like that is perfectly fine, you will not be the last person to encounter a similar problem. And when another person encounters a similar problem, our answer better not be "oh, LilyPond cannot deal with that, but if you dig up its internals and squeeze something different in, it might or might not do the trick. At least until the next version with changed internals." That is: when solving a problem, we should strive to have it spawn new solutions, not new problems.
Sign in to reply to this message.
On Sep 8, 2014, at 03:54 , dak@gnu.org wrote: > "Keith OHara" <k-ohara5a5a@oco.net> writes: > > I'm trying to let people move NullVoice to Staff without > reintroducing the known issue of the cancellation accidentals in >> \new Staff << >> \new NullVoice {cis dis es fis} >> \new Voice {r4 c d e } >> > > Again: I would strongly suggest that you backpedal and consider the > question "what was my first approach which did not work, for > reasons?". > > And then we see what it would take to address the reasons. How about placing the voices inside a VoiceGroup context, moving the Accidental_engraver there, and leaving the NullVoice outside, like in my recent partcombine experiments? VoiceGroup would by default consist of nothing, accept any kind of Voice, and be accepted by any kind of Staff. — Dan
Sign in to reply to this message.
On 2014/09/08 23:28:31, dan_faithful.be wrote: > On Sep 8, 2014, at 03:54 , mailto:dak@gnu.org wrote: > > > Again: I would strongly suggest that you backpedal and consider the > > question "what was my first approach which did not work, for > > reasons?". > > > > And then we see what it would take to address the reasons. > > How about placing the voices inside a VoiceGroup context, moving the > Accidental_engraver there, and leaving the NullVoice outside, like in my recent > partcombine experiments? > > VoiceGroup would by default consist of nothing, accept any kind of Voice, and be > accepted by any kind of Staff. A solution that reorganizes the default voice hierarchy will cause interference with all user code fiddling with hierarchy. At the current point of time, such code has some place for cross-voice placements of items like stems, ties, or slurs. That kind of thing has its place in piano music. So touching the default hierarchy should be a last resort. One can use music functions for juggling with \accepts/\denies in local context modifications and thus change the hierarchy ad-hoc just at the place where an effect is desired. I am not sure I understand your proposal correctly, namely whether you are thinking of changing stuff like the \defaultchild of Staff. If you don't, all VoiceGroup staves would have to be instantiated manually. If you do, many things will change. The former could be used for providing music functions solving a particular problem. But it sounds like a solution that has quite a bit of potential of interfering with other solutions focused around similar problems. It would, of course, at least spare us the hackery in the internals of the grob implementation. But I think we can do better, see <URL:https://code.google.com/p/lilypond/issues/detail?id=4093#c6>
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
