On 2020/04/25 07:29:06, hanwenn wrote: > On 2020/04/24 22:04:52, dak wrote: > > On 2020/04/24 21:33:12, Carl wrote: > > > On 2020/04/24 21:19:57, dak wrote: > > > > On 2020/04/24 21:18:12, dak wrote: > > > > > > > > > > > > > > > https://codereview.appspot.com/579630043/diff/555740043/lily/stencil-integral.cc > > > > > File lily/stencil-integral.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.appspot.com/579630043/diff/555740043/lily/stencil-integral.cc#newcode465 > > > > > lily/stencil-integral.cc:465: // more convoluted, but it's fairly hot > > path. > > > > > Sorry for not being clear: the question was not why this change was > > > effective > > > > in > > > > > saving time, but why it was valid. When thickness is zero, you only > > update > > > > the > > > > > upper skyline. Why would the lower skyline no longer need updating? > > > > > > > > Well, other way round, but apart from that the question stands. > > > > > > When thickness is zero, the upper and lower curves are the same. Either one > > > completes the skyline. > > > > Of course they are the same. That is not the question. The question is why I > > am considering the identical curve for the minimum skyline while I don't let > it > > participate in the maximum skyline any more. > > > > If everything is of thickness null, the maximum skyline will be non-existent > > while the minimum skyline is there. While the skylines should be identical > > rather than only one of them being there. > > the Direction d is up/down, but note that it calculates d * > normal(curve-direction), so you get to two curves that correspond to the outer > and inner curve of a stroked bezier. > > The curve has no particular orientation relative to the axes, so -assuming the > previous code was correct- it can't be that the UP points are for the UP > skyline.
Right. At the end of the function, both curves are added into boxes without orientation. Which begs the question why they are stored into a points array in the first place. Adding into successive boxes requires just remembering the respective last point pair. Also some conditions read "th > 0" while others read "th == 0" and they depend on one another for their behavior. There is nothing that precludes negative values in this code, so it seems like all tests should be consistent (not quite sure what the implications of negative th should be: one could argue forcing to zero as well as just taking the absolute value). > > You can see this happening here near the bottom of the function: the points are > line segments, and then each line segment is interpreted as a box. The boxes are > then added to the curve. > > I think this all can be more succinctly expressed by just calculating the curve > once and fattening the resulting box by .5 thickness on all sides, but all the > same, it would be better to get rid of the boxes entirely, and work from curves > directly to skylines. > > I have plans to do this, but they require overhaul of calling conventions in the > stencil -> {box,building} -> skyline pipeline. https://codereview.appspot.com/579630043/