Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)
On 22 déc. 2012, at 23:14, k-ohara5...@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...
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)
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/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)
On Nov 9, 2011, at 11:19 PM, k-ohara5...@oco.net wrote: > On 2011/08/30 16:28:28, mikesol_ufl.edu wrote: >> On Aug 30, 2011, at 5:53 PM, mailto:joenee...@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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)
On 2011/08/30 16:28:28, mikesol_ufl.edu wrote: On Aug 30, 2011, at 5:53 PM, mailto:joenee...@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. http://codereview.appspot.com/4917046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)
On Sep 24, 2011, at 9:31 PM, pkx1...@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-issues-detail-id-1846-td4837220.html > > 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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)
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-issues-detail-id-1846-td4837220.html http://codereview.appspot.com/4917046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)
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-allow-span-bar.ly
File input/regression/span-bar-allow-span-bar.ly (right):
http://codereview.appspot.com/4917046/diff/18001/input/regression/span-bar-allow-span-bar.ly#newcode2
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-allow-span-bar.ly#newcode5
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-allow-span-bar.ly#newcode6
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-allow-span-bar.ly#newcode14
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-engraver.cc
File lily/pure-from-neighbor-engraver.cc (right):
http://codereview.appspot.com/4917046/diff/18001/lily/pure-from-neighbor-engraver.cc#newcode46
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-engraver.cc#newcode52
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-engraver.cc#newcode53
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.apps
Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)
On 17 September 2011 15:45, Mike Solomon 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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)
http://codereview.appspot.com/4917046/diff/18001/input/regression/span-bar-allow-span-bar.ly
File input/regression/span-bar-allow-span-bar.ly (right):
http://codereview.appspot.com/4917046/diff/18001/input/regression/span-bar-allow-span-bar.ly#newcode2
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-allow-span-bar.ly#newcode5
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-allow-span-bar.ly#newcode6
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-allow-span-bar.ly#newcode14
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-engraver.cc
File lily/pure-from-neighbor-engraver.cc (right):
http://codereview.appspot.com/4917046/diff/18001/lily/pure-from-neighbor-engraver.cc#newcode46
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-engraver.cc#newcode52
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-engraver.cc#newcode53
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-engraver.cc#newcode57
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-interface.cc
File lily/pure-from-neighbor-interface.cc (right):
http://codereview.appspot.com/4917046/diff/18001/lily/pure-from-neighbor-interface.cc#newcode34
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#newcode72
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.cc#newcode55
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.cc#newcode116
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.cc#newcode122
lily/span-bar-stub-engraver.cc:122: it->set_property ("Y-extent",
ly_interval2scm (Interval (infinity_f, -infinity_f)));
SCM_BOOL_F
http://codereview.appspot.com/4917046/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)
On Sep 9, 2011, at 7:09 PM, tdanielsmu...@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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)
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
}
}
>>
}
http://codereview.appspot.com/4917046/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)
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.cc#newcode4 lily/span-bar-stub-engraver.cc:4: Copyright (C) 1997--2011 Han-Wen Nienhuys You're looking different today, Han-Wen. http://codereview.appspot.com/4917046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)
new patch passes make and reg tests http://codereview.appspot.com/4917046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)
On Sep 1, 2011, at 6:12 PM, bordage.bertr...@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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)
This requires an update. The patch fails to apply. Bertrand http://codereview.appspot.com/4917046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)
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 http://codereview.appspot.com/4917046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)
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 http://codereview.appspot.com/4917046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)
On Tue, Aug 30, 2011 at 8:58 AM, Mike Solomon wrote: > On Aug 30, 2011, at 5:53 PM, joenee...@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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)
On Aug 30, 2011, at 5:53 PM, joenee...@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#newcode116 > 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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)
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#newcode116 lily/span-bar-stub-engraver.cc:116: //it->set_parent (y_parents[j], Y_AXIS); Obsolete code? http://codereview.appspot.com/4917046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)
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.cc#newcode4
lily/pure-from-neighbor-engraver.cc:4: Copyright (C) 1997--2011 Han-Wen
Nienhuys
On 2011/08/27 14:23:16, hanwenn wrote:
?
Changed.
http://codereview.appspot.com/4917046/diff/1/lily/pure-from-neighbor-engraver.cc#newcode53
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_.
http://codereview.appspot.com/4917046/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)
On Aug 27, 2011, at 4:23 PM, hanw...@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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)
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.cc#newcode4
lily/pure-from-neighbor-engraver.cc:4: Copyright (C) 1997--2011 Han-Wen
Nienhuys
?
http://codereview.appspot.com/4917046/diff/1/lily/pure-from-neighbor-engraver.cc#newcode53
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 ?
http://codereview.appspot.com/4917046/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)
On Aug 27, 2011, at 10:40 AM, pkx1...@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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)
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/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)
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 http://codereview.appspot.com/4917046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)
Reviewers: , Message: 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 Description: Improves horizontal spacing of axis groups that span bars traverse. Please review this at http://codereview.appspot.com/4917046/ Affected files: A input/regression/span-bar-allow-span-bar.ly M lily/align-interface.cc M lily/include/align-interface.hh A lily/pure-from-neighbor-engraver.cc A lily/pure-from-neighbor-interface.cc M lily/span-bar-engraver.cc A lily/span-bar-stub-engraver.cc M lily/span-bar.cc M ly/engraver-init.ly M scm/define-grob-properties.scm M scm/define-grobs.scm M scm/output-lib.scm ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
