Re: Sets TabVoice Stem height to ##f (issue 6303065)

[mtsolo]

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/