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

[address@hidden]

On Oct 26, 2011, at 10:13 PM, address@hidden wrote:

I don't understand beam-quanting well enough to evaluate most of the
code, but I've seen some concerns in properties and regtests.

Thanks,

Carl

Carl,

In general, you're absolutely right about the regtests.  I've scrapped them all and replaced them with the most pertinent extracts from the old regtest.

http://codereview.appspot.com/5293060/diff/2004/lily/beam-quanting.cc
File lily/beam-quanting.cc (right):

http://codereview.appspot.com/5293060/diff/2004/lily/beam-quanting.cc#newcode411
lily/beam-quanting.cc:411: init_instance_variables ();//if
(!consistent_broken_slope) printf ("STATS0 EH %4.4f %4.4f X%4.4f Y %4.4f
%4.4f\n", extremal_hangover_[LEFT], extremal_hangover_[RIGHT], x_span_,
unquanted_y_[LEFT], unquanted_y_[RIGHT]);
IMO, you should have a separate branch without the printf statements.
We should not be asked to approve code containing commented out
statements with the assurance that they will be removed before pushing.
We should only be reviewing code intended for pushing (at least if this
patch is intended for pushing as opposed to a design sketch).

The printf statements, particularly in this format, make it very hard to
read the code.

OK - removed.

http://codereview.appspot.com/5293060/diff/2004/lily/beam-quanting.cc#newcode1058
lily/beam-quanting.cc:1058: /*
Nice comment here.  This is very helpful.

Thanks!

http://codereview.appspot.com/5293060/diff/2004/lily/beam.cc
File lily/beam.cc (right):

http://codereview.appspot.com/5293060/diff/2004/lily/beam.cc#newcode979
lily/beam.cc:979: bool consistent_broken_slope = broken_beam_style !=
ly_symbol2scm ("individual");
Here (and in scm/define-grobs.scm) you use "individual"; in
scm/define-grob-properties.scm you use "separate".

Will change.

And I see no code here for "peters".

Check Beam::quanting

http://codereview.appspot.com/5293060/diff/2004/lily/beam.cc#newcode1459
lily/beam.cc:1459: "The property @code{broken-beam-style} can be
@code{separate},"
Values should not be listed here.  They are listed in
scm/define-grob-properties.scm.  If they are listed in two places, they
can get out of step and nobody will know which one is correct.  IMO,
these two lines should be completely removed.  There is no need to list
the property that is part of the interface here; it will be
automatically generated by the docs.

OK - removed.

http://codereview.appspot.com/5293060/diff/2004/lily/spanner.cc
File lily/spanner.cc (right):

http://codereview.appspot.com/5293060/diff/2004/lily/spanner.cc#newcode240
lily/spanner.cc:240: Interval (1,-1));
Some comments in here about the strategy to get the spanner length would
probably be helpful.  You have three different methods, IIUC, that are
called depending on the situation.

Added.

http://codereview.appspot.com/5293060/diff/2004/scm/define-grobs.scm
File scm/define-grobs.scm (right):

http://codereview.appspot.com/5293060/diff/2004/scm/define-grobs.scm#newcode354
scm/define-grobs.scm:354: (broken-beam-style . individual)
This is not one of the alternatives listed in
scm/define-grob-properties.scm

Naming fixed.

Your comments are really helpful - I don't expect people to know how beam-quanting works (it took me a long long time to figure it out), but I think I focused so much on the technical details of quanting that I let some basic stuff (like consistency in naming) slip.  I certainly appreciate your catching it!

Cheers,

MS