Handle Serialization and Deserialization

[lassade]

Objective

Add serialization for Handle this will allows every type of bevy to implement Serialize and Deserialize this will also allows for scene and others data to also load all the dependencies with ease;

Usage

struct MyDataType {
    mesh_asset: Handle<Mesh>,
}

let data = asset_server.with_asset_refs_serialization(|| MyDataType::deserialize(&mut de));

asset_server.with_asset_refs_serialization(|| data.serialize(&mut ser));

will result in:

r#"MyDataType(mesh: External("path/to/mesh.glb#Mesh2"))"#
// or when the asset wasn't loaded from a file
r#"MyDataType(mesh: Local("8ecbac0f-f545-4473-ad43-e1f4243af51e", 12031280123816238))"#

Requiriments

• Fix get_handle_path() #1290

Solves

Allow #2216 to be merged

[lassade]

We need still need a way of pointing to static handles like PBR_PIPELINE_HANDLE

Edit: Done!

[NathanSWard]

Thanks for opening this PR!!
Handle serialization is definitely something we need to support 😄

I know that there is work to incorporate distill for our asset library.
This would also be nice, as distill supports handle (de)serialization already.
However, I'm not sure how long that will take, so I'm more than happy to support our own version of handle (de)serialization :)

I left some initial comments, please let me know if I need to clarify anything!

[NathanSWard]

Could you also add some tests for this?

[NathanSWard]

Suggested change

///////////////////////////////////////////////////////////////////////////////

[lassade]

Could you also add some tests for this?

I'm using it elsewhere, so maybe later;

About panics on missing AssetServer when deserializing (the review disappeared to me)

I'm not sure if panic or not to panic, maybe a warning/error log will be better

Any weird behavior the game makes because of a missing asset or this will better than a crash;

[NathanSWard]

I'm using it elsewhere, so maybe later;

Not quite sure what this means haha 😅
However, since this is a new feature addition, before this gets merged I would like to see a few tests 😉

I'm not sure if panic or not to panic, maybe a warning/error log will be better

Yea I'm not sure either.
It just feels weird for someone to want to (de)serialize something that contains a Handle and purposefully not have it actually point to the actual associated asset. If they wanted this behavior I would suggest they use Option<Handle> instead.

[lassade]

It's possible to reduce the amount of locks needed to query the asset paths down to 1

[NathanSWard]

Do you have an example use case of how this API is better than the previous one?
It worries me here that T is not constrained on Serialize or Deserialize

[lassade]

the last API didn't allow to de::DeserializeSeed to be used for stateful de-serialization

[NathanSWard]

Yep, that makes sense.
However, I'm still not a big fan of just having the parameter be f: impl FnOnce() -> T.
you could give this any f that returns any type, which as a user of the function, isn't very informative.
If we could come up with some way to constrain T to better express that T needs to be serializable/deserializable via the AssetServer context, that would be best IMO.

[NathanSWard]

Could you give f a more descriptive name?

[lassade]

f, func are commonly used in this situations, anything in mind?

[NathanSWard]

Something like serde_func (that expresses the functions does some sort of (de)serialization) would be nice.

[NathanSWard]

This PR makes me wish serde had support for stateful (de)serialization

[lassade]

This PR makes me wish serde had support for stateful (de)serialization

But it does! but it you must do it manually implement it (witch is hard), that's why no one think it supports I guess, but you can find it on the docs.

This PR is motivated by this prefab plugin (https://github.com/lassade/bevy_prefab) it uses only stateful de-serialization to describe prefabs and entities and it's a WIP btw

[NathanSWard]

But it does! but it you must do it manually implement it (witch is hard), that's why no one think it supports I guess, but you can find it on the docs.

Oh? I was aware of DeseralizeSeed however i didn't think there's was something similar for Seralize.

Perhaps I was conflating the word stateful with context.

More specifically I would like it to be able to have some context parameter that gets passed to the serialize function. So ideally our code would look like:

handle.seralze(&mut serializer, &asset_server);

So we can avoid the global state.

[lassade]

In that case you can do:

(&my_data, &mut my_state).serialize(serializer);

impl<'a> Serialize for (&'a MyData, &'a mut MyState) { ... }

But again, it must be impl down the chain to work, many types lets say pet_graph::Graph will not support stateful serialization/deserialization unless you run a hand made custom impl for it.

Thats just too much work to ask from users I think

[alice-i-cecile]

This has been resolved.

[alice-i-cecile]

Thoughts

I think the foundations of this are good, but it needs clean up.

I would like to see:

• docs for AssetRef
• an example or two for how this works
• cleanup of the small nits
• a trait bound on the serialization method

Due to the age, I'm going to apply the Adopt-Me label.

[MrGVSV]

I wonder if this should instead be achieved through reflection, using something like #5923. Reflection would probably better integrate with scenes and such, as well as allow better control over these assets.

For example, the type registry could contain more metadata about an asset that is inserted on load, such as external URLs, dependencies, etc.— either in ReflectHandle or on a new type data.

[alice-i-cecile]

Yep, I agree with your analysis there. Closing this out, but we can learn from this going forward.