Fixes bad slur heights by limiting fit_factor to the interior of slurs.

Fixes bad slur heights by limiting fit_factor to the interior of slurs.

[MikeSol, 2011-08-05 00:20:18]

This is much shorter than my previous work on this issue and is in the spirit of
Han-Wen's 6c6ae92410f087d0318a3c302eb6877cfc7fae45.  fit_factor only makes sense
when applied to the middle of curves (what we can consider their steady states).
 Near their attachments (what could be called their "transients"), there is a
lot of volatility because the way they look and the avoidance obstacles they
face are of a different order.

Instead of defining the middle of curve in terms of curve length, I use a
constant number here.  It seems to do the trick better than a variable length.

This fixes Issue 163 and only changes one regtest (for the better, I think).

Cheers,
MS

[hanwenn, 2011-08-05 01:50:15]

LGTM

On Thu, Aug 4, 2011 at 9:20 PM,  <mtsolo@gmail.com> wrote:

> Instead of defining the middle of curve in terms of curve length, I use
> a constant number here.  It seems to do the trick better than a variable
> length.



8fdea86edb73b0a1fb4829e0a4fcf39932828a8f..083fc5935df3b626ea9a13a7207354f8497a13a2
> 100644
> --- a/lily/slur-configuration.cc
> +++ b/lily/slur-configuration.cc
> @@ -95,7 +95,10 @@ fit_factor (Offset dz_unit, Offset dz_perp,
>       Real eps = 0.01;
>       Interval pext = eps * Interval (-1, 1) + p[X_AXIS];
>       pext.intersect (curve_xext);
> -      if (pext.is_empty () || pext.length () <= 1.999 * eps)
> +      if ((pext.is_empty ()
> +          || pext.length () <= 1.999 * eps
> +          || (p[X_AXIS] - curve_xext[LEFT])  < 2.5
> +          || (curve_xext[RIGHT] - p[X_AXIS]) < 2.5))

Basic logic seems sane.  You may adopt a todo to make the 2.5
parameterized; whether or not the point is 'far' from the edge depends
on the shape and length of the curve, so it could theoretically be
calculated from a scale-free parameter.

Two requests:

* add a bool close_to_edge to make the condition easier to read.

* add a parameter rather than hardcoding.  I know we have too many
parameters, but given that this uses an arbitrary number 2.5,  it's
better to have that number together with other numbers.

-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

[MikeSol, 2011-08-05 09:03:32]

On 2011/08/05 01:50:15, hanwenn wrote:
> LGTM
> 
> On Thu, Aug 4, 2011 at 9:20 PM,  <mailto:mtsolo@gmail.com> wrote:
> 
> > Instead of defining the middle of curve in terms of curve length, I use
> > a constant number here. &nbsp;It seems to do the trick better than a
variable
> > length.
> 
> 
> 
>
8fdea86edb73b0a1fb4829e0a4fcf39932828a8f..083fc5935df3b626ea9a13a7207354f8497a13a2
> > 100644
> > --- a/lily/slur-configuration.cc
> > +++ b/lily/slur-configuration.cc
> > @@ -95,7 +95,10 @@ fit_factor (Offset dz_unit, Offset dz_perp,
> > &nbsp; &nbsp; &nbsp; Real eps = 0.01;
> > &nbsp; &nbsp; &nbsp; Interval pext = eps * Interval (-1, 1) + p[X_AXIS];
> > &nbsp; &nbsp; &nbsp; pext.intersect (curve_xext);
> > - &nbsp; &nbsp; &nbsp;if (pext.is_empty () || pext.length () <= 1.999 * eps)
> > + &nbsp; &nbsp; &nbsp;if ((pext.is_empty ()
> > + &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;|| pext.length () <= 1.999 * eps
> > + &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;|| (p[X_AXIS] - curve_xext[LEFT])
&nbsp;< 2.5
> > + &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;|| (curve_xext[RIGHT] - p[X_AXIS]) <
2.5))
> 
> Basic logic seems sane.  You may adopt a todo to make the 2.5
> parameterized; whether or not the point is 'far' from the edge depends
> on the shape and length of the curve, so it could theoretically be
> calculated from a scale-free parameter.
> 
> Two requests:
> 
> * add a bool close_to_edge to make the condition easier to read.
> 
> * add a parameter rather than hardcoding.  I know we have too many
> parameters, but given that this uses an arbitrary number 2.5,  it's
> better to have that number together with other numbers.
> 
> -- 
> Han-Wen Nienhuys - mailto:hanwen@xs4all.nl - http://www.xs4all.nl/%7Ehanwen

New patch w/ Han-Wen's suggestions & a regtest online.

[hanwenn, 2011-08-05 12:08:29]

LGTM

http://codereview.appspot.com/4810072/diff/3002/input/regression/slur-height-...
File input/regression/slur-height-capping.ly (right):

http://codereview.appspot.com/4810072/diff/3002/input/regression/slur-height-...
input/regression/slur-height-capping.ly:4: texidoc = "Lilypond ignores
extra-collision-objects towards
"slur shape is not adapted to accomodate objects towards the edges of the slurs"

(we should still score for collisions)

http://codereview.appspot.com/4810072/diff/3002/lily/slur-configuration.cc
File lily/slur-configuration.cc (right):

http://codereview.appspot.com/4810072/diff/3002/lily/slur-configuration.cc#ne...
lily/slur-configuration.cc:102: close_to_edge = close_to_edge || -d * (p[X_AXIS]
- curve_xext[d]) < close_to_edge_length;
i'd simply use fabs.

[MikeSol, 2011-08-05 18:15:11]

http://codereview.appspot.com/4810072/diff/3002/input/regression/slur-height-...
File input/regression/slur-height-capping.ly (right):

http://codereview.appspot.com/4810072/diff/3002/input/regression/slur-height-...
input/regression/slur-height-capping.ly:4: texidoc = "Lilypond ignores
extra-collision-objects towards
On 2011/08/05 12:08:29, hanwenn wrote:
> "slur shape is not adapted to accomodate objects towards the edges of the
slurs"
> 
> (we should still score for collisions)

Changed.

http://codereview.appspot.com/4810072/diff/3002/lily/slur-configuration.cc
File lily/slur-configuration.cc (right):

http://codereview.appspot.com/4810072/diff/3002/lily/slur-configuration.cc#ne...
lily/slur-configuration.cc:102: close_to_edge = close_to_edge || -d * (p[X_AXIS]
- curve_xext[d]) < close_to_edge_length;
On 2011/08/05 12:08:29, hanwenn wrote:
> i'd simply use fabs.

I'm reticent to use fabs just cuz its not very lilypondish given the way other
stuff like this is written (see, for example, get_y_attachment_range, written by
a young Han-Wen Nienhuys :) )

[Keith, 2011-08-06 08:04:18]

I like this approach.

http://codereview.appspot.com/4810072/diff/3002/lily/slur-configuration.cc
File lily/slur-configuration.cc (right):

http://codereview.appspot.com/4810072/diff/3002/lily/slur-configuration.cc#ne...
lily/slur-configuration.cc:102: close_to_edge = close_to_edge || -d * (p[X_AXIS]
- curve_xext[d]) < close_to_edge_length;
This is quite obfuscated.  I am too old to remember details like whether LEFT<0
in Lilypond.

close_to_edge =  p[X_AXIS] < curve_xext[LEFT]  + c_t_e_l
              || p[X_AXIS] > curve_xext[RIGHT] - c_t_e_l;

I think you are right to have the ignore region be length in staff-spaces,
rather than a fraction of the length of the slur.  Did you try both ways?

[Janek Warchol, 2011-08-06 09:09:32]

Hi Mike,

i tried writing a review, but i don't understand what's going on here. Can you
add some comments to the code?

cheers,
Janek

[mike_apollinemike.com, 2011-08-06 15:37:12]

On Aug 6, 2011, at 11:09 AM, lemniskata.bernoullego@gmail.com wrote:

> Hi Mike,
> 
> i tried writing a review, but i don't understand what's going on here.
> Can you add some comments to the code?
> 
> cheers,
> Janek
> 

The function fit_factor pushes up the height of a slur if there is an
extra-encompass-object in the way.  My change makes this function ignore all
extra-encompass-objects close to the extremes of a slur, as these objects have a
tendency to push a slur up disproportionately high.  These objects are still
penalized in the scoring, which results in their avoidance if possible.

Cheers,
MS

[pkx166h, 2011-08-06 21:04:51]

Passes make and one reg test difference

http://code.google.com/p/lilypond/issues/detail?id=163#c23

see attached there.

[Janek Warchol, 2011-08-07 09:33:51]

2011/8/6 mike@apollinemike.com <mike@apollinemike.com>:
> On Aug 6, 2011, at 11:09 AM, lemniskata.bernoullego@gmail.com wrote:
>> i tried writing a review, but i don't understand what's going on here.
>> Can you add some comments to the code?
>
> The function fit_factor pushes up the height of a slur if there is an
extra-encompass-object in the way.  My change makes this function ignore all
extra-encompass-objects close to the extremes of a slur, as these objects have a
tendency to push a slur up disproportionately high.  These objects are still
penalized in the scoring, which results in their avoidance if possible.

Thanks.  Could you add this to the source as a comment?
This sounds reasonable, however i still have a hard time reading this
code because variable names aren't descriptive...  What dz_unit,
dz_perp, curve_xext are?  What pext.intersect (curve_xext) does?

cheers,
Janek

[mike_apollinemike.com, 2011-08-07 09:46:59]

On Aug 7, 2011, at 11:33 AM, Janek Warchoł wrote:

> 2011/8/6 mike@apollinemike.com <mike@apollinemike.com>:
>> On Aug 6, 2011, at 11:09 AM, lemniskata.bernoullego@gmail.com wrote:
>>> i tried writing a review, but i don't understand what's going on here.
>>> Can you add some comments to the code?
>> 
>> The function fit_factor pushes up the height of a slur if there is an
extra-encompass-object in the way.  My change makes this function ignore all
extra-encompass-objects close to the extremes of a slur, as these objects have a
tendency to push a slur up disproportionately high.  These objects are still
penalized in the scoring, which results in their avoidance if possible.
> 
> Thanks.  Could you add this to the source as a comment?
> This sounds reasonable, however i still have a hard time reading this
> code because variable names aren't descriptive...  What dz_unit,
> dz_perp, curve_xext are?  What pext.intersect (curve_xext) does?
> 
> cheers,
> Janek

Hey Janek,

I'm certainly not against adding comments, but I think that when the code does a
good job of explaining stuff via variable names, comments can clutter what's
going on.  For example, dz_unit to me sounds like a unit vector (which it is),
dz_perp is the vector perpendicular to this, and curve_xext is the extent of the
curve from attachment point to attachment point.  This could potentially be the
subject of a future GOP, but I am of the opinion that if code is easy to read
because of good variable naming, it is not necessary to add more comments.

Cheers,
MS

[Janek Warchol, 2011-08-07 13:00:24]

2011/8/7 mike@apollinemike.com <mike@apollinemike.com>:
> On Aug 7, 2011, at 11:33 AM, Janek Warchoł wrote:
>
>> 2011/8/6 mike@apollinemike.com <mike@apollinemike.com>:
>>> On Aug 6, 2011, at 11:09 AM, lemniskata.bernoullego@gmail.com wrote:
>>>> i tried writing a review, but i don't understand what's going on here.
>>>> Can you add some comments to the code?
>>>
>>> The function fit_factor pushes up the height of a slur if there is an
extra-encompass-object in the way.  My change makes this function ignore all
extra-encompass-objects close to the extremes of a slur, as these objects have a
tendency to push a slur up disproportionately high.  These objects are still
penalized in the scoring, which results in their avoidance if possible.
>>
>> Thanks.  Could you add this to the source as a comment?
>> This sounds reasonable, however i still have a hard time reading this
>> code because variable names aren't descriptive...  What dz_unit,
>> dz_perp, curve_xext are?  What pext.intersect (curve_xext) does?
>>
>> cheers,
>> Janek
>
> Hey Janek,
>
> I'm certainly not against adding comments, but I think that when the code does
a good job of explaining stuff via variable names, comments can clutter what's
going on.  For example, dz_unit to me sounds like a unit vector (which it is),
dz_perp is the vector perpendicular to this, and curve_xext is the extent of the
curve from attachment point to attachment point.  This could potentially be the
subject of a future GOP

Probably.  For me these names aren't descriptive at all, both in terms
of what they are and what they do (why not dz_perpendicular for
example? and what dz means?), but i think there's no point discussing
this now.

cheers,
Janek

[Graham Percival, 2011-08-07 19:02:09]

On Sun, Aug 07, 2011 at 11:46:51AM +0200, mike@apollinemike.com wrote:
> I'm certainly not against adding comments, but I think that when
> the code does a good job of explaining stuff via variable names,
> comments can clutter what's going on.  For example, dz_unit to
> me sounds like a unit vector (which it is),

To me, dz_unit sounds like the difference in the z_unit vectors.
I mean, "d" is "delta", right?

I personally would have used z_unit_vector unless there was a
clear tradition of using _v for a vector, in which case I'd use
z_unit_v.

... BTW, you *are* using "z" as the "vector going away from the
page", right?  I mean, x_unit would be the unit vector going from
left to right along the page, y_unit would be the unit vector
going from bottom to top along the page, while z_unit is going
away from the page?  (like, to implement "layers" or something
like that?)


If any of the above is incorrect, then maybe consider adding some
comment(s) where you define the variables, to make it clear what
each variable represents?

Cheers,
- Graham

[Keith, 2011-08-08 03:52:52]

LGTM.

I had time to try it on several scores; it often helped and never hurt.

> If any of the above is incorrect, 
> then maybe consider adding some
> comment(s) where you define the variables,

Now, Mike created none of these variables, nor does his added code use them.  If
he adds helpful comments, great, but he has no more duty to add comments than do
the rest of us who reviewed the code.

http://codereview.appspot.com/4810072/diff/7001/input/regression/slur-height-...
File input/regression/slur-height-capping.ly (right):

http://codereview.appspot.com/4810072/diff/7001/input/regression/slur-height-...
input/regression/slur-height-capping.ly:5: towards the edges of the slurs.  said
objects are thus ignored.
"said objects are thus ignored" confuses me.
You should say something to let us easily check if the test passes; maybe "the
two slurs should be similar in shape"

http://codereview.appspot.com/4810072/diff/7001/input/regression/slur-height-...
input/regression/slur-height-capping.ly:10: \relative c' {
Even after the bug-fix, this output looks odd enough that people might waste
time wondering if something is wrong.
I'd drop it; the second test is enough.

http://codereview.appspot.com/4810072/diff/7001/lily/slur-configuration.cc
File lily/slur-configuration.cc (right):

http://codereview.appspot.com/4810072/diff/7001/lily/slur-configuration.cc#ne...
lily/slur-configuration.cc:105: if ((pext.is_empty ()
You might move your code, and 
  if (close_to_edge) continue;
up to line 94, because your test is conceptually independent of the pext
computation.

[Graham Percival, 2011-08-08 03:55:10]

On Mon, Aug 08, 2011 at 03:52:52AM +0000, k-ohara5a5a@oco.net wrote:
> >If any of the above is incorrect,
> >then maybe consider adding some
> >comment(s) where you define the variables,
> 
> Now, Mike created none of these variables, nor does his added code use
> them.  If he adds helpful comments, great, but he has no more duty to
> add comments than do the rest of us who reviewed the code.

Ah, I didn't realize that those were the existing variable names.
I certainly agree that he has no more duty to clarify stuff than
anybody else!

Cheers,
- Graham

[MikeSol, 2011-08-08 06:44:04]

http://codereview.appspot.com/4810072/diff/7001/input/regression/slur-height-...
File input/regression/slur-height-capping.ly (right):

http://codereview.appspot.com/4810072/diff/7001/input/regression/slur-height-...
input/regression/slur-height-capping.ly:5: towards the edges of the slurs.  said
objects are thus ignored.
On 2011/08/08 03:52:52, Keith wrote:
> "said objects are thus ignored" confuses me.
> You should say something to let us easily check if the test passes; maybe "the
> two slurs should be similar in shape"

Done.

http://codereview.appspot.com/4810072/diff/7001/input/regression/slur-height-...
input/regression/slur-height-capping.ly:10: \relative c' {
On 2011/08/08 03:52:52, Keith wrote:
> Even after the bug-fix, this output looks odd enough that people might waste
> time wondering if something is wrong.
> I'd drop it; the second test is enough.

Done.

http://codereview.appspot.com/4810072/diff/7001/lily/slur-configuration.cc
File lily/slur-configuration.cc (right):

http://codereview.appspot.com/4810072/diff/7001/lily/slur-configuration.cc#ne...
lily/slur-configuration.cc:105: if ((pext.is_empty ()
On 2011/08/08 03:52:52, Keith wrote:
> You might move your code, and 
>   if (close_to_edge) continue;
> up to line 94, because your test is conceptually independent of the pext
> computation.

Done.

[Janek Warchol, 2011-08-08 08:07:51]

2011/8/8 Graham Percival <graham@percival-music.ca>:
> On Mon, Aug 08, 2011 at 03:52:52AM +0000, k-ohara5a5a@oco.net wrote:
>> >If any of the above is incorrect,
>> >then maybe consider adding some
>> >comment(s) where you define the variables,
>>
>> Now, Mike created none of these variables, nor does his added code use
>> them.  If he adds helpful comments, great, but he has no more duty to
>> add comments than do the rest of us who reviewed the code.
>
> I certainly agree that he has no more duty to clarify stuff than
> anybody else!

+1, ofc.

cheers,
Janek

[hanwenn, 2011-08-08 13:30:42]

On Sun, Aug 7, 2011 at 4:02 PM, Graham Percival
<graham@percival-music.ca> wrote:
> On Sun, Aug 07, 2011 at 11:46:51AM +0200, mike@apollinemike.com wrote:
>> I'm certainly not against adding comments, but I think that when
>> the code does a good job of explaining stuff via variable names,
>> comments can clutter what's going on.  For example, dz_unit to
>> me sounds like a unit vector (which it is),
>
> To me, dz_unit sounds like the difference in the z_unit vectors.
> I mean, "d" is "delta", right?
>
> I personally would have used z_unit_vector unless there was a
> clear tradition of using _v for a vector, in which case I'd use
> z_unit_v.
>
> ... BTW, you *are* using "z" as the "vector going away from the
> page", right?  I mean, x_unit would be the unit vector going from
> left to right along the page, y_unit would be the unit vector
> going from bottom to top along the page, while z_unit is going
> away from the page?  (like, to implement "layers" or something
> like that?)

Most of the offset code implicitly identifies an offset with a complex
number, and z is commonly used to name a complex number.  The complex
number analogon is probably more confusing than it is useful for
anything.  It could be cleaned up if someone is interested in cleaning
things.

-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

[Neil Puttock, 2011-08-11 20:23:26]

Needs a rebase.

http://codereview.appspot.com/4810072/diff/1003/input/regression/slur-height-...
File input/regression/slur-height-capping.ly (right):

http://codereview.appspot.com/4810072/diff/1003/input/regression/slur-height-...
input/regression/slur-height-capping.ly:4: texidoc = "Slur shaping is not
adapted to accomodate objects
accommodate

http://codereview.appspot.com/4810072/diff/1003/input/regression/slur-height-...
input/regression/slur-height-capping.ly:14: c,( des' as des c, des' as des) |
c,8

[MikeSol, 2011-08-11 23:04:09]

Pushed as 94ea10f3f341fff503599a9eb947c81f1803290f.

Cheers,
MS