Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in

Issue 553980043: Calculate skylines only once. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
1 year, 1 month ago by hanwenn
1 year, 1 month ago


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)

Patch Set 1 #

Total comments: 2

Patch Set 2 : fix snafu #

Unified diffs Side-by-side diffs Delta from patch set Stats (+43 lines, -34 lines) Patch
M lily/axis-group-interface.cc View 1 3 chunks +25 lines, -29 lines 0 comments Download
M lily/figured-bass-position-engraver.cc View 3 chunks +8 lines, -0 lines 0 comments Download
M lily/rhythmic-column-engraver.cc View 2 chunks +6 lines, -5 lines 0 comments Download
M scm/define-grobs.scm View 4 chunks +4 lines, -0 lines 0 comments Download


Total messages: 5
LGTM. The figured bass stuff is a separate commit, I guess...
1 year, 1 month ago (2020-05-01 19:35:53 UTC) #1
no, it's in this commit. I actually folded in https://codereview.appspot.com/555770044/ , but maybe it will ...
1 year, 1 month ago (2020-05-01 19:54:18 UTC) #2
not LGTM - I got my baselines mixed up. Something is off here.
1 year, 1 month ago (2020-05-01 21:49:18 UTC) #3
On 2020/05/01 21:49:18, hanwenn wrote: > not LGTM - I got my baselines mixed up. ...
1 year, 1 month ago (2020-05-01 21:52:35 UTC) #4
1 year, 1 month ago (2020-05-01 22:10:03 UTC) #5
fix snafu
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b