Menu
▾
▴
2 Attachments
#3026 staff-padding should measure to the alignment-points
Originally created by: *anonymous
Originally created by: k-ohara5...@oco.net
Originally owned by: k-ohara5...@oco.net
The description at
LearningManual 4.5.2: staff-padding property
talks about aligning to a baseline, which would implies that we want the baselines of the objects on a common line.
The Internals documentation agrees that that was the plan:
staff-padding (dimension, in staff space)
Maintain this much space between reference points and the staff. Its effect is to align objects of differing sizes (like the dynamics p and f) on their baselines.
However, the code (Side_position_interface::aligned_side) applies staff-padding to the closest point of the object.
Originally posted by: mts...@gmail.com
This looks like a bigger problem...even without staff padding there is no alignment between the dynamics (at least below the staff). I'm not sure if there ever was. If there were, there'd be chain_offset_callback from DynamicText to DynamicLineSpanner at the engraving stage and a positioning-done callback for DynamicLineSpanner that uses Pango to grab baselines and align accordingly. A day-ish of work - can give guidance if needed.
Originally posted by: k-ohara5...@oco.net
Comment 1 sounds like the request of issue 1362.
Here, not just dynamics, but everything that has a property 'staff-padding' could be aligned on the baseline, as the documentation implies. This might need follow-up to facilitate the spacing of text that we really want above versus below the staff. (Maybe the text/lyrics baseline should be at the center of the x-height, for text that is laid out in music as opposed to titles.) In any event this will be post-2.18
http://codereview.appspot.com/7005056/
Labels: Patch-new
Originally posted by: k-ohara5...@oco.net
(No comment was entered for this change.)
Owner: k-ohara5...@oco.net
Originally posted by: pkx1...@gmail.com
Patchy still has trouble applying this to current master.
James
Labels: -Patch-new Patch-needs_work
Originally posted by: k-ohara5...@oco.net
(No comment was entered for this change.)
Blockedon: lilypond:3404
Labels: -Patch-needs_work Patch-waiting
Originally posted by: dak@gnu.org
(No comment was entered for this change.)
Blockedon: -lilypond:3404
Labels: -Patch-waiting Patch-new
Status: Started
Originally posted by: dak@gnu.org
Patchy the autobot says: Path does not apply.
Labels: -Patch-new Patch-needs_work
Originally posted by: k-ohara5...@oco.net
Rebased.
There are many small changes, especially in text. I have adjusted the default paddings, but not tweaked any regtests that might have depended on the old way of spacing.
http://codereview.appspot.com/7005056/
Labels: -Patch-needs_work Patch-new
Originally posted by: dak@gnu.org
Patchy the autobot says: Lots of spacing changes, usually increading distance between scripts and score. However, with woodwind-diagram-empty, diagrams get crammed on the score. The regtest fermata-rest-position states "Fermatas over multimeasure rests are positioned as over normal rests." but this is no longer the case: the normal rest fermata is lower. Similar difference with stencil-scale. harp-pedals-tweaking shows different lines, but those may be rasterization artifacts. Last system in outside-staff-placement-direction is staggered completely differently.
Labels: -Patch-new Patch-needs_work
Originally posted by: k-ohara5...@oco.net
If James can easily make a .zip archive as he or his computer often does, it might get me some other comments that might help me tweaking to get what we want.
Labels: -Patch-needs_work Patch-review
Originally posted by: dak@gnu.org
Well, the description claims padding/alignment to a baseline, and text markups mostly shift a bit upwards. However, if you take a look at actual scripts with a descender like in part-combine-force.ly, you'll see that nothing like that happens: compare the first "a2" with "apart" in the before/after shot.
Originally posted by: k-ohara5...@oco.net
That's a point. The different classes of text
TextScript
CombineTextScript
etc.
have different defaults, such as CombineTextScript is bold, and has independent staff-padding. In my proposed patch I increased the staff-padding to the TextScripts so that "apart" would sit at the same height as "chord" if there were no notes in the way, but saw no benefit to bring the visually-different bold "a2" in line with the normal-weight text. We could align them.
It is hard to pick a good default 'staff-padding with text, because a reasonable 'staff-padding to the baseline above the staff is closer than a reasonable distance below the staff. We could redefine the baseline of text to be at the center height of the 'x' in the font, so that 'staff-padding works symmetrically. If applied to Lyrics, it would simplify centering Lyrics between staves. (Different adjustments would be needed for other things that go in a TextScript: \fermataMarkup, \woodwind-diagrams, \fret-boards, etc.)
I suspect it wisest to remove the default 'staff-padding from TextScript (its current broken implementation makes it no different from 'padding) and let users apply it when they have a special case where it is useful.
Dynamics text, by contrast, has a baseline near its vertical center, so 'staff-padding will be very useful for the DynamicLineSpanner, when it works as documented.
Originally posted by: k-ohara5...@oco.net
(No comment was entered for this change.)
Labels: -Patch-review Patch-needs_work
Originally posted by: k-ohara5...@oco.net
I'm planning to fix issue 3255 and issue 2910 (outside-staff-padding) first, then this, for the sake of more understandable reg-test changes.
Originally posted by: dak@gnu.org
> I'm planning to fix issue 3255 and issue 2910 (outside-staff-padding) first, then this, for the sake of more understandable reg-test changes.
I am quite annoyed at the current level of spacing regression fixes going on. Not so much that people work on them, but rather that there is such a lot of them that has not been addressed for a whole lot of time.
Now this issue here looks like it's not a 5-minute dead-obvious thing, and it also looks like it is not as much a regression as a disgrace. In the interest of getting a release out, it seems prudent to postpone tackling it until 2.18 has been branched off: it very much looks like addressing this one will trigger several fine-tuning phases in its wake.
Originally posted by: k-ohara5...@oco.net
Comment #15
Just to be clear, this seems to be not _at_all_ a regression; we never found evidence that this worked as advertised.
The code fix is a "5-minute dead-obvious thing", actually, removing a blunder to make the code do what code comments say was intended. The broken 'staff-padding implementation is redundant with 'padding and very rarely has had effect.
My earlier patch was intended to see if people found it useful as an effective default on TextScripts (which would require followup because so many special-case markups live in TextScripts) but it seems not to be.
The workaround in the manual Notation Reference 1.3.1 works just as staff-padding was intended,
\override TextScript.Y-extent = #'(-1.5 . 1.5)
c2^\markup { \huge gorgeous } c^\markup { \huge fantastic }
because extents now have effect only in a few circumstances, one of them being spacing to the bare staff.
Originally posted by: k-ohara5...@oco.net
The patch above, but leaving the defaults alone wherever possible.
This enables setting TextScript.staff-padding = 2 to solve problems like the snippet "Vertically aligned dynamics and textscripts" http://lsr.dsi.unimi.it/LSR/Item?id=387 and gives "Vertically aligning dynamics across multiple notes" http://lsr.dsi.unimi.it/LSR/Item?id=450 the desired common baseline.
Expect 'woodwind-diagrams-empty.ly' to show the piccolo diagram crammed as close to the staff as the diagrams for other instruments. Trill spanners are now put the "tr" about as close to the staff as does \trill. 'dynamics-glyphs.ly' has the baselines line-up in s\mp\< s\mf
http://codereview.appspot.com/7005056/
Labels: -Patch-needs_work Patch-new
Originally posted by: pkx1...@gmail.com
Passes make, make check and a full make doc.
Reg test diffs here
https://www.hightail.com/download/OGhmRE9oZ1B0NjlqQTlVag
Labels: -Patch-new Patch-review
Originally posted by: k-ohara5...@oco.net
The patch changes the measurement point of a padding, so lots of tests change slightly. They all look fine to me.
The point of the patch is not to make any regression tests look better, but to make aligning things easier. See the docs changes at http://codereview.appspot.com/7005056/ to see if you like the patch.
Originally posted by: pkx1...@gmail.com
Patch on countdown for September 12 - 06:00 GMT
Trevor has some typo/corrections
Labels: -Patch-review Patch-countdown
Originally posted by: pkx1...@gmail.com
Patch counted down - please push
NB Trevor had some typo corrections.
Labels: -Patch-countdown Patch-push
Originally posted by: k-ohara5...@oco.net
I had intended to have this wait for 2.19, but now I think I should push sooner.
The workaround using Y-extent in one of the snippets is new after the skyline spacing patches, and we could avoid having a workaround in the documentation for just one stable release.
I need to add a convert-ly rule so people who used staff-padding in the past are not confused.
Labels: -Patch-push Patch-needs_work
Originally posted by: k-ohara5...@oco.net
added a convert-ly rule
Labels: -Patch-needs_work Patch-new
Originally posted by: dak@gnu.org
Patchy the autobot says: passes tests. Wagonloads of differences, likely mostly expected. No idea how I'm supposed to deal with them, however, but keeping stuck at Patch-new can't be the solution.
Labels: -Patch-new Patch-review
Originally posted by: k-ohara5...@oco.net
The changes to the patch are only documentation. The regression test changes are the same as in comments 17 and 18.