Add perspective support to SkMatrix44 initializers.

Add perspective support to SkMatrix44 initializers.

I noticed SkMatrix <-> SkMatrix44 conversions were dropping the
perspective values on the floor.  As we use SkMatrix44 heavily in
Chromium, I'm concerned this missing code will cause a bug eventually.
It should be correct to simply use the bottom row of the 4x4 matrix
excluding the third column.

Previously committed and reverted, second attempt with fix for
incorrect use of SkMScalar/SkScalar.

BUG=

Committed: http://code.google.com/p/skia/source/detail?r=11624

[aelias_OOO_until_Jul13, 2013-10-01 21:33:47]

Hi Mike, just cleaning up a loose end I noticed in the code.

[reed1, 2013-10-02 08:35:20]

seems correct. Can we write a test to confirm this?

[aelias_OOO_until_Jul13, 2013-10-02 23:40:16]

Added a test, PTAL.

[reed1, 2013-10-03 09:00:40]

https://codereview.chromium.org/25484006/diff/5001/include/utils/SkMatrix44.h
File include/utils/SkMatrix44.h (right):

https://codereview.chromium.org/25484006/diff/5001/include/utils/SkMatrix44.h...
include/utils/SkMatrix44.h:265: * perspective components remain untouched. */
the i.e. section is incorrect: set3x3 sets *all* elements of the matrix: the
top-left corner is set by the params, but translate and perspective elements are
set to 0 (with [3][3] set to 1).

This is consistent with skia's general matrix naming convention. all of its
set... methods always overwrite the entire matrix, as opposed to its various
concat/preconcat/postconcat methods, which may ignore some fields, or
read-modify others.

[aelias_OOO_until_Jul13, 2013-10-03 19:05:36]

https://codereview.chromium.org/25484006/diff/5001/include/utils/SkMatrix44.h
File include/utils/SkMatrix44.h (right):

https://codereview.chromium.org/25484006/diff/5001/include/utils/SkMatrix44.h...
include/utils/SkMatrix44.h:265: * perspective components remain untouched. */
On 2013/10/03 09:00:40, reed1 wrote:
> the i.e. section is incorrect: set3x3 sets *all* elements of the matrix: the
> top-left corner is set by the params, but translate and perspective elements
are
> set to 0 (with [3][3] set to 1).

Right, fixed the comment.

[reed1, 2013-10-04 08:33:32]

lgtm

[commit-bot: I haz the power, 2013-10-04 18:22:01]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/aelias@chromium.org/25484006/10001

[commit-bot: I haz the power, 2013-10-04 18:22:04]

Failed to apply patch for src/utils/SkMatrix44.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file src/utils/SkMatrix44.cpp
  Hunk #1 succeeded at 902 with fuzz 2 (offset 22 lines).
  Hunk #2 FAILED at 888.
  Hunk #3 succeeded at 936 (offset 30 lines).
  Hunk #4 succeeded at 947 (offset 30 lines).
  1 out of 4 hunks FAILED -- saving rejects to file src/utils/SkMatrix44.cpp.rej

Patch:       src/utils/SkMatrix44.cpp
Index: src/utils/SkMatrix44.cpp
diff --git a/src/utils/SkMatrix44.cpp b/src/utils/SkMatrix44.cpp
index
658d5c1aa845915a0418371ba7851b66fced6657..eb0819f1df3d57fc6310dc0d51dc6ddb61d1c97e
100644
--- a/src/utils/SkMatrix44.cpp
+++ b/src/utils/SkMatrix44.cpp
@@ -880,8 +880,6 @@ void SkMatrix44::dump() const {
 
 ///////////////////////////////////////////////////////////////////////////////
 
-// TODO: make this support src' perspective elements
-//
 static void initFromMatrix(SkMScalar dst[4][4], const SkMatrix& src) {
     sk_bzero(dst, 16 * sizeof(SkMScalar));
     dst[0][0] = SkScalarToMScalar(src[SkMatrix::kMScaleX]);
@@ -890,7 +888,10 @@ static void initFromMatrix(SkMScalar dst[4][4], const
SkMatrix& src) {
     dst[0][1] = SkScalarToMScalar(src[SkMatrix::kMSkewY]);
     dst[1][1] = SkScalarToMScalar(src[SkMatrix::kMScaleY]);
     dst[3][1] = SkScalarToMScalar(src[SkMatrix::kMTransY]);
-    dst[2][2] = dst[3][3] = 1;
+    dst[0][3] = SkScalarToMScalar(src[SkMatrix::kMPersp0]);
+    dst[1][3] = SkScalarToMScalar(src[SkMatrix::kMPersp1]);
+    dst[3][3] = SkScalarToMScalar(src[SkMatrix::kMPersp2]);
+    dst[2][2] = 1;
 }
 
 SkMatrix44::SkMatrix44(const SkMatrix& src) {
@@ -908,11 +909,8 @@ SkMatrix44& SkMatrix44::operator=(const SkMatrix& src) {
     return *this;
 }
 
-// TODO: make this support our perspective elements
-//
 SkMatrix44::operator SkMatrix() const {
     SkMatrix dst;
-    dst.reset();    // setup our perspective correctly for identity
 
     dst[SkMatrix::kMScaleX]  = SkMScalarToScalar(fMat[0][0]);
     dst[SkMatrix::kMSkewX]  = SkMScalarToScalar(fMat[1][0]);
@@ -922,5 +920,9 @@ SkMatrix44::operator SkMatrix() const {
     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;
 }

[commit-bot: I haz the power, 2013-10-04 20:04:41]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/aelias@chromium.org/25484006/15001

[commit-bot: I haz the power, 2013-10-04 20:38:11]

Change committed as 11622

[aelias_OOO_until_Jul13, 2013-10-04 20:56:30]

Sorry, looks like this is causing a build failure on 64-bit bots:
http://108.170.217.252:10117/builders/Build-Mac10.6-GCC-x86-Debug/builds/1815...

"../tests/Matrix44Test.cpp:491: warning: implicit conversion shortens 64-bit
value into a 32-bit value"

I'm not sure how sheriffing/reverts work on Skia.

[caryclark, 2013-10-04 22:13:21]

On 2013/10/04 20:56:30, aelias wrote:
> Sorry, looks like this is causing a build failure on 64-bit bots:
>
http://108.170.217.252:10117/builders/Build-Mac10.6-GCC-x86-Debug/builds/1815...
> 
> "../tests/Matrix44Test.cpp:491: warning: implicit conversion shortens 64-bit
> value into a 32-bit value"
> 
> I'm not sure how sheriffing/reverts work on Skia.

I'm the sherriff, but I can't revert from here. Can you follow these
instructions to revert your checkin?

https://sites.google.com/site/skiadocs/developer-documentation/tree-sheriff
(search for revert)

[aelias_OOO_until_Jul13, 2013-10-04 22:27:40]

Reverted in r11623.

[aelias_OOO_until_Jul13, 2013-10-05 01:06:36]

The problem was using SkMScalar instead of SkScalar in the part of the test that
used SkMatrix.  Relanding with fix for that.

[commit-bot: I haz the power, 2013-10-05 01:06:59]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/aelias@chromium.org/25484006/30001

[commit-bot: I haz the power, 2013-10-05 01:16:33]

Change committed as 11624