Re: Better pure heights for slurs (issue 5431065)

[mike]

On Thu, 24 Nov 2011 19:16:46 +0000, address@hidden wrote:
> I like the direction of this change.

Does it actually work?

> The code has your trademark incomprehensibility,

Thank you, Keith.  You know, sometimes, people think that it comes 
naturally to be so incomprehensible, but it's actually hard work.  
Usually, before writing LilyPond code, I'll read a passage from either 
Finnegans Wake or Waiting for Godot and then recite some Gertrude Stein 
poetry.  It helps me get my head in the right place.  Of course, one 
likes to believe that one can always find room for improvement, but I 
actually think that I hit my zenith with one of my first commits via 
path-min-max in stencil.scm.  The US government is working on its 
inverse function in hopes that it will help decrypt Chinese telegrams.

> so I don't know if it
> does what you want based on your introduction.  Maybe just decide 
> that
> you want what it does.

It basically finds a horrendous approximation for the slope of the slur 
and then ret holds the y component of this height.  I could have used 
arctans, but I decided to solve for y using the Pythagorian theorem 
instead.  The only danger is that my estimate of the slope is likely too 
steep in most cases (thus yielding a smaller height than desirable), but 
this is offset by the fact that the height is tacked onto the highest 
point of the slur.

> http://codereview.appspot.com/5431065/diff/1/lily/slur.cc
> File lily/slur.cc (right):
>
> http://codereview.appspot.com/5431065/diff/1/lily/slur.cc#newcode102
> lily/slur.cc:102: height *= 0.5;
> Not too harmful here, but in general redefining variables with
> meaningful names causes other programmers to scan lines 77 through 
> 101
> for use of the original height, and then lines 103 through 118 for 
> the
> new use of height, to try to determine why it changed halfway 
> through.
>
> If the conceptual "height" didn't change, just leave 'height' alone.
>
> Oh, the concept is actually "height-limit". Maybe the original 
> variable
> name wasn't perfect.  Since ret.length() is also an estimate of the
> actual height of the slur, I'd take the time to change the variable
> names.

Doable.

> http://codereview.appspot.com/5431065/diff/1/lily/slur.cc#newcode107
> lily/slur.cc:107: between two columns
> Trop de mots,
> und ein, dass ich nicht in meinem Wörterbuch gefunden haben.
>
> http://codereview.appspot.com/5431065/diff/1/lily/slur.cc#newcode115
> lily/slur.cc:115: ret[dir] += dir * (sqrt (height * height / ((s * s) 
> +
> 1)) + 0.5);
> Reduces to abs(0.5*height_limit) / sqrt(s²+1).
> Simplifying the math, it looks like the result is nearly this:
>  Real encompassing_height = max(1.0, ret.length());
>  // Coarse estimate of how far the slur will bow upwards
>  ret[dir] += dir * no_elts * height_limit / encompassing_height;
>  ret += 0.5 * dir;

Yup - this'll come in slightly larger than the result in the patch, but 
still smaller than the original estimate.  I can use it instead.

Cheers,
MS