mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
101.74k stars 35.3k forks source link

Insufficient float accuracy in Quaternion.setFromUnitVectors #24932

Closed gromgull closed 1 year ago

gromgull commented 1 year ago

Describe the bug

After a bunch of rotations, I call `Quaternion.setFromUnitVectors on two normalised vectors, and get the wrong result.

image

because the dot product fails the if ( r < Number.EPSILON ) { test.

To Reproduce

new Quaternion().setFromUnitVectors(new Vector3(0,0,-1), new Vector3(0,0,1))
Quaternion {isQuaternion: true, _x: 0, _y: 1, _z: 0, _w: 0}

new Quaternion().setFromUnitVectors(new Vector3(0,1.1102230246251563e-16,-0.9999999999999998), new Vector3(0,0,1))
Quaternion {isQuaternion: true, _x: 0.4472135954999578, _y: -0, _z: 0, _w: 0.8944271909999159}

Live example

https://jsfiddle.net/gromgull/3bfgrj9e/ (console output only)

Expected behavior

My vectors are preeetty close to pointing in the opposite direction. I "fixed" this by using my own setFromUnitVectors utility function where I compare to EPSILON*2

In this particular case, doing <= Number.EPSILON also works, but I wanted some safety margin ;)

Platform:

Any

Mugen87 commented 1 year ago

Finding a threshold that handles all use cases "correctly" is almost impossible. Even if we double the epsilon, someone could came up with a similar request for an even greater threshold.

One possible fix is to parametrize the epsilon with Number.EPSILON as the default. However, doing this in all methods were an epsilon is used will also clutter the signatures.

gromgull commented 1 year ago

You could expose it as a parameter, like done here: https://threejs.org/docs/#examples/en/math/OBB.intersectsOBB

But I guess there's quite a few of these cases throughout threejs

In this particular case, people can't come with crazy values in though - the dot product is necessarily between -1 and 1, here it should be possible to find a threshold that works for "everyone"

gromgull commented 1 year ago

No wait - my logic is a bit wrong. Although the output value is in [-1,1] - the input values can be arbitrarily inaccurate.

WestLangley commented 1 year ago

@gromgull Your input vectors are not normalized, as required.

gromgull commented 1 year ago

@WestLangley that's a great tip! That does indeed fix it!

Interestingly, the vector passed in was from a face normal, it was previously normalized, but I have applied a quaternion to it to rotate it. I had assumed a rotated normalized vector was still normalized!

Then I will close this! Thank you so much!

WestLangley commented 1 year ago

I had assumed a rotated normalized vector was still normalized!

In theory it should still be normalized. So for computational efficiency, three.js does not normalize the result.

You apparently found a case where not doing so matters.

trepidacious commented 8 months ago

I realise this was closed since there's no "perfect" value for the tolerance, however for interest I ran into a pair of vectors that really are completely, exactly opposite but fail the test, generating an identity rotation (at least on a mac M1 in Chrome, if that makes a difference):

{"x":0.49112229567929927,"y":0.7946570131445772,"z":0.3568180518791531} {"x":-0.49112229567929927,"y":-0.7946570131445772,"z":-0.3568180518791531}

This was very unexpected - I ended up also implementing my own function that uses a larger tolerance. It does make me think - if there's a choice between having the tolerance a bit too large and a bit too small, isn't it better to have it a bit too large? If it's a little "too large", it will presumably "snap" rotations that should be extremely close to 180 degrees to exactly 180 degrees, which will be hard to notice. If it's a little too small, it will produce an identity rotation instead of a 180 degree rotation, which is extremely easy to notice (and very bizarre when you first encounter it). Give it's currently set to the smallest possible value, it does seem reasonable it could be a bit larger, even if just to cover cases like this?

The vectors popped up as the (normalized) face centers of an icosahedron.