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

[address@hidden]

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-number-outside-staff-positioning.ly
> File input/regression/tuplet-number-outside-staff-positioning.ly
> (right):
> 
> https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-number-outside-staff-positioning.ly#newcode15
> 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-number-outside-staff-priority.ly
> File input/regression/tuplet-number-outside-staff-priority.ly (right):
> 
> https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-number-outside-staff-priority.ly#newcode13
> 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.cc#newcode595
> 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.cc#newcode713
> 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.cc#newcode755
> 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-interface.hh
> File lily/include/axis-group-interface.hh (right):
> 
> https://codereview.appspot.com/7185044/diff/32003/lily/include/axis-group-interface.hh#newcode69
> 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-element-interface.hh
> File lily/include/directional-element-interface.hh (right):
> 
> https://codereview.appspot.com/7185044/diff/32003/lily/include/directional-element-interface.hh#newcode26
> 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-interface.hh
> File lily/include/side-position-interface.hh (right):
> 
> https://codereview.appspot.com/7185044/diff/32003/lily/include/side-position-interface.hh#newcode45
> 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-interface.cc
> File lily/side-position-interface.cc (right):
> 
> https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interface.cc#newcode184
> 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-interface.cc#newcode464
> 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-interface.cc#newcode497
> 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-interface.cc#newcode538
> 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-interface.cc#newcode539
> 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-interface.cc#newcode565
> 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-interface.cc#newcode593
> 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