Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

testcase and fix for issue #1388 (Quaternion javadoc is misleading) #1451

Merged
merged 4 commits into from Mar 16, 2021
Merged

testcase and fix for issue #1388 (Quaternion javadoc is misleading) #1451

merged 4 commits into from Mar 16, 2021

Conversation

stephengold
Copy link
Member

This PR corrects the documentation for 4 methods in com.jme3.math.Quaternion:

  1. fromAngles(float, float, float)
  2. fromAngles(float[])
  3. the Quaternion(angles) constructor
  4. toAngles(float[])

It also adds a test case which provides strong evidence that all 4 methods implement X-Z-Y rotation order.

@pspeed42
Copy link
Contributor

X-Z-Y order... meaning that if you want to duplicate that rotation in steps, first you rotate around Y then you rotate around Z and then you rotate around X. We should be explicit because even though its common to state rotation from "child up", it's backwards in most conversational descriptions of rotations.

For example, to duplicate the same rotation with separate nodes parent, child, grandchild... rotate parent Y, rotate child Z, rotate grandchild X.

And even though JME is Z forward, it chose to implement default rotation order as if it were X forward... which is unfortunate because it means Z (roll) is basically useless for 99% of games that might have otherwise been able to use the angle triple. Such are the joys of 'backwards bugpatability'.

@stephengold
Copy link
Member Author

Child-up or parent-down ordering only matter in a scene graph, and there's no scene graph here.

In order to duplicate a fromAngles() rotation in steps, you would actually apply X then Z then Y. (See the test case.)

If "X-Z-Y" means something different to you, then I should rephase the documentation to say "X then Z then Y" instead of "X-Z-Y".

@tlf30
Copy link
Contributor

tlf30 commented Dec 21, 2020

That looks good to me @stephengold

@pspeed42
Copy link
Contributor

re: "In order to duplicate a fromAngles() rotation in steps, you would actually apply X then Z then Y. (See the test case.)

If "X-Z-Y" means something different to you, then I should rephase the documentation to say "X then Z then Y" instead of "X-Z-Y"."

Here is kind of where we get to the crux of the confusion and how the context of the reader becomes super important because we've given them nothing to go on.

Your test is multiplying the vector once by each quaternion which from that perspective is indeed going to be x then z then y. But if you are thinking about composing rotations (and why wouldn't you because fromAngles() IS composing a rotation) then it's backwards.

For example, if you replace this code block:

        Vector3f outXZY = qx.mult(in);
        qz.mult(outXZY, outXZY);
        qy.mult(outXZY, outXZY);

With:

        Quaternion qAll = qx.mult(qz).mult(qy);
        qAll.mult(in, outXZY);

Then you get a different answer and the order is backwards.

        Quaternion qAll = qy.mult(qz).mult(qx);
        qAll.mult(in, outXZY);

Fixes the test again.

Your test is treating the object like it's sitting on the desk in front of you. Rotate around X. Grab that and rotatearound Z, etc..

qy.mult(qz).mult(qx) is from the perspective of the object itself... rotate me around y then rotate me around z, etc.. This can be a more useful way of looking at rotations in the environment of a scene graph.

Put another way, if you were trying to explain to a user what is happening, which looks right based on your javadoc:

    Quaternion qResult = new Quaternion().fromAngles(xAngle, 0, 0)
                        .mult(new Quaternion().fromAngles(0, 0, zAngle))
                        .mult(new Quaternion().fromAngles(0, yAngle, 0));

Or:

    Quaternion qResult = new Quaternion().fromAngles(0, yAngle, 0)
                        .mult(new Quaternion().fromAngles(0, 0, zAngle))
                        .mult(new Quaternion().fromAngles(xAngle, 0, 0));

This bottom up versus top down rotational ordering has been the subject of many a discussion and even involves post order versus pre-order quaternion/matrix multiplication and so on. So if we are going to talk about order then we need to talk about what order means.

Or just provide a succinct example... given the nature of the method, you could do worse than showing how it would look if you multiplied the three separate calls together as:

    Quaternion qResult = new Quaternion().fromAngles(0, yAngle, 0)
                        .mult(new Quaternion().fromAngles(0, 0, zAngle))
                        .mult(new Quaternion().fromAngles(xAngle, 0, 0));

Note: a small quibble with the test is that it's using fromAngleAxis() which might inadvertently be testing the difference between fromAngles() and fromAngleAxis(). (It's not but it could be.)

@stephengold
Copy link
Member Author

Thank you for the detailed explanation. I agree that the javadoc should be clear and unambiguous. I thought I knew what Euler angles were, but I was mistaken.

I'll come back and try again after I've done some more research into the various conventions for defining Euler angles.

@stephengold stephengold added this to the v3.4.0 milestone Feb 20, 2021
@stephengold
Copy link
Member Author

According to my reading, the formalism implemented in the Quaternion class is "Tait–Bryan angles" so I have updated the documentation accordingly. Some sources refer to these as "Euler angles" but that term is more properly applied to "classic Euler angles" in which the first and last rotations are around the same axis. "Tait-Bryan" is less ambiguous.

The best way I've found to clarify the order (in which rotations are applied) is to distinguish "intrinsic" rotations (around movable body axes) from "extrinsic" rotations (around fixed world axes). For maximum clarity, I've documented the implementation both ways.

If/when this PR is integrated, please perform a "squash and merge".

@stephengold stephengold added the Documentation Issues that affect the Wiki, Javadoc or any other form of documentation label Mar 13, 2021
@stephengold
Copy link
Member Author

Unless there's substantive discussion, I plan to self-integrate this PR in 24 hours.

@stephengold stephengold merged commit 9183f0e into jMonkeyEngine:master Mar 16, 2021
@stephengold stephengold deleted the sgold-issue-1388 branch March 16, 2021 02:16
@stephengold stephengold linked an issue Mar 16, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Issues that affect the Wiki, Javadoc or any other form of documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

misleading javadoc for Quaternion fromAngles()
3 participants