bevy_reflect: Add DeserializeWithRegistry and SerializeWithRegistry

[MrGVSV]

Objective

The Problem

Currently, the reflection deserializers give little control to users for how a type is deserialized. The most control a user can have is to register ReflectDeserialize, which will use a type's Deserialize implementation.

However, there are times when a type may require slightly more control.

For example, let's say we want to make Bevy's Mesh easier to deserialize via reflection (assume Mesh actually implemented Reflect). Since we want this to be extensible, we'll make it so users can use their own types so long as they satisfy Into<Mesh>. The end result should allow users to define a RON file like:

{
  "my_game::meshes::Sphere": (
    radius: 2.5
  )
}

The Current Solution

Since we don't know the types ahead of time, we'll need to use reflection. Luckily, we can access type information dynamically via the type registry. Let's make a custom type data struct that users can register on their types:

pub struct ReflectIntoMesh {
  // ...
}

impl<T: FromReflect + Into<Mesh>> FromType<T> for ReflectIntoMesh {
  fn from_type() -> Self {
    // ...
  }
}

Now we'll need a way to use this type data during deserialization. Unfortunately, we can't use Deserialize since we need access to the registry. This is where DeserializeSeed comes in handy:

pub struct MeshDeserializer<'a> {
  pub registry: &'a TypeRegistry
}

impl<'a, 'de> DeserializeSeed<'de> for MeshDeserializer<'a> {
  type Value = Mesh;

  fn deserialize<D>(self, deserializer: D) -> Result<Self::Value, D::Error>
  where
    D: serde::Deserializer<'de>,
  {
    struct MeshVisitor<'a> {
      registry: &'a TypeRegistry
    }

    impl<'a, 'de> Visitor<'de> for MeshVisitor<'a> {
      fn expecting(&self, formatter: &mut Formatter) -> std::fmt::Result {
        write!(formatter, "map containing mesh information")
      }

      fn visit_map<A>(self, mut map: A) -> Result<Self::Value, serde::de::Error> where A: MapAccess<'de> {
        // Parse the type name
        let type_name = map.next_key::<String>()?.unwrap();

        // Deserialize the value based on the type name
        let registration = self.registry
          .get_with_name(&type_name)
          .expect("should be registered");
        let value = map.next_value_seed(TypedReflectDeserializer {
          registration,
          registry: self.registry,
        })?;

        // Convert the deserialized value into a `Mesh`
        let into_mesh = registration.data::<ReflectIntoMesh>().unwrap();
        Ok(into_mesh.into(value))
      }
    }
  }
}

The Problem with the Current Solution

The solution above works great when all we need to do is deserialize Mesh directly. But now, we want to be able to deserialize a struct like this:

struct Fireball {
  damage: f32,
  mesh: Mesh,
}

This might look simple enough and should theoretically be no problem for the reflection deserializer to handle, but this is where our MeshDeserializer solution starts to break down.

In order to use MeshDeserializer, we need to have access to the registry. The reflection deserializers have access to that, but we have no way of borrowing it for our own deserialization since they have no way of knowing about MeshDeserializer.

This means we need to implement another DeserializeSeed— this time for Fireball!
And if we decided to put Fireball inside another type, well now we need one for that type as well.

As you can see, this solution does not scale well and results in a lot of unnecessary boilerplate for the user.

Solution

Note

This PR originally only included the addition of DeserializeWithRegistry. Since then, a corresponding SerializeWithRegistry trait has also been added. The reasoning and usage is pretty much the same as the former so I didn't bother to update the full PR description.

Created the DeserializeWithRegistry trait and ReflectDeserializeWithRegistry type data.

The DeserializeWithRegistry trait works like a standard Deserialize but provides access to the registry. And by registering the ReflectDeserializeWithRegistry type data, the reflection deserializers will automatically use the DeserializeWithRegistry implementation, just like it does for Deserialize.

All we need to do is make the following changes:

#[derive(Reflect)]
+ #[reflect(DeserializeWithRegistry)]
struct Mesh {
  // ...
}

- impl<'a, 'de> DeserializeSeed<'de> for MeshDeserializer<'a> {
-   type Value = Mesh;
-   fn deserialize<D>(self, deserializer: D) -> Result<Self::Value, D::Error>
+ impl<'de> DeserializeWithRegistry<'de> for Mesh {
+   fn deserialize<D>(deserializer: D, registry: &TypeRegistry) -> Result<Self, D::Error>
    where
      D: serde::Deserializer<'de>,
    {
      // ...
    }
}

Now, any time the reflection deserializer comes across Mesh, it will opt to use its DeserializeWithRegistry implementation. And this means we no longer need to create a whole slew of DeserializeSeed types just to deserialize Mesh.

Why not a trait like DeserializeSeed?

While this would allow for anyone to define a deserializer for Mesh, the problem is that it means anyone can define a deserializer for Mesh. This has the unfortunate consequence that users can never be certain that their registration of ReflectDeserializeSeed is the one that will actually be used.

We could consider adding something like that in the future, but I think this PR's solution is much safer and follows the example set by ReflectDeserialize.

What if we made the TypeRegistry globally available?

This is one potential solution and has been discussed before (#6101). However, that change is much more controversial and comes with its own set of disadvantages (can't have multiple registries such as with multiple worlds, likely some added performance cost with each access, etc.).

Followup Work

Once this PR is merged, we should consider merging ReflectDeserialize into DeserializeWithRegistry. There is already a blanket implementation to make this transition generally pretty straightforward. The blanket implementations were removed for the sake of this PR and will need to be re-added in the followup. I would propose that we first mark ReflectDeserialize as deprecated, though, before we outright remove it in a future release.

---

Changelog

• Added the DeserializeReflect trait and ReflectDeserializeReflect type data
• Added the SerializeReflect trait and ReflectSerializeReflect type data
• Added TypedReflectDeserializer::of convenience constructor

[djeedai]

I'm way over my head here with reflection to be able to confidently Approve this, but it's definitely a very useful change and the change looks OK to me. Probably need review from a reflection expert though.

And better naming 😂

[djeedai]

As discussed on Discord, the naming is really not ideal. The variable here deserialize_reflect even ends up being the same as the one in the other block above, despite the types being different.

I proposed using Registry somewhere since that data/trait inherently adds a dependency to the type registry. So maybe ReflectDeserializeRegistry? (with Registry as suffix like Seed is, since the registry value kind of replace the seed in DeserializeSeed)

[MrGVSV]

Agreed. I think DeserializeRegistry (ReflectDeserializeRegistry) is pretty good.

A few others:

• RegistryDeserialize (ReflectRegistryDeserialize)
• DeserializeTypeRegistry (ReflectDeserializeTypeRegistry)
• DeserializeReflected (ReflectDeserializeReflected), though, maybe that one's still not different enough

Does anyone else have any other thoughts?

[cBournhonesque]

I agree. I've been reading through the reflection code and in general seeing stuff like with Reflect multiple times in the name (ReflectFromReflect) can become quite confusing.

Maybe ReflectSerializeWithRegistry and ReflectDeserializeWithRegistry?

[MrGVSV]

Hm, I think I like the WithRegistry suffix the best. I'll switch it over to that for now, unless anyone else has any other opinions.

[djeedai]

I don't have a good feeling of whether this new block should be after or before the one above. Shouldn't we first try to deserialize with the help of the registry, if that allows customized deserializing, and fallback to the one without if not (so, the reverse order)?

[djeedai]

(and if there's an argument for the current order, can you please document it in code?)

[MrGVSV]

Yeah I think that's a good idea.

I wonder if we should just merge ReflectDeserialize with this one, since DeserializeReflect has a blanket implementation for any T: Reflect + Deserialize. 🤔

[djeedai]

If we can that would certainly remove a chunk of confusion for users.

[MrGVSV]

I think we can do this in a followup PR just to keep the discussion a bit more focused. We can also consider removing the blanket impl until then as well.

[djeedai]

Yes let's merge this and iterate if needed with a separate change.

[MrGVSV]

I added a commit that contains similar changes for Serialize, introducing a new SerializeReflect and ReflectSerializeReflect.

(I opted to wait until I get more feedback before renaming these types/traits.)

[djeedai]

Want! 😂 I went back and re-read the entire scene serialization/deserialization code, and this change makes a lot of sense to me. This will help any crate better integrate with Bevy's reflection, and helps handling trait objects, which in turn will likely help adoption of the Scene de/serialization feature. Thank you!

[cBournhonesque]

I wonder if another option could be to modify the return type of UntypedReflectDeserializer.

My understanding is:

• TypedReflectDeserializer deserializes only the value itself returns the dyn Reflect
• UntypedReflectDeserializer deserializes the type name, fetches the type info using the registry, and then calls TypedReflectDeserializer to return the dyn Reflect
  It could instead return (&TypeRegistration, dyn Reflect) or just (TypeId, dyn Reflect), or some kind of struct encapsulating these 2 pieces of information

Then the caller can have access to the type registration of the type and use it to do whatever else they need (presumably call from_reflect or take::<T> to get the actual concrete type). This would be enough to solve the problem presented in the PR description with Fireball and Mesh

The benefit is that the user doesn't have to understand the extra complexity associated with ReflectDeserializeReflect

[cBournhonesque]

This is a probably a dumb question; but won't this prevent users from providing a custom DeserializeReflect implementation where the deserializer can make use of the registry?

Or is the plan that users need to manually create ReflectDeserializeReflect in that scenario?

[MrGVSV]

Not a dumb question! Yes, this will prevent custom impls, but only where a Deserialize impl already exists.

The reasoning is that if you can deserialize a value without the TypeRegistry, you shouldn't need the DeserializeReflect trait.

Of course, there could be users who want to have a separate reflection-based deserialization impl from their normal serde impl. But I don't forsee that being a common practice or at least one we may want to support, as it means we'd probably need to keep both ReflectDeserialize and ReflectDeserializeReflect.

By using the blanket impl, we reduce the friction for when we do actually merge the two concepts (whether that happens in this PR or a future one).

I'm open to holding off on this until that happens though (assuming it doesn't happen in this PR).

Thoughts?

[cBournhonesque]

I guess in that case I'd prefer to not adding the blanket implementation until it is actually needed.
As you said, if the value is Deserialize we shouldn't need to implement DeserializeReflect :)

[cBournhonesque]

I agree. I've been reading through the reflection code and in general seeing stuff like with Reflect multiple times in the name (ReflectFromReflect) can become quite confusing.

Maybe ReflectSerializeWithRegistry and ReflectDeserializeWithRegistry?

[MrGVSV]

I wonder if another option could be to modify the return type of UntypedReflectDeserializer.

My understanding is:

• TypedReflectDeserializer deserializes only the value itself returns the dyn Reflect
• UntypedReflectDeserializer deserializes the type name, fetches the type info using the registry, and then calls TypedReflectDeserializer to return the dyn Reflect
  It could instead return (&TypeRegistration, dyn Reflect) or just (TypeId, dyn Reflect), or some kind of struct encapsulating these 2 pieces of information

Then the caller can have access to the type registration of the type and use it to do whatever else they need (presumably call from_reflect or take::<T> to get the actual concrete type). This would be enough to solve the problem presented in the PR description with Fireball and Mesh

The benefit is that the user doesn't have to understand the extra complexity associated with ReflectDeserializeReflect

This is a good suggestion, and we probably could expose a method to return both the dyn Reflect and the &TypeRegistration.

However, I don't think that would solve the full problem. To even use one of the deserializers, you'd have to have access to the TypeRegistry. This might be possible for the "root" type, but nested types pose a challenge since you can't define a plain Deserialize impl that can construct one of those deserializers.

Additionally, the example in the PR description is just one example (albeit, probably the most common). There may be other cases where a user wants to use the registry for other things, such as accessing type data of a field of the returned dyn Reflect rather than one on the main dyn Reflect type itself.

[djeedai]

Looks good to me, thanks @MrGVSV!
Couple of nits, no need to block on them, just if you have time.

[MrGVSV]

I've removed the blanket impls for these traits (i.e. DeserializeWithRegistry for all T: Deserialize and SerializeWithRegistry for all T: Serialize). We'll plan to add this back in a followup PR.

[alice-i-cecile]

I really appreciate the detailed PR description, and the motivation and code quality both seem sensible.

That said, this needs a concrete, ideally practical and approachable example to teach users about this. Even with the PR description on hand and a fair bit of experience working with reflection and serialization, I'm struggling to understand why you might want to augment your serialization and deserialization with type registry data.

I have a sense that this might be useful, but I don't have the intuition yet. And new users certainly won't. Without that, it's very easy for this addition to become neglected and challenging to maintain.

[aecsocket]

I think I've come up with a concrete and relatively approachable example for where this can be used in #15518

Effectively, if we want to deserialize:

(
    ty: "bevy_animation_graph::node::SpeedNode"
    inner: (
        speed_multiplier: 2.0,
    ),
)

This is impossible with Deserialize because we don't have type registry access. This is possible with DeserializeSeed if you write a custom struct which holds a &'a TypeRegistry, but you would need to write a custom AnimationNodeDeserializer. What would be even nicer is this PR's solution of DeserializeFromRegistry, which wouldn't require writing a whole new struct for each thing you want to deserialize like this.

[MrGVSV]

I'll try to rebase this soon!

[alice-i-cecile]

I'm happy with that motivation. @MrGVSV bother me when this is merge-conflict free?

[alice-i-cecile]

I would merge this but your doc links are broken :)

[alice-i-cecile]

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1711 if you'd like to help out.