From b60a8dc6b90bd607353d376a962f6df05239614e Mon Sep 17 00:00:00 2001 From: Krzysztof Magiera Date: Mon, 17 Jul 2017 18:22:17 -0700 Subject: [PATCH] Fix rotation matrix decomposition. Summary: This PR fixes an issue with rotation decomposition matrix on android. The issue can be illustrated with this sample code https://snack.expo.io/r1SHEJpVb It surfaces when we have non-zero rotation in Y or X axis and when rotation Z is greater than 90deg or less than -90deg. In that case the decomposition code doesn't give a valid output and as a result the view gets rotated by 180deg in Z axis. You may want to run the code linked above on android and iOS to see the difference. Basically the example app renders first image rotated only by 89deg and the next one by 91deg. As a result you should see the second view being pivoted just slightly more than the first image. Apparently on android the second image is completely flipped: iOS: ![screen shot 2017-07-07 at 12 40 30](https://user-images.githubusercontent.com/726445/27954719-7cf6d02c-6311-11e7-9104-5c3cc8e9b9c1.png) Android: ![screen shot 2017-07-07 at 12 41 21](https://user-images.githubusercontent.com/726445/27954737-981f57e8-6311-11e7-8c72-af1824426c30.png) The bug seemed to be caused by the code that decomposes the matrix into axis angles. It seems like that whole code has been overly complicated and we've been converting matrix first into quaternion just to extract angles. Whereas it is sufficient to extract angles directly from rotation matrix as described here: http://nghiaho.com/?page_id=846 This formula produces way simpler code and also gives correct result in the aforementioned case, so I decided not to debug quaternion code any further. sidenote: New formula's y angle output range is now -90 to 90deg hence changes in tests. Closes https://github.com/facebook/react-native/pull/14888 Reviewed By: astreet Differential Revision: D5414006 Pulled By: shergin fbshipit-source-id: 2e0a68cf4b2a9e32f10f6bfff2d484867a337fa3 --- .../react/uimanager/MatrixMathHelper.java | 80 ++----------------- .../java/com/facebook/react/uimanager/BUCK | 2 +- .../react/uimanager/MatrixMathHelperTest.java | 29 ++++++- 3 files changed, 31 insertions(+), 80 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/MatrixMathHelper.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/MatrixMathHelper.java index aacc78ca5a63bd..cbf665a7ed8fd3 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/MatrixMathHelper.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/MatrixMathHelper.java @@ -12,7 +12,6 @@ public class MatrixMathHelper { public static class MatrixDecompositionContext { double[] perspective = new double[4]; - double[] quaternion = new double[4]; double[] scale = new double[3]; double[] skew = new double[3]; double[] translation = new double[3]; @@ -65,7 +64,6 @@ public static void decomposeMatrix(double[] transformMatrix, MatrixDecomposition // output values final double[] perspective = ctx.perspective; - final double[] quaternion = ctx.quaternion; final double[] scale = ctx.scale; final double[] skew = ctx.skew; final double[] translation = ctx.translation; @@ -170,35 +168,11 @@ public static void decomposeMatrix(double[] transformMatrix, MatrixDecomposition } // Now, get the rotations out - quaternion[0] = - 0.5 * Math.sqrt(Math.max(1 + row[0][0] - row[1][1] - row[2][2], 0)); - quaternion[1] = - 0.5 * Math.sqrt(Math.max(1 - row[0][0] + row[1][1] - row[2][2], 0)); - quaternion[2] = - 0.5 * Math.sqrt(Math.max(1 - row[0][0] - row[1][1] + row[2][2], 0)); - quaternion[3] = - 0.5 * Math.sqrt(Math.max(1 + row[0][0] + row[1][1] + row[2][2], 0)); - - if (row[2][1] > row[1][2]) { - quaternion[0] = -quaternion[0]; - } - if (row[0][2] > row[2][0]) { - quaternion[1] = -quaternion[1]; - } - if (row[1][0] > row[0][1]) { - quaternion[2] = -quaternion[2]; - } - - // correct for occasional, weird Euler synonyms for 2d rotation - - if (quaternion[0] < 0.001 && quaternion[0] >= 0 && - quaternion[1] < 0.001 && quaternion[1] >= 0) { - // this is a 2d rotation on the z-axis - rotationDegrees[0] = rotationDegrees[1] = 0d; - rotationDegrees[2] = roundTo3Places(Math.atan2(row[0][1], row[0][0]) * 180 / Math.PI); - } else { - quaternionToDegreesXYZ(quaternion, rotationDegrees); - } + // Based on: http://nghiaho.com/?page_id=846 + double conv = 180 / Math.PI; + rotationDegrees[0] = roundTo3Places(-Math.atan2(row[2][1], row[2][2]) * conv); + rotationDegrees[1] = roundTo3Places(-Math.atan2(-row[2][0], Math.sqrt(row[2][1] * row[2][1] + row[2][2] * row[2][2])) * conv); + rotationDegrees[2] = roundTo3Places(-Math.atan2(row[1][0], row[0][0]) * conv); } public static double determinant(double[] matrix) { @@ -334,50 +308,6 @@ public static double[] v3Cross(double[] a, double[] b) { }; } - /** - * Based on: - * http://www.euclideanspace.com/maths/geometry/rotations/conversions/quaternionToEuler/ - * and: - * http://quat.zachbennett.com/ - * - * Note that this rounds degrees to the thousandth of a degree, due to - * floating point errors in the creation of the quaternion. - * - * Also note that this expects the qw value to be last, not first. - * - * Also, when researching this, remember that: - * yaw === heading === z-axis - * pitch === elevation/attitude === y-axis - * roll === bank === x-axis - */ - public static void quaternionToDegreesXYZ(double[] q, double[] result) { - double qx = q[0], qy = q[1], qz = q[2], qw = q[3]; - double qw2 = qw * qw; - double qx2 = qx * qx; - double qy2 = qy * qy; - double qz2 = qz * qz; - double test = qx * qy + qz * qw; - double unit = qw2 + qx2 + qy2 + qz2; - double conv = 180 / Math.PI; - - if (test > 0.49999 * unit) { - result[0] = 0; - result[1] = 2 * Math.atan2(qx, qw) * conv; - result[2] = 90; - return; - } - if (test < -0.49999 * unit) { - result[0] = 0; - result[1] = -2 * Math.atan2(qx, qw) * conv; - result[2] = -90; - return; - } - - result[0] = roundTo3Places(Math.atan2(2 * qx * qw - 2 * qy * qz, 1 - 2 * qx2 - 2 * qz2) * conv); - result[1] = roundTo3Places(Math.atan2(2 * qy * qw - 2 * qx * qz, 1 - 2 * qy2 - 2 * qz2) * conv); - result[2] = roundTo3Places(Math.asin(2 * qx * qy + 2 * qz * qw) * conv); - } - public static double roundTo3Places(double n) { return Math.round(n * 1000d) * 0.001; } diff --git a/ReactAndroid/src/test/java/com/facebook/react/uimanager/BUCK b/ReactAndroid/src/test/java/com/facebook/react/uimanager/BUCK index 9b25be3663086d..2ab16ccf698150 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/uimanager/BUCK +++ b/ReactAndroid/src/test/java/com/facebook/react/uimanager/BUCK @@ -4,7 +4,7 @@ rn_robolectric_test( name = "uimanager", # TODO Disabled temporarily until Yoga linking is fixed t14964130 # srcs = glob(['**/*.java']), - srcs = ["SimpleViewPropertyTest.java"], + srcs = ["SimpleViewPropertyTest.java", "MatrixMathHelperTest.java"], # Please change the contact to the oncall of your team contacts = ["oncall+fbandroid_sheriff@xmail.facebook.com"], visibility = [ diff --git a/ReactAndroid/src/test/java/com/facebook/react/uimanager/MatrixMathHelperTest.java b/ReactAndroid/src/test/java/com/facebook/react/uimanager/MatrixMathHelperTest.java index c7d332813f15e6..87cba4bebda7e2 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/uimanager/MatrixMathHelperTest.java +++ b/ReactAndroid/src/test/java/com/facebook/react/uimanager/MatrixMathHelperTest.java @@ -36,6 +36,20 @@ private void verifyXRotatedMatrix(double degrees, double rotX, double rotY, doub assertThat(ctx.rotationDegrees).containsSequence(rotX, rotY, rotZ); } + private void verifyRotatedMatrix(double degreesX, double degreesY, double degreesZ, double rotX, double rotY, double rotZ) { + MatrixMathHelper.MatrixDecompositionContext ctx = + new MatrixMathHelper.MatrixDecompositionContext(); + double[] matrixX = createRotateX(degreesToRadians(degreesX)); + double[] matrixY = createRotateY(degreesToRadians(degreesY)); + double[] matrixZ = createRotateZ(degreesToRadians(degreesZ)); + double[] matrix = MatrixMathHelper.createIdentityMatrix(); + MatrixMathHelper.multiplyInto(matrix, matrix, matrixX); + MatrixMathHelper.multiplyInto(matrix, matrix, matrixY); + MatrixMathHelper.multiplyInto(matrix, matrix, matrixZ); + MatrixMathHelper.decomposeMatrix(matrix, ctx); + assertThat(ctx.rotationDegrees).containsSequence(rotX, rotY, rotZ); + } + @Test public void testDecomposing4x4MatrixToProduceAccurateZaxisAngles() { @@ -82,17 +96,17 @@ public void testDecomposing4x4MatrixToProduceAccurateZaxisAngles() { @Test public void testDecomposing4x4MatrixToProduceAccurateYaxisAngles() { - double[] angles = new double[]{30, 45, 60, 75, 90, 100, 110, 120, 133, 167}; + double[] angles = new double[]{30, 45, 60, 75, 90}; for (double angle : angles) { verifyYRotatedMatrix(angle, 0d, angle, 0d); verifyYRotatedMatrix(-angle, 0d, -angle, 0d); } - // all values are between 0 and 180; + // all values are between -90 and 90; // change of sign and direction in the third and fourth quadrant - verifyYRotatedMatrix(222, 0d, -138d, 0d); + verifyYRotatedMatrix(222, -180d, -42d, -180d); - verifyYRotatedMatrix(270, 0d, -90d, 0d); + verifyYRotatedMatrix(270, -180d, -90d, -180d); verifyYRotatedMatrix(360, 0d, 0d, 0d); } @@ -114,6 +128,13 @@ public void testDecomposing4x4MatrixToProduceAccurateXaxisAngles() { verifyXRotatedMatrix(360, 0d, 0d, 0d); } + @Test + public void testDecomposingComplex4x4MatrixToProduceAccurateAngles() { + verifyRotatedMatrix(10, -80, 0, 10, -80, 0); + // x and y will filp + verifyRotatedMatrix(10, -95, 0, -170, -85, -180); + } + private static double degreesToRadians(double degrees) { return degrees * Math.PI / 180; }