Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-transforms-2] Permit piecewise rotation interpolation when 0deg angle is present #3236 #3250

Merged
merged 1 commit into from
Nov 12, 2018

Conversation

alancutter
Copy link
Contributor

[css-transforms-2] Permit piecewise rotation interpolation when 0deg angle is present #3236

This change makes rotations like rotateX(45deg) and rotateY(0deg) compatible for simple interpolation without triggering matrix interpolation. Any 0deg rotation can interpolate cleanly with any other rotation.

@alancutter
Copy link
Contributor Author

Looks like I need a W3C account. Request sent.

@alancutter
Copy link
Contributor Author

Have some WIP tests though Firefox and Chrome disagree on how to perform matrix interpolation in the negative case heh.
alancutter/web-platform-tests@2d25a0f#diff-44fa4a5f6830eb8dcb8a4f9b848f43b9R89

@birtles
Copy link
Contributor

birtles commented Oct 26, 2018

Thanks Alan!

I was actually hoping you could explain this part too:

  double dot = a.axis.Dot(b.axis);
  if (dot < 0)
    return false;

  double a_squared = a.axis.LengthSquared();
  double b_squared = b.axis.LengthSquared();
  double error = std::abs(1 - (dot * dot) / (a_squared * b_squared));
  if (error > kAngleEpsilon)
    return false;

  result_axis = a.axis;
  result_angle_a = a.angle;
  result_angle_b = b.angle;
  return true;

@birtles
Copy link
Contributor

birtles commented Oct 26, 2018

Have some WIP tests though Firefox and Chrome disagree on how to perform matrix interpolation in the negative case heh.

That could be because of the issues I am trying to debug in #3230.

@alancutter
Copy link
Contributor Author

alancutter commented Oct 26, 2018

I didn't actually write this algorithm, I just moved it around.
The dot < 0 is a cheap way to test whether the axies are more than 90 degrees apart from each other.
The next bit is seeing how close the dot product of the normalised vectors is to 1 (which indicates they're 0 degrees apart). It's squaring the dot product so that any vector length component in it becomes squared also so that we can divide by squared lengths which is better because they can be computed without needing to call sqrt().

@birtles
Copy link
Contributor

birtles commented Oct 26, 2018

Oh ok, Gecko just compares the vector components after normalizing them. I have no idea if that's equivalent or not.

@alancutter
Copy link
Contributor Author

Ooh Rust.
Seems equivalent to me (minus the epsilon leeway).

@birtles
Copy link
Contributor

birtles commented Oct 29, 2018

Have some WIP tests though Firefox and Chrome disagree on how to perform matrix interpolation in the negative case heh.

After adding the "match zero angles" behavior to Gecko I get the following errors from those wpt:

FAIL Match on rotation vector: Animation between "rotateX(90deg) translateX(100px)" and "rotate3d(50, 0, 0, 180deg) translateY(200px)" at progress 0.25 - assert_equals: expected "matrix3d(1, 0, 0, 0, 0, -0.707107, 0.707107, 0, 0, -0.707107, -0.707107, 0, 75, -35.3553, 35.3553, 1)" but got "matrix3d(1, 0, 0, 0, 0, -0.382683, 0.92388, 0, 0, -0.92388, -0.382683, 0, 75, -19.1342, 46.194, 1)"
FAIL Mismatch on rotation vector: Animation between "rotateX(90deg) translateX(100px)" and "rotateY(90deg) translateY(200px)" at progress 0.25 - assert_equals: expected "matrix3d(0, 0, -1, 0, 0, 1, 0, 0, 1, 0, 0, 0, 0, 50, -75, 1)" but got "matrix3d(0.910684, 0.244017, -0.333333, 0, 0.244017, 0.333333, 0.910684, 0, 0.333333, -0.910684, 0.244017, 0, 80.5021, 34.9679, 20.5342, 1)"
FAIL Match on rotation due to 0deg angle: Animation between "rotateX(90deg) translateX(100px)" and "rotateY(0deg) translateY(200px)" at progress 0.25 - assert_equals: expected "matrix3d(1, 0, 0, 0, 0, 0.573576, 0.819152, 0, 0, -0.819152, 0.573576, 0, 75, 28.6788, 40.9576, 1)" but got "matrix3d(1, 0, 0, 0, 0, 0.382683, 0.92388, 0, 0, -0.92388, 0.382683, 0, 75, 19.1342, 46.194, 1)"

@birtles
Copy link
Contributor

birtles commented Oct 29, 2018

Looking at the first test failure, we have:

rotateX(90deg) translateX(100px) → rotate3d(50, 0, 0, 180deg) translateY(200px)

Which I would expect to be equivalent to:

rotateX(90deg) translateX(100px) → rotateX(180deg) translateY(200px)

And at 25% progress I would expect the result to be:

rotateX(112.5deg) translate(75px, 50px)

i.e.

matrix3d(1, 0, 0, 0, 0, -0.382683, 0.92388, 0, 0, -0.92388, -0.382683, 0, 75, -19.1342, 46.194, 1)

which is what Gecko (with the patch for matching zero angles applied) gives.

Chrome 71 gives me:

matrix3d(1, 0, 0, 0, 0, -1, 1.22465e-16, 0, 0, -1.22465e-16, -1, 0, 75, -50, 6.12323e-15, 1)

which differs from Gecko and the result expected in the test.

@birtles
Copy link
Contributor

birtles commented Oct 29, 2018

For the second test failure, we should be using matrix interpolation and I suspect we're hitting #3230.

By my calculation the result should be:

matrix3d(0.910684, 0.244017, -0.333333, 0, 0.244017, 0.333333, -0.910684, 0, -0.333333, 0.910684, 0.244017, 0, 80.502117, 34.967937, -70.53418, 1)

But Gecko is giving us:

matrix3d(0.910684, 0.244017, -0.333333, 0, 0.244017, 0.333333, 0.910684, 0, 0.333333, -0.910684, 0.244017, 0, 80.5021, 34.9679, 20.5342, 1)

And Chrome and the test expect:

matrix3d(0, 0, -1, 0, 0, 1, 0, 0, 1, 0, 0, 0, 0, 50, -75, 1)

@birtles
Copy link
Contributor

birtles commented Oct 29, 2018

For the third test failure we have:

rotateX(90deg) translateX(100px) → rotateY(0deg) translateY(200px)

which we expect to become:

rotateX(90deg) translateX(100px) → rotateX(0deg) translateY(200px)

and at 25% that should give us:

rotateX(67.5deg) translate(75px, 50px)

i.e.

matrix3d(1, 0, 0, 0, 0, 0.382683, 0.92388, 0, 0, -0.92388, 0.382683, 0, 75, 19.1342, 46.194, 1)

which is what Gecko (with the zero angle matching patch applied) gives.

The test, however, expects:

matrix3d(1, 0, 0, 0, 0, 0.573576, 0.819152, 0, 0, -0.819152, 0.573576, 0, 75, 28.6788, 40.9576, 1)

While Chrome gives:

matrix(1, 0, 0, 1, 75, 50)

@birtles
Copy link
Contributor

birtles commented Oct 29, 2018

In summary, of the three test failures I think we have:

  1. "Match on rotation vector" - test is wrong
  2. "Mismatch on rotation vector" - needs investigation, possibly due to [css-transforms-2] Algorithm for decomposing a 3D matrix, produces different results to 2D matrix #3230
  3. "Match on rotation due to 0deg angle" - test is wrong

@birtles
Copy link
Contributor

birtles commented Oct 29, 2018

For the "Match on rotation vector" test, the angle should be 112.5deg, not 135deg. I suppose 135deg was the result of calculating assuming a progress of 0.5 instead of 0.25.

For the "Match on rotation due to 0deg angle" test, the angle should be 67.5deg not 55deg. I have no idea where the 55deg comes from.

@alancutter Do you mind if I commit those tests with the above fixes and drop the "Mismatch on rotation vector" test for now?

@birtles
Copy link
Contributor

birtles commented Oct 29, 2018

@alancutter Do you mind if I commit those tests with the above fixes and drop the "Mismatch on rotation vector" test for now?

Specifically, this is the patch I am currently looking to commit:
https://hg.mozilla.org/try/rev/b634297ab9ae209e7f380597395736032b7d33fc

@alancutter
Copy link
Contributor Author

@alancutter Do you mind if I commit those tests with the above fixes and drop the "Mismatch on rotation vector" test for now?

No problem! I'm a bit limited in the time I can spend on this so that's certainly appreciated.

@birtles
Copy link
Contributor

birtles commented Oct 30, 2018

Thank you!

@birtles
Copy link
Contributor

birtles commented Nov 11, 2018

The tests for this have now landed in wpt: web-platform-tests/wpt@7d0ccc0

@tabatkins Do you mind reviewing/merging this? This should fix #3236 which I believe you were onboard with.

@tabatkins tabatkins merged commit 59c8b53 into w3c:master Nov 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants