Code review - Issue 6035053: beam.cc:pure-rest-collision-callback Place on staff lines; issue 2468https://codereview.appspot.com/2012-04-27T06:17:59+00:00rietveld
Message from unknown
2012-04-15T06:54:18+00:00Keithurn:md5:333c28c1f05113b366bcc2b1029ba238
Message from graham@percival-music.ca
2012-04-15T15:20:13+00:00Graham Percivalurn:md5:05f3d48c9553b8908147a5dcd4101618
wow, looks very nice and simple. I vote for pushing immediately, but please wait for at least one other such vote (or a countdown).
Message from dak@gnu.org
2012-04-15T15:32:10+00:00dakurn:md5:f4aea0eef5f7edaed93831ef6a1d1808
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.
Message from k-ohara5a5a@oco.net
2012-04-15T16:45:54+00:00Keithurn:md5:5c7df34e3fc12ed16d6653c1139aaed3
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.
Message from k-ohara5a5a@oco.net
2012-04-15T20:06:55+00:00Keithurn:md5:ed6677d0de95f1d9b400cea06ade59e0
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()
Message from unknown
2012-04-15T20:15:38+00:00Keithurn:md5:23c268016883694f11e31e5d4a67e826
Message from dak@gnu.org
2012-04-21T16:10:09+00:00dakurn:md5:34023564c4171d115fdb6c72c1e8bcfc
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>
Message from unknown
2012-04-21T18:11:37+00:00Keithurn:md5:b9427f3bd4300dd0ded08897295e9324
Message from k-ohara5a5a@oco.net
2012-04-21T18:22:05+00:00Keithurn:md5:059d59b03f7a9e1c3913cc3a0a96e357
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.
Message from unknown
2012-04-22T07:01:43+00:00Keithurn:md5:0e9e8ed12863734aa66e68e8c4a912f4
Message from unknown
2012-04-25T06:01:04+00:00Keithurn:md5:6c2ac5de18be8335c0b7069b2bfa5a95
Message from k-ohara5a5a@oco.net
2012-04-25T06:21:29+00:00Keithurn:md5:073e3dcb0e2eb91addc224de1db1f10d
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);
Message from mike@mikesolomon.org
2012-04-25T06:29:02+00:00mike7urn:md5:cc614d280ffa9c5a7442a9eec3beae9b
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?
Message from k-ohara5a5a@oco.net
2012-04-25T07:31:53+00:00Keithurn:md5:a96945d5161e18cdf833330b154c666b
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.
Message from unknown
2012-04-27T02:30:11+00:00Keithurn:md5:4556c2291f27b49145015657c1b6dac4
Message from unknown
2012-04-27T06:01:49+00:00Keithurn:md5:b6c5fe87ffeb045930159d642cd3abb0
Message from k-ohara5a5a@oco.net
2012-04-27T06:17:59+00:00Keithurn:md5:ba213c963a745975bb06a04cdb0555f5
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.