Skip to content

feat: SyncTestSession GgrsEvent::MismatchedChecksum #98

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.

Sign up for GitHub

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

Closed
JorgeLGonzalez wants to merge 3 commits into from
Closed

feat: SyncTestSession GgrsEvent::MismatchedChecksum #98

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e4ab4bb
feat: SyncTestSession GgrsEvent::MismatchedChecksum
jorge-gonzalez-lexia Dec 21, 2024
458071d
lint: malformed references in doc comments
jorge-gonzalez-lexia Dec 21, 2024
bf7e74a
doc: improve comment doc ref to GgrsEvent
jorge-gonzalez-lexia Dec 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,15 @@ where
/// remote address of the endpoint.
addr: T::Address,
},
/// In a [`SyncTestSession`], sent whenever the checksums of resimulated frames do not match up with the original checksum.
///
/// [`SyncTestSession`]: crate::SyncTestSession
MismatchedChecksum {
/// The frame at which the mismatch occurred.
current_frame: Frame,
/// The oldest frame with mismatched checksum
mismatched_frame: Frame,
},
}

/// Requests that you can receive from the session. Handling them is mandatory.
Expand Down
1 change: 1 addition & 0 deletions src/sessions/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,7 @@ impl<T: Config> SessionBuilder<T> {
/// Due to the decentralized nature of saving and loading gamestates, checksum comparisons can only be made if `check_distance` is 2 or higher.
/// This is a great way to test if your system runs deterministically.
/// After creating the session, add a local player, set input delay for them and then start the session.
/// Optionally, inspect [`SyncTestSession::events`] to detect events such as [`crate::GgrsEvent::MismatchedChecksum`].
pub fn start_synctest_session(self) -> Result<SyncTestSession<T>, GgrsError> {
if self.check_dist >= self.max_prediction {
return Err(GgrsError::InvalidRequest {
Expand Down
30 changes: 27 additions & 3 deletions src/sessions/sync_test_session.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
use std::collections::HashMap;

use crate::error::GgrsError;
use crate::frame_info::PlayerInput;
use crate::network::messages::ConnectionStatus;
use crate::sync_layer::SyncLayer;
use crate::{Config, Frame, GgrsRequest, PlayerHandle};
use crate::{Config, Frame, GgrsEvent, GgrsRequest, PlayerHandle};
use std::collections::vec_deque::Drain;
use std::collections::{HashMap, VecDeque};

const MAX_EVENT_QUEUE_SIZE: usize = 100;

/// During a [`SyncTestSession`], GGRS will simulate a rollback every frame and resimulate the last n states, where n is the given check distance.
/// The resimulated checksums will be compared with the original checksums and report if there was a mismatch.
/// Optionally use [`SyncTestSession::events`] to detect events such as [`GgrsEvent::MismatchedChecksum`].
pub struct SyncTestSession<T>
where
T: Config,
Expand All @@ -18,6 +21,8 @@ where
sync_layer: SyncLayer<T>,
dummy_connect_status: Vec<ConnectionStatus>,
checksum_history: HashMap<Frame, Option<u128>>,
/// Contains all events to be forwarded to the user.
event_queue: VecDeque<GgrsEvent<T>>,
local_inputs: HashMap<PlayerHandle, PlayerInput<T::Input>>,
}

Expand Down Expand Up @@ -45,6 +50,7 @@ impl<T: Config> SyncTestSession<T> {
sync_layer,
dummy_connect_status,
checksum_history: HashMap::new(),
event_queue: VecDeque::new(),
local_inputs: HashMap::new(),
}
}
Expand Down Expand Up @@ -95,6 +101,11 @@ impl<T: Config> SyncTestSession<T> {
.collect();

if !mismatched_frames.is_empty() {
self.push_event(GgrsEvent::MismatchedChecksum {
Copy link
Copy Markdown
Owner

@gschup gschup Dec 21, 2024

Choose a reason for hiding this comment

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

Hi, thanks for the PR!

Why do you want SyncTestSession to emit a GgrsEvent::MismatchedChecksum in addition to returning GgrsError::MismatchedChecksum? Is there something you cannot do with error instead?

Copy link
Copy Markdown
Author

@JorgeLGonzalez JorgeLGonzalez Dec 21, 2024

Choose a reason for hiding this comment

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

I am using bevy_ggrs and have a system fn that checks the Session. For the P2P session variant, it can easily check any events, so it seemed desirable to have something similar for SyncTestSessions. bevy_ggrs calls advance_frame and handles error results by simply logging a warning. (This could certainly be misguided, btw!)

Copy link
Copy Markdown
Contributor

@caspark caspark Dec 22, 2024

Choose a reason for hiding this comment

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

I suggested/agreed that this would be a nice change to make, though for a different reason: my reasoning was that the actions I want to take on a checksum mismatch in a synctest session vs a normal p2p session (I wrap them to expose the same interface to each) are basically the same: log an error, dump game states, and end the game session (exit, return to main menu, return to lobby, etc). So having to check for desyncs in two different places makes that a little bit harder than it would be otherwise.

However, two things are a bit messy in this first cut:

  1. Having 2 ways to communicate a desync for a synctest session (returning result and the new event) is redundant.
    • I would remove the error variant and only rely on the event, thereby allowing the synctest session to advance as normal even though there was a desync detected. This is consistent with P2PSession's behavior of happily continuing on even in case of a desync.
  2. Having 2 separate events for desyncs complicates the handling code a bit if you want to do the same thing for each - but also there's different information available for synctest vs p2p desyncs, so I get why you went down this approach @JorgeLGonzalez .
    • I would instead try to reuse the DesyncDetected event, maybe something like this:
enum GgrsEvent {
    // ...
    DesyncDetected {
        /// First frame where the desync was detected.
        /// Note that in a [`P2PSession`](crate::P2PSession), this may be some time
        /// after the desync actually occurred if you have set desync detection to a
        /// value greater than 1.
        /// In a [`SyncTestSession`](crate::SyncTestSession), only the first
        /// frame that was detected to have a desync will have an event issued for it;
        /// subsequent desync-detected frames may also have events issued but the
        /// exact details of which events will be issued will vary based on when you
        /// poll and advance the session.
        frame: Frame,
        /// The local checksum for the given frame.
        local_checksum: u128,
        /// Remote checksum for the given frame.
        remote_checksum: u128,
        /// Remote address of the endpoint; only populated for [`P2PSession`](crate::P2PSession)
        /// desyncs.
        remote_addr: Option<T::Address>,
    },
}

Some thoughts on this sketch:

  • local and remote checksums isn't the best naming in a synctest context, but nothing better is immediately coming to mind. I do think there is some small value in copying the checksum values themselves into the event even for a synctest failure (will require a little more work) - I've used the them to diagnose & fix a bug in ggrs's p2p desync detection before.
  • Personally I don't think there's value in including the full set of desync'd frames from a synctest session in the event, but if we still want that then that could be another optional field or we could change the contract so that an event is issued for each desync-detected frame.

@JorgeLGonzalez just to be clear, these are my opinions based on using ggrs and raising a couple PRs lately, but I'm not a ggrs maintainer, so don't take my words as law. Defer to @gschup :)

Copy link
Copy Markdown
Author

@JorgeLGonzalez JorgeLGonzalez Dec 22, 2024

Choose a reason for hiding this comment

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

Thanks @caspark . I have no strong opinion here. My first instinct was to use DesyncDeteced, but I didn't know what to set for addr and making it an Option seemed too invasive, so then I tried to mirror the error payload but using only the first mismatched frame since Vec isn't Copy. I think I see how to add the checksums.
I can make those changes and will await @gschup to see if and how this should PR be moved forward.

current_frame,
mismatched_frame: mismatched_frames[0],
});

return Err(GgrsError::MismatchedChecksum {
current_frame,
mismatched_frames,
Expand Down Expand Up @@ -169,6 +180,11 @@ impl<T: Config> SyncTestSession<T> {
self.check_distance
}

/// Returns all events that happened since last queried for events. If the number of stored events exceeds `MAX_EVENT_QUEUE_SIZE`, the oldest events will be discarded.
pub fn events(&mut self) -> Drain<GgrsEvent<T>> {
self.event_queue.drain(..)
}

/// Updates the `checksum_history` and checks if the checksum is identical if it already has been recorded once
fn checksums_consistent(&mut self, frame_to_check: Frame) -> bool {
// remove entries older than the `check_distance`
Expand Down Expand Up @@ -215,4 +231,12 @@ impl<T: Config> SyncTestSession<T> {
}
assert_eq!(self.sync_layer.current_frame(), start_frame);
}

fn push_event(&mut self, event: GgrsEvent<T>) {
while self.event_queue.len() >= MAX_EVENT_QUEUE_SIZE {
self.event_queue.pop_front();
}

self.event_queue.push_back(event);
}
}