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

Support for Serde Rollback #86

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

bushrat011899
Copy link
Contributor

Objective

In my personal project, I've hit an issue where the Rapier library has a large state object, RapierContext, which cannot be cloned, copied, or reflected for snapshotting. However, it does implement Serde Serialize + Deserialize. It would be convenient and broadly applicable if snapshots natively supported Serde.

Solution

I've created an optional snapshot strategy using Serde and Postcard, gated by the newly added serde feature:

// With serde feature enabled
app.rollback_resource_with_postcard::<RapierContext>();

Notes

I haven't done extensive performance testing with this strategy, but I think it's a safe bet it's probably the slowest. This is why I've chosen Postcard as the target format for de/serialization, as it is designed for embedded no-std use, where performance would be highly valued.

I also don't believe Rapier requires this addition to work, and this could also be added by an end user entirely through the current public API. However, I can imagine many others potentially reaching for this functionality, and I wrote it for myself, so I figured I might as well offer it here as well!

Finally, I've made this branch based off of my #83 for testing purposes, but these changes could be easily ported to any alternate Bevy 0.12 upgrade branch as required.

@gschup
Copy link
Owner

gschup commented Nov 7, 2023

I remember writing a ggrs rapier demo at some point in the past, where I had to serialize the RapierContext, too. Performance wise, It was not very good. I am not sure we should offer this way of saving and loading state as a part of the crate. I am worried it might mislead users to use it in all the wrong places :)

Is there a way users can add this plugin themselves without it being distributed as part of this crate? What do you think @johanhelsing ?

@bushrat011899
Copy link
Contributor Author

I remember writing a ggrs rapier demo at some point in the past, where I had to serialize the RapierContext, too. Performance wise, It was not very good. I am not sure we should offer this way of saving and loading state as a part of the crate. I am worried it might mislead users to use it in all the wrong places :)

Is there a way users can add this plugin themselves without it being distributed as part of this crate? What do you think @johanhelsing ?

Oh yes, that's why I was so excited to have the SaveWorld and LoadWorld schedules added, because my entire PR (ignoring the convenience methods added to GgrsApp) could be collapsed to:

use std::marker::PhantomData;
use postcard::{from_bytes, to_allocvec};
use serde::{de::DeserializeOwned, Serialize};
use crate::Strategy;

/// A [`Strategy`] based on [`Reflect`] and [`FromWorld`]
pub struct PostcardStrategy<T: Serialize + DeserializeOwned>(PhantomData<T>);

impl<T: Serialize + DeserializeOwned> Strategy for PostcardStrategy<T> {
    type Target = T;

    type Stored = Vec<u8>;

    #[inline(always)]
    fn store(target: &Self::Target) -> Self::Stored {
        to_allocvec(target).unwrap()
    }

    #[inline(always)]
    fn load(stored: &Self::Stored) -> Self::Target {
        from_bytes(stored).unwrap()
    }
}

// ...

app.add_plugins(ComponentSnapshotPlugin::<PostcardStrategy<RapierContext>>::default());

Where the only type imported is the public Strategy. That's part of my motivation for keeping the plugins behind GgrsApp public, as it allows for some pretty clean end-user customisation. If you don't believe this is a good fit for the main repo I am totally fine with leaving this as something I personally use in my project. I just thought I'd share it as a PR since I already made it.

Copy link
Collaborator

@johanhelsing johanhelsing left a comment

Choose a reason for hiding this comment

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

I think it could be useful to have this for cases like the rapier serialization.

Cargo.toml Outdated
@@ -13,7 +13,9 @@ categories = ["network-programming", "game-development"]
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[features]
default = ["serde"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps not enable it by default? Does bevy depend on serde or postcard?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! I've had a look through bevy, matchbox (with ggrs feature enabled), and ggrs. Out of those three crates, serde version 1 is always included, ron version 0.8 is included (from bevy), and bincode is included (matchbox and ggrs)

I've updated this PR to use ron and bincode instead, as they are already in the dependency graph. I've still kept them behind feature flags so you can remove them if desired.

@cscorley
Copy link

cscorley commented Nov 19, 2023

Just wanted to call-out here that while this strategy is cool and could be very useful for integrating with Rapier directly via your own RapierContext, I'm not sure it's compatible with bevy_rapier directly.

There are several HashMaps that are not serialized as part of the bevy_rapier::RapierContext which map between Bevy Entity to various Rapier *Handle types (e.g., entity2collider). Without this, saving/restoring directly to the bevy_rapier::RapierContext means the plugin loses its context immediately and bevy_rapier will not work for anything the App changes (user inputs to update Transforms, Velocity, etc.). Regular, non-Bevy physics continues to work, however.

I am curious to know how you've handled this case. For me, I've settled on creating N "blessed entities" and initializing Rapier with all of them, promising myself I can only rollback those N entities. Later, I will do all the (de)serializing directly and then only updating the appropriate properties on the bevy_rapier::RapierContext.

(I have not looked into this yet, but I'm assuming those maps are not serialized because Entity isn't serializable. I don't see why Entity couldn't be, though! Turns out it is (just thru direct impl instead of derive, which I was looking for) and I think it's a possible future now?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants