From 6e7c59cfd7b590d3afad8ad2918a3bd21470d3c8 Mon Sep 17 00:00:00 2001 From: Stephen Gold Date: Sun, 20 Dec 2020 11:57:54 -0800 Subject: [PATCH 1/4] testcase and fix for issue #1388 (Quaternion javadoc is misleading) --- .../main/java/com/jme3/math/Quaternion.java | 58 +++++---- .../java/com/jme3/math/TestIssue1388.java | 114 ++++++++++++++++++ 2 files changed, 141 insertions(+), 31 deletions(-) create mode 100644 jme3-core/src/test/java/com/jme3/math/TestIssue1388.java diff --git a/jme3-core/src/main/java/com/jme3/math/Quaternion.java b/jme3-core/src/main/java/com/jme3/math/Quaternion.java index 40936652c9..f81fe425d5 100644 --- a/jme3-core/src/main/java/com/jme3/math/Quaternion.java +++ b/jme3-core/src/main/java/com/jme3/math/Quaternion.java @@ -185,12 +185,11 @@ public Quaternion set(Quaternion q) { } /** - * Constructor instantiates a new Quaternion object from a - * collection of rotation angles. + * Instantiate a new Quaternion from Euler rotation angles, + * applying the rotations in X-Z-Y order. * - * @param angles - * the angles of rotation (x, y, z) that will define the - * Quaternion. + * @param angles an array of Euler rotation angles (in radians, exactly 3 + * elements, in X-Y-Z order!, unaffected) */ public Quaternion(float[] angles) { fromAngles(angles); @@ -244,11 +243,11 @@ public boolean isIdentity() { } /** - * fromAngles builds a quaternion from the Euler rotation - * angles (x,y,z) aka (pitch, yaw, roll). + * Reconfigure this Quaternion based on Euler rotation angles, + * applying the rotations in X-Z-Y order. * - * @param angles - * the Euler angles of rotation (in radians). + * @param angles an array of Euler rotation angles (in radians, exactly 3 + * elements, in X-Y-Z order(!), unaffected) * @return this */ public Quaternion fromAngles(float[] angles) { @@ -261,22 +260,17 @@ public Quaternion fromAngles(float[] angles) { } /** - * fromAngles builds a Quaternion from the Euler rotation - * angles (x,y,z) aka (pitch, yaw, roll)). - * Note that we are applying in order: (y, x, z) aka (yaw, pitch, roll) - * but we've ordered them in x, y, and z for convenience. + * Reconfigure this Quaternion based on Euler rotations, applying the + * rotations in X-Z-Y order. This suits applications where +X is the forward + * direction and +Y is the up direction, in which case the order of + * application is (roll, pitch, yaw). * - * @see http://www.euclideanspace.com/maths/geometry/rotations/conversions/eulerToQuaternion/index.htm + * @see + * http://www.euclideanspace.com/maths/geometry/rotations/conversions/eulerToQuaternion/index.htm * - * @param xAngle - * the Euler pitch of rotation (in radians). (aka Attitude, often rot - * around x) - * @param yAngle - * the Euler yaw of rotation (in radians). (aka Heading, often - * rot around y) - * @param zAngle - * the Euler roll of rotation (in radians). (aka Bank, often - * rot around z) + * @param xAngle the X-axis rotation angle (in radians) + * @param yAngle the Y-axis rotation angle (in radians) + * @param zAngle the Z-axis rotation angle (in radians) * @return this */ public Quaternion fromAngles(float xAngle, float yAngle, float zAngle) { @@ -308,16 +302,18 @@ public Quaternion fromAngles(float xAngle, float yAngle, float zAngle) { } /** - * toAngles returns this quaternion converted to Euler rotation - * angles (x,y,z) aka (pitch, yaw, roll). + * Convert this Quaternion to Euler rotation angles, to be + * applied in X-Z-Y order, for instance by {@link #fromAngles(float[])}. * - * Note that the result is not always 100% accurate due to the implications of euler angles. - * @see http://www.euclideanspace.com/maths/geometry/rotations/conversions/quaternionToEuler/index.htm + * Note that the result is not always 100% accurate due to the implications + * of Euler angles. * - * @param angles - * the float[] in which the angles should be stored, or null if - * you want a new float[] to be created - * @return the float[] in which the angles are stored. + * @see + * http://www.euclideanspace.com/maths/geometry/rotations/conversions/quaternionToEuler/index.htm + * + * @param angles an array of 3 floats in which to store the result, or else + * null (If null, a new array will be allocated.) + * @return an array of 3 angles (in radians, in X-Y-Z order) */ public float[] toAngles(float[] angles) { if (angles == null) { diff --git a/jme3-core/src/test/java/com/jme3/math/TestIssue1388.java b/jme3-core/src/test/java/com/jme3/math/TestIssue1388.java new file mode 100644 index 0000000000..658f87a9d1 --- /dev/null +++ b/jme3-core/src/test/java/com/jme3/math/TestIssue1388.java @@ -0,0 +1,114 @@ +/* + * Copyright (c) 2020 jMonkeyEngine + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * + * * Neither the name of 'jMonkeyEngine' nor the names of its contributors + * may be used to endorse or promote products derived from this software + * without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR + * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, + * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, + * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR + * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING + * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package com.jme3.math; + +import org.junit.Assert; +import org.junit.Test; + +/** + * Verify the order in which Euler angles are applied. This was issue #1388 at + * GitHub. + * + * @author Stephen Gold + */ +public class TestIssue1388 { + + @Test + public void testIssue1388() { + Vector3f in = new Vector3f(4f, 6f, 9f); // test vector, never modified + Vector3f saveIn = in.clone(); + /* + * arbitrary Euler angles between -PI/2 and +PI/2 + */ + final float xAngle = 1.23f; + final float yAngle = 0.765f; + final float zAngle = -0.456f; + float[] angles = new float[]{xAngle, yAngle, zAngle}; + float[] saveAngles = new float[]{xAngle, yAngle, zAngle}; + /* + * Apply the rotations to the "in" vector in X,Z,Y order. + */ + Quaternion qx = new Quaternion().fromAngleAxis(xAngle, Vector3f.UNIT_X); + Quaternion qy = new Quaternion().fromAngleAxis(yAngle, Vector3f.UNIT_Y); + Quaternion qz = new Quaternion().fromAngleAxis(zAngle, Vector3f.UNIT_Z); + Vector3f outXZY = qx.mult(in); + qz.mult(outXZY, outXZY); + qy.mult(outXZY, outXZY); + /* + * Construct a Quaternion using fromAngles(float, float, float), + * use it to rotate the "in" vector, and compare. + */ + Quaternion q1 = new Quaternion().fromAngles(xAngle, yAngle, zAngle); + Vector3f out1 = q1.mult(in); + assertEquals(outXZY, out1, 1e-5f); + /* + * Construct a Quaternion using fromAngles(float[]), + * use it to rotate the "in" vector, and compare. + */ + Quaternion q2 = new Quaternion().fromAngles(angles); + Vector3f out2 = q2.mult(in); + assertEquals(outXZY, out2, 1e-5f); + /* + * Construct a Quaternion using only the constructor, + * use it to rotate the "in" vector, and compare. + */ + Quaternion q3 = new Quaternion(angles); + Vector3f out3 = q3.mult(in); + assertEquals(outXZY, out3, 1e-5f); + /* + * Verify that fromAngles() reverses toAngles() for the chosen angles. + */ + float[] out4 = q1.toAngles(null); + assertEquals(angles, out4, 1e-5f); + float[] out5 = q2.toAngles(null); + assertEquals(angles, out5, 1e-5f); + float[] out6 = q3.toAngles(null); + assertEquals(angles, out6, 1e-5f); + /* + * Verify that the values of "saveAngles" and "in" haven't changed. + */ + assertEquals(saveAngles, angles, 0f); + assertEquals(saveIn, in, 0f); + } + + void assertEquals(float[] expected, float[] actual, float tolerance) { + Assert.assertEquals(expected[0], actual[0], tolerance); + Assert.assertEquals(expected[1], actual[1], tolerance); + Assert.assertEquals(expected[2], actual[2], tolerance); + } + + void assertEquals(Vector3f expected, Vector3f actual, float tolerance) { + Assert.assertEquals(expected.x, actual.x, tolerance); + Assert.assertEquals(expected.y, actual.y, tolerance); + Assert.assertEquals(expected.z, actual.z, tolerance); + } +} From 45370653fe1102da9005d75901bda5982d4f8928 Mon Sep 17 00:00:00 2001 From: Stephen Gold Date: Sun, 20 Dec 2020 15:56:04 -0800 Subject: [PATCH 2/4] privatize 2 methods to please Codacy --- jme3-core/src/test/java/com/jme3/math/TestIssue1388.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/jme3-core/src/test/java/com/jme3/math/TestIssue1388.java b/jme3-core/src/test/java/com/jme3/math/TestIssue1388.java index 658f87a9d1..22e8b05ce2 100644 --- a/jme3-core/src/test/java/com/jme3/math/TestIssue1388.java +++ b/jme3-core/src/test/java/com/jme3/math/TestIssue1388.java @@ -100,13 +100,15 @@ public void testIssue1388() { assertEquals(saveIn, in, 0f); } - void assertEquals(float[] expected, float[] actual, float tolerance) { + private void assertEquals(float[] expected, float[] actual, + float tolerance) { Assert.assertEquals(expected[0], actual[0], tolerance); Assert.assertEquals(expected[1], actual[1], tolerance); Assert.assertEquals(expected[2], actual[2], tolerance); } - void assertEquals(Vector3f expected, Vector3f actual, float tolerance) { + private void assertEquals(Vector3f expected, Vector3f actual, + float tolerance) { Assert.assertEquals(expected.x, actual.x, tolerance); Assert.assertEquals(expected.y, actual.y, tolerance); Assert.assertEquals(expected.z, actual.z, tolerance); From 2153488d80ae3faef84f4580646b77ce60a8c8ca Mon Sep 17 00:00:00 2001 From: Stephen Gold Date: Mon, 8 Mar 2021 17:01:04 -0800 Subject: [PATCH 3/4] Quaternion: distinguish intrinsic-vs-extrinsic rotation order in cmts --- .../main/java/com/jme3/math/Quaternion.java | 44 ++++++++++--------- 1 file changed, 24 insertions(+), 20 deletions(-) diff --git a/jme3-core/src/main/java/com/jme3/math/Quaternion.java b/jme3-core/src/main/java/com/jme3/math/Quaternion.java index f81fe425d5..16faed48be 100644 --- a/jme3-core/src/main/java/com/jme3/math/Quaternion.java +++ b/jme3-core/src/main/java/com/jme3/math/Quaternion.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2009-2020 jMonkeyEngine + * Copyright (c) 2009-2021 jMonkeyEngine * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -185,11 +185,13 @@ public Quaternion set(Quaternion q) { } /** - * Instantiate a new Quaternion from Euler rotation angles, - * applying the rotations in X-Z-Y order. + * Instantiate a new Quaternion from Tait-Bryan angles, + * applying the rotations in x-z-y extrinsic order or y-z'-x" intrinsic + * order. * - * @param angles an array of Euler rotation angles (in radians, exactly 3 - * elements, in X-Y-Z order!, unaffected) + * @param angles an array of Tait-Bryan angles (in radians, exactly 3 + * elements, the X angle in angles[0], the Y angle in angles[1], and the Z + * angle in angles[2], unaffected) */ public Quaternion(float[] angles) { fromAngles(angles); @@ -243,11 +245,13 @@ public boolean isIdentity() { } /** - * Reconfigure this Quaternion based on Euler rotation angles, - * applying the rotations in X-Z-Y order. + * Reconfigure this Quaternion based on Tait-Bryan angles, + * applying the rotations in x-z-y extrinsic order or y-z'-x" intrinsic + * order. * - * @param angles an array of Euler rotation angles (in radians, exactly 3 - * elements, in X-Y-Z order(!), unaffected) + * @param angles an array of Tait-Bryan angles (in radians, exactly 3 + * elements, the X angle in angles[0], the Y angle in angles[1], and the Z + * angle in angles[2], unaffected) * @return this */ public Quaternion fromAngles(float[] angles) { @@ -260,17 +264,15 @@ public Quaternion fromAngles(float[] angles) { } /** - * Reconfigure this Quaternion based on Euler rotations, applying the - * rotations in X-Z-Y order. This suits applications where +X is the forward - * direction and +Y is the up direction, in which case the order of - * application is (roll, pitch, yaw). + * Reconfigure this Quaternion based on Tait-Bryan rotations, applying the + * rotations in x-z-y extrinsic order or y-z'-x" intrinsic order. * * @see * http://www.euclideanspace.com/maths/geometry/rotations/conversions/eulerToQuaternion/index.htm * - * @param xAngle the X-axis rotation angle (in radians) - * @param yAngle the Y-axis rotation angle (in radians) - * @param zAngle the Z-axis rotation angle (in radians) + * @param xAngle the X angle (in radians) + * @param yAngle the Y angle (in radians) + * @param zAngle the Z angle (in radians) * @return this */ public Quaternion fromAngles(float xAngle, float yAngle, float zAngle) { @@ -302,18 +304,20 @@ public Quaternion fromAngles(float xAngle, float yAngle, float zAngle) { } /** - * Convert this Quaternion to Euler rotation angles, to be - * applied in X-Z-Y order, for instance by {@link #fromAngles(float[])}. + * Convert this Quaternion to Tait-Bryan angles, to be applied + * in x-z-y intrinsic order or y-z'-x" extrinsic order, for instance by + * {@link #fromAngles(float[])}. * * Note that the result is not always 100% accurate due to the implications - * of Euler angles. + * of Tait-Bryan angles. * * @see * http://www.euclideanspace.com/maths/geometry/rotations/conversions/quaternionToEuler/index.htm * * @param angles an array of 3 floats in which to store the result, or else * null (If null, a new array will be allocated.) - * @return an array of 3 angles (in radians, in X-Y-Z order) + * @return an array of 3 angles (in radians, the X angle in angles[0], the Y + * angle in angles[1], and the Z angle in angles[2]) */ public float[] toAngles(float[] angles) { if (angles == null) { From 462282f24b5871b4550e38ecb60740a2ec5d7993 Mon Sep 17 00:00:00 2001 From: Stephen Gold Date: Mon, 8 Mar 2021 17:01:46 -0800 Subject: [PATCH 4/4] TestIssue1388: verify intrinsic rotation order --- .../java/com/jme3/math/TestIssue1388.java | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/jme3-core/src/test/java/com/jme3/math/TestIssue1388.java b/jme3-core/src/test/java/com/jme3/math/TestIssue1388.java index 22e8b05ce2..23fe86168a 100644 --- a/jme3-core/src/test/java/com/jme3/math/TestIssue1388.java +++ b/jme3-core/src/test/java/com/jme3/math/TestIssue1388.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2020 jMonkeyEngine + * Copyright (c) 2020-2021 jMonkeyEngine * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -35,8 +35,8 @@ import org.junit.Test; /** - * Verify the order in which Euler angles are applied. This was issue #1388 at - * GitHub. + * Verify the order in which Tait-Bryan angles are applied by the Quaternion + * class. This was issue #1388 at GitHub. * * @author Stephen Gold */ @@ -47,7 +47,7 @@ public void testIssue1388() { Vector3f in = new Vector3f(4f, 6f, 9f); // test vector, never modified Vector3f saveIn = in.clone(); /* - * arbitrary Euler angles between -PI/2 and +PI/2 + * Three arbitrary rotation angles between -PI/2 and +PI/2 */ final float xAngle = 1.23f; final float yAngle = 0.765f; @@ -55,7 +55,9 @@ public void testIssue1388() { float[] angles = new float[]{xAngle, yAngle, zAngle}; float[] saveAngles = new float[]{xAngle, yAngle, zAngle}; /* - * Apply the rotations to the "in" vector in X,Z,Y order. + * Part 1: verify that the extrinsic rotation order is x-z-y + * + * Apply extrinsic rotations to the "in" vector in x-z-y order. */ Quaternion qx = new Quaternion().fromAngleAxis(xAngle, Vector3f.UNIT_X); Quaternion qy = new Quaternion().fromAngleAxis(yAngle, Vector3f.UNIT_Y); @@ -93,6 +95,14 @@ public void testIssue1388() { assertEquals(angles, out5, 1e-5f); float[] out6 = q3.toAngles(null); assertEquals(angles, out6, 1e-5f); + /* + * Part 2: verify intrinsic rotation order + * + * Apply intrinsic rotations to the "in" vector in y-z'-x" order. + */ + Quaternion q4 = qy.mult(qz).mult(qx); + Vector3f out7 = q4.mult(in); + assertEquals(outXZY, out7, 1e-5f); /* * Verify that the values of "saveAngles" and "in" haven't changed. */