Re: PATCH: Lyrics break estimation of vertical spacing

[Joe Neeman]

On Wed, 2010-03-31 at 01:46 -0400, Boris Shingarov wrote:
> Wooo, hang on, my patch is sour!  It segfaults if the system is not a 
> group of staves -- for example, this is the case of markup.  I'm 
> fixing it.

Ok, so I'll save the full review for later, but here are a few quick
things I noticed:

> @@ -512,7 +524,6 @@ Line_details::Line_details (Prob *pb, Output_def
> *paper)
>  
>    last_column_ = 0;
>    force_ = 0;
> -  extent_ = unsmob_stencil (pb->get_property ("stencil")) ->extent

You'll need to do something better here, rather than just leaving all
the extent information out for markup blocks.

>  (Y_AXIS);
>    bottom_padding_ = 0;
>    space_ = 0.0;
>    inverse_hooke_ = 1.0;
> @@ -530,3 +541,33 @@ Line_details::Line_details (Prob *pb, Output_def
> *paper)
>    SCM first_scm = pb->get_property ("first-markup-line");
>    first_markup_line_ = to_boolean (first_scm);
>  }
> +
> +/*
> + * I measure hanging from top of page.  It is positive for everything
> + * below the top of page.  Lower things have bigger hanging.
> + * NB!!! These hangings are artificial in that they do not take into
> + * account any padding/spacing.
hangings need to take padding (but not spacing) into account, because
padding is non-stretchable space. See (the old version of)
Page_breaking::min_page_count, which uses padding.

>   They are as if systems were stacked
> + * on top of each other; as such, hangings are only used/useful for
> the
> + * calculation of ext_len in Page_breaking.
> + */
> +void Line_details::compute_hangings (double previous_begin, double
> previous_rest)
we use Real rather than double (not quite sure why, but let's be
consistent). Also, the code style is

void
Line_details::compute_hangings ...

> +{
> +  double a = begin_of_line_extent_[UP];
> +  double b = rest_of_line_extent_[UP];
> +  double midline_hanging = max (previous_begin + a, previous_rest +
> b);
> +  hanging_begin_ = midline_hanging - begin_of_line_extent_[DOWN];
> +  hanging_rest_ = midline_hanging - rest_of_line_extent_[DOWN];
> +}
> +
> +double Line_details::hanging ()
double
Line_details::hanging ()

> +{
> +  return max (hanging_begin_, hanging_rest_);
> +}
> +
> +double Line_details::full_height ()
> +{
> +  Interval ret;
> +  ret.unite(begin_of_line_extent_);
> +  ret.unite(rest_of_line_extent_);
> +  return ret.length();
> +}
> diff --git a/lily/include/constrained-breaking.hh
> b/lily/include/constrained-breaking.hh
> index e6d898e..1f0e952 100644
> --- a/lily/include/constrained-breaking.hh
> +++ b/lily/include/constrained-breaking.hh
> @@ -27,7 +27,11 @@
>  struct Line_details {
>    Grob *last_column_;
>    Real force_;
> -  Interval extent_;   /* Y-extent of the system */
> +  Interval begin_of_line_extent_;
> +  Interval rest_of_line_extent_;
> +  double hanging_begin_;
> +  double hanging_rest_;
> +  double y_extent_; /* Y-extent, adjusted according to
When do you set this? Doesn't full_height() remove the need for this
anyway?

>  begin/rest-of-line*/
>  
>    Real padding_;  /* compulsory space after this system (if we're not
>                      last on a page) */
> @@ -78,6 +82,16 @@ struct Line_details {
>    }
>  
>    Line_details (Prob *pb, Output_def *paper);
> +
> +  /*
> +   * Pure procedure.
> +   * Based on the arguments which indicate how low the previous
> system
> +   * hangs, and on the internal state (*_of_line_extents), store into
> +   * internal state (hanging_*) how low our own low margin hangs.
> +   */
> +  void compute_hangings (double previous_begin, double
> previous_rest);
> +  double hanging ();
> +  double full_height ();
>  };
>  
>  /*
> diff --git a/lily/include/system.hh b/lily/include/system.hh
> index 509a65d..b8fa664 100644
> --- a/lily/include/system.hh
> +++ b/lily/include/system.hh
> @@ -65,9 +65,15 @@ public:
>    void typeset_grob (Grob *);
>    void pre_processing ();
>  
> +  Interval begin_of_line_pure_height (vsize start, vsize end);
> +  Interval rest_of_line_pure_height (vsize start, vsize end);
> +
>  protected:
>    virtual void derived_mark () const;
>    virtual Grob *clone () const;
> +
> +private:
> +  Interval part_of_line_pure_height (vsize start, vsize end, bool
> begin);
>  };
>  
>  void set_loose_columns (System *which, Column_x_positions const
> *posns);
> diff --git a/lily/page-breaking.cc b/lily/page-breaking.cc
> index 7dbac44..1741f5c 100644
> --- a/lily/page-breaking.cc
> +++ b/lily/page-breaking.cc
> @@ -101,8 +101,6 @@ compress_lines (const vector<Line_details> &orig)
>           Line_details compressed = orig[i];
>           Real padding = orig[i].title_ ? old.title_padding_ :
> old.padding_;
>  
> -         compressed.extent_[DOWN] = old.extent_[DOWN];
> -         compressed.extent_[UP] = old.extent_[UP] +
> orig[i].extent_.length () + padding;
>           compressed.space_ += old.space_;
>           compressed.inverse_hooke_ += old.inverse_hooke_;
>  
> @@ -853,11 +851,27 @@ Page_breaking::min_page_count (vsize
> configuration, vsize first_page_num)
>  
>    for (vsize i = 0; i < cached_line_details_.size (); i++)
>      {
> -      Real ext_len = cached_line_details_[i].extent_.length ();
> +      double previous_hanging_begin = 0;
> +      double previous_hanging_rest = 0;
> +      if (i > 0)
> +      {
indent the brace:
if (i > 0)
  {
    foo;
  }

> +        previous_hanging_begin =
> cached_line_details_[i-1].hanging_begin_;
> +        previous_hanging_rest =
> cached_line_details_[i-1].hanging_rest_;
> +      }
> +      cached_line_details_[i].compute_hangings
> (previous_hanging_begin, previous_hanging_rest);
> +      double prev_hanging = 0;
> +      if (i > 0)
> +      {
> +        prev_hanging = cached_line_details_[i-1].hanging ();
> +      }
> +      Real ext_len = cached_line_details_[i].hanging () -
> prev_hanging;
> +      cached_line_details_[i].y_extent_ = ext_len;
> +
>        Real padding = 0;
>        if (cur_rod_height > 0)
> -       padding = cached_line_details_[i].title_ ?
> -         cached_line_details_[i-1].title_padding_ :
> cached_line_details_[i-1].padding_;
> +        padding = cached_line_details_[i].title_ ?
> +          cached_line_details_[i-1].title_padding_ :
> +          cached_line_details_[i-1].padding_;
>  
>        Real next_rod_height = cur_rod_height + ext_len + padding;
>        Real next_spring_height = cur_spring_height +
> cached_line_details_[i].space_;
> @@ -870,6 +884,8 @@ Page_breaking::min_page_count (vsize
> configuration, vsize first_page_num)
>           || (i > 0
>               && cached_line_details_[i-1].page_permission_ ==
> ly_symbol2scm ("force")))
>         {
> +         // ok, this page is filled, move to the beginning of next
> page
> +         cached_line_details_[i].compute_hangings (0, 0);
>           line_count =
> cached_line_details_[i].compressed_nontitle_lines_count_;
>           cur_rod_height = ext_len;
>           cur_spring_height = cached_line_details_[i].space_;
> @@ -909,7 +925,7 @@ Page_breaking::min_page_count (vsize
> configuration, vsize first_page_num)
>    if (!too_few_lines (line_count - cached_line_details_.back
> ().compressed_nontitle_lines_count_)
>        && cur_height > cur_page_height
>        /* don't increase the page count if the last page had only one
> system */
> -      && cur_rod_height > cached_line_details_.back ().extent_.length
> ())
> +      && cur_rod_height > cached_line_details_.back ().full_height
> ())
>      ret++;
>  
>    assert (ret <= cached_line_details_.size ());
> @@ -1388,7 +1404,8 @@ Page_breaking::min_whitespace_at_top_of_page
> (Line_details const &line) const
>                                           ly_symbol2scm ("padding"));
>  
>    // FIXME: take into account the height of the header
> -  return max (0.0, max (padding, min_distance - line.extent_[UP]));
> +  double translate = max (line.begin_of_line_extent_[UP],
> line.rest_of_line_extent_[UP]);
> +  return max (0.0, max (padding, min_distance - translate));
>  }
>  
>  Real
> @@ -1406,7 +1423,8 @@ Page_breaking::min_whitespace_at_bottom_of_page
> (Line_details const &line) const
>                                           ly_symbol2scm ("padding"));
>  
>    // FIXME: take into account the height of the footer
> -  return max (0.0, max (padding, min_distance + line.extent_[DOWN]));
> +  double translate = min (line.begin_of_line_extent_[DOWN],
> line.rest_of_line_extent_[DOWN]);
> +  return max (0.0, max (padding, min_distance + translate));
>  }
>  
>  int
> diff --git a/lily/page-spacing.cc b/lily/page-spacing.cc
> index f78cc17..3f2bf45 100644
> --- a/lily/page-spacing.cc
> +++ b/lily/page-spacing.cc
> @@ -52,7 +52,7 @@ Page_spacing::append_system (const Line_details
> &line)
>  
>    rod_height_ += line.title_ ? last_line_.title_padding_ :
> last_line_.padding_;
>  
> -  rod_height_ += line.extent_.length ();
> +  rod_height_ += line.y_extent_;
You'll need to use the same hanging logic here as you do in
min_page_count. Otherwise, min_page_count will be able to pack pages
tighter than the page-spacer can space them, which will cause problems.
Same for space_systems_on_2_pages and space_systems_on_1_page.

>    spring_len_ += line.space_;
>    inverse_spring_k_ += line.inverse_hooke_;
>  
> @@ -69,7 +69,7 @@ Page_spacing::prepend_system (const Line_details
> &line)
>    else
>      last_line_ = line;
>  
> -  rod_height_ += line.extent_.length ();
> +  rod_height_ += line.y_extent_;
>    spring_len_ += line.space_;
>    inverse_spring_k_ += line.inverse_hooke_;
>  
> diff --git a/lily/system.cc b/lily/system.cc
> index 1be38ea..5d370fe 100644
> --- a/lily/system.cc
> +++ b/lily/system.cc
> @@ -570,6 +570,37 @@ System::get_extremal_staff (Direction dir,
> Interval const &iv)
>    return 0;
>  }
>  
> +Interval
> +System::part_of_line_pure_height (vsize start, vsize end, bool begin)
> +{
> +  Grob *alignment = get_vertical_alignment (); //TODO check for null
please check for null