|
|
DescriptionImproves horizontal spacing of axis groups that SpanBar grobs traverse.
Patch Set 1 #
Total comments: 9
Patch Set 2 : Incorporates Joe's suggestions #Patch Set 3 : Rebased against current master. #
Total comments: 36
Patch Set 4 : Responses to Neil and Joe's comments. #Patch Set 5 : Rebase against current master. #Patch Set 6 : Fix for 1846 rebased against current master #
Total comments: 1
MessagesTotal messages: 27
Hey all, I've been thinking about Neil's comment regarding a better interface for cross-staff stems. While this patch has nothing to do with stems per se, it lays down the framework for dealing with cross-staff stems via a test-case with SpanBar grobs (which are like cross-staff stems insofar as they span between staves and take their alignment anchors from grobs with pure height approximations). In doing so, it fixes issue 910 (regest added to show this) and gets rid of an unreported but bad problem that SpanBar pure heights are either empty or not depending on the common refpoint being used. In theory, a pure height should return the same value for .length () every time. Cheers, MS
Sign in to reply to this message.
Hey all, I just realized that in this patch, the Span_bar_stub_engraver could be bumped down a level to the Lyric context to avoid the Grob_info rerouting. Lemme know if this seems like a good idea. Another solution as well would be to create multiple SpanBars for the same timestep in the same context (so that they never crossed places where they're disallowed). It'd get rid of a lot of the code, but it wouldn't solve the problem of the inconsistent pure heights and it'd make overrides for individual span bars a lot more difficult. Furthermore, I'm not positive that cross-staff stems could have pure heights that traverse multiple staves, and I'd like to use the same logic (creating stubs in intermediary contexts between cross staff stems to help with pure height approximations) for stems. So, all that is to say that I'm trying to weigh the several options available for this implementation and, as always, figure out the ones that are most elegant while at the same time offering the most re-usability for similar problems. Cheers, MS
Sign in to reply to this message.
ok, this passes make but I get a lot of reg tests show up but I cannot see any diffs at all (i.e no 'green' shadows that indicate the changes). Either it's unbelievably subtle or something else is triggering the reg tests to show up. They are to big to post them all here but he diffs that show up are bar-extent.ly tablature-tie-behaviour.ly slur-cross-staff.ly volta-multi-staff.ly volta-multi-staff-inner-staff.ly tablature-grace-notes.ly span-bar-partial.ly double-repeat.ly spanner-alignment.ly beam-cross-staff.ly palm-mute.ly bar-line-segno.ly dead-notes.ly context-nested-staffgroup.ly tablature-glissando.ly spacing-knee-compressed.ly beam-cross-staff-slope.ly slur-extreme.ly note-line.ly span-bar-break.ly voice-follower.ly cluster-cross-staff.ly auto-change.ly page-spacing-staff-group-nested.ly tablature-harmonic-tie.ly remove-empty-staves-auto-knee.ly grace-staff-length.ly span-bar-spacing.ly arpeggio-span-collision.ly etc. Not all but a *lot* So I don't know if this needs more work or not really, but am putting this as 'review' for those that know better than I.
Sign in to reply to this message.
On Aug 27, 2011, at 10:40 AM, pkx166h@gmail.com wrote: > ok, this passes make but I get a lot of reg tests show up but I cannot > see any diffs at all (i.e no 'green' shadows that indicate the changes). > Either it's unbelievably subtle or something else is triggering the reg > tests to show up. They are to big to post them all here but he diffs > that show up are > > bar-extent.ly > tablature-tie-behaviour.ly > slur-cross-staff.ly > volta-multi-staff.ly > volta-multi-staff-inner-staff.ly > tablature-grace-notes.ly > span-bar-partial.ly > double-repeat.ly > spanner-alignment.ly > beam-cross-staff.ly > palm-mute.ly > bar-line-segno.ly > dead-notes.ly > context-nested-staffgroup.ly > tablature-glissando.ly > spacing-knee-compressed.ly > beam-cross-staff-slope.ly > slur-extreme.ly > note-line.ly > span-bar-break.ly > voice-follower.ly > cluster-cross-staff.ly > auto-change.ly > page-spacing-staff-group-nested.ly > tablature-harmonic-tie.ly > remove-empty-staves-auto-knee.ly > grace-staff-length.ly > span-bar-spacing.ly > arpeggio-span-collision.ly > > etc. > > Not all but a *lot* > > So I don't know if this needs more work or not really, but am putting > this as 'review' for those that know better than I. > > http://codereview.appspot.com/4917046/ Hey James, This is because the height of the Span Bar changes w/ the addition of the new SpanBarStub (that acts as a proxy for all height calculations), while the actual look of the span bar does not. Cheers, MS
Sign in to reply to this message.
I read through this patch, but I can't tell what it does or is supposed to do. Maybe one of our pure gurus should look at this. Joe? http://codereview.appspot.com/4917046/diff/1/lily/align-interface.cc File lily/align-interface.cc (right): http://codereview.appspot.com/4917046/diff/1/lily/align-interface.cc#newcode399 lily/align-interface.cc:399: Align_interface::vertical_sort (Grob *g1, Grob *g2) this doesn't sort. http://codereview.appspot.com/4917046/diff/1/lily/pure-from-neighbor-engraver.cc File lily/pure-from-neighbor-engraver.cc (right): http://codereview.appspot.com/4917046/diff/1/lily/pure-from-neighbor-engraver... lily/pure-from-neighbor-engraver.cc:4: Copyright (C) 1997--2011 Han-Wen Nienhuys <hanwen@xs4all.nl> ? http://codereview.appspot.com/4917046/diff/1/lily/pure-from-neighbor-engraver... lily/pure-from-neighbor-engraver.cc:53: && !i.grob ()->internal_has_interface (ly_symbol2scm ("pure-from-neighbor-interface"))) can't you put this in acknowledge_pure_from_neighbor() so you don't have to check the interface ?
Sign in to reply to this message.
On Aug 27, 2011, at 4:23 PM, hanwenn@gmail.com wrote: > I read through this patch, but I can't tell what it does or is supposed > to do. Maybe one of our pure gurus should look at this. Joe? > On an immediate level, it fixes Issue 910. On a more long-term level, it lays down some code that will be useful for cross staff stems (using this code, it should be possible to horizontally shift voices in certain cases to avoid collisions with cross-staff stems). Cheers, MS
Sign in to reply to this message.
Thanks Han-Wen! Cheers, MS http://codereview.appspot.com/4917046/diff/1/lily/align-interface.cc File lily/align-interface.cc (right): http://codereview.appspot.com/4917046/diff/1/lily/align-interface.cc#newcode399 lily/align-interface.cc:399: Align_interface::vertical_sort (Grob *g1, Grob *g2) On 2011/08/27 14:23:16, hanwenn wrote: > this doesn't sort. Renamed. http://codereview.appspot.com/4917046/diff/1/lily/pure-from-neighbor-engraver.cc File lily/pure-from-neighbor-engraver.cc (right): http://codereview.appspot.com/4917046/diff/1/lily/pure-from-neighbor-engraver... lily/pure-from-neighbor-engraver.cc:4: Copyright (C) 1997--2011 Han-Wen Nienhuys <hanwen@xs4all.nl> On 2011/08/27 14:23:16, hanwenn wrote: > ? Changed. http://codereview.appspot.com/4917046/diff/1/lily/pure-from-neighbor-engraver... lily/pure-from-neighbor-engraver.cc:53: && !i.grob ()->internal_has_interface (ly_symbol2scm ("pure-from-neighbor-interface"))) On 2011/08/27 14:23:16, hanwenn wrote: > can't you put this in acknowledge_pure_from_neighbor() so you don't have to > check the interface ? I don't think this'd work - I am weeding out all pure-from-neighbor-interface grobs from the list. Thus, I need to nip them in the bud before they become part of items_now_.
Sign in to reply to this message.
Correct me if I'm wrong, but this is my understanding: wherever there's a SpanBar, you're creating SpanBarStubs between every relevant pair of staves. These don't actually get printed; they're just there for the pure-height (because the SpanBar height is pretty much the whole system, so it doesn't tell you where the gaps are). If that's correct, I have two broad comments: it's worth commenting somewhere (span-bar-stub-engraver.cc, maybe) that the purpose of SpanBarStub is for pure-height only. But more importantly, isn't SpanBar now obsoleted by SpanBarStub? That is, you can just remove the SpanBar altogether and print the SpanBarStubs. http://codereview.appspot.com/4917046/diff/1/lily/align-interface.cc File lily/align-interface.cc (right): http://codereview.appspot.com/4917046/diff/1/lily/align-interface.cc#newcode363 lily/align-interface.cc:363: Align_interface::get_vertical_alignment (Grob *g) Our convention is that in Align_interface::foo_bar (Grob *g) the Grob *g should be something that implements Align_interface. So this function should really be Grob::get_vertical_alignment. Same for the functions below it. http://codereview.appspot.com/4917046/diff/1/lily/align-interface.cc#newcode374 lily/align-interface.cc:374: Align_interface::get_vertical_axis_group (Grob *g) Seems to me that what you really want here is the top-level VerticalAlignment (as opposed to, say, a BassFigureAlignment, which also implements Axis_group_interface and Align_interface). Try g->get_system ()->get_vertical_alignment () instead. http://codereview.appspot.com/4917046/diff/1/lily/span-bar-stub-engraver.cc File lily/span-bar-stub-engraver.cc (right): http://codereview.appspot.com/4917046/diff/1/lily/span-bar-stub-engraver.cc#n... lily/span-bar-stub-engraver.cc:116: //it->set_parent (y_parents[j], Y_AXIS); Obsolete code?
Sign in to reply to this message.
On Aug 30, 2011, at 5:53 PM, joeneeman@gmail.com wrote: > Correct me if I'm wrong, but this is my understanding: wherever there's > a SpanBar, you're creating SpanBarStubs between every relevant pair of > staves. These don't actually get printed; they're just there for the > pure-height (because the SpanBar height is pretty much the whole system, > so it doesn't tell you where the gaps are). > Yes. > If that's correct, I have two broad comments: it's worth commenting > somewhere (span-bar-stub-engraver.cc, maybe) that the purpose of > SpanBarStub is for pure-height only. Will do. > But more importantly, isn't SpanBar > now obsoleted by SpanBarStub? That is, you can just remove the SpanBar > altogether and print the SpanBarStubs. > Mmm...you're not unright, but I'd prefer to do that in another patch if that's OK. It'd require a lot of deleting and moving around, and I'd first like to make sure that these stubs are the code base and bug free for a while before I deprecate an entire grob (and figure out how to deal with the syntax changes that come with said deprecation). > http://codereview.appspot.com/4917046/diff/1/lily/align-interface.cc > File lily/align-interface.cc (right): > > http://codereview.appspot.com/4917046/diff/1/lily/align-interface.cc#newcode363 > lily/align-interface.cc:363: Align_interface::get_vertical_alignment > (Grob *g) > Our convention is that in > Align_interface::foo_bar (Grob *g) > the Grob *g should be something that implements Align_interface. So this > function should really be Grob::get_vertical_alignment. Same for the > functions below it. > Will do. > http://codereview.appspot.com/4917046/diff/1/lily/align-interface.cc#newcode374 > lily/align-interface.cc:374: Align_interface::get_vertical_axis_group > (Grob *g) > Seems to me that what you really want here is the top-level > VerticalAlignment (as opposed to, say, a BassFigureAlignment, which also > implements Axis_group_interface and Align_interface). Try g->get_system > ()->get_vertical_alignment () instead. > Yes'r. > http://codereview.appspot.com/4917046/diff/1/lily/span-bar-stub-engraver.cc > File lily/span-bar-stub-engraver.cc (right): > > http://codereview.appspot.com/4917046/diff/1/lily/span-bar-stub-engraver.cc#n... > lily/span-bar-stub-engraver.cc:116: //it->set_parent (y_parents[j], > Y_AXIS); > Obsolete code? > I'm git bisecting now, but if it's obsolete, it'll be removed in the not-too-distant future. Thanks for the comments, Joe! Cheers, MS
Sign in to reply to this message.
On Tue, Aug 30, 2011 at 8:58 AM, Mike Solomon <mikesol@ufl.edu> wrote: > On Aug 30, 2011, at 5:53 PM, joeneeman@gmail.com wrote: > >> Correct me if I'm wrong, but this is my understanding: wherever there's >> a SpanBar, you're creating SpanBarStubs between every relevant pair of >> staves. These don't actually get printed; they're just there for the >> pure-height (because the SpanBar height is pretty much the whole system, >> so it doesn't tell you where the gaps are). >> > > Yes. > >> If that's correct, I have two broad comments: it's worth commenting >> somewhere (span-bar-stub-engraver.cc, maybe) that the purpose of >> SpanBarStub is for pure-height only. > > Will do. > >> But more importantly, isn't SpanBar >> now obsoleted by SpanBarStub? That is, you can just remove the SpanBar >> altogether and print the SpanBarStubs. >> > > Mmm...you're not unright, but I'd prefer to do that in another patch if that's OK. It'd require a lot of deleting and moving around, and I'd first like to make sure that these stubs are the code base and bug free for a while before I deprecate an entire grob (and figure out how to deal with the syntax changes that come with said deprecation). Ok, then lgtm. Probably worth putting in a comment that we intend to remove SpanBar. Cheers, Joe
Sign in to reply to this message.
Hey all, I'm not sure if my previous message went through, so sorry for a double posting. I've uploaded a new patchset with most of Joe's suggestions incorporated: some of them are difficult to implement because BarLine's parents are not set until after process_acknowledged is called, but other than that they were all doable. Could someone please apply this patch and test it against several documents with SpanBars, letting me know if the SpanBars are printing correctly. I'm noticing light differences in the PDF visualization between the thickness of SpanBars and those of their BarLine buddies. I have no clue why this would arise, as I don't touch that portion of the code, and furthermore, this variation in thickness is only in certain documents and only certain times when I open up my PDF viewer. I get the same thing periodically on current master as well, but I am noticing it more here. If someone could respond with either: (1) Yes, your patch is changing SpanBar width and/or alignment; or (2) No, it's just your PDF viewer that's bugging out - the document prints just fine (I don't own a printer, otherwise I'd just print it). I'd be much obliged! Cheers, MS
Sign in to reply to this message.
Hey all, So that people can confirm the visual output of this, I'm going to wait to push until I get back. Cheers, MS
Sign in to reply to this message.
This requires an update. The patch fails to apply. Bertrand
Sign in to reply to this message.
On Sep 1, 2011, at 6:12 PM, bordage.bertrand@gmail.com wrote: > This requires an update. > The patch fails to apply. > Thanks for the heads up! I've uploaded a new patchset rebased against current master. Cheers, MS
Sign in to reply to this message.
new patch passes make and reg tests
Sign in to reply to this message.
lgtm http://codereview.appspot.com/4917046/diff/18001/lily/grob.cc File lily/grob.cc (right): http://codereview.appspot.com/4917046/diff/18001/lily/grob.cc#newcode593 lily/grob.cc:593: return get_maybe_root_vertical_alignment (g, 0); I still think this should say return g->get_system ()->get_vertical_alignment (); because there are several grobs that implement Align_interface and you want to make sure you get the right one. http://codereview.appspot.com/4917046/diff/18001/lily/span-bar-stub-engraver.cc File lily/span-bar-stub-engraver.cc (right): http://codereview.appspot.com/4917046/diff/18001/lily/span-bar-stub-engraver.... lily/span-bar-stub-engraver.cc:4: Copyright (C) 1997--2011 Han-Wen Nienhuys <hanwen@xs4all.nl> You're looking different today, Han-Wen.
Sign in to reply to this message.
Hi Mike I found a visual difference with one of my scores. I finally tracked this down to a dependence on the stem direction. Try this: SopranoMusic = \relative g' { b2 b \once \override Staff . BarLine #'allow-span-bar = ##f b2 b } TenorMusic = \relative a' { b2 b | b2 b | } \score { \new GrandStaff << \new Staff { \new Voice = "Soprano" { \stemDown \SopranoMusic } } \new Lyrics \lyricsto "Soprano" { a b long-syllable a } \new Staff { \new Voice = "Tenor" { \TenorMusic } } >> } \score { \new GrandStaff << \new Staff { \new Voice = "Soprano" { \stemUp \SopranoMusic } } \new Lyrics \lyricsto "Soprano" { a b long-syllable a } \new Staff { \new Voice = "Tenor" { \TenorMusic } } >> }
Sign in to reply to this message.
On Sep 9, 2011, at 7:09 PM, tdanielsmusic@googlemail.com wrote: > Hi Mike > I found a visual difference with one of my scores. I finally tracked > this down to a dependence on the stem direction. Try this: > > SopranoMusic = \relative g' { > b2 b > \once \override Staff . BarLine #'allow-span-bar = ##f > b2 b > } > > TenorMusic = \relative a' { > b2 b | > b2 b | > } > > \score { > \new GrandStaff << > \new Staff { > \new Voice = "Soprano" { > \stemDown > \SopranoMusic > } > } > \new Lyrics \lyricsto "Soprano" { > a b long-syllable a > } > \new Staff { > \new Voice = "Tenor" { > \TenorMusic > } > } >>> > } > > \score { > \new GrandStaff << > \new Staff { > \new Voice = "Soprano" { > \stemUp > \SopranoMusic > } > } > \new Lyrics \lyricsto "Soprano" { > a b long-syllable a > } > \new Staff { > \new Voice = "Tenor" { > \TenorMusic > } > } >>> > } Good catch! The problem comes not from this patch but from the calculation of the horizontal skylines for NonMusicalPaperColumns. Try adding: \override Score . NonMusicalPaperColumn #'stencil = #ly:separation-item::print To the beginning of SopranoMusic and then running LilyPond with -ddebug-skylines. You'll see that in the first example, the downward stem pushes the lyrics under the end point of the NonMusicalPaperColumn, whereas this does not happen in the second example. Thus, the dependency is not stems (drop the soprano an octave and you'll see that) but NonMusicalPaperColumn grobs. I'd recommend fixing this in a separate patch. I also think it should be put on the tracker, although I don't feel comfortable reporting it because I'm not sure how to describe this bug (my gut reaction would be to say that NonMusicalPaperColumn horizontal skylines are sometimes incorrect, but I'm not sure if these overshoots are in fact incorrect or if they need to be there to prevent other problems from happening, in which case the description would be more like "Over-extended NonMusicalPaper columns must be present for X but interfere with lyric spacing in certain cases"). So, as it has passed a countdown and I've gotten no responses saying that the SpanBars look different with the patch applied, I'll push the SpanBarStub patch w/in the next 24ish hours. Cheers, MS
Sign in to reply to this message.
http://codereview.appspot.com/4917046/diff/18001/input/regression/span-bar-al... File input/regression/span-bar-allow-span-bar.ly (right): http://codereview.appspot.com/4917046/diff/18001/input/regression/span-bar-al... input/regression/span-bar-allow-span-bar.ly:2: \version "2.15.10" 2.15.12 http://codereview.appspot.com/4917046/diff/18001/input/regression/span-bar-al... input/regression/span-bar-allow-span-bar.ly:5: texidoc = "The SpanBarStub grob takes care of horizontal spacing for @code{SpanBarStub} http://codereview.appspot.com/4917046/diff/18001/input/regression/span-bar-al... input/regression/span-bar-allow-span-bar.ly:6: SpanBar grobs. When the SpanBar is disallowed, objects in contexts that @code{SpanBar} http://codereview.appspot.com/4917046/diff/18001/input/regression/span-bar-al... input/regression/span-bar-allow-span-bar.ly:14: \repeat unfold 64 { c''8 } } fix indentation http://codereview.appspot.com/4917046/diff/18001/lily/grob.cc File lily/grob.cc (right): http://codereview.appspot.com/4917046/diff/18001/lily/grob.cc#newcode617 lily/grob.cc:617: g->programming_error ("could not find this grob's vertical axis group in the vertical alignment."); remove full stop/period http://codereview.appspot.com/4917046/diff/18001/lily/grob.cc#newcode627 lily/grob.cc:627: g1->programming_error ("grob does not belong to a Vertical Alignment?"); VerticalAlignment http://codereview.appspot.com/4917046/diff/18001/lily/grob.cc#newcode643 lily/grob.cc:643: g1->programming_error ("could not situate this grob in its axis group"); prefer `place' to `situate' http://codereview.appspot.com/4917046/diff/18001/lily/pure-from-neighbor-engr... File lily/pure-from-neighbor-engraver.cc (right): http://codereview.appspot.com/4917046/diff/18001/lily/pure-from-neighbor-engr... lily/pure-from-neighbor-engraver.cc:46: pure_relevant_p_ = ly_lily_module_constant ("pure-relevant?"); you only use this once, so I'd get rid of it http://codereview.appspot.com/4917046/diff/18001/lily/pure-from-neighbor-engr... lily/pure-from-neighbor-engraver.cc:52: if (to_boolean (scm_apply_1 (pure_relevant_p_, i.item ()->self_scm (), SCM_EOL)) scm_call_1 ? http://codereview.appspot.com/4917046/diff/18001/lily/pure-from-neighbor-engr... lily/pure-from-neighbor-engraver.cc:53: && !i.grob ()->internal_has_interface (ly_symbol2scm ("pure-from-neighbor-interface"))) swap http://codereview.appspot.com/4917046/diff/18001/lily/pure-from-neighbor-engr... lily/pure-from-neighbor-engraver.cc:57: // note that this can get outta hand if there are lots of vertical axis groups... out of http://codereview.appspot.com/4917046/diff/18001/lily/pure-from-neighbor-inte... File lily/pure-from-neighbor-interface.cc (right): http://codereview.appspot.com/4917046/diff/18001/lily/pure-from-neighbor-inte... lily/pure-from-neighbor-interface.cc:34: MAKE_SCHEME_CALLBACK (Pure_from_neighbor_interface, elements_callback, 1); this sounds like it returns elements, whereas you're just processing the elements array http://codereview.appspot.com/4917046/diff/18001/lily/span-bar-engraver.cc File lily/span-bar-engraver.cc (right): http://codereview.appspot.com/4917046/diff/18001/lily/span-bar-engraver.cc#ne... lily/span-bar-engraver.cc:72: Grob *vag = Grob::get_root_vertical_alignment (bars_[0]); is this safe? http://codereview.appspot.com/4917046/diff/18001/lily/span-bar-stub-engraver.cc File lily/span-bar-stub-engraver.cc (right): http://codereview.appspot.com/4917046/diff/18001/lily/span-bar-stub-engraver.... lily/span-bar-stub-engraver.cc:55: // note that this can get outta hand if there are lots of vertical axis groups... out of http://codereview.appspot.com/4917046/diff/18001/lily/span-bar-stub-engraver.... lily/span-bar-stub-engraver.cc:116: //it->set_parent (y_parents[j], Y_AXIS); remove? http://codereview.appspot.com/4917046/diff/18001/lily/span-bar-stub-engraver.... lily/span-bar-stub-engraver.cc:122: it->set_property ("Y-extent", ly_interval2scm (Interval (infinity_f, -infinity_f))); SCM_BOOL_F
Sign in to reply to this message.
On 17 September 2011 15:45, Mike Solomon <mikesol@ufl.edu> wrote: > The problem comes not from this patch but from the calculation of the horizontal skylines for NonMusicalPaperColumns. Try adding: > > \override Score . NonMusicalPaperColumn #'stencil = #ly:separation-item::print > > To the beginning of SopranoMusic and then running LilyPond with -ddebug-skylines. You'll see that in the first example, the downward stem pushes the lyrics under the end point of the NonMusicalPaperColumn, whereas this does not happen in the second example. > > Thus, the dependency is not stems (drop the soprano an octave and you'll see that) but NonMusicalPaperColumn grobs. This isn't really a bug. NonMusicalPaperColumn has a default setting for skyline-vertical-padding which extends the skyline slightly. Cheers, Neil
Sign in to reply to this message.
Hey all, Responses to Neil and Joe below, and a new patchset posted. I'd like this patch to go on another countdown so that we can fully sort out the implementation of get_root_vertical_alignment. Cheers, MS http://codereview.appspot.com/4917046/diff/18001/input/regression/span-bar-al... File input/regression/span-bar-allow-span-bar.ly (right): http://codereview.appspot.com/4917046/diff/18001/input/regression/span-bar-al... input/regression/span-bar-allow-span-bar.ly:2: \version "2.15.10" On 2011/09/17 19:01:38, Neil Puttock wrote: > 2.15.12 Done. http://codereview.appspot.com/4917046/diff/18001/input/regression/span-bar-al... input/regression/span-bar-allow-span-bar.ly:5: texidoc = "The SpanBarStub grob takes care of horizontal spacing for On 2011/09/17 19:01:38, Neil Puttock wrote: > @code{SpanBarStub} Done. http://codereview.appspot.com/4917046/diff/18001/input/regression/span-bar-al... input/regression/span-bar-allow-span-bar.ly:6: SpanBar grobs. When the SpanBar is disallowed, objects in contexts that On 2011/09/17 19:01:38, Neil Puttock wrote: > @code{SpanBar} Done. http://codereview.appspot.com/4917046/diff/18001/input/regression/span-bar-al... input/regression/span-bar-allow-span-bar.ly:14: \repeat unfold 64 { c''8 } } On 2011/09/17 19:01:38, Neil Puttock wrote: > fix indentation Done. http://codereview.appspot.com/4917046/diff/18001/lily/grob.cc File lily/grob.cc (right): http://codereview.appspot.com/4917046/diff/18001/lily/grob.cc#newcode593 lily/grob.cc:593: return get_maybe_root_vertical_alignment (g, 0); On 2011/09/02 23:27:44, joeneeman wrote: > I still think this should say > return g->get_system ()->get_vertical_alignment (); > because there are several grobs that implement Align_interface and you want to > make sure you get the right one. When the grobs' root vertical alignment is accessed, get_system doesn't work yet for BarLine grobs because their ancestry along the X_AXIS has not yet been fully established. Grobs' Y_AXIS ancestry is established earlier in a timestep. Try replacing this function with: Grob* Grob::get_root_vertical_alignment (Grob *g) { System *s = g->get_system (); return s ? s->get_vertical_alignment () : 0; } Span bar stubs will cease to work because get_system will, for BarLines, always return 0. I'm not saying that your solution is impossible - I can imagine somehow setting BarLines' X-ancestors earlier in the translation process so that get_system always yields a system, but my preliminary attempts to implement this in this patch are coming up short and it seems like it will be a larger undertaking. I'd rather push this as it is and then have a discussion about whether get_root_vertical_alignment or get_system->get_vertical_alignment is a cleaner way to get the root vertical alignment. I see what you're saying that there are several grobs that implement the Align_interface, but get_root_vertical_alignment keeps recursing until it returns the one that has no Y_AXIS parent. I believe that all grobs save VerticalAlignment that implement the Align_interface have Y_AXIS parents. http://codereview.appspot.com/4917046/diff/18001/lily/grob.cc#newcode617 lily/grob.cc:617: g->programming_error ("could not find this grob's vertical axis group in the vertical alignment."); On 2011/09/17 19:01:38, Neil Puttock wrote: > remove full stop/period Done. http://codereview.appspot.com/4917046/diff/18001/lily/grob.cc#newcode627 lily/grob.cc:627: g1->programming_error ("grob does not belong to a Vertical Alignment?"); On 2011/09/17 19:01:38, Neil Puttock wrote: > VerticalAlignment Done. http://codereview.appspot.com/4917046/diff/18001/lily/grob.cc#newcode643 lily/grob.cc:643: g1->programming_error ("could not situate this grob in its axis group"); On 2011/09/17 19:01:38, Neil Puttock wrote: > prefer `place' to `situate' I'm officially losing my English! Done. http://codereview.appspot.com/4917046/diff/18001/lily/pure-from-neighbor-engr... File lily/pure-from-neighbor-engraver.cc (right): http://codereview.appspot.com/4917046/diff/18001/lily/pure-from-neighbor-engr... lily/pure-from-neighbor-engraver.cc:46: pure_relevant_p_ = ly_lily_module_constant ("pure-relevant?"); On 2011/09/17 19:01:38, Neil Puttock wrote: > you only use this once, so I'd get rid of it Done. http://codereview.appspot.com/4917046/diff/18001/lily/pure-from-neighbor-engr... lily/pure-from-neighbor-engraver.cc:52: if (to_boolean (scm_apply_1 (pure_relevant_p_, i.item ()->self_scm (), SCM_EOL)) On 2011/09/17 19:01:38, Neil Puttock wrote: > scm_call_1 ? Done. http://codereview.appspot.com/4917046/diff/18001/lily/pure-from-neighbor-engr... lily/pure-from-neighbor-engraver.cc:53: && !i.grob ()->internal_has_interface (ly_symbol2scm ("pure-from-neighbor-interface"))) On 2011/09/17 19:01:38, Neil Puttock wrote: > swap Done. http://codereview.appspot.com/4917046/diff/18001/lily/pure-from-neighbor-engr... lily/pure-from-neighbor-engraver.cc:57: // note that this can get outta hand if there are lots of vertical axis groups... On 2011/09/17 19:01:38, Neil Puttock wrote: > out of In spite of the fact that I'm losing my English, my New-Jerseyisms are not fading away. Done. http://codereview.appspot.com/4917046/diff/18001/lily/pure-from-neighbor-inte... File lily/pure-from-neighbor-interface.cc (right): http://codereview.appspot.com/4917046/diff/18001/lily/pure-from-neighbor-inte... lily/pure-from-neighbor-interface.cc:34: MAKE_SCHEME_CALLBACK (Pure_from_neighbor_interface, elements_callback, 1); On 2011/09/17 19:01:38, Neil Puttock wrote: > this sounds like it returns elements, whereas you're just processing the > elements array Renamed to filter_elements. http://codereview.appspot.com/4917046/diff/18001/lily/span-bar-engraver.cc File lily/span-bar-engraver.cc (right): http://codereview.appspot.com/4917046/diff/18001/lily/span-bar-engraver.cc#ne... lily/span-bar-engraver.cc:72: Grob *vag = Grob::get_root_vertical_alignment (bars_[0]); On 2011/09/17 19:01:38, Neil Puttock wrote: > is this safe? The presence of make_spanbar_ guarantees that bars_[0] will exist. http://codereview.appspot.com/4917046/diff/18001/lily/span-bar-stub-engraver.cc File lily/span-bar-stub-engraver.cc (right): http://codereview.appspot.com/4917046/diff/18001/lily/span-bar-stub-engraver.... lily/span-bar-stub-engraver.cc:4: Copyright (C) 1997--2011 Han-Wen Nienhuys <hanwen@xs4all.nl> On 2011/09/02 23:27:44, joeneeman wrote: > You're looking different today, Han-Wen. Indeed he is... Done. http://codereview.appspot.com/4917046/diff/18001/lily/span-bar-stub-engraver.... lily/span-bar-stub-engraver.cc:55: // note that this can get outta hand if there are lots of vertical axis groups... On 2011/09/17 19:01:38, Neil Puttock wrote: > out of Done. http://codereview.appspot.com/4917046/diff/18001/lily/span-bar-stub-engraver.... lily/span-bar-stub-engraver.cc:116: //it->set_parent (y_parents[j], Y_AXIS); On 2011/09/17 19:01:38, Neil Puttock wrote: > remove? Done. http://codereview.appspot.com/4917046/diff/18001/lily/span-bar-stub-engraver.... lily/span-bar-stub-engraver.cc:122: it->set_property ("Y-extent", ly_interval2scm (Interval (infinity_f, -infinity_f))); On 2011/09/17 19:01:38, Neil Puttock wrote: > SCM_BOOL_F This doesn't work for some reason...
Sign in to reply to this message.
Passes make, reg tests that get spewed out look identical, too many to attach but they are zipped here http://lilypond-stuff.1065243.n5.nabble.com/http-code-google-com-p-lilypond-i...
Sign in to reply to this message.
On Sep 24, 2011, at 9:31 PM, pkx166h@gmail.com wrote: > Passes make, reg tests that get spewed out look identical, too many to > attach but they are zipped here > > http://lilypond-stuff.1065243.n5.nabble.com/http-code-google-com-p-lilypond-i... > > http://codereview.appspot.com/4917046/ Just so you all know, the changes show up because SpanBar no longer has a Y-extent in this patch. Cheers, MS
Sign in to reply to this message.
On 2011/08/30 16:28:28, mikesol_ufl.edu wrote: > On Aug 30, 2011, at 5:53 PM, mailto:joeneeman@gmail.com wrote: > > > Correct me if I'm wrong, but this is my understanding: wherever there's > > a SpanBar, you're creating SpanBarStubs between every relevant pair of > > staves. These don't actually get printed; they're just there for the > > pure-height (because the SpanBar height is pretty much the whole system, > > so it doesn't tell you where the gaps are). > > > > Yes. > > > If that's correct, I have two broad comments: it's worth commenting > > somewhere (span-bar-stub-engraver.cc, maybe) that the purpose of > > SpanBarStub is for pure-height only. > > Will do. > Haven't done yet. > > But more importantly, isn't SpanBar > > now obsoleted by SpanBarStub? That is, you can just remove the SpanBar > > altogether and print the SpanBarStubs. > > > > Mmm...you're not unright, but I'd prefer to do that in another patch if that's > OK. It'd require a lot of deleting and moving around, and I'd first like to > make sure that these stubs are the code base and bug free for a while before I > deprecate an entire grob (and figure out how to deal with the syntax changes > that come with said deprecation). > Well, the SpanBarStub extent is not the full length to bridge between BarLines, it is only the height of the Lyrics, and only those Lyrics in the neighboring measures. SpanBarStub needs to change size, or get a bar-extent different from its Y-extent, before it can take over the job of printing SpanBars. I'm worried the code will be quite difficult to understand for quite a while unless Mike retreats.
Sign in to reply to this message.
On Nov 9, 2011, at 11:19 PM, k-ohara5a5a@oco.net wrote: > On 2011/08/30 16:28:28, mikesol_ufl.edu wrote: >> On Aug 30, 2011, at 5:53 PM, mailto:joeneeman@gmail.com wrote: > >> > Correct me if I'm wrong, but this is my understanding: wherever > there's >> > a SpanBar, you're creating SpanBarStubs between every relevant pair > of >> > staves. These don't actually get printed; they're just there for the >> > pure-height (because the SpanBar height is pretty much the whole > system, >> > so it doesn't tell you where the gaps are). >> > > >> Yes. > >> > If that's correct, I have two broad comments: it's worth commenting >> > somewhere (span-bar-stub-engraver.cc, maybe) that the purpose of >> > SpanBarStub is for pure-height only. > >> Will do. > > > Haven't done yet. Done now. > >> > But more importantly, isn't SpanBar >> > now obsoleted by SpanBarStub? That is, you can just remove the > SpanBar >> > altogether and print the SpanBarStubs. >> > > >> Mmm...you're not unright, but I'd prefer to do that in another patch > if that's >> OK. It'd require a lot of deleting and moving around, and I'd first > like to >> make sure that these stubs are the code base and bug free for a while > before I >> deprecate an entire grob (and figure out how to deal with the syntax > changes >> that come with said deprecation). > > > Well, the SpanBarStub extent is not the full length to bridge between > BarLines, it is only the height of the Lyrics, and only those Lyrics in > the neighboring measures. Yup. > > SpanBarStub needs to change size, or get a bar-extent different from its > Y-extent, before it can take over the job of printing SpanBars. > You're right - I think that was a bad idea, which is why I haven't followed up on it. > I'm worried the code will be quite difficult to understand for quite a > while unless Mike retreats. > I would rather create a killer flow chart that explains in as much detail as possible how all of this works than give up on code that I believe is correct given the way that this problem needs to be formulated. If someone said to me, for example, "Mike, music doesn't actually work like that - sharps should be allowed to pass over time signatures and clefs (pertaining to the example I sent out before), then I'd say "my bad, my research was incorrect, let's revert the patch." As I've said before, after reverting the patch, there are other ways to tackle the problem that this patch was originally designed to fix. Remembering back to the beam collision days (ah, the good old days...), we saw several critical issues arise from that, but at no point did people question the validity of getting rid of beam collisions. I think this is because, visually, everyone can say "ouch" just by looking at them. However, as I've studied the present issue more, I've realized that collisions between accidentals and the airspace above or below non-musical grobs is just as much of a 'collision' in traditional typesetting and needs a similar approach. Beams collect all of the colliding grobs in an array and then deal with them during their positioning callbacks. BarLines and other non musical grobs also need an array of pertinent grobs (in this case, "neighbors") that they can use during their callbacks. Cheers, MS
Sign in to reply to this message.
Message was sent while issue was closed.
https://codereview.appspot.com/4917046/diff/34001/lily/grob.cc File lily/grob.cc (right): https://codereview.appspot.com/4917046/diff/34001/lily/grob.cc#newcode647 lily/grob.cc:647: g1->programming_error ("could not place this grob in its axis group"); It sounds like you're mixing together "axis groups" and "Axis group engraver." The axis group engraver creates VerticalAxisGroups, which implement the axis-group-interface. There are, however, numerous grobs that implement the axis-group-interface that are not created in the Axis_group_engraver.
Sign in to reply to this message.
On 22 déc. 2012, at 23:14, k-ohara5a5a@oco.net wrote: > > https://codereview.appspot.com/4917046/diff/34001/lily/grob.cc > File lily/grob.cc (right): > > https://codereview.appspot.com/4917046/diff/34001/lily/grob.cc#newcode647 > lily/grob.cc:647: g1->programming_error ("could not place this grob in > its axis group"); > It sounds like you're mixing together "axis groups" and "Axis group > engraver." The axis group engraver creates VerticalAxisGroups, which > implement the axis-group-interface. There are, however, numerous grobs > that implement the axis-group-interface that are not created in the > Axis_group_engraver. > > https://codereview.appspot.com/4917046/ Couldn't have said it better myself! Wait a minute...
Sign in to reply to this message.
|