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

DynamicScene (de)serialization to postcard and bincode fails for some components. #6713

Closed
sQu1rr opened this issue Nov 21, 2022 · 9 comments
Assignees
Labels
A-Reflection Runtime information about types C-Bug An unexpected or incorrect behavior

Comments

@sQu1rr
Copy link
Contributor

sQu1rr commented Nov 21, 2022

Bevy version

0.9.0

What you did

According to the changelog, Bevy 0.9.0 should now support the (de)serialization of the DynamicScene with postcard and bincode. #6140 was merged and includes tests for both bincode and postcard, which do work for me.

However, when I try to (de)serialize to binary in practice, bevy fails. When deserializing SpotLightBundle and CameraBundle, for example, bincode fails with no field at index 0 on struct bevy_render::primitives::Frustum. For the PointLightBundle it's no field at index 0 on struct bevy_render::primitives::CubemapFrusta. There could be other examples too.

What surprised me the most is the behaviour of a seemingly innocent empty entity marker:

#[derive(Component, Serialize, Deserialize, Reflect, Default)]
#[reflect(Component)]
struct Marker;

It seems to work at first, but if I call app.register_type::<Marker>(), it gives an error on deserialization. When transferring data over the network, I register the incoming types on the client side, so it is important for me that it works.

(De)serializing into ron works as expected.

What went wrong

what were you expecting?

I expect the (de)serialization to work identically well whether I serialize to ron, or to binary formats. I also expect the component to (de)serialize regardless of whether it was registered manually or automatically.

I would also like the postcard to give more informative errors. As you can see from the error output provided below, they are very generic and unhepful.

Additional information

Here is an example that tests different entities (de)serialization into ron, postcard and bincode: https://pastebin.com/wxGvE9ke
Due to the issue #6573, I am using the unmerged PR #6580 to avoid failing on deserializing GlobalTransform.

Update: The actual error happens during deserialization, so made some edits to avoid confusion. However, I am not sure whether the actual issue is due to incorrect serialization or deserialization or both.

Output

Test #0: Automatically registered marker
        ron       : OK
        bincode   : OK
        postcard  : OK
Test #1: Pre-registered marker
        ron       : OK
        bincode   : Error! no field at index 0 on struct test_postcard_roundtrip::RegisteredMarker
        postcard  : Error! Serde Deserialization Error
Test #2: PbrBundle
        ron       : OK
        bincode   : OK
        postcard  : OK
Test #3: Spot Light
        ron       : OK
        bincode   : Error! no field at index 0 on struct bevy_render::primitives::Frustum
        postcard  : Error! Serde Deserialization Error
Test #4: Point Light
        ron       : OK
        bincode   : Error! no field at index 0 on struct bevy_render::primitives::CubemapFrusta
        postcard  : Error! Serde Deserialization Error
Test #5: Camera
        ron       : OK
        bincode   : Error! no field at index 0 on struct bevy_render::primitives::Frustum
        postcard  : Error! Serde Deserialization Error

@sQu1rr sQu1rr added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Nov 21, 2022
@alice-i-cecile alice-i-cecile added A-Reflection Runtime information about types and removed S-Needs-Triage This issue needs to be labelled labels Nov 21, 2022
@sQu1rr sQu1rr changed the title DynamicScene serialization to postcard and bincode fails for some components. DynamicScene (de)serialization to postcard and bincode fails for some components. Nov 21, 2022
@MrGVSV
Copy link
Member

MrGVSV commented Nov 21, 2022

Hm, looks like the deserializer is not properly accounting for structs with zero reflectable fields, including unit structs and structs where all fields are ignored (e.g. Frustum and CubemapFrusta).

Will look into this. Thank you for such a detailed writeup, this is very helpful!

@MrGVSV
Copy link
Member

MrGVSV commented Nov 21, 2022

@sQu1rr could you run your tests using the changes made in #6722 and verify it resolves this issue?

@sQu1rr
Copy link
Contributor Author

sQu1rr commented Nov 21, 2022

I am now getting a seemingly unrelated error

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Message("no registration found for dynamic type with name bevy_render::view::visibility::ComputedVisibilityFlags")', src/main.rs:78:58
n

Looks like it is about the changes in: #6305
Not sure how I can work around it to test

@MrGVSV
Copy link
Member

MrGVSV commented Nov 21, 2022

I am now getting a seemingly unrelated error

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Message("no registration found for dynamic type with name bevy_render::view::visibility::ComputedVisibilityFlags")', src/main.rs:78:58
n

Looks like it is about the changes in: #6305 Not sure how I can work around it to test

Sounds like it just needs to be registered in bevy_render. Can you try registering it manually temporarily? I'll put up a PR (or issue) for the registration.

@sQu1rr
Copy link
Contributor Author

sQu1rr commented Nov 21, 2022

ComputedVisibilityFlags is not public, so I cannot access it at the moment

@MrGVSV
Copy link
Member

MrGVSV commented Nov 21, 2022

ComputedVisibilityFlags is not public, so I cannot access it at the moment

Could you register it directly in the source of bevy_render just for testing purposes?

@sQu1rr
Copy link
Contributor Author

sQu1rr commented Nov 22, 2022

I can confirm that pub ComputedVisibilityFlags + registration + #6580 together with #6722 fix all issues for me (https://github.com/sQu1rr/bevy/tree/testing-dynamicscene-reflect). I tested it on the minimal example I provided, and also in my networking code, where I discovered the issue in the first place. It works like a charm so far with both bincode and postcard.

Would be very useful to fix the postcard errors though, as new issues may appear in the future, and they are quite unhelpful.

@MrGVSV
Copy link
Member

MrGVSV commented Nov 22, 2022

I can confirm that pub ComputedVisibilityFlags + registration + #6580 together with #6722 fix all issues for me (https://github.com/sQu1rr/bevy/tree/testing-dynamicscene-reflect). I tested it on the minimal example I provided, and also in my networking code, where I discovered the issue in the first place. It works like a charm so far with both bincode and postcard.

Awesome! Glad to hear that, I'll work on getting the registration fixes underway. Could you give it your review/approval?

Would be very useful to fix the postcard errors though, as new issues may appear in the future, and they are quite unhelpful.

I agree. I'm not sure why the error gets replaced when deserializing with postcard. But that might be an upstream issue rather than something Bevy is doing wrong (I think). Maybe consider making an issue on the postcard repo?

@MrGVSV
Copy link
Member

MrGVSV commented Nov 22, 2022

For reference, I just registered the missing bevy_render types in this PR.

@bors bors bot closed this as completed in 4e23743 Nov 23, 2022
Shatur pushed a commit to projectharmonia/bevy that referenced this issue Nov 25, 2022
…bevyengine#6722) [skip ci]

Fixes bevyengine#6713

Binary deserialization is failing for unit structs as well as structs with all ignored/skipped fields.

Add a check for the number of possible fields in a struct before deserializing. If empty, don't attempt to deserialize any fields (as there will be none).

Note: ~~This does not apply to enums as they do not properly handle skipped fields (see bevyengine#6721).~~ Enums still do not properly handle skipped fields, but I decided to include the logic for it anyways to account for `#[reflect(ignore)]`'d fields in the meantime.

---

- Fix bug where deserializing unit structs would fail for non-self-describing formats
cart pushed a commit that referenced this issue Nov 30, 2022
…#6722)

# Objective 

Fixes #6713

Binary deserialization is failing for unit structs as well as structs with all ignored/skipped fields.

## Solution

Add a check for the number of possible fields in a struct before deserializing. If empty, don't attempt to deserialize any fields (as there will be none).

Note: ~~This does not apply to enums as they do not properly handle skipped fields (see #6721).~~ Enums still do not properly handle skipped fields, but I decided to include the logic for it anyways to account for `#[reflect(ignore)]`'d fields in the meantime.

---

## Changelog

- Fix bug where deserializing unit structs would fail for non-self-describing formats
@MrGVSV MrGVSV mentioned this issue Dec 3, 2022
3 tasks
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
…bevyengine#6722)

# Objective 

Fixes bevyengine#6713

Binary deserialization is failing for unit structs as well as structs with all ignored/skipped fields.

## Solution

Add a check for the number of possible fields in a struct before deserializing. If empty, don't attempt to deserialize any fields (as there will be none).

Note: ~~This does not apply to enums as they do not properly handle skipped fields (see bevyengine#6721).~~ Enums still do not properly handle skipped fields, but I decided to include the logic for it anyways to account for `#[reflect(ignore)]`'d fields in the meantime.

---

## Changelog

- Fix bug where deserializing unit structs would fail for non-self-describing formats
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-Bug An unexpected or incorrect behavior
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants