[Merged by Bors] - bevy_reflect: Get owned fields

[MrGVSV]

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]

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.

1. 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.
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]

bors r+

[bors]

Pull request successfully merged into main.

Build succeeded:

• build-and-install-on-iOS
• build-android
• build (macos-latest)
• build (ubuntu-latest)
• build-wasm
• build (windows-latest)
• check-compiles
• check-doc
• check-missing-examples-in-docs
• ci
• markdownlint
• run-examples
• run-examples-on-wasm
• run-examples-on-windows-dx12