SkMatrix44::preserves2dAxisAlignment()

src/utils/SkMatrix44.cpp

Index: src/utils/SkMatrix44.cpp
diff --git a/src/utils/SkMatrix44.cpp b/src/utils/SkMatrix44.cpp
index a7133ec1a75fb9d8c82fae09aa31197b250e7b12..1016a85cf1f5f2dfcf17ee7574b8d8d415f6b9d6 100644
--- a/src/utils/SkMatrix44.cpp
+++ b/src/utils/SkMatrix44.cpp
@@ -7,6 +7,13 @@
 
 #include "SkMatrix44.h"
 
+namespace {
+
+// Taken from Chromium's gfx::Transform

[danakj, 2014/09/19 16:29:47]

Haha, hilarious since it says "Taken from SkMatrix44."

[Ian Vollick, 2014/09/19 16:32:06]

Yeah, it was taken from here, but has since been removed. :)

+const SkMScalar kEpsilon = 1e-8f;

[reed1, 2014/09/19 16:45:14]

Can we actually document here the meaning/reason for this value?

[tomhudson, 2014/09/19 18:21:19]

If you go back a few years I think the comment in this code reads
  "just picked a small value. not sure how to pick the "right" one"

It looks like SK_ScalarNearlyZero is sufficient, so I changed this to a
parameter as suggested below and made that the default value.

+
+}
+
 static inline bool eq4(const SkMScalar* SK_RESTRICT a,
                       const SkMScalar* SK_RESTRICT b) {
     return (a[0] == b[0]) & (a[1] == b[1]) & (a[2] == b[2]) & (a[3] == b[3]);
@@ -857,6 +864,40 @@ void SkMatrix44::map2(const double src2[], int count, double dst4[]) const {
     proc(fMat, src2, count, dst4);
 }
 
+bool SkMatrix44::preserves2dAxisAlignment () const {

[reed1, 2014/09/19 16:45:14]

Did you consider taking an explicit epsilon (perhaps with a default parameter
value) so we aren't tied to the current heuristic?

[tomhudson, 2014/09/19 18:21:19]

Done.

+
+    // Can't check (mask & kPerspective_Mask) because Z isn't relevant here.
+    if (0 != perspX() || 0 != perspY()) return false;
+
+    int col0 = 0;
+    int col1 = 0;
+    int row0 = 0;
+    int row1 = 0;
+
+    // Must test against kEpsilon, not 0, because we can get values
+    // around 6e-17 in the matrix that "should" be 0.
+
+    if (sk_float_abs(fMat[0][0]) > kEpsilon) {

[reed1, 2014/09/19 16:45:14]

Since SkMScalar may not always be a float, we should use some indirection for
abs, so we don't accidentally force a downscale of the value just to perform the
compare.

e.g. create SkMScalarAbs()

[tomhudson, 2014/09/19 18:21:19]

Done.

+        col0++;
+        row0++;
+    }
+    if (sk_float_abs(fMat[0][1]) > kEpsilon) {
+        col1++;
+        row0++;
+    }
+    if (sk_float_abs(fMat[1][0]) > kEpsilon) {
+        col0++;
+        row1++;
+    }
+    if (sk_float_abs(fMat[1][1]) > kEpsilon) {
+        col1++;
+        row1++;
+    }
+    if (col0 > 1 || col1 > 1 || row0 > 1 || row1 > 1) return false;

[reed1, 2014/09/19 16:45:14]

add comment why 1 big value is OK, but two are not?

nit: needs {}

[tomhudson, 2014/09/19 18:21:19]

Done.

+
+    return true;
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 
 void SkMatrix44::dump() const {