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

fix: Sync local time with server more accurately #3028

Closed

Conversation

BenHamrick
Copy link

When syncing the local time from the server to the client we need to use half the RTT time to get the local time in sync with the server. Otherwise the local time drifts relative to how long the RTT time is.

Changelog

  • Changed: Made half the RTT time be used in the Sync function.

Testing and Documentation

  • No tests have been added.
  • Includes unit tests.
  • No documentation changes or additions were necessary as docs already reflect this behavior.

@BenHamrick BenHamrick requested a review from a team as a code owner August 25, 2024 04:46
@unity-cla-assistant
Copy link

unity-cla-assistant commented Aug 25, 2024

CLA assistant check
All committers have signed the CLA.

@NoelStephensUnity
Copy link
Collaborator

@BenHamrick
That does indeed look like it makes sense... half RTT vs 1 full RTT for the delta time adjustment.
Running some tests against this, but so far it looks like that would be a welcome contribution.
👍

@BenHamrick
Copy link
Author

BenHamrick commented Aug 27, 2024

@NoelStephensUnity I am really glad to hear that! We are using the tick in our own custom physics engine to do predictive rollback on the server and client. I have been so confused for a long time about why the tick is not in sync between the client and the server. We even started to write our own syncing system that worked around this problem like people are in this thread: https://discussions.unity.com/t/network-messaging-latencies-very-high/912504/2 . I probably spent more than 40 hours trying to work around the issue by dynamically changing the NetworkManager.NetworkTimeSystem.LocalBufferSec

Btw to figure this problem out I used the relay server with 2 instances of unity running on the same machine. Then I was able to use local epoch time to see if each of the ticks are in sync between the server and client. They where not!

@BenHamrick
Copy link
Author

@NoelStephensUnity how long does it take to merge and backport a simple change like this?

@BenHamrick
Copy link
Author

@NoelStephensUnity any updates on this? Is the fix valid?

@NoelStephensUnity NoelStephensUnity added stat:commited stat:Investigating Issue is currently being investigated labels Jan 9, 2025
@NoelStephensUnity
Copy link
Collaborator

NoelStephensUnity commented Jan 9, 2025

@NoelStephensUnity any updates on this? Is the fix valid?

I am up-ticking the status and we will get it migrated to run through the CI process.
You should see activity of this by early next week.
Apologies for the delay... holidays and now we are running through doing a clean-up on the GitHub issues.

Thank you for your patience and the contribution. 👍

@NoelStephensUnity NoelStephensUnity added the Tracking Has been added to tracking label Jan 10, 2025
@NoelStephensUnity
Copy link
Collaborator

@BenHamrick
Thank you for your contribution! This will help keep a more accurate server time calculation on client instances which will indeed benefit everyone who uses Netcode for GameObjects.
Your contribution has been imported into the v1.x branch (#3206) and the v2.x branch (#3212) at this time.
#3206 will be included in the next update after an already in progress v1.12.1 patch.
#3212 will be included in the next update of v2.x.

I am closing this PR at this time since.

Once again, thank you for taking the time and for your patience on getting your contribution merged. 🥇

Happy Netcoding!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat:awaiting triage Status - Awaiting triage from the Netcode team. stat:Investigating Issue is currently being investigated Tracking Has been added to tracking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants