Better pure heights for slurs

Better pure heights for slurs

[MikeSol, 2011-11-24 09:39:53]

Hey all,

I'm not sure if this patch will make a big difference, but it should help the
pure height problem a little bit.  Before, pure heights were calculated using
the entire height of the slur.  For slurs with a slope, this is an overshoot.  I
thus use an estimate of the Y portion of this height by calculating a dummy
slope and then tacking on the appropriate Y value.  In certain cases it could
lead to an underestimation of the pure height, but in general it should still
lead to an overestimation that overshoots less than the original function.

If someone could run a lilypond file with tons of slurs and see if this makes a
difference, I'd be much obliged.

Cheers,
MS

[pkx166h, 2011-11-24 17:39:32]

Passes make and reg test diff here

http://lilypond-stuff.1065243.n5.nabble.com/Tracker-issue-2051-24-November-td...

James

[Keith, 2011-11-24 19:16:46]

I like the direction of this change.

The code has your trademark incomprehensibility, so I don't know if it does what
you want based on your introduction.  Maybe just decide that you want what it
does.

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.

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;

[mike_apollinemike.com, 2011-11-25 05:01:39]

On Thu, 24 Nov 2011 19:16:46 +0000, k-ohara5a5a@oco.net 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

[MikeSol, 2011-11-25 06:10:50]

I've uploaded a new patch set where I've eliminated the 0.5 * height_limit. 
This will likely make slur pure heights worse (meaning they'll overshoot more),
but I cannot find a good rationale for why the 0.5 was there in the first place
so I took it out.

[wl_gnu.org, 2011-11-25 06:55:36]

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

To further improve your skills, I suggest to read Schwitter's
`Ursonate' which starts like this:

  Fümms bö wö tää zää Uu, pögiff, kwii Ee.

You might listen to some excerpts:

  http://www.youtube.com/watch?v=6X7E2i0KMqM&feature=related

And no, this is not German. :-)


     Werner

[pkx166h, 2011-11-25 08:48:43]

passes make - reg test diffs attached here:

http://code.google.com/p/lilypond/issues/detail?id=2051#c4

James

[pkx166h, 2011-11-25 21:05:25]

Passes Make and reg tests are here

http://code.google.com/p/lilypond/issues/detail?id=2051#c6

james

[Keith, 2011-11-26 01:42:33]

On 2011/11/25 05:01:39, mike_apollinemike.com wrote:
> On Thu, 24 Nov 2011 19:16:46 +0000, mailto:k-ohara5a5a@oco.net wrote:
> > I like the direction of this change.
> 
> Does it actually work?

It used to.  You should try your patches, occasionally.
Take Carl's example from 
<http://lists.gnu.org/archive/html/lilypond-devel/2011-11/msg00426.html>
and replace his debugging \paper block with
 \paper{ annotate-spacing = ##t ragged-bottom = ##t}

The good versions space this nicely on 4 pages.  (Patch set 3 uses 35 pages.)

http://codereview.appspot.com/5431065/diff/5003/lily/slur.cc
File lily/slur.cc (right):

http://codereview.appspot.com/5431065/diff/5003/lily/slur.cc#newcode101
lily/slur.cc:101: ret[downup] = minmax (downup, d[dir], ret[dir]);
?? ret.add_point(d[dir]);

http://codereview.appspot.com/5431065/diff/5003/lily/slur.cc#newcode106
lily/slur.cc:106: extremal_heights[RIGHT] = d[dir];
extremal_heights[LEFT (Right)] holds the position of the slur-side end of the
first (respectively, last) nonempty encompassed grob we found.

http://codereview.appspot.com/5431065/diff/5003/lily/slur.cc#newcode113
lily/slur.cc:113: // we dampen the height approximation by the slur's likely
slope
But why bother?  height_approximation is typically 0.3 staff-space, and always
vertical even if the slur slopes?

http://codereview.appspot.com/5431065/diff/5003/lily/slur.cc#newcode119
lily/slur.cc:119: 0.5 staff spaces from the note-head.
Should this 0.5 have been free-height-distance all along?

[Keith, 2011-11-26 02:08:07]

On 2011/11/26 01:42:33, Keith wrote:
> On 2011/11/25 05:01:39, http://mike_apollinemike.com wrote:
> > 
> > Does it actually work?
> 
> It used to.  

I take that back.  The override you emailed a while back setting pure-height = 0
 worked great, but none of these patches help much.

The patch based on my comments failed miserably because I forgot to cast the
unsigned vsize(no_elts) to (int).  I'll run `make check
Maybe the *0.5 was converting

[Keith, 2011-11-26 02:09:57]

On 2011/11/26 01:42:33, Keith wrote:
> On 2011/11/25 05:01:39, http://mike_apollinemike.com wrote:
> > 
> > Does it actually work?
> 
> It used to.  

I take that back.  The override you emailed a while back setting pure-height = 0
 worked great, but none of these patches help much.

The patch based on my comments failed miserably because I forgot to cast the
unsigned vsize(no_elts) to (int).  

Maybe the *0.5 was converting half-staff-spaces to staff-spaces

[Keith, 2011-11-26 03:11:25]

On 2011/11/26 02:09:57, Keith wrote:
> Maybe the *0.5 was converting half-staff-spaces to staff-spaces
No; the units are fine.

The problem seems to be not so much the overestimate of the slur extent, but
that said overestimate of space is allowed before each each script with
avoid-slur 'outside :

\paper{ annotate-spacing = ##t }
  \layout { \context { \Score
  \override PaperColumn #'stencil = #ly:separation-item::print
 }} #(ly:set-option 'debug-skylines)
{ b2( d')_>_\downbow_3 }

[pkx166h, 2011-11-26 08:38:50]

Keith,

retested.

http://code.google.com/p/lilypond/issues/detail?id=2051#c9

passes make and a new set of reg test diffs are up.

James

[mike_apollinemike.com, 2011-11-26 18:15:42]

On Nov 26, 2011, at 4:11 AM, k-ohara5a5a@oco.net wrote:

> On 2011/11/26 02:09:57, Keith wrote:
>> Maybe the *0.5 was converting half-staff-spaces to staff-spaces
> No; the units are fine.
> 
> The problem seems to be not so much the overestimate of the slur extent,
> but that said overestimate of space is allowed before each each script
> with avoid-slur 'outside :
> 
> \paper{ annotate-spacing = ##t }
>  \layout { \context { \Score
>  \override PaperColumn #'stencil = #ly:separation-item::print
> }} #(ly:set-option 'debug-skylines)
> { b2( d')_>_\downbow_3 }

Pacifier prints to the command line from side-position-interface.cc are
verifying to me that that's indeed the real problem.  Do you feel like tackling
it?  If not, I'll have some time in a couple weeks.

Cheers,
MS

[Keith, 2011-11-27 22:16:52]

LGTM
Removing the contribution from 'height-limit' to estimated slur heights is very
helpful.  It was never appropriate to add 'height-limit' to the extent of the
slurred notes, because the note or stem furthest in the direction of the slur is
much closer to the curve of the slur than 'height-limit'.  Piano music often has
to increase height-limit, and the formerly ridiculous height estimates made the
page-breaker useless for me.

Estimating the slur extent as an offset from the notes in the direction of the
slur, rather than a widening of the notes extent on both sides, should also help
a bit.

The stuff with slur slopes is fümms bö wö tää zää uu, but seems harmless.

http://codereview.appspot.com/5431065/diff/8002/lily/slur.cc
File lily/slur.cc (right):

http://codereview.appspot.com/5431065/diff/8002/lily/slur.cc#newcode101
lily/slur.cc:101: ret[downup] = minmax (downup, d[dir], ret[downup]);
any different from ret.add_point(d[dir])

http://codereview.appspot.com/5431065/diff/8002/lily/slur.cc#newcode113
lily/slur.cc:113: // we dampen the height approximation by the slur's likely
slope
'height_approximation' now stores free_head_distance_, which is applied
vertically so its effect doesn't depend on slope of the slur.  Its value is very
small by default, and is only a request to the slur scoring routine, so why
bother with it?

http://codereview.appspot.com/5431065/diff/8002/lily/slur.cc#newcode114
lily/slur.cc:114: // steeper slopes get smaller factors
Without a count of the note-columns spanned, this is more of a height than a
slope. 

And larger slopes/heights get larger 'factor's, smaller '(1 - factor)'s

http://codereview.appspot.com/5431065/diff/8002/lily/slur.cc#newcode116
lily/slur.cc:116: ret[dir] += height_approximation * (1 - factor);
dir *

[mike_apollinemike.com, 2011-11-28 07:59:09]

On Nov 27, 2011, at 11:16 PM, k-ohara5a5a@oco.net wrote:

> LGTM
> Removing the contribution from 'height-limit' to estimated slur heights
> is very helpful.  It was never appropriate to add 'height-limit' to the
> extent of the slurred notes, because the note or stem furthest in the
> direction of the slur is much closer to the curve of the slur than
> 'height-limit'.  Piano music often has to increase height-limit, and the
> formerly ridiculous height estimates made the page-breaker useless for
> me.
> 
> Estimating the slur extent as an offset from the notes in the direction
> of the slur, rather than a widening of the notes extent on both sides,
> should also help a bit.
> 
> The stuff with slur slopes is fümms bö wö tää zää uu, but seems
> harmless.
> 
> 

The idea is that if there is no vertical gap between the start and end notes,
then the "height" will be fully vertical (and height_approximation should thus
be multiplied by 1).  If, on the other hand, there is a big gap between the
start and end notes, there will be almost no vertical height (and
heigh_approximation should thus be multiplied by 0).  The reason I use 1 -
arctan (some_stuff) is because it gives a smooth, continuous function that
encapsulates this idea.


> 
> http://codereview.appspot.com/5431065/diff/8002/lily/slur.cc
> File lily/slur.cc (right):
> 
> http://codereview.appspot.com/5431065/diff/8002/lily/slur.cc#newcode101
> lily/slur.cc:101: ret[downup] = minmax (downup, d[dir], ret[downup]);
> any different from ret.add_point(d[dir])
> 

Nope - changed.

> http://codereview.appspot.com/5431065/diff/8002/lily/slur.cc#newcode113
> lily/slur.cc:113: // we dampen the height approximation by the slur's
> likely slope
> 'height_approximation' now stores free_head_distance_, which is applied
> vertically so its effect doesn't depend on slope of the slur.  Its value
> is very small by default, and is only a request to the slur scoring
> routine, so why bother with it?
> 

Mm...I'm not sure.  I know we're talking about pocket change (between 0.0 and
0.3 vertical space) in an approximation that'll only ever determine how the page
breaker does its thing, but if we eliminate it, it could theoretically result in
too-cramped pages.  That said, I've followed your advice and scrapped it in my
most recent patch set, but with a comment that it may need to be added back in.

> http://codereview.appspot.com/5431065/diff/8002/lily/slur.cc#newcode114
> lily/slur.cc:114: // steeper slopes get smaller factors
> Without a count of the note-columns spanned, this is more of a height
> than a slope.
> 
> And larger slopes/heights get larger 'factor's, smaller '(1 - factor)'s
> 
> http://codereview.appspot.com/5431065/diff/8002/lily/slur.cc#newcode116
> lily/slur.cc:116: ret[dir] += height_approximation * (1 - factor);
> dir *
> 
> http://codereview.appspot.com/5431065/

[pkx166h, 2011-11-28 18:01:53]

Passes Make.


Reg tests here
http://lilypond-stuff.1065243.n5.nabble.com/Tracker-2051-28-November-Test-res...

James