Interpolated rotations should end cleanly where possible.

Interpolated rotations should end cleanly where possible.

Due to numeric issues, interpolated rotations of 90 degrees end at 'unclean' transforms. That is, they have some entries that are not 0, 1, or -1. With this patch, interpolated rotations detect when rotations should end cleanly and massage matrix entries where appropriate.

BUG=chrome-os-partner:9166
TEST=InterpolatedTransformTest.ScreenRotationEndsCleanly, InterpolatedTransformTest.MaximizeEndsCleanly

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133986

[Ian Vollick, 2012-04-24 17:03:28]

Interpolated rotations should be smarter about cleaning up rotation matrices
when they represent a rotation of a multiple of 90 degrees. James, I'm hoping
this is the lower-level fix you were looking for.

[piman, 2012-04-24 17:15:43]

https://chromiumcodereview.appspot.com/10210002/diff/2001/ui/gfx/interpolated...
File ui/gfx/interpolated_transform.cc (right):

https://chromiumcodereview.appspot.com/10210002/diff/2001/ui/gfx/interpolated...
ui/gfx/interpolated_transform.cc:46: if (fabs(entry) < EPSILON)
I worry that we may miss this, especially for large values of degrees.
In https://code.google.com/p/chrome-os-partner/issues/detail?id=9166 there is an
error that is larger than your epsilon.

For a rotation of n*90 degrees, there's only 4 matrices:
1, 0
0, 1

0, 1
-1, 0

-1, 0
0, -1

0, -1
1, 0

Could we snap degrees/90 to the nearest integer, mod 4, and pick the exact
matrix?

[James Cook, 2012-04-24 17:18:18]

LGTM - Based on your test passing I think this would solve my problem, but I
defer to piman's expertise.

https://chromiumcodereview.appspot.com/10210002/diff/2001/ui/gfx/interpolated...
File ui/gfx/interpolated_transform.cc (right):

https://chromiumcodereview.appspot.com/10210002/diff/2001/ui/gfx/interpolated...
ui/gfx/interpolated_transform.cc:20: bool IsAMultipleOfNinetyDegrees(float
degrees)
nit: Maybe just IsMultipleOfNinetyDegrees()?

https://chromiumcodereview.appspot.com/10210002/diff/2001/ui/gfx/interpolated...
ui/gfx/interpolated_transform.cc:204:
MassageRotationIfMultipleOfNinetyDegrees(&result, interpolated_degrees);
Do we want to always do this or just when t == 0 or t == 1?

https://chromiumcodereview.appspot.com/10210002/diff/2001/ui/gfx/interpolated...
File ui/gfx/interpolated_transform_unittest.cc (right):

https://chromiumcodereview.appspot.com/10210002/diff/2001/ui/gfx/interpolated...
ui/gfx/interpolated_transform_unittest.cc:193: TEST(InterpolatedTransformTest,
ScreenRotationEndsCleanly) {
Awesome that you added a test for this!

[Ian Vollick, 2012-04-24 18:00:00]

On 2012/04/24 17:15:43, piman wrote:
>
https://chromiumcodereview.appspot.com/10210002/diff/2001/ui/gfx/interpolated...
> File ui/gfx/interpolated_transform.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10210002/diff/2001/ui/gfx/interpolated...
> ui/gfx/interpolated_transform.cc:46: if (fabs(entry) < EPSILON)
> I worry that we may miss this, especially for large values of degrees.
> In https://code.google.com/p/chrome-os-partner/issues/detail?id=9166 there is
an
> error that is larger than your epsilon.
> 
> For a rotation of n*90 degrees, there's only 4 matrices:
> 1, 0
> 0, 1
> 
> 0, 1
> -1, 0
> 
> -1, 0
> 0, -1
> 
> 0, -1
> 1, 0
> 
> Could we snap degrees/90 to the nearest integer, mod 4, and pick the exact
> matrix?

Good idea. Done.

[Ian Vollick, 2012-04-24 18:00:16]

On 2012/04/24 17:18:18, James Cook (Chromium) wrote:
> LGTM - Based on your test passing I think this would solve my problem, but I
> defer to piman's expertise.
> 
>
https://chromiumcodereview.appspot.com/10210002/diff/2001/ui/gfx/interpolated...
> File ui/gfx/interpolated_transform.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10210002/diff/2001/ui/gfx/interpolated...
> ui/gfx/interpolated_transform.cc:20: bool IsAMultipleOfNinetyDegrees(float
> degrees)
> nit: Maybe just IsMultipleOfNinetyDegrees()?
> 
Done.
>
https://chromiumcodereview.appspot.com/10210002/diff/2001/ui/gfx/interpolated...
> ui/gfx/interpolated_transform.cc:204:
> MassageRotationIfMultipleOfNinetyDegrees(&result, interpolated_degrees);
> Do we want to always do this or just when t == 0 or t == 1?
> 
Good point. Done.
>
https://chromiumcodereview.appspot.com/10210002/diff/2001/ui/gfx/interpolated...
> File ui/gfx/interpolated_transform_unittest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10210002/diff/2001/ui/gfx/interpolated...
> ui/gfx/interpolated_transform_unittest.cc:193: TEST(InterpolatedTransformTest,
> ScreenRotationEndsCleanly) {
> Awesome that you added a test for this!

[Ian Vollick, 2012-04-24 18:01:15]

+sky for OWNERS

[piman, 2012-04-24 18:40:38]

lgtm

[sky, 2012-04-24 19:43:00]

LGTM

https://chromiumcodereview.appspot.com/10210002/diff/11002/ui/gfx/interpolate...
File ui/gfx/interpolated_transform.cc (right):

https://chromiumcodereview.appspot.com/10210002/diff/11002/ui/gfx/interpolate...
ui/gfx/interpolated_transform.cc:21: {
nit: { on previous line (33 too).

[commit-bot: I haz the power, 2012-04-25 20:13:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vollick@chromium.org/10210002/20001

[commit-bot: I haz the power, 2012-04-25 21:39:13]

Change committed as 133986