Use SkMatrix44 for the underlying implementation of ui::Transform

ui/gfx/transform.cc

Index: ui/gfx/transform.cc
diff --git a/ui/gfx/transform.cc b/ui/gfx/transform.cc
index 03e2873d27c5ca1e795c3d4c5049854bf87c0c42..629624f3ec775ec517406781472fc70ec4fffdaa 100644
--- a/ui/gfx/transform.cc
+++ b/ui/gfx/transform.cc
@@ -10,105 +10,187 @@
 #include "ui/gfx/rect.h"
 #include "ui/gfx/skia_util.h"
 
+namespace {
+  // Should be in a standard header
+  static inline int SymmetricRound(float x) {

[sky, 2011/06/09 17:20:29]

don't indent this.

+    return static_cast<int>(
+      x > 0.0f
+      ? std::floor(x + 0.5f)
+      : std::ceil(x - 0.5f));
+  }
+}
+
 namespace ui {
 
-Transform::Transform() {
+Transform::Transform()
+  : hasInverse_(false) {
   matrix_.reset();
+  inverse_.reset();
 }
 
 Transform::~Transform() {}
 
 void Transform::SetRotate(float degree) {
-  matrix_.setRotate(SkFloatToScalar(degree));

[reed1, 2011/06/09 19:22:47]

I think this can just be

matrix.setRotateDegreesAbout(degree, 0, 0, 1);
hasInverse_ = false;

SkMatrix44 only works if SkScalar==float, so there are no fixed-point
considerations. Plus, SkMScalar will either be float or double, either of which
will correctly handle pass a float (degree) and literals (0 and 1)

+  matrix_.setRotateDegreesAbout(
+    static_cast<SkMScalar>(0.0),
+    static_cast<SkMScalar>(0.0),
+    static_cast<SkMScalar>(1.0),
+    SkFloatToScalar(degree));
+  hasInverse_ = false;
 }
 
 void Transform::SetScaleX(float x) {
-  matrix_.setScaleX(SkFloatToScalar(x));

[reed1, 2011/06/09 19:22:47]

For this and SetScaleY,Z, see my no-need-to-cast comments from SetRotate()

+  matrix_.setScale(
+    SkFloatToScalar(x),
+    static_cast<SkMScalar>(1.0),
+    static_cast<SkMScalar>(1.0));

[wjmaclean, 2011/06/09 17:20:58]

Should this be altering other scales?

+  hasInverse_ = false;
 }
 
 void Transform::SetScaleY(float y) {
-  matrix_.setScaleY(SkFloatToScalar(y));
+  matrix_.setScale(
+    static_cast<SkMScalar>(1.0),
+    SkFloatToScalar(y),
+    static_cast<SkMScalar>(1.0));
+  hasInverse_ = false;
 }
 
 void Transform::SetScale(float x, float y) {
-  matrix_.setScale(SkFloatToScalar(x), SkFloatToScalar(y));
+  matrix_.setScale(
+    SkFloatToScalar(x),
+    SkFloatToScalar(y),
+    static_cast<SkMScalar>(1.0));
+  hasInverse_ = false;
 }
 
 void Transform::SetTranslateX(float x) {
-  matrix_.setTranslateX(SkFloatToScalar(x));
+  matrix_.setTranslate(
+    SkFloatToScalar(x),
+    static_cast<SkMScalar>(0.0),
+    static_cast<SkMScalar>(0.0));

[wjmaclean, 2011/06/09 17:20:58]

Again, should this alter existing Ty components?

+  hasInverse_ = false;
 }
 
 void Transform::SetTranslateY(float y) {
-  matrix_.setTranslateY(SkFloatToScalar(y));
+  matrix_.setTranslate(
+    static_cast<SkMScalar>(0.0),
+    SkFloatToScalar(y),
+    static_cast<SkMScalar>(0.0));
+  hasInverse_ = false;
 }
 
 void Transform::SetTranslate(float x, float y) {
-  matrix_.setTranslate(SkFloatToScalar(x), SkFloatToScalar(y));
+  matrix_.setTranslate(
+    SkFloatToScalar(x),
+    SkFloatToScalar(y),
+    static_cast<SkMScalar>(0.0));
+  hasInverse_ = false;
 }
 
 void Transform::ConcatRotate(float degree) {
-  matrix_.postRotate(SkFloatToScalar(degree));
+  SkMatrix44 rot;
+  rot.reset();
+  rot.setRotateDegreesAbout(
+    static_cast<SkMScalar>(0.0),
+    static_cast<SkMScalar>(0.0),
+    static_cast<SkMScalar>(1.0),
+    SkFloatToScalar(degree));
+  matrix_.postConcat(rot);
+  hasInverse_ = false;
 }
 
 void Transform::ConcatScale(float x, float y) {
-  matrix_.postScale(SkFloatToScalar(x), SkFloatToScalar(y));
+  SkMatrix44 scale;
+  scale.reset();
+  scale.setScale(
+    SkFloatToScalar(x),
+    SkFloatToScalar(y),
+    static_cast<SkMScalar>(1.0));
+  matrix_.postConcat(scale);
+  hasInverse_ = false;
 }
 
 void Transform::ConcatTranslate(float x, float y) {
-  matrix_.postTranslate(SkFloatToScalar(x), SkFloatToScalar(y));
-}
-
-bool Transform::PreconcatTransform(const Transform& transform) {
-  return matrix_.setConcat(matrix_, transform.matrix_);
+  SkMatrix44 translate;

[reed1, 2011/06/09 19:22:47]

setTranslate() by definition always overwrites the entire matrix, so there is no
need to call reset() first. The same is true for all of the setFoo() methods
(setScale, setRotate... , etc).

The methods beginning with pre... or post... by definition are modifying an
existing matrix, so their previous contents do matter.

All of that said, SkMatrix44 (unlike SkMatrix) is always initialized at
constructor time. Hence in this example...

SkMatrix44 matrix;
matrix.reset();

The reset() call is redundant.

+  translate.reset();
+  translate.setTranslate(
+    SkFloatToScalar(x),
+    SkFloatToScalar(y),
+    static_cast<SkMScalar>(0.0));
+  matrix_.postConcat(translate);
+  hasInverse_ = false;
+}
+
+void Transform::PreconcatTransform(const Transform& transform) {
+  if (!transform.matrix_.isIdentity()) {
+    matrix_.preConcat(transform.matrix_);
+    hasInverse_ = false;
+  }
 }
 
-bool Transform::ConcatTransform(const Transform& transform) {
-  return matrix_.setConcat(transform.matrix_, matrix_);
+void Transform::ConcatTransform(const Transform& transform) {
+  if (!transform.matrix_.isIdentity()) {
+    matrix_.postConcat(transform.matrix_);
+    hasInverse_ = false;
+  }
 }
 
 bool Transform::HasChange() const {
   return !matrix_.isIdentity();
 }
 
-bool Transform::TransformPoint(gfx::Point* point) {
-  SkPoint skp;
-  matrix_.mapXY(SkIntToScalar(point->x()), SkIntToScalar(point->y()), &skp);
-  point->SetPoint(static_cast<int>(std::floor(skp.fX)),
-                  static_cast<int>(std::floor(skp.fY)));
-  return true;
+void Transform::TransformPoint(gfx::Point* point) {
+  TransformPointInternal(matrix_, point);
 }
 
 bool Transform::TransformPointReverse(gfx::Point* point) {
-  SkMatrix inverse;
   // TODO(sad): Try to avoid trying to invert the matrix.
-  if (matrix_.invert(&inverse)) {
-    SkPoint skp;
-    inverse.mapXY(SkIntToScalar(point->x()), SkIntToScalar(point->y()), &skp);
-    point->SetPoint(static_cast<int>(std::floor(skp.fX)),
-                    static_cast<int>(std::floor(skp.fY)));
-    return true;
-  }
-  return false;
+  if (!ComputeInverse())
+    return false;
+
+  TransformPointInternal(inverse_, point);
+  return true;
 }
 
 bool Transform::TransformRect(gfx::Rect* rect) {
   SkRect src = gfx::RectToSkRect(*rect);
-  if (!matrix_.mapRect(&src))
+  SkMatrix matrix = matrix_;
+  if (!matrix.mapRect(&src))
     return false;
   *rect = gfx::SkRectToRect(src);
   return true;
 }
 
 bool Transform::TransformRectReverse(gfx::Rect* rect) {
-  SkMatrix inverse;
-  if (!matrix_.invert(&inverse))
+  if (!ComputeInverse())
     return false;
-
+  SkMatrix matrix = inverse_;
   SkRect src = gfx::RectToSkRect(*rect);
-  if (!inverse.mapRect(&src))
+  if (!matrix.mapRect(&src))
     return false;
   *rect = gfx::SkRectToRect(src);
   return true;
 }
 
+void Transform::TransformPointInternal(const SkMatrix44& xform,
+                                       gfx::Point* point) {
+  SkScalar p[4] = {
+    SkIntToScalar(point->x()),
+    SkIntToScalar(point->y()),
+    static_cast<SkScalar>(0),
+    static_cast<SkScalar>(1)
+  };
+
+  xform.map(p);
+  point->SetPoint(SymmetricRound(p[0]),

[sky, 2011/06/09 17:20:29]

Why do we want SymmetricRound and not floor?

+                  SymmetricRound(p[1]));
+}
+
+bool Transform::ComputeInverse() {
+  if (!hasInverse_) {

[sky, 2011/06/09 17:20:29]

Why do we bother storing the inverse?

+    hasInverse_ = matrix_.invert(&inverse_);
+  }
+  return hasInverse_;
+}
+
 }  // namespace ui