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.

Sign up for GitHub

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

Jump to bottom

Support glam and macaw types #85

Merged
merged 4 commits into from
Jan 16, 2023
Merged

Support glam and macaw types #85

merged 4 commits into from
Jan 16, 2023

Conversation

davidpdrsn
Copy link
Contributor

Checklist

  • I have read the Contributor Guide
  • I have read and agree to the Code of Conduct
  • I have added a description of my changes and why I'd like them included in the section below

Description of Changes

This adds impls for Reflect and friends for types from glam and macaw, behind a feature flag.

@davidpdrsn davidpdrsn requested a review from bnjbvr January 12, 2023 15:56
@davidpdrsn davidpdrsn marked this pull request as draft January 13, 2023 08:19
@davidpdrsn davidpdrsn removed the request for review from bnjbvr January 13, 2023 08:20
@davidpdrsn
Copy link
Contributor Author

Can't quite merge this yet. We need some special handling of glam's types that changes depending on architecture (such as Vec4).

@davidpdrsn davidpdrsn marked this pull request as ready for review January 13, 2023 08:45
@davidpdrsn
Copy link
Contributor Author

This should be good to go now. I've tested compiling it with and without simd support for glam (using its core-simd feature) and it works. I just decided not to support the types that don't have a stable definition for now.

@davidpdrsn davidpdrsn requested a review from bnjbvr January 13, 2023 08:47
@davidpdrsn davidpdrsn enabled auto-merge (squash) January 13, 2023 08:47
bnjbvr
bnjbvr approved these changes Jan 16, 2023
View reviewed changes
Copy link
Contributor

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks! Deref magic in glam is magic.

crates/mirror-mirror/src/foreign_impls/mod.rs
@@ -80,3 +80,52 @@ __private_derive_reflect_foreign! {
end: Idx,
}
}

#[cfg(feature = "glam")]
mod glam_impls {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to put this in an glam.rs file? It's a bit weird to have it in the mod.rs module here (I did look for it in another file when trying that PR locally.)

crates/mirror-mirror/src/foreign_impls/mod.rs
}

#[cfg(feature = "macaw")]
mod macaw_impls {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto here; even if it makes for a tiny file, it's nice to have it separate from the rest of the core types imo

@davidpdrsn davidpdrsn merged commit ff26ca1 into main Jan 16, 2023
@davidpdrsn davidpdrsn deleted the glam-macaw-support branch January 16, 2023 13:46
@bnjbvr
Copy link
Contributor

bnjbvr commented Jan 16, 2023

Hah, auto-merge is fast! I'll open a PR with the suggested changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnjbvr bnjbvr bnjbvr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@davidpdrsn @bnjbvr