Skip to content
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.

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

[Merged by Bors] - Add FromReflect trait to convert dynamic types to concrete types #1395

Closed
wants to merge 4 commits into from

Conversation

Davier
Copy link
Contributor

@Davier Davier commented Feb 4, 2021

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.

@alice-i-cecile alice-i-cecile added core S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Feb 17, 2021
@Davier Davier mentioned this pull request Feb 17, 2021
8 tasks
Base automatically changed from master to main February 19, 2021 20:44
@cart
Copy link
Member

cart commented Mar 3, 2021

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 alice-i-cecile added the A-Reflection Runtime information about types label Apr 14, 2021
@NathanSWard
Copy link
Contributor

To help track here are some related:

Issues:
#2215

PRs:
#2216 (attempts to fix)
#2359 (blocked)
#1429 (blocked)

@alice-i-cecile alice-i-cecile removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 26, 2021
@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@Davier
Copy link
Contributor Author

Davier commented Jul 25, 2021

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

  • add the FromReflect trait, which calls itself recursively on fields to construct a concrete copy of a possibly dynamic value
  • derive FromReflect along with Reflect, add a Default bound to ignored fields
  • manually implement FromReflect on SmallVec, Vec, Hashmap and Cow<'static, str>, fix List::push() on SmallVec and Vec
  • add a test to deserialize containers which items do not implement FromWorld/Default
  • change FromWorld bound to FromReflect in ReflectComponent (this could also be trimmed)

What I removed

  • reflect and enable deserialization on Text (that was my original motivation, but I will make a separate PR)
  • use FromReflect in Reflect::clone_value() implementations (I think it should be done, but we can talk about it later)

@alice-i-cecile alice-i-cecile removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 25, 2021
@Davier
Copy link
Contributor Author

Davier commented Jul 26, 2021

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:

  • don't use FromReflect in ReflectComponent
  • split FromReflect into its own derive

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).

@alice-i-cecile
Copy link
Member

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.

@Davier
Copy link
Contributor Author

Davier commented Jul 27, 2021

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 Local in this case.

After thinking a bit more about it, introducing FromReflect (especially as a separate derive) may not be worth the effort if the only remaining use is in List::push(). We might as well "fix" the issues by requiring that types inside containers that need reflection must implement Default, the same way we already require that reflected components must implement FromWorld. It's a small change that could be reverted later if we find a better solution, but we would need to impl Default for Entity because of Children, which I don't like.

@Davier
Copy link
Contributor Author

Davier commented Jul 29, 2021

Here is how the alternative looks like: Davier@e621fcc
Please tell me which way you prefer, I'd be fine with either at this point.

@alice-i-cecile
Copy link
Member

but we would need to impl Default for Entity because of Children, which I don't like.

Other users structs containing Entity are very common too. The other option here is actually to use niches to make Option<Entity> the same size as Entity. IIRC @cart and @BoxyUwU were discussing this in some other thread; a NonZero u64 would work fine for us here IMO and Option<Entity> comes up in enough other places that I want this anyways.

Then, we could change Children to store Option<Entity>, giving us the correct semantics: it starts explicitly uninitialized.

Last time this came up, there was some frustration that we can't assign None to u64::MAX and the risk of off-by-one errors that can create. But given that the alternative is to create a dummy Entity id that is only used for defaulting or bloating the storage cost of every entity-containing component we need to serialize, that very much seems like the lesser of the evils.

@Davier
Copy link
Contributor Author

Davier commented Jul 29, 2021

I hate having to handle the invalid None case everywhere even more than initializing to an invalid entity id TBH

crates/bevy_reflect/src/impls/std.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/impls/smallvec.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/impls/std.rs Outdated Show resolved Hide resolved
@Davier
Copy link
Contributor Author

Davier commented Aug 9, 2021

I reverted the modifications to the reflection example, since FromReflect will not be expected to always be derived along Reflect. Should I make a specific example deriving it for a struct that is inside a container, or is that too niche?

Also, should I do the refactoring to deduplicate the code between derive_reflect and derive_from_reflect? Or is it be better left for another PR, like #2377?

@alice-i-cecile alice-i-cecile added the S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged label Sep 22, 2021
@Davier
Copy link
Contributor Author

Davier commented Oct 26, 2021

As promised a while ago, here is a summary of this PR.

The problems

This PR aims to solve two issues.

Issue 1: Reflecting a component requires that it implements FromWorld (or Default)

The reason is an implementation detail, that I will try to explain here.

When deserializing a (complex) reflected type, bevy creates a "dynamic" version of that type using DynamicStruct or DynamicTuple, ... (Notable exception: types reflected using reflect_value are not replaced by dynamic types). Using these dynamic types is required because they can be incomplete: some fields (e.g. fields that are not reflected) may be missing from the serialized scene.
However, in order to store something in the ECS, we need the actual, concrete type. Currently, we construct a default value of that type using FromWorld, and apply the dynamic type on top of it, such that the missing fields are default initialized.

Why is it an issue? Some types do not have a valid 'default' value. The perfect example is Entity: ideally only valid entity ids should ever exist, and it needs to be reflected/serialized. Currently, the Parent component uses an entity id of u32::MAX as a default value.
The associated comment code also explains this issue:

// TODO: We need to impl either FromWorld or Default so Parent can be registered as Properties.
// This is because Properties deserialize by creating an instance and apply a patch on top.
// However Parent should only ever be set with a real user-defined entity. Its worth looking into
// better ways to handle cases like this.
impl FromWorld for Parent {
fn from_world(_world: &mut World) -> Self {
Parent(Entity::new(u32::MAX))
}
}

Besides, this process of default initialization + applying patch is also how we copy components between worlds (i.e. when spawning a scene).

TL;DR: both deserializing and spawning a scene currently needs to default-construct every component, but some components do not have a sane default.

Issue 2: Reflection of a component that has a non-empty container causes a panic #2215

The panic comes from list_apply():

pub fn list_apply<L: List>(a: &mut L, b: &dyn Reflect) {
if let ReflectRef::List(list_value) = b.reflect_ref() {
for (i, value) in list_value.iter().enumerate() {
if i < a.len() {
if let Some(v) = a.get_mut(i) {
v.apply(value);
}
} else {
List::push(a, value.clone_value());
}
}
} else {
panic!("Attempted to apply a non-list type to a list type.");
}
}

The reason is linked to the explanation of Issue 1: when a component is deserialized or copied,
we may get a dynamic type. If one field of the component is a container, the contained items may also be dynamic types.

As explained before, we default-initialize the component (with an empty container), and apply the dynamic type on it. In the case of a Vec<T>, this calls list_apply(), which tries to push every new item in the container. However, if what we have is not the concrete type T, but a dynamic one, this panic occurs.

I'm aware of two components that have containers in bevy:

  • Children: it works because Entity uses reflect_value, there are no dynamic types
  • Text: currently cannot be correctly reflected because of the sections field. Trying to deserialize any UI with text triggers the panic.

TL;DR: it's currently impossible to have containers of complex types in a scene

The solutions

Require Default for types inside reflected containers (NOT this PR)

One way to fix Issue 2 is to use the same trick as for reflecting components. We require that any type inside a reflected container implements Default. In list_apply(), if an item is a dynamic type instead of a concrete one, we default-initialize it and apply the dynamic type.

Obviously, this means that Issue 1 is now also applicable for these contained types.

Use Option<T> instead of T if a type has no sane default (NOT this PR)

A solution to Issue 1 suggested by @alice-i-cecile is to place any type that does not have a sane default in an Option, and use None as the default.

However, this means we would have to handle that invalid value everywhere. This is IMO a significant downgrade in ergonomics. Invalid values should not exist, at all.

(Note: this solution would benefit from the optimization in #3022, but is otherwise orthogonal)

Create the concrete types directly from the reflected values (this PR)

We add a way to directly construct concrete types from the reflected values. This removes an unnecessary step, along with the FromWorld/Default requirement.

Issue 1 is solved because there is no need for a FromWorld bound on components anymore.
Issue 2 is solved because we have a way to systematically construct the concrete types.

Status of the PR

This PR introduces the FromReflect trait, along with a derive macro.

pub trait FromReflect: Reflect + Sized {
    /// Creates a clone of a reflected value, converting it to a concrete type if it was a dynamic types (e.g. [DynamicStruct])
    fn from_reflect(reflect: &dyn Reflect) -> Option<Self>;
}

New constraints:

  • fields that are ignored for reflection and derive FromReflect must implement Default
  • reflected containers (e.g. Vec<T>) now have a FromReflect bound on T instead of Reflect

At first, deriving Reflect also automatically derived FromReflect, but as requested I split it into its own macro. This means there is a lot of duplicated boilerplate, but it can be merged back in a later PR if all goes well.

I was also asked to revert the FromWorld bound on components, so currently Issue 1 is not fixed by this PR.

Personally, this PR is blocking for:

Issue 2 also seem to block #2359 (see here) and #1429.

I hope this explanation is useful, and that I managed to show that the PR:

  • is not such a big change (it's now completely opt-in)
  • fixes an important bug
  • enables cool stuff

@cart cart added this to the Bevy 0.6 milestone Dec 6, 2021
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

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!

crates/bevy_reflect/bevy_reflect_derive/src/lib.rs Outdated Show resolved Hide resolved
@cart
Copy link
Member

cart commented Dec 26, 2021

bors r+

bors bot pushed a commit that referenced this pull request Dec 26, 2021
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 <[email protected]>
@bors bors bot changed the title Add FromReflect trait to convert dynamic types to concrete types [Merged by Bors] - Add FromReflect trait to convert dynamic types to concrete types Dec 26, 2021
@bors bors bot closed this Dec 26, 2021
@Davier Davier deleted the from_reflect branch December 26, 2021 19:41
bors bot pushed a commit that referenced this pull request May 30, 2022
…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 <[email protected]>
bors bot pushed a commit that referenced this pull request May 30, 2022
…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 <[email protected]>
james7132 pushed a commit to james7132/bevy that referenced this pull request Jun 7, 2022
…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 <[email protected]>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants