Re: Make sure slurs actually avoid stafflines. (issue 15400049)

[janek . lilypond]

Thanks for the review. I'll post a new patch tomorrow.

Janek


https://codereview.appspot.com/15400049/diff/1/lily/slur-configuration.cc
File lily/slur-configuration.cc (right):

https://codereview.appspot.com/15400049/diff/1/lily/slur-configuration.cc#newcode60
lily/slur-configuration.cc:60: // rather than decrease, because we want
to avoid too flat slurs.
On 2013/10/23 04:22:23, Keith wrote:
> I can't find that feature in the implementation.
> In fact, the required clearance is greater outside the slur than
inside, so you
> flatten more than your fatten.

??
This patch increases curvature of all slurs that would by default
have the belly above a staffline (so that they are at least
min_gap_above_staffline away from the staffline), and decreases
curvature of all slurs that would by default have the belly below
a staffline (so that they are at least min_gap_below_staffline away)

Precisely because min_gap_above_staffline > min_gap_below_staffline
we are more likely to increase than decrease curvature.

https://codereview.appspot.com/15400049/diff/1/lily/slur-configuration.cc#newcode84
lily/slur-configuration.cc:84: Real const slur_th = state.thickness_ *
staff_th * 10;
On 2013/10/23 04:22:23, Keith wrote:
> Why 10?  Is that the thickness of the fattest part of the slur?

No, it's because staff_th is measured in staffspaces, while
state.thickness_ appears to be measured in 1/10ths of
staffline-thicknesses (i'm sure that it's not the real thickness of the
slur). To get real slur thickness, we have to multiply these values, but
not when they are measured in differrent units.

For example, in

\new Staff \with { \override StaffSymbol thickness = #5 }
\relative c'' {
  a2( d)
}

state.thickness_ reports to be 0.12, and staff_th is 0.5.

https://codereview.appspot.com/15400049/diff/1/lily/slur-configuration.cc#newcode93
lily/slur-configuration.cc:93: // TODO: to handle weird staves well, we
should round to staffline positions.
On 2013/10/23 04:22:23, Keith wrote:
> Staves with an even number of lines are fairly common.  The old code
found the
> nearest staff-position, as opposed to staff-line, and moved the slur
if there
> was a staff-line drawn there and the slur would be too close. Maybe
restore that
> aspect of the old code.

That would be problematic. Imagine a slur where norm_y = n+0.26
staffspaces, where n is an integer.  I.e, the clearance between belly of
the curve  and staffline is 0.26ss - (slur thickness + staff
thickness)/2, i.e. most likely 0.15ss or less.  We want to correct such
slurs, but if we rounded to positions, we would not be measuring this
distance (because n+0.26 ss would get rounded to n+0.5 ss).

If we want to support non-default staves, we need to write a function
round_to_staffline.

https://codereview.appspot.com/15400049/diff/1/lily/slur-configuration.cc#newcode99
lily/slur-configuration.cc:99: && Staff_symbol_referencer::on_staff_line
(staff, int (2 * round)))
On 2013/10/23 04:22:23, Keith wrote:
> 'round' has a sign-change depending on the slur direction, so it is
not right
> for passing to on_staff_line().

Good catch!

> Maybe keep the Real quantities with signs meaning UP/DOWN and use only
> 'which_side' to keep track of whether the nearest potential staff-line
is inside
> or outside the slur.

I'll see what i can do.

https://codereview.appspot.com/15400049/diff/1/lily/slur-configuration.cc#newcode114
lily/slur-configuration.cc:114: b = P0*(1-t)^3 + P2*t*(1-t)^2 +
P3*(1-t)*t^2 + P4*t^3
On 2013/10/23 04:22:23, Keith wrote:
> Below, you number them P0 P1 P2 P3

oops, sorry!

https://codereview.appspot.com/15400049/diff/1/lily/slur-configuration.cc#newcode121
lily/slur-configuration.cc:121: Real mid_pts_cor = 0.6 * correction / (3
* (t - (t * t)));
On 2013/10/23 04:22:23, Keith wrote:
> This could still move the control points quite a lot, and make a
funny-shaped
> slur, if the horizontal part is near one end of the slur.  Then t=0 or
t=1
> nearly.

Indeed, i've encountered this problem today.

> Maybe try first to move only the middle control points, but limit
their motion
> to half a staff space
> mid_pts_cor = min(correction/(3t-3t²)*state.dir_, 0.5) *state.dir_
> all_pts_cor = correction - mid_pts_cor * (3t-3t²)

I would prefer an exact solution.
Actually, i think that we should fix this by handling the slur more
completely, i.e. by including slur-end staffline avoidance in this
function and deciding on the slur based on its whole configuration.

But implementing this will take time...

https://codereview.appspot.com/15400049/diff/1/lily/slur-configuration.cc#newcode121
lily/slur-configuration.cc:121: Real mid_pts_cor = 0.6 * correction / (3
* (t - (t * t)));
On 2013/10/23 07:16:37, Keith wrote:
> It is not too bad, and we just started review.
> 0.6 is one empirical constant from which 0.4 = 1.0 - 0.6 follows,
> 3 comes from the definition of cubic Bezier curves, etc.

Indeed, it's like you wrote.

https://codereview.appspot.com/15400049/diff/1/lily/slur.cc
File lily/slur.cc (right):

https://codereview.appspot.com/15400049/diff/1/lily/slur.cc#newcode520
lily/slur.cc:520: "Minimum clearance to staffline above slur belly.\n"
On 2013/10/23 04:22:23, Keith wrote:
> "... to the staffline outside the slur"
> not necessarily above.

Done.

https://codereview.appspot.com/15400049/diff/1/lily/slur.cc#newcode522
lily/slur.cc:522: "Minimum clearance to staffline below slur belly.\n"
On 2013/10/23 04:22:23, Keith wrote:
> "... to the staffline inside the slur"

Done.

https://codereview.appspot.com/15400049/