Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
Ugh, found those unsent "Draft Comments" in my backlog, no idea whether they still apply. Might be worth glancing over. https://codereview.appspot.com/6498077/diff/28/lily/phrasing-slur-engraver.cc File lily/phrasing-slur-engraver.cc (right): https://codereview.appspot.com/6498077/diff/28/lily/phrasing-slur-engraver.cc#newcode322 lily/phrasing-slur-engraver.cc:322: // There are likely SlurStubs we don't need. Get rid of them. Why don't you need them? Which don't you need? What happens with the rest? https://codereview.appspot.com/6498077/diff/6033/lily/phrasing-slur-engraver.cc File lily/phrasing-slur-engraver.cc (right): https://codereview.appspot.com/6498077/diff/6033/lily/phrasing-slur-engraver.cc#newcode324 lily/phrasing-slur-engraver.cc:324: // There are likely SlurStubs we don't need. Get rid of them. The whole loop is uncommented. What SlurStubs do you not need? What SlurStubs _do_ you need? How do you distinguish them? Why don't you need some? How do you get rid of them? What happens with those that you keep? https://codereview.appspot.com/6498077/diff/6033/lily/slur-engraver.cc File lily/slur-engraver.cc (right): https://codereview.appspot.com/6498077/diff/6033/lily/slur-engraver.cc#newcode124 lily/slur-engraver.cc:124: * For every active slur, we create a slur stub. Meaning notecolumns x slurs stubs. https://codereview.appspot.com/6498077/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
http://codereview.appspot.com/6498077/diff/15001/lily/script-column.cc File lily/script-column.cc (right): http://codereview.appspot.com/6498077/diff/15001/lily/script-column.cc#newcode59 lily/script-column.cc:59: * avoid-staff of slur trumps script priority. If one grob is 'avoid-slur supersedes 'script-priority http://codereview.appspot.com/6498077/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
http://codereview.appspot.com/6498077/diff/6033/lily/phrasing-slur-engraver.cc
File lily/phrasing-slur-engraver.cc (right):
http://codereview.appspot.com/6498077/diff/6033/lily/phrasing-slur-engraver.cc#newcode116
lily/phrasing-slur-engraver.cc:116: Grob *stub = make_spanner
("SlurStub", info.grob ()->self_scm ());
Why do you use the NoteColumn here as the cause for the SlurStub? It
would make much more sense to use the respective slur. Then you don't
need a separate "surrogate" field since you can just use 'cause instead,
and you can \tweak a SlurStub via its Slur event, and most importantly
you don't need a special "initialize from surrogate" utility function
but can rather use an "initialize from cause" function.
http://codereview.appspot.com/6498077/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
On 10 sept. 2012, at 23:26, k-ohara5...@oco.net wrote:
> On 2012/09/07 16:23:21, mike7 wrote:
>> On 7 sept. 2012, at 09:34, mailto:k-ohara5...@oco.net wrote:
>
>>> Do you still think it possible to use just the real Slurs ?...
>>>
>>> 1) setting tentative control points using pre-line-breaking
>>> estimates of heights (which are later replaced when the Slurs
>>> go through their post-line-breaking shaping-and-scoring cycle).
>
>> This doesn't work because all of this skyline stuff happens
>> pre-line breaking but post-vertical spacing.
>
> Did you mean "after line-breaking but before vertical-spacing"?
Yes, was tired.
>
>> And furthermore, there needs to be a SlurStub for
>> each axis group traversed.
>
> I didn't know that. I thought one skyline per Slur would suffice, since
> it is applied for spacing the Systems.
I thought this too when I started working on this bit of code but I learned
otherwise, ergo the approach. Check out
Page_layout_problem::build_system_skyline. The system skyline used for spacing
is actually built from the vertical axis groups.
>
>>> 2) Determining their extremal-side-only skylines, either through
>>> callbacks on a property other than the real "vertical-skylines", > >
> or not as a callback at all but through a direct function call.
I'm wary of direct function calls to control placement - I like the use of
properties and callbacks whenever possible.
>
>> Even if this were done, it couldn't be applied to the Slur proper.
>> For example, if you have slur S that cross staves X Y and Z from
>> bottom to top, the SlurStub on X would only have a bottom skyline,
>> the SlurStub on Y would have no skyline and the slur stub on Z
>> would have an upper skyline. If the Slur had these two skylines
>> via some sort of pre-skyline callback, LilyPond would think that
>> the VerticalAxisGroup were the height of the Slur and would space
>> it way far apart from its upper or lower neighbor. That's why
>> separate stubs need to be in each axis group.
>
> Then we don't store the extremal-side-only skylines as a property of the
> Slur. We merely use the data in the Slur to compute extremal-side-only
> skylines, and merge them with the System skylines for system-system
> spacing.
This results in a fair bit of extra code - I had a version of the patchset like
this, but it leads to funniness like, for example, the skylines not appearing
when debug-skylines is set to #t because the skylines are not part of the
vertical axis group. In general, the way of doing things in the pushed
patchset (creating grobs, assigning them properties and letting them get placed
in skylines and such via the normal flow of things) is more lilypondish and
leads to more maintainable code because it is more predictable.
>
> But, that does seem more trouble than it is worth, given that the
> estimated slur shapes are only accurate enough to resolve about 50% of
> the collisions.
>
I think this accuracy can be improved - I stand by what I stated before, which
is that improvements can come later if the method in place is sound.
>
>
> http://codereview.appspot.com/6498077/diff/29/input/regression/cross-staff-slur-vertical-spacing.ly
> File input/regression/cross-staff-slur-vertical-spacing.ly (right):
>
> http://codereview.appspot.com/6498077/diff/29/input/regression/cross-staff-slur-vertical-spacing.ly#newcode61
> input/regression/cross-staff-slur-vertical-spacing.ly:61: 8 )
> 8 )(
>
> http://codereview.appspot.com/6498077/diff/29/input/regression/cross-staff-slur-vertical-spacing.ly#newcode63
> input/regression/cross-staff-slur-vertical-spacing.ly:63: e8 dis e
> e8 dis e)
>
> http://codereview.appspot.com/6498077/diff/29/input/regression/cross-staff-slur-vertical-spacing.ly#newcode70
> input/regression/cross-staff-slur-vertical-spacing.ly:70: a8 a8 a8 a8 a8
> a8 a8_\markup \column { "f" "o" "o" } a8 a8 a8
>
> Stubs for down-sloped slurs are not helping.
>
> http://codereview.appspot.com/6498077/diff/29/lily/script-column.cc
> File lily/script-column.cc (right):
>
> http://codereview.appspot.com/6498077/diff/29/lily/script-column.cc#newcode58
> lily/script-column.cc:58: /*
> This seems entirely unrelated to spacing of systems. It looks like a
> sensible fix to issue 2589, but it causes a collision in the Chopin
> Op.45 measure 85, for reasons I cannot quite figure out. One fix for
> that score is
> \once\override Script #'avoid-slur = #'inside
I'll try to have a look at it today or tomorrow.
>
> I'd rather have a message in debug builds than a collision in user
> scores. (Maybe that message should be a warning rather than 'programming
> error', since LilyPond allows users to make inconsistent spacing
> requests like I did with Fingering inside and Accents avoiding slurs,
> yet fingering outside the accent when they occur together.)
>
> http://codereview.appspot.com/6498077/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
ht
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
On 2012/09/07 16:23:21, mike7 wrote:
On 7 sept. 2012, at 09:34, mailto:k-ohara5...@oco.net wrote:
> Do you still think it possible to use just the real Slurs ?...
>
> 1) setting tentative control points using pre-line-breaking
> estimates of heights (which are later replaced when the Slurs
> go through their post-line-breaking shaping-and-scoring cycle).
This doesn't work because all of this skyline stuff happens
pre-line breaking but post-vertical spacing.
Did you mean "after line-breaking but before vertical-spacing"?
And furthermore, there needs to be a SlurStub for
each axis group traversed.
I didn't know that. I thought one skyline per Slur would suffice, since
it is applied for spacing the Systems.
> 2) Determining their extremal-side-only skylines, either through
> callbacks on a property other than the real "vertical-skylines", > >
or not as a callback at all but through a direct function call.
Even if this were done, it couldn't be applied to the Slur proper.
For example, if you have slur S that cross staves X Y and Z from
bottom to top, the SlurStub on X would only have a bottom skyline,
the SlurStub on Y would have no skyline and the slur stub on Z
would have an upper skyline. If the Slur had these two skylines
via some sort of pre-skyline callback, LilyPond would think that
the VerticalAxisGroup were the height of the Slur and would space
it way far apart from its upper or lower neighbor. That's why
separate stubs need to be in each axis group.
Then we don't store the extremal-side-only skylines as a property of the
Slur. We merely use the data in the Slur to compute extremal-side-only
skylines, and merge them with the System skylines for system-system
spacing.
But, that does seem more trouble than it is worth, given that the
estimated slur shapes are only accurate enough to resolve about 50% of
the collisions.
http://codereview.appspot.com/6498077/diff/29/input/regression/cross-staff-slur-vertical-spacing.ly
File input/regression/cross-staff-slur-vertical-spacing.ly (right):
http://codereview.appspot.com/6498077/diff/29/input/regression/cross-staff-slur-vertical-spacing.ly#newcode61
input/regression/cross-staff-slur-vertical-spacing.ly:61: 8 )
8 )(
http://codereview.appspot.com/6498077/diff/29/input/regression/cross-staff-slur-vertical-spacing.ly#newcode63
input/regression/cross-staff-slur-vertical-spacing.ly:63: e8 dis e
e8 dis e)
http://codereview.appspot.com/6498077/diff/29/input/regression/cross-staff-slur-vertical-spacing.ly#newcode70
input/regression/cross-staff-slur-vertical-spacing.ly:70: a8 a8 a8 a8 a8
a8 a8_\markup \column { "f" "o" "o" } a8 a8 a8
Stubs for down-sloped slurs are not helping.
http://codereview.appspot.com/6498077/diff/29/lily/script-column.cc
File lily/script-column.cc (right):
http://codereview.appspot.com/6498077/diff/29/lily/script-column.cc#newcode58
lily/script-column.cc:58: /*
This seems entirely unrelated to spacing of systems. It looks like a
sensible fix to issue 2589, but it causes a collision in the Chopin
Op.45 measure 85, for reasons I cannot quite figure out. One fix for
that score is
\once\override Script #'avoid-slur = #'inside
I'd rather have a message in debug builds than a collision in user
scores. (Maybe that message should be a warning rather than 'programming
error', since LilyPond allows users to make inconsistent spacing
requests like I did with Fingering inside and Accents avoiding slurs,
yet fingering outside the accent when they occur together.)
http://codereview.appspot.com/6498077/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
On 8 sept. 2012, at 18:43, m...@mikesolomon.org wrote: > > On 8 sept. 2012, at 09:06, Keith OHara wrote: > >> On Fri, 07 Sep 2012 09:23:08 -0700, m...@mikesolomon.org >> wrote: >> >>> >>> On 7 sept. 2012, at 09:34, k-ohara5...@oco.net wrote: >>> Having the invisible Grobs taking up space will confuse the innocent. >>> >>> I tried to add comments about this in the source - perhaps the CG needs an >>> invisible grob bit, as we have StemStubs and SpanBarStubs as well. >> >> People who have read the source code and contributors guide are not "the >> innocent". > > Ah, ok. I can put the idea of stubs in the docs too - this is an entirely > comprehensible concept for those who understand basic engraving principles, > as this is how engravers reserve space in many cases. They use physical > objects that take up space but are never printed to block other objects. Did some thinking, and I think it'd be good to have a separate patch after this with a section in Chapter 1 of the notation manual about invisible grobs. In LilyPond, we currently have: --) Collection grobs that facilitate alignment (DynamicLineSpanner, ScriptColumn, NoteColumn) --) Spacer grobs that block other ones (LeftEdge, StemStub, SpanBarStub) Both of these categories are related in that they're invisible and therefore may not be intuitive to override. I'm not very good at doc writing but if no one else wants to write about these grobs I could give it a shot. Cheers, MS ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
On 8 sept. 2012, at 09:06, Keith OHara wrote: > On Fri, 07 Sep 2012 09:23:08 -0700, m...@mikesolomon.org > wrote: > >> >> On 7 sept. 2012, at 09:34, k-ohara5...@oco.net wrote: >> >>> Having the invisible Grobs taking up space will confuse the innocent. >> >> I tried to add comments about this in the source - perhaps the CG needs an >> invisible grob bit, as we have StemStubs and SpanBarStubs as well. > > People who have read the source code and contributors guide are not "the > innocent". Ah, ok. I can put the idea of stubs in the docs too - this is an entirely comprehensible concept for those who understand basic engraving principles, as this is how engravers reserve space in many cases. They use physical objects that take up space but are never printed to block other objects. > >>> >>> In measure 36 of my example, it seems I used a text script for custom >>> fingering placement. That "4" moves to avoid the SlurStub. (How would >>> you put it back where it belongs, as a user? ) >> >> \override TextScript #'avoid-slur = #'inside > > But, looking at the output, the "4" is /already/ inside the slur, so why > would anybody think to try that? These invisible grobs pushing things around > would cause confusion in released code. Hmm...will think it over. I agree with you that it's confusing - I think that a combination of documentation and perhaps warnings would help users with this. Cheers, MS > >> http://codereview.appspot.com/6498077/ >> > ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
On Fri, 07 Sep 2012 09:23:08 -0700, m...@mikesolomon.org wrote: On 7 sept. 2012, at 09:34, k-ohara5...@oco.net wrote: Having the invisible Grobs taking up space will confuse the innocent. I tried to add comments about this in the source - perhaps the CG needs an invisible grob bit, as we have StemStubs and SpanBarStubs as well. People who have read the source code and contributors guide are not "the innocent". In measure 36 of my example, it seems I used a text script for custom fingering placement. That "4" moves to avoid the SlurStub. (How would you put it back where it belongs, as a user? ) \override TextScript #'avoid-slur = #'inside But, looking at the output, the "4" is /already/ inside the slur, so why would anybody think to try that? These invisible grobs pushing things around would cause confusion in released code. http://codereview.appspot.com/6498077/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
On 7 sept. 2012, at 09:34, k-ohara5...@oco.net wrote:
> On 2012/09/04 08:09:21, mike7 wrote:
>
>> On 4 sept. 2012, at 09:45, mailto:k-ohara5...@oco.net wrote:
>
>> > It makes no change for the Chopin; can you give an
>> > example where it helps?
>
>> In the Chopin, ragged-bottom is false so the difference can't
>> really be seen. The piece isn't a good test case for how the
>> patch changes engraving but it is an excellent test case for
>> efficiency.
>
> Did you need me to type \paper{ ragged-bottom=##5 } for you?
>
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
On 2012/09/04 08:09:21, mike7 wrote:
On 4 sept. 2012, at 09:45, mailto:k-ohara5...@oco.net wrote:
> It makes no change for the Chopin; can you give an
> example where it helps?
In the Chopin, ragged-bottom is false so the difference can't
really be seen. The piece isn't a good test case for how the
patch changes engraving but it is an excellent test case for
efficiency.
Did you need me to type \paper{ ragged-bottom=##5 } for you?
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
On 5 sept. 2012, at 00:33, m...@mikesolomon.org wrote: > > On 4 sept. 2012, at 17:45, joenee...@gmail.com wrote: > >> On 2012/09/02 06:25:58, MikeSol wrote: >>> On 2012/09/01 23:58:37, Keith wrote: I might have a test case for you at http://www.mutopiaproject.org/cgibin/piece-info.cgi?id=1776 It seems you copy each slur into a "slur-stub", and from those keep >> only the ones with "cross-staff" set. Then when figuring system skylines you >> insert all Grobs with the slur-stub-interface into the skylines for each staff. Why not insert the original Slur into the skylines, if it has >> "cross-staff" instead of the SlurStub. For that matter, why not insert all Grobs >> marked "cross-staff"? all the cross-staff >> >>> It's not a copy of the original slur because it is using pure heights >> and >>> offsets. >> >> I realize I'm a bit late to this party, but why do you need a separate >> grob instead of just a Slur::get_maybe_pure_vertical_skylines function? >> >> >> http://codereview.appspot.com/6498077/ > > Welcome to the party! > > We're on the same page - that's exactly what I've been working on over the > past day-ish in my spare time. I don't have a clean result yet as it'd > require some trickery with finding minimum translations of VerticalAxisGroups > by hand (if the stubs belong to different vertical axis groups this is done > automatically when grob offsets are calculated). I'm almost there though - > I'll post a patch once this is done. > > We need to come up with a term for this that's not `pure' as `pure' means in > LilyPond jargon "anything that does not result in a set_object, set_property, > suicide, the calling of the SpacingSpanner or the calling of a > VerticalAlignment." This would result in the calling of the spacing spanner > were it called further upstream, so we need a different thing to call it. > > Cheers, > MS After further reflection, the idea of stashing the callback in a single skyline doesn't work :-( If they are part of a single VerticalAxisGroup, they will force tons of space between the VerticalAxisGroup to which the skyline belongs and whatever axis group falls immediately above or below. You'll notice that I set the bottom of the top skyline and the top of the bottom skyline to empty so that this doesn't happen. If you remove this in Slur::extremal_stub_vertical_skylines, you'll see huge vertical shifts in the regtest and other scores. I know this is supposed to be a comment as per Graham's suggestion but out of force of habit I wrote it in an e-mail. Will write up as a comment. Cheers, MS ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
On 4 sept. 2012, at 17:45, joenee...@gmail.com wrote: > On 2012/09/02 06:25:58, MikeSol wrote: >> On 2012/09/01 23:58:37, Keith wrote: >>> I might have a test case for you at >>> http://www.mutopiaproject.org/cgibin/piece-info.cgi?id=1776 >>> >>> It seems you copy each slur into a "slur-stub", and from those keep > only the >>> ones with "cross-staff" set. Then when figuring system skylines you > insert all >>> Grobs with the slur-stub-interface into the skylines for each staff. >>> >>> Why not insert the original Slur into the skylines, if it has > "cross-staff" >>> instead of the SlurStub. For that matter, why not insert all Grobs > marked >>> "cross-staff"? >>> >>> all the cross-staff > >> It's not a copy of the original slur because it is using pure heights > and >> offsets. > > I realize I'm a bit late to this party, but why do you need a separate > grob instead of just a Slur::get_maybe_pure_vertical_skylines function? > > > http://codereview.appspot.com/6498077/ Welcome to the party! We're on the same page - that's exactly what I've been working on over the past day-ish in my spare time. I don't have a clean result yet as it'd require some trickery with finding minimum translations of VerticalAxisGroups by hand (if the stubs belong to different vertical axis groups this is done automatically when grob offsets are calculated). I'm almost there though - I'll post a patch once this is done. We need to come up with a term for this that's not `pure' as `pure' means in LilyPond jargon "anything that does not result in a set_object, set_property, suicide, the calling of the SpacingSpanner or the calling of a VerticalAlignment." This would result in the calling of the spacing spanner were it called further upstream, so we need a different thing to call it. Cheers, MS ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
On 2012/09/02 06:25:58, MikeSol wrote: On 2012/09/01 23:58:37, Keith wrote: > I might have a test case for you at > http://www.mutopiaproject.org/cgibin/piece-info.cgi?id=1776 > > It seems you copy each slur into a "slur-stub", and from those keep only the > ones with "cross-staff" set. Then when figuring system skylines you insert all > Grobs with the slur-stub-interface into the skylines for each staff. > > Why not insert the original Slur into the skylines, if it has "cross-staff" > instead of the SlurStub. For that matter, why not insert all Grobs marked > "cross-staff"? > > all the cross-staff It's not a copy of the original slur because it is using pure heights and offsets. I realize I'm a bit late to this party, but why do you need a separate grob instead of just a Slur::get_maybe_pure_vertical_skylines function? http://codereview.appspot.com/6498077/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
On Tue, Sep 04, 2012 at 08:33:45AM +, d...@gnu.org wrote: -snip review- > So, _now_ please take every sentence and every answer in this mail, > rewrite it in the form of a comment and stick it in the file in the > places where people would be looking for it. Yes. > _This_ is the kind of information that needs to be in comments in the > file, at the places where you would be looking for it. Putting it in > some comments in a Rietveld tracker does not make sense. Yes. > Probably we should make it a rule that a patch submitter can't post > followup comments but only followup patches. That way the information > ends up where it needs to be. I don't think we should have such a blanket rule for everybody (particularly for new contributors), but I would *really* like it if Mike tried this as an experiment. I suggest that for the next 7 days, we forbid Mike from replying to any reviews. When people ask questions about his patches and what they're doing, he should either watch the fumbling discussion to see how badly people interpret how the patch works, or upload new patches with comments. Of course, this exercise will only work if other developers make a point of trying to understand what Mike's patches are doing. - Graham ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
On 2012/09/04 08:09:21, mike7 wrote: On 4 sept. 2012, at 09:45, mailto:k-ohara5...@oco.net wrote: > Works for me. 16% slower than master. > (I'll try make clean and make.) > It makes no change for the Chopin; can you give an example where it > helps? In the Chopin, ragged-bottom is false so the difference can't really be seen. The piece isn't a good test case for how the patch changes engraving but it is an excellent test case for efficiency. > > I still do not understand how creating separate SlurStubs helps. Check out the regtest. Also checkout les-nereides.ly (which is what the patch was created to fix (and does fix (why don't we have les-nereides.ly in the regtest comparison?))). > At the > time when we build system skylines, what information is in the stubs > that is not also in the slurs themselves? It is not that there is more information in the stubs - it's just different. They use an approximation of the vertical-skylines instead of the actual ones at a point in the code where skylines are needed but the actual ones cannot be used (to wit: Axis_group_interface::skyline_spacing). If we calculated the actual control points for cross staff slurs at the time VerticalAxisGroup skylines were calculated, it would trigger a vertical alignment. As an experiment, try removing the lines that weed out cross-staff grobs in Axis_group_interface::skyline_spacing and run all of the beam regtests - you'll see what I mean. > Can you explain the need for > stubs ? > -- without using the word 'pure' because I do not know what you mean > when you say that word -- you could mean 'with the boolean of that name > set to true' in any one of dozens of functions. > I need to use the word pure, but I'll use it judiciously :-) SlurStubs run the control point calculations using not the actual heights and coordinates (which would trigger a vertical alignment) but rather pure heights and coordinates. Checkout the changes to slur-scoring.cc: almost all of them are changing extent to maybe_pure_extent and stuff of that ilk. In the case of stubs, this generates approximations of control points and therefore an approximation of the vertical skylines. We feed this approximation into the vertical skyline calculations. > > http://codereview.appspot.com/6498077/diff/4046/lily/axis-group-interface.cc > File lily/axis-group-interface.cc (right): > > http://codereview.appspot.com/6498077/diff/4046/lily/axis-group-interface.cc#newcode806 > lily/axis-group-interface.cc:806: > Axis_group_interface::add_interior_skylines > move this back up to line 616 for the sake of history > ok > http://codereview.appspot.com/6498077/diff/4046/lily/slur-engraver.cc > File lily/slur-engraver.cc (right): > > http://codereview.appspot.com/6498077/diff/4046/lily/slur-engraver.cc#newcode347 > lily/slur-engraver.cc:347: vector::const_iterator it = > // Keep just one stub per Staff-like construct > // Remove stubs if we have seen one already in the same vag, > > ?? Do slurs that stay within a single staff produce stubs, and do we > remove them here? Ya. There is no way to know if a slur will stay within a single staff until all its noteheads have been acknowledged. http://codereview.appspot.com/6498077/ So, _now_ please take every sentence and every answer in this mail, rewrite it in the form of a comment and stick it in the file in the places where people would be looking for it. _This_ is the kind of information that needs to be in comments in the file, at the places where you would be looking for it. Putting it in some comments in a Rietveld tracker does not make sense. Probably we should make it a rule that a patch submitter can't post followup comments but only followup patches. That way the information ends up where it needs to be. http://codereview.appspot.com/6498077/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
On 4 sept. 2012, at 09:45, k-ohara5...@oco.net wrote: > Works for me. 16% slower than master. > (I'll try make clean and make.) > It makes no change for the Chopin; can you give an example where it > helps? In the Chopin, ragged-bottom is false so the difference can't really be seen. The piece isn't a good test case for how the patch changes engraving but it is an excellent test case for efficiency. > > I still do not understand how creating separate SlurStubs helps. Check out the regtest. Also checkout les-nereides.ly (which is what the patch was created to fix (and does fix (why don't we have les-nereides.ly in the regtest comparison?))). > At the > time when we build system skylines, what information is in the stubs > that is not also in the slurs themselves? It is not that there is more information in the stubs - it's just different. They use an approximation of the vertical-skylines instead of the actual ones at a point in the code where skylines are needed but the actual ones cannot be used (to wit: Axis_group_interface::skyline_spacing). If we calculated the actual control points for cross staff slurs at the time VerticalAxisGroup skylines were calculated, it would trigger a vertical alignment. As an experiment, try removing the lines that weed out cross-staff grobs in Axis_group_interface::skyline_spacing and run all of the beam regtests - you'll see what I mean. > Can you explain the need for > stubs ? > -- without using the word 'pure' because I do not know what you mean > when you say that word -- you could mean 'with the boolean of that name > set to true' in any one of dozens of functions. > I need to use the word pure, but I'll use it judiciously :-) SlurStubs run the control point calculations using not the actual heights and coordinates (which would trigger a vertical alignment) but rather pure heights and coordinates. Checkout the changes to slur-scoring.cc: almost all of them are changing extent to maybe_pure_extent and stuff of that ilk. In the case of stubs, this generates approximations of control points and therefore an approximation of the vertical skylines. We feed this approximation into the vertical skyline calculations. > > http://codereview.appspot.com/6498077/diff/4046/lily/axis-group-interface.cc > File lily/axis-group-interface.cc (right): > > http://codereview.appspot.com/6498077/diff/4046/lily/axis-group-interface.cc#newcode806 > lily/axis-group-interface.cc:806: > Axis_group_interface::add_interior_skylines > move this back up to line 616 for the sake of history > ok > http://codereview.appspot.com/6498077/diff/4046/lily/slur-engraver.cc > File lily/slur-engraver.cc (right): > > http://codereview.appspot.com/6498077/diff/4046/lily/slur-engraver.cc#newcode347 > lily/slur-engraver.cc:347: vector::const_iterator it = > // Keep just one stub per Staff-like construct > // Remove stubs if we have seen one already in the same vag, > > ?? Do slurs that stay within a single staff produce stubs, and do we > remove them here? Ya. There is no way to know if a slur will stay within a single staff until all its noteheads have been acknowledged. http://codereview.appspot.com/6498077/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
(I'll try make clean and make.) 16% slower than master. It makes no change for the Chopin; can you give an example where it helps? http://codereview.appspot.com/6498077/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
Works for me. 16% slower than master. (I'll try make clean and make.) It makes no change for the Chopin; can you give an example where it helps? I still do not understand how creating separate SlurStubs helps. At the time when we build system skylines, what information is in the stubs that is not also in the slurs themselves? Can you explain the need for stubs ? -- without using the word 'pure' because I do not know what you mean when you say that word -- you could mean 'with the boolean of that name set to true' in any one of dozens of functions. http://codereview.appspot.com/6498077/diff/4046/lily/axis-group-interface.cc File lily/axis-group-interface.cc (right): http://codereview.appspot.com/6498077/diff/4046/lily/axis-group-interface.cc#newcode806 lily/axis-group-interface.cc:806: Axis_group_interface::add_interior_skylines move this back up to line 616 for the sake of history http://codereview.appspot.com/6498077/diff/4046/lily/slur-engraver.cc File lily/slur-engraver.cc (right): http://codereview.appspot.com/6498077/diff/4046/lily/slur-engraver.cc#newcode347 lily/slur-engraver.cc:347: vector::const_iterator it = // Keep just one stub per Staff-like construct // Remove stubs if we have seen one already in the same vag, ?? Do slurs that stay within a single staff produce stubs, and do we remove them here? http://codereview.appspot.com/6498077/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
The speed problem was twofold - some cruft in callbacks coupled with the fact that I wasn't doing make cleans, so something about the way that gcc was putting together the old .o files was slowing things down. I learned my lesson: always do make clean before testing a patch. A full make clean brings compilation time on Keith's test case down to: PATCH real0m7.324s user0m6.944s sys 0m0.192s CURRENT MASTER real0m7.407s user0m6.828s sys 0m0.304s Usually about a 3% increase for scores with lots of cross staff slurs, almost nothing otherwise. Note that this is with an optimized build that is not debugging or profiled enabled. All of those flags add about 4ish%. http://codereview.appspot.com/6498077/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
On 2012/09/03 13:46:33, mike7 wrote:
On 3 sept. 2012, at 07:07, mailto:mts...@gmail.com wrote:
> On 2012/09/02 20:38:28, Keith wrote:
>> On 2012/09/02 06:25:58, MikeSol wrote:
>
>>> It's not a copy of the original slur because it is using
>>> pure heights and offsets.
>
>> I saw you interrogating SlurStub regarding its purity, but did not
> notice that
>> SlurStub took any different shape based on 'pure' estimates.
>
>> The SlurStubs in the regtest were shaped just like the printed
Slurs,
> but now I
>> see the difference in the Chopin prelude, with
>
>> \layout { \context { \Score
>> \override SlurStub #'color = #green
>> \override SlurStub #'transparent = ##f
>> \override Slur #'color = #darkgreen
>> \override PhrasingSlur #'color = #darkgreen }}
>
>> The SlurStubs are not sufficiently accurate to help, and they cost
me
> time.
>
>> Chopin op45: patch 'skylines' 2.16
>>real0m21.666s 0m16.245s 0m12.862s
>>user0m20.814s 0m15.573s 0m12.232s
>>sys 0m0.240s0m0.254s0m0.217s
>
> I'm working on a new version of the patch. There is no way to
improve
> accuracy of the curve, but the Y-offset is often wrong and that can
be
> improved.
>
> The time hike seems awfully expensive - there must be a way to
optimize
> it. I'll post something that works when I have it and then we can
> figure out how to optimize it.
>
> http://codereview.appspot.com/6498077/
>
> ___
> lilypond-devel mailing list
> mailto:lilypond-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/lilypond-devel
New version posted. I'm also getting a 25% increase on the Op. 45
test case.
I am getting severe hikes across the board, though, even in scores
without cross
staff slurs. What's odd is that the hikes are happening in the
"Drawing
systems" phase (I think), although I'm positive via prints to the
command line
that the SlurStub's control points and vertical skylines are never
being
calculated.
Lemme know if you see something slipping through the cracks - gprof
shows a hike
in stencil integral and skyline code for my patch, so it must be
generating
extra skylines somewhere.
Cheers,
MS
After doing some digging, it looks like that even if not a single new
grob is created (meaning if I comment out all SlurStub stuff in the
engraver), the time increase is still there. It is tough to see where
this is coming from using profiling tools, but it seems like the hike is
in the Drawing Systems phase. I'm guessing that it may have to do with
all of the maybe_pure stuff. If anyone has time to do profiling I'd
appreciate help on this patch - it leads to an appreciable improvement
in several scores I've checked out w/ cross-staff slurs, but it
obviously can't be put on a countdown until the performance hit gets
taken care of.
http://codereview.appspot.com/6498077/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
On 3 sept. 2012, at 07:07, mts...@gmail.com wrote:
> On 2012/09/02 20:38:28, Keith wrote:
>> On 2012/09/02 06:25:58, MikeSol wrote:
>
>>> It's not a copy of the original slur because it is using
>>> pure heights and offsets.
>
>> I saw you interrogating SlurStub regarding its purity, but did not
> notice that
>> SlurStub took any different shape based on 'pure' estimates.
>
>> The SlurStubs in the regtest were shaped just like the printed Slurs,
> but now I
>> see the difference in the Chopin prelude, with
>
>> \layout { \context { \Score
>> \override SlurStub #'color = #green
>> \override SlurStub #'transparent = ##f
>> \override Slur #'color = #darkgreen
>> \override PhrasingSlur #'color = #darkgreen }}
>
>> The SlurStubs are not sufficiently accurate to help, and they cost me
> time.
>
>> Chopin op45: patch 'skylines' 2.16
>>real 0m21.666s 0m16.245s 0m12.862s
>>user 0m20.814s 0m15.573s 0m12.232s
>>sys 0m0.240s0m0.254s0m0.217s
>
> I'm working on a new version of the patch. There is no way to improve
> accuracy of the curve, but the Y-offset is often wrong and that can be
> improved.
>
> The time hike seems awfully expensive - there must be a way to optimize
> it. I'll post something that works when I have it and then we can
> figure out how to optimize it.
>
> http://codereview.appspot.com/6498077/
>
> ___
> lilypond-devel mailing list
> lilypond-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/lilypond-devel
New version posted. I'm also getting a 25% increase on the Op. 45 test case.
I am getting severe hikes across the board, though, even in scores without
cross staff slurs. What's odd is that the hikes are happening in the "Drawing
systems" phase (I think), although I'm positive via prints to the command line
that the SlurStub's control points and vertical skylines are never being
calculated.
Lemme know if you see something slipping through the cracks - gprof shows a
hike in stencil integral and skyline code for my patch, so it must be
generating extra skylines somewhere.
Cheers,
MS
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
On 2012/09/02 20:38:28, Keith wrote:
On 2012/09/02 06:25:58, MikeSol wrote:
> It's not a copy of the original slur because it is using
> pure heights and offsets.
I saw you interrogating SlurStub regarding its purity, but did not
notice that
SlurStub took any different shape based on 'pure' estimates.
The SlurStubs in the regtest were shaped just like the printed Slurs,
but now I
see the difference in the Chopin prelude, with
\layout { \context { \Score
\override SlurStub #'color = #green
\override SlurStub #'transparent = ##f
\override Slur #'color = #darkgreen
\override PhrasingSlur #'color = #darkgreen }}
The SlurStubs are not sufficiently accurate to help, and they cost me
time.
Chopin op45:patch 'skylines' 2.16
real 0m21.666s 0m16.245s 0m12.862s
user 0m20.814s 0m15.573s 0m12.232s
sys0m0.240s0m0.254s0m0.217s
I'm working on a new version of the patch. There is no way to improve
accuracy of the curve, but the Y-offset is often wrong and that can be
improved.
The time hike seems awfully expensive - there must be a way to optimize
it. I'll post something that works when I have it and then we can
figure out how to optimize it.
http://codereview.appspot.com/6498077/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
On 2012/09/02 06:25:58, MikeSol wrote:
It's not a copy of the original slur because it is using
pure heights and offsets.
I saw you interrogating SlurStub regarding its purity, but did not
notice that SlurStub took any different shape based on 'pure' estimates.
The SlurStubs in the regtest were shaped just like the printed Slurs,
but now I see the difference in the Chopin prelude, with
\layout { \context { \Score
\override SlurStub #'color = #green
\override SlurStub #'transparent = ##f
\override Slur #'color = #darkgreen
\override PhrasingSlur #'color = #darkgreen }}
The SlurStubs are not sufficiently accurate to help, and they cost me
time.
Chopin op45:patch 'skylines' 2.16
real0m21.666s 0m16.245s 0m12.862s
user0m20.814s 0m15.573s 0m12.232s
sys 0m0.240s0m0.254s0m0.217s
http://codereview.appspot.com/6498077/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
http://codereview.appspot.com/6498077/diff/21/lily/phrasing-slur-engraver.cc File lily/phrasing-slur-engraver.cc (right): http://codereview.appspot.com/6498077/diff/21/lily/phrasing-slur-engraver.cc#newcode119 lily/phrasing-slur-engraver.cc:119: if (slur_stubs_.find (slurs_[j]) == slur_stubs_.end ()) On 2012/09/02 16:45:14, MikeSol wrote: On 2012/09/02 15:59:00, dak wrote: > It is quite nonsensical to have slur_stubs be a map indexed via slurs_[j] rather > than just an array indexed through j. > > That is inefficient use of time, memory, and complexity. It is sensical because: --) It avoids having two vectors (slur_stubs_ and end_slur_stubs_ would be needed), thus making the code easier to read and maintain. --) It has a "find" method built into it. I don't see that a "find" method is advantageous over just indexing. And typical scores _have_ thousands of slurs. Since you don't bother writing even a single comment, it is rather hard to figure out what you are doing, and you are doing it in complicated way. http://codereview.appspot.com/6498077/diff/21/lily/phrasing-slur-engraver.cc#newcode332 lily/phrasing-slur-engraver.cc:332: map vag_to_slur; On 2012/09/02 16:45:14, MikeSol wrote: On 2012/09/02 15:59:00, dak wrote: > Again: why a map here? find method, see above. I know that find exists for vectors in algorithm, but I think this is easier to read and more consistent. You are apparently going to a lot of effort of getting your map entries out again when you could just keep arrays with an identical structure to the slurs_ and end_slurs_ arrays. Again, since you don't bother writing any comment regarding what you are trying to do, it is hard to figure out what the purpose is. http://codereview.appspot.com/6498077/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
Thanks for the review!
http://codereview.appspot.com/6498077/diff/21/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):
http://codereview.appspot.com/6498077/diff/21/lily/axis-group-interface.cc#newcode419
lily/axis-group-interface.cc:419: */
On 2012/09/02 15:59:00, dak wrote:
See above.
Removed - was old code I needed to read to make sure I waas getting
stuff right. Forgot it waas there.
http://codereview.appspot.com/6498077/diff/21/lily/page-layout-problem.cc
File lily/page-layout-problem.cc (right):
http://codereview.appspot.com/6498077/diff/21/lily/page-layout-problem.cc#newcode1004
lily/page-layout-problem.cc:1004: if (is_spaceable (staves[i]))
On 2012/09/02 15:59:00, dak wrote:
This indentation is simply wrong and does not match the program
structure.
Cruft. Fixed.
http://codereview.appspot.com/6498077/diff/21/lily/phrasing-slur-engraver.cc
File lily/phrasing-slur-engraver.cc (right):
http://codereview.appspot.com/6498077/diff/21/lily/phrasing-slur-engraver.cc#newcode119
lily/phrasing-slur-engraver.cc:119: if (slur_stubs_.find (slurs_[j]) ==
slur_stubs_.end ())
On 2012/09/02 15:59:00, dak wrote:
It is quite nonsensical to have slur_stubs be a map indexed via
slurs_[j] rather
than just an array indexed through j.
That is inefficient use of time, memory, and complexity.
It is sensical because:
--) It avoids having two vectors (slur_stubs_ and end_slur_stubs_ would
be needed), thus making the code easier to read and maintain.
--) It has a "find" method built into it.
Unless people are crunching scores with thousands of slurs on Strong
Bad's original Tandy 400, I'm not too worried about the tradeoff.
http://codereview.appspot.com/6498077/diff/21/lily/phrasing-slur-engraver.cc#newcode332
lily/phrasing-slur-engraver.cc:332: map vag_to_slur;
On 2012/09/02 15:59:00, dak wrote:
Again: why a map here?
find method, see above.
I know that find exists for vectors in algorithm, but I think this is
easier to read and more consistent.
http://codereview.appspot.com/6498077/diff/21/lily/script-column.cc
File lily/script-column.cc (right):
http://codereview.appspot.com/6498077/diff/21/lily/script-column.cc#newcode60
lily/script-column.cc:60: if (unsmob_grob (i1->get_object ("slur")) &&
unsmob_grob (i2->get_object ("slur")))
On 2012/09/02 15:59:00, dak wrote:
If both scripts have a field "slur" that is a grob, return #t is the
avoid-slur
property of the second script is "outside" or "around" while that of
the first
is neither.
Why don't you write this in a comment? Why this complex code? And
what is the
ultimate purpose?
I didn't put it in a comment because it can be understood by reading it,
as you explain with your eloquent prose :-)
This exists to avoid the situation where a grob has a lower slur
priority than another but is typeset outside a slur whereas the other is
put inside, leading to a contradiction. Priority is given to the slur
directive.
http://codereview.appspot.com/6498077/diff/21/lily/slur.cc
File lily/slur.cc (right):
http://codereview.appspot.com/6498077/diff/21/lily/slur.cc#newcode623
lily/slur.cc:623: "thickness "
On 2012/09/02 15:59:00, dak wrote:
No entry for "surrogate"?
scm/define-grob-interfaces.scm
http://codereview.appspot.com/6498077/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
http://codereview.appspot.com/6498077/diff/21/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):
http://codereview.appspot.com/6498077/diff/21/lily/axis-group-interface.cc#newcode390
lily/axis-group-interface.cc:390: /*
Where is the point in putting a whole callback inside of a comment? Is
this a TODO "make callback work again"? Or what? Of course, using /*
*/ for outcommenting the whole callback relies on no single comment ever
appearing inside of the whole function since comments don't nest.
I have no doubt that this is the predominant coding style in LilyPond,
and a syntax error is a small punishment for violating this convention,
but the usual (and nestable) way of outcommenting a part of source is to
use #if 0. As well as an explanatory comment just _why_ things are
outcommented instead of removed.
http://codereview.appspot.com/6498077/diff/21/lily/axis-group-interface.cc#newcode419
lily/axis-group-interface.cc:419: */
See above.
http://codereview.appspot.com/6498077/diff/21/lily/page-layout-problem.cc
File lily/page-layout-problem.cc (right):
http://codereview.appspot.com/6498077/diff/21/lily/page-layout-problem.cc#newcode1004
lily/page-layout-problem.cc:1004: if (is_spaceable (staves[i]))
This indentation is simply wrong and does not match the program
structure.
http://codereview.appspot.com/6498077/diff/21/lily/phrasing-slur-engraver.cc
File lily/phrasing-slur-engraver.cc (right):
http://codereview.appspot.com/6498077/diff/21/lily/phrasing-slur-engraver.cc#newcode119
lily/phrasing-slur-engraver.cc:119: if (slur_stubs_.find (slurs_[j]) ==
slur_stubs_.end ())
It is quite nonsensical to have slur_stubs be a map indexed via
slurs_[j] rather than just an array indexed through j.
That is inefficient use of time, memory, and complexity.
http://codereview.appspot.com/6498077/diff/21/lily/phrasing-slur-engraver.cc#newcode332
lily/phrasing-slur-engraver.cc:332: map vag_to_slur;
Again: why a map here?
http://codereview.appspot.com/6498077/diff/21/lily/script-column.cc
File lily/script-column.cc (right):
http://codereview.appspot.com/6498077/diff/21/lily/script-column.cc#newcode49
lily/script-column.cc:49: ? 1
? 1 : 0 is totally unnecessary since || already returns 1 or 0.
http://codereview.appspot.com/6498077/diff/21/lily/script-column.cc#newcode60
lily/script-column.cc:60: if (unsmob_grob (i1->get_object ("slur")) &&
unsmob_grob (i2->get_object ("slur")))
If both scripts have a field "slur" that is a grob, return #t is the
avoid-slur property of the second script is "outside" or "around" while
that of the first is neither.
Why don't you write this in a comment? Why this complex code? And what
is the ultimate purpose?
http://codereview.appspot.com/6498077/diff/21/lily/slur-engraver.cc
File lily/slur-engraver.cc (right):
http://codereview.appspot.com/6498077/diff/21/lily/slur-engraver.cc#newcode56
lily/slur-engraver.cc:56: map > slur_stubs_;
see comments in phrasing slur engraver.
http://codereview.appspot.com/6498077/diff/21/lily/slur.cc
File lily/slur.cc (right):
http://codereview.appspot.com/6498077/diff/21/lily/slur.cc#newcode623
lily/slur.cc:623: "thickness "
No entry for "surrogate"?
http://codereview.appspot.com/6498077/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
Reviewers: Keith, Message: On 2012/09/01 23:58:37, Keith wrote: I might have a test case for you at http://www.mutopiaproject.org/cgibin/piece-info.cgi?id=1776 It seems you copy each slur into a "slur-stub", and from those keep only the ones with "cross-staff" set. Then when figuring system skylines you insert all Grobs with the slur-stub-interface into the skylines for each staff. Why not insert the original Slur into the skylines, if it has "cross-staff" instead of the SlurStub. For that matter, why not insert all Grobs marked "cross-staff"? all the cross-staff It's not a copy of the original slur because it is using pure heights and offsets. The original slur can't be inserted into the VerticalAxisGroup skylines because its height and offset depends on the distance between axis groups, which is not known at the time the skyline is created. So we use the pure version (the stub) to build the system skylines in page-layout-problem.cc. Note that we don't use it for the minimum-translations (I made a minimum-translation-vertical-skylines property for that) because we don't want cross-staff grobs to affect the distance between two axis groups in the same system. We want them just to appear in the aggregate of all vertical axis groups smooshed into one system, created in the function Page_layout_problem::build_system_skyline. The test case shows that the patch is not ready - I need to make it overshoot less. But the general idea will remain the same. Description: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. The approximation is almost always an overshoot for the time being. A more robust strategy would be to create SlurStubs for every VerticalAxisGroup on which the cross-staff slur encompassed a note column. Then, the skyline added would be the union of all these skylines where the top skyline acted as a top bound and the low skyline acted as a lower bound, so if intermediary skylines went over these bounds, those parts of the skyline would be thrown out. This can be done in a separate commit: it'd require making a Skyline::crop function that crops one skyline by another and would require creating multiple SlurStubs in the engraver as well as figuring out the right amount to shift them for each vertical axis group. Please review this at http://codereview.appspot.com/6498077/ Affected files: A input/regression/cross-staff-slur-vertical-spacing.ly M lily/align-interface.cc M lily/axis-group-interface.cc M lily/figured-bass-position-engraver.cc M lily/grob.cc M lily/include/axis-group-interface.hh M lily/include/grob.hh M lily/include/slur-scoring.hh M lily/include/slur.hh M lily/melody-engraver.cc M lily/phrasing-slur-engraver.cc M lily/slur-engraver.cc M lily/slur-scoring.cc M lily/slur.cc M lily/tab-tie-follow-engraver.cc M scm/define-grob-interfaces.scm 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
Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
I might have a test case for you at http://www.mutopiaproject.org/cgibin/piece-info.cgi?id=1776 It seems you copy each slur into a "slur-stub", and from those keep only the ones with "cross-staff" set. Then when figuring system skylines you insert all Grobs with the slur-stub-interface into the skylines for each staff. Why not insert the original Slur into the skylines, if it has "cross-staff" instead of the SlurStub. For that matter, why not insert all Grobs marked "cross-staff"? all the cross-staff http://codereview.appspot.com/6498077/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
