pmndrs / use-cannon

👋💣 physics based hooks for @react-three/fiber
https://cannon.pmnd.rs
2.76k stars 155 forks source link

Initial body rotation and api.rotation are inconsistent about Euler angle order. #286

Closed RobRendell closed 3 years ago

RobRendell commented 3 years ago

After some investigation, it turns out that cannon-es has inconsistent defaults for converting Quaternions to and from Euler angles.

The web worker for @react-three/cannon calls these functions without explicit order parameters, so it gets the defaults.

This means that initial rotations passed to useBody (and descendents) and calls to api.rotation.set are in XYZ order, but the callback from api.rotation.subscribe receives a triple of numbers in YZX order. The rotation received from api.rotation.subscribe is thus unsuitable to use to set the rotation on a different body without conversion.

At the very least, this should be documented in the README with large warning symbols. Ideally it should be made consistent, but given cannon-es' inability to convert to anything other than YZX order, the easiest fix would be to treat all Euler angles as YZX, which would be a breaking change for lots of user code that doesn't care about api.rotation.subscribe but does set rotations initially or on the fly.

Another approach to address this would be to give users the option of providing a quaternion argument to useBody as an alternative to rotation, and expose an api.quaternion object which can be set and subscribed to, bypassing the Euler conversions. Since cannon-es uses quaternions internally rather than Euler angles, this would also be safer, avoiding errors that might accrue due to converting to/from Euler angles.

bjornstar commented 3 years ago

We should fix the arguments to be in XYZ order in the subscribe callback on rotation.

I'd be happy to expose quaternion directly as well, it makes sense that you should be able to use it.

bjornstar commented 3 years ago

It would be great to have some kind of example code using this. Do you have a codesandbox I can look at to make sure I fix it correctly?

RobRendell commented 3 years ago

Hi! Yes, I posted this one to #react-three--cannon on the Poimandres discord: https://codesandbox.io/s/bug-demo-create-object-with-copied-position-rotation-ggd2b?file=/src/App.js

After identifying the underlying issue, I modified it by adding a convertYZXtoXYZ function to demonstrate the issue went away. If you fork it, you can remove the call to convertYZXtoXYZ to see the problem, and then could confirm the issue was gone after your fix.

However, you're either going to have to do that sort of on-the-fly conversion in the @react-three/cannon code, or make a pull request for cannon-es, because as noted, it throws an exception if you pass any order other than 'YZX' to Quaternion.toEuler

I'm looking forward to direct quaternion access! :) Thanks!

bjornstar commented 3 years ago

Thank you for the example, I went ahead and adapted it into the examples.

I added a quaternion API and use Euler.setFromQuaternion to get the correct order. The inconsistency with the rotation part of the Vector API was bugging me so I was happy to make this change.

RobRendell commented 3 years ago

Great, thanks! I look forward to the next release :)