search

gschup / ggrs

GGRS is a reimagination of GGPO, enabling P2P rollback networking in Rust. Rollback to the future!
Other
507 stars 25 forks source link

Possible fix for PredictionThreshold error dropping rollback requests causing desync #77

Open MaxCWhitehead opened 4 months ago

MaxCWhitehead commented 4 months ago

Here are some changes and comments that seem to fix https://github.com/gschup/ggrs/issues/75

This is a draft PR as I am not necessarily suggesting we merge this as is (especially the test - but included for reference to help see what conditions cause this issue. Putting in PR mostly so easier to comment on specific pieces of diff here.

This fixes the test, and alsp seems to prevent any further desync in Jumpy.

Overview

The core of the problem is that when adding input at max prediction, we return PredictThreshold error and then requests are not returned and processed by client. By this point, we had already cleared first_incorrect_frame, updated confirmed frame on sync layer, and dropped inputs from before confirmed frame. This is a problem because the pre-confirmed frame inputs sent to be used in correction were not actually processed and are now gone.

Now we speculatively check if client will hit prediction error earlier in advance_frame. At this point, sync_layer.confirmed_frame has not been updated, so we use the 'speculative' confirmed frame local value in advance_frame.

We now only reset first_incorrect_frame and commit speculative confirmed frame to sync layer if and only if we are certain we are not at max prediction window. This means if we do error, in future frames we will still determine we need to rollback due to first_incorrect_frame being preserved, and our inputs / last confirmed frame are still valid.

Other Notes:

I put a couple TODOs around other spots I expect will cause desync, have not handled all failure cases related to this issue. Will investigate those further later + repro desync for those cases in their own tests to verify.

I also have not reviewed sync test session or any spectator code, possibly related issues there.

gschup commented 3 months ago

Sorry for not responding until now. Thanks for the PR and special shoutout to all the test code you wrote! 💪

I will take a look, see how it interacts with #79 and will follow up later this week.

MaxCWhitehead commented 3 months ago

Sounds good - no rush on reviewing this at all. It seems to have been working so far for our purposes - but I am now currently moving so I probably won't be looking at anything here for a bit / doing any cleanup on this PR in near future.