Fix common axes logic to detect negated axes as not common

Fix common axes logic to detect negated axes as not common

When interpolating rotate3d transforms we were considering axes that are
the negation of each other to be in the same direction.
This patch fixes this mistake by checking the sign of the dot product.

Spec text for rotate3d interpolation:
https://drafts.csswg.org/css-transforms/#ref-for-funcdef-rotate3d-3
For interpolations with the primitive rotate3d(), the direction vectors
of the transform functions get normalized first. If the normalized
vectors are equal, the rotation angle gets interpolated numerically.
Otherwise the transform functions get converted into 4x4 matrices first
and interpolated as defined in section Interpolation of Matrices
afterwards.

BUG=652705
Committed: https://crrev.com/b69d3d2ec0d8bd333157dc33f5e1da1eaf76e5fb
Cr-Commit-Position: refs/heads/master@{#423802}

[alancutter (OOO until 2018), 2016-10-05 06:19:55]

alancutter@chromium.org changed reviewers:
+ jbroman@chromium.org

[jbroman, 2016-10-06 22:28:09]

Apologies for the review delay.

This is unintuitive to me; I'd have expected them to still be considered to have
a common axis.
Can you clarify why this is the correct fix, rather than than doing this:

resultAngleB = dot < 0 ? -b.angle : b.angle;

(i.e. making them considered to have the same axis, with the angles adjusted to
compensate)

[alancutter (OOO until 2018), 2016-10-07 00:30:43]

Description was changed from

==========
Fix common axes logic to detect negated axes as not common

When interpolating rotate3d transforms we were considering axes that are
the negation of each other to be in the same direction.
This patch fixes this mistake by checking the sign of the dot product.

BUG=652705
==========

to

==========
Fix common axes logic to detect negated axes as not common

When interpolating rotate3d transforms we were considering axes that are
the negation of each other to be in the same direction.
This patch fixes this mistake by checking the sign of the dot product.

Spec text for rotate3d interpolation:
https://drafts.csswg.org/css-transforms/#ref-for-funcdef-rotate3d-3

BUG=652705
==========

[alancutter (OOO until 2018), 2016-10-07 00:31:28]

Description was changed from

==========
Fix common axes logic to detect negated axes as not common

When interpolating rotate3d transforms we were considering axes that are
the negation of each other to be in the same direction.
This patch fixes this mistake by checking the sign of the dot product.

Spec text for rotate3d interpolation:
https://drafts.csswg.org/css-transforms/#ref-for-funcdef-rotate3d-3

BUG=652705
==========

to

==========
Fix common axes logic to detect negated axes as not common

When interpolating rotate3d transforms we were considering axes that are
the negation of each other to be in the same direction.
This patch fixes this mistake by checking the sign of the dot product.

Spec text for rotate3d interpolation:
https://drafts.csswg.org/css-transforms/#ref-for-funcdef-rotate3d-3
For interpolations with the primitive rotate3d(), the direction vectors
of the transform functions get normalized first. If the normalized
vectors are equal, the rotation angle gets interpolated numerically.
Otherwise the transform functions get converted into 4x4 matrices first
and interpolated as defined in section Interpolation of Matrices
afterwards.

BUG=652705
==========

[alancutter (OOO until 2018), 2016-10-07 00:33:45]

On 2016/10/06 at 22:28:09, jbroman wrote:
> Apologies for the review delay.
> 
> This is unintuitive to me; I'd have expected them to still be considered to
have a common axis.
> Can you clarify why this is the correct fix, rather than than doing this:
> 
> resultAngleB = dot < 0 ? -b.angle : b.angle;
> 
> (i.e. making them considered to have the same axis, with the angles adjusted
to compensate)

That looks like a sensible way to handle this case to me. I double checked the
spec and it doesn't seem to allow for that behaviour so leaving patch as is.

[alancutter (OOO until 2018), 2016-10-07 00:34:01]

On 2016/10/07 at 00:33:45, alancutter wrote:
> On 2016/10/06 at 22:28:09, jbroman wrote:
> > Apologies for the review delay.
> > 
> > This is unintuitive to me; I'd have expected them to still be considered to
have a common axis.
> > Can you clarify why this is the correct fix, rather than than doing this:
> > 
> > resultAngleB = dot < 0 ? -b.angle : b.angle;
> > 
> > (i.e. making them considered to have the same axis, with the angles adjusted
to compensate)
> 
> That looks like a sensible way to handle this case to me. I double checked the
spec and it doesn't seem to allow for that behaviour so leaving patch as is.

(Added spec text to the description.)

[jbroman, 2016-10-07 00:41:45]

lgtm

Thanks for the spec reference.

[alancutter (OOO until 2018), 2016-10-07 00:42:46]

Thanks for the review.

[alancutter (OOO until 2018), 2016-10-07 00:42:58]

The CQ bit was checked by alancutter@chromium.org

[commit-bot: I haz the power, 2016-10-07 00:43:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 01:47:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-07 01:47:20]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[alancutter (OOO until 2018), 2016-10-07 03:12:54]

The CQ bit was checked by alancutter@chromium.org

[alancutter (OOO until 2018), 2016-10-07 03:12:55]

The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2391003004/#ps20001
(title: "Update expectation")

[commit-bot: I haz the power, 2016-10-07 03:13:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 04:30:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-07 04:30:17]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[alancutter (OOO until 2018), 2016-10-07 04:33:31]

The CQ bit was checked by alancutter@chromium.org

[commit-bot: I haz the power, 2016-10-07 04:33:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 05:54:21]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-07 05:56:45]

Description was changed from

==========
Fix common axes logic to detect negated axes as not common

When interpolating rotate3d transforms we were considering axes that are
the negation of each other to be in the same direction.
This patch fixes this mistake by checking the sign of the dot product.

Spec text for rotate3d interpolation:
https://drafts.csswg.org/css-transforms/#ref-for-funcdef-rotate3d-3
For interpolations with the primitive rotate3d(), the direction vectors
of the transform functions get normalized first. If the normalized
vectors are equal, the rotation angle gets interpolated numerically.
Otherwise the transform functions get converted into 4x4 matrices first
and interpolated as defined in section Interpolation of Matrices
afterwards.

BUG=652705
==========

to

==========
Fix common axes logic to detect negated axes as not common

When interpolating rotate3d transforms we were considering axes that are
the negation of each other to be in the same direction.
This patch fixes this mistake by checking the sign of the dot product.

Spec text for rotate3d interpolation:
https://drafts.csswg.org/css-transforms/#ref-for-funcdef-rotate3d-3
For interpolations with the primitive rotate3d(), the direction vectors
of the transform functions get normalized first. If the normalized
vectors are equal, the rotation angle gets interpolated numerically.
Otherwise the transform functions get converted into 4x4 matrices first
and interpolated as defined in section Interpolation of Matrices
afterwards.

BUG=652705

Committed: https://crrev.com/b69d3d2ec0d8bd333157dc33f5e1da1eaf76e5fb
Cr-Commit-Position: refs/heads/master@{#423802}
==========

[commit-bot: I haz the power, 2016-10-07 05:56:46]

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