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 (GLM_FORCE_QUAT_DATA_WXYZ and GLM_FORCE_QUAT_DATA_XYZW) #1203

Closed
wants to merge 1 commit into from

Conversation

stephomi
Copy link

By default quat used to be:

  • data packed as w, x, y, z
  • constructor as quat(w, y, y, z)
  • GLM_FORCE_QUAT_DATA_XYZW to change the behaviour of both data/ctor

Now by default it's:

  • data as x, y, z, w
  • ctor is still w, x, y, z

And there is 2 flags that conflict between each other (GLM_FORCE_QUAT_DATA_WXYZ and GLM_FORCE_QUAT_DATA_XYZW)
For example decompose doesn't read the correct flag, so you'll get broken decompose for example.

For this PR, I assumed the default needs to be x, y, z, w(?) for both data/ctor, so I kept only GLM_FORCE_QUAT_DATA_WXYZ

@christophe-lunarg
Copy link

People strongly disagree with this change so it was reverted. @xiaozhuai, @gottfriedleibniz

@gottfriedleibniz
Copy link

gottfriedleibniz commented Dec 29, 2023

Changes that were made as consequence to #1069 and #1074 should also be reverted (or fixed). The layout, constructor, and configuration of quaternions should match 0.9.9.8 behaviour and GLM_FORCE_QUAT_DATA_XYZW should not exist or be referenced at this time 1.

The issue being reported here is that: matrix_decompose and (multiple) type_quat headers still make reference to the reverted implementation. The changes to quaternion constructors was a nasty compatibility break; something stephomi also had problems with: #1076.

Footnotes

  1. Reintroducing GLM_FORCE_QUAT_DATA_XYZW as an option that controls GLM_FORCE_QUAT_DATA_WXYZ should be fine.

@stephomi
Copy link
Author

The changes to quaternion constructors was a nasty compatibility break

Technically it wasn't the constructor but rather the change in packing behaviour.
Any code that doesn't rely on algebraic access needs the ifdef check.

Do we really want an option to customize the constructor behaviour though?
I remade a PR by fixing decompose/type_quat. But also fixing/renaming GLM_FORCE_QUAT_CTOR_XYZW into GLM_FORCE_QUAT_CTOR_XYZW but not sure if it's a good idea to allow ctor change?

@gottfriedleibniz
Copy link

Technically it wasn't the constructor but rather the change in packing behaviour.

Ah - in your case certainly.

Do we really want an option to customize the constructor behaviour though?

At this time it would probably be best to match the previous implementation. Changing the constructor arguments can be saved for later (e.g., when we know things aren't broken).

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