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

Quaternion rotation fix and minor documentation fix #421

Closed
wants to merge 2 commits into from

Conversation

Aztro-dev
Copy link

Previously, quaternions were limited from -PI / 2.0 to PI / 2.0, but that is now fixed and quaternions can now use all angles. Code was translated from Java to Rust using this link

I also fixed a minor documentation error where the word used was used twice on accident.

@bitshifter
Copy link
Owner

bitshifter commented Aug 1, 2023

This looks like it removes the functionality of specifying what order the Euler angles should be in?

The original contributor based the implementation on this post https://web.archive.org/web/20210410215436/http://bediyap.com/programming/convert-quaternion-to-euler-rotations/

Before someone else contributed this feature I was looking to port it from lmath, you could look at that if the current implementation is not sufficient, it would be good to know if lmath algorithm fixes your issue first though. https://github.com/AcademySoftwareFoundation/Imath/blob/main/src/Imath/ImathEuler.h

@n3vu0r
Copy link

n3vu0r commented Aug 1, 2023

You might also wanna have a look at the implementation of nalgebra. It's based on S2CID 123059499.

@Aztro-dev
Copy link
Author

Aztro-dev commented Aug 1, 2023

I'm sorry if I'm new to the codebase/concept, but what is the point of an EulerRot? To me it just seemed like it moved around where the X, Y, and Z values were in the result. I can add EulerRots to the pull request if you need.

@bitshifter
Copy link
Owner

Rotations using Euler angles give different results depending on the order that the angles are applied in, EulerRot allowed users to specify what ordering they were using, glam does not make an assumption about a particular order being used.

I'm going to close this for now. I'd suggest opening an issue describing the problem that you are trying to solve, e.g. what you expect to happen and what is actually happening so that I can better understand the issue and assess any future PR.

@bitshifter bitshifter closed this Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants