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

[k-ohara5a5a]

Worked nicely on some piano music and a Dvorak symphony.

The code is bewildering.  So far I've mostly read the comments.

The bit with the activation-factor is pretty ugly.
What was the size of the problem ?  Did the script that should have fit
lack space of padding, 0.5*padding, or epsilon ?


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.
It is not obvious we want this for TextScripts,
  {b'^"Élever" b'^"brusquement"}
so don't enshrine the requirement in a regtest.

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.
Good, but "fit" not "kerned"
To me, 'to kern' includes making many aesthetic judgments to come up
with individual adjustments for each fitting pair.

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

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

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
I can't find where the padding was ever added.

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

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

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc#newcode843
lily/axis-group-interface.cc:843: Three rounds:
Does this part of the comment apply to the code below ?

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc#newcode890
lily/axis-group-interface.cc:890: feature present in one regrest :(
You tease us.  Which regression test ?

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;
* beamdir ?

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.
Worth mentioning that the shift direction d is LEFT RIGHT for vertical
skylines, all axes reversed for horizontal.

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_;
(y - height(x))

http://codereview.appspot.com/5626052/diff/105001/lily/skyline.cc#newcode287
lily/skyline.cc:287: // Since a skyline should always be normalized, we
can
I think you mean "this function requires the skyline to be normalized,
so that ..."

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.
.. in the given direction /along/ the skyline, locally called the
horizon.

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

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)
For Flags, I would think horizontal-skylines-from-stencil would be
useful ...
but maybe it is only used for outside-staff placement ?

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
Can't you adjust for that on something more specific to the symptom,
like DotColumn or Accidental ?

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