gfx::Transform API clean-up

ui/gfx/interpolated_transform.cc

Index: ui/gfx/interpolated_transform.cc
diff --git a/ui/gfx/interpolated_transform.cc b/ui/gfx/interpolated_transform.cc
index 111f6217962bedb3e1cbb4319358b645d7a4bc5d..b3b151958d401fa7d891d47b58574cf64f8c3006 100644
--- a/ui/gfx/interpolated_transform.cc
+++ b/ui/gfx/interpolated_transform.cc
@@ -104,62 +104,6 @@ void InterpolatedTransform::SetChild(InterpolatedTransform* child) {
   child_.reset(child);
 }
 
-bool InterpolatedTransform::FactorTRS(const gfx::Transform& transform,
-                                      gfx::Point* translation,
-                                      float* rotation,
-                                      gfx::Point3F* scale) {
-  const SkMatrix44& m = transform.matrix();
-  double m00 = SkMScalarToDouble(m.get(0, 0));
-  double m01 = SkMScalarToDouble(m.get(0, 1));
-  double m10 = SkMScalarToDouble(m.get(1, 0));
-  double m11 = SkMScalarToDouble(m.get(1, 1));
-
-  // A factorable 2D TRS matrix must be of the form:
-  //    [ sx*cos_theta -(sy*sin_theta) 0 tx ]
-  //    [ sx*sin_theta   sy*cos_theta  0 ty ]
-  //    [ 0              0             1 0  ]
-  //    [ 0              0             0 1  ]
-  if (!IsApproximatelyZero(SkMScalarToDouble(m.get(0, 2))) ||
-      !IsApproximatelyZero(SkMScalarToDouble(m.get(1, 2))) ||
-      !IsApproximatelyZero(SkMScalarToDouble(m.get(2, 0))) ||
-      !IsApproximatelyZero(SkMScalarToDouble(m.get(2, 1))) ||
-      !IsApproximatelyZero(SkMScalarToDouble(m.get(2, 2)) - 1) ||
-      !IsApproximatelyZero(SkMScalarToDouble(m.get(2, 3))) ||
-      !IsApproximatelyZero(SkMScalarToDouble(m.get(3, 0))) ||
-      !IsApproximatelyZero(SkMScalarToDouble(m.get(3, 1))) ||
-      !IsApproximatelyZero(SkMScalarToDouble(m.get(3, 2))) ||
-      !IsApproximatelyZero(SkMScalarToDouble(m.get(3, 3)) - 1)) {
-    return false;
-  }
-
-  double scale_x = std::sqrt(m00 * m00 + m10 * m10);
-  double scale_y = std::sqrt(m01 * m01 + m11 * m11);
-
-  if (scale_x == 0 || scale_y == 0)
-    return false;
-
-  double cos_theta = m00 / scale_x;
-  double sin_theta = m10 / scale_x;
-
-  if (!IsApproximatelyZero(cos_theta - (m11 / scale_y)) ||
-      !IsApproximatelyZero(sin_theta + (m01 / scale_y)) ||
-      !IsApproximatelyZero(cos_theta*cos_theta + sin_theta*sin_theta - 1.0f))
-    return false;
-
-  double radians = std::atan2(sin_theta, cos_theta);
-
-  if (translation)
-    *translation = gfx::Point(SkMScalarToFloat(m.get(0, 3)),
-                              SkMScalarToFloat(m.get(1, 3)));
-  if (rotation)
-    *rotation = static_cast<float>(radians * 180.0 / M_PI);
-  if (scale)
-    *scale = gfx::Point3F(static_cast<float>(scale_x),
-                          static_cast<float>(scale_y),
-                          1.0f);
-  return true;
-}
-
 inline float InterpolatedTransform::ValueBetween(float time,
                                                  float start_value,
                                                  float end_value) const {
@@ -209,7 +153,7 @@ InterpolatedRotation::~InterpolatedRotation() {}
 gfx::Transform InterpolatedRotation::InterpolateButDoNotCompose(float t) const {
   gfx::Transform result;
   float interpolated_degrees = ValueBetween(t, start_degrees_, end_degrees_);
-  result.SetRotate(interpolated_degrees);
+  result.Rotate(interpolated_degrees);

[danakj, 2012/11/16 19:46:12]

I didn't like this change with Set at first, but seeing the implementation of
the Rotate() etc methods now, I think this is pretty cool. I am hoping that when
we drop this into calcDrawEtc, we will get a drop in our multiply times, and
increase our frame rate just like that.

   if (t == 0.0f || t == 1.0f)
     MassageRotationIfMultipleOfNinetyDegrees(&result, interpolated_degrees);
   return result;
@@ -220,7 +164,7 @@ gfx::Transform InterpolatedRotation::InterpolateButDoNotCompose(float t) const {
 //
 
 InterpolatedAxisAngleRotation::InterpolatedAxisAngleRotation(
-    gfx::Point3F axis,
+    gfx::Vector3dF axis,

[danakj, 2012/11/16 19:46:12]

const&

[Ian Vollick, 2012/11/16 21:51:24]

Done, here and in the other spots.

     float start_degrees,
     float end_degrees)
     : InterpolatedTransform(),
@@ -230,7 +174,7 @@ InterpolatedAxisAngleRotation::InterpolatedAxisAngleRotation(
 }
 
 InterpolatedAxisAngleRotation::InterpolatedAxisAngleRotation(
-    gfx::Point3F axis,
+    gfx::Vector3dF axis,

[danakj, 2012/11/16 19:46:12]

const&

     float start_degrees,
     float end_degrees,
     float start_time,
@@ -246,7 +190,7 @@ InterpolatedAxisAngleRotation::~InterpolatedAxisAngleRotation() {}
 gfx::Transform
 InterpolatedAxisAngleRotation::InterpolateButDoNotCompose(float t) const {
   gfx::Transform result;
-  result.SetRotateAbout(axis_, ValueBetween(t, start_degrees_, end_degrees_));
+  result.RotateAbout(axis_, ValueBetween(t, start_degrees_, end_degrees_));
   return result;
 }
 
@@ -290,7 +234,7 @@ gfx::Transform InterpolatedScale::InterpolateButDoNotCompose(float t) const {
   float scale_x = ValueBetween(t, start_scale_.x(), end_scale_.x());
   float scale_y = ValueBetween(t, start_scale_.y(), end_scale_.y());
   // TODO(vollick) 3d xforms.
-  result.SetScale(scale_x, scale_y);
+  result.Scale(scale_x, scale_y);
   return result;
 }
 
@@ -320,7 +264,7 @@ gfx::Transform
 InterpolatedTranslation::InterpolateButDoNotCompose(float t) const {
   gfx::Transform result;
   // TODO(vollick) 3d xforms.
-  result.SetTranslate(ValueBetween(t, start_pos_.x(), end_pos_.x()),
+  result.Translate(ValueBetween(t, start_pos_.x(), end_pos_.x()),
                       ValueBetween(t, start_pos_.y(), end_pos_.y()));
   return result;
 }
@@ -376,8 +320,8 @@ void InterpolatedTransformAboutPivot::Init(const gfx::Point& pivot,
                                            InterpolatedTransform* xform) {
   gfx::Transform to_pivot;
   gfx::Transform from_pivot;
-  to_pivot.SetTranslate(-pivot.x(), -pivot.y());
-  from_pivot.SetTranslate(pivot.x(), pivot.y());
+  to_pivot.Translate(-pivot.x(), -pivot.y());
+  from_pivot.Translate(pivot.x(), pivot.y());
 
   scoped_ptr<InterpolatedTransform> pre_transform(
     new InterpolatedConstantTransform(to_pivot));
@@ -389,14 +333,14 @@ void InterpolatedTransformAboutPivot::Init(const gfx::Point& pivot,
   transform_.reset(pre_transform.release());
 }
 
-InterpolatedTRSTransform::InterpolatedTRSTransform(
+InterpolatedMatrixTransform::InterpolatedMatrixTransform(
     const gfx::Transform& start_transform,
     const gfx::Transform& end_transform)
     : InterpolatedTransform() {
   Init(start_transform, end_transform);
 }
 
-InterpolatedTRSTransform::InterpolatedTRSTransform(
+InterpolatedMatrixTransform::InterpolatedMatrixTransform(
     const gfx::Transform& start_transform,
     const gfx::Transform& end_transform,
     float start_time,
@@ -405,47 +349,25 @@ InterpolatedTRSTransform::InterpolatedTRSTransform(
   Init(start_transform, end_transform);
 }
 
-InterpolatedTRSTransform::~InterpolatedTRSTransform() {}
+InterpolatedMatrixTransform::~InterpolatedMatrixTransform() {}
 
 gfx::Transform
-InterpolatedTRSTransform::InterpolateButDoNotCompose(float t) const {
-  if (transform_.get()) {
-    return transform_->Interpolate(t);
-  }
-  return gfx::Transform();
-}
-
-void InterpolatedTRSTransform::Init(const gfx::Transform& start_transform,
-                                    const gfx::Transform& end_transform) {
-  gfx::Point start_translation, end_translation;
-  gfx::Point3F start_scale, end_scale;
-  float start_degrees, end_degrees;
-  if (FactorTRS(start_transform,
-                &start_translation,
-                &start_degrees,
-                &start_scale) &&
-      FactorTRS(end_transform,
-                &end_translation,
-                &end_degrees,
-                &end_scale)) {
-    scoped_ptr<InterpolatedTranslation> translation(
-        new InterpolatedTranslation(start_translation, end_translation,
-                                    start_time(), end_time()));
-
-    scoped_ptr<InterpolatedScale> scale(
-        new InterpolatedScale(start_scale, end_scale,
-                              start_time(), end_time()));
-
-    scoped_ptr<InterpolatedRotation> rotation(
-        new InterpolatedRotation(start_degrees, end_degrees,
-                                 start_time(), end_time()));
-
-    rotation->SetChild(translation.release());
-    scale->SetChild(rotation.release());
-    transform_.reset(scale.release());
-  } else {
-    transform_.reset(new InterpolatedConstantTransform(end_transform));
-  }
+InterpolatedMatrixTransform::InterpolateButDoNotCompose(float t) const {

[danakj, 2012/11/16 19:46:12]

Shawn can you double check the mathy bits here? I have trouble verifying this
stuff for correctness.

[shawnsingh, 2012/11/16 20:51:25]

It seems like the implementation of this function should be simply called
GetInterpolatedTransform(t).

In particular the "but do not compose" part seems like a misnomer?  The
implementation seems to be taking two decomposed transforms, blending them, and
then "re-composing" the blended decomposition into a transform, right?  so "but
do not compose" seems wrong as I understand it...

[Ian Vollick, 2012/11/16 21:51:24]

This class is analogous to WebTransformOperations. It's a string of things that
can each be interpolated individually, and ultimately produce a matrix when
these interpolated bits are composed/multiplied.

Very soon, this class going to disappear. It will be completely obsolete when
WebTransformOperations has GTFO'd. So I don't think it deserves much attention.
That said, the meaning of the name is
Interpolate-This-Guy-But-Do-Not-Combine-The-Result-With-The-Next-Guy. This
allows ::Interpolate(t) to be implemented as this->InterpolateButDoNotCompose(t)
* child_->Interpolate(t).

I think the confusion comes from the fact that the word compose has been used
here, which now has loaded meaning given the Decompose work in transform_util.
This is change in the verbage I'd been using. Previously, I'd used the word
'factor' rather than 'decompose', so I think the situation was less confusing
then. But no sense renaming things now since this class is going to get deleted
soon anyhow.

+  gfx::DecomposedTransform blended;
+  bool success = gfx::BlendDecomposedTransforms(&blended,
+                                                end_decomp_,
+                                                start_decomp_,
+                                                t);
+  DCHECK(success);
+  return gfx::ComposeTransform(blended);
+}
+
+void InterpolatedMatrixTransform::Init(const gfx::Transform& start_transform,
+                                       const gfx::Transform& end_transform) {
+  bool success = gfx::DecomposeTransform(&start_decomp_, start_transform);
+  DCHECK(success);
+  success = gfx::DecomposeTransform(&end_decomp_, end_transform);
+  DCHECK(success);
 }
 
 } // namespace ui