Re: Gets vertical skylines from grob stencils (issue 5626052)

[mtsolo]

Thanks for the review!


http://codereview.appspot.com/5626052/diff/105001/input/regression/text-script-vertical-skylines.ly
File input/regression/text-script-vertical-skylines.ly (right):

http://codereview.appspot.com/5626052/diff/105001/input/regression/text-script-vertical-skylines.ly#newcode5
input/regression/text-script-vertical-skylines.ly:5: kerning.
On 2012/08/14 04:21:33, Keith wrote:
> It is not obvious we want this for TextScripts,
>    {b'^"Élever" b'^"brusquement"}
> so don't enshrine the requirement in a regtest.

Done.

http://codereview.appspot.com/5626052/diff/105001/input/regression/volta-bracket-vertical-skylines.ly
File input/regression/volta-bracket-vertical-skylines.ly (right):

http://codereview.appspot.com/5626052/diff/105001/input/regression/volta-bracket-vertical-skylines.ly#newcode4
input/regression/volta-bracket-vertical-skylines.ly:4: texidoc = "Volta
brackets are vertically kerned to objects below them.
On 2012/08/14 04:21:33, Keith wrote:
> Good, but "fit" not "kerned"
> To me, 'to kern' includes making many aesthetic judgments to come up
with
> individual adjustments for each fitting pair.

Done.

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc#newcode629
lily/axis-group-interface.cc:629: Three rounds:
On 2012/08/14 04:21:33, Keith wrote:
> I don't understand the comment, but what I needed to figure out was :
> Given pre-padded vertical skylines 'pair' for an outside-staff Grob,
this
> figures out the vertical position of the Grob.  It raises the skylines
in 'pair'
> and shifts (along the skyline) 'horizontal_skylines' by the same
amount.

> Using horizontal_skylines (with the buildings pointing horizontally)
to
> determine vertical position, is a new concept worth mentioning.

> I'm still figuring out 'exempt', '*_forest', etc.

Better comment added.

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc#newcode658
lily/axis-group-interface.cc:658: check for horizontal edges jinnying up
On 2012/08/14 04:21:33, Keith wrote:
> I learned the local dialect 200 miles north, so I don't know what this
means. Is
> this the case where the edges lie side a by each, or kitty corner ?

Elaine Gould uses the phrase "jinnying up" in behind bars, and given
that we are using that as one of our base texts, I'm ok using the phrase
here.

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc#newcode728
lily/axis-group-interface.cc:728: We need to take the padding away now
that it has been shifted.  Otherwise
On 2012/08/14 04:21:33, Keith wrote:
> I can't find where the padding was ever added.

The call to grow in add_grobs_of_one_priority.

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc#newcode751
lily/axis-group-interface.cc:751: edge of the just-placed grob).
Otherwise, we skip it until the next pass.
On 2012/08/14 04:21:33, Keith wrote:
> This comment describes the old way of solving the problem you saw with
> 'midi-dynamics.pdf' but you removed the old code.  Maybe you want to
use the old
> method, but make code and comment agree one way or another.

Excellent observation - I'd completely forgotten this. Perhaps this
takes care of the midi-dynamics.ly problem altogether...will
investigate.

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc#newcode778
lily/axis-group-interface.cc:778: Note we only use 75% of padding and
apply the full compliment only if
On 2012/08/14 04:21:33, Keith wrote:
> It looks like you add 50% padding here, then pass to
> avoid_outside_staff_collision() which removes 37.5% of padding away.
Probably
> not what you meant to do.

> The verbs 'use' and 'apply' are vague enough that the comment didn't
help me see
> what you meant to do.

I think that both remove 50%, no?

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc#newcode843
lily/axis-group-interface.cc:843: Three rounds:
On 2012/08/14 04:21:33, Keith wrote:
> Does this part of the comment apply to the code below ?

The comment's blech...I'll redo it in a later patchset (remind me if I
don't).

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc#newcode890
lily/axis-group-interface.cc:890: feature present in one regrest :(
On 2012/08/14 04:21:33, Keith wrote:
> You tease us.  Which regression test ?

Done.

http://codereview.appspot.com/5626052/diff/105001/lily/beam.cc
File lily/beam.cc (right):

http://codereview.appspot.com/5626052/diff/105001/lily/beam.cc#newcode1368
lily/beam.cc:1368: shift += 0.01;
On 2012/08/14 04:21:33, Keith wrote:
> * beamdir ?

Done.

http://codereview.appspot.com/5626052/diff/105001/lily/skyline-pair.cc
File lily/skyline-pair.cc (right):

http://codereview.appspot.com/5626052/diff/105001/lily/skyline-pair.cc#newcode103
lily/skyline-pair.cc:103: // other[UP] then we will not intersect.
On 2012/08/14 04:21:33, Keith wrote:
> Worth mentioning that the shift direction d is LEFT RIGHT for vertical
skylines,
> all axes reversed for horizontal.

Done.

http://codereview.appspot.com/5626052/diff/105001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/105001/lily/skyline.cc#newcode157
lily/skyline.cc:157: Real ret = (y - y_intercept_ - slope_ * x) /
slope_;
On 2012/08/14 04:21:33, Keith wrote:
> (y - height(x))

Done.

http://codereview.appspot.com/5626052/diff/105001/lily/skyline.cc#newcode287
lily/skyline.cc:287: // Since a skyline should always be normalized, we
can
On 2012/08/14 04:21:33, Keith wrote:
> I think you mean "this function requires the skyline to be normalized,
so that
> ..."

Done.

http://codereview.appspot.com/5626052/diff/105001/lily/skyline.cc#newcode922
lily/skyline.cc:922: // direction which will result in THIS and OTHER
not overlapping.
On 2012/08/14 04:21:33, Keith wrote:
> .. in the given direction /along/ the skyline, locally called the
horizon.

Done.

http://codereview.appspot.com/5626052/diff/105001/scm/define-grob-properties.scm
File scm/define-grob-properties.scm (right):

http://codereview.appspot.com/5626052/diff/105001/scm/define-grob-properties.scm#newcode663
scm/define-grob-properties.scm:663:
(outside-staff-padding-activation-factor ,number? "The percentage
On 2012/08/14 04:21:33, Keith wrote:
> This is extremely confusing.  It sounds like you allow grobs to come
within a
> factor of the user-requested padding of protrusions from the staff,
but if the
> protrusion comes out too far you jump away to full padding.

That's how it's currently implemented.  It is the only way I've found to
get the old behavior in stuff like midi-pedal.ly, but I'll try
reimplementing the comment above add_grobs_of_one_priority.

http://codereview.appspot.com/5626052/diff/105001/scm/define-grobs.scm
File scm/define-grobs.scm (right):

http://codereview.appspot.com/5626052/diff/105001/scm/define-grobs.scm#newcode913
scm/define-grobs.scm:913: (vertical-skylines .
,ly:grob::vertical-skylines-from-stencil)
On 2012/08/14 04:21:33, Keith wrote:
> For Flags, I would think horizontal-skylines-from-stencil would be
useful ...
> but maybe it is only used for outside-staff placement ?

I'll look into this after pushing all of this stuff - remind me if I
forget!

http://codereview.appspot.com/5626052/diff/105001/scm/define-grobs.scm#newcode1590
scm/define-grobs.scm:1590: ; allows double flat of F to be nestled over
dots of C
On 2012/08/14 04:21:33, Keith wrote:
> Can't you adjust for that on something more specific to the symptom,
like
> DotColumn or Accidental ?

No, as padding is added on only at the PaperColumn level. It may be
possible to make this more refined, though...I'll look into it in a
separate commit.  As usual, remind me if I forget.

http://codereview.appspot.com/5626052/