1389429 - stylo: The interpolations of rotatex and rotatey are not correct.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P2)

Description

[Boris Chiou [:boris]]

Attachment: Assertion on non-normalizable rotate3d transform function

We don't check normalizable for the direction vector (i.e. rotate axis) in add_weighted_transform_lists(). This may cause an assertion.

[1] http://searchfox.org/mozilla-central/rev/4b79f3b23aebb4080ea85e94351fd4046116a957/servo/components/style/properties/helpers/animated_properties.mako.rs#1714-1717

Comment 1

[Boris Chiou [:boris]]

It seems rotate3d has more bugs than I think, so let's fix them together in this bug.

Comment 2

[Boris Chiou [:boris]]

Attachment: Bug 1389429 - Part 1: Add a test for rotate3d with the non-nomalizable direction vector.

While normalizing the direction vector, we have to make sure we don't
divide the components by zero length to avoid any assertion of "NaN".

Review commit: https://reviewboard.mozilla.org/r/168766/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168766/

Comment 3

[Boris Chiou [:boris]]

Attachment: Bug 1389429 - Part 2: Update test_transitions_per_property.html for rotatex and rotatey.

Let's see the example: "rotatex(360deg)" to "none".

While we do interpolation between "rotatex" and "none", the original code
path is:
1. Build an identity transform for none and always use (0, 0, 1) as the
   direction vector, i.e. Rotate(0, 0, 1, 0deg).
2. Do interpolation between rotatex (i.e. Rotate(1, 0, 0, 360deg)) and
   Rotate(0, 0, 1, 0deg). Because their direction vectors are different,
   so we do matrix decomposition/interpolation/recomposition on both
   functions. The problem is, matrix decomposition makes the 360deg
   disappear, so it looks like "rotatex(0deg)".
3. Obviously, we are trying to interpolate from rotatex(0deg) to none, so
   the interpolated matrix is always an identity matrix.

I think rotatex, rotatey, and rotatez are special cases which should
really rotate according to the angle, so we should build the identity
transform for them according to the normalized direction vector, and so we do
interpolation on the angle, instead of converting them into matrix.

Replacing build_identity_transform_list() with
add_weighted_transform_lists(list, list, 0, 0) might be another solution;
However, I didn't do that because build_identity_transform_list() is
much simpler.

Review commit: https://reviewboard.mozilla.org/r/168768/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168768/

Comment 4

[Brian Birtles (:birtles)]

Comment on attachment 8897466 [details]
Bug 1389429 - Part 1: Add a test for rotate3d with the non-nomalizable direction vector.

https://reviewboard.mozilla.org/r/168766/#review174264

::: dom/animation/test/mochitest.ini:129
(Diff revision 1)
>  [style/test_animation-setting-effect.html]
>  [style/test_composite.html]
>  [style/test_interpolation-from-interpolatematrix-to-none.html]
>  [style/test_missing-keyframe.html]
>  [style/test_missing-keyframe-on-compositor.html]
> +[style/test_transform_non-normalizable_rotate3d.html]

This file name is odd. I think it should be test_transform-non-normalizable-rotate3d.html

(The idea is that we normally use - to separate words (this might have been a web-platform-tests requirement at one stage, I don't recall). However, our test harness looks for 'test_'. So, when we wrote the tests in dom/animation, because we expected them to mostly end up in web-platform-tests, we used - to separate the words, but test_ as the prefix.)

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1950
(Diff revision 1)
> +    fn values(&self) -> (f32, f32, f32) {
> +        (self.0.x as f32, self.0.y as f32, self.0.z as f32)
> +    }

Any reason we convert to single-precision float at the same time?

If there is, I wonder if there is a better name for this, or a most Rust-like way of expressing this?

Comment 5

[Brian Birtles (:birtles)]

Comment on attachment 8897467 [details]
Bug 1389429 - Part 2: Update test_transitions_per_property.html for rotatex and rotatey.

https://reviewboard.mozilla.org/r/168768/#review174268

::: commit-message-80488:3
(Diff revision 1)
> +Let's see the example: "rotatex(360deg)" to "none".
> +
> +While we do interpolation between "rotatex" and "none", the original code
> +path is:
> +1. Build an identity transform for none and always use (0, 0, 1) as the
> +   direction vector, i.e. Rotate(0, 0, 1, 0deg).
> +2. Do interpolation between rotatex (i.e. Rotate(1, 0, 0, 360deg)) and
> +   Rotate(0, 0, 1, 0deg). Because their direction vectors are different,
> +   so we do matrix decomposition/interpolation/recomposition on both
> +   functions. The problem is, matrix decomposition makes the 360deg
> +   disappear, so it looks like "rotatex(0deg)".
> +3. Obviously, we are trying to interpolate from rotatex(0deg) to none, so
> +   the interpolated matrix is always an identity matrix.
> +
> +I think rotatex, rotatey, and rotatez are special cases which should
> +really rotate according to the angle, so we should build the identity
> +transform for them according to the normalized direction vector, and so we do
> +interpolation on the angle, instead of converting them into matrix.

Excellent explanation! Thank you!!

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1357
(Diff revision 1)
> -                result.push(TransformOperation::Rotate(0.0, 0.0, 1.0, Angle::zero()));
> +                let (vector, _) = get_normalized_vector_and_angle(x, y, z, a);
> +                let (x, y, z) = vector.values();
> +                result.push(TransformOperation::Rotate(x, y, z, Angle::zero()));

It seems like 2 of the 3 call sites of this function end up splitting the vector into its component values again. And then there's the issue of going from f32 -> f64 -> f32.

Would it make sense to just have get_normalized_vector_and_angle return (x, y, z, angle) and then, in the one call site where we want a vector use destructuring assignment to build a DirectionVector there? Would that even work?

Up to you. I'm fine with this as it is, but that might be a little neater if it works.

Comment 6

[Boris Chiou [:boris]]

Comment on attachment 8897466 [details]
Bug 1389429 - Part 1: Add a test for rotate3d with the non-nomalizable direction vector.

https://reviewboard.mozilla.org/r/168766/#review174264

> This file name is odd. I think it should be test_transform-non-normalizable-rotate3d.html
> 
> (The idea is that we normally use - to separate words (this might have been a web-platform-tests requirement at one stage, I don't recall). However, our test harness looks for 'test_'. So, when we wrote the tests in dom/animation, because we expected them to mostly end up in web-platform-tests, we used - to separate the words, but test_ as the prefix.)

Thanks for the explanation of the name. I will update it!

> Any reason we convert to single-precision float at the same time?
> 
> If there is, I wonder if there is a better name for this, or a most Rust-like way of expressing this?

The caller uses these componenets as the input of a new Rotate(x, y, z, angle), whose parameter types are all f32. Converting to f32 here or after the caller is ok to me. I guess a Rust-like way is From<T> trait, but our output is a tuple, not a single type. I can move the conversion to the caller side to avoid conversion here at the same time.

Comment 7

[Boris Chiou [:boris]]

Comment on attachment 8897467 [details]
Bug 1389429 - Part 2: Update test_transitions_per_property.html for rotatex and rotatey.

https://reviewboard.mozilla.org/r/168768/#review174268

> It seems like 2 of the 3 call sites of this function end up splitting the vector into its component values again. And then there's the issue of going from f32 -> f64 -> f32.
> 
> Would it make sense to just have get_normalized_vector_and_angle return (x, y, z, angle) and then, in the one call site where we want a vector use destructuring assignment to build a DirectionVector there? Would that even work?
> 
> Up to you. I'm fine with this as it is, but that might be a little neater if it works.

Yes. I can do that that (just need to update some function signature.)

Comment 8

[Boris Chiou [:boris]]

Will rebase these patches later after nox's PR is landed.

Comment 9

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #8)
> Will rebase these patches later after nox's PR is landed.

it seems nox's patches also need to be rebased, so I'd like to land these first because these block my other patches.

Comment 10

[Boris Chiou [:boris]]

Comment on attachment 8897466 [details]
Bug 1389429 - Part 1: Add a test for rotate3d with the non-nomalizable direction vector.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168766/diff/1-2/

Comment 11

[Boris Chiou [:boris]]

Comment on attachment 8897467 [details]
Bug 1389429 - Part 2: Update test_transitions_per_property.html for rotatex and rotatey.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168768/diff/1-2/

Comment 12

[Boris Chiou [:boris]]

Attachment: Servo PR, #18156

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1440eae49c64
Part 1: Add a test for rotate3d with the non-nomalizable direction vector. r=birtles
https://hg.mozilla.org/integration/autoland/rev/a569340d8992
Part 2: Update test_transitions_per_property.html for rotatex and rotatey. r=birtles

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1440eae49c64
https://hg.mozilla.org/mozilla-central/rev/a569340d8992