Skylines: reject steep-sloped buildings; issue 3383

Skylines: reject steep-sloped buildings; issue 3383

Skylines: comments to warn of potential endless loop

[lemzwerg, 2013-06-01 06:01:02]

LGTM.

[dak, 2013-06-03 19:03:26]

https://codereview.appspot.com/9890045/diff/1/lily/skyline.cc
File lily/skyline.cc (right):

https://codereview.appspot.com/9890045/diff/1/lily/skyline.cc#newcode117
lily/skyline.cc:117: slope_ = 0.0; /* if they were both infinite, we would get
nan, not 0, from the next line */
That comment is a piece of crock since it is completely unclear what "both" is
supposed to mean and "next line" is a comparison and can't yield nan.

https://codereview.appspot.com/9890045/diff/1/lily/skyline.cc#newcode128
lily/skyline.cc:128: else if (fabs(slope_) > 1e6)
That assumes that end - start is close to 0.  I see nothing that precludes end -
start to actually be 0, and then the above assert will fail anyway.

https://codereview.appspot.com/9890045/diff/1/lily/skyline.cc#newcode141
lily/skyline.cc:141: return isinf (x) ? y_intercept_ : slope_ * x +
y_intercept_;
Basically, the whole representation is a bad idea numerically.  If I understand
correctly, this is not going to extrapolate, anyway.  So the result should
always be an interpolation between the left and right y value, and instead of an
"y intercept", a left border y coordinate would make more sense.  Which is
available without further ado anyway.

https://codereview.appspot.com/9890045/diff/1/lily/skyline.cc#newcode527
lily/skyline.cc:527: if (x1 < x2)
That very much looks like silently dropping zero-width buildings from
consideration again.

[dak, 2013-06-04 14:46:33]

"Keith OHara" <k-ohara5a5a@oco.net> writes:

> On Mon, 03 Jun 2013 12:03:26 -0700, <dak@gnu.org> wrote:
>
> I was able to figure out the old comment, once I realized it explained
> the reason for the line it is one, while referring to the following
> lines.  I'll remove this change.
>
>> https://codereview.appspot.com/9890045/diff/1/lily/skyline.cc#newcode128
>> lily/skyline.cc:128: else if (fabs(slope_) > 1e6)
>> That assumes that end - start is close to 0.  I see nothing that
>> precludes end - start to actually be 0, and then the above assert will
>> fail anyway.
>>
>
> The added code is ready to handle the case start - end == 0 because
> fabs(±inf) > 1e6
>
> As you say, in a developer's build, the assert() would abort the
> program before reaching this point,

"assert" actually should abort also in production builds, cf issue 2787.
Its standard use is "the following code is not going to deal with
anything but the asserted values".  Switching off assertions via
-DNDEBUG only makes sense when memory is sparse and production
conditions mean that it does not make a difference just how a program is
going to crash, via failed assertion or followup error.

> but if an infinite slope leaks through from some unforeseen user input
> on a released build, I think we want to catch it here, rather than use
> slope-intercept form.
>
>> https://codereview.appspot.com/9890045/diff/1/lily/skyline.cc#newcode141
>> lily/skyline.cc:141: return isinf (x) ? y_intercept_ : slope_ * x +
>> y_intercept_;
>> Basically, the whole representation is a bad idea numerically.
>
> I tend to agree.  Interpolation from endpoints would be more accurate,
> and yes we do always interpolate inside buildings, but on the other
> hand accuracy is only an issue in cases of catastrophic failure like
> the infinite slope.  Based on comments and the way he wrote the code,
> Joe seemed to be concerned about speed (I would not be) and using
> slope-intercept form does save one division on each use.

Well, the nicest form is

y0 + (y1-y0) * ((x-x0)/(x1-x0))

but since we are talking about floating point here, we are still
reasonably well off using

y0 + (x-x0) * ((y1-y0)/(x1-x0))

which is still free from cancellation issues in the mantissa (the only
problem may be exceeding the exponent range).  As opposed to

(y0 - x0 * ((y1-y0)/(x1-x0))) + x * ((y1-y0)/(x1-x0))

which we have currently.  It saves one subtraction and stomps all over
y0 when the slope and/or the x coordinates are large in comparison.

-- 
David Kastrup