Use cheapComputeDirection to determine normal facing in GrAAConvexPathRenderer

[bsalomon, 2012-02-13 15:59:00]

http://codereview.appspot.com/5649083/diff/1/include/core/SkPoint.h
File include/core/SkPoint.h (left):

http://codereview.appspot.com/5649083/diff/1/include/core/SkPoint.h#oldcode445
include/core/SkPoint.h:445: if (kLeft_Side == side) {
When I first this code I forgot that y points down (unlike the way most people
would normally draw a Cartesian graph).

[guanqun, 2012-02-14 02:05:00]

Thanks for the fixup, I ran the GM and it renders correctly for convexpaths.

But one thing I'm a bit unclear is: fanPt sometimes get value nan, is it safe to
assign it to a point?

Aside from that, LGTM

http://codereview.appspot.com/5649083/diff/1/src/gpu/GrAAConvexPathRenderer.cpp
File src/gpu/GrAAConvexPathRenderer.cpp (right):

http://codereview.appspot.com/5649083/diff/1/src/gpu/GrAAConvexPathRenderer.c...
src/gpu/GrAAConvexPathRenderer.cpp:235: path.cheapComputeDirection(&dir);
I didn't notice there's such an API. Quite useful here. :)

[guanqun, 2012-02-14 07:29:49]

On 2012/02/14 02:05:00, guanqun wrote:
> But one thing I'm a bit unclear is: fanPt sometimes get value nan, is it safe
to
> assign it to a point?

I checked with OpenGL spec:

"""
In IEEE arithmetic, for example, providing a negative zero or a
denormalized number to a GL command yields predictable results, while providing
a NaN or an infinity yields unspecified results.
"""

So I think we should fix that too?

[bsalomon, 2012-02-15 16:22:51]

On 2012/02/14 02:05:00, guanqun wrote:
> Thanks for the fixup, I ran the GM and it renders correctly for convexpaths.
> 
> But one thing I'm a bit unclear is: fanPt sometimes get value nan, is it safe
to
> assign it to a point?

Yeah, that's really bad :) The set of points used to compute the fan point can
be a degenerate (zero area poly) even though the path itself is not. The updated
patch just returns the average of the points in that case (which should be
rare).

We could use all the control points to calculate the fan point, but then we'd
have to handle tessellating a quad where the hull contains the fan point
correctly.

> 
> Aside from that, LGTM
> 
>
http://codereview.appspot.com/5649083/diff/1/src/gpu/GrAAConvexPathRenderer.cpp
> File src/gpu/GrAAConvexPathRenderer.cpp (right):
> 
>
http://codereview.appspot.com/5649083/diff/1/src/gpu/GrAAConvexPathRenderer.c...
> src/gpu/GrAAConvexPathRenderer.cpp:235: path.cheapComputeDirection(&dir);
> I didn't notice there's such an API. Quite useful here. :)

The API was added by reed fairly recently.

Adding reed as a reviewer.

[bsalomon, 2012-02-15 16:53:16]

On 2012/02/15 16:22:51, bsalomon wrote:
> On 2012/02/14 02:05:00, guanqun wrote:
> > Thanks for the fixup, I ran the GM and it renders correctly for convexpaths.
> > 
> > But one thing I'm a bit unclear is: fanPt sometimes get value nan, is it
safe
> to
> > assign it to a point?
> 
> Yeah, that's really bad :) The set of points used to compute the fan point can
> be a degenerate (zero area poly) even though the path itself is not. The
updated
> patch just returns the average of the points in that case (which should be
> rare).
> 
> We could use all the control points to calculate the fan point, but then we'd
> have to handle tessellating a quad where the hull contains the fan point
> correctly.
> 
> > 
> > Aside from that, LGTM
> > 
> >
>
http://codereview.appspot.com/5649083/diff/1/src/gpu/GrAAConvexPathRenderer.cpp
> > File src/gpu/GrAAConvexPathRenderer.cpp (right):
> > 
> >
>
http://codereview.appspot.com/5649083/diff/1/src/gpu/GrAAConvexPathRenderer.c...
> > src/gpu/GrAAConvexPathRenderer.cpp:235: path.cheapComputeDirection(&dir);
> > I didn't notice there's such an API. Quite useful here. :)
> 
> The API was added by reed fairly recently.
> 
> Adding reed as a reviewer.

I got a verbal LGTM from reed, committed at r3198

[guanqun, 2012-02-16 05:31:15]

On 2012/02/15 16:22:51, bsalomon wrote:
> Yeah, that's really bad :) The set of points used to compute the fan point can
> be a degenerate (zero area poly) even though the path itself is not. The
updated
> patch just returns the average of the points in that case (which should be
> rare).
> 
> We could use all the control points to calculate the fan point, but then we'd
> have to handle tessellating a quad where the hull contains the fan point
> correctly.

What's the benefit of finding the fanPt and using it? Since it's a convex
polygon, can we simply use the first end point in segments as the fan point?
Therefore we can save the cost of calculation. I've tried that idea and found
there are few pixel differences on the edges of lines for some cases.

Thanks!

[bsalomon, 2012-02-16 13:21:52]

On 2012/02/16 05:31:15, guanqun wrote:
> On 2012/02/15 16:22:51, bsalomon wrote:
> > Yeah, that's really bad :) The set of points used to compute the fan point
can
> > be a degenerate (zero area poly) even though the path itself is not. The
> updated
> > patch just returns the average of the points in that case (which should be
> > rare).
> > 
> > We could use all the control points to calculate the fan point, but then
we'd
> > have to handle tessellating a quad where the hull contains the fan point
> > correctly.
> 
> What's the benefit of finding the fanPt and using it? Since it's a convex
> polygon, can we simply use the first end point in segments as the fan point?
> Therefore we can save the cost of calculation. I've tried that idea and found
> there are few pixel differences on the edges of lines for some cases.
> 
> Thanks!

The AA softens pixels within 1 pixel distance of both the interior and exterior
sides of the path's boundary. If the fan point lies on a straight edge (or
within 1 pixel of a curved edge) then the interior part of the AA isn't applied.
You can see this on the two shapes you added to the GM. In GPU mode the left
edge gets no AA because the fan point lies on it.

[guanqun, 2012-02-16 13:56:10]

On 2012/02/16 13:21:52, bsalomon wrote:
> The AA softens pixels within 1 pixel distance of both the interior and
exterior
> sides of the path's boundary. If the fan point lies on a straight edge (or
> within 1 pixel of a curved edge) then the interior part of the AA isn't
applied.
> You can see this on the two shapes you added to the GM. In GPU mode the left
> edge gets no AA because the fan point lies on it.

By magnifying the picture and comparing pixel by pixel, I get what you mean by
"softening pixels", thanks for your detailed explaination. :D