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

Issue 5934050: Triggers X-extent calculation for NoteColumn before setting stem-begin-position (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 3 months ago by MikeSol
Modified:
6 years, 7 months ago
Reviewers:
Keith, mike, Trevor Daniels
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Triggers X-extent calculation for NoteColumn before setting stem-begin-position

Patch Set 1 #

Total comments: 1

Patch Set 2 : Triggers NoteColumn spacing before setting attachment. #

Patch Set 3 : Adds regtest #

Patch Set 4 : Gets rid of valgrind stuff #

Patch Set 5 : Goes back to pure property approach #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+39 lines, -4 lines) Patch
A input/regression/flag-stem-begin-position.ly View 1 2 3 4 1 chunk +10 lines, -0 lines 0 comments Download
M lily/flag.cc View 2 3 4 3 chunks +21 lines, -1 line 0 comments Download
M lily/stem.cc View 1 2 3 4 1 chunk +4 lines, -1 line 3 comments Download
M lily/stem-tremolo.cc View 1 2 3 4 1 chunk +3 lines, -1 line 0 comments Download
M scm/define-grobs.scm View 2 3 4 2 chunks +1 line, -1 line 0 comments Download

Messages

Total messages: 7
Keith
LGTM, but if the solution is to avoid looking at the stem extent, then there ...
7 years, 2 months ago (2012-03-29 04:37:37 UTC) #1
MikeSol
There is a nasty bug in LilyPond that this patch only exacerbates - as a ...
7 years, 2 months ago (2012-03-29 06:27:14 UTC) #2
Trevor Daniels
Mike, I don't understand how the commit message and title relate to this change, probably ...
7 years, 2 months ago (2012-03-29 09:25:00 UTC) #3
mike_apollinemike.com
On Mar 29, 2012, at 11:25 AM, tdanielsmusic@googlemail.com wrote: > Mike, I don't understand how ...
7 years, 2 months ago (2012-03-29 09:26:04 UTC) #4
Keith
Works good for me. http://codereview.appspot.com/5934050/diff/7001/lily/stem.cc File lily/stem.cc (right): http://codereview.appspot.com/5934050/diff/7001/lily/stem.cc#newcode131 lily/stem.cc:131: Real stem_beg = internal_calc_stem_begin_position (me, ...
7 years, 2 months ago (2012-04-02 07:42:55 UTC) #5
MikeSol
http://codereview.appspot.com/5934050/diff/7001/lily/stem.cc File lily/stem.cc (right): http://codereview.appspot.com/5934050/diff/7001/lily/stem.cc#newcode131 lily/stem.cc:131: Real stem_beg = internal_calc_stem_begin_position (me, false); On 2012/04/02 07:42:55, ...
7 years, 2 months ago (2012-04-02 07:48:11 UTC) #6
Keith
7 years, 2 months ago (2012-04-04 04:02:47 UTC) #7
http://codereview.appspot.com/5934050/diff/7001/lily/stem.cc
File lily/stem.cc (right):

http://codereview.appspot.com/5934050/diff/7001/lily/stem.cc#newcode131
lily/stem.cc:131: Real stem_beg = internal_calc_stem_begin_position (me, false);
On 2012/04/02 07:48:11, MikeSol wrote:
> On 2012/04/02 07:42:55, Keith wrote:
> > I'm assuming something within calc_stem_begin_position 
> > triggers note-collision resolution 

That was nonsense.  Note-collision resolution happens in the "Preprocessing
graphical objects..." phase, which completes for all three scores in your
regtest before the "Fitting music on 1 page..." phase, which is when we pass the
code with the comment //trigger note collision 

> Yup.  It allows calc_stem_position to skip all of the beam business and get
to
> the setting of note positions.

You must mean something like "get to the business of /looking up/ stem
attachment points" that were set long ago.

Could you change the comment and commit message to whatever you thought I meant
when you said "Yup", because what I thought it meant isn't true.
Sign in to reply to this message.

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