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

euler_to_quat_lh also some euler_to_quat bugfixes #377

Merged
merged 5 commits into from
Dec 30, 2023

Conversation

telephone001
Copy link
Contributor

Creates euler_to_quat_lh
makes euler_to_quat_rh and lh struct functions
fixes up euler_to_quat documentation

made it early so I remember about it

… the lh file. Made a handed struct file so I remember to do that
…->mat4->quat tests because there is no left handed version of those. But I could try to find a way to change it
…otta decide what to name the macros for controlling lefthand and also make call functions for rh and lh conditionally declared
@telephone001
Copy link
Contributor Author

telephone001 commented Dec 28, 2023

For left handed operations in the future, do you think it will be nessesary to have CGLM_EULER_CONTROL_RH_BIT. It fits with the style but for euler, there is only left or right so we can just use CGLM_FORCE_LEFT_HANDED in the code.

#define CGLM_EULER_CONTROL_RH_BIT (1 << 0) /* LEFT_HANDED, For DirectX, Metal, Vulkan /
#define CGLM_EULER_CONTROL_LH_BIT (1 << 1) /
RIGHT_HANDED, For OpenGL, default in GLM */

#ifdef CGLM_FORCE_LEFT_HANDED
#define CGLM_CONFIG_EULER_CONTROL
#else
#define CGLM_CONFIG_EULER_CONTROL
#endif

@telephone001
Copy link
Contributor Author

telephone001 commented Dec 29, 2023

Also, the only difference between euler to quat rh vs lh is that the zsin part is negative. This might be helpful info for someone I think.
I was thinking of doing
#ifdef CGLM_FORCE_LEFT_HANDED
#define CGLM_EULER_ZS -sinf(angles[2] * 0.5f)
#else
#define CGLM_EULER_ZS sinf(angles[2] * 0.5f)
and using that to choose between lh and rh. Currently idk how to do it so there is an option for both using that.

@recp
Copy link
Owner

recp commented Dec 30, 2023

Many thanks @telephone001 for the work!

For now, I think it is enough to continue with CGLM_FORCE_LEFT_HANDED for LH/RH separation.

Also, the only difference between euler to quat rh vs lh is that the zsin part is negative. This might be helpful info for someone I think.
I was thinking of doing
#ifdef CGLM_FORCE_LEFT_HANDED
#define CGLM_EULER_ZS -sinf(angles[2] * 0.5f)
#else
#define CGLM_EULER_ZS sinf(angles[2] * 0.5f)
and using that to choose between lh and rh. Currently idk how to do it so there is an option for both using that.

that would makes it easy to maintain. But we can rewrite xyz:

  // rH
  dest[0] = xc * ys * zs + xs * yc * zc;
  dest[1] = xc * ys * zc - xs * yc * zs;
  dest[2] = xc * yc * zs + xs * ys * zc;
  dest[3] = xc * yc * zc - xs * ys * zs;

  // Lh
  dest[0] = xs * yc * zc - xc * ys * zs;
  dest[1] = xc * ys * zc + xs * yc * zs;
  dest[2] = xs * ys * zc - xc * yc * zs;
  dest[3] = xc * yc * zc + xs * ys * zs;

by keeping this part same:

  xs = sinf(angles[0] * 0.5f); xc = cosf(angles[0] * 0.5f);
  ys = sinf(angles[1] * 0.5f); yc = cosf(angles[1] * 0.5f);
  zs = sinf(angles[2] * 0.5f); zc = cosf(angles[2] * 0.5f);

not sure yet but maybe we can apply some simd on these in the future and separate implementations would make things easier. Or maybe negating single angle then applying same implementation will be better choice.

For now we can continue as your implementation. Adding Also, the only difference between euler to quat rh vs lh is that the zsin part is negative. This might be helpful info for someone I think. to somewhere (e.g. top of the header) would be helpful for the future.

I guess we are ready to merge the PR?

@telephone001
Copy link
Contributor Author

telephone001 commented Dec 30, 2023

@recp
yeah I think we can merge now

@recp recp merged commit 3a2a26e into recp:master Dec 30, 2023
49 checks passed
@recp
Copy link
Owner

recp commented Dec 30, 2023

@telephone001 thanks for your contributions, the PR is merged 🚀

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.

None yet

2 participants