Rollback requests ignored when max_predictions number of local inputs is saved

[aleksa2808]

Describe the bug

Take a sample situation that can occur in a P2P session in laggy conditions:

1. A peer sends you input different than you predicted.
2. In adjust_gamestate you schedule a rollback/LoadGameState request and modify the session's internal state in preparation for it.
3. However, because of the laggy conditions, you already have max_predictions number of local inputs stored so you drop all created request and instead return a PredictionThreshold error from the advance_frame/add_local_input methods.
4. On the next advance_frame call, since the session state was modified in preparation for a rollback as if the rollback was guaranteed to happen, you now think there is no need to rollback anymore.

Following these steps leads to a desync as you received a peer's input that didn't match your prediction but didn't rollback.

Expected behavior

In the described situation I would expect the next advance_frame call to remake the requests which were cancelled in the last frame, including the rollback one.

[aleksa2808]

Here is a (messy) patch I'm using for my game currently. It boils down to checking if the local player should advance the frame (if it isn't predicting too far ahead) at the start of the advance_frame method and then skipping most things in the method if the frame is being skipped. This is in contrast to the current behavior of dropping all pending requests and returning an error if we decide mid-method that the local player's frame cannot be advanced.

[MaxCWhitehead]

Oh I totally missed that this issue already existed and opened #75, totally welcome to close my issue as dupe if desired.

@aleksa2808 I implemented my own attempt at fix here: #77 and a unit test to reproduce this case - will take a look at yours later and see how they compare.

[gschup]

Hi! thanks for posting this bug report. Sorry for not responding so far. I have just posted a PR that slightly alters the logic for handling inputs. I am not 100% sure this fixes this issue, but it might. Would you be able to test this again on the lockstep branch?
-> #79

[gschup]

Duplicate of #75.