convex path renderer

[bsalomon, 2012-01-11 15:54:55]

Adds a new path renderer for convex paths. Disable for now. Will require GM
rebaselines when enabled. I need to fix an issue where hairlines would choose
this path renderer over the hairline renderer before enabling.
(SkDrawTreatAsHairline is overly restrictive for the gpu device.)

[reed1, 2012-01-12 18:49:26]

http://codereview.appspot.com/5533061/diff/6014/src/gpu/GrAAConvexPathRendere...
File src/gpu/GrAAConvexPathRenderer.cpp (right):

http://codereview.appspot.com/5533061/diff/6014/src/gpu/GrAAConvexPathRendere...
src/gpu/GrAAConvexPathRenderer.cpp:307: 
wow -- no obvious way to use a loop or small table? just curious.

http://codereview.appspot.com/5533061/diff/6014/src/gpu/GrGLProgram.cpp
File src/gpu/GrGLProgram.cpp (right):

http://codereview.appspot.com/5533061/diff/6014/src/gpu/GrGLProgram.cpp#newco...
src/gpu/GrGLProgram.cpp:540: if (GrDrawState::kQuad_EdgeType ==
fProgramDesc.fVertexEdgeType) {
They don't have a cool length(dfdx, dfdy) function (or invLength(...)? boohoo.

http://codereview.appspot.com/5533061/diff/6014/src/gpu/GrPathUtils.cpp
File src/gpu/GrPathUtils.cpp (right):

http://codereview.appspot.com/5533061/diff/6014/src/gpu/GrPathUtils.cpp#newco...
src/gpu/GrPathUtils.cpp:195: SkMatrix UVpts;
SK_Scalar1 is nice to see, but this code won't run correctly if scalar is fixed,
as SkMatrix will interpret the perspective elements as 2.30, not 16.16. You
could (if you like) just say 1, 0.5f, etc. instead.

http://codereview.appspot.com/5533061/diff/6014/src/gpu/GrPathUtils.cpp#newco...
src/gpu/GrPathUtils.cpp:207: matrix->setPerspY(0);
Do you want to add this?

    matrix->set(SkMatrix::kMPersp2] = 1;

http://codereview.appspot.com/5533061/diff/6014/src/gpu/GrPathUtils.cpp#newco...
src/gpu/GrPathUtils.cpp:259: SkPoint chopped[13];
chopped[10]

[bsalomon, 2012-01-12 20:14:53]

http://codereview.appspot.com/5533061/diff/6014/src/gpu/GrAAConvexPathRendere...
File src/gpu/GrAAConvexPathRenderer.cpp (right):

http://codereview.appspot.com/5533061/diff/6014/src/gpu/GrAAConvexPathRendere...
src/gpu/GrAAConvexPathRenderer.cpp:307: 
On 2012/01/12 18:49:26, reed1 wrote:
> wow -- no obvious way to use a loop or small table? just curious.

Maybe this could be improved. This is likely to change in a future revision
where we selectively add the wedges at sharp corners vertex rather than between
all segments. When the first and third verts of the quad don't need a wedge we
can use a simpler bound.

http://codereview.appspot.com/5533061/diff/6014/src/gpu/GrGLProgram.cpp
File src/gpu/GrGLProgram.cpp (right):

http://codereview.appspot.com/5533061/diff/6014/src/gpu/GrGLProgram.cpp#newco...
src/gpu/GrGLProgram.cpp:540: if (GrDrawState::kQuad_EdgeType ==
fProgramDesc.fVertexEdgeType) {
On 2012/01/12 18:49:26, reed1 wrote:
> They don't have a cool length(dfdx, dfdy) function (or invLength(...)? boohoo.

Done.

http://codereview.appspot.com/5533061/diff/6014/src/gpu/GrPathUtils.cpp
File src/gpu/GrPathUtils.cpp (right):

http://codereview.appspot.com/5533061/diff/6014/src/gpu/GrPathUtils.cpp#newco...
src/gpu/GrPathUtils.cpp:195: SkMatrix UVpts;
On 2012/01/12 18:49:26, reed1 wrote:
> SK_Scalar1 is nice to see, but this code won't run correctly if scalar is
fixed,
> as SkMatrix will interpret the perspective elements as 2.30, not 16.16. You
> could (if you like) just say 1, 0.5f, etc. instead.

Done.

http://codereview.appspot.com/5533061/diff/6014/src/gpu/GrPathUtils.cpp#newco...
src/gpu/GrPathUtils.cpp:207: matrix->setPerspY(0);
On 2012/01/12 18:49:26, reed1 wrote:
> Do you want to add this?
> 
>     matrix->set(SkMatrix::kMPersp2] = 1;

The matrix may have a scale factor (but should never be perspective). The new
helper function adds asserts and fixes up kMPersp2.

http://codereview.appspot.com/5533061/diff/6014/src/gpu/GrPathUtils.cpp#newco...
src/gpu/GrPathUtils.cpp:259: SkPoint chopped[13];
On 2012/01/12 18:49:26, reed1 wrote:
> chopped[10]

Done.

[reed1, 2012-01-12 20:16:20]

lgtm

[TomH, 2012-01-17 15:35:24]

Didn't finish review, and it isn't feeling timely any more - you already have
reed@'s comments, here's what I'd written.

http://codereview.appspot.com/5533061/diff/6014/gm/convexpaths.cpp
File gm/convexpaths.cpp (right):

http://codereview.appspot.com/5533061/diff/6014/gm/convexpaths.cpp#newcode87
gm/convexpaths.cpp:87: 
Could we have a clean initial commit? Either remote this, or add a comment about
why it isn't part of the GM?

http://codereview.appspot.com/5533061/diff/6014/gm/convexpaths.cpp#newcode103
gm/convexpaths.cpp:103: }
Everything is at the origin? Which is the easy way to do things given the nice
consistent style you're using, but I worry about it, particularly point
degenerate.

I suppose down below you add translations; as long as we're in canvas
coordinates that obviates this comment.

http://codereview.appspot.com/5533061/diff/6014/src/gpu/GrAAConvexPathRendere...
File src/gpu/GrAAConvexPathRenderer.cpp (right):

http://codereview.appspot.com/5533061/diff/6014/src/gpu/GrAAConvexPathRendere...
src/gpu/GrAAConvexPathRenderer.cpp:93: // so we detect this case and will not
draw.
This description is unclear.

http://codereview.appspot.com/5533061/diff/6014/src/gpu/GrAAConvexPathRendere...
src/gpu/GrAAConvexPathRenderer.cpp:151: // requires modifying the distance calc
in the shader actually shade differently
fix grammar? not clear.

[bsalomon, 2012-01-17 16:40:38]

Original checkin at r3040, Tom's comments incorporated at r3045.

http://codereview.appspot.com/5533061/diff/6014/gm/convexpaths.cpp
File gm/convexpaths.cpp (right):

http://codereview.appspot.com/5533061/diff/6014/gm/convexpaths.cpp#newcode87
gm/convexpaths.cpp:87: 
On 2012/01/17 15:35:25, TomH wrote:
> Could we have a clean initial commit? Either remote this, or add a comment
about
> why it isn't part of the GM?

Added a comment

http://codereview.appspot.com/5533061/diff/6014/gm/convexpaths.cpp#newcode103
gm/convexpaths.cpp:103: }
On 2012/01/17 15:35:25, TomH wrote:
> Everything is at the origin? Which is the easy way to do things given the nice
> consistent style you're using, but I worry about it, particularly point
> degenerate.
> 
> I suppose down below you add translations; as long as we're in canvas
> coordinates that obviates this comment.

I made the point degenerates all be at 50,50

http://codereview.appspot.com/5533061/diff/6014/src/gpu/GrAAConvexPathRendere...
File src/gpu/GrAAConvexPathRenderer.cpp (right):

http://codereview.appspot.com/5533061/diff/6014/src/gpu/GrAAConvexPathRendere...
src/gpu/GrAAConvexPathRenderer.cpp:93: // so we detect this case and will not
draw.
On 2012/01/17 15:35:25, TomH wrote:
> This description is unclear.

I attempted to clarify

http://codereview.appspot.com/5533061/diff/6014/src/gpu/GrAAConvexPathRendere...
src/gpu/GrAAConvexPathRenderer.cpp:151: // requires modifying the distance calc
in the shader actually shade differently
On 2012/01/17 15:35:25, TomH wrote:
> fix grammar? not clear.
Comment is improved.