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

[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))
+      #'((minimum-distance . 20))
   }
 }

Index: lily/align-interface.cc
diff --git a/lily/align-interface.cc b/lily/align-interface.cc
index  
2fd49f5162a2547c57606d76a5ddd4d0386e5cfd..403f903e490ccea3d811be8b1f99f6b0c341b960  
100644
--- a/lily/align-interface.cc
+++ b/lily/align-interface.cc
@@ -226,9 +226,6 @@ Align_interface::internal_get_minimum_translations  
(Grob *me,
 	  if (Page_layout_problem::read_spacing_spec (spec, &min_distance,  
ly_symbol2scm ("minimum-distance")))
            dy = max (dy, min_distance);

-         if (include_fixed_spacing)
-	    dy = max (dy, Page_layout_problem::get_fixed_spacing (elems[j-1],  
elems[j], spaceable_count, pure, start, end));
-
 	  if (include_fixed_spacing && Page_layout_problem::is_spaceable  
(elems[j]) && last_spaceable_element)
            {
              // Spaceable staves may have
Index: lily/page-layout-problem.cc
diff --git a/lily/page-layout-problem.cc b/lily/page-layout-problem.cc
index  
9ee2b3fea11b9ae3a22ef74ebb140515b9224fd4..07c99586ee29c4715d21f1db56ad89255b9dfbc1  
100644
--- a/lily/page-layout-problem.cc
+++ b/lily/page-layout-problem.cc
@@ -775,12 +775,7 @@ Page_layout_problem::get_fixed_spacing (Grob *before,  
Grob *after, int spaceable
        return robust_scm2double (cached, 0.0);
     }

-  SCM spec = Page_layout_problem::get_spacing_spec (before, after, pure,  
start, end);
   Real ret = -infinity_f;
-  Real stretchability = 0;
-  if (Page_layout_problem::read_spacing_spec (spec, &stretchability,  
ly_symbol2scm ("stretchability"))
-      && stretchability == 0)
-    Page_layout_problem::read_spacing_spec (spec, &ret, ly_symbol2scm  
("basic-distance"));

   // If we're pure, then paper-columns have not had their systems set,
   // and so elts[i]->get_system () is unreliable.
@@ -894,10 +889,7 @@ Page_layout_problem::alter_spring_from_spacing_spec  
(SCM spec, Spring* spring)
   spring->set_default_strength ();

   if (read_spacing_spec (spec, &stretch, ly_symbol2scm ("stretchability")))
-    {
-      spring->set_inverse_stretch_strength (stretch);
-      spring->set_inverse_compress_strength (stretch);
-    }
+    spring->set_inverse_stretch_strength (stretch);
 }

 vector<Grob*>
Index: lily/simple-spacer.cc
diff --git a/lily/simple-spacer.cc b/lily/simple-spacer.cc
index  
14c7cbeebd113007989867aa65355a4b4c15320f..32de9d38585091c7248054eeb2dffc2007eb22c5  
100644
--- a/lily/simple-spacer.cc
+++ b/lily/simple-spacer.cc
@@ -196,7 +196,6 @@ Simple_spacer::expand_line ()
 Real
 Simple_spacer::compress_line ()
 {
-  double inv_hooke = 0;
   double cur_len = configuration_length (force_);
   double cur_force = force_;
   bool compressed = false;
@@ -213,23 +212,25 @@ Simple_spacer::compress_line ()
     }

   fits_ = true;
-  for (vsize i=0; i < springs_.size (); i++)
-    inv_hooke += compressed
-      ? springs_[i].inverse_compress_strength ()
-      : springs_[i].inverse_stretch_strength ();

   assert (line_len_ <= cur_len);

   vector<Spring> sorted_springs = springs_;
   sort (sorted_springs.begin (), sorted_springs.end (), greater<Spring>  
());

-  for (vsize i = 0; i < sorted_springs.size (); i++)
+  /* inv_hooke is the total flexibility of currently-active springs */
+  double inv_hooke = 0;
+  vsize i = sorted_springs.size ();
+  while (i-- > 0 && sorted_springs[i].blocking_force () < cur_force)
+    inv_hooke += compressed
+      ? sorted_springs[i].inverse_compress_strength ()
+      : sorted_springs[i].inverse_stretch_strength ();
+
+  /* All sorted_springs[] from 0 through i are inactive, so */
+  for (++i; i < sorted_springs.size (); i++)
     {
       Spring sp = sorted_springs[i];

-      if (sp.blocking_force () > cur_force)
-       continue;
-
       if (isinf (sp.blocking_force ()))
        break;

Index: lily/spring.cc
diff --git a/lily/spring.cc b/lily/spring.cc
index  
60bc0aec77ca736084ed1afd18430b6a4bc0d9ea..aa69bdd2779fd4e6ecb2ee688cceb575581f0f8f  
100644
--- a/lily/spring.cc
+++ b/lily/spring.cc
@@ -45,20 +45,21 @@ Spring::Spring (Real dist, Real min_dist)
 void
 Spring::update_blocking_force ()
 {
+  // blocking_force_ is the value of force
+  //   below which length(force) is constant, and
+  //   above which length(force) varies according to inverse_*_strength.
+  // Simple_spacer::compress_line() depends on the contition above.
+  // We assume inverse_*_strength are non-negative.
   if (min_distance_ > distance_)
-    blocking_force_ = (min_distance_ - distance_) /  
inverse_stretch_strength_;
+    if (inverse_stretch_strength_ > 0.0)
+      blocking_force_ = (min_distance_ - distance_) /  
inverse_stretch_strength_;
+    else
+      blocking_force_ = 0.0;
   else
-    blocking_force_ = (min_distance_ - distance_) /  
inverse_compress_strength_;
-
-  // If the spring is fixed, it's not clear what the natural value
-  // of blocking_force_ would be (because it always blocks).
-  // -infinity_f works fine for now.
-  // If inverse_stretch_strength > 0, the spring is not fixed (because it  
can stretch).
-  if (isnan (blocking_force_) || blocking_force_ == infinity_f)
-    blocking_force_ = (inverse_stretch_strength_ > 0) ? 0.0 : -infinity_f;
-
-  if (blocking_force_ >= 0)
-    inverse_compress_strength_ = 0;
+    if (inverse_compress_strength_ > 0.0)
+      blocking_force_ = (min_distance_ - distance_) /  
inverse_compress_strength_;
+    else
+      blocking_force_ = 0.0;
 }

 /* scale a spring, but in a way that doesn't violate min_distance */