Caches the interior skylines of vertical axis groups and systems.

lily/axis-group-interface.cc

 /*
   This file is part of LilyPond, the GNU music typesetter.
 
   Copyright (C) 2000--2012 Han-Wen Nienhuys <hanwen@xs4all.nl>
 
   LilyPond is free software: you can redistribute it and/or modify
   it under the terms of the GNU General Public License as published by
   the Free Software Foundation, either version 3 of the License, or
   (at your option) any later version.
 
@@ skipping 24 matching lines @@
 #include "staff-grouper-interface.hh"
 #include "stem.hh"
 #include "stencil.hh"
 #include "system.hh"
 #include "warn.hh"
 #include "unpure-pure-container.hh"
 
 // NON MEMBER FUNCTIONS
 
 bool
 staff_priority_less (Grob *const &g1, Grob *const &g2)

[dak, 2013/12/11 13:44:25]

There is no point in passing references to pointers when the pointers are not
actually being changed.

Do you mean to pass references to the grobs instead?

 {
   Real priority_1 = robust_scm2double (g1->get_property ("outside-staff-priority"), -infinity_f);
   Real priority_2 = robust_scm2double (g2->get_property ("outside-staff-priority"), -infinity_f);
 
   if (priority_1 < priority_2)
     return true;
   else if (priority_1 > priority_2)
     return false;
 
   /* if neither grob has an outside-staff priority, the ordering will have no
      effect -- we just need to choose a consistent ordering. We do this to
      avoid the side-effect of calculating extents. */
   if (isinf (priority_1))
     return g1 < g2;

[dak, 2013/12/11 13:44:25]

Making decisions based on memory address is a bad idea since it leads to
irreproducible behavior.  Since the order does not need changing, one can just
return true here.

 
   /* if there is no preference in staff priority, choose the left-most one */
   Grob *common = g1->common_refpoint (g2, X_AXIS);
   Real start_1 = g1->extent (common, X_AXIS)[LEFT];
   Real start_2 = g2->extent (common, X_AXIS)[LEFT];
   return start_1 < start_2;
 }
 
 bool
 pure_staff_priority_less (Grob *const &g1, Grob *const &g2)
 {
   Real priority_1 = robust_scm2double (g1->get_property ("outside-staff-priority"), -infinity_f);
   Real priority_2 = robust_scm2double (g2->get_property ("outside-staff-priority"), -infinity_f);
 
   return priority_1 < priority_2;
 }
 
 static void

[dak, 2013/12/11 13:44:25]

There is no comment that indicates what add_interior_skylines can be used for,
what its input and output are, where and when it should be used.

This is write-only code.

 add_interior_skylines (Grob *me, Grob *x_common, Grob *y_common, vector<Skyline_pair> *skylines)
 {
   if (Grob_array *elements = unsmob_grob_array (me->get_object ("elements")))
     {
       for (vsize i = 0; i < elements->size (); i++)
         add_interior_skylines (elements->grob (i), x_common, y_common, skylines);
     }
   else if (!scm_is_number (me->get_property ("outside-staff-priority"))
            && !to_boolean (me->get_property ("cross-staff")))
     {
       Skyline_pair *maybe_pair = Skyline_pair::unsmob (me->get_property ("vertical-skylines"));
       if (!maybe_pair)
         return;
       if (maybe_pair->is_empty ())
         return;
       skylines->push_back (Skyline_pair (*maybe_pair));
       skylines->back ().shift (me->relative_coordinate (x_common, X_AXIS));
       skylines->back ().raise (me->relative_coordinate (y_common, Y_AXIS));
     }
 }
 
 SCM

[dak, 2013/12/11 13:44:25]

There is no comment what valid_outside_staff_placement_directive is called for
and what its results are.

 valid_outside_staff_placement_directive (Grob *me)
 {
   SCM directive = me->get_property ("outside-staff-placement-directive");
 
   if ((directive == ly_symbol2scm ("left-to-right-greedy"))

[dak, 2013/12/11 13:44:25]

If there is only a limited set of valid values, this should be checked by an
appropriate type definition of outside-staff-placement-directive at assigment
time rather than blowing up at run time.

       || (directive == ly_symbol2scm ("left-to-right-polite"))
       || (directive == ly_symbol2scm ("right-to-left-greedy"))
       || (directive == ly_symbol2scm ("right-to-left-polite")))
     return directive;
 
   if (directive == SCM_EOL)
     return ly_symbol2scm ("left-to-right-polite");
 
   me->warning (_f ("\"%s\" is not a valid outside-staff-placement-directive",
                    robust_symbol2string (directive, "").c_str ()));
@@ skipping 209 matching lines @@
   vector<Interval> mid_line_staff_heights;
   begin_line_heights.resize (ranks.size () - 1);
   mid_line_heights.resize (ranks.size () - 1);
 
   for (vsize i = 0; i < elts.size (); ++i)
     {
       Grob *g = elts[i];
 
       if (to_boolean (g->get_property ("cross-staff"))
           && !has_interface (g))
+        continue;
+
+      if (!g->is_live ())
         continue;
 
       bool outside_staff = scm_is_number (g->get_property ("outside-staff-priority"));
       Real padding = robust_scm2double (g->get_property ("outside-staff-padding"), get_default_outside_staff_padding ());
 
       // When we encounter the first outside-staff grob, make a copy
       // of the current heights to use as an estimate for the staff heights.
       // Note that the outside-staff approximation that we use here doesn't
       // consider any collisions that might occur between outside-staff grobs,
       // but only the fact that outside-staff grobs may need to be raised above
@@ skipping 217 matching lines @@
   Grob *me = unsmob_grob (smob);
   return internal_calc_pure_relevant_grobs (me, "elements");
 }
 
 SCM
 Axis_group_interface::internal_calc_pure_relevant_grobs (Grob *me, string grob_set_name)
 {
   extract_grob_set (me, grob_set_name.c_str (), elts);
 
   vector<Grob *> relevant_grobs;
-  SCM pure_relevant_p = ly_lily_module_constant ("pure-relevant?");
 
   for (vsize i = 0; i < elts.size (); i++)
     {
-      if (to_boolean (scm_apply_1 (pure_relevant_p, elts[i]->self_scm (), SCM_EOL)))
-        relevant_grobs.push_back (elts[i]);
-
-      if (Item *it = dynamic_cast<Item *> (elts[i]))
+      if (elts[i] && elts[i]->is_live ())
         {
-          for (LEFT_and_RIGHT (d))
+          relevant_grobs.push_back (elts[i]);
+          if (Item *it = dynamic_cast<Item *> (elts[i]))
             {
-              Item *piece = it->find_prebroken_piece (d);
-              if (piece && to_boolean (scm_apply_1 (pure_relevant_p, piece->self_scm (), SCM_EOL)))
-                relevant_grobs.push_back (piece);
+              for (LEFT_and_RIGHT (d))
+                {
+                  Item *piece = it->find_prebroken_piece (d);
+                  if (piece && piece->is_live ())
+                    relevant_grobs.push_back (piece);
+                }
             }
         }
     }
 
   vector_sort (relevant_grobs, pure_staff_priority_less);
   SCM grobs_scm = Grob_array::make_array ();
   unsmob_grob_array (grobs_scm)->set_array (relevant_grobs);
 
   return grobs_scm;
 }
@@ skipping 46 matching lines @@
 Interval
 Axis_group_interface::pure_group_height (Grob *me, int start, int end)
 {
   Grob *common = unsmob_grob (me->get_object ("pure-Y-common"));
 
   if (!common)
     {
       programming_error ("no pure Y common refpoint");
       return Interval ();
     }
-  Real my_coord = me->relative_coordinate (common, Y_AXIS);
+  Real my_coord = me->pure_relative_y_coordinate (common, start, end);
   Interval r (relative_pure_height (me, start, end));
 
   return r - my_coord;
 }
 
 void
 Axis_group_interface::get_children (Grob *me, vector<Grob *> *found)
 {
   found->push_back (me);
 
@@ skipping 16 matching lines @@
   Grob *parent = me->get_parent (Y_AXIS);
   if (!parent)
     return 0;
 
   if (scm_is_number (parent->get_property ("outside-staff-priority")))
     return parent;
 
   return outside_staff_ancestor (parent);
 }
 
 MAKE_SCHEME_CALLBACK (Axis_group_interface, calc_inside_staff_skylines, 1);

[dak, 2013/12/11 13:44:25]

There is no comment what calc_inside_staff_skylines is supposed to calculate,
what marks an "inside_staff_skyline", and how it differs from other skylines.

 SCM
 Axis_group_interface::calc_inside_staff_skylines (SCM smob)
 {
   Grob *me = unsmob_grob (smob);
   extract_grob_set (me, "vertical-skyline-elements", elements);
 
   Grob *x_common = common_refpoint_of_array (elements, me, X_AXIS);
   Grob *y_common = common_refpoint_of_array (elements, me, Y_AXIS);
 
   assert (y_common == me);

[dak, 2013/12/11 13:44:25]

If skylines are disabled, why would this assertion be true?

 
   vector<Skyline_pair> inside_staff_skylines;
 
   for (vsize i = 0; i < elements.size ()
        && !scm_is_number (elements[i]->get_property ("outside-staff-priority")); i++)
     {
       Grob *elt = elements[i];
       Grob *ancestor = outside_staff_ancestor (elt);
       if (!ancestor)
         add_interior_skylines (elt, x_common, y_common, &inside_staff_skylines);
     }
 
   Skyline_pair skylines (inside_staff_skylines);
   return skylines.smobbed_copy ();
 }
 
 /*
   Only returns false if a Y-child of a grob has a lower outside-staff-priority
   than its Y-parent.
 */
 
 bool
 sane_outside_staff_priority_parental_relationship (Grob *me)

[dak, 2013/12/11 13:44:25]

What has this to do with sanity?  Are childs only allowed larger staff
priorities?  Why?

 {
   // no outside staff priority
   if (!scm_is_number (me->get_property ("outside-staff-priority")))
     return true;
 
   Grob *ancestor = Axis_group_interface::outside_staff_ancestor (me);
   // no outside staff ancestor
   if (!ancestor)
     return true;
 
   return staff_priority_less (ancestor, me);
 }
 
 SCM
 ancestor_priority_plus_a_bit (Grob *me)

[dak, 2013/12/11 13:44:25]

Why do we need this?

 {
   Grob *ancestor = Axis_group_interface::outside_staff_ancestor (me);
   // no outside staff ancestor
   if (!ancestor)
     {
       me->programming_error ("This function should only be called if ancestry is sure.");
       return SCM_BOOL_F;
     }
 
   return scm_from_double (scm_to_double (ancestor->get_property ("outside-staff-priority")) + 0.0001);

[dak, 2013/12/11 13:44:25]

This does not distinguish between various ancestors, so both a child as well as
a grandchild will get assigned the same priority based on their common ancestor.
 If the degree of ancestry does not count into the result, it would appear that
this function only serves for disambiguation at a single level.

 }
 
 /*
   Before doing calculations involving outside staff priority are done, we
   suicide any grobs that need to be suicided and make sure that grobs that
   depend on the placement of a Y parent do not have a lower
   outside-staff-priority than the parent, which would cause their being
   placed first.
 */
 void
 Axis_group_interface::prepare_for_outside_staff_calculations (Grob *me)
 {
   extract_grob_set (me, "vertical-skyline-elements", elements);
   for (vsize i = 0; i < elements.size (); i++)
     /*
       As a sanity check, we make sure that no grob with an outside staff priority
       has a Y-parent that also has an outside staff priority, which would result
       in two movings.

[dak, 2013/12/11 13:44:25]

Huh?  Above we called a staff priority relation "sane" when the parent had a
smaller outside staff priority.  Now it must not have any outside staff
priority?

     */
     if (!sane_outside_staff_priority_parental_relationship (elements[i]))
        {
          elements[i]->warning ("An elements' Y parent must have a lower outside staff priority than the element.");

[dak, 2013/12/11 13:44:25]

Looks like the above comment got things wrong.

          elements[i]->set_property ("outside-staff-priority", ancestor_priority_plus_a_bit (elements[i]));
        }
 
   /* For grobs with an outside-staff-priority, the sorting function might
      call extent and cause suicide. This breaks the contract that is required
      for the STL sort function. To avoid this, we make sure that any suicides
      are triggered beforehand.

[dak, 2013/12/11 13:44:25]

But we do not _do_ any sorting here.  Why would we not call the extents before
we do the actual sorting rather than in an unrelated function?

   */
   for (vsize i = 0; i < elements.size (); i++)
     if (scm_is_number (elements[i]->get_property ("outside-staff-priority")))
       elements[i]->extent (elements[i], X_AXIS);
 }
 
 MAKE_SCHEME_CALLBACK (Axis_group_interface, vertical_skyline_elements, 1);

[dak, 2013/12/11 13:44:25]

There is no comment what this function returns, and it obviously does not merely
return the elements as its name would suggest.

 SCM
 Axis_group_interface::vertical_skyline_elements (SCM smob)
 {
   Grob *me = unsmob_grob (smob);
 
   extract_grob_set (me, "elements", origelements);
   vector<Grob *> elements (origelements);
   prepare_for_outside_staff_calculations (me);
   sort_outside_staff_elements (me, elements);
 
@@ skipping 35 matching lines @@
   vector<Grob *> outside_staff_elements;
 
   vsize i = 0;
 
   for (i = 0; i < elements.size ()
        && !scm_is_number (elements[i]->get_property ("outside-staff-priority")); i++)
     {
       Grob *elt = elements[i];
       if (!sane_outside_staff_priority_parental_relationship (elements[i]))
         {
           elt->programming_error ("Sorting function should have made sure that I have an outside-staff-priority although my parent does. Setting to parent's...");

[dak, 2013/12/11 13:44:25]

The error message does not make sense. "Sorting function should have made sure
that I have an outside-staff-priority although my parent does."?

           elt->set_property ("outside-staff-priority", ancestor_priority_plus_a_bit (elements[i]));
         }
       else
         inside_staff_elements.push_back (elt);
     }
 
   SCM directive
     = valid_outside_staff_placement_directive (me);
 
   bool l2r = ((directive == ly_symbol2scm ("left-to-right-greedy"))

[dak, 2013/12/11 13:44:25]

Actually, this strongly suggests that the "directive" should be split into two
properties, one being a Direction property, the other being a boolean.  It makes
no sense to conflate orthogonal properties into a single property just to pick
them apart afterwards.

               || (directive == ly_symbol2scm ("left-to-right-polite")));
 
   bool polite = ((directive == ly_symbol2scm ("left-to-right-polite"))
                  || (directive == ly_symbol2scm ("right-to-left-polite")));
 
 
   // We want to avoid situations like this:
   //           still more text
   //      more text
   //   text
@@ skipping 15 matching lines @@
   for (; i < elements.size (); i++)
     {
       if (to_boolean (elements[i]->get_property ("cross-staff")))
         continue;
 
       // Collect all the outside-staff grobs that have a particular priority.
       SCM priority = elements[i]->get_property ("outside-staff-priority");
       vector<Grob *> current_elts;
       current_elts.push_back (elements[i]);
       while (i + 1 < elements.size ()
              && scm_is_eq (elements[i + 1]->get_property ("outside-staff-priority"), priority))

[dak, 2013/12/11 13:44:25]

Priorities are generally numeric and cannot reliably be compared with scm_is_eq.

         {
           if (!to_boolean (elements[i + 1]->get_property ("cross-staff")))
             current_elts.push_back (elements[i + 1]);
           ++i;
         }
 
     // this loop orders the current elements
     while (!current_elts.empty ())
       {
         Drul_array<Real> last_end (-infinity_f, -infinity_f);
         vector<Grob *> skipped_elements;
         for (vsize j = l2r ? 0 : current_elts.size ();

[dak, 2013/12/11 13:44:25]

This is rather inscrutable, and it is worth noting that this does not work
symmetrically since the sort order, as it has been established here, is _always_
according to the _left_ edge of a grob.  So for l->r placement, we are sorted
according to trailing edge, for r->l placement, according to the leading edge.

It also seems utterly pointless to go through the loop when polite == false. 
One can just return the array, potentially reversed.

Since r->l order is simulated by going reverse anyway, one can just do the
conditional reverse independent of polite, return on !polite and then do the
loop in forward direction either way.

              l2r ? j < current_elts.size () : j--;
              l2r ? j++ : 0)
           {
             Grob *elt = current_elts[j];
             Real horizon_padding
               = robust_scm2double (elt->get_property ("outside-staff-horizontal-padding"), 0.0);
             Interval x_extent = elt->extent (x_common, X_AXIS);
             x_extent.widen (horizon_padding);
 
             Direction dir = get_grob_direction (elt);
             if (dir == CENTER)
               {
                 warning (_ ("an outside-staff object should have a direction, defaulting to up"));
                 elt->set_property ("direction", scm_from_int (UP));
                 dir = UP;
               }
 
             if (x_extent[LEFT] <= last_end[dir] && polite)

[dak, 2013/12/11 13:44:25]

Shouldn't that be < here?

               {
                 skipped_elements.push_back (elt);
                 continue;
               }
             last_end[dir] = x_extent[RIGHT];
 
             outside_staff_elements.push_back (elt);
           }
         swap (current_elts, skipped_elements);
         skipped_elements.clear ();

[dak, 2013/12/11 13:44:25]

If we have r->l order here, the loop was run backwards, pushing in this
backwards order into skipped_elements.  However, the next loop will again be run
backwards, meaning that in the case of r->l ordering, skipped_elements will be
ordered the wrong way round.

       }
   }
 
   elements.clear ();
   elements.insert (elements.end (), inside_staff_elements.begin (), inside_staff_elements.end ());
   elements.insert (elements.end (), outside_staff_elements.begin (), outside_staff_elements.end ());
 }
 
 // It is tricky to correctly handle skyline placement of cross-staff grobs.
 // For example, cross-staff beams cannot be formatted until the distance between
@@ skipping 22 matching lines @@
 
 
   Drul_array<vector<Skyline_pair> > all_v_skylines;
 
   for (vsize i = 0; i < elements.size (); i++)
     {
       // don't do anything with grobs present
       // in the inside staff skyline
       Grob *elt = elements[i];
       Grob *ancestor = outside_staff_ancestor (elt);
       if (!ancestor && !scm_is_number (elt->get_property ("outside-staff-priority")))

[dak, 2013/12/11 13:44:25]

There is no point in calculating the outside_staff_ancestor when this element
has an outside-staff-priority.

         continue;
 
       Skyline_pair *v_orig = Skyline_pair::unsmob (elt->get_property ("vertical-skylines"));
       if (v_orig->is_empty ())
         continue;
 
       Skyline_pair v_skylines (*v_orig);
       // recursive in case the grobs' parent has a direction set but not the grob
       Direction dir = recursive_get_grob_direction (elt);
       v_skylines.shift (elt->relative_coordinate (x_common, X_AXIS));
@@ skipping 90 matching lines @@
                "pure-relevant-items "
                "pure-relevant-spanners "
                "pure-Y-common "
                "staff-affinity "
                "staff-grouper "
                "staff-staff-spacing "
                "system-Y-offset "
                "X-common "
                "Y-common "
               );