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

[dak]

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.cc#oldcode1032
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.cc#oldcode1041
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.cc#newcode45
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.cc#newcode59
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.cc#newcode77
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.cc#newcode99
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.cc#newcode104
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.cc#newcode691
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.cc#newcode701
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.cc#newcode724
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.cc#newcode739
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.cc#newcode749
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.cc#newcode767
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.cc#newcode771
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.cc#newcode778
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.cc#newcode785
lily/axis-group-interface.cc:785: MAKE_SCHEME_CALL

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

[mtsolo]

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

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

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[m...@mikesolomon.org]

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

> 
> On 25 févr. 2013, at 16:29, d...@gnu.org wrote:
> 
>> On 2013/02/24 16:20:53, mike7 wrote:
>>> On 24 févr. 2013, at 17:18, David Kastrup  wrote:
>> 
 "m...@mikesolomon.org"  writes:
 
> On 24 févr. 2013, at 16:37, mailto:d...@gnu.org wrote:
> 
>> On 2013/02/24 13:27:39, mike7 wrote:
>>> On 24 févr. 2013, at 12:40, mailto:d...@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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[m...@mikesolomon.org]

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

> On 2013/02/24 16:20:53, mike7 wrote:
>> On 24 févr. 2013, at 17:18, David Kastrup  wrote:
> 
>> > "m...@mikesolomon.org"  writes:
>> >
>> >> On 24 févr. 2013, at 16:37, mailto:d...@gnu.org wrote:
>> >>
>> >>> On 2013/02/24 13:27:39, mike7 wrote:
>>  On 24 févr. 2013, at 12:40, mailto:d...@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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[dak]

On 2013/02/24 16:20:53, mike7 wrote:

On 24 févr. 2013, at 17:18, David Kastrup  wrote:



> "m...@mikesolomon.org"  writes:
>
>> On 24 févr. 2013, at 16:37, mailto:d...@gnu.org wrote:
>>
>>> On 2013/02/24 13:27:39, mike7 wrote:
 On 24 févr. 2013, at 12:40, mailto:d...@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.


https://codereview.appspot.com/7185044/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[m...@mikesolomon.org]

On 24 févr. 2013, at 11:45, k-ohara5...@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#newcode76
> 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.cc#oldcode886
> lily/axis-group-interface.cc:886: multimap 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-interface.cc
> File lily/side-position-interface.cc (right):
> 
> https://codereview.appspot.com/7185044/diff/45018/lily/side-position-interface.cc#newcode694
> 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.


___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[m...@mikesolomon.org]

On 24 févr. 2013, at 17:18, David Kastrup  wrote:

> "m...@mikesolomon.org"  writes:
> 
>> On 24 févr. 2013, at 16:37, d...@gnu.org wrote:
>> 
>>> On 2013/02/24 13:27:39, mike7 wrote:
 On 24 févr. 2013, at 12:40, mailto:d...@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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[David Kastrup]

"m...@mikesolomon.org"  writes:

> On 24 févr. 2013, at 16:37, d...@gnu.org wrote:
>
>> On 2013/02/24 13:27:39, mike7 wrote:
>>> On 24 févr. 2013, at 12:40, mailto:d...@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

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[m...@mikesolomon.org]

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

> On 2013/02/24 13:27:39, mike7 wrote:
>> On 24 févr. 2013, at 12:40, mailto:d...@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


___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[dak]

On 2013/02/24 13:27:39, mike7 wrote:

On 24 févr. 2013, at 12:40, mailto:d...@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?


https://codereview.appspot.com/7185044/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[m...@mikesolomon.org]

On 24 févr. 2013, at 12:40, d...@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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[dak]

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/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[k-ohara5a5a]

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#newcode76
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.cc#oldcode886
lily/axis-group-interface.cc:886: multimap riders;
Yipee!! no more riders!
I dance on the grave of the mulitmap.

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

https://codereview.appspot.com/7185044/diff/45018/lily/side-position-interface.cc#newcode694
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

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

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[tdanielsmusic]

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#newcode73
Documentation/changes.tely:73: @code{Slur}, for example, are all grobs
are traditionally not outside
... grobs that are ...

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

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[dak]

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

https://codereview.appspot.com/7185044/diff/45018/input/regression/tuplet-bracket-outside-staff-priority.ly#newcode17
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-number-outside-staff-positioning.ly
File input/regression/tuplet-number-outside-staff-positioning.ly
(right):

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

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

https://codereview.appspot.com/7185044/diff/45018/input/regression/tuplet-number-outside-staff-priority.ly#newcode12
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#newcode1376
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.

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

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[dak]

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#newcode68
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#newcode69
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?

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

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[m...@mikesolomon.org]

On 19 févr. 2013, at 19:27, d...@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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[dak]

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
otherwi

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

[Trevor Daniels]

From: 
Sent: Tuesday, February 19, 2013 2:22 PM
>

>On 19 févr. 2013, at 16:11, Trevor Daniels  wrote:
>> 
>> From: 
>> Sent: Monday, February 18, 2013 8:32 PM
>> 
>>> On 18 févr. 2013, at 20:07, d...@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-objects#the-outside_002dstaff_002dpriority-property

Trevor
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[m...@mikesolomon.org]

On 19 févr. 2013, at 16:11, Trevor Daniels  wrote:

> 
> From: 
> Sent: Monday, February 18, 2013 8:32 PM
> 
>> On 18 févr. 2013, at 20:07, d...@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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[Trevor Daniels]

From: 
Sent: Monday, February 18, 2013 8:32 PM

> On 18 févr. 2013, at 20:07, d...@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
 
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[m...@mikesolomon.org]

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

> On 2013/02/15 06:37:20, mike7 wrote:
> 
> 
> 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.
> 
> [...]
> 
> 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:


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.


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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[m...@mikesolomon.org]

On 18 févr. 2013, at 20:07, d...@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
  
}

does nothing.  Even worse:

\relative c''' {
  \once \override MultiMeasureRest #'outside-staff-priority = #10
  R1
  1
}

This patch will eventually fix things like this.

Cheers,
MS
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[dak]

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


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.


[...]

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.


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

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[k-ohara5a5a]

The reorganization looks reasonable.

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

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


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

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

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

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


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

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


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

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

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

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

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

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

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

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

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

https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interface.cc#newcode512
lily/side-position-interface.cc:512: && !pure)
I think the remainder of th

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

[m...@mikesolomon.org]

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

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

[dak]

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-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?

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.

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?

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?

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 &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-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?

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.

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_of

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

[m...@mikesolomon.org]

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

> On 2013/02/03 08:45:13, mike7 wrote:
>> On 1 févr. 2013, at 16:04, mailto:d...@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-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.
> 
> 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-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.
> 
> 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-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?
> 
> 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.

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

[dak]

On 2013/02/03 08:45:13, mike7 wrote:

On 1 févr. 2013, at 16:04, mailto:d...@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-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.


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


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-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?


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


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

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

[mike]

On 1 févr. 2013, at 16:04, d...@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-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 t

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

[dak]

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

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.

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).

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-interface.cc#newcode296
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-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?

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.

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.

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?

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 

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

[mtsolo]

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

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

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[mike]

On 28 janv. 2013, at 06:33, Keith OHara  wrote:

> On Sun, 27 Jan 2013 01:45:16 -0800, m...@mikesolomon.org 
>  wrote:
> 
>> On 26 janv. 2013, at 23:21, k-ohara5...@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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[mtsolo]

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.

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

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[Keith OHara]

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


On 26 janv. 2013, at 23:21, k-ohara5...@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.


___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[m...@mikesolomon.org]

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

> "m...@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___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[dak]

"m...@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


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

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[m...@mikesolomon.org]

On 26 janv. 2013, at 23:21, k-ohara5...@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.scm#newcode1115
> 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.scm#newcode1117
> 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.
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[k-ohara5a5a]

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.scm#newcode1115
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.scm#newcode1117
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.

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

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel