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

[address@hidden]

Thanks Keith!
I've incorporated most of your comments into the code and otherwise have a 
couple questions below.

No change in the regtests & a new patch set up.

Cheers,
MS

On Nov 1, 2011, at 5:19 AM, address@hidden wrote:

> 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?
> 

There's no extra cost for unbroken beams and a cost of anywhere between 2 & 3 
times longer for broken beams, as it runs two extra quants - one full and one 
that skips all the inits (this is why the figure is between 2 & 3, as I'm not 
sure how long the inits take with respect to the quanting).

> 
> 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

Comment added to regtest.

> 
> 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
> 

You say, "despite the advertising, peters-prolungation [sic, or device to help 
people fight emphysema :) ?] does not lengthem beams, nor anything else for 
that matter."  Where is this advertising?  Is it the word prolongation?  To 
make things more consistent, I could call the functions individual-breaking, 
strict-breaking, and peters-breaking.

> 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.
> 

That's a good question...I'm not sure.  A change in horizontal spacing that 
preserves the same beam breaking would not be a problem, nor would a test that 
improves the beam breaking.  For now, my response is that hopefully someone 
will see that "beam-broken" is in the title and it will occur to them to pay 
special attention to how the beam breaks look.  I think that the additional 
comments in the test itself help.

> 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 ...).
> 

More explanation given.

> 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.
> 

Done.

> 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?
> 

Done.

> 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.
> 

Comment added.

> 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.
> 

Yes, but the previous one needs to exist for the overwritten value to take 
effect (check out Beam::get_beam_segments (I think it's called that...) in 
current master - this is how it gets its common_x grob).

> 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
> 

Comment added.

> 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.
> 

Excellent catch - this was a mistake on my part.  It was a useless (and 
harmful) variable that I've deleted.  The x_pos variable is the one to use here.

> 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.
> 

You're right on both counts - fixed.

> 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.
> 

Yes.  I think this is encapsulated in the previous comment regarding x_pan.

> 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.
> 

Comment added. 

> 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.
> 

Not necessarily - in peters-prolongation, this is set to false for one of the 
quants even though we are doing broken beaming (to find the quants of the 
individual beam).
I like do_initial_slope_calculations_ because I think it reads cleaner in the 
constructor

> 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.

Exactly.
Should I add this as a comment?

> 
> 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?

Yes - because this used to yield two equal quants, the regtests were changing 
with my new code even though the values of the quants didn't.  The fact that 
they fell on correct quants before is pure luck (the correct quant was the 
first in the pack).  This allows kneed beams to attain the correct values of 
the old regtests.

> 
> 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
> 

Done.

> 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 ?
> 

We'd have to use it for TupletBracket too.  left-bound-info and 
right-bound-info contain both X and Y information, whereas Beam and 
TupletBracket have X-positions and positions.
This could be rewritten, but it'd be better to do it in a separate commit.

> 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
> 

I see what you mean in your comments above - what do you think would be a 
clearer way to name variables and/or add comments so that it is clearer?

> 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.
> 

You're right that this is what the function does.

Cheers,
MS