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

Move bevy_math Reflect impls #13520

Merged
merged 9 commits into from
May 27, 2024
Merged

Conversation

Olle-Lukowski
Copy link
Contributor

Objective

Fixes #13456

Solution

Moved bevy_math's Reflect impls from bevy_reflect to bevy_math.

Quick note

I accidentally used the same commit message while resolving a merge conflict (first time I had to resolve a conflict). Sorry about that.

@MrGVSV MrGVSV added A-Reflection Runtime information about types A-Math Fundamental domain-agnostic mathematical operations C-Feature A new feature, making something new possible labels May 26, 2024
crates/bevy_internal/Cargo.toml Outdated Show resolved Hide resolved
crates/bevy_math/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

We might also want to consider registering these types somewhere if they're not already. Maybe in bevy_core?

crates/bevy_math/src/cubic_splines.rs Outdated Show resolved Hide resolved
@Olle-Lukowski
Copy link
Contributor Author

Oops, forgot to actually update the code after removing the feature. my bad.

@Olle-Lukowski
Copy link
Contributor Author

Alright, ready for review now @MrGVSV @alice-i-cecile. sorry for the many issues with my commits, was very tired but wanted to finish this anyway.

@alice-i-cecile alice-i-cecile added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label May 26, 2024
@@ -3,6 +3,9 @@ use crate::{
Quat, Rotation2d, Vec2, Vec3, Vec3A,
};

#[cfg(all(feature = "serialize", feature = "bevy_reflect"))]
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 avoid glob importing here? The serialize feature really only applies to ReflectSerialize and ReflectDeserialize. Other items, like Reflect and ReflectDefault, don't require that feature. I think specifying the items manually clarifies why this particular feature gate is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sure, I'll go ahead and update it along with your other suggestions, they are all valid concerns.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you!

@@ -269,6 +281,15 @@ impl approx::UlpsEq for Dir2 {
/// A normalized vector pointing in a direction in 3D space
#[derive(Clone, Copy, Debug, PartialEq)]
#[cfg_attr(feature = "serialize", derive(serde::Serialize, serde::Deserialize))]
#[cfg_attr(
feature = "bevy_reflect",
derive(bevy_reflect::Reflect),
Copy link
Member

Choose a reason for hiding this comment

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

Nit: We can avoid the bevy_reflect:: prefix on all of these if we change the import up top to something like:

#[cfg(feature = "bevy_reflect")]
use bevy_reflect::Reflect;
#[cfg(all(feature = "bevy_reflect", feature = "serialize"))]
use bevy_reflect::{ReflectDeserialize, ReflectSerialize};

This isn't mandatory, but since we'll probably need to add a separate import for ReflectDefault anyways, we might as well.

#[cfg_attr(
feature = "bevy_reflect",
derive(bevy_reflect::Reflect),
reflect(Debug, PartialEq)
Copy link
Member

Choose a reason for hiding this comment

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

Some of these types also implement Default, which means we should probably register ReflectDefault as well:

reflect(Debug, PartialEq, Default)

@Olle-Lukowski
Copy link
Contributor Author

Also, the reason I didn't derive the traits on things like Arc2d is that it wasn't manually implemented before my changes either, so I wanted to ask it here if there was a reason for that or if that was a mistake (and should be corrected).

@mweatherley
Copy link
Contributor

Also, the reason I didn't derive the traits on things like Arc2d is that it wasn't manually implemented before my changes either, so I wanted to ask it here if there was a reason for that or if that was a mistake (and should be corrected).

I'm pretty sure that's just because those are very new and never had Reflect implemented anywhere. Adding them would be fine!

@Olle-Lukowski
Copy link
Contributor Author

Olle-Lukowski commented May 27, 2024

Also, the reason I didn't derive the traits on things like Arc2d is that it wasn't manually implemented before my changes either, so I wanted to ask it here if there was a reason for that or if that was a mistake (and should be corrected).

I'm pretty sure that's just because those are very new and never had Reflect implemented anywhere. Adding them would be fine!

Should I simply implement it for all math types? Even older types like Ray2d didn't have it yet. IMO that should be a follow up PR.

@mweatherley
Copy link
Contributor

Should I simply implement it for all math types? Even older types like Ray2d didn't have it yet. IMO that should be a follow up PR.

Doing it in a follow-up probably makes more sense, yeah.

@Olle-Lukowski
Copy link
Contributor Author

I'll open an issue for implementing the reflect traits for all math types. I'll work on it when this gets merged.

@MrGVSV MrGVSV added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 27, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 27, 2024
Merged via the queue into bevyengine:main with commit 8c7f73a May 27, 2024
32 checks passed
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 A-Reflection Runtime information about types C-Feature A new feature, making something new possible 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.

Move bevy_math's implementations of Reflect into bevy_math
4 participants