Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1225)

Issue 554030043: Issue 4182: avoid checking the offset of cross-staff stems too early

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months, 1 week ago by barrykp
Modified:
3 months ago
Reviewers:
Dan Eble, lemzwerg
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Issue 4182: avoid checking the offset of cross-staff stems too early Calculating the 'X-offset of a cross-staff Stem too early causes the Stem's 'direction property to be accessed while it is still "calculation-in-progress" - always returning "UP". This causes the X-offset to be set incorrectly when the eventual stem direction will be "DOWN". This problem occurs in the scenario where the X-offset of a notehead is calculated early, when NoteHead::stem_x_shift calls Stem::calc_positioning_done (via get_property). Update NoteHead::stem_x_shift to only check a stem's "positioning-done" property if it is not cross-staff. This will prevent the above scenario from triggering the problem. The Stem's offset will be calculated again later anyway, and this does not change the return value of NoteHead::stem_x_shift as it always returns 0. Add a suitable regression test.

Patch Set 1 #

Total comments: 4

Patch Set 2 : Fix texidoc style issues in cross-staff-stem-offset.ly #

Total comments: 2

Patch Set 3 : Fix style issues #

Unified diffs Side-by-side diffs Delta from patch set Stats (+23 lines, -1 line) Patch
A input/regression/cross-staff-stem-offset.ly View 1 1 chunk +22 lines, -0 lines 0 comments Download
M lily/note-head.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 8
lemzwerg
LGTM https://codereview.appspot.com/554030043/diff/582040043/input/regression/cross-staff-stem-offset.ly File input/regression/cross-staff-stem-offset.ly (right): https://codereview.appspot.com/554030043/diff/582040043/input/regression/cross-staff-stem-offset.ly#newcode5 input/regression/cross-staff-stem-offset.ly:5: Cross-staff stems should not have their @code{'X-offset} calculated ...
3 months, 1 week ago (2020-05-07 20:37:38 UTC) #1
barrykp
Fix texidoc style issues in cross-staff-stem-offset.ly
3 months, 1 week ago (2020-05-08 07:25:50 UTC) #2
barrykp
Thank you for looking at this! Issues should be fixed. https://codereview.appspot.com/554030043/diff/582040043/input/regression/cross-staff-stem-offset.ly File input/regression/cross-staff-stem-offset.ly (right): https://codereview.appspot.com/554030043/diff/582040043/input/regression/cross-staff-stem-offset.ly#newcode5 ...
3 months, 1 week ago (2020-05-08 07:30:26 UTC) #3
Dan Eble
https://codereview.appspot.com/554030043/diff/583870043/lily/note-head.cc File lily/note-head.cc (right): https://codereview.appspot.com/554030043/diff/583870043/lily/note-head.cc#newcode114 lily/note-head.cc:114: if (stem and ! ly_scm2bool (get_property (stem, "cross-staff"))) Please ...
3 months ago (2020-05-09 13:43:44 UTC) #4
barrykp
Fix style issues
3 months ago (2020-05-10 08:06:05 UTC) #5
barrykp
Thank you for looking at this! https://codereview.appspot.com/554030043/diff/583870043/lily/note-head.cc File lily/note-head.cc (right): https://codereview.appspot.com/554030043/diff/583870043/lily/note-head.cc#newcode114 lily/note-head.cc:114: if (stem and ...
3 months ago (2020-05-10 08:07:28 UTC) #6
Dan Eble
This is an area of LilyPond that I don't understand very well, so feel free ...
3 months ago (2020-05-10 15:49:43 UTC) #7
barrykp
3 months ago (2020-05-10 17:38:35 UTC) #8
On 2020/05/10 15:49:43, Dan Eble wrote:
> This is an area of LilyPond that I don't understand very well, so feel free to
> ignore this question if the answer should be obvious.  This patch removes a
call
> to get_property (stem, "positioning-done") for cross-staff stems, but doesn't
> add an alternative anywhere.  Is something else already calling it at an
> appropriate time for cross-staff stems?  Speaking from general programming
> experience and looking at the change narrowly, I wonder whether a state
machine
> is now left dangling in a state it shouldn't be.

I'd be lying if I said this was an area that I understand very well either. I
did
try to research the code that this modifies. It's from 2005 (except for one
recent
syntax change). The commit message doesn't make any mention of it (it's a big
commit). Surrounding commits have no log messages (pre-git days I guess).

Comments in lily/stem.cc ("WARNING: triggers direction") suggest that checking
the
direction of a Stem is sometimes not desirable. Quite why the X-offset of a
NoteHead runs a callback for the Stem before returning
0 isn't clear. My gut feeling is it could be removed altogether with no ill
effects.

The chain is:
NoteHead:stem_x_shift
-> Stem::calc_positioning_done
   -> get_grob_direction
You can simulate the complete removal of this today in any score by explicitly
setting the X-offset property of NoteHeads to 0. This prevents
Stem::calc_positioning_done from being called (I confirmed this with gdb).

The property will be checked/set by some other mechanism, but not via
calc_positioning_done. A few stack traces in testing suggest that the Stem
offset
will be calculated when whatever axis group contains it calculates its own
extents.

So the right thing to do might be to set NoteHead.X-offset to 0 and remove the
stem_x_shift callback altogether. This small patch just aims to fix the critical
regression, so I didn't test that.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b