feat: hybrid interop tests - #4176
NoelStephensUnity wants to merge 9 commits into
Conversation
CreateHybridPrefab takes an optional GhostMode. Predicted modes are applied after NetworkObjectBridge is added, because its editor OnValidate resets the supported ghost modes to interpolated. HybridPredictionTests: an owner-predicted hybrid prefab predicts and re-simulates on the owning client. HybridInteropTests combines N4E and NGO features on one hybrid prefab: - An NGO RPC sent from PredictionUpdate repeats for re-simulated ticks, and sends each tick once when gated on IsFirstTimeFullyPredictingTick. - Unified remote to NGO RPC to unified remote, and NGO RPC to unified remote to NGO RPC. - A NetworkVariable read during prediction is not tick-aligned, while a tick-stamped value applied from its stamp tick is consistent across re-simulation.
…on hybrid prefabs - A NetworkVariable written from PredictionUpdate moves backwards when older ticks re-simulate, and only moves forward when gated on IsFirstTimeFullyPredictingTick. - An NGO ownership change does not change the ghost's N4E owner.
- UnifiedBootstrap registers the worlds it created for other NetworkManagers again after each bootstrap, since every ClientServerBootstrap constructor clears N4E's ServerWorlds and ClientWorlds. Remote methods send through those lists, so with several NetworkManagers in one process server-to-client remotes were dropped. - NetworkObjectBridge defaults a GhostObject to interpolation only when the bridge is first added, instead of on every OnValidate, so prediction enabled on a hybrid prefab is kept. - NGO spawn ownership and ownership changes set the ghost's owner, so an owner-predicted ghost is predicted by its NGO owner. - NetworkManager shutdown disposes only its own world instead of every world in the process. NetworkObjectDontDestroyWithOwnerTests and NetworkSpawnManagerTests now run in hybrid prefab mode. NetworkShowThenClientDisconnects is ignored for hybrid prefabs: a scene migration update can reach a client before the object's ghost has spawned there.
|
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
|
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Harness Review
The change adds hybrid prediction and interop coverage and keeps ghost ownership and world disposal scoped to each network manager, but the spawn callback order exposes an inconsistent owner state and two test assertions leave intended behavior unprotected.
Reviewed commit f6fee1d
🤖 Helpful? 👍/👎
- The ghost owner is now assigned right after SetupOnSpawn, so OnNetworkSpawn sees the same owner on the NetworkObject and its ghost. The interop test records the ghost owner in OnNetworkSpawn and asserts it. - NetworkVariableReadDuringPredictionIsNotTickAligned asserts that a re-simulated tick reads a different value than its first prediction, rather than counting reads before the stamp tick. - NetworkShowThenClientDisconnects is only ignored for UnifiedServer, the fixture where the scene migration failure occurs.
|
@u-pr please review again |
|
The changes addressing the previous review comments look good:
Summary of New Observations
🤖 Helpful? 👍/👎 |
…ation in the backwards-write test - UpdateGhostOwner leaves the ghost unowned if a host NetworkManager has no NetcodeWorld. - NetworkVariableWrittenFromPredictionUpdateMovesBackwards keeps writing until a backwards move has happened, instead of stopping after 20 changes, which could come before any re-simulation.
|
@u-pr please review again |
|
The latest changes in commit
Review Summary
Overall, the changes are clean, well-tested, and ready to go. 🤖 Helpful? 👍/👎 |
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## develop-3.x.x #4176 +/- ##
=================================================
+ Coverage 78.19% 78.23% +0.03%
=================================================
Files 153 153
Lines 26272 26324 +52
=================================================
+ Hits 20544 20595 +51
- Misses 5728 5729 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
…connects UnifiedNetcodeTransport.DisconnectLocalClient only requested the N4E disconnect. N4E reports it later, after NGO's shutdown has stopped listening, so the disconnecting client never received its ClientDisconnected event. It now notifies immediately, as UnityTransport does, and ignores N4E's later event. A server-initiated disconnect removes the connection before notifying, so the shutdown it triggers does not notify a second time. DisconnectTests and PeerDisconnectCallbackTests now run in hybrid prefab mode.
…ntegration tests The integration test hook that replaces GhostSpawnManager.RegisterGhostBridge also registered the server's own instance. On a dedicated server, which is never a connected client, that moved the instance into the DontDestroyOnLoad scene. The server then sent a scene migration for it to every client, and none of them had spawned it. The hook now skips the server, as the runtime does. NetworkShowThenClientDisconnects no longer needs to be ignored for UnifiedServer.
…brid spawns into their scene ObjectSceneChanged was sent to every connected client, including clients that did not observe the migrated NetworkObjects and had never spawned them, which logged "Trying to synchronize NetworkObjectId but it was not spawned". In client-server mode it is now only sent to clients that observe at least one migrated NetworkObject, and each client is only sent the NetworkObjects it observes. Distributed authority is unchanged. A hybrid prefab instance that is part of a client's initial synchronization can spawn after the synchronization has completed, once its ghost arrives. It missed the step that migrates synchronized NetworkObjects into their server-side scene, and stayed in the active scene. It now runs that step when it spawns.
…r's scene A client that starts observing a NetworkObject with NetworkShow after the NetworkObject migrated into another scene was never sent that migration, and instantiated the NetworkObject in its active scene. The client now moves a spawned NetworkObject into its server-side scene, as it already does for NetworkObjects that are part of its initial synchronization. Adds CHANGELOG entries for this and for scene migrations being sent to clients that do not observe the NetworkObject.
|
@u-pr please review again |
Review SummaryThe latest commits (
Findings
🤖 Helpful? 👍/👎 |
| var networkObject = NetworkObject.DeserializeAndSpawnObject(serializedObject, reader, m_NetworkManager); | ||
|
|
||
| // The synchronization moved the other NetworkObjects into their server-side scenes before this one spawned | ||
| if (networkObject != null) |
There was a problem hiding this comment.
If NetworkConfig.EnableSceneManagement is false, m_NetworkManager.SceneManager is null. Guarding with EnableSceneManagement (consistent with CreateObjectMessage.cs) prevents a potential NullReferenceException.
| if (networkObject != null) | |
| if (networkObject != null && m_NetworkManager.NetworkConfig.EnableSceneManagement) |
🤖 Helpful? 👍/👎
| GetDisconnectMessageFromNetworkStreamDisconnectReason(connectionEvent.DisconnectReason) | ||
| ); | ||
| // Removed before notifying, so the shutdown this triggers does not notify again from DisconnectLocalClient. | ||
| m_Connections.Remove(connectionEvent.Id.Value); |
There was a problem hiding this comment.
In DisconnectLocalClient, wasConnected is determined by removing (int)ServerClientId. If connectionEvent.Id.Value is not equal to (int)ServerClientId, DisconnectLocalClient could still find (int)ServerClientId in m_Connections and invoke a duplicate disconnect event. Have you considered removing (int)ServerClientId here as well?
| m_Connections.Remove(connectionEvent.Id.Value); | |
| m_Connections.Remove(connectionEvent.Id.Value); | |
| m_Connections.Remove((int)ServerClientId); |
🤖 Helpful? 👍/👎
This branch has not been deployed
1 participant
Purpose of this PR
This PR adds 29 hybrid prefab test cases, and fixes 9 issues the tests found: 6 NGO issues in hybrid prefab mode, 2 NGO scene migration issues that affect every NGO project, and 1 test harness issue. The results back the hybrid mode compatibility matrix (N4E prediction, unified remotes and
GhostFields used with NGO RPCs, NetworkVariables and ownership).PR Scope:
New tests (
Tests/Runtime/Unified,UnifiedHost/UnifiedServer):PredictionUpdateIsFirstTimeFullyPredictingTick.PredictionUpdatePredictionUpdateCreateHybridPrefabtakes an optionalGhostModeso tests can create predicted and owner-predicted hybrid prefabs.Fixes:
UnifiedBootstrapre-registers the worlds it created for otherNetworkManagers. EveryClientServerBootstrapconstructor clears N4E'sServerWorldsandClientWorlds, and eachNetworkManagercreates its own bootstrap. Remote methods send through those lists, so with severalNetworkManagers in one process, server-to-client remotes were dropped.NetworkObjectBridgedefaults aGhostObjectto interpolation only when the bridge is first added (Reset). It no longer does it on everyOnValidate, which reverted prediction set in the inspector.GhostObject.OwnerNetworkId. Before this, an owner-predicted ghost was never given an owner, so no client predicted it.NetworkManagershutdown disposes only its own world instead of callingWorld.DisposeAllWorlds(). This means one peer shutting down no longer breaks the others.ClientDisconnectedevent.UnifiedNetcodeTransport.DisconnectLocalClientonly requested the N4E disconnect, which N4E reports after NGO's shutdown has stopped listening. It now notifies immediately, asUnityTransportdoes.NGO scene migration fixes (affect every NGO project, with CHANGELOG entries, and are also ported to
develop-2.0.0):SceneEventType.ObjectSceneChanged) was sent to every connected client, including clients that did not observe the migrated NetworkObject and logged "Trying to synchronize NetworkObjectId but it was not spawned". In client-server mode it is now only sent to the clients that observe at least one migrated NetworkObject, and each client is only sent the NetworkObjects it observes. Distributed authority is unchanged.NetworkShowafter it migrated into another scene, while hidden from that client, was instantiated in the client's active scene, since the client was never sent that migration. The client now moves the spawned NetworkObject into its server-side scene, as it already does during its initial synchronization.Test harness fix:
DontDestroyOnLoadscene, and the server then sent every client a scene migration for an object none of them had spawned ("Trying to synchronize NetworkObjectId but it was not spawned",SceneEventData.cs:1302). The hook now skips the server, as the runtime does.NetworkObjectDontDestroyWithOwnerTests(6 cases) andNetworkSpawnManagerTests(4 cases) now run in hybrid prefab mode. Both failed before the shutdown fix.DisconnectTests(4 cases) andPeerDisconnectCallbackTests(12 cases) also run in hybrid prefab mode. Their client-initiated cases failed before the disconnect fix.New
NetworkObjectSceneMigrationObserverTests(4 tests,Host/Server/UnifiedHost/UnifiedServer) cover both scene migration fixes and the late synchronization fix.Out of this PR's scope:
GatherInput,PredictedPhysicsUpdateand prediction switching combined with NGO features (not covered yet).GhostObject.ParentReplicationis not in N4E 7.0.0; NGO will defer parenting to it in a separate PR).InstantiateAndSpawnand hybrid player prefabs not selecting the world (to be resolved withGhostObject.DelaySpawning/Spawn()).Jira ticket
MTT-16222
Changelog
NetworkObjectinto another scene made the clients that did not observe it log "Trying to synchronize NetworkObjectId but it was not spawned". The scene migration is now only sent to the clients that observe theNetworkObject.NetworkObjectthat was moved into another scene while hidden from a client spawned in that client's active scene when it was shown withNetworkShow, instead of the scene it is in on the server.Documentation
Testing & QA (How your changes can be verified during release Playtest)
Headless PlayMode runs on 6000.7.0b1 (N4E 7.0.0) with the unified job's scripting defines, filtered to the hybrid fixtures from #4172 plus the fixtures in this PR.
UNIFIED_TESTS=trueUNIFIED_TESTSunset, all ofUnity.Netcode.RuntimeTestsNetworkVariableTests, which fails the same 96 cases before these changes, atd4fda26e8. It only fails with the unified scripting defines andUNIFIED_TESTSunset, a combination CI does not run)NetworkSceneManager*,NetworkObjectSceneMigrationTests, DontDestroyOnLoad, scene event tests),UNIFIED_TESTSunsetNetworkObjectSceneMigrationObserverTests,UNIFIED_TESTS=trueand unsetUNIFIED_TESTS=true, 3 runsNetworkObjectDontDestroyWithOwnerTests,UNIFIED_TESTS=true, 3 runsUNIFIED_TESTS=true, 3 runsUNIFIED_TESTSunsetFunctional Testing
Manual testing :
Manual testing doneAutomated tests:
Covered by existing automated testsCovered by new automated testsDoes the change require QA team to:
Review automated tests?Execute manual tests?Provide feedback about the PR?Up-port
Not needed. Hybrid prefab mode only exists on
develop-3.x.x. The two NGO scene migration fixes are ported todevelop-2.0.0in #TBD.Backports
Not needed.