|
|
DescriptionMinor cleanups in stencil-integral.cc
Patch Set 1 #Patch Set 2 : save another curve_point call #Patch Set 3 : comment #
Total comments: 2
MessagesTotal messages: 10
save another curve_point call
Sign in to reply to this message.
comment
Sign in to reply to this message.
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... 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?
Sign in to reply to this message.
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... > 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.
Sign in to reply to this message.
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... > > 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.
Sign in to reply to this message.
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... > > > 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.
Sign in to reply to this message.
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... > > > > 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. 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.
Sign in to reply to this message.
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... > > > > > 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.
Sign in to reply to this message.
On 2020/04/25 08:32:40, dak wrote: > > I have plans to do this, but they require overhaul of calling conventions in > the > > stencil -> {box,building} -> skyline pipeline. this was actually much simpler than I thought. See https://codereview.appspot.com/581960043/
Sign in to reply to this message.
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... lily/stencil-integral.cc:465: // more convoluted, but it's fairly hot path. On 2020/04/24 21:18:12, dak wrote: > 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? Acknowledged.
Sign in to reply to this message.
|