use quads for mixed radius rrects

use quads for mixed radius rrects

Create a specialized version of adding a pair of corner quads
that avoids the overhead of the full arc machinery.

This is on the way to changing Chrome to calling Skia directly to create fully general round rects rather than rolling their own.

Committed: http://code.google.com/p/skia/source/detail?r=12190

[caryclark, 2013-11-05 16:25:17]

Rob, please review to see if
a) this looks understandable and maintainable and
b) it alleviates your concerns about calling a helper

I have done a quick sanity check to make sure the logic works but I have not run
gm yet

[robertphillips, 2013-11-05 16:47:56]

I think this looks reasonable. Two questions inline.

https://codereview.chromium.org/60203002/diff/1/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

https://codereview.chromium.org/60203002/diff/1/src/core/SkPath.cpp#newcode1011
src/core/SkPath.cpp:1011: */
Would it make sense to pass in the SkRRect itself (and use corner to get the
right radii) rather than bounds and radii separately?

https://codereview.chromium.org/60203002/diff/1/src/core/SkPath.cpp#newcode1107
src/core/SkPath.cpp:1107: if (kCW_Direction == dir) {
Should 0 be 3 here? One of the Corner enum values could help with clarity.

[caryclark, 2013-11-05 20:29:54]

https://codereview.chromium.org/60203002/diff/1/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

https://codereview.chromium.org/60203002/diff/1/src/core/SkPath.cpp#newcode1011
src/core/SkPath.cpp:1011: */
On 2013/11/05 16:47:56, robertphillips wrote:
> Would it make sense to pass in the SkRRect itself (and use corner to get the
> right radii) rather than bounds and radii separately?

Done.

https://codereview.chromium.org/60203002/diff/1/src/core/SkPath.cpp#newcode1107
src/core/SkPath.cpp:1107: if (kCW_Direction == dir) {
On 2013/11/05 16:47:56, robertphillips wrote:
> Should 0 be 3 here? One of the Corner enum values could help with clarity.

Done.

[caryclark, 2013-11-06 12:51:59]

this CL is ready for formal review

[robertphillips, 2013-11-06 13:31:58]

lgtm modulo a possible issue (the #ifdef protection) & a question.

https://codereview.chromium.org/60203002/diff/90001/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

https://codereview.chromium.org/60203002/diff/90001/src/core/SkPath.cpp#newco...
src/core/SkPath.cpp:1099: this->addOval(bounds, dir);
I don't think this needs to be #ifdef protected. Chrome should already be
exposed to this functionality.

https://codereview.chromium.org/60203002/diff/90001/src/core/SkPath.cpp#newco...
src/core/SkPath.cpp:1108: SkAutoPathBoundsUpdate apbu(this, bounds);
Do we need a "SkAutoDisableDirectionCheck(this);" here?

[caryclark, 2013-11-06 13:39:55]

https://codereview.chromium.org/60203002/diff/90001/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

https://codereview.chromium.org/60203002/diff/90001/src/core/SkPath.cpp#newco...
src/core/SkPath.cpp:1099: this->addOval(bounds, dir);
On 2013/11/06 13:31:58, robertphillips wrote:
> I don't think this needs to be #ifdef protected. Chrome should already be
> exposed to this functionality.

This is ifdef protected to avoid recursive functions. As written, addRoundRect
calls addRRect and vice versa.

https://codereview.chromium.org/60203002/diff/90001/src/core/SkPath.cpp#newco...
src/core/SkPath.cpp:1108: SkAutoPathBoundsUpdate apbu(this, bounds);
On 2013/11/06 13:31:58, robertphillips wrote:
> Do we need a "SkAutoDisableDirectionCheck(this);" here?

I didn't know about this helper. Yes, it looks like a good idea.

[caryclark, 2013-11-06 13:58:27]

https://codereview.chromium.org/60203002/diff/90001/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

https://codereview.chromium.org/60203002/diff/90001/src/core/SkPath.cpp#newco...
src/core/SkPath.cpp:1108: SkAutoPathBoundsUpdate apbu(this, bounds);
On 2013/11/06 13:39:55, caryclark wrote:
> On 2013/11/06 13:31:58, robertphillips wrote:
> > Do we need a "SkAutoDisableDirectionCheck(this);" here?
> 
> I didn't know about this helper. Yes, it looks like a good idea.

Done.

[reed1, 2013-11-08 14:04:31]

lgtm w/ unit-test-reference suggestion

https://codereview.chromium.org/60203002/diff/150001/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

https://codereview.chromium.org/60203002/diff/150001/src/core/SkPath.cpp#newc...
src/core/SkPath.cpp:1021: // and the convex cross product sign equality test.
In the comment, can we point to a unittest that would break if we didn't do this
2-bit bump?

[caryclark, 2013-11-08 15:22:30]

https://codereview.chromium.org/60203002/diff/150001/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

https://codereview.chromium.org/60203002/diff/150001/src/core/SkPath.cpp#newc...
src/core/SkPath.cpp:1021: // and the convex cross product sign equality test.
On 2013/11/08 14:04:32, reed1 wrote:
> In the comment, can we point to a unittest that would break if we didn't do
this
> 2-bit bump?

Mike's suspicion paid off. The 2 bit bump is only required when constructing arc
geometry, to fix gm cases like arcofzorro that construct arcs that end near the
quadratic approximation midpoint. For round rects, the bump is unneeded.

(Fixed)

[commit-bot: I haz the power, 2013-11-08 15:43:05]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/caryclark@google.com/60203002/240001

[reed1, 2013-11-08 15:46:39]

https://codereview.chromium.org/60203002/diff/240001/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

https://codereview.chromium.org/60203002/diff/240001/src/core/SkPath.cpp#newc...
src/core/SkPath.cpp:1020: SkScalar midPtX = rx - rx * (SK_Scalar1 +
SK_ScalarTanPIOver8) / 2;
micro-perf-nit:

can/should we rewrite this expression as:

static SkScalar const kSomeConst = (SK_Scalar1 + SK_ScalarTanPIOver8) / 2;

midPtX = rx * kSomeConst;
midPtY = ry * kSomeConst;

[commit-bot: I haz the power, 2013-11-08 15:51:15]

Change committed as 12190