[Merged by Bors] - bevy_reflect: Get owned fields by MrGVSV · Pull Request #5728 · bevyengine/bevy

MrGVSV · 2022-08-18T01:54:09Z

Objective

Sometimes it's useful to be able to retrieve all the fields of a container type so that they may be processed separately. With reflection, however, we typically only have access to references.

The only alternative is to "clone" the value using Reflect::clone_value. This, however, returns a Dynamic type in most cases. The solution there would be to use FromReflect instead, but this also has a problem in that it means we need to add FromReflect as an additional bound.

Solution

Add a drain method to all container traits. This returns a Vec<Box<dyn Reflect>> (except for Map which returns Vec<(Box<dyn Reflect>, Box<dyn Reflect>)>).

This allows us to do things a lot simpler. For example, if we finished processing a struct and just need a particular value:

// === OLD === //
/// May or may not return a Dynamic*** value (even if `container` wasn't a `DynamicList`)
fn get_output(container: Box<dyn List>, output_index: usize) -> Box<dyn Reflect> {
  container.get(output_index).unwrap().clone_value()
}

// === NEW === //
/// Returns _exactly_ whatever was in the given list
fn get_output(container: Box<dyn List>, output_index: usize) -> Box<dyn Reflect> {
  container.drain().remove(output_index).unwrap()
}

Discussion

Is drain the best method name? It makes sense that it "drains" all the fields and that it consumes the container in the process, but I'm open to alternatives.

Changelog

Added a drain method to the following traits:
- ~~Struct~~
- ~~TupleStruct~~
- Tuple
- Array
- List
- Map
- ~~Enum~~

Changes from initial PR state

Removed drain methods on Struct, TupleStruct, and Enum types as they lacked specific use-cases to validate their inclusion in this PR. They may be added back in at some point in the future.

cart

I'm on board! Especially for "collection" types like Lists / Arrays / Maps etc.

My only reason to not do this for structs and enums is that it adds more per-type code-gen for something that could be pretty niche. Do you have real use cases for those scenarios? If not, I think we should hold off until absolutely necessary, in the interest of preserving compile time.

MrGVSV · 2022-08-30T06:07:41Z

Do you have real use cases for those scenarios?

I don't really have any actual scenarios. I was just looking at the Struct trait and realized that there was no way for me to get an owned value back from it.

Here's a rather contrived example of when this could be useful:

#[derive(Reflect, FromReflect, Clone, PartialEq, Debug)]
struct Data {
  #[reflect(ignore)]
  value: String
}

#[derive(Reflect, FromReflect, Clone, PartialEq)]
struct SomeStruct {
  data: Data
}

let expected = SomeStruct {
  data: Data {
    value: String::from("Hello World")
  }
};

let dyn_struct: Box<dyn Struct> = Box::new(expected.clone());
// 1.
let field_cloned: Box<dyn Reflect> = dyn_struct.field("data").unwrap().clone_value();
// 2.
let field: Data = <Data as FromReflect>::from_reflect(field_cloned.as_ref()).unwrap();

assert_eq!(expected.data, field); // <- Panic!
// thread 'blah' panicked at 'assertion failed: `(left == right)`
//  left: `Data { value: "Hello World" }`,
// right: `Data { value: "" }`',

This cause of the failure here is due to ignoring Data::value. This causes a problem when we get to the two labeled steps that we need to perform in order to get the owned value.

We call Reflect::clone_value. Because Data is a struct, the returned Box<dyn Reflect> is actually a Box<DynamicStruct>. This is why we need to include step 2.
We then call FromReflect::from_reflect to get back the actual type. Because we ignored Data::value, the derived FromReflect implementation has no idea how to clone that value. So it defaults to using Default::default() to generate the ignored field.

This is why we end up with a blank string and a panicked assertion.

By using something like drain, we can actually take the value directly. No cloning or FromReflect-ing involved.

let dyn_struct: Box<dyn Struct> = Box::new(expected.clone());
let field: Data = dyn_struct.drain().pop().unwrap().take::<Data>().unwrap();
assert_eq!(expected.data, field);

Now, I say this is a contrived example since we (1) already know and have the concrete type of the data we want and (2) we could simply update FromReflect to be smarter (maybe using a #[reflect(cloneable)] attribute).

But yeah— no real-world example on hand. I do see that the compile-time cost could make this part of the PR not really worth it right now (or at least in need of more discussion). I can remove the changes on structs, tuple structs, and enums for the time being.

cart

Cool cool. I see the theoretical value of the struct/enum impls. Happy to re-consider if the need ever shows up, but I appreciate that you removed them in the short term.

This looks good to me!

cart · 2022-08-30T21:15:02Z

bors r+

# Objective Sometimes it's useful to be able to retrieve all the fields of a container type so that they may be processed separately. With reflection, however, we typically only have access to references. The only alternative is to "clone" the value using `Reflect::clone_value`. This, however, returns a Dynamic type in most cases. The solution there would be to use `FromReflect` instead, but this also has a problem in that it means we need to add `FromReflect` as an additional bound. ## Solution Add a `drain` method to all container traits. This returns a `Vec<Box<dyn Reflect>>` (except for `Map` which returns `Vec<(Box<dyn Reflect>, Box<dyn Reflect>)>`). This allows us to do things a lot simpler. For example, if we finished processing a struct and just need a particular value: ```rust // === OLD === // /// May or may not return a Dynamic*** value (even if `container` wasn't a `DynamicStruct`) fn get_output(container: Box<dyn Struct>, output_index: usize) -> Box<dyn Reflect> { container.field_at(output_index).unwrap().clone_value() } // === NEW === // /// Returns _exactly_ whatever was in the given struct fn get_output(container: Box<dyn Struct>, output_index: usize) -> Box<dyn Reflect> { container.drain().remove(output_index).unwrap() } ``` ### Discussion * Is `drain` the best method name? It makes sense that it "drains" all the fields and that it consumes the container in the process, but I'm open to alternatives. --- ## Changelog * Added a `drain` method to the following traits: * `Struct` * `TupleStruct` * `Tuple` * `Array` * `List` * `Map` * `Enum`

bors · 2022-08-30T21:39:41Z

Pull request successfully merged into main.

Build succeeded:

# Objective Sometimes it's useful to be able to retrieve all the fields of a container type so that they may be processed separately. With reflection, however, we typically only have access to references. The only alternative is to "clone" the value using `Reflect::clone_value`. This, however, returns a Dynamic type in most cases. The solution there would be to use `FromReflect` instead, but this also has a problem in that it means we need to add `FromReflect` as an additional bound. ## Solution Add a `drain` method to all container traits. This returns a `Vec<Box<dyn Reflect>>` (except for `Map` which returns `Vec<(Box<dyn Reflect>, Box<dyn Reflect>)>`). This allows us to do things a lot simpler. For example, if we finished processing a struct and just need a particular value: ```rust // === OLD === // /// May or may not return a Dynamic*** value (even if `container` wasn't a `DynamicStruct`) fn get_output(container: Box<dyn Struct>, output_index: usize) -> Box<dyn Reflect> { container.field_at(output_index).unwrap().clone_value() } // === NEW === // /// Returns _exactly_ whatever was in the given struct fn get_output(container: Box<dyn Struct>, output_index: usize) -> Box<dyn Reflect> { container.drain().remove(output_index).unwrap() } ``` ### Discussion * Is `drain` the best method name? It makes sense that it "drains" all the fields and that it consumes the container in the process, but I'm open to alternatives. --- ## Changelog * Added a `drain` method to the following traits: * `Struct` * `TupleStruct` * `Tuple` * `Array` * `List` * `Map` * `Enum`

MrGVSV added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types labels Aug 18, 2022

MrGVSV force-pushed the reflect-drain branch from b62e8f8 to 5f9bae3 Compare August 18, 2022 02:04

cart reviewed Aug 30, 2022

View reviewed changes

MrGVSV added 2 commits August 29, 2022 23:10

Add a drain method to all reflect container traits

1da7c8b

Revert add drain method on structs and enums

c9c7481

MrGVSV force-pushed the reflect-drain branch from 7fb9fd4 to c9c7481 Compare August 30, 2022 06:10

cart approved these changes Aug 30, 2022

View reviewed changes

bors bot changed the title ~~bevy_reflect: Get owned fields~~ [Merged by Bors] - bevy_reflect: Get owned fields Aug 30, 2022

bors bot closed this Aug 30, 2022

MrGVSV deleted the reflect-drain branch September 19, 2022 04:20

mockersf mentioned this pull request Sep 19, 2022

Add reflection support for VecDeque #5792

Closed

Shatur mentioned this pull request Oct 30, 2022

Add FromReflect::from_reflect_owned #6419

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Merged by Bors] - bevy_reflect: Get owned fields#5728

[Merged by Bors] - bevy_reflect: Get owned fields#5728
MrGVSV wants to merge 2 commits intobevyengine:mainfrom
MrGVSV:reflect-drain

MrGVSV commented Aug 18, 2022 •

edited

Loading

Uh oh!

cart left a comment

Uh oh!

MrGVSV commented Aug 30, 2022

Uh oh!

cart left a comment

Uh oh!

cart commented Aug 30, 2022

Uh oh!

bors bot commented Aug 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

MrGVSV commented Aug 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Discussion

Changelog

Changes from initial PR state

Uh oh!

cart left a comment

Choose a reason for hiding this comment

Uh oh!

MrGVSV commented Aug 30, 2022

Uh oh!

cart left a comment

Choose a reason for hiding this comment

Uh oh!

cart commented Aug 30, 2022

Uh oh!

bors bot commented Aug 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MrGVSV commented Aug 18, 2022 •

edited

Loading