Re: Issue3383: old-straight-flag + smaller Stem.thickness gives no output and huge over (issue 10086043)

[dak]

On 2013/06/08 05:59:45, Keith wrote:
> I thought you were suggesting to store the endpoints (not slopes) and
do
> interpolation between the endpoints.

Well, divisions _are_ expensive, so storing the inverse makes sense.
With floating point, it is not a significant source of error.
Extrapolating from x=0 rather than start_ is much worse.

> This change doesn't exactly simplify the code.  This still stores
slopes that
> can be completely imprecise due to round-off error.

Uh, no?  The slope is (end_height - start_height)/(end - start).  The
division does not lose significant precision because it is floating
point, and the subtractions rather produce excess precision over the
quantization of the starting values.  You are right that because of the
quantization of the starting values, the slope may be rather imprecise,
just as it was before.  However, since we apply the slope only starting
on (start, start_height), the error from the application does not exceed
the original error of the height specification as long as we are
interpolating, namely staying before end.

> I see that by using the
> start_height_ instead of the intercept you keep the resulting errors
small, but
> I can easily imagine someone coming along, thinking the code would be
simpler if
> we base the sloped lines on a y_intercept at x=0, and unknowingly
re-create the
> problem you just fixed.

I have a hard time figuring out what you are objecting to.  Are you
objecting to the code, or would you want warnings/explanations in
comments?

> https://codereview.appspot.com/10086043/diff/4/lily/skyline.cc
> File lily/skyline.cc (right):


https://codereview.appspot.com/10086043/diff/4/lily/skyline.cc#newcode194
> lily/skyline.cc:194: // Since the code is pretty sensitive to the
exact
> semantics, we try
> Probably you mean "semantics is sensitive to the exact code".

No.  Slightly change the semantics, and the code explodes around your
ears.

> The output really should not be sensitive to the changes you make
below.

It is.  To the degree where LilyPond dies from memory starvation for the
simplest of files.

> Building::conceals() should be simply height()>other.height

That one took a definite hit on memory usage on make check but I can't
rule out that this was just a test-baseline that was slightly too old
(in which case some commit in between has had a definite memory impact).

If you use height () >= other.height (), LilyPond dies from memory
exhaustion.  It is my opinion that this kind of change should not cause
a crash like that, but then it would appear that some caller of
"conceals" depends on some particular semantics that are, of course,
nowhere documented.

> The version in master seems written the way it was to save one or two
> floating-point multiplies (but at the cost of a few tests and
branches).


https://codereview.appspot.com/10086043/diff/4/lily/skyline.cc#newcode209
> lily/skyline.cc:209: return slope_ > other.slope_ ? height (x) >=
other.height
> (x)
> What test changed that forced you to add this test?
> It might be simply a png-preview artifact.



https://codereview.appspot.com/10086043/