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

jme3-core: document a few places where normalization is assumed #2026

Merged
merged 5 commits into from
Jun 2, 2023

Conversation

stephengold
Copy link
Member

The PR documents a few places where normalization is assumed.

JME assumes that the local rotations of scene-graph spatials are normalized. A violation of this assumption caused the issue @yaRnMcDonuts recently reported at the Discourse Forum/Hub.

@stephengold stephengold added the Documentation Issues that affect the Wiki, Javadoc or any other form of documentation label May 28, 2023
@stephengold stephengold modified the milestones: Future Release, v3.6.1 May 28, 2023
@stephengold
Copy link
Member Author

Unless there's further discussion, I plan to integrate this PR in about 24 hours.

@stephengold
Copy link
Member Author

I realized that transformVector() and transformInverseVector() both use Quaternion.rot(Vector3f, Vector3f) and therefore make the same assumption of normality. I've added appropriate javadoc to this PR.

Resetting the clock, I plan to integrate this PR in 24 hours.

@stephengold stephengold merged commit 0767163 into master Jun 2, 2023
@stephengold stephengold deleted the sgold/pr/abnormal branch June 2, 2023 19:06
@pspeed42
Copy link
Contributor

pspeed42 commented Jun 2, 2023

Probably too late comment so do with it what you will...

There was once a time that someone wanted to rename "mult" to "rotate". The ultimate issue was that Quaternion is a math concept that can be used for 3D graphics but that is not necessarily it's only use. When a Quaternion is "normal" then "mult" can be used for rotation but that might not be the only reason to call Quaternion.mult in a mathematics sense.

Any time we talk about rotation, then it is kind of necessary for Quaternions to be normalized and so it 100% makes sense to mention the requirement for the methods that are specifically dealing with rotations. (like the one returning a rotation matrix, etc.). It may make less sense for the "purely math but can be used for rotation" ones.

That said, anyone using Quaternions for something other than rotation probably already knows this.

@stephengold
Copy link
Member Author

stephengold commented Jun 2, 2023

I assume that the Quaternion.mult() and multLocal() methods involving vectors were named by analogy with Matrix3f. Both sets of methods serve analogous purposes: for restricted values (unit quaternions and 3x3 rotation matrices) the methods perform 3-D rotations on vectors.

Unfortunately, mathematicians defined multiplication of a quaternion with a vector to suit their purposes, and that definition conflicts with what Quaternion.mult() actually does to a 3-D vector, namely: rotate (not multiply!) the vector. Seeing these methods named mult() caused me to add javadoc disclaimers at #1725, lest someone using Quaternion for something other than rotation be misled by the names. (Interestingly, the mult() methods that don't involve vectors do agree with the mathematical definition of multiplication, which only magnifies the confusion.)

While it seems expedient for rotation methods to assume their inputs are normalized and (for such inputs) produce normalized outputs, it leads to subtle bugs. For instance, if you start with a normalized value and then apply enough math operations, floating-point round-off may cause the norms to diverge substantially from 1.

Furthermore, naive users and buggy tools (such as the one that bit Ryan) generate unintentional non-normalized quaternions with surprising frequency.

I realize the disclaimers added in this PR are unlikely to mean much to naive users. They're mainly to soothe my conscience at the lack of run-time checks.

In my own projects, I prefer to check for a normal quaternion before each rotation. The cost seems slight compared to the time I spend debugging issues. But such run-time checks seem contrary to the JME Way, so I have not added them here.

stephengold added a commit that referenced this pull request Jun 8, 2023
* jme3-core:  document a few places where normalization is assumed

* Spatial:  document the assumption in setLocalRotation()

* Spatial:  document the argument to setLocalTransform()

* javadoc:  say "approximately equal to 1"

* document assumptions of transformVector() and transformInverseVector()
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.

3 participants