bevy_reflect: Fix ignored/skipped field order by MrGVSV · Pull Request #7575 · bevyengine/bevy

MrGVSV · 2023-02-09T03:38:24Z

Objective

Fixes #5101
Alternative to #6511

Solution

Corrected the behavior for ignored fields in FromReflect, which was previously using the incorrect field indexes.

Similarly, fields marked with #[reflect(skip_serializing)] no longer break when using FromReflect after deserialization. This was done by modifying SerializationData to store a function pointer that can later be used to generate a default instance of the skipped field during deserialization.

The function pointer points to a function generated by the derive macro using the behavior designated by #[reflect(default)] (or just Default if none provided). The entire output of the macro is now wrapped in an unnamed constant which keeps this behavior hygienic.

Rationale

The biggest downside to this approach is that it requires fields marked #[reflect(skip_serializing)] to provide the ability to create a default instance— either via a Default impl or by specifying a custom one. While this isn't great, I think it might be justified by the fact that we really need to create this value when using FromReflect on a deserialized object. And we need to do this during deserialization because after that (at least for tuples and tuple structs) we lose information about which field is which: "is the value at index 1 in this DynamicTupleStruct the actual value for index 1 or is it really the value for index 2 since index 1 is skippable...?"

Alternatives

An alternative would be to store Option<Box<dyn Reflect>> within DynamicTuple and DynamicTupleStruct instead of just Box<dyn Reflect>. This would allow us to insert "empty"/"missing" fields during deserialization, thus saving the positional information of the skipped fields. However, this may require changing the API of Tuple and TupleStruct such that they can account for their dynamic counterparts returning None for a skipped field. In practice this would probably mean exposing the Option-ness of the dynamics onto implementors via methods like Tuple::drain or TupleStruct::field.

Personally, I think requiring Default would be better than muddying up the API to account for these special cases. But I'm open to trying out this other approach if the community feels that it's better.

Changelog

Public Changes

Fixed

The behaviors of #[reflect(ignore)] and #[reflect(skip_serializing)] are no longer dependent on field order

Changed

Fields marked with #[reflect(skip_serializing)] now need to either implement Default or specify a custom default function using #[reflect(default = "path::to::some_func")]
Deserializing a type with fields marked #[reflect(skip_serializing)] will now include that field initialized to its specified default value
SerializationData::new now takes the new SkippedField struct along with the skipped field index
Renamed SerializationData::is_ignored_field to SerializationData::is_field_skipped

Added

Added SkippedField struct
Added methods SerializationData::generate_default and SerializationData::iter_skipped

Internal Changes

Changed

Replaced members_to_serialization_denylist and BitSet<u32> with SerializationDataDef
The Reflect derive is more hygienic as it now outputs within an unnamed constant
StructField::index has been split up into StructField::declaration_index and StructField::reflection_index

Removed

Removed bitset dependency

Migration Guide

Fields marked #[reflect(skip_serializing)] now must implement Default or specify a custom default function with #[reflect(default = "path::to::some_func")]

#[derive(Reflect)]
struct MyStruct {
  #[reflect(skip_serializing)]
  #[reflect(default = "get_foo_default")]
  foo: Foo, // <- `Foo` does not impl `Default` so requires a custom function
  #[reflect(skip_serializing)]
  bar: Bar, // <- `Bar` impls `Default`
}

#[derive(Reflect)]
struct Foo(i32);

#[derive(Reflect, Default)]
struct Bar(i32);

fn get_foo_default() -> Foo {
  Foo(123)
}

SerializationData::new has been changed to expect an iterator of (usize, SkippedField) rather than one of just usize

// BEFORE
SerializationData::new([0, 3].into_iter());

// AFTER
SerializationData::new([
  (0, SkippedField::new(field_0_default_fn)),
  (3, SkippedField::new(field_3_default_fn)),
].into_iter());

Serialization::is_ignored_field has been renamed to Serialization::is_field_skipped
Fields marked #[reflect(skip_serializing)] are now included in deserialization output. This may affect logic that expected those fields to be absent.

MrGVSV · 2023-05-23T05:03:15Z

Rebased. Not sure about the check-bans failure, but this should be ready for review again!

mockersf · 2023-05-23T08:39:29Z

Not sure about the check-bans failure

You can ignore it, Bevy is a little late in a few dependencies and the config file should be updated after more crates are up-to-date.

Similarly, fields marked with #[reflect(skip_serializing)] no longer break when using FromReflect after deserialization. This was done by modifying SerializationData to store a function pointer that can later be used to generate a default instance of the skipped field during deserialization.

I disagree that skip_serializing should impact deserialization. serde has skip, skip_serializing and skip_deserializing. Deserializing should require the field even if it's marked as skip_serializing, to make it use a default value it should be marked as skip_deserializing too. If we want to indicate both, I think we should use a different name for that attribute.

MrGVSV · 2023-05-23T17:06:08Z

Similarly, fields marked with #[reflect(skip_serializing)] no longer break when using FromReflect after deserialization. This was done by modifying SerializationData to store a function pointer that can later be used to generate a default instance of the skipped field during deserialization.

I disagree that skip_serializing should impact deserialization. serde has skip, skip_serializing and skip_deserializing. Deserializing should require the field even if it's marked as skip_serializing, to make it use a default value it should be marked as skip_deserializing too. If we want to indicate both, I think we should use a different name for that attribute.

Yeah that makes sense and would again be further aligned with serde so users have an easier time making the switch or at least conceptualizing the similarities.

That being said, I think it should be considered out of scope for this PR. This fixes a pretty longstanding and annoying bug, and shouldn't be blocked by the naming of the attribute used. We can probably create a separate ticket for that.

In fact, maybe I can work on splitting up the attributes while this PR gets reviewed so that we can hopefully be able to merge that change pretty soon before/after this one.

cart · 2023-07-08T00:46:34Z

Moving this to 0.12 as we didn't get it reviewed in time.

alice-i-cecile · 2023-09-29T15:37:17Z

@MrGVSV I'd like to not let this slip again; can you rebase?

MrGVSV · 2023-10-04T01:58:11Z

I've removed the AssociatedData stuff. While that may be useful in the future, it wasn't really needed for this PR. I think when I originally wrote this, I was unaware that a non-capturing closure can also satisfy the function pointer type 😅.

I did, however, keep the change that puts the macro output in an unnamed constant since I think that's just generally useful regardless.

soqb

overall looks like excellent improvement; a couple nits and one question for correctness.

crates/bevy_reflect/bevy_reflect_derive/src/serialization.rs

soqb · 2023-10-20T15:41:57Z

crates/bevy_reflect/src/tuple_struct.rs

+        }
+    }
+}
+


yet another place where we might be better off merging Tuple and TupleStruct 🤔

Personally, I still think we should keep the concepts separate, both for user clarity and the potential to allow their definitions to diverge as needed. If anything, we might try to reduce some code duplication by making better use of macro_rules.

crates/bevy_reflect/bevy_reflect_derive/src/lib.rs

alice-i-cecile

Thorough and well-tested, fixes an important bug. Code is clear and well-documented, as always <3

Wrap impl-only macros inside unnamed constants.

alice-i-cecile · 2023-10-22T01:47:52Z

@MrGVSV, are you ready for this to be merged now?

MrGVSV · 2023-10-22T02:10:37Z

@MrGVSV, are you ready for this to be merged now?

@alice-i-cecile should be good to go!

# Objective Fixes bevyengine#5101 Alternative to bevyengine#6511 ## Solution Corrected the behavior for ignored fields in `FromReflect`, which was previously using the incorrect field indexes. Similarly, fields marked with `#[reflect(skip_serializing)]` no longer break when using `FromReflect` after deserialization. This was done by modifying `SerializationData` to store a function pointer that can later be used to generate a default instance of the skipped field during deserialization. The function pointer points to a function generated by the derive macro using the behavior designated by `#[reflect(default)]` (or just `Default` if none provided). The entire output of the macro is now wrapped in an [unnamed constant](https://doc.rust-lang.org/stable/reference/items/constant-items.html#unnamed-constant) which keeps this behavior hygienic. #### Rationale The biggest downside to this approach is that it requires fields marked `#[reflect(skip_serializing)]` to provide the ability to create a default instance— either via a `Default` impl or by specifying a custom one. While this isn't great, I think it might be justified by the fact that we really need to create this value when using `FromReflect` on a deserialized object. And we need to do this _during_ deserialization because after that (at least for tuples and tuple structs) we lose information about which field is which: _"is the value at index 1 in this `DynamicTupleStruct` the actual value for index 1 or is it really the value for index 2 since index 1 is skippable...?"_ #### Alternatives An alternative would be to store `Option<Box<dyn Reflect>>` within `DynamicTuple` and `DynamicTupleStruct` instead of just `Box<dyn Reflect>`. This would allow us to insert "empty"/"missing" fields during deserialization, thus saving the positional information of the skipped fields. However, this may require changing the API of `Tuple` and `TupleStruct` such that they can account for their dynamic counterparts returning `None` for a skipped field. In practice this would probably mean exposing the `Option`-ness of the dynamics onto implementors via methods like `Tuple::drain` or `TupleStruct::field`. Personally, I think requiring `Default` would be better than muddying up the API to account for these special cases. But I'm open to trying out this other approach if the community feels that it's better. --- ## Changelog ### Public Changes #### Fixed - The behaviors of `#[reflect(ignore)]` and `#[reflect(skip_serializing)]` are no longer dependent on field order #### Changed - Fields marked with `#[reflect(skip_serializing)]` now need to either implement `Default` or specify a custom default function using `#[reflect(default = "path::to::some_func")]` - Deserializing a type with fields marked `#[reflect(skip_serializing)]` will now include that field initialized to its specified default value - `SerializationData::new` now takes the new `SkippedField` struct along with the skipped field index - Renamed `SerializationData::is_ignored_field` to `SerializationData::is_field_skipped` #### Added - Added `SkippedField` struct - Added methods `SerializationData::generate_default` and `SerializationData::iter_skipped` ### Internal Changes #### Changed - Replaced `members_to_serialization_denylist` and `BitSet<u32>` with `SerializationDataDef` - The `Reflect` derive is more hygienic as it now outputs within an [unnamed constant](https://doc.rust-lang.org/stable/reference/items/constant-items.html#unnamed-constant) - `StructField::index` has been split up into `StructField::declaration_index` and `StructField::reflection_index` #### Removed - Removed `bitset` dependency ## Migration Guide * Fields marked `#[reflect(skip_serializing)]` now must implement `Default` or specify a custom default function with `#[reflect(default = "path::to::some_func")]` ```rust #[derive(Reflect)] struct MyStruct { #[reflect(skip_serializing)] #[reflect(default = "get_foo_default")] foo: Foo, // <- `Foo` does not impl `Default` so requires a custom function #[reflect(skip_serializing)] bar: Bar, // <- `Bar` impls `Default` } #[derive(Reflect)] struct Foo(i32); #[derive(Reflect, Default)] struct Bar(i32); fn get_foo_default() -> Foo { Foo(123) } ``` * `SerializationData::new` has been changed to expect an iterator of `(usize, SkippedField)` rather than one of just `usize` ```rust // BEFORE SerializationData::new([0, 3].into_iter()); // AFTER SerializationData::new([ (0, SkippedField::new(field_0_default_fn)), (3, SkippedField::new(field_3_default_fn)), ].into_iter()); ``` * `Serialization::is_ignored_field` has been renamed to `Serialization::is_field_skipped` * Fields marked `#[reflect(skip_serializing)]` are now included in deserialization output. This may affect logic that expected those fields to be absent.

Fixes bevyengine#5101 Alternative to bevyengine#6511 Corrected the behavior for ignored fields in `FromReflect`, which was previously using the incorrect field indexes. Similarly, fields marked with `#[reflect(skip_serializing)]` no longer break when using `FromReflect` after deserialization. This was done by modifying `SerializationData` to store a function pointer that can later be used to generate a default instance of the skipped field during deserialization. The function pointer points to a function generated by the derive macro using the behavior designated by `#[reflect(default)]` (or just `Default` if none provided). The entire output of the macro is now wrapped in an [unnamed constant](https://doc.rust-lang.org/stable/reference/items/constant-items.html#unnamed-constant) which keeps this behavior hygienic. The biggest downside to this approach is that it requires fields marked `#[reflect(skip_serializing)]` to provide the ability to create a default instance— either via a `Default` impl or by specifying a custom one. While this isn't great, I think it might be justified by the fact that we really need to create this value when using `FromReflect` on a deserialized object. And we need to do this _during_ deserialization because after that (at least for tuples and tuple structs) we lose information about which field is which: _"is the value at index 1 in this `DynamicTupleStruct` the actual value for index 1 or is it really the value for index 2 since index 1 is skippable...?"_ An alternative would be to store `Option<Box<dyn Reflect>>` within `DynamicTuple` and `DynamicTupleStruct` instead of just `Box<dyn Reflect>`. This would allow us to insert "empty"/"missing" fields during deserialization, thus saving the positional information of the skipped fields. However, this may require changing the API of `Tuple` and `TupleStruct` such that they can account for their dynamic counterparts returning `None` for a skipped field. In practice this would probably mean exposing the `Option`-ness of the dynamics onto implementors via methods like `Tuple::drain` or `TupleStruct::field`. Personally, I think requiring `Default` would be better than muddying up the API to account for these special cases. But I'm open to trying out this other approach if the community feels that it's better. --- - The behaviors of `#[reflect(ignore)]` and `#[reflect(skip_serializing)]` are no longer dependent on field order - Fields marked with `#[reflect(skip_serializing)]` now need to either implement `Default` or specify a custom default function using `#[reflect(default = "path::to::some_func")]` - Deserializing a type with fields marked `#[reflect(skip_serializing)]` will now include that field initialized to its specified default value - `SerializationData::new` now takes the new `SkippedField` struct along with the skipped field index - Renamed `SerializationData::is_ignored_field` to `SerializationData::is_field_skipped` - Added `SkippedField` struct - Added methods `SerializationData::generate_default` and `SerializationData::iter_skipped` - Replaced `members_to_serialization_denylist` and `BitSet<u32>` with `SerializationDataDef` - The `Reflect` derive is more hygienic as it now outputs within an [unnamed constant](https://doc.rust-lang.org/stable/reference/items/constant-items.html#unnamed-constant) - `StructField::index` has been split up into `StructField::declaration_index` and `StructField::reflection_index` - Removed `bitset` dependency * Fields marked `#[reflect(skip_serializing)]` now must implement `Default` or specify a custom default function with `#[reflect(default = "path::to::some_func")]` ```rust #[derive(Reflect)] struct MyStruct { #[reflect(skip_serializing)] #[reflect(default = "get_foo_default")] foo: Foo, // <- `Foo` does not impl `Default` so requires a custom function #[reflect(skip_serializing)] bar: Bar, // <- `Bar` impls `Default` } #[derive(Reflect)] struct Foo(i32); #[derive(Reflect, Default)] struct Bar(i32); fn get_foo_default() -> Foo { Foo(123) } ``` * `SerializationData::new` has been changed to expect an iterator of `(usize, SkippedField)` rather than one of just `usize` ```rust // BEFORE SerializationData::new([0, 3].into_iter()); // AFTER SerializationData::new([ (0, SkippedField::new(field_0_default_fn)), (3, SkippedField::new(field_3_default_fn)), ].into_iter()); ``` * `Serialization::is_ignored_field` has been renamed to `Serialization::is_field_skipped` * Fields marked `#[reflect(skip_serializing)]` are now included in deserialization output. This may affect logic that expected those fields to be absent.

# Objective Fixes bevyengine#5101 Alternative to bevyengine#6511 ## Solution Corrected the behavior for ignored fields in `FromReflect`, which was previously using the incorrect field indexes. Similarly, fields marked with `#[reflect(skip_serializing)]` no longer break when using `FromReflect` after deserialization. This was done by modifying `SerializationData` to store a function pointer that can later be used to generate a default instance of the skipped field during deserialization. The function pointer points to a function generated by the derive macro using the behavior designated by `#[reflect(default)]` (or just `Default` if none provided). The entire output of the macro is now wrapped in an [unnamed constant](https://doc.rust-lang.org/stable/reference/items/constant-items.html#unnamed-constant) which keeps this behavior hygienic. #### Rationale The biggest downside to this approach is that it requires fields marked `#[reflect(skip_serializing)]` to provide the ability to create a default instance— either via a `Default` impl or by specifying a custom one. While this isn't great, I think it might be justified by the fact that we really need to create this value when using `FromReflect` on a deserialized object. And we need to do this _during_ deserialization because after that (at least for tuples and tuple structs) we lose information about which field is which: _"is the value at index 1 in this `DynamicTupleStruct` the actual value for index 1 or is it really the value for index 2 since index 1 is skippable...?"_ #### Alternatives An alternative would be to store `Option<Box<dyn Reflect>>` within `DynamicTuple` and `DynamicTupleStruct` instead of just `Box<dyn Reflect>`. This would allow us to insert "empty"/"missing" fields during deserialization, thus saving the positional information of the skipped fields. However, this may require changing the API of `Tuple` and `TupleStruct` such that they can account for their dynamic counterparts returning `None` for a skipped field. In practice this would probably mean exposing the `Option`-ness of the dynamics onto implementors via methods like `Tuple::drain` or `TupleStruct::field`. Personally, I think requiring `Default` would be better than muddying up the API to account for these special cases. But I'm open to trying out this other approach if the community feels that it's better. --- ## Changelog ### Public Changes #### Fixed - The behaviors of `#[reflect(ignore)]` and `#[reflect(skip_serializing)]` are no longer dependent on field order #### Changed - Fields marked with `#[reflect(skip_serializing)]` now need to either implement `Default` or specify a custom default function using `#[reflect(default = "path::to::some_func")]` - Deserializing a type with fields marked `#[reflect(skip_serializing)]` will now include that field initialized to its specified default value - `SerializationData::new` now takes the new `SkippedField` struct along with the skipped field index - Renamed `SerializationData::is_ignored_field` to `SerializationData::is_field_skipped` #### Added - Added `SkippedField` struct - Added methods `SerializationData::generate_default` and `SerializationData::iter_skipped` ### Internal Changes #### Changed - Replaced `members_to_serialization_denylist` and `BitSet<u32>` with `SerializationDataDef` - The `Reflect` derive is more hygienic as it now outputs within an [unnamed constant](https://doc.rust-lang.org/stable/reference/items/constant-items.html#unnamed-constant) - `StructField::index` has been split up into `StructField::declaration_index` and `StructField::reflection_index` #### Removed - Removed `bitset` dependency ## Migration Guide * Fields marked `#[reflect(skip_serializing)]` now must implement `Default` or specify a custom default function with `#[reflect(default = "path::to::some_func")]` ```rust #[derive(Reflect)] struct MyStruct { #[reflect(skip_serializing)] #[reflect(default = "get_foo_default")] foo: Foo, // <- `Foo` does not impl `Default` so requires a custom function #[reflect(skip_serializing)] bar: Bar, // <- `Bar` impls `Default` } #[derive(Reflect)] struct Foo(i32); #[derive(Reflect, Default)] struct Bar(i32); fn get_foo_default() -> Foo { Foo(123) } ``` * `SerializationData::new` has been changed to expect an iterator of `(usize, SkippedField)` rather than one of just `usize` ```rust // BEFORE SerializationData::new([0, 3].into_iter()); // AFTER SerializationData::new([ (0, SkippedField::new(field_0_default_fn)), (3, SkippedField::new(field_3_default_fn)), ].into_iter()); ``` * `Serialization::is_ignored_field` has been renamed to `Serialization::is_field_skipped` * Fields marked `#[reflect(skip_serializing)]` are now included in deserialization output. This may affect logic that expected those fields to be absent.

MrGVSV added C-Bug An unexpected or incorrect behavior A-Reflection Runtime information about types M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Feb 9, 2023

MrGVSV mentioned this pull request Feb 9, 2023

bevy_reflect: Ignored field order #6511

Closed

MrGVSV added this to the 0.11 milestone Apr 21, 2023

MrGVSV force-pushed the reflect-fix-ignore-order branch 3 times, most recently from 01b4ec7 to 113851b Compare April 23, 2023 06:06

MrGVSV force-pushed the reflect-fix-ignore-order branch from 113851b to b21bbeb Compare May 23, 2023 04:19

MrGVSV force-pushed the reflect-fix-ignore-order branch from b21bbeb to 48ab927 Compare May 30, 2023 06:02

MrGVSV force-pushed the reflect-fix-ignore-order branch from 48ab927 to 4ce56a6 Compare June 9, 2023 06:35

cart modified the milestones: 0.11, 0.12 Jul 8, 2023

MrGVSV force-pushed the reflect-fix-ignore-order branch 2 times, most recently from 9716ed9 to e8203aa Compare October 2, 2023 19:40

soqb approved these changes Oct 20, 2023

View reviewed changes

alice-i-cecile approved these changes Oct 20, 2023

View reviewed changes

alice-i-cecile added 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 Oct 20, 2023

MrGVSV added 5 commits October 21, 2023 13:18

Fix unintuitive ignored field ordering issues with FromReflect

16058ec

Fix field ordering issues with #[reflect(skip_serializing)]

c489a5b

Remove AssociatedData

9effa6f

Fix skip_serializing not using custom default functions

f10dd3a

Improve reflection hygiene

973a147

Wrap impl-only macros inside unnamed constants.

MrGVSV force-pushed the reflect-fix-ignore-order branch from d8edc81 to 973a147 Compare October 21, 2023 23:29

alice-i-cecile added this pull request to the merge queue Oct 22, 2023

Merged via the queue into bevyengine:main with commit 60773e6 Oct 22, 2023

MrGVSV deleted the reflect-fix-ignore-order branch October 26, 2023 21:32

Uh oh!

Conversation

MrGVSV commented Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Rationale

Alternatives

Changelog

Public Changes

Fixed

Changed

Added

Internal Changes

Changed

Removed

Migration Guide

Uh oh!

MrGVSV commented May 23, 2023

Uh oh!

mockersf commented May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MrGVSV commented May 23, 2023

Uh oh!

cart commented Jul 8, 2023

Uh oh!

alice-i-cecile commented Sep 29, 2023

Uh oh!

MrGVSV commented Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soqb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

soqb Oct 20, 2023

Choose a reason for hiding this comment

Uh oh!

MrGVSV Oct 21, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented Oct 22, 2023

Uh oh!

MrGVSV commented Oct 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MrGVSV commented Feb 9, 2023 •

edited

Loading

mockersf commented May 23, 2023 •

edited

Loading

MrGVSV commented Oct 4, 2023 •

edited

Loading