Re: Caches the interior skylines of vertical axis groups and systems. (issue 7185044)

[k-ohara5a5a]

The reorganization looks reasonable.

   { g4\> g'_"pico"  g' g\! }

Assuming for simplicity that this is set in one line, one
VerticalAxisGroup contains all the graphical objects as 'elements'.  At
some point the Y-offset of TextScript "pico" is evaluated, calling
aligned_side() and then calc_outside_staff_offset() methods of "pico".


"Pico" asks the VerticalAxisGroup to please sort her elements according
to the order in which their vertical position is determined, consulting
the outside-staff-priority of each and breaking ties, and to store the
sorted list as 'vertical-skyline-elements'.

Next, "Pico" asks those of his fellow elements that get placed before
him, such as the Hairpin, to please deterine their 'vertical-skylines'
and relative_coordinate().  (This would require that Hairpin, in turn,
ask any elements that are placed before Hairpin, and so on.)   Now
"pico" can find his home between the skylines of the staff and Hairpin,
and return his position to be stored as a value, replacing the callback,
in 'pico's Y-offset.

The decision-making is still top-down in the sorting, bottom-up in
choosing padding, as it was before.  The change is that evaluation of
Y-offset of any grob initiates the decision-making, and decisions are
stored in properties of the appropriate grobs (grouping- or graphical-
objects) so that any of these properties could be reset to its callback
function, and evaluated again.

There seems to be considerable repeated work, with each grob building
'all_v_skylines', the array of skylines for all the grobs placed before
him on the same line.  I found score compilation times, however, at most
0.5% longer than before the patch.  It looks like the extra work is
mostly moving pointers around in C-code.


A lot of the code is devoted to TupletBrackets and TupletNumbers
(riders).  These usually go inside the staff, but we allow users to give
an 'outside-staff-priority' to either one or both.  If the TupletNumber
is given earlier priority than the TupletBracket, when the TupletBracket
is present, those requests conflict with the usual behavior of child,
the Number, following his parent, the Bracket.

I got satisfactory results when I removed your original 'riders' code,
and kept only the step where a grob's outside-staff-priority is reset to
be the same as that of his parent (when a parent with
outside-staff-priority is present).  You might want to simplify this
way, if tuplets get too complicated.


https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-bracket-outside-staff-priority.ly
File input/regression/tuplet-bracket-outside-staff-priority.ly (right):

https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-bracket-outside-staff-priority.ly#newcode18
input/regression/tuplet-bracket-outside-staff-priority.ly:18: \override
TupletBracket.Y-offset =
#ly:side-position-interface::calc-outside-staff-y-offset
If changes in the code newly require this callback, you need to set the
callback in define-grobs.scm, like it is for all the other objects that
can take outside-staff-priority.

https://codereview.appspot.com/7185044/diff/32003/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

https://codereview.appspot.com/7185044/diff/32003/lily/axis-group-interface.cc#newcode732
lily/axis-group-interface.cc:732: are triggered beforehand.
This preparation has to go down to line 758, then.  This is more
important now that the use of vector_sort(), for which you need to
prepare, is in a callback.

https://codereview.appspot.com/7185044/diff/32003/lily/axis-group-interface.cc#newcode746
lily/axis-group-interface.cc:746: vector<Grob *> elements
(fakeelements);
Rather than 'fakeelements' it seems be 'originalelements'.  Do you
really need to make this extra copy, or does the extract_grob_set macro
itself create a copy?

https://codereview.appspot.com/7185044/diff/32003/lily/axis-group-interface.cc#newcode758
lily/axis-group-interface.cc:758: vector_sort (elements,
staff_priority_less);
This is the point where you need to ensure that remove-empty is
completed.

https://codereview.appspot.com/7185044/diff/32003/lily/axis-group-interface.cc#newcode940
lily/axis-group-interface.cc:940: // UGHHHH - kludge that will go away
when Y property stuff is fixed...
Remove.

https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interface.cc
File lily/side-position-interface.cc (right):

https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interface.cc#newcode496
lily/side-position-interface.cc:496: // pure: for now facultative, but
eventually "is this pure"
I'm guessing you are thinking in French.  The English for 'facultative'
is 'optional'.  English uses 'faclutative' only in scientific ways that
imply a benefit that can be optionally had by including the optional
thing.

https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interface.cc#newcode512
lily/side-position-interface.cc:512: && !pure)
I think the remainder of the function is in the if-block, so probably
better to early-return on the inverse condition.

https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interface.cc#newcode556
lily/side-position-interface.cc:556: // skip inside staff elts
Maybe not 'skip' but "ensure inside staff elements are placed"

https://codereview.appspot.com/7185044/