Code review - Issue 6303065: Sets TabVoice Stem height to ##fhttps://codereview.appspot.com/2012-07-14T07:52:16+00:00rietveld
Message from unknown
2012-06-10T18:19:56+00:00MikeSolurn:md5:0379747ce56eb9687149331e94059d73
Message from mtsolo@gmail.com
2012-06-10T18:22:50+00:00MikeSolurn:md5:353799a0e87a3c2fb2ea019e2fd2ed2d
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.
Message from dak@gnu.org
2012-06-10T18:32:58+00:00dakurn:md5:a9dc6a8c96b8807ff7ebe3462930a5c0
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.
Message from k-ohara5a5a@oco.net
2012-06-11T03:44:32+00:00Keithurn:md5:aafe996c83dc72dae2d0875efeff3315
http://codereview.appspot.com/6303065/diff/4001/lily/stem.cc
File lily/stem.cc (right):
http://codereview.appspot.com/6303065/diff/4001/lily/stem.cc#newcode703
lily/stem.cc:703: if (calc_beam && !unsmob_stencil (me->get_property ("stencil")))
beam-stem-stemlet.ly shows that LilyPond produces no stencil for the invisible stems on beamed rests, yet she depends on stem extents for tuplet brackets. Maybe
if (calc_beam && !beam && !stenxil) ?
Message from mtsolo@gmail.com
2012-06-11T05:34:23+00:00MikeSolurn:md5:8948ab3c67c13ce93b4279a42f81328f
On 2012/06/11 03:44:32, Keith wrote:
> http://codereview.appspot.com/6303065/diff/4001/lily/stem.cc
> File lily/stem.cc (right):
>
> http://codereview.appspot.com/6303065/diff/4001/lily/stem.cc#newcode703
> lily/stem.cc:703: if (calc_beam && !unsmob_stencil (me->get_property
> ("stencil")))
> beam-stem-stemlet.ly shows that LilyPond produces no stencil for the invisible
> stems on beamed rests, yet she depends on stem extents for tuplet brackets.
> Maybe
> if (calc_beam && !beam && !stenxil) ?
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.
Message from dak@gnu.org
2012-06-11T07:10:48+00:00dakurn:md5:cf60611997cfcb2062dc6b1eba51a443
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.
Message from mtsolo@gmail.com
2012-06-11T07:24:33+00:00MikeSolurn:md5:8ccde779926574a5f99fbf396b2101c4
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.
Message from dak@gnu.org
2012-06-11T07:48:34+00:00dakurn:md5:4fb1a066192f253ca9171786295521d6
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.
Message from unknown
2012-06-11T11:44:33+00:00MikeSolurn:md5:279ce453cc4f74ff05aac2988491f67c
Message from mtsolo@gmail.com
2012-06-11T11:46:31+00:00MikeSolurn:md5:ce20264d6716918610f205f1938c316b
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.
Message from k-ohara5a5a@oco.net
2012-06-11T16:33:27+00:00Keithurn:md5:f4986cce90e507905b89e9985fc6a967
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.
Message from unknown
2012-06-11T18:55:52+00:00MikeSolurn:md5:e9a1350c45ed8222a48f353d40e975f0
Message from dak@gnu.org
2012-06-12T12:32:37+00:00dakurn:md5:1d10714f3118ba10ccd354efdfc845d1
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?
Message from mtsolo@gmail.com
2012-06-12T12:39:23+00:00MikeSolurn:md5:af1d247f46fdf4a9acadd3e6488265e9
On 2012/06/12 12:32:37, 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;
> 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?
Sometimes there is an infinite offset (either positive or negative) which, when added to an empty interval of (+inf.0 . -inf.0), will create nan (or -nan) in one of the two directions.
Message from dak@gnu.org
2012-06-12T12:49:45+00:00dakurn:md5:628310887bb446c3e93812c52e7c70b1
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?
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?
Message from mtsolo@gmail.com
2012-06-12T12:54:40+00:00MikeSolurn:md5:6b9fb502eb49f652034ada0f72b59f1f
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. So (3 . 2) is empty, but (3 . 2) can be translated by an infinite value in either direction and not result in nan. So just checking for emptiness isn't enough.
An infinite value in an interval should not be translated by another infinite value. It either does nothing (if they both have the same sign) or results in a nan.
> 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. If this is appropriate, then the check can go there.
Message from dak@gnu.org
2012-06-12T13:22:10+00:00dakurn:md5:1b48ed4d9f28786317f20977440683da
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.
Message from mtsolo@gmail.com
2012-06-12T13:27:23+00:00MikeSolurn:md5:ec8bb0a00f8c7bbde004391e7c79ae19
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
Message from dak@gnu.org
2012-06-12T13:43:09+00:00dakurn:md5:8013fb8786a1aa15577f65e8b9feae6d
Here is a code example from beam.cc:
Interval weights (1 - multiplier, multiplier);
if (feather_dir != LEFT)
weights.swap ();
This is _hogwash_. weights is not an _interval_ here, but a pair of numbers. Swapping the bounds of an interval does not even make _sense_. Where is the point in type abstraction if one creates total conceptual chaos without inherent relation to the invariants of the mathematical model underlying the type?
swap never should have been a public member function (it is used internally when mirroring intervals).
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.
Message from mtsolo@gmail.com
2012-06-12T13:47:34+00:00MikeSolurn:md5:ac4cb04f2ad611c12d369f159dfd9c01
On 2012/06/12 13:43:09, dak wrote:
> Here is a code example from beam.cc:
>
> Interval weights (1 - multiplier, multiplier);
>
> if (feather_dir != LEFT)
> weights.swap ();
>
> This is _hogwash_. weights is not an _interval_ here, but a pair of numbers.
> Swapping the bounds of an interval does not even make _sense_. Where is the
> point in type abstraction if one creates total conceptual chaos without inherent
> relation to the invariants of the mathematical model underlying the type?
>
> swap never should have been a public member function (it is used internally when
> mirroring intervals).
>
> 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.
Y-position of beams were stored as intervals for years (they may still be - I forget).
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.
Message from dak@gnu.org
2012-06-12T14:09:35+00:00dakurn:md5:1cd6c2e5bf30271a793c5cec774be250
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.
Message from dak@gnu.org
2012-06-15T07:33:31+00:00dakurn:md5:2ad177044add93ebafe8fd3373f4249d
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 inf<inf can't be determined.
Could you please explain the cases where you expect to get sensible
results with that approach?
Message from mike@apollinemike.com
2012-06-15T16:36:45+00:00mike_apollinemike.comurn:md5:43ba8755d541f854e30fbb11b1c4281a
On 15 juin 2012, at 09:33, dak@gnu.org wrote:
> 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 inf<inf can't be determined.
>
> Could you please explain the cases where you expect to get sensible
> results with that approach?
>
Hey,
Sorry - I've been swamped the past few days w/ concerts and forgot about your suggestion. I'll take some time this weekend or next week to think it through and let you know.
Cheers,
MS
Message from k-ohara5a5a@oco.net
2012-07-14T07:52:16+00:00Keithurn:md5:e3a008fe93491b676d9d799e5acf591e
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.