Issue3383: old-straight-flag + smaller Stem.thickness gives no output and huge over

Issue3383: old-straight-flag + smaller Stem.thickness gives no output and huge over

This replaces the numerically imprudent y_intercept_ in buildings with
start_height_ and adapts calculations.


The one thing I have no actual clue about is what
Skyline::set_minimum_height does and how.  So the code in there might
be wrong.  It is based on the assumption that a skyline starts at
minus infinity and has its first start_height_ equal to the previous
y_intercept_.

[dak, 2013-06-07 17:23:20]

Try mimicking old behavior more slavishly

[Keith, 2013-06-08 05:59:45]

I thought you were suggesting to store the endpoints (not slopes) and do
interpolation between the endpoints.  

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

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".
The output really should not be sensitive to the changes you make below. 
Building::conceals() should be simply height()>other.height
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.

[dak, 2013-06-08 08:13:29]

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.

[dak, 2013-06-08 10:52:34]

Make a pitch for addressing Keith's concerns

[dak, 2013-06-08 13:30:17]

When adjusting start_, need to adjust start_height_ as well

[dak, 2013-06-08 13:53:51]

Replace b.height (b.start_) with b.start_height_ where applicable