-
-
Notifications
You must be signed in to change notification settings - Fork 240
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
euler to quat functions #369
Conversation
…tly trying to test them. I have created all the testing functions as well
I'm thinking of:
I'd like some feedback of these changes |
…o move my tests into a single euler test because it wouldn't work outside that one test. Maybe later I will create test_euler.h like how test_quat.h works
include/cglm/euler.h
Outdated
float yc = cosf(angles[1] / 2.0f); | ||
|
||
float zs = sinf(angles[2] / 2.0f); | ||
float zc = cosf(angles[2] / 2.0f); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know new compiler and C versions can handle this but I prefer to do declarations beginning of a scope block:
float xs, xc, ys, yc, zs, zc;
xs = sinf(angles[0] * 0.5f);
also even compilers do optimizations it would be nice to multiply with 0.5 rather than dividing by 2.
@telephone001 many thanks for the work, just a reminder, as mentioned: #306 (comment) better to double check this before merge docs/euler.rst:
In the future euler api can be improved even may provide global/extrinsic too but currently as docs states we must stick above constraints. |
…lculating my quaternion rotations the opposite way (zyx order instead of xyz order)
Currently my functions work like doing glm_quat_mul 3 times with the axis vectors. But when compared to the glm_euler_by_order, all the values are negative compared to using glm_euler_by_order. I pushed just to give an update. This commit is messy because I'm in the process of debugging it and debugging tests in euler.c is harder than in quat.h. Currently i'm not really getting what extrinsic and intrinsic angles are. I based my calculations thinking that xyz corresponds to yaw pitch roll. |
https://dominicplein.medium.com/extrinsic-intrinsic-rotation-do-i-multiply-from-right-or-left-357c38c1abfd explains and shows extrinsic and intrinsic graphically (animated) which could help to understand better. Tait-Bryan vs. Classic, https://en.wikipedia.org/wiki/Euler_angles I'll check glm_euler_by_order to validate glm_euler_by_order (..., GLM_EULER_XYZ, ..) is same as glm_euler_xyz()... asap |
…ther types. I thought I changed it yesterday. Also there is still a problem with quaternion axis multiplication vs euler to mat4 to quat
Currently all my tests pass when only checking multiplication of quaternion axises. I'm pretty sure glm_euler_by_order is the same as glm_euler_xyz and its variants because it returns the same results when testing. I'm thinking the error might be in the mat4 ->quat or euler->mat4. Also, I remember not being able to make different tester functions even when I declared and entered them. I did the exact same thing I did when making the tests in testquat.h. they worked in testquat but not euler.c. Could I possibly be missing something? |
@telephone001 many thanks, I'll try to take a deeper look asap |
Based on those action logs, I suspect you should be testing |
when test_assert_quat_eq_abs used, all tests pass. But I still want the directions of my function to be right and consistent with the other glm functions |
That may be a bit difficult given the nature of Another question is whether the ordering of parameters be flipped? Out pointers (i.e., "dest") parameters are usually the last parameters to a function, e.g., |
Oh yeah I'll make the destination the last parameter if thats needed. Some of the quat functions have the output as the first parameter though when they are creating a new quaternion. |
I'm thinking of either removing the euler->mat4->quat tests and merging then dealing with mat4 in another request or making another pull request and fixing mat4 there then after its done, go back here and merge. |
I also figured out how to make the tests separate. I'm gonna be busy for a while but later I'm gonna try and make better tests for euler_xyz and possibly fix them |
@telephone001 thanks for the work. I'll also check tests. May your /* verify that it acts the same as glm_euler_by_order */
mat4 expected_mat4;
glm_euler_by_order(angles, GLM_EULER_XZY, expected_mat4);
glm_mat4_quat(expected_mat4, expected);
ASSERTIFY(test_assert_quat_eq(result, expected)); may not be valid approach, rotating a vector then comparing rotated vectors may be an option. But it would be nice to get similar quaternion by |
@recp |
@telephone001 thanks 👍 I cant validate that mat4_quat always gives short path, I'll check it when I available asap. |
@recp |
@telephone001, are we ready to merge this PR? I'll check the implementation this weekend. One question: since cglm supports LH operations too, I guess we may need to provide LH versions too e.g. One option is to implement this as RH then LH asap to prevent this become giant PR see: #322 |
@recp |
Sorry for the delay :(
That would be awesome. I think we can move LH/RH related implementations into If you dont enough time I can merge this PR and add _rh() suffixes and move them to
👍 |
Should I make euler_to_quat_rh.h and euler_to_quat_lh.h in that folder? Also instead of clipspace and handed folders, I just have 1 handed folder and put all handed stuff in that folder? |
…ons. Still deciding on what to name the macro for lefthanded stuff
currently I have a handed folder and also the rh stuff in that folder. I didn't know what to name the macro that controls righthandedness so I just used CGLM_FORCE_LEFT_HANDED |
euler_rh.h/euler_lh.h would be enough but euler_to_quat_rh.h is also ok.
using |
@telephone001 the PR is merged, many thanks for your contributions 🚀 Let's continue with separate PR for LH versions and keep track bugs and improvements for a while :) |
added all 6 combinations of glm_euler_xyz_quat. The tests work on my computer but they don't seem to work in the meson webassembly and the checks fail. I might be misreading or missing something though.