Caches the interior skylines of vertical axis groups and systems.

Caches the interior skylines of vertical axis groups and systems.

This will allow for a more modular approach to outside-staff-spacing,
eventually eliminating the call to translate_axis in axis-group-interface.cc.

[Keith, 2013-01-26 22:21:55]

This would take me a couple more hours to figure out.  Do I want to ?

It seems the "caching" is not the type that saves us from re-computation.  It
simply stores the skylines Scheme objects.

The tracker says the overall goal is to remove a call to the function
translate_axis.  In the example 
 { g4\> g'_"pico"  g' g\! } 
when we decide to move the "pico" between hairpin and staff, the call to
translate_axis records the new position in the TextScript grob itself.

Sometime before printing, I assume you want the positioning information to be
updated in the grob.  When do you propose to do so ?   How will storing the
outline of the staff without the hairpin in 'inside-staff-skylines' help ?

You can post a commit series to Reitveld if you like.  On the branch with the
final commit, just checkout the first commit, `git-cl upload master`, checkout
the next commit...

https://codereview.appspot.com/7185044/diff/4001/scm/define-grob-properties.scm
File scm/define-grob-properties.scm (right):

https://codereview.appspot.com/7185044/diff/4001/scm/define-grob-properties.s...
scm/define-grob-properties.scm:1115: grobs that are elements of an axis group
that do not have an
"elements of a @code{VerticalAxisGroup} and that do not"

https://codereview.appspot.com/7185044/diff/4001/scm/define-grob-properties.s...
scm/define-grob-properties.scm:1117: or other vertical axis like lyrics,
dynamics, etc..")
"or other @code{VerticalAxisGroup}s like @code{Lyrics}, @code{Dynamics}"
because dynamics like \p \< \f typically do have outside-staff-priority.

[mike7, 2013-01-27 09:45:28]

On 26 janv. 2013, at 23:21, k-ohara5a5a@oco.net wrote:

> This would take me a couple more hours to figure out.  Do I want to ?
> 
> It seems the "caching" is not the type that saves us from
> re-computation.  It simply stores the skylines Scheme objects.
> 

Yup.

> The tracker says the overall goal is to remove a call to the function
> translate_axis.  In the example
> { g4\> g'_"pico"  g' g\! }
> when we decide to move the "pico" between hairpin and staff, the call to
> translate_axis records the new position in the TextScript grob itself.
> 
> Sometime before printing, I assume you want the positioning information
> to be updated in the grob.  When do you propose to do so ?   How will
> storing the outline of the staff without the hairpin in
> 'inside-staff-skylines' help ?

The broad goal is:

1) Cache interior skylines.
2) Move outside-staff-priority purely to the side-position interface.  Objects
with lower outside-staff-priorities (+ the interior skylines of vertical axis
groups) become support objects for those with higher.  This means that all
positioning information is calculatable via a Y-offset callback, so no more need
for translate_axis.  This, in turn, means that a pure version of this Y-offset
can be cached, cleared, and refined over time.
3) Use these pure properties way farther downstream.  So, for example, if any
object's Y-position depends on the Y-position of a cross-staff grob, only
calculate it's pure height for vertical spacing calculations.  Note that, for a
system without cross-staff grobs, pure-Y-offset and Y-offset would be
equivalent.
4) Do skyline spacing in the page breaker and Page_layout_problem with pure
heights instead of heights.
5) Trigger the calculation of real heights for cross-staff-grobs (and the offset
for grobs that do side-positioning) after Page_layout_problem::solve spaces
stuff.

Bottom line: currently, pure heights are used to make good guesses for
horizontal spacing, and then once the spacing is chosen, real heights are used. 
What I want is for pure heights to be used for horizontal spacing, then cleared
from the cache and recomputed for vertical spacing, then use real heights for
final vertical layout.

Stuff like your cross-staff-grob-full piano piece would benefit a lot from
this.

> 
> You can post a commit series to Reitveld if you like.  On the branch
> with the final commit, just checkout the first commit, `git-cl upload
> master`, checkout the next commit...
> 
> 
>
https://codereview.appspot.com/7185044/diff/4001/scm/define-grob-properties.scm
> File scm/define-grob-properties.scm (right):
> 
>
https://codereview.appspot.com/7185044/diff/4001/scm/define-grob-properties.s...
> scm/define-grob-properties.scm:1115: grobs that are elements of an axis
> group that do not have an
> "elements of a @code{VerticalAxisGroup} and that do not"
> 
>
https://codereview.appspot.com/7185044/diff/4001/scm/define-grob-properties.s...
> scm/define-grob-properties.scm:1117: or other vertical axis like lyrics,
> dynamics, etc..")
> "or other @code{VerticalAxisGroup}s like @code{Lyrics}, @code{Dynamics}"
> because dynamics like \p \< \f typically do have outside-staff-priority.
> 

Maybe it's not worth it to do all this intermediate stuff...you're right that it
takes time for others to understand (and even for me to understand) as I'm
working towards this goal.  The project is even more ambitious as the last
skyline one in terms of the amount of code that will be touched, so it may be
difficult for people to understand the utility of things until I have a final
working vertical spacing version of LilyPond.  The only problem is that there
will be a branch that is significantly different from staging that I won't be
able to refactor into separate commits without a heapload of work.  But it will
avoid people unnecessary review of intermediary steps.

[dak, 2013-01-27 10:51:48]

"mike@mikesolomon.org" <mike@mikesolomon.org> writes:

> Maybe it's not worth it to do all this intermediate stuff...you're
> right that it takes time for others to understand (and even for me to
> understand) as I'm working towards this goal.  The project is even
> more ambitious as the last skyline one in terms of the amount of code
> that will be touched, so it may be difficult for people to understand
> the utility of things until I have a final working vertical spacing
> version of LilyPond.  The only problem is that there will be a branch
> that is significantly different from staging that I won't be able to
> refactor into separate commits without a heapload of work.  But it
> will avoid people unnecessary review of intermediary steps.

Mike, try the following: don't write code.  Only write comments.  Those comments
explain what the code will accomplish (not _how_ it accomplishes that), and how
it fits together with other code.  Debug the comments.  Make sure that the plan
laid out in those comments is coherent.  Go through the various cases and
scenarios.

Once you have all the structure and the comments together, let's review them. 
Onlz the skeleton of comments, nothing else.  If you can't focus without writing
code, remove the code for the sake of reviewing.  We don't want to see it.  It
is an implementation detail that can be filled in by a coding monkey in case you
are busy with something else.

Architecture is not the art of building walls until a building emerges.  The
purpose of a review is not just making sure that there is enough mortar on every
brick.  That's masonry, not architecture.  You are putting out arbitrary code
pieces for review.  You are right that reviewing all the code in summary is a
bit much.

So instead start with all the comments, so that we can review how you plan to
fit anything together instead of looking at the execution of fitting stuff
together.

-- 
David Kastrup

[mike7, 2013-01-27 11:56:26]

On 27 janv. 2013, at 11:51, dak@gnu.org wrote:

> "mike@mikesolomon.org" <mike@mikesolomon.org> writes:
> 
>> Maybe it's not worth it to do all this intermediate stuff...you're
>> right that it takes time for others to understand (and even for me to
>> understand) as I'm working towards this goal.  The project is even
>> more ambitious as the last skyline one in terms of the amount of code
>> that will be touched, so it may be difficult for people to understand
>> the utility of things until I have a final working vertical spacing
>> version of LilyPond.  The only problem is that there will be a branch
>> that is significantly different from staging that I won't be able to
>> refactor into separate commits without a heapload of work.  But it
>> will avoid people unnecessary review of intermediary steps.
> 
> Mike, try the following: don't write code.  Only write comments.  Those
> comments explain what the code will accomplish (not _how_ it
> accomplishes that), and how it fits together with other code.  Debug the
> comments.  Make sure that the plan laid out in those comments is
> coherent.  Go through the various cases and scenarios.
> 

Hey David,

I see the value of this approach - in music, I know a lot of composers that work
this way.  I do not work this way, not because I have any objection to it, but
because that's not how my brain is wired.  I have tried and it gets me nowhere. 
I can only grasp how things are organized by trying, failing, and mostly writing
code.  So, I'm ok with your suggestion to write code and then remove it or
annotate it with comments, but I have to write the code first.  Otherwise I
won't fully understand what's going on.  For a sketch of where I'm going, you
can read over the response I sent Keith.  Above and beyond that level of
precision (which is rather broad), I cannot go into more detail without working
through the problem first.

Cheers,
MS

[Keith, 2013-01-28 05:34:06]

On Sun, 27 Jan 2013 01:45:16 -0800, mike@mikesolomon.org <mike@mikesolomon.org>
wrote:

> On 26 janv. 2013, at 23:21, k-ohara5a5a@oco.net wrote:
>
>> The tracker says the overall goal is to remove a call to the function
>> translate_axis.  In the example
>> { g4\> g'_"pico"  g' g\! }
>> when we decide to move the "pico" between hairpin and staff, the call to
>> translate_axis records the new position in the TextScript grob itself.

> The broad goal is:
[...]
> 2) Move outside-staff-priority purely to the side-position interface.  Objects
with lower outside-staff-priorities (+ the interior skylines of vertical axis
groups) become support objects for those with higher.  This means that all
positioning information is calculatable via a Y-offset callback, so no more need
for translate_axis.

So in the example
   { g4\> g'_"pico"  g' g\! }
the text "pico" has its Y position referenced to the staff, its Y-parent.

Currently, the TextScript holding "pico" has a Y-offset property that is a
function computing the position to clear everything on the staff.  (Y-offset
considers the parent-child relationship only.)  The results of the function
pointed to by Y-offset are stored in the C++ data-structure representing this
TextScript.

Currently, the TextScript and Hairpin, both having the staff as Y-parent, are
then arranged relative to each other.  Any shifts required by this arranging are
applied, by translate_axis(), to the stored positions in the Hairpin and
TextScript.

You propose (I think) to avoid collisions between Hairpin and TextScript
directly in the functions used to compute Y-offset.  Hairpins are placed first,
then TextScripts, so in this case the TextScript gets a pointer to the Hairpin
added to its list of side-support-elements.  We would need to walk through
outside-staff priorities and put these pointers in place *before* computing
Y-offsets, opposite to the current order of opertions, so that the TextScript's
Y-offset computation has the information to see and avoid the Hairpin.  (Now I
have a guess why you might want the skylines outlining the staff and outlining
Hairpin to be "cached" in Scheme objects.)

This would change Y-offset to be the position relative to parent, from all
causes, rather than just the offset *due* to the parent.  Some care would be
required to avoid circular dependencies, but it should be possible because the
data dependencies are already there in the current method.

The change to remove the call to translate_axis() in
axis-group-interface.cc::avoid_outside_staff_collisions(), plus whatever
preparation is necessary for that change, would be conceptually easier to review
all at once.


I have a glimmer understading of your plan for cross-staff items.  If the
Hairpin in the example was instead a cross-staff Slur, I see that you hope to
(re)-position the "pico" by computing the Y-offset, which will use whatever
position information is available at the time.  It seems you would need two
passes of positioning "pico": one to compute staff skylines (conservatively) so
LilyPond can space the staves, and then one to position the "pico" around the
slur.  It do not yet have more than glimmer of hope that it will actually be
possible.

[MikeSol, 2013-01-28 12:27:47]

Eliminates translate_axis call from outside-staff-positioning.

[MikeSol, 2013-01-28 12:33:38]

People can review this if they have time...it is a lot of code, but it is more a
progress report than anything else.

To resume a bit of what's been said before: once translate_axis is called, the
dim_cache of grobs is set and it is very difficult to work with approximations. 
This makes cross-staff grobs very difficult to deal with in vertical placement. 
The goal is to eliminate this property call for outside-staff placement so that
pure skylines can be used in a first pass without setting the Y-offset.  Then,
in the second pass, real skylines will be used and the Y-offset will be set.

[mike7, 2013-01-28 12:43:54]

On 28 janv. 2013, at 06:33, Keith OHara <k-ohara5a5a@oco.net> wrote:

> On Sun, 27 Jan 2013 01:45:16 -0800, mike@mikesolomon.org
<mike@mikesolomon.org> wrote:
> 
>> On 26 janv. 2013, at 23:21, k-ohara5a5a@oco.net wrote:
>> 
>>> The tracker says the overall goal is to remove a call to the function
>>> translate_axis.  In the example
>>> { g4\> g'_"pico"  g' g\! }
>>> when we decide to move the "pico" between hairpin and staff, the call to
>>> translate_axis records the new position in the TextScript grob itself.
> 
>> The broad goal is:
> [...]
>> 2) Move outside-staff-priority purely to the side-position interface. 
Objects with lower outside-staff-priorities (+ the interior skylines of vertical
axis groups) become support objects for those with higher.  This means that all
positioning information is calculatable via a Y-offset callback, so no more need
for translate_axis.
> 
> So in the example
> { g4\> g'_"pico"  g' g\! }
> the text "pico" has its Y position referenced to the staff, its Y-parent.
> 
> Currently, the TextScript holding "pico" has a Y-offset property that is a
function computing the position to clear everything on the staff.  (Y-offset
considers the parent-child relationship only.)  The results of the function
pointed to by Y-offset are stored in the C++ data-structure representing this
TextScript.
> 
> Currently, the TextScript and Hairpin, both having the staff as Y-parent, are
then arranged relative to each other.  Any shifts required by this arranging are
applied, by translate_axis(), to the stored positions in the Hairpin and
TextScript.
> 

Yup.

> You propose (I think) to avoid collisions between Hairpin and TextScript
directly in the functions used to compute Y-offset.  Hairpins are placed first,
then TextScripts, so in this case the TextScript gets a pointer to the Hairpin
added to its list of side-support-elements.  We would need to walk through
outside-staff priorities and put these pointers in place *before* computing
Y-offsets, opposite to the current order of opertions, so that the TextScript's
Y-offset computation has the information to see and avoid the Hairpin.  (Now I
have a guess why you might want the skylines outlining the staff and outlining
Hairpin to be "cached" in Scheme objects.)

Yup, except that in the new patch I proposed, instead of adding it to this list,
I use the vertical-skyline-elements array of the vertical axis group to which
the grob belongs.  That is because outside-staff-placement is conceptually
different from normal side placement, as outside-staff objects can slide under
other outside-staff objects whereas we don't want this for normal side
placement.

> 
> This would change Y-offset to be the position relative to parent, from all
causes, rather than just the offset *due* to the parent.  Some care would be
required to avoid circular dependencies, but it should be possible because the
data dependencies are already there in the current method.

Everything should be fine in the patchset I just uploaded.

> 
> The change to remove the call to translate_axis() in
axis-group-interface.cc::avoid_outside_staff_collisions(), plus whatever
preparation is necessary for that change, would be conceptually easier to review
all at once.

Done.

> 
> I have a glimmer understading of your plan for cross-staff items.  If the
Hairpin in the example was instead a cross-staff Slur, I see that you hope to
(re)-position the "pico" by computing the Y-offset, which will use whatever
position information is available at the time.  It seems you would need two
passes of positioning "pico": one to compute staff skylines (conservatively) so
LilyPond can space the staves, and then one to position the "pico" around the
slur.  It do not yet have more than glimmer of hope that it will actually be
possible.
> 

I have a conceptual roadmap in my head and, so far, the elimination of the
translate_axis call proposed in the most recent patchset has gone OK (it passes
regtests).  Now, we just need to implement better pure skylines, use them for
vertical axis group spacing with the most robust information possible (i.e. real
beam placement can be used after X positioning is done - no need to be pure),
and then we should be set...

Cheers,
MS

[MikeSol, 2013-01-28 19:06:06]

Compiles with current master

[MikeSol, 2013-01-31 23:32:41]

Hey all,

After letting this newest round sit for a few days, I'm pretty confident that
the patch is ready for review so I'd ask people to review it.  First, if anyone
needs more comments in the code to let them know what's going on, lemme know
where and I'll add it.  Second, if people have performance concerns, lemme know
- I haven't noticed a slow down but I haven't done rigorous testing.  Third, I'm
interested to see if people like this approach.  The long-term goal is to get
rid of translate_axis as much as possible, as using it destroys downstream
possibilities to do pure height calculations on grobs.

Cheers,
MS

[dak, 2013-02-01 15:04:37]

As previously, there are no comments whatsoever putting the code into
perspective.  This is an amorphous heap of code without an attempt of explaining
its design or inner logic.  There is a single function comment giving a glimpse
of what that function is supposed to do.  However, there is no explanation of
the context this function is supposed to be used in or for, the function naming
bears no relation to its return value, the comment is unclear, half of the named
entities don't exist in the function interface, and half of the existing
entities are not mentioned.

https://codereview.appspot.com/7185044/diff/17001/lily/directional-element-in...
File lily/directional-element-interface.cc (right):

https://codereview.appspot.com/7185044/diff/17001/lily/directional-element-in...
lily/directional-element-interface.cc:25: recursive_get_grob_direction (Grob
*me)
Why is not direction-source used instead?  This code seems like being intended
to replace proper information by heuristics that mask a problem most of the
time.

https://codereview.appspot.com/7185044/diff/17001/lily/script-interface.cc
File lily/script-interface.cc (right):

https://codereview.appspot.com/7185044/diff/17001/lily/script-interface.cc#ne...
lily/script-interface.cc:81: // ie DynamicText takes direction from
DynamicLineSpanner
Why then would DynamicLineSpanner not be the direction-source of DynamicText?  I
applaud that you are bucking the trend of making the entire patch without
comments, but it looks like the code is intended to mask a bug by sometimes
doing the right thing by accident.

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

https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interfac...
lily/side-position-interface.cc:185: // or with anything in other_v_skylines.
Congrats.  This is actually the first comment that actually gives _any_
information for the actual intent of a function.  Unfortunately, there is no
indication _what_ is being returned.  "avoid_outside_staff_collisions" does not
sound like it would return any value.  And what information do we get?  "Returns
the value for the grob elt" (there is no grob elt in the arguments) "whose
skylines are given by h_skyline..." (there is no h_skyline in the arguments) "so
that it doesn't intersect with staff_skyline" (there is no staff_skyline in the
arguments).

And what is the grob going to do with that value to stop intersecting?  Scale
itself by that value?  Buy itself a hoverboard worth the value in order to ride
the skylines?

Please give the function a name making any sense in connection with its return
value, and if you bother writing a comment, please make sure that it refers to
actually involved variables and arguments.  And what's with padding,
horizon_padding, other_padding, and other_horizon_padding?  What's with dir?

https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interfac...
lily/side-position-interface.cc:296: //printf ("        Q %s\n", e->name
().c_str ());
What's this supposed to be?

https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interfac...
lily/side-position-interface.cc:461: MAKE_SCHEME_CALLBACK
(Side_position_interface, outside_staff_aligned_side, 1);
Sure, something called "outside_staff_aligned_side" is so obvious in meaning
that we don't need to explain what arguments it gets and what the meaning of the
returned value would be.  Just for the record: I am being sarcastical here.

Mike, how is _anybody_ going to be able to understand _anything_ using an
undocumented callback with that name?

https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interfac...
lily/side-position-interface.cc:481:
Side_position_interface::calc_outside_staff_offset (Grob *me, Axis a, bool pure,
int start, int end, Real current_offset)
Now here is a function name that gives an _inkling_ of a clue what the returned
value is: an offset.  It still is totally undocumented with regard to the
meaning of its arguments.

https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interfac...
lily/side-position-interface.cc:498: goto skip_skyline_stuff;
A goto.  Fabulous.

https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interfac...
lily/side-position-interface.cc:505: // ugh...gets called too often...?
This comment sounds like you had unexpected effects when debugging.  Are you
going to leave them in as easter eggs?

https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interfac...
lily/side-position-interface.cc:515: if (Skyline_pair *inside_staff_skylines =
Skyline_pair::unsmob (vag->get_object ("inside-staff-skylines")))
get_object can _create_ an object via callback etc, and then the _only_ SCM
referring to it is unsmobbed by you.  The Skyline_pair is _immediately_ eligible
for garbage collection for that reason.  You need to assign the return value of
get_object to an SCM variable.  And you either unsmob it for every use, _or_ you
need to use scm_remember_upto_here_1 on it right behind the last use of the
unsmobbed Skyline_pair since otherwise the optimizer might optimize the variable
away and then the Scheme garbage collector does not see it is still in use.

https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interfac...
lily/side-position-interface.cc:539: for (; i < elements.size () && elements[i]
!= me; i++)
What's that condition?  You stop _completely_ when reaching me?  Do you not
rather mean to merely _skip_ that particular element?

https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interfac...
lily/side-position-interface.cc:542: Skyline_pair *v_orig = Skyline_pair::unsmob
(elt->get_property ("vertical-skylines"));
Is every element _guaranteed_ to have a property vertical-skylines?  Or do we
just crash if it doesn't?

https://codereview.appspot.com/7185044/diff/17001/lily/slur.cc
File lily/slur.cc (right):

https://codereview.appspot.com/7185044/diff/17001/lily/slur.cc#newcode301
lily/slur.cc:301: Interval yext = robust_relative_extent (script, script,
Y_AXIS);
What happens with cy?

https://codereview.appspot.com/7185044/diff/17001/lily/system.cc
File lily/system.cc (right):

https://codereview.appspot.com/7185044/diff/17001/lily/system.cc#newcode423
lily/system.cc:423: Axis_group_interface::sort_outside_staff_elements (me,
vertical_skyline_grobs);
Which algorithm requires the outside_staff_elements to be sorted, where is that
requirement documented?

https://codereview.appspot.com/7185044/diff/17001/lily/tuplet-bracket.cc
File lily/tuplet-bracket.cc (right):

https://codereview.appspot.com/7185044/diff/17001/lily/tuplet-bracket.cc#newc...
lily/tuplet-bracket.cc:494: my_offset is, for now, only used for cross-staff
grobs.
The code belies this statement.  In fact, my_offset has been _removed_ from the
code conditioned on !cross-staff, and is used below in code not depending either
way on cross-staff.

So it would be quite important to document what it is really supposed to be used
for.

[MikeSol, 2013-02-03 08:44:18]

Incorporates David's suggestions

[mike7, 2013-02-03 08:45:13]

On 1 févr. 2013, at 16:04, dak@gnu.org wrote:

> As previously, there are no comments whatsoever putting the code into
> perspective.  This is an amorphous heap of code without an attempt of
> explaining its design or inner logic.  There is a single function
> comment giving a glimpse of what that function is supposed to do.
> However, there is no explanation of the context this function is
> supposed to be used in or for, the function naming bears no relation to
> its return value, the comment is unclear, half of the named entities
> don't exist in the function interface, and half of the existing entities
> are not mentioned.

Thanks for the review - comments are addressed below.
The code exists in order to use Y-offsets of grobs, via the side position
interface, to do outside staff placement instead of translate axis.

> 
> 
>
https://codereview.appspot.com/7185044/diff/17001/lily/directional-element-in...
> File lily/directional-element-interface.cc (right):
> 
>
https://codereview.appspot.com/7185044/diff/17001/lily/directional-element-in...
> lily/directional-element-interface.cc:25: recursive_get_grob_direction
> (Grob *me)
> Why is not direction-source used instead?  This code seems like being
> intended to replace proper information by heuristics that mask a problem
> most of the time.

direction-source, if anything, seems like a work-around for the more logical
recursive_get_grob_direction.  If a Grob foo has direction UP and it has
children (or grandchildren), the assumption is that these grobs should have the
same direction as foo unless otherwise specified.  I don't think that's
replacing proper information.

> 
> https://codereview.appspot.com/7185044/diff/17001/lily/script-interface.cc
> File lily/script-interface.cc (right):
> 
>
https://codereview.appspot.com/7185044/diff/17001/lily/script-interface.cc#ne...
> lily/script-interface.cc:81: // ie DynamicText takes direction from
> DynamicLineSpanner
> Why then would DynamicLineSpanner not be the direction-source of
> DynamicText?  I applaud that you are bucking the trend of making the
> entire patch without comments, but it looks like the code is intended to
> mask a bug by sometimes doing the right thing by accident.

Same logic as above - when all else fails, take the direction of the Y parent. 
It seems to me that a reasonable default is "if no direction property is set,
assume that the grob has the same direction as its Y-parent."  This type of
logic is all over lilypond - for example, if a grob does not have a Y-offset,
its Y-offset is the same as that of its parent.  I'm just reimplementing the "if
no information given, use parents' information" with the direction property.

> 
>
https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interfac...
> File lily/side-position-interface.cc (right):
> 
>
https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interfac...
> lily/side-position-interface.cc:185: // or with anything in
> other_v_skylines.
> Congrats.  This is actually the first comment that actually gives _any_
> information for the actual intent of a function.  Unfortunately, there
> is no indication _what_ is being returned.
> "avoid_outside_staff_collisions" does not sound like it would return any
> value.  And what information do we get?  "Returns the value for the grob
> elt" (there is no grob elt in the arguments) "whose skylines are given
> by h_skyline..." (there is no h_skyline in the arguments) "so that it
> doesn't intersect with staff_skyline" (there is no staff_skyline in the
> arguments).
> 

Fixed.

> And what is the grob going to do with that value to stop intersecting?
> Scale itself by that value?  Buy itself a hoverboard worth the value in
> order to ride the skylines?

I like the second option!  Comment expanded...

> 
> Please give the function a name making any sense in connection with its
> return value, and if you bother writing a comment, please make sure that
> it refers to actually involved variables and arguments.  And what's with
> padding, horizon_padding, other_padding, and other_horizon_padding?
> What's with dir?

More explination added

> 
>
https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interfac...
> lily/side-position-interface.cc:296: //printf ("        Q %s\n", e->name
> ().c_str ());
> What's this supposed to be?
> 

Erased.

>
https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interfac...
> lily/side-position-interface.cc:461: MAKE_SCHEME_CALLBACK
> (Side_position_interface, outside_staff_aligned_side, 1);
> Sure, something called "outside_staff_aligned_side" is so obvious in
> meaning that we don't need to explain what arguments it gets and what
> the meaning of the returned value would be.  Just for the record: I am
> being sarcastical here.
> 
> Mike, how is _anybody_ going to be able to understand _anything_ using
> an undocumented callback with that name?

Comment added.

> 
>
https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interfac...
> lily/side-position-interface.cc:481:
> Side_position_interface::calc_outside_staff_offset (Grob *me, Axis a,
> bool pure, int start, int end, Real current_offset)
> Now here is a function name that gives an _inkling_ of a clue what the
> returned value is: an offset.  It still is totally undocumented with
> regard to the meaning of its arguments.

Comment added.  bool pure, int start, int end are almost never documented
anywhere.  Ditto for Grob *me.  I will do my best to get into the practice of
holding myself to the standard of documenting arguments and return values for
every new function I add.  But I think it is overkill.

> 
>
https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interfac...
> lily/side-position-interface.cc:498: goto skip_skyline_stuff;
> A goto.  Fabulous.
> 

Are goto's really that evil when they're the easiest way to skip code?

>
https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interfac...
> lily/side-position-interface.cc:505: // ugh...gets called too often...?
> This comment sounds like you had unexpected effects when debugging.  Are
> you going to leave them in as easter eggs?

No, it is a reminder to me to do performance checks.  So far, no one has
reported slow downs, so I will remove this.

> 
>
https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interfac...
> lily/side-position-interface.cc:515: if (Skyline_pair
> *inside_staff_skylines = Skyline_pair::unsmob (vag->get_object
> ("inside-staff-skylines")))
> get_object can _create_ an object via callback etc, and then the _only_
> SCM referring to it is unsmobbed by you.  The Skyline_pair is
> _immediately_ eligible for garbage collection for that reason.  You need
> to assign the return value of get_object to an SCM variable.  And you
> either unsmob it for every use, _or_ you need to use
> scm_remember_upto_here_1 on it right behind the last use of the
> unsmobbed Skyline_pair since otherwise the optimizer might optimize the
> variable away and then the Scheme garbage collector does not see it is
> still in use.

Changed.

> 
>
https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interfac...
> lily/side-position-interface.cc:539: for (; i < elements.size () &&
> elements[i] != me; i++)
> What's that condition?  You stop _completely_ when reaching me?  Do you
> not rather mean to merely _skip_ that particular element?
> 

No, we stop completely, because the list is sorted, which means that all other
elements afterwards are not support elements but rather go outside of the
current element.  Will make this clearer in the docstring.

>
https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interfac...
> lily/side-position-interface.cc:542: Skyline_pair *v_orig =
> Skyline_pair::unsmob (elt->get_property ("vertical-skylines"));
> Is every element _guaranteed_ to have a property vertical-skylines?  Or
> do we just crash if it doesn't?

grob.cc line 83.

> 
> https://codereview.appspot.com/7185044/diff/17001/lily/slur.cc
> File lily/slur.cc (right):
> 
> https://codereview.appspot.com/7185044/diff/17001/lily/slur.cc#newcode301
> lily/slur.cc:301: Interval yext = robust_relative_extent (script,
> script, Y_AXIS);
> What happens with cy?
> 

We regain the effect of using cy by adding the offset via yextent.translate. 
Otherwise we get a circular dependency for the height of the script.

> https://codereview.appspot.com/7185044/diff/17001/lily/system.cc
> File lily/system.cc (right):
> 
> https://codereview.appspot.com/7185044/diff/17001/lily/system.cc#newcode423
> lily/system.cc:423: Axis_group_interface::sort_outside_staff_elements
> (me, vertical_skyline_grobs);
> Which algorithm requires the outside_staff_elements to be sorted, where
> is that requirement documented?

Added this to the vertical-skyline-grobs docstring.  The algorithm used is
Axis_group_interface::sort_outside_staff_elements.

> 
> https://codereview.appspot.com/7185044/diff/17001/lily/tuplet-bracket.cc
> File lily/tuplet-bracket.cc (right):
> 
>
https://codereview.appspot.com/7185044/diff/17001/lily/tuplet-bracket.cc#newc...
> lily/tuplet-bracket.cc:494: my_offset is, for now, only used for
> cross-staff grobs.
> The code belies this statement.  In fact, my_offset has been _removed_
> from the code conditioned on !cross-staff, and is used below in code not
> depending either way on cross-staff.
> 
> So it would be quite important to document what it is really supposed to
> be used for.
> 
> https://codereview.appspot.com/7185044/

[dak, 2013-02-03 20:33:11]

On 2013/02/03 08:45:13, mike7 wrote:
> On 1 févr. 2013, at 16:04, mailto:dak@gnu.org wrote:
> 
> > As previously, there are no comments whatsoever putting the code into
> > perspective.  This is an amorphous heap of code without an attempt of
> > explaining its design or inner logic.  There is a single function
> > comment giving a glimpse of what that function is supposed to do.
> > However, there is no explanation of the context this function is
> > supposed to be used in or for, the function naming bears no relation to
> > its return value, the comment is unclear, half of the named entities
> > don't exist in the function interface, and half of the existing entities
> > are not mentioned.
> 
> Thanks for the review - comments are addressed below.
> The code exists in order to use Y-offsets of grobs, via the side position
> interface, to do outside staff placement instead of translate axis.

What does "instead of translate axis" mean?

> It seems to me that a reasonable default is "if no direction property is set,
> assume that the grob has the same direction as its Y-parent."

That assumes that the direction is always specified relative to the
same entity, and that a child is always placed further from the axis
than its parent.  I am not convinced of that.

>
https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interfac...
> > lily/side-position-interface.cc:461: MAKE_SCHEME_CALLBACK
> > (Side_position_interface, outside_staff_aligned_side, 1);
> > Sure, something called "outside_staff_aligned_side" is so obvious in
> > meaning that we don't need to explain what arguments it gets and what
> > the meaning of the returned value would be.  Just for the record: I am
> > being sarcastical here.
> > 
> > Mike, how is _anybody_ going to be able to understand _anything_ using
> > an undocumented callback with that name?
> 
> Comment added.

That's good to have, but no substitute for a sensible name.  If you
return a shift or offset, the function needs to be named accordingly.
"outside_staff_aligned_side" does not jibe with something returning a
Real value.

>
https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interfac...
> > lily/side-position-interface.cc:481:
> > Side_position_interface::calc_outside_staff_offset (Grob *me, Axis a,
> > bool pure, int start, int end, Real current_offset)
> > Now here is a function name that gives an _inkling_ of a clue what the
> > returned value is: an offset.  It still is totally undocumented with
> > regard to the meaning of its arguments.
> 
> Comment added.  bool pure, int start, int end are almost never documented
> anywhere.

It's perfectly fine to just state "standard arguments for grob
callbacks, see xxx.xxx for explanation".  However, I think that "bool
pure" is an invention of your own, and that makes it likely that there
is no explanation anywhere to be found.  I'd definitely prefer one in
a central place, and just a crossreference here.

That does not mean that I prefer a patch that leaves out all
information on the assumption that somebody, sometime, somewhere will
provide the missing parts.

> Ditto for Grob *me.  I will do my best to get into the practice of
> holding myself to the standard of documenting arguments and return
> values for every new function I add.  But I think it is overkill.

It is overkill _if_ there is a central point where they are documented
_and_ you refer there.  It is overkill _if_ the function is named in a
manner that the meaning is obvious _and_ matching a family of
functions documented in a central point you refer to and/or documented
at the call site.

Anything that takes someone longer than a minute to work out is worth
half the minute writing the comment.

>
https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interfac...
> > lily/side-position-interface.cc:498: goto skip_skyline_stuff;
> > A goto.  Fabulous.
> > 
> 
> Are goto's really that evil when they're the easiest way to skip
> code?

The easiest way is to return the value you want to see returned.  It
would appear you have not even bothered looking at the target of the
goto, and yet you expect the reader of the code to search for the
target of the goto.  Why him and and not you?

>
https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interfac...
> > lily/side-position-interface.cc:542: Skyline_pair *v_orig =
> > Skyline_pair::unsmob (elt->get_property ("vertical-skylines"));
> > Is every element _guaranteed_ to have a property vertical-skylines?  Or
> > do we just crash if it doesn't?
> 
> grob.cc line 83.

Fine, so you write

assert (v_orig);  // Should be assured by function Grob::whatever

here before using that value.  If someone changes grob.cc line 83 for
some reason, this assertion will make sure that the now-violated
assumptions will not go unnoticed.

> > https://codereview.appspot.com/7185044/diff/17001/lily/slur.cc#newcode301
> > lily/slur.cc:301: Interval yext = robust_relative_extent (script,
> > script, Y_AXIS);
> > What happens with cy?
> > 
> 
> We regain the effect of using cy by adding the offset via
> yextent.translate. 

So why do we even calculate it?

I have not looked at the code changes so far; this is just a followup
on your comments.

[mike7, 2013-02-03 22:29:04]

On 3 févr. 2013, at 21:33, dak@gnu.org wrote:

> On 2013/02/03 08:45:13, mike7 wrote:
>> On 1 févr. 2013, at 16:04, mailto:dak@gnu.org wrote:
> 
>> > As previously, there are no comments whatsoever putting the code
> into
>> > perspective.  This is an amorphous heap of code without an attempt
> of
>> > explaining its design or inner logic.  There is a single function
>> > comment giving a glimpse of what that function is supposed to do.
>> > However, there is no explanation of the context this function is
>> > supposed to be used in or for, the function naming bears no relation
> to
>> > its return value, the comment is unclear, half of the named entities
>> > don't exist in the function interface, and half of the existing
> entities
>> > are not mentioned.
> 
>> Thanks for the review - comments are addressed below.
>> The code exists in order to use Y-offsets of grobs, via the side
> position
>> interface, to do outside staff placement instead of translate axis.
> 
> What does "instead of translate axis" mean?

Instead of calling Grob::translate_axis.

> 
>> It seems to me that a reasonable default is "if no direction property
> is set,
>> assume that the grob has the same direction as its Y-parent."
> 
> That assumes that the direction is always specified relative to the
> same entity,

Yes, it assumes that if grob foo has no direction specified and its parent has
one, then its direction is that of the parent.  In music, that makes sense for
me.  Can you think of an outside-staff grob that has a Y child whose direction
is not that of the Y parent.

> and that a child is always placed further from the axis
> than its parent.

That is not the logic implemented in the code.

> I am not convinced of that.
> 
> 
>
https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interfac...
>> > lily/side-position-interface.cc:461: MAKE_SCHEME_CALLBACK
>> > (Side_position_interface, outside_staff_aligned_side, 1);
>> > Sure, something called "outside_staff_aligned_side" is so obvious in
>> > meaning that we don't need to explain what arguments it gets and
> what
>> > the meaning of the returned value would be.  Just for the record: I
> am
>> > being sarcastical here.
>> >
>> > Mike, how is _anybody_ going to be able to understand _anything_
> using
>> > an undocumented callback with that name?
> 
>> Comment added.
> 
> That's good to have, but no substitute for a sensible name.  If you
> return a shift or offset, the function needs to be named accordingly.
> "outside_staff_aligned_side" does not jibe with something returning a
> Real value.

Fixed.

> 
> 
>
https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interfac...
>> > lily/side-position-interface.cc:481:
>> > Side_position_interface::calc_outside_staff_offset (Grob *me, Axis
> a,
>> > bool pure, int start, int end, Real current_offset)
>> > Now here is a function name that gives an _inkling_ of a clue what
> the
>> > returned value is: an offset.  It still is totally undocumented with
>> > regard to the meaning of its arguments.
> 
>> Comment added.  bool pure, int start, int end are almost never
> documented
>> anywhere.
> 
> It's perfectly fine to just state "standard arguments for grob
> callbacks, see xxx.xxx for explanation".  However, I think that "bool
> pure" is an invention of your own, and that makes it likely that there
> is no explanation anywhere to be found.  I'd definitely prefer one in
> a central place, and just a crossreference here.

grob.cc, line 411.
But where would you want the central explanation.

> 
> That does not mean that I prefer a patch that leaves out all
> information on the assumption that somebody, sometime, somewhere will
> provide the missing parts.
> 
>> Ditto for Grob *me.  I will do my best to get into the practice of
>> holding myself to the standard of documenting arguments and return
>> values for every new function I add.  But I think it is overkill.
> 
> It is overkill _if_ there is a central point where they are documented
> _and_ you refer there.  It is overkill _if_ the function is named in a
> manner that the meaning is obvious _and_ matching a family of
> functions documented in a central point you refer to and/or documented
> at the call site.

Where?

> 
> Anything that takes someone longer than a minute to work out is worth
> half the minute writing the comment.

I'm totally cool with this, but where should I put it?

> 
> 
>
https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interfac...
>> > lily/side-position-interface.cc:498: goto skip_skyline_stuff;
>> > A goto.  Fabulous.
>> >
> 
>> Are goto's really that evil when they're the easiest way to skip
>> code?
> 
> The easiest way is to return the value you want to see returned.  It
> would appear you have not even bothered looking at the target of the
> goto, and yet you expect the reader of the code to search for the
> target of the goto.  Why him and and not you?

Fixed.

> 
> 
>
https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interfac...
>> > lily/side-position-interface.cc:542: Skyline_pair *v_orig =
>> > Skyline_pair::unsmob (elt->get_property ("vertical-skylines"));
>> > Is every element _guaranteed_ to have a property vertical-skylines?
> Or
>> > do we just crash if it doesn't?
> 
>> grob.cc line 83.
> 
> Fine, so you write
> 
> assert (v_orig);  // Should be assured by function Grob::whatever
> 
> here before using that value.  If someone changes grob.cc line 83 for
> some reason, this assertion will make sure that the now-violated
> assumptions will not go unnoticed.

Fixed.

> 
>> >
> https://codereview.appspot.com/7185044/diff/17001/lily/slur.cc#newcode301
>> > lily/slur.cc:301: Interval yext = robust_relative_extent (script,
>> > script, Y_AXIS);
>> > What happens with cy?
>> >
> 
>> We regain the effect of using cy by adding the offset via
>> yextent.translate.
> 
> So why do we even calculate it?

It's used elsewhere in the function.

Thanks for the review!

Cheers,
MS

> 
> I have not looked at the code changes so far; this is just a followup
> on your comments.
> 
> 
> https://codereview.appspot.com/7185044/

[MikeSol, 2013-02-03 22:34:03]

Incorporates David's comments

[dak, 2013-02-14 19:20:20]

I've not really reviewed everything here, just highlights.  Regarding the
commenting problems: it is important for the reviewer or maintainer or rereader
to get the gist of the story.  Much of the LilyPond codebase has been written
with a total disregard to people not present during the writing.  For that
reason, it is not a useful metric for writing LilyPond code that is supposed to
get reviewed and maintained by people other than the original author.

If a review is to make sense, "I think Han-Wen would likely understand this" is
not enough since the reviewers are different from Han-Wen, and we want more
people than just you and him to be able to maintain this code in future.

The LilyPond code base has a maintainability problem due to historic reasons,
and we need to move away from that legacy rather than adding to it.

Only part of this review is actually specific to this particular patch.  A lot
is rather trying to bring across some general advice to coding and commenting
practice.

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

https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-num...
input/regression/tuplet-number-outside-staff-positioning.ly:15: \override
TupletBracket.Y-offset =
#ly:side-position-interface::calc-outside-staff-y-offset
This is indicating a fundamental design problem: this override is not related to
the topic of the regtest.  If it is necessary for getting the desired result, it
will be necessary in a whole _lot_ of user-created situations.  But it is not an
easily user-accessibly override, and it is not documented in normal user
documentation.

This suspiciously looks like tampering with unrelated regtests in order to mask
fundamental problems from review.  Can you explain why this is not the case?

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

https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-num...
input/regression/tuplet-number-outside-staff-priority.ly:13: \override
TupletNumber.Y-offset = #ly:side-position-interface::calc-outside-staff-y-offset
See above comment.  If outside-staff-priority ceases to work, a lot of
user-level documentation would warrant adaption.

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.c...
lily/axis-group-interface.cc:595: Axis_group_interface::staff_priority_less
(Grob *const &g1, Grob *const &g2)
Is there a reason you turn this into member functions?

https://codereview.appspot.com/7185044/diff/32003/lily/axis-group-interface.c...
lily/axis-group-interface.cc:713:
Axis_group_interface::prepare_for_outside_staff_calculations (Grob *me)
There is no description anywhere what this preparation will entail, what it
looks at, and what it does.  Is this used even more than once?

https://codereview.appspot.com/7185044/diff/32003/lily/axis-group-interface.c...
lily/axis-group-interface.cc:755:
Axis_group_interface::sort_outside_staff_elements (Grob *me, vector<Grob *>
&elements)
This looks like it does a whole lot more than just sorting some elements.  What
does it do?  Why does it do that?

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

https://codereview.appspot.com/7185044/diff/32003/lily/include/axis-group-int...
lily/include/axis-group-interface.hh:69: static bool staff_priority_less (Grob
*const &g1, Grob *const &g2);
All those were previously staic functions inside of axis-group-interface.cc, and
thus limited to that file.  Now you have made them part of the class definition,
and in addition you have turned then into a _public_ part of the class
definition, suggesting that they are part of the external interface of the
class.

Why?

https://codereview.appspot.com/7185044/diff/32003/lily/include/directional-el...
File lily/include/directional-element-interface.hh (right):

https://codereview.appspot.com/7185044/diff/32003/lily/include/directional-el...
lily/include/directional-element-interface.hh:26: // what is the advantage not
having these two as STATICs of GROB -- jcn
"these three" it would appear now.  And the question seems valid.

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

https://codereview.appspot.com/7185044/diff/32003/lily/include/side-position-...
lily/include/side-position-interface.hh:45: static Real
calc_outside_staff_offset (Grob *me, bool pure, int start, int end, Real
current_offset);
Isn't that just something used internally by calc_outside_staff_y_offset?  Is
there a reason this is part of a _public_ interface?

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

https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interfac...
lily/side-position-interface.cc:184: // v_skyline) needs to move UP or DOWN so
that it doesn't intersect with
Presumably the function does not pick "UP or DOWN" on a whim, but it is rather
"in direction of dir".  However, if it can be _added_ to the grob's Y-offset, it
is _not_ the value to move UP or DOWN, but rather the offset to add to Y-offset
in order to position it beyond all other_v_skylines with regard to direction dir
(UP or DOWN).

Wait: you say "dir is the placement above or below the staff".  Does that mean
that dir can be UP, but the placement of the offset can be _below_ the
other_v_skylines but above the staff?  If so, will it first attempt to fit elt
_between_ staff and other_v_skylines, or will it go outside in any case?

https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interfac...
lily/side-position-interface.cc:464: Real outside_staff_offset = a == Y_AXIS
This is quite contorted and hard to read, what with = == ?:...

What you mean is
if (a == Y_AXIS)
  return scm_from_double (total_off
                          + calc_outside ...);
return scm_from_double (total_off);

If you want to, with an else.  Depends on your style.  Or the other way round,
starting with if (a == X_AXIS)...

But the point is that it is pointless to juggle with ?: to avoid an "if" if one
branch of ?: is going to "add" 0, namely do nothing anyway.  This means not just
different offsets, but different _actions_.  ?: can always be replaced, but it
has a few convenient use cases.  This isn't one.  Code should not be an
intelligence test, but boring.

https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interfac...
lily/side-position-interface.cc:497: // start: start of pure calculation for
spanners
Well, you are right that this place is strange to do a full documentation of
pure function arguments.  But we need to get one eventually somewhere (not
necessarily as part of this issue), and we are talking about how to do proper
commenting, so let's focus on what's wrong about these comments anyway, even
though they don't necessarily belong here exactly.

Let's take apart what is wrong in detail:

They don't add useful information.

Well, apart from "for now facultative", but that's information about the
implementation rather than the use.

eventually "is this pure": it does not specify the target for "eventually", and
it does not specify "this".

What presumably is meant is "is the returned offset to be derived via pure
calculation?".

Then start and end are presumably _only_ relevant when it _is_ a pure
calculation.  Not mentioned.  Then the comment "start/end of pure calculation
for spanners" is totally awful.  The parameters are already _called_ start/end,
only repeating that is not helpful.  It is not at all apparent what the
connection with _spanners_ would be.  And "start of pure calculation" is
nondescriptive.  What is the unit that the "start" is specified in?  Is it
seconds since Jan 1 1970 UTC?  Or is it rather some index into some data
structure?  Which one?  Give a term that can be reasonably found using "git
grep", and "start" and "end" alone are not really helpful for that.

https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interfac...
lily/side-position-interface.cc:538: if (Skyline_pair *inside_staff_skylines =
Skyline_pair::unsmob (iss))
After this line, GUILE no longer sees iss as something it needs to protect from
garbage collection.

https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interfac...
lily/side-position-interface.cc:539: skylines = Skyline_pair
(*inside_staff_skylines);
Now if Skyline_pair contains any elements allocated by GUILE (non-immediate
Scheme values), it will ask GUILE for new elements, and GUILE might allocate
them from *inside_staff_skylines data.  It turns out from looking through the
headers that Skyline_pair is free from SCM data.  But doing that kind of
research every time one reads the code is costly, so it is easier if you just
stick with the SCM value until you have created your copy or finished with the
whole thing.

unsmob it for every use until then, and GUILE will let it stick around until the
last unsmob.  Or put an scm_remember_upto_here_1 on the SCM value after the last
use of an unsmobbed pointer from it.

https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interfac...
lily/side-position-interface.cc:565: Skyline_pair *v_orig = Skyline_pair::unsmob
(elt->get_property ("vertical-skylines"));
Same here.

https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interfac...
lily/side-position-interface.cc:593: Skyline_pair v_skylines (*v_orig);
Too much distance to above, and several calls of get_property, potentially
allocating memory, in between.  I explained all this in the previous review. 
Please generalize one explanation to all occurences of the same problem.  If the
problem is not clear to you, ask questions until it is.  Reviews (and things
like valgrind) are a safety net, not a trampoline.

[mike7, 2013-02-15 06:37:20]

Thank you for the _excellent_ review.  This is _exactly_ the type of stuff I
need.

>
https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-num...
> File input/regression/tuplet-number-outside-staff-positioning.ly
> (right):
> 
>
https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-num...
> input/regression/tuplet-number-outside-staff-positioning.ly:15:
> \override TupletBracket.Y-offset =
> #ly:side-position-interface::calc-outside-staff-y-offset
> This is indicating a fundamental design problem: this override is not
> related to the topic of the regtest.  If it is necessary for getting the
> desired result, it will be necessary in a whole _lot_ of user-created
> situations.  But it is not an easily user-accessibly override, and it is
> not documented in normal user documentation.
> 
> This suspiciously looks like tampering with unrelated regtests in order
> to mask fundamental problems from review.  Can you explain why this is
> not the case?

Your question gets to the core of the logic of the patch, so I'll explain it and
then people can comment upon how that links up with this regtest.

In LilyPond, about 85% of grob properties are set as the result of the
evaluation of a callback or the use of a rote value.  About 15% are set via
calls to set_property and translate_axis (the latter of which only controls Y
and X offset).

The top-down approach to property setting (i.e. a VerticalAxisGroup controlling
the Y-offset of its elements, which is currently the case for outside staff
positioning) poses the problem that once a value is cached, it is very difficult
to run calculations again for that value once more precise information is
available.  With the bottom-up approach (grobs reporting their properties to
their collecting grobs), however, this is easy and is done all over the code
base.  For example, in separation-item.cc, approximations of heights are used in
Separation_item::boxes to do horizontal spacing.  I am trying to get LilyPond to
a point where it does vertical spacing the same way.  This will allow for much
better approximation of distances between staves containing cross-staff grobs.

If this is the case that grobs' offsets will be controlled by callbacks and not
by uber-grobs like VerticalAxisGroup calling translate_axis a bunch of times,
then grobs that are going to be placed outside the staff need to have their
Y-offset function reflect that.  Now, LilyPond, for various callbacks, other
properties must be set for those callbacks to make sense.  For example, if I
do:

\override NoteHead #'stencil = #ly:text-interface::print

Nothing will happen.  But if I do:

\override NoteHead #'text = #"foo"
\override NoteHead #'stencil = #ly:text-interface::print

The NoteHead will be printed as foo.  This is exactly what we're seeing in the
regtest tuplet-number-outside-staff-positioning.ly.  A callback is set for
Y-offset that allows the grob to be positioned outside the staff.  But, just as
the text interface needs to know what text to print, the side-position-interface
needs to know what outside-staff-priority to use to place the grob.  Thus the
use of two overrides instead of one.

A music function could be done in the form of:

\addOutsideStaffPriorityToGrob #'TupletBracket #100

that rolls the two overrides into one.  This is a UI issue about which I have
not thought yet but that absolutely deserves attention.  For those who are more
adept at UI than I, I'm very interested in your ideas.  Furthermore, how could
this be documented to make sense to the other user.  For me, the sentence "some
overrides require supplemental overrides to kick in" seems kind of abstract and
geeky.  How can this be communicated?

> 
>
https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-num...
> File input/regression/tuplet-number-outside-staff-priority.ly (right):
> 
>
https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-num...
> input/regression/tuplet-number-outside-staff-priority.ly:13: \override
> TupletNumber.Y-offset =
> #ly:side-position-interface::calc-outside-staff-y-offset
> See above comment.  If outside-staff-priority ceases to work, a lot of
> user-level documentation would warrant adaption.

I agree, but this would only be the case for grobs not already using
ly:side-position-interface::y-aligned-side.  Currently, every grob using
outside-staff-priority is using this function as a callback.  So the only thing
that users will need to be made aware of is the effect of this change on
overrides.  What would be a good way to communicate that?

> 
>
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.c...
> lily/axis-group-interface.cc:595:
> Axis_group_interface::staff_priority_less (Grob *const &g1, Grob *const
> &g2)
> Is there a reason you turn this into member functions?
> 

There was, but that is no longer relevant.  I will change the code to reflect
that.

>
https://codereview.appspot.com/7185044/diff/32003/lily/axis-group-interface.c...
> lily/axis-group-interface.cc:713:
> Axis_group_interface::prepare_for_outside_staff_calculations (Grob *me)
> There is no description anywhere what this preparation will entail, what
> it looks at, and what it does.  Is this used even more than once?

It is used twice - once in axis-group-interface.cc and once in
side-position-interface.cc.  I have added a comment in the code base that I've
copied and pasted below:

/*
  Before doing calculations involving outside staff priority are done, we
  suicide any grobs that need to be suicided and make sure that grobs that
  depend on the placement of a Y parent do not have a lower
  outside-staff-priority than the parent, which would cause their being
  placed first.
*/

> 
>
https://codereview.appspot.com/7185044/diff/32003/lily/axis-group-interface.c...
> lily/axis-group-interface.cc:755:
> Axis_group_interface::sort_outside_staff_elements (Grob *me, vector<Grob
> *> &elements)
> This looks like it does a whole lot more than just sorting some
> elements.  What does it do?  Why does it do that?

I have added a comment in the code base that I've copied and pasted below:

/*
  When placing a group of outside-staff-elements outside the staff, we want
  to sort their placement order based on three criteria.
  
  1) Grobs with a lower outside-staff-priority will always be placed before
     grobs with a higher outside-staff-priority.
  2) Grobs with outside-staff-priority set that are the Y-parents of other
     grobs will be placed before the children grobs are placed.
  3) For grobs with equal outside-staff-priority, we use the
     outside-staff-placement-directive property to break ties. The docstring
     for outside-staff-placement-directive describes how these ties are
     broken.

  After the grobs are sorted using these three criteria, their Y-offset  
  can be calculated.
*/

There is also a long-ish comment in the function with more information.

> 
>
https://codereview.appspot.com/7185044/diff/32003/lily/include/axis-group-int...
> File lily/include/axis-group-interface.hh (right):
> 
>
https://codereview.appspot.com/7185044/diff/32003/lily/include/axis-group-int...
> lily/include/axis-group-interface.hh:69: static bool staff_priority_less
> (Grob *const &g1, Grob *const &g2);
> All those were previously staic functions inside of
> axis-group-interface.cc, and thus limited to that file.  Now you have
> made them part of the class definition, and in addition you have turned
> then into a _public_ part of the class definition, suggesting that they
> are part of the external interface of the class.
> 
> Why?

It is based on a previous idea that I abandoned, so it's set back to the way it
was.

> 
>
https://codereview.appspot.com/7185044/diff/32003/lily/include/directional-el...
> File lily/include/directional-element-interface.hh (right):
> 
>
https://codereview.appspot.com/7185044/diff/32003/lily/include/directional-el...
> lily/include/directional-element-interface.hh:26: // what is the
> advantage not having these two as STATICs of GROB -- jcn
> "these three" it would appear now.  And the question seems valid.

This is a good question.  It is a trivial change but the reasons justifying
making these static member functions of Grob are over my head.  It seems like a
separate patch, pushed before or after this one, can deal with that.

> 
>
https://codereview.appspot.com/7185044/diff/32003/lily/include/side-position-...
> File lily/include/side-position-interface.hh (right):
> 
>
https://codereview.appspot.com/7185044/diff/32003/lily/include/side-position-...
> lily/include/side-position-interface.hh:45: static Real
> calc_outside_staff_offset (Grob *me, bool pure, int start, int end, Real
> current_offset);
> Isn't that just something used internally by
> calc_outside_staff_y_offset?  Is there a reason this is part of a
> _public_ interface?
> 

No.  There are a lot of internal placement functions that are part of a public
interface, so I just followed that convention.

For example, Self_alignment_interface::aligned_on_self is only ever used
internally in self-alignment-interface.cc.  So why is it a public function?  I
don't know the answer because I don't know enough about C++ coding to determine
what should be public or private.  However, this sort of thing is all over the
code base.  It may be worth asking Han Wen and Jan why they did it this way, as
I wouldn't want to change the convention unless there was a solid reason.

>
https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interfac...
> File lily/side-position-interface.cc (right):
> 
>
https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interfac...
> lily/side-position-interface.cc:184: // v_skyline) needs to move UP or
> DOWN so that it doesn't intersect with
> Presumably the function does not pick "UP or DOWN" on a whim, but it is
> rather "in direction of dir".  However, if it can be _added_ to the
> grob's Y-offset, it is _not_ the value to move UP or DOWN, but rather
> the offset to add to Y-offset in order to position it beyond all
> other_v_skylines with regard to direction dir (UP or DOWN).
> 
> Wait: you say "dir is the placement above or below the staff".  Does
> that mean that dir can be UP, but the placement of the offset can be
> _below_ the other_v_skylines but above the staff?  If so, will it first
> attempt to fit elt _between_ staff and other_v_skylines, or will it go
> outside in any case?

I've added the following comment:

// Note that this function will try to keep elements as close to the staff
// as possible. So, if the element fits under other elements above the staff,
// it will be shifted under instead of over these elements.  So, the dir
// property does not refer to the placement with respect to other vertical
// skylines, but rather with respect to the staff.

Let me know if that's clear.

> 
>
https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interfac...
> lily/side-position-interface.cc:464: Real outside_staff_offset = a ==
> Y_AXIS
> This is quite contorted and hard to read, what with = == ?:...
> 
> What you mean is
> if (a == Y_AXIS)
>  return scm_from_double (total_off
>                          + calc_outside ...);
> return scm_from_double (total_off);
> 
> If you want to, with an else.  Depends on your style.  Or the other way
> round, starting with if (a == X_AXIS)...
> 
> But the point is that it is pointless to juggle with ?: to avoid an "if"
> if one branch of ?: is going to "add" 0, namely do nothing anyway.  This
> means not just different offsets, but different _actions_.  ?: can
> always be replaced, but it has a few convenient use cases.  This isn't
> one.  Code should not be an intelligence test, but boring.

Fixed.

> 
>
https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interfac...
> lily/side-position-interface.cc:497: // start: start of pure calculation
> for spanners
> Well, you are right that this place is strange to do a full
> documentation of pure function arguments.

Removed.

> But we need to get one
> eventually somewhere (not necessarily as part of this issue), and we are
> talking about how to do proper commenting, so let's focus on what's
> wrong about these comments anyway, even though they don't necessarily
> belong here exactly.

Ok.

> 
> Let's take apart what is wrong in detail:
> 
> They don't add useful information.
> 
> Well, apart from "for now facultative", but that's information about the
> implementation rather than the use.
> 
> eventually "is this pure": it does not specify the target for
> "eventually", and it does not specify "this".
> 
> What presumably is meant is "is the returned offset to be derived via
> pure calculation?".
> 
> Then start and end are presumably _only_ relevant when it _is_ a pure
> calculation.  Not mentioned.  Then the comment "start/end of pure
> calculation for spanners" is totally awful.  The parameters are already
> _called_ start/end, only repeating that is not helpful.  It is not at
> all apparent what the connection with _spanners_ would be.  And "start
> of pure calculation" is nondescriptive.  What is the unit that the
> "start" is specified in?  Is it seconds since Jan 1 1970 UTC?  Or is it
> rather some index into some data structure?  Which one?  Give a term
> that can be reasonably found using "git grep", and "start" and "end"
> alone are not really helpful for that.
> 

The reason this comment was so brief is because I didn't understand why you
wanted to see documentation of pure functions in this place and not in the 20 or
so other places where this convention is used.  I get what you're saying now and
I agree that it needs to be documented thoroughly.  An issue should be opened up
for this.


>
https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interfac...
> lily/side-position-interface.cc:538: if (Skyline_pair
> *inside_staff_skylines = Skyline_pair::unsmob (iss))
> After this line, GUILE no longer sees iss as something it needs to
> protect from garbage collection.
> 

I've just recently learned from your reviews that this is a bad idea.  It is
currently present in 20-ish places in the code base, so it'd be worth it in a
separate patch to eliminate all of them. For the purpose of this patch, I have
eliminated only this one plus others I have run across while reading your
review.  An issue can be opened for the rest.

>
https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interfac...
> lily/side-position-interface.cc:539: skylines = Skyline_pair
> (*inside_staff_skylines);
> Now if Skyline_pair contains any elements allocated by GUILE
> (non-immediate Scheme values), it will ask GUILE for new elements, and
> GUILE might allocate them from *inside_staff_skylines data.  It turns
> out from looking through the headers that Skyline_pair is free from SCM
> data.  But doing that kind of research every time one reads the code is
> costly, so it is easier if you just stick with the SCM value until you
> have created your copy or finished with the whole thing.
> 
> unsmob it for every use until then, and GUILE will let it stick around
> until the last unsmob.  Or put an scm_remember_upto_here_1 on the SCM
> value after the last use of an unsmobbed pointer from it.

I'm not sure what you mean here.  Could you present a C++ code example of
exactly what would be need to added here in order for it to reflect what you're
saying?

> 
>
https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interfac...
> lily/side-position-interface.cc:565: Skyline_pair *v_orig =
> Skyline_pair::unsmob (elt->get_property ("vertical-skylines"));
> Same here.
> 

Here I understand exactly what to do - I'm getting the SCM object first and then
unsmobbing.  But in the case above, that's already what's happening, so that's
why I need a C++ example of exactly what to do.

>
https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interfac...
> lily/side-position-interface.cc:593: Skyline_pair v_skylines (*v_orig);
> Too much distance to above, and several calls of get_property,
> potentially allocating memory, in between.  I explained all this in the
> previous review.  Please generalize one explanation to all occurences of
> the same problem.  If the problem is not clear to you, ask questions
> until it is.  Reviews (and things like valgrind) are a safety net, not a
> trampoline.
> 

There was a mistake here - I accidentally named two different variables the same
thing.  Fixed, so this is no longer v_orig.

The general problem is that I want to make a C++ copy of unsmobbed objects so
that I can manipulate the copy without the original changing.  So,

SCM foo_scm = me->get_property ("foo"); // gets the property
Skyline_pair *foo = Skyline_pair::unsmob (foo); // unsmobs it
Skyline_pair foo_copy (*foo); // makes a copy of foo that will be changed so
that the original skyline is not changed.

Is the above the best way to do this?  If not, what is?

Many thanks for the review - it was very helpful!

Cheers,
MS

[MikeSol, 2013-02-15 06:37:34]

Responses to David's review.

[Keith, 2013-02-16 21:33:55]

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-bra...
File input/regression/tuplet-bracket-outside-staff-priority.ly (right):

https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-bra...
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.c...
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.c...
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.c...
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.c...
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-interfac...
File lily/side-position-interface.cc (right):

https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interfac...
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-interfac...
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-interfac...
lily/side-position-interface.cc:556: // skip inside staff elts
Maybe not 'skip' but "ensure inside staff elements are placed"

[dak, 2013-02-18 18:07:38]

On 2013/02/15 06:37:20, mike7 wrote:

>
https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-num...
> > input/regression/tuplet-number-outside-staff-positioning.ly:15:
> > \override TupletBracket.Y-offset =
> > #ly:side-position-interface::calc-outside-staff-y-offset
> > This is indicating a fundamental design problem: this override is not
> > related to the topic of the regtest.  If it is necessary for getting the
> > desired result, it will be necessary in a whole _lot_ of user-created
> > situations.  But it is not an easily user-accessibly override, and it is
> > not documented in normal user documentation.
> > 
> > This suspiciously looks like tampering with unrelated regtests in order
> > to mask fundamental problems from review.  Can you explain why this is
> > not the case?
> 
> Your question gets to the core of the logic of the patch, so I'll
> explain it and then people can comment upon how that links up with
> this regtest.
> 
> In LilyPond, about 85% of grob properties are set as the result of the
> evaluation of a callback or the use of a rote value.

[...]

Mike, that's a whole bunch of smoke screen.  The fact is that your
change, which purports to be just some "factoring" of stuff by its
description, breaks existing and documented functionality.

And you offer no reason for that.

> Now, LilyPond, for various callbacks, other properties must be set
> for those callbacks to make sense.  For example, if I do:
> 
> \override NoteHead #'stencil = #ly:text-interface::print
> 
> Nothing will happen.  But if I do:
> 
> \override NoteHead #'text = #"foo"
> \override NoteHead #'stencil = #ly:text-interface::print
> 
> The NoteHead will be printed as foo.  This is exactly what we're
> seeing in the regtest tuplet-number-outside-staff-positioning.ly.

No, it is not.

> A callback is set for Y-offset that allows the grob to be positioned
> outside the staff.  But, just as the text interface needs to know
> what text to print, the side-position-interface needs to know what
> outside-staff-priority to use to place the grob.  Thus the use of
> two overrides instead of one.

You got your logic backwards.  The _preexisting_ override in the
regtest overrides outside-staff-priority.  This is a documented way of
changing the default order of outside-staff elements.  There are 17
grobs with a preassigned outside-staff-priority.  There are 369
occurences of outside-staff-priority in the LilyPond code base.

You break that.  You use callbacks that ignore outside-staff-priority
and thus break existing functionality.  And then you revert to the
previous callbacks in the regtests in order to mask that you are
breaking functionality.

You can't just throw functionality overboard when you are "improving"
things and tell people they have to revert to the old code if they
care about that functionality.  After all, it is totally unclear how
elements with the old callbacks and elements with the new
outside-staff-priority ignoring callbacks will even combine.

> A music function could be done in the form of:
> 
> \addOutsideStaffPriorityToGrob #'TupletBracket #100
> 
> that rolls the two overrides into one.  This is a UI issue about
> which I have not thought yet but that absolutely deserves attention.

Then give it attention.  Heed outside-staff-priority properly in your
versions of the callbacks.  Until this is done, this patch is not
ready for maintime.

This is a showstopper.

And messing with the regtests in order to hide this is not a proper
way of addressing this showstopper.

[mike7, 2013-02-19 12:37:57]

On 18 févr. 2013, at 20:07, dak@gnu.org wrote:

> On 2013/02/15 06:37:20, mike7 wrote:
> 
> 
>
https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-num...
>> > input/regression/tuplet-number-outside-staff-positioning.ly:15:
>> > \override TupletBracket.Y-offset =
>> > #ly:side-position-interface::calc-outside-staff-y-offset
>> > This is indicating a fundamental design problem: this override is
> not
>> > related to the topic of the regtest.  If it is necessary for getting
> the
>> > desired result, it will be necessary in a whole _lot_ of
> user-created
>> > situations.  But it is not an easily user-accessibly override, and
> it is
>> > not documented in normal user documentation.
>> >
>> > This suspiciously looks like tampering with unrelated regtests in
> order
>> > to mask fundamental problems from review.  Can you explain why this
> is
>> > not the case?
> 
>> Your question gets to the core of the logic of the patch, so I'll
>> explain it and then people can comment upon how that links up with
>> this regtest.
> 
>> In LilyPond, about 85% of grob properties are set as the result of the
>> evaluation of a callback or the use of a rote value.
> 
> [...]
> 
> Mike, that's a whole bunch of smoke screen.  The fact is that your
> change, which purports to be just some "factoring" of stuff by its
> description, breaks existing and documented functionality.
> 
> And you offer no reason for that.

Currently, the previous functionality that breaks is that setting
outside-staff-priority by itself no longer has an effect, it must be accompanied
by a function that uses this to compute Y-offset, which is now in the
side-position-interface.

The reason is best summarized by Keith, which I've copied and pasted below:

<snip>
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.
</snip>

Please let me know what other information you (or anyone) needs to understand
what is going on.  It is an important change, and I want to make sure it is
properly discussed on this list, documented, commented and well written before
it is pushed.

> 
>> Now, LilyPond, for various callbacks, other properties must be set
>> for those callbacks to make sense.  For example, if I do:
> 
>> \override NoteHead #'stencil = #ly:text-interface::print
> 
>> Nothing will happen.  But if I do:
> 
>> \override NoteHead #'text = #"foo"
>> \override NoteHead #'stencil = #ly:text-interface::print
> 
>> The NoteHead will be printed as foo.  This is exactly what we're
>> seeing in the regtest tuplet-number-outside-staff-positioning.ly.
> 
> No, it is not.

Could you elaborate?

> 
>> A callback is set for Y-offset that allows the grob to be positioned
>> outside the staff.  But, just as the text interface needs to know
>> what text to print, the side-position-interface needs to know what
>> outside-staff-priority to use to place the grob.  Thus the use of
>> two overrides instead of one.
> 
> You got your logic backwards.  The _preexisting_ override in the
> regtest overrides outside-staff-priority.  This is a documented way of
> changing the default order of outside-staff elements.  There are 17
> grobs with a preassigned outside-staff-priority.  There are 369
> occurences of outside-staff-priority in the LilyPond code base.
> 
> You break that.  You use callbacks that ignore outside-staff-priority
> and thus break existing functionality.  And then you revert to the
> previous callbacks in the regtests in order to mask that you are
> breaking functionality.
> 
> You can't just throw functionality overboard when you are "improving"
> things and tell people they have to revert to the old code if they
> care about that functionality.  After all, it is totally unclear how
> elements with the old callbacks and elements with the new
> outside-staff-priority ignoring callbacks will even combine.

 It is true that this breaks old functionality for user overrides.

The goal is certainly not to mask things.  I will make sure to put this in the
change log and will write a not-smart convert-ly rule in my next patch set.

Cheers,
MS

[mike7, 2013-02-19 12:37:58]

On 18 févr. 2013, at 20:07, dak@gnu.org wrote:

> 
> 
>> A music function could be done in the form of:
> 
>> \addOutsideStaffPriorityToGrob #'TupletBracket #100
> 
>> that rolls the two overrides into one.  This is a UI issue about
>> which I have not thought yet but that absolutely deserves attention.
> 
> Then give it attention.  Heed outside-staff-priority properly in your
> versions of the callbacks.  Until this is done, this patch is not
> ready for maintime.
> 
> This is a showstopper.

I've been stepping through grob by grob to see the way that
outside-staff-priority behaves with this new patch-set.

Take, for example, MultiMeasureRest.  In current master, it is possible to give
this an outside-staff-priority and it will be shifted above the staff.  However,
with my patch, this would break.  I'm going to keep this patch on hold until I
finish a series of other changes that make it easier to deal with pure
properties.

Note that, after my patch set, outside-staff-priority will be factored into pure
heights.  This will make behavior in LilyPond more consistent.  For example, in
current master:

\relative c'' {
  \once \override Rest #'outside-staff-priority = #10
  r2
  <aes bes ces>
}

does nothing.  Even worse:

\relative c''' {
  \once \override MultiMeasureRest #'outside-staff-priority = #10
  R1
  <feses geses aeses beses ceses>1
}

This patch will eventually fix things like this.

Cheers,
MS

[t.daniels_treda.co.uk, 2013-02-19 14:11:48]

From: <mike@mikesolomon.org>
Sent: Monday, February 18, 2013 8:32 PM

> On 18 févr. 2013, at 20:07, dak@gnu.org wrote:
>
>> You can't just throw functionality overboard when you are "improving"
>> things and tell people they have to revert to the old code if they
>> care about that functionality.  After all, it is totally unclear how
>> elements with the old callbacks and elements with the new
>> outside-staff-priority ignoring callbacks will even combine.
>
> It is true that this breaks old functionality for user overrides.
>
> The goal is certainly not to mask things.  I will make sure to put this in 
> the change log and will write a not-smart convert-ly rule in my next patch
set.

That's not good enough.  The present overrides of outside-staff-priority
figure quite extensively in the user documentation.  This will all need to be
re-written as part of an acceptable patch to show users how to rearrange
the order in which outside-staff objects are placed.

Trevor
 

[mike7, 2013-02-19 14:57:48]

On 19 févr. 2013, at 16:11, Trevor Daniels <t.daniels@treda.co.uk> wrote:

> 
> From: <mike@mikesolomon.org>
> Sent: Monday, February 18, 2013 8:32 PM
> 
>> On 18 févr. 2013, at 20:07, dak@gnu.org wrote:
>> 
>>> You can't just throw functionality overboard when you are "improving"
>>> things and tell people they have to revert to the old code if they
>>> care about that functionality.  After all, it is totally unclear how
>>> elements with the old callbacks and elements with the new
>>> outside-staff-priority ignoring callbacks will even combine.
>> 
>> It is true that this breaks old functionality for user overrides.
>> 
>> The goal is certainly not to mask things.  I will make sure to put this in 
>> the change log and will write a not-smart convert-ly rule in my next patch
set.
> 
> That's not good enough.  The present overrides of outside-staff-priority
> figure quite extensively in the user documentation.  This will all need to be
> re-written as part of an acceptable patch to show users how to rearrange
> the order in which outside-staff objects are placed.
> 
> Trevor

Hey Trevor,

Thanks for giving me input from a documentation point of view.  When writing the
patch, this was one of the fundamental things I took into account.  All of the
examples in the notation reference, learning manual and snippets show no change
with this patch applied.

The current documentation on outside-staff grobs reads: "Intuitively, there are
some objects in musical notation that belong to the staff and there are other
objects that should be placed outside the staff. Objects belonging outside the
staff include things such as rehearsal marks, text and dynamic markings (from
now on, these will be called outside-staff objects). LilyPond’s rule for the
vertical placement of outside-staff objects is to place them as close to the
staff as possible but not so close that they collide with another object."

If we accept this definition of outside-staff grobs, then this patch correctly
typesets all outside staff grobs.  The main thing this patch effects are
esoteric grobs that normally don't get outside-staff-priority, like
MultiMeasureRest, NoteHeads and Rests.

In light of this, what type of additions to the NR do you feel would be
necessary to indicate the new behavior?

Cheers,
MS

[t.daniels_treda.co.uk, 2013-02-19 16:25:54]

From: <mike@mikesolomon.org>
Sent: Tuesday, February 19, 2013 2:22 PM
>

>On 19 févr. 2013, at 16:11, Trevor Daniels <t.daniels@treda.co.uk> wrote:
>> 
>> From: <mike@mikesolomon.org>
>> Sent: Monday, February 18, 2013 8:32 PM
>> 
>>> On 18 févr. 2013, at 20:07, dak@gnu.org wrote:
>>> 
>>>> You can't just throw functionality overboard when you are "improving"
>>>> things and tell people they have to revert to the old code if they
>>>> care about that functionality.  After all, it is totally unclear how
>>>> elements with the old callbacks and elements with the new
>>>> outside-staff-priority ignoring callbacks will even combine.
>>> 
>>> It is true that this breaks old functionality for user overrides.
>>> 
>>> The goal is certainly not to mask things.  I will make sure to put this in 
>>> the change log and will write a not-smart convert-ly rule in my next patch
set.
>> 
>> That's not good enough.  The present overrides of outside-staff-priority
>> figure quite extensively in the user documentation.  This will all need to
be
>> re-written as part of an acceptable patch to show users how to rearrange
>> the order in which outside-staff objects are placed.
>> 
> Thanks for giving me input from a documentation point of view.  When 
> writing the patch, this was one of the fundamental things I took into
> account.  All of the examples in the notation reference, learning manual and 
> snippets show no change with this patch applied.

OK, Mike, this is reassuring.  Seems like I read too much into
"It is true that this breaks old functionality for user overrides."

> The main thing this patch effects are esoteric grobs that normally don't get 
> outside-staff-priority, like MultiMeasureRest, NoteHeads and Rests.

One of these that is commonly given an outside-staff-priority is Slur.  This
is used as an example in the LM.

> In light of this, what type of additions to the NR do you feel would be
necessary 
> to indicate the new behavior?

Probably nothing the the NR (although I haven't read it carefully to check)
but have a look at this section in the LM, particularly the bit on Slurs, to see
if
the text, as well as the examples, would remain correct.

http://www.lilypond.org/doc/v2.17/Documentation/learning/outside_002dstaff-ob...

Trevor

[dak, 2013-02-19 17:27:31]

On 2013/02/19 14:57:48, mike7 wrote:

> The current documentation on outside-staff grobs reads:
> "Intuitively, there are some objects in musical notation that belong
> to the staff and there are other objects that should be placed
> outside the staff. Objects belonging outside the staff include
> things such as rehearsal marks, text and dynamic markings (from now
> on, these will be called outside-staff objects). LilyPond’s rule for
> the vertical placement of outside-staff objects is to place them as
> close to the staff as possible but not so close that they collide
> with another object."

> If we accept this definition of outside-staff grobs, then this patch
> correctly typesets all outside staff grobs.

"such as" is not a definition.

> The main thing this patch effects are esoteric grobs that normally
> don't get outside-staff-priority, like MultiMeasureRest, NoteHeads
> and Rests.

The tampered regtests affect TupletBracket and TupletNumber.

Your grob definitions change BassFigureLine, DynamicText,
MeasureCounter, System, VerticalAxisGroup, and that's all.

So why are tuplet brackets even affected?

Your issue description is
"Caches the interior skylines of vertical axis groups and systems.

This will allow for a more modular approach to outside-staff-spacing,
eventually eliminating the call to translate_axis in
axis-group-interface.cc."

"Caching" means a technique for avoiding repeated calculations by
storing intermediate results.  Caching does _not_ change results.  So
obviously, the issue description is blatantly misleading.  And there
are no discoverable design comments that would explain that
discrepancy.

The definition of TupletBracket has _not_ changed, yet it has stopped
reacting to outside-staff-priority.  That means that _every_ unchanged
grob will also stop reacting to outside-staff-priority.

You don't document what it takes to _keep_ adhering to
outside-staff-priority.  This was previously something that worked
without extra measures.  What does it take now?

Apparently something like
\override TupletBracket.Y-offset =
#ly:side-position-interface::calc-outside-staff-y-offset

Now why is that not the default provided by the interfaces that have
provided it previously?

> In light of this, what type of additions to the NR do you feel would
> be necessary to indicate the new behavior?

First, clear the situation up for the programmer.  It very much looks
like you are trying to make things hard for the documentation writers
and users because you can't be bothered to add what it takes to,
indeed, make your code only "cache" something without changing the
user-visible behavior.

At the current point of time, the complete mismatch between the
issue/codereview description and the actual content is already enough
to render it unreviewable.

And indeed, I have not properly reviewed it.  I am just poking around
with a stick in the code, and even that turns out enough things that
make it _very_ obvious that this patch is in need of thorough review,
and the provided materials are not sufficient for a thorough review,
and not consistent.

It is taking _enormous_ amounts of time to hunt this kind of stuff
through the dark.  And it taking an enormous drain of energy because
at every point one feels stupid and helpless, and the reason for that
is that the information you are venturing through code and comments is
quite insufficient to get a picture of what you are doing in the first
place, and the issue "description" does not help one bit.  Now
obviously it should not contain anything of relevance for
understanding that is not also present in code comments (after all,
the code has to be read without the issue description), but that does
not mean that the remedy for omitting the design from the code would
be to make a misleading issue description.

I don't have the time and energy to go through the process of what
amounts to disassembling code.  And anybody needing to maintain the
code itself and anybody having to deal with the resulting behavior
will also not have this time.

I _have_ in my younger years disassembled code.  One of the
best-written programs I have had the pleasure to peruse was a Z80
assembly language implementation of Reversi.  In the old times, people
passed around code, and I don't know who was the original author.  But
the code was textbook quality, a very clean and straightforward
implementation of Alpha-Beta pruning, straightforward layout of
scoring tables, consistent use of index registers for certain tasks,
every piece of code a straightforward rendition of a straightforward
task fitting in the overall design, everything as simple and direct as
possible.  It was _rewarding_ to be reading the mind of a master.

These reviews, in contrast, are not rewarding.  I never get the
impression that I can see the mind of the surgeon dissecting a problem
along its principal lines, making it fall into the obvious and
necessary pieces.

And so you need to document and describe what you are doing because
otherwise the whole review procedure is a pure mockery and code gets
"accepted" when everybody has given up understanding it.

[MikeSol, 2013-02-19 17:46:07]

Fixes bug in slurs

[mike7, 2013-02-19 17:46:21]

On 19 févr. 2013, at 19:27, dak@gnu.org wrote:

> You don't document what it takes to _keep_ adhering to
> outside-staff-priority.  This was previously something that worked
> without extra measures.  What does it take now?

I have put things in the change-log regarding this.  Let me know if this is
clear.

Cheers,
MS

[dak, 2013-02-23 18:00:19]

https://codereview.appspot.com/7185044/diff/45018/Documentation/changes.tely
File Documentation/changes.tely (right):

https://codereview.appspot.com/7185044/diff/45018/Documentation/changes.tely#...
Documentation/changes.tely:68:
@code{ly:side-position-interface::calc-outside-staff-y-offse}
Probably a missing t at the end.

https://codereview.appspot.com/7185044/diff/45018/Documentation/changes.tely#...
Documentation/changes.tely:69: or
@code{ly:side-position-interface::y-aligned-side}.  All grobs
Why are the names "calc-outside-staff-y-offset" and "y-aligned-side" (the latter
is totally cryptic to boot) so utterly dissimilar when doing the same kind of
job?  How is one supposed to choose between them when the difference in name is
not related to the difference in function but is completely changed in every
aspect?

[dak, 2013-02-23 18:07:54]

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

https://codereview.appspot.com/7185044/diff/45018/input/regression/tuplet-bra...
input/regression/tuplet-bracket-outside-staff-priority.ly:17:
\tupletOutsideStaffPriority #1
Why a changed regtest when purportedly, according to the changes entry, the
functionality should not be changed?

https://codereview.appspot.com/7185044/diff/45018/input/regression/tuplet-num...
File input/regression/tuplet-number-outside-staff-positioning.ly (right):

https://codereview.appspot.com/7185044/diff/45018/input/regression/tuplet-num...
input/regression/tuplet-number-outside-staff-positioning.ly:14:
\tupletOutsideStaffPriority #1
Same here.

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

https://codereview.appspot.com/7185044/diff/45018/input/regression/tuplet-num...
input/regression/tuplet-number-outside-staff-priority.ly:12:
\tupletOutsideStaffPriority #1
same here.

https://codereview.appspot.com/7185044/diff/45018/ly/music-functions-init.ly
File ly/music-functions-init.ly (right):

https://codereview.appspot.com/7185044/diff/45018/ly/music-functions-init.ly#...
ly/music-functions-init.ly:1376: tupletOutsideStaffPriority =
Does that mean that the statement in changes.tely about tuplets' reaction to
outside-staff-priority being the same as before is false?

This kind of function does not make any sense.  For one thing, it does not serve
a high level function but manipulates a single low-level parameter.  Providing a
music function for this kind of numerical manipulation is decidedly weird.  For
another, it only provides an override method while this parameter will often be
in need of tweaking.

[Trevor Daniels, 2013-02-23 22:24:21]

https://codereview.appspot.com/7185044/diff/45018/Documentation/changes.tely
File Documentation/changes.tely (right):

https://codereview.appspot.com/7185044/diff/45018/Documentation/changes.tely#...
Documentation/changes.tely:73: @code{Slur}, for example, are all grobs are
traditionally not outside
... grobs that are ...

[Keith, 2013-02-24 09:45:54]

This still needs some cleanup, at least to remove the now-redundant
\tupletOutsideStaffPriority

The goal was to have outside-staff grobs figure their own Y-offset, including
the offset required for the outside-staff grobs to avoid each other.  The group
of all objects on a line (VerticalAxisGroup) still does most of the
organizational work, but any outside-staff grob figures its own position based
on that organizational work.

How will this help positioning around cross-staff objects?  Presumably you plan
to reset the 'Y-offset property of some Grobs after page-spacing, so that the
callback function is called again.  Which Grobs get the reset ?  If it is all
the Grobs touched by a cross-staff object, then maybe it is better to keep the
job of arranging outside-staff objects with the VerticalAxisGroup and reset a
property of that one object.

https://codereview.appspot.com/7185044/diff/45018/Documentation/changes.tely
File Documentation/changes.tely (right):

https://codereview.appspot.com/7185044/diff/45018/Documentation/changes.tely#...
Documentation/changes.tely:76: 
How does Fingering work, without any 'calc-outside-staff-y-offset' ? 
{ \override Staff.Fingering.outside-staff-priority = #1100
  \tempo "Adagio" g'-3}

Maybe: "The property 'outside-staff-priority' that determines relative placement
of objects outside the staff is now implemented by 'side-position-interface'. 
All built-in objects that formerly responded to this option still do so, but
user-created grobs will need to include the 'side-position-interface' to get the
effect of 'outside-staff-priority'."

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

https://codereview.appspot.com/7185044/diff/45018/lily/axis-group-interface.c...
lily/axis-group-interface.cc:886: multimap<Grob *, Grob *> riders;
Yipee!! no more riders!
I dance on the grave of the mulitmap.

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

https://codereview.appspot.com/7185044/diff/45018/lily/side-position-interfac...
lily/side-position-interface.cc:694: "padding "
If you are serious about this re-arrangement you should move the
"outside-staff-priority " from grob.cc to here

[dak, 2013-02-24 10:40:48]

Stupid question: could you not just check whether outside-staff-priority has
been set, and if it has, pass the buck to the right kind of callback
automatically?  That way, the user does not need to meddle with callbacks
himself.

[mike7, 2013-02-24 13:27:39]

On 24 févr. 2013, at 12:40, dak@gnu.org wrote:

> Stupid question: could you not just check whether outside-staff-priority
> has been set, and if it has, pass the buck to the right kind of callback
> automatically?  That way, the user does not need to meddle with
> callbacks himself.
> 
> https://codereview.appspot.com/7185044/

Yup, but it'd require creating an Outside_staff_callback engraver with a call to
acknowledge_grob.  Calls to acknowledge_grob are expensive and should be used
only if there's no other viable solution.

Cheers,
MS

[dak, 2013-02-24 14:37:14]

On 2013/02/24 13:27:39, mike7 wrote:
> On 24 févr. 2013, at 12:40, mailto:dak@gnu.org wrote:
>
> > Stupid question: could you not just check whether
> > outside-staff-priority has been set, and if it has, pass the buck
> > to the right kind of callback automatically?  That way, the user
> > does not need to meddle with callbacks himself.
> > 
> > https://codereview.appspot.com/7185044/
> 
> Yup, but it'd require creating an Outside_staff_callback engraver
> with a call to acknowledge_grob.  Calls to acknowledge_grob are
> expensive and should be used only if there's no other viable
> solution.

Why can't the current Y-offset callback check outside-staff-priority
on the grob?  Why would there be a need for an acknowledger?

[MikeSol, 2013-02-24 14:56:06]

Uses Keith

[mike7, 2013-02-24 14:57:37]

On 24 févr. 2013, at 16:37, dak@gnu.org wrote:

> On 2013/02/24 13:27:39, mike7 wrote:
>> On 24 févr. 2013, at 12:40, mailto:dak@gnu.org wrote:
> 
>> > Stupid question: could you not just check whether
>> > outside-staff-priority has been set, and if it has, pass the buck
>> > to the right kind of callback automatically?  That way, the user
>> > does not need to meddle with callbacks himself.
>> >
>> > https://codereview.appspot.com/7185044/
> 
>> Yup, but it'd require creating an Outside_staff_callback engraver
>> with a call to acknowledge_grob.  Calls to acknowledge_grob are
>> expensive and should be used only if there's no other viable
>> solution.
> 
> Why can't the current Y-offset callback check outside-staff-priority
> on the grob?  Why would there be a need for an acknowledger?
> 

That's exactly how the new Y-offset callbacks are implemented in this patch set.
 The problem is that not all grobs have a Y-offset callback.

Cheers,
MS

[dak, 2013-02-24 15:18:15]

"mike@mikesolomon.org" <mike@mikesolomon.org> writes:

> On 24 févr. 2013, at 16:37, dak@gnu.org wrote:
>
>> On 2013/02/24 13:27:39, mike7 wrote:
>>> On 24 févr. 2013, at 12:40, mailto:dak@gnu.org wrote:
>> 
>>> > Stupid question: could you not just check whether
>>> > outside-staff-priority has been set, and if it has, pass the buck
>>> > to the right kind of callback automatically?  That way, the user
>>> > does not need to meddle with callbacks himself.
>>> >
>>> > https://codereview.appspot.com/7185044/
>> 
>>> Yup, but it'd require creating an Outside_staff_callback engraver
>>> with a call to acknowledge_grob.  Calls to acknowledge_grob are
>>> expensive and should be used only if there's no other viable
>>> solution.
>> 
>> Why can't the current Y-offset callback check outside-staff-priority
>> on the grob?  Why would there be a need for an acknowledger?
>> 
>
> That's exactly how the new Y-offset callbacks are implemented in this
> patch set.  The problem is that not all grobs have a Y-offset
> callback.

Well, something heeded outside-staff-priority previously, apparently
calculating an offset based on it.  What happened to it?

-- 
David Kastrup

[mike7, 2013-02-24 16:20:53]

On 24 févr. 2013, at 17:18, David Kastrup <dak@gnu.org> wrote:

> "mike@mikesolomon.org" <mike@mikesolomon.org> writes:
> 
>> On 24 févr. 2013, at 16:37, dak@gnu.org wrote:
>> 
>>> On 2013/02/24 13:27:39, mike7 wrote:
>>>> On 24 févr. 2013, at 12:40, mailto:dak@gnu.org wrote:
>>> 
>>>>> Stupid question: could you not just check whether
>>>>> outside-staff-priority has been set, and if it has, pass the buck
>>>>> to the right kind of callback automatically?  That way, the user
>>>>> does not need to meddle with callbacks himself.
>>>>> 
>>>>> https://codereview.appspot.com/7185044/
>>> 
>>>> Yup, but it'd require creating an Outside_staff_callback engraver
>>>> with a call to acknowledge_grob.  Calls to acknowledge_grob are
>>>> expensive and should be used only if there's no other viable
>>>> solution.
>>> 
>>> Why can't the current Y-offset callback check outside-staff-priority
>>> on the grob?  Why would there be a need for an acknowledger?
>>> 
>> 
>> That's exactly how the new Y-offset callbacks are implemented in this
>> patch set.  The problem is that not all grobs have a Y-offset
>> callback.
> 
> Well, something heeded outside-staff-priority previously, apparently
> calculating an offset based on it.  What happened to it?
> 

Before, there was a call to translate_axis.  It wasn't done as a callback for a
Y-offset property but rather a vertical-skylines property for VerticalAxisGroup
that called translate_axis many times (follow, in current master,
Axis_group_interface::calc_vertical_skylines to avoid_outside_staff_collisions
and you'll see how this is done).  This makes it impossible to wipe caches and
recompute properties with more information.  The goal of this patch is to
eliminate that call to translate_axis so that the callback(s) for Y-offset
calculate the entire offset, which means we can cache and recache pure
equivalents.

I think the current patch is fine.  Any behavior that this patch changes (for
example, setting an outside-staff-priority on MultiMeasureRest) is undocumented
and was likely never intended when outside-staff-priority was created in the
first place.  The original language in the documentation only states that
outside-staff-priority is relevant for grobs that are always (Measure Marks) or
sometimes (Slurs) placed outside the staff.  All of these cases are covered by
the patch.

Cheers,
MS

[mike7, 2013-02-24 17:20:23]

On 24 févr. 2013, at 11:45, k-ohara5a5a@oco.net wrote:

> This still needs some cleanup, at least to remove the now-redundant
> \tupletOutsideStaffPriority
> 

It's not redundant just cuz if it's not set, LilyPond will now issue a warning. 
If you want the regtest to have a warning, we can convert it to the old syntax
and then let the regtest test the warning.  This wouldn't be a bad idea, as we
want to make sure LilyPond warns the user if outside-staff-priority
discrepancies exist between parent and children grobs.

> The goal was to have outside-staff grobs figure their own Y-offset,
> including the offset required for the outside-staff grobs to avoid each
> other.  The group of all objects on a line (VerticalAxisGroup) still
> does most of the organizational work, but any outside-staff grob figures
> its own position based on that organizational work.
> 

Yup.

> How will this help positioning around cross-staff objects?  Presumably
> you plan to reset the 'Y-offset property of some Grobs after
> page-spacing, so that the callback function is called again.

Yup.

> Which
> Grobs get the reset ?  If it is all the Grobs touched by a cross-staff
> object, then maybe it is better to keep the job of arranging
> outside-staff objects with the VerticalAxisGroup and reset a property of
> that one object.

I'm not there yet, but this is definitely a possibility.  The next big project
is this.

> 
> 
> https://codereview.appspot.com/7185044/diff/45018/Documentation/changes.tely
> File Documentation/changes.tely (right):
> 
>
https://codereview.appspot.com/7185044/diff/45018/Documentation/changes.tely#...
> Documentation/changes.tely:76:
> How does Fingering work, without any 'calc-outside-staff-y-offset' ?
> { \override Staff.Fingering.outside-staff-priority = #1100
>  \tempo "Adagio" g'-3}

Still works because the ly:side-position-interface::y-aligned-side automatically
checks for outside-staff-priority at the end.

> 
> Maybe: "The property 'outside-staff-priority' that determines relative
> placement of objects outside the staff is now implemented by
> 'side-position-interface'.  All built-in objects that formerly responded
> to this option still do so, but user-created grobs will need to include
> the 'side-position-interface' to get the effect of
> 'outside-staff-priority'."
> 
>
https://codereview.appspot.com/7185044/diff/45018/lily/axis-group-interface.cc
> File lily/axis-group-interface.cc (left):
> 
>
https://codereview.appspot.com/7185044/diff/45018/lily/axis-group-interface.c...
> lily/axis-group-interface.cc:886: multimap<Grob *, Grob *> riders;
> Yipee!! no more riders!
> I dance on the grave of the mulitmap.

Funeral services will be next week.

> 
>
https://codereview.appspot.com/7185044/diff/45018/lily/side-position-interfac...
> File lily/side-position-interface.cc (right):
> 
>
https://codereview.appspot.com/7185044/diff/45018/lily/side-position-interfac...
> lily/side-position-interface.cc:694: "padding "
> If you are serious about this re-arrangement you should move the
> "outside-staff-priority " from grob.cc to here

True 'dat.

[MikeSol, 2013-02-24 21:03:54]

Adds side-position-interface to certain grobs

[dak, 2013-02-25 15:29:22]

On 2013/02/24 16:20:53, mike7 wrote:
> On 24 févr. 2013, at 17:18, David Kastrup <mailto:dak@gnu.org> wrote:
> 
> > "mike@mikesolomon.org" <mailto:mike@mikesolomon.org> writes:
> > 
> >> On 24 févr. 2013, at 16:37, mailto:dak@gnu.org wrote:
> >> 
> >>> On 2013/02/24 13:27:39, mike7 wrote:
> >>>> On 24 févr. 2013, at 12:40, mailto:dak@gnu.org wrote:
> >>> 
> >>>>> Stupid question: could you not just check whether
> >>>>> outside-staff-priority has been set, and if it has, pass the buck
> >>>>> to the right kind of callback automatically?  That way, the user
> >>>>> does not need to meddle with callbacks himself.
> >>> 
> >>>> Yup, but it'd require creating an Outside_staff_callback engraver
> >>>> with a call to acknowledge_grob.  Calls to acknowledge_grob are
> >>>> expensive and should be used only if there's no other viable
> >>>> solution.
> >>> 
> >>> Why can't the current Y-offset callback check outside-staff-priority
> >>> on the grob?  Why would there be a need for an acknowledger?
> >> 
> >> That's exactly how the new Y-offset callbacks are implemented in this
> >> patch set.  The problem is that not all grobs have a Y-offset
> >> callback.
> > 
> > Well, something heeded outside-staff-priority previously, apparently
> > calculating an offset based on it.  What happened to it?
> 
> Before, there was a call to translate_axis.  It wasn't done as a
> callback for a Y-offset property but rather a vertical-skylines
> property for VerticalAxisGroup that called translate_axis many times
> (follow, in current master,
> Axis_group_interface::calc_vertical_skylines to
> avoid_outside_staff_collisions and you'll see how this is done).
> This makes it impossible to wipe caches and recompute properties
> with more information.  The goal of this patch is to eliminate that
> call to translate_axis so that the callback(s) for Y-offset
> calculate the entire offset, which means we can cache and recache
> pure equivalents.

This is really like pulling teeth.  Something _obviously_ consults
Y-offset, or overriding it with a callback would not have an effect.
Now said something can also check Y-offset for being unset (presumably
the default) and outside-staff-priority for being set (the
non-default) and call up the most likely of the totally dissimilarly
named callbacks you mention in changes.tely without bothering to say
which one to choose under what circumstances.

Can you _merge_ those callbacks?  Then there is no need to pick one.

Setting something like outside-staff-priority is still stuff that
users can comprehend (and that's actually documented in the manuals).
Having to pick one of several totally differently named callbacks
without useful documentation is _not_ a user interface.

That's internals.

> I think the current patch is fine.

It does not have a usable user interface.  It is not an option for a
user to choose between several undocumented callbacks.

[mike7, 2013-02-25 22:34:53]

On 25 févr. 2013, at 16:29, dak@gnu.org wrote:

> On 2013/02/24 16:20:53, mike7 wrote:
>> On 24 févr. 2013, at 17:18, David Kastrup <mailto:dak@gnu.org> wrote:
> 
>> > "mike@mikesolomon.org" <mailto:mike@mikesolomon.org> writes:
>> >
>> >> On 24 févr. 2013, at 16:37, mailto:dak@gnu.org wrote:
>> >>
>> >>> On 2013/02/24 13:27:39, mike7 wrote:
>> >>>> On 24 févr. 2013, at 12:40, mailto:dak@gnu.org wrote:
>> >>>
>> >>>>> Stupid question: could you not just check whether
>> >>>>> outside-staff-priority has been set, and if it has, pass the
> buck
>> >>>>> to the right kind of callback automatically?  That way, the user
>> >>>>> does not need to meddle with callbacks himself.
>> >>>
>> >>>> Yup, but it'd require creating an Outside_staff_callback engraver
>> >>>> with a call to acknowledge_grob.  Calls to acknowledge_grob are
>> >>>> expensive and should be used only if there's no other viable
>> >>>> solution.
>> >>>
>> >>> Why can't the current Y-offset callback check
> outside-staff-priority
>> >>> on the grob?  Why would there be a need for an acknowledger?
>> >>
>> >> That's exactly how the new Y-offset callbacks are implemented in
> this
>> >> patch set.  The problem is that not all grobs have a Y-offset
>> >> callback.
>> >
>> > Well, something heeded outside-staff-priority previously, apparently
>> > calculating an offset based on it.  What happened to it?
> 
>> Before, there was a call to translate_axis.  It wasn't done as a
>> callback for a Y-offset property but rather a vertical-skylines
>> property for VerticalAxisGroup that called translate_axis many times
>> (follow, in current master,
>> Axis_group_interface::calc_vertical_skylines to
>> avoid_outside_staff_collisions and you'll see how this is done).
>> This makes it impossible to wipe caches and recompute properties
>> with more information.  The goal of this patch is to eliminate that
>> call to translate_axis so that the callback(s) for Y-offset
>> calculate the entire offset, which means we can cache and recache
>> pure equivalents.
> 
> This is really like pulling teeth.  Something _obviously_ consults
> Y-offset, or overriding it with a callback would not have an effect.
> Now said something can also check Y-offset for being unset (presumably
> the default) and outside-staff-priority for being set (the
> non-default) and call up the most likely of the totally dissimilarly
> named callbacks you mention in changes.tely without bothering to say
> which one to choose under what circumstances.
> 
> Can you _merge_ those callbacks?  Then there is no need to pick one.
> 

They are currently merged in that y-aligned-side automatically calls
calc-outside-staff-y-offset at the end.  I forget why I didn't merge them
completely - there was a reason at one point, but they may be mergeable.  Will
look into it.

> Setting something like outside-staff-priority is still stuff that
> users can comprehend (and that's actually documented in the manuals).
> Having to pick one of several totally differently named callbacks
> without useful documentation is _not_ a user interface.
> 
> That's internals.

The goal of the patchset is for all outside-staff grobs to still respond to
outside-staff-priority.  Are there any outside-staff grobs (Slur, Script,
TupletBracket, whatever) that currently don't function like they used to in the
given patch set?  I didn't find a single one.

> 
>> I think the current patch is fine.
> 
> It does not have a usable user interface.  It is not an option for a
> user to choose between several undocumented callbacks.
> 

I will document those two callbacks or the one callback if I merge them.  What
I'd appreciate is a list of outside-staff grobs as defined in the documentation
that, with my patch set apply, no longer respond to outside-staff-priority.  I
believe that I've covered all the pertinent grobs.

Cheers,
MS

[mike7, 2013-02-25 23:11:22]

On 25 févr. 2013, at 23:34, mike@mikesolomon.org wrote:

> 
> On 25 févr. 2013, at 16:29, dak@gnu.org wrote:
> 
>> On 2013/02/24 16:20:53, mike7 wrote:
>>> On 24 févr. 2013, at 17:18, David Kastrup <mailto:dak@gnu.org> wrote:
>> 
>>>> "mike@mikesolomon.org" <mailto:mike@mikesolomon.org> writes:
>>>> 
>>>>> On 24 févr. 2013, at 16:37, mailto:dak@gnu.org wrote:
>>>>> 
>>>>>> On 2013/02/24 13:27:39, mike7 wrote:
>>>>>>> On 24 févr. 2013, at 12:40, mailto:dak@gnu.org wrote:
>>>>>> 
>>>>>>>> Stupid question: could you not just check whether
>>>>>>>> outside-staff-priority has been set, and if it has, pass the
>> buck
>>>>>>>> to the right kind of callback automatically?  That way, the user
>>>>>>>> does not need to meddle with callbacks himself.
>>>>>> 
>>>>>>> Yup, but it'd require creating an Outside_staff_callback engraver
>>>>>>> with a call to acknowledge_grob.  Calls to acknowledge_grob are
>>>>>>> expensive and should be used only if there's no other viable
>>>>>>> solution.
>>>>>> 
>>>>>> Why can't the current Y-offset callback check
>> outside-staff-priority
>>>>>> on the grob?  Why would there be a need for an acknowledger?
>>>>> 
>>>>> That's exactly how the new Y-offset callbacks are implemented in
>> this
>>>>> patch set.  The problem is that not all grobs have a Y-offset
>>>>> callback.
>>>> 
>>>> Well, something heeded outside-staff-priority previously, apparently
>>>> calculating an offset based on it.  What happened to it?
>> 
>>> Before, there was a call to translate_axis.  It wasn't done as a
>>> callback for a Y-offset property but rather a vertical-skylines
>>> property for VerticalAxisGroup that called translate_axis many times
>>> (follow, in current master,
>>> Axis_group_interface::calc_vertical_skylines to
>>> avoid_outside_staff_collisions and you'll see how this is done).
>>> This makes it impossible to wipe caches and recompute properties
>>> with more information.  The goal of this patch is to eliminate that
>>> call to translate_axis so that the callback(s) for Y-offset
>>> calculate the entire offset, which means we can cache and recache
>>> pure equivalents.
>> 
>> This is really like pulling teeth.  Something _obviously_ consults
>> Y-offset, or overriding it with a callback would not have an effect.
>> Now said something can also check Y-offset for being unset (presumably
>> the default) and outside-staff-priority for being set (the
>> non-default) and call up the most likely of the totally dissimilarly
>> named callbacks you mention in changes.tely without bothering to say
>> which one to choose under what circumstances.
>> 
>> Can you _merge_ those callbacks?  Then there is no need to pick one.
>> 
> 
> They are currently merged in that y-aligned-side automatically calls
calc-outside-staff-y-offset at the end.  I forget why I didn't merge them
completely - there was a reason at one point, but they may be mergeable.  Will
look into it.

I see now why I didn't merge these two functions.

The tuplet-bracket-interface currently implements a property called 'padding
that would be read by helper functions called via
ly:side-position-interface::y-aligned-side.  We don't want this, so we write a
second function that circumvents this and only does outside staff positioning.

Cheers,
MS

[MikeSol, 2013-02-26 18:49:42]

Removes Side_position_interface::calc_outside_staff_y_offset

[MikeSol, 2013-02-27 10:30:51]

Ignores staff-padding for cross-staff grobs

[MikeSol, 2013-03-05 04:48:03]

rebase off current master

[MikeSol, 2013-03-13 17:05:46]

Creates outside-staff-interface

[MikeSol, 2013-03-20 21:23:02]

Hey all,

Following up to my comment on the tracker, I'd like to not push this until
everyone has had a chance to think through David's e-mails about 2.18.  I
anticipate this needing 2-3 months of user testing to catch and fix any bugs. 
Given that this is my first big patch since August, it'll likely take me another
6-7 months to do the next phase on top of this patch. Comfortably after 2.18
(hopefully).

I am confident in the patch and think it is a step in the right direction for
numerous reasons, but it's obviously not worth pushing it if people want to get
2.18 out in the next couple months.  If I did push it, I'd send an e-mail to
lilypond-user once the next development version came out encouraging people to
compile big scores and report back any anomalies.  I haven't seen any in my
scores.

Cheers,
MS

[MikeSol, 2013-03-28 19:09:41]

Rebase off of current master.

[MikeSol, 2013-04-16 06:29:29]

Gives Hairpin outside-staff-interface

[dak, 2013-12-11 13:44:23]

I have a headache after the first file of 30, so this is just this one file and
does not imply that the others are fine.

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

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface....
lily/axis-group-interface.cc:1032: "outside-staff-placement-directive "
There is probably a reason that this

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface....
lily/axis-group-interface.cc:1041: "vertical-skyline-elements "
and this property have been removed from the Axis_group_interface even though
they are still being used.  What's the reason?

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

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface....
lily/axis-group-interface.cc:45: staff_priority_less (Grob *const &g1, Grob
*const &g2)
There is no point in passing references to pointers when the pointers are not
actually being changed.

Do you mean to pass references to the grobs instead?

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface....
lily/axis-group-interface.cc:59: return g1 < g2;
Making decisions based on memory address is a bad idea since it leads to
irreproducible behavior.  Since the order does not need changing, one can just
return true here.

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface....
lily/axis-group-interface.cc:77: static void
There is no comment that indicates what add_interior_skylines can be used for,
what its input and output are, where and when it should be used.

This is write-only code.

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface....
lily/axis-group-interface.cc:99: SCM
There is no comment what valid_outside_staff_placement_directive is called for
and what its results are.

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface....
lily/axis-group-interface.cc:104: if ((directive == ly_symbol2scm
("left-to-right-greedy"))
If there is only a limited set of valid values, this should be checked by an
appropriate type definition of outside-staff-placement-directive at assigment
time rather than blowing up at run time.

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface....
lily/axis-group-interface.cc:691: MAKE_SCHEME_CALLBACK (Axis_group_interface,
calc_inside_staff_skylines, 1);
There is no comment what calc_inside_staff_skylines is supposed to calculate,
what marks an "inside_staff_skyline", and how it differs from other skylines.

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface....
lily/axis-group-interface.cc:701: assert (y_common == me);
If skylines are disabled, why would this assertion be true?

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface....
lily/axis-group-interface.cc:724:
sane_outside_staff_priority_parental_relationship (Grob *me)
What has this to do with sanity?  Are childs only allowed larger staff
priorities?  Why?

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface....
lily/axis-group-interface.cc:739: ancestor_priority_plus_a_bit (Grob *me)
Why do we need this?

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface....
lily/axis-group-interface.cc:749: return scm_from_double (scm_to_double
(ancestor->get_property ("outside-staff-priority")) + 0.0001);
This does not distinguish between various ancestors, so both a child as well as
a grandchild will get assigned the same priority based on their common ancestor.
 If the degree of ancestry does not count into the result, it would appear that
this function only serves for disambiguation at a single level.

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface....
lily/axis-group-interface.cc:767: in two movings.
Huh?  Above we called a staff priority relation "sane" when the parent had a
smaller outside staff priority.  Now it must not have any outside staff
priority?

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface....
lily/axis-group-interface.cc:771: elements[i]->warning ("An elements' Y parent
must have a lower outside staff priority than the element.");
Looks like the above comment got things wrong.

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface....
lily/axis-group-interface.cc:778: are triggered beforehand.
But we do not _do_ any sorting here.  Why would we not call the extents before
we do the actual sorting rather than in an unrelated function?

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface....
lily/axis-group-interface.cc:785: MAKE_SCHEME_CALLBACK (Axis_group_interface,
vertical_skyline_elements, 1);
There is no comment what this function returns, and it obviously does not merely
return the elements as its name would suggest.

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface....
lily/axis-group-interface.cc:841: elt->programming_error ("Sorting function
should have made sure that I have an outside-staff-priority although my parent
does. Setting to parent's...");
The error message does not make sense. "Sorting function should have made sure
that I have an outside-staff-priority although my parent does."?

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface....
lily/axis-group-interface.cc:851: bool l2r = ((directive == ly_symbol2scm
("left-to-right-greedy"))
Actually, this strongly suggests that the "directive" should be split into two
properties, one being a Direction property, the other being a boolean.  It makes
no sense to conflate orthogonal properties into a single property just to pick
them apart afterwards.

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface....
lily/axis-group-interface.cc:887: && scm_is_eq (elements[i + 1]->get_property
("outside-staff-priority"), priority))
Priorities are generally numeric and cannot reliably be compared with scm_is_eq.

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface....
lily/axis-group-interface.cc:899: for (vsize j = l2r ? 0 : current_elts.size ();
This is rather inscrutable, and it is worth noting that this does not work
symmetrically since the sort order, as it has been established here, is _always_
according to the _left_ edge of a grob.  So for l->r placement, we are sorted
according to trailing edge, for r->l placement, according to the leading edge.

It also seems utterly pointless to go through the loop when polite == false. 
One can just return the array, potentially reversed.

Since r->l order is simulated by going reverse anyway, one can just do the
conditional reverse independent of polite, return on !polite and then do the
loop in forward direction either way.

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface....
lily/axis-group-interface.cc:917: if (x_extent[LEFT] <= last_end[dir] && polite)
Shouldn't that be < here?

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface....
lily/axis-group-interface.cc:927: skipped_elements.clear ();
If we have r->l order here, the loop was run backwards, pushing in this
backwards order into skipped_elements.  However, the next loop will again be run
backwards, meaning that in the case of r->l ordering, skipped_elements will be
ordered the wrong way round.

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface....
lily/axis-group-interface.cc:970: if (!ancestor && !scm_is_number
(elt->get_property ("outside-staff-priority")))
There is no point in calculating the outside_staff_ancestor when this element
has an outside-staff-priority.