Adding my Bitmap2Path sample for 1on1 meeting.

Adding my Bitmap2Path sample for 1on1 meeting.

BUG=

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

[reed1, 2013-06-13 17:58:41]

https://codereview.chromium.org/16829003/diff/3001/samplecode/SampleBitmap2Pa...
File samplecode/SampleBitmap2Path.cpp (right):

https://codereview.chromium.org/16829003/diff/3001/samplecode/SampleBitmap2Pa...
samplecode/SampleBitmap2Path.cpp:16: bool getBit( const char *buffer,
any function local to your file... static bool ...

https://codereview.chromium.org/16829003/diff/3001/samplecode/SampleBitmap2Pa...
samplecode/SampleBitmap2Path.cpp:17: const int x ) {
for parameters, const on a non-ptr is never meaningful.

https://codereview.chromium.org/16829003/diff/3001/samplecode/SampleBitmap2Pa...
samplecode/SampleBitmap2Path.cpp:19: int byte = x / CHAR_BIT;
nit: when you know your modulus is a power of two, we often explicitly write &
and >>

e.g.

byte = x >> 8;
bit = x & 7;

Once in a while, the compiler may forget that optimization, and actually
generate a div or mod (ugh)

https://codereview.chromium.org/16829003/diff/3001/samplecode/SampleBitmap2Pa...
samplecode/SampleBitmap2Path.cpp:22: return buffer[byte] & (1 << bit);
this is not a bool, but an int (which is fine), but you may get a warning as the
compiler turns that into a real 'bool'.

One conversion macro:

bool convertToBool(int predicate) {
    return !!predicate;
or
    return predicate != 0;
}

https://codereview.chromium.org/16829003/diff/3001/samplecode/SampleBitmap2Pa...
samplecode/SampleBitmap2Path.cpp:34: SkRect r = { i, lineIdx, i+1, lineIdx+1 };
alternate:

SkRect::MakeXYWH(i, lineIdx, 1, 1)

convenience:

path->addRect(SkRect::Make(...), ...);

https://codereview.chromium.org/16829003/diff/3001/samplecode/SampleBitmap2Pa...
samplecode/SampleBitmap2Path.cpp:40: void line2path_pixelCircle( SkPath* path,
all these guys can be marked 'static'

https://codereview.chromium.org/16829003/diff/3001/samplecode/SampleBitmap2Pa...
samplecode/SampleBitmap2Path.cpp:44: for (int i=0; i<width; i++) {
very nitty: need more space, and pre-increment

for (int i = 0; i < width; ++i)

https://codereview.chromium.org/16829003/diff/3001/samplecode/SampleBitmap2Pa...
samplecode/SampleBitmap2Path.cpp:46: if (getBit(line,i)==1) {
if (getBit(...)) {

https://codereview.chromium.org/16829003/diff/3001/samplecode/SampleBitmap2Pa...
samplecode/SampleBitmap2Path.cpp:48: path->addCircle( i+.5, lineIdx+.5, SQRT_2 /
2.0 );
all 3 params will generate doubles...

use 0.5f if you mean float... or SkFloatToScalar(0.5f)

https://codereview.chromium.org/16829003/diff/3001/samplecode/SampleBitmap2Pa...
samplecode/SampleBitmap2Path.cpp:136: void (*mode[numModes])( SkPath*, const
char*,
const void (*....

maybe typdef this guy to a name up above

https://codereview.chromium.org/16829003/diff/3001/samplecode/SampleBitmap2Pa...
samplecode/SampleBitmap2Path.cpp:159: canvas->scale(10.0, 10.0);
more doubles

canvas->scale(10, 10);

[reed1, 2013-06-14 18:59:38]

With an eye toward future tests, might use the fact that you can animate in a
sample to generate random bit patterns and 'test' those (besides drawing them).

1. convert bits to path
2. convert bits to regular bitmap (by hand)
3. draw path into another bitmap (using canvas)
4. assert that #3 == #4 calling memcmp.

https://codereview.chromium.org/16829003/diff/9001/samplecode/SampleBitmap2Pa...
File samplecode/SampleBitmap2Path.cpp (right):

https://codereview.chromium.org/16829003/diff/9001/samplecode/SampleBitmap2Pa...
samplecode/SampleBitmap2Path.cpp:54: bool state = 0;
does state mean "inside a run of pixels"?

https://codereview.chromium.org/16829003/diff/9001/samplecode/SampleBitmap2Pa...
samplecode/SampleBitmap2Path.cpp:58: if (i==width-1) { // if the end of a
scanline, close open paths
rather than check for this every time through the loop, why not just fall
through to this code after the loop (i < width - 1) terminates?

https://codereview.chromium.org/16829003/diff/9001/samplecode/SampleBitmap2Pa...
samplecode/SampleBitmap2Path.cpp:70: }else if ( (getBit(line,i)!=0) != state ) {
// if transition
yick -- we're calling getBit more than once per-bit. Can we just assign it to a
local var, and then perform the logic ?

[dierk, 2013-06-25 18:29:04]

Uploaded a bunch of changes. 

One weird issue I'm having is that I can't link the PathUtils files that I've
made to the PathOps functions (so I've temporarily commented out the path
Simplification). How can I properly link these? (it worked fine from the sample
code)

Going to start building a Bench test now.

[dierk, 2013-06-26 15:38:10]

Added SkRegion mode to PathUtils and test.

[reed1, 2013-06-26 16:54:24]

https://codereview.chromium.org/16829003/diff/24001/include/utils/SkPathUtils.h
File include/utils/SkPathUtils.h (right):

https://codereview.chromium.org/16829003/diff/24001/include/utils/SkPathUtils...
include/utils/SkPathUtils.h:15: //#include "SkPathOps.h" // this file can't be
found
Not needed anyway for a header (just the impl)

https://codereview.chromium.org/16829003/diff/24001/include/utils/SkPathUtils...
include/utils/SkPathUtils.h:17: typedef void (*line2path)(SkPath*, const char*,
int, int);
These 4 lines need not be in the header

https://codereview.chromium.org/16829003/diff/24001/include/utils/SkPathUtils...
include/utils/SkPathUtils.h:33: static void bitmap2path(SkPath* path, const
char* bitmap,
1. I think only these last 2 need to be in the public header
2. static methods are Captialized (e.g. BitmapToPath)

https://codereview.chromium.org/16829003/diff/24001/include/utils/SkPathUtils...
include/utils/SkPathUtils.h:34: const line2path l2p_fn,
Since "bitmap" is a pretty loaded word in Skia, perhaps these last two are
called BitsToPath or something like that. Also need to add some documentation...

/**
 * Return the boundary path given a 1-bit bitmap, specified by width/height and
stride. The bits are interpreted as 1 being "in" the path, and 0 being "out".
The bits are interpreted as MSB on the left, and LSB on the right... etc.
*/

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

https://codereview.chromium.org/16829003/diff/24001/src/utils/SkPathUtils.cpp...
src/utils/SkPathUtils.cpp:147: //Simplify(*path, path); // simplify the path?
may be uneccessary
Could test, but I think this is never necessary.

[dierk, 2013-06-27 18:10:11]

I've moved everything out of the PathUtils.h file except for public methods and
documentation. Doing so caused some methods <fillRandomBits() and getBit()> to
need definition inside of Sample, Test, and Bench.

I've tried including SkPathOps.h inside of SkPathUtils.cpp but the compiler
can't find it. How do I correctly link this? I had no issues including it from
SamplePathUtils.cpp.

[reed1, 2013-06-28 19:35:34]

lgtm w/ comments

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

https://codereview.chromium.org/16829003/diff/35001/src/utils/SkPathUtils.cpp...
src/utils/SkPathUtils.cpp:151: //Simplify(*path, path); // simplify the path?
may be uneccessary
not needed. remove comment.

https://codereview.chromium.org/16829003/diff/35001/src/utils/SkPathUtils.cpp...
src/utils/SkPathUtils.cpp:156: 
lets remove this unless you plan to work on this in the near future.

[commit-bot: I haz the power, 2013-06-28 21:10:41]

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

[commit-bot: I haz the power, 2013-06-28 21:20:29]

Retried try job too often on Build-Ubuntu12-GCC-x86_64-Release-Trybot for
step(s) BuildTests
http://108.170.217.252:10117/buildstatus?builder=Build-Ubuntu12-GCC-x86_64-Re...

[commit-bot: I haz the power, 2013-06-28 21:29:41]

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

[commit-bot: I haz the power, 2013-06-28 21:39:39]

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

[commit-bot: I haz the power, 2013-06-28 21:39:46]

Failed to apply patch for gyp/utils.gyp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file gyp/utils.gyp
  Hunk #1 FAILED at 10.
  Hunk #2 succeeded at 82 (offset 2 lines).
  1 out of 2 hunks FAILED -- saving rejects to file gyp/utils.gyp.rej

Patch:       gyp/utils.gyp
Index: gyp/utils.gyp
diff --git a/gyp/utils.gyp b/gyp/utils.gyp
index
4ac9284d5371112ac12da0e34fa96719c6c7bc0d..6d7c4b72335a59681751a0ecef2856cca8d7a85a
100644
--- a/gyp/utils.gyp
+++ b/gyp/utils.gyp
@@ -10,6 +10,7 @@
         '../include/core',
         '../include/effects',
         '../include/images',
+        '../include/pathops',
         '../include/pipe',
         '../include/utils',
         '../include/utils/mac',
@@ -80,6 +81,7 @@
         '../src/utils/SkParseColor.cpp',
         '../src/utils/SkParsePath.cpp',
         '../src/utils/SkPictureUtils.cpp',
+        '../src/utils/SkPathUtils.cpp',
         '../src/utils/SkProxyCanvas.cpp',
         '../src/utils/SkSHA1.cpp',
         '../src/utils/SkSHA1.h',

[commit-bot: I haz the power, 2013-06-28 21:46:29]

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

[commit-bot: I haz the power, 2013-06-28 21:46:31]

Failed to apply patch for gyp/utils.gyp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file gyp/utils.gyp
  Hunk #1 FAILED at 10.
  Hunk #2 succeeded at 82 (offset 2 lines).
  1 out of 2 hunks FAILED -- saving rejects to file gyp/utils.gyp.rej

Patch:       gyp/utils.gyp
Index: gyp/utils.gyp
diff --git a/gyp/utils.gyp b/gyp/utils.gyp
index
4ac9284d5371112ac12da0e34fa96719c6c7bc0d..6d7c4b72335a59681751a0ecef2856cca8d7a85a
100644
--- a/gyp/utils.gyp
+++ b/gyp/utils.gyp
@@ -10,6 +10,7 @@
         '../include/core',
         '../include/effects',
         '../include/images',
+        '../include/pathops',
         '../include/pipe',
         '../include/utils',
         '../include/utils/mac',
@@ -80,6 +81,7 @@
         '../src/utils/SkParseColor.cpp',
         '../src/utils/SkParsePath.cpp',
         '../src/utils/SkPictureUtils.cpp',
+        '../src/utils/SkPathUtils.cpp',
         '../src/utils/SkProxyCanvas.cpp',
         '../src/utils/SkSHA1.cpp',
         '../src/utils/SkSHA1.h',

[commit-bot: I haz the power, 2013-06-28 21:54:49]

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

[commit-bot: I haz the power, 2013-06-28 21:54:51]

Failed to apply patch for gyp/utils.gyp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file gyp/utils.gyp
  Hunk #1 FAILED at 10.
  Hunk #2 succeeded at 82 (offset 2 lines).
  1 out of 2 hunks FAILED -- saving rejects to file gyp/utils.gyp.rej

Patch:       gyp/utils.gyp
Index: gyp/utils.gyp
diff --git a/gyp/utils.gyp b/gyp/utils.gyp
index
4ac9284d5371112ac12da0e34fa96719c6c7bc0d..6d7c4b72335a59681751a0ecef2856cca8d7a85a
100644
--- a/gyp/utils.gyp
+++ b/gyp/utils.gyp
@@ -10,6 +10,7 @@
         '../include/core',
         '../include/effects',
         '../include/images',
+        '../include/pathops',
         '../include/pipe',
         '../include/utils',
         '../include/utils/mac',
@@ -80,6 +81,7 @@
         '../src/utils/SkParseColor.cpp',
         '../src/utils/SkParsePath.cpp',
         '../src/utils/SkPictureUtils.cpp',
+        '../src/utils/SkPathUtils.cpp',
         '../src/utils/SkProxyCanvas.cpp',
         '../src/utils/SkSHA1.cpp',
         '../src/utils/SkSHA1.h',

[commit-bot: I haz the power, 2013-07-01 15:53:14]

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

[commit-bot: I haz the power, 2013-07-01 16:02:55]

Retried try job too often on Build-Ubuntu12-GCC-x86_64-Release-Trybot for
step(s) BuildBench, BuildGm, BuildMost, BuildSkiaLib, BuildTests, BuildTools
http://108.170.217.252:10117/buildstatus?builder=Build-Ubuntu12-GCC-x86_64-Re...

[commit-bot: I haz the power, 2013-07-01 16:05:24]

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

[commit-bot: I haz the power, 2013-07-01 16:15:15]

Retried try job too often on Build-Mac10.7-Clang-x86-Release-Trybot for step(s)
BuildTests
http://108.170.217.252:10117/buildstatus?builder=Build-Mac10.7-Clang-x86-Rele...

[commit-bot: I haz the power, 2013-07-01 16:51:44]

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

[commit-bot: I haz the power, 2013-07-01 16:57:24]

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

[commit-bot: I haz the power, 2013-07-01 17:50:32]

Change committed as 9843