Re: Fix Issue 1290 (issue3832046)

[joeneeman]

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode396
lily/skyline.cc:396: if ((i->slope_ == 0) && !isinf (i->y_intercept_) &&
i->y_intercept_ >= 0)
On 2011/01/03 03:48:09, Carl wrote:
> On 2011/01/02 05:19:58, joeneeman wrote:
> > Why restrict to y_intercept_ < 0?
>   Because if I have a y intercept that is less than zero, and create a
box with
> that y intercept, the padded skyline comes out with all zero heights.
I don't
> know why it works that way, but it does.

That's very strange. Do you have an example file that triggers this
behaviour?

> I tried several things to get it to behave differently, but was
unsuccessful.
> And I don't think that a negative skyline will ever be limiting in
inter-system
> spacing, so I didn't feel that it was a problem.

Probably not, but I still feel that it's bad to have methods with
strange corner cases. At the very least, there should be a comment
explaining that the distance function will throw away any constraints
with negative height.

http://codereview.appspot.com/3832046/diff/27001/lily/skyline.cc#newcode391
lily/skyline.cc:391: {
I still don't understand why this function is so complicated. Does this
not work?

Real start = -infinity_f;
vector<Box> boxes;
list<Building>::const_iterator end = src.buildings_.end ();
for (list<Building>::const_iterator i = src.buildings_.begin (); i !=
end; sta
     rt=i->end_, i++)
  if ((i->slope_ == 0) && !isinf (i->y_intercept_))
    boxes.push_back (Box (Interval (start, i->end_),
                          Interval (-infinity_f, i->y_intercept_)));

buildings_ = internal_build_skyline (&boxes, horizon_padding, X_AXIS,
UP);
sky_ = src.sky_;

By the way, the above code (and your version also) will break if we
every switch to using more accurate outlines for grobs (ie. more
accurate than bounding boxes) because it could be throwing away
interesting things when it discards non-horizontal segments.

http://codereview.appspot.com/3832046/diff/27001/lily/skyline.cc#newcode419
lily/skyline.cc:419: Axis vert_axis = other_axis (horizon_axis);
This should have X_AXIS instead of "a", because you put the horizon axis
into the X_AXIS of your boxes.

http://codereview.appspot.com/3832046/