Animated the PathUtils Sample to show path contours

Animated the PathUtils Sample to show path contours# Enter a description of the change.



Animated the PathUtils Sample to path contours

BUG=

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

[dierk, 2013-07-02 20:13:08]

Here's the updated SampleApp with animations. Should I go ahead and patch this
in?

[dierk, 2013-07-08 15:20:59]

Mike had suggested that I add you as a reviewer of my code as you'll be familiar
with the area (and I'll need multiple reviewers for my midterm and final
evaluations). 

Thanks!
-Nate

[reed1, 2013-07-08 16:04:07]

lets rename the new gm to something more content-specific

[scroggo, 2013-07-08 19:11:31]

https://codereview.chromium.org/18552005/diff/5001/gm/nate_gradients.cpp
File gm/nate_gradients.cpp (right):

https://codereview.chromium.org/18552005/diff/5001/gm/nate_gradients.cpp#newc...
gm/nate_gradients.cpp:3: * Copyright 2011 Google Inc.
2013

https://codereview.chromium.org/18552005/diff/5001/gm/nate_gradients.cpp#newc...
gm/nate_gradients.cpp:93: class GradientsGM : public GM {
What is this new GM demonstrating that isn't covered by our other gradient GMs?

https://codereview.chromium.org/18552005/diff/5001/samplecode/SamplePathUtils...
File samplecode/SamplePathUtils.cpp (right):

https://codereview.chromium.org/18552005/diff/5001/samplecode/SamplePathUtils...
samplecode/SamplePathUtils.cpp:49: protected:
nit: Need the member variables be protected?

https://codereview.chromium.org/18552005/diff/5001/samplecode/SamplePathUtils...
samplecode/SamplePathUtils.cpp:51: static const int h=8, w=12, stride=2; //
stride is in bytes
nit: Typically byte counts are represented as size_t. Also, in SkBitmap we use
the name 'rowBytes' instead of 'stride'. Should this be the same?

https://codereview.chromium.org/18552005/diff/5001/samplecode/SamplePathUtils...
samplecode/SamplePathUtils.cpp:54: SkPaint bmp_paint;
nit: In Skia we name member variables in camelCase with an 'f' at the beginning,
so this should be fBmpPaint

https://codereview.chromium.org/18552005/diff/5001/samplecode/SamplePathUtils...
samplecode/SamplePathUtils.cpp:55: SkPaint outline_paint;
nit: fOutlinePaint

https://codereview.chromium.org/18552005/diff/5001/samplecode/SamplePathUtils...
samplecode/SamplePathUtils.cpp:56: SkScalar gPhase;
nit: Unless this is global, it should be fPhase.

https://codereview.chromium.org/18552005/diff/5001/samplecode/SamplePathUtils...
samplecode/SamplePathUtils.cpp:69: virtual void onDrawContent(SkCanvas* canvas)
{
I love SamplePathUtils's images :)

https://codereview.chromium.org/18552005/diff/5001/samplecode/SamplePathUtils...
samplecode/SamplePathUtils.cpp:82: gBitsToPath_fns[j](&path, (char*) &gBits[i],
h, w, stride);
Most of our API supplies width as a parameter before height. Shouldn't the
SkPathUtils functions use the same ordering for clarity?

[dierk, 2013-07-08 20:00:05]

I've updated the Sample integrating Leon's comments. Leon, could you tear apart
src/utils/SkPathUtils.cpp as well?

Cheers!
-Nate

[djsollen, 2013-07-09 12:22:36]

https://codereview.chromium.org/18552005/diff/15001/samplecode/SamplePathUtil...
File samplecode/SamplePathUtils.cpp (right):

https://codereview.chromium.org/18552005/diff/15001/samplecode/SamplePathUtil...
samplecode/SamplePathUtils.cpp:37: class samplePathUtils : public SampleView {
capitalize... SamplePathUtils

https://codereview.chromium.org/18552005/diff/15001/samplecode/SamplePathUtil...
samplecode/SamplePathUtils.cpp:75: fOutlinePaint.setPathEffect(new
SkComposePathEffect(dash, corner) )->unref();
I know I'm late to the review party, but this seems like an awful lot of refcnt
code that would be unnecessary if you just created fOutlinePaint as a local
variable.  You could then just put all the effects on the stack.

[dierk, 2013-07-09 13:56:21]

https://codereview.chromium.org/18552005/diff/15001/samplecode/SamplePathUtil...
File samplecode/SamplePathUtils.cpp (right):

https://codereview.chromium.org/18552005/diff/15001/samplecode/SamplePathUtil...
samplecode/SamplePathUtils.cpp:37: class samplePathUtils : public SampleView {
On 2013/07/09 12:22:36, djsollen wrote:
> capitalize... SamplePathUtils

Done.

https://codereview.chromium.org/18552005/diff/15001/samplecode/SamplePathUtil...
samplecode/SamplePathUtils.cpp:75: fOutlinePaint.setPathEffect(new
SkComposePathEffect(dash, corner) )->unref();
On 2013/07/09 12:22:36, djsollen wrote:
> I know I'm late to the review party, but this seems like an awful lot of
refcnt
> code that would be unnecessary if you just created fOutlinePaint as a local
> variable.  You could then just put all the effects on the stack.

Done.

[djsollen, 2013-07-09 14:12:36]

https://codereview.chromium.org/18552005/diff/22001/samplecode/SamplePathUtil...
File samplecode/SamplePathUtils.cpp (right):

https://codereview.chromium.org/18552005/diff/22001/samplecode/SamplePathUtil...
samplecode/SamplePathUtils.cpp:75: new SkCornerPathEffect(.25f)
I'm pretty sure this will leak the dash and corner effects.  You need to put the
effects on the stack as well to not leak them.

SkDashEffect dash();
SkCorner...
SkComposePathEffect compose(&dash, &corner)
...setPathEffect(&compose)

[dierk, 2013-07-09 15:11:27]

https://codereview.chromium.org/18552005/diff/22001/samplecode/SamplePathUtil...
File samplecode/SamplePathUtils.cpp (right):

https://codereview.chromium.org/18552005/diff/22001/samplecode/SamplePathUtil...
samplecode/SamplePathUtils.cpp:75: new SkCornerPathEffect(.25f)
On 2013/07/09 14:12:36, djsollen wrote:
> I'm pretty sure this will leak the dash and corner effects.  You need to put
the
> effects on the stack as well to not leak them.
> 
> SkDashEffect dash();
> SkCorner...
> SkComposePathEffect compose(&dash, &corner)
> ...setPathEffect(&compose)

Done.

[scroggo, 2013-07-09 15:34:22]

https://codereview.chromium.org/18552005/diff/26001/samplecode/SamplePathUtil...
File samplecode/SamplePathUtils.cpp (right):

https://codereview.chromium.org/18552005/diff/26001/samplecode/SamplePathUtil...
samplecode/SamplePathUtils.cpp:72: SkPaint OutlinePaint;
Local variables should be lowercase (e.g. outlinePaint). See
https://sites.google.com/site/skiadocs/developer-documentation/contributing-c...

https://codereview.chromium.org/18552005/diff/26001/src/utils/SkPathUtils.cpp
File src/utils/SkPathUtils.cpp (right):

https://codereview.chromium.org/18552005/diff/26001/src/utils/SkPathUtils.cpp...
src/utils/SkPathUtils.cpp:100: int w, int h, int stride) {
These should be fixed in the header as well.

https://codereview.chromium.org/18552005/diff/26001/src/utils/SkPathUtils.cpp...
src/utils/SkPathUtils.cpp:107: Simplify(*path, path); // simplify resulting
bitmap
nit: simplify resulting bitmap.

[dierk, 2013-07-09 15:42:02]

https://codereview.chromium.org/18552005/diff/26001/samplecode/SamplePathUtil...
File samplecode/SamplePathUtils.cpp (right):

https://codereview.chromium.org/18552005/diff/26001/samplecode/SamplePathUtil...
samplecode/SamplePathUtils.cpp:72: SkPaint OutlinePaint;
On 2013/07/09 15:34:22, scroggo wrote:
> Local variables should be lowercase (e.g. outlinePaint). See
>
https://sites.google.com/site/skiadocs/developer-documentation/contributing-c...

Done.

https://codereview.chromium.org/18552005/diff/26001/src/utils/SkPathUtils.cpp
File src/utils/SkPathUtils.cpp (right):

https://codereview.chromium.org/18552005/diff/26001/src/utils/SkPathUtils.cpp...
src/utils/SkPathUtils.cpp:100: int w, int h, int stride) {
On 2013/07/09 15:34:22, scroggo wrote:
> These should be fixed in the header as well.

Done. Also changed stride to rowBytes.

https://codereview.chromium.org/18552005/diff/26001/src/utils/SkPathUtils.cpp...
src/utils/SkPathUtils.cpp:107: Simplify(*path, path); // simplify resulting
bitmap
On 2013/07/09 15:34:22, scroggo wrote:
> nit: simplify resulting bitmap.

Done.

[scroggo, 2013-07-09 15:57:29]

On 2013/07/08 20:00:05, dierk wrote:
> I've updated the Sample integrating Leon's comments. Leon, could you tear
apart
> src/utils/SkPathUtils.cpp as well?
> 
> Cheers!
> -Nate

I haven't looked over the existing SkPathUtils.cpp in detail, but this CL LGTM
at patchset 9.

[commit-bot: I haz the power, 2013-07-09 16:05:20]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/dierk@google.com/18552005/35001

[commit-bot: I haz the power, 2013-07-09 16:30:40]

Change committed as 9934