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

Coordinate system agnostic #79

Closed
emilk opened this issue Sep 11, 2020 · 5 comments
Closed

Coordinate system agnostic #79

emilk opened this issue Sep 11, 2020 · 5 comments
Labels
discussion Feedback from users about this proposal is desired

Comments

@emilk
Copy link
Contributor

emilk commented Sep 11, 2020

glam is mostly agnostic to coordinate systems and handedness - and I think that's great. When a choice must be made, glam often leaves it to the user to pick, like with orthographic_lh + orthographic_rh.

There is, however, one exception to this rule: from_rotation_ypr (found in Quat, Mat3 and Mat4).

On Mat3::from_rotation_ypr the documentation simply says Creates a 3x3 rotation matrix from the given Euler angles (in radians). which is deceptive, as it implies only one way to construct such a matrix from yaw/pitch/roll.

Quat::from_rotation_ypr is better, and states: "Create a quaternion from the given yaw (around y), pitch (around x) and roll (around z) in radians.". It still neglects to mention the order. Is this Quat::from_axis_angle(Y, yaw) * Quat::from_axis_angle(X, pitch) * Quat::from_axis_angle(Z, roll) or Quat::from_axis_angle(Z, roll) * Quat::from_axis_angle(X, pitch) * Quat::from_axis_angle(Y, yaw) ?

But the deeper issue is that this implies that we all agree that +Y is up. A lot of people use +Z as up and may reach for from_rotation_ypr and not realize it won't do what they think it will.

Solutions

A few potential solutions spring to mind:

  • Remove: Just delete the function and let people explicitly chain several from_axis_angle calls like above (though less efficiently).
  • Rename: Call attention to the implied coordinate system: from_yxz_angles (or is it from_zxy_angles ?)
  • Replace: Replace with a more generic function, like from_cardinal_angles(glam::Axes::YXZ, yaw, pitch, roll)
  • Move: Hide it behind a trait, like use glam::coordinate_systems::xyz_right_up_back::Rotation;
@bitshifter
Copy link
Owner

I did want to provide something at least and in this case at the time I wrote this I didn't want to try provide a generic solution. The main reason I wanted to provide something is a lot of beginners use Euler angles and I think struggle with other methods for creating rotations.

DirectXMath is largely handedness agnostic but does provide a function for creating rotations from Euler angles - https://docs.microsoft.com/en-us/windows/win32/api/directxmath/nf-directxmath-xmquaternionrotationrollpitchyawfromvector so it seemed like a reasonable approach. Perhaps their documentation is clearer about what is going on though.

Coming up with something completely general would be nice in some ways. It would be good if a user of glam could have their chosen convention supported out of the box. I guess people can come up with an extension trait and implement their own Euler conversion functions. Most projects would want to pick one convention and stick with it. In that sense providing every possible combination might actually be worse, if there's only one people can't pick the wrong one :)

@bitshifter bitshifter added the discussion Feedback from users about this proposal is desired label Sep 17, 2020
@bitshifter
Copy link
Owner

This has been in the back of my mind for a while. I was thinking perhaps an approach would be to have a separate coordinate system crate. The idea wouldn't be to provide every option possible permutation of coordinate systems, just perhaps one or two that are in common use and it would be easy enough for people to write their own. The idea being a project uses a single coordinate system crate, importing multiple would clash because they'd define the same methods.

So the coordinate system crate could have extension methods for going to and from euler angles. They could also define constants for up, left and right. There might be other coordinate system specific stuff that could be added later.

@bitshifter
Copy link
Owner

The look_at / look_to methods currently assume y up, this would be addressed to be truly agnostic

@bitshifter
Copy link
Owner

ultraviolet has a projection module that covers common coordinate systems (also handles gl/vk/dx conventions) as an example. not sure this is the approach I want but it is coordinate system agnostic.

@bitshifter
Copy link
Owner

bitshifter commented Oct 30, 2024

Closing this in favour of #569 which is a bit more precise around the change I still want to make.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Feedback from users about this proposal is desired
Projects
None yet
Development

No branches or pull requests

2 participants