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

Fix quat packing XYZW usage #1204

Merged
merged 1 commit into from
Dec 30, 2023
Merged

Conversation

stephomi
Copy link

Round number 2 because I'm 100% sure the code is broken at the moment.

I don't care about the default by the way, my code works no matter the packing.
But OK, I'm assuming the default behaviour should be packed as XYZW and ctor as WXYZ (I guess that's the only thing people care, @xiaozhuai, @gottfriedleibniz)

The problems are still there:

  • QUAT_DATA_XYZW does nothing right now, see the code in type_quat.inl (and it does nothing in type_quat.hpp)
  • If you want to keep QUAT_DATA_XYZW then it should be renamed QUAT_CTOR_XYZW imo (also, do we really want to keep this flag)?
  • decompose is 100% broken no matter what, it reads the ctor flag (_XYZW) instead of the packing (_WXYZ)
  • same for type_quat_simd.inl

@stephomi
Copy link
Author

Unrelated but I believe it should be #if __cplusplus < 201103L or #if __cplusplus <= 201103L in hash.pp.
Currently it spams warning on modern compilers.

@christophe-lunarg
Copy link

It seems to me that accepting these PR introducing these GLM_FORCE_QUAT__ was just entering a rabbit hole... that didn't matter. Anyway, thanks for the PR, sorry for the too quick answer for the previous PR, I hope this PR brings closure of this topic but I have doubt about this..

@christophe-lunarg christophe-lunarg merged commit 8d337c0 into g-truc:master Dec 30, 2023
68 checks passed
Zuzu-Typ pushed a commit to Zuzu-Typ/glm that referenced this pull request Oct 11, 2024
Zuzu-Typ added a commit to Zuzu-Typ/glm that referenced this pull request Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants