-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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 constructor of struct qua to honor macro "GLM_FORCE_QUAT_DATA_WXYZ" #1069
Fix constructor of struct qua to honor macro "GLM_FORCE_QUAT_DATA_WXYZ" #1069
Conversation
Signed-off-by: Gaoyang Zhang <gy@blurgy.xyz>
Signed-off-by: Gaoyang Zhang <gy@blurgy.xyz>
Signed-off-by: Gaoyang Zhang <gy@blurgy.xyz>
I have just run |
The idea looks great except that it changed the default behavior which will break application. Instead of GLM_FORCE_QUAT_DATA_WXYZ, I think you should create a GLM_FORCE_QUAT_DATA_XYZW to enforce your new behavior. |
This reverts commit e800c41.
This reverts commit 37842c7.
…DATA_WXYZ"" This reverts commit f931c49.
The default data layout for quat has been changed to w,x,y,z to agree with order of input parameters for quat's constructor. Signed-off-by: Gaoyang Zhang <gy@blurgy.xyz>
Signed-off-by: Gaoyang Zhang <gy@blurgy.xyz>
Thank you for your advice, this sounds more reasonable. I have made the following changes:
This keeps the constructor's behavior, but still changes the previous behavior of Another thing I have noticed is that the test file |
Thanks for contributing! |
@christophe-lunarg @blurgyy |
Erm, I agree that the default should be xyzw and that change broke compatibility... :( |
@christophe-lunarg |
Problem: C.I. is completely broken and should be upgraded to Github actions otherwise I am not really confident with releasing a new version... |
But yes I would defintely take a PR to switch the default behavior... |
I can help with CI if you like : ) |
That would be huge! If you look at:
You should be able to see the number of combinasions that were tested. At least a PR with initial github actions setup so that I can see how that works would be huge. |
I've take a look at these two files. Yes, it is. I use github actions for my own project too. |
Thanks, I'll have a look! |
Someone tried to start a actions MR for this awhile ago |
Hi @christophe-lunarg Best wishes. |
All commits related to this Otherwise,
should be implemented. |
This reverts commit 00a0581.
What
This makes the constructor of the quaternion struct
qua
behave more intuitively.Why
The original version of the constructor takes four values
(T _w, T _x, T _y, T _z)
, regardless of whether the macroGLM_FORCE_QUAT_DATA_WXYZ
is defined. While the behaviours of the operator[]
changes when the macroGLM_FORCE_QUAT_DATA_WXYZ
is defined.An illustrative example can be produced with the following code:
The above code outputs
false
without-DGLM_FORCE_QUAT_DATA_WXYZ
(becauseq1.w
is0
andq2.w
is3
), and outputstrue
with-DGLM_FORCE_QUAT_DATA_WXYZ
(expected).This behavior is explainable as long as the author of the above code has previous knowledge about the implementation of the struct
qua
, but as the macroGLM_FORCE_QUAT_DATA_WXYZ
is not defined by default, I think it's more intuitive to alter the behavior of the constructor ofqua
regarding this macro.How
To achieve this, I have changed 2 files:
glm/detail/type_quat.hpp
andglm/detail/type_quat.inl
.glm/detail/type_quat.hpp
I removed parameter names in the declaration of the constructor (for readability), because the order of them may change with respect to the macro
GLM_FORCE_QUAT_DATA_WXYZ
.glm/detail/type_quat.inl
Instead of fixing the constructor to take
(T _w, T _x, T _y, T _z)
as input, I conditioned the order of the 4 parameters with respect to the macroGLM_FORCE_QUAT_DATA_WXYZ
, and use(T _x, T _y, T _z, T _w)
as input order whenGLM_FORCE_QUAT_DATA_WXYZ
is defined.Testing
With the change proposed in this pull request, the above code example will construct two identical quaternions
q1
andq2
as expected.Notes
glm
should not be affected.glm
to break, I think it eases the uses of futureglm
users. If this commit is considered to be merged, I can add some macro to give an option to keep the old behavior for compatibility reasons.Thank you for patiently reading through this proposal!
Signed-off-by: Gaoyang Zhang gy@blurgy.xyz