Re: Vertical spacing, distinguish stretchability/compressibility (issue4517136)

2011-06-16 Thread percival . music . ca 

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)

2011-06-10 Thread joeneeman 

ok, lgtm


http://codereview.appspot.com/4517136/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Vertical spacing, distinguish stretchability/compressibility (issue4517136)

2011-06-04 Thread 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));
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

Re: Vertical spacing, distinguish stretchability/compressibility (issue4517136)

2011-06-04 Thread k-ohara5a5a 

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

Re: Vertical spacing, distinguish stretchability/compressibility (issue4517136)

2011-06-04 Thread joeneeman 


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)

2011-06-04 Thread k-ohara5a5a 

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