[Merged by Bors] - bevy_reflect: Add `#[reflect(default)]` attribute for `FromReflect` #4140

MrGVSV · 2022-03-07T10:26:36Z

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.

Example

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

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

#[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.

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

MrGVSV · 2022-05-08T01:28:40Z

With #3733 merged, I'm wondering: would also be good to make it so we use the struct's Default impl for unmarked ignored fields (if #[reflect(Default)] is defined)?

So for example, value would default to 123 instead of its usual 0 here:

#[reflect(Default)]
struct Foo {
  value: i32
}

impl Default for Foo {
  fn default() -> Self {
    Self {
      value: 123
    }
  }
}

let foo: Foo = FromReflect::from_reflect(dyn_foo_struct);
assert_eq!(123, foo.value);

This could be convenient for when defining custom Default impls. However, it could also be counter-intuitive and possibly footgunny since it would only be applicable for when #[reflect(Default)] is used.

Alternatively, we could define a container attribute for the struct itself, like serde does. This might result in more macro clutter, though...

PROMETHIA-27

Tests for correct behavior would probably also be a good idea.

I like the changes overall, but mixing all of the cleanup changes with the actual feature additions does make it harder to review.

crates/bevy_reflect/bevy_reflect_derive/src/attributes.rs

MrGVSV · 2022-05-09T18:18:09Z

Tests for correct behavior would probably also be a good idea.

Lol was just adding some since I noticed that too.

I like the changes overall, but mixing all of the cleanup changes with the actual feature additions does make it harder to review.

Would it be better if I split it up then? I like the changes because they make the end product a lot nicer to read/maintain. But if it's too difficult to review, then I can try to turn the cleanup changes into a followup PR.

MrGVSV · 2022-05-09T18:24:21Z

Also @PROMETHIA-27, any thoughts on #4140 (comment)?

I left a review note on your PR (#4540) saying that this PR would help with ctor stuff. However, looking at it now I realize I never actually implemented that portion of it (although it would still work if you defined #[reflect(default)] on all fields).

PROMETHIA-27 · 2022-05-09T21:41:27Z

I think it makes sense that if you reflect Default on a type, then creating an instance of that type from e.g. an empty dynamic struct should have all default values. So yeah, I think it's a good idea. I'm somewhat hesitant but I think it makes more sense than each value defaulting to its own default value. But also, it might cause a niche backwards incompatibility issue if that's done- I'm not sure if it's worth worrying about (or worth intentionally causing anyway) but good to keep in mind I think.
E.g. if I have the type

#[derive(Reflect)]
#[reflect(Default)]
struct Foo {
    x: i32
}

where x defaults to 12, this would suddenly change behavior from a 0'd x to 12 without changing the code. But reflect(Default) was also just added and hasn't been released in a version yet, so chances are no one is going to run into this.

MrGVSV · 2022-05-09T23:56:46Z

where x defaults to 12, this would suddenly change behavior from a 0'd x to 12 without changing the code.

Yeah that was one of the concerns. Although, I think it makes sense that using #[reflect(Default)] would act much like serde's container #[serde(default)]. And thinking on it more, using an alternative mechanism (like our own container #[reflect(default)]) could result in weird/unexpected behavior.

So I think if a user lets the reflection system know it implements Default, then we should use it.

But reflect(Default) was also just added and hasn't been released in a version yet, so chances are no one is going to run into this.

Yeah this might be a good reason to add it before 0.8 is released (ideally).

MrGVSV · 2022-05-10T07:29:08Z

Going to put this in draft form until #4712 is merged. We could just revert to the first commit of this PR and rebase from there, but I think it would just be easier to wait and see if the cleanup PR gets accepted/merged first so I can use some of the niceties added there.

If it's rejected or gets blocked for a long time, I'll go ahead and do the revert and rebase.

MrGVSV · 2022-05-17T06:43:26Z

Rebased and reopened!

MrGVSV · 2022-05-17T07:36:13Z

Although, I think it makes sense that using #[reflect(Default)] would act much like serde's container #[serde(default)]

Added this behavior!

crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs

crates/bevy_reflect/src/lib.rs

alice-i-cecile

Some small nits to clean up, then this LGTM. We'll need a second reviewer (@PROMETHIA-27?) before I can merge this though; it's not trivial and more eyes will help.

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

PROMETHIA-27

LGTM!

alice-i-cecile · 2022-05-30T18:16:07Z

bors r+

…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>

bors · 2022-05-30T18:35:50Z

Build failed (retrying...):

markdownlint

…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>

bors · 2022-05-30T19:21:28Z

Pull request successfully merged into main.

Build succeeded:

…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>

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 7, 2022

james7132 added A-Reflection Runtime information about types C-Enhancement A new feature and removed S-Needs-Triage This issue needs to be labelled labels Mar 7, 2022

MrGVSV changed the title ~~Add #[reflect(default)] attribute for FromReflect~~ bevy_reflect: Add #[reflect(default)] attribute for FromReflect Mar 28, 2022

MrGVSV mentioned this pull request Apr 23, 2022

[Merged by Bors] - Add macro to implement reflect for struct types and migrate glam types #4540

Closed

PROMETHIA-27 suggested changes May 9, 2022

View reviewed changes

crates/bevy_reflect/bevy_reflect_derive/src/attributes.rs Outdated Show resolved Hide resolved

MrGVSV marked this pull request as draft May 10, 2022 07:29

alice-i-cecile mentioned this pull request May 11, 2022

[Merged by Bors] - bevy_reflect_derive: Tidying up the code #4712

Closed

MrGVSV added 2 commits May 16, 2022 23:30

Added #[reflect(default)] attribute

e4a5bda

Add FromReflect default attr tests

7fcce32

MrGVSV force-pushed the reflect-default-attribute branch from 3c16ac3 to 7fcce32 Compare May 17, 2022 06:34

Improve docs

1127f65

MrGVSV marked this pull request as ready for review May 17, 2022 06:43

MrGVSV requested a review from PROMETHIA-27 May 17, 2022 06:43

Add proper FromReflect support for #[reflect(Default)]

667b903

alice-i-cecile requested a review from jakobhellermann May 30, 2022 16:11

alice-i-cecile reviewed May 30, 2022

View reviewed changes

crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed May 30, 2022

View reviewed changes

crates/bevy_reflect/src/lib.rs Outdated Show resolved Hide resolved

alice-i-cecile requested changes May 30, 2022

View reviewed changes

Apply suggestions from code review

d536d7f

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

PROMETHIA-27 approved these changes May 30, 2022

View reviewed changes

bors bot changed the title ~~bevy_reflect: Add #[reflect(default)] attribute for FromReflect~~ [Merged by Bors] - bevy_reflect: Add #[reflect(default)] attribute for FromReflect May 30, 2022

bors bot closed this May 30, 2022

MrGVSV deleted the reflect-default-attribute branch May 30, 2022 19:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - bevy_reflect: Add `#[reflect(default)]` attribute for `FromReflect` #4140

[Merged by Bors] - bevy_reflect: Add `#[reflect(default)]` attribute for `FromReflect` #4140

MrGVSV commented Mar 7, 2022 •

edited

Loading

MrGVSV commented May 8, 2022 •

edited

Loading

PROMETHIA-27 left a comment

MrGVSV commented May 9, 2022

MrGVSV commented May 9, 2022

PROMETHIA-27 commented May 9, 2022

MrGVSV commented May 9, 2022 •

edited

Loading

MrGVSV commented May 10, 2022

MrGVSV commented May 17, 2022

MrGVSV commented May 17, 2022

alice-i-cecile left a comment

PROMETHIA-27 left a comment

alice-i-cecile commented May 30, 2022

bors bot commented May 30, 2022

bors bot commented May 30, 2022

[Merged by Bors] - bevy_reflect: Add #[reflect(default)] attribute for FromReflect #4140

[Merged by Bors] - bevy_reflect: Add #[reflect(default)] attribute for FromReflect #4140

Conversation

MrGVSV commented Mar 7, 2022 • edited Loading

Objective

Solution

Example

Active Fields

Container Defaults

Related

Changelog

MrGVSV commented May 8, 2022 • edited Loading

PROMETHIA-27 left a comment

Choose a reason for hiding this comment

MrGVSV commented May 9, 2022

MrGVSV commented May 9, 2022

PROMETHIA-27 commented May 9, 2022

MrGVSV commented May 9, 2022 • edited Loading

MrGVSV commented May 10, 2022

MrGVSV commented May 17, 2022

MrGVSV commented May 17, 2022

alice-i-cecile left a comment

Choose a reason for hiding this comment

PROMETHIA-27 left a comment

Choose a reason for hiding this comment

alice-i-cecile commented May 30, 2022

bors bot commented May 30, 2022

bors bot commented May 30, 2022

[Merged by Bors] - bevy_reflect: Add `#[reflect(default)]` attribute for `FromReflect` #4140

[Merged by Bors] - bevy_reflect: Add `#[reflect(default)]` attribute for `FromReflect` #4140

MrGVSV commented Mar 7, 2022 •

edited

Loading

MrGVSV commented May 8, 2022 •

edited

Loading

MrGVSV commented May 9, 2022 •

edited

Loading