Re: Minor cleanups in stencil-integral.cc (issue 579630043 by address@hidden)

[dak]

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/