Repository navigation
fix: network clock adjustment overshoot and per-frame NetworkManager scene search - #4195
NoelStephensUnity wants to merge 10 commits into
Conversation
…ing past it Once a client's clock reached the offset it adjusts towards, each frame stepped the full AdjustmentRatio past it and back, so LocalTime and ServerTime alternated between running fast and slow every frame.
…r them every frame UnifiedUpdateConnections called Object.FindObjectsByType<NetworkManager>() on every update, whose cost grows with the number of MonoBehaviours in the scene: about 1.5 ms a call with 10,000 and 14 ms with 50,000. NetworkManager.Instances is filled in Awake and emptied in OnDestroy, and the system visits that.
|
Check the configuration's TOML syntax, permitted settings, and size limit. See the error below. Error message: Configuration content:# This is the configuration for the u-pr bot
# https://github.cds.internal.unity3d.com/unity/u-pr
# For configuration of this file:
# https://developer.portal.internal.unity.com/catalog/default/component/u-pr/docs/configuration/automatic_runs/
[github_app]
handle_pr_actions = ['opened', 'ready_for_review'] # PR events that auto-run pr_commands
pr_commands = ["/harness-review"] # what runs on those events
reviewer_commands = ["/harness-review"] # what runs when @u-pr is added as reviewer
|
UnifiedUpdateConnections now runs only in client and server worlds that hold the NetworkManager they were created for, and updates that NetworkManager alone. This replaces the NetworkManager.Instances list, which a derived NetworkManager declaring its own Awake never entered.
Codecov ReportAttention: Patch coverage is @@ Coverage Diff @@
## develop-3.x.x #4195 +/- ##
=================================================
- Coverage 76.11% 76.03% -0.09%
=================================================
Files 159 159
Lines 27333 27345 +12
=================================================
- Hits 20805 20791 -14
- Misses 6528 6554 +26
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
@u-pr please review again |
Review SummaryThe shift to binding the A few observations and suggestions: Observations & Suggestions1. Null / Destroyed
|
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
…uilds Netcode for Entities cannot create a single world host in a dedicated server build, which every hybrid server used. A build for only one side now logs an error when asked to start the side it does not support.
EmandM
left a comment
EmandM
left a comment
There was a problem hiding this comment.
A couple of nit-pick comments but this looks fantastic! I really, really like this approach!!
| /// </summary> | ||
| internal struct HybridNetworkManager : IComponentData | ||
| { | ||
| public UnityObjectRef<NetworkManager> Value; |
There was a problem hiding this comment.
Oh I LIKE this!!
Though a deep nit-pick, maybe Value isn't the best name here? It's very vague.
There was a problem hiding this comment.
Good call, Value didn't say much. Renamed it to NetworkManager, so it is now .NetworkManager.Value
| m_CurrentLocalTimeOffset += deltaTimeSec * (m_DesiredLocalTimeOffset > m_CurrentLocalTimeOffset ? AdjustmentRatio : -AdjustmentRatio); | ||
| m_CurrentServerTimeOffset += deltaTimeSec * (m_DesiredServerTimeOffset > m_CurrentServerTimeOffset ? AdjustmentRatio : -AdjustmentRatio); | ||
| // Stop on the desired offset rather than step past it, or a converged clock alternates between running | ||
| // fast and slow by the whole ratio every frame, and everything rendered from it judders. |
There was a problem hiding this comment.
nit: I don't understand this code comment. It wouldn't help me know what's going on here if I ever needed to interact with this code.
There was a problem hiding this comment.
That is a fair point. The old comment described the symptom rather than what the code does. It now reads:
Move each offset toward its target by at most AdjustmentRatio of this frame's delta, stopping once it gets there. Stepping the full amount every frame overshot the target, so a synced clock alternated between running fast and slow, and anything driven by it juddered.
…updates The hybrid world outlives its NetworkManager if ShutdownInternal throws before disposing it, so UnifiedUpdateConnections now disconnects every connection and announces none instead of throwing each frame. HybridNetworkManager.Value is renamed to NetworkManager, the clock adjustment comment says what the clamp does, and the unused Unity.Entities using that failed the Standards check is removed.
|
@u-pr please review again. |
Review SummaryThe recent updates nicely address the previous feedback:
A couple of minor observations and suggestions: Suggestions1. Repeated
|
| // make sure we have disconnected all known connections. | ||
| if (isShuttingDown) | ||
| { | ||
| foreach (var (networkId, entity) in SystemAPI.Query<NetworkId>().WithEntityAccess()) |
There was a problem hiding this comment.
If the world outlives its NetworkManager across multiple frames while isShuttingDown is true, querying NetworkId without filtering will re-query these entities and invoke OnNetCodeDisconnect on every frame (since NetworkId is not removed from the entities).
Have you considered filtering the query with .WithAll<ConnectionState>()?
foreach (var (networkId, entity) in SystemAPI.Query<NetworkId>().WithAll<ConnectionState>().WithEntityAccess())This ensures OnNetCodeDisconnect is invoked only once per active connection, as ConnectionState is removed in the first pass.
🤖 Helpful? 👍/👎
…ssure theory Revert before merge.
A client drains its incoming queue while it disconnects, after the unified transport has removed the server connection, so a queued TimeSyncMessage made GetCurrentRtt throw KeyNotFoundException. It now returns 0 for a connection it no longer has, as UnityTransport does.
… whether 6000.7.0b6 raised memory use Replaces the b1.xlarge test. Revert before merge.
…ed test pass The IL post-processor and compiler processes the editor starts stay resident through the test pass and, on a b1.large agent, leave too little memory for the hybrid tests. The project is now imported and compiled in its own batchmode session that exits first, any build process that outlives it is stopped, and the job log lists the largest processes left before the tests start. Also drops the temporary 6000.7.0b4 editor pin.
This branch has not been deployed
2 participants
Purpose of this PR
Three small, independent fixes.
Network clock adjustment overshoot: without this fix, a client's clock keeps alternating between running slightly fast and slightly slow on every frame, even after it has synchronized with the server. Interpolated objects then judder (a periodic motion artifact caused by mismatched frame rates): they move in uneven steps instead of smoothly. It is barely noticeable at the default
NetworkTimeSystem.AdjustmentRatio, and clearly visible at higher values.Per-frame NetworkManager scene search: without this fix,
UnifiedUpdateConnectionssearches the whole scene forNetworkManagerinstances every frame, in every Entities world, even when there is no hybrid session, so the frame time grows with the size of the scene (about 1.5 ms per frame with 10,000 MonoBehaviours). It now only exists in client and server worlds, and only runs in a world created for a hybrid session, where it updates the connections of that world's ownNetworkManager.Hybrid mode in dedicated server builds: Netcode for Entities cannot create a single world host in a dedicated server build, which every hybrid server used. A dedicated server build now creates a server-only world, and a build for only one side logs an error when asked to start the other side.
(The last two are excluded from the changelog since the unified feature set has not yet been released)
Jira ticket
None.
Changelog
NetworkTimeSystem.AdjustmentRatio, and clearly visible at higher values.Documentation
Testing & QA (How your changes can be verified during release Playtest)
Functional Testing
Automated tests:
Covered by new automated tests:ClientNetworkTimeSystemTests.ConvergedClockAdvancesAtRealRate,UnifiedNetworkTransformTest.EachWorldUpdatesConnectionsForItsOwnNetworkManager,UnifiedUpdateConnectionsTests.NoWorldUpdatesConnectionsWithoutGhostPrefabsUp-port
Not needed: this targets develop-3.x.x.
Backports
None.