Uses single algorithm for side-position spacing.

Uses single algorithm for side-position spacing.

To implement this, uses grob-extent based boxes for simple skylines
instead of stencil-extent based boxes. Stencils are still used for
more complicated skylines.

Generalizes algorithm used in Fingeing_column code, allowing for better
skyline spacing and fixing some collisions.

[dak, 2012-11-11 13:50:14]

http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc
File lily/side-position-interface.cc (right):

http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#...
lily/side-position-interface.cc:85: Position next to support, taking into
account my own dimensions and padding.
Incomprehensible comment.  Is position verb or noun?  What do your own
dimensions have to do with it?

http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#...
lily/side-position-interface.cc:88: axis_aligned_side_helper (SCM smob, Axis a,
bool pure, int start, int end, SCM current_off_scm)
Why is the meaning of the arguments undocumented?

http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#...
lily/side-position-interface.cc:101: MAKE_SCHEME_CALLBACK_WITH_OPTARGS
(Side_position_interface, x_aligned_side, 2, 1, "");
It is bad enough if you omit all comments from the code, but if the declaration
of a function _explicitly_ includes a DOC string, specifying this as "" for a
function with a vague name is not really helpful.

http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#...
lily/side-position-interface.cc:108: MAKE_SCHEME_CALLBACK_WITH_OPTARGS
(Side_position_interface, y_aligned_side, 2, 1, "");
See above.

http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#...
lily/side-position-interface.cc:115: MAKE_SCHEME_CALLBACK_WITH_OPTARGS
(Side_position_interface, pure_y_aligned_side, 4, 1, "");
See above.

http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#...
lily/side-position-interface.cc:144: // long function - each stage is clearly
marked
Too bad that there is
a) no documentation what the long function does
b) no documentation what each step does.

http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#...
lily/side-position-interface.cc:275: // necessary for the InstrumentName grob
Well, if there is no _apparent_ logic to it, that would seem to warrant
explaining the logic, wouldn't it, instead of proudly announcing another puzzle
to the reader?

http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#...
lily/side-position-interface.cc:309: /* FIXME: 1000 should relate to paper size.
 */
FIXME: the meaning of no variable at all is documented, so how should the reader
even know this is "paper size" related?

[mike7, 2012-11-11 15:26:48]

On 11 nov. 2012, at 14:50, dak@gnu.org wrote:

> 
> http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc
> File lily/side-position-interface.cc (right):
> 
>
http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#...
> lily/side-position-interface.cc:85: Position next to support, taking
> into account my own dimensions and padding.
> Incomprehensible comment.  Is position verb or noun?  What do your own
> dimensions have to do with it?

It is a verb.  "own dimensions" means that the dimensions of the object itself
are taken into account (as opposed to just the offset).

> 
>
http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#...
> lily/side-position-interface.cc:88: axis_aligned_side_helper (SCM smob,
> Axis a, bool pure, int start, int end, SCM current_off_scm)
> Why is the meaning of the arguments undocumented?

I'm getting the sense that what you want to do is set up a commenting style for
the entire code base.  I'd recommend starting a discussion about this.  We could
establish a rule that there must be a comment for every argument going to every
function (this is perhaps not a bad idea) but the code is full of undocumented
functions and arguments.  So it seems like an issue to tackle apart.

> 
>
http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#...
> lily/side-position-interface.cc:101: MAKE_SCHEME_CALLBACK_WITH_OPTARGS
> (Side_position_interface, x_aligned_side, 2, 1, "");
> It is bad enough if you omit all comments from the code, but if the
> declaration of a function _explicitly_ includes a DOC string, specifying
> this as "" for a function with a vague name is not really helpful.

Do a git grep for MAKE_SCHEME_CALLBACK_WITH_OPTARGS and you'll see that the
majority are un-doc-stringed.  Again, it sounds like you want to put in place an
overarching system for comments and documentation.  This is not a bad idea, but
I think it needs to be addressed as a separate issue.  I don't mind at all your
using this patch as a test-case, but I'd recommend first establishing a policy
and then going through the code base and applying it everywhere.

> 
>
http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#...
> lily/side-position-interface.cc:108: MAKE_SCHEME_CALLBACK_WITH_OPTARGS
> (Side_position_interface, y_aligned_side, 2, 1, "");
> See above.

See above.

> 
>
http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#...
> lily/side-position-interface.cc:115: MAKE_SCHEME_CALLBACK_WITH_OPTARGS
> (Side_position_interface, pure_y_aligned_side, 4, 1, "");
> See above.

See above.

> 
>
http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#...
> lily/side-position-interface.cc:144: // long function - each stage is
> clearly marked
> Too bad that there is
> a) no documentation what the long function does

See above.

> b) no documentation what each step does.
> 

Every time there is an important new thing happening there is a comment.

>
http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#...
> lily/side-position-interface.cc:275: // necessary for the InstrumentName
> grob
> Well, if there is no _apparent_ logic to it, that would seem to warrant
> explaining the logic, wouldn't it, instead of proudly announcing another
> puzzle to the reader?

Sorry, the problem is the word "apparent".  There is actually no logic to it,
period.  It is an arbitrary decision that allowed InstrumentName to work.  I
will reword it to make it clear that the problem is that 0 is arbitrary.

> 
>
http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#...
> lily/side-position-interface.cc:309: /* FIXME: 1000 should relate to
> paper size.  */
> FIXME: the meaning of no variable at all is documented, so how should
> the reader even know this is "paper size" related?

I'm not sure who originally wrote this comment - I understand it to mean that
the number 1000 is a magic number that should change with the size of the paper
being used.

To be clear, I have no problem implementing a LilyPond documentation and
commenting policy to move towards the establishment of an API.  This sounds like
a worthwhile yet difficult task.  I would start talking about this separately
from any one patch.  Of course, this does not exempt patches from using
comments.  I feel that my comments in this patch are clear for someone who reads
the code with them.  What you're talking about is something more systematic,
which is a great idea and worth opening a tracker issue about.

Cheers,
MS

[dak, 2012-11-11 15:33:28]

On 2012/11/11 15:26:48, mike7 wrote:
 
> To be clear, I have no problem implementing a LilyPond documentation and
> commenting policy to move towards the establishment of an API.  This sounds
like
> a worthwhile yet difficult task.  I would start talking about this separately
> from any one patch.

Mike, you document _nothing_, _ever_.  And whenever one asks you to put even the
slightest amount of documentation to any patch of yours, you reply "this would
be a separate issue".

You are pretty much the only person who _deletes_ more comment lines than he
ever writes.

I don't care about what "policy" we want to have for comments, but you _need_ to
document the _purpose_ and the _working_ of your code, or nobody will ever be
able to figure out how it works.

You have no problem tripling the complexity of code without writing a single
comment, and your excuse always is "it was badly documented before, so lets
document it even worse".

And no, answering a question in review is not the same as documenting code.

[dak, 2012-11-11 19:57:32]

To make it short: a function needs documentation that tells its purpose in the
overall scheme of things (for what purpose is it used), its outline (what does
it do), its input and output.

A "policy" may describe how they are formatted and whether or not they are used
as Scheme documentation strings.

But whether or not there is "policy", this information _must_ be present or the
function is incomplete.  That's not "policy" but "sanity".

As long as you don't have an argument better than "I never did that" or "other
code does not do that", there is _no_ point in refusing to add this information.
 In what respect will the code be better maintainable if you omit this
information?

If you have had a coherent design in mind when writing the code, it should be
fresh in your memory and it is just a matter of writing it down.  If you did not
have a coherent design in mind when writing the code, maybe you'll even discover
something dishy when trying to describe the design.

So again: in what respect is the code improved by you not writing down what it
is supposed to achieve in what context by doing what and how?

[mike7, 2012-11-11 22:53:17]

On 11 nov. 2012, at 20:57, dak@gnu.org wrote:

> To make it short: a function needs documentation that tells its purpose
> in the overall scheme of things (for what purpose is it used), its
> outline (what does it do), its input and output.
> 

This sounds great.

> A "policy" may describe how they are formatted and whether or not they
> are used as Scheme documentation strings.
> 

This sounds even better - this is the type of thing that would be excellent.

> But whether or not there is "policy", this information _must_ be present
> or the function is incomplete.  That's not "policy" but "sanity".
> 
> As long as you don't have an argument better than "I never did that" or
> "other code does not do that", there is _no_ point in refusing to add
> this information.  In what respect will the code be better maintainable
> if you omit this information?
> 

I don't refuse to add any of these things.  I think it'd be worthwhile to have
commenting and docstring guidelines for functions - this will help me figure out
how to best add these things.

> If you have had a coherent design in mind when writing the code, it
> should be fresh in your memory and it is just a matter of writing it
> down.  If you did not have a coherent design in mind when writing the
> code, maybe you'll even discover something dishy when trying to describe
> the design.
> 
> So again: in what respect is the code improved by you not writing down
> what it is supposed to achieve in what context by doing what and how?

I have no problem doing this.  I learned to write LilyPond code by reading
LilyPond code, and similarly, I learned to not write comments by reading
LilyPond code.  So, I don't know how to write comments effectively and I need
the help of someone who can bring this skill to the LilyPond code base.  It'd be
great if you could open up a tracker issue and post what you believe to be a
good template for function commenting.  I will use that model whenever I write a
new function.

Cheers,
MS

[Keith, 2012-11-13 04:38:07]

{\clef bass
 <g^3 a^5>2..->
 << r16 \\ r \\ r  \\ r \\ >> eeses'16
 \set fingeringOrientations = #'(right)
  <c e>8-1-4 <c^1 e^4> <g,-3 b,-4> r
  r2 }

http://codereview.appspot.com/6827072/diff/1/input/regression/dynamics-avoid-...
File input/regression/dynamics-avoid-cross-staff-stem.ly (right):

http://codereview.appspot.com/6827072/diff/1/input/regression/dynamics-avoid-...
input/regression/dynamics-avoid-cross-staff-stem.ly:14: a8 \p [ \change Staff =
"PnRH" \stemDown gis'8 \fff ]
Before your patch, the dynamics below the staff clear each other; after your
patch, they collide.

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

http://codereview.appspot.com/6827072/diff/1/lily/axis-group-interface.cc#new...
lily/axis-group-interface.cc:403: Axis_group_interface::cross_staff (SCM smob)
For what situation?   Which object that supports axis-group-interface
(PianoPedalSpanner, DynamicLineSpanner) should be potentially considered a
cross-staff object?

http://codereview.appspot.com/6827072/diff/1/lily/box-quarantine.cc
File lily/box-quarantine.cc (right):

http://codereview.appspot.com/6827072/diff/1/lily/box-quarantine.cc#newcode70
lily/box-quarantine.cc:70: return abs (ii0.index_ - mid) < abs (ii1.index_ -
mid);
gcc 4.5.1 complains that he cannot determine which version of the overloaded
abs() should be called.  (Polymorphism is l33t, isn't it.)  I just removed the
abs() calls to get it to compile, because index_ is an unsigned type so the
arguments are always positive anyway.

http://codereview.appspot.com/6827072/diff/1/lily/script-engraver.cc
File lily/script-engraver.cc (right):

http://codereview.appspot.com/6827072/diff/1/lily/script-engraver.cc#newcode270
lily/script-engraver.cc:270: // we never want scripts to be tucked down next to
stems
This disagrees with 	 
input/regression/finger-chords.ly

http://codereview.appspot.com/6827072/diff/1/lily/stencil-integral.cc
File lily/stencil-integral.cc (right):

http://codereview.appspot.com/6827072/diff/1/lily/stencil-integral.cc#newcode988
lily/stencil-integral.cc:988: pure = Rest::has_interface (me) ? true : pure;
Worse than losing ledgers, the 'pure' extent is in the wrong position for those
rests that might have been moved.

At this point in determining the layout, shouldn't you ensure that the rests are
positioned, by testing 'positioning-done, before figuring their skylines for use
in note-spacing ?

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

http://codereview.appspot.com/6827072/diff/1/scm/define-grob-properties.scm#n...
scm/define-grob-properties.scm:668: (other-axis-padding ,number? "The amount to
pad the axis
traditionally called horizon-padding, which I suppose makes sense in the
metaphor of a city skyline, as this padding is in the direction of the horizon.

[mike7, 2012-11-14 07:12:46]

> Re: Uses single algorithm for side-position spacing. (issue 6827072)
> 
> {\clef bass
>  <g^3 a^5>2..->
>  << r16 \\ r \\ r  \\ r \\ >> eeses'16
>  \set fingeringOrientations = #'(right)
>   <c e>8-1-4 <c^1 e^4> <g,-3 b,-4> r
>   r2 }
> 

Beautiful ugly test case.  Even with current master it is atrocious. I've fixed
everything but the double-flat on rest collision, which'd require a separate
patch.

> 
>
http://codereview.appspot.com/6827072/diff/1/input/regression/dynamics-avoid-...
> 
> File input/regression/dynamics-avoid-cross-staff-stem.ly (right):
> 
> 
>
http://codereview.appspot.com/6827072/diff/1/input/regression/dynamics-avoid-...
> 
> input/regression/dynamics-avoid-cross-staff-stem.ly:14: a8 \p [ \change
> Staff = "PnRH" \stemDown gis'8 \fff ]
> Before your patch, the dynamics below the staff clear each other; after
> your patch, they collide.
> 

Fixed.

> 
> http://codereview.appspot.com/6827072/diff/1/lily/axis-group-interface.cc
> 
> File lily/axis-group-interface.cc (right):
> 
> 
>
http://codereview.appspot.com/6827072/diff/1/lily/axis-group-interface.cc#new...
> 
> lily/axis-group-interface.cc:403: Axis_group_interface::cross_staff (SCM
> smob)
> For what situation?   Which object that supports axis-group-interface
> (PianoPedalSpanner, DynamicLineSpanner) should be potentially considered
> a cross-staff object?

NoteColumn

> http://codereview.appspot.com/6827072/diff/1/lily/box-quarantine.cc
> 
> File lily/box-quarantine.cc (right):
> 
> 
> http://codereview.appspot.com/6827072/diff/1/lily/box-quarantine.cc#newcode70
> 
> lily/box-quarantine.cc:70: return abs (ii0.index_ - mid) < abs
> (ii1.index_ - mid);
> gcc 4.5.1 complains that he cannot determine which version of the
> overloaded abs() should be called.  (Polymorphism is l33t, isn't it.)  I
> just removed the abs() calls to get it to compile, because index_ is an
> unsigned type so the arguments are always positive anyway.

index_ is always positive and mid_ is always positive but index_ - mid_ will be
positive or negative depending on which one is larger. Changing to fabs.

> 
> 
> http://codereview.appspot.com/6827072/diff/1/lily/script-engraver.cc
> 
> File lily/script-engraver.cc (right):
> 
> 
>
http://codereview.appspot.com/6827072/diff/1/lily/script-engraver.cc#newcode270
> 
> lily/script-engraver.cc:270: // we never want scripts to be tucked down
> next to stems
> This disagrees with 	
> input/regression/finger-chords.ly

True - I mean Script grobs.  I'll make that clearer.

> 
> 
> http://codereview.appspot.com/6827072/diff/1/lily/stencil-integral.cc
> 
> File lily/stencil-integral.cc (right):
> 
> 
>
http://codereview.appspot.com/6827072/diff/1/lily/stencil-integral.cc#newcode988
> 
> lily/stencil-integral.cc:988: pure = Rest::has_interface (me) ? true :
> pure;
> Worse than losing ledgers, the 'pure' extent is in the wrong position
> for those rests that might have been moved.
> 
> At this point in determining the layout, shouldn't you ensure that the
> rests are positioned, by testing 'positioning-done, before figuring
> their skylines for use in note-spacing ?
> 

Even this doesn't work.  It fails maybe every 5th time I compile, so it must be
one of those uniq () sorting problems that I run into from time to time.  The
solution for now is just to use ly:grob::vertical-skylines-from-stencil on the
rests, which don't muddle with extents or positioning and just check the glyph. 
But this is me going into problem-avoidance mode.  There are comments all over
that part of the code base warning about RestColumn positioning, but a big fat
"THIS IS A TICKING CIRCULAR DEPENDENCY TIME BOMB" comment somewhere in there
wouldn't hurt.

> 
> http://codereview.appspot.com/6827072/diff/1/scm/define-grob-properties.scm
> 
> File scm/define-grob-properties.scm (right):
> 
> 
>
http://codereview.appspot.com/6827072/diff/1/scm/define-grob-properties.scm#n...
> 
> scm/define-grob-properties.scm:668: (other-axis-padding ,number? "The
> amount to pad the axis
> traditionally called horizon-padding, which I suppose makes sense in the
> metaphor of a city skyline, as this padding is in the direction of the
> horizon.
> 
> 

Fixed.

Thanks for the review!  Will upload a new patch set in the not-too-distant
future.

Cheers,
MS

[mike7, 2012-11-16 20:32:54]

On 14 nov. 2012, at 07:33, mike@mikesolomon.org wrote:

>> http://codereview.appspot.com/6827072/diff/1/lily/axis-group-interface.cc
>> 
>> File lily/axis-group-interface.cc (right):
>> 
>> 
>>
http://codereview.appspot.com/6827072/diff/1/lily/axis-group-interface.cc#new...
>> 
>> lily/axis-group-interface.cc:403: Axis_group_interface::cross_staff (SCM
>> smob)
>> For what situation?   Which object that supports axis-group-interface
>> (PianoPedalSpanner, DynamicLineSpanner) should be potentially considered
>> a cross-staff object?
> 
> NoteColumn

Hey all,

One result of my approach is that grobs that were not previously cross staff
are.  This allows for better positioning with respect to a system, but results
in collisions with other systems.  Try the following on current master and this
patch :

foo = \new PianoStaff <<
 \new Staff = "up" {   
   s1 |
 }
 \new Staff = "down" {
   \clef bass
   \stemDown 
   % keep staff alive
   <c,, c,>8 [ <c,, c,>8_\f
   \change Staff = "up"
   g' g' ]
   r2 |   
 }
>> 

\score { \foo }
\score { \foo }

In current master, there is a dynamic-on-beam intersection, and w/ my patch
there is a dynamic-on-system intersection.  Both of them are bad, but in terms
of future work on LilyPond, I think the dynamic-on-system is a better
alternative.  The long-term goal of this work is to get cross-staff grobs into
vertical calculations, which will require removing several calls to
translate_axis.  All of this work is moving in that direction by simplifying
grob positioning.  The next step will be to eliminate the function
avoid_outside_staff_collisions and replace it with the the new
Side_position_interface::y_aligned_side that comes w/ this patch.

Cheers,
MS

[mike7, 2012-11-16 23:19:16]

On 16 nov. 2012, at 21:32, mike@mikesolomon.org wrote:

> 
> On 14 nov. 2012, at 07:33, mike@mikesolomon.org wrote:
> 
>>> http://codereview.appspot.com/6827072/diff/1/lily/axis-group-interface.cc
>>> 
>>> File lily/axis-group-interface.cc (right):
>>> 
>>> 
>>>
http://codereview.appspot.com/6827072/diff/1/lily/axis-group-interface.cc#new...
>>> 
>>> lily/axis-group-interface.cc:403: Axis_group_interface::cross_staff (SCM
>>> smob)
>>> For what situation?   Which object that supports axis-group-interface
>>> (PianoPedalSpanner, DynamicLineSpanner) should be potentially considered
>>> a cross-staff object?
>> 
>> NoteColumn
> 
> Hey all,
> 
> One result of my approach is that grobs that were not previously cross staff
are.

On this subject, can someone explain to me how the bounding boxes for the
regtest images are calculated? It seems like they have something to with the
extents of non-cross-staff grobs.  This makes certain regtests unduly croppeed.

Cheers,
MS

[Keith, 2012-11-17 23:19:23]

On 2012/11/14 07:12:46, mike7 wrote:
> > lily/axis-group-interface.cc:403: Axis_group_interface::cross_staff (SCM
> > smob)
> > For what situation?   Which object that supports axis-group-interface
> > (PianoPedalSpanner, DynamicLineSpanner) should be potentially considered
> > a cross-staff object?
> 
> NoteColumn
> 

Of course this leads to the obvious question: Why?
What is the meaning of marking the entire NoteColumn as "cross-staff" ?
It seems you already iterated through the elements of a NoteColumn, including
NoteHeads in skylines but excluding any cross-staff stems.

[Keith, 2012-11-17 23:55:53]

I haven't gotten so far to see the main point, yet.

http://codereview.appspot.com/6827072/diff/11002/lily/box-quarantine.cc
File lily/box-quarantine.cc (right):

http://codereview.appspot.com/6827072/diff/11002/lily/box-quarantine.cc#newco...
lily/box-quarantine.cc:24: Box_quarantine::Box_quarantine (Real padding, Axis a)
So far it looks like this handles fingering only.  Other similar problems are
handled with a XX_Collision object, so maybe just call this Fingering_Collision
?

http://codereview.appspot.com/6827072/diff/11002/lily/box-quarantine.cc#newco...
lily/box-quarantine.cc:56: the beam cannot start.  it iterates through these
boxes,
Stray comment.  This routine is not used for anything to do with beams.

http://codereview.appspot.com/6827072/diff/11002/lily/box-quarantine.cc#newco...
lily/box-quarantine.cc:70: return fabs (ii0.index_ - mid) < fabs (ii1.index_ -
mid);
C provides subtraction on unsigned integers, so you can do hardware-oriented
things like 0xFFFE - 3 => 0xFFEA  and have the result come out automatically as
an unsigned type.

Here, index_ is unsigned, so the type of the result of the subtraction is
unsigned, so sort_towards_middle() --- which is really only the comparison
function 'closer_to_middle()' --- gives you not the order you intended.  For
example, try
  <c-1 d-2 e-3 f-4 g-5 a-6 b-7>2

However, I think performing the arrangement from the middle out does help in the
algorithm below.  It seems to work just as well without the sort.

http://codereview.appspot.com/6827072/diff/11002/lily/box-quarantine.cc#newco...
lily/box-quarantine.cc:86: scratch.intersect (boxes_to_quarantine_[i + 1]);
If you look only at adjacent boxes, and work from middle out, then with boxes  
A B C D E F G   you might push box B clear beyond A before even looking for an
A-B collision.

Maybe you intended to widen by padding_, otherwise the resulting boxes do not
get moved if they are closer to each other than padding_.

http://codereview.appspot.com/6827072/diff/11002/lily/box-quarantine.cc#newco...
lily/box-quarantine.cc:102: while (dirty);
Can be quite a long loop.  If you have three overlapping boxes A B C, you move
B-A apart to clear, worsening the overlap B-C, then move B-C apart to clear,
restoring most of the A-B overlap.  Convergence is in the style of Xeno's
paradox, sped along somewhat by epsilon and padding.

Additional overlapping boxes D add additional nesting layers to the convergence
process.

http://codereview.appspot.com/6827072/diff/11002/lily/script-engraver.cc
File lily/script-engraver.cc (right):

http://codereview.appspot.com/6827072/diff/11002/lily/script-engraver.cc#newc...
lily/script-engraver.cc:270: // we never want Script grobs to be tucked down
next to stems
The patch changes  'chord-scripts.ly'  in a way inconsistent with this comment
at the moment.

However, I can imagine that in dense music someone would want
'add-stem-support=##f for their Scripts, so that a staccato can tuck in next to
a stem, much like we often do with Fingering in piano music.

[Keith, 2012-11-18 00:00:47]

On 2012/11/16 23:19:16, mike7 wrote:

> On this subject, can someone explain to me how the bounding boxes for the
> regtest images are calculated? It seems like they have something to with the
> extents of non-cross-staff grobs.  This makes certain regtests unduly
croppeed.
> 

This is issue http://code.google.com/p/lilypond/issues/detail?id=2849

Cross-staff objects are excluded from skylines in all steps so far as I can see.
 It seems cross-staff objects never get included in the extents of any
higher-level constructions, so the PNG and EPS bounding boxes would have no
natural way of including them.

After spacing staves and systems on each page, the system skylines could be
re-computed to include cross-staff objects.  (At this point the systems could
potentially be re-spaced to address issue 2801.)

[mike7, 2012-11-18 16:03:24]

On 18 nov. 2012, at 00:19, k-ohara5a5a@oco.net wrote:

> On 2012/11/14 07:12:46, mike7 wrote:
>> > lily/axis-group-interface.cc:403: Axis_group_interface::cross_staff
> (SCM
>> > smob)
>> > For what situation?   Which object that supports
> axis-group-interface
>> > (PianoPedalSpanner, DynamicLineSpanner) should be potentially
> considered
>> > a cross-staff object?
> 
>> NoteColumn
> 
> 
> Of course this leads to the obvious question: Why?
> What is the meaning of marking the entire NoteColumn as "cross-staff" ?
> It seems you already iterated through the elements of a NoteColumn,
> including NoteHeads in skylines but excluding any cross-staff stems.
> 

Try running beam-cross-staff-script.ly with my patch and setting
NoteColumn.cross-staff to ##f.  You'll see that it triggers beam slope
calculations before vertical spacing is done of the staves because NoteColumn,
which is a support for the script, has its height checked prematurely.  This
does not happen in current master because NoteColumn is not a support for
Scripts.

So why did I make NoteColumn a support for Scripts?  I use the NoteColumn as a
side support object because it combines the width of the note head with the
height of the stem.  We don't want UP staccatos, for example, getting tucked
down to the left of UP stems but they will if skylines are used.  So, we use the
NoteColumn as the side support.

Cheers,
MS

[mike7, 2012-11-18 16:11:07]

On 18 nov. 2012, at 00:55, k-ohara5a5a@oco.net wrote:

> I haven't gotten so far to see the main point, yet.
> 

The main point is:

1) Unify all side spacing into one algorithm (this patch).  In addition to
fixing several fingering bugs, this helps...
2) Eventually eliminate the translate axis call used to position outside_staff
grobs, which will then help...
3) Do multiple-pass vertical spacing to help take things like cross-staff slurs
and beams into account.


> 
> http://codereview.appspot.com/6827072/diff/11002/lily/box-quarantine.cc
> File lily/box-quarantine.cc (right):
> 
>
http://codereview.appspot.com/6827072/diff/11002/lily/box-quarantine.cc#newco...
> lily/box-quarantine.cc:24: Box_quarantine::Box_quarantine (Real padding,
> Axis a)
> So far it looks like this handles fingering only.  Other similar
> problems are handled with a XX_Collision object, so maybe just call this
> Fingering_Collision ?

Will do.

> 
>
http://codereview.appspot.com/6827072/diff/11002/lily/box-quarantine.cc#newco...
> lily/box-quarantine.cc:56: the beam cannot start.  it iterates through
> these boxes,
> Stray comment.  This routine is not used for anything to do with beams.
> 

Will remove.

>
http://codereview.appspot.com/6827072/diff/11002/lily/box-quarantine.cc#newco...
> lily/box-quarantine.cc:70: return fabs (ii0.index_ - mid) < fabs
> (ii1.index_ - mid);
> C provides subtraction on unsigned integers, so you can do
> hardware-oriented things like 0xFFFE - 3 => 0xFFEA  and have the result
> come out automatically as an unsigned type.
> 
> Here, index_ is unsigned, so the type of the result of the subtraction
> is unsigned, so sort_towards_middle() --- which is really only the
> comparison function 'closer_to_middle()' --- gives you not the order you
> intended.  For example, try
>  <c-1 d-2 e-3 f-4 g-5 a-6 b-7>2
> 
> However, I think performing the arrangement from the middle out does
> help in the algorithm below.  It seems to work just as well without the
> sort.
> 

This is a bit over my head - do you recommend I just remove the sorting?  If I
do this, then I think the fingerings will fan up or down instead of fanning out
from their middle?  But I could be wrong...

>
http://codereview.appspot.com/6827072/diff/11002/lily/box-quarantine.cc#newco...
> lily/box-quarantine.cc:86: scratch.intersect (boxes_to_quarantine_[i +
> 1]);
> If you look only at adjacent boxes, and work from middle out, then with
> boxes   A B C D E F G   you might push box B clear beyond A before even
> looking for an A-B collision.
> 
> Maybe you intended to widen by padding_, otherwise the resulting boxes
> do not get moved if they are closer to each other than padding_.
> 

I'll have to look at this more closely...basically, I want an algorithm that
looks at how things are grouped and moves them around along one axis so that
there are no more collisions while maintaining the general grouping.  I'm sure
someone who knows something about CS can suggest something more efficient, but
for now, this seems to work for all of the fingering regtests.

>
http://codereview.appspot.com/6827072/diff/11002/lily/box-quarantine.cc#newco...
> lily/box-quarantine.cc:102: while (dirty);
> Can be quite a long loop.  If you have three overlapping boxes A B C,
> you move B-A apart to clear, worsening the overlap B-C, then move B-C
> apart to clear, restoring most of the A-B overlap.  Convergence is in
> the style of Xeno's paradox, sped along somewhat by epsilon and
> padding.
> 
> Additional overlapping boxes D add additional nesting layers to the
> convergence process.

True.  I stand by what I say above - anyone who can see a better way to
implement this can and should!  It's just a first step.

> 
> http://codereview.appspot.com/6827072/diff/11002/lily/script-engraver.cc
> File lily/script-engraver.cc (right):
> 
>
http://codereview.appspot.com/6827072/diff/11002/lily/script-engraver.cc#newc...
> lily/script-engraver.cc:270: // we never want Script grobs to be tucked
> down next to stems
> The patch changes  'chord-scripts.ly'  in a way inconsistent with this
> comment at the moment.
> 
> However, I can imagine that in dense music someone would want
> 'add-stem-support=##f for their Scripts, so that a staccato can tuck in
> next to a stem, much like we often do with Fingering in piano music.
> 

I'm still wondering the best way to implement this is.  For me, the cleanest
thing to do is to either use or not use the NoteColumn as a support (see my
previous response to you on this subject).  When used, there's no tucking, and
when not used, there is.

My schedule is swamped so I'll have to put this patch to the side for 2-ish
weeks, but I appreciate your responses and will try to chip away at it whenever
I have time!

Cheers,
MS

[Keith, 2012-11-29 02:23:38]

http://codereview.appspot.com/6827072/diff/18003/lily/box-quarantine.cc
File lily/box-quarantine.cc (right):

http://codereview.appspot.com/6827072/diff/18003/lily/box-quarantine.cc#newco...
lily/box-quarantine.cc:69: int mid = ii0.mid_;
  assert ((double)(ii0.index - mid) >= 0.0)
  assert ((double)(ii1.index - mid) >= 0.0)

http://codereview.appspot.com/6827072/diff/18003/lily/box-quarantine.cc#newco...
lily/box-quarantine.cc:93: Offset shift (-(epsilon + (ii[0].amount_ + padding_)
/ 2.0), 0.0);
The padding appears in some places but not others; see 'fingering-column.ly'

http://codereview.appspot.com/6827072/diff/18003/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/6827072/diff/18003/lily/skyline.cc#newcode234
lily/skyline.cc:234: // Flatten to height h
What is the difference between this and Skyline:::set_minimum_height() at line
817 ?

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

http://codereview.appspot.com/6827072/diff/18003/scm/define-grob-properties.s...
scm/define-grob-properties.scm:492: (horizon-padding ,number? "The amount to pad
the axis
We already have 'skyline-horizontal-padding' on line 815 and
'skyline-vertical-padding', so it would be better to use those.  You could look
up one or the other depending on which axis, X or Y, in which the buildings
grow.

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

http://codereview.appspot.com/6827072/diff/18003/scm/define-grobs.scm#newcode253
scm/define-grobs.scm:253: (horizon-padding . 0.05)
This could be 'skyline-horizontal-padding' as in System and VerticalAxisGroup.

http://codereview.appspot.com/6827072/diff/18003/scm/define-grobs.scm#newcode886
scm/define-grobs.scm:886: (add-stem-support . #t)
Several regression tests assume this is false by default, and then toggle it to
true, so as to test both cases.

add-stem-support = #f doesn't seem to work very well with beams with this patch.

http://codereview.appspot.com/6827072/diff/18003/scm/define-grobs.scm#newcode909
scm/define-grobs.scm:909: (FingeringCollision
This would need a convert-ly rule.  Why change the name ?

[mike7, 2012-11-29 08:28:31]

On 29 nov. 2012, at 03:23, k-ohara5a5a@oco.net wrote:

> 
> http://codereview.appspot.com/6827072/diff/18003/lily/box-quarantine.cc
> File lily/box-quarantine.cc (right):
> 
>
http://codereview.appspot.com/6827072/diff/18003/lily/box-quarantine.cc#newco...
> lily/box-quarantine.cc:69: int mid = ii0.mid_;
>  assert ((double)(ii0.index - mid) >= 0.0)
>  assert ((double)(ii1.index - mid) >= 0.0)
> 

This will fail often...it is possible, for example, that ii0.index is 0 and mid
is 3 (mid represents the middle index of the vector).

>
http://codereview.appspot.com/6827072/diff/18003/lily/box-quarantine.cc#newco...
> lily/box-quarantine.cc:93: Offset shift (-(epsilon + (ii[0].amount_ +
> padding_) / 2.0), 0.0);
> The padding appears in some places but not others; see
> 'fingering-column.ly'
> 

Fixed.

> http://codereview.appspot.com/6827072/diff/18003/lily/skyline.cc
> File lily/skyline.cc (right):
> 
> http://codereview.appspot.com/6827072/diff/18003/lily/skyline.cc#newcode234
> lily/skyline.cc:234: // Flatten to height h
> What is the difference between this and Skyline:::set_minimum_height()
> at line 817 ?
> 

Set minimum height allows for things over the minimum height to retain their
skyline-ness, whereas this creates a flat skyline at height X.

>
http://codereview.appspot.com/6827072/diff/18003/scm/define-grob-properties.scm
> File scm/define-grob-properties.scm (right):
> 
>
http://codereview.appspot.com/6827072/diff/18003/scm/define-grob-properties.s...
> scm/define-grob-properties.scm:492: (horizon-padding ,number? "The
> amount to pad the axis
> We already have 'skyline-horizontal-padding' on line 815 and
> 'skyline-vertical-padding', so it would be better to use those.  You
> could look up one or the other depending on which axis, X or Y, in which
> the buildings grow.
> 
> http://codereview.appspot.com/6827072/diff/18003/scm/define-grobs.scm
> File scm/define-grobs.scm (right):
> 
>
http://codereview.appspot.com/6827072/diff/18003/scm/define-grobs.scm#newcode253
> scm/define-grobs.scm:253: (horizon-padding . 0.05)
> This could be 'skyline-horizontal-padding' as in System and
> VerticalAxisGroup.
> 

I definitely agree, but this'd be for another patch set.  The eventual goal of
all of this is to get all spacing out of axis-group-interface and into side
position interface via one general algorithm.  When that happens, I'll make
these merges.  For now, it is a bit difficult, as there are two concurrent
systems - one that specifies vertical versus horizontal padding and one that
specifies padding versus horizon-padding.  The latter system (side-position)
probably needs to be changed, but it'd require a major convert-ly rule.

>
http://codereview.appspot.com/6827072/diff/18003/scm/define-grobs.scm#newcode886
> scm/define-grobs.scm:886: (add-stem-support . #t)
> Several regression tests assume this is false by default, and then
> toggle it to true, so as to test both cases.
> 
> add-stem-support = #f doesn't seem to work very well with beams with
> this patch.
> 

One can write a lambda function that returns #t if there is a beam present and
#f otherwise.  Otherwise, the beam would have to be added at the engraver stage.
 I prefer the former.

>
http://codereview.appspot.com/6827072/diff/18003/scm/define-grobs.scm#newcode909
> scm/define-grobs.scm:909: (FingeringCollision
> This would need a convert-ly rule.  Why change the name ?
> 

You recommended that in a previous review. I can push the convert-ly rule as a
separate patch.

> http://codereview.appspot.com/6827072/

[Keith, 2012-11-29 09:13:34]

On 2012/11/29 08:28:31, mike7 wrote:
> On 29 nov. 2012, at 03:23, mailto:k-ohara5a5a@oco.net wrote:
> 
>
http://codereview.appspot.com/6827072/diff/18003/lily/box-quarantine.cc#newco...
> > lily/box-quarantine.cc:69: int mid = ii0.mid_;
> >  assert ((double)(ii0.index - mid) >= 0.0)

> it is possible, for example, that ii0.index is 0 and mid
> is 3 (mid represents the middle index of the vector).

  ii0.index_ = 0;
  mid = 3;
  printf("%f\n",(double)(ii0.index_ - mid));

4294967293.000000


> > add-stem-support = #f doesn't seem to work very well with beams 
> > with this patch.
> 
> One can write a lambda function ...

But I want to set Wilder Reiter (my favorite piece when I was eight, and the
example we discussed where fingerings go alongside stems)
<http://imslp.org/wiki/File:PMLP02707-Schumann_-_Album_für_die_Jugend.pdf>
and I don't know how to write lambda functions.

>
http://codereview.appspot.com/6827072/diff/18003/scm/define-grobs.scm#newcode909
> > scm/define-grobs.scm:909: (FingeringCollision
> > This would need a convert-ly rule.  Why change the name ?
> > 
> 
> You recommended that in a previous review.

Well I must have been suffering from a case of the stupids, because
FingeringColumn was a perfect name for a column of fingering indications.   Now
I'm suffering a case of amnesia trying to think of what I could have meant.

[Keith, 2012-11-29 09:24:43]

It was Box_quarantine that seemed a confusing name. (Why 'quarantine'? Is there
something special about 40 boxes?)  I take back my suggestion.

http://codereview.appspot.com/6827072/diff/7041/lily/box-quarantine.cc
File lily/box-quarantine.cc (right):

http://codereview.appspot.com/6827072/diff/7041/lily/box-quarantine.cc#newcode63
lily/box-quarantine.cc:63: TODO: not sure if this loop causes infinite
behavior...
The while(dirty) loop runs 2366 times for the last chord in
'fingering-collision.ly' but that's an extreme case.

http://codereview.appspot.com/6827072/diff/7041/lily/fingering-column.cc
File lily/fingering-column.cc (left):

http://codereview.appspot.com/6827072/diff/7041/lily/fingering-column.cc#oldc...
lily/fingering-column.cc:63: fingerings[i]->translate_axis
(-fingerings[i]->extent (common[Y_AXIS], Y_AXIS).length () / 2, Y_AXIS);
This shifted all the fingerings down to align them horizontally to their
note-heads, and is missing from the new code

[mike7, 2012-11-29 10:01:26]

On 29 nov. 2012, at 10:24, k-ohara5a5a@oco.net wrote:

> It was Box_quarantine that seemed a confusing name. (Why 'quarantine'?
> Is there something special about 40 boxes?)  I take back my suggestion.
> 
> 
> http://codereview.appspot.com/6827072/diff/7041/lily/box-quarantine.cc
> File lily/box-quarantine.cc (right):
> 
>
http://codereview.appspot.com/6827072/diff/7041/lily/box-quarantine.cc#newcode63
> lily/box-quarantine.cc:63: TODO: not sure if this loop causes infinite
> behavior...
> The while(dirty) loop runs 2366 times for the last chord in
> 'fingering-collision.ly' but that's an extreme case.

I'm not proud of this...

> 
> http://codereview.appspot.com/6827072/diff/7041/lily/fingering-column.cc
> File lily/fingering-column.cc (left):
> 
>
http://codereview.appspot.com/6827072/diff/7041/lily/fingering-column.cc#oldc...
> lily/fingering-column.cc:63: fingerings[i]->translate_axis
> (-fingerings[i]->extent (common[Y_AXIS], Y_AXIS).length () / 2, Y_AXIS);
> This shifted all the fingerings down to align them horizontally to their
> note-heads, and is missing from the new code
> 

Fixed.

> http://codereview.appspot.com/6827072/

[mike7, 2012-11-29 10:37:35]

On 29 nov. 2012, at 10:13, k-ohara5a5a@oco.net wrote:

> On 2012/11/29 08:28:31, mike7 wrote:
>> On 29 nov. 2012, at 03:23, mailto:k-ohara5a5a@oco.net wrote:
> 
> 
>
http://codereview.appspot.com/6827072/diff/18003/lily/box-quarantine.cc#newco...
>> > lily/box-quarantine.cc:69: int mid = ii0.mid_;
>> >  assert ((double)(ii0.index - mid) >= 0.0)
> 
>> it is possible, for example, that ii0.index is 0 and mid
>> is 3 (mid represents the middle index of the vector).
> 
>  ii0.index_ = 0;
>  mid = 3;
>  printf("%f\n",(double)(ii0.index_ - mid));
> 
> 4294967293.000000
> 

woah...totally over my head, but ok, convinced

> 
>> > add-stem-support = #f doesn't seem to work very well with beams
>> > with this patch.
> 
>> One can write a lambda function ...
> 
> But I want to set Wilder Reiter (my favorite piece when I was eight, and
> the example we discussed where fingerings go alongside stems)
> <http://imslp.org/wiki/File:PMLP02707-Schumann_-_Album_für_die_Jugend.pdf>
> and I don't know how to write lambda functions.
> 

I wrote a lambda function. Regest added.

> 
>
http://codereview.appspot.com/6827072/diff/18003/scm/define-grobs.scm#newcode909
>> > scm/define-grobs.scm:909: (FingeringCollision
>> > This would need a convert-ly rule.  Why change the name ?
>> >
> 
>> You recommended that in a previous review.
> 
> Well I must have been suffering from a case of the stupids, because
> FingeringColumn was a perfect name for a column of fingering
> indications.   Now I'm suffering a case of amnesia trying to think of
> what I could have meant.


from days of yore...

http://codereview.appspot.com/6827072/diff/11002/lily/box-quarantine.cc#newco...
lily/box-quarantine.cc:24: Box_quarantine::Box_quarantine (Real padding,
Axis a)
So far it looks like this handles fingering only.  Other similar
problems are handled with a XX_Collision object, so maybe just call this
Fingering_Collision ?

[Keith, 2012-11-30 06:25:56]

'finger-chords.ly' is still in disagreement with its texidoc (therefore
failing).  You could adjust that regtest in light of the new defaults, of
course.

Better might be to make Fingering.add-stem-support = #only-if-beamed  the new
default.  That is in better agreement with common practice.  Regtests come out
just as good.  (Les-neréides.ly can remove a couple "tweaks", which seems to be
how somebody was keeping score.)  Then users will less often want to override
add-stem-support, and when they do it will be to the simpler ##f or ##t

> > The while(dirty) loop runs 2366 times for the last chord in
> > 'fingering-collision.ly' but that's an extreme case.
> >
> I'm not proud of this...

It is at least comprehensible; while the code it replaced was utterly baffling. 
I simplified the loop <http://codereview.appspot.com/6854121/>

You should at least put the filenames back to what they were, and adjust any
tests or documentation or snippets using add-stem support, before pushing.

I started to test with real music.  The usual Chopin test case
<http://www.mutopiaproject.org/cgibin/piece-info.cgi?id=1776> 
has a badly broken cross-staff beam in measure 27.  I don't yet see the cause.

[Keith, 2012-11-30 21:14:48]

>> Ugh...this is a really nasty cyclic dependency.

Then you could use as a test-case 
<http://www.mutopiaproject.org/cgibin/piece-info.cgi?id=1778>
which has no fingerings, no cyclic dependencies, but still shows the problem of
confused beams.

[Keith, 2012-12-05 06:16:58]

https://codereview.appspot.com/6827072/diff/19006/lily/side-position-interfac...
File lily/side-position-interface.cc (right):

https://codereview.appspot.com/6827072/diff/19006/lily/side-position-interfac...
lily/side-position-interface.cc:259: : "vertical-skylines",
Now I see why you were marking the entire NoteColumn as cross-staff, if its Stem
is cross-staff.  

<< \new Staff="up" { g'8 \change Staff="down" g' r4 r2}
   \new Staff="down" { s2\sustainOn s2\sustainOff } >>

After line- and page-breaking, but before the spreading of staves across the
page, we want to estimate the positioning of the pedal spanner.

Comments above indicated that this code would see the NoteHeads, that is, a
layer deeper in the packaging.

[mike7, 2012-12-06 07:34:06]

On 5 déc. 2012, at 07:16, k-ohara5a5a@oco.net wrote:

> 
>
https://codereview.appspot.com/6827072/diff/19006/lily/side-position-interfac...
> File lily/side-position-interface.cc (right):
> 
>
https://codereview.appspot.com/6827072/diff/19006/lily/side-position-interfac...
> lily/side-position-interface.cc:259: : "vertical-skylines",
> Now I see why you were marking the entire NoteColumn as cross-staff, if
> its Stem is cross-staff.
> 
> << \new Staff="up" { g'8 \change Staff="down" g' r4 r2}
>   \new Staff="down" { s2\sustainOn s2\sustainOff } >>
> 
> After line- and page-breaking, but before the spreading of staves across
> the page, we want to estimate the positioning of the pedal spanner.
> 
> Comments above indicated that this code would see the NoteHeads, that
> is, a layer deeper in the packaging.
> 
> https://codereview.appspot.com/6827072/

That's a good example. In general, anytime a NoteColumn is used as a side
positioning object and has a cross-staff stem, it has the potential to cause
problems w my patch. It'll take me a while to iron it out - there are lots of
circular dependencies. I have a chunk of time in June that I can devote to
development, so it may take a while to get this push-ready but it'll get
there...

[mike7, 2012-12-16 07:39:18]

On 30 nov. 2012, at 22:14, k-ohara5a5a@oco.net wrote:

>>> Ugh...this is a really nasty cyclic dependency.
> 
> Then you could use as a test-case
> <http://www.mutopiaproject.org/cgibin/piece-info.cgi?id=1778>
> which has no fingerings, no cyclic dependencies, but still shows the
> problem of confused beams.
> 
> https://codereview.appspot.com/6827072/

It's the OttavaBracket that's causing the problem in mm. 49-50.  The issue is
more or less the same for pedals.  OttavaBracket is side-aligning to cross-staff
grobs (or grobs that contain cross-staff grobs in the elements list) and
triggering Stem::height too early.

Cheers,
MS

[Keith, 2012-12-17 04:02:32]

It doesn't like it when I end hairpins on note columns with stems attached to
cross-staff beams.  I do not know why.

\new PianoStaff <<
  \new Staff = "up" <<
    {\voiceOne e''8. fis''16 g''2 fis''4 }
    {s8\< s s s\! s\> s s s\!} >>
  \new Staff = "down" {
    \clef bass \voiceTwo
    \change Staff = "up"
    <cis' g'>8[
    \change Staff = "down"
    a16
    \change Staff = "up"
    a'16 b'8
    \change Staff = "down"
    a8] a a
    \change Staff = "up"
    <d' a'>8
    \change Staff = "down"
    a } >>

[mike7, 2012-12-17 10:44:23]

On 17 déc. 2012, at 05:02, k-ohara5a5a@oco.net wrote:

> It doesn't like it when I end hairpins on note columns with stems
> attached to cross-staff beams.  I do not know why.
> 

Fixed in the most recent patch-set. It is because
Axis_group_interface::generic_bound_extent did not recurse into the elements
list of cross-staff grobs implementing axis-group-interface.

Cheers,
MS

[Keith, 2012-12-22 06:43:50]

On 2012/12/17 10:44:23, mike7 wrote:

> Fixed in the most recent patch-set. It is because
> Axis_group_interface::generic_bound_extent did not recurse into the elements
> list of cross-staff grobs implementing axis-group-interface.
> 

I haven't tried this yet, but I notice a lot of other places where a
grouping-object is marked 'cross-staff' where we might also need to look at the
'elements' to see which individual graphical-objects are 'cross-staff'

I guess it is logical to flag the grouping-object as 'cross-staff' if it
contains a cross-staff element, but previously 'cross-staff' meant more like "I
will be spanning across one or more staves, so please completely ignore me for
vertical spacing purposes"

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

https://codereview.appspot.com/6827072/diff/34001/lily/axis-group-interface.c...
lily/axis-group-interface.cc:62: return SCM_BOOL_T;
Now the whole note-column is marked cross-staff if its stem spans staves.  Check
if Note_column::cross_staff_extent() still makes sense

https://codereview.appspot.com/6827072/diff/34001/lily/axis-group-interface.c...
lily/axis-group-interface.cc:258: continue;
This might now ignore protruding note-heads under a cross-staff beam when
estimating system heights for page breaking.

https://codereview.appspot.com/6827072/diff/34001/lily/axis-group-interface.c...
lily/axis-group-interface.cc:845: // really depend on position of the
cross-staff grobs that lie between them.
An old comment, but is it backwards? 
Obviously, the position of a cross-staff beam depends on the distance between
the staves that it spans, not the other way around.

[mike7, 2012-12-22 06:50:46]

On 22 déc. 2012, at 07:43, k-ohara5a5a@oco.net wrote:

> 
>
https://codereview.appspot.com/6827072/diff/34001/lily/axis-group-interface.c...
> lily/axis-group-interface.cc:845: // really depend on position of the
> cross-staff grobs that lie between them.
> An old comment, but is it backwards?
> Obviously, the position of a cross-staff beam depends on the distance
> between the staves that it spans, not the other way around.

It is cyclical for cross staff grobs in general.  As staves spread apart
vertically, cross-staff grobs' shapes change, which changes potential
positioning of grobs above/below them, which changes staff spacing, which
changes the shape of cross staff grobs, which...

The work I'm doing now is preparing a way to better estimate all this stuff by
removing calls to translate_axis.

Cheers,
MS

[Keith, 2012-12-22 07:40:01]

On 2012/12/22 06:50:46, mike7 wrote:

> It is cyclical for cross staff grobs in general.  As staves spread apart
> vertically, cross-staff grobs' shapes change, which changes potential
> positioning of grobs above/below them, which changes staff spacing, which
> changes the shape of cross staff grobs, which...
> 
But just to be clear, this is happening in your head only; LilyPond has been
ignoring graphical-objects marked 'cross-staff' for purposes of vertical
spacing.

Generally, the layout is conceptually a large dependency tree (a directed graph)
with each piece of positioning data a separate node.  Several situations (issue
2589 for example) would give a cycle in that graph.  

Whenever LilyPond evaluates a piece of positioning data that is associated with
a grob property that references a Scheme procedure, she sets a flag to break any
cycle if the evaluation of the procedure tries references the positioning data
being calculated.

In some cases cycles are broken by using tentative positioning data instead of
the real data.  The node is split, with one version being called 'pure'
(misleadingly hinting at an analogy to "pure functions") which means roughly
that the 'pure' positioning data depends on a limited set of other nodes.

Sometimes we can divide the layout into stages, where some types of data depend
on a small set of decisions, that depend in turn on a disjoint set of data.  At
line breaking, for example, all minimum horizontal distances between columns are
determined, but no stretching of lines has been done thus no slur shapes have
been set.

Nowhere that I have seen are there iterations to converge on a solution.  
Instead, systems are set up that can be solved, such as the rods-and-springs
system used for both note-spacing and vertical layout.

[mike7, 2012-12-22 07:47:16]

On 22 déc. 2012, at 08:40, k-ohara5a5a@oco.net wrote:

> On 2012/12/22 06:50:46, mike7 wrote:
> 
>> It is cyclical for cross staff grobs in general.  As staves spread
> apart
>> vertically, cross-staff grobs' shapes change, which changes potential
>> positioning of grobs above/below them, which changes staff spacing,
> which
>> changes the shape of cross staff grobs, which...
> 
> But just to be clear, this is happening in your head only;

cross-staff grobs are not the only thing about which one could say to me "that
is happening in your head only"...

> LilyPond has
> been ignoring graphical-objects marked 'cross-staff' for purposes of
> vertical spacing.

True.

> 
> Generally, the layout is conceptually a large dependency tree (a
> directed graph) with each piece of positioning data a separate node.
> Several situations (issue 2589 for example) would give a cycle in that
> graph.

True.

> 
> Whenever LilyPond evaluates a piece of positioning data that is
> associated with a grob property that references a Scheme procedure, she
> sets a flag to break any cycle if the evaluation of the procedure tries
> references the positioning data being calculated.

True.

> 
> In some cases cycles are broken by using tentative positioning data
> instead of the real data.  The node is split, with one version being
> called 'pure' (misleadingly hinting at an analogy to "pure functions")
> which means roughly that the 'pure' positioning data depends on a
> limited set of other nodes.

True.

> 
> Sometimes we can divide the layout into stages, where some types of data
> depend on a small set of decisions, that depend in turn on a disjoint
> set of data.  At line breaking, for example, all minimum horizontal
> distances between columns are determined, but no stretching of lines has
> been done thus no slur shapes have been set.

True.

> 
> Nowhere that I have seen are there iterations to converge on a solution.

There are several loops in LilyPond that iterate while dirty and converge to a
solution for limited spacing problems (beams and fingering come to mind).

I'd like for this to be the case for vertical spacing.  Or at least for certain
pure estimations to be periodically updated so that the spacing algorithm,
during its one pass, uses the most accurate data available.

>  Instead, systems are set up that can be solved, such as the
> rods-and-springs system used for both note-spacing and vertical layout.
> 
> https://codereview.appspot.com/6827072/

[mike7, 2012-12-22 16:11:10]

On 22 déc. 2012, at 07:43, k-ohara5a5a@oco.net wrote:

> 
>
https://codereview.appspot.com/6827072/diff/34001/lily/axis-group-interface.cc
> File lily/axis-group-interface.cc (right):
> 
>
https://codereview.appspot.com/6827072/diff/34001/lily/axis-group-interface.c...
> lily/axis-group-interface.cc:62: return SCM_BOOL_T;
> Now the whole note-column is marked cross-staff if its stem spans
> staves.  Check if Note_column::cross_staff_extent() still makes sense

It does, because this function is a replacement for Grob::extent for
NoteColumns.  Axis_group_interface::height, even in unpure, won't take
cross-staff stems into account.  Here we want to do this, so we use the more
accurate Note_column::cross_staff_extent.  It is a strange exception, tho...it's
worth trying to get rid of it one day.

> 
>
https://codereview.appspot.com/6827072/diff/34001/lily/axis-group-interface.c...
> lily/axis-group-interface.cc:258: continue;
> This might now ignore protruding note-heads under a cross-staff beam
> when estimating system heights for page breaking.
> 

Added a line of code to make sure this doesn't happen.

The regtests checked out clean last time I checked, so if this code
inadvertently causes NoteColumns to be ignored when they shouldn't be
(experience dictates that someone will find something somewhere that doesn't
work), it'll likely be an easy fix.  A git grep of cross-staff confirms this -
there are no circumstances I can see (aside those you've identified) where
NoteColumns' will by unduly excluded because of their cross-staffitude.

Cheers,
MS

[Keith, 2012-12-23 19:47:34]

The changes to side-position-interface.cc are more extensive than I am willing
to figure out.  Good luck.

https://codereview.appspot.com/6827072/diff/38003/lily/box-quarantine.cc
File lily/box-quarantine.cc (right):

https://codereview.appspot.com/6827072/diff/38003/lily/box-quarantine.cc#newc...
lily/box-quarantine.cc:81: Real epsilon = 1.0e-10;
A bit small.  If anyone ever does anything strange,
like overriding one fingering to a point stencil and also setting
FingeringColumn 'padding = 0, then the shift in each iteration is actually
epsilon.  I figure it would take about 3 million seconds to set a 3-finger
chord, so the boxes would be in quarantine for just about forty days.

> > I simplified the loop <http://codereview.appspot.com/6854121/>
>
> I'm OK w/ the simplification, but I'd like it in box-quarantine if possible.

I cannot figure out how to put it in box-quarantine.

https://codereview.appspot.com/6827072/diff/38003/lily/fingering-column.cc
File lily/fingering-column.cc (right):

https://codereview.appspot.com/6827072/diff/38003/lily/fingering-column.cc#ne...
lily/fingering-column.cc:72: origs.push_back(Interval (y_ext));
The array origs[] is not quite the same as the actual fingering positions, after
the translate_axis() above.  The difference is only important if the fingerings
were different heights, but that could happen 
{ \set fingeringOrientations = #'(left)
 <g-3 a-\tweak #'text "---" -0 b-3> }

https://codereview.appspot.com/6827072/diff/38003/lily/skyline.cc
File lily/skyline.cc (right):

https://codereview.appspot.com/6827072/diff/38003/lily/skyline.cc#newcode236
lily/skyline.cc:236: Skyline::flatten_to_height (Real h)
On 2012/11/29 08:28:31, mike7 wrote:

> > http://codereview.appspot.com/6827072/diff/18003/lily/skyline.cc#newcode234
> > lily/skyline.cc:234: // Flatten to height h
> > What is the difference between this and Skyline:::set_minimum_height()
> > at line 817 ?
> > 
> 
> Set minimum height allows for things over the minimum height to retain their
> skyline-ness, whereas this creates a flat skyline at height X.
 
The only places you use it, you could use set_minimum_height(), or just insert()
a box of the height you need, since the skylines in question are empty.  Looking
up unfamiliar functions takes time for code maintainers.

[dak, 2013-01-15 09:58:17]

https://codereview.appspot.com/6827072/diff/38003/scm/output-lib.scm
File scm/output-lib.scm (right):

https://codereview.appspot.com/6827072/diff/38003/scm/output-lib.scm#newcode54
scm/output-lib.scm:54: (define-public (non-event-cause grob)
Mike, this function is total crap.  What is it supposed to do?  Events are
_never_ caused by grobs, so the first branch in the cond is nonsense.  This is
essentially
(let ((cause (ly:grob-property grob 'cause)))
    (and (ly:grob? cause) cause))
and nothing else.  What you presumably meant (but it is hard to guess, given
that you fail to document anything) is
(and (ly:grob? cause) (or (non-event-cause cause) cause))
namely the last grob cause in a row, if any.  Of course,
non-event-cause for that function would then be quite silly,
and last-grob-cause more appropriate.

Can you _please_ make this do anything useful _and_ document what it is supposed
to do?

[dak, 2013-02-19 18:44:03]

https://codereview.appspot.com/6827072/diff/38003/lily/multi-measure-rest.cc
File lily/multi-measure-rest.cc (right):

https://codereview.appspot.com/6827072/diff/38003/lily/multi-measure-rest.cc#...
lily/multi-measure-rest.cc:123: Spanner *sp = dynamic_cast<Spanner *> (me);
The compiler complains that sp is not being used here.  I tend to agree.

[mike7, 2013-02-19 19:02:49]

On 19 févr. 2013, at 20:44, dak@gnu.org wrote:

> 
> https://codereview.appspot.com/6827072/diff/38003/lily/multi-measure-rest.cc
> File lily/multi-measure-rest.cc (right):
> 
>
https://codereview.appspot.com/6827072/diff/38003/lily/multi-measure-rest.cc#...
> lily/multi-measure-rest.cc:123: Spanner *sp = dynamic_cast<Spanner *>
> (me);
> The compiler complains that sp is not being used here.  I tend to agree.
> 
> https://codereview.appspot.com/6827072/

Fluff - sorry for having missed it. Fix pushed to staging.

Cheers,
MS

[Keith, 2013-03-14 05:42:17]

issue 3242

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

https://codereview.appspot.com/6827072/diff/38003/lily/axis-group-interface.c...
lily/axis-group-interface.cc:902: Skyline_pair skylines (inside_staff_skylines);
\relative c'' {
  r2^\f r4 f8^[ r]
}

https://codereview.appspot.com/6827072/diff/38003/lily/stencil-integral.cc
File lily/stencil-integral.cc (right):

https://codereview.appspot.com/6827072/diff/38003/lily/stencil-integral.cc#ne...
lily/stencil-integral.cc:992: xex.set_full ();
Oops.

[mike7, 2013-03-14 05:44:28]

On 14 mars 2013, at 06:42, k-ohara5a5a@oco.net wrote:

> issue 3242
> 
> 
>
https://codereview.appspot.com/6827072/diff/38003/lily/axis-group-interface.cc
> File lily/axis-group-interface.cc (right):
> 
>
https://codereview.appspot.com/6827072/diff/38003/lily/axis-group-interface.c...
> lily/axis-group-interface.cc:902: Skyline_pair skylines
> (inside_staff_skylines);
> \relative c'' {
>  r2^\f r4 f8^[ r]
> }
> 
> https://codereview.appspot.com/6827072/diff/38003/lily/stencil-integral.cc
> File lily/stencil-integral.cc (right):
> 
>
https://codereview.appspot.com/6827072/diff/38003/lily/stencil-integral.cc#ne...
> lily/stencil-integral.cc:992: xex.set_full ();
> Oops.
> 
> https://codereview.appspot.com/6827072/

Yup, just caught this myself, we're on the same page.
This set_full needs to happen for other grobs, but by giving invisible stems a
point stencil instead of an empty stencil, things are fixed.  Running the
regtests.

Cheers,
MS

[mike7, 2013-03-14 10:24:07]

On 14 mars 2013, at 06:44, mike@mikesolomon.org wrote:

> 
> On 14 mars 2013, at 06:42, k-ohara5a5a@oco.net wrote:
> 
>> issue 3242
>> 
>> 
>>
https://codereview.appspot.com/6827072/diff/38003/lily/axis-group-interface.cc
>> File lily/axis-group-interface.cc (right):
>> 
>>
https://codereview.appspot.com/6827072/diff/38003/lily/axis-group-interface.c...
>> lily/axis-group-interface.cc:902: Skyline_pair skylines
>> (inside_staff_skylines);
>> \relative c'' {
>> r2^\f r4 f8^[ r]
>> }
>> 
>> https://codereview.appspot.com/6827072/diff/38003/lily/stencil-integral.cc
>> File lily/stencil-integral.cc (right):
>> 
>>
https://codereview.appspot.com/6827072/diff/38003/lily/stencil-integral.cc#ne...
>> lily/stencil-integral.cc:992: xex.set_full ();
>> Oops.
>> 
>> https://codereview.appspot.com/6827072/
> 
> Yup, just caught this myself, we're on the same page.
> This set_full needs to happen for other grobs, but by giving invisible stems a
point stencil instead of an empty stencil, things are fixed.  Running the
regtests.
> 
> Cheers,
> MS

Another solution is to introduce the idea of "blocking grob."  The comment I put
about SystemStartBracket and BassFigureAlignmentPositioning is because both of
them conceptually need to extend forever in both directions to never be moved
over/under/left/right of another grob.  This could perhaps be done via two
properties:

horizontal-skyline-minimum-height

and

vertical-skyline-minimum-height

Thoughts?

Cheers,
MS

[Keith, 2013-04-20 07:02:09]

https://codereview.appspot.com/6827072/diff/38003/lily/side-position-interfac...
File lily/side-position-interface.cc (right):

https://codereview.appspot.com/6827072/diff/38003/lily/side-position-interfac...
lily/side-position-interface.cc:151: // Commented out because of cross staff
issues
Neil points out

<< \new Staff = "up" \relative c' {
 f8
 \change Staff = "down"
 c e\f %oops
 \change Staff = "up"
 f
 }
 \new Staff = "down" { \clef bass s2 }
>>

[Keith, 2013-05-12 21:22:04]

issue 3363

https://codereview.appspot.com/6827072/diff/38003/lily/side-position-interfac...
File lily/side-position-interface.cc (right):

https://codereview.appspot.com/6827072/diff/38003/lily/side-position-interfac...
lily/side-position-interface.cc:271: pure || cross_staff,
At some point we need the real, as opposed to pure, shape of the object 'e' if
we are to position against it.

[Keith, 2013-08-16 20:25:37]

http://code.google.com/p/lilypond/issues/detail?id=3503

On 2012/11/16 20:32:54, mike7 wrote:
> On 14 nov. 2012, at 07:33, mailto:mike@mikesolomon.org wrote:
> 
>
http://codereview.appspot.com/6827072/diff/1/lily/axis-group-interface.cc#new...
> >> 
> >> lily/axis-group-interface.cc:403: Axis_group_interface::cross_staff (SCM
> >> smob)
> >> For what situation?   Which object that supports axis-group-interface
> >> (PianoPedalSpanner, DynamicLineSpanner) should be potentially considered
> >> a cross-staff object?
> > 
> > NoteColumn
> 
> Hey all,
> 
> One result of my approach is that grobs that were not previously cross staff
> are.  This allows for better positioning with respect to a system, but results
> in collisions with other systems.  Try the following on current master and
this
> patch :
>  [what is now input\regression\dynamics-avoid-cross-staff-stem-2.ly]

This was a side-effect of marking the entire NoteColumn as cross-staff if it
contains a cross-staff-stem.  The DynamicLineSpanner responds by becoming
cross-staff (as has been done for a long time, for spanners normally belonging
to a Voice) and delays placement of the 'f' until after staff-spacing (thus
possibly overwriting the next staff down the page).

Grouping objects like NoteColumn, however, are not usually marked cross-staff if
some contents of the group are cross-staff.  Marking NoteColumn as cross-staff
requires the marking to be ignored in few places 
https://codereview.appspot.com/6827072/diff/38003/lily/axis-group-interface.c...
https://codereview.appspot.com/6827072/diff/38003/lily/axis-group-interface.c...
https://codereview.appspot.com/6827072/diff/38003/lily/axis-group-interface.c...

The 'f' would then avoid the stem, but tuck in alongside and cross beam, as the
beam is not part of the NoteColumn.  Another special case, though, replaces the
skyline of the note and stem with a box
https://codereview.appspot.com/6827072/diff/38003/lily/side-position-interfac...

These changes were all originally added for fingerings, but for fingerings we
had to restore the old 'add-stem-support' mechanism to choose whether slide down
alongside the stem.

> In current master, there is a dynamic-on-beam intersection, and w/ my patch
> there is a dynamic-on-system intersection.  Both of them are bad, but in terms
> of future work on LilyPond, I think the dynamic-on-system is a better
> alternative.  The long-term goal of this work is to get cross-staff grobs into
> vertical calculations,

We can tell dynamics to acknowledge Stems and NoteHeads individually, and become
cross-staff if the stem is, *after* you include cross-staff grobs into vertical
calculations.  Until that time, the old code does a better job of dynamics
overall.

[Keith, 2013-08-16 21:00:16]

https://codereview.appspot.com/6827072/diff/38003/lily/side-position-interfac...
File lily/side-position-interface.cc (right):

https://codereview.appspot.com/6827072/diff/38003/lily/side-position-interfac...
lily/side-position-interface.cc:331: // alignment to cross-staff grobs.
The comment does not help to understand the effect of the code.
The code flattens the skyline to the level that was its maximum height (the
highest point for an up-pointing skyline, lowest for a down-pointer).  

The code has effect in 'dynamics-avoid-cross-staff-stem-2.ly' by replacing the
skyline of a cross-staff stem with a box of the same hight, but as wide as the
note column, moving the 'f' away from the beam as a side-effect.  This
cross-staff grob actually does have a "good height estimation".

An example of a cross-staff grob with a bad height-estimation is in issue 3363,
where a stem position is mis-estimated by the distance between staves, and
flattening the misplaced skyline did not help there.  

The lines below could only help if the cross-staff grob with a bad height
estimate was on an object sitting next to a grob with a good estimate, and
another object is trying to avoid both but could otherwise slide alongside the
grob with the good estimate, and clearing the box happens to be better.  For
example, it keeps the 'f' from sliding alongside the stem in
\new PianoStaff <<
   \new Staff = RH s1
   \new Staff = LH {
     b'4\f\>
     \change Staff = RH b'8 
     \change Staff = LH b'8
     b'2\mf } >>
but that would only help to clear the cross-staff beam by accident.

[Keith, 2013-08-17 07:02:53]

https://codereview.appspot.com/6827072/diff/38003/lily/side-position-interfac...
File lily/side-position-interface.cc (right):

https://codereview.appspot.com/6827072/diff/38003/lily/side-position-interfac...
lily/side-position-interface.cc:194: common[ax] = common_refpoint_of_array
(support,
One recurring problem, present in the old code but appearing more with the new,
is that the common_refpoint of a set of 'support' that crosses staves could be
the overall VerticalAlignment.

When the first attempt to estimate the space required by staves estimates the
position of something with 'support' on multiple staves, the common_refpoint is
the VerticalAlignment, and the call to maybe_pure_coordinate() on line 234 tries
to estimate the position of our staff in the VerticalAlignment, which is what we
were in the middle of doing.
programming error: cyclic dependency: calculation-in-progress encountered for
#'adjacent-pure-heights (VerticalAxisGroup)

[Keith, 2013-08-19 20:31:52]

I'm away for a few days.

https://codereview.appspot.com/6827072/diff/38003/lily/pointer-group-interfac...
File lily/pointer-group-interface.cc (right):

https://codereview.appspot.com/6827072/diff/38003/lily/pointer-group-interfac...
lily/pointer-group-interface.cc:74: // computation time when skylines are
calculated.
Probably we should not recursively add elements numerous times.

https://codereview.appspot.com/6827072/diff/38003/lily/side-position-interfac...
File lily/side-position-interface.cc (right):

https://codereview.appspot.com/6827072/diff/38003/lily/side-position-interfac...
lily/side-position-interface.cc:57:
Side_position_interface::recursive_add_support (Grob *me, Grob *e)
Probably this is for stacking scripts alongside a note, so that if the furthest
can slip around the inner script, it still avoids the note and flag.  It should
not add elements multiple times.

https://codereview.appspot.com/6827072/diff/38003/lily/stencil-integral.cc
File lily/stencil-integral.cc (right):

https://codereview.appspot.com/6827072/diff/38003/lily/stencil-integral.cc#ne...
lily/stencil-integral.cc:975: : me->extent (me, X_AXIS);
maybe_pure_extent

[Keith, 2013-08-22 06:27:45]

On 2012/12/22 16:11:10, mike7 wrote:
> On 22 déc. 2012, at 07:43, mailto:k-ohara5a5a@oco.net wrote:
> 
>
https://codereview.appspot.com/6827072/diff/34001/lily/axis-group-interface.c...
> > lily/axis-group-interface.cc:62: return SCM_BOOL_T;
> > Now the whole note-column is marked cross-staff if its stem spans
> > staves.  Check if Note_column::cross_staff_extent() still makes sense
> 
> It does, because this function is a replacement for Grob::extent for
> NoteColumns.  Axis_group_interface::height, even in unpure, won't take
> cross-staff stems into account.

The function Axis_group_interface::height() does not seem to skip cross-staff
grobs; it calls functions that eventually try to set stem directions, including
those for cross-staff stems.

[Keith, 2013-08-22 06:28:26]

https://codereview.appspot.com/6827072/diff/38003/lily/script-column.cc
File lily/script-column.cc (right):

https://codereview.appspot.com/6827072/diff/38003/lily/script-column.cc#newco...
lily/script-column.cc:159: Side_position_interface::recursive_add_support (g,
last);
Reversing this change doesn't change any regression tests.