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

Remove requirement of Hashing from GameState #17

Closed
ValorZard opened this issue Jun 25, 2021 · 2 comments
Closed

Remove requirement of Hashing from GameState #17

ValorZard opened this issue Jun 25, 2021 · 2 comments

Comments

@ValorZard
Copy link
Contributor

ValorZard commented Jun 25, 2021

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
Copy link

ErnWong commented Jul 15, 2021

(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?

pub fn save(self, state: T::State) {
let mut hasher = DefaultHasher::new();
state.hash(&mut hasher);
self.save_with_hash(state, hasher.finish());
}

But there's already the ability to save the state without a hash and with an explicit hash:

/// Saves a single frame's state to the session's state buffer without
/// a saved checksum.
///
/// This consumes the SaveState, saving multiple times is not allowed.
pub fn save_without_hash(self, state: T::State) {
self.save_state(state, None);
}
/// Saves a single frame's state to the session's state buffer with a
/// provided checksum.
///
/// This consumes the SaveState, saving multiple times is not allowed.
pub fn save_with_hash(self, state: T::State, checksum: u64) {
self.save_state(state, Some(checksum));
}

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
Copy link
Member

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 014fd15; 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.

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

No branches or pull requests

3 participants