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: Binary formats #6140

Closed
wants to merge 5 commits into from

Conversation

MrGVSV
Copy link
Member

@MrGVSV MrGVSV commented Oct 2, 2022

Objective

Closes #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:

#[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

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.

@MrGVSV MrGVSV added C-Feature A new feature, making something new possible A-Reflection Runtime information about types labels Oct 2, 2022
@@ -73,6 +73,7 @@ pub struct StructInfo {
type_name: &'static str,
type_id: TypeId,
fields: Box<[NamedField]>,
field_names: Box<[&'static str]>,
Copy link
Member Author

@MrGVSV MrGVSV Oct 2, 2022

Choose a reason for hiding this comment

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

@cart I know we decided against this in this previous comment, but I discovered that the deserializer.deserialize_struct hint is actually used at least by bincode and postcard to determine how many fields exist in struct-like types.

If we don't want to support bincode we could just remove it.

Alternatively, we could attempt to split the logic based on deserializer.is_human_readable or something. Then if we're deserializing a non-self-describing format (such as bincode), we manually deserialize it as a tuple like deserializer.deserialize_tuple(struct_info.field_len(), StructVisitor{...}). And if it is human-readable, we just handle it as we currently do.

One concern with that is there might still be a difference between how certain formats deserialize. I actually ran into an issue where rmp-serde deserializes variant names as identifiers while bincode deserializes them as integers. This meant a simple is_human_readable couldn't actually handle both cases and a brand new VariantVisitor had to be made.

So any thoughts on this with the new context? Keep this change? Replace it with the human-readable check? Something else?

Copy link
Member Author

@MrGVSV MrGVSV Oct 3, 2022

Choose a reason for hiding this comment

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

My variant name issue might actually be related to this comment. So the split might not be as bad as originally thought.

The only other concern would be making that file a lot more complicated. But I think that can be addressed later with a good ol' refactor (unless we'd prefer to do the split and refactor at the same time).

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine to wait to refactor after.

@MrGVSV MrGVSV added this to the Bevy 0.9 milestone Oct 3, 2022
@phaux
Copy link

phaux commented Oct 3, 2022

I'm pretty sure that rmp-serde is a self-describing format because it's like CBOR and (de)serializing scene to/from CBOR already worked.

@MrGVSV
Copy link
Member Author

MrGVSV commented Oct 3, 2022

I'm pretty sure that rmp-serde is a self-describing format because it's like CBOR and (de)serializing scene to/from CBOR already worked.

Oh didn't know that. I just assumed it wasn't self-describing because it was a binary format haha. Thanks for letting me know!

Yeah adding the deserialize_any call back in still allows rmp-serde to work. bincode and postcard both fail, though.

Copy link
Contributor

@tim-blackbird tim-blackbird left a comment

Choose a reason for hiding this comment

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

I don't have much insight into the implementation so just a nit :)

crates/bevy_reflect/src/enums/enum_trait.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/enums/variants.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/struct_trait.rs Outdated Show resolved Hide resolved
@zicklag
Copy link
Member

zicklag commented Oct 18, 2022

I didn't look closely enough into the implementation to give any feedback, but I did just run into this exact issue today. I'm using reflection to power the serialization of components for sending over the network, and I'd like to be able to use postcard for it's size and speed.

For now I'll have to use a different data format, so this would definitely be appreciated!

@MrGVSV
Copy link
Member Author

MrGVSV commented Oct 20, 2022

It was discovered on Discord that the scene deserializer does not respect non-self-describing formats. I've pinpointed the issue and will work on a fix so that this PR will actually be useful to people using scenes (and not just reflect). Blocking this in the meantime.

@MrGVSV MrGVSV added S-Blocked This cannot move forward until something else changes and removed S-Blocked This cannot move forward until something else changes labels Oct 20, 2022
@MrGVSV
Copy link
Member Author

MrGVSV commented Oct 20, 2022

DynamicScene (de)serializers should now support non-self-describing formats!

I added in tests for both postcard and bincode (since they seem to sometimes differ slightly) to ensure that we catch regressions a little quicker/easier.

@alice-i-cecile alice-i-cecile added the D-Complex Quite challenging from either a design or technical perspective. Ask for help! label Oct 31, 2022
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

This LGTM now, and @inodentry was helpful to my understanding of this PR as well. I think those nits would be nice to address, but I wouldn't block on it.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Oct 31, 2022
@MrGVSV
Copy link
Member Author

MrGVSV commented Oct 31, 2022

@mockersf Reverted the change. While cleaner, I forgot that there's no guarantee that we get the names back in the proper order from the HashMap.

@mockersf
Copy link
Member

oh makes sense if they need to be ordered 👍

MrGVSV and others added 3 commits October 31, 2022 14:42
Now tests self-describing and non-self-describing formats.
@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Oct 31, 2022
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me! Ultimately I think we'll want a completely different set of serializers and deserializers for binary formats, but this is a pretty easy win that unlocks these scenarios now. Great work!

@cart
Copy link
Member

cart commented Nov 4, 2022

bors r+

bors bot pushed a commit that referenced this pull request Nov 4, 2022
# Objective

Closes #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 <[email protected]>
@zicklag
Copy link
Member

zicklag commented Nov 4, 2022

Ultimately I think we'll want a completely different set of serializers and deserializers for binary formats

I was wondering about that. In one iteration of a networking solution for a game I was working on, I ended up making a compact serializer/deserializer that would use a string cache to reduce the type names to indexes in order to reduce bandwidth usage.

I'm not sure if that's too specific to want to work into Bevy, but we could do something like that if we wanted to.

@bors bors bot changed the title bevy_reflect: Binary formats [Merged by Bors] - bevy_reflect: Binary formats Nov 4, 2022
@bors bors bot closed this Nov 4, 2022
@MrGVSV
Copy link
Member Author

MrGVSV commented Nov 4, 2022

I ended up making a compact serializer/deserializer that would use a string cache to reduce the type names to indexes in order to reduce bandwidth usage.

I actually have a branch where I implemented that haha. But yeah talking with Cart about it, it's likely something we want to create a specialized mechanism for to be as efficient as possible, and reduce inadvertently placing limitations on the human-readable (de)serializers or vice-versa.

@MrGVSV MrGVSV deleted the reflect-binary branch November 8, 2022 06:09
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# 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 <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Mar 12, 2024
# Objective

The `U32Visitor` struct has been unused since its introduction in #6140.
It's made itself known now by causing a recent [CI
failure](https://github.com/bevyengine/bevy/actions/runs/8243333274/job/22543736624).

## Solution

Remove the unused `U32Visitor` struct.

Also removed `PrepassLightsViewFlush` as it was causing a [similar CI
failure](https://github.com/bevyengine/bevy/actions/runs/8243838066/job/22545103746?pr=12433#step:6:269)
on this PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Feature A new feature, making something new possible D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Controversial There is active debate or serious implications around merging this PR
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Alternative to deserialize_any on ReflectDeserializer
7 participants