Re: Adds dimension stencil command to correct with-dimension (issue 12957047)

[dak]

On 2013/08/26 06:42:38, mike7 wrote:
> On 26 août 2013, at 08:35, mailto:address@hidden wrote:

> > So why do you talk about that in the first place?

> Because I'd rather spend more time implementing a solid system than
less time
> implementing a kludge.

In the context of this issue, we are currently talking about not doing
anything at all.  It seems like the best course forward with regard to
resolving the reported regression would be to just adapt Keith's
patch.

> What I'm proposing (making skylines a stencil data
> member) is more work but seems less kludgy than creating a stencil
primitive
> containing skylines.

Again: this is _totally_ orthogonal to the issue at hand.  If you want
to talk about that, create a new issue rather than drive an existing
issue into the ground.

> >> It would be better and more consistent with currently practices in
> >> the code base if stencils carried their own skyline information (as
> >> they do their own dimensions), which would require a lot of
juggling
> >> code around.
> >
> > In the context of reviewing this patch, this is a straw man
argument.

> In the context of figuring out the best way to do things, this is
important.  It
> would be better to decide what problem we're trying to solve and
implement it
> correctly now rather than creating a temporary solution that we do
away with
> later.

Ok, let's get to the gist of it: for arranging items inside of a
markup, there are several concepts: stencil geometry, reference
points, escapement values (after placing a glyph at the current
reference point, move by the respective escapement for the current
direction of alignmen).  For lining up stencils, _only_ escapement
values and reference points are of interest.  The outlines come into
play _between_ independent markups, when we create a graphical
arrangement on the page.

So in general, we don't need outlines until the finished markup.
\with-dimensions operates on the dimensions interesting for working
within markups.  Unfortunately, we raised the expectation that those
are the same that are valid outside and advertised \with-dimensions
accordingly.  This is what this issue is about.

Generating a skyline from individual elements is a complex operation.
When done all at once, we have a complexity of O(n lg n) for n
elements.  When integrating a single element at a time, the complexity
rises to O(n^2).  So we don't want to assemble skylines one by one if
we can avoid it.  Markup functions assemble stencils to stencils.  If
we can calculate the skyline (when needed) in one go from the complete
stencil arrangement, we save a lot of effort, and that's the rule
rather than the exception.

So we don't want to carry skylines along with stencils that will not
be used individually on the page but will rather be integrated into a
markup.

> It seems like stencil expressions should not contain dimensional
> information.  Where it does (like, for example, the stencil command
> 'translate-stencil defined in define-stencil-commands.scm), this
> seems like legacy code that has not evolved with LilyPond and is
> barely used.

The dimensional information is used for stencil-add-at-edge, and most
certainly for all the various stencil stacking operations, forming
lines and columns.  "barely used" is a mischaracterization if I ever
saw one.

> So, as long as we're going to solve this problem, we should have a
> broader discussion about how stencils are put together.

In the course of this issue, we should fix the perceived regression.
It seems like Keith's code does that just fine.

> I argue that we should not be putting dimension information in the
> stencil expression itself.

Markups plug together stencils based on dimension information, and
that is what fonts do as well.  You can't row up letters convincingly
based on outlines since "visual distance" is a concept not captured
satisfactorily by computational treatment of outlines.  That is the
reason that escapement values are hardcoded by the font designer.


https://codereview.appspot.com/12957047/