Re: beam.cc:pure-rest-collision-callback Place on staff lines; issue 2468 (issue 6035053)
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);
http://codereview.appspot.com/6035053/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: beam.cc:pure-rest-collision-callback Place on staff lines; issue 2468 (issue 6035053)
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? http://codereview.appspot.com/6035053/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: beam.cc:pure-rest-collision-callback Place on staff lines; issue 2468 (issue 6035053)
On Tue, 24 Apr 2012 23:29:02 -0700, m...@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. ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: beam.cc:pure-rest-collision-callback Place on staff lines; issue 2468 (issue 6035053)
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 http://codereview.appspot.com/6035053/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: beam.cc:pure-rest-collision-callback Place on staff lines; issue 2468 (issue 6035053)
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. http://codereview.appspot.com/6035053/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
beam.cc:pure-rest-collision-callback Place on staff lines; issue 2468 (issue 6035053)
wow, looks very nice and simple. I vote for pushing immediately, but please wait for at least one other such vote (or a countdown). http://codereview.appspot.com/6035053/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: beam.cc:pure-rest-collision-callback Place on staff lines; issue 2468 (issue 6035053)
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. http://codereview.appspot.com/6035053/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: beam.cc:pure-rest-collision-callback Place on staff lines; issue 2468 (issue 6035053)
Reviewers: Graham Percival, dak, Message: 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. Description: beam.cc:pure-rest-collision-callback Place on staff lines; issue 2468 Please review this at http://codereview.appspot.com/6035053/ Affected files: M lily/beam.cc Index: lily/beam.cc diff --git a/lily/beam.cc b/lily/beam.cc index 0cfc3e193cba6b3feea911abb09a6f9e5118d6c2..0cb9946eec038e9fca0960072a0fa5d49e712a7c 100644 --- a/lily/beam.cc +++ b/lily/beam.cc @@ -1372,7 +1372,7 @@ Beam::pure_rest_collision_callback (SCM smob, then bound it by the minimum and maximum positions outside the staff. 4.0 = 2.0 to get out of staff space * 2.0 for the average */ - amount = min (max ((Stem::head_positions (left)[beamdir] + Stem::head_positions (right)[beamdir]) / 4.0, rest_max_pos[DOWN]), rest_max_pos[UP]); + amount = rint( min (max ((Stem::head_positions (left)[beamdir] + Stem::head_positions (right)[beamdir]) / 4.0, rest_max_pos[DOWN]), rest_max_pos[UP])); return scm_from_double (amount); } ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: beam.cc:pure-rest-collision-callback Place on staff lines; issue 2468 (issue 6035053)
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() http://codereview.appspot.com/6035053/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
