On Sat, Jul 14, 2012 at 8:33 PM, <mts...@gmail.com> wrote: > > http://codereview.appspot.com/**5626052/diff/85004/lily/** > skyline.cc#newcode302<http://codereview.appspot.com/5626052/diff/85004/lily/skyline.cc#newcode302> > lily/skyline.cc:302: while (dirty); > On 2012/07/14 14:01:08, joeneeman wrote: > >> 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. >> > > I'll rewrite this so that it combines all interior skylines with an > infinite y intercept and then only does one pass.
This sounds like a good idea. If you put the skyline combination part in a separate function, we could just use it after merges, etc, so that the property of not having adjacent empty buildings is always maintained. > http://codereview.appspot.com/**5626052/diff/90001/lily/** > include/skyline.hh<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<http://codereview.appspot.com/5626052/diff/90001/lily/include/skyline.hh#newcode58> > lily/include/skyline.hh:58: NOT_ENOUGH_INFO > On 2012/07/14 14:01:08, joeneeman wrote: > >> 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 >> > > This logic uses two calls to distance, which is expensive, whereas using > the e-num gets this info from one call to distance. I can use the > method you propose, but don't you think that'd cause a performance hit? Won't you need two skyline comparisons anyway? If T's up skyline is always above S's down skyline, then S and T may intersect but you won't know until you've compared T's down skyline with S's up skyline. Anyway, I'm always in favor of writing simple code first and then optimizing once things have been measured. Cheers, Joe
_______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
