Fixes heights and pure heights of stems.

Fixes heights and pure heights of stems.

[MikeSol, 2011-08-14 16:25:28]

Hey all,

This patch fixes a bug in LilyPond that has nagged me for some time: incorrect
heights and pure heights for stems.

THE GOOD:

With this patch, heights for stems are correct (they weren't before) and pure
heights are better approximations.  There is one new regtest that shows it in
action, and the code works favorably in several other regtests.  To wit:

beam-collision-beamcount.ly : leads to the lesser of two evils in a beam
collision
spacing-correction-accidentals.ly: actually works now (before the spacing was
off)
tablature-tremolo.ly: the heights between systems corresponds to the actual
system skyline (lengths of stems in this context used to be set to zero)

Also, the Stem grob's property list is cleaned up.  It looses three confusing
properties: length, stem-begin-position, and stem-end-position.  It also leads
to cleaner overrides: if the user wants to do some work with stem ends but does
not want to trigger beam calculations, she can work with the pure callback. 
Idem for stem beginnings.

To see this patch do its thing, add:

\override Stem #'stencil =
#(lambda (grob)
  (let ((y-ext (ly:stem::pure-height grob 0 1000))
         (stem (ly:stem::print grob)))
  (if (null? stem) (ly:make-stencil '() '(0 . 0) '(0 . 0)) (ly:stencil-add stem
    (stencil-with-color (ly:stencil-translate-axis (ly:stencil-translate-axis
(make-line-stencil 0.2 0 (car y-ext) 0 (cdr y-ext)) 0 Y) -0.3 X) red)))))

or

\override Stem #'stencil =
#(lambda (grob)
  (let ((y-ext (ly:stem::height grob))
         (stem (ly:stem::print grob)))
  (if (null? stem) (ly:make-stencil '() '(0 . 0) '(0 . 0)) (ly:stencil-add stem
    (stencil-with-color (ly:stencil-translate-axis (ly:stencil-translate-axis
(make-line-stencil 0.2 0 (car y-ext) 0 (cdr y-ext)) 0 Y) -0.3 X) red)))))

depending on if you want to see the height or pure height.  You'll see that the
results of this patch compared to current master are much better.

THE BAD:

The pure height calculation for beamed stems is more computationally intensive,
which increases compile time for pieces with lots of beams, especially if those
beams hold lots of stems.  A separate patch to cache pure heights would speed
this (and many other things) up a great deal.
At a certain point, beam-slope-stemlet.ly gave me periodic errors of infinite
stencil offset for the tuplet bracket.  It no longer does this, but I'm not sure
if this is because I actually fixed it or because I'm lucky.  The problem
originally came from the change in tuplet-brackets.cc.

THE UGLY:

Because this patch effects stem extents across the board, the regtest
comparisons are nightmarish to check.  The layout probably does not change at
all in most regtests (at least not to the naked eye), but because of the change
in Y-extent, almost every regtest with stems comes back as having changed.  So,
spotting regressions is very difficult.  The files I list above are the only
ones where I see a noteworthy change, and I believe all of these are for the
better.

PARTING SHOT:

This is a labor of love that will pave the way for non-buggy glissando stems in
LilyPond as well as (probably) no more stem-accidental collisions in extreme
beaming situations.  It may also fix other collision-related issues in the
tracker (I'll have to do some snooping).  The existing code logic does not
change at all save the stem pure_height function.  stem.cc is, however,
significantly rearranged in order to consolidate properties and standardize
height/pure-height approximations.

Thanks for your time looking at this, and I'm looking forward to any and all
comments!

Cheers,
MS

[Bertrand Bordage, 2011-08-14 19:22:21]

Benchmark done on a book with many beams:
without the patch: 51.8s and 80 pages
with the patch: 52.2s and 83 pages

I also noticed some ugliness's between beams and figured bass. The figures stay
exactly where they were, but the stems are longer and collide a bit with them.

Bertrand

[Bertrand Bordage, 2011-08-14 20:16:41]

Same remark for fingerings.
See input/regression/fingering-cross-staff.ly

Consecutive ties look better in input/regression/tie-single.ly

[Trevor Daniels, 2011-08-14 20:33:32]

Wow, this is a major rewrite!

I can't comment on the logic, and at present I can't test it, so I've just made
a few nit-picking comments on style.

Trevor

http://codereview.appspot.com/4898044/diff/1/input/regression/stem-length-est...
File input/regression/stem-length-estimation.ly (right):

http://codereview.appspot.com/4898044/diff/1/input/regression/stem-length-est...
input/regression/stem-length-estimation.ly:4: texidoc = "Stems with overridden
'stem-end-position should
'Y-extent

http://codereview.appspot.com/4898044/diff/1/lily/include/stem.hh
File lily/include/stem.hh (right):

http://codereview.appspot.com/4898044/diff/1/lily/include/stem.hh#newcode55
lily/include/stem.hh:55: static Grob* get_reference_head (Grob *);
Grob *get_reference_head

http://codereview.appspot.com/4898044/diff/1/lily/stem-tremolo.cc
File lily/stem-tremolo.cc (right):

http://codereview.appspot.com/4898044/diff/1/lily/stem-tremolo.cc#newcode219
lily/stem-tremolo.cc:219: Real ss = Staff_symbol_referencer::staff_space (me);
delete

http://codereview.appspot.com/4898044/diff/1/lily/stem.cc
File lily/stem.cc (right):

http://codereview.appspot.com/4898044/diff/1/lily/stem.cc#newcode34
lily/stem.cc:34: The only exception to this is ::pure_height, whcih calls
internal_pure_height
which

http://codereview.appspot.com/4898044/diff/1/lily/stem.cc#newcode141
lily/stem.cc:141: if (stemlet && !lh && beam)
All three conditions include !lh; better test !lh outside

http://codereview.appspot.com/4898044/diff/1/lily/stem.cc#newcode149
lily/stem.cc:149: + beam_translation * max (0, (beam_count - 1))
indent

[Neil Puttock, 2011-08-14 23:03:49]

Hi Mike,

This looks like a work in progress.

What's going on with the begin/end position methods?  You've removed the grob
properties, but kept the exported callbacks plus an entry in
pure-conversions-alist which does nothing.

Cheers,
Neil

http://codereview.appspot.com/4898044/diff/1/scm/flag-styles.scm
File scm/flag-styles.scm (right):

http://codereview.appspot.com/4898044/diff/1/scm/flag-styles.scm#newcode188
scm/flag-styles.scm:188: ((if (eqv? d DOWN)
index-cell

[MikeSol, 2011-08-15 12:31:17]

Hey all,

Thanks for your comments!  I'm outta town now, so I won't be able to fully reply
till tomorrow, but I'm working on everything you've suggested with the code and
regtests.

Also, just a quick reply to let you know that the calc_stem_end and
calc_stem_begin methods are left in the code base for people who want to
override the Y-extent of the stem while conserving either the beginning or end
of the stem, ie:

\override Stem #'Y-extent = #(lambda (grob) (let ((foo
(ly:stem::calc-stem-begin-position grob))) (cons foo (+ foo 10))))

Cheers,
MS

[MikeSol, 2011-08-15 14:06:21]

Hey all,

I've addressed all the concerns sent in so far in a new patch set.

Cheers,
MS

[Neil Puttock, 2011-08-15 21:33:07]

On 15 August 2011 13:31,  <mtsolo@gmail.com> wrote:

> Also, just a quick reply to let you know that the calc_stem_end and
> calc_stem_begin methods are left in the code base for people who want to
> override the Y-extent of the stem while conserving either the beginning
> or end of the stem, ie:
>
> \override Stem #'Y-extent = #(lambda (grob) (let ((foo
> (ly:stem::calc-stem-begin-position grob))) (cons foo (+ foo 10))))

That's every users who wants cross-staff stems for chords.  Unless you
can come up with a better interface for dealing with cross-staff
stems, I'd rather you keep 'length for this case.

BTW, I had a head-scratching moment with the above override until I
realized that the begin/end-position callbacks return half-staff-space
values.

Cheers,
Neil

[hanwenn, 2011-08-15 21:48:57]

On Sun, Aug 14, 2011 at 1:25 PM,  <mtsolo@gmail.com> wrote:

> THE UGLY:
>
> Because this patch effects stem extents across the board, the regtest
> comparisons are nightmarish to check.  The layout probably does not
> change at all in most regtests (at least not to the naked eye), but
> because of the change in Y-extent, almost every regtest with stems comes
> back as having changed.  So, spotting regressions is very difficult.
> The files I list above are the only ones where I see a noteworthy
> change, and I believe all of these are for the better.

Someone on the team is running pixel by pixel comparisons, which will
correctly show differences.  You can also hack up the
output-distance.py script to ignore all Stem grobs (The grob name is
in the .signature file, so it should be pretty trivial).

[mikesol_ufl.edu, 2011-08-15 21:51:20]

On Aug 15, 2011, at 11:33 PM, Neil Puttock wrote:

> On 15 August 2011 13:31,  <mtsolo@gmail.com> wrote:
> 
>> Also, just a quick reply to let you know that the calc_stem_end and
>> calc_stem_begin methods are left in the code base for people who want to
>> override the Y-extent of the stem while conserving either the beginning
>> or end of the stem, ie:
>> 
>> \override Stem #'Y-extent = #(lambda (grob) (let ((foo
>> (ly:stem::calc-stem-begin-position grob))) (cons foo (+ foo 10))))
> 
> That's every users who wants cross-staff stems for chords.  Unless you
> can come up with a better interface for dealing with cross-staff
> stems, I'd rather you keep 'length for this case.
> 

How about in output-lib.scm something like:

(define (stem::length grob val)
  (let* ((d (ly:grob-property grob 'direction))
    (half-space (* 0.5 (ly:staff-symbol-staff-space grob)))
    (beg (ly:stem::calc-stem-begin-position grob))
    (ext (if (eqv? d DOWN) (cons (- beg val) beg) (cons beg (+ beg val)))))
 (cons (* (car ext) half-space) (* (cdr ext) half-space))))

Then, for the user,
\override Stem #'Y-extent = #(stem::length 20)

(note that the above is not tested...I am too tired to figure out how half-space
needs to function here (division or multiplication), but I will tomorrow)

+ a convert-ly rule.

Do you think that would suffice?

It should also be possible to connect cross staff stems with code that's not
unlike that which connects cross staff arpeggios (I haven't tried it yet, but I
don't see why it wouldn't work...).

> BTW, I had a head-scratching moment with the above override until I
> realized that the begin/end-position callbacks return half-staff-space
> values.
> 

Sorry - forgot about that!  The functions should, in theory, return the values
that the originals did.

Cheers,
MS

[hanwenn, 2011-08-15 21:57:43]

overall comment: since you're hijacking the Y-extent property for storing the
logical end of the stem, how will you deal with stem flags that are oddly
shaped?  I think it is legit for a flag to be larger than the end point of the
stem.

consider a hypothetical flag 


   
  /
 |/
x|

(hope you get the idea.)

other than that, there seem to be lots of small errors with dimensions vs.
posititions.

http://codereview.appspot.com/4898044/diff/8002/lily/dot-column.cc
File lily/dot-column.cc (right):

http://codereview.appspot.com/4898044/diff/8002/lily/dot-column.cc#newcode140
lily/dot-column.cc:140: + stem->extent (stem, Y_AXIS)[get_grob_direction
(stem)];
this looks like a dimension error. what if staffspace != 1.0?

http://codereview.appspot.com/4898044/diff/8002/lily/note-spacing.cc
File lily/note-spacing.cc (right):

http://codereview.appspot.com/4898044/diff/8002/lily/note-spacing.cc#newcode276
lily/note-spacing.cc:276: stem_posns[d] = stem->pure_height (stem, 0, INT_MAX);
again, height is in ss, while posns are relative to 0.5*ss

http://codereview.appspot.com/4898044/diff/8002/lily/stem.cc
File lily/stem.cc (right):

http://codereview.appspot.com/4898044/diff/8002/lily/stem.cc#newcode160
lily/stem.cc:160: me->set_property ("Y-extent", ly_interval2scm (height));
why don't you a callback directly on Y-extent?

[MikeSol, 2011-08-16 07:38:24]

I see what you mean about flags, and I think there are two ways to go with it.

1)  Reinstate stem-begin-position and stem-end-position in this patch (trivial:
would take 10 minutes).

2)  Create a Flag grob (less trivial, I'd say two hours of work to create the
grob in define-grobs.scm, get it made in a new finalize method for the stem
engraver, copy and paste all of the flag stuff in stem.cc into a new flag.cc,
create an X-offset and Y-offset callback for flags, and rewrite any .ly material
that makes references to flags - the docs is another story).

I like #2 for the same reason that I'd be against conflating stem tremolos into
stems.  Flags, like stem tremolos, are unique entities that are appended to
stems in certain conditions.

Cheers,
MS

http://codereview.appspot.com/4898044/diff/8002/lily/dot-column.cc
File lily/dot-column.cc (right):

http://codereview.appspot.com/4898044/diff/8002/lily/dot-column.cc#newcode140
lily/dot-column.cc:140: + stem->extent (stem, Y_AXIS)[get_grob_direction
(stem)];
On 2011/08/15 21:57:43, hanwenn wrote:
> this looks like a dimension error. what if staffspace != 1.0?

You're right - the multiplier needs to go around both elements.  Fixed in a new
patchset.

http://codereview.appspot.com/4898044/diff/8002/lily/note-spacing.cc
File lily/note-spacing.cc (right):

http://codereview.appspot.com/4898044/diff/8002/lily/note-spacing.cc#newcode276
lily/note-spacing.cc:276: stem_posns[d] = stem->pure_height (stem, 0, INT_MAX);
On 2011/08/15 21:57:43, hanwenn wrote:
> again, height is in ss, while posns are relative to 0.5*ss

Thanks for spotting this - fixed.

http://codereview.appspot.com/4898044/diff/8002/lily/stem.cc
File lily/stem.cc (right):

http://codereview.appspot.com/4898044/diff/8002/lily/stem.cc#newcode160
lily/stem.cc:160: me->set_property ("Y-extent", ly_interval2scm (height));
On 2011/08/15 21:57:43, hanwenn wrote:
> why don't you a callback directly on Y-extent?  

I think that a call to set_property needs to be done here irrespective of the
implementation (the same was true of the old function, it was just a different
property (stem-end-position)).

[mail_philholmes.net, 2011-08-16 08:16:22]

----- Original Message ----- 
From: "Han-Wen Nienhuys" <hanwenn@gmail.com>
To: <mtsolo@gmail.com>; <lilypond-devel@gnu.org>; 
<reply@codereview.appspotmail.com>
Sent: Monday, August 15, 2011 10:48 PM
Subject: Re: Fixes heights and pure heights of stems. (issue 4898044)


> On Sun, Aug 14, 2011 at 1:25 PM,  <mtsolo@gmail.com> wrote:
>
>> THE UGLY:
>>
>> Because this patch effects stem extents across the board, the regtest
>> comparisons are nightmarish to check. The layout probably does not
>> change at all in most regtests (at least not to the naked eye), but
>> because of the change in Y-extent, almost every regtest with stems comes
>> back as having changed. So, spotting regressions is very difficult.
>> The files I list above are the only ones where I see a noteworthy
>> change, and I believe all of these are for the better.
>
> Someone on the team is running pixel by pixel comparisons, which will
> correctly show differences.  You can also hack up the
> output-distance.py script to ignore all Stem grobs (The grob name is
> in the .signature file, so it should be pretty trivial).


Puts hand up.  However, I run this on Windows and so would need a Windows 
.exe to do the comparison.

--
Phil Holmes

[MikeSol, 2011-08-16 09:24:10]

On 2011/08/15 21:33:07, Neil Puttock wrote:
> On 15 August 2011 13:31,  <mailto:mtsolo@gmail.com> wrote:
> 

> That's every users who wants cross-staff stems for chords.  Unless you
> can come up with a better interface for dealing with cross-staff
> stems, I'd rather you keep 'length for this case.
> 

I've posted a new patch with a length function and a regtest showing its
functionality.

Cheers,
MS

[hanwenn, 2011-08-16 10:51:44]

On Tue, Aug 16, 2011 at 4:38 AM,  <mtsolo@gmail.com> wrote:
> 2)  Create a Flag grob (less trivial, I'd say two hours of work to
> create the grob in define-grobs.scm, get it made in a new finalize
> method for the stem engraver, copy and paste all of the flag stuff in
> stem.cc into a new flag.cc, create an X-offset and Y-offset callback for
> flags, and rewrite any .ly material that makes references to flags - the
> docs is another story).
>
> I like #2 for the same reason that I'd be against conflating stem
> tremolos into stems.  Flags, like stem tremolos, are unique entities
> that are appended to stems in certain conditions.


#2) sounds neat, but maybe Janek (who has spent some time messing
around with flags) wants to weigh in.

If it is a viable path, you should probably do it before applying this patch.

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

[janek, 2011-08-23 22:09:21]

2011/8/16 Han-Wen Nienhuys <hanwenn@gmail.com>:
> #2) sounds neat, but maybe Janek (who has spent some time messing
> around with flags) wants to weigh in.

As i've said in a private mail to Mike, i don't have anything against doing so.

Mike, i understand that your patch changes some beams.  I'd like to
check what effect does it have on my scores, but i'm not sure if i can
do it now or rather should i wait until flag grob is pushed.  I also
don't understand what '\override Stem #'stencil = #(lambda (grob) ...'
is about.

cheers,
Janek

[mikesol_ufl.edu, 2011-08-24 06:45:59]

On Aug 24, 2011, at 12:09 AM, Janek Warchoł wrote:

> 2011/8/16 Han-Wen Nienhuys <hanwenn@gmail.com>:
>> #2) sounds neat, but maybe Janek (who has spent some time messing
>> around with flags) wants to weigh in.
> 
> As i've said in a private mail to Mike, i don't have anything against doing
so.
> 
> Mike, i understand that your patch changes some beams.  I'd like to
> check what effect does it have on my scores, but i'm not sure if i can
> do it now or rather should i wait until flag grob is pushed.

You can check it now - the flag grob won't really have an effect, and will break
the patch until I upload a new set.

> I also
> don't understand what '\override Stem #'stencil = #(lambda (grob) ...'
> is about.

Where was this lambda function talked about?

Cheers,
MS

[janek, 2011-08-24 11:46:28]

2011/8/24 Mike Solomon <mikesol@ufl.edu>:
> On Aug 24, 2011, at 12:09 AM, Janek Warchoł wrote:
>
>> 2011/8/16 Han-Wen Nienhuys <hanwenn@gmail.com>:
>>> #2) sounds neat, but maybe Janek (who has spent some time messing
>>> around with flags) wants to weigh in.
>>
>> As i've said in a private mail to Mike, i don't have anything against doing
so.
>>
>> Mike, i understand that your patch changes some beams.  I'd like to
>> check what effect does it have on my scores, but i'm not sure if i can
>> do it now or rather should i wait until flag grob is pushed.
>
> You can check it now - the flag grob won't really have an effect, and will
break the patch until I upload a new set.
>
>> I also
>> don't understand what '\override Stem #'stencil = #(lambda (grob) ...'
>> is about.
>
> Where was this lambda function talked about?

In your first mail,
http://lists.gnu.org/archive/html/lilypond-devel/2011-08/msg00572.html

cheers,
Janek

[mikesol_ufl.edu, 2011-08-24 11:48:40]

On Aug 24, 2011, at 1:46 PM, Janek Warchoł wrote:

> 2011/8/24 Mike Solomon <mikesol@ufl.edu>:
>> On Aug 24, 2011, at 12:09 AM, Janek Warchoł wrote:
>> 
>>> 2011/8/16 Han-Wen Nienhuys <hanwenn@gmail.com>:
>>>> #2) sounds neat, but maybe Janek (who has spent some time messing
>>>> around with flags) wants to weigh in.
>>> 
>>> As i've said in a private mail to Mike, i don't have anything against doing
so.
>>> 
>>> Mike, i understand that your patch changes some beams.  I'd like to
>>> check what effect does it have on my scores, but i'm not sure if i can
>>> do it now or rather should i wait until flag grob is pushed.
>> 
>> You can check it now - the flag grob won't really have an effect, and will
break the patch until I upload a new set.
>> 
>>> I also
>>> don't understand what '\override Stem #'stencil = #(lambda (grob) ...'
>>> is about.
>> 
>> Where was this lambda function talked about?
> 
> In your first mail,
> http://lists.gnu.org/archive/html/lilypond-devel/2011-08/msg00572.html
> 
> cheers,
> Janek
> 

Ah, I see.
These lambda functions allow you to see heights and/or pure heights next to the
stems.  Throw them into a regtest with lots of stems (beamed and unbeamed), run
it with current master, and then run it with my patch.  You'll see that the pure
height approximations and the heights are more accurately represented w/ this
patch (at least I think they are...I haven't yet heard any feedback to the
contrary).

Cheers,
MS

[MikeSol, 2011-08-27 16:41:38]

Pushed as aaacb8cdd5bc029a8d0c87f90b817d97fcd5ad80.  Thanks to everyone for
their help!

Cheers,
MS

[Keith, 2012-03-27 04:47:02]

On 2011/08/14 16:25:28, MikeSol wrote:
> Hey all,
> 
> This patch fixes a bug in LilyPond that has nagged me for some time: incorrect
> heights and pure heights for stems.

> Also, the Stem grob's property list is cleaned up.  It looses three confusing
> properties: length, stem-begin-position, and stem-end-position.  It also leads
> to cleaner overrides: if the user wants to do some work with stem ends but
does
> not want to trigger beam calculations, she can work with the pure callback. 
> Idem for stem beginnings.
> 

Mike,
  The code in lily/note-collision.cc:219 needs an update to adjust the stem
beginning (issue 2441).  I tried but I can't find the "pure callback for stem
beginnings".

[mikesol_ufl.edu, 2012-03-27 06:14:22]

> Mike,
>  The code in lily/note-collision.cc:219 needs an update to adjust the
> stem beginning (issue 2441).  I tried but I can't find the "pure
> callback for stem beginnings".
> 

ly:stem::pure-calc-stem-begin-position

Lemme know if I can be of help w/ fixing this!

Cheers,
MS