Re: Sets TabVoice Stem height to ##f (issue 6303065)
mike at apollinemike.com mike at apollinemike.com writes: On 2012/06/12 12:49:45, dak wrote: http://codereview.appspot.com/6303065/diff/10003/lily/grob.cc File lily/grob.cc (right): http://codereview.appspot.com/6303065/diff/10003/lily/grob.cc#newcode472 lily/grob.cc:472: real_ext[d] += offset; On 2012/06/12 12:32:37, dak wrote: I don't understand this. The only way to get a nan from adding an offset to infinity is by adding another nan or an infinite offset with different sign. What case is this supposed to catch? The Flags in TabVoice had empty extent, and position at the end of their Stems, which also had empty extents. So we had (inf . -inf) being offset by -inf. The skyline code is not robust to NaNs, so with skylines being made for all objects, NaNs in extents become more of a problem. ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Sets TabVoice Stem height to ##f (issue 6303065)
http://codereview.appspot.com/6303065/diff/9001/lily/stem.cc File lily/stem.cc (right): http://codereview.appspot.com/6303065/diff/9001/lily/stem.cc#newcode710 lily/stem.cc:710: if (calc_beam !beam !unsmob_stencil (me-get_property (stencil))) On 2012/06/11 16:33:27, Keith wrote: If you have time to test the output, consider removing the test for calc_beam, Bad suggestion, apparently, given that it disconnects the stems form merged triangle note-heads. I'll put back the test for calc_beam after I get through regression testing. http://codereview.appspot.com/6303065/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Sets TabVoice Stem height to ##f (issue 6303065)
On 2012/06/12 13:22:10, dak wrote: On 2012/06/12 12:54:40, MikeSol wrote: On 2012/06/12 12:49:45, dak wrote: http://codereview.appspot.com/6303065/diff/10003/lily/grob.cc File lily/grob.cc (right): http://codereview.appspot.com/6303065/diff/10003/lily/grob.cc#newcode472 lily/grob.cc:472: real_ext[d] += offset; On 2012/06/12 12:32:37, dak wrote: I don't understand this. The only way to get a nan from adding an offset to infinity is by adding another nan or an infinite offset with different sign. What case is this supposed to catch? So what you actually meant to say was if (!real_ext.is_empty ()) real_ext.translate (offset); If that's what you mean, why don't you write it instead of some puzzle? This is not what I mean. An empty interval is, in LilyPond, an interval whose left is greater than it's right. I disagree. An empty interval is an interval not containing any point. The details are not important. I see that you chose to push this unchanged instead of using my suggestion. The result is that both interval ends are translated _independently_. Treating them _independently_ in this obfuscate manner only makes sense when you expect them to be set independently, namely one interval end being infinite, the other finite. For one thing, you have not explained how you expect such a configuration to come about. For another it means that (inf,3) which is an empty interval, gets transformed under an infinite shift to (inf,inf) an interval that will likely blow the emptiness check. Your example, (3,2) is transformed with an infinite shift into (inf,inf) again blowing the emptiness check since infinf can't be determined. Could you please explain the cases where you expect to get sensible results with that approach? http://codereview.appspot.com/6303065/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Sets TabVoice Stem height to ##f (issue 6303065)
http://codereview.appspot.com/6303065/diff/10003/lily/grob.cc File lily/grob.cc (right): http://codereview.appspot.com/6303065/diff/10003/lily/grob.cc#newcode472 lily/grob.cc:472: real_ext[d] += offset; I don't understand this. The only way to get a nan from adding an offset to infinity is by adding another nan or an infinite offset with different sign. What case is this supposed to catch? http://codereview.appspot.com/6303065/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Sets TabVoice Stem height to ##f (issue 6303065)
On 2012/06/12 13:22:10, dak wrote: On 2012/06/12 12:54:40, MikeSol wrote: On 2012/06/12 12:49:45, dak wrote: http://codereview.appspot.com/6303065/diff/10003/lily/grob.cc File lily/grob.cc (right): http://codereview.appspot.com/6303065/diff/10003/lily/grob.cc#newcode472 lily/grob.cc:472: real_ext[d] += offset; On 2012/06/12 12:32:37, dak wrote: I don't understand this. The only way to get a nan from adding an offset to infinity is by adding another nan or an infinite offset with different sign. What case is this supposed to catch? So what you actually meant to say was if (!real_ext.is_empty ()) real_ext.translate (offset); If that's what you mean, why don't you write it instead of some puzzle? This is not what I mean. An empty interval is, in LilyPond, an interval whose left is greater than it's right. I disagree. An empty interval is an interval not containing any point. The details are not important. So (3 . 2) is empty, but (3 . 2) can be translated by an infinite value in either direction and not result in nan. What meaning is there in translating an empty interval and afterwards getting an interval no longer being empty since its lower bound no longer is smaller than its upper bound? Are you, by chance, confusing an _interval_ with a tuple of points? You can _implement_ a closed interval as a tuple of points, but that does not lend meaning to shifting an empty interval. So just checking for emptiness isn't enough. For mimicking all side effects of the current implementation, it isn't. But if some code relies on those side effects, it is broken in my opinion. If you want to manipulate a two-dimensional vector without inherent relation between its two elements, use a Drul, not an Interval. And frankly, shouldn't we rather put this into flower/include/interval.hh instead of trying to catch this in arbitrary places in our code? I had toyed with this idea - this is a bit out of my league, as I don't know if LilyPond will take a performance hit if we add extra operations to something as elemental as translate or is_empty. Every piece of inherent safety takes a performance hit. Personally I think that the only case where we have is_empty true is for (+inf, -inf). And I don't mean that we should change the test: the test is fine. There just isn't a combination of interval bounds that makes more sense. We could equally well define the empty interval as (0, -1) and get consistent behavior from that. But while the interval is empty, its bounds should not be interpreted as conveying meaning. If all empty intervals are the same then, by design, we need to make sure that all of them equal (+inf.0 . -inf.0). This means that: --) Interval should have a method set_to_empty () that sets it to (+inf.0 . -inf.0) (maybe it already does). --) Anytime an interval is set to its left being greater than its right such that it is not infinitely empty, a programming error should be issued. --) Perhaps all math done on empty intervals should raise programming errors. I'm all for making the code more coherent. It sounds like we're talking about a much deeper problem than this patch, and perhaps it's wiser to come up w/ a solution to that first before pushing this. Cheers, MS http://codereview.appspot.com/6303065/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Sets TabVoice Stem height to ##f (issue 6303065)
On 2012/06/12 13:47:34, MikeSol wrote: On 2012/06/12 13:43:09, dak wrote: If a type is named Interval, it needs to be employed as an Interval, not as something totally different that relies on implementation details. Otherwise type abstraction makes code _less_ maintainable rather than more, since you always need to take all side-effects into consideration. Dunno - it sounds like time for clean up. Maybe. It does not make sense in my opinion to pepper the whole code with fixups intended to cater for cases where intervals are supposed to be used as non-intervals, or with fixups in order to cater for cases where intervals are supposed to be used as intervals, and we can't put the respective fixes into the interval implementation itself since it would violate the assumptions of those uses where intervals are used as non-intervals. Y-position of beams were stored as intervals for years (they may still be - I forget). Storing Y-positions of beams in intervals is fine when we are talking about a possible set of definitions for Y-positions at one point. When we are talking about lower interval bound is left Y-position, higher interval bound is right Y-position, and left Y-position may be higher than right Y-position, we are venturing into the domain of nonsense. This sounds like a pretty major task, so as always, I'd touch base w/ Han-Wen to see what Intervals were supposed to be at their inception and then evaluate what they've grown into. We can then firm up an Interval API and write a strongly-worded comment in interval.hh and interval.tcc NOT to touch either of these files. Sounds like it would make sense. And _if_ we have use cases for ordered point/value pairs (like using Linear_combination or whatever), then we should make sure that those are available on _appropriate_ data types as well rather than misusing Intervals for them because their internals happen to be convenient for that. http://codereview.appspot.com/6303065/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Sets TabVoice Stem height to ##f (issue 6303065)
On 2012/06/11 05:34:23, MikeSol wrote: This shows a case where stem height cannot be determined by stem stencil presence. The first version of the patch works under the assumption that information about height cannot be gleaned from the stencil and must be made explicit through overrides. I think that, even though this requires redundancy (specifying no stencil and an empty height), if LilyPond treats cases where there is no stencil and a non-empty height (like beam-stem-stemlet.ly), then the method in the first patch set is better. What do you mean with treats cases? That it implements some useful behavior based on the exact given height? Or merely that it catches the case? Personally, I consider it an accident waiting to happen if downing the stencil is not enough to suppress its consideration. _Iff_ there is a situation where it is really required to have a non-zero height, then setting the stencil to ##f is the wrong measure: it should be a point-stencil instead. http://codereview.appspot.com/6303065/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Sets TabVoice Stem height to ##f (issue 6303065)
On 2012/06/11 07:10:48, dak wrote: On 2012/06/11 05:34:23, MikeSol wrote: This shows a case where stem height cannot be determined by stem stencil presence. The first version of the patch works under the assumption that information about height cannot be gleaned from the stencil and must be made explicit through overrides. I think that, even though this requires redundancy (specifying no stencil and an empty height), if LilyPond treats cases where there is no stencil and a non-empty height (like beam-stem-stemlet.ly), then the method in the first patch set is better. What do you mean with treats cases? That it implements some useful behavior based on the exact given height? Or merely that it catches the case? Personally, I consider it an accident waiting to happen if downing the stencil is not enough to suppress its consideration. _Iff_ there is a situation where it is really required to have a non-zero height, then setting the stencil to ##f is the wrong measure: it should be a point-stencil instead. So when stencil is #f for stems, the height should always be empty? In this case, patch set 2 was the correct approach, and the fact that it failed regtests reveals a hidden bug in the handling of beamed rests under tuplets. If you think this is the best way to tackle it, then I'll do that. It'll likely mean a larger patch as it'll unearth some bugs that didn't surface with the stem work in 2.15. http://codereview.appspot.com/6303065/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Sets TabVoice Stem height to ##f (issue 6303065)
On 2012/06/11 07:24:33, MikeSol wrote: On 2012/06/11 07:10:48, dak wrote: Personally, I consider it an accident waiting to happen if downing the stencil is not enough to suppress its consideration. _Iff_ there is a situation where it is really required to have a non-zero height, then setting the stencil to ##f is the wrong measure: it should be a point-stencil instead. So when stencil is #f for stems, the height should always be empty? In this case, patch set 2 was the correct approach, and the fact that it failed regtests reveals a hidden bug in the handling of beamed rests under tuplets. Not necessarily a hidden bug, but rather a hidden expectation. If LilyPond will, when setting the stencil to #f for some reason of its own, accompany this with consistent height data, then the behavior might be consistent and not exhibit problems. Passing information with that sort of secret contract, however, is a bad idea when users and/or other programmers are expected to be able to unstencil grobs. In that case, passing information in the height (or otherwise manipulating it) without taking the nonexistence of the stencil into consideration is a rather bad idea. If you think this is the best way to tackle it, then I'll do that. It'll likely mean a larger patch as it'll unearth some bugs that didn't surface with the stem work in 2.15. As I said: these may not necessarily bugs but rather larvae: things that are rather certain to grow into bugs under reasonably natural conditions. I think it would be worth the trouble to get rid of them. In case we have code that relies on passing information through the Y-height of a stencil even when the stencil has been set to #f by the user, this will require finding a different solution. http://codereview.appspot.com/6303065/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Sets TabVoice Stem height to ##f (issue 6303065)
On 2012/06/11 07:48:34, dak wrote: On 2012/06/11 07:24:33, MikeSol wrote: On 2012/06/11 07:10:48, dak wrote: Personally, I consider it an accident waiting to happen if downing the stencil is not enough to suppress its consideration. _Iff_ there is a situation where it is really required to have a non-zero height, then setting the stencil to ##f is the wrong measure: it should be a point-stencil instead. So when stencil is #f for stems, the height should always be empty? In this case, patch set 2 was the correct approach, and the fact that it failed regtests reveals a hidden bug in the handling of beamed rests under tuplets. Not necessarily a hidden bug, but rather a hidden expectation. If LilyPond will, when setting the stencil to #f for some reason of its own, accompany this with consistent height data, then the behavior might be consistent and not exhibit problems. Passing information with that sort of secret contract, however, is a bad idea when users and/or other programmers are expected to be able to unstencil grobs. In that case, passing information in the height (or otherwise manipulating it) without taking the nonexistence of the stencil into consideration is a rather bad idea. If you think this is the best way to tackle it, then I'll do that. It'll likely mean a larger patch as it'll unearth some bugs that didn't surface with the stem work in 2.15. As I said: these may not necessarily bugs but rather larvae: things that are rather certain to grow into bugs under reasonably natural conditions. I think it would be worth the trouble to get rid of them. In case we have code that relies on passing information through the Y-height of a stencil even when the stencil has been set to #f by the user, this will require finding a different solution. My compromise solution in the most recent patch set is to use Keith's idea but to leave a comment so that people know that beam, stencil-less stems are treated as having a height of 0 at the point of the beam whereas unbeamed stems are heightless. The former is the case because otherwise the beam doesn't know where to go - it seems like a reasonable exception. http://codereview.appspot.com/6303065/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Sets TabVoice Stem height to ##f (issue 6303065)
LGTM if you re-read the comment before you push. http://codereview.appspot.com/6303065/diff/9001/lily/stem.cc File lily/stem.cc (right): http://codereview.appspot.com/6303065/diff/9001/lily/stem.cc#newcode707 lily/stem.cc:707: return an empty interval when there is no beam. The comment doesn't exactly match the code (we the full stem interval for the imaginary stems on beamed rests, not just a point) so take another read before you push. Maybe let the code show what you do, and just say why you do it. If there is a beam but no stem, slope calculations depend on this routine to return where the stem end /would/ be. http://codereview.appspot.com/6303065/diff/9001/lily/stem.cc#newcode710 lily/stem.cc:710: if (calc_beam !beam !unsmob_stencil (me-get_property (stencil))) If you have time to test the output, consider removing the test for calc_beam, and combining with similar line 692. http://codereview.appspot.com/6303065/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Sets TabVoice Stem height to ##f (issue 6303065)
Reviewers: ,
Message:
Another way of going about this would be changing the Stem::height
function so that it returned an empty interval for stencil-less stems.
Then the overrides wouldn't be necessary. It's a design question more
than anything else.
Description:
Sets TabVoice Stem height to ##f
Please review this at http://codereview.appspot.com/6303065/
Affected files:
M lily/grob.cc
M ly/engraver-init.ly
M ly/property-init.ly
Index: lily/grob.cc
diff --git a/lily/grob.cc b/lily/grob.cc
index
cc39c979f0e9cf146b40975b01f82ed9441d08bc..828ae7f07b40687457f8031475f0a42d9992dc67
100644
--- a/lily/grob.cc
+++ b/lily/grob.cc
@@ -466,7 +466,10 @@ Grob::extent (Grob *refp, Axis a) const
((Grob *)this)-dim_cache_[a].extent_ = new Interval (real_ext);
}
- real_ext.translate (offset);
+ // We never want nan, so we avoid shifting infinite values.
+ for (LEFT_and_RIGHT (d))
+if (!isinf (real_ext[d]))
+ real_ext[d] += offset;
return real_ext;
}
Index: ly/engraver-init.ly
diff --git a/ly/engraver-init.ly b/ly/engraver-init.ly
index
9bb731883cba5d9754fdc7c2030508b49bde06f4..2c28af1f23f0472b34887bed7e3cbf3fa577d330
100644
--- a/ly/engraver-init.ly
+++ b/ly/engraver-init.ly
@@ -825,6 +825,9 @@ context.
%% after all, the stubs of the stems may still be visible, so ...
\override Stem #'stencil = ##f
\override Flag #'stencil = ##f
+ %% and they certainly don't have heights...
+ \override Stem #'Y-extent = ##f
+ \override Flag #'Y-extent = ##f
%% automatic beams should be suppressed for similar reasons ...
autoBeaming = ##f
%% remove beams, dots and rests ...
Index: ly/property-init.ly
diff --git a/ly/property-init.ly b/ly/property-init.ly
index
c0b352254ad0f195cb20d99465180ca5c706440a..edd117518a458c76460a277b48982c24cd048f2d
100644
--- a/ly/property-init.ly
+++ b/ly/property-init.ly
@@ -451,6 +451,8 @@ tabFullNotation = {
\revert TabVoice.Stem #'details
\revert TabVoice.Stem #'stencil
\revert TabVoice.Flag #'stencil
+ \revert TabVoice.Stem #'Y-extent
+ \revert TabVoice.Flag #'Y-extent
\override TabVoice.Stem #'stencil =
#tabvoice::draw-double-stem-for-half-notes
\override TabVoice.Stem #'X-extent =
#tabvoice::make-double-stem-width-for-half-notes
\set TabVoice.autoBeaming = ##t
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Sets TabVoice Stem height to ##f (issue 6303065)
On 2012/06/10 18:22:50, MikeSol wrote: Another way of going about this would be changing the Stem::height function so that it returned an empty interval for stencil-less stems. I'd consider that eminently reasonable. It makes much more sense to me than having to wipe out some non-sensical resulting height explicitly. http://codereview.appspot.com/6303065/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
