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

Add reflect impls to IRect and URect #9191

Merged
merged 2 commits into from
Jul 23, 2023
Merged

Add reflect impls to IRect and URect #9191

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4cf3468
Add reflect impls to IRect and URect
RCoder01 Jul 18, 2023
aa5f10f
Add eq/hash derive and hash reflect
RCoder01 Jul 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion crates/bevy_reflect/src/impls/rect.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,18 @@
use crate as bevy_reflect;
use crate::prelude::ReflectDefault;
use crate::{ReflectDeserialize, ReflectSerialize};
use bevy_math::{Rect, Vec2};
use bevy_math::{IRect, IVec2, Rect, URect, UVec2, Vec2};
use bevy_reflect_derive::impl_reflect_struct;

impl_reflect_struct!(
#[reflect(Debug, PartialEq, Serialize, Deserialize, Default)]
Copy link
Member

Choose a reason for hiding this comment

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

Both of these types should also add Hash to this list, since they implement Hash (unlike Rect).

Copy link
Contributor Author

@RCoder01 RCoder01 Jul 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Should Hash be put behind a #[cfg_attr(not(target_arch = "spirv"), derive(Hash))] attribute since IVec2/UVec2 have that cfg?

I just added the Hash derive but without the cfg flag because IVec2/UVec2's Debug implementation is also #[cfg_attr(not(target_arch = "spirv")], yet Debug is still derived directly on IRect/URect.

I also added Eq derives for convenience, but I couldn't add Eq to the reflect derive since the compiler complained about ReflectEq, which I couldn't find. Does there exist an Eq reflect trait?

Copy link
Member

@MrGVSV MrGVSV Jul 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I see. Yeah I would try to match the same flags that the source code defines. Actually, I don't think it's necessary right? As long as the rect types are fine I think it should be fine too. I could be wrong though.

And no, there is no ReflectEq. Eq is not treated specially by the macro either (unlike PartialEq, Debug, and Hash).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My guess is that bevy would just fail to compile for the spirv target architecture, since the Hash and Debug derive macro requirements wouldn't be satisfied. But bevy would probably already fail to compile on this arch for many other reasons, so I don't see this as a problem.

#[type_path = "bevy_math"]
struct IRect {
min: IVec2,
max: IVec2,
}
);

impl_reflect_struct!(
#[reflect(Debug, PartialEq, Serialize, Deserialize, Default)]
#[type_path = "bevy_math"]
Expand All @@ -12,3 +21,12 @@ impl_reflect_struct!(
max: Vec2,
}
);

impl_reflect_struct!(
#[reflect(Debug, PartialEq, Serialize, Deserialize, Default)]
#[type_path = "bevy_math"]
struct URect {
min: UVec2,
max: UVec2,
}
);