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

RESOLVED FIXED in Firefox 57

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: boris, Assigned: boris)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Assignee)

Description

7 months ago
Created attachment 8896197 [details]
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
(Assignee)

Updated

6 months ago
Blocks: 1387990
(Assignee)

Comment 1

6 months ago
It seems rotate3d has more bugs than I think, so let's fix them together in this bug.
(Assignee)

Updated

6 months ago
Priority: -- → P2
(Assignee)

Updated

6 months ago
Status: NEW → ASSIGNED
(Assignee)

Updated

6 months ago
Summary: stylo: Divided by zero on non-normalizable direction vector of rotate3d transform function → stylo: The interpolations of rotatex, rotatey and rotate3d are not correct.
(Assignee)

Updated

6 months ago
Summary: stylo: The interpolations of rotatex, rotatey and rotate3d are not correct. → stylo: The interpolations of rotatex and rotatey are not correct.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

6 months ago
mozreview-review
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?
Attachment #8897466 - Flags: review?(bbirtles) → review+

Comment 5

6 months ago
mozreview-review
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.
Attachment #8897467 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 6

6 months ago
mozreview-review-reply
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.
(Assignee)

Comment 7

6 months ago
mozreview-review-reply
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.)
(Assignee)

Comment 8

6 months ago
Will rebase these patches later after nox's PR is landed.
(Assignee)

Comment 9

6 months ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

6 months ago
Created attachment 8899170 [details] [review]
Servo PR, #18156

Comment 13

6 months ago
Pushed by bchiou@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/1440eae49c64
https://hg.mozilla.org/mozilla-central/rev/a569340d8992
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.