Re: Uses single algorithm for side-position spacing. (issue 6827072)

[k-ohara5a5a]

On 2012/12/22 16:11:10, mike7 wrote:

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



https://codereview.appspot.com/6827072/diff/34001/lily/axis-group-interface.cc#newcode62

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


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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[k-ohara5a5a]

I'm away for a few days.


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

https://codereview.appspot.com/6827072/diff/38003/lily/pointer-group-interface.cc#newcode74
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-interface.cc
File lily/side-position-interface.cc (right):

https://codereview.appspot.com/6827072/diff/38003/lily/side-position-interface.cc#newcode57
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#newcode975
lily/stencil-integral.cc:975: : me->extent (me, X_AXIS);
maybe_pure_extent

https://codereview.appspot.com/6827072/

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[k-ohara5a5a]

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

https://codereview.appspot.com/6827072/diff/38003/lily/side-position-interface.cc#newcode194
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)

https://codereview.appspot.com/6827072/

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[k-ohara5a5a]

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:m...@mikesolomon.org wrote:



http://codereview.appspot.com/6827072/diff/1/lily/axis-group-interface.cc#newcode403

>>
>> 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.cc#newcode115
https://codereview.appspot.com/6827072/diff/38003/lily/axis-group-interface.cc#newcode257
https://codereview.appspot.com/6827072/diff/38003/lily/axis-group-interface.cc#newcode896

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

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.

https://codereview.appspot.com/6827072/

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[k-ohara5a5a]

issue 3363


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

https://codereview.appspot.com/6827072/diff/38003/lily/side-position-interface.cc#newcode271
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.

https://codereview.appspot.com/6827072/

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[k-ohara5a5a]

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

https://codereview.appspot.com/6827072/diff/38003/lily/side-position-interface.cc#newcode151
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 }


https://codereview.appspot.com/6827072/

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[m...@mikesolomon.org]

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

> 
> On 14 mars 2013, at 06:42, k-ohara5...@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.cc#newcode902
>> 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#newcode992
>> 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


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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[m...@mikesolomon.org]

On 14 mars 2013, at 06:42, k-ohara5...@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.cc#newcode902
> 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#newcode992
> 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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[k-ohara5a5a]

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.cc#newcode902
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#newcode992
lily/stencil-integral.cc:992: xex.set_full ();
Oops.

https://codereview.appspot.com/6827072/

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[m...@mikesolomon.org]

On 19 févr. 2013, at 20:44, d...@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#newcode123
> lily/multi-measure-rest.cc:123: Spanner *sp = dynamic_cast
> (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



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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[dak]

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#newcode123
lily/multi-measure-rest.cc:123: Spanner *sp = dynamic_cast
(me);
The compiler complains that sp is not being used here.  I tend to agree.

https://codereview.appspot.com/6827072/

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[dak]

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?

https://codereview.appspot.com/6827072/

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[k-ohara5a5a]

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#newcode81
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 



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#newcode72
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)
  }

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.

https://codereview.appspot.com/6827072/

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[m...@mikesolomon.org]

On 22 déc. 2012, at 07:43, k-ohara5...@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.cc#newcode62
> 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.cc#newcode258
> 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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[m...@mikesolomon.org]

On 22 déc. 2012, at 08:40, k-ohara5...@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/


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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[k-ohara5a5a]

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.

https://codereview.appspot.com/6827072/

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[m...@mikesolomon.org]

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

> 
> https://codereview.appspot.com/6827072/diff/34001/lily/axis-group-interface.cc#newcode845
> 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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[k-ohara5a5a]

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.cc#newcode62
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.cc#newcode258
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.cc#newcode845
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.

https://codereview.appspot.com/6827072/

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[m...@mikesolomon.org]

On 17 déc. 2012, at 05:02, k-ohara5...@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


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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[k-ohara5a5a]

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"
8[
\change Staff = "down"
a16
\change Staff = "up"
a'16 b'8
\change Staff = "down"
a8] a a
\change Staff = "up"
8
\change Staff = "down"
a } >>


https://codereview.appspot.com/6827072/

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[m...@mikesolomon.org]

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

>>> Ugh...this is a really nasty cyclic dependency.
> 
> Then you could use as a test-case
> 
> 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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[m...@mikesolomon.org]

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

> 
> https://codereview.appspot.com/6827072/diff/19006/lily/side-position-interface.cc
> File lily/side-position-interface.cc (right):
> 
> https://codereview.appspot.com/6827072/diff/19006/lily/side-position-interface.cc#newcode259
> 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...
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[k-ohara5a5a]

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

https://codereview.appspot.com/6827072/diff/19006/lily/side-position-interface.cc#newcode259
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/

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[m...@mikesolomon.org]

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

>>> Ugh...this is a really nasty cyclic dependency.
> 
> Then you could use as a test-case
> 
> which has no fingerings, no cyclic dependencies, but still shows the
> problem of confused beams.
> 
> https://codereview.appspot.com/6827072/

Good call - I'll do that first before trying to tackle the other problems. In 
general, wherever I find cyclic dependencies, calls to set_property are 
present. Grumble grumble...

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[k-ohara5a5a]

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


Then you could use as a test-case

which has no fingerings, no cyclic dependencies, but still shows the
problem of confused beams.

https://codereview.appspot.com/6827072/

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[m...@mikesolomon.org]

On 30 nov. 2012, at 20:42, Keith OHara  wrote:

> On Fri, 30 Nov 2012 01:37:48 -0800,  wrote:
> 
>> Ugh...this is a really nasty cyclic dependency.
> 
> There is sometimes a cyclic dependency message, but that is not the problem.  
> I'm pretty sure I used \stemUp to determine the stem directions and break 
> this dependency chain.
> 

I think it is still a problem insofar as no patch should knowingly add cyclic 
dependencies to the code base unless it has a surefire followup that'll remove 
them.

>> 1) note_head_x_shift
>> 2) stem direction
>> 3) beam direction
> 
> There is a problem whenever a cross-staff beam /would/ auto-knee, even if the 
> knee is prevented by overridden stem directions.  The beam is drawn as if the 
> note-heads were all on one staff.
> 

I'll be able to slog through all of these around Christmas-ish if not sooner.

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[Keith OHara]

On Fri, 30 Nov 2012 01:37:48 -0800,  wrote:


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


There is sometimes a cyclic dependency message, but that is not the problem.  
I'm pretty sure I used \stemUp to determine the stem directions and break this 
dependency chain.


1) note_head_x_shift
2) stem direction
3) beam direction


There is a problem whenever a cross-staff beam /would/ auto-knee, even if the 
knee is prevented by overridden stem directions.  The beam is drawn as if the 
note-heads were all on one staff.


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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[mike]

On 30 nov. 2012, at 07:25, k-ohara5...@oco.net wrote:

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

I'm OK w/ the simplification, but I'd like it in box-quarantine if possible.  
I'd like to separate the shifting algorithm from the class.  This is part of an 
effort, in general, to simplify and clarify the distinction between positioning 
algorithms and classes that use these algorithms (this is what I was doing w/ 
interval-minefield.cc as well).  Eventually, box-quarantine will be used for 
the ScriptColumn grob.

> 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
> 
> has a badly broken cross-staff beam in measure 27.  I don't yet see the
> cause.
> 

Ugh...this is a really nasty cyclic dependency.  In the old algorithm, when Y 
side positioning was done, X extents weren't necessary because we just assumed 
that things had infinite X extents.  Now with the skylines, both X and Y 
extents are necessary to build the skylines.  The cyclic dependency is with 
beam directions.  The triggering chain is something to the effect of:

1) note_head_x_shift
2) stem direction
3) beam direction
4) pure y coordinates of stems to figure out auto knees
5) pure internal minimum translations via align interface
6) lots of pure heights in the axis group interface
7) pure heights of fingerings depend on new skyline side positioning algorithm
8) this algorithm needs the width of a note_head whose stem is attached to the 
same beam in (3) above, so note_head_x_shift
9) stem direction of the stem attached to that note
10) beam direction

The dependency seems breakable, but it'll require some mental gymnastics to 
figure out how.  I'm on it.

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[k-ohara5a5a]

'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


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

has a badly broken cross-staff beam in measure 27.  I don't yet see the
cause.

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[m...@mikesolomon.org]

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

> On 2012/11/29 08:28:31, mike7 wrote:
>> On 29 nov. 2012, at 03:23, mailto:k-ohara5...@oco.net wrote:
> 
> 
> http://codereview.appspot.com/6827072/diff/18003/lily/box-quarantine.cc#newcode69
>> > 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.00
> 

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[m...@mikesolomon.org]

On 29 nov. 2012, at 10:24, k-ohara5...@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#oldcode63
> 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/


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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[k-ohara5a5a]

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#oldcode63
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

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

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[k-ohara5a5a]

On 2012/11/29 08:28:31, mike7 wrote:

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



http://codereview.appspot.com/6827072/diff/18003/lily/box-quarantine.cc#newcode69

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



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

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.

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[m...@mikesolomon.org]

On 29 nov. 2012, at 03:23, k-ohara5...@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#newcode69
> 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#newcode93
> 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.scm#newcode492
> 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/


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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[k-ohara5a5a]

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#newcode69
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#newcode93
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.scm#newcode492
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 ?

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

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[mike]

On 19 nov. 2012, at 03:46, Keith OHara  wrote:

> On Sun, 18 Nov 2012 12:21:51 -0800, m...@mikesolomon.org 
>  wrote:
> 
>> if (to_boolean (me->get_property ("add-stem-support"))
>>  && Stem::has_interface (e))
>> skyline.set_min_height (e->extent (common_y, _Y_AXIS)[dir]);
>> 
>> That's pseudo-code, but do you get the idea?  Does that seem reasonable?
> 
> Depends on where it goes, and of what 'skyline' is the skyline.
> 

Hey Keith,

This is implemented in my newest patchset.  I'd like it to go on a countdown 
soonish, so if you have time to give it a spin I'd appreciate it!

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[m...@mikesolomon.org]

On 19 nov. 2012, at 03:46, Keith OHara  wrote:

> On Sun, 18 Nov 2012 12:21:51 -0800, m...@mikesolomon.org 
>  wrote:
> 
>> if (to_boolean (me->get_property ("add-stem-support"))
>>   && Stem::has_interface (e))
>> skyline.set_min_height (e->extent (common_y, _Y_AXIS)[dir]);
>> 
>> That's pseudo-code, but do you get the idea?  Does that seem reasonable?
> 
> Depends on where it goes, and of what 'skyline' is the skyline.
> 

Hey Keith,

This is implemented in my newest patchset.  I'd like it to go on a countdown 
soonish, so if you have time to give it a spin I'd appreciate it!

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[Keith OHara]

On Sun, 18 Nov 2012 12:21:51 -0800, m...@mikesolomon.org  
wrote:


if (to_boolean (me->get_property ("add-stem-support"))
   && Stem::has_interface (e))
 skyline.set_min_height (e->extent (common_y, _Y_AXIS)[dir]);

That's pseudo-code, but do you get the idea?  Does that seem reasonable?


Depends on where it goes, and of what 'skyline' is the skyline.


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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[m...@mikesolomon.org]

On 18 nov. 2012, at 21:06, Keith OHara  wrote:

>>> 
>> 
>> 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 on 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.
>> 
> 
> I suppose that, strictly speaking, NoteColumn can be 'cross-staff=#t, meaning 
> "beware; I might move or change shape when staves are spread across the 
> page."  But, it seems to preserve more information if we can leave the 
> 'cross-staff indication specifically on the Stem.
> 
> Scripts clearing Stems is already a special case, controlled by 
> 'add-stem-support, which has in the past meant "keep this Script clear of the 
> end of the Stem and Flag of its parent, even if it could slide alongside."  
> Maybe 'add-stem-support could be implemented as "Consider the Script widened 
> enough to include a point on the extended line of the Stem, and keep the 
> widened Script clear of the Stem."
> 

I'm picking up what you're putting down...
What'd be easy is to add something to the effect of :

if (to_boolean (me->get_property ("add-stem-support"))
   && Stem::has_interface (e))
 skyline.set_min_height (e->extent (common_y, _Y_AXIS)[dir]);

That's pseudo-code, but do you get the idea?  Does that seem reasonable?  If 
so, I can get rid of all of that note column business in the engravers and then 
note column would no longer have to be cross staff.

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[Keith OHara]

On Sun, 18 Nov 2012 08:10:49 -0800, m...@mikesolomon.org  
wrote:


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


http://codereview.appspot.com/6827072/diff/11002/lily/box-quarantine.cc#newcode70
lily/box-quarantine.cc:70: return fabs (ii0.index_ - mid) < fabs
(ii1.index_ - mid);


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?


The calls you make to translate() shift boxes in pairs by equal amounts in 
opposite directions, so the groups fan from their centers with any order of 
opening the vanes in the fan.
(Strangely, though, all the fingerings involved in any collision shift up a 
half space.)


http://codereview.appspot.com/6827072/diff/11002/lily/box-quarantine.cc#newcode86
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.


Simple stacking of the boxes from the middle out (one pass) would probably be 
good enough.  The Dot_Collision solves a similar problem, though it is more 
complex than you need here.  There is also the rods-and-springs code that is 
used in several places: successive Fingerings would be linked by springs with 
natural length equal to the spacing between their parent note-heads, and rods 
long enough to clear collisions.

The patch as it is shows some unevenness in 'fingering-column.ly', and would be 
challenging to understand by anyone who wants to fix that.


http://codereview.appspot.com/6827072/diff/11002/lily/script-engraver.cc#newcode270
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.



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.



In your patch, though, NoteColumn is used as 'support' for Scripts, yet the 
accents in 'chord-scripts.ly' do tuck down alongside the stems.  Something is 
not working as intended.



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



I suppose that, strictly speaking, NoteColumn can be 'cross-staff=#t, meaning 
"beware; I might move or change shape when staves are spread across the page."  
But, it seems to preserve more information if we can leave the 'cross-staff indication 
specifically on the Stem.

Scripts clearing Stems is already a special case, controlled by 'add-stem-support, which has in the 
past meant "keep this Script clear of the end of the Stem and Flag of its parent, even if it 
could slide alongside."  Maybe 'add-stem-support could be implemented as "Consider the 
Script widened enough to include a point on the extended line of the Stem, and keep the widened 
Script clear of the Stem."


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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[m...@mikesolomon.org]

On 18 nov. 2012, at 00:55, k-ohara5...@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#newcode24
> 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#newcode56
> 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#newcode70
> 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
>  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#newcode86
> 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#newcode102
> 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#newcode270
> 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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[m...@mikesolomon.org]

On 18 nov. 2012, at 00:19, k-ohara5...@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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[k-ohara5a5a]

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

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

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[k-ohara5a5a]

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#newcode24
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#newcode56
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#newcode70
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
  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#newcode86
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#newcode102
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#newcode270
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.

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

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[k-ohara5a5a]

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.

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

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[m...@mikesolomon.org]

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

> 
> On 14 nov. 2012, at 07:33, m...@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#newcode403
>>> 
>>> 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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[m...@mikesolomon.org]

On 14 nov. 2012, at 07:33, m...@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#newcode403
>> 
>> 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
   8 [ 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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[mike]

> Re: Uses single algorithm for side-position spacing. (issue 6827072)
> 
> {\clef bass
>  2..->
>  << r16 \\ r \\ r  \\ r \\ >> eeses'16
>  \set fingeringOrientations = #'(right)
>   8-1-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-cross-staff-stem.ly
> 
> File input/regression/dynamics-avoid-cross-staff-stem.ly (right):
> 
> 
> http://codereview.appspot.com/6827072/diff/1/input/regression/dynamics-avoid-cross-staff-stem.ly#newcode14
> 
> 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#newcode403
> 
> 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#newcode668
> 
> 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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[k-ohara5a5a]

{\clef bass
 2..->
 << r16 \\ r \\ r  \\ r \\ >> eeses'16
 \set fingeringOrientations = #'(right)
  8-1-4   r
  r2 }




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

http://codereview.appspot.com/6827072/diff/1/input/regression/dynamics-avoid-cross-staff-stem.ly#newcode14
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#newcode403
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#newcode668
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.

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

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[m...@mikesolomon.org]

On 11 nov. 2012, at 20:57, d...@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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[dak]

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?

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

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[dak]

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.

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

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

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[m...@mikesolomon.org]

On 11 nov. 2012, at 14:50, d...@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#newcode85
> 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#newcode88
> 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#newcode101
> 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#newcode108
> 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#newcode115
> 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#newcode144
> 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#newcode275
> 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#newcode309
> 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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Uses single algorithm for side-position spacing. (issue 6827072)

[dak]

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#newcode85
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#newcode88
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#newcode101
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#newcode108
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#newcode115
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#newcode144
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#newcode275
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#newcode309
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?

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

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