Re: Standardizes use of empty extents in pure heights and skylines. (issue 7310075)
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/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Standardizes use of empty extents in pure heights and skylines. (issue 7310075)
On 12 avr. 2013, at 22:29, k-ohara5...@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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Standardizes use of empty extents in pure heights and skylines. (issue 7310075)
On 12 mars 2013, at 23:44, janek.lilyp...@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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Standardizes use of empty extents in pure heights and skylines. (issue 7310075)
m...@mikesolomon.org m...@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. -- David Kastrup ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Standardizes use of empty extents in pure heights and skylines. (issue 7310075)
Just sent this to developer list only, so here a copy for the record: m...@mikesolomon.org m...@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. https://codereview.appspot.com/7310075/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Standardizes use of empty extents in pure heights and skylines. (issue 7310075)
On Wed, 13 Mar 2013 02:44:33 -0700, d...@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. ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Standardizes use of empty extents in pure heights and skylines. (issue 7310075)
Keith OHara k-ohara5...@oco.net writes: On Wed, 13 Mar 2013 02:44:33 -0700, d...@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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Standardizes use of empty extents in pure heights and skylines. (issue 7310075)
Hi, On Wed, Mar 13, 2013 at 10:44 AM, d...@gnu.org wrote: m...@mikesolomon.org m...@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 0002-add-a-comment-explaining-numerical-accuracy-issues.patch Description: Binary data ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Standardizes use of empty extents in pure heights and skylines. (issue 7310075)
On 2013/03/13 20:40:36, dak wrote: Keith OHara mailto:k-ohara5...@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. https://codereview.appspot.com/7310075/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Standardizes use of empty extents in pure heights and skylines. (issue 7310075)
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/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Standardizes use of empty extents in pure heights and skylines. (issue 7310075)
https://codereview.appspot.com/7310075/diff/53001/input/regression/skyline-point-extent.ly
File input/regression/skyline-point-extent.ly (right):
https://codereview.appspot.com/7310075/diff/53001/input/regression/skyline-point-extent.ly#newcode5
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#newcode148
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: 1000))
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.
https://codereview.appspot.com/7310075/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Standardizes use of empty extents in pure heights and skylines. (issue 7310075)
Thanks for the review!
https://codereview.appspot.com/7310075/diff/53001/input/regression/skyline-point-extent.ly
File input/regression/skyline-point-extent.ly (right):
https://codereview.appspot.com/7310075/diff/53001/input/regression/skyline-point-extent.ly#newcode5
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#newcode148
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: 1000))
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.
https://codereview.appspot.com/7310075/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Standardizes use of empty extents in pure heights and skylines. (issue 7310075)
On 27 févr. 2013, at 07:03, k-ohara5...@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#newcode1825 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#newcode1992 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#newcode1993 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#newcode629 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___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Standardizes use of empty extents in pure heights and skylines. (issue 7310075)
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#newcode1825 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#newcode1992 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#newcode1993 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#newcode629 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. https://codereview.appspot.com/7310075/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Standardizes use of empty extents in pure heights and skylines. (issue 7310075)
On 2013/02/18 02:59:58, Keith wrote: On Sun, 17 Feb 2013 02:07:19 -0800, mailto:m...@mikesolomon.org mailto:m...@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. https://codereview.appspot.com/7310075/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Standardizes use of empty extents in pure heights and skylines. (issue 7310075)
On 16 févr. 2013, at 21:20, k-ohara5...@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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Standardizes use of empty extents in pure heights and skylines. (issue 7310075)
On Sun, 17 Feb 2013 02:07:19 -0800, m...@mikesolomon.org m...@mikesolomon.org wrote: On 16 févr. 2013, at 21:20, k-ohara5...@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. ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Standardizes use of empty extents in pure heights and skylines. (issue 7310075)
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#newcode156 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 (vectorBox const, etc.) */ Is this a hide-and-seek game? I don't see what kind of filtering Skyline (vectorBox 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 (vectorBox const, etc.) */ This is no longer the same filtering as in Skyline (vectorBox... 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#newcode1785 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#newcode646 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. https://codereview.appspot.com/7310075/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Standardizes use of empty extents in pure heights and skylines. (issue 7310075)
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#newcode156 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#newcode1951 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. https://codereview.appspot.com/7310075/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Standardizes use of empty extents in pure heights and skylines. (issue 7310075)
On Sat, 09 Feb 2013 23:24:41 -0800, mts...@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/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Standardizes use of empty extents in pure heights and skylines. (issue 7310075)
https://codereview.appspot.com/7310075/diff/10/input/regression/skyline-point-extent.ly
File input/regression/skyline-point-extent.ly (right):
https://codereview.appspot.com/7310075/diff/10/input/regression/skyline-point-extent.ly#newcode7
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-extent.ly#newcode12
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-extent.ly#newcode20
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#newcode153
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#newcode159
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
https://codereview.appspot.com/7310075/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Standardizes use of empty extents in pure heights and skylines. (issue 7310075)
New patch set coming after I finish running the regtests.
https://codereview.appspot.com/7310075/diff/10/input/regression/skyline-point-extent.ly
File input/regression/skyline-point-extent.ly (right):
https://codereview.appspot.com/7310075/diff/10/input/regression/skyline-point-extent.ly#newcode7
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#newcode153
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#newcode159
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.
https://codereview.appspot.com/7310075/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
