Re: Spacing with empty contexts; issue 1669 (issue4515158)

[k-ohara5a5a]

Reviewers: joeneeman,


http://codereview.appspot.com/4515158/diff/2009/lily/align-interface.cc
File lily/align-interface.cc (right):

http://codereview.appspot.com/4515158/diff/2009/lily/align-interface.cc#newcode222
lily/align-interface.cc:222: dy = max (dy, min_distance);
On 2011/06/07 04:48:01, joeneeman wrote:
> If pure is true, there may be some staves that are going to be
removed. However,
> these staves won't be removed until after line-breaking.

... and, as you say, these staves are not yet removed when
Y-extent-estimates for the page breaker are calculated, so this patch
messes up the page breaker.

If the page breaker depends on empty, but living, staves disappearing
for vertical spacing purposes, then maybe empty staves should be treated
that way consistently. That would be tantamount to giving all empty-able
contexts the hara-kiri engraver, as you suggested much earlier.

The obvious alternative would be to have the empty staves decide whether
to suicide before page breaking, but my suspicion is that is awkward, or
it would have been done that way at first.

Having minimum_distances() change behavior based on whether 'pure' is
true is too tricky for me. (how do we know that any staff that is empty
we are pure will suicide later)

I'll think on it, but mark the patch as abandoned for now.

Description:
Align_interface::get_minimum_translations() should return a
translation for empty lines that respects minimum-distances.

When merging the skyline of an empty line, or one with gaps
between grobs, preserve the padding of the previous line.


Please review this at http://codereview.appspot.com/4515158/

Affected files:
  A input/regression/dynamics-empty.ly
  M lily/align-interface.cc


Index: input/regression/dynamics-empty.ly
diff --git a/input/regression/dynamics-empty.ly  
b/input/regression/dynamics-empty.ly
new file mode 100644
index  
0000000000000000000000000000000000000000..085208c983ab28894c6ca0e0760a27358cbc680b
--- /dev/null
+++ b/input/regression/dynamics-empty.ly
@@ -0,0 +1,27 @@
+\version "2.13.39"
+
+\header {
+  texidoc = "An empty Dynamics context does not confuse the spacing."
+  poet = "poet.........................."
+}
+
+\score {
+  \new PianoStaff <<
+    \new Dynamics { s1}
+    \new Dynamics { s2\f\> s4 s\!\p }
+    \new Staff {d'2 g''}
+    \new Dynamics { s1 }
+    \new Dynamics { s2\sustainOn s4 s\sustainOff }
+    \new Dynamics { s1 }
+    \new Staff {d'2 g''}
+    \new Dynamics { s1 }
+    \new Dynamics { s2\sustainOn s4 s\sustainOff }
+  >>
+  \layout {
+    \context {
+      \Dynamics
+      \override VerticalAxisGroup #'nonstaff-relatedstaff-spacing
+       = #'((minimum-distance . 5) (stretchability . 0) (padding . 1))
+    }
+  }
+}
Index: lily/align-interface.cc
diff --git a/lily/align-interface.cc b/lily/align-interface.cc
index  
2fd49f5162a2547c57606d76a5ddd4d0386e5cfd..344b9e8394d099f45a2084df395a86f4176dcfb2  
100644
--- a/lily/align-interface.cc
+++ b/lily/align-interface.cc
@@ -62,24 +62,23 @@ Align_interface::align_to_ideal_distances (SCM smob)
   return SCM_BOOL_T;
 }

-/* for each grob, find its upper and lower skylines. If the grob has
-   an empty extent, delete it from the list instead. If the extent is
+/* for each grob, find its upper and lower skylines. If the extent is
    non-empty but there is no skyline available (or pure is true), just
    create a flat skyline from the bounding box */
 // TODO(jneem): the pure and non-pure parts seem to share very little
 // code. Split them into 2 functions, perhaps?
 static void
 get_skylines (Grob *me,
-             vector<Grob*> *const elements,
+             vector<Grob*> const &elements,
              Axis a,
              bool pure, int start, int end,
              vector<Skyline_pair> *const ret)
 {
-  Grob *other_common = common_refpoint_of_array (*elements, me, other_axis  
(a));
+  Grob *other_common = common_refpoint_of_array (elements, me, other_axis  
(a));

-  for (vsize i = elements->size (); i--;)
+  for (vsize i = elements.size (); i--;)
     {
-      Grob *g = (*elements)[i];
+      Grob *g = elements[i];
       Skyline_pair skylines;

       if (!pure)
@@ -135,10 +134,7 @@ get_skylines (Grob *me,
            }
        }

-      if (skylines.is_empty ())
-       elements->erase (elements->begin () + i);
-      else
-       ret->push_back (skylines);
+      ret->push_back (skylines);
     }
   reverse (*ret);
 }
@@ -178,7 +174,7 @@  
Align_interface::get_minimum_translations_without_min_dist (Grob *me,
 //   else centered dynamics will break when there is a fixed alignment).
 vector<Real>
 Align_interface::internal_get_minimum_translations (Grob *me,
-                                                   vector<Grob*> const 
&all_grobs,
+                                                   vector<Grob*> const &elems,
                                                    Axis a,
                                                    bool include_fixed_spacing,
                                                    bool pure, int start, int 
end)
@@ -195,10 +191,9 @@ Align_interface::internal_get_minimum_translations  
(Grob *me,

   Direction stacking_dir = robust_scm2dir (me->get_property  
("stacking-dir"),
                                           DOWN);
-  vector<Grob*> elems (all_grobs); // writable copy
   vector<Skyline_pair> skylines;

-  get_skylines (me, &elems, a, pure, start, end, &skylines);
+  get_skylines (me, elems, a, pure, start, end, &skylines);

   Real where = 0;
   Real default_padding = robust_scm2double (me->get_property ("padding"),  
0.0);
@@ -213,8 +208,8 @@ Align_interface::internal_get_minimum_translations  
(Grob *me,
       Real dy = 0;
       Real padding = default_padding;

-      if (j == 0)
-       dy = skylines[j][-stacking_dir].max_height () + padding;
+      if (j == 0 || down_skyline.is_empty ())
+       dy = skylines[j][-stacking_dir].max_height ();
       else
        {
 	  SCM spec = Page_layout_problem::get_spacing_spec (elems[j-1], elems[j],  
pure, start, end);
@@ -229,6 +224,15 @@ Align_interface::internal_get_minimum_translations  
(Grob *me,
          if (include_fixed_spacing)
 	    dy = max (dy, Page_layout_problem::get_fixed_spacing (elems[j-1],  
elems[j], spaceable_count, pure, start, end));

+         // 'stretchability also determines compressibility in
+         // the function Page_layout_problem:: alter_spring_from_spacing_spec()
+         // So if stretchability=0, treat basic-distance as a minimum-distance.
+         Real stretchability = 0.0;
+	  if (Page_layout_problem::read_spacing_spec (spec, &stretchability,  
ly_symbol2scm ("stretchability"))
+             && stretchability == 0.0
+	      && Page_layout_problem::read_spacing_spec (spec, &min_distance,  
ly_symbol2scm ("basic-distance")))
+           dy = max (dy, min_distance);
+
 	  if (include_fixed_spacing && Page_layout_problem::is_spaceable  
(elems[j]) && last_spaceable_element)
            {
              // Spaceable staves may have
@@ -257,7 +261,11 @@ Align_interface::internal_get_minimum_translations  
(Grob *me,
        dy = 0.0;

       dy = max (0.0, dy);
-      down_skyline.raise (-stacking_dir * dy);
+
+      // Shift down_skyline to its closest possible position relative to  
this line,
+      // but also grow it by the padding between itself and this line,
+      // so that padding remains effective across any gaps in the current  
skyline[j].
+      down_skyline.raise (-stacking_dir * (dy - padding));
       down_skyline.merge (skylines[j][stacking_dir]);
       where += stacking_dir * dy;
       translates.push_back (where);
@@ -271,21 +279,7 @@ Align_interface::internal_get_minimum_translations  
(Grob *me,
        }
     }

-  // So far, we've computed the translates for all the non-empty elements.
-  // Here, we set the translates for the empty elements: an empty element
-  // gets the same translation as the last non-empty element before it.
-  vector<Real> all_translates;
-  if (!translates.empty ())
-    {
-      Real w = translates[0];
-      for  (vsize i = 0, j = 0; j < all_grobs.size (); j++)
-       {
-         if (i < elems.size () && all_grobs[j] == elems[i])
-           w = translates[i++];
-         all_translates.push_back (w);
-       }
-    }
-  return all_translates;
+  return translates;
 }

 void