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

Fix max rollback and lockstep handling #88

Merged

Conversation

caspark
Copy link
Contributor

@caspark caspark commented Dec 14, 2024

NB: This builds on PR #82 so we better merge that one first to make this diff easier to review. (is Github working on stacked diff support yet?)


This fixes several issues that (I am fairly certain) came from merging #79:

  • When at the limit of the prediction window, rolling back would crash. This is fairly easy to trigger by adding a lot of latency in a network simulator. The cause was an off by one error in SavedStates' calculation of how many states need to be saved.
  • Clearer logic controlling when it's okay to request advancing the frame. I am not 100% certain but I suspect that the Fix/Feature: Lockstep #79 logic wasn't handling NULL_FRAME correctly? In any case I think this new structure makes it a lot easier to reason about.
  • Lockstep mode specific stuff:
    • Document lockstep mode as first class thing.
    • Fix disconnect handling in lockstep mode; previously the game would crash if a player disconnects (trying to rollback to the current frame, or perhaps the frame before the current? I can't quite remember). The fix is to skip all the "should we roll back" logic entirely if lockstep mode is on.
    • Make lockstep mode not issue save requests ever, since that's pointless - and besides, the fix for the previous bullet makes this almost necessary.
      • Because of this, we also need to turn off sparse saving mode in lockstep mode, since sparse saving mode somewhat confusingly causes us to not mark frames as confirmed, and not having frames confirmed means we won't advance.
      • Update example to more easily catch the case where lockstep mode is on but somehow we got issued a save or rollback request, by panicking in that case.

I have tested manually with 2 and 3 player example games, with lockstep mode and 8 frames of prediction and with sparse saving on and off. In a previous version of these fixes (those on the main branch of my personal fork) I also did a lot of testing with high latency and such to verify the "rollback when at limit of prediction window" fix, but in the name of full disclosure, I haven't done that with this branch specifically (I have a smallish test harness that makes it easy to do that testing with my game and my ggrs fork).

Anyway, based on that testing, everything seems to work.. but this is fiddly stuff so 🤞. I am at least pretty confident that it's more reliable than what's in main currently :)

// in lockstep mode, saving will never happen, but we use the last saved frame to mark
// control marking frames confirmed, so we need to turn off sparse saving to ensure that
// frames are marked as confirmed - otherwise we will never advance the game state.
false
Copy link
Contributor Author

@caspark caspark Dec 14, 2024

Choose a reason for hiding this comment

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

Ideally we'd log a warning (or at least at debug level) here to let the user know their sparse saving request is being ignored, but we don't have a logging framework right now.

Edit: raised #89 to discuss adding some logging.

@caspark caspark mentioned this pull request Dec 14, 2024
@gschup
Copy link
Owner

gschup commented Dec 14, 2024

this looks good, but the commit history needs to be cleaned up. thanks for all the good work on ggrs :)

As a result of gschup#79, the synclayer and
the logic in p2psession's advance_frame() were off by one in their
agreement on the number of world states that needed to be stored.

This fixes that so that they agree.
By making sure that in lockstep mode we never issue save or load
requests, we avoid various edge cases, such as trying to rollback to
the frame where the disconnect happened when we don't actually have any
historical game states stored.

However, we do need to make sure that sparse saving is off, otherwise
the game won't advance, because with sparse saving we'll end up never
marking a frame as confirmed.

This also cleans up some convoluted logic for deciding whether to
advance frames, which was also possibly incorrectly handling the case
where no frames were yet confirmed.

Lastly, we document lockstep mode as a first class thing.
@caspark caspark force-pushed the fix-max-rollback-and-disconnects-and-lockstep branch from e837824 to dd8b30d Compare December 14, 2024 23:49
@caspark
Copy link
Contributor Author

caspark commented Dec 14, 2024

Oh, sorry, I had actually squashed some commits locally to remove the WIP and dead end commits, but I forgot to actually push that! Have rebased onto main again now.

@gschup gschup merged commit c2814a1 into gschup:main Dec 15, 2024
2 checks 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.

2 participants