Re: Fixes slope errors from incorrect X extents in Beam::print. (issue 5293060)

[k-ohara5a5a]

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/