Menu
▾
▴
#1470 Change keep-inside-line defaults to true
Originally created by: *anonymous
Originally created by: PhilEHol...@googlemail.com
By default, PaperColumn #'keep-inside-line and NonMusicalPaperColumn #'keep-inside-line are both false, with the result that LilyPond allows text to disappear off the edge of the paper. This is said to be in order to improve performance. In practice, on a real-world score of 30 pages, performance is only improved by 0.2%, and even with a contrived score with 30 pieces of text designed to overflow the page edge, performance is only 9% better with the poor layout of the text.
The default should be changed so that good layout with a slight performance hit is the standard behaviour. The learning manual will need changing at section 4.6.5 to mention a possible over-ride if performance is paramount.
Related
Discussion
Originally posted by: PhilEHol...@googlemail.com
Presumably this could be done quite simply by adding
\override PaperColumn #'keep-inside-line = ##t
\override NonMusicalPaperColumn #'keep-inside-line = ##t
to one of the *init.ly files? paper-defaults-init.ly would seem fairly appropriate.
Originally posted by: Carl.D.S...@gmail.com
No, the proper way to do it is to add a setting to each of the grobs PaperColumn and NonMusicalPaperColumn in scm/define-grobs.scm
Thanks,
Carl
Originally posted by: ahawry...@gmail.com
I have tested the edit below and it works great. I can make the appropriate changes in documentation; should I also remove all of the 'keep-inside-line' overrides from the docs and the relevant regression tests?
diff --git a/scm/define-grobs.scm b/scm/define-grobs.scm
index 02160ee..8e33f64 100644
--- a/scm/define-grobs.scm
+++ b/scm/define-grobs.scm
@@ -1306,6 +1306,7 @@
(horizontal-skylines . ,ly:separation-item::calc-skylines)
;; (stencil . ,ly:paper-column::print)
+ (keep-inside-line . #t)
(line-break-permission . allow)
(non-musical . #t)
(page-break-permission . allow)
@@ -1440,6 +1441,7 @@
(axes . (,X))
(before-line-breaking . ,ly:paper-column::before-line-breaking)
(horizontal-skylines . ,ly:separation-item::calc-skylines)
+ (keep-inside-line . #t)
;; (stencil . ,ly:paper-column::print)
(X-extent . ,ly:axis-group-interface::width)
Originally posted by: percival.music.ca@gmail.com
By "it works great", I assume that you have create a regtest comparison and can argue that every change is acceptable? ;)
http://lilypond.org/doc/v2.13/Documentation/contributor/identifying-code-regressions
That's the next step to take here. As a temporary measure, I would propose that you add:
keep-inside-line = ##f
to any regtest which changes significantly, unless it is absolutely obvious that the change is good. Then we can push that patch, while you (or anybody else who's interested in this) works on documentation, and examining regtests in more detail.
Originally posted by: ahawry...@gmail.com
This issue is an excellent frog task for me: I just built my first reg-test comparison.
While some of them are funny looking (and should probably be set back to keep-inside-line = ##f), they all make sense except the first one. Does anyone know why the 'mensural-ligatures' regression test would have been affected by keep-inside-line = ##t?
Originally posted by: benko....@gmail.com
> Does anyone know why the 'mensural-ligatures' regression test
> would have been affected by keep-inside-line = ##t?
mensural ligatures interact with horizontal spacing quite ghastly.
perhaps the last ligature was not out of bounds, but the added space was.
Originally posted by: ahawry...@gmail.com
Ah, that explanation of the mensural ligatures makes sense. Given that the horizontal spacing with keep-inside-line == ##t is no more odd than the original spacing, I don't see this as a problem. I will add keep-inside-line == ##f to a few of the regression tests and submit a patch.
Originally posted by: percival.music.ca@gmail.com
(No comment was entered for this change.)
Labels: Patch-new
Originally posted by: hanw...@gmail.com
@phil - I'm not sure the cost depends on whetehr the texts overflow or not. Can you check that this also does not have a lot of impact when you add many small texts?
Originally posted by: PhilEHol...@googlemail.com
In my initial report, I said "In practice, on a real-world score of 30 pages, performance is only improved by 0.2%, and even with a contrived score with 30 pieces of text designed to overflow the page edge, performance is only 9% better with the poor layout of the text."
So I conclude that there is only a small performance penalty when there is a lot of over-flowing text, and essentially zero with a normal piece of music.
Originally posted by: ahawry...@gmail.com
Here's the promised patch, including appropriate changes to the regression tests and docs.
The only change in the regression tests is test-output-distance.ly, which claims that it should be different every time as a test, no?
Originally posted by: percival.music.ca@gmail.com
lgtm, passes the regtests, but could we get this uploaded to reitveld?
Labels: -Patch-new Patch-review
Originally posted by: percival.music.ca@gmail.com
sorry, but I can't insist that other people review this without a reitveld issue.
Labels: -Patch-review Patch-needs_work
Originally posted by: percival.music.ca@gmail.com
ok, I uploaded it.
http://codereview.appspot.com/4243041
I'll add it to the next set of 48-hour patches.
Labels: -Patch-needs_work Patch-review
Originally posted by: percival.music.ca@gmail.com
Thanks, pushed.
Labels: -Patch-review fixed_2_13_52
Status: Fixed
Originally posted by: PhilEHol...@googlemail.com
Hoorah!
Status: Verified