Re: Fixing issue 37 with extra position callback (issue3928041)

[mtsolo]

I made a lot of changes to get things looking more lilypond-y.

I also now have it better handling scenarios where the subdivisions are
wacky.  Check out:

\relative c' { << { b32 [ c,16 c'8 d,64 e'8. c,32 e'' ] } \\ { f8 [ c32
d'32 bes,,64 c'64 c64 c'128 ] } >> }

The low D is snug as a bug in a rug, as is the high C.

Cheers,
MS


http://codereview.appspot.com/3928041/diff/30001/input/regression/beam-collision.ly
File input/regression/beam-collision.ly (right):

http://codereview.appspot.com/3928041/diff/30001/input/regression/beam-collision.ly#newcode1
input/regression/beam-collision.ly:1: \version "2.13.46"
On 2011/01/23 17:53:45, Graham Percival wrote:
> 2.13.47

Done.

http://codereview.appspot.com/3928041/diff/30001/input/regression/beam-collision.ly#newcode2
input/regression/beam-collision.ly:2: \header{
On 2011/01/23 17:46:58, Neil Puttock wrote:
> \header {

Done.

http://codereview.appspot.com/3928041/diff/30001/input/regression/beam-collision.ly#newcode3
input/regression/beam-collision.ly:3: texidoc="@cindex Beam Collisions
On 2011/01/23 17:46:58, Neil Puttock wrote:
> indent

Done.

http://codereview.appspot.com/3928041/diff/30001/input/regression/beam-collision.ly#newcode12
input/regression/beam-collision.ly:12: \time 4/4
On 2011/01/23 17:46:58, Neil Puttock wrote:
> indent

Done.

http://codereview.appspot.com/3928041/diff/30001/input/regression/beam-collision.ly#newcode13
input/regression/beam-collision.ly:13: << { s128 e [ s cis, ] } \\ { b''
[ s b ] } >> r8..
On 2011/01/23 17:46:58, Neil Puttock wrote:
> e[

> etc.

Done.

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver.cc
File lily/beam-collision-engraver.cc (right):

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver.cc#newcode24
lily/beam-collision-engraver.cc:24: #include "item.hh"
On 2011/01/23 17:46:58, Neil Puttock wrote:
> resort alphabetically

Done.

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver.cc#newcode45
lily/beam-collision-engraver.cc:45: void process_acknowledged ();
On 2011/01/23 17:46:58, Neil Puttock wrote:
> remove

Done.

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver.cc#newcode52
lily/beam-collision-engraver.cc:52:
Beam_collision_engraver::stop_translation_timestep()
On 2011/01/23 17:46:58, Neil Puttock wrote:
> timestep ()

Done.

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver.cc#newcode54
lily/beam-collision-engraver.cc:54: for (vsize i=0; i <
covered_interior_grobs_.size (); i++)
On 2011/01/23 17:46:58, Neil Puttock wrote:
> i = 0 (etc.)

> Why use a separate vector?  There's no difference in processing, so it
would
> make more sense to add all the grobs to covered_grobs_.

Key signatures, time signatures, and clefs that come in at the same
timestep as the beam's beginning will not come under the beam.  Thus,
they need to be treated differently than noteheads, which could
intersect with a beam that begins during their timestep.

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver.cc#newcode70
lily/beam-collision-engraver.cc:70: in auto beaming, end beams are
signaled with their beams at a later timestep.
On 2011/01/23 17:46:58, Neil Puttock wrote:
> acknowledge manual beams only instead, then there's no need for this
hack

OK - but as far as I know, there is no way to have DECLARE_ACKNOWLEDGER
(manual_beam) .  What would be the appropriate way to do this?

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver.cc#newcode96
lily/beam-collision-engraver.cc:96: active_beams_.erase
(active_beams_.begin() + j);
On 2011/01/23 17:46:58, Neil Puttock wrote:
> begin ()

Done.

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver.cc#newcode103
lily/beam-collision-engraver.cc:103:
Beam_collision_engraver::Beam_collision_engraver() {}
On 2011/01/23 17:46:58, Neil Puttock wrote:
> engraver ()

Done.

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver.cc#newcode106
lily/beam-collision-engraver.cc:106:
Beam_collision_engraver::process_acknowledged() {}
On 2011/01/23 17:46:58, Neil Puttock wrote:
> remove

Done.

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver.cc#newcode115
lily/beam-collision-engraver.cc:115:
Beam_collision_engraver::acknowledge_accidental (Grob_info i)
On 2011/01/23 17:46:58, Neil Puttock wrote:
> Since you're acknowledging noteheads, they already cache accidentals,
so you
> could extract them later.

True - I was imagining the scenario where an accidental somehow shows up
w/o a notehead.  If this is inconceivable, I can change the code.

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc
File lily/beam.cc (right):

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode972
lily/beam.cc:972: sw[LEFT] = covered_grob->relative_coordinate (commonx,
X_AXIS) + stencil->extent(X_AXIS)[LEFT] - x0;
On 2011/01/23 18:05:39, hanwenn wrote:
> why not use covered_grob->extent(common_x, X)[LEFT] instead?

> the same for all other stencil->extent manipulations.

Done.

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode973
lily/beam.cc:973: sw[RIGHT] = covered_grob->relative_coordinate
(commony, Y_AXIS) + stencil->extent(Y_AXIS)[DOWN] - pos[LEFT];
On 2011/01/23 18:05:39, hanwenn wrote:
> * what is sw ?  can you rename to something more explanatory?

> * you are mixing X and Y in an interval.  Shouldnt sw be an Offset ?

> * Why is this code not symmetric in UP and DOWN, using flip() ?  This
looks
> error prone wrt collisions that work for upstems, but not for down
stems and
> vice-versa

Fixed namings to better reflect what's actually going on.

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode974
lily/beam.cc:974: width = stencil->extent(X_AXIS)[RIGHT] -
stencil->extent(X_AXIS)[LEFT];
On 2011/01/23 17:46:58, Neil Puttock wrote:
> = extent (X_AXIS).length ()

Done.

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode979
lily/beam.cc:979: height = stencil->extent(Y_AXIS)[UP] -
stencil->extent(Y_AXIS)[DOWN];
On 2011/01/23 17:46:58, Neil Puttock wrote:
> = extent (Y_AXIS).length ()

Done.

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode1014
lily/beam.cc:1014: pos[RIGHT] += offset;
On 2011/01/23 18:11:08, hanwenn wrote:
> also, these offsets disregard carefully computed beam quantizations.
Even though
> these are floats, the positions are not continuously variable.

I don't quite get what you mean - the offsets tack on extra space iff
there is a collision.  It keeps all of the quanting data.

I hope that my clearer code will assuage your fears that this affects
anything other than certain limited scenarios - in the regtests, it
eliminates collisions in certain regtests that have them and leaves
everything else alone.

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode1015
lily/beam.cc:1015: return ly_interval2scm (pos);
On 2011/01/23 18:05:39, hanwenn wrote:
> it looks like this only handles the 1st collision found, and exits
after
> circumventing the first one.  The risks are

> * in case of multiple grobs, the resolution is dependent on the order
of
> constructing the covered grobs list.  Unpredictable and arbitrary.

> * that you will park the beam on top of something else, like the next
grob you
> are skipping with this return.

> * in the last case, you may even create a beam with enormous stems
(ugly) that
> still has a collision.

> Why not calculate all the locations of all covered grobs, and work
collisions
> into the scores for beam positions?

> See beam-quanting.cc; you'd have to add another scoring pass to
> Beam::quanting().

It does not disregard other collisions (check out the regtest).

I don't know beam quanting well enough - someone who knows how to do
this better than I could integrate it in there.  For now, I believe that
this method works well.

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode1015
lily/beam.cc:1015: return ly_interval2scm (pos);
On 2011/01/23 18:05:39, hanwenn wrote:
> it looks like this only handles the 1st collision found, and exits
after
> circumventing the first one.  The risks are

> * in case of multiple grobs, the resolution is dependent on the order
of
> constructing the covered grobs list.  Unpredictable and arbitrary.

> * that you will park the beam on top of something else, like the next
grob you
> are skipping with this return.

> * in the last case, you may even create a beam with enormous stems
(ugly) that
> still has a collision.

> Why not calculate all the locations of all covered grobs, and work
collisions
> into the scores for beam positions?

> See beam-quanting.cc; you'd have to add another scoring pass to
> Beam::quanting().

This is not the case.

Try:

\relative c' {
  \time 2/4
  << { \times 4/5 { s32 e[ s g s g, s e'' s b'] } } \\
     { \times 4/5 { c,,32[ s c'' s g, s b s d'' s] } } >>
}

and you'll see.

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode1015
lily/beam.cc:1015: return ly_interval2scm (pos);
On 2011/01/23 18:12:57, hanwenn wrote:
> On 2011/01/23 18:05:39, hanwenn wrote:
> > it looks like this only handles the 1st collision found, and exits
after
> > circumventing the first one.  The risks are
> >
> > * in case of multiple grobs, the resolution is dependent on the
order of
> > constructing the covered grobs list.  Unpredictable and arbitrary.
> >
> > * that you will park the beam on top of something else, like the
next grob you
> > are skipping with this return.
> >
> > * in the last case, you may even create a beam with enormous stems
(ugly) that
> > still has a collision.
> >
> > Why not calculate all the locations of all covered grobs, and work
collisions
> > into the scores for beam positions?
> >
> > See beam-quanting.cc; you'd have to add another scoring pass to
> > Beam::quanting().

> doing it with beam quant scoring instead will free you of getting the
symmetry
> wrt up/down correct (which has been tricky in my experience).

I do not believe the symmetry to be an issue.

\relative c' {
  \time 2/4
  << { \times 4/5 { s32 e[ s g s g, s e, s e'''] } } \\
     { \times 4/5 { c''32[ s c'' s g, s b s d'' s] } } >>
}

(that is crazy...yes...but I think it yields a great result w/ the
proposed patch!)

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

http://codereview.appspot.com/3928041/diff/30001/scm/define-grob-properties.scm#newcode972
scm/define-grob-properties.scm:972: (covered-grobs ,ly:grob-array? "Note
heads that could potentially
On 2011/01/23 17:46:58, Neil Puttock wrote:
> Grobs

Done.

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

http://codereview.appspot.com/3928041/diff/30001/scm/define-grobs.scm#newcode380
scm/define-grobs.scm:380: ly:beam::move-to-avoid-collisions
On 2011/01/23 17:46:58, Neil Puttock wrote:
> fix indent (tabs)

Done.

http://codereview.appspot.com/3928041/