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

[Han-Wen Nienhuys]

On Mon, Jan 24, 2011 at 12:58 AM,  <address@hidden> 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?

Look at the cause of the beam grob. If it is a BeamEvent, it is manual.



> 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 am talking about the case that there is a collision.  Look at Beam::quanting,

  Real straddle = 0.0;
  Real sit = (beam_thickness - slt) / 2;
  Real inter = 0.5;
  Real hang = 1.0 - (beam_thickness - slt) / 2;
  Real quants [] = {straddle, sit, inter, hang };

as you can see, for each end of the beam, there are 4 allowed
positions per staff space (with the inter being forbidden if the end
of the beam is inside the staff, iirc).

In the case of a collision, your code adds an amount derived from

  covered_grob->extent(Y)

which is an arbitrary amount.  Such offsets could cause horizontal
beams to appear like

  ========
  ========  beam
  ========
                   tiny sliver of whitespace that should not be there.
  ======== staffline

for sloped beams, unquanted positions are not desirable either.

> I hope that my clearer code will assuage your fears that this affects
> anything other than certain limited scenarios - in the regtests, it

that is not so much my fear.  I am concerned with putting in code that
we know is flawed in advance, when we have a much better mechanism to
deal with this (see below).


> 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).

Let me try to cook up some examples.

> 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.

The quanting code is not that difficult to understand; it is actually
a lot easier to follow than most of the collision code, as it is
declarative: the code gives scores to configurations, and then there
is a separate step that picks the one with the minimum badness score.

You already have written most of the logic to implement the scoring version.

It requires a function that takes a (left-y, right-y) pair and
determines if the beam produced collides with a grobs. There is no
need to calculate how much to shift anything, and the scoring will
have the possibility to tilt the beam to avoid collisions, or to
shorten stems relative to the default; something that you are
foregoing by just shifting in the beam-direction.

> 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
>


I am sorry, I misread this code here.  You are completely right that
it handles multiple collisions.


> symmetry
>>
>> wrt up/down correct (which has been tricky in my experience).
>
> I do not believe the symmetry to be an issue.

Can you try make use of symmetry anyway, to keep our code base clean and tight?

+          if (dir == UP)
+            offset = max (offset, temp_offset);
+          else
+            offset = min (offset, temp_offset);

you could write

  minmax(dir, offset, temp_offset)

similarly,

+              if (dir == UP)
+                {
+                  if (y_val > beam_y1)
..
+              else
+                {
+                  if (y_val < beam_y1)


could be

  if (dir * (y_val - beam_y1) > 0)


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