Make GrAAHairLinePathRenderer support perspective

[bsalomon, 2011-09-09 21:28:09]

Happy monday!

http://codereview.appspot.com/4969071/diff/1/gpu/src/GrAAHairLinePathRenderer...
File gpu/src/GrAAHairLinePathRenderer.cpp (left):

http://codereview.appspot.com/4969071/diff/1/gpu/src/GrAAHairLinePathRenderer...
gpu/src/GrAAHairLinePathRenderer.cpp:157: static const int kMaxSubdivs = 30;
This was absurdly large

[TomH, 2011-09-12 13:18:59]

LGTM

http://codereview.appspot.com/4969071/diff/2001/gpu/src/GrAAHairLinePathRende...
File gpu/src/GrAAHairLinePathRenderer.cpp (right):

http://codereview.appspot.com/4969071/diff/2001/gpu/src/GrAAHairLinePathRende...
gpu/src/GrAAHairLinePathRenderer.cpp:156: const SkScalar distanceSqdTol =
tolScale * SK_Scalar1;
Why do we get a scalar in and then multiply it by 1?
(And this uses raw *; doesn't that break with Fixed?)

http://codereview.appspot.com/4969071/diff/2001/gpu/src/GrAAHairLinePathRende...
gpu/src/GrAAHairLinePathRenderer.cpp:195: 
I missed this on previous reading - do we have a comment at
SkChopCubicAtInflections guaranteeing it returns in [0,3]?

http://codereview.appspot.com/4969071/diff/2001/gpu/src/GrAAHairLinePathRende...
gpu/src/GrAAHairLinePathRenderer.cpp:276: * device space. We will do a device
space bloat to account for the 1pixel
Would "Generates" be a clearer word than "Retrieves"? Is "get_" the right name
for the function, or maybe "convert_to_"?

http://codereview.appspot.com/4969071/diff/2001/gpu/src/GrAAHairLinePathRende...
gpu/src/GrAAHairLinePathRenderer.cpp:340: } break;
You've got {} for these two cases, but not the one above (kLine_PathCmd)?

http://codereview.appspot.com/4969071/diff/2001/gpu/src/GrAAHairLinePathRende...
gpu/src/GrAAHairLinePathRenderer.cpp:535: GrAssert(subdiv >= 0);
Is it just skia convention that we do this instead of making subdiv unsigned?

http://codereview.appspot.com/4969071/diff/2001/gpu/src/GrAAHairLinePathRende...
gpu/src/GrAAHairLinePathRenderer.cpp:616: return true;
This early-out is getting complicated; is it worth a 2-line comment summarizing
why we exit? Or is it something that's transparent and I'm just not seeing it
because I'm not in context?

[bsalomon, 2011-09-12 13:40:54]

http://codereview.appspot.com/4969071/diff/2001/gpu/src/GrAAHairLinePathRende...
File gpu/src/GrAAHairLinePathRenderer.cpp (right):

http://codereview.appspot.com/4969071/diff/2001/gpu/src/GrAAHairLinePathRende...
gpu/src/GrAAHairLinePathRenderer.cpp:156: const SkScalar distanceSqdTol =
tolScale * SK_Scalar1;
On 2011/09/12 13:18:59, TomH wrote:
> Why do we get a scalar in and then multiply it by 1?
> (And this uses raw *; doesn't that break with Fixed?)

Nice catch, the * was just wrong. Also, I didn't mean to change this from 2 to 1
for the base tol. I must have edited in when playing around with various
parameters.

http://codereview.appspot.com/4969071/diff/2001/gpu/src/GrAAHairLinePathRende...
gpu/src/GrAAHairLinePathRenderer.cpp:195: 
On 2011/09/12 13:18:59, TomH wrote:
> I missed this on previous reading - do we have a comment at
> SkChopCubicAtInflections guaranteeing it returns in [0,3]?

Done.

http://codereview.appspot.com/4969071/diff/2001/gpu/src/GrAAHairLinePathRende...
gpu/src/GrAAHairLinePathRenderer.cpp:276: * device space. We will do a device
space bloat to account for the 1pixel
On 2011/09/12 13:18:59, TomH wrote:
> Would "Generates" be a clearer word than "Retrieves"? Is "get_" the right name
> for the function, or maybe "convert_to_"?

Done.

http://codereview.appspot.com/4969071/diff/2001/gpu/src/GrAAHairLinePathRende...
gpu/src/GrAAHairLinePathRenderer.cpp:340: } break;
On 2011/09/12 13:18:59, TomH wrote:
> You've got {} for these two cases, but not the one above (kLine_PathCmd)?

Done.

http://codereview.appspot.com/4969071/diff/2001/gpu/src/GrAAHairLinePathRende...
gpu/src/GrAAHairLinePathRenderer.cpp:535: GrAssert(subdiv >= 0);
On 2011/09/12 13:18:59, TomH wrote:
> Is it just skia convention that we do this instead of making subdiv unsigned?

Yes, its the Google-wide C++ convention to use int for counts and uints for
bitfields.

http://codereview.appspot.com/4969071/diff/2001/gpu/src/GrAAHairLinePathRende...
gpu/src/GrAAHairLinePathRenderer.cpp:616: return true;
On 2011/09/12 13:18:59, TomH wrote:
> This early-out is getting complicated; is it worth a 2-line comment
summarizing
> why we exit? Or is it something that's transparent and I'm just not seeing it
> because I'm not in context?

Done.

[reed1, 2011-09-12 13:45:43]

http://codereview.appspot.com/4969071/diff/2001/gpu/src/GrAAHairLinePathRende...
File gpu/src/GrAAHairLinePathRenderer.cpp (right):

http://codereview.appspot.com/4969071/diff/2001/gpu/src/GrAAHairLinePathRende...
gpu/src/GrAAHairLinePathRenderer.cpp:156: const SkScalar distanceSqdTol =
tolScale * SK_Scalar1;
+1

On 2011/09/12 13:18:59, TomH wrote:
> Why do we get a scalar in and then multiply it by 1?
> (And this uses raw *; doesn't that break with Fixed?)

http://codereview.appspot.com/4969071/diff/2001/include/core/SkPoint.h
File include/core/SkPoint.h (right):

http://codereview.appspot.com/4969071/diff/2001/include/core/SkPoint.h#newcod...
include/core/SkPoint.h:192: static void Offset(SkPoint points[], int count,
const SkPoint& offset) {
Can we take dx, dy instead of a point instead of requiring a point to be cons'd
up? I'm fine with a "helper" that takes a point and then calls the lower-level
version.

[bsalomon, 2011-09-12 13:59:51]

http://codereview.appspot.com/4969071/diff/2001/include/core/SkPoint.h
File include/core/SkPoint.h (right):

http://codereview.appspot.com/4969071/diff/2001/include/core/SkPoint.h#newcod...
include/core/SkPoint.h:192: static void Offset(SkPoint points[], int count,
const SkPoint& offset) {
On 2011/09/12 13:45:44, reed1 wrote:
> Can we take dx, dy instead of a point instead of requiring a point to be
cons'd
> up? I'm fine with a "helper" that takes a point and then calls the lower-level
> version.

Done.

[bsalomon, 2011-09-12 18:10:29]

closed with r2249.