Skip to content

bevy_reflect: Fix FromReflect derive for opaque remote wrappers #19158

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
Merged
merged 4 commits into from
Aug 28, 2025
Merged

bevy_reflect: Fix FromReflect derive for opaque remote wrappers #19158

merged 4 commits into from
Aug 28, 2025

Conversation

MrGVSV
Copy link
Member

@MrGVSV MrGVSV commented May 10, 2025

Objective

Fixes #19017

Solution

The FromReflect derive logic for opaque types was not properly accounting for remote reflection. This PR fixes that by making it downcast to the remote type and then converting it back to the wrapper.

Testing

You can test locally by running:

cargo test -p bevy_reflect

@MrGVSV MrGVSV force-pushed the mrgvsv/reflect/fix-opaque-remote-fromreflect branch from cbdbf8e to db00d9d Compare May 10, 2025 06:41
@MrGVSV MrGVSV added C-Bug An unexpected or incorrect behavior A-Reflection Runtime information about types D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Macros Code that generates Rust code labels May 10, 2025
@MrGVSV MrGVSV moved this from Open to In Progress in Reflection May 10, 2025
@MrGVSV MrGVSV force-pushed the mrgvsv/reflect/fix-opaque-remote-fromreflect branch from db00d9d to 6333480 Compare May 10, 2025 16:07
@MrGVSV MrGVSV mentioned this pull request May 20, 2025
Open
@alice-i-cecile alice-i-cecile added this to the 0.17 milestone Aug 17, 2025
BenjaminBrienen
BenjaminBrienen approved these changes Aug 28, 2025
View reviewed changes
Copy link
Contributor

@BenjaminBrienen BenjaminBrienen left a comment

Choose a reason for hiding this comment

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

the diff algorithm kind of fails here lol

@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 Aug 28, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 28, 2025
Merged via the queue into bevyengine:main with commit 8d33e75 Aug 28, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

+1 more reviewer

@BenjaminBrienen BenjaminBrienen BenjaminBrienen approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
A-Reflection Runtime information about types C-Bug An unexpected or incorrect behavior D-Macros Code that generates Rust code D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Reflection
Status: In Progress
Milestone
0.17
Development

Successfully merging this pull request may close these issues.

#[reflect(opaque)] remote type fails to register at runtime, making ReflectSerializer unusable
3 participants
@MrGVSV @alice-i-cecile @BenjaminBrienen