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

## Issue 4961041: Sketch for broken beams with consistent slopes (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years ago by MikeSol
Modified:
7 years, 10 months ago
Reviewers:
Keith, Neil Puttock, mikesol, joeneeman, pkx166h, hanwenn
CC:
lilypond-devel_gnu.org
Visibility:
Public.

### Description

Sketch for broken beams with consistent slopes

### Patch Set 16 : Gets x_span_ right. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+380 lines, -444 lines) Patch
M Documentation/changes.tely View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +19 lines, -0 lines 4 comments Download
A input/regression/beam-consistent-broken-slope.ly View 1 2 3 4 5 6 1 chunk +31 lines, -0 lines 3 comments Download
M lily/beam.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +4 lines, -3 lines 0 comments Download
M lily/beam-quanting.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 chunks +288 lines, -358 lines 4 comments Download
M lily/include/beam.hh View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +1 line, -5 lines 0 comments Download
M lily/include/beam-scoring-problem.hh View 1 2 3 4 5 6 7 8 9 10 4 chunks +17 lines, -7 lines 0 comments Download
M ly/music-functions-init.ly View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -12 lines 0 comments Download
M scm/define-grob-properties.scm View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +3 lines, -0 lines 1 comment Download
M scm/define-grobs.scm View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -9 lines 1 comment Download

### Messages

Total messages: 29
MikeSol
 Hey all, This patch changes the name of several variables that I'll need to work ...
8 years ago (2011-08-25 07:29:38 UTC) #1
pkx166h
 passes make and reg tests
8 years ago (2011-08-27 08:49:50 UTC) #2
hanwenn
 LGTM On Thu, Aug 25, 2011 at 4:29 AM, wrote: > Reviewers: , > ...
8 years ago (2011-08-27 14:11:29 UTC) #3
joeneeman
 http://codereview.appspot.com/4961041/diff/3001/lily/include/beam-scoring-problem.hh File lily/include/beam-scoring-problem.hh (right): http://codereview.appspot.com/4961041/diff/3001/lily/include/beam-scoring-problem.hh#newcode114 lily/include/beam-scoring-problem.hh:114: TODO - use trailing _ on data members. I ...
8 years ago (2011-09-01 15:20:17 UTC) #4
MikeSol
 Hey all, Sorry for hijacking this issue to turn it into another one. I'll take ...
8 years ago (2011-09-01 16:13:00 UTC) #5
mikesol_ufl.edu
 On Sep 1, 2011, at 6:13 PM, mtsolo@gmail.com wrote: > Other than that, I believe ...
8 years ago (2011-09-01 16:22:12 UTC) #6
MikeSol
 Update: looks like cross-staff beams are broken because I removed all of the commony code ...
8 years ago (2011-09-01 19:00:12 UTC) #7
MikeSol
 Hey all, This is now in a state where people can run regtests on it ...
8 years ago (2011-09-18 18:19:28 UTC) #8
MikeSol
 All of the previously-reported bugs are now squashed with this newest patchset: the regtests only ...
8 years ago (2011-09-19 09:14:59 UTC) #9
pkx166h
 Passes make but there are a few reg tests that someone should look at See ...
7 years, 12 months ago (2011-09-24 19:57:04 UTC) #10
mikesol_ufl.edu
 On Sep 24, 2011, at 9:57 PM, pkx166h@gmail.com wrote: > Passes make but there are ...
7 years, 12 months ago (2011-09-25 03:44:36 UTC) #11
MikeSol
7 years, 11 months ago (2011-10-03 04:05:09 UTC) #12
hanwenn
 i know it's annoying, but could you separate out the cosmetics (adding _ ) to ...
7 years, 11 months ago (2011-10-03 04:53:48 UTC) #13
pkx166h
 Passes make. I get a few reg tests pop up; see attached at http://code.google.com/p/lilypond/issues/detail?id=1952#c1 but ...
7 years, 11 months ago (2011-10-03 07:20:33 UTC) #14
mikesol_ufl.edu
 On Oct 3, 2011, at 6:53 AM, hanwenn@gmail.com wrote: > i know it's annoying, but ...
7 years, 11 months ago (2011-10-03 18:50:27 UTC) #15
mikesol_ufl.edu
 On Oct 3, 2011, at 6:53 AM, hanwenn@gmail.com wrote: > i know it's annoying, but ...
7 years, 11 months ago (2011-10-03 18:51:01 UTC) #16
mikesol_ufl.edu
 On Oct 3, 2011, at 8:50 PM, Mike Solomon wrote: > On Oct 3, 2011, ...
7 years, 11 months ago (2011-10-04 09:57:55 UTC) #17
hanwenn
 You skipped the cosmetic patch that folds together the (SCM, SCM) callbacks into one big ...
7 years, 11 months ago (2011-10-04 13:26:46 UTC) #18
mikesol_ufl.edu
 On Oct 4, 2011, at 3:26 PM, Han-Wen Nienhuys wrote: > You skipped the cosmetic ...
7 years, 11 months ago (2011-10-04 13:31:01 UTC) #19
mikesol_ufl.edu
 On Oct 4, 2011, at 3:26 PM, Han-Wen Nienhuys wrote: > You skipped the cosmetic ...
7 years, 11 months ago (2011-10-04 14:48:41 UTC) #20
hanwenn
 On Tue, Oct 4, 2011 at 10:30 AM, Mike Solomon wrote: > On Oct ...
7 years, 11 months ago (2011-10-04 17:47:12 UTC) #21
pkx166h
 passes make but I get some reg tests show up. See http://code.google.com/p/lilypond/issues/detail?id=1844#c11
7 years, 11 months ago (2011-10-05 07:21:58 UTC) #22
mikesol_ufl.edu
 On Oct 4, 2011, at 7:47 PM, Han-Wen Nienhuys wrote: > On Tue, Oct 4, ...
7 years, 11 months ago (2011-10-05 08:20:17 UTC) #23
pkx166h
 Passes make, make check gives me the usual suspects on the reg test http://code.google.com/p/lilypond/issues/detail?id=1952#c3 but ...
7 years, 11 months ago (2011-10-06 19:16:44 UTC) #24
Neil Puttock
 http://codereview.appspot.com/4961041/diff/60001/Documentation/changes.tely File Documentation/changes.tely (right): http://codereview.appspot.com/4961041/diff/60001/Documentation/changes.tely#newcode66 Documentation/changes.tely:66: \override Beam #'breakable = ##t no indent http://codereview.appspot.com/4961041/diff/60001/Documentation/changes.tely#newcode68 Documentation/changes.tely:68: ...
7 years, 11 months ago (2011-10-20 19:24:44 UTC) #25
mikesol_ufl.edu
 Thanks Neil! On Oct 20, 2011, at 9:24 PM, n.puttock@gmail.com wrote: > http://codereview.appspot.com/4961041/diff/60001/scm/define-grob-properties.scm > File ...
7 years, 11 months ago (2011-10-21 06:50:18 UTC) #26
Neil Puttock
 On 21 October 2011 07:50, Mike Solomon wrote: > Of this I am not ...
7 years, 11 months ago (2011-10-21 11:20:00 UTC) #27
Keith
 Rather indecipherable. The 350 lines of code re-ordering make it difficult to find the change ...
7 years, 10 months ago (2011-10-26 18:57:38 UTC) #28
MikeSol
 Thanks for the comments! Just a technical note - this issue was closed after the ...
7 years, 10 months ago (2011-10-26 19:18:07 UTC) #29
Thanks for the comments!
Just a technical note - this issue was closed after the patch was pushed to
current master, but I don't mind opening it again to continue a discussion about
what this patch (if Colin has no objections).

The code I'm dealing with in beam-quanting and beam is difficult to decipher to
begin with: it took me at least a year to get my head fully around what it's
doing and to figure out where the gaps were.  I know there is a lot of
reorganization, but I wouldn't do it unless I felt that it was necessary to make
the code perform more consistently (i.e. to standardize the measuring of X
extent).

Cheers,
MS

http://codereview.appspot.com/4961041/diff/60001/lily/beam-quanting.cc
File lily/beam-quanting.cc (right):

http://codereview.appspot.com/4961041/diff/60001/lily/beam-quanting.cc#newcod...
lily/beam-quanting.cc:202: for (vsize i = 0; i < beams.size (); i++)
On 2011/10/26 18:57:38, Keith wrote:
> When there is more than one beam in 'beams' what do you intend the members
> Beam_scoring_problem to represent ?  The beam as if it were on one line, but
> allowing the extra horizontal span due to the line-break ?  Should the members
> like segments_ and is_knee_ include both halves of the beam ?

All of Beam_scoring_problem happens in an artificial space as if there was no
line break (which is why this function is so long - you'll see x_span_ and
x_left being used quite a bit to bring elements to their positions on this
imaginary line).  So, Beam_scoring_problem does not know what line breaking is,
and it is only at the very end that the normalized endpoints are used to take
care of the breaks.

One of the reasons for my most recent patch is that the normalized endpoints of
the beam don't sync up with its actual extent, which is only reliably derived
from the segments.

http://codereview.appspot.com/4961041/diff/60001/lily/beam-quanting.cc#newcod...
lily/beam-quanting.cc:947: final_positions[RIGHT] -= (1 -
normalized_endpoints[RIGHT]) * y_length;
On 2011/10/26 18:57:38, Keith wrote:
> At what horizontal locations are these final_positions? First and last stems
or
> the end of the beam including overhang or x_span_ ?

Including overhang.  This is improved in the most recent patch set.