Add a map homogenous points to SkMatrix

Add a map homogenous points to SkMatrix

BUG=
R=bsalomon@google.com

Committed: https://code.google.com/p/skia/source/detail?r=10659

[egdaniel, 2013-08-07 17:21:32]

I'm not sure if it is necessary to leave in the Fixed point checks our not, but
that is how the other matrix versions are implemented so I kept that formatting.

[egdaniel, 2013-08-07 17:52:10]

Changed function name to say Homogenous

[bsalomon, 2013-08-07 17:54:01]

lgtm with naming suggestion.

https://codereview.chromium.org/22330004/diff/6001/src/core/SkMatrix.cpp
File src/core/SkMatrix.cpp (right):

https://codereview.chromium.org/22330004/diff/6001/src/core/SkMatrix.cpp#newc...
src/core/SkMatrix.cpp:1237: SkScalar sz = src[2];
Can we call this sw?

https://codereview.chromium.org/22330004/diff/6001/src/core/SkMatrix.cpp#newc...
src/core/SkMatrix.cpp:1246: SkScalar z = SkScalarMul(sx, m.fMat[kMPersp0]) +
w?

[bsalomon, 2013-08-07 17:54:43]

homogenous -> homogenEous

[egdaniel, 2013-08-07 18:04:23]

On 2013/08/07 17:54:43, bsalomon wrote:
> homogenous -> homogenEous

Agree on the w's.

And another failed speeling attempt by me :(

[reed1, 2013-08-07 18:16:50]

https://codereview.chromium.org/22330004/diff/13001/include/core/SkMatrix.h
File include/core/SkMatrix.h (right):

https://codereview.chromium.org/22330004/diff/13001/include/core/SkMatrix.h#n...
include/core/SkMatrix.h:434: @param count The number of points in src to read,
and then transform
Lets be very clear what 'count' is measuring. Its is the number of
homogeneous_points, not SkPoints. Maybe the dox could say the number of triples?
Perhaps a tiny argument for declaring a struct here, so that count would be
unambiguous (tho I don't know what to call it).

https://codereview.chromium.org/22330004/diff/13001/include/core/SkMatrix.h#n...
include/core/SkMatrix.h:682: static void Homogeneous_pts(const SkMatrix&,
SkScalar dst[], const SkScalar[], int);
Why is there a separate static function? Can just put its impl inside the
method.

[reed1, 2013-08-07 18:17:02]

need unittests

[egdaniel, 2013-08-09 13:37:27]

Added unittests for homogeneous mapping. Please let me know if there are any you
think should be added/changed.

Also added the check in the mapping function for if the matrix is the identity.

[bsalomon, 2013-08-09 13:53:25]

lgtm, with one nit and one question.

https://codereview.chromium.org/22330004/diff/23001/src/core/SkMatrix.cpp
File src/core/SkMatrix.cpp (right):

https://codereview.chromium.org/22330004/diff/23001/src/core/SkMatrix.cpp#new...
src/core/SkMatrix.cpp:1231: SkASSERT((dst && src && count > 0) || count == 0);
0 == count. (I know the mapPoints() assert is like this but it was probably
written before we had this style).

https://codereview.chromium.org/22330004/diff/23001/tests/MatrixTest.cpp
File tests/MatrixTest.cpp (right):

https://codereview.chromium.org/22330004/diff/23001/tests/MatrixTest.cpp#newc...
tests/MatrixTest.cpp:671: mats[0].mapHomogeneousPoints(NULL, NULL, -1);
This doesn't trigger an assert?

[egdaniel, 2013-08-09 13:58:24]

https://codereview.chromium.org/22330004/diff/23001/src/core/SkMatrix.cpp
File src/core/SkMatrix.cpp (right):

https://codereview.chromium.org/22330004/diff/23001/src/core/SkMatrix.cpp#new...
src/core/SkMatrix.cpp:1231: SkASSERT((dst && src && count > 0) || count == 0);
Should I go back and change mapPoints as well then?

On 2013/08/09 13:53:25, bsalomon wrote:
> 0 == count. (I know the mapPoints() assert is like this but it was probably
> written before we had this style).

https://codereview.chromium.org/22330004/diff/23001/tests/MatrixTest.cpp
File tests/MatrixTest.cpp (right):

https://codereview.chromium.org/22330004/diff/23001/tests/MatrixTest.cpp#newc...
tests/MatrixTest.cpp:671: mats[0].mapHomogeneousPoints(NULL, NULL, -1);
So from what Mike was telling me, the tests are not really meant to check
"failures" but more check "successes", since the tests are not run in debug
mode. Basically this is here to just show that if for some reason it was passed
in with bad values the function won't crash. At least that's what I took
thought.
 
On 2013/08/09 13:53:25, bsalomon wrote:
> This doesn't trigger an assert?

[bsalomon, 2013-08-09 14:04:58]

https://codereview.chromium.org/22330004/diff/23001/src/core/SkMatrix.cpp
File src/core/SkMatrix.cpp (right):

https://codereview.chromium.org/22330004/diff/23001/src/core/SkMatrix.cpp#new...
src/core/SkMatrix.cpp:1231: SkASSERT((dst && src && count > 0) || count == 0);
On 2013/08/09 13:58:24, egdaniel wrote:
> Should I go back and change mapPoints as well then?
> 
> On 2013/08/09 13:53:25, bsalomon wrote:
> > 0 == count. (I know the mapPoints() assert is like this but it was probably
> > written before we had this style).
> 

sure

https://codereview.chromium.org/22330004/diff/23001/tests/MatrixTest.cpp
File tests/MatrixTest.cpp (right):

https://codereview.chromium.org/22330004/diff/23001/tests/MatrixTest.cpp#newc...
tests/MatrixTest.cpp:671: mats[0].mapHomogeneousPoints(NULL, NULL, -1);
On 2013/08/09 13:58:24, egdaniel wrote:
> So from what Mike was telling me, the tests are not really meant to check
> "failures" but more check "successes", since the tests are not run in debug
> mode. Basically this is here to just show that if for some reason it was
passed
> in with bad values the function won't crash. At least that's what I took
> thought.
>  
> On 2013/08/09 13:53:25, bsalomon wrote:
> > This doesn't trigger an assert?
> 

The tests *are* run in debug builds :) So if -1 is illegal input that we assert
on (which seems totally reasonable) then we can't pass it here. We have the
asserts in the debug build so that we will crash, notice, and fix the call site.
In general, if our contract says passing negative values is illegal then it's OK
if the function crashes (even though it won't in this case). If it's legal to
pass negative then we have to change the assert.

[egdaniel, 2013-08-09 15:07:25]

Committed patchset #7 manually as r10659 (presubmit successful).