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

Solve inconsistency issues in client gametests #4334

Merged

Conversation

Earthcomputer
Copy link
Contributor

@Earthcomputer Earthcomputer commented Dec 28, 2024

This PR fixes the two biggest remaining consistency issues with client gametests that I have encountered:

  • I had assumed that the vanilla task queue on the client runs every tick, but it actually runs every frame, leading to ticks sometimes not running when waitTick is run.
    • I now allow multiple iterations of the client task loop to run (i.e. multiple frames) within the client-server tick synchronizer, and only hand control back to the gametest thread when there is going to be a tick this frame.
    • I also limit the number of ticks in a single frame to 1. This simplifies the implementation compared to what it otherwise would be, and also prevents there from being ticks where tasks are not run (and packets are not handled).
  • I had assumed that packets in singleplayer reach the other side immediately, and that synchronizing the vanilla task loops was enough to make packets consistent. In fact, even though packets are not serialized in singleplayer, they can still take an arbitrarily long amount of time to reach the receiving side (but typically don't take long) just as they can in multiplayer. This was a silly assumption in hindsight.
    • I have introduced NetworkSynchronizer to solve this issue.
    • A packet can be either "in-flight", which is between the time it is sent and the time it is handled on the Netty thread on the receiving side, or it can be queued for handling on the receiving main thread, which it between when it is handled on the Netty thread and it is removed from the main thread task queue.
      • Some packets are handled directly on the Netty thread and never enter the second stage. The NetworkSynchronizer is careful not to assume that all packets must be added to the task queue.
    • Once the packets are tracked in this way, the key change is that the client and server now continue running their task loops until there are no in-flight packets and no packets waiting to be handled in the vanilla task queues.
    • Network synchronization can be disabled via a system property, which is useful for mods which directly interface with the Netty pipeline, which would desynchronize the in-flight packet counter.

@Earthcomputer Earthcomputer marked this pull request as draft December 29, 2024 08:36
@Earthcomputer Earthcomputer changed the title Make sure client locking only happens if there will actually be a tick Solve inconsistency issues in client gametests Dec 29, 2024
@Earthcomputer Earthcomputer force-pushed the client-gametest-locking-once-per-tick branch from 69c045f to 707004e Compare January 5, 2025 17:59
@Earthcomputer Earthcomputer marked this pull request as ready for review January 7, 2025 16:51
…entally change this value without using the setter.
@modmuss50 modmuss50 added enhancement New feature or request test priority:high High priority PRs that need review and work now. Review these first. labels Jan 7, 2025
Copy link
Member

@modmuss50 modmuss50 left a comment

Choose a reason for hiding this comment

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

Im not going to pretend to understand all the logic, but as with the previous PRs the reasoning is sound. Thanks for all the effort with this.

@modmuss50 modmuss50 added the merge me please Pull requests that are ready to merge label Jan 7, 2025
@modmuss50 modmuss50 merged commit e5be219 into FabricMC:1.21.4 Jan 7, 2025
4 checks passed
@Earthcomputer Earthcomputer deleted the client-gametest-locking-once-per-tick branch January 7, 2025 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request merge me please Pull requests that are ready to merge priority:high High priority PRs that need review and work now. Review these first. test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants