[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Skylines: reject steep-sloped buildings; issue 3383 (issue 9890045)
From: |
Keith OHara |
Subject: |
Re: Skylines: reject steep-sloped buildings; issue 3383 (issue 9890045) |
Date: |
Mon, 03 Jun 2013 22:58:04 -0700 |
User-agent: |
Opera Mail/12.15 (Win32) |
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/