Repository navigation
fix: hybrid prefab component gating and distributed authority rejection - #4184
Conversation
A hybrid prefab's NetworkRigidbodyBase derivatives were destroyed at runtime so a user could try hybrid mode without first stripping components from the prefab. Removing an entry from ChildNetworkBehaviours is not safe: InitializeChildNetworkBehaviours rebuilds unconditionally and has five call sites, two of them the public GetNetworkBehaviourOrderIndex and GetNetworkBehaviourAtOrderIndex, while Destroy is deferred to end of frame. A rebuild in a later frame therefore renumbers every behaviour after the removed one, and the NetworkVariable half of object synchronization is positional with no id, so a one-entry disagreement corrupts the whole buffer for that object. NetworkTransform was already handled by early-returning on HasGhost rather than being destroyed - the removal behind UNIFIED_NETCODE_DESTROY was dead, that define exists nowhere. This extends the same treatment to NetworkRigidbodyBase and closes four NetworkTransform paths that had no gate: OnSynchronize, InternalOnNetworkPostSpawn, InternalOnNetworkSessionSynchronized and InternalOnNetworkObjectParentChanged. The first of those means a hybrid object no longer writes a full teleport state to every joining client. The rigidbody is now forced kinematic on non-server peers rather than destroyed, restored from m_OriginalKinematicState on destroy as the class already provided for. That decision is made at spawn, not during Awake, because Initialize runs from Awake where an in-scene placed instance has no session to ask whether it is the server. UnregisterRigidbody had no call sites and is now called from the gate, which is what keeps a second Initialize from leaving a registration pointing at a component that no longer drives anything. [RequireComponent(typeof(Rigidbody))] is unconditional again. Behind #if !UNIFIED_NETCODE it was stripped from every NetworkRigidbody in the project the moment N4E was installed, hybrid or not, after which NetworkRigidbodyBase dereferences a null rigidbody in Awake.
Distributed authority does not support hybrid prefabs. A start with any GhostObject prefab registered now fails in CanStart with an error naming each prefab, before Initialize replaces the transport with UnifiedNetcodeTransport. A hybrid prefab added during a distributed authority session is refused, so HasGhostPrefabs cannot switch the running session into hybrid mode and stop its send queue.
…refab-component-gating
|
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 keeps hybrid components present to preserve behaviour IDs and adds distributed authority prefab checks, but the checks and rigidbody gating miss several reachable paths.
Reviewed commit 03ba4e1
🤖 Helpful? 👍/👎
- NetworkRigidbodyBase no longer re-applies the authority kinematic rule on an ownership change for a hybrid prefab. - NetworkPrefabs recomputes HasGhostPrefabs on initialize, and keeps a single subscription to each NetworkPrefabsList (a prefab added to a list during a session was registered more than once). - RejectGhostPrefabs is set in Initialize, after SetRole accepts the start, so a failed start cannot leave it set. - A NetworkPrefab override with a hybrid source or target prefab is detected as a hybrid registration. N4E instantiates the server's prefab from the ghost type, so overrides are not supported for hybrid prefabs yet: a start with one registered fails in any topology, and one added during a session is rejected.
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## develop-3.x.x #4184 +/- ##
=================================================
- Coverage 78.19% 78.17% -0.03%
=================================================
Files 153 153
Lines 26272 26286 +14
=================================================
+ Hits 20544 20549 +5
- Misses 5728 5737 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| #if UNIFIED_NETCODE | ||
| // An override is a hybrid registration when either its source or its target prefab has a GhostObject. | ||
| HasGhost = (Override == NetworkPrefabOverride.Prefab && SourcePrefabToOverride.GetComponent<NetworkObject>().HasGhost) | ||
| || (OverridingTargetPrefab.TryGetComponent(out NetworkObject targetNetworkObject) && targetNetworkObject.HasGhost); |
There was a problem hiding this comment.
Hmmm, I have two thoughts here:
- I'm not sure that inside the
Validatemethod is the right place to be setting this value.Validateis supposed to be "Is this valid to be a prefab or not", not "Is this valid to be a prefab and also please finish setting this prefab up for me". - If we do leave setting this value in this function, I think it should be set in the various switch statements. We're already doing the
TryGetComponent(out NetworkObject networkObject)in each of the branches of this function,GetComponentis expensive enough that we should be re-using the value we've already got.
There was a problem hiding this comment.
Good call on both points. Validate no longer sets HasGhost for overrides at all: a hybrid override is now invalid, so it logs an error and returns false like the other invalid override cases, reusing the NetworkObject already fetched in the Prefab branch. The only HasGhost assignment left in Validate is the one already on develop-3.x.x for non-override prefabs.
There was a problem hiding this comment.
HasGhost is still being set in Validate on line 181. I don't think the new logic here is fixing the issue. The structure of the code here should allow validation without having to make an additional GetComponent call.
There was a problem hiding this comment.
The only HasGhost assignment left in Validate is the one already on develop-3.x.x for non-override prefabs.
I put that there months ago and it is not part of this PR.
If we want to discuss changing this flow, then that would be a separate PR.
| // SetRole keeps IsServer set when it rejects the start, which makes the NetworkManager throw when destroyed. | ||
| m_StandaloneNetworkManager.ConnectionManager.LocalClient.SetRole(false, false); |
There was a problem hiding this comment.
This seems like a bug! We should fix it!
There was a problem hiding this comment.
Agreed, it's a bug in plain NGO on both develop lines. Keeping it out of this PR since it's unrelated to hybrid prefabs: it'll be its own PR on develop-2.0.0 with a test and a CHANGELOG entry, then up-ported. The test resets the role by hand until then.
There was a problem hiding this comment.
Or I could fix this here and do a back port... your call?
There was a problem hiding this comment.
Really depends on the size of the fix. If the fix is 10 lines or less, I'd rather it goes into this PR. If it's more complicated, happy for it to go on a separate PR.
There was a problem hiding this comment.
It is targeted for another PR that will make it cleaner to review.
- Move the distributed authority hybrid prefab check and its session flag into NetworkPrefabs, called through NetworkConfig.InitializePrefabsForStart. - NetworkPrefab.Validate treats an override with a hybrid source or target as invalid, so it is logged and ignored in any topology. - Add NetworkPrefab.GetDebugName, used by the prefab logs and the debugger. - Add NetcodeIntegrationTest.CreateNetworkObjectPrefab(string, bool). - Trim the hybrid prefab tests to the behaviour id and kinematic checks, and rework the validation tests onto the harness helpers.
| // Owner authority makes an ungated NetworkRigidbody change the kinematic state on an ownership change. | ||
| m_Prefab.AddComponent<NetworkTransform>().AuthorityMode = NetworkTransform.AuthorityModes.Owner; | ||
| m_Prefab.AddComponent<Rigidbody>(); | ||
| m_Prefab.AddComponent<NetworkRigidbody>(); |
There was a problem hiding this comment.
Is it worth maybe adding a GhostRigidbody component for this test? It'd be like we did for the DisconnectTests, just a dummy component that basically checks that everything replicates like we expect.
| m_Prefab.AddComponent<NetworkRigidbody>(); | |
| m_Prefab.AddComponent<NetworkRigidbody>(); | |
| m_Prefab.AddComponent<GhostRigidbody>(); |
There was a problem hiding this comment.
Good thought. Reading N4E, GhostRigidbody only replicates to predicted clients and rewrites isKinematic from the server every prediction tick, so for a predicted hybrid prefab N4E would override the kinematic state NetworkRigidbody sets at spawn (interpolated clients don't get that data). #4176 adds predicted hybrid prefabs to CreateHybridPrefab, so I've logged it as a follow-up to test once both PRs land, along with skipping our kinematic override when a GhostRigidbody is present.
KinematicStateSurvivesOwnershipChange already runs the same behaviour table and gated component checks.
2 participants
Purpose of this PR
This PR assures the
NetworkBehaviourIdvalues on hybrid prefabs are maintained by keepingNetworkTransformandNetworkRigidbodyeffectively a "nop" on the instance instead of destroying them at runtime like the POC was doing. Included in this PR, hybrid prefabs are rejected when using a distributed authority network topology.NetworkRigidbodyBasederived classes were originally destroyed in the POC during runtime on hybrid prefab instances. This PR keeps the components but turns them into "nop components". This fixes some edge cases:ChildNetworkBehaviourswhileDestroyis deferred to the end of the frame.NetworkBehaviourIdrelative, so any deviation from the original indices could break delta synchronization.NetworkRigidbodyBasenow early-exits onHasGhostsimilar toNetworkTransform. The body is made kinematic on every peer except the server at spawn, and its authored kinematic state is restored on destroy.NetworkTransform, the new owner's body would become dynamic).NetworkTransformneeded additionalHasGhostgating withinOnSynchronize,InternalOnNetworkPostSpawn,InternalOnNetworkSessionSynchronizedandInternalOnNetworkObjectParentChanged. A hybrid object no longer writes a full transform state to every late joining client.NetworkTransform.UnregisterRigidbodyhad no call sites. It is now called from the gate, so reparenting a hybrid object that has aNetworkRigidbodyno longer dereferences a destroyed component.[RequireComponent(typeof(Rigidbody))]and[RequireComponent(typeof(Rigidbody2D))]are unconditional again. UnderUNIFIED_NETCODEthey were dropped from everyNetworkRigidbodyin the project, hybrid or not.The final suggested pattern, if a user decides to keep the hybrid prefab setting, will be to completely remove
NetworkTransformandNetworkRigidbody.The
UNIFIED_NETCODE_DESTROYblock inNetworkObjectis removed (that define is never used and was a left over artifact from the POC).Distributed authority does not support hybrid prefabs:
UnifiedNetcodeTransportand attempt to start the session.NetworkManagerwill shutdown before attempting to start the session.NetworkPrefab overrides are not supported for hybrid prefabs (yet):
NetworkPrefab.Validatenow treats an override with a hybrid source or target prefab as invalid (in any network topology).NetworkPrefabssubscribed to eachNetworkPrefabsListagain every time it was initialized, so a prefab added to a list during a session was registered more than once. It now keeps a single subscription, andHasGhostPrefabsis recomputed each time the prefabs are initialized.NetworkPrefab.GetDebugNamenames a prefab, including its override target, for the prefab logs and the debugger (DebuggerDisplay).NetcodeIntegrationTest.CreateNetworkObjectPrefab(string, bool)creates a prefab without aGhostObjectin a unified test.PR Scope:
NetworkObject,NetworkTransform,NetworkRigidbodyBase,NetworkRigidbody,NetworkRigidbody2D,NetworkManager,NetworkConfig,NetworkPrefab,NetworkPrefabsandNetworkPrefabHandler. All changes are withinUNIFIED_NETCODEpaths except theNetworkPrefabsListsubscription fix andGetDebugName. No wire format changes.Out of scope:
NetworkRigidbodyintegration coverage (NetworkRigidbodyTestnever compiles because the test assembly is missing the physics2d versionDefine. This will be fixed in a separate PR using thefix/rigidbody-tests-never-compiledbranch).UnifiedNetcodeTransportis not restored after a hybrid session ends.NetworkClient.SetRoleleavesIsServerset when it rejects a distributed authorityStartServer, so destroying thatNetworkManagerthrows (plain NGO, both develop lines; will be fixed in a separate PR).Jira ticket
N/A
Changelog
NetworkPrefabsListwhile a session was running was registered more than once and logged a duplicateGlobalObjectIdHasherror.Documentation
Testing & QA (How your changes can be verified during release Playtest)
UnifiedHybridPrefabBehaviourIdTests(UnifiedHost)UnifiedHybridPrefabValidationTests(UnifiedHost)UNIFIED_TESTS=true, all runtime tests)NetworkTransform,NetworkRigidbodyandNetworkManagerStartExceptionTestsfixturesFunctional 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. This PR targets
develop-3.x.x.Backports
The hybrid prefab changes are not needed on
develop-2.0.0(hybrid prefabs exist ondevelop-3.x.xonly). TheNetworkPrefabsListsubscription fix applies todevelop-2.0.0as well, and a port will follow.