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

[hanwenn]

Hi Carl,

thanks for diving into this!

I love the idea of handling beam collisions, but I think the design of
the backend code is flawed.  See my comments.


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;
why not use covered_grob->extent(common_x, X)[LEFT] instead?

the same for all other stencil->extent manipulations.

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];
* 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

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode1015
lily/beam.cc:1015: return ly_interval2scm (pos);
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().

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