stylo: Bug 1362896 - Implement ComputeSquaredDistance for TransformList #18086

BorisChiou · 2017-08-15T08:15:32Z

We implement ComputeSquaredDistance for TransformList.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Bug 1362896.
These changes do not require tests because Gecko has related tests.

This change is

highfive · 2017-08-15T08:15:36Z

Heads up! This PR modifies the following files:

@bholley: components/style/values/distance.rs, components/style/values/computed/length.rs, components/style/properties/helpers/animated_properties.mako.rs
@canaltinova: components/style/values/distance.rs, components/style/values/computed/length.rs, components/style/properties/helpers/animated_properties.mako.rs
@emilio: components/style/values/distance.rs, components/style/values/computed/length.rs, components/style/properties/helpers/animated_properties.mako.rs

highfive · 2017-08-15T08:15:37Z

Warning

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

BorisChiou · 2017-08-15T08:15:44Z

r?@nox

BorisChiou · 2017-08-15T08:16:07Z

Most parts are also reviewed by @birtles.

BorisChiou · 2017-08-15T08:38:58Z

@bors-servo try

bors-servo · 2017-08-15T08:39:03Z

⌛ Trying commit 7285b51 with merge 36b01f4...


        Auto merge of #18086 - BorisChiou:stylo/transform/distance, r=<try>

stylo: Bug 1362896 - Implement ComputeSquaredDistance for TransformList We implement ComputeSquaredDistance for TransformList. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix [Bug 1362896](https://bugzilla.mozilla.org/show_bug.cgi?id=1362896). - [X] These changes do not require tests because Gecko has related tests.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18086)

BorisChiou · 2017-08-15T09:40:03Z

gecko try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4098866107b84b5367df50749a698e73cac2d4c

bors-servo · 2017-08-15T10:07:38Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
State: approved= try=True

BorisChiou · 2017-08-16T03:24:38Z

Hi @emilio, it seems nox is on PTO in these days? Could you please take a look at these patches? Thanks.

emilio · 2017-08-16T05:55:22Z

@nox should be back, yesterday was holiday in France. I can take a look if he can't, though.

nox · 2017-08-16T12:17:09Z

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

+    fn compute_squared_distance(&self, other: &Self) -> Result<SquaredDistance, ()> {
+        // If possible, we should convert the InnerMatrix2D into types with physical meaning.
+        // Therefore, we compute the squared distance from each matrix item, and this makes the
+        // result different from that in Gecko if we have skew/shear factor in the ComputedMatrix.


Shear is used in Gecko. I think I should remove this word to avoid confusing people.

nox · 2017-08-16T12:17:09Z

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

+impl ComputeSquaredDistance for InnerMatrix2D {
+    #[inline]
+    fn compute_squared_distance(&self, other: &Self) -> Result<SquaredDistance, ()> {
+        // If possible, we should convert the InnerMatrix2D into types with physical meaning.


Is that something that should be done? If yes, add "FIXME: " before the comment.

Thanks. Yes, I will add "FIXME:"

nox · 2017-08-16T12:17:09Z

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

@@ -1563,6 +1563,19 @@ impl Animatable for InnerMatrix2D {
    }
 }

+impl ComputeSquaredDistance for InnerMatrix2D {


This implementation can be derived.

nox · 2017-08-16T12:17:09Z

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

@@ -1572,6 +1585,14 @@ impl Animatable for Translate2D {
    }
 }

+impl ComputeSquaredDistance for Translate2D {


This implementation can be derived.

nox · 2017-08-16T12:17:09Z

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

@@ -1581,6 +1602,14 @@ impl Animatable for Scale2D {
    }
 }

+impl ComputeSquaredDistance for Scale2D {


This implementation can be derived.

nox · 2017-08-16T12:17:09Z

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

@@ -2104,6 +2271,18 @@ impl Animatable for MatrixDecomposed3D {
    }
 }

+impl ComputeSquaredDistance for MatrixDecomposed3D {


This implementation can be derived if you implement that trait for Quaternion, which you should instead of having an intrinsic distance method.

I see. This sounds much better.

nox · 2017-08-16T12:17:09Z

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

-        // FIXME: This should be implemented.
-        Err(())
+    fn compute_squared_distance(&self, other: &Self) -> Result<SquaredDistance, ()> {
+        match (&self.0, &other.0) {


Nit: you could otherwise match on (self.0.as_ref(), other.0.as_ref()).

nox · 2017-08-16T12:17:09Z

components/style/values/computed/length.rs

@@ -78,7 +78,8 @@ impl ComputeSquaredDistance for CalcLengthOrPercentage {
        // FIXME(nox): This looks incorrect to me, to add a distance between lengths
        // with a distance between percentages.
        Ok(
-            self.unclamped_length().compute_squared_distance(&other.unclamped_length())? +
+            self.unclamped_length().to_f64_px().compute_squared_distance(
+                &other.unclamped_length().to_f64_px())? +


Why is it correct at all to mix lengths and percentages? Cf. the FIXME I left just above that change.

Actually, this doesn't make any sense. However, while computing the distance or doing interpolation, we only have computed values (i.e. we shouldn't use used values), so this is a temporary? way to define its distance, and the test cases in Gecko use this way to produce the expected values.
We should keep the "FIXME:" because I hope one day we could use a better way to define the distance.

nox · 2017-08-16T12:17:09Z

components/style/values/distance.rs

+    fn add_assign(&mut self, rhs: Self) {
+        *self = *self + rhs;
+    }
+}


This shouldn't be needed.

nox · 2017-08-16T12:17:09Z

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

+                                            to_list: &[TransformOperation])
+                                            -> Result<SquaredDistance, ()> {
+    let mut squared_distance = SquaredDistance::Value(0.);
+    for (from, to) in from_list.iter().zip(to_list.iter()) {


You don't need a for loop at all here, just map on the iterator and sum it at the end.

nox · 2017-08-16T12:17:33Z

"Tweak CalcLengthOrPerecentage to use pixel value." fix that typo.


        Tweak CalcLengthOrPercentage to use pixel value.

We compute the distance for eCSSUnit_Calc by pixel value in Gecko, so let's follow the same rules.


        Use f64 for Quaternion.

The unit of gfxQuaternion in Gecko is gfxFloat, which is "double", so it's better to use f64 to match the precision of Gecko.


        Implement ComputeSquaredDistance for TransformList.


        Implement ComputeSquaredDistance for Matrix and Perspective.

BorisChiou · 2017-08-17T07:14:04Z

r? @nox again

gecko try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd3bcf48ad64054d6784bcbf1875ea47f8318ab0

nox · 2017-08-17T12:09:08Z

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

@@ -2013,6 +2162,15 @@ impl Animatable for Skew {
    }
 }

+impl ComputeSquaredDistance for Skew {


nox · 2017-08-17T12:09:08Z

components/style/values/computed/length.rs

@@ -78,7 +78,8 @@ impl ComputeSquaredDistance for CalcLengthOrPercentage {
        // FIXME(nox): This looks incorrect to me, to add a distance between lengths
        // with a distance between percentages.
        Ok(
-            self.unclamped_length().compute_squared_distance(&other.unclamped_length())? +
+            self.unclamped_length().to_f64_px().compute_squared_distance(
+                &other.unclamped_length().to_f64_px())? +


nox · 2017-08-17T12:09:36Z

r=me if the geckotry failures are unrelated.

bors-servo · 2017-08-18T05:52:46Z

📌 Commit e8fad23 has been approved by nox,

BorisChiou · 2017-08-18T05:53:02Z

@bors-servo r=nox,birtles

Thanks, nox. Those geckotry failures are intermittents.

bors-servo · 2017-08-18T05:53:02Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: #18139

bors-servo · 2017-08-18T05:53:03Z

📌 Commit e8fad23 has been approved by nox,birtles

bors-servo · 2017-08-18T05:53:28Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: #18139

bors-servo · 2017-08-18T05:53:29Z

📌 Commit e8fad23 has been approved by nox,

bors-servo · 2017-08-18T05:53:34Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: #18139

bors-servo · 2017-08-18T05:53:35Z

📌 Commit e8fad23 has been approved by nox,birtles

bors-servo · 2017-08-18T07:24:35Z

⌛ Testing commit e8fad23 with merge 494dcd7...


        Auto merge of #18086 - BorisChiou:stylo/transform/distance, r=nox,bir…

…tles stylo: Bug 1362896 - Implement ComputeSquaredDistance for TransformList We implement ComputeSquaredDistance for TransformList. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix [Bug 1362896](https://bugzilla.mozilla.org/show_bug.cgi?id=1362896). - [X] These changes do not require tests because Gecko has related tests.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18086)

bors-servo · 2017-08-18T08:28:45Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: nox,birtles
Pushing 494dcd7 to master...

highfive assigned emilio Aug 15, 2017

highfive added the S-awaiting-review label Aug 15, 2017

highfive assigned nox and unassigned emilio Aug 15, 2017

nox requested changes Aug 16, 2017

View changes

BorisChiou added 4 commits Aug 9, 2017

Tweak CalcLengthOrPercentage to use pixel value.

a5433ad

We compute the distance for eCSSUnit_Calc by pixel value in Gecko, so let's follow the same rules.

Use f64 for Quaternion.

03e1794

The unit of gfxQuaternion in Gecko is gfxFloat, which is "double", so it's better to use f64 to match the precision of Gecko.

Implement ComputeSquaredDistance for TransformList.

3a5cbfb

Implement ComputeSquaredDistance for Matrix and Perspective.

Loading status checks…

e8fad23

BorisChiou force-pushed the BorisChiou:stylo/transform/distance branch from 7285b51 to e8fad23 Aug 17, 2017

nox approved these changes Aug 17, 2017

View changes

nox removed the S-awaiting-review label Aug 17, 2017

highfive added the S-awaiting-merge label Aug 18, 2017

bors-servo merged commit e8fad23 into servo:master Aug 18, 2017
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 Aug 18, 2017

servo / servo

Join GitHub today

stylo: Bug 1362896 - Implement ComputeSquaredDistance for TransformList #18086

stylo: Bug 1362896 - Implement ComputeSquaredDistance for TransformList #18086

Conversation

BorisChiou commented Aug 15, 2017 • edited by SimonSapin

highfive commented Aug 15, 2017

highfive commented Aug 15, 2017

BorisChiou commented Aug 15, 2017

BorisChiou commented Aug 15, 2017

BorisChiou commented Aug 15, 2017

bors-servo commented Aug 15, 2017

BorisChiou commented Aug 15, 2017

bors-servo commented Aug 15, 2017

BorisChiou commented Aug 16, 2017

emilio commented Aug 16, 2017

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.

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.

nox commented Aug 16, 2017

BorisChiou commented Aug 17, 2017

This comment has been minimized.

This comment has been minimized.

nox commented Aug 17, 2017

bors-servo commented Aug 18, 2017

BorisChiou commented Aug 18, 2017 • edited

bors-servo commented Aug 18, 2017

bors-servo commented Aug 18, 2017

bors-servo commented Aug 18, 2017

bors-servo commented Aug 18, 2017

bors-servo commented Aug 18, 2017

bors-servo commented Aug 18, 2017

bors-servo commented Aug 18, 2017

bors-servo commented Aug 18, 2017

BorisChiou commented Aug 15, 2017 •

edited by SimonSapin

BorisChiou commented Aug 18, 2017 •

edited