Menu
▾
▴
1 Attachments
#2527 Finger-flag collision
Originally created by: *anonymous
Originally created by: ma...@gregoriana.sk
Originally owned by: mts...@gmail.com
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.
Originally posted by: ColinPKC...@gmail.com
(No comment was entered for this change.)
Labels: -Patch-review Patch-countdown
Originally posted by: mts...@gmail.com
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
Labels: -Patch-countdown Patch-new
Originally posted by: pkx1...@gmail.com
Patchy the autobot says: passes make, make test and a full make doc. Reg test diffs here: https://www.yousendit.com/download/WUJaQndEVEhoeVpWeHNUQw
Labels: -Patch-new Patch-review
Originally posted by: ColinPKC...@gmail.com
(No comment was entered for this change.)
Labels: -Patch-review Patch-countdown
Originally posted by: ColinPKC...@gmail.com
Counted down to 20121230, please push.
Labels: -Patch-countdown Patch-push
Originally posted by: mts...@gmail.com
Pushed as [r7d3d28de0ce6e2f018aff599cecd944d1754fe3c].
Labels: Fixed_2_17_10
Status: Fixed
Originally posted by: Elu...@gmail.com
(No comment was entered for this change.)
Status: Verified
Originally posted by: k-ohara5...@oco.net
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)
<a-5 c-3>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
Related
Issues:
#3109Issues:
#3242Issues:
#3327Issues:
#3363Issues:
#3465Originally posted by: dak@gnu.org
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.
Cc: k-ohara5...@oco.net
Originally posted by: dak@gnu.org
To put some numbers to it (and this includes the ps2pdf conversion):
one = \new Staff \repeat unfold 8192 <c e g>
two = \new Staff \with { \RemoveEmptyStaves } \repeat unfold 2048 [r1]
\score { \new StaffGroup << \one \two \one \two \one \two \one \two \one \two >> }
% time before [r7d3d28de0ce6e2f018aff599cecd944d1754fe3c]:
%real 6m33.478s
%user 6m24.216s
%sys 0m6.676s
% time after [r7d3d28de0ce6e2f018aff599cecd944d1754fe3c]:
%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.
Originally posted by: k-ohara5...@oco.net
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.
Cc: -k-ohara5...@oco.net
Originally posted by: mts...@gmail.com
Ok, go for it.
Originally posted by: k-ohara5...@oco.net
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.
Related
Issues:
#3365Issues:
#3503.