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

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MaxCWhitehead
Copy link
Contributor

@MaxCWhitehead MaxCWhitehead commented Apr 11, 2024

Here are some changes and comments that seem to fix #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
Copy link
Owner

gschup commented Jun 3, 2024

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

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.

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.

PredictionThreshold error drops requests missing correction
2 participants