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 Custom Checksum Methods #85

Merged
merged 2 commits into from
Nov 8, 2023

Conversation

bushrat011899
Copy link
Contributor

Objective

As identified by @johanhelsing (and now experienced by myself in my own project!), it is currently quite cumbersome to add checksums for 3rd party types which did not already implement Hash. It is entirely possible, but it requires a substantial amount of boilerplate compared to types which implement Hash. Ideally, users could provide a custom method to map !Hash types into a checksum value.

Solution

I've modified the ComponentChecksumHashPlugin and ResourceChecksumHashPlugin plugins to take a parameter, hasher, which is a function describing how to hash the desired type. By default, hasher is a function using Hash if implemented:

pub struct ComponentChecksumHashPlugin<C: Component>(pub for<'a> fn(&'a C) -> u64);

fn default_hasher<C: Component + Hash>(component: &C) -> u64 {
    let mut hasher = bevy::utils::FixedState.build_hasher();
    component.hash(&mut hasher);
    hasher.finish()
}

// Note that we can only provide a default if `C` implements `Hash`
impl<C> Default for ComponentChecksumHashPlugin<C>
where
    C: Component + Hash,
{
    fn default() -> Self {
        Self(default_hasher::<C>)
    }
}

To add to the ergonomics, I've extended GgrsApp to directly accept this custom hashing method as an alternate method:

// Snipped from the Particles Stress Test
app.insert_resource(args)
    // Components can be added to the frame checksum automatically if they implement Hash...
    .checksum_component_with_hash::<Velocity>()
    // ...or you can provide a custom hashing process
    .checksum_component::<Transform>(|transform| {
        let mut hasher = bevy::utils::FixedState.build_hasher();

        // In this demo we only translate particles, so only that value
        // needs to be tracked.
        transform.translation.x.to_bits().hash(&mut hasher);
        transform.translation.y.to_bits().hash(&mut hasher);
        transform.translation.z.to_bits().hash(&mut hasher);

        hasher.finish()
    });

Notes

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.

@johanhelsing
Copy link
Collaborator

johanhelsing commented Nov 7, 2023

This is perhaps a more overarching change, but I was wondering if it would make sense to have traits for a type's preferred way of doing rollback and checksum.

e.g. RollbackCheksum.

We would then have extension method .checksum_component::<Foo>()

We can implement it for bevys built in types. And users could implement it for their own types.

Due to the orphan rule, there are still cases where we need .checksum_component_with_hash::<Bar>(), but there are possibly also other alternatives (see how bevy_reflect now does it, inspired by serde).

We could also implement the trait for glam types, like Vec3 and even add a derive macro for it, so all you'll have to do is:

#[derive(RollbackCheksum)]
struct Foo {
    vel: Vec3, // we've implemented it for Vec3, no annotations needed
    #[checksum(Hash)]]
    baz: PeerId,
    #[checksum(ignore)]
    internal_computed: u32
}

Just wanted to share my thoughts on it.

@bushrat011899
Copy link
Contributor Author

I do like the idea of a trait describing the checksum, but I think Rust's orphan rule and specialisation rules would get in the way of a really useful trait, beyond the current Hash at least. Ideally, we could provide a default implementation for RollbackChecksum for Hash types, and allow end users to override it as desired.

@johanhelsing
Copy link
Collaborator

I do like the idea of a trait describing the checksum, but I think Rust's orphan rule and specialisation rules would get in the way of a really useful trait, beyond the current Hash at least. Ideally, we could provide a default implementation for RollbackChecksum for Hash types, and allow end users to override it as desired.

That's not possible i think. I have some ideas for how to do this with a minimum of boiler plate, but perhaps it's better to discuss it over an actual pr, I'm hoping i get some time to work on it in a couple of weeks.

In the meantime, this pr is a step up over the current state, and would be great in the bevy 0.12 release.

@gschup
Copy link
Owner

gschup commented Nov 7, 2023

In the meantime, this pr is a step up over the current state, and would be great in the bevy 0.12 release.

Agreed! In my opinion, we should merge #80 and this PR before doing the next release (together with ggrs).

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.

Looking good to me!

@gschup gschup merged commit 9ae1503 into gschup:main Nov 8, 2023
1 check passed
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.

3 participants