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

[mike]

On 1 févr. 2013, at 16:04, address@hidden 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-interface.cc
> File lily/directional-element-interface.cc (right):
> 
> https://codereview.appspot.com/7185044/diff/17001/lily/directional-element-interface.cc#newcode25
> 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#newcode81
> 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-interface.cc
> File lily/side-position-interface.cc (right):
> 
> https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interface.cc#newcode185
> 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-interface.cc#newcode296
> 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-interface.cc#newcode461
> 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-interface.cc#newcode481
> 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-interface.cc#newcode498
> 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-interface.cc#newcode505
> 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-interface.cc#newcode515
> 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-interface.cc#newcode539
> 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-interface.cc#newcode542
> 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#newcode494
> 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/