bug-lilypond
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: PATCH: Lyrics break estimation of vertical spacing


From: Boris Shingarov
Subject: Re: PATCH: Lyrics break estimation of vertical spacing
Date: Fri, 09 Apr 2010 02:30:40 -0400
User-agent: Webmail 5.0

What I am stuck on now, is the function
Align_interface::get_minimum_translations(). Could you explain the high-level design of what this is doing?
In all cases in the book I am trying to typeset, everything appears
to work A-ok, but on the "hara-kiri-pianostaff" regtest, this now returns
an empty vector of offsets when called from System::part_of_line_pure_height(),
but the corresponding vector of staves is non-empty, which causes
a segfault when trying to translate the 0th staff.
On Sun, 04 Apr 2010 11:47:48 -0700, Joe Neeman  wrote:
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
 >
 >
 >
 >






reply via email to

[Prev in Thread] Current Thread [Next in Thread]