|
|
DescriptionPotential 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 #
MessagesTotal messages: 22
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 broken pieces cannot ever share a commonx. 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) why not make it a real member funtcion?
Sign in to reply to this message.
Thanks for the comments! Below I have a question about commonx - the other fix will be easy to implement. 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)); On 2011/03/09 23:03:44, hanwenn wrote: > this looks wrong - different broken pieces cannot ever share a commonx. Odd - it gets correct results for some reason (I honestly don't know how...). What would be a better way of grabbing these coordinates (the left and right points of the beam)? 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/09 23:03:44, hanwenn wrote: > why not make it a real member funtcion? Doable! I'll do it w/ my next patch.
Sign in to reply to this message.
Thanks for the comments! Below I have a question about commonx - the other fix will be easy to implement.
Sign in to reply to this message.
On Thu, Mar 10, 2011 at 5:04 AM, <mtsolo@gmail.com> wrote: >> this looks wrong - different broken pieces cannot ever share a > > commonx. > > Odd - it gets correct results for some reason (I honestly don't know > how...). Lily tries not to crash, and things may just work in this case, but better not rely on it. The correct way is to use each beam's System() as a reference point, as the System always is the ultimate parent in both X and Y directions. -- Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen
Sign in to reply to this message.
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 all of the if (feather) code. You're duplicating an awful lot of logic. http://codereview.appspot.com/4237057/diff/1/lily/beam.cc#newcode647 lily/beam.cc:647: } I see dup code. Can you fold? http://codereview.appspot.com/4237057/diff/1/lily/beam.cc#newcode653 lily/beam.cc:653: : segments[segments.size () - 1].vertical_count_); use segments.back() http://codereview.appspot.com/4237057/diff/1/lily/beam.cc#newcode665 lily/beam.cc:665: * (placements[RIGHT] - placements[LEFT]) placements.delta (), placements.length() ? http://codereview.appspot.com/4237057/diff/1/lily/beam.cc#newcode682 lily/beam.cc:682: weights[LEFT] = 1; can you set multiplier to 1 in this case?
Sign in to reply to this message.
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 (orig->broken_intos_[i], commonx)); On 2011/03/09 23:03:44, hanwenn wrote: > this looks wrong - different broken pieces cannot ever share a commonx. Fixed. http://codereview.appspot.com/4237057/diff/1/lily/beam.cc#newcode620 lily/beam.cc:620: { On 2011/03/11 14:01:29, hanwenn wrote: > it would be nice if you could collapse all of the if (feather) code. You're > duplicating an awful lot of logic. I'm not exactly sure what you mean here by "collapse." I folded the code that you mention below, but I'm not sure where it'd be possible to reduce this code. http://codereview.appspot.com/4237057/diff/1/lily/beam.cc#newcode647 lily/beam.cc:647: } On 2011/03/11 14:01:29, hanwenn wrote: > I see dup code. Can you fold? Done. http://codereview.appspot.com/4237057/diff/1/lily/beam.cc#newcode653 lily/beam.cc:653: : segments[segments.size () - 1].vertical_count_); On 2011/03/11 14:01:29, hanwenn wrote: > use segments.back() Done. http://codereview.appspot.com/4237057/diff/1/lily/beam.cc#newcode665 lily/beam.cc:665: * (placements[RIGHT] - placements[LEFT]) On 2011/03/11 14:01:29, hanwenn wrote: > placements.delta (), placements.length() ? Done. http://codereview.appspot.com/4237057/diff/1/lily/beam.cc#newcode682 lily/beam.cc:682: weights[LEFT] = 1; On 2011/03/11 14:01:29, hanwenn wrote: > can you set multiplier to 1 in this case? Done. 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/09 23:03:44, hanwenn wrote: > why not make it a real member funtcion? Actually - sorry, I spoke too soon. I see that there's a function get_break_index. Could these two functions be merged? Is there a reason that the two functions exist separately? If not, I'll merge them together.
Sign in to reply to this message.
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: > On 2011/03/09 23:03:44, hanwenn wrote: > > why not make it a real member funtcion? > > Actually - sorry, I spoke too soon. I see that there's a function > get_break_index. Could these two functions be merged? Is there a reason that > the two functions exist separately? If not, I'll merge them together. good point. Can you replace broken_spanner_index by get_break_index everywhere? should be separate commit to go in before this one. If it passes the regtest cleanly, does not need to be reviewed. http://codereview.appspot.com/4237057/diff/8001/lily/beam.cc File lily/beam.cc (right): http://codereview.appspot.com/4237057/diff/8001/lily/beam.cc#newcode144 lily/beam.cc:144: Beam::get_beam_span (Spanner* me) Why can't you use spanner::spanner_length()? these numbers don't need to be that exact, do they? http://codereview.appspot.com/4237057/diff/8001/lily/beam.cc#newcode160 lily/beam.cc:160: extract_grob_set (me, "stems", stems); you already have them. line 145. In this case you might as well use get_bound() rather than the stems, btw. http://codereview.appspot.com/4237057/diff/8001/lily/beam.cc#newcode169 lily/beam.cc:169: Beam::get_span_data (Spanner *me) _data -> _widths ? http://codereview.appspot.com/4237057/diff/8001/lily/beam.cc#newcode171 lily/beam.cc:171: drop http://codereview.appspot.com/4237057/diff/8001/lily/beam.cc#newcode634 lily/beam.cc:634: if (Spanner::broken_spanner_index (me) == (int)i) use newstyle casting int(i) http://codereview.appspot.com/4237057/diff/8001/lily/beam.cc#newcode646 lily/beam.cc:646: } how about you store this in a property? You could compute the property in a separate function, say feather-fractions pair? "how much of feathering for this beam" and the computation would only need to be done once. 1st beam gets: (0.0 . 0.25) 2nd: (0.25 . 0.75) 3rd: (0.75 . 1.0) (assuming total beam length of 2x linesize). then print() only needs to do the part for its own system. unfeathered beams get (1.0 . 1.0) and shrinking beams (1.0 . 0.0)
Sign in to reply to this message.
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 (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: >> On 2011/03/09 23:03:44, hanwenn wrote: >> > why not make it a real member funtcion? > >> Actually - sorry, I spoke too soon. I see that there's a function >> get_break_index. Could these two functions be merged? Is there a > reason that >> the two functions exist separately? If not, I'll merge them together. > > good point. Can you replace broken_spanner_index by get_break_index > everywhere? should be separate commit to go in before this one. If it > passes the regtest cleanly, does not need to be reviewed. > Passes the regtests and pushed.
Sign in to reply to this message.
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 (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: >> On 2011/03/09 23:03:44, hanwenn wrote: >> > why not make it a real member funtcion? > >> Actually - sorry, I spoke too soon. I see that there's a function >> get_break_index. Could these two functions be merged? Is there a > reason that >> the two functions exist separately? If not, I'll merge them together. > > good point. Can you replace broken_spanner_index by get_break_index > everywhere? should be separate commit to go in before this one. If it > passes the regtest cleanly, does not need to be reviewed. > > http://codereview.appspot.com/4237057/diff/8001/lily/beam.cc > File lily/beam.cc (right): > > http://codereview.appspot.com/4237057/diff/8001/lily/beam.cc#newcode144 > lily/beam.cc:144: Beam::get_beam_span (Spanner* me) > Why can't you use spanner::spanner_length()? these numbers don't need > to be that exact, do they? > > http://codereview.appspot.com/4237057/diff/8001/lily/beam.cc#newcode160 > lily/beam.cc:160: extract_grob_set (me, "stems", stems); > you already have them. line 145. In this case you might as well use > get_bound() rather than the stems, btw. > > http://codereview.appspot.com/4237057/diff/8001/lily/beam.cc#newcode169 > lily/beam.cc:169: Beam::get_span_data (Spanner *me) > _data -> _widths ? > > http://codereview.appspot.com/4237057/diff/8001/lily/beam.cc#newcode171 > lily/beam.cc:171: > drop > > http://codereview.appspot.com/4237057/diff/8001/lily/beam.cc#newcode634 > lily/beam.cc:634: if (Spanner::broken_spanner_index (me) == (int)i) > use newstyle casting int(i) > > http://codereview.appspot.com/4237057/diff/8001/lily/beam.cc#newcode646 > lily/beam.cc:646: } > how about you store this in a property? You could compute the property > in a separate function, say > > feather-fractions pair? "how much of feathering for this beam" > > and the computation would only need to be done once. > > 1st beam gets: (0.0 . 0.25) > 2nd: (0.25 . 0.75) > 3rd: (0.75 . 1.0) > > (assuming total beam length of 2x linesize). > > then print() only needs to do the part for its own system. > > unfeathered beams get (1.0 . 1.0) and shrinking beams (1.0 . 0.0) > All issues addressed. New patch set up at http://codereview.appspot.com/4237057/ Cheers, MS
Sign in to reply to this message.
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 suggestion was for fraction to be a real fraction, ie. a number from 0.0 to 1.0, relative to the total length of the beam. That also gives users a way to tune the featheriness they want from their beams (they could set it to 0.0 - 2.0 to exaggerate the effect for instance), in a scale-free way. My idea was also to put the effect of feather-dir into this pair, ie. feather=LEFT => (1.0 . 0.0) and RIGHT => (0.0 . 1.0) Does that sound right? I think you would be able to do without feather-dir in the print callback. http://codereview.appspot.com/4237057/diff/11001/lily/include/spanner.hh File lily/include/spanner.hh (right): http://codereview.appspot.com/4237057/diff/11001/lily/include/spanner.hh#newc... lily/include/spanner.hh:76: static int broken_spanner_index (Spanner const *sp); this can go now? http://codereview.appspot.com/4237057/diff/11001/scm/define-grobs.scm File scm/define-grobs.scm (right): http://codereview.appspot.com/4237057/diff/11001/scm/define-grobs.scm#newcode329 scm/define-grobs.scm:329: (after-line-breaking . ,ly:beam::calc-feather-widths) I recommend hooking this up to feather-fraction directly, so you can be sure it's always calculated at the right time, namely, when needed. Beyond setting the fractions for all beams, you'd have to return the fraction pair for the beam part on which the callback gets called
Sign in to reply to this message.
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 (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 suggestion was for fraction to be a real fraction, ie. a number from > 0.0 to 1.0, relative to the total length of the beam. That also gives > users a way to tune the featheriness they want from their beams (they > could set it to 0.0 - 2.0 to exaggerate the effect for instance), in a > scale-free way. My idea was also to put the effect of feather-dir into > this pair, ie. feather=LEFT => (1.0 . 0.0) and RIGHT => (0.0 . 1.0) > > Does that sound right? I think you would be able to do without > feather-dir in the print callback. Ah, OK, yes. Totally doable, although it'd require a convert-ly rule that I'm not sure I know how to write. But I can work on the code part. > > http://codereview.appspot.com/4237057/diff/11001/lily/include/spanner.hh > File lily/include/spanner.hh (right): > > http://codereview.appspot.com/4237057/diff/11001/lily/include/spanner.hh#newc... > lily/include/spanner.hh:76: static int broken_spanner_index (Spanner > const *sp); > this can go now? > Yes, this is vestigial from the last patch (old functions die hard). > http://codereview.appspot.com/4237057/diff/11001/scm/define-grobs.scm > File scm/define-grobs.scm (right): > > http://codereview.appspot.com/4237057/diff/11001/scm/define-grobs.scm#newcode329 > scm/define-grobs.scm:329: (after-line-breaking . > ,ly:beam::calc-feather-widths) > I recommend hooking this up to feather-fraction directly, so you can be > sure it's always calculated at the right time, namely, when needed. > > Beyond setting the fractions for all beams, > you'd have to return the fraction pair for the beam part on which the > callback gets called > This is way doable (and will require less code than what I have now), but it'll require more loops. If you calculate these all at once, you only ever need one loop. If you calculate it for n broken beams, you need n loops that can break after they hit the correct broken spanner. This doesn't seem like a lot of overhead, but it does redo some calculations. If this is OK, I'll implement it. ~Mike
Sign in to reply to this message.
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 (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 suggestion was for fraction to be a real fraction, ie. a number from > 0.0 to 1.0, relative to the total length of the beam. That also gives > users a way to tune the featheriness they want from their beams (they > could set it to 0.0 - 2.0 to exaggerate the effect for instance), in a > scale-free way. My idea was also to put the effect of feather-dir into > this pair, ie. feather=LEFT => (1.0 . 0.0) and RIGHT => (0.0 . 1.0) > > Does that sound right? I think you would be able to do without > feather-dir in the print callback. > > http://codereview.appspot.com/4237057/diff/11001/lily/include/spanner.hh > File lily/include/spanner.hh (right): > > http://codereview.appspot.com/4237057/diff/11001/lily/include/spanner.hh#newc... > lily/include/spanner.hh:76: static int broken_spanner_index (Spanner > const *sp); > this can go now? > > http://codereview.appspot.com/4237057/diff/11001/scm/define-grobs.scm > File scm/define-grobs.scm (right): > > http://codereview.appspot.com/4237057/diff/11001/scm/define-grobs.scm#newcode329 > scm/define-grobs.scm:329: (after-line-breaking . > ,ly:beam::calc-feather-widths) > I recommend hooking this up to feather-fraction directly, so you can be > sure it's always calculated at the right time, namely, when needed. > > Beyond setting the fractions for all beams, > you'd have to return the fraction pair for the beam part on which the > callback gets called I've sketched this out using your suggestion above (calculating it once and returning the fraction for the called beam) - nevermind my previous question about redoing calculations. A new patch set is on-line. I still need to do the math for the longer slopes - I'll have time to do that later today or tomorrow. In the spirit of the one-change-per-push idea, I'd like to push the fix to 1504 first before I push the change to feather-direction. Does this seem like a good idea? Cheers, MS
Sign in to reply to this message.
On Mon, Mar 14, 2011 at 9:16 AM, <mike@apollinemike.com> wrote: > I've sketched this out using your suggestion above (calculating it once and returning the fraction for the called beam) - nevermind my previous question about redoing calculations. A new patch set is on-line. > I still need to do the math for the longer slopes - I'll have time to do that later today or tomorrow. > > In the spirit of the one-change-per-push idea, I'd like to push the fix to 1504 first before I push the change to feather-direction. Does this seem like a good idea? Do you mean: push an earlier version of this patch first? I think it's not a good idea, because you would rewrite it directly after pushing, cluttering up the history of what is happening. The idea of one-change-per-push is that all the individual changes are independent. -- Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen
Sign in to reply to this message.
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 length-fraction; what's calculated over here is something generic to spanners. You could even move it there. 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 (feather_fractions[i][RIGHT] / total_width)); can you use a meaningful name? Even a 1 letter name than 'temp' http://codereview.appspot.com/4237057/diff/14002/lily/beam.cc#newcode672 lily/beam.cc:672: to the total length. ? should the total feathering dy be proportional to that?
Sign in to reply to this message.
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 (feather_fractions[i][RIGHT] / total_width)); do you mean interval2scm(feather_factions[i] / total_width) ?
Sign in to reply to this message.
On Mar 15, 2011, at 9:03 AM, Han-Wen Nienhuys wrote: > On Mon, Mar 14, 2011 at 9:16 AM, <mike@apollinemike.com> wrote: > >> I've sketched this out using your suggestion above (calculating it once and returning the fraction for the called beam) - nevermind my previous question about redoing calculations. A new patch set is on-line. > >> I still need to do the math for the longer slopes - I'll have time to do that later today or tomorrow. >> >> In the spirit of the one-change-per-push idea, I'd like to push the fix to 1504 first before I push the change to feather-direction. Does this seem like a good idea? > > Do you mean: push an earlier version of this patch first? I think > it's not a good idea, because you would rewrite it directly after > pushing, cluttering up the history of what is happening. The idea of > one-change-per-push is that all the individual changes are > independent. No, I mean that changing the feather-dir property from ly:dir to a pair seems like a different problem than fixing issue 1504. It effectively adds a new feature to lilypond, and thus seems like it should be the object of its own patch/push. However, if you think I should roll this into the 1504 fix, I will. Cheers, MS
Sign in to reply to this message.
On Tue, Mar 15, 2011 at 10:08 AM, mike@apollinemike.com <mike@apollinemike.com> wrote: > On Mar 15, 2011, at 9:03 AM, Han-Wen Nienhuys wrote: > >> On Mon, Mar 14, 2011 at 9:16 AM, <mike@apollinemike.com> wrote: >> >>> I've sketched this out using your suggestion above (calculating it once and returning the fraction for the called beam) - nevermind my previous question about redoing calculations. A new patch set is on-line. >> >>> I still need to do the math for the longer slopes - I'll have time to do that later today or tomorrow. >>> >>> In the spirit of the one-change-per-push idea, I'd like to push the fix to 1504 first before I push the change to feather-direction. Does this seem like a good idea? >> >> Do you mean: push an earlier version of this patch first? I think >> it's not a good idea, because you would rewrite it directly after >> pushing, cluttering up the history of what is happening. The idea of >> one-change-per-push is that all the individual changes are >> independent. > > No, I mean that changing the feather-dir property from ly:dir to a pair seems like a different problem than fixing issue 1504. It effectively adds a new feature to lilypond, and thus seems like it should be the object of its own patch/push. However, if you think I Ah right. My proposal was for feather-dir to be used to init feather-fractions (or whatever they're called.) - please do what you think is best, but if you are pushing 2 commits where the 2nd mostly rewrites the 1st, you might as well skip the 1st. I would call this a feature, btw; I don't believe we ever suggested that breaking feathered beams works. -- Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen
Sign in to reply to this message.
On Mar 15, 2011, at 9:18 AM, Han-Wen Nienhuys wrote: > On Tue, Mar 15, 2011 at 10:08 AM, mike@apollinemike.com > <mike@apollinemike.com> wrote: >> On Mar 15, 2011, at 9:03 AM, Han-Wen Nienhuys wrote: >> >>> On Mon, Mar 14, 2011 at 9:16 AM, <mike@apollinemike.com> wrote: >>> >>>> I've sketched this out using your suggestion above (calculating it once and returning the fraction for the called beam) - nevermind my previous question about redoing calculations. A new patch set is on-line. >>> >>>> I still need to do the math for the longer slopes - I'll have time to do that later today or tomorrow. >>>> >>>> In the spirit of the one-change-per-push idea, I'd like to push the fix to 1504 first before I push the change to feather-direction. Does this seem like a good idea? >>> >>> Do you mean: push an earlier version of this patch first? I think >>> it's not a good idea, because you would rewrite it directly after >>> pushing, cluttering up the history of what is happening. The idea of >>> one-change-per-push is that all the individual changes are >>> independent. >> >> No, I mean that changing the feather-dir property from ly:dir to a pair seems like a different problem than fixing issue 1504. It effectively adds a new feature to lilypond, and thus seems like it should be the object of its own patch/push. However, if you think I > > Ah right. My proposal was for feather-dir to be used to init > feather-fractions (or whatever they're called.) - please do what you > think is best, but if you are pushing 2 commits where the 2nd mostly > rewrites the 1st, you might as well skip the 1st. I'm going to push the first commit after people sign off on it and then work on the feather-dir bit (w/ the appropriate convert-ly rule). It won't require many rewrites: it'll just require some more math in the calc_feather_fractions function. Cheers, MS
Sign in to reply to this message.
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: Beam::calc_feather_widths (SCM smob) On 2011/03/15 13:03:36, hanwenn wrote: > actually, you could just call this length-fraction; what's calculated over here > is something generic to spanners. You could even move it there. Done. 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 (feather_fractions[i][RIGHT] / total_width)); On 2011/03/15 13:04:39, hanwenn wrote: > do you mean interval2scm(feather_factions[i] / total_width) ? Done, but for some reason, the division operator does not work, so I had to separate it out into LEFT and 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 (feather_fractions[i][RIGHT] / total_width)); On 2011/03/15 13:03:36, hanwenn wrote: > can you use a meaningful name? > > Even a 1 letter name than 'temp' Done. http://codereview.appspot.com/4237057/diff/14002/lily/beam.cc#newcode672 lily/beam.cc:672: to the total length. On 2011/03/15 13:03:36, hanwenn wrote: > ? > > should the total feathering dy be proportional to that? Yes (I think...).
Sign in to reply to this message.
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)); space after , http://codereview.appspot.com/4237057/diff/13002/lily/beam.cc#newcode624 lily/beam.cc:624: weights[RIGHT] = 1 - multiplier; weights.swap() ? http://codereview.appspot.com/4237057/diff/13002/lily/beam.cc#newcode636 lily/beam.cc:636: + beam_dy * segments[extreme].vertical_count_; how about int idx = {i, extreme} int translations[2] for (j = 0;j<2;j++) translations[i] = slope * (segments[idx[j]] .. etc) 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)); i personally like 'result' better, almost as short, and a full word. You could init to SCM_EOL instead? 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)); 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)
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
|