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.

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

[Merged by Bors] - bevy_reflect: Fix deserialization with readers #6894

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion crates/bevy_reflect/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ downcast-rs = "1.2"
parking_lot = "0.12.1"
thiserror = "1.0"
once_cell = "1.11"
serde = "1"
serde = { version = "1", features = ["derive"] }
smallvec = { version = "1.6", features = ["serde", "union", "const_generics"], optional = true }
glam = { version = "0.22", features = ["serde"], optional = true }

Expand Down
18 changes: 14 additions & 4 deletions crates/bevy_reflect/src/serde/de.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use serde::de::{
};
use serde::Deserialize;
use std::any::TypeId;
use std::borrow::Cow;
use std::fmt;
use std::fmt::{Debug, Display, Formatter};
use std::slice::Iter;
Expand Down Expand Up @@ -210,6 +211,13 @@ impl<'de> Visitor<'de> for U32Visitor {
}
}

/// Helper struct for deserializing strings without allocating (when possible).
///
/// Based on [this comment](https://github.com/bevyengine/bevy/pull/6894#discussion_r1045069010).
#[derive(Deserialize)]
#[serde(transparent)]
struct BorrowableCowStr<'a>(#[serde(borrow)] Cow<'a, str>);
Copy link
Member Author

Choose a reason for hiding this comment

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

Since this is used in two different crates, should this construct be moved to something like bevy_utils? Or is it fine to keep this duplication since it's only a few lines?

Copy link
Contributor

@soqb soqb Dec 23, 2022

Choose a reason for hiding this comment

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

i think the implementation is short and niche enough to leave as is in this pr, but we should probably keep an eye on it. three uses is where i'd probably stick it into a shared crate.

edit: although it could be exported from bevy_reflect::de since bevy_scene depends on it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, should this be something that is owned by bevy_reflect::de and depended on by bevy_scene? I feel like bevy_reflect shouldn't really have that responsibility, right?

I can export it if that's preferred.


/// A general purpose deserializer for reflected types.
///
/// This will return a [`Box<dyn Reflect>`] containing the deserialized data.
Expand Down Expand Up @@ -273,8 +281,9 @@ impl<'a, 'de> Visitor<'de> for UntypedReflectDeserializerVisitor<'a> {
A: MapAccess<'de>,
{
let type_name = map
.next_key::<String>()?
.ok_or_else(|| Error::invalid_length(0, &"at least one entry"))?;
.next_key::<BorrowableCowStr>()?
.ok_or_else(|| Error::invalid_length(0, &"at least one entry"))?
.0;

let registration = self.registry.get_with_name(&type_name).ok_or_else(|| {
Error::custom(format_args!("No registration found for `{type_name}`"))
Expand Down Expand Up @@ -1536,10 +1545,11 @@ mod tests {
108, 101, 144, 146, 100, 145, 101,
];

let deserializer = UntypedReflectDeserializer::new(&registry);
let mut reader = std::io::BufReader::new(input.as_slice());

let deserializer = UntypedReflectDeserializer::new(&registry);
let dynamic_output = deserializer
.deserialize(&mut rmp_serde::Deserializer::new(input.as_slice()))
.deserialize(&mut rmp_serde::Deserializer::new(&mut reader))
.unwrap();

let output = <MyStruct as FromReflect>::from_reflect(dynamic_output.as_ref()).unwrap();
Expand Down
1 change: 1 addition & 0 deletions crates/bevy_scene/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,4 @@ thiserror = "1.0"
[dev-dependencies]
postcard = { version = "1.0", features = ["alloc"] }
bincode = "1.3"
rmp-serde = "1.1"
59 changes: 56 additions & 3 deletions crates/bevy_scene/src/serde.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use serde::{
ser::SerializeStruct,
Deserialize, Deserializer, Serialize, Serializer,
};
use std::borrow::Cow;
use std::fmt::Formatter;

pub const SCENE_STRUCT: &str = "Scene";
Expand Down Expand Up @@ -352,14 +353,14 @@ impl<'a, 'de> Visitor<'de> for ComponentVisitor<'a> {
{
let mut added = HashSet::new();
let mut components = Vec::new();
while let Some(key) = map.next_key::<&str>()? {
if !added.insert(key) {
while let Some(BorrowableCowStr(key)) = map.next_key()? {
if !added.insert(key.clone()) {
return Err(Error::custom(format!("duplicate component: `{key}`")));
}

let registration = self
.registry
.get_with_name(key)
.get_with_name(&key)
.ok_or_else(|| Error::custom(format!("no registration found for `{key}`")))?;
components.push(
map.next_value_seed(TypedReflectDeserializer::new(registration, self.registry))?,
Expand All @@ -384,6 +385,13 @@ impl<'a, 'de> Visitor<'de> for ComponentVisitor<'a> {
}
}

/// Helper struct for deserializing strings without allocating (when possible).
///
/// Based on [this comment](https://github.com/bevyengine/bevy/pull/6894#discussion_r1045069010).
#[derive(Deserialize)]
#[serde(transparent)]
struct BorrowableCowStr<'a>(#[serde(borrow)] Cow<'a, str>);

#[cfg(test)]
mod tests {
use crate::serde::{SceneDeserializer, SceneSerializer};
Expand All @@ -394,6 +402,8 @@ mod tests {
use bevy_reflect::{FromReflect, Reflect, ReflectSerialize};
use bincode::Options;
use serde::de::DeserializeSeed;
use serde::Serialize;
use std::io::BufReader;

#[derive(Component, Reflect, Default)]
#[reflect(Component)]
Expand Down Expand Up @@ -567,6 +577,49 @@ mod tests {
assert_scene_eq(&scene, &deserialized_scene);
}

#[test]
fn should_roundtrip_messagepack() {
let mut world = create_world();

world.spawn(MyComponent {
foo: [1, 2, 3],
bar: (1.3, 3.7),
baz: MyEnum::Tuple("Hello World!".to_string()),
});

let registry = world.resource::<AppTypeRegistry>();

let scene = DynamicScene::from_world(&world, registry);

let scene_serializer = SceneSerializer::new(&scene, &registry.0);
let mut buf = Vec::new();
let mut ser = rmp_serde::Serializer::new(&mut buf);
scene_serializer.serialize(&mut ser).unwrap();

assert_eq!(
vec![
145, 129, 0, 145, 129, 217, 37, 98, 101, 118, 121, 95, 115, 99, 101, 110, 101, 58,
58, 115, 101, 114, 100, 101, 58, 58, 116, 101, 115, 116, 115, 58, 58, 77, 121, 67,
111, 109, 112, 111, 110, 101, 110, 116, 147, 147, 1, 2, 3, 146, 202, 63, 166, 102,
102, 202, 64, 108, 204, 205, 129, 165, 84, 117, 112, 108, 101, 172, 72, 101, 108,
108, 111, 32, 87, 111, 114, 108, 100, 33
],
buf
);

let scene_deserializer = SceneDeserializer {
type_registry: &registry.0.read(),
};
let mut reader = BufReader::new(buf.as_slice());

let deserialized_scene = scene_deserializer
.deserialize(&mut rmp_serde::Deserializer::new(&mut reader))
.unwrap();

assert_eq!(1, deserialized_scene.entities.len());
assert_scene_eq(&scene, &deserialized_scene);
}

#[test]
fn should_roundtrip_bincode() {
let mut world = create_world();
Expand Down