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

glam::Quat/Vec4/Mat4 support #134

Open
Athosvk opened this issue Feb 26, 2024 · 1 comment
Open

glam::Quat/Vec4/Mat4 support #134

Athosvk opened this issue Feb 26, 2024 · 1 comment
Labels
enhancement New feature or request

Comments

@Athosvk
Copy link

Athosvk commented Feb 26, 2024

Problem
Currently the glam support does not include types that are architecture dependent, e.g. Quat and Vec4. This is due to Vec4 either being a [float; 4] or m128 depending on the target architecture and glam using wrappers and using Deref to allow trivial access to each vector's components.

Unfortunately the lack of reflection supports means either skipping those fields with #[reflect(skip)], manually implementing Reflect, FromReflect and DescribeType for the type containing these fields in some way, or supporting a wrapper type around glam::Vec4 and equivalents which then requires replacements in all of the codebase. Hopefully this can start a discussion on potential ways to tackle this.

Preferred Solution
As the current comment in mirror-mirror's glam implementation states, manually re-implementing config checks and hacks that are done by glam doesn't seem great. Ideally we can still use glam's types without wrappers (within structs), whether that as Struct or maybe as Scalar (?).

I've tried implementing Struct for Vec4 and this seems to be almost possible from what I've found, with the only problem being fields_mut. fields_mut ends up requiring a borrow to each field, but Vec4 is a m128 with SIMD on which is only a single field. Even if we were to thread the land of unsafe, I'd bet it's undefined behaviour to read/write into the same m128 at different offsets simultaneously, so fields_mut would never work without potential UB. The other trait functions are trivial, though this is a manual implementation rather than just use the #[derive] syntax as one would expect.

I'd argue Vec4 as a Scalar type makes sense (probably with Vec2/3 to match), also because the (often) underlying m128 is scalar-like, but it perhaps does litter the code with [cfg]s to check if glam support should be on. Perhaps there's also limitations that I'm not aware of for Scalar types.

Alternatives
Maybe an alternative is to just provide a wrapper type for these that easily allow conversions from/to glam::Vec4/Mat4/Quat if the above options aren't really feasible.

@Athosvk Athosvk added the enhancement New feature or request label Feb 26, 2024
@fu5ha
Copy link
Member

fu5ha commented May 3, 2024

Even if we were to thread the land of unsafe, I'd bet it's undefined behaviour to read/write into the same m128 at different offsets simultaneously, so fields_mut would never work without potential UB.

Fwiw this is not true, an m128 is trivially type-punnable as a [f32; 4] and therefore also a

#[repr(C)]
struct XYZW {
    x: f32,
    y: f32,
    z: f32,
    w: f32,
}

Which are both then destructurable. That's exactly how the internal implementations of glam's Deref and DerefMut impls work

@davidpdrsn davidpdrsn mentioned this issue May 3, 2024
5 tasks
davidpdrsn added a commit that referenced this issue May 6, 2024
### Checklist

* [x] I have read the [Contributor Guide](../../CONTRIBUTING.md)
* [x] I have read and agree to the [Code of
Conduct](../../CODE_OF_CONDUCT.md)
* [x] I have added a description of my changes and why I'd like them
included in the section below

### Description of Changes

Does seem to actually be doable.

### TODO

- [x] Fix `fields_mut` for `Quat`
- [x] Fix `fields_mut` for `Mat2`

### Related Issues

#134
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants