Avoid failed assertion in stem tremolo code (issue 572550043 by dak@gnu.org)

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Avoid failed assertion in stem tremolo code (issue 572550043 by dak@gnu.org)

k-ohara5a5a
If I understand, the example
   \relative { a32 8..:32 }
fails the assertion in the call to center() on line 182, because the
interval 'ph' is empty.

There are several problems in the existing code, that would seem to
prevent it from working.  The proposed change deviates further from what
seems to be the intent of the code, so I suggest trapping for an empty
interval explicitly.


https://codereview.appspot.com/572550043/diff/560590043/lily/stem-tremolo.cc
File lily/stem-tremolo.cc (left):

https://codereview.appspot.com/572550043/diff/560590043/lily/stem-tremolo.cc#oldcode156
lily/stem-tremolo.cc:156: Stem_tremolo::pure_height (SCM smob, SCM, SCM)
Stem_tremolo::pure_height (SCM smob, SCM /*start*/, SCM /*end*/)
The duty of this function is to return the vertical extent of the
tremolo, for purposes of figuring the length required to set a line of
music that begins at column 'start' and ends at column 'end'.

https://codereview.appspot.com/572550043/diff/560590043/lily/stem-tremolo.cc#oldcode175
lily/stem-tremolo.cc:175: Interval ph = stem->pure_y_extent (stem, 0,
INT_MAX);
The code clearly assumed that 'ph' would never be empty.
In the given example, it certainly should not be empty, so there is a
problem in stem.cc.

If in future stem->pure_y_extent() is repaired, and validly returns an
empty interval, this function should behave as it does with no stem at
all:
   if (is_empty(ph)) return s1.extent(Y_AXIS);

https://codereview.appspot.com/572550043/diff/560590043/lily/stem-tremolo.cc#oldcode177
lily/stem-tremolo.cc:177: ph[-dir] = si.shortest_y_;
shortest_y is the endpoint of the stem (including space for tremolo)
when it is compressed as short as it can be.  So the operation above
would normally be a shortening of the interval.
Doing instead add_point() would defeat its purpose.

https://codereview.appspot.com/572550043/diff/560590043/lily/stem-tremolo.cc#oldcode182
lily/stem-tremolo.cc:182: ph = ph - ph.center ();
There is the obvious problem that line 182 destroys the effort of line
181.   It reports every stem-tremolo to be centered on the central
staff-line, which makes line-spacing less accurate but could easily go
un-noticed.

https://codereview.appspot.com/572550043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Avoid failed assertion in stem tremolo code (issue 572550043 by dak@gnu.org)

k-ohara5a5a

https://codereview.appspot.com/572550043/diff/560590043/lily/stem-tremolo.cc
File lily/stem-tremolo.cc (left):

https://codereview.appspot.com/572550043/diff/560590043/lily/stem-tremolo.cc#oldcode175
lily/stem-tremolo.cc:175: Interval ph = stem->pure_y_extent (stem, 0,
INT_MAX);
My suggested change to return s1.extent will probably cause
green-highlights in the regression tests, because the highlighting
function compares the numerical /extents/ of the graphical objects.  If
the spacing of the black-printed glyphs remains good, everything should
be fine.

https://codereview.appspot.com/572550043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Avoid failed assertion in stem tremolo code (issue 572550043 by dak@gnu.org)

David Kastrup
In reply to this post by k-ohara5a5a
Thanks for looking at this, Keith!  I've tried to make a version that
does the right thing with regard to the assertion failure.  That does
not address the root cause of the problem, nor does it fix the centering
thing you pointed out.  Quite lacklustre.


https://codereview.appspot.com/572550043/diff/560590043/lily/stem-tremolo.cc
File lily/stem-tremolo.cc (left):

https://codereview.appspot.com/572550043/diff/560590043/lily/stem-tremolo.cc#oldcode182
lily/stem-tremolo.cc:182: ph = ph - ph.center ();
On 2019/04/03 08:16:22, Keith wrote:
> There is the obvious problem that line 182 destroys the effort of line
181.   It
> reports every stem-tremolo to be centered on the central staff-line,
which makes
> line-spacing less accurate but could easily go un-noticed.

This is likely the famous "somebody must have been something"
phenomenon.  Since this does not concern the segfaulting, I am leaving
this for someone else to fix.  Ok, I'll add a TODO.

https://codereview.appspot.com/572550043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel