Skip to content

bevy_reflect: Add clone registrations project-wide #18307

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

Conversation

MrGVSV
Copy link
Member

@MrGVSV MrGVSV commented Mar 14, 2025

Objective

Now that #13432 has been merged, it's important we update our reflected types to properly opt into this feature. If we do not, then this could cause issues for users downstream who want to make use of reflection-based cloning.

Solution

This PR is broken into 4 commits:

  1. Add #[reflect(Clone)] on all types marked #[reflect(opaque)] that are also Clone. This is mandatory as these types would otherwise cause the cloning operation to fail for any type that contains it at any depth.

  2. Update the reflection example to suggest adding #[reflect(Clone)] on opaque types.

  3. Add #[reflect(clone)] attributes on all fields marked #[reflect(ignore)] that are also Clone. This prevents the ignored field from causing the cloning operation to fail.

    Note that some of the types that contain these fields are also Clone, and thus can be marked #[reflect(Clone)]. This makes the #[reflect(clone)] attribute redundant. However, I think it's safer to keep it marked in the case that the Clone impl/derive is ever removed. I'm open to removing them, though, if people disagree.

  4. Finally, I added #[reflect(Clone)] on all types that are also Clone. While not strictly necessary, it enables us to reduce the generated output since we can just call Clone::clone directly instead of calling PartialReflect::reflect_clone on each variant/field. It also means we benefit from any optimizations or customizations made in the Clone impl, including directly dereferencing Copy values and increasing reference counters.

    Along with that change I also took the liberty of adding any missing registrations that I saw could be applied to the type as well, such as Default, PartialEq, and Hash. There were hundreds of these to edit, though, so it's possible I missed quite a few.

That last commit is massive. There were nearly 700 types to update. So it's recommended to review the first three before moving onto that last one.

Additionally, I can break the last commit off into its own PR or into smaller PRs, but I figured this would be the easiest way of doing it (and in a timely manner since I unfortunately don't have as much time as I used to for code contributions).

Testing

You can test locally with a cargo check:

cargo check --workspace --all-features

@MrGVSV MrGVSV added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types A-Cross-Cutting Impacts the entire engine D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 14, 2025
@MrGVSV MrGVSV added this to the 0.16 milestone Mar 14, 2025
@MrGVSV MrGVSV moved this from Open to In Progress in Reflection Mar 14, 2025
Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

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

Good bit of maintenance. A Bevy CLI tool to automatically lint against missing reflected traits can't come soon enough!

@MrGVSV
Copy link
Member Author

MrGVSV commented Mar 17, 2025

Good bit of maintenance. A Bevy CLI tool to automatically lint against missing reflected traits can't come soon enough!

Yep I was wanting to work on one. We'll see if I can do that before 0.17 is out 😅

@alice-i-cecile alice-i-cecile 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 Mar 17, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 17, 2025
Merged via the queue into bevyengine:main with commit 9b32e09 Mar 17, 2025
38 checks passed
@MrGVSV MrGVSV deleted the mrgvsv/reflect/reflect-clone-on-bevy-types branch March 17, 2025 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Cross-Cutting Impacts the entire engine A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes 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.

3 participants