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

Add reflect impls to IRect and URect #9191

Merged
merged 2 commits into from
Jul 23, 2023
Merged

Conversation

RCoder01
Copy link
Contributor

Objective

This attempts to make the new IRect and URect structs in bevy_math more similar to the existing Rect struct.

Solution

Add reflect implementations for IRect and URect, since one already exists for Rect.

@MrGVSV MrGVSV added A-Reflection Runtime information about types A-Math Fundamental domain-agnostic mathematical operations labels Jul 18, 2023
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

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

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.

@MrGVSV MrGVSV 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 Jul 22, 2023
@james7132 james7132 added this pull request to the merge queue Jul 22, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 22, 2023
@mockersf mockersf added this pull request to the merge queue Jul 23, 2023
Merged via the queue into bevyengine:main with commit bc8e274 Jul 23, 2023
21 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 S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants