Issue 3330: redo much of the stencil stacking/spacing/empty-check

Issue 3330: redo much of the stencil stacking/spacing/empty-check

This contains a number of commits:


Stop quite weird positioning for dots

Some code here used a translation on dots that should never have had
an effect but causes problems in relation to the issue 3330 fix.


Use (+inf . -inf) consistently for empty stencil intervals


Make Box::is_empty more selective and create an axis-specific variant

Boxes are not really "empty" just because they show negative
advancement on some axis.  A box is empty if its original dimensions
of (+inf . -inf) are still unchanged for both axes.  And boxes can
make an impact in one axis but not another (pretty much the definition
of spacing), so is_empty can take an axis as argument now.


Let Stencil::is_empty use Box::is_empty

This is a change in semantics as an axis interval of '(1 . -1) is
considered empty as an interval, while it is not sufficient for making
a box axis empty.  Things like backspaces would have similar metrics
and are different from no information at all.


Create Stencil::is_empty (Axis)


Give ly:stencil-empty? an optional axis argument


Let Box::translate ignore empty axes


Rewrite Stencil::add_at_edge to treat "spacing" stencils differently

A spacing stencil is one that is non-empty in its own axis, but empty
in the orthogonal axis.  Since they don't actually have a bounding box
to convey, they don't need padding, and they don't suffer from the
equality of the lower left bounding box corner with the reference
point.


Let Stencil::align_to abort only if the stencil is empty in the alignment axis

Previously, the emptiness check was not axis specific.


Let stack-lines deal properly with vertical spacing (X-empty stencils)


simplify stack-stencils and rewrite stack-stencils-padding-list


implement stack-stencil-line in terms of stack-stencils


Let \hspace and \vspace use Y-empty and X-empty spacing, respectively


Keep buildings which are empty in either axis out of skylines


Issue 3330: bracketify-stencil moves grob's refpoint

Also fixes parenthesize-stencil.

[dak, 2013-04-28 13:16:27]

Forgot the original fix

[janek, 2013-04-28 14:50:06]

could you put this in a branch?  I strongly prefer reading git log --patch than
a single diff.

Janek

[dak, 2013-04-28 15:58:34]

On 2013/04/28 14:50:06, janek wrote:
> could you put this in a branch?  I strongly prefer reading git log --patch
than
> a single diff

Small wonder.  Pushed it into dev/issue3330

[dak, 2013-04-28 18:57:44]

Backspacing did not work

[Keith, 2013-04-28 20:04:30]

You are struggling because the code recursively combines the tail of a sequence
of stencils, before adding the first.

Presumably we want the first line to overstrike, and the second to 
  \markup{ "A" \hspace #-2.5 "--- the end" }
  \markup{ "A" \center-column {"up" "down"}}
but both attach the "A" to tails with the same shape
  [.A] [   .    ]

One way to distinguish the cases is to remember a cursor position, the
extent-start of the most-recently-added element in the tail, rather than use the
extent-start of the combined tail.  (More easily thought of in a loop
implementation, rather than recursion.)

[janek, 2013-04-28 21:59:37]

changes i see generally LGTM (but i may be missing something, in particular
Keith seems to make a good point).

> Let Stencil::is_empty use Box::is_empty [...]

I think the desription of this commit could use some more detail, for example
that we need (1 . -1) stencils for backspacing.

> Let Stencil::add_at_edge skip offsets
> for half-empty stencils [...]

I think "half-empty" is confusing (i at least was confused).  I think "empty
along only one axis" would be clearer.

> Let Stencil::align_to check axis-specific
> emptiness before punting

could you use a more odrinary word?  I had to check 'punt'
in a dictionary, and i consider my English to be fairly good.

https://codereview.appspot.com/8869044/diff/9001/lily/box.cc
File lily/box.cc (right):

https://codereview.appspot.com/8869044/diff/9001/lily/box.cc#newcode66
lily/box.cc:66: return interval_a_[a][LEFT] == empty[LEFT]
Do we need to define 'empty'?  I.e. why not just write
interval_a_[a][LEFT] == infinity_f ?
Are we expecting that the definition of empty extent will change further?

https://codereview.appspot.com/8869044/diff/9001/lily/stencil.cc
File lily/stencil.cc (right):

https://codereview.appspot.com/8869044/diff/9001/lily/stencil.cc#newcode253
lily/stencil.cc:253: 
whitespace error

https://codereview.appspot.com/8869044/diff/9001/lily/stencil.cc#newcode259
lily/stencil.cc:259: offset += d * padding;
I think this is worth a comment with an example.  It's not obvious why sometimes
we want padding and sometimes not.

https://codereview.appspot.com/8869044/diff/9001/scm/markup.scm
File scm/markup.scm (right):

https://codereview.appspot.com/8869044/diff/9001/scm/markup.scm#newcode67
scm/markup.scm:67: between the end of each stencil and the reference point of
the following
Hmm, shouldn't this read "between the end of the stencil and the *beginning* of
the following stencil"?

[Keith, 2013-04-28 22:16:14]

https://codereview.appspot.com/8869044/diff/9001/scm/define-markup-commands.scm
File scm/define-markup-commands.scm (right):

https://codereview.appspot.com/8869044/diff/9001/scm/define-markup-commands.s...
scm/define-markup-commands.scm:3765: * \\translate #'(2 . 3) translate
Probably this example should place the translated markup relative to music,
rather than in a line of \markup; otherwise, someone might experiment with this
and conclude that \translate cannot move things to the left.

I agree that it should be fine to have a line of markup space a \translated
piece so there is no overlap.  We have never been able to use \translate to
overlap a following piece of text in a line of markup.  All the examples of
\translate that I found in real scores apply it to the entire markup (to shift
the markup relative to its alignment point).

[dak, 2013-04-28 22:38:16]

On 2013/04/28 22:16:14, Keith wrote:
>
https://codereview.appspot.com/8869044/diff/9001/scm/define-markup-commands.scm
> File scm/define-markup-commands.scm (right):
> 
>
https://codereview.appspot.com/8869044/diff/9001/scm/define-markup-commands.s...
> scm/define-markup-commands.scm:3765: * \\translate #'(2 . 3) translate
> Probably this example should place the translated markup relative to music,
> rather than in a line of \markup; otherwise, someone might experiment with
this
> and conclude that \translate cannot move things to the left.

Well, the example works, and I'd like to avoid meddling with it right now
(making this issue even larger and slower to get right).

Changing it does not depend on this issue: does not matter whether it is
committed before or after if somebody wants to do it.

[dak, 2013-04-28 23:16:28]

On 2013/04/28 21:59:37, janek wrote:
> changes i see generally LGTM (but i may be missing something, in particular
> Keith seems to make a good point).
> 
> > Let Stencil::is_empty use Box::is_empty [...]
> 
> I think the desription of this commit could use some more detail, for example
> that we need (1 . -1) stencils for backspacing.

I disagree with that reasoning since it is backwards.  It's just simpler if
Stencil and Box use the same criteria (actually, I wanted to throw out the check
for the stencil expression being '() in Stencil::is_empty () as well, but that
tripped up something in the page breaker and it looked like it would take a lot
of time finding the cause).

This simplicity was part of the reason to change Box's is_empty tests.

> https://codereview.appspot.com/8869044/diff/9001/lily/box.cc
> File lily/box.cc (right):
> 
> https://codereview.appspot.com/8869044/diff/9001/lily/box.cc#newcode66
> lily/box.cc:66: return interval_a_[a][LEFT] == empty[LEFT]
> Do we need to define 'empty'?  I.e. why not just write
> interval_a_[a][LEFT] == infinity_f ?
> Are we expecting that the definition of empty extent will change further?

I am more worried that infinity_f might be defined with a different
type/precision than that stored in Interval, making the comparison fail.  It's
not like the C++ compiler would generate a lot of code here, and I can probably
even leave off the set_empty as it should be the default.

> https://codereview.appspot.com/8869044/diff/9001/lily/stencil.cc
> File lily/stencil.cc (right):
> 
> https://codereview.appspot.com/8869044/diff/9001

> https://codereview.appspot.com/8869044/diff/9001/lily/stencil.cc#newcode259
> lily/stencil.cc:259: offset += d * padding;
> I think this is worth a comment with an example.  It's not obvious why
sometimes
> we want padding and sometimes not.

The current version has a bit more here.

> https://codereview.appspot.com/8869044/diff/9001/scm/markup.scm
> File scm/markup.scm (right):
> 
> https://codereview.appspot.com/8869044/diff/9001/scm/markup.scm#newcode67
> scm/markup.scm:67: between the end of each stencil and the reference point of
> the following
> Hmm, shouldn't this read "between the end of the stencil and the *beginning*
of
> the following stencil"?

Depends on the version.  In the reviewed version, this is pretty accurate, but
now I've changed behavior again.

[dak, 2013-04-28 23:37:57]

Complete overhaul again, assembling stencils front to back

[dak, 2013-04-28 23:50:34]

Typo

[dak, 2013-04-28 23:54:12]

And another typo

[dak, 2013-04-29 01:07:04]

Fix chordname kerning

[dak, 2013-04-29 01:24:31]

Yet another typo

[Keith, 2013-04-29 03:31:14]

I worry a little about making the extent of markup different from the union of
its components.  Someone may have used  c4^\markup{\hspace #-3 "text"}  and the
left side of the "text" is now vulnerable to collisions.

The alternative is to use a variable to remember the cursor position in a loop
that assembles a line of markup.

In patch-set "yet another typo" the extent of the markup assembled-so-far is
used to record that cursor position, but in \markup {"_pple" \hspace#-6.5 "A"}
the you really don't want the markup extent to end the cursor position, after
the "A", because the "pple" extends farther right.

On 2013/04/28 22:38:16, dak wrote:
> On 2013/04/28 22:16:14, Keith wrote:
>
https://codereview.appspot.com/8869044/diff/9001/scm/define-markup-commands.s...
> > scm/define-markup-commands.scm:3765: * \\translate #'(2 . 3) translate
> > Probably this example should place the translated markup relative to music,
> > rather than in a line of \markup; otherwise, someone might experiment with
> > this and conclude that \translate cannot move things to the left.
> 
> Well, the example works, and I'd like to avoid meddling with it right now

The example might not work after your patch.
\translate #(x . y) moves the stencil relative to its reference point, but the
extents go with the stencil.  If you space a line of markup based on extents
(not reference points) then these examples with \translate #(x . y) in a line of
markup will show no visible effect of the x.

https://codereview.appspot.com/8869044/diff/27001/lily/stencil.cc
File lily/stencil.cc (right):

https://codereview.appspot.com/8869044/diff/27001/lily/stencil.cc#newcode257
lily/stencil.cc:257: bool separate = !is_empty (other_axis (a)) && !s.is_empty
(other_axis (a));
suggest  separate =>  !special_spacing

https://codereview.appspot.com/8869044/diff/27001/lily/stencil.cc#newcode266
lily/stencil.cc:266: // determined by their respective stencils to allow for
spacing
This is where the magic happens, so good to have some search-words in the
comment.

// If either stencil has special spacing, such as \hspace#3, 
// use the individual stencil extents for the corresponding
// limit of the resulting extent, to allow \hspace#-3 to backspace

https://codereview.appspot.com/8869044/diff/27001/scm/define-markup-commands.scm
File scm/define-markup-commands.scm (right):

https://codereview.appspot.com/8869044/diff/27001/scm/define-markup-commands....
scm/define-markup-commands.scm:3765: * \\translate #'(2 . 3) translate
might fail

https://codereview.appspot.com/8869044/diff/27001/scm/define-markup-commands....
scm/define-markup-commands.scm:3767: * \\translate-scaled #'(2 . 3)
translate-scaled
might fail

https://codereview.appspot.com/8869044/diff/27001/scm/define-markup-commands....
scm/define-markup-commands.scm:3898: Translate @var{arg} relative to its
surroundings.  @var{offset}
might become "... relative to its alignment point."

https://codereview.appspot.com/8869044/diff/27001/scm/define-markup-commands....
scm/define-markup-commands.scm:3904: \\translate #'(2 . 3)
might fail

https://codereview.appspot.com/8869044/diff/27001/scm/markup.scm
File scm/markup.scm (right):

https://codereview.appspot.com/8869044/diff/27001/scm/markup.scm#newcode69
scm/markup.scm:69: @var{space} before them and don't avoid overlapping other
stencils."
The last sentence of this comment should go with its implementation in
stencil.cc

[janek, 2013-04-29 07:26:50]

On 2013/04/29 03:31:14, Keith wrote:
> I worry a little about making the extent of markup different from the union of
> its components.  Someone may have used  c4^\markup{\hspace #-3 "text"}  and
the
> left side of the "text" is now vulnerable to collisions.

I'd say that such a result is ok.  I think that a proper way to move a markup is
to translate the markup, or maybe internally translating its content.  By
"internally translating content of a markup/stencil" i mean transforming this

[.FooBar]

into

[Foobar.]

(i.e. the moving the graphic and the extent to the left, so that the reference
point will be right.  I may be missing something, though.)

Janek

[janek, 2013-04-29 07:35:05]

On 2013/04/28 23:16:28, dak wrote:
> On 2013/04/28 21:59:37, janek wrote:
> > changes i see generally LGTM (but i may be missing something, in particular
> > Keith seems to make a good point).
> > 
> > > Let Stencil::is_empty use Box::is_empty [...]
> > 
> > I think the desription of this commit could use some more detail, for
example
> > that we need (1 . -1) stencils for backspacing.
> 
> I disagree with that reasoning since it is backwards.  It's just simpler if
> Stencil and Box use the same criteria (actually, I wanted to throw out the
check
> for the stencil expression being '() in Stencil::is_empty () as well, but that
> tripped up something in the page breaker and it looked like it would take a
lot
> of time finding the cause).

ok, and i agree that adding a comment like this into the source may not be a
good idea.  However, git commit message has a different purpose (to myself at
least); i want to know why something changed.  When i'm searching lilypond
history to learn something about the code i'm trying to modify, my usual problem
is that i find a change and the commit message just says what changed - it
doesn't answer my questions: "what was wrong with previous version?  Was this
just a cosmetic change that accidentally introduced a bug, or was some other
design idea behind it?"
I'm pretty surprised by your opinion here since you always advocate writing hows
and whys into commit messages.

> > https://codereview.appspot.com/8869044/diff/9001/lily/box.cc
> > File lily/box.cc (right):
> > 
> > https://codereview.appspot.com/8869044/diff/9001/lily/box.cc#newcode66
> > lily/box.cc:66: return interval_a_[a][LEFT] == empty[LEFT]
> > Do we need to define 'empty'?  I.e. why not just write
> > interval_a_[a][LEFT] == infinity_f ?
> > Are we expecting that the definition of empty extent will change further?
> 
> I am more worried that infinity_f might be defined with a different
> type/precision than that stored in Interval, making the comparison fail.  It's
> not like the C++ compiler would generate a lot of code here, and I can
probably
> even leave off the set_empty as it should be the default.

ok.

[Keith, 2013-04-29 07:39:04]

On 2013/04/29 07:26:50, janek wrote:
> On 2013/04/29 03:31:14, Keith wrote:
> > I worry a little about making the extent of markup different from the union
of
> > its components.  Someone may have used  c4^\markup{\hspace #-3 "text"}  and
> > the left side of the "text" is now vulnerable to collisions.
> 
> I'd say that such a result is ok.  I think that a proper way to move a markup
is
> to translate the markup, or maybe internally translating its content.  

The result is probably OK, but the change might cause trouble for people who
have used \hspace at the front of markup to shift the markup left.

In the manual, the example in section 2.9.3 about time-signatures, and
input/regression/morgenlied.ly, use this method.

[dak, 2013-04-29 07:51:38]

Fix spacing thinkos (hopefully)

[dak, 2013-04-29 08:10:27]

On 2013/04/29 07:39:04, Keith wrote:
> On 2013/04/29 07:26:50, janek wrote:
> > On 2013/04/29 03:31:14, Keith wrote:
> > > I worry a little about making the extent of markup different from the
union
> of
> > > its components.  Someone may have used  c4^\markup{\hspace #-3 "text"} 
and
> > > the left side of the "text" is now vulnerable to collisions.

If you disable skylines, I should think.  I am not fabulously enthused about the
schizophrenous nature of extents and skylines, but it should make this problem
harder to trigger for "legacy" documents, at least for now.

> > I'd say that such a result is ok.  I think that a proper way to move a
markup
> is
> > to translate the markup, or maybe internally translating its content.  
> 
> The result is probably OK, but the change might cause trouble for people who
> have used \hspace at the front of markup to shift the markup left.

I really think it makes more sense (and is more consistent) to use it for
"kerning" in either direction.
 
> In the manual, the example in section 2.9.3 about time-signatures, and
> input/regression/morgenlied.ly, use this method.

Even while we probably won't get hit by it fully due to the current state of
skylines, it would make sense to change those sections.

What I like about this change set is that it makes things work out rather
consistently.  Accumulating a text line does not need special rules differing
from stacking material.

So now there is reasonably predictable behavior: it's easier to explain what
happens now than what happened before since a lot of shoestring and magic is
gone.

But you are right that we should comb through our code base and match the
presented approaches to the cleaned-up interface.

[dak, 2013-04-29 08:14:16]

On 2013/04/29 07:35:05, janek wrote:
> On 2013/04/28 23:16:28, dak wrote:

> ok, and i agree that adding a comment like this into the source may not be a
> good idea.  However, git commit message has a different purpose (to myself at
> least); i want to know why something changed.  When i'm searching lilypond
> history to learn something about the code i'm trying to modify, my usual
problem
> is that i find a change and the commit message just says what changed - it
> doesn't answer my questions: "what was wrong with previous version?  Was this
> just a cosmetic change that accidentally introduced a bug, or was some other
> design idea behind it?"
> I'm pretty surprised by your opinion here since you always advocate writing
hows
> and whys into commit messages.

Which is why the how and why is written in the commit message for the changed
behavior of Box where it belongs.

[...]
> > type/precision than that stored in Interval, making the comparison fail. 
It's
> > not like the C++ compiler would generate a lot of code here, and I can
> probably
> > even leave off the set_empty as it should be the default.
> 
> ok.

Phrased differently, I was lazy and wanted to avoid looking every implementation
detail up in order to get this do the right thing because I worked diligently
rather than it getting to do the right thing as it obviously can't do anything
else.

But then if I am too lazy to look up every detail when writing, do I really want
the reader to have to look up every detail when reading and/or debugging the
code?

Better leave the stuff written in a manner where it's immediately obvious that
it does the right thing.

[Keith, 2013-04-30 03:10:52]

The commit 'simplify stack-stencils' seems strange, and wrong in one detail.
I suppose that stacking the stencils from start of list to end is a bit simpler
conceptually.  From my point-of-view as a non-programmer, and certainly
non-schemer, the new code is not simpler to understand.

https://codereview.appspot.com/8869044/diff/22012/scm/stencil.scm
File scm/stencil.scm (right):

https://codereview.appspot.com/8869044/diff/22012/scm/stencil.scm#newcode46
scm/stencil.scm:46: (lambda (tail head)
Usually, the head is fixed-length and the tail grows, so maybe
'accumulated_stencil' and 'next_stencil'

https://codereview.appspot.com/8869044/diff/22012/scm/stencil.scm#newcode56
scm/stencil.scm:56: (let loop ((head (car stils)) (rest (cdr stils)))
Somewhere you need to step through the list of 'padding', in order to do what
the old code did.

[Keith, 2013-04-30 03:28:11]

https://codereview.appspot.com/8869044/diff/22012/scm/stencil.scm
File scm/stencil.scm (right):

https://codereview.appspot.com/8869044/diff/22012/scm/stencil.scm#newcode46
scm/stencil.scm:46: (lambda (tail head)
On 2013/04/30 03:10:52, Keith wrote:
> Usually, the head is fixed-length and the tail grows, so maybe
> 'accumulated_stencil' and 'next_stencil'

I guess that should be the other way around
lambda (next_stencil accumulated_stencil)

[dak, 2013-04-30 05:04:24]

https://codereview.appspot.com/8869044/diff/22012/scm/stencil.scm
File scm/stencil.scm (right):

https://codereview.appspot.com/8869044/diff/22012/scm/stencil.scm#newcode46
scm/stencil.scm:46: (lambda (tail head)
On 2013/04/30 03:10:52, Keith wrote:
> Usually, the head is fixed-length and the tail grows, so maybe
> 'accumulated_stencil' and 'next_stencil'

Don't like those.  Perhaps front and next or so.

https://codereview.appspot.com/8869044/diff/22012/scm/stencil.scm#newcode56
scm/stencil.scm:56: (let loop ((head (car stils)) (rest (cdr stils)))
On 2013/04/30 03:10:52, Keith wrote:
> Somewhere you need to step through the list of 'padding', in order to do what
> the old code did.

Huh.  And I've not been using "reduce" here exactly because it does not allow
for several list variables.  Embarrassing.

[dak, 2013-04-30 06:20:02]

On 2013/04/30 03:10:52, Keith wrote:
> The commit 'simplify stack-stencils' seems strange, and wrong in one detail.
> I suppose that stacking the stencils from start of list to end is a bit
simpler
> conceptually.

"reduce" is appropriate to use here.  The line stacking (with baseline), in
contrast, is pretty ugly, though.  At some point in iterating the code, there
was a necessity to stack from start to end in order to get combining spaces
right.  Not sure where this is right now.

One thing I consider reasonable is to let \translate #'(+x . 0) ... in a list
not be a noop but indeed move right, while \translate #'(-x . 0) is restrained. 
How much?  Either allowing 0 of sticking out into the left construct, or
allowing "padding" of sticking out, preventing only overlap but nothing else.

Still working on it.

[dak, 2013-05-01 10:31:50]

Quite a bit of cleanup/reorg.  chord flats are still uncorrected but not
relevant for review.

[dak, 2013-05-03 01:37:25]

Simple fix of justify-lines.

[Keith, 2013-05-03 05:18:26]

Shucks.  It gets difficult to space a list of stencils if all you have to work
with are the refpoint and extents of the next stencil, and the composite of
those accumulated so far.

What we would like is :
\line {a b c ... } places the left extent of each markup in the list against the
right extent of the previous markup, plus padding specified by the property
'word-space.  The result is a markup with extent that encloses the extents of
all the individual markups, and overall reference point at the reference point
of the first markup 'a'.

We just need to keep track of one variable: the right extent of the
most-recently placed markup.  Too bad Scheme doesn't provide a way to keep a
running variable as you walk the list.  I guess that is why procedural languages
are so much better than functional languages.

[lemzwerg, 2013-05-03 05:49:55]

https://codereview.appspot.com/8869044/diff/60001/scripts/auxiliar/make-regte...
File scripts/auxiliar/make-regtest-pngs.sh (left):

https://codereview.appspot.com/8869044/diff/60001/scripts/auxiliar/make-regte...
scripts/auxiliar/make-regtest-pngs.sh:9: #
Isn't this patch already commit c8a3b177?

[dak, 2013-05-03 07:10:45]

https://codereview.appspot.com/8869044/diff/60001/scripts/auxiliar/make-regte...
File scripts/auxiliar/make-regtest-pngs.sh (left):

https://codereview.appspot.com/8869044/diff/60001/scripts/auxiliar/make-regte...
scripts/auxiliar/make-regtest-pngs.sh:9: #
On 2013/05/03 05:49:55, lemzwerg wrote:
> Isn't this patch already commit c8a3b177?

Huh, yes.  Looks like my order of git cl upload and rebasing and stuff was not
done prudently.  I suppose this will disappear with the next git cl upload. 
Sorry.

On the bright side, it is only this file in the patch set that needs to get
ignored as that independent issue touched only this.

[dak, 2013-05-03 07:37:52]

On 2013/05/03 05:18:26, Keith wrote:
> Shucks.  It gets difficult to space a list of stencils if all you have to work
> with are the refpoint and extents of the next stencil, and the composite of
> those accumulated so far.
> 
> What we would like is :
> \line {a b c ... } places the left extent of each markup in the list against
the
> right extent of the previous markup, plus padding specified by the property
> 'word-space.  The result is a markup with extent that encloses the extents of
> all the individual markups, and overall reference point at the reference point
> of the first markup 'a'.
> 
> We just need to keep track of one variable: the right extent of the
> most-recently placed markup.  Too bad Scheme doesn't provide a way to keep a
> running variable as you walk the list.  I guess that is why procedural
languages
> are so much better than functional languages.

Uh what?  Named lets are pretty fine for doing that sort of thing.  I don't keep
additional state variables in order to make the behavior more predictable and
become less dependent on assembly order.

Come to think of it: stack-stencil-lines should be doable without additional
state variables by going backwards in the list.

[dak, 2013-05-03 15:04:51]

Work out kinks in footnote ordering, add new "overdraw" argument to
stencil-combine

[dak, 2013-05-04 08:46:38]

Fix \line \vspace #3 and a few other things

[dak, 2013-05-04 09:55:52]

Compatibility conversion for \vspace

[Keith, 2013-05-05 01:50:23]

On Fri, 03 May 2013 00:37:52 -0700, <dak@gnu.org> wrote:

> On 2013/05/03 05:18:26, Keith wrote:
>
>> What we would like is :
>> \line {a b c ... } places the left extent of each markup in the list
>> against the right extent of the previous markup, plus padding
>> specified by the property 'word-space.  The result is a markup with
>> extent that encloses the extents of all the individual markups, and
>> overall reference point at the reference point of the first markup 'a'.
>
>> We just need to keep track of one variable: the right extent of the
>> most-recently placed markup.

> Named lets are pretty fine for doing that sort of thing.

If you know how to keep track of a cursor position in Scheme, the you could put
the implementation of backspace in stack-stencil-line and any other functions
that assemble a line of stencils.

That would probably be easier for future programmers to understand than the
encoding of backspaces in half-empty stencils, implemented by the function (now
misleadingly) called add_at_edge().

[dak, 2013-05-05 08:20:16]

On 2013/05/05 01:50:23, Keith wrote:

> If you know how to keep track of a cursor position in Scheme, the you could
put
> the implementation of backspace in stack-stencil-line and any other functions
> that assemble a line of stencils.
> 
> That would probably be easier for future programmers to understand than the
> encoding of backspaces in half-empty stencils, implemented by the function
(now
> misleadingly) called add_at_edge().

add_at_edge is not actually named misleadingly in the current iteration since
the only information of the base grob that is being used for placement is indeed
only the location of the given edge, independent from its absolute position and
other edges.

The _added_ stencil is checked for backspacing metrics and similar.

Now if we are talking about empty extents, they don't have edges to talk about,
so we are actually more or less free to choose behavior that makes sense.

Maintaining a "cursor position" in stack-stencil-line and friends means that you
can't assemble partial lines yourself in a manner blending in.  That's bad.

There is a simple way out: maintain "cursor positions", namely x and y
escapements (or better, escapements for every of the four edges) explicitly in
each stencil.  Fonts actually carry this kind of information for each glyph, but
LilyPond discards it.

That is the way that LilyPond should really go in the long run.  In the mean
time, encoding escapement-only information into grobs with partly empty extent
at least gives us a natural-behaving variant of hspace and vspace.

I was actually going to rewrite much of the line wrapping functions as well in
order to stop them from keeping separate line width calculations.  Adding one
stencil to a line and measuring it is an O(1) operation, so there is no point in
keeping a separate tally: one has to assemble the line anyway.

Justification can simple be done by assembling the line twice with different
values of spacing and extrapolating the correct spacing from there.

Not having to keep tallies will make the code robust even if we eventually do
get escapements into stencils.  Also the code does not need to know itself any
more about the specifics of hspace and vspace.

Indeed, the last few iterations removed quite a lot of explicit (and partly
inconsistent) treatment of empty stencils: there is no need to weed them out
when add_to_stencil does the right thing with them.

[Keith, 2013-05-05 16:47:49]

On Sun, 05 May 2013 01:20:17 -0700, <dak@gnu.org> wrote:

> Maintaining a "cursor position" in stack-stencil-line and friends means
> that you can't assemble partial lines yourself in a manner blending in.
> That's bad.

Oh. I thought it was good.  If the possibly-backspaced cursor is forgotten when
\line is finished, then the user doesn't have to remember where the cursor ended
up.
\markup { A
  \line {reverse turn \hspace#-5 \raise #2 \musicglyph #"scripts.reverseturn" }
  is found in the original manuscript.}

I suppose this comes out the same in your more-recent iterations.  Only
backspace at the end of a \line has a difference.  Even in that case I would
rather forget about pending backspaces.  The original fix to the issue suffered
because a parenthesized item (with repaired refpoint) blended in when we
assembled partial lines
   parenFlat = \markup {\hspace#-2 ( \flat )}
   \markup {An editorial \parenFlat flat}
It is a little surprising if \parenFlat carries a pending backspace. If the
markup building \parenFlat implements the backspace, and then tells no-one about
it, it makes a simpler interface.

It looks very good that you made 'empty' mean (+inf . inf) so that negative
extents don't count as empty.  But then defining 'half-empty' complicates things
again, and I have not found a use for it.

[dak, 2013-05-06 10:21:33]

"Keith OHara" <k-ohara5a5a@oco.net> writes:

> On Sun, 05 May 2013 01:20:17 -0700, <dak@gnu.org> wrote:
>
>> Maintaining a "cursor position" in stack-stencil-line and friends means
>> that you can't assemble partial lines yourself in a manner blending in.
>> That's bad.
>
> Oh. I thought it was good.  If the possibly-backspaced cursor is
> forgotten when \line is finished, then the user doesn't have to
> remember where the cursor ended up.

Why would he backspace when he doesn't mean it?

> \markup { A
>  \line {reverse turn \hspace#-5 \raise #2 \musicglyph #"scripts.reverseturn"
}
>  is found in the original manuscript.}
>
> I suppose this comes out the same in your more-recent iterations.
> Only backspace at the end of a \line has a difference.  Even in that
> case I would rather forget about pending backspaces.

Which would mean that it would become impossible to assemble lines from
partial lines.

> The original fix to the issue suffered because a parenthesized item
> (with repaired refpoint) blended in when we assembled partial lines
> parenFlat = \markup {\hspace#-2 ( \flat )} \markup {An editorial
> \parenFlat flat} It is a little surprising if \parenFlat carries a
> pending backspace. If the markup building \parenFlat implements the
> backspace, and then tells no-one about it, it makes a simpler
> interface.

You should take a look at the regtest differences of this issue.  It
fixes several alignment problems with parenthesized items which are now
positioned according to their content reference point rather than the
start of the opening paren.

In a similar vein, it fixes the positioning of horizontal brackets,
making the bracket contents line up with the rest of the line rather
than using the lower bracket as a baseline.

> It looks very good that you made 'empty' mean (+inf . inf) so that
> negative extents don't count as empty.  But then defining 'half-empty'
> complicates things again, and I have not found a use for it.

Uh, reality check?  It fixes the hspace/concat bug you encountered
elsewhere in a clean manner, it lets page numbers in table of contents
actually appear flush to the right (like the page number in the page
headings does), it makes \hspace add a predictable amount of space, it
makes indentations at the start of auto-filled paragraphs remain rigid
rather than being expanded along with other spaces...

It might make sense to introduce another primitive instead of
combine-at-edge, like ly:stack-stencils or similar for the operation of
stacking stencils.  But at any rate, I don't see that there is a
reasonable substitute to creating a coherent and dependable operation
for this.  It is easy to see that all the variety of different
hard-coded stencil stacking operations have lead to blooming
inconsistency in the behavior of basic LilyPond markup commands.  In
response to bug reports, various commands get various, partly
conflicting fixes, and the game repeats.

I want to get this over with.

-- 
David Kastrup

[Keith, 2013-05-07 06:03:07]

On Mon, 06 May 2013 03:21:18 -0700, David Kastrup <dak@gnu.org> wrote:

"Keith OHara" <k-ohara5a5a@oco.net> writes:

Oh. I thought it was good.  If the possibly-backspaced cursor is
forgotten when \line is finished, then the user doesn't have to
remember where the cursor ended up.

Why would he backspace when he doesn't mean it?

I was thinking of something like this, where you want to backspace to place a
graphic, but then use the whole extent of the overlaid text for spacing the line
:

\markup { A
  \line {reverse turn \hspace#-5 \raise #2 \musicglyph #"scripts.reverseturn" }
  is found in the original manuscript.}

Another case would be  symbol = \markup { \hspace#-1 \char ##x2230 }
where the \hspace gives somebody's desired alignment over music, but \symbol is
also used as a word in text.

It looks very good that you made 'empty' mean (+inf . inf) so that
negative extents don't count as empty.  But then defining 'half-empty'
complicates things again, and I have not found a use for it.

Uh, reality check?  It fixes the hspace/concat bug you encountered
elsewhere in a clean manner, it lets page numbers in table of contents
actually appear flush to the right (like the page number in the page
headings does), it makes \hspace add a predictable amount of space, it
makes indentations at the start of auto-filled paragraphs remain rigid
rather than being expanded along with other spaces...

Any way of flagging the stencil made by \hspace as purely spacing, so that it
doesn't get 'word-space around it, would have those nice effects.  Half-empty
extents are an awkward way to flag the stencil, especially when its empty
stencil-expression is a more-easily understandable flag.

Your comments at the start of Stencil::add_at_edge() seem to indicate that you
find the interface awkward, and I would agree, so I was pushing for other
solutions.  I'll stop.

[dak, 2013-05-07 09:39:00]

Please try indicating the quoting levels.  This is a bit confusing
otherwise.  Fixing in the reply

On 2013/05/07 06:03:07, Keith wrote:
> On Mon, 06 May 2013 03:21:18 -0700, David Kastrup <mailto:dak@gnu.org> wrote:
> 
>> "Keith OHara" <mailto:k-ohara5a5a@oco.net> writes:
> 
>>> Oh. I thought it was good.  If the possibly-backspaced cursor is
>>> forgotten when \line is finished, then the user doesn't have to
>>> remember where the cursor ended up.
> 
>> Why would he backspace when he doesn't mean it?
> 
> I was thinking of something like this, where you want to backspace to place a
> graphic, but then use the whole extent of the overlaid text for spacing the
line
> :
> 
> \markup { A
>   \line {reverse turn \hspace#-5 \raise #2 \musicglyph #"scripts.reverseturn"
}
>   is found in the original manuscript.}

Uh, this seems rather haphazard.  Do you really think a user would
expect a backspace to allow him to position only the following markup,
jumping back to the right afterwards?  So that with

\markup { x \hspace #-20 hello dolly }

"hello" is to the left of "x", but "dolly" to the right?  That would
seem like a rather strange expectation.  It is not what the current
code does either, by the way.  Both examples look just the same after
the patch.  It is true that the behavior of ly:stencil-combine-at-edge
has changed from the behavior you describe to the behavior used in
\line.  

> Another case would be

>  symbol = \markup { \hspace#-1 \char ##x2230 }

Again, both with the current version and the current proposal, the
output for

    symbol = \markup { \hspace#-1 \char ##x2230 }

    \markup \line { a b c \symbol d e f }
    \markup \column { a b \symbol }

is identical (namely overlap).  With

    symbol = \markup { \null \hspace#-1 \char ##x2230 }

    \markup \line { a b c \symbol d e f }
    \markup \column { a b \symbol }

again both before-and-after version give the same rendition, the one
that you describe.

So your objections don't hold water where the behavior of \line is
concerned.  The only situation where they start making sense is when
ly:stencil-combine-at-edge is used in isolation.  In that case, the
equivalent of the \symbol code without \null in it is like \line
_with_ \null in the current code, and \line _without_ \null in the
proposal.

That use case is the one actually worrying me.  Currently around 60
regtests appear different.  This number can only be marginally reduced
using the (finally working but not yet submitted) \vline fix.  Some of
them are due to "we should likely keep those" changes: a horizontal
line stacked vertically is currently having its height of half a line
width ignored in stacking while its depth is being registered.  That
does not even make sense.

Introducing a primitive different from ly:stencil-combine-at-edge
would have the advantage of keeping the regtest differences down.  But
one would have to scour the code base for instances where
ly:stencil-combine-at-edge is being used for a situation that would be
more appropriately handled by ly:stack-stencils (or whatever),
changing those individually.  Which is likely 3/4 of the uses of
Stencil::add_at_edge in our code base.

Unfortunately, not 100%.  There are a few differences not accounted
for yet.  The bad dot spacing was just lunatic code, explicitly adding
offsets to a stencil that it relied on being ignored.  It is
conceivable that some other instances are similar, but that does not
cover 100%.

>>> It looks very good that you made 'empty' mean (+inf . inf) so that
>>> negative extents don't count as empty.  But then defining 'half-empty'
>>> complicates things again, and I have not found a use for it.
> 
>> Uh, reality check?  It fixes the hspace/concat bug you encountered
>> elsewhere in a clean manner, it lets page numbers in table of contents
>> actually appear flush to the right (like the page number in the page
>> headings does), it makes \hspace add a predictable amount of space, it
>> makes indentations at the start of auto-filled paragraphs remain rigid
>> rather than being expanded along with other spaces...

> Any way of flagging the stencil made by \hspace as purely spacing,
> so that it doesn't get 'word-space around it, would have those nice
> effects.

So what information is stored in a stencil?  The stencil expression,
and the two stencil extents, each being a pair of numbers.  How do we
flag "spacing-purely"?  An obvious way would be to set the stencil
expression to '() for that while still storing spacing information in
the extents.

It turns out ly:stencil-empty? returns #t without looking at the
extents when the stencil expression is '().  Changing the respective
test to look at the extents only causes changed regtests: there is
code that relies on the stencil expression '() being sufficient for
flagging ly:stencil-empty? as true.  And there is quite a bit of code
that just throws away expressions marked ly:stencil-empty?.

So saying "spacing is an expression of '()" does not work compatibly.
"spacing is an expression of "" " seems rather worse since it would
apply, for example, to \markup "" which is _not_ pure spacing.

\hspace #x naturally has an empty Y extent, \vspace #y naturally has
an empty X extent.  So the condition that is being checked is quite
natural.

Whether the check should be (ly:stencil-empty? st Y) rather than
(ly:spacing? st X) and what (ly:spacing? null-stencil X) should then
return is another question.

> Half-empty extents are an awkward way to flag the stencil,

Not really.  They are a natural property of spacing.  It may just be
an awkward way to _check_ for them.

> especially when its empty stencil-expression is a more-easily
> understandable flag.

I tried with that first, but it seems like quite a larger can of
worms.  For one thing, if somebody explicitly writes

(ly:make-stencil "" '(0 . 4) '(0 . 0))

which is basically the old \hspace sans spacing correction, its
behavior will stay the same with this code.  Similar for

(ly:make-stencil "" '(0 . 0) '(0 . 4)).

I consider that an advantage.  Anything starting with

(ly:make-stencil '() ...

is currently being ignored in more contexts than one would want to.
For that reason, people are unlikely to have been using this with any
expectation other than it getting not heeded universally.

> Your comments at the start of Stencil::add_at_edge() seem to
> indicate that you find the interface awkward, and I would agree, so
> I was pushing for other solutions.  I'll stop.

As I said: a sensible solution to me appears to create a primitive
separate from Stencil::add_at_edge aka ly:stencil-combine-at-edge for
the purpose of stacking stencils, and use that consistently.

That should provide reasonable compatibility with past uses of both
ly:stencil-combine-at-edge as well as \line, while still providing
most of the advantages of a unified code base.

But most of the "other solutions" you are pushing for seem worse than
what they are supposedly fixing, and your examples very much suggest
that they would not actually be fixing anything that you think they
should.

[dak, 2013-05-12 19:27:39]

Create Stencil::stack and tune back chord flats

[Keith, 2013-05-12 21:50:13]

This works.

I found a only couple cases on mutopiaproject that depended on the old behavior.
 \markup { "V" \translate #'(-2.5 . 0.325) \rotate #70 "—" }
This would work with \hspace#'-2.5, or even better with \combine, which is what
the manual suggests to use.

The way that \hspace#-1 clips the extents will cause some problem, but I cannot
foresee what.  The only problem I can imagine is that someone will complain that
\textLengthOn no longer works as it did
 {\textLengthOn
 \repeat unfold 4 b^\markup{\hspace#-3 Shifted}  }
but here \translate would be a good solution.

[dak, 2013-05-12 22:10:21]

On 2013/05/12 21:50:13, Keith wrote:
> This works.
> 
> I found a only couple cases on mutopiaproject that depended on the old
behavior.
>  \markup { "V" \translate #'(-2.5 . 0.325) \rotate #70 "—" }
> This would work with \hspace#'-2.5, or even better with \combine, which is
what
> the manual suggests to use.
> 
> The way that \hspace#-1 clips the extents will cause some problem, but I
cannot
> foresee what.

-dpreview or other extent-based code might lose material when the
intent was just shifting it around.

I've considered providing both \hspace and \kern where both distribute
the shift differently between left and right extent of the spacing
stencil, and where kern would be less prone to shifting the reference
point of the following stencil when not combined with a preceding
stencil.

I've not really managed to find an overly convincing and useful
combination, however.  It might make sense revisiting this idea when
cursor movement and stencil extents are made independent stencil
properties, but at least currently I've not been able to make a good
case for that.

> The only problem I can imagine is that someone will complain that
> \textLengthOn no longer works as it did
>  {\textLengthOn
>  \repeat unfold 4 b^\markup{\hspace#-3 Shifted}  }
> but here \translate would be a good solution.

Likely.  The behavior for an isolated \hspace at the start of an
expression is not really independently useful all that much.  Its most
endearing trait is that inside of a line, it is equivalent to a
\hspace at the end of the preceding expression, and that multiple
\hspace can be combined by adding the displacements.

[Keith, 2013-05-12 23:09:00]

On Sun, 12 May 2013 15:10:21 -0700, <dak@gnu.org> wrote:

> Likely.  The behavior for an isolated \hspace at the start of an
> expression is not really independently useful all that much.

\hspace is used in that way, though.  Including in the 'Morgenleid' regression
test.

Possibly the documentation might have some other surprising use of tricky
markup, but I haven't looked at that.

I did try your suggestion to use named let
<http://codereview.appspot.com/9319046/> and played with assembling a line based
on stacking extents, tracking the cursor position within the \line {}, so the
result would have the full extent of the union of the extents of the parts.

[dak, 2013-05-14 07:52:14]

Fix wordwrap-stencils to not get confused by spacing

[dak, 2013-05-23 20:16:30]

Add mindist argument to ly:stencil-stack, fix page accounting bug