Skip to content

Add reflect conversions #18757

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
Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add reflect conversions #18757

wants to merge 5 commits into from

Conversation

Person-93
Copy link
Contributor

Objective

Solution

  • Add container attribute
  • Add field attribute
  • Use attributes to try more things in derived implementations of FromReflect::from_reflect

Testing

  • Add new example that uses the conversions

@Person-93 Person-93 force-pushed the feature/reflect-conversions branch 5 times, most recently from d7f2a38 to dd18684 Compare April 8, 2025 01:08
@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Reflection Runtime information about types S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Apr 8, 2025
@alice-i-cecile alice-i-cecile requested a review from MrGVSV April 8, 2025 20:58
@MrGVSV MrGVSV mentioned this pull request Apr 10, 2025
Open
MrGVSV
MrGVSV reviewed Apr 27, 2025
View reviewed changes
Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

This looks really good so far! Just a few comments.

I'm planning to do a major rewrite to FromReflect, and I think your changes here provide a really solid basis for that :)

@Person-93 Person-93 force-pushed the feature/reflect-conversions branch 5 times, most recently from a88df1d to e362bf9 Compare May 1, 2025 12:33
@Person-93 Person-93 force-pushed the feature/reflect-conversions branch from e362bf9 to 92a3361 Compare May 2, 2025 16:06
@Person-93
Copy link
Contributor Author

I removed the example because changes to Cargo.toml will cause the build to fail until the advisory is dealt with or the Dependencies workflow is changed. #19010

@Person-93 Person-93 requested a review from MrGVSV May 2, 2025 18:23
@mockersf
Copy link
Member

mockersf commented May 2, 2025

I removed the example because changes to Cargo.toml will cause the build to fail until the advisory is dealt with or the Dependencies workflow is changed. #19010

You don't need to do that, the dependencies workflow is not required and isn't blocking when merging

@Person-93 Person-93 force-pushed the feature/reflect-conversions branch from f799ffb to 92a3361 Compare May 2, 2025 18:34
@Person-93
Copy link
Contributor Author

@MrGVSV are you still able to review this? The failing check is non-blocking.

@MrGVSV
Copy link
Member

MrGVSV commented May 14, 2025

@MrGVSV are you still able to review this? The failing check is non-blocking.

Yeah sorry I had it open a couple days ago but got a bit too busy. I'll try to re-review this week though!

MrGVSV
MrGVSV reviewed May 20, 2025
View reviewed changes
Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

Thank you for your patience! Just a few comments/suggestions

crates/bevy_reflect/derive/src/from_reflect.rs
@@ -27,14 +27,21 @@ pub(crate) fn impl_opaque(meta: &ReflectMeta) -> proc_macro2::TokenStream {
let bevy_reflect_path = meta.bevy_reflect_path();
let (impl_generics, ty_generics, where_clause) = type_path.generics().split_for_impl();
let where_from_reflect_clause = WhereClauseOptions::new(meta).extend_where_clause(where_clause);

let exact_match = quote! {
if let #FQOption::Some(value) = <dyn #bevy_reflect_path::PartialReflect>::try_downcast_ref::<#type_path #ty_generics>(reflect) {
Copy link
Member

Choose a reason for hiding this comment

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

I recently had to fix this function to properly account for remote types in #19158. Might be worth adding that here or waiting for that PR to be merged.

crates/bevy_reflect/derive/src/from_reflect.rs
Comment on lines +332 to +333
}
else {
Copy link
Member

Choose a reason for hiding this comment

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

Nit:

Suggested change
}
else {
} else {

crates/bevy_reflect/derive/src/from_reflect.rs
let some_branch = if conversions.is_empty() {
quote! { #value }
} else {
quote! { #(#conversions)else* else { #value } }
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Does #(#conversions else)* { #value } work? Because not only would that remove the else duplication, but might allow you to do away with some_branch altogether.

examples/reflection/convert.rs
}

/// Deriving `Reflect` implements the relevant reflection traits. In this case, it implements the
/// `Reflect` trait the `Struct` trait, and the `FromReflect` trait. `derive(Reflect)` assumes that
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// `Reflect` trait the `Struct` trait, and the `FromReflect` trait. `derive(Reflect)` assumes that
/// `Reflect` trait, the `Struct` trait, and the `FromReflect` trait. `derive(Reflect)` assumes that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrGVSV MrGVSV MrGVSV left review comments

Assignees
No one assigned
Labels
A-Reflection Runtime information about types C-Feature A new feature, making something new possible S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Person-93 @mockersf @MrGVSV @alice-i-cecile