Bug 1458715: Fix perspective interpolation. r=hiro
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 03 May 2018 10:15:15 +0200
changeset 416848 5d477ed8d62de3ff678519f79fc3ac9f23bb91cc
parent 416847 2d910b41bff477568a84ab3c65656cb744b2ae84
child 416849 d67ad5505bc5ebb2e0f4ecc0864b6021adc2473f
push id102880
push useraciure@mozilla.com
push dateThu, 03 May 2018 21:56:36 +0000
treeherdermozilla-inbound@1557686206e3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershiro
bugs1458715
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1458715: Fix perspective interpolation. r=hiro It's not sound to insert random matrices in random positions in the transform operation list. I cannot make any sense of what the old code was trying to do. MozReview-Commit-ID: 5BtCiueEPlR
servo/components/style/properties/helpers/animated_properties.mako.rs
servo/components/style/values/computed/transform.rs
--- a/servo/components/style/properties/helpers/animated_properties.mako.rs
+++ b/servo/components/style/properties/helpers/animated_properties.mako.rs
@@ -1286,26 +1286,18 @@ impl Animate for ComputedTransformOperat
                 Ok(TransformOperation::Rotate(
                     fa.animate(&ta, procedure)?
                 ))
             },
             (
                 &TransformOperation::Perspective(ref fd),
                 &TransformOperation::Perspective(ref td),
             ) => {
-                let mut fd_matrix = Matrix3D::identity();
-                let mut td_matrix = Matrix3D::identity();
-                if fd.px() > 0. {
-                    fd_matrix.m34 = -1. / fd.px();
-                }
-                if td.px() > 0. {
-                    td_matrix.m34 = -1. / td.px();
-                }
-                Ok(TransformOperation::Matrix3D(
-                    fd_matrix.animate(&td_matrix, procedure)?,
+                Ok(TransformOperation::Perspective(
+                    fd.animate(td, procedure)?
                 ))
             },
             _ if self.is_translate() && other.is_translate() => {
                 self.to_translate_3d().animate(&other.to_translate_3d(), procedure)
             }
             _ if self.is_scale() && other.is_scale() => {
                 self.to_scale_3d().animate(&other.to_scale_3d(), procedure)
             }
@@ -2635,34 +2627,27 @@ impl ComputeSquaredDistance for Computed
                 &TransformOperation::Rotate(ta),
             ) => {
                 fa.compute_squared_distance(&ta)
             }
             (
                 &TransformOperation::Perspective(ref fd),
                 &TransformOperation::Perspective(ref td),
             ) => {
-                let mut fd_matrix = Matrix3D::identity();
-                let mut td_matrix = Matrix3D::identity();
-                if fd.px() > 0. {
-                    fd_matrix.m34 = -1. / fd.px();
-                }
-
-                if td.px() > 0. {
-                    td_matrix.m34 = -1. / td.px();
-                }
-                fd_matrix.compute_squared_distance(&td_matrix)
+                fd.compute_squared_distance(td)
             }
             (
                 &TransformOperation::Perspective(ref p),
                 &TransformOperation::Matrix3D(ref m),
             ) | (
                 &TransformOperation::Matrix3D(ref m),
                 &TransformOperation::Perspective(ref p),
             ) => {
+                // FIXME(emilio): Is this right? Why interpolating this with
+                // Perspective but not with anything else?
                 let mut p_matrix = Matrix3D::identity();
                 if p.px() > 0. {
                     p_matrix.m34 = -1. / p.px();
                 }
                 p_matrix.compute_squared_distance(&m)
             }
             // Gecko cross-interpolates amongst all translate and all scale
             // functions (See ToPrimitive in layout/style/StyleAnimationValue.cpp)
--- a/servo/components/style/values/computed/transform.rs
+++ b/servo/components/style/values/computed/transform.rs
@@ -246,21 +246,21 @@ impl ToAnimatedZero for TransformOperati
                 Ok(generic::TransformOperation::RotateY(Angle::zero()))
             },
             generic::TransformOperation::RotateZ(_) => {
                 Ok(generic::TransformOperation::RotateZ(Angle::zero()))
             },
             generic::TransformOperation::Rotate(_) => {
                 Ok(generic::TransformOperation::Rotate(Angle::zero()))
             },
-            generic::TransformOperation::Perspective(..) |
+            generic::TransformOperation::Perspective(ref l) => {
+                Ok(generic::TransformOperation::Perspective(l.to_animated_zero()?))
+            },
             generic::TransformOperation::AccumulateMatrix { .. } |
             generic::TransformOperation::InterpolateMatrix { .. } => {
-                // Perspective: We convert a perspective function into an equivalent
-                //     ComputedMatrix, and then decompose/interpolate/recompose these matrices.
                 // AccumulateMatrix/InterpolateMatrix: We do interpolation on
                 //     AccumulateMatrix/InterpolateMatrix by reading it as a ComputedMatrix
                 //     (with layout information), and then do matrix interpolation.
                 //
                 // Therefore, we use an identity matrix to represent the identity transform list.
                 // http://dev.w3.org/csswg/css-transforms/#identity-transform-function
                 Ok(generic::TransformOperation::Matrix3D(Matrix3D::identity()))
             },