[Merged by Bors] - Add FromReflect trait to convert dynamic types to concrete types#1395
[Merged by Bors] - Add FromReflect trait to convert dynamic types to concrete types#1395Davier wants to merge 4 commits intobevyengine:mainfrom
Conversation
alice-i-cecile
added
core
S-Ready-For-Final-Review
|
This is definitely worth considering, but its also a pretty big change to the "scene/entity" lifecycle so I'll need to spend some time considering the design. I'll get to this as soon as I can, but I probably won't prioritize this for the 0.5 release. |
alice-i-cecile
removed
the
S-Ready-For-Final-Review
cart
added
the
S-Pre-Relicense
|
I updated the PR and trimmed it down, I hope it feels more like a small improvement to fix an issue and less like a big change to the "scene/entity" lifecycle :-) What I kept
What I removed
|
alice-i-cecile
removed
the
S-Pre-Relicense
|
This PR was discussed here: https://discord.com/channels/691052431525675048/745805740274614303/869276523503435817 I think the main point was: "Reflect should be derivable for every type, including those which cannot be constructed from a combination of FromReflect and Default impls" (from cart) Suggested changes are:
For future reference, I wanted to use FromReflect in ReflectComponent because I had several cases where a Component could not correctly implement Default/FromWorld. For instance, components that contain an Entity must implement Default with a fake Entity in order to be reflected (see Parent). |
|
Have you seen the technique used in #2491? Perhaps we could use it here as well, to get elegant handling of the various initialization methods. |
I don't think so unfortunately, there is no wrapping type like After thinking a bit more about it, introducing |
|
Here is how the alternative looks like: Davier@e621fcc |
Other users structs containing Then, we could change Last time this came up, there was some frustration that we can't assign |
|
I hate having to handle the invalid |
|
I reverted the modifications to the reflection example, since Also, should I do the refactoring to deduplicate the code between |
alice-i-cecile
added
the
S-Needs-Design-Doc
|
As promised a while ago, here is a summary of this PR. The problemsThis PR aims to solve two issues. Issue 1: Reflecting a component requires that it implements
|
cart
left a comment
cart
left a comment
There was a problem hiding this comment.
Ok so I like FromReflect / it seems like a relatively uncontroversial addition to the reflection toolset. I'm also convinced that using FromReflect in collections is a good solution in the short term. Just one small nit and I think we should merge this!
|
bors r+ |
Dynamic types (`DynamicStruct`, `DynamicTupleStruct`, `DynamicTuple`, `DynamicList` and `DynamicMap`) are used when deserializing scenes, but currently they can only be applied to existing concrete types. This leads to issues when trying to spawn non trivial deserialized scene. For components, the issue is avoided by requiring that reflected components implement ~~`FromResources`~~ `FromWorld` (or `Default`). When spawning, a new concrete type is created that way, and the dynamic type is applied to it. Unfortunately, some components don't have any valid implementation of these traits. In addition, any `Vec` or `HashMap` inside a component will panic when a dynamic type is pushed into it (for instance, `Text` panics when adding a text section). To solve this issue, this PR adds the `FromReflect` trait that creates a concrete type from a dynamic type that represent it, derives the trait alongside the `Reflect` trait, drops the ~~`FromResources`~~ `FromWorld` requirement on reflected components, ~~and enables reflection for UI and Text bundles~~. It also adds the requirement that fields ignored with `#[reflect(ignore)]` implement `Default`, since we need to initialize them somehow. Co-authored-by: Carter Anderson <mcanders1@gmail.com>
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
…4140) # Objective Currently, `FromReflect` makes a couple assumptions: * Ignored fields must implement `Default` * Active fields must implement `FromReflect` * The reflected must be fully populated for active fields (can't use an empty `DynamicStruct`) However, one or both of these requirements might be unachievable, such as for external types. In these cases, it might be nice to tell `FromReflect` to use a custom default. ## Solution Added the `#[reflect(default)]` derive helper attribute. This attribute can be applied to any field (ignored or not) and will allow a default value to be specified in place of the regular `from_reflect()` call. It takes two forms: `#[reflect(default)]` and `#[reflect(default = "some_func")]`. The former specifies that `Default::default()` should be used while the latter specifies that `some_func()` should be used. This is pretty much [how serde does it](https://serde.rs/field-attrs.html#default). ### Example ```rust #[derive(Reflect, FromReflect)] struct MyStruct { // Use `Default::default()` #[reflect(default)] foo: String, // Use `get_bar_default()` #[reflect(default = "get_bar_default")] #[reflect(ignore)] bar: usize, } fn get_bar_default() -> usize { 123 } ``` ### Active Fields As an added benefit, this also allows active fields to be completely missing from their dynamic object. This is because the attribute tells `FromReflect` how to handle missing active fields (it still tries to use `from_reflect` first so the `FromReflect` trait is still required). ```rust let dyn_struct = DynamicStruct::default(); // We can do this without actually including the active fields since they have `#[reflect(default)]` let my_struct = <MyStruct as FromReflect>::from_reflect(&dyn_struct); ``` ### Container Defaults Also, with the addition of #3733, people will likely start adding `#[reflect(Default)]` to their types now. Just like with the fields, we can use this to mark the entire container as "defaultable". This grants us the ability to completely remove the field markers altogether if our type implements `Default` (and we're okay with fields using that instead of their own `Default` impls): ```rust #[derive(Reflect, FromReflect)] #[reflect(Default)] struct MyStruct { foo: String, #[reflect(ignore)] bar: usize, } impl Default for MyStruct { fn default() -> Self { Self { foo: String::from("Hello"), bar: 123, } } } // Again, we can now construct this from nothing pretty much let dyn_struct = DynamicStruct::default(); let my_struct = <MyStruct as FromReflect>::from_reflect(&dyn_struct); ``` Now if _any_ field is missing when using `FromReflect`, we simply fallback onto the container's `Default` implementation. This behavior can be completely overridden on a per-field basis, of course, by simply defining those same field attributes like before. ### Related * #3733 * #1395 * #2377 --- ## Changelog * Added `#[reflect(default)]` field attribute for `FromReflect` * Allows missing fields to be given a default value when using `FromReflect` * `#[reflect(default)]` - Use the field's `Default` implementation * `#[reflect(default = "some_fn")]` - Use a custom function to get the default value * Allow `#[reflect(Default)]` to have a secondary usage as a container attribute * Allows missing fields to be given a default value based on the container's `Default` impl when using `FromReflect` Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
…4140) # Objective Currently, `FromReflect` makes a couple assumptions: * Ignored fields must implement `Default` * Active fields must implement `FromReflect` * The reflected must be fully populated for active fields (can't use an empty `DynamicStruct`) However, one or both of these requirements might be unachievable, such as for external types. In these cases, it might be nice to tell `FromReflect` to use a custom default. ## Solution Added the `#[reflect(default)]` derive helper attribute. This attribute can be applied to any field (ignored or not) and will allow a default value to be specified in place of the regular `from_reflect()` call. It takes two forms: `#[reflect(default)]` and `#[reflect(default = "some_func")]`. The former specifies that `Default::default()` should be used while the latter specifies that `some_func()` should be used. This is pretty much [how serde does it](https://serde.rs/field-attrs.html#default). ### Example ```rust #[derive(Reflect, FromReflect)] struct MyStruct { // Use `Default::default()` #[reflect(default)] foo: String, // Use `get_bar_default()` #[reflect(default = "get_bar_default")] #[reflect(ignore)] bar: usize, } fn get_bar_default() -> usize { 123 } ``` ### Active Fields As an added benefit, this also allows active fields to be completely missing from their dynamic object. This is because the attribute tells `FromReflect` how to handle missing active fields (it still tries to use `from_reflect` first so the `FromReflect` trait is still required). ```rust let dyn_struct = DynamicStruct::default(); // We can do this without actually including the active fields since they have `#[reflect(default)]` let my_struct = <MyStruct as FromReflect>::from_reflect(&dyn_struct); ``` ### Container Defaults Also, with the addition of #3733, people will likely start adding `#[reflect(Default)]` to their types now. Just like with the fields, we can use this to mark the entire container as "defaultable". This grants us the ability to completely remove the field markers altogether if our type implements `Default` (and we're okay with fields using that instead of their own `Default` impls): ```rust #[derive(Reflect, FromReflect)] #[reflect(Default)] struct MyStruct { foo: String, #[reflect(ignore)] bar: usize, } impl Default for MyStruct { fn default() -> Self { Self { foo: String::from("Hello"), bar: 123, } } } // Again, we can now construct this from nothing pretty much let dyn_struct = DynamicStruct::default(); let my_struct = <MyStruct as FromReflect>::from_reflect(&dyn_struct); ``` Now if _any_ field is missing when using `FromReflect`, we simply fallback onto the container's `Default` implementation. This behavior can be completely overridden on a per-field basis, of course, by simply defining those same field attributes like before. ### Related * #3733 * #1395 * #2377 --- ## Changelog * Added `#[reflect(default)]` field attribute for `FromReflect` * Allows missing fields to be given a default value when using `FromReflect` * `#[reflect(default)]` - Use the field's `Default` implementation * `#[reflect(default = "some_fn")]` - Use a custom function to get the default value * Allow `#[reflect(Default)]` to have a secondary usage as a container attribute * Allows missing fields to be given a default value based on the container's `Default` impl when using `FromReflect` Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
…evyengine#4140) # Objective Currently, `FromReflect` makes a couple assumptions: * Ignored fields must implement `Default` * Active fields must implement `FromReflect` * The reflected must be fully populated for active fields (can't use an empty `DynamicStruct`) However, one or both of these requirements might be unachievable, such as for external types. In these cases, it might be nice to tell `FromReflect` to use a custom default. ## Solution Added the `#[reflect(default)]` derive helper attribute. This attribute can be applied to any field (ignored or not) and will allow a default value to be specified in place of the regular `from_reflect()` call. It takes two forms: `#[reflect(default)]` and `#[reflect(default = "some_func")]`. The former specifies that `Default::default()` should be used while the latter specifies that `some_func()` should be used. This is pretty much [how serde does it](https://serde.rs/field-attrs.html#default). ### Example ```rust #[derive(Reflect, FromReflect)] struct MyStruct { // Use `Default::default()` #[reflect(default)] foo: String, // Use `get_bar_default()` #[reflect(default = "get_bar_default")] #[reflect(ignore)] bar: usize, } fn get_bar_default() -> usize { 123 } ``` ### Active Fields As an added benefit, this also allows active fields to be completely missing from their dynamic object. This is because the attribute tells `FromReflect` how to handle missing active fields (it still tries to use `from_reflect` first so the `FromReflect` trait is still required). ```rust let dyn_struct = DynamicStruct::default(); // We can do this without actually including the active fields since they have `#[reflect(default)]` let my_struct = <MyStruct as FromReflect>::from_reflect(&dyn_struct); ``` ### Container Defaults Also, with the addition of bevyengine#3733, people will likely start adding `#[reflect(Default)]` to their types now. Just like with the fields, we can use this to mark the entire container as "defaultable". This grants us the ability to completely remove the field markers altogether if our type implements `Default` (and we're okay with fields using that instead of their own `Default` impls): ```rust #[derive(Reflect, FromReflect)] #[reflect(Default)] struct MyStruct { foo: String, #[reflect(ignore)] bar: usize, } impl Default for MyStruct { fn default() -> Self { Self { foo: String::from("Hello"), bar: 123, } } } // Again, we can now construct this from nothing pretty much let dyn_struct = DynamicStruct::default(); let my_struct = <MyStruct as FromReflect>::from_reflect(&dyn_struct); ``` Now if _any_ field is missing when using `FromReflect`, we simply fallback onto the container's `Default` implementation. This behavior can be completely overridden on a per-field basis, of course, by simply defining those same field attributes like before. ### Related * bevyengine#3733 * bevyengine#1395 * bevyengine#2377 --- ## Changelog * Added `#[reflect(default)]` field attribute for `FromReflect` * Allows missing fields to be given a default value when using `FromReflect` * `#[reflect(default)]` - Use the field's `Default` implementation * `#[reflect(default = "some_fn")]` - Use a custom function to get the default value * Allow `#[reflect(Default)]` to have a secondary usage as a container attribute * Allows missing fields to be given a default value based on the container's `Default` impl when using `FromReflect` Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
…evyengine#4140) # Objective Currently, `FromReflect` makes a couple assumptions: * Ignored fields must implement `Default` * Active fields must implement `FromReflect` * The reflected must be fully populated for active fields (can't use an empty `DynamicStruct`) However, one or both of these requirements might be unachievable, such as for external types. In these cases, it might be nice to tell `FromReflect` to use a custom default. ## Solution Added the `#[reflect(default)]` derive helper attribute. This attribute can be applied to any field (ignored or not) and will allow a default value to be specified in place of the regular `from_reflect()` call. It takes two forms: `#[reflect(default)]` and `#[reflect(default = "some_func")]`. The former specifies that `Default::default()` should be used while the latter specifies that `some_func()` should be used. This is pretty much [how serde does it](https://serde.rs/field-attrs.html#default). ### Example ```rust #[derive(Reflect, FromReflect)] struct MyStruct { // Use `Default::default()` #[reflect(default)] foo: String, // Use `get_bar_default()` #[reflect(default = "get_bar_default")] #[reflect(ignore)] bar: usize, } fn get_bar_default() -> usize { 123 } ``` ### Active Fields As an added benefit, this also allows active fields to be completely missing from their dynamic object. This is because the attribute tells `FromReflect` how to handle missing active fields (it still tries to use `from_reflect` first so the `FromReflect` trait is still required). ```rust let dyn_struct = DynamicStruct::default(); // We can do this without actually including the active fields since they have `#[reflect(default)]` let my_struct = <MyStruct as FromReflect>::from_reflect(&dyn_struct); ``` ### Container Defaults Also, with the addition of bevyengine#3733, people will likely start adding `#[reflect(Default)]` to their types now. Just like with the fields, we can use this to mark the entire container as "defaultable". This grants us the ability to completely remove the field markers altogether if our type implements `Default` (and we're okay with fields using that instead of their own `Default` impls): ```rust #[derive(Reflect, FromReflect)] #[reflect(Default)] struct MyStruct { foo: String, #[reflect(ignore)] bar: usize, } impl Default for MyStruct { fn default() -> Self { Self { foo: String::from("Hello"), bar: 123, } } } // Again, we can now construct this from nothing pretty much let dyn_struct = DynamicStruct::default(); let my_struct = <MyStruct as FromReflect>::from_reflect(&dyn_struct); ``` Now if _any_ field is missing when using `FromReflect`, we simply fallback onto the container's `Default` implementation. This behavior can be completely overridden on a per-field basis, of course, by simply defining those same field attributes like before. ### Related * bevyengine#3733 * bevyengine#1395 * bevyengine#2377 --- ## Changelog * Added `#[reflect(default)]` field attribute for `FromReflect` * Allows missing fields to be given a default value when using `FromReflect` * `#[reflect(default)]` - Use the field's `Default` implementation * `#[reflect(default = "some_fn")]` - Use a custom function to get the default value * Allow `#[reflect(Default)]` to have a secondary usage as a container attribute * Allows missing fields to be given a default value based on the container's `Default` impl when using `FromReflect` Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
5 participants
Dynamic types (
DynamicStruct,DynamicTupleStruct,DynamicTuple,DynamicListandDynamicMap) are used when deserializing scenes, but currently they can only be applied to existing concrete types. This leads to issues when trying to spawn non trivial deserialized scene.For components, the issue is avoided by requiring that reflected components implement
FromResourcesFromWorld(orDefault). When spawning, a new concrete type is created that way, and the dynamic type is applied to it. Unfortunately, some components don't have any valid implementation of these traits.In addition, any
VecorHashMapinside a component will panic when a dynamic type is pushed into it (for instance,Textpanics when adding a text section).To solve this issue, this PR adds the
FromReflecttrait that creates a concrete type from a dynamic type that represent it, derives the trait alongside theReflecttrait, drops theFromResourcesFromWorldrequirement on reflected components,and enables reflection for UI and Text bundles. It also adds the requirement that fields ignored with#[reflect(ignore)]implementDefault, since we need to initialize them somehow.