http://codereview.appspot.com/5293060/diff/2001/lily/beam.cc File lily/beam.cc (right): http://codereview.appspot.com/5293060/diff/2001/lily/beam.cc#newcode987 lily/beam.cc:987: Beam::calc_x_span (Grob *me_non_spanner, Grob *commonx) Why should the x-span ...
13 years, 5 months ago
(2011-10-25 03:52:31 UTC)
#1
http://codereview.appspot.com/5293060/diff/2001/lily/beam.cc
File lily/beam.cc (right):
http://codereview.appspot.com/5293060/diff/2001/lily/beam.cc#newcode987
lily/beam.cc:987: Beam::calc_x_span (Grob *me_non_spanner, Grob *commonx)
Why should the x-span of a line-broken beam depend on whether we requested that
it have consistent slope across the break?
Shouldn't the 'span' of a broken beam always go to the end of the beam, beyond
the last stem, for the purposes used in Beam::print() ?
If not, what use-case will break when we choose consistent-broken-slope after
this patch ?
On Oct 25, 2011, at 5:52 AM, k-ohara5a5a@oco.net wrote: > > http://codereview.appspot.com/5293060/diff/2001/lily/beam.cc > File lily/beam.cc ...
13 years, 5 months ago
(2011-10-25 05:19:47 UTC)
#2
On Oct 25, 2011, at 5:52 AM, k-ohara5a5a@oco.net wrote:
>
> http://codereview.appspot.com/5293060/diff/2001/lily/beam.cc
> File lily/beam.cc (right):
>
> http://codereview.appspot.com/5293060/diff/2001/lily/beam.cc#newcode987
> lily/beam.cc:987: Beam::calc_x_span (Grob *me_non_spanner, Grob
> *commonx)
> Why should the x-span of a line-broken beam depend on whether we
> requested that it have consistent slope across the break?
> Shouldn't the 'span' of a broken beam always go to the end of the beam,
> beyond the last stem, for the purposes used in Beam::print() ?
> If not, what use-case will break when we choose consistent-broken-slope
> after this patch ?
Before this patch, the x_span of beams was only ever calculated between the
first normal stem and last normal stem of a beam (omitting any trailing beamage
on the left or right coming from breaks and/or stemlets). If it has a
consistent slope, however, the x_span of a broken part of a beam should be the
whole length, as the trailing beamage on the right and/or left are part of the
length between two stems. This is where the difference comes from.
When you say "what use-case will break when we choose consistent-broken-slope
after this patch ?", I'm not sure what you mean.
Thanks for the feedback!
Cheers,
MS
All problems fixed and ready for a review. A few things: 1) please ignore the ...
13 years, 5 months ago
(2011-10-26 15:41:12 UTC)
#3
All problems fixed and ready for a review.
A few things:
1) please ignore the commented out printf statements. i use them for debugging,
and i'll delete them before pushing.
2) the regtest may be a little excessive :) unless people think this is a good
idea, is there a way to shorten it and still test it out in situations that
approximate real music?
3) there are a few regtests that change - all are intentional, and represent
either better handling of kneed beaming or quanted broken beams (quanting in
broken beaming used to be broken because the quant point was taken from normal
stems and not from the beam hangover - this is fixed).
4) there's now a few warning messages in regtests because of a warning i added
for cases where lilypond tries to estimate the slope of a beam with 1 stem,
encouraging the user to use a different broken-beam-style. if
peters-prolongation becomes the default, this message will disappear in all but
the most absurd cases (beams with one stem, for example).
On Oct 26, 2011, at 8:22 PM, k-ohara5a5a@oco.net wrote: > On 2011/10/26 15:41:12, MikeSol wrote: ...
13 years, 5 months ago
(2011-10-26 18:40:33 UTC)
#5
On Oct 26, 2011, at 8:22 PM, k-ohara5a5a@oco.net wrote:
> On 2011/10/26 15:41:12, MikeSol wrote:
>> All problems fixed and ready for a review.
>
> Thoroughly indecipherable.
>
> http://codereview.appspot.com/5293060/
Sorry if anything's unclear - your intuition from a previous e-mail was right in
that there was some broken code that was quanting broken beams incorrectly. The
ends of broken beams should be the reference points for quanting, not the
extremal normal stems Most changes are due to that.
What needs comments?
Here's a bit of a summary:
beam.cc
-) get_beam_segments is turned into a Scheme callback
-) the x-span of the Beam are stored in X-positions (calc_x_positions). these
are used everywhere that the span of the beam used to be calculated. the slopes
of broken beams were off before because x_spans were calculated 3 different ways
in 3 different functions, and this standardizes it.
-) beam quanting can do three broken beam styles:
---) individual (old style)
---) strict-prolongation (new style, split beam slopes and offsets line up
exactly)
---) peters-prolongation (uses the style from turn-of-the-century editions
peters scores)
beam-quanting.cc
-) don't pay attention to line 411ish (it has lots of printfs commented out - if
this is really distracting, I can get it out of the patch, but I don't like
retyping this stuff every time I have to hunt for errors)
-) account_for_extremal_hangover () and
update_x_span_after_extremal_hangover_compensation () are there to make sure
broken beams' quanting figures out y coordinates at the beam's extremes instead
of between the first and last normal slope (this is standard practice in all
major editions but thus far missing in LilyPond).
Everything else represents little modifications needed to make the new x-span
stuff work.
Take it for a spin! I think you'll like your Scirabin example with it :)
Cheers,
MS
On Wed, 26 Oct 2011 11:40:27 -0700, mike@apollinemike.com <mike@apollinemike.com> wrote: > What needs comments? Description ...
13 years, 5 months ago
(2011-10-26 19:41:33 UTC)
#6
On Wed, 26 Oct 2011 11:40:27 -0700, mike@apollinemike.com
<mike@apollinemike.com> wrote:
> What needs comments?
Description of the different setup of the three instances of
Beam_scoring_problem, and their goals.
> -) the x-span of the Beam are stored in X-positions (calc_x_positions). these
are used everywhere that the span of the beam used to be calculated. the slopes
of broken beams were off before because x_spans were calculated 3 different ways
in 3 different functions, and this standardizes it.
What is the standard ?
What is the span of the beam r8[ g' \beam a' r] ?
What about the x_span_ of the Beam_scoring_problem ?
Consistent slopes help a bit, but they way there is done here, the risks of code
complexity seem to outweigh the benefit. The difference between
consistent_broken_slope_ and consistent_broken_slope is dangerous all by itself.
> Take it for a spin!
>
Sorry. I have lost interest.
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#newcode383
lily/beam-quanting.cc:383: x_span_ -= extremal_hangover_[d];
so x_span_ is the span from first normal stem to last normal stem.
On Oct 26, 2011, at 10:13 PM, Carl.D.Sorensen@gmail.com wrote: > I don't understand beam-quanting well ...
13 years, 5 months ago
(2011-10-27 08:34:18 UTC)
#8
On Oct 26, 2011, at 10:13 PM, Carl.D.Sorensen@gmail.com 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#newcode...
> 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
On Oct 26, 2011, at 9:41 PM, k-ohara5a5a@oco.net wrote: > On Wed, 26 Oct 2011 ...
13 years, 5 months ago
(2011-10-27 08:34:21 UTC)
#9
On Oct 26, 2011, at 9:41 PM, k-ohara5a5a@oco.net wrote:
> On Wed, 26 Oct 2011 11:40:27 -0700, mike@apollinemike.com
> <mike@apollinemike.com> wrote:
>
>> What needs comments?
>
> Description of the different setup of the three instances of
> Beam_scoring_problem, and their goals.
>
Done
>> -) the x-span of the Beam are stored in X-positions
> (calc_x_positions). these are used everywhere that the span of the beam
> used to be calculated. the slopes of broken beams were off before
> because x_spans were calculated 3 different ways in 3 different
> functions, and this standardizes it.
>
> What is the standard ?
From the left end of the visible beam (including all overhang) to the right end.
It is gotten by taking a union between the extents of all the segments under
the assumption (which I believe to be correct) that the union of the X-extent of
all of the beam segments is the only true measure of a beam's extent. Measuring
between normal stems or left and right bounds or mixtures of the two (which were
other ways of doing it in the old code) lead to correct results where the
first-normal-stem = bound = overhang, but for broken beams, this is rarely the
case. So, the standard is a union between the extents of all the segments.
> What is the span of the beam r8[ g' \beam a' r] ?
Thanks for the idea - I've put it as a regtest (see the attached PDF for the
output).
\paper { ragged-right = ##t }
{
r2.
\override Beam #'breakable = ##t
r8[ g' \break a' r]
}
{
r2.
\override Beam #'broken-beam-style = #'strict-prolong'ation
\override Beam #'breakable = ##t
r8[ g' \break a' r]
}
{
r2.
\override Beam #'broken-beam-style = #'peters-prolong'ation
\override Beam #'breakable = ##t
r8[ g' \break a' r]
}
I think all three options do what they're supposed to. In the first, the two
slopes do not know of each other's existence and thus give flat slopes (with the
new warning message that slopes cannot be calculated from a stem of 1). In the
second, as expected, the slope on the second line is a prolongation of that on
the first. And in the third example, the beam on the second line is slightly
higher to represent the higher change in note.
Granted, it's not perfect. Change it to r8[g \break a'' r] and you'll see same
ugliness in current master and in the present patchset. But I'd rather save
this fix for a different patchset given the size of the current patchset.
> What about the x_span_ of the Beam_scoring_problem ?
>
It represents two things at two different stages of Beam_scoring_problem.
In least_squares_positions (), slope_damping (), and shift_region_to_valid () it
represents the distance between the first and last normal stem in the
Beam_scoring_problem space, which always starts at 0 for X and extends until the
end of the beam (either an individual beam or all the parts of a broken beam if
we're calculating consistent slopes).
account_for_extremal_hangover () doesn't touch x_span_.
Starting at update_x_span_after_extremal_hangover_compensation (), the x_span_
becomes the real span of the beam, tacking on left and right overhang that may
come from stemlets or broken beams. This is necessary for the quanting (we
always want edges quanted) but not possible for the slope calculations, which
are predicated on the idea that unquanted_y_ represents the beginning and end of
the beam (otherwise, we'd have to compensate for extremal hangover in each
function).
> Consistent slopes help a bit, but they way there is done here, the risks
> of code complexity seem to outweigh the benefit.
I do not see where the code is complex save perhaps Beam::quanting, which now
has lots of comments as per your suggestion.
Otherwise, the bulwark of changes in beam.cc come from the transformation of two
internal calculations (the beam segments and the x positions) into properties
with callbacks. I think that this simplifies and standardizes the code in
several areas. In beam-quanting.cc, the only major changes come from the
addition of account_for_extremal_hangover () and
update_x_span_after_extremal_hangover_compensation (), which are necessary to
use the correct x_span_.
Of course, my goal is to make the change with the least complexity possible. If
you have any suggestions on how to cut down complexity, I definitely want to
hear them.
> The difference between
> consistent_broken_slope_ and consistent_broken_slope is dangerous all by
> itself.
>
I'm not sure what you mean - how is this dangerous?
>> Take it for a spin!
>
> Sorry. I have lost interest.
>
>
> 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#newcode383
> lily/beam-quanting.cc:383: x_span_ -= extremal_hangover_[d];
> so x_span_ is the span from first normal stem to last normal stem.
In least_squares_positions (), slope_damping (), and shift_region_to_valid ().
Starting from update_x_span_after_extremal_hangover_compensation (), this is
tacked back on.
On Oct 28, 2011, at 7:30 AM, Keith OHara wrote: > On Thu, 27 Oct ...
13 years, 5 months ago
(2011-10-28 09:09:56 UTC)
#10
On Oct 28, 2011, at 7:30 AM, Keith OHara wrote:
> On Thu, 27 Oct 2011 01:34:00 -0700, mike@apollinemike.com
<mike@apollinemike.com> wrote:
>
>>> What about the x_span_ of the Beam_scoring_problem ?
>>
>> It represents two things at two different stages of Beam_scoring_problem.
>
> Too bad you didn't use two different variables.
I've fixed the code so that x_span_ (and all variables containing x information)
only ever represent one thing: the full x span of the beam. The distinction
between normal-stem to normal-stem versus left-extremity to right-extremity no
longer exists. This reduces the complexity (and size) of the code without
altering the visual result.
>
>> Starting at update_x_span_after_extremal_hangover_compensation (), the
x_span_
>> becomes the real span of the beam, tacking on left and right overhang that
may
>> come from stemlets or broken beams. This is necessary for the quanting (we
>> always want edges quanted) but not possible for the slope calculations, which
>> are predicated on the idea that unquanted_y_ represents the beginning and end
>> of the beam (otherwise, we'd have to compensate for extremal hangover in each
>> function).
>
> You are telling me _what_ happens, which I could see from the code. Some
things
> looked strange, possibly accidental, so I wanted to know _why_.
>
> I see that the slope-determining steps are influenced by note-heads, so the
> interval with normal stems is _slightly_ more convenient to use there. I
notice
> that least_squares_positions() has a comment wishing it could see the
note-heads
> on the other portion(s) of a broken beam, for exactly your purposes.
>
This comment is no longer relevant (my patch fixes this issue). I didn't see it
before & I'm removing it now.
> The interface through \override Beam 'positions used to control heights at the
> positions of the first- and last- normal stems, but that wasn't documented --
> or very convenient in case of overhangs.
>
I discovered this by accident.
The real kicker is that this problem did not just plague broken beams - even
unbroken ones have an overhang from the center of the extremal stem by half the
stem width (usually 0.065 units). This is non-negligible. In
beam-quanting-32nd.ly, for example, there were several too-low quantings that
the correction of this minuscule overhang fixes.
> You make a good point that quanting should work with the ends of the full beam
> as printed including overhangs, but I assume that change is a separate commit.
>
You don't need a separate commit: one of the benefits of standardizing x extent
across functions is that this is automatically fixed.
Attached are the results from current master and from my patch:
\paper { ragged-right = ##t }
{
d'8[ c' b e' r r r r r r r r r]
}
In the patch, the functions individual-slope and peters-prolongation guarantee
quanted beams on both ends. The only one that doesn't is strict-prolongation,
as it is impossible to have beams strictly link up and quant their broken ends
(or rather, the chances are infinitesimally small).
I've included this snippet as a regtest.
>
>>> The difference between
>>> consistent_broken_slope_ and consistent_broken_slope is dangerous all by
>>> itself.
>>
>> I'm not sure what you mean - how is this dangerous?
>>
> Similar names (formerly also similar to the property 'consistent-broken-slope)
> with different values. Only dangerous if someone changing the code later is
> un-careful about distinguishing them. The new patch is better about this.
>
Good to hear! Lemme know if you have any other comments, and many thanks for
those you've given so far.
New patch-set up.
Cheers,
MS
On Oct 28, 2011, at 11:09 AM, mike@apollinemike.com wrote: > On Oct 28, 2011, at ...
13 years, 5 months ago
(2011-10-28 12:24:39 UTC)
#11
On Oct 28, 2011, at 11:09 AM, mike@apollinemike.com wrote:
> On Oct 28, 2011, at 7:30 AM, Keith OHara wrote:
>
>> On Thu, 27 Oct 2011 01:34:00 -0700, mike@apollinemike.com
<mike@apollinemike.com> wrote:
>>
>>>> What about the x_span_ of the Beam_scoring_problem ?
>>>
>>> It represents two things at two different stages of Beam_scoring_problem.
>>
>> Too bad you didn't use two different variables.
>
> I've fixed the code so that x_span_ (and all variables containing x
information) only ever represent one thing: the full x span of the beam. The
distinction between normal-stem to normal-stem versus left-extremity to
right-extremity no longer exists. This reduces the complexity (and size) of the
code without altering the visual result.
>
>>
>>> Starting at update_x_span_after_extremal_hangover_compensation (), the
x_span_
>>> becomes the real span of the beam, tacking on left and right overhang that
may
>>> come from stemlets or broken beams. This is necessary for the quanting (we
>>> always want edges quanted) but not possible for the slope calculations,
which
>>> are predicated on the idea that unquanted_y_ represents the beginning and
end
>>> of the beam (otherwise, we'd have to compensate for extremal hangover in
each
>>> function).
>>
>> You are telling me _what_ happens, which I could see from the code. Some
things
>> looked strange, possibly accidental, so I wanted to know _why_.
>>
>> I see that the slope-determining steps are influenced by note-heads, so the
>> interval with normal stems is _slightly_ more convenient to use there. I
notice
>> that least_squares_positions() has a comment wishing it could see the
note-heads
>> on the other portion(s) of a broken beam, for exactly your purposes.
>>
>
> This comment is no longer relevant (my patch fixes this issue). I didn't see
it before & I'm removing it now.
>
>> The interface through \override Beam 'positions used to control heights at
the
>> positions of the first- and last- normal stems, but that wasn't documented --
>> or very convenient in case of overhangs.
>>
>
> I discovered this by accident.
> The real kicker is that this problem did not just plague broken beams - even
unbroken ones have an overhang from the center of the extremal stem by half the
stem width (usually 0.065 units). This is non-negligible. In
beam-quanting-32nd.ly, for example, there were several too-low quantings that
the correction of this minuscule overhang fixes.
>
>> You make a good point that quanting should work with the ends of the full
beam
>> as printed including overhangs, but I assume that change is a separate
commit.
>>
>
> You don't need a separate commit: one of the benefits of standardizing x
extent across functions is that this is automatically fixed.
> Attached are the results from current master and from my patch:
>
> \paper { ragged-right = ##t }
> {
> d'8[ c' b e' r r r r r r r r r]
> }
>
> In the patch, the functions individual-slope and peters-prolongation guarantee
quanted beams on both ends. The only one that doesn't is strict-prolongation,
as it is impossible to have beams strictly link up and quant their broken ends
(or rather, the chances are infinitesimally small).
>
> I've included this snippet as a regtest.
>
>>
>>>> The difference between
>>>> consistent_broken_slope_ and consistent_broken_slope is dangerous all by
>>>> itself.
>>>
>>> I'm not sure what you mean - how is this dangerous?
>>>
>> Similar names (formerly also similar to the property
'consistent-broken-slope)
>> with different values. Only dangerous if someone changing the code later is
>> un-careful about distinguishing them. The new patch is better about this.
>>
>
> Good to hear! Lemme know if you have any other comments, and many thanks for
those you've given so far.
>
> New patch-set up.
>
> Cheers,
> MS
Sorry, forgot the attachments. The one labeled keith-example-patch is with the
most recent patchset & shows the quanting in action. Not exactly what's above
in terms of ly code, but it gets across the same idea.
Cheers,
MS
S
Thanks Keith! I've incorporated most of your comments into the code and otherwise have a ...
13 years, 5 months ago
(2011-11-01 11:41:30 UTC)
#13
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, k-ohara5a5a@oco.net 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#n...
> 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#n...
> 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...
> File input/regression/beam-broken-classic.ly (right):
>
>
http://codereview.appspot.com/5293060/diff/19014/input/regression/beam-broken...
> 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-quanti...
> File input/regression/beam-quanting-overhang.ly (right):
>
>
http://codereview.appspot.com/5293060/diff/19014/input/regression/beam-quanti...
> 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#newcod...
> 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#newcod...
> 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#newcod...
> 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#newcod...
> 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#newcod...
> 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#newcod...
> 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#newcod...
> 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#newcod...
> 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#newcod...
> 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#newcod...
> 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#newcod...
> 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
Hey all, I've decided to pull my patch for issue 1986 off the current countdown. ...
13 years, 5 months ago
(2011-11-02 09:15:09 UTC)
#14
Hey all,
I've decided to pull my patch for issue 1986 off the current countdown. After
incorporating Keith's suggestions and moving the calculations for a beam's
concaveness into Beam_scoring_problem, the patch has changed significantly
enough from yesterday to merit a new countdown.
With these additional changes there are no differences from the previous regtest
comparisons save beam-broken-classic.ly, where strict-prolongation is more
accurate (before, errors in the concaveness calculation led to slight slope
discrepancies for strict-prolongation when the slope was flat).
Lastly, there is one regtest change that's been bothering me, and I want to get
feedback on it before i push the patch. It is in chord-tremolo.ly (see
attached).
The shorter stem in the 1/4 bar doesn't bother me (I actually prefer it, but
then again, I know nothing about classical typesetting and don't have access to
any of the canonical texts), but it is not consistent with respect to the 2/4
and 3/4 versions. This comes from the fact that the 1/4 version doesn't link up
with the stem and thus has a shorter x_span_ than its 2/3 and 3/4 sistren. The
fix is easy (something like x_span_ = max (x_span_,
distance_between_normal_stems);), but I first want to make sure that this is the
right move to make. Specifically, if Ross, Reed, or Gould have anything to say,
please let me know!
Cheers,
MS
>
On Nov 1, 2011, at 7:59 PM, Keith OHara wrote: > On Tue, 01 Nov ...
13 years, 5 months ago
(2011-11-02 10:19:10 UTC)
#15
On Nov 1, 2011, at 7:59 PM, Keith OHara wrote:
> On Tue, 01 Nov 2011 04:41:22 -0700, mike@apollinemike.com
<mike@apollinemike.com> wrote:
>
>> Where is this advertising? Is it the word prolongation?
>
> Yes. When a beam grob is passed to a function called prolongation people will
at first think the function lengthens the beam.
>
>> To make things more consistent, I could call the functions
individual-breaking, strict-breaking, and peters-breaking.
>
> Do these functions break the beam?
>
> I thought they placed the beam, or a part of a broken beam, with different
methods like place-individually align-with-broken-parts
slope-like-broken-parts.
>
Renamed following your suggestions.
>
>>> 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).
>
> That's kind of what confused me. In a case where the upper-level scheme code
is trying to make beam slopes consistent, but not necessarily continuous in
height, you tell the lower-level code consistent_broken_slope=0. I see how it
works, but I would have seen it sooner if the variable name were beelzebub.
>
> My problem is that the boolean consistent_broken_slope doesn't control whether
things are consistent, and affects height in addition to slope. It works at the
detailed level, when quanting a (part of a) beam, choosing whether to align him
with his fellow broken-intos. So I renamed it in my mind as align_broken_intos
>
Renamed following your suggestion.
>> I like do_initial_slope_calculations_ because I think it reads cleaner in the
constructor
> That boolean name is fine. My comment was still talking about the interface to
Beam_scoring_problem
>
>>
>>>
http://codereview.appspot.com/5293060/diff/19014/lily/beam-quanting.cc#newcod...
>>> 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?
>>
> Well, my statement was one of confusion, because I say we /would/ need to look
harder for good positions, but the code says we do not look in this case.
> A non-confused comment would help.
>
Sorry - I'm still not quite getting what would help. Could you please suggest a
comment to put here?
>>>
>>>
http://codereview.appspot.com/5293060/diff/19014/lily/beam-quanting.cc#newcod...
>>> 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.
>>
> That must have been surprising.
'twas
> Sounds like a pre-commit independent of the main commit
>
'tis possible - I can push as 2 commits.
Cheers,
MS
On 2011/11/02 10:19:10, mike_apollinemike.com wrote: > http://codereview.appspot.com/5293060/diff/19014/lily/beam-quanting.cc#newcode743 > >>> lily/beam-quanting.cc:743: if (do_initial_slope_calculations_) > >>> Even ...
13 years, 5 months ago
(2011-11-04 07:43:07 UTC)
#17
On 2011/11/02 10:19:10, mike_apollinemike.com wrote:
>
http://codereview.appspot.com/5293060/diff/19014/lily/beam-quanting.cc#newcod...
> >>> 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?
> >>
> > Well, my statement was one of confusion, because I say we /would/ need to
look
> harder for good positions, but the code says we do not look in this case.
> > A non-confused comment would help.
> >
>
> Sorry - I'm still not quite getting what would help. Could you please suggest
a
> comment to put here?
>
I have no idea, because I remain confused.
I do not understand the purpose of the 'if' in
if (do_initial_slope_calculations_) /* make knees pretty */
It seemed to me that we need to make knees pretty whether or not we
do_initial_slope_calculations_ now, or already did_initial_slope_calculations_
on an earlier pass.
On 2011/11/04 07:43:07, Keith wrote: > On 2011/11/02 10:19:10, http://mike_apollinemike.com wrote: > > > http://codereview.appspot.com/5293060/diff/19014/lily/beam-quanting.cc#newcode743 ...
13 years, 5 months ago
(2011-11-04 22:46:44 UTC)
#18
On 2011/11/04 07:43:07, Keith wrote:
> On 2011/11/02 10:19:10, http://mike_apollinemike.com wrote:
> >
>
http://codereview.appspot.com/5293060/diff/19014/lily/beam-quanting.cc#newcod...
> > >>> 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?
> > >>
> > > Well, my statement was one of confusion, because I say we /would/ need to
> look
> > harder for good positions, but the code says we do not look in this case.
> > > A non-confused comment would help.
> > >
> >
> > Sorry - I'm still not quite getting what would help. Could you please
suggest
> a
> > comment to put here?
> >
>
> I have no idea, because I remain confused.
> I do not understand the purpose of the 'if' in
> if (do_initial_slope_calculations_) /* make knees pretty */
>
> It seemed to me that we need to make knees pretty whether or not we
> do_initial_slope_calculations_ now, or already did_initial_slope_calculations_
> on an earlier pass.
Fair' nuf. New patch set uploaded.
Issue 5293060: Fixes slope errors from incorrect X extents in Beam::print.
(Closed)
Created 13 years, 6 months ago by MikeSol
Modified 13 years, 5 months ago
Reviewers: Keith, mike_apollinemike.com, carl.d.sorensen_gmail.com, pkx166h
Base URL:
Comments: 36
