Reported by Mike Solomon: http://lists.gnu.org/archive/html/bug-lilypond/2012-05/msg00064.html

\relative c' { \set fingeringOrientations = #'(right) <a-5 c-3>8 }

Results in flag-on-number and number-on-number collisions.

Adds Flag to Fingering side support list

http://codereview.appspot.com/6201079

The new patch exposes an area of the code base where the new skyline work will be key. Treating flags and stems as all-or-nothing objects pushes fingerings away too much, but if we used their vertical and horizontal skylines, the fingerings would only shift enough to avoid the part that they'd actually intersect with. So this patch would be a sort of stop gate solution before the skyline work.

I'm ambivalent about the patch. Normally I'd be for non-collision over collision, but this results in much wider default spacing. I'd be interested what pianists have to say. Specifically, la raison d'être of the patch comes from perusing Phil Hézaine's work on Schumann w/ fingerings.

What would Elaine Gould do?

Elaine Gould would beat sense into the engraver given either choice. But then this is easier to do given human engravers.

The flag side is the last resort for fingering, so we only need do it when the music is very tight. Moving the numbers away from the stem would make them apply to the next chord. When it gets this tight I whiteout.

Specifically, la raison d'être of the patch comes from perusing Phil Hézaine's work on Schumann Could you be a bit more specific? In Album for the Young, the music does not get tight enough to put fingerings on the side.

Do you really want the patchy run for this ?

I've already run the regtests - it results in a few changed files.

Check out "Le cavalier sauvage" in http://superbonus.project.free.fr/IMG/pdf/Schumann-Album-pour-la-Jeunesse-avec-doigtes.pdf, left hand.

I'd be interested to see this work run through the patch to see if it improves the result. The downward pointing flagged stems are fine but the upward pointing flagged stems look meh with the fingering collisions.

Album for the Young is in German, Mike. You mean 8. Wilder Reiter (I looked first at ReiterStück)

We raise these: \relative c' { 8-1-4-"ja" _"nein" }

When you say "we raise these", do you mean "conventional engraving dictates that these be raised" or "these should be raised in Phil's version" or both?

LilyPond drivers raise the fingering (add-stem-support does it if you put them in chords).

This was actually my favorite piece when I was eight, and I still have the music I learned it from. Human engravers move the fingering a bit off-center, it seems, so the fingering is not quite centered over the head, if that helps to clear the stem.

imslp.org has the B&H edition, so you can look and tell me if I'm imagining the shift.

Patchy tests pass. Reg test diffs (significant ones) attached.

They look fine mostly, although it might be subjective if 'finger chords' and 'repetition chords' are now too 'high'.

Gould says to place the fingerings above or below the notes, to the left or even to the right. So she wouldn't generally put them where they could collide with the flag. Given that, moving them "too far" away in extremis would seem OK.

Comment deleted

Version 2.14 had an option add-stem-support that controlled this behavior. I tried searching mutopiaproject, but found only me using the option.

\set fingeringOrientations = #'(right) \override Fingering #'add-stem-support = ##t 8-1-4 r

The patch (1) restores its effectiveness on Flags, and (2) makes add-stem-support the default.

I suggest leaving the default add-stem-support off. (Note that docs and reg-test 'finger-chords' would need changes if the default changes.)

Adds Flag to Fingering side support list

http://codereview.appspot.com/6201079

Patchy the autobot says: LGTM.

The latest patch restores the behavior of 2.14.
When the Flag was made separate from the Stem in LilyPond internals, the add-stem-support option no longer made scripts clear flags, so there was no easy way to make fingerings clear flags in several 2.15.x

I think this feature was an unintentional side effect, but we seem happy to get it back.

I think there is something wrong with the patch (or its characterization). If you take a look at comment #10, you'll find that most of the visual differences appear with quarter notes which don't even have a flag.

It does not particularly help that apparently this has already been pushed without waiting for the end of countdown as cfe2e355b0630322f7d2f43934243af63844268f if I am not mistaken.

Comment 10 showed the effects of an earlier patch-set (which turned add-stem-support option on by default).

The pushed patch looks like the last one reviewed, which restores version 2.14 behavior.

Ah, ok. Sorry for the confusion.

Comment deleted

The original example (and the regression test) needs an override in order for fingering to avoid the flag.
\relative c' { \override Fingering #'add-stem-support \set fingeringOrientations = #'(right) 8 } Fingerings still collide with each other.

I think you can push this override directly to staging.

Regtest combined with 'finger-chords.ly'.

Could somebody confirm if this has been resolved?

Looking at it now, Mike.

With lilypond-2.16.0 on x86_64 GNU/Linux the output looks very similar to the PNG in the initial entry of this issue tracker. Source and output attached.

Sorry, here's the bug.ly source for my last comment, Comment #28.

As Keith suggested in Comment #23 I tried:

\override Fingering #'add-stem-support % syntax error, my ignorance on that one

\override Fingering #'add-stem-support = ##t % no fingering at all in output

\override Fingering #'add-stem-support = ##f % same output as my Comment #28.

Cheers, Colin.

Thanks Colin! I'll have a look at it next week.

Uses single algorithm for side-position spacing.

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

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

http://codereview.appspot.com/6827072

Patchy the autobot says: passes tests. Reg tests here: https://www.yousendit.com/download/WUJaOGNVdVVRWUp1a3NUQw

Per Keith's comments on Rietveld, this is not ready for countdown.

Uses single algorithm for side-position spacing.

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

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

http://codereview.appspot.com/6827072

Patchy the autobot says: passes tests. Reg tests here: https://www.yousendit.com/download/WUJiZGVWeWE5RmFKUmNUQw

Per Kieth's comments on Rietveld, this needs further work.

Uses single algorithm for side-position spacing.

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

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

http://codereview.appspot.com/6827072

Patchy the autobot says: passes tests. Reg tests here: https://www.yousendit.com/download/WUJZT2pKbWdOMUJMWE1UQw

In 'finger-chords.ly' fingering -5 clears the flag, which is nice, but...

Breaks 'finger-chords.ly' "possible to add horizontal fingerings to notes" -- no longer lined-up horizontally "Fingering clears ... if @code{'add-stem-support} is set." -- now always in effect

Breaks 'fingering-column.ly' "Non-intersecting Fingerings are left alone" --they are shifted half-space up the tall column of six finger-numbers becomes un-evenly spaced

Uses single algorithm for side-position spacing.

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

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

http://codereview.appspot.com/6827072

Uses single algorithm for side-position spacing.

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

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

http://codereview.appspot.com/6827072

Patchy the autobot says: passes tests. Reg tests here:

https://www.yousendit.com/download/WUJaVWRqY1N3TGgzZU1UQw

Counted down to 20121203, but I'm not sure if Keith's comment in #21 on Rietveld was addressed. If so, please push; if not, please mark as "needs-work".

Per Mike's comment on Rietveld.

Uses single algorithm for side-position spacing.

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

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

http://codereview.appspot.com/6827072

Uses single algorithm for side-position spacing.

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

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

http://codereview.appspot.com/6827072

The most recent patchset makes NoteColumn cross-staff when it contains a cross-staff stem. Using this model, most (if not all) problems from the previous patch-sets are cleared up.

Sorry for the commented-out printf's - they'll disappear with the final push.

Patchy the autobot says: passes tests. Reg Tests: https://www.yousendit.com/download/WUJZblFJQTZEbUlPd3NUQw

Uses single algorithm for side-position spacing.

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

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

http://codereview.appspot.com/6827072

Patchy the autobot says: passes tests. Reg Tests: https://www.yousendit.com/download/WUJiZm1lcTJQb0xtcXNUQw

Uses single algorithm for side-position spacing.

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

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

http://codereview.appspot.com/6827072

Patchy the autobot says: passes make, make test and a full make doc. Reg test diffs here: https://www.yousendit.com/download/WUJaQndEVEhoeVpWeHNUQw

Counted down to 20121230, please push.

Pushed as 7d3d28de0ce6e2f018aff599cecd944d1754fe3c.

The finger-flag collision was fixed for version 2.16, if we tell Fingerings to clear Stem:

\relative c' { \override Fingering #'add-stem-support = ##t \set fingeringOrientations = #'(right) 8 }

The later patch, in version 2.17.10, staggers the fingering and requires no override, but causes issue 3363 issue 3109 issue 3242 issue 3327 issue 3363 issue 3465

Ok, by now I am for reverting. I just found the following in the patch: +// If the grob array is unordered, we assume that duplicates should +// be removed. This makes sense for things like side-position-elements, +// which may be added recursively numerous times and thus will eat up +// computation time when skylines are calculated. +// If the array is ordered, then we don't remove duplicates. + void Pointer_group_interface::add_grob (Grob *me, SCM sym, Grob *p) { Grob_array *arr = get_grob_array (me, sym); arr->add (p); + if (!arr->ordered ()) + arr->remove_duplicates (); }

void @@ -81,6 +89,7 @@ Pointer_group_interface::add_unordered_grob (Grob *me, SCM sym Grob_array *arr = get_grob_array (me, sym); arr->add (p); arr->set_ordered (false); + arr->remove_duplicates (); }

What does this mean? It means that remove_duplicates is called for every grob added with these functions, meaning O(n^2 log(n)) computational complexity.

In particular with things like \RemoveEmptyStaves, this makes LilyPond unsuitable for large scores.

This patch needs to be done properly, split into sensible and independent commits, and reviewed carefully. All this has not been done and we are all at fault for it. Apart from all the bad other side effects we've seen so far, this performance problem resulting from a mismatch of the used data structures with the desired behavior is just totally inacceptable for a stable release.

LilyPond has to remain usable for things like an opera or symphony.

To put some numbers to it (and this includes the ps2pdf conversion): one = \new Staff \repeat unfold 8192 two = \new Staff \with { \RemoveEmptyStaves } \repeat unfold 2048 R1 \score { \new StaffGroup << \one \two \one \two \one \two \one \two \one \two >> }

% time before 7d3d28de0ce6e2f018aff599cecd944d1754fe3c: %real 6m33.478s %user 6m24.216s %sys 0m6.676s

% time after 7d3d28de0ce6e2f018aff599cecd944d1754fe3c: %real 7m51.206s %user 7m35.736s %sys 0m8.196s

Now that's only a single iteration, so it's not sure that there is no other reason for variation, but it still looks ugly.

Removing just those changes noted in comment 61 does not change the compilation times of the example in comment 62. (That is, the time LilyPond takes using the current master codebase is the same as the time she takes after removing the changes noted in comment 61.)

Removing those changes does not change any regression tests, either, so the remove_duplicates() can be removed. The only obvious reason for adding remove_duplicates() was an added function recursive_add_support(), but this also can be removed without changing any tests.

Ok, go for it.

I remove the recursive_add_support() with the patch for issue 3503, and patchy confirmed my findings of not change.

I did not at the time remove remove_duplicates() but since then I've found that backing out that change gives some (3%) speed-up on an orchestral score master avoid remove_duplicates real 3m3.004s 2m55.687s user 2m53.331s 2m47.076s sys 0m1.172s 0m1.198s and I saw that after issue 3365 a remove_duplicates() means we sort, remove, and then un-sort the array, which is rather ugly. I'll review backing out remove_duplicates() with issue 3503.