beam.cc:pure-rest-collision-callback Place on staff lines; issue 2468

beam.cc:pure-rest-collision-callback Place on staff lines; issue 2468

[Graham Percival, 2012-04-15 15:20:13]

wow, looks very nice and simple.  I vote for pushing immediately, but please
wait for at least one other such vote (or a countdown).

[dak, 2012-04-15 15:32:10]

On 2012/04/15 15:20:13, Graham Percival wrote:
> wow, looks very nice and simple.  I vote for pushing immediately, but please
> wait for at least one other such vote (or a countdown).

if max/min trigger, rint is applied to some rest_max_pos.  It would appear that
it should rather apply only to the calculated mean.

The calculated mean uses the current rounding mode.  If it is IEEE rounding,
this means round to even: (1+2)/2.0 will be rounded to 2.0, and (2+3)/2.0 will
be rounded to 2.0 as well.  That looks suspicious to me, like every second staff
line will get avoided for values right in between.

[Keith, 2012-04-15 16:45:54]

On 2012/04/15 15:32:10, dak wrote:
> 
> if max/min trigger, rint is applied to some rest_max_pos.  It would > appear
that it should rather apply only to the calculated mean.
> 
I want the final output to be an integral number of staff spaces, and
rest_max_pos might not be.  (Despite its name, rest_max_pos is in units of
staff-spaces, usually 2.5)

> every second staff line will get avoided for values in between.

Yes, the tentative placement for beamed rests favors the middle, uppermost, and
lowermost lines, in the usual five-line staff.  

An improvement would be to round-to-larger, as in rest_collision_callback() when
it sets the final position of the rest.  That function is also more careful to
make the shift relative to 'prev_offset' an integral number of staff spaces,
rather than the position relative to staff-center.

[Keith, 2012-04-15 20:06:55]

On 2012/04/15 16:45:54, Keith wrote:
>
> An improvement would be to round-to-larger

Done, along with other cleanup following the similar function
rest_collision_callback()

[dak, 2012-04-21 16:10:09]

On 2012/04/15 20:06:55, Keith wrote:
> On 2012/04/15 16:45:54, Keith wrote:
> >
> > An improvement would be to round-to-larger
> 
> Done, along with other cleanup following the similar function
> rest_collision_callback()

The "cleanup" would seem to break layout totally.

<URL:http://permalink.gmane.org/gmane.comp.gnu.lilypond.general/71476>

[Keith, 2012-04-21 18:22:05]

On 2012/04/21 16:10:09, dak wrote:

> The "cleanup" would seem to break layout totally.
> 
> <URL:http://permalink.gmane.org/gmane.comp.gnu.lilypond.general/71476>

That's disappointing.  The property 'previous_offset' sometimes has values
comparable to the page height.  On the early returns I'll return zero like the
real (that is, not pure) rest_collision_callback() does.

[Keith, 2012-04-25 06:21:29]

The function being patched is the pure-alternative to a chained-offset-callback
to the original Y-position
callback for rests (Rest::y_offset_callback).
I cannot follow the data between C and Scheme well enough to understand what
this function should return if it needs to bail out early.

The problem with patch set 2 (included in 2.15.37) was that I returned
'prev_offset' on early returns.  Sometimes, however, 'prev_offset' contains
unrealistically large values, and returning them fools the page-breaker (issue
2486).  I haven't yet found where the large values come from.

Returning 0.0 seems to work most of the time, but caused minor problems like
that mentioned on the tracker
 << {g''8[ r8. g''16]}\\{r8 <g' a' b'>8. r16} >>

This patch returns 0.0 when it returns early, but avoids returning early when it
doesn't need to.

http://codereview.appspot.com/6035053/diff/16001/lily/beam.cc
File lily/beam.cc (right):

http://codereview.appspot.com/6035053/diff/16001/lily/beam.cc#newcode1323
lily/beam.cc:1323: return scm_from_double (0.0);
I'm confused.  There is a remaining problem with dots on
 << {g''8[ r8. g''16]}\\{r8 <g' a' b'>8. r16} >>
which I thought was caused by this code returning 0.0 on its early returns.  It
is a minor bug, but a regression since 2.14.

This is the pure-alternative to a chained-callback to the original Y-position
callback for rests, Rest::y_offset_callback

http://codereview.appspot.com/6035053/diff/16001/lily/beam.cc#newcode1328
lily/beam.cc:1328: return scm_from_double (0.0);
It would seem we do not want an early return in this case; if the beam direction
is not yet know, we would prefer to estimate the rest position to returning the
wrong value.
But this condition was added recently, with the fix for issue 774.

http://codereview.appspot.com/6035053/diff/21001/lily/beam.cc
File lily/beam.cc (right):

http://codereview.appspot.com/6035053/diff/21001/lily/beam.cc#newcode1323
lily/beam.cc:1323: return scm_from_double (0.0);
or possibly
   return scm_from_double (previous);

http://codereview.appspot.com/6035053/diff/21001/lily/beam.cc#newcode1327
lily/beam.cc:1327: return scm_from_double (0.0);
or possibly
   return scm_from_double (previous);

http://codereview.appspot.com/6035053/diff/21001/lily/beam.cc#newcode1359
lily/beam.cc:1359: return scm_from_double (0.0);
or possibly
   return scm_from_double (previous);

[mike7, 2012-04-25 06:29:02]

http://codereview.appspot.com/6035053/diff/16001/lily/beam.cc
File lily/beam.cc (right):

http://codereview.appspot.com/6035053/diff/16001/lily/beam.cc#newcode1328
lily/beam.cc:1328: return scm_from_double (0.0);
On 2012/04/25 06:21:29, Keith wrote:
> It would seem we do not want an early return in this case; if the beam
direction
> is not yet know, we would prefer to estimate the rest position to returning
the
> wrong value.
> But this condition was added recently, with the fix for issue 774.

What would the "wrong value" be here?

[Keith, 2012-04-25 07:31:53]

On Tue, 24 Apr 2012 23:29:02 -0700, <mike@mikesolomon.org> wrote:

> http://codereview.appspot.com/6035053/diff/16001/lily/beam.cc
> File lily/beam.cc (right):
>
> http://codereview.appspot.com/6035053/diff/16001/lily/beam.cc#newcode1328
> lily/beam.cc:1328: return scm_from_double (0.0);
> On 2012/04/25 06:21:29, Keith wrote:
>> It would seem we do not want an early return in this case; if the beam
>> direction is not yet know, we would prefer to estimate the rest position
>> to returning the wrong value.
>
> What would the "wrong value" be here?

0.0, indicating rest on centerline, is the wrong value.  If there is another
voice, the first approximation for the rest is to avoid the other voice.  The
only case where I see it cause trouble is when dots on a rest try to avoid dots
in another voice, as if the rest needed to stay at centerline.

> http://codereview.appspot.com/5908043/diff/1/lily/beam.cc#newcode1326
> lily/beam.cc:1326: || beam->get_property_data ("direction") ==
> ly_symbol2scm ("calculation-in-progress"))
> On 2012/04/25 06:21:54, Keith wrote:
>> Mike, any idea why you added this?
>
> If this bit isn't there, a circular dependency could be called up later
> on in the function while looking for the direction.

Thanks.  Now I see.  Later on we get_grob_direction(beam) so we can pick the
head closest to the beam.
I'll move this test down there to protect the get_grob_direction() next time I
have Linux up.

[Keith, 2012-04-27 06:17:59]

On 2012/04/25 07:31:53, Keith wrote:
> > lily/beam.cc:1326: || beam->get_property_data ("direction") ==
> > ly_symbol2scm ("calculation-in-progress"))
> I'll move this test down there to protect the get_grob_direction() 
> next time I have Linux up.

I dropped the plan above because now I see how to get the correct prev_offsets.

Patch set 7 is the same as patch set 2, except the input args are connected
correctly.