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(core): further improve precision of time sync. #249

Merged
merged 2 commits into from May 30, 2019

Conversation

pkaminski
Copy link
Contributor

Two main changes:

  1. Fire off a separate time sync packet before the potentially big client approval message.
  2. Update the Transport SPI to optionally provide a more precise reception time for events.

The change to Transport is backwards-compatible but introduces extra complexity. It could be simplified if you're willing to break backwards compatibility and update all the transports.

pkaminski and others added 2 commits May 30, 2019 11:30
Two main changes:
1. Fire off a separate time sync packet before the potentially big client approval message.
2. Update the Transport SPI to optionally provide a more precise reception time for events.
@TwoTenPvP
Copy link
Contributor

Just to clarify, what you are trying to remove is the time during which the approval message is being deserialized and read? or why is the large approval message problematic?

@pkaminski
Copy link
Contributor Author

I have empirically gotten bad time sync results with the approval message. Didn't really prove why, but I think it's because:

  1. The sending time gets written before all the objects, which could take a while. I considered moving the time field after the objects but reading the stream can be partially deferred on the client, so it was hard to do that.

  2. If the message gets large it can get fragmented, and won't be considered "received" until all the fragments make it in.

  3. If a packet for the message gets dropped, it will get resent and mess up the RTT offset we apply to the time.

Overall, syncing time over a reliable channel with large messages is just bad... I kept the time in there as a fallback, since it's better to have a bad sync than no sync at all (if the dedicated packet gets lost), but it should be considered a last resort.

@TwoTenPvP TwoTenPvP merged commit 721e68f into Unity-Technologies:master May 30, 2019
@MidLevel-Bot
Copy link

🎉 This PR is included in version 10.8.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@MidLevel-Bot MidLevel-Bot added the stat:commited Status - Commited to work on this issue. label May 30, 2019
MrCool92 pushed a commit to MrCool92/MLAPI that referenced this pull request Dec 6, 2020
…s#249)

* fix(core): further improve precision of time sync.

Two main changes:
1. Fire off a separate time sync packet before the potentially big client approval message.
2. Update the Transport SPI to optionally provide a more precise reception time for events.

* Update InternalMessageHandler.cs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat:commited Status - Commited to work on this issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants