SkMatrix44::preserves2dAxisAlignment()

src/utils/SkMatrix44.cpp

 /*
  * Copyright 2011 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #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]);
 }
 
 bool SkMatrix44::operator==(const SkMatrix44& other) const {
     if (this == &other) {
         return true;
     }
 
@@ skipping 830 matching lines @@
 void SkMatrix44::map2(const double src2[], int count, double dst4[]) const {
     static const Map2Procd gProc[] = {
         map2_id, map2_td, map2_sd, map2_sd, map2_ad, map2_ad, map2_ad, map2_ad
     };
 
     TypeMask mask = this->getType();
     Map2Procd proc = (mask & kPerspective_Mask) ? map2_pd : gProc[mask];
     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 {
     static const char* format =
         "[%g %g %g %g][%g %g %g %g][%g %g %g %g][%g %g %g %g]\n";
 #if 0
     SkDebugf(format,
              fMat[0][0], fMat[1][0], fMat[2][0], fMat[3][0],
              fMat[0][1], fMat[1][1], fMat[2][1], fMat[3][1],
              fMat[0][2], fMat[1][2], fMat[2][2], fMat[3][2],
@@ skipping 53 matching lines @@
     dst[SkMatrix::kMSkewY]  = SkMScalarToScalar(fMat[0][1]);
     dst[SkMatrix::kMScaleY] = SkMScalarToScalar(fMat[1][1]);
     dst[SkMatrix::kMTransY] = SkMScalarToScalar(fMat[3][1]);
 
     dst[SkMatrix::kMPersp0] = SkMScalarToScalar(fMat[0][3]);
     dst[SkMatrix::kMPersp1] = SkMScalarToScalar(fMat[1][3]);
     dst[SkMatrix::kMPersp2] = SkMScalarToScalar(fMat[3][3]);
 
     return dst;
 }