Issue 7310075: Standardizes use of empty extents in pure heights and skylines. (Closed)
DescriptionStandardizes use of empty extents in pure heights and skylines.
Patch Set 1 #Patch Set 2 : Rebased off current master. #Patch Set 3 : Adds regtest. #Patch Set 4 : Damn you valgrind... #
Total comments: 22
Patch Set 5 : Responses to Keith's comments #Patch Set 6 : Changes standard-stencil-height to pure-safe-stencil-height #
Total comments: 14
Patch Set 7 : Implements Keith's suggestion of eliminating EPS #Patch Set 8 : Rebases against current mster #Patch Set 9 : Correct function name change #Patch Set 10 : Better handling of point stencils for repeat ties. #
Total comments: 8
Patch Set 11 : Uses stencil to get RepeatTie height #Patch Set 12 : Better pure heights for SpanBarStub #Patch Set 13 : Removes commented-out Y-extent #
Total comments: 10
Patch Set 14 : Rebase against current master #Patch Set 15 : Responds to David's comments #
Total comments: 1
MessagesTotal messages: 35
Rebased off current master.
Sign in to reply to this message.
Adds regtest.
Sign in to reply to this message.
Damn you valgrind...
Sign in to reply to this message.
https://codereview.appspot.com/7310075/diff/10/input/regression/skyline-point... File input/regression/skyline-point-extent.ly (right): https://codereview.appspot.com/7310075/diff/10/input/regression/skyline-point... input/regression/skyline-point-extent.ly:7: } ... The accents should follow the descending melody line, even though the note-head stencils are empty."} { \override NoteHead #'stencil = #point-stencil c'2.-> b8-- a-- g1-> } #(ly:set-option 'debug-skylines) https://codereview.appspot.com/7310075/diff/10/input/regression/skyline-point... input/regression/skyline-point-extent.ly:12: \type Engraver_group You don't need a custom context to test this, so there's no reason to make anyone who might investigate a change in this output try to figure all this out. https://codereview.appspot.com/7310075/diff/10/input/regression/skyline-point... input/regression/skyline-point-extent.ly:20: \override VerticalAxisGroup #'default-staff-staff-spacing = #'((basic_distance . 0) (minimum_distance . 10) (padding . 6) (stretchability . 0)) minimum_distance is misspelled, so it has no effect. if minimum-distance is 10, your patch has no effect on the output. https://codereview.appspot.com/7310075/diff/10/lily/grob.cc File lily/grob.cc (right): https://codereview.appspot.com/7310075/diff/10/lily/grob.cc#newcode488 lily/grob.cc:488: Interval iv = robust_scm2interval (iv_scm, Interval ()); I assume you will apply this change and the change in separation-item.cc in a separate commit, the "empty extents in pure-heights" part of the patch set. https://codereview.appspot.com/7310075/diff/10/lily/separation-item.cc File lily/separation-item.cc (right): https://codereview.appspot.com/7310075/diff/10/lily/separation-item.cc#newcod... lily/separation-item.cc:153: x[LEFT] += extra_width[LEFT]; // The conventional empty extent is (+inf.0 . -inf.0) // but (-inf.0 . +inf.0) is used as extra-spacing-height // on items that must not overlap other note-columns. // If these two uses of inf combine, leave the empty extent. if (!isinf(x[LEFT]) x[LEFT] += extra_width[LEFT]; if (!isinf(x[RIGHT]) x[RIGHT] += extra_width[RIGHT]; https://codereview.appspot.com/7310075/diff/10/lily/separation-item.cc#newcod... lily/separation-item.cc:159: // empty interval with infinity at either end Other than the addition above, what other 'operation' can produce a NaN ? https://codereview.appspot.com/7310075/diff/10/lily/skyline.cc File lily/skyline.cc (right): https://codereview.appspot.com/7310075/diff/10/lily/skyline.cc#newcode59 lily/skyline.cc:59: /* If we start including very thin buildings, numerical accuracy errors can Did you find where these numerical accuracy errors occured? I don't see anything obvious. The most likely seems to be the way we step through the two skylines in internal_merge_skylines(); https://codereview.appspot.com/7310075/diff/10/lily/skyline.cc#newcode61 lily/skyline.cc:61: than epsilon wide. In merges, we omit them. Any such buildings are created in merge_skyline() are omitted from the merged result. https://codereview.appspot.com/7310075/diff/10/lily/skyline.cc#newcode104 lily/skyline.cc:104: Interval start_end = fatten_skinny_buildings (start, end); The old-fashioned way would be // Buildings all have width at least EPS end = min(end, start + EPS); and the same in other constructors, but I know how you hate code-duplication. https://codereview.appspot.com/7310075/diff/10/lily/skyline.cc#newcode119 lily/skyline.cc:119: // Buildings all have width at least EPS end = min(end, start + EPS); https://codereview.appspot.com/7310075/diff/10/lily/skyline.cc#newcode497 lily/skyline.cc:497: Boxes should have fatness in the horizon_axis, otherwise they are ignored. This comment would no longer belong here https://codereview.appspot.com/7310075/diff/10/lily/skyline.cc#newcode519 lily/skyline.cc:519: Buildings should have fatness in the horizon_axis, otherwise they are ignored. This comment would no longer belong here
Sign in to reply to this message.
New patch set coming after I finish running the regtests. https://codereview.appspot.com/7310075/diff/10/input/regression/skyline-point... File input/regression/skyline-point-extent.ly (right): https://codereview.appspot.com/7310075/diff/10/input/regression/skyline-point... input/regression/skyline-point-extent.ly:7: } On 2013/02/10 03:08:04, Keith wrote: > ... The accents should follow the descending melody line, even though the > note-head stencils are empty."} > { \override NoteHead #'stencil = #point-stencil > c'2.-> b8-- a-- g1-> } > #(ly:set-option 'debug-skylines) Regtest changed. I've removed the Stem_engraver in the regtest so that the only possible side support element is the note head. https://codereview.appspot.com/7310075/diff/10/lily/grob.cc File lily/grob.cc (right): https://codereview.appspot.com/7310075/diff/10/lily/grob.cc#newcode488 lily/grob.cc:488: Interval iv = robust_scm2interval (iv_scm, Interval ()); On 2013/02/10 03:08:04, Keith wrote: > I assume you will apply this change and the change in separation-item.cc in a > separate commit, the "empty extents in pure-heights" part of the patch set. I'm not sure if these changes can be separated from the skyline stuff without causing regtest havoc. I can do this, but it'd take me more time (figure out what changes to isolate, run regtests, etc.). Is it necessary? https://codereview.appspot.com/7310075/diff/10/lily/separation-item.cc File lily/separation-item.cc (right): https://codereview.appspot.com/7310075/diff/10/lily/separation-item.cc#newcod... lily/separation-item.cc:153: x[LEFT] += extra_width[LEFT]; On 2013/02/10 03:08:04, Keith wrote: > // The conventional empty extent is (+inf.0 . -inf.0) > // but (-inf.0 . +inf.0) is used as extra-spacing-height > // on items that must not overlap other note-columns. > // If these two uses of inf combine, leave the empty extent. > > if (!isinf(x[LEFT]) x[LEFT] += extra_width[LEFT]; > if (!isinf(x[RIGHT]) x[RIGHT] += extra_width[RIGHT]; Done. https://codereview.appspot.com/7310075/diff/10/lily/separation-item.cc#newcod... lily/separation-item.cc:159: // empty interval with infinity at either end On 2013/02/10 03:08:04, Keith wrote: > Other than the addition above, what other 'operation' can produce a NaN ? Comment eliminated with change above. https://codereview.appspot.com/7310075/diff/10/lily/skyline.cc File lily/skyline.cc (right): https://codereview.appspot.com/7310075/diff/10/lily/skyline.cc#newcode59 lily/skyline.cc:59: /* If we start including very thin buildings, numerical accuracy errors can On 2013/02/10 03:08:04, Keith wrote: > Did you find where these numerical accuracy errors occured? I don't see > anything obvious. The most likely seems to be the way we step through the two > skylines in internal_merge_skylines(); No idea... https://codereview.appspot.com/7310075/diff/10/lily/skyline.cc#newcode61 lily/skyline.cc:61: than epsilon wide. In merges, we omit them. On 2013/02/10 03:08:04, Keith wrote: > Any such buildings are created in merge_skyline() are omitted from the merged > result. Done. https://codereview.appspot.com/7310075/diff/10/lily/skyline.cc#newcode104 lily/skyline.cc:104: Interval start_end = fatten_skinny_buildings (start, end); On 2013/02/10 03:08:04, Keith wrote: > The old-fashioned way would be > // Buildings all have width at least EPS > end = min(end, start + EPS); > and the same in other constructors, but I know how you hate code-duplication. But this adds the EPS arbitrarily to the right instead of adding an equal amount to the right and left... And I hate code duplication. https://codereview.appspot.com/7310075/diff/10/lily/skyline.cc#newcode119 lily/skyline.cc:119: On 2013/02/10 03:08:04, Keith wrote: > // Buildings all have width at least EPS > end = min(end, start + EPS); See above. https://codereview.appspot.com/7310075/diff/10/lily/skyline.cc#newcode497 lily/skyline.cc:497: Boxes should have fatness in the horizon_axis, otherwise they are ignored. On 2013/02/10 03:08:04, Keith wrote: > This comment would no longer belong here Changed to represent use of EPS as minimum fatness. https://codereview.appspot.com/7310075/diff/10/lily/skyline.cc#newcode519 lily/skyline.cc:519: Buildings should have fatness in the horizon_axis, otherwise they are ignored. On 2013/02/10 03:08:04, Keith wrote: > This comment would no longer belong here Changed to represent use of EPS as minimum fatness.
Sign in to reply to this message.
Responses to Keith's comments
Sign in to reply to this message.
Changes standard-stencil-height to pure-safe-stencil-height
Sign in to reply to this message.
On Sat, 09 Feb 2013 23:24:41 -0800, <mtsolo@gmail.com> wrote: > https://codereview.appspot.com/7310075/diff/10/lily/grob.cc#newcode488 > lily/grob.cc:488: Interval iv = robust_scm2interval (iv_scm, Interval > ()); > On 2013/02/10 03:08:04, Keith wrote: >> I assume you will apply this change and the change in > separation-item.cc in a >> separate commit, the "empty extents in pure-heights" part of the patch > set. > > I'm not sure if these changes can be separated from the skyline stuff > without causing regtest havoc. Oh, right. If zero-horizon-dimension boxes are included in skylines they would be included in the skylines for note-spacing as well, so the default height should be empty, not zero, and a special kind of empty so that extra-spacing-height still applies. OK > https://codereview.appspot.com/7310075/
Sign in to reply to this message.
https://codereview.appspot.com/7310075/diff/12001/lily/separation-item.cc File lily/separation-item.cc (right): https://codereview.appspot.com/7310075/diff/12001/lily/separation-item.cc#new... lily/separation-item.cc:156: // If these two uses of inf combine, leave the empty extent. The description is inaccurate since "combine" suggests a symmetry. Instead, the existing infinity trumps extra_width, regardless of which extent is infinite and which is empty. https://codereview.appspot.com/7310075/diff/12001/lily/skyline.cc File lily/skyline.cc (right): https://codereview.appspot.com/7310075/diff/12001/lily/skyline.cc#newcode90 lily/skyline.cc:90: fatten_skinny_buildings (Real start, Real end) The function does nothing whatsoever to buildings. It works on intervals or ranges. So it is named misleadingly. In addition, it also "fattens" empty or undefined intervals (which have length 0). The "widening" is symmetric, meaning that a building at the left or the right of a skyline causes the skyline to expand. What is the point? Except for back-and-forth rotations, there is no operation that will cause an interval to move from non-empty to empty. The worst that can happen is that a non-zero interval collapses to a single point, but single points are treated just like non-zero intervals anyway. https://codereview.appspot.com/7310075/diff/12001/lily/skyline.cc#newcode359 lily/skyline.cc:359: ret->push_back (Building (-infinity_f, -infinity_f, Three times -infinity_f? Looks fishy. If intentional, a comment is missing. https://codereview.appspot.com/7310075/diff/12001/lily/skyline.cc#newcode499 lily/skyline.cc:499: given a minimum fatness of EPS. This comment has nothing whatsoever to do with the actions of this function. https://codereview.appspot.com/7310075/diff/12001/lily/skyline.cc#newcode522 lily/skyline.cc:522: given a minimum fatness of EPS. Again, this comment has nothing to do with the actions of the function. https://codereview.appspot.com/7310075/diff/12001/lily/skyline.cc#newcode617 lily/skyline.cc:617: /* do the same filtering as in Skyline (vector<Box> const&, etc.) */ Is this a hide-and-seek game? I don't see what kind of filtering Skyline (vector<Box> const&, etc.) does. In fact, it does something different than what you do now. https://codereview.appspot.com/7310075/diff/12001/lily/skyline.cc#newcode617 lily/skyline.cc:617: /* do the same filtering as in Skyline (vector<Box> const&, etc.) */ This is no longer the same filtering as in Skyline (vector<Box>... https://codereview.appspot.com/7310075/diff/12001/lily/skyline.cc#newcode689 lily/skyline.cc:689: warning ("Skyline horizon padding is too small. Widening to avoid numerical errors."); What numerical error are you thinking of here? With what consequences? https://codereview.appspot.com/7310075/diff/12001/ly/engraver-init.ly File ly/engraver-init.ly (right): https://codereview.appspot.com/7310075/diff/12001/ly/engraver-init.ly#newcode896 ly/engraver-init.ly:896: \override Clef.Y-extent = #pure-safe-stencil-height What's pure-safe? uncached? Or what? https://codereview.appspot.com/7310075/diff/12001/scm/define-grobs.scm File scm/define-grobs.scm (right): https://codereview.appspot.com/7310075/diff/12001/scm/define-grobs.scm#newcod... scm/define-grobs.scm:1785: (Y-extent . ,(ly:make-unpure-pure-container #f small-empty-interval)) What's that? No information for pure, a "small-empty-interval" for unpure? What happened to point intervals? https://codereview.appspot.com/7310075/diff/12001/scm/lily-library.scm File scm/lily-library.scm (right): https://codereview.appspot.com/7310075/diff/12001/scm/lily-library.scm#newcod... scm/lily-library.scm:646: (define-public small-empty-interval '(0.01 . -0.01)) In the absence of units, this is very fuzzy. It also looks like a huge kludge waiting for problems: the interval is just as empty as the empty interval. Its only use is that it is less likely to make code blow up that can't deal with empty intervals but should. So the only purpose of this thing is to make it harder to fix bugs. https://codereview.appspot.com/7310075/diff/12001/scm/output-lib.scm File scm/output-lib.scm (right): https://codereview.appspot.com/7310075/diff/12001/scm/output-lib.scm#newcode61 scm/output-lib.scm:61: (define-public pure-safe-stencil-height We had the discussion about this already. What makes it "pure-safe"? The name is non-descriptive. Why would "pure" be "unsafe"? There is no comment explaining what you mean, and the naming is not giving any useful information. The information content of your identifiers is that of disassembled code, except that one tends to add comments to disassembled code.
Sign in to reply to this message.
Joe disallowed buildings of zero width in his skylines for reasons given only as "avoiding numerical inaccuracies". Could be something like depending on the +/- sign of 0.0, or depending on advancement through loops when we increment by a building width. It seems a bit better, for consistency, to allow zero-width buildings, so that point-stencils get their requested space around them as requested by the padding settings. Best, of course, would be to find and repair the code that cannot handle zero-widths. We do not yet have the choice between "a bit better" and "best", merely "a bit better" or nothing. https://codereview.appspot.com/7310075/diff/12001/lily/separation-item.cc File lily/separation-item.cc (right): https://codereview.appspot.com/7310075/diff/12001/lily/separation-item.cc#new... lily/separation-item.cc:156: // If these two uses of inf combine, leave the empty extent. On 2013/02/16 08:05:23, dak wrote: > The description is inaccurate ... Comments do not say what the code does; they explain its desired effect. This code combines extents with extra-spacings, avoiding inf - inf. The comment describes the case that could give inf - inf for which we depend on particular behavior. https://codereview.appspot.com/7310075/diff/12001/scm/define-grobs.scm File scm/define-grobs.scm (right): https://codereview.appspot.com/7310075/diff/12001/scm/define-grobs.scm#newcod... scm/define-grobs.scm:1951: (Y-extent . ,(ly:make-unpure-pure-container #f small-empty-interval)) Better to have commented magic numbers here, in the Grob definition, where they can be understood. % Empty for purposes of positioning grobs, but % extra-spacing-height makes it non-empty for note-spacing Y-extent = #'(+0.01 . -0.01) Giving two similar magic number pairs a name is not worth the complication (make-unpure-pure-container) required.
Sign in to reply to this message.
On 16 févr. 2013, at 21:20, k-ohara5a5a@oco.net wrote: > Best, of course, would be to find and repair the code that cannot handle > zero-widths. We do not yet have the choice between "a bit better" and > "best", merely "a bit better" or nothing. > One solution is to just allow 0-width buildings and then propose a bug fix whenever numerical inaccuracies actually creep in. In this case, we'd change the comment to something like "be attentive to 0-width buildings, as they may cause numerical errors." What would help, of course, is if the comment told what these errors could look like. Cheers, MS
Sign in to reply to this message.
On Sun, 17 Feb 2013 02:07:19 -0800, mike@mikesolomon.org <mike@mikesolomon.org> wrote: > On 16 févr. 2013, at 21:20, k-ohara5a5a@oco.net wrote: > >> Best, of course, would be to find and repair the code that cannot handle >> zero-widths. We do not yet have the choice between "a bit better" and >> "best", merely "a bit better" or nothing. >> > > One solution is to just allow 0-width buildings and then propose a bug fix whenever numerical inaccuracies actually creep in. > In this case, we'd change the comment to something like "be attentive to 0-width buildings, as they may cause numerical errors." > What would help, of course, is if the comment told what these errors could look like. > That sounds reasonable to me. The code in skyline.cc looks robust to zero-width Buildings. A helpful warning could be "Such things as point-stencils cause Buildings with zero width, so consider proper behavior in cases where start_ == end_." LilyPond already uses point-stencils, with extents that are the same floating point number on either side, so we already have to be careful. The classic error with floating point is to do math on the pair, something like (left . right) + shift/3.0 , where left==right, and then ask is_empty(). For some compiler options, the optimizer could keep left+shift/3 in register while right+shift/3 was stored to memory. If the register has finer precision than the memory format, and right+shift/3 was rounded down when stored, then is_empty() can return true. People avoid these problems by doing floating-point comparisons only as often as they are willing to be careful, and early in the data stream while they still remember where the data came from, which usually determines how to treat ambiguous results. Don't process a mixed set of zero-intervals and empty-intervals and expect to be able to sort them out later; filter out empty intervals before inserting them in the set.
Sign in to reply to this message.
Implements Keith's suggestion of eliminating EPS
Sign in to reply to this message.
Rebases against current mster
Sign in to reply to this message.
Correct function name change
Sign in to reply to this message.
On 2013/02/18 02:59:58, Keith wrote: > On Sun, 17 Feb 2013 02:07:19 -0800, mailto:mike@mikesolomon.org <mailto:mike@mikesolomon.org> > wrote: > > The classic error with floating point is to do math on the pair, > something like (left . right) + shift/3.0 , where left==right, and > then ask is_empty(). For some compiler options, the optimizer could > keep left+shift/3 in register while right+shift/3 was stored to > memory. If the register has finer precision than the memory format, > and right+shift/3 was rounded down when stored, then is_empty() can > return true. This is sick enough to be right. One can explicitly cast to the used floating point type or assign to variables, however. I think that the standards allow this sort of "higher precision than warranted" operation only for intermediate results. With Scheme code, there is not much of a danger since the additions will pass through the same code path and thus get the same treatment for same values.
Sign in to reply to this message.
Better handling of point stencils for repeat ties.
Sign in to reply to this message.
Just needs cleanup of some leftover unneeded complications. https://codereview.appspot.com/7310075/diff/38001/lily/skyline.cc File lily/skyline.cc (right): https://codereview.appspot.com/7310075/diff/38001/lily/skyline.cc#newcode488 lily/skyline.cc:488: will be ignored. What do you mean by "they will be ignored"? On line 501, empty segments seem to be reversed, making non-empty segments. https://codereview.appspot.com/7310075/diff/38001/ly/engraver-init.ly File ly/engraver-init.ly (right): https://codereview.appspot.com/7310075/diff/38001/ly/engraver-init.ly#newcode896 ly/engraver-init.ly:896: \override Clef.Y-extent = #grob::all-heights-from-stencil Why the \overrride to the same value as its default ? https://codereview.appspot.com/7310075/diff/38001/ly/gregorian.ly File ly/gregorian.ly (right): https://codereview.appspot.com/7310075/diff/38001/ly/gregorian.ly#newcode103 ly/gregorian.ly:103: \once \override BreathingSign.Y-extent = #grob::all-heights-from-stencil This is now the default, so no need for overrides. https://codereview.appspot.com/7310075/diff/38001/scm/define-grobs.scm File scm/define-grobs.scm (right): https://codereview.appspot.com/7310075/diff/38001/scm/define-grobs.scm#newcod... scm/define-grobs.scm:1825: (Y-extent . ,grob::unpure-empty-pure-point-height) (Y-extent . ,grob::all-heights-from-stencil) We do not /want/ to allow outside-staff objects to slide down alongside note-heads protruding from the staff, if by doing so they overlap any repeat ties that might be there. https://codereview.appspot.com/7310075/diff/38001/scm/define-grobs.scm#newcod... scm/define-grobs.scm:1992: ; should preserve the span bar's presence for horizontal spacing "want this to be ignored" when ? Presumably during staff-spacing, but what problem could it cause if not ignored ? If staves would otherwise move away trying to make room for it, the SpanBar avoids this using cross-staff = ##t https://codereview.appspot.com/7310075/diff/38001/scm/define-grobs.scm#newcod... scm/define-grobs.scm:1993: (Y-extent . ,grob::unpure-empty-pure-point-height) (Y-extent . (0 . 0)) https://codereview.appspot.com/7310075/diff/38001/scm/lily-library.scm File scm/lily-library.scm (right): https://codereview.appspot.com/7310075/diff/38001/scm/lily-library.scm#newcod... scm/lily-library.scm:629: (define-public small-empty-interval '(0.01 . -0.01)) orphaned https://codereview.appspot.com/7310075/diff/38001/scm/output-lib.scm File scm/output-lib.scm (right): https://codereview.appspot.com/7310075/diff/38001/scm/output-lib.scm#newcode69 scm/output-lib.scm:69: (ly:make-unpure-pure-container #f '(0 . 0))) The changes to define-grobs.scm work for me, so this can be orphaned, thankfully.
Sign in to reply to this message.
On 27 févr. 2013, at 07:03, k-ohara5a5a@oco.net wrote: > Just needs cleanup of some leftover unneeded complications. > > > https://codereview.appspot.com/7310075/diff/38001/lily/skyline.cc > File lily/skyline.cc (right): > > https://codereview.appspot.com/7310075/diff/38001/lily/skyline.cc#newcode488 > lily/skyline.cc:488: will be ignored. > What do you mean by "they will be ignored"? On line 501, empty segments > seem to be reversed, making non-empty segments. > Fixed. Bad copy and paste. > https://codereview.appspot.com/7310075/diff/38001/ly/engraver-init.ly > File ly/engraver-init.ly (right): > > https://codereview.appspot.com/7310075/diff/38001/ly/engraver-init.ly#newcode896 > ly/engraver-init.ly:896: \override Clef.Y-extent = > #grob::all-heights-from-stencil > Why the \overrride to the same value as its default ? Fixed. > > https://codereview.appspot.com/7310075/diff/38001/ly/gregorian.ly > File ly/gregorian.ly (right): > > https://codereview.appspot.com/7310075/diff/38001/ly/gregorian.ly#newcode103 > ly/gregorian.ly:103: \once \override BreathingSign.Y-extent = > #grob::all-heights-from-stencil > This is now the default, so no need for overrides. Fixed. > > https://codereview.appspot.com/7310075/diff/38001/scm/define-grobs.scm > File scm/define-grobs.scm (right): > > https://codereview.appspot.com/7310075/diff/38001/scm/define-grobs.scm#newcod... > scm/define-grobs.scm:1825: (Y-extent . > ,grob::unpure-empty-pure-point-height) > (Y-extent . ,grob::all-heights-from-stencil) > > We do not /want/ to allow outside-staff objects to slide down alongside > note-heads protruding from the staff, if by doing so they overlap any > repeat ties that might be there. I'll take this for a spin...not sure if it'll work but no harm regtesting it. My goal was to preserve the original behavior, but if this is better no harm in using it. > > https://codereview.appspot.com/7310075/diff/38001/scm/define-grobs.scm#newcod... > scm/define-grobs.scm:1992: ; should preserve the span bar's presence for > horizontal spacing > "want this to be ignored" when ? > Presumably during staff-spacing, but what problem could it cause if not > ignored ? > It will be part of the skyline, potentially blocking things that shouldn't be blocked. > If staves would otherwise move away trying to make room for it, the > SpanBar avoids this using cross-staff = ##t The SpanBar has no height, so it is never taken into account in spacing. > > https://codereview.appspot.com/7310075/diff/38001/scm/define-grobs.scm#newcod... > scm/define-grobs.scm:1993: (Y-extent . > ,grob::unpure-empty-pure-point-height) > (Y-extent . (0 . 0)) > See above. > https://codereview.appspot.com/7310075/diff/38001/scm/lily-library.scm > File scm/lily-library.scm (right): > > https://codereview.appspot.com/7310075/diff/38001/scm/lily-library.scm#newcod... > scm/lily-library.scm:629: (define-public small-empty-interval '(0.01 . > -0.01)) > orphaned > Abandoned. > https://codereview.appspot.com/7310075/diff/38001/scm/output-lib.scm > File scm/output-lib.scm (right): > > https://codereview.appspot.com/7310075/diff/38001/scm/output-lib.scm#newcode69 > scm/output-lib.scm:69: (ly:make-unpure-pure-container #f '(0 . 0))) > The changes to define-grobs.scm work for me, so this can be orphaned, > thankfully. I'm with you on RepeatTie, but not SpanBarStub. SpanBarStub needs to only have height in Separation_item::boxes which is accomplished via the addition of extra-spacing-height. Otherwise we don't want the spacing engine to know it's there. This can be accomplished (for now) by having a point pure height and adding the extra-spacing-height. The problem is that the notion "horizontal spacing equals pure height plus extra spacing height" is hardcoded into Separation_item::boxes. There should be horizontal-spacing-height and horizontal-spacing-width instead of extra-spacing-height and extra-spacing-width that has set as a default pure-height and width unless specified otherwise. But changing that would be a UI nightmare, so I'm reluctant to do it. Cheers, MS
Sign in to reply to this message.
Uses stencil to get RepeatTie height
Sign in to reply to this message.
Better pure heights for SpanBarStub
Sign in to reply to this message.
Removes commented-out Y-extent
Sign in to reply to this message.
https://codereview.appspot.com/7310075/diff/53001/input/regression/skyline-po... File input/regression/skyline-point-extent.ly (right): https://codereview.appspot.com/7310075/diff/53001/input/regression/skyline-po... input/regression/skyline-point-extent.ly:5: even though the @code{NoteHead} stencils are empty. The @code{Stem_engraver} This talks about the NoteHead stencil being empty, but the actual code uses a point-stencil (which is _not_ empty). https://codereview.appspot.com/7310075/diff/53001/lily/separation-item.cc File lily/separation-item.cc (right): https://codereview.appspot.com/7310075/diff/53001/lily/separation-item.cc#new... lily/separation-item.cc:148: Interval extra_width = robust_scm2interval (elts[i]->get_property ("extra-spacing-width"), This is not related to this patch, but isn't it complete nonsense to use an Interval here rather than a Drul_array or other form of offset pair? The LEFT and RIGHT values don't form an interval as far as I can see. https://codereview.appspot.com/7310075/diff/53001/lily/skyline.cc File lily/skyline.cc (right): https://codereview.appspot.com/7310075/diff/53001/lily/skyline.cc#newcode649 lily/skyline.cc:649: return ret; Why not just return *this; here? The function does not return a reference, so a copy is constructed anyway. There is no point in doing yet another copy, is there? https://codereview.appspot.com/7310075/diff/53001/scm/output-lib.scm File scm/output-lib.scm (right): https://codereview.appspot.com/7310075/diff/53001/scm/output-lib.scm#newcode472 scm/output-lib.scm:472: 10000000)) Don't we have some constants for the full range START and END values? https://codereview.appspot.com/7310075/diff/53001/scm/output-lib.scm#newcode478 scm/output-lib.scm:478: (define-public pure-from-neighbor-interface::unobtrusive-height "unobtrusive" is a bit obscure: I suspected the only-a-bit-empty interval at first. "height-if-pure" would be quite more descriptive.
Sign in to reply to this message.
Rebase against current master
Sign in to reply to this message.
Responds to David's comments
Sign in to reply to this message.
Thanks for the review! https://codereview.appspot.com/7310075/diff/53001/input/regression/skyline-po... File input/regression/skyline-point-extent.ly (right): https://codereview.appspot.com/7310075/diff/53001/input/regression/skyline-po... input/regression/skyline-point-extent.ly:5: even though the @code{NoteHead} stencils are empty. The @code{Stem_engraver} On 2013/03/08 14:24:11, dak wrote: > This talks about the NoteHead stencil being empty, but the actual code uses a > point-stencil (which is _not_ empty). Done. https://codereview.appspot.com/7310075/diff/53001/lily/separation-item.cc File lily/separation-item.cc (right): https://codereview.appspot.com/7310075/diff/53001/lily/separation-item.cc#new... lily/separation-item.cc:148: Interval extra_width = robust_scm2interval (elts[i]->get_property ("extra-spacing-width"), On 2013/03/08 14:24:11, dak wrote: > This is not related to this patch, but isn't it complete nonsense to use an > Interval here rather than a Drul_array or other form of offset pair? > > The LEFT and RIGHT values don't form an interval as far as I can see. you're right https://codereview.appspot.com/7310075/diff/53001/lily/skyline.cc File lily/skyline.cc (right): https://codereview.appspot.com/7310075/diff/53001/lily/skyline.cc#newcode649 lily/skyline.cc:649: return ret; On 2013/03/08 14:24:11, dak wrote: > Why not just > return *this; > here? The function does not return a reference, so a copy is constructed > anyway. There is no point in doing yet another copy, is there? Done. https://codereview.appspot.com/7310075/diff/53001/scm/output-lib.scm File scm/output-lib.scm (right): https://codereview.appspot.com/7310075/diff/53001/scm/output-lib.scm#newcode472 scm/output-lib.scm:472: 10000000)) On 2013/03/08 14:24:11, dak wrote: > Don't we have some constants for the full range START and END values? I don't think we have one for integers. I made one in lily-library.scm https://codereview.appspot.com/7310075/diff/53001/scm/output-lib.scm#newcode478 scm/output-lib.scm:478: (define-public pure-from-neighbor-interface::unobtrusive-height On 2013/03/08 14:24:11, dak wrote: > "unobtrusive" is a bit obscure: I suspected the only-a-bit-empty interval at > first. "height-if-pure" would be quite more descriptive. Done.
Sign in to reply to this message.
Hi Mike, i've read changes in code and i don't quite get what this change is for. What makes it possible that we can now accept boxes that are narrower than epsilon? What can we achieve with that and why? I'm sorry for asking such boring questions, but this is one of your smallest patches and therefore i'd like to actually understand what you are doing here - with your regular changes it's way too difficult for me ;) cheers, Janek
Sign in to reply to this message.
On 12 mars 2013, at 23:44, janek.lilypond@gmail.com wrote: > Hi Mike, > > i've read changes in code and i don't quite get what this change is for. > What makes it possible that we can now accept boxes that are narrower > than epsilon? What can we achieve with that and why? > > I'm sorry for asking such boring questions, but this is one of your > smallest patches and therefore i'd like to actually understand what you > are doing here - with your regular changes it's way too difficult for me > ;) > > cheers, > Janek > > https://codereview.appspot.com/7310075/ Good question! Imagine that there is a notehead with a width smaller than epsilon. We'd like to use it to position elements, but if skylines throw away anything with a width less than epsilon, the note head will not be part of the skyline and things will be positioned on top of it. The concern before was a comment about numerical inaccuracy, but after having tested the patch, this seems not to be an issue. Cheers, MS
Sign in to reply to this message.
Just sent this to developer list only, so here a copy for the record: "mike@mikesolomon.org" <mike@mikesolomon.org> writes: > The concern before was a comment about numerical inaccuracy, but after > having tested the patch, this seems not to be an issue. Like Keith pointed out, it could become one if more than one operation is done before storing the result, and/or there are different code paths for doing the operations to the different ends of an interval. If left and right have equal values to start with, C++ is still not required to have left and right receive the same value after left = left*factor + offset; right = right*factor + offset; That's totally sick. It may be worth using GCC compiler options to disallow extended precision for intermediate results and/or the choice to store intermediates with less than full precision and try to retain some kind of deterministic behavior that way.
Sign in to reply to this message.
On Wed, 13 Mar 2013 02:44:33 -0700, <dak@gnu.org> wrote: > If left and right have equal values to start with, C++ is still not > required to have left and right receive the same value after > > left = left*factor + offset; > right = right*factor + offset; The C standard requires the variables to be equal after assignment. C99 5.4.2.2: "Except for assignment and cast (which remove all extra range and precision), the values of operations with floating operands [...] are evaluated to a format whose range and precision may be greater than required by the type." GCC had bug 323, failing to convert to storage formats when required by the standard. That seems to be fixed with about version 4.5 <http://gcc.gnu.org/ml/gcc-patches/2008-11/msg00105.html> For a long time it was good to have guard digits, letting round-off affect bits less-significant than those stored. When memory was expensive we didn't want to store garbage bits affected by round-off. With or without guard digits, we need to be careful with /comparison/ between floating point values, because different computation paths can round off differently, even if we might expect them to lead to the same result. (Guard digits reduce the frequency of differences in round-off in stored values, but then we depend on subtle points of the C99 standard to communicate with the compiler about when computations are converted to the stored format.) > That's totally sick. It may be worth using GCC compiler options to > disallow extended precision for intermediate results and/or the choice > to store intermediates with less than full precision and try to retain > some kind of deterministic behavior that way. > > https://codereview.appspot.com/7310075/ It looks like the relevant compiler options are still buggy before GCC version 4.5 We were only /speculating/ what problems could arise, in an attempt to review the code for possible problems with "numerical inaccuracies" that the old code was trying to avoid. We found no potential problems in this code.
Sign in to reply to this message.
"Keith OHara" <k-ohara5a5a@oco.net> writes: > On Wed, 13 Mar 2013 02:44:33 -0700, <dak@gnu.org> wrote: > >> If left and right have equal values to start with, C++ is still not >> required to have left and right receive the same value after >> >> left = left*factor + offset; >> right = right*factor + offset; > > The C standard requires the variables to be equal after assignment. > C99 5.4.2.2: "Except for assignment and cast (which remove all extra > range and precision), the values of operations with floating operands > [...] are evaluated to a format whose range and precision may be > greater than required by the type." "May be". It may be greater for left*factor, and not greater for right*factor. The reduction of extra precision happens only _after_ adding offset. -- David Kastrup
Sign in to reply to this message.
Hi, On Wed, Mar 13, 2013 at 10:44 AM, <dak@gnu.org> wrote: > > "mike@mikesolomon.org" <mike@mikesolomon.org> writes: >> The concern before was a comment about numerical inaccuracy, but after >> having tested the patch, this seems not to be an issue. > > Like Keith pointed out, it could become one if more than one operation > is done before storing the result, and/or there are different code paths > for doing the operations to the different ends of an interval. > > If left and right have equal values to start with, C++ is still not > required to have left and right receive the same value after > > left = left*factor + offset; > right = right*factor + offset; > > That's totally sick. It may be worth using GCC compiler options to > disallow extended precision for intermediate results and/or the choice > to store intermediates with less than full precision and try to retain > some kind of deterministic behavior that way. attached is a patch that turns your explanations into a comment and contains some explanatory text for commit message. You may want to double-check whether i got everything right. thanks, Janek
Sign in to reply to this message.
On 2013/03/13 20:40:36, dak wrote: > "Keith OHara" <mailto:k-ohara5a5a@oco.net> writes: > > > The C standard requires the variables to be equal after assignment. > > C99 5.4.2.2: "Except for assignment and cast (which remove all extra > > range and precision), the values of operations with floating operands > > [...] are evaluated to a format whose range and precision may be > > greater than required by the type." > > "May be". It may be greater for left*factor, and not greater for > right*factor. The reduction of extra precision happens only _after_ > adding offset. > There may be further sentences in the C standard that specify consistent use of precision. On there other hand, there may be an option for an implementation to say its use of precision is "indeterminable". This still describes possible problems with floating point. I have found no specific problem with this patch.
Sign in to reply to this message.
https://codereview.appspot.com/7310075/diff/67001/lily/skyline.cc File lily/skyline.cc (right): https://codereview.appspot.com/7310075/diff/67001/lily/skyline.cc#newcode370 lily/skyline.cc:370: if (x1 >= last_end) Oops. This controls whether to put an empty, -inf height, building in the gap. (When do we ever want Building data structures marking gaps? The skyline concept seems to allow for gaps.) These zero-width anti-buildings would not have any effect, except that they are drawn (issue 3311) but they are not the boxes that surprised us when they disappeared (issue 3161). Probably safest to put back the x1 > last_end + EPS
Sign in to reply to this message.
On 12 avr. 2013, at 22:29, k-ohara5a5a@oco.net wrote: > > https://codereview.appspot.com/7310075/diff/67001/lily/skyline.cc > File lily/skyline.cc (right): > > https://codereview.appspot.com/7310075/diff/67001/lily/skyline.cc#newcode370 > lily/skyline.cc:370: if (x1 >= last_end) > Oops. This controls whether to put an empty, -inf height, building in > the gap. (When do we ever want Building data structures marking gaps? > The skyline concept seems to allow for gaps.) > > These zero-width anti-buildings would not have any effect, except that > they are drawn (issue 3311) but they are not the boxes that surprised us > when they disappeared (issue 3161). > > Probably safest to put back the x1 > last_end + EPS > > https://codereview.appspot.com/7310075/ Fair 'nuf. Can you write a patch? Cheers, MS
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
