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

Quaternion memory layout changed to [x, y, z, w] #500

Merged
merged 1 commit into from
Nov 23, 2020

Conversation

aloucks
Copy link
Collaborator

@aloucks aloucks commented Apr 14, 2020

The From and Into impls for [S; 4] and (S, S, S, S) have been changed accordingly. This is consistent with other libraries like glm, nalgebra, and glam, as well as the glTF spec.

Note that the Quaternion::new constructor has not yet been updated.

Related: #499

@kvark I hadn't previously thought much about the From/Into conversions for [S; 4] and (S, S, S, S). It makes me a little nervous since this change is going to break safe code. We should probably also update the Quaternion::new constructor but I wanted to see if there was any feedback first.

The `From` and `Into` impls for `[S; 4]` and `(S, S, S, S)` have been
changed accordingly. This is consistent with other libraries like glm,
nalgebra, and glam, as well as the glTF spec.

Note that the `Quaternion::new` constructor has **not** yet been
updated.
@kvark
Copy link
Collaborator

kvark commented Apr 14, 2020

I think that relying on [S; 4] be any representation is smelly. It's intended to just be a serialization form, only compatible with cgmath itself. If the user needs a particular form, they need to build it, or rely on mint, which is the intermediate.

With this position, it doesn't feel to me that cgmath needs any changes on how slices are converted. Nobody should rely on that implicitly anyway, and if they do - we are going to break their code.

@aloucks
Copy link
Collaborator Author

aloucks commented Apr 14, 2020

With this position, it doesn't feel to me that cgmath needs any changes on how slices are converted. Nobody should rely on that implicitly anyway, and if they do - we are going to break their code.

The slice layout needs to match the struct layout. There's a bunch of unit tests that enforce this and are broken if the slice and tuple conversions aren't updated. I would find it conceptually confusing if the slice/array didn't match the in memory representation to be honest.

@kvark
Copy link
Collaborator

kvark commented Apr 14, 2020

That's a good point, I didn't realize it's repr(C).

@aloucks aloucks mentioned this pull request Nov 22, 2020
@kvark kvark merged commit 164808e into rustgd:master Nov 23, 2020
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.

2 participants