Fix for bug where composited rotate3d should SLERP

Fix for bug where composited rotate3d should SLERP instead of interpolating the axes and angles numerically.

This aligns with the behavior specified in the specs : http://dev.w3.org/csswg/css-transforms/#interpolation-of-transform-functions

BUG=500797
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/76ccfb05fe20d7c5007ceba84a011fc1453d621c
Cr-Commit-Position: refs/heads/master@{#336520}

[soonm, 2015-06-23 05:02:24]

soonm@google.com changed reviewers:
+ ajuma@chromium.org, vollick@chromium.org

[ajuma, 2015-06-23 13:24:58]

https://codereview.chromium.org/1194073007/diff/1/cc/animation/transform_oper...
File cc/animation/transform_operation.cc (right):

https://codereview.chromium.org/1194073007/diff/1/cc/animation/transform_oper...
cc/animation/transform_operation.cc:134: !Is3DRotation(from) &&
!Is3DRotation(to)) {
I'm not sure I follow how this corresponds to the spec. This says that 3d
rotations should never be interpolated numerically, but the spec seems to say
that they should be interpolated numerically when they have the same normalized
direction vector. So maybe it's ShareSameAxis that needs fixing? Please add
tests (to transform_operations_unittest.cc) that show that we're interpolating
according to the spec in both cases (numerical interpolation for 3d rotations
with the same direction vector, and matrix interpolation when the direction
vectors are different).

[soonm, 2015-06-23 23:43:42]

https://codereview.chromium.org/1194073007/diff/1/cc/animation/transform_oper...
File cc/animation/transform_operation.cc (right):

https://codereview.chromium.org/1194073007/diff/1/cc/animation/transform_oper...
cc/animation/transform_operation.cc:134: !Is3DRotation(from) &&
!Is3DRotation(to)) {
On 2015/06/23 at 13:24:58, ajuma wrote:
> I'm not sure I follow how this corresponds to the spec. This says that 3d
rotations should never be interpolated numerically, but the spec seems to say
that they should be interpolated numerically when they have the same normalized
direction vector. So maybe it's ShareSameAxis that needs fixing? Please add
tests (to transform_operations_unittest.cc) that show that we're interpolating
according to the spec in both cases (numerical interpolation for 3d rotations
with the same direction vector, and matrix interpolation when the direction
vectors are different).

Thanks ajuma, I think you're right. I misunderstood the spec, thinking that
rotate3d should always be interpolated with SLERP. With reference to the bug,
the error is on the main thread RotateTransformOperation.cpp because that does
not check the normalized axis correctly.

Closing this patch.

[soonm, 2015-06-24 08:01:49]

On 2015/06/23 at 23:43:42, soonm wrote:
>
https://codereview.chromium.org/1194073007/diff/1/cc/animation/transform_oper...
> File cc/animation/transform_operation.cc (right):
> 
>
https://codereview.chromium.org/1194073007/diff/1/cc/animation/transform_oper...
> cc/animation/transform_operation.cc:134: !Is3DRotation(from) &&
!Is3DRotation(to)) {
> On 2015/06/23 at 13:24:58, ajuma wrote:
> > I'm not sure I follow how this corresponds to the spec. This says that 3d
rotations should never be interpolated numerically, but the spec seems to say
that they should be interpolated numerically when they have the same normalized
direction vector. So maybe it's ShareSameAxis that needs fixing? Please add
tests (to transform_operations_unittest.cc) that show that we're interpolating
according to the spec in both cases (numerical interpolation for 3d rotations
with the same direction vector, and matrix interpolation when the direction
vectors are different).
> 
> Thanks ajuma, I think you're right. I misunderstood the spec, thinking that
rotate3d should always be interpolated with SLERP. With reference to the bug,
the error is on the main thread RotateTransformOperation.cpp because that does
not check the normalized axis correctly.
> 
> Closing this patch.

Re-opened. I got it wrong again. Sorry mixed up normalized quaternions with
normalized axis. I think the spec should be referring to normalized axis.

If we rotated from

rotate3d(0, 0, 1, 0deg) to
rotate3d(0, 1, 0, 450deg)

SLERP should happen.

This patch should now have that in the test as well as the case where we rotate

rotate3d(0, 0, 1, 0deg) to
rotate3d(0, 0, 1, 450deg)

numeric interpolation should happen.

[ajuma, 2015-06-24 14:47:45]

Thanks! Just a couple more things.

The logic that Blink uses in RotateTransformOperation.cpp doesn't seem to
completely match the spec either, since it never slerps when the |from| axis is
either the x, y, or z axis, regardless of the |to| axis (see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
Please have a look at that logic, and follow up with a fix if it needs to be
changed (so that Blink and cc interpolate 3d rotations the same way).

https://codereview.chromium.org/1194073007/diff/20001/cc/animation/transform_...
File cc/animation/transform_operations.cc (right):

https://codereview.chromium.org/1194073007/diff/20001/cc/animation/transform_...
cc/animation/transform_operations.cc:281: operations_.size() <= i ? 0 :
&operations_[i], progress,
Please add a test for this change too.

[soonm, 2015-06-26 07:46:41]

On 2015/06/24 at 14:47:45, ajuma wrote:
> Thanks! Just a couple more things.
> 
> The logic that Blink uses in RotateTransformOperation.cpp doesn't seem to
completely match the spec either, since it never slerps when the |from| axis is
either the x, y, or z axis, regardless of the |to| axis (see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
Please have a look at that logic, and follow up with a fix if it needs to be
changed (so that Blink and cc interpolate 3d rotations the same way).

Good eye, this is wrong. I'm filing a separate patch to fix that code.

> 
>
https://codereview.chromium.org/1194073007/diff/20001/cc/animation/transform_...
> File cc/animation/transform_operations.cc (right):
> 
>
https://codereview.chromium.org/1194073007/diff/20001/cc/animation/transform_...
> cc/animation/transform_operations.cc:281: operations_.size() <= i ? 0 :
&operations_[i], progress,
> Please add a test for this change too.

[soonm, 2015-06-26 07:47:19]

On 2015/06/26 at 07:46:41, soonm wrote:
> On 2015/06/24 at 14:47:45, ajuma wrote:
> > Thanks! Just a couple more things.
> > 
> > The logic that Blink uses in RotateTransformOperation.cpp doesn't seem to
completely match the spec either, since it never slerps when the |from| axis is
either the x, y, or z axis, regardless of the |to| axis (see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
Please have a look at that logic, and follow up with a fix if it needs to be
changed (so that Blink and cc interpolate 3d rotations the same way).
> 
> Good eye, this is wrong. I'm filing a separate patch to fix that code.
>

New issue for blink at https://codereview.chromium.org/1211353003
 
> > 
> >
https://codereview.chromium.org/1194073007/diff/20001/cc/animation/transform_...
> > File cc/animation/transform_operations.cc (right):
> > 
> >
https://codereview.chromium.org/1194073007/diff/20001/cc/animation/transform_...
> > cc/animation/transform_operations.cc:281: operations_.size() <= i ? 0 :
&operations_[i], progress,
> > Please add a test for this change too.

[soonm, 2015-06-26 07:48:38]

https://codereview.chromium.org/1194073007/diff/20001/cc/animation/transform_...
File cc/animation/transform_operations.cc (right):

https://codereview.chromium.org/1194073007/diff/20001/cc/animation/transform_...
cc/animation/transform_operations.cc:281: operations_.size() <= i ? 0 :
&operations_[i], progress,
On 2015/06/24 at 14:47:45, ajuma wrote:
> Please add a test for this change too.

The new tests I've added for the corrected shareSameAxis checks depend on this
behavior.

So they will correctly fail if this part is reverted. The identity transform
tests currently in the
transform_operations_unittest does test this code part.

I think the original code is an optimization on figuring out if the whole list
of transform operations
are already identity, if so, then just use a 0. However, this is bad for
rotations because rotate axis
does not get encoded into the matrix representations when the rotation angle is
0 deg. (ie no rotation)

so isIdentity will return true...which is sort of correct.

In this change, I allow the for-loop to use those functions even though they
could possibly be identity.
I suspect only a performance test with many transforms will be able to pick it
up.

[ajuma, 2015-06-26 13:13:31]

lgtm

https://codereview.chromium.org/1194073007/diff/20001/cc/animation/transform_...
File cc/animation/transform_operations.cc (right):

https://codereview.chromium.org/1194073007/diff/20001/cc/animation/transform_...
cc/animation/transform_operations.cc:281: operations_.size() <= i ? 0 :
&operations_[i], progress,
On 2015/06/26 07:48:38, soonm wrote:
> On 2015/06/24 at 14:47:45, ajuma wrote:
> > Please add a test for this change too.
> 
> The new tests I've added for the corrected shareSameAxis checks depend on this
> behavior.
> 
> So they will correctly fail if this part is reverted. The identity transform
> tests currently in the
> transform_operations_unittest does test this code part.
> 
> I think the original code is an optimization on figuring out if the whole list
> of transform operations
> are already identity, if so, then just use a 0. However, this is bad for
> rotations because rotate axis
> does not get encoded into the matrix representations when the rotation angle
is
> 0 deg. (ie no rotation)

Ah, of course, thanks for the explanation!

[soonm, 2015-06-28 23:35:22]

The CQ bit was checked by soonm@google.com

[commit-bot: I haz the power, 2015-06-28 23:35:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1194073007/20001

[commit-bot: I haz the power, 2015-06-29 00:29:48]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-06-29 00:30:48]

Patchset 2 (id:??) landed as
https://crrev.com/76ccfb05fe20d7c5007ceba84a011fc1453d621c
Cr-Commit-Position: refs/heads/master@{#336520}