This looks more reasonable. I read through a few times until I had it figured out. I left comments where I really needed either code comments or better variable names.
Do you know the performance cost of turning on peters-prolongation? http://codereview.appspot.com/5293060/diff/19014/Documentation/changes.tely File Documentation/changes.tely (right): http://codereview.appspot.com/5293060/diff/19014/Documentation/changes.tely#newcode68 Documentation/changes.tely:68: \once \override Beam #'positions = #beam::strict-prolongation strict-prolongation is the one that returns y-positions at the ends of the beam such that beams align-across-breaks http://codereview.appspot.com/5293060/diff/19014/Documentation/changes.tely#newcode70 Documentation/changes.tely:70: \once \override Beam #'positions = #beam::peters-prolongation Despite the advertising, peters-prolungation does not lengthen beams, nor anything else for that matter. It returns y-positions for the ends of the beam such that beams have similar-slope-across-breaks http://codereview.appspot.com/5293060/diff/19014/input/regression/beam-broken-classic.ly File input/regression/beam-broken-classic.ly (right): http://codereview.appspot.com/5293060/diff/19014/input/regression/beam-broken-classic.ly#newcode4 input/regression/beam-broken-classic.ly:4: texidoc="Some classic examples of broken beams, all taken from How do we know if the test passes or not ? Suppose some future patch changes the note-spacing or something. http://codereview.appspot.com/5293060/diff/19014/input/regression/beam-quanting-overhang.ly File input/regression/beam-quanting-overhang.ly (right): http://codereview.appspot.com/5293060/diff/19014/input/regression/beam-quanting-overhang.ly#newcode4 input/regression/beam-quanting-overhang.ly:4: texidoc = "Beam quanting accounds for beam overhang. Here you really need to say what constitutes a successful test. The ends of the beam above the rests should rest on staff lines (or otherwise be aligned to staff lines as shown in Ross p ...). http://codereview.appspot.com/5293060/diff/19014/lily/beam-quanting.cc File lily/beam-quanting.cc (right): http://codereview.appspot.com/5293060/diff/19014/lily/beam-quanting.cc#newcode184 lily/beam-quanting.cc:184: void Beam_scoring_problem::init_instance_variables () Logically, this is part of the constructor. If you are splitting it just because the constructor is long, you need to make some logical sorting of what goes in which part, or else it is even harder to read than one long constructor. http://codereview.appspot.com/5293060/diff/19014/lily/beam-quanting.cc#newcode191 lily/beam-quanting.cc:191: consistent_broken_slope_ = false; Can you make these tests and adjust the value of the boolean at the point where you first read the property? http://codereview.appspot.com/5293060/diff/19014/lily/beam-quanting.cc#newcode200 lily/beam-quanting.cc:200: x_span_ = 0.0; x_span_ is a single scalar, cumulatively summing the length of all the segments the parent beam was broken-into. http://codereview.appspot.com/5293060/diff/19014/lily/beam-quanting.cc#newcode215 lily/beam-quanting.cc:215: common[a] = common_refpoint_of_array (stems, beams[i], Axis (a)); Looks like one of these will be overwritten in the next loop. http://codereview.appspot.com/5293060/diff/19014/lily/beam-quanting.cc#newcode222 lily/beam-quanting.cc:222: const Interval x_pos = robust_scm2interval (beams[i]->get_property ("X-positions"), Interval (0.0, 0.0)); positions of the endpoints of this beam segment, including any overhangs http://codereview.appspot.com/5293060/diff/19014/lily/beam-quanting.cc#newcode228 lily/beam-quanting.cc:228: local_x_span[d] = edge_stems[d] ? edge_stems[d]->relative_coordinate (common[X_AXIS], X_AXIS) : 0.0; So local_x_span includes *only* the portion with normal stems, differently from the other *x_span* variables. http://codereview.appspot.com/5293060/diff/19014/lily/beam-quanting.cc#newcode272 lily/beam-quanting.cc:272: is_knee_ = dirs_found[LEFT] && dirs_found[RIGHT]; Why not UP and DOWN? Are we still in the loop over broken portions? If so, this will be left set according to the last portion of a broken beam, not necessarily the portion we are working on. http://codereview.appspot.com/5293060/diff/19014/lily/beam-quanting.cc#newcode355 lily/beam-quanting.cc:355: x_span_ += beams[i]->spanner_length (); Ultimately from Beam::calc_x_positions(), so x_span_ is the total length, including overhangs, of previous segments that the parent beam was broken-into. http://codereview.appspot.com/5293060/diff/19014/lily/beam-quanting.cc#newcode359 lily/beam-quanting.cc:359: Beam_scoring_problem::Beam_scoring_problem (Grob *me, Drul_array<Real> ys, bool consistent_broken_slope) If 'ys' are finite, use them as starting points for y-positions of the ends of the beam, instead of the best-fit through the natural ends of the stems. The boolean really means, if 'me' is a segment of a broken beam, then beam 'me' together with my fellow broken-intos. Maybe the boolean should be align-broken-segments or something. http://codereview.appspot.com/5293060/diff/19014/lily/beam-quanting.cc#newcode743 lily/beam-quanting.cc:743: if (do_initial_slope_calculations_) Even if we made an earlier pass, and avoid the collisions and/or made a pretty knee, the averaging in peters-prolungation messed up the results so we would have to do it again. http://codereview.appspot.com/5293060/diff/19014/lily/beam-quanting.cc#newcode989 lily/beam-quanting.cc:989: and can either be quanted up or down. Does this have something to do with X-extents, or Beam::print, or broken beams? http://codereview.appspot.com/5293060/diff/19014/lily/spanner.cc File lily/spanner.cc (right): http://codereview.appspot.com/5293060/diff/19014/lily/spanner.cc#newcode238 lily/spanner.cc:238: X-positions or left-bound-info and right-bound-info. These are left-bound-info.X and right-bound-info.X http://codereview.appspot.com/5293060/diff/19014/lily/spanner.cc#newcode244 lily/spanner.cc:244: For those writing a new spanner, DO NOT use both X-positions and Why not just use the existing {left|right}-bound-info.X for broken beams ? http://codereview.appspot.com/5293060/diff/19014/scm/output-lib.scm File scm/output-lib.scm (right): http://codereview.appspot.com/5293060/diff/19014/scm/output-lib.scm#newcode65 scm/output-lib.scm:65: ;; calculates each slope of a broken beam individually This might make people think beam::individual-slopes returns one or more slopes, or iterates over segments of a broken beam. Rather, it computes y-positions of the ends of the beam. It does not even look to see if the beam is actually a broken part of a larger beam. http://codereview.appspot.com/5293060/diff/19014/scm/output-lib.scm#newcode69 scm/output-lib.scm:69: ;; calculates the slope of a beam as a single unit, Moreover, this function actively finds if the passed beam section is part of a broken beam, and returns y-positions that will match the neighboring segment(s). http://codereview.appspot.com/5293060/diff/19014/scm/output-lib.scm#newcode82 scm/output-lib.scm:82: (let* ((quant1 (ly:beam::quanting grob '(+inf.0 . -inf.0) #t)) quant1 are from beam-together http://codereview.appspot.com/5293060/diff/19014/scm/output-lib.scm#newcode89 scm/output-lib.scm:89: (let* ((quant2 (ly:beam::quanting grob '(+inf.0 . -inf.0) #f)) quant2 are from beam-alone http://codereview.appspot.com/5293060/diff/19014/scm/output-lib.scm#newcode96 scm/output-lib.scm:96: (factor (/ (atan (abs slope1)) PI-OVER-TWO)) The trig function does not seem to have a direct geometric meaning, but is used to build a weighting function going from 0 to 1 linearly as the beam angle goes from 0 to 90 degrees. http://codereview.appspot.com/5293060/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
