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

Derive PartialEq, Serialize, Deserialize and Reflect on primitives #11514

Merged
merged 6 commits into from
Jan 28, 2024

Conversation

NiseVoid
Copy link
Contributor

Objective

  • Implement common traits on primitives

Solution

  • Derive PartialEq on types that were missing it.
  • Derive Copy on small types that were missing it.
  • Derive Serialize/Deserialize if the feature on bevy_math is enabled.
  • Add a lot of cursed stuff to the bevy_reflect impls module.

@NiseVoid
Copy link
Contributor Author

Deriving Serialize/Deserialize on the types with const generics isn't currently possible. But we could manually implement it with some extra code.
Reflect doesn't seem to work with Box<[T]>, we could skip the Boxed variants, manually implement reflect for Box<[T]>, or change the types to Vec<T>

Copy link
Member

Choose a reason for hiding this comment

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

Nit: Could we maybe move these files into a new math module? That way we can group it with the rect and glam modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since they're sort of behind different features, I kept glam separate from math, but grouped all the bevy_math stuff together ... We could also change the features so glam is the same feature (tho that might be really annoying if anyone uses glam and bevy_reflect, but not bevy_math outside of bevy)

@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Math Fundamental domain-agnostic mathematical operations labels Jan 25, 2024
@NiseVoid NiseVoid force-pushed the primitive_reflect_serde branch from c93b6b3 to 0c40b8e Compare January 26, 2024 14:38
@NiseVoid
Copy link
Contributor Author

NiseVoid commented Jan 27, 2024

I fixed the serialize thing on primitives with a fairly common workaround, but implementing Reflect for Box<[T]> seems a bit more complicated. If it's something we want that should probably be in a different PR later.

Also not quite sure why check-bans is failing 🤔

@NiseVoid NiseVoid marked this pull request as ready for review January 27, 2024 13:53
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

LGTM once dead code is cleaned up. It's annoying that we can't just use the derive macro, but circular dependencies are real.

@Jondolf Jondolf added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 28, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 28, 2024
Merged via the queue into bevyengine:main with commit 755917f Jan 28, 2024
26 of 27 checks passed
tjamaan pushed a commit to tjamaan/bevy that referenced this pull request Feb 6, 2024
…evyengine#11514)

# Objective

- Implement common traits on primitives

## Solution

- Derive PartialEq on types that were missing it.
- Derive Copy on small types that were missing it.
- Derive Serialize/Deserialize if the feature on bevy_math is enabled.
- Add a lot of cursed stuff to the bevy_reflect `impls` module.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Math Fundamental domain-agnostic mathematical operations C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants