Re: Vertical spacing, distinguish stretchability/compressibility (issue4517136)
countdown over, please push whenever it's convenient, Keith. http://codereview.appspot.com/4517136/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Vertical spacing, distinguish stretchability/compressibility (issue4517136)
ok, lgtm http://codereview.appspot.com/4517136/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Vertical spacing, distinguish stretchability/compressibility (issue4517136)
On 2011/06/04 17:32:15, joeneeman wrote: > When compressing, unstretchable springs never get put on the > active list, since their blocking_force is 0. It seems, though, that unstretchable but compressible springs should be on the active list when compressing. They are. They have blocking_force -1.0 (compressive) without any special treatment, and I had forgotten about them when I wrote above. What about +inf? I like that concept, but... The starting condition for simple-spacer is to pull until all springs unblock, which could then require +inf force, and then the watchdog in length() accuses me of "cruelty to springs". I thought about going inf-robust, and retiring that watchdog, but it looks like that would complicate Simple_spacer::compress(). http://codereview.appspot.com/4517136/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Vertical spacing, distinguish stretchability/compressibility (issue4517136)
http://codereview.appspot.com/4517136/diff/1001/lily/simple-spacer.cc File lily/simple-spacer.cc (right): http://codereview.appspot.com/4517136/diff/1001/lily/simple-spacer.cc#newcode230 lily/simple-spacer.cc:230: for (++i; i < sorted_springs.size (); i++) On 2011/06/04 10:48:09, Keith wrote: On 2011/06/04 07:09:05, joeneeman wrote: > It seems like this loop will never see i=0, even if it is active. If springs[0] is active, the i-- above leaves i at UINT_MAX on loop exit, so ++i would be zero. I don't know a good loop idiom for a down-loop using an unsigned type (vsize) for the index. I'll re-write as two up-loops starting with "first_active" or something. (In the old code, some inactive springs were added to inv_hooke and never removed, but other functions made sure these had zero flexibility so it didn't matter. That seems too tricky to maintain, now keeping track of compressibility distinct from stretchability. So my reason for change here was an attempt to make the code /easier/ to figure out.) Sorry, my mistake. I wouldn't bother rewriting this if I were you. Perhaps it's worth a comment, though, just to point out the corner case. http://codereview.appspot.com/4517136/diff/1001/lily/spring.cc File lily/spring.cc (left): http://codereview.appspot.com/4517136/diff/1001/lily/spring.cc#oldcode55 lily/spring.cc:55: // -infinity_f works fine for now. On 2011/06/04 10:48:09, Keith wrote: On 2011/06/04 07:09:05, joeneeman wrote: > fixed springs have a blocking_force_ of zero instead > of -infinity_f. Does that work properly in simple-spacer? My comment tries to state the contitions ^H^H conditions that I can see as required for simple-spacer to work properly. With blocking_force 0, the unstretchable springs are on the "active" list when stretching the layout, but with zero stretchability they do nothing. When compressing, unstretchable springs never get put on the active list, since their blocking_force is 0. It seems, though, that unstretchable but compressible springs should be on the active list when compressing. With the former blocking force -inf, infinite compression, such springs sorted to be the very last ones removed from the active list. I could see no reason to keep them active until the end. What about +inf? I'm not vehemently opposed to 0.0, but it seems strange to me that springs with are always blocking (or never blocking, depending on how you see it) should be sorted in the middle instead of at one end. http://codereview.appspot.com/4517136/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Vertical spacing, distinguish stretchability/compressibility (issue4517136)
Reviewers: joeneeman, http://codereview.appspot.com/4517136/diff/1001/lily/align-interface.cc File lily/align-interface.cc (left): http://codereview.appspot.com/4517136/diff/1001/lily/align-interface.cc#oldcode230 lily/align-interface.cc:230: dy = max (dy, Page_layout_problem::get_fixed_spacing (elems[j-1], elems[j], spaceable_count, pure, start, end)); On 2011/06/04 07:09:05, joeneeman wrote: I think this is still necessary because of alignment-distances. We call get_fixed_spacing() for spaceable staves at line 251 below (and I believe only spaceable staves have alignment-distances). http://codereview.appspot.com/4517136/diff/1001/lily/page-layout-problem.cc File lily/page-layout-problem.cc (right): http://codereview.appspot.com/4517136/diff/1001/lily/page-layout-problem.cc#newcode768 lily/page-layout-problem.cc:768: : ly_symbol2scm ("loose-fixed-spacing"); If we remove the special case that creates loose-fixed-spacing then I guess we should also remove this symbol. This function, get_fixed_spacing(), would no longer do anything for loose lines, after this patch. http://codereview.appspot.com/4517136/diff/1001/lily/simple-spacer.cc File lily/simple-spacer.cc (right): http://codereview.appspot.com/4517136/diff/1001/lily/simple-spacer.cc#newcode230 lily/simple-spacer.cc:230: for (++i; i < sorted_springs.size (); i++) On 2011/06/04 07:09:05, joeneeman wrote: It seems like this loop will never see i=0, even if it is active. If springs[0] is active, the i-- above leaves i at UINT_MAX on loop exit, so ++i would be zero. I don't know a good loop idiom for a down-loop using an unsigned type (vsize) for the index. I'll re-write as two up-loops starting with "first_active" or something. (In the old code, some inactive springs were added to inv_hooke and never removed, but other functions made sure these had zero flexibility so it didn't matter. That seems too tricky to maintain, now keeping track of compressibility distinct from stretchability. So my reason for change here was an attempt to make the code /easier/ to figure out.) http://codereview.appspot.com/4517136/diff/1001/lily/simple-spacer.cc#newcode234 lily/simple-spacer.cc:234: if (isinf (sp.blocking_force ())) Now using finite blocking_forces, this test seems un-necessary, but I left it in just in case something like a minimum-distance=-inf slips through. http://codereview.appspot.com/4517136/diff/1001/lily/spring.cc File lily/spring.cc (left): http://codereview.appspot.com/4517136/diff/1001/lily/spring.cc#oldcode55 lily/spring.cc:55: // -infinity_f works fine for now. On 2011/06/04 07:09:05, joeneeman wrote: fixed springs have a blocking_force_ of zero instead of -infinity_f. Does that work properly in simple-spacer? My comment tries to state the contitions ^H^H conditions that I can see as required for simple-spacer to work properly. With blocking_force 0, the unstretchable springs are on the "active" list when stretching the layout, but with zero stretchability they do nothing. When compressing, unstretchable springs never get put on the active list, since their blocking_force is 0. With the former blocking force -inf, infinite compression, such springs sorted to be the very last ones removed from the active list. I could see no reason to keep them active until the end. Description: When stretchability is changed, leave inverse_compression_strength alone %{ The LilyPond Notation Reference says: stretchability – a measure of the dimension’s relative propensity to stretch. [...] Note that the dimension’s propensity to compress cannot be directly set by the user and is equal to (basic-distance - minimum-distance). but in ver2.13.62, 'stretchability affects the ability to compress. The documented behavior would be nice for things like piano pedal lines where we want to disallow stretching, but allow compression to get out of the way of high notes on the next line. %} \paper { ragged-right = ##t paper-height = 80\mm system-system-spacing = #'((minimum-distance . 8) % be close, when needed (basic-distance . 16) (stretchability . 0) (padding . 1) ) } { g'1 \break g, \break g''' } Please review this at http://codereview.appspot.com/4517136/ Affected files: M input/regression/page-breaking-min-distance2.ly M lily/align-interface.cc M lily/page-layout-problem.cc M lily/simple-spacer.cc M lily/spring.cc Index: input/regression/page-breaking-min-distance2.ly diff --git a/input/regression/page-breaking-min-distance2.ly b/input/regression/page-breaking-min-distance2.ly index 57f9d592309f7bf7aaa5420c63e55a56e16cef6b..2408c13c67a5a2bc3098eb9acac8328fd0d23b3f 100644 --- a/input/regression/page-breaking-min-distance2.ly +++ b/input/regression/page-breaking-min-distance2.ly @@ -8,8 +8,7 @@ \context { \Score \override VerticalAxisGroup #'staff-staff-spacing = - #'((basic-distance . 20) - (stretchability . 0)) + #'((m
Vertical spacing, distinguish stretchability/compressibility (issue4517136)
http://codereview.appspot.com/4517136/diff/1001/lily/align-interface.cc File lily/align-interface.cc (left): http://codereview.appspot.com/4517136/diff/1001/lily/align-interface.cc#oldcode230 lily/align-interface.cc:230: dy = max (dy, Page_layout_problem::get_fixed_spacing (elems[j-1], elems[j], spaceable_count, pure, start, end)); I think this is still necessary because of alignment-distances. http://codereview.appspot.com/4517136/diff/1001/lily/simple-spacer.cc File lily/simple-spacer.cc (right): http://codereview.appspot.com/4517136/diff/1001/lily/simple-spacer.cc#newcode230 lily/simple-spacer.cc:230: for (++i; i < sorted_springs.size (); i++) It seems like this loop will never see i=0, even if it is active. http://codereview.appspot.com/4517136/diff/1001/lily/spring.cc File lily/spring.cc (left): http://codereview.appspot.com/4517136/diff/1001/lily/spring.cc#oldcode55 lily/spring.cc:55: // -infinity_f works fine for now. So now, you make fixed springs have a blocking_force_ of zero instead of -infinity_f. Does that work properly in simple-spacer? It seems to me like it will sort springs in the wrong order (and also line 237 won't work as intended). http://codereview.appspot.com/4517136/diff/1001/lily/spring.cc File lily/spring.cc (right): http://codereview.appspot.com/4517136/diff/1001/lily/spring.cc#newcode51 lily/spring.cc:51: // Simple_spacer::compress_line() depends on the contition above. condition http://codereview.appspot.com/4517136/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel