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

Issue 4237057: Potential fix for issue 1504. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
13 years ago by MikeSol
Modified:
12 years, 11 months ago
Reviewers:
mike, hanwenn
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Potential fix for issue 1504. Changes feathered beaming such that broken feather beams pick up at the same degree of feather-ness as the previous line.

Patch Set 1 #

Total comments: 17

Patch Set 2 : Responses to Han Wen's suggestions #

Total comments: 6

Patch Set 3 : Response to Han Wen's suggestions #

Total comments: 3

Patch Set 4 : Normalizes feather-fraction between 0 and 1 #

Total comments: 8

Patch Set 5 : Responses to Han Wen's suggestions #

Total comments: 10

Patch Set 6 : Responses to Han Wen's suggestions #

Unified diffs Side-by-side diffs Delta from patch set Stats (+250 lines, -10 lines) Patch
A input/regression/beam-feather-breaking.ly View 1 chunk +137 lines, -0 lines 0 comments Download
M lily/beam.cc View 1 2 3 4 5 3 chunks +58 lines, -9 lines 0 comments Download
M lily/include/spanner.hh View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M lily/spanner.cc View 1 2 3 4 5 2 chunks +50 lines, -0 lines 0 comments Download
M scm/define-grob-properties.scm View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M scm/define-grobs.scm View 1 2 3 4 2 chunks +1 line, -1 line 0 comments Download

Messages

Total messages: 22
hanwenn
http://codereview.appspot.com/4237057/diff/1/lily/beam.cc File lily/beam.cc (right): http://codereview.appspot.com/4237057/diff/1/lily/beam.cc#newcode173 lily/beam.cc:173: span_data.push_back (get_beam_span (orig->broken_intos_[i], commonx)); this looks wrong - different ...
13 years ago (2011-03-09 23:03:44 UTC) #1
MikeSol
Thanks for the comments! Below I have a question about commonx - the other fix ...
13 years ago (2011-03-10 08:04:34 UTC) #2
MikeSol
Thanks for the comments! Below I have a question about commonx - the other fix ...
13 years ago (2011-03-10 08:04:35 UTC) #3
hanwenn
On Thu, Mar 10, 2011 at 5:04 AM, <mtsolo@gmail.com> wrote: >> this looks wrong - ...
13 years ago (2011-03-11 13:37:33 UTC) #4
hanwenn
http://codereview.appspot.com/4237057/diff/1/lily/beam.cc File lily/beam.cc (right): http://codereview.appspot.com/4237057/diff/1/lily/beam.cc#newcode620 lily/beam.cc:620: { it would be nice if you could collapse ...
13 years ago (2011-03-11 14:01:29 UTC) #5
MikeSol
Thanks for the comments! New patch uploaded. http://codereview.appspot.com/4237057/diff/1/lily/beam.cc File lily/beam.cc (right): http://codereview.appspot.com/4237057/diff/1/lily/beam.cc#newcode173 lily/beam.cc:173: span_data.push_back (get_beam_span ...
13 years ago (2011-03-12 10:18:05 UTC) #6
hanwenn
http://codereview.appspot.com/4237057/diff/1/lily/spanner.cc File lily/spanner.cc (right): http://codereview.appspot.com/4237057/diff/1/lily/spanner.cc#newcode405 lily/spanner.cc:405: Spanner::broken_spanner_index (Spanner const *sp) On 2011/03/12 10:18:06, MikeSol wrote: ...
13 years ago (2011-03-12 13:49:27 UTC) #7
mike_apollinemike.com
On Mar 12, 2011, at 2:49 PM, hanwenn@gmail.com wrote: > > http://codereview.appspot.com/4237057/diff/1/lily/spanner.cc > File lily/spanner.cc ...
13 years ago (2011-03-12 18:05:31 UTC) #8
mike_apollinemike.com
On Mar 12, 2011, at 8:49 AM, hanwenn@gmail.com wrote: > > http://codereview.appspot.com/4237057/diff/1/lily/spanner.cc > File lily/spanner.cc ...
13 years ago (2011-03-13 21:16:07 UTC) #9
hanwenn
http://codereview.appspot.com/4237057/diff/11001/lily/beam.cc File lily/beam.cc (right): http://codereview.appspot.com/4237057/diff/11001/lily/beam.cc#newcode206 lily/beam.cc:206: orig->set_property ("feather-fraction", scm_cons (scm_from_double (0.0), scm_from_double (orig->spanner_length ()))); my ...
13 years ago (2011-03-14 02:30:22 UTC) #10
mike_apollinemike.com
On Mar 13, 2011, at 10:30 PM, hanwenn@gmail.com wrote: > > http://codereview.appspot.com/4237057/diff/11001/lily/beam.cc > File lily/beam.cc ...
13 years ago (2011-03-14 09:48:33 UTC) #11
mike_apollinemike.com
On Mar 13, 2011, at 10:30 PM, hanwenn@gmail.com wrote: > > http://codereview.appspot.com/4237057/diff/11001/lily/beam.cc > File lily/beam.cc ...
13 years ago (2011-03-14 12:16:26 UTC) #12
hanwenn
On Mon, Mar 14, 2011 at 9:16 AM, <mike@apollinemike.com> wrote: > I've sketched this out ...
13 years ago (2011-03-15 13:03:24 UTC) #13
hanwenn
http://codereview.appspot.com/4237057/diff/14002/lily/beam.cc File lily/beam.cc (right): http://codereview.appspot.com/4237057/diff/14002/lily/beam.cc#newcode176 lily/beam.cc:176: Beam::calc_feather_widths (SCM smob) actually, you could just call this ...
13 years ago (2011-03-15 13:03:36 UTC) #14
hanwenn
http://codereview.appspot.com/4237057/diff/14002/lily/beam.cc File lily/beam.cc (right): http://codereview.appspot.com/4237057/diff/14002/lily/beam.cc#newcode204 lily/beam.cc:204: SCM temp = scm_cons (scm_from_double (feather_fractions[i][LEFT] / total_width), scm_from_double ...
13 years ago (2011-03-15 13:04:39 UTC) #15
mike_apollinemike.com
On Mar 15, 2011, at 9:03 AM, Han-Wen Nienhuys wrote: > On Mon, Mar 14, ...
13 years ago (2011-03-15 13:08:30 UTC) #16
hanwenn
On Tue, Mar 15, 2011 at 10:08 AM, mike@apollinemike.com <mike@apollinemike.com> wrote: > On Mar 15, ...
13 years ago (2011-03-15 13:18:17 UTC) #17
mike_apollinemike.com
On Mar 15, 2011, at 9:18 AM, Han-Wen Nienhuys wrote: > On Tue, Mar 15, ...
13 years ago (2011-03-15 14:11:05 UTC) #18
MikeSol
New patch set uploaded. Thanks for the comments, Han-Wen! http://codereview.appspot.com/4237057/diff/14002/lily/beam.cc File lily/beam.cc (right): http://codereview.appspot.com/4237057/diff/14002/lily/beam.cc#newcode176 lily/beam.cc:176: ...
13 years ago (2011-03-15 22:32:30 UTC) #19
hanwenn
lgtm http://codereview.appspot.com/4237057/diff/13002/lily/beam.cc File lily/beam.cc (right): http://codereview.appspot.com/4237057/diff/13002/lily/beam.cc#newcode593 lily/beam.cc:593: Interval placements = robust_scm2interval (me->get_property ("normalized-endpoints"), Interval (0.0,0.0)); ...
13 years ago (2011-03-16 02:17:28 UTC) #20
MikeSol
Done - after an LGTM from Han Wen, I'll run the regtests and push l8r ...
13 years ago (2011-03-16 10:06:40 UTC) #21
mike_apollinemike.com
13 years ago (2011-03-16 19:10:41 UTC) #22
On Mar 16, 2011, at 6:06 AM, mtsolo@gmail.com wrote:

> Done - after an LGTM from Han Wen, I'll run the regtests and push l8r
> today unless anyone has more comments.
> 
> 
> http://codereview.appspot.com/4237057/diff/13002/lily/beam.cc
> File lily/beam.cc (right):
> 
> http://codereview.appspot.com/4237057/diff/13002/lily/beam.cc#newcode593
> lily/beam.cc:593: Interval placements = robust_scm2interval
> (me->get_property ("normalized-endpoints"), Interval (0.0,0.0));
> On 2011/03/16 02:17:28, hanwenn wrote:
>> space after ,
> 
> Done.
> 
> http://codereview.appspot.com/4237057/diff/13002/lily/beam.cc#newcode624
> lily/beam.cc:624: weights[RIGHT] = 1 - multiplier;
> On 2011/03/16 02:17:28, hanwenn wrote:
>> weights.swap() ?
> 
> Done.
> 
> http://codereview.appspot.com/4237057/diff/13002/lily/beam.cc#newcode636
> lily/beam.cc:636: + beam_dy * segments[extreme].vertical_count_;
> On 2011/03/16 02:17:28, hanwenn wrote:
> 
>> how about
> 
>> int idx = {i, extreme}
>> int translations[2]
>> for (j = 0;j<2;j++)
>>   translations[i] = slope * (segments[idx[j]]  .. etc)
> 
> Done.
> 
> http://codereview.appspot.com/4237057/diff/13002/lily/spanner.cc
> File lily/spanner.cc (right):
> 
> http://codereview.appspot.com/4237057/diff/13002/lily/spanner.cc#newcode406
> lily/spanner.cc:406: SCM ret = scm_cons (scm_from_double (0.0),
> scm_from_double (0.0));
> On 2011/03/16 02:17:28, hanwenn wrote:
>> i personally like 'result' better, almost as short, and a full word.
> 
>> You could init to SCM_EOL instead?
> 
> Done.
> 
> http://codereview.appspot.com/4237057/diff/13002/lily/spanner.cc#newcode433
> lily/spanner.cc:433: SCM t = ly_interval2scm (Interval
> (unnormalized_endpoints[i][LEFT] / total_width,
> unnormalized_endpoints[i][RIGHT] / total_width));
> On 2011/03/16 02:17:28, hanwenn wrote:
>> try using interval * 1/width (or 1/width * interval).  The operator
> overloading
>> for interval is hit and miss.  You can add a operator/ if you want
> (separate
>> commit)
> 
> Done.
> 
> http://codereview.appspot.com/4237057/
> 

Pushed, w/ whitespace errors squelched & the version number in the regtest
updated to 2.13.55.

Cheers,
MS

Sign in to reply to this message.

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