Re: Skylines: reject steep-sloped buildings; issue 3383 (issue 9890045)

[David Kastrup]

"Keith OHara" <address@hidden> writes:

> On Mon, 03 Jun 2013 12:03:26 -0700, <address@hidden> 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