Re: Gets vertical skylines from grob stencils (issue 5626052)

[mtsolo]

Thanks for the performance tests!  I have no problem changing the
function avoid_outside_staff_collisions to be faster - it's just that I
haven't wrapped my head around how distance alone can do it.


http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.cc#newcode945
lily/axis-group-interface.cc:945: feature present in
tuplet-number-outside-staff-priority.
On 2012/08/18 02:42:37, Keith wrote:
> On 2012/08/17 17:16:25, MikeSol wrote:
> > On 2012/08/17 08:12:56, Keith wrote:
> > If you comment out the rider business, the vertical-skylines will
not be
> correct
> > for axis-group-interface.

> That is very subtle, very minor, changes only the debug output, not
what would
> normally be printed from that example, and is different from the
comment
> indicates.

\relative c'' {
   \override TupletBracket #'outside-staff-priority = #1
   \override TupletNumber #'font-size = #5
   \times 2/3 { a4\trill a\trill^"foo" a\trill }
}

I added this as a regtest.  If you comment out the rider business,
you'll see that foo is printed on top of the tuplet number.

http://codereview.appspot.com/5626052/diff/104124/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5626052/diff/104124/lily/axis-group-interface.cc#newcode697
lily/axis-group-interface.cc:697: bumps.push_back (dir *
vertical_skyline_forest[dir][j][d].distance ((*pair)[-d]));
On 2012/08/18 02:42:37, Keith wrote:
> When d = dir, the direction we intend to move, this tells us how far
we need to
> move, but when d = -dir, this tells us by how far we have encroached
into the
> next obstacle, which we will eventually need to hop over.

> What you really want to put on the bumps list is how far we need to
move to hope
> over this next obstacle.  But we only tested the relevant skylines for
> 'intersection' ... if only we had stached them distance().

I'm not sure what you mean when you say "we only tested the relevant
skylines for intersection ... if only we had stached them distance()" -
one because I don't know what stached means in this context (from urban
dictionary: The art of placing a ficticious mustache on an individual or
oneself, drawing laughter and humor to this occurance.) and two because
every intersection call is followed by a distance call, so there is
bumping going on in this case.

http://codereview.appspot.com/5626052/diff/104124/lily/axis-group-interface.cc#newcode701
lily/axis-group-interface.cc:701: bumps.push_back (dir *
vertical_skyline_forest[dir][j][d].distance (to_flip));
On 2012/08/18 02:42:37, Keith wrote:
> This finds the distance to move such that the top skyline of the new
object just
> encloses, Russian egg doll style, one of the already-placed skylines.
We never
> want that to be the final position, so don't put it on the bumps list.

That is a possible scenario, but Russian egg doll style enclosing is
tested for below, so even if this does happen it'd be rectified on the
next pass.

http://codereview.appspot.com/5626052/diff/104124/lily/axis-group-interface.cc#newcode732
lily/axis-group-interface.cc:732: pair->raise (min_bump);
On 2012/08/18 02:42:37, Keith wrote:
>    printf("bumping %.3f\n", min_bump);
> shows that for a simple case
>    \relative f {f'^"mouse" c'' f,,^"EEEEK" d''}
> we bump around quite a lot:
> "mouse"
> bumping 0.567
> bumping 1.894
> "EEEK"
> bumping 0.443
> bumping 0.048
> bumping 0.095
> bumping 0.191
> bumping 0.382
> bumping 0.459
> bumping 1.344
> bumping 0.161
> bumping 0.322
> bumping 0.578

Agreed.  I'd love for this to go faster.
I am for getting this into current master ASAP so that people can work
on speeding it up - I'm not at all adverse to the idea of using distance
the way you're talking about, but I don't completely understand how it'd
work yet and it seems like something that could be done in a separate
commit.  What'd be nice about having this in master is that it'd be
easier to spot in the regtests if efficiency changes to the algorithm
have a layout impact.  Of course, one could run regtests with this patch
as a baseline, but it is a pain.

http://codereview.appspot.com/5626052/