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

Fix false positives in remote desync detection #64

Closed johanhelsing closed 10 months ago

johanhelsing commented 10 months ago

Fixes #63 and also fixes a bug that checksums are removed prematurely if the interval is > MAX_CHECKSUM_HISTORY_SIZE

Confirmed working with the test case for #63

Bonus: Remote checksums are now only checked once (removed once verified).

johanhelsing commented 10 months ago

@HeatXD may want to review, because they did the initial implementation https://github.com/gschup/ggrs/pull/50

PraxTube commented 10 months ago

I keep getting the following panic right after peers connect in my bevy_ggrs project.

thread 'main' panicked at 'cell not found!: frame 100', ggrs-c119670c0a115c50/94604ce/src/sessions/p2p_session.rs:921:44

The frame not found seems to depend on the specified desync interval.

.with_desync_detection_mode(DesyncDetection::On { interval: 100 })

Changing the max prediction window and max frames behind didn't do much either

.with_max_prediction_window()
.with_max_frames_behind()

Is there anything that has to be configured in order for this PR to properly work in exisiting bevy_ggrs projects, or this not expected to work with bevy_ggrs at all yet?

johanhelsing commented 10 months ago

I keep getting the following panic right after peers connect in my bevy_ggrs project.

thread 'main' panicked at 'cell not found!: frame 100', ggrs-c119670c0a115c50/94604ce/src/sessions/p2p_session.rs:921:44

The frame not found seems to depend on the specified desync interval.

.with_desync_detection_mode(DesyncDetection::On { interval: 100 })

Changing the max prediction window and max frames behind didn't do much either

.with_max_prediction_window()
.with_max_frames_behind()

Is there anything that has to be configured in order for this PR to properly work in exisiting bevy_ggrs projects, or this not expected to work with bevy_ggrs at all yet?

It's expected to work with bevy_ggrs. But very possible that I overlooked something.

I tested with box_game_p2p on this branch https://github.com/gschup/bevy_ggrs/compare/main...johanhelsing:bevy_ggrs:trigger-desync-issues?expand=1 and in my own game.

johanhelsing commented 10 months ago

@PraxTube are you able to reproduce your panic by modifying this branch? https://github.com/gschup/bevy_ggrs/compare/main...johanhelsing:bevy_ggrs:desync-debug?expand=1

EDIT: Managed to by setting input_delay back to 2. Will look into the issue

PraxTube commented 10 months ago

Ah yeah, for input_delay = 0 I don't get the panic (I had it on 2 in my tests). The desync detection seems to work properly when using 0 delay.

johanhelsing commented 10 months ago

I added a test, which is good in itself, but for some reason I don't get the panic there :/

EDIT: managed to repro with a higher input delay :)

johanhelsing commented 10 months ago

This is ready for review now. There is one thing that bugs me (see comment above), but I'm giving up trying to understand it for now.

This is a lot more correct than the previous implementation. And has a test case that failed prior to this PR (and another one to make sure the panic reported by @PraxTube is fixed)