Re: PATCH: Lyrics break estimation of vertical spacing
Boris Shingarov b...@shingarov.com writes: What makes me really depressed about the situation with pure-height, is that we have fixed a number of reasonable bugs in this area (intersystem begin/rest, overridden stem length, deprecated space, padding of markup -- these are the ones that I did in the immediate past, -- the slur fix from Joe yesterday, and I also recall a bunch of other fixes in this area in the last few months), but we are not only not closer to having reasonable trouble-free page layout, but starting to look at page overfill/underfill problems which are very deeply rooted in the nature of pure-height estimation. So much so that I am starting to think that sacrificing the benefit of linebreaking/pagebreaking integration in the sake of always running real (non-pure) height, would be the path to having a reasonable layout for our book. That is, calculate the line breaks disregarding page breaking; calculate tallness of all lines; then run the page breaking algorithm. In case the line breaking algorithm is TeX-like, namely using a shortest graph traversal algorithm (Viterbi), it would be a considerable improvement if the first pass did not establish the optimal breaks, but rather the maximum and minimum heights of material. Then the page breaker would pick its goals (avoid widows, avoid extra pages and so on), and the line breaking would be done again, this time using optimal breaks while keeping the page break goals focused. A complete separation of line- and page breaking makes it infeasible to avoid things like widows and clubs (single lines on one page) by picking better line breaks. That actually is a rather severe limitation with TeX: while TeX has penalties for widows and clubs (and hyphens at the end of a page), they come into play only once line breaking is already completed. With a rigid layout, there is nothing you can do to fix the previous bad decision. You can only call it foul and have TeX emit a warning. I think we need to be better than that. -- David Kastrup ___ bug-lilypond mailing list bug-lilypond@gnu.org http://lists.gnu.org/mailman/listinfo/bug-lilypond
Re: PATCH: Lyrics break estimation of vertical spacing
On Thu, 22 Apr 2010 13:39:35 -0700, Joe Neeman wrote: Never mind, I just did that hunk manually and pushed. Then, I am marking bug 1053 as fixed. ___ bug-lilypond mailing list bug-lilypond@gnu.org http://lists.gnu.org/mailman/listinfo/bug-lilypond
Re: PATCH: Lyrics break estimation of vertical spacing
On Wed, 21 Apr 2010 20:37:52 -0400, Boris Shingarov wrote: not closer to having reasonable trouble-free page layout, but starting to look at page overfill/underfill problems which are very deeply rooted in the nature of pure-height estimation. I meant Bug 1061 in particular. ___ bug-lilypond mailing list bug-lilypond@gnu.org http://lists.gnu.org/mailman/listinfo/bug-lilypond
Re: PATCH: Lyrics break estimation of vertical spacing
What makes me really depressed about the situation with pure-height, is that we have fixed a number of reasonable bugs in this area (intersystem begin/rest, overridden stem length, deprecated space, padding of markup -- these are the ones that I did in the immediate past, -- the slur fix from Joe yesterday, and I also recall a bunch of other fixes in this area in the last few months), but we are not only not closer to having reasonable trouble-free page layout, but starting to look at page overfill/underfill problems which are very deeply rooted in the nature of pure-height estimation. So much so that I am starting to think that sacrificing the benefit of linebreaking/pagebreaking integration in the sake of always running real (non-pure) height, would be the path to having a reasonable layout for our book. That is, calculate the line breaks disregarding page breaking; calculate tallness of all lines; then run the page breaking algorithm. ___ bug-lilypond mailing list bug-lilypond@gnu.org http://lists.gnu.org/mailman/listinfo/bug-lilypond
Re: PATCH: Lyrics break estimation of vertical spacing
Ok, latest patch is uploaded now. ___ bug-lilypond mailing list bug-lilypond@gnu.org http://lists.gnu.org/mailman/listinfo/bug-lilypond
Re: PATCH: Lyrics break estimation of vertical spacing
Hi Joe, Have you seen the last patchset I uploaded to Rietveld 3 days ago? Is there anything expected from me at this point to further the progress on this bug? ___ bug-lilypond mailing list bug-lilypond@gnu.org http://lists.gnu.org/mailman/listinfo/bug-lilypond
Re: PATCH: Lyrics break estimation of vertical spacing
Oops, I had missed it. Reviewed now. On Mon, 2010-04-19 at 19:48 -0400, Boris Shingarov wrote: Hi Joe, Have you seen the last patchset I uploaded to Rietveld 3 days ago? Is there anything expected from me at this point to further the progress on this bug? ___ bug-lilypond mailing list bug-lilypond@gnu.org http://lists.gnu.org/mailman/listinfo/bug-lilypond
Re: PATCH: Lyrics break estimation of vertical spacing
On 12 April 2010 18:46, Boris Shingarov b...@shingarov.com wrote: Ok, this patch is ready for code review: http://codereview.appspot.com/872044 Can you add this to the previous patch set so it's easier to see what's changed since the last review? Thanks, Neil ___ bug-lilypond mailing list bug-lilypond@gnu.org http://lists.gnu.org/mailman/listinfo/bug-lilypond
Re: PATCH: Lyrics break estimation of vertical spacing
On Mon, 12 Apr 2010 20:57:00 0100, Neil Puttock wrote: On 12 April 2010 18:46, Boris Shingarov b...@shingarov.com wrote: Ok, this patch is ready for code review: http://codereview.appspot.com/872044 Can you add this to the previous patch set so it's easier to see what's changed since the last review? This is the whole patch. I understand your idea, but how do I add it to the previous set? Git will not help here, because neither of the two were committed yet. ___ bug-lilypond mailing list bug-lilypond@gnu.org http://lists.gnu.org/mailman/listinfo/bug-lilypond
Re: PATCH: Lyrics break estimation of vertical spacing
On 12 April 2010 21:03, Boris Shingarov b...@shingarov.com wrote: This is the whole patch. I understand your idea, but how do I add it to the previous set? Git will not help here, because neither of the two were committed yet. If you use the same test branch as your original patch (which is associated with a particular issue on Rietveld), you can just apply the revised patch and upload again (git-cl will ask you for a description which becomes the title of the new patch set on Rietveld.) Cheers, Neil ___ bug-lilypond mailing list bug-lilypond@gnu.org http://lists.gnu.org/mailman/listinfo/bug-lilypond
Re: PATCH: Lyrics break estimation of vertical spacing
Hi Neil, If you use the same test branch as your original patch (which is associated with a particular issue on Rietveld), you can just apply the revised patch and upload again (git-cl will ask you for a description which becomes the title of the new patch set on Rietveld.) I am not sure I am following, esp. the git-cl part. The state of the test branch right now is simply a clone of the HEAD. I had made some changes, then ran the upload.py script to generate the first patch. This was without any commits. Then, I made some corrections, and re-ran upload.py again -- this is the revised patch. Sure I can do a diff of the two patches, but how do I get that diff to Rietveld? ___ bug-lilypond mailing list bug-lilypond@gnu.org http://lists.gnu.org/mailman/listinfo/bug-lilypond
Re: PATCH: Lyrics break estimation of vertical spacing
Am Montag, 12. April 2010 22:43:30 schrieb Boris Shingarov: Hi Neil, If you use the same test branch as your original patch (which is associated with a particular issue on Rietveld), you can just apply the revised patch and upload again (git-cl will ask you for a description which becomes the title of the new patch set on Rietveld.) I am not sure I am following, esp. the git-cl part. The state of the test branch right now is simply a clone of the HEAD. I had made some changes, then ran the upload.py script to generate the first patch. Ah, you might want to install git-cl, which is a git interface to the upload.py script. It stores the issue for you current branch, and when you do a second upload on the same branch, it will reuse that issue number... This was without any commits. Then, I made some corrections, and re-ran upload.py again -- this is the revised patch. Sure I can do a diff of the two patches, but how do I get that diff to Rietveld? You don't have to do that yourself. Just upload the new patch to the same issue on Rietveld, and the server will create the diff of the patches for you... Cheers, Reinhold -- -- Reinhold Kainhofer, reinh...@kainhofer.com, http://reinhold.kainhofer.com/ * Financial Actuarial Math., Vienna Univ. of Technology, Austria * http://www.fam.tuwien.ac.at/, DVR: 0005886 * LilyPond, Music typesetting, http://www.lilypond.org ___ bug-lilypond mailing list bug-lilypond@gnu.org http://lists.gnu.org/mailman/listinfo/bug-lilypond
Re: PATCH: Lyrics break estimation of vertical spacing
On Mon, Apr 12, 2010 at 1:43 PM, Boris Shingarov b...@shingarov.com wrote: If you use the same test branch as your original patch (which is associated with a particular issue on Rietveld), you can just apply the revised patch and upload again (git-cl will ask you for a description which becomes the title of the new patch set on Rietveld.) I am not sure I am following, esp. the git-cl part. You need to install git-cl. It's just a matter of cloning this repo and adding symlinks to git-cl and upload.py in your PATH. http://neugierig.org/software/git/index.cgi?url=git-cl/ -Patrick ___ bug-lilypond mailing list bug-lilypond@gnu.org http://lists.gnu.org/mailman/listinfo/bug-lilypond
Re: PATCH: Lyrics break estimation of vertical spacing
Ah, you might want to install git-cl, which is a git interface to the upload.py script Thanks -- this does make life a lot easier!!! ___ bug-lilypond mailing list bug-lilypond@gnu.org http://lists.gnu.org/mailman/listinfo/bug-lilypond
Re: PATCH: Lyrics break estimation of vertical spacing
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
Re: PATCH: Lyrics break estimation of vertical spacing
On Fri, 2010-04-09 at 02:30 -0400, Boris Shingarov wrote: 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? The function builds a skyline for each staff/lyrics/etc, then compares the skylines to find the minimum amounts that each staff must be moved down so that no collisions occur. 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. get_skylines modifies its elems argument, removing all staves that are empty. If every staff is empty (which happens in hara-kiri-pianostaff, IIRC) then get_skylines doesn't return any skylines and hence get_minimum_translations (see align-interface.cc:226) returns an empty vector. I'm not actually sure why it does this (instead of returning, say, a vector of zeros) so if the regression tests pass, I'd be ok with changing this. Alternatively, you could just check explicitly for an empty result in System::part_of_line_pure_height. Cheers, Joe ___ bug-lilypond mailing list bug-lilypond@gnu.org http://lists.gnu.org/mailman/listinfo/bug-lilypond
Re: PATCH: Lyrics break estimation of vertical spacing
On Mon, 2010-04-05 at 03:00 -0400, Boris Shingarov wrote: Grob *alignment = get_vertical_alignment (); //TODO check for null please check for null But what do we do if it's null? Maybe print a programming_error and return an empty extent? It doesn't particularly matter what you do, just don't crash. Joe ___ bug-lilypond mailing list bug-lilypond@gnu.org http://lists.gnu.org/mailman/listinfo/bug-lilypond
Re: PATCH: Lyrics break estimation of vertical spacing
On Thu, 08 Apr 2010 23:47:15 -0700, Joe Neeman wrote: But what do we do if it's null? Maybe print a programming_error and return an empty extent? It doesn't particularly matter what you do, just don't crash. But it's never null, on any input that I've seen. Not on any of the regtests nor on our book anyway. So ok, I'll do as you suggest, but I'll add a comment explaining that this is somewhat arbitrary. ___ bug-lilypond mailing list bug-lilypond@gnu.org http://lists.gnu.org/mailman/listinfo/bug-lilypond
Re: PATCH: Lyrics break estimation of vertical spacing
Alternatively, you could just check explicitly for an empty result in System::part_of_line_pure_height. And if it's empty, then not do the translation at all, right? Will do this right now and see how much better this gets us. BTW, what is the correct (supposed) way to run the regtests? I am just running Lilypond on all the files from a bash script, as I can't seem to find the script that would be supposed to run the suite -- if such a beast exists. ___ bug-lilypond mailing list bug-lilypond@gnu.org http://lists.gnu.org/mailman/listinfo/bug-lilypond
Re: PATCH: Lyrics break estimation of vertical spacing
On Fri, 2010-04-09 at 02:55 -0400, Boris Shingarov wrote: Alternatively, you could just check explicitly for an empty result in System::part_of_line_pure_height. And if it's empty, then not do the translation at all, right? Will do this right now and see how much better this gets us. BTW, what is the correct (supposed) way to run the regtests? I am just running Lilypond on all the files from a bash script, as I can't seem to find the script that would be supposed to run the suite -- if such a beast exists. Checkout a known good commit (eg. origin/master) make test-baseline git checkout my-branch make make test-clean make check There's probably something in the contributor's guide about this... Joe ___ bug-lilypond mailing list bug-lilypond@gnu.org http://lists.gnu.org/mailman/listinfo/bug-lilypond
Re: PATCH: Lyrics break estimation of vertical spacing
Hi Joe,
You'll need to do something better here, rather than just leaving all
the extent information out for markup blocks.
Right, this is exactly why that oldest version of the patch was segfaulting.
The second version (attached to the bug and formatted
on codreview.appspot, also linked to from the bug) has this corrected.
* 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.
Correct; the code does do it, the meaning of the comment is
to warn that it happens not in the hanging method but in its caller;
this is ok because the hangings are only used to calculate the rod heights.
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).
I did put some thought into this;
it appears that in some places we actually use double,
and I *think* the pattern is, Real for allowing /-Inf,
double when we want to specifilally allow proper finite values only.
I this case, the method demands the value of the argument to be
finite, this is why I chose double.
Also, the code style is
void
Line_details::compute_hangings ...
Thanks, corrected.
double y_extent_; /* Y-extent, adjusted according to
When do you set this?
In min_page_count().
Doesn't full_height() remove the need for this
anyway?
No, full_height() is different.
full_height() is similar to the old unify() of the begin/rest;
y_extent_ is the fine-adjusted depending on the surrounding lines
(well, the previous line to speak correctly).
if (i 0)
{
indent the brace:
if (i 0)
{
foo;
}
Thanks.
- 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.
Thanks, I missed this!
Grob *alignment = get_vertical_alignment (); //TODO check for null
please check for null
But what do we do if it's null?
Boris
___
bug-lilypond mailing list
bug-lilypond@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-lilypond
Re: PATCH: Lyrics break estimation of vertical spacing
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 vectorLine_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
Re: PATCH: Lyrics break estimation of vertical spacing
Hi Graham, If you send a minimal example to this list, and no developer responds within a few days to begin discussing a patch, then I would expect somebody to add it to the tracker. I'm not certain if that has always happened, though. So in other words, not all bugs have bug reports in the tracker? If the discussion takes a technical turn from the very beginning, then we just discuss the patch through into the codebase, but no trail in the tracker remains? Hmm. I'd find it useful if we documented and kept trail of all bugs in the tracker. You know, putting tags on pushes in the form fix for bug #XYZ, or if someone asks what does this code do you can reply see bug #XYZ, that kind of stuff. ___ bug-lilypond mailing list bug-lilypond@gnu.org http://lists.gnu.org/mailman/listinfo/bug-lilypond
Re: PATCH: Lyrics break estimation of vertical spacing
У ср, 2010-03-31 у 02:05 -0400, Boris Shingarov пише:
Hi Graham,
If you send a minimal example to this list, and no developer
responds within a few days to begin discussing a patch, then I
would expect somebody to add it to the tracker. I'm not certain
if that has always happened, though.
So in other words, not all bugs have bug reports in the tracker?
If the discussion takes a technical turn from the very beginning,
then we just discuss the patch through into the codebase, but
no trail in the tracker remains?
Hi, Boris!
As one of persons who ideally should track all these discussions and
submit proper bugreports, i can say that i can not decide in every case
where it is a bug and where it is not. But if you can provide a minimal
example, it will be easier.
See also:
http://lilypond.org/doc/v2.13/Documentation/web/bug-reports
I understand that probably not every case may be illustrated by minimal
example --- that is the difficulty for Bug Squad and for developers, i
guess.
ps.
In this particular case --- i read every post, i assume there can be a
bug somewhere, but for me --- it's hard to say *what* is a bug, *where*
is it. You say: Lyrics break estimation of vertical spacing --- so,
no-lyrics.ly should contain no unwanted vertical space? But it does,
with 2.13.16. Or i've missed something?
Next, i've commented out everything in \paper {} block (left paper-size
alone) and problem disappeared. So, how this bug should be titled?
Lyrics... breaks.. *if* some other conditions or like that?
I'd *love* to submit a bugreport :O) But i can not, sorry :-(
pps. Melody in your example can be minimized to
\repeat unfold 392 { f e }
It's quote good for minimal example, i believe.
Thank You!
Hmm. I'd find it useful if we documented and kept trail of all
bugs in the tracker. You know, putting tags on pushes in the
form fix for bug #XYZ, or if someone asks what does this
code do you can reply see bug #XYZ, that kind of stuff.
--
Dmytro O. Redchuk
___
bug-lilypond mailing list
bug-lilypond@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-lilypond
Re: PATCH: Lyrics break estimation of vertical spacing
On Wed, Mar 31, 2010 at 7:05 AM, Boris Shingarov b...@shingarov.com wrote: Hi Graham, ... I would expect somebody to add it to the tracker. I'm not certain if that has always happened, though. So in other words, not all bugs have bug reports in the tracker? It depends on how well the Bug Squad have been doing their job. I'd estimate that between 90% and 100% of bugs have been correctly entered into the tracker. I'm doing everything I can to increase the number of people involved in lilypond, including the bug squad. If you would like to volunteer to help with this, I'd love to have you on board. As an aside to the general policy discussion, I'm comfortable giving you permission to add items directly to the tracker. If you'd like to do this, please send me the name of your gmail account (creating a new one if necessary). If the discussion takes a technical turn from the very beginning, then we just discuss the patch through into the codebase, but no trail in the tracker remains? Correct. Again, help acepted. Hmm. I'd find it useful if we documented and kept trail of all bugs in the tracker. You know, putting tags on pushes in the form fix for bug #XYZ, or if someone asks what does this code do you can reply see bug #XYZ, that kind of stuff. Well, the mailing list archives *are* available. But again, if we had more people on the Bug Squad, especially more technically-minded people, we could increase the scope of their coverage. Cheers, - Graham ___ bug-lilypond mailing list bug-lilypond@gnu.org http://lists.gnu.org/mailman/listinfo/bug-lilypond
Re: PATCH: Lyrics break estimation of vertical spacing
Hi Graham, I'm doing everything I can to increase the number of people involved in lilypond, including the bug squad. If you would like to volunteer to help with this, I'd love to have you on board. My involvement with Lilypond is pragmatic. I am enabling the publication of a top-quality, musicologically definitive, academic edition. (Not pretending to be a Berenreiter, it is, nevertheless, definitely a book of status). I need to do whatever I have to do to make sure this book happens. Therefore, I am interested in working on issues (bugs / missing / new functionality) that stand in the way of this book. Right now, the are many code issues. I need to have those resolved, and of course contribute these resoltions back to the community. This needs to happen according to proper software discipline. This is why I do care about properly documenting this particular bug in the tracker. you permission to add items directly to the tracker. If you'd like to do this, please send me the name of your gmail account (creating a new one if necessary). It's shingarov. I'll probably ask Dmytro for some direction with filing this bug, as I am new to the Lilypond bug procedure. Boris ___ bug-lilypond mailing list bug-lilypond@gnu.org http://lists.gnu.org/mailman/listinfo/bug-lilypond
Re: PATCH: Lyrics break estimation of vertical spacing
Hi Dmytro, If you will not add this issue at the tracker (as Graham mentioned), i guess i will. I'll do it, but I'll ask you qustions if I stumble upon something unclear, because I am new to the Lilypond bug procedure. Boris ___ bug-lilypond mailing list bug-lilypond@gnu.org http://lists.gnu.org/mailman/listinfo/bug-lilypond
Re: PATCH: Lyrics break estimation of vertical spacing
Hi Dmytro, where it is a bug and where it is not. But if you can provide a minimal example, it will be easier. The idea definitely is to provide a minimal example. Otherwise, it does not count as a bug report, and shouldn't really even be posted on the -bug list. The problem is that our understanding of a bug evolves with time -- it is usually very unclear at the beginning. So was the case with this particular bug. When I first started looking at this behavior to try to understand what was causing it, I only knew that the low-hanging lyrics were somehow responsible. So I named the email, Lyrics break estimation of vertical spacing. We now know exactly what the bug is caused by, and we know that this case with lyrics, is only one scenario. The file lyrics.ly is one minimal example. But the critical ingredient of the bug, is not lyrics, but anything hanging very low under the staff. It can be lyrics, or it can be notes on ledger lines: this is what my second example (no-lyrics.ly) illustrates. Now, is this name a good name for the bug report? No, it's not a good name. But I didn't change the subject line on the thread, because it was just a continuation of the technical discussion about how to fix the bug. Boris ___ bug-lilypond mailing list bug-lilypond@gnu.org http://lists.gnu.org/mailman/listinfo/bug-lilypond
Re: PATCH: Lyrics break estimation of vertical spacing
У ср, 2010-03-31 у 04:29 -0400, Boris Shingarov пише: Hi Dmytro, Hi Boris, If you will not add this issue at the tracker (as Graham mentioned), i guess i will. I'll do it, but I'll ask you qustions please do :-) if I stumble upon something unclear, because I am new to the Lilypond bug procedure. me too ;-) Boris -- Dmytro O. Redchuk ___ bug-lilypond mailing list bug-lilypond@gnu.org http://lists.gnu.org/mailman/listinfo/bug-lilypond
Re: PATCH: Lyrics break estimation of vertical spacing
On Wed, 31 Mar 2010 01:46:12 -0400, Boris Shingarov wrote: Here is the patch for the begin/rest-of-line problem. 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. ___ bug-lilypond mailing list bug-lilypond@gnu.org http://lists.gnu.org/mailman/listinfo/bug-lilypond
Re: PATCH: Lyrics break estimation of vertical spacing
У ср, 2010-03-31 у 03:47 -0400, Boris Shingarov пише: Hi Dmytro, Hi Boris, thank you for you explanation. If you will not add this issue at the tracker (as Graham mentioned), i guess i will. Anyway --- invalid issue report is not a huge problem, too .) As for me. where it is a bug and where it is not. But if you can provide a minimal example, it will be easier. The idea definitely is to provide a minimal example. Otherwise, it does not count as a bug report, and shouldn't really even be posted on the -bug list. The problem is that our understanding of a bug evolves with time -- it is usually very unclear at the beginning. So was the case with this particular bug. When I first started looking at this behavior to try to understand what was causing it, I only knew that the low-hanging lyrics were somehow responsible. So I named the email, Lyrics break estimation of vertical spacing. We now know exactly what the bug is caused by, and we know that this case with lyrics, is only one scenario. The file lyrics.ly is one minimal example. But the critical ingredient of the bug, is not lyrics, but anything hanging very low under the staff. It can be lyrics, or it can be notes on ledger lines: this is what my second example (no-lyrics.ly) illustrates. Now, is this name a good name for the bug report? No, it's not a good name. But I didn't change the subject line on the thread, because it was just a continuation of the technical discussion about how to fix the bug. Boris -- Dmytro O. Redchuk ___ bug-lilypond mailing list bug-lilypond@gnu.org http://lists.gnu.org/mailman/listinfo/bug-lilypond
PATCH: Lyrics break estimation of vertical spacing
Here is the patch for the begin/rest-of-line problem.
(Sorry, couldn't upload to appspot for various reasons).
I have also attached two score examples illustrating the bug.
We should probably augment the test suite, too, for the bug
to be considered done done.
Btw, how does one file a proper file report?
The bug tracker says scary things to the effect of never create
a bug report directly in this tracker, or risk being permanently
killed; instead, write to the -bug maillist. Well at least this is
my (mis-?)understanding of it. I have written numeruos reports
here, but never actually saw it result in a tracked report.
What is the correct procedure?
--
Boris Shingarov
Work on Lilypond under grant from Sonus Paradisi / Jiri Zurek (Prague),
Czech Science Foundation, Project No. 401/09/0419
diff --git a/lily/constrained-breaking.cc b/lily/constrained-breaking.cc
index 0db98cc..78079ef 100644
--- a/lily/constrained-breaking.cc
+++ b/lily/constrained-breaking.cc
@@ -458,7 +471,8 @@ Constrained_breaking::fill_line_details (Line_details *const out, vsize start, v
int start_rank = Paper_column::get_rank (all_[breaks_[start]]);
int end_rank = Paper_column::get_rank (all_[breaks_[end]]);
System *sys = pscore_-root_system ();
- Interval extent = sys-pure_height (sys, start_rank, end_rank);
+ Interval begin_of_line_extent = sys-begin_of_line_pure_height (start_rank, end_rank);
+ Interval rest_of_line_extent = sys-rest_of_line_pure_height (start_rank, end_rank);
Grob *c = all_[breaks_[end]];
out-last_column_ = c;
@@ -476,20 +490,18 @@ Constrained_breaking::fill_line_details (Line_details *const out, vsize start, v
out-turn_permission_ = min_permission (out-page_permission_,
out-turn_permission_);
- // TODO: see the hack regarding begin_of_line and
- // rest_of_line extents in align-interface. Perhaps we
- // should do the same thing here so that the effect extends
- // between systems as well as within systems. It isn't as
- // crucial here, however, because the effect is largest when
- // dealing with large systems.
- out-extent_ = (extent.is_empty ()
- || isnan (extent[LEFT])
- || isnan (extent[RIGHT]))
-? Interval (0, 0) : extent;
+ out-begin_of_line_extent_ = (begin_of_line_extent.is_empty ()
+ || isnan (begin_of_line_extent[LEFT])
+ || isnan (begin_of_line_extent[RIGHT]))
+? Interval (0, 0) : begin_of_line_extent;
+ out-rest_of_line_extent_ = (rest_of_line_extent.is_empty ()
+ || isnan (rest_of_line_extent[LEFT])
+ || isnan (rest_of_line_extent[RIGHT]))
+? Interval (0, 0) : rest_of_line_extent;
out-padding_ = between_system_padding_;
out-title_padding_ = before_title_padding_;
out-space_ = between_system_space_;
- out-inverse_hooke_ = extent.length () + between_system_space_;
+ out-inverse_hooke_ = out-full_height () + between_system_space_;
}
Real
@@ -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 (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. 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)
+{
+ 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 ()
+{
+ 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 begin/rest-of-line*/
Real padding_; /* compulsory space after this system (if we're not
last on a page) */
@@
Re: PATCH: Lyrics break estimation of vertical spacing
On Wed, Mar 31, 2010 at 01:46:12AM -0400, Boris Shingarov wrote: Btw, how does one file a proper file report? The bug tracker says scary things to the effect of never create a bug report directly in this tracker, or risk being permanently killed; instead, write to the -bug maillist. Well at least this is my (mis-?)understanding of it. I have written numeruos reports here, but never actually saw it result in a tracked report. The people who handle bug reports are normal users, and there's a natural tendency to be intimidated by discussions like this one. If a technical discussion is going on between a contributor (you) and a developer (Joe), we just tend to leave it alone. If you send a minimal example to this list, and no developer responds within a few days to begin discussing a patch, then I would expect somebody to add it to the tracker. I'm not certain if that has always happened, though. Cheers, - Graham ___ bug-lilypond mailing list bug-lilypond@gnu.org http://lists.gnu.org/mailman/listinfo/bug-lilypond
