add isSimilarityTransform() and some tests

add isSimilarityTransform() and some tests

As discussed, this function is separated from the big CL (circle drawing:
http://codereview.appspot.com/5696086/).

The tests are run successfully both in SK_SCALAR_IS_FLOAT and SK_SCALAR_IS_FIXED configuration.

BUG=
TEST=

[guanqun, 2012-04-11 06:20:58]

1. Use Brian's algorithm to save a few operations when setting up 'vec'.
2. Add check for all zero case.

Please review!

[reed1, 2012-04-11 13:00:38]

http://codereview.appspot.com/5999050/diff/1/include/core/SkMatrix.h
File include/core/SkMatrix.h (right):

http://codereview.appspot.com/5999050/diff/1/include/core/SkMatrix.h#newcode89
include/core/SkMatrix.h:89: *  if the matrix doesn't have skew and it doesn't
contains perspective
and its scale is square (i.e. scaleX == scaleY).

Put another way: the matrix only includes square-scale, rotation, translation.

http://codereview.appspot.com/5999050/diff/1/src/core/SkMatrix.cpp
File src/core/SkMatrix.cpp (right):

http://codereview.appspot.com/5999050/diff/1/src/core/SkMatrix.cpp#newcode1154
src/core/SkMatrix.cpp:1154: 
Do we want tol^2 for both of these?

[bsalomon, 2012-04-11 13:35:01]

http://codereview.appspot.com/5999050/diff/1/src/core/SkMatrix.cpp
File src/core/SkMatrix.cpp (right):

http://codereview.appspot.com/5999050/diff/1/src/core/SkMatrix.cpp#newcode1154
src/core/SkMatrix.cpp:1154: 
On 2012/04/11 13:00:38, reed1 wrote:
> Do we want tol^2 for both of these?

I suppose they are both checking length squared values so maybe.

[guanqun, 2012-04-12 06:52:44]

I'm removing the interface in SkMatrix, because as we don't track rotated
circles, it's used once once in SkGpuDevice.cpp file.

If I'm using tol^2, then that fails on SK_FLOAT_IS_FIXED, on fixed platform,
SK_ScalarNearlyZero squared would give back zero and then it fails in
SkASSERT(tolerance > 0) check.

From the comments:
308 /*  <= is slower than < for floats, so we use < for our tolerance test
309 */

So is it a good time to add back "<=" or we agree to use "tol" for comparison
here?

Thanks!

[reed1, 2012-04-12 12:52:21]

I think that "< is faster than <=" is dumb. I've changed it to <=
Does that resolve your issue?

[guanqun, 2012-04-13 01:13:38]

On 2012/04/12 12:52:21, reed1 wrote:
> I think that "< is faster than <=" is dumb. I've changed it to <=
> Does that resolve your issue?

Yes, it can resolve the SK_SCALAR_IS_FIXED issue.

Please review again. Thanks.

[reed1, 2012-04-13 12:14:28]

I wonder if there is a way to use tables of data to reduce the amount of
copy/paste of code in all of these tests.

If not, LGTM

[guanqun, 2012-04-14 13:13:07]

On 2012/04/13 12:14:28, reed1 wrote:
> I wonder if there is a way to use tables of data to reduce the amount of
> copy/paste of code in all of these tests.
> 
> If not, LGTM

There's a way to use tables of data. e.g. add the structure
struct data {
 skscalar a, b, c, d, e, f, g, h, i;
 bool expectedresult;
} arrays[] = {
 ......
};
then,
for each one in arrays:
   construct matrix from a, b, c, d, e, f, g, h, i;
   test the function with expected result.

I'm relunctant to change to this style because:
1) I have to precalculate the result for each elements in skmatrix. however, in
the current way, I can use postTranslate() etc.
2) If some test fails, the failure would only output the file line. It would be
much easier to track which test case fails in the current expanded style than
using a for loop.

[bsalomon, 2012-04-16 14:07:05]

On 2012/04/14 13:13:07, guanqun wrote:
> On 2012/04/13 12:14:28, reed1 wrote:
> > I wonder if there is a way to use tables of data to reduce the amount of
> > copy/paste of code in all of these tests.
> > 
> > If not, LGTM
> 
> There's a way to use tables of data. e.g. add the structure
> struct data {
>  skscalar a, b, c, d, e, f, g, h, i;
>  bool expectedresult;
> } arrays[] = {
>  ......
> };
> then,
> for each one in arrays:
>    construct matrix from a, b, c, d, e, f, g, h, i;
>    test the function with expected result.
> 
> I'm relunctant to change to this style because:
> 1) I have to precalculate the result for each elements in skmatrix. however,
in
> the current way, I can use postTranslate() etc.
> 2) If some test fails, the failure would only output the file line. It would
be
> much easier to track which test case fails in the current expanded style than
> using a for loop.

Committed at r3681

[bsalomon, 2012-04-16 14:41:17]

On 2012/04/16 14:07:05, bsalomon wrote:
> On 2012/04/14 13:13:07, guanqun wrote:
> > On 2012/04/13 12:14:28, reed1 wrote:
> > > I wonder if there is a way to use tables of data to reduce the amount of
> > > copy/paste of code in all of these tests.
> > > 
> > > If not, LGTM
> > 
> > There's a way to use tables of data. e.g. add the structure
> > struct data {
> >  skscalar a, b, c, d, e, f, g, h, i;
> >  bool expectedresult;
> > } arrays[] = {
> >  ......
> > };
> > then,
> > for each one in arrays:
> >    construct matrix from a, b, c, d, e, f, g, h, i;
> >    test the function with expected result.
> > 
> > I'm relunctant to change to this style because:
> > 1) I have to precalculate the result for each elements in skmatrix. however,
> in
> > the current way, I can use postTranslate() etc.
> > 2) If some test fails, the failure would only output the file line. It would
> be
> > much easier to track which test case fails in the current expanded style
than
> > using a for loop.
> 
> Committed at r3681

I reverted this at r3684 because the fixed point build fails MatrixTest.

[guanqun.lu_gmail.com, 2012-04-16 15:39:54]

oops, which platform is seeing this failure? I've tested it with fixed
point on x86_64 and both debug and release build pass the matrix test.

On Mon, Apr 16, 2012 at 10:41 PM,  <bsalomon@google.com> wrote:
>
> I reverted this at r3684 because the fixed point build fails MatrixTest.
>
>
>
> http://codereview.appspot.com/5999050/



-- 
Guanqun

[bsalomon, 2012-04-16 15:52:08]

Actually looking back on the bot runs it appears to only have failed on the
Mac.

On Mon, Apr 16, 2012 at 11:39 AM, Lu Guanqun <guanqun.lu@gmail.com> wrote:

> oops, which platform is seeing this failure? I've tested it with fixed
> point on x86_64 and both debug and release build pass the matrix test.
>
> On Mon, Apr 16, 2012 at 10:41 PM,  <bsalomon@google.com> wrote:
> >
> > I reverted this at r3684 because the fixed point build fails MatrixTest.
> >
> >
> >
> > http://codereview.appspot.com/5999050/
>
>
>
> --
> Guanqun
>

[guanqun, 2012-04-17 01:32:56]

Tolerance is too tight for fixed build. So relax it.

I've tested it on an IMac machine(Lion), it works OK for both float and fixed
build.

Please try to re-land this CL and see if it would break your build again...

[reed1, 2012-04-17 12:31:06]

1/64 is a pretty loose tolerance. Do we need to look at how we compute
is-similar in the fixed case? Can we do it and keep more precision?

[guanqun, 2012-04-18 05:02:45]

In the latest patchset, I propose to casting SkScalar to float and do the
calculation. Therefore, all the problems are set. Do you think this is good way
to go? (I've tested this on both Linux and iMac with both float and fixed
build).

Otherwise, there are several possible workarounds:
1. make fix build's tolerance to be 1/2^8.
2. compare dot result with tol while compare lengthSqd with tol^2.

[reed1, 2012-04-20 20:46:46]

I think the use of the tolerance parameter in the function is incorrect, but I
don't know what the correct usage is. e.g. if the matrix has a very large, or
very small scale applied, the computed error terms grow big or small, but our
tolerance values do not. That seems like a problem (but I'm not sure).

If we are already using this function, I would rather have some tests then none,
so I'm going to suggest that we just suppress the fixed-point tests that fail at
the moment (with some big comment) rather than tweak the tolerance or some other
workaround, since doing so might suggest that we think that is a "correct" fix.

[guanqun, 2012-04-24 02:23:54]

Hi Mike,

In latest patchset, I bypass the failed tests on Mac platform and added some
comments to it. And I use a previous implementation of isSimilarityTransform()
without converting fixed value to float etc.

Please review. thanks!

Tested both on linux and mac platform and both for fixed build and float build.

[reed1, 2012-04-24 16:10:30]

lgtm, w/ a question about the test for 4 zeros in the middle of the function.

http://codereview.appspot.com/5999050/diff/28001/tests/MatrixTest.cpp
File tests/MatrixTest.cpp (right):

http://codereview.appspot.com/5999050/diff/28001/tests/MatrixTest.cpp#newcode206
tests/MatrixTest.cpp:206: 
What does this test do that isn't done by the tests below?

[guanqun, 2012-04-25 07:53:39]

Question answered. And please help land this CL if it's OK.

http://codereview.appspot.com/5999050/diff/28001/tests/MatrixTest.cpp
File tests/MatrixTest.cpp (right):

http://codereview.appspot.com/5999050/diff/28001/tests/MatrixTest.cpp#newcode206
tests/MatrixTest.cpp:206: 
On 2012/04/24 16:10:30, reed1 wrote:
> What does this test do that isn't done by the tests below?

No. The tests below would return true when all numbers are zero, which should
return false. So we have to check it explicitly.

[bsalomon, 2012-04-25 15:16:34]

On 2012/04/25 07:53:39, guanqun wrote:
> Question answered. And please help land this CL if it's OK.
> 
> http://codereview.appspot.com/5999050/diff/28001/tests/MatrixTest.cpp
> File tests/MatrixTest.cpp (right):
> 
>
http://codereview.appspot.com/5999050/diff/28001/tests/MatrixTest.cpp#newcode206
> tests/MatrixTest.cpp:206: 
> On 2012/04/24 16:10:30, reed1 wrote:
> > What does this test do that isn't done by the tests below?
> 
> No. The tests below would return true when all numbers are zero, which should
> return false. So we have to check it explicitly.

Landed at r3762