HouraiTeahouse / backroll-rs

A (almost) 100% pure safe Rust implementation of GGPO-style rollback netcode.
ISC License
351 stars 20 forks source link

Remove requirement of Hashing from GameState #17

Closed ValorZard closed 3 years ago

ValorZard commented 3 years ago

Currently in Backroll, gamestates that get saved and loaded HAVE to be able to be hashed. This leads to a lot of issues, since important gamestate stuff like floats and vectors aren't able to be hashed. This should be replaced with something more universal. Note: as far as I know I think this issue is with bevy_backroll but it could extend to the original backroll library as well

ErnWong commented 3 years ago

(Copying this from a discord conversation)

FYI, as a temporary workaround, I'm manually serializing vec2s into OrderedFloat<f32> for the fishgame backroll integration prototype.

Is the hash requirement only used for creating checksums of the state? https://github.com/HouraiTeahouse/backroll-rs/blob/3c6bdc08a17d2f4e87524595a935d7f28de8e7a4/backroll/src/command.rs#L65-L69

But there's already the ability to save the state without a hash and with an explicit hash: https://github.com/HouraiTeahouse/backroll-rs/blob/3c6bdc08a17d2f4e87524595a935d7f28de8e7a4/backroll/src/command.rs#L71-L85

To remove the Hash requirement, would it be enough to conditionally implement the save method whenever Config::State impls Hash? I'm assuming that SaveState::save is not relied upon internally by backroll, but I haven't checked it thoroughly yet.

For example, something like this seems to compile. Rust Playground

struct A<T>(std::marker::PhantomData<T>);

impl<T> A<T> {
    pub fn save(state: T) where T: std::hash::Hash {
        let mut hasher = std::collections::hash_map::DefaultHasher::new();
        state.hash(&mut hasher);
        todo!();
    }
    pub fn save_without_hash(_state: T) {
        todo!();
    }
    pub fn save_with_hash(_state: T) {
        todo!();
    }
}
james7132 commented 3 years ago

Thought this would be harder given that Rust does not allow constraining on associated types, but turns out we just needed to restructure the generic type parameters a bit, and it does indeed work. This should be fixed as of 014fd15da35a74a3fed620f44d0f5bd68a506f48; however, this has led to a silent failure case show in #20. This should be fixed in some user visible way before releasing the next public version of backroll and the bevy plugin.