Implement matrix decomposition and interpolation #13188

canova · 2016-09-06T22:29:20Z

Implement 2D and 3D matrix decomposition and interpolation for animated properties.
r? @Manishearth

./mach build -d does not report any errors
./mach test-tidy does not report any errors

There are tests for these changes OR
These changes do not require tests because there are no tests for it and we manually tested it.

This change is


        Implement decompose_matrix function


        Implement Interpolate traits for matrix structs


        Implement recompose_matrix function

highfive · 2016-09-06T22:29:24Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/helpers/animated_properties.mako.rs

highfive · 2016-09-06T22:29:25Z

Warning

These commits modify style code, but no tests are modified. Please consider adding a test!

canova · 2016-09-06T22:31:07Z

components/style/properties/helpers/animated_properties.mako.rs

@@ -751,12 +769,236 @@ impl Interpolate for LengthOrNone {
            }
        } else {
            // TODO(gw): Implement matrix decomposition and interpolation
+            // TODO(canaltinova): Do I need to interpolate each item of the list?


I'm not sure how to implement interpolation here

ignore this for now

canova · 2016-09-06T22:53:28Z

components/style/properties/helpers/animated_properties.mako.rs

+                        let mut fd_matrix = ComputedMatrix::identity();
+                        let mut td_matrix = ComputedMatrix::identity();
+                        fd_matrix.m43 = -1. / fd.0 as f32;
+                        td_matrix.m43 = -1. / _td.0 as f32;


I had to convert Au value to f32 but I'm not sure if it's correct or not.

You have Au::to_f32_px() if I'm not wrong.

You are right, thanks!

Manishearth · 2016-09-07T03:28:39Z

components/style/properties/helpers/animated_properties.mako.rs

@@ -701,8 +703,11 @@ impl Interpolate for LengthOrNone {
                match (from, to) {
                    (&TransformOperation::Matrix(from),
                     &TransformOperation::Matrix(_to)) => {
-                        // TODO(gw): Implement matrix decomposition and interpolation
-                        result.push(TransformOperation::Matrix(from));
+                        let decomposed_from = MatrixDecomposed2D::from(from);


leave comments that it doesn't yet handle cases where one of the two matrices is not 2D

Manishearth · 2016-09-07T03:30:06Z

components/style/properties/helpers/animated_properties.mako.rs

+
+                            let decomposed_f = MatrixDecomposed2D::from(matrix_f);
+                            let decomposed_t = MatrixDecomposed2D::from(matrix_t);
+                            let interpolated = decomposed_f.interpolate(&decomposed_t, time).unwrap();


this code is common (and will get more complicated when we handle 3d interpolation), perhaps we should implement Interpolate for ComputedMatrix?

Manishearth · 2016-09-07T03:30:34Z

components/style/properties/helpers/animated_properties.mako.rs

            result.extend_from_slice(from_list);
        }

        TransformList(Some(result))
    }

+    fn rotate_to_matrix(x: f32, y: f32, z: f32, a: SpecifiedAngle) -> ComputedMatrix {


We could probably just use the same code from layout/display_list_builder.rs?

let theta = 2.0f32 * f32::consts::PI - theta.radians(); Matrix4D::create_rotation(ax, ay, az, Radians::new(theta))

Or just leave a spec link to https://drafts.csswg.org/css-transforms/#Rotate3dDefined

Manishearth · 2016-09-07T03:38:28Z

components/style/properties/helpers/animated_properties.mako.rs

+        let sq = 1.0 / 2.0 * (1.0 - (rad).cos());
+
+        ComputedMatrix {
+            m11: 1.0 - 2.0 * (y * y + z * z) * sq, m12: 2.0 * (x * y * sq - z * sc),


might be best to indent this as:

m11: m12: m13: m14: m21: ...

Manishearth · 2016-09-07T03:50:56Z

components/style/properties/helpers/animated_properties.mako.rs

+
+    #[derive(Clone, Copy, Debug)]
+    #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
+    pub struct DecomposedMatrix {


2DInnerMatrix might be a better name

I tried it but rust is not allowing structs to start with integer value(It's same with MatrixDecomposed2D. I tried 2DMatrixDecomposed but couldn't make it), so I'll change it to InnerMatrix2D, if it's ok.

InnerMatrix2D works

Manishearth · 2016-09-07T05:16:21Z

components/style/properties/helpers/animated_properties.mako.rs

+            // Translate matrix.
+            computed_matrix.m41 = decomposed.translate.0 * decomposed.matrix.m11 +
+                                  decomposed.translate.1 * decomposed.matrix.m21;
+            computed_matrix.m42 = decomposed.translate.0 * decomposed.matrix.m11 +


decomposed_matrix.m12 and m22 here

Manishearth · 2016-09-07T05:32:55Z

Looks correct aside from a few issues. Once those are fixed, could you test this with various matrices (those which are guaranteed to follow the "is 2d matrix" property in the spec) and ensure the animation matches the one firefox does. (Alternatively, just implement the 3D one too so that it's easier to test)

There aren't unit tests in firefox (probably are reftests though). I can maybe generate some test data by logging https://dxr.mozilla.org/mozilla-central/source/layout/style/StyleAnimationValue.cpp#1634, but it's not entirely necessary for now.

Manishearth · 2016-09-07T05:38:54Z

Firefox's layout/style/test/test_transitions_per_property.html has a bunch of testcases under transformTests. But we'll work on those in a future thing, or I might add them directly later.


        Move decompose and recompose functions to impl and implement interpol…

…ations

canova · 2016-09-07T11:38:24Z

Fixed issues except using Matrix4D::create_rotation instead of rotate_to_matrix function. I would prefer avoiding code duplication here but I have to convert Matrix4D to ComputedMatrix afterwards. I need to implement impl From for ComputedMatrix probably. Which one do you prefer, leave it this way or use Matrix4D::create_rotation?

Manishearth · 2016-09-07T14:11:08Z

Yeah, ignore my comment about create_rotation, it's fine the way it is.

Manishearth · 2016-09-08T02:50:46Z

02:57 #servo: < canaltinova> Manishearth: Hi, I'm trying to test the changes. But the firefox test is a bit complicated and I couldn't test it on servo yet :/
02:59 #servo: < canaltinova> Manishearth: I tried to add debug!'s for getting the matrices but these codes never run in my scenarios :/
03:01 #servo: < canaltinova> Manishearth: Are there any tips to test them? :)

For now, don't write unit tests -- just test it manually with simple transitions (https://robots.thoughtbot.com/transitions-and-transforms). Have a div which moves between two matrices or something

Manishearth · 2016-09-08T02:53:19Z

It might be better to first implement the 3D interpolation, so that we can test all cases. I'll test this one for you for now.

canova · 2016-09-08T12:16:11Z

Ok, thanks! So should I implement 3D interpolation in this pull request too or is it better to make it in another pr?

Manishearth · 2016-09-08T14:10:15Z

Same PR is fine. I don't want to merge this unless I'm able to manually test it (or we have unit tests), and it may take a few days for me to get time for that, so you can just append to this PR

canova · 2016-09-08T15:35:17Z

Ok, I will work on interpolation of 3D matrices now on. So I have to determine in interpolate_transform_list function that the matrix is 2D or 3D. Is there a way to determine it?

Manishearth · 2016-09-08T15:38:07Z

https://drafts.csswg.org/css-transforms/#2d-matrix

canova · 2016-09-08T15:42:14Z

Thanks, I'll look at it!

canova · 2016-09-15T15:41:28Z

@bors-servo try

bors-servo · 2016-09-15T15:41:31Z

⌛ Trying commit 3cc3aee with merge 032a364...


        Auto merge of #13188 - canaltinova:matrix, r=<try>

@Manishearth

Implement matrix decomposition and interpolation  Implement 2D and 3D matrix decomposition and interpolation for animated properties. r? @Manishearth ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13188)

bors-servo · 2016-09-15T16:39:46Z

💔 Test failed - mac-rel-wpt

Manishearth · 2016-09-15T16:47:44Z

css build still ongoing

http://build.servo.org/builders/mac-rel-css/builds/3008

notriddle · 2016-09-15T17:55:56Z

The CSS build passed.

Manishearth · 2016-09-16T04:00:12Z

Webkit's 2d recomposition agrees with ours wrt translate.

Additionally, webkit (well, safari) doesn't seem to have this bug

Manishearth · 2016-09-16T04:01:30Z

Overall https://github.com/WebKit/webkit/blob/284f456f1d22f45d2a68cf6d88bafc49a63ee018/Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp is a pretty good bit of code which is close to matching the spec. Or perhaps the spec was written by looking at this code.

Manishearth · 2016-09-16T04:05:52Z

Anyway, I'm good with merging this (once I test it out myself on your testcases), could you squash (just the last two commits is fine). Though squashing isn't necessary here, so I might just merge directly -- let me know what you prefer.

Thanks!

Manishearth · 2016-09-16T07:56:06Z

@bors-servo r+

Everything from your pastebin passes now, except for one.

There's a bit of an jump with

matrix(0.879385, -0.507713, 0.921605, 0.532089, 10, -100) -> matrix3d(-0.648456, 0.744262, -0.159932, 0, 0.574503, 0.340618, -0.744262, 0, -0.49945, -0.574503, -0.648456, 0, 0, 0, 0, 1); // translate(10px, -100px) rotate(320deg) skew(20deg, 10deg) -> rotate3d(1, 2, -1, 192deg)

however it's a 3D transform and Servo has issues with those anyway. It's also 2D->3D which is broken in other browsers. I'd rather wait for the spec to figure this stuff out before fixing this.

I'm not sure if it's even an issue, because the transition keeps flickering so I'm not sure if it's a jump or a flicker.

Thanks for sticking with this! Great work!

bors-servo · 2016-09-16T07:56:07Z

📌 Commit 3cc3aee has been approved by Manishearth


        Add decomposition and interpolation of 3D matrices

Manishearth · 2016-09-16T10:01:20Z

@bors-servo r+

bors-servo · 2016-09-16T10:01:21Z

📌 Commit 63c329b has been approved by Manishearth

bors-servo · 2016-09-16T10:01:23Z

⌛ Testing commit 63c329b with merge ad7befd...


        Auto merge of #13188 - canaltinova:matrix, r=Manishearth

@Manishearth

Implement matrix decomposition and interpolation  Implement 2D and 3D matrix decomposition and interpolation for animated properties. r? @Manishearth ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors  - [ ] There are tests for these changes OR - [X] These changes do not require tests because there are no tests for it and we manually tested it.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13188)

bors-servo · 2016-09-16T11:05:37Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

canova · 2016-09-16T11:23:38Z

🎉 🎉 🎉 I enjoyed it and you helped me a lot during whole process! Thanks for helping me and reviewing it @Manishearth!

canova added 3 commits Sep 5, 2016

Implement decompose_matrix function

32810a9

Implement Interpolate traits for matrix structs

61e2d85

Implement recompose_matrix function

10b631d

highfive assigned Manishearth Sep 6, 2016

highfive added the S-awaiting-review label Sep 6, 2016

canova reviewed Sep 6, 2016
View changes

canova force-pushed the canova:matrix branch from c6ee9fe to 4332243 Sep 6, 2016

canova reviewed Sep 6, 2016
View changes

Manishearth reviewed Sep 7, 2016
View changes

Move decompose and recompose functions to impl and implement interpol…

Loading status checks…

5ee93b2

…ations

canova force-pushed the canova:matrix branch from 4332243 to 5ee93b2 Sep 7, 2016

canova force-pushed the canova:matrix branch from f47987f to 47edb53 Sep 11, 2016

highfive added the S-tests-failed label Sep 15, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-tests-failed labels Sep 16, 2016

Add decomposition and interpolation of 3D matrices

Loading status checks…

63c329b

canova force-pushed the canova:matrix branch from 3cc3aee to 63c329b Sep 16, 2016

highfive added S-awaiting-review and removed S-awaiting-merge labels Sep 16, 2016

highfive removed the S-awaiting-review label Sep 16, 2016

highfive added the S-awaiting-merge label Sep 16, 2016

bors-servo merged commit 63c329b into servo:master Sep 16, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Sep 16, 2016

canova deleted the canova:matrix branch Sep 16, 2016

servo / servo

Join GitHub today

Implement matrix decomposition and interpolation #13188

Implement matrix decomposition and interpolation #13188

Conversation

canova commented Sep 6, 2016 • edited

highfive commented Sep 6, 2016

highfive commented Sep 6, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth Sep 7, 2016 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented Sep 7, 2016 • edited

Manishearth commented Sep 7, 2016

canova commented Sep 7, 2016

Manishearth commented Sep 7, 2016

Manishearth commented Sep 8, 2016

Manishearth commented Sep 8, 2016

canova commented Sep 8, 2016

Manishearth commented Sep 8, 2016

canova commented Sep 8, 2016

Manishearth commented Sep 8, 2016

canova commented Sep 8, 2016

canova commented Sep 15, 2016

bors-servo commented Sep 15, 2016

bors-servo commented Sep 15, 2016

Manishearth commented Sep 15, 2016

notriddle commented Sep 15, 2016

Manishearth commented Sep 16, 2016 • edited

Manishearth commented Sep 16, 2016

Manishearth commented Sep 16, 2016 • edited

Manishearth commented Sep 16, 2016

bors-servo commented Sep 16, 2016

Manishearth commented Sep 16, 2016

bors-servo commented Sep 16, 2016

bors-servo commented Sep 16, 2016

bors-servo commented Sep 16, 2016

canova commented Sep 16, 2016

canova commented Sep 6, 2016 •

edited

Manishearth Sep 7, 2016 •

edited

Manishearth commented Sep 7, 2016 •

edited

Manishearth commented Sep 16, 2016 •

edited

Manishearth commented Sep 16, 2016 •

edited