lilypond-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Fix Issue 1290 (issue3832046)


From: Carl Sorensen
Subject: Re: Fix Issue 1290 (issue3832046)
Date: Fri, 7 Jan 2011 10:06:49 -0700



On 1/7/11 9:47 AM, "address@hidden" <address@hidden> wrote:

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

The regression test file in the patch triggers this behavior.

> 
>> 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_;

I will try this.  I have not yet tried it. I'll get back to you on the
results.

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

That is true, but at that point we will not be using Boxes to define
skylines.

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

I'll fix this.  Thanks for the comments.

Thanks,

Carl




reply via email to

[Prev in Thread] Current Thread [Next in Thread]