Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines.

Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines.

[Keith, 2012-09-01 23:58:37]

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

[MikeSol, 2012-09-02 06:25:58]

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.

[dak, 2012-09-02 15:59:00]

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#ne...
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#ne...
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#new...
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#...
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#...
lily/phrasing-slur-engraver.cc:332: map<Grob *, Grob *> 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<Grob *, vector<Grob *> > 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"?

[MikeSol, 2012-09-02 16:45:14]

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#ne...
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#new...
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#...
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#...
lily/phrasing-slur-engraver.cc:332: map<Grob *, Grob *> 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

[dak, 2012-09-02 17:02:12]

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#...
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#...
lily/phrasing-slur-engraver.cc:332: map<Grob *, Grob *> 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.

[Keith, 2012-09-02 20:38:28]

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.240s	0m0.254s	0m0.217s

[MikeSol, 2012-09-03 05:07:41]

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.240s	0m0.254s	0m0.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.

[mike7, 2012-09-03 13:46:33]

On 3 sept. 2012, at 07:07, mtsolo@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.240s	0m0.254s	0m0.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

[MikeSol, 2012-09-03 19:26:51]

On 2012/09/03 13:46:33, mike7 wrote:
> On 3 sept. 2012, at 07:07, mailto:mtsolo@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.240s	0m0.254s	0m0.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.

[MikeSol, 2012-09-04 06:40:58]

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

real	0m7.324s
user	0m6.944s
sys	0m0.192s

CURRENT MASTER


real	0m7.407s
user	0m6.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%.

[Keith, 2012-09-04 07:45:00]

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#...
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<Grob *>::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?

[Keith, 2012-09-04 07:53:39]

> (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?

[mike7, 2012-09-04 08:09:21]

On 4 sept. 2012, at 09:45, k-ohara5a5a@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#...
> 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<Grob *>::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/

[mail_philholmes.net, 2012-09-04 08:27:20]

----- Original Message ----- 
From: <mike@mikesolomon.org>
To: <mtsolo@gmail.com>; <k-ohara5a5a@oco.net>; <dak@gnu.org>; 
<mike@mikesolomon.org>; <lilypond-devel@gnu.org>; 
<reply@codereview-hr.appspotmail.com>
Sent: Tuesday, September 04, 2012 9:09 AM
Subject: Re: Approximates cross-staff slurs in 
VerticalAxisGroupvertical-skylines. (issue 6498077)
>
> 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?))).
>


AFAIK, we do.  It's just that the mechanism used to "test" the regtests is 
not guaranteed to find all changes :-(

--
Phil Holmes 

[dak, 2012-09-04 08:33:45]

On 2012/09/04 08:09:21, mike7 wrote:
> On 4 sept. 2012, at 09:45, mailto:k-ohara5a5a@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#...
> > 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<Grob *>::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.

[Graham Percival, 2012-09-04 11:15:30]

On Tue, Sep 04, 2012 at 08:33:45AM +0000, dak@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

[joeneeman, 2012-09-04 15:45:22]

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?

[mike7, 2012-09-04 22:33:35]

On 4 sept. 2012, at 17:45, joeneeman@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

[mike7, 2012-09-04 23:23:26]

On 5 sept. 2012, at 00:33, mike@mikesolomon.org wrote:

> 
> On 4 sept. 2012, at 17:45, joeneeman@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

[Keith, 2012-09-07 07:34:19]

On 2012/09/04 08:09:21, mike7 wrote:

> On 4 sept. 2012, at 09:45, mailto:k-ohara5a5a@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? 
  <http://k-ohara.oco.net/Lilypond/>
With ragged-bottoms, master allows about 8 collisions between slurs and the
pedaling in the system above, of which the patch removes 4.  That's not good
enough to be worth the extra %15 time for me, but I guess I can just turn it
off.

> 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.  

So SlurStubs have a different shape than slurs.  That would be an example of the
different information that I was asking about.

Having the invisible Grobs taking up space will confuse 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? )

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).

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.

> http://codereview.appspot.com/6498077/

[mike7, 2012-09-07 16:23:21]

On 7 sept. 2012, at 09:34, k-ohara5a5a@oco.net wrote:

> On 2012/09/04 08:09:21, mike7 wrote:
> 
>> On 4 sept. 2012, at 09:45, mailto:k-ohara5a5a@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?
>  <http://k-ohara.oco.net/Lilypond/>
> With ragged-bottoms, master allows about 8 collisions between slurs and
> the pedaling in the system above, of which the patch removes 4.  That's
> not good enough to be worth the extra %15 time for me, but I guess I can
> just turn it off.

In terms of sustainability, the most important question to ask at this point is
"does this method seem like one that could eventually suppress 8/8 collisions?"
I think it can, but I probably won't have the time to get it there as my summer
of lily comes to a close in 2 weeks and I'll disappear back into a hole save bug
fixes and occasional complaining.  So if we're gonna go w/ this, it's important
to agree that grob "stubs" are a good solution to this problem.  I am, of
course, open to other solutions.

The nice thing about this one is that it's easy to turn off.  Or, even better,
it's easy to turn on and we can leave it off as default.  The same goes for my
recent skyline work - I'm all for having different includable files in ly/ like

"fast.ly" % turns off tons of calculations to create an ugly score fast
"medium.ly" % does most calculations but avoids fine-tuned collision avoidance
"slow.ly" % the full monty

> 
>> 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.
> 
> So SlurStubs have a different shape than slurs.  That would be an
> example of the different information that I was asking about.
> 

Ja.

> 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.

> 
> 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
\override TextScript #'outside-staff-priority = ##f

you may also have to set the Y-offset to something in side-position-interface.cc
- I forget what the default Y-offset is.

> 
> Do you still think it possible to use just the real Slurs ?...
> 

No.  But, as I said above, I'm open to other suggestions.

> 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.  And furthermore, there needs to be a SlurStub for
each axis group traversed.

> 
> 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.

> 
>> http://codereview.appspot.com/6498077/
> 
> 
> http://codereview.appspot.com/6498077/

[Keith, 2012-09-08 06:06:56]

On Fri, 07 Sep 2012 09:23:08 -0700, mike@mikesolomon.org <mike@mikesolomon.org>
wrote:

>
> On 7 sept. 2012, at 09:34, k-ohara5a5a@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/
>

[mike7, 2012-09-08 15:44:01]

On 8 sept. 2012, at 09:06, Keith OHara <k-ohara5a5a@oco.net> wrote:

> On Fri, 07 Sep 2012 09:23:08 -0700, mike@mikesolomon.org
<mike@mikesolomon.org> wrote:
> 
>> 
>> On 7 sept. 2012, at 09:34, k-ohara5a5a@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/
>> 
> 

[mike7, 2012-09-10 04:19:35]

On 8 sept. 2012, at 18:43, mike@mikesolomon.org wrote:

> 
> On 8 sept. 2012, at 09:06, Keith OHara <k-ohara5a5a@oco.net> wrote:
> 
>> On Fri, 07 Sep 2012 09:23:08 -0700, mike@mikesolomon.org
<mike@mikesolomon.org> wrote:
>> 
>>> 
>>> On 7 sept. 2012, at 09:34, k-ohara5a5a@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

[Keith, 2012-09-10 20:26:07]

On 2012/09/07 16:23:21, mike7 wrote:
> On 7 sept. 2012, at 09:34, mailto:k-ohara5a5a@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-sl...
File input/regression/cross-staff-slur-vertical-spacing.ly (right):

http://codereview.appspot.com/6498077/diff/29/input/regression/cross-staff-sl...
input/regression/cross-staff-slur-vertical-spacing.ly:61: <g'' c''>8 )
 <g'' c''>8 )(

http://codereview.appspot.com/6498077/diff/29/input/regression/cross-staff-sl...
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-sl...
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.)

[mike7, 2012-09-11 04:55:08]

On 10 sept. 2012, at 23:26, k-ohara5a5a@oco.net wrote:

> On 2012/09/07 16:23:21, mike7 wrote:
>> On 7 sept. 2012, at 09:34, mailto:k-ohara5a5a@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-sl...
> File input/regression/cross-staff-slur-vertical-spacing.ly (right):
> 
>
http://codereview.appspot.com/6498077/diff/29/input/regression/cross-staff-sl...
> input/regression/cross-staff-slur-vertical-spacing.ly:61: <g'' c''>8 )
> <g'' c''>8 )(
> 
>
http://codereview.appspot.com/6498077/diff/29/input/regression/cross-staff-sl...
> 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-sl...
> 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/

[dak, 2012-09-11 20:52:34]

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.c...
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.

[Keith, 2012-09-22 18:06:47]

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

[dak, 2012-12-13 12:08:37]

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...
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....
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#newcod...
lily/slur-engraver.cc:124: * For every active slur, we create a slur stub.
Meaning notecolumns x slurs stubs.