Just looked at skyline-related stuff for now...
http://codereview.appspot.com/5626052/diff/85004/lily/skyline.cc File lily/skyline.cc (right): http://codereview.appspot.com/5626052/diff/85004/lily/skyline.cc#newcode292 lily/skyline.cc:292: Real p1 = left->end_ * left->slope_ + left->y_intercept_; use left->height (left_end_) instead of calculating it by hand http://codereview.appspot.com/5626052/diff/85004/lily/skyline.cc#newcode293 lily/skyline.cc:293: Real p2 = i->start_ * i->slope_ + i->y_intercept_; i->height (i->start_) http://codereview.appspot.com/5626052/diff/85004/lily/skyline.cc#newcode295 lily/skyline.cc:295: center->y_intercept_ = p2 - (center->end_ * center->slope_); I think that *center = Building (center->start_, p1, p2, center->end_) is clearer http://codereview.appspot.com/5626052/diff/85004/lily/skyline.cc#newcode302 lily/skyline.cc:302: while (dirty); I don't understand this do while (dirty) part. It seems to me that unless you have two empty segments in a row, you only need one pass. On the other hand, if you have two empty segments in a row then this function will fail anyway because you will end up setting center->slope_ and center->y_intercept_ both to -infinity, which is bound to start creating NaN sooner or later. http://codereview.appspot.com/5626052/diff/90001/lily/include/skyline.hh File lily/include/skyline.hh (right): http://codereview.appspot.com/5626052/diff/90001/lily/include/skyline.hh#newcode58 lily/include/skyline.hh:58: NOT_ENOUGH_INFO I wish I could convince you to think of a skyline as a region instead of just the boundary of that region. Once you think of it that way, it becomes clear that this information can be easily obtained from the Skyline_pair, using the existing distance function. Suppose s and t are Skyline_pairs. max(s[UP].distance(t[DOWN]), s[DOWN].distance(t[UP])) = -infinity means the objects don't overlap horizontally at all, so it's meaningless to talk about which one is higher (I think this is what you're calling NOT_ENOUGH_INFO). min(s[UP].distance(t[DOWN]), s[DOWN].distance(t[UP])) > 0 means that the objects intersect. s[UP].distance(t[DOWN]) <= 0 and s[DOWN].distance(t[UP]) > 0 means that s is below t t[UP].distance(s[DOWN]) <= 0 and t[DOWN].distance(s[UP]) > 0 means that t is below s http://codereview.appspot.com/5626052/diff/90001/lily/skyline.cc File lily/skyline.cc (right): http://codereview.appspot.com/5626052/diff/90001/lily/skyline.cc#newcode796 lily/skyline.cc:796: */ Since padding doesn't seem to be involved in this function, you should delete this comment. http://codereview.appspot.com/5626052/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
