-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Move bevy_math
Reflect
impls
#13520
Conversation
There was a problem hiding this 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
?
Oops, forgot to actually update the code after removing the feature. my bad. |
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. |
@@ -3,6 +3,9 @@ use crate::{ | |||
Quat, Rotation2d, Vec2, Vec3, Vec3A, | |||
}; | |||
|
|||
#[cfg(all(feature = "serialize", feature = "bevy_reflect"))] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
crates/bevy_math/src/direction.rs
Outdated
@@ -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), |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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)
Also, the reason I didn't derive the traits on things like |
I'm pretty sure that's just because those are very new and never had |
Should I simply implement it for all math types? Even older types like |
Doing it in a follow-up probably makes more sense, yeah. |
I'll open an issue for implementing the reflect traits for all math types. I'll work on it when this gets merged. |
Objective
Fixes #13456
Solution
Moved
bevy_math
'sReflect
impls frombevy_reflect
tobevy_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.