LilyIssues / Issues / #3342 page breaking regression

Google Importer - 2013-05-01

Originally posted by: adam.spi...@gmail.com

Why do I keep getting assigned as owner of issues which aren't related to me?

Owner: ---

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-05-01

Originally posted by: k-ohara5...@oco.net

metronome/tempo marks: accurate initial placment, for page breaking

http://codereview.appspot.com/9073043

Labels: Patch-new
Owner: k-ohara5...@oco.net
Status: Started

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-05-01

Originally posted by: dak@gnu.org

@Adam: because you are first in the alphabetic list of possible issue owners, and the web interface is so awfully clunky that it is easy to trigger the first menu entry by accident even when you wanted a different one. Happened to me a few times as well.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-05-01

Originally posted by: dak@gnu.org

Patchy the autobot says: passes tests. No visible changes when testing. How to check?

Labels: -Patch-new Patch-review

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-05-01

Originally posted by: k-ohara5...@oco.net

We have tests like ‘page-breaking-outside-staff-estimation*.ly’ that reveal when the page-breaker estimation is off enough to change the number of pages used. The tiny adjustment in this patch doesn't change those, though.

Probably I shouldn't tweak layout changes just to make the (admittedly crude) page-breaker estimate the same. I just looked into this trying to figure out what caused the change reported here <https://lists.gnu.org/archive/html/lilypond-user/2013-05/msg00000.html> that shows a 5-staff-space change in the estimated height of one system. There aren't any visible \tempo or \marks there, but an invisible \mark with some overrides is the only explanation I could think of.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-05-01

Originally posted by: k-ohara5...@oco.net

It looks like this was the problem reported on -user,
now at <https://lists.gnu.org/archive/html/bug-lilypond/2013-05/msg00009.html>

The original example hid a tempo mark with 'tempoHideNote'. The example below shows the page-breaker estimates before and after the change for issue 3279. The extent-estimate is the height of the staff estimated for page-breaking; the staff and its notes extend from 0 down to -7.6.

Before tempo-spacing (image left) the tempo marks were assumed to sit on top of the staff-lines, so there is no allowance for the tempo. After tempo-spacing (image right) the tempo marks are assumed to sit on top of the highest notes on the line-broken line (calculated for each tentative line-break solution) *plus* a little bit to clear any padding applied at the note-spacing step. (If there are several tempo marks on a line they are assumed to set beside each other, not on top of each other, and their estimated heights do not accumulate.)

The allowance is too small before the patch for issue 3279, and in the special case of an empty tempo mark, too much after the issue 3279 patch by the amount of the padding and extra allowance. The patch above for this issue cuts the extra 1.5 staff-space down to 0.8 staff-space.

\paper{annotate-spacing = ##t ragged-right = ##t
indent = 2\cm short-indent = \indent }
{ \clef bass
\tempo "Vif" 2=80 g'1 \break
\tempo 2=80 g'1 \break
\set Score.tempoHideNote = ##t
\tempo "Vif" 2=80 g'1 \break
\tempo 2=80 g'1 }

3342.png

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-05-04

Originally posted by: k-ohara5...@oco.net

Revert spacing of tempo marks
Leave \markLengthOn available to users

http://codereview.appspot.com/9210044

Labels: -Patch-review Patch-new

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-05-04

Originally posted by: k-ohara5...@oco.net

The bug reported in the email linked to comment 6 used something like
<<\new Staff \with {
    \RemoveEmptyStaves
    \override VerticalAxisGroup #'remove-first = ##t
} [r1]*6
\new Staff { \clef bass
\tempo "Vif" 2=80 g1 \break
\tempo 2=80 g1 \break
\set Score.tempoHideNote = ##t
\tempo "Vif" 2=80 g1 \break
\tempo 2=80 g1 } >>
During page-breaking planning the staff with the notes is pushed down by the minimum-distance to the empty staff above. Annotate-spacing shows the staves at negative positions. The \tempo mark is at the top of the System, at positive positions, and the estimated height of each System is overestimated by one minimum-distance between staves.

The patch in comment 7 goes back to the old way of sliding the \tempo down against the uppermost staff-symbol, before figuring horizontal spacing.   This patch supplies \markLengthOn as an option, not default, implemented in a less-pretty way that raises \tempo 4 spaces for horizontal-spacing purposes only, without increasing the system-height estimates for page-breaking. It is still a good thing to do, so I'll repeat the link
http://codereview.appspot.com/9210044

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-05-06

Originally posted by: dak@gnu.org

Patchy the autobot says: Lots of changes with text padding that generally look like an improvement except possibly size16 and size20 ("cantabile" shifted down). However, all fret diagrams shift a bit to the right, making the measures take more space without a reason that would seem connected to collision avoidance. Patch is rejected because it has nothing to do with issue 3342. Did you confuse reviews?

Labels: -Patch-new Patch-needs_work

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-05-06

Originally posted by: k-ohara5...@oco.net

https://codereview.appspot.com/9073043/

Labels: -Patch-needs_work Patch-new

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-05-07

Originally posted by: dak@gnu.org

Sorry, but same result as previously, and the downloaded patch is the one for the wrong issue. Presumably git-cl's issue scraper does not like https:, so I'll try the same with http: instead.

http://codereview.appspot.com/9073043/

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-05-07

Originally posted by: dak@gnu.org

Patchy the autobot says: Lots and lots of vertical layout changes, but related to marks and tempos. Not all of them are an improvement. For example, key-option-midi.ly gives the tempo mark more room, making for a nicer impression, while voice-2-midi and quantize-duration-2-midi cram it almost into notes. voice-4-midi and voice-5-midi are also worsened. It may seem strange that I mention the Midi regtests, but after all, they typically carry a \tempo mark. In fingering-directions.ly, the marks are _definitely_ crammed much too close to the fingerings. I am not enthused about how context-mod-with.ly stacks letters "T", "R" and "B" into sharps from the key signature, but "T" is definitely the worst of the three. In tablature-full-notation, the mark "A" in the last system creeps too close to a slur. In staff-map-voice-midi and staff-map-instrument-midi, the tempo marks come too close to music as well.

Labels: -Patch-new Patch-needs_work

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-05-07

Originally posted by: k-ohara5...@oco.net

Those changes are from reverting the troublesome commit from the patch for issue 3279. I got a complaint abut these changes in the other direction as well (https://code.google.com/p/lilypond/issues/detail?id=3279#c37)

Let's do this as a simple revert of [rb6f94447415dded7c6e146b41b6139fe76cb84c4]
"Tempo and Rehearsal marks horizontal positioning; issue 3279"

https://codereview.appspot.com/9073043/

Expect loss of padding between tempo marks and anything protruding beyond the staff lines.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-05-07

Originally posted by: k-ohara5...@oco.net

(No comment was entered for this change.)

Labels: -Patch-needs_work Patch-new

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-05-08

Originally posted by: pkx1...@gmail.com

Patchy the autobot says: Reg test diffs throws up problem in rest-positioning.ly see full diffs here https://www.yousendit.com/download/UVJoK2VuQVNHa05qQTlVag

Labels: -Patch-new Patch-needs_work

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-05-08

Originally posted by: k-ohara5...@oco.net

I forgot to mention that. This patch is a pure revert so it also removes the \markLengthOn function so tempo and rehearsal marks loose their space.

'rest-positioning.ly' was added after the patch that I am now reverting, and it used rehearsal marks to label the test cases, depending on the spacing to look nice. It is still readable, though, and the rests remain positioned as they were intended.

No work can be done on a simple revert.

Labels: -Patch-needs_work Patch-review

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-05-10

Originally posted by: pkx1...@gmail.com

Patch on countdown for May 13 - 06:00 GMT

Labels: -Patch-review Patch-countdown

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-05-13

Originally posted by: pkx1...@gmail.com

Patch counted down - please push

Labels: -Patch-countdown Patch-push

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-05-13

Originally posted by: k-ohara5...@oco.net

commit [refec448b936bd63d82571cf79f8435768605c66d]

Labels: -Patch-push Fixed_2_17_19
Status: Fixed

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-05-27

Originally posted by: Elu...@gmail.com

(No comment was entered for this change.)

Status: Verified

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

page breaking regression

Issue Tracker for LilyPond

Searches

Help

#3342 page breaking regression

Discussion