Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle Serialization and Deserialization #2306

Closed
wants to merge 6 commits into from
Closed

Handle Serialization and Deserialization #2306

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1ac0833
add unsafe handle serialization
lassade Jun 6, 2021
3e09b8c
safe handle serialization
lassade Jun 6, 2021
2d8f849
serialize local asset references
lassade Jun 6, 2021
2c4ce2e
clippy fix
lassade Jun 6, 2021
e71c300
typo
lassade Jun 6, 2021
bc96a93
make no assumptions on how data will be (de)serialized
lassade Jun 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 4 additions & 21 deletions crates/bevy_asset/src/ser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,31 +75,14 @@ impl<'de, T: Asset> Deserialize<'de> for Handle<T> {
}

impl AssetServer {
pub fn serialize_with_asset_refs<S, T>(
&self,
serializer: S,
value: &T,
) -> Result<S::Ok, S::Error>
/// Enables asset references to be serialized or deserialized
pub fn with_asset_refs_serialization<F, T>(&self, f: F) -> T
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you give f a more descriptive name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@NathanSWard NathanSWard Jun 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

where
S: Serializer,
T: Serialize,
{
ASSET_SERVER.with(|key| {
key.replace(Some(self.clone()));
let result = value.serialize(serializer);
key.replace(None);
result
})
}

pub fn deserialize_with_asset_refs<'de, D, T>(&self, deserializer: D) -> Result<T, D::Error>
where
D: Deserializer<'de>,
T: Deserialize<'de>,
F: FnOnce() -> T,
{
ASSET_SERVER.with(|key| {
key.replace(Some(self.clone()));
let result = T::deserialize(deserializer);
let result = (f)();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

key.replace(None);
result
})
Expand Down