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

Alternative to deserialize_any on ReflectDeserializer #5934

Closed
afonsolage opened this issue Sep 10, 2022 · 1 comment
Closed

Alternative to deserialize_any on ReflectDeserializer #5934

afonsolage opened this issue Sep 10, 2022 · 1 comment
Labels
A-Reflection Runtime information about types C-Feature A new feature, making something new possible

Comments

@afonsolage
Copy link
Contributor

What problem does this solve or what need does it fill?

Current implementation of ReflectDeserializer uses deserialize_any to deserialize objects which was serialized with ReflectSerializer, but there are many serde crates which doesn't support self-describing, like bincode or rkyv.

What solution would you like?

An alternative implementation of ReflectDeserializer which doesn't uses deserialize_any, which will allow both self-describing and non-self-describing serde crates to be used with.

What alternative(s) have you considered?

Not using non-self-describing crates, but non-self-describing crates usually have better serde performance.

Additional context

The docs of deserialize_any also states a similar thing:

Require the Deserializer to figure out how to drive the visitor based on what data type is in the input.

When implementing Deserialize, you should avoid relying on Deserializer::deserialize_any unless you need to be told by the Deserializer what type is in the input. Know that relying on Deserializer::deserialize_any means your data type will be able to deserialize from self-describing formats only, ruling out Postcard and many others.
@afonsolage afonsolage added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Sep 10, 2022
@Weibye Weibye added A-Reflection Runtime information about types and removed S-Needs-Triage This issue needs to be labelled labels Sep 10, 2022
@MrGVSV
Copy link
Member

MrGVSV commented Oct 15, 2022

Would this be closed by #6140?

@MrGVSV MrGVSV moved this to Open in Reflection Oct 15, 2022
@MrGVSV MrGVSV mentioned this issue Oct 17, 2022
Closed
@MrGVSV MrGVSV moved this from Open to In Progress in Reflection Oct 17, 2022
@bors bors bot closed this as completed in 97f7a1a Nov 4, 2022
Repository owner moved this from In Progress to Done in Reflection Nov 4, 2022
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
@MrGVSV @ItsDoot
bevy_reflect: Binary formats (bevyengine#6140)
6e6b9a8 
# Objective

Closes bevyengine#5934

Currently it is not possible to de/serialize data to non-self-describing formats using reflection.

## Solution

Add support for non-self-describing de/serialization using reflection.

This allows us to use binary formatters, like [`postcard`](https://crates.io/crates/postcard):

```rust
#[derive(Reflect, FromReflect, Debug, PartialEq)]
struct Foo {
  data: String
}

let mut registry = TypeRegistry::new();
registry.register::<Foo>();

let input = Foo {
  data: "Hello world!".to_string()
};

// === Serialize! === //
let serializer = ReflectSerializer::new(&input, &registry);
let bytes: Vec<u8> = postcard::to_allocvec(&serializer).unwrap();

println!("{:?}", bytes); // Output: [129, 217, 61, 98, ...]

// === Deserialize! === //
let deserializer = UntypedReflectDeserializer::new(&registry);

let dynamic_output = deserializer
  .deserialize(&mut postcard::Deserializer::from_bytes(&bytes))
  .unwrap();

let output = <Foo as FromReflect>::from_reflect(dynamic_output.as_ref()).unwrap();

assert_eq!(expected, output); // OK!
```

#### Crates Tested

- ~~[`rmp-serde`](https://crates.io/crates/rmp-serde)~~ Apparently, this _is_ self-describing
- ~~[`bincode` v2.0.0-rc.1](https://crates.io/crates/bincode/2.0.0-rc.1) (using [this PR](bincode-org/bincode#586 This actually works for the latest release (v1.3.3) of [`bincode`](https://crates.io/crates/bincode) as well. You just need to be sure to use fixed-int encoding.
- [`postcard`](https://crates.io/crates/postcard)

## Future Work

Ideally, we would refactor the `serde` module, but I don't think I'll do that in this PR so as to keep the diff relatively small (and to avoid any painful rebases). This should probably be done once this is merged, though.

Some areas we could improve with a refactor:

* Split deserialization logic across multiple files
* Consolidate helper functions/structs
* Make the logic more DRY

---

## Changelog

- Add support for non-self-describing de/serialization using reflection.


Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-Reflection Runtime information about types C-Feature A new feature, making something new possible
Projects
Reflection
Status: Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Merged by Bors] - bevy_reflect: Binary formats MrGVSV/bevy
3 participants
@afonsolage @Weibye @MrGVSV