[Merged by Bors] - Add `reflect(skip_serializing)` which retains reflection but disables automatic serialization #5250

makspll · 2022-07-08T13:47:55Z

Objective

To address problems outlined in Reflection: Add distinction between ignoring fields for purposes of scene deserialization and external visibility #5245

Solution

Introduce reflect(skip_serializing) on top of reflect(ignore) which disables automatic serialisation to scenes, but does not disable reflection of the field.

Changelog

Adds:
- bevy_reflect::serde::type_data module
- SerializationData structure for describing which fields are to be/not to be ignored, automatically registers as type_data for struct-based types
- the skip_serialization flag for #[reflect(...)]
Removes:
- ability to ignore Enum variants in serialization, since that didn't work anyway

Migration Guide

Change #[reflect(ignore)] to #[reflect(skip_serializing)] where disabling reflection is not the intended effect.
Remove ignore/skip attributes from enum variants as these won't do anything anymore

alice-i-cecile · 2022-07-08T13:53:32Z

Bikeshed: I think I prefer do_not_serialize as a name here.

makspll · 2022-07-08T13:56:59Z

done!

MrGVSV · 2022-07-08T14:05:56Z

More bike shedding 😄: no_serde or skip_serde.

It's just a bit less to write and cleaner. But do_not_serialize isn't bad either haha

Edit: or use skip_serializing like serde does

makspll · 2022-07-08T14:09:41Z

Hmm, I am against serde mentions since this might make people think (more than it does now) this applies serde's skip_serializing to the field which it doesn't.

Right now this only disables automatic Reflect serialization.

Edit: skip_serializing does sound good too, which one should I go for ?

alice-i-cecile · 2022-07-08T16:34:23Z

skip_serializing for consistency with serde is my new favorite 😄

alice-i-cecile

@PROMETHIA-27 @MrGVSV can I get a second review here?

MrGVSV · 2022-09-03T00:31:55Z

This probably needs to be rebased. @makspll would you be able to do that?

makspll · 2022-09-04T16:44:01Z

I am currently on holiday, but can get on this ~monday

makspll · 2022-09-08T10:56:29Z

Aight, so I am working on this now, it's been a while so I'll take my time with this (and reflection changed a lot by the looks of it) but one thing I noticed so far:
when running the scene example,

bevy_asset::asset_server: encountered an error while loading an asset: No registration found for `glam::vec3::Vec3`

Now I can't remember if that was there before, probably was but I didn't notice, what I think is happening is that because of the way glam imports changed, the type names are now platform dependent, meaning that on my machine the type name this is saved under is now: glam::f32::vec3::Vec3, this is not a problem with this PR in that case and it might have already been addressed somewhere else (and might be fixed after I re-base) but I am just gonna leave this mental note here.

I've squashed all the commits for ease of re-basing, apologies for the force push

MrGVSV · 2022-09-08T14:25:47Z

Now I can't remember if that was there before, probably was but I didn't notice, what I think is happening is that because of the way glam imports changed, the type names are now platform dependent, meaning that on my machine the type name this is saved under is now: glam::f32::vec3::Vec3, this is not a problem with this PR in that case and it might have already been addressed somewhere else (and might be fixed after I re-base) but I am just gonna leave this mental note here.

Yep this is a known issue and we're trying to fix it via #5805.

makspll · 2022-09-08T14:27:44Z

Yep this is a known issue and we're trying to fix it via #5805.

Okay that's fantastic, I remember bumping into that problem earlier with scripting but it completely fell out my head

makspll · 2022-09-08T16:44:31Z

One question @MrGVSV, are enum variants supposed to be ignorable by reflection/serialization ?

If so, how would the serialziation/deserialization of ignored variants look like?

MrGVSV · 2022-09-08T17:00:11Z

One question @MrGVSV, are enum variants supposed to be ignorable by reflection/serialization ?

I added it as an option. I don't know what specific use-cases it has, though. My original thought was that it would allow for enums to contain variants meant for runtime data and variants meant for serialized data, such as HandleId.

If you find that it's not necessary or should be removed, feel free to make an issue for it, though! I have no problem removing it if it isn't useful haha

makspll · 2022-09-09T11:21:40Z

One question @MrGVSV, are enum variants supposed to be ignorable by reflection/serialization ?

I added it as an option. I don't know what specific use-cases it has, though. My original thought was that it would allow for enums to contain variants meant for runtime data and variants meant for serialized data, such as HandleId.

If you find that it's not necessary or should be removed, feel free to make an issue for it, though! I have no problem removing it if it isn't useful haha

Hmm I see, Yeah I definitely see possible uses for it so removing is not what I want to do. I am wondering however if the current reflect(ignore) works at all (unless I am missing something, and I very well might be, I just didn't see tests for this)? Since the deserialization requires a variant field, which one can only access at serialization IFF the variant is not ignored (the macros don't process inactive variants at all). The solution is probably modifying the macros to generate variant names for all variants regardless if they are ignored.

First bit of serialization:

        let variant_type = self.enum_value.variant_type(); // crash here on ignored variants
        let variant_name = self.enum_value.variant_name();

First bit of deserialization:

        let key = map.next_key::<String>()?;
        match key.as_deref() {
            Some(type_fields::VARIANT) => {}
            Some(key) => return Err(V::Error::unknown_field(key, &[type_fields::VARIANT])),
            _ => {
                return Err(V::Error::missing_field(type_fields::VARIANT));
            }
        }

Relevant macro code on main:

  for variant in reflect_enum.active_variants() {
        let ident = &variant.data.ident;
        let name = ident.to_string();
        let unit = reflect_enum.get_unit(ident);

       // ... more code ...

        let mut add_fields_branch = |variant, info_type, arguments, field_len| {
            // .. more code ..
            // note this code doesn't run for ignored variants
            enum_field_len.push(quote! {
                #unit{..} => #field_len
            });
            enum_variant_name.push(quote! {
                #unit{..} => #name
            });
            enum_variant_type.push(quote! {
                #unit{..} => #bevy_reflect_path::VariantType::#variant
            });
        };

MrGVSV · 2022-09-14T01:28:51Z

Hmm I see, Yeah I definitely see possible uses for it so removing is not what I want to do. I am wondering however if the current reflect(ignore) works at all (unless I am missing something, and I very well might be, I just didn't see tests for this)? Since the deserialization requires a variant field, which one can only access at serialization IFF the variant is not ignored (the macros don't process inactive variants at all). The solution is probably modifying the macros to generate variant names for all variants regardless if they are ignored.

Oh great catch! Yeah, it's a bit weird because methods like Enum::variant_type work directly on the type, making it difficult to partially ignore. I see three options:

Do like you suggest, and just don't ignore the variants for any method that relies on current variant info
Have those methods return sensible defaults rather than throwing unreachable!() (e.g. return Option<&str> rather than &str)
Do not allow variants to be ignored

While I see the possible usefulness of ignoring variants, perhaps it's better to just go with Option 3 and remove this functionality altogether (at least for the time being). Option 1 isn't awful but I think it will have a breadth that basically matches Option 3. Option 2 just hurts ergonomics imo.

I think it will be better to remove this as it's a really exposed footgun (oversight on my part). We can always add it back in, and it would be better to remove it now before 0.9 releases with it (forcing a possible future removal to be a breaking change).

I'll make an issue for this unless there's any pushback.

makspll · 2022-09-14T11:36:49Z

@MrGVSV yeah okay, I haven't touched much code in a month so I assumed I was tripping :P

Alright well, In that case I am in favour of removing the variant ignore behaviour for now on the basis of what's already been said + the fact that adding it complicates this PR a bit:

without variant serialization, I can represent "Serialization Ignoring Behaviour" with a set of field indices, with variants I now need an Enum, with either that or a set of strings for variant names. Keeping just index sets is probably more performant and ergonomic, since the serialization process is just:

for (idx,field) in my_thing.iter_fields().enumerate(){
  if !ignored.contains(idx){
     // serialize 
  }
}

…akspll/bevy into no-serialize-attribute-reflect

makspll · 2022-09-15T12:22:22Z

Alright so that should be it then, I've removed the variant ignoring mechanisms, and re-based everything, should be ready for review

makspll · 2022-09-18T15:32:18Z

Alrighty, that should be @MrGVSV's review implemented, biggest change is SerializationData is now under data in the registry

MrGVSV

Just a few more comments

crates/bevy_reflect/bevy_reflect_derive/src/impls/structs.rs

crates/bevy_reflect/src/serde/mod.rs

crates/bevy_reflect/src/serde/type_data.rs

Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>

…akspll/bevy into no-serialize-attribute-reflect

MrGVSV

LGTM!

makspll · 2022-09-18T18:14:23Z

Fantastic,
This PR will of course need a follow up updating the existing #[reflect(ignore)] and swapping to #[reflect(skip_serializing)] where possible, I am happy to do that after this PR is merged

crates/bevy_reflect/bevy_reflect_derive/src/derive_data.rs

alice-i-cecile

Looks good to me now! Those dead_code annotations are surprising to me, but they do seem to be needed.

alice-i-cecile · 2022-09-19T15:56:35Z

bors r+

… automatic serialization (#5250) # Objective - To address problems outlined in #5245 ## Solution - Introduce `reflect(skip_serializing)` on top of `reflect(ignore)` which disables automatic serialisation to scenes, but does not disable reflection of the field. --- ## Changelog - Adds: - `bevy_reflect::serde::type_data` module - `ReflectSerializableWithData` trait for retrieving `SerializationData` which is implemented via macros - `SerializationData` structure for describing which fields are to be/not to be ignored - the `skip_serialization` flag for `#[reflect(...)]` - Removes: - ability to ignore Enum variants in serialization, since that didn't work anyway ## Migration Guide - Change `#[reflect(ignore)]` to `#[reflect(skip_serializing)]` where disabling reflection is not the intended effect. - Remove ignore/skip attributes from enum variants as these won't do anything anymore

bors · 2022-09-19T16:11:54Z

Build failed (retrying...):

build-and-install-on-iOS

… automatic serialization (#5250) # Objective - To address problems outlined in #5245 ## Solution - Introduce `reflect(skip_serializing)` on top of `reflect(ignore)` which disables automatic serialisation to scenes, but does not disable reflection of the field. --- ## Changelog - Adds: - `bevy_reflect::serde::type_data` module - `SerializationData` structure for describing which fields are to be/not to be ignored, automatically registers as type_data for struct-based types - the `skip_serialization` flag for `#[reflect(...)]` - Removes: - ability to ignore Enum variants in serialization, since that didn't work anyway ## Migration Guide - Change `#[reflect(ignore)]` to `#[reflect(skip_serializing)]` where disabling reflection is not the intended effect. - Remove ignore/skip attributes from enum variants as these won't do anything anymore

bors · 2022-09-19T16:33:30Z

Pull request successfully merged into main.

Build succeeded:

… automatic serialization (bevyengine#5250) # Objective - To address problems outlined in bevyengine#5245 ## Solution - Introduce `reflect(skip_serializing)` on top of `reflect(ignore)` which disables automatic serialisation to scenes, but does not disable reflection of the field. --- ## Changelog - Adds: - `bevy_reflect::serde::type_data` module - `SerializationData` structure for describing which fields are to be/not to be ignored, automatically registers as type_data for struct-based types - the `skip_serialization` flag for `#[reflect(...)]` - Removes: - ability to ignore Enum variants in serialization, since that didn't work anyway ## Migration Guide - Change `#[reflect(ignore)]` to `#[reflect(skip_serializing)]` where disabling reflection is not the intended effect. - Remove ignore/skip attributes from enum variants as these won't do anything anymore

alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types labels Jul 8, 2022

makspll changed the title ~~Add reflect(ignore_serialization) which retains reflection but disables automatic serialization~~ Add reflect(do_not_serialize) which retains reflection but disables automatic serialization Jul 8, 2022

makspll changed the title ~~Add reflect(do_not_serialize) which retains reflection but disables automatic serialization~~ Add reflect(skip_serializing) which retains reflection but disables automatic serialization Jul 8, 2022

makspll mentioned this pull request Jul 9, 2022

Implement Reflect for PhantomData #5145

Closed

alice-i-cecile approved these changes Sep 2, 2022

View reviewed changes

add skip_serializing flag to reflect macros

cf9b287

makspll force-pushed the no-serialize-attribute-reflect branch from 95dc399 to cf9b287 Compare September 8, 2022 10:32

add skip_serializing flag to reflect macros

d614252

MrGVSV mentioned this pull request Sep 14, 2022

Remove ability to #[reflect(ignore)] enum variants #5983

Closed

makspll added 4 commits September 15, 2022 12:48

rebase changes

e249d97

Merge branch 'no-serialize-attribute-reflect' of https://github.com/m…

7089018

…akspll/bevy into no-serialize-attribute-reflect

fixes for clippy

6f12dad

remove needless test

d7ef6d8

MrGVSV reviewed Sep 18, 2022

View reviewed changes

makspll and others added 4 commits September 18, 2022 17:21

Update crates/bevy_reflect/src/serde/type_data.rs

0d61a97

Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>

Update crates/bevy_reflect/src/serde/type_data.rs

ad1521c

Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>

implement further review changes

980daa3

Merge branch 'no-serialize-attribute-reflect' of https://github.com/m…

4c8cd6f

…akspll/bevy into no-serialize-attribute-reflect

MrGVSV approved these changes Sep 18, 2022

View reviewed changes