Unexpected side effect of Quaternion.RotationAxis(...)

Hello,

I just stumbled over the fact that BABYLON.Quaternion.RotationAxis(axis, ...) normalizes the input axis in place, even though it is declared as a DeepImmutable<Vector3>.

Example:

  • The thin tube, which uses the original axis, is quite long.
  • The thick tube, which uses the axis after a call to RotationAxis(...) only has length 1. The length should be unchanged.

The problem is apparently caused by this line and the fact that axis.normalize() actually normalizes in place.

Since there is a lot of code relying on the fact that .normalize() modifies the vector in place, we cannot fix the problem by changing that method. We rather have to change BABYLON.Quaternion.RotationAxis(...) or actually BABYLON.Quaternion.RotationAxisToRef(...).

Here is my suggestion:

    /**
     * Creates a rotation around an axis and stores it into the given quaternion
     * Example Playground https://playground.babylonjs.com/#L49EJ7#73
     * @param axis defines the axis to use
     * @param angle defines the angle to use
     * @param result defines the target quaternion
     * @returns the target quaternion
     */
    public static RotationAxisToRef<T extends Quaternion>(axis: DeepImmutable<Vector3>, angle: number, result: T): T {
        result._w = Math.cos(angle / 2);
        const sinByLength = Math.sin(angle / 2) / axis.length();
        result._x = axis._x * sinByLength;
        result._y = axis._y * sinByLength;
        result._z = axis._z * sinByLength;
        result._isDirty = true;
        return result;
    }

I’ve had a look at the 16 occurrences of .normalize() in the source file. Many of them are applied to MathTmp vectors, which should be harmless. Other cases are apparently intended. But these cases look buggy as well:

Regards,
Heribert

Could you change the parameter when calling RotationAxis() from “axis” to “axis.clone()” and acheive the same thing?

Yes, the problem can be worked around (as expected) by passing axis.clone() to BABYLON.Quaternion.RotationAxis(...). See here:

So in theory one could leave the code as is and add a warning to the documentation that the method modifies its parameter. But I do not think that this is a good idea.

BTW, I’m already working on a PR. Shouldn’t be that much effort.

Here’s the PR:

The PR has been built and the playground example looks good:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/15381/merge/index.html#A7MRX4#2

From a performance standpoint, perhaps no normalization would be best? I suppose, though, many would miss that and increase confusion when a non-normalized axis is used. Though I think there are other methods that require pre-normalized inputs.

Yes, requiring the axis to be pre-normalized would definitely make things more efficient. But unfortunately this would be a breaking change for existing code passing in an axis that is not normalized.

At least my solution is probably slightly more efficient as it performs only one division by the axis length whereas the old code performs 3 divisions (one per coordinate).

1 Like

At the end of my initial post I mentioned three more places with similar issues.
For the first and second issue (in rotation-related code) I have submitted another pull request:

For the reflection code I didn’t submit a fix because

  • I have too little understanding of planes in BabylonJS and
  • I got the impression that normalizing a plane might be less harmful than normalizing a vector.
1 Like