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

[Keith OHara]

On Mon, 03 Jun 2013 12:03:26 -0700, <address@hidden> wrote:

> 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.
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, 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.

Interpolation would also be easier to understand where the Building is used, 
but I am not ready to re-write the implementation of Skyline on my first trip 
in the code.

> 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.

Yes, some of them.  It was not zero-width Buildings that we wanted to keep, but 
zero-width stencils, so that users get sensible behavior when they write "" or 
point-stencil.

Skylines built from segments, as opposed to boxes, come from the stencil 
outlining that Mike added recently.  Often the outline has some vertical 
segments, and it is a bit of a waste to create Buildings for them, since they 
are adjacent to other Buildings that have width.

I guess this is an optimization, though, so I'll pull out this change next time 
I'm on Linux.

> https://codereview.appspot.com/9890045/