Objective

To address problems outlined in https://github.com/bevyengine/bevy/issues/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:
- TypeRegistry::get_serialization_data
- TypeRegistry::get_serialization_data_mut
- SerializationData structure for describing which fields are to be/not to be ignored
- SerializationData::is_ignored_field
- 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

Jul 08 '22 13:07 makspll

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

Jul 08 '22 13:07 alice-i-cecile

done!

Jul 08 '22 13:07 makspll

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

Jul 08 '22 14:07 MrGVSV

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 ?

Jul 08 '22 14:07 makspll

skip_serializing for consistency with serde is my new favorite 😄

Jul 08 '22 16:07 alice-i-cecile

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

Sep 03 '22 00:09 MrGVSV

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

Sep 04 '22 16:09 makspll

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

Sep 08 '22 10:09 makspll

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.

Sep 08 '22 14:09 MrGVSV

Yep this is a known issue and we're trying to fix it via https://github.com/bevyengine/bevy/pull/5805.

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

Sep 08 '22 14:09 makspll

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?

Sep 08 '22 16:09 makspll

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

Sep 08 '22 17:09 MrGVSV

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
            });
        };

Sep 09 '22 11:09 makspll

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.

Sep 14 '22 01:09 MrGVSV

@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 
  }
}

Sep 14 '22 11:09 makspll

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

Sep 15 '22 12:09 makspll

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

Sep 18 '22 15:09 makspll

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

Sep 18 '22 18:09 makspll

bors r+

Sep 19 '22 15:09 alice-i-cecile

Build failed (retrying...):

build-and-install-on-iOS

Sep 19 '22 16:09 bors[bot]

Pull request successfully merged into main.

Build succeeded:

Sep 19 '22 16:09 bors[bot]

Add `reflect(skip_serializing)` which retains reflection but disables automatic serialization

Objective

Solution

Changelog

Migration Guide