feat: SyncTestSession GgrsEvent::MismatchedChecksum

[JorgeLGonzalez]

SyncTestSession will expose events similar to P2PSession allowing consumers to handle events such as MismatchedCheckum.

Suggested minor enhancement for SyncTestSession after discussions with @caspark.

I am FAR from having my head around GGRS, so forgive my fumbling. Note I added a variant to the GgrsEvent enum, which would cause problems to any previously exhaustive match, for example.

[gschup]

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?

[JorgeLGonzalez]

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!)

[caspark]

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 :)

[JorgeLGonzalez]

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.

[caspark]

Not a huge deal but to avoid the warning suppression you can do this in doc comments:

/// ... detect events such as [`MismatchedChecksum`](crate::GgrsEvent::MismatchedChecksum)