[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Calculate skylines only once. (issue 553980043 by address@hidden)
|
From: |
hanwenn |
|
Subject: |
Re: Calculate skylines only once. (issue 553980043 by address@hidden) |
|
Date: |
Fri, 01 May 2020 12:54:18 -0700 |
Reviewers: lemzwerg,
Message:
no, it's in this commit. I actually folded in
https://codereview.appspot.com/555770044/ , but maybe it will be
submitted separately.
https://codereview.appspot.com/553980043/diff/549960044/lily/figured-bass-position-engraver.cc
File lily/figured-bass-position-engraver.cc (right):
https://codereview.appspot.com/553980043/diff/549960044/lily/figured-bass-position-engraver.cc#newcode99
lily/figured-bass-position-engraver.cc:99: support_.push_back (info.grob
());
here
https://codereview.appspot.com/553980043/diff/549960044/scm/define-grobs.scm
File scm/define-grobs.scm (right):
https://codereview.appspot.com/553980043/diff/549960044/scm/define-grobs.scm#newcode319
scm/define-grobs.scm:319: (add-stem-support . #t)
and here
Description:
Calculate skylines only once.
Before, Axis_group_interface::skyline_spacing() would call the
function add_interior_skylines(), which recursed into
VerticalAxisGroups. This caused staff-skylines to be computed twice:
once as part of the Staff's VerticalAxisGroup, and once to compute
the skyline for System.
Instead, in Axis_group_interface::skyline_spacing(), compute the
vertical-skylines for all constituent elements. Since the property is
subject to caching, the Staff skyline is only computed once.
To make this work
* Add flags to the NoteColumn using Axis_group_interface::add_element
* add vertical-skylines callbacks for NoteColumn and
NoteCollision, which are also X,Y Axis groups.
* declare add-stem-support for bass figures (or the digits are meshed
in with stems that stick out of the staff.)
* calculate a skyline for BassFigureAlignment, otherwise, the skyline
is computed from the extent of the alignment, which is inaccurate if
some bass figures have accidentals.
Formatting impact:
* ottava-edge.ly - the ottava bracket meshes better with the stem,
leading to tighter spacing.
Timing impact
ac49229cdf - Calculate skylines only once.
baseline: eaf40071f5 Use vectors rather than lists for skylines.
args: -I carver MSDM
memory: med diff 1916 (stddevs 103 135, n=5)
memory: med diff 0.2 % (ac49229cdf is fatter)
time: med diff -0.37 (stddevs 0.08 0.14, n=5)
time: med diff -0.8 % (ac49229cdf is faster)
Please review this at https://codereview.appspot.com/553980043/
Affected files (+43, -34 lines):
M lily/axis-group-interface.cc
M lily/figured-bass-position-engraver.cc
M lily/rhythmic-column-engraver.cc
M scm/define-grobs.scm
Index: lily/axis-group-interface.cc
diff --git a/lily/axis-group-interface.cc b/lily/axis-group-interface.cc
index
68453459f37eade1aef3e08e9d24835647e697dd..9a4a47067cb8d04d140dac3b838aa0d3b09fb7d8
100644
--- a/lily/axis-group-interface.cc
+++ b/lily/axis-group-interface.cc
@@ -655,28 +655,6 @@ pure_staff_priority_less (Grob *const &g1, Grob *const &g2)
return priority_1 < priority_2;
}
-static void
-add_interior_skylines (Grob *me, Grob *x_common, Grob *y_common,
vector<Skyline_pair> *skylines)
-{
- if (Grob_array *elements = unsmob<Grob_array> (get_object (me, "elements")))
- {
- for (vsize i = 0; i < elements->size (); i++)
- add_interior_skylines (elements->grob (i), x_common, y_common,
skylines);
- }
- else if (!scm_is_number (get_property (me, "outside-staff-priority"))
- && !to_boolean (get_property (me, "cross-staff")))
- {
- Skyline_pair *maybe_pair = unsmob<Skyline_pair> (get_property (me,
"vertical-skylines"));
- if (!maybe_pair)
- return;
- if (maybe_pair->is_empty ())
- return;
- skylines->push_back (Skyline_pair (*maybe_pair));
- skylines->back ().shift (me->relative_coordinate (x_common, X_AXIS));
- skylines->back ().raise (me->relative_coordinate (y_common, Y_AXIS));
- }
-}
-
// Raises the grob elt (whose skylines are given by h_skyline
// and v_skyline) so that it doesn't intersect with staff_skyline,
// or with anything in other_h_skylines and other_v_skylines.
@@ -880,7 +858,12 @@ Axis_group_interface::outside_staff_ancestor (Grob *me)
Skyline_pair
Axis_group_interface::skyline_spacing (Grob *me)
{
- extract_grob_set (me, unsmob<Grob_array> (get_object (me,
"vertical-skyline-elements")) ? "vertical-skyline-elements" : "elements",
fakeelements);
+ extract_grob_set (
+ me,
+ unsmob<Grob_array> (get_object (me, "vertical-skyline-elements"))
+ ? "vertical-skyline-elements"
+ : "elements",
+ fakeelements);
vector<Grob *> elements (fakeelements);
for (vsize i = 0; i < elements.size (); i++)
/*
@@ -922,15 +905,28 @@ Axis_group_interface::skyline_spacing (Grob *me)
vsize i = 0;
vector<Skyline_pair> inside_staff_skylines;
- for (i = 0; i < elements.size ()
- && !scm_is_number (get_property (elements[i],
"outside-staff-priority")); i++)
+ for (i = 0;
+ i < elements.size ()
+ && !scm_is_number (get_property (elements[i],
"outside-staff-priority"));
+ i++)
{
Grob *elt = elements[i];
- Grob *ancestor = outside_staff_ancestor (elt);
- if (!(to_boolean (get_property (elt, "cross-staff")) || ancestor))
- add_interior_skylines (elt, x_common, y_common,
&inside_staff_skylines);
- if (ancestor)
+ if (Grob *ancestor = outside_staff_ancestor (elt))
riders.insert (pair<Grob *, Grob *> (ancestor, elt));
+ else if (!to_boolean (get_property (elt, "cross-staff")))
+ {
+ Skyline_pair *maybe_pair
+ = unsmob<Skyline_pair> (get_property (elt, "vertical-skylines"));
+ if (!maybe_pair)
+ continue;
+ if (maybe_pair->is_empty ())
+ continue;
+ inside_staff_skylines.push_back (Skyline_pair (*maybe_pair));
+ inside_staff_skylines.back ().shift (
+ me->relative_coordinate (x_common, X_AXIS));
+ inside_staff_skylines.back ().raise (
+ me->relative_coordinate (y_common, Y_AXIS));
+ }
}
Skyline_pair skylines (inside_staff_skylines);
Index: lily/figured-bass-position-engraver.cc
diff --git a/lily/figured-bass-position-engraver.cc
b/lily/figured-bass-position-engraver.cc
index
0fcca7216642bc859f28dfc23d975259b76a8031..1156a1bcc4c1227c33c091748329793534feff4b
100644
--- a/lily/figured-bass-position-engraver.cc
+++ b/lily/figured-bass-position-engraver.cc
@@ -40,6 +40,7 @@ class Figured_bass_position_engraver : public Engraver
protected:
void acknowledge_note_column (Grob_info);
void acknowledge_slur (Grob_info);
+ void acknowledge_stem (Grob_info);
void acknowledge_end_slur (Grob_info);
void acknowledge_end_tie (Grob_info);
void acknowledge_bass_figure_alignment (Grob_info);
@@ -92,6 +93,12 @@ Figured_bass_position_engraver::acknowledge_note_column
(Grob_info info)
support_.push_back (info.grob ());
}
+void
+Figured_bass_position_engraver::acknowledge_stem (Grob_info info)
+{
+ support_.push_back (info.grob ());
+}
+
void
Figured_bass_position_engraver::acknowledge_end_slur (Grob_info info)
{
@@ -146,6 +153,7 @@ Figured_bass_position_engraver::boot ()
{
ADD_ACKNOWLEDGER (Figured_bass_position_engraver, note_column);
ADD_ACKNOWLEDGER (Figured_bass_position_engraver, slur);
+ ADD_ACKNOWLEDGER (Figured_bass_position_engraver, stem);
ADD_END_ACKNOWLEDGER (Figured_bass_position_engraver, slur);
ADD_END_ACKNOWLEDGER (Figured_bass_position_engraver, tie);
ADD_ACKNOWLEDGER (Figured_bass_position_engraver, bass_figure_alignment);
Index: lily/rhythmic-column-engraver.cc
diff --git a/lily/rhythmic-column-engraver.cc b/lily/rhythmic-column-engraver.cc
index
189c99d551d42ad121c0a430e1d254826553c2b3..f4e5b12daefe3b8daee5a5cd75a11ea44d923b51
100644
--- a/lily/rhythmic-column-engraver.cc
+++ b/lily/rhythmic-column-engraver.cc
@@ -17,13 +17,14 @@
along with LilyPond. If not, see <http://www.gnu.org/licenses/>.
*/
+#include "axis-group-interface.hh"
+#include "dot-column.hh"
#include "engraver.hh"
-#include "rhythmic-head.hh"
-#include "stem.hh"
-#include "note-column.hh"
#include "item.hh"
-#include "dot-column.hh"
+#include "note-column.hh"
#include "pointer-group-interface.hh"
+#include "rhythmic-head.hh"
+#include "stem.hh"
#include "translator.icc"
@@ -105,7 +106,7 @@ Rhythmic_column_engraver::process_acknowledged ()
if (flag_)
{
- Pointer_group_interface::add_grob (note_column_, ly_symbol2scm
("elements"), flag_);
+ Axis_group_interface::add_element (note_column_, flag_);
flag_ = 0;
}
}
Index: scm/define-grobs.scm
diff --git a/scm/define-grobs.scm b/scm/define-grobs.scm
index
391cbafe930ba9ef9f730b581b9ffc6dead2ee17..25fd272eba75884b4384451df1756d880d772d97
100644
--- a/scm/define-grobs.scm
+++ b/scm/define-grobs.scm
@@ -302,6 +302,7 @@
(stacking-dir . ,DOWN)
(X-extent . ,ly:axis-group-interface::width)
(Y-extent . ,axis-group-interface::height)
+ (vertical-skylines . ,ly:axis-group-interface::calc-skylines)
(meta . ((class . Spanner)
(object-callbacks . ((pure-Y-common .
,ly:axis-group-interface::calc-pure-y-common)
(pure-relevant-grobs .
,ly:axis-group-interface::calc-pure-relevant-grobs)))
@@ -315,6 +316,7 @@
(direction . ,UP)
(padding . 0.5)
(side-axis . ,Y)
+ (add-stem-support . #t)
(staff-padding . 1.0)
(X-extent . ,ly:axis-group-interface::width)
(Y-extent . ,axis-group-interface::height)
@@ -1680,6 +1682,7 @@
(prefer-dotted-right . #t)
(X-extent . ,ly:axis-group-interface::width)
(Y-extent . ,axis-group-interface::height)
+ (vertical-skylines . ,ly:axis-group-interface::calc-skylines)
(meta . ((class . Item)
(object-callbacks . ((pure-Y-common .
,ly:axis-group-interface::calc-pure-y-common)
(pure-relevant-grobs .
,ly:axis-group-interface::calc-pure-relevant-grobs)))
@@ -1694,6 +1697,7 @@
(skyline-vertical-padding . 0.15)
(X-extent . ,ly:axis-group-interface::width)
(Y-extent . ,axis-group-interface::height)
+ (vertical-skylines . ,ly:axis-group-interface::calc-skylines)
(meta . ((class . Item)
(object-callbacks . ((pure-Y-common .
,ly:axis-group-interface::calc-pure-y-common)
(pure-relevant-grobs .
,ly:axis-group-interface::calc-pure-relevant-grobs)))