Sketch for broken beams with consistent slopes

Sketch for broken beams with consistent slopes

[MikeSol, 2011-08-25 07:29:38]

Hey all,

This patch changes the name of several variables that I'll need to work on my
beam slope stuff.  If any developers are working on beam-quanting.cc and would
like me to wait until their patch is pushed, please speak up today.  Otherwise,
I'll push it so that I can continue with my work.

Cheers,
MS

[pkx166h, 2011-08-27 08:49:50]

passes make and reg tests

[hanwenn, 2011-08-27 14:11:29]

LGTM

On Thu, Aug 25, 2011 at 4:29 AM,  <mtsolo@gmail.com> wrote:
> Reviewers: ,
>
> Message:
> Hey all,
>
> This patch changes the name of several variables that I'll need to work
> on my beam slope stuff.  If any developers are working on
> beam-quanting.cc and would like me to wait until their patch is pushed,
> please speak up today.  Otherwise, I'll push it so that I can continue
> with my work.
>
> Cheers,
> MS
>
> Description:
> Changes variable names in include/beam-scoring-problem.hh and
> beam-quanting.cc
>
> Please review this at http://codereview.appspot.com/4961041/
>
> Affected files:
>  M lily/beam-quanting.cc
>  M lily/include/beam-scoring-problem.hh
>
>
>
> _______________________________________________
> lilypond-devel mailing list
> lilypond-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/lilypond-devel
>



-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

[joeneeman, 2011-09-01 15:20:17]

http://codereview.appspot.com/4961041/diff/3001/lily/include/beam-scoring-pro...
File lily/include/beam-scoring-problem.hh (right):

http://codereview.appspot.com/4961041/diff/3001/lily/include/beam-scoring-pro...
lily/include/beam-scoring-problem.hh:114: TODO - use trailing _ on data members.
I think you can delete this TODO now.

[MikeSol, 2011-09-01 16:13:00]

Hey all,

Sorry for hijacking this issue to turn it into another one.  I'll take care of
the variable name stuff in a second patch.

My summer of lily comes to a close with this monstrosity.  Some gotchyas:

1)  Do not compile the regtests against this: they will break, as several parts
of Lily and several regtests use callbacks that I've deleted (I don't have time
to fix that before I leave).  Instead, copy and paste any regtest with the word
beam in it into a directory and compile those to get a sense of what this patch
does.

2)  This patch currently breaks cross-staff beam nice slope finding (it'll
probably take me an hour-ish to figure out why and fix it).  Other than that, I
believe that it leaves other slopes in tact.

3)  You'll see that, by approaching beam slope this way, a lot of code
duplication that arose from chaining functions together is avoided.

4)  It'd be nice to have more callbacks to fine tune how Beam_scoring_problem
does its thing as it is now gimungous.

5) Most importantly, apply this patch and compile:

\version "2.15.10"

\relative c' {
\override Beam #'breakable = ##t
a8 [ b c d e f g \bar "" \break f e d c b a ]
}

\relative c' {
\override Beam #'breakable = ##t
\override Beam #'consistent-broken-slope = ##t
a8 [ b c d e f g \bar "" \break f e d c b a ]
}

\relative c' {
\override Beam #'breakable = ##t
a8 [ b c d e f \bar "" \break a c e g b ]
}

\relative c' {
\override Beam #'breakable = ##t
\override Beam #'consistent-broken-slope = ##t
a8 [ b c d e f \bar "" \break a c e g b ]
}

And there ya have it!

Cheers,
MS

P.S. Sorry for the generally kludgy nature of some aspects of this patch, which
may or may not include wholesale deletion of certain things that are actually
important.  This will be addressed in later patchsets - this was thrown together
in a rush :)

[mikesol_ufl.edu, 2011-09-01 16:22:12]

On Sep 1, 2011, at 6:13 PM, mtsolo@gmail.com wrote:

> Other than that, I believe that it leaves other slopes in tact.
> 

intact

Cheers,
MS

[MikeSol, 2011-09-01 19:00:12]

Update: looks like cross-staff beams are broken because I removed all of the
commony code that takes care of cross staff beams.  D'oh!  Will put back in in a
way that preserves the old cross-staff beam slopes while allowing them to work
with the broken beam code.

Cheers,
MS

[MikeSol, 2011-09-18 18:19:28]

Hey all,

This is now in a state where people can run regtests on it and comment upon it.

There are a few things in the regtests that I still need to work on:

-) my feathered beam regtest has more vertical spacing between systems...I still
need to figure out why
-) i think the change in spanner-break-overshoot is kinda cool, but i need to
figure out a way to scrub the warning message.
-) beam-slope-stemlet, perennial thorn in my side, shows a change that i need to
scrub.

What I'd appreciate from people at this stage of the game are four things:

1) Regtest running to confirm that my checks are valid.
2) General comments on design (properties I could add to make it more tweakable,
etc.).
3) A critical eye at dividing by staff_space.  You'll see that in
beam-quanting.cc, there are several Real values that are divided by staff_space.
 I've avoided doing this in all the new stuff I've added, and it seems to have
had no effect, but if someone could create a good test file I could use to check
the effects of staff space on beam positions, I'd be much obliged.  Or,
alternatively, for someone who gets staff_space better than I do, if they spot
any obvious problems, please let me know.
4) Figuring out cases that break this code.  I'm sure that crazy broken cross
staff beams lead to odd output, but I'll deal with esoteric cross-staff stuff in
another patch if I need to.  Mostly, please try to think of odd cases that may
cause my work to segfault or return bad beams.

Thank you all!

Cheers,
MS

[MikeSol, 2011-09-19 09:14:59]

All of the previously-reported bugs are now squashed with this newest patchset:
the regtests only show two changes that results in things moving (and one change
that shows nothing moving but a lot of green...weird), both of which seem to be
welcome side effects of the code consolidation in beam-quanting.

Cheers,
MS

[pkx166h, 2011-09-24 19:57:04]

Passes make but there are a few reg tests that someone should look at

See

http://code.google.com/p/lilypond/issues/detail?id=1844#c5

[mikesol_ufl.edu, 2011-09-25 03:44:36]

On Sep 24, 2011, at 9:57 PM, pkx166h@gmail.com wrote:

> Passes make but there are a few reg tests that someone should look at
> 
> See
> 
> http://code.google.com/p/lilypond/issues/detail?id=1844#c5


The only thing that I'm not sure about is beam-slope-stemlet.ly.  What do people
think of this compared to the old version?

Cheers,
MS

[MikeSol, 2011-10-03 04:05:09]

Hey all,

I've already heard back from Neil about this patch - he's gonna have a look at
it if/when he has a spare moment (he's busy these days).

I would really appreciate some review on this - please give it a look over if
you have some time!

Cheers,
MS

[hanwenn, 2011-10-03 04:53:48]

i know it's annoying, but could you separate out the cosmetics (adding _ ) to
members from the rest of this change?  The cosmetic changes make it difficult to
see the essence of what you are trying to do.

http://codereview.appspot.com/4961041/diff/39001/lily/include/beam-scoring-pr...
File lily/include/beam-scoring-problem.hh (right):

http://codereview.appspot.com/4961041/diff/39001/lily/include/beam-scoring-pr...
lily/include/beam-scoring-problem.hh:151: vector<Real> stem_ypositions_;
organize so it's clear to what members the comment pertains.

[pkx166h, 2011-10-03 07:20:33]

Passes make. I get a few reg tests pop up; see attached at

http://code.google.com/p/lilypond/issues/detail?id=1952#c1

but also

'spanner-break-overshoot.log'

gives me:

--snip--

@@ -4,8 +4,6 @@
 Preprocessing graphical objects...
 Calculating line breaks... 
 Drawing systems... 
-programming error: Disagree on common x. Skipping collisions in beam scoring.
-continuing, cross fingers
 Writing header field `texidoc' to
`/home/jlowe/lilypond-git/build/out/lybook-testdb/2f/lily-050456a8.texidoc'...
 Writing
/home/jlowe/lilypond-git/build/out/lybook-testdb/2f/lily-050456a8-1.signature
 Writing
/home/jlowe/lilypond-git/build/out/lybook-testdb/2f/lily-050456a8-2.signature

--snip--

is this bad?

[mikesol_ufl.edu, 2011-10-03 18:50:27]

On Oct 3, 2011, at 6:53 AM, hanwenn@gmail.com wrote:

> i know it's annoying, but could you separate out the cosmetics (adding _
> ) to members from the rest of this change?  The cosmetic changes make it
> difficult to see the essence of what you are trying to do.
> 
> 
>
http://codereview.appspot.com/4961041/diff/39001/lily/include/beam-scoring-pr...
> File lily/include/beam-scoring-problem.hh (right):
> 
>
http://codereview.appspot.com/4961041/diff/39001/lily/include/beam-scoring-pr...
> lily/include/beam-scoring-problem.hh:151: vector<Real> stem_ypositions_;
> organize so it's clear to what members the comment pertains.
> 
> http://codereview.appspot.com/4961041/

It's good, as it eliminates the programming error and makes the line-break
slicker (IMHO).

Cheers,
MS

[mikesol_ufl.edu, 2011-10-03 18:51:01]

On Oct 3, 2011, at 6:53 AM, hanwenn@gmail.com wrote:

> i know it's annoying, but could you separate out the cosmetics (adding _
> ) to members from the rest of this change?  The cosmetic changes make it
> difficult to see the essence of what you are trying to do.
> 
> 
>
http://codereview.appspot.com/4961041/diff/39001/lily/include/beam-scoring-pr...
> File lily/include/beam-scoring-problem.hh (right):
> 
>
http://codereview.appspot.com/4961041/diff/39001/lily/include/beam-scoring-pr...
> lily/include/beam-scoring-problem.hh:151: vector<Real> stem_ypositions_;
> organize so it's clear to what members the comment pertains.
> 
> http://codereview.appspot.com/4961041/

Doable.
I'll push the cosmetic to master after running regtests on it and then try to
sort out the patch either tonight or tomorrow morning.

Cheers,
MS

[mikesol_ufl.edu, 2011-10-04 09:57:55]

On Oct 3, 2011, at 8:50 PM, Mike Solomon wrote:

> On Oct 3, 2011, at 6:53 AM, hanwenn@gmail.com wrote:
> 
>> i know it's annoying, but could you separate out the cosmetics (adding _
>> ) to members from the rest of this change?  The cosmetic changes make it
>> difficult to see the essence of what you are trying to do.
>> 
>> 
>>
http://codereview.appspot.com/4961041/diff/39001/lily/include/beam-scoring-pr...
>> File lily/include/beam-scoring-problem.hh (right):
>> 
>>
http://codereview.appspot.com/4961041/diff/39001/lily/include/beam-scoring-pr...
>> lily/include/beam-scoring-problem.hh:151: vector<Real> stem_ypositions_;
>> organize so it's clear to what members the comment pertains.
>> 
>> http://codereview.appspot.com/4961041/
> 
> Doable.
> I'll push the cosmetic to master after running regtests on it and then try to
sort out the patch either tonight or tomorrow morning.
> 
> Cheers,
> MS

Hey all,

The cosmetic stuff is pushed to current master and I've posted a new slope patch
on Rietveld that applies cleanly to current master.

The only concern I have is that, running regtests this morning, I am getting
sporadic differences in graphviz.log.  These have appeared since I pushed the
cosmetic patch.  Does anyone know where these could be coming from?  Perhaps an
uninitialized variable?  Everything else builds cleanly with no warnings, but
for some reason, this is persistent.

Cheers,
MS

[hanwenn, 2011-10-04 13:26:46]

You skipped the cosmetic patch that folds together the (SCM, SCM)
callbacks into one big quanting callback.

I assume this is the real patch,

+  if (consistent_broken_slope_)
+    {
+      Interval normalized_endpoints = robust_scm2interval
(beam_->get_property ("normalized-endpoints"), Interval (0, 1));
+      Real y_length = final_positions[RIGHT] - final_positions[LEFT];
+
+      final_positions[LEFT] += normalized_endpoints[LEFT] * y_length;
+      final_positions[RIGHT] -= (1 - normalized_endpoints[RIGHT]) * y_length;
+    }


am I correct?


On Tue, Oct 4, 2011 at 6:57 AM, Mike Solomon <mikesol@ufl.edu> wrote:

> Hey all,
>
> The cosmetic stuff is pushed to current master and I've posted a new slope
patch on Rietveld that applies cleanly to current master.
>
> The only concern I have is that, running regtests this morning, I am getting
sporadic differences in graphviz.log.  These have appeared since I pushed the
cosmetic patch.  Does anyone know where these could be coming from?  Perhaps an
uninitialized variable?  Everything else builds cleanly with no warnings, but
for some reason, this is persistent.

graphviz draws dependencies between grobs, so if you change the
internal dependencies of properties, it may shift things around.


-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

[mikesol_ufl.edu, 2011-10-04 13:31:01]

On Oct 4, 2011, at 3:26 PM, Han-Wen Nienhuys wrote:

> You skipped the cosmetic patch that folds together the (SCM, SCM)
> callbacks into one big quanting callback.
> 

You and I have different definitions of cosmetics.  Living with a French woman,
I am constantly told that I know nothing about cosmetics, so this does not
surprise me.
That said, the "one big callback" thing is not doable without the giant rewrite
attached to it, because in doing so, lots of subtle tweaks have to be made to
the functions from beam.cc so that they are less reliant on the beam grob and
more reliant on vectors of information.  So, I believe that the current
patchset, as it stands, is reviewable.

Cheers,
MS

> I assume this is the real patch,
> 
> +  if (consistent_broken_slope_)
> +    {
> +      Interval normalized_endpoints = robust_scm2interval
> (beam_->get_property ("normalized-endpoints"), Interval (0, 1));
> +      Real y_length = final_positions[RIGHT] - final_positions[LEFT];
> +
> +      final_positions[LEFT] += normalized_endpoints[LEFT] * y_length;
> +      final_positions[RIGHT] -= (1 - normalized_endpoints[RIGHT]) * y_length;
> +    }
> 
> 
> am I correct?
> 
> 
> On Tue, Oct 4, 2011 at 6:57 AM, Mike Solomon <mikesol@ufl.edu> wrote:
> 
>> Hey all,
>> 
>> The cosmetic stuff is pushed to current master and I've posted a new slope
patch on Rietveld that applies cleanly to current master.
>> 
>> The only concern I have is that, running regtests this morning, I am getting
sporadic differences in graphviz.log.  These have appeared since I pushed the
cosmetic patch.  Does anyone know where these could be coming from?  Perhaps an
uninitialized variable?  Everything else builds cleanly with no warnings, but
for some reason, this is persistent.
> 
> graphviz draws dependencies between grobs, so if you change the
> internal dependencies of properties, it may shift things around.
> 
> 
> -- 
> Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

[mikesol_ufl.edu, 2011-10-04 14:48:41]

On Oct 4, 2011, at 3:26 PM, Han-Wen Nienhuys wrote:

> You skipped the cosmetic patch that folds together the (SCM, SCM)
> callbacks into one big quanting callback.
> 
> I assume this is the real patch,
> 
> +  if (consistent_broken_slope_)
> +    {
> +      Interval normalized_endpoints = robust_scm2interval
> (beam_->get_property ("normalized-endpoints"), Interval (0, 1));
> +      Real y_length = final_positions[RIGHT] - final_positions[LEFT];
> +
> +      final_positions[LEFT] += normalized_endpoints[LEFT] * y_length;
> +      final_positions[RIGHT] -= (1 - normalized_endpoints[RIGHT]) * y_length;
> +    }
> 
> 
> am I correct?
> 
> 
> On Tue, Oct 4, 2011 at 6:57 AM, Mike Solomon <mikesol@ufl.edu> wrote:
> 
>> Hey all,
>> 
>> The cosmetic stuff is pushed to current master and I've posted a new slope
patch on Rietveld that applies cleanly to current master.
>> 
>> The only concern I have is that, running regtests this morning, I am getting
sporadic differences in graphviz.log.  These have appeared since I pushed the
cosmetic patch.  Does anyone know where these could be coming from?  Perhaps an
uninitialized variable?  Everything else builds cleanly with no warnings, but
for some reason, this is persistent.
> 
> graphviz draws dependencies between grobs, so if you change the
> internal dependencies of properties, it may shift things around.
> 

What's odd is that the chart didn't change - just the order that things were
printed.  As this normally shouldn't change in a deterministic system, I wanted
to give the heads up.  Lemme know if others spot the same fishy behavior.

Cheers,
MS

[hanwenn, 2011-10-04 17:47:12]

On Tue, Oct 4, 2011 at 10:30 AM, Mike Solomon <mikesol@ufl.edu> wrote:
> On Oct 4, 2011, at 3:26 PM, Han-Wen Nienhuys wrote:
>
>> You skipped the cosmetic patch that folds together the (SCM, SCM)
>> callbacks into one big quanting callback.
>>
>
> You and I have different definitions of cosmetics.  Living with a French
woman, I am constantly told that I know nothing about cosmetics, so this does
not surprise me.

:)

> That said, the "one big callback" thing is not doable without the giant
rewrite attached to it, because in doing so, lots of
subtle tweaks have to be made to the functions from beam.cc so that
they are less reliant on the beam grob and more reliant on vectors of
information.  So, I believe that the current patchset, as it stands,
is reviewable.

Right; the confusing thing is that the bulk of the change is not about
consistent beams, but about reorganizing code.

In general,  I always to try to do the reorganization (that does not
change functionality) first and separately, and then do the new
feature in a new change

-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

[pkx166h, 2011-10-05 07:21:58]

passes make but I get some reg tests show up.

See

http://code.google.com/p/lilypond/issues/detail?id=1844#c11

[mikesol_ufl.edu, 2011-10-05 08:20:17]

On Oct 4, 2011, at 7:47 PM, Han-Wen Nienhuys wrote:

> On Tue, Oct 4, 2011 at 10:30 AM, Mike Solomon <mikesol@ufl.edu> wrote:
>> On Oct 4, 2011, at 3:26 PM, Han-Wen Nienhuys wrote:
>> 
>>> You skipped the cosmetic patch that folds together the (SCM, SCM)
>>> callbacks into one big quanting callback.
>>> 
>> 
>> You and I have different definitions of cosmetics.  Living with a French
woman, I am constantly told that I know nothing about cosmetics, so this does
not surprise me.
> 
> :)
> 
>> That said, the "one big callback" thing is not doable without the giant
rewrite attached to it, because in doing so, lots of
> subtle tweaks have to be made to the functions from beam.cc so that
> they are less reliant on the beam grob and more reliant on vectors of
> information.  So, I believe that the current patchset, as it stands,
> is reviewable.
> 
> Right; the confusing thing is that the bulk of the change is not about
> consistent beams, but about reorganizing code.
> 
> In general,  I always to try to do the reorganization (that does not
> change functionality) first and separately, and then do the new
> feature in a new change
> 

I've pushed another cosmetic patch (only code moving, no difference in regtests
save a graphviz change...grr...).

The diff is now more readable on Rietveld.  The only other cosmetic change I
could make is lumping together init_stems and init_collisions in current mater
(as they are currently lumped together in the patch), which would improve
readability.  Lemme know if you want me to do this.  Otherwise, it is good to
go.

Cheers,
MS

[pkx166h, 2011-10-06 19:16:44]

Passes make, make check gives me the usual suspects on the reg test

http://code.google.com/p/lilypond/issues/detail?id=1952#c3

 but I still get;

input/regression/spanner-break-overshoot.log

--snip--


@@ -4,8 +4,6 @@
 Preprocessing graphical objects...
 Calculating line breaks... 
 Drawing systems... 
-programming error: Disagree on common x. Skipping collisions in beam scoring.
-continuing, cross fingers
 Writing header field `texidoc' to
`/home/jlowe/lilypond-git/build/out/lybook-testdb/2f/lily-050456a8.texidoc'...
 Writing
/home/jlowe/lilypond-git/build/out/lybook-testdb/2f/lily-050456a8-1.signature
 Writing
/home/jlowe/lilypond-git/build/out/lybook-testdb/2f/lily-050456a8-2.signature

--snip--

[Neil Puttock, 2011-10-20 19:24:44]

http://codereview.appspot.com/4961041/diff/60001/Documentation/changes.tely
File Documentation/changes.tely (right):

http://codereview.appspot.com/4961041/diff/60001/Documentation/changes.tely#n...
Documentation/changes.tely:66: \override Beam #'breakable = ##t
no indent

http://codereview.appspot.com/4961041/diff/60001/Documentation/changes.tely#n...
Documentation/changes.tely:68: a8 [ b c d e f g \bar "" \break f e d c b a ]
a8[
a]

http://codereview.appspot.com/4961041/diff/60001/Documentation/changes.tely#n...
Documentation/changes.tely:78: the beam grob. All Scheme functions that used
these callbacks are now
two spaces after full stop/period

http://codereview.appspot.com/4961041/diff/60001/Documentation/changes.tely#n...
Documentation/changes.tely:79: simplified.  For examples, see
@file{input/regression/beam-concave.ly}
You shouldn't reference regression tests here.  Will frighten users. :)

http://codereview.appspot.com/4961041/diff/60001/input/regression/beam-consis...
File input/regression/beam-consistent-broken-slope.ly (right):

http://codereview.appspot.com/4961041/diff/60001/input/regression/beam-consis...
input/regression/beam-consistent-broken-slope.ly:2: \version "2.15.12"
2.15.15

http://codereview.appspot.com/4961041/diff/60001/input/regression/beam-consis...
input/regression/beam-consistent-broken-slope.ly:12: \override Beam #'breakable
= ##t
indent, code style

http://codereview.appspot.com/4961041/diff/60001/input/regression/beam-consis...
input/regression/beam-consistent-broken-slope.ly:13: a8 [ b c d e f g \bar ""
\break f e d c b a ]
a8[
a]

etc.

http://codereview.appspot.com/4961041/diff/60001/scm/define-grob-properties.scm
File scm/define-grob-properties.scm (right):

http://codereview.appspot.com/4961041/diff/60001/scm/define-grob-properties.s...
scm/define-grob-properties.scm:749: (skip-quanting ,boolean? "Should beam
quanting be skipped?")
Is this likely to be used outside regression testing?

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

http://codereview.appspot.com/4961041/diff/60001/scm/define-grobs.scm#newcode386
scm/define-grobs.scm:386: (positions .  ,ly:beam::quanting)
remove extra space

http://codereview.appspot.com/4961041/diff/60001/scm/layout-beam.scm
File scm/layout-beam.scm (right):

http://codereview.appspot.com/4961041/diff/60001/scm/layout-beam.scm#newcode65
scm/layout-beam.scm:65: grob
indent

http://codereview.appspot.com/4961041/diff/60001/scm/layout-beam.scm#newcode72
scm/layout-beam.scm:72: grob
indent

[mikesol_ufl.edu, 2011-10-21 06:50:18]

Thanks Neil!

On Oct 20, 2011, at 9:24 PM, n.puttock@gmail.com wrote:

>
http://codereview.appspot.com/4961041/diff/60001/scm/define-grob-properties.scm
> File scm/define-grob-properties.scm (right):
> 
>
http://codereview.appspot.com/4961041/diff/60001/scm/define-grob-properties.s...
> scm/define-grob-properties.scm:749: (skip-quanting ,boolean? "Should
> beam quanting be skipped?")
> Is this likely to be used outside regression testing?

Of this I am not sure.  My gut says yes, but I don't know why the regtest that
skips quanting was added and the extent to which quantless-beams are used by
users.

Cheers,
MS

[Neil Puttock, 2011-10-21 11:20:00]

On 21 October 2011 07:50, Mike Solomon <mikesol@ufl.edu> wrote:

> Of this I am not sure.  My gut says yes, but I don't know why the regtest that
skips quanting was added and the extent to which quantless-beams are used by
users.

Never, I'd imagine.  I certainly don't recall any users wanting to
switch it off.  I'm just thinking it would be better off being an
internal property.

I might have a few more comments once I've got Ubuntu running again.

Cheers,
Neil

[Keith, 2011-10-26 18:57:38]

Rather indecipherable.

The 350 lines of code re-ordering make it difficult to find the change in
function.  Therefore I am not willing to say LGTM, and looks like nobody else
was, either.

It looks like the intent is, if consistent_broken_slope=#t, to join the parts of
a broken beam together and present the combination to Beam_scoring_problem,
but...

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

http://codereview.appspot.com/4961041/diff/60001/lily/beam-quanting.cc#newcod...
lily/beam-quanting.cc:202: for (vsize i = 0; i < beams.size (); i++)
When there is more than one beam in 'beams' what do you intend the members
Beam_scoring_problem to represent ?  The beam as if it were on one line, but
allowing the extra horizontal span due to the line-break ?  Should the members
like segments_ and is_knee_ include both halves of the beam ?

http://codereview.appspot.com/4961041/diff/60001/lily/beam-quanting.cc#newcod...
lily/beam-quanting.cc:947: final_positions[RIGHT] -= (1 -
normalized_endpoints[RIGHT]) * y_length;
At what horizontal locations are these final_positions? First and last stems or
the end of the beam including overhang or x_span_ ?

[MikeSol, 2011-10-26 19:18:07]

Thanks for the comments!
Just a technical note - this issue was closed after the patch was pushed to
current master, but I don't mind opening it again to continue a discussion about
what this patch (if Colin has no objections).

The code I'm dealing with in beam-quanting and beam is difficult to decipher to
begin with: it took me at least a year to get my head fully around what it's
doing and to figure out where the gaps were.  I know there is a lot of
reorganization, but I wouldn't do it unless I felt that it was necessary to make
the code perform more consistently (i.e. to standardize the measuring of X
extent).

Cheers,
MS

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

http://codereview.appspot.com/4961041/diff/60001/lily/beam-quanting.cc#newcod...
lily/beam-quanting.cc:202: for (vsize i = 0; i < beams.size (); i++)
On 2011/10/26 18:57:38, Keith wrote:
> When there is more than one beam in 'beams' what do you intend the members
> Beam_scoring_problem to represent ?  The beam as if it were on one line, but
> allowing the extra horizontal span due to the line-break ?  Should the members
> like segments_ and is_knee_ include both halves of the beam ?

All of Beam_scoring_problem happens in an artificial space as if there was no
line break (which is why this function is so long - you'll see x_span_ and
x_left being used quite a bit to bring elements to their positions on this
imaginary line).  So, Beam_scoring_problem does not know what line breaking is,
and it is only at the very end that the normalized endpoints are used to take
care of the breaks.

One of the reasons for my most recent patch is that the normalized endpoints of
the beam don't sync up with its actual extent, which is only reliably derived
from the segments.

http://codereview.appspot.com/4961041/diff/60001/lily/beam-quanting.cc#newcod...
lily/beam-quanting.cc:947: final_positions[RIGHT] -= (1 -
normalized_endpoints[RIGHT]) * y_length;
On 2011/10/26 18:57:38, Keith wrote:
> At what horizontal locations are these final_positions? First and last stems
or
> the end of the beam including overhang or x_span_ ?

Including overhang.  This is improved in the most recent patch set.