Repository navigation
chore: enable unified netcode from N4E 7.2.0, only for hybrid prefab sessions - #4186
NoelStephensUnity wants to merge 10 commits into
Conversation
N4E is removing NETCODE_GAMEOBJECT_BRIDGE_EXPERIMENTAL and making its GameObject layer public, so UNIFIED_NETCODE is now the only define the unified path needs. testproject takes N4E from unity/unity trunk until that change ships in a package, which makes manifest-unified.json redundant.
…sions UNIFIED_NETCODE is now an asmdef versionDefine on com.unity.netcode 7.2.0, the release that makes the GameObject layer public, so no project define is needed. UNIFIED_NETCODE_7_0_0 is gone since every supported N4E uses the new namespace. UnifiedBootstrap is marked DisableBootstrapOverrides and created by NetworkManager only when the session has hybrid prefabs, so projects without them keep N4E's own startup bootstrap. A hybrid prefab added after a session started without one is rejected. testproject takes N4E from the remove-experimental-define branch until that PR merges.
|
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 version-gated unified path has concrete regressions in session startup, world cleanup, and test-project dependency resolution.
Stopping an NGO hybrid session also destroys unrelated Entities worlds when N4E's startup bootstrap has created them. [DisableBootstrapOverrides] now lets N4E's bootstrap run before NGO creates its hybrid world. ShutdownInternal still calls World.DisposeAllWorlds() whenever HasGhostPrefabs is true. That call disposes every Entities world, including ones outside NetworkManager.NetcodeWorld. Track the worlds created for the NGO session. Dispose only those worlds during NGO shutdown.
Reviewed commit e107a83
🤖 Helpful? 👍/👎
A hybrid PlayerPrefab outside the prefab lists was registered after the transport choice and the session-start rejection, so it was rejected. It now registers with the prefab lists before start, and distributed authority rejects it like any other hybrid prefab.
…in testproject UNIFIED_NETCODE is now defined automatically from N4E 7.2.0, so the Standards check formats code behind it for the first time.
…-n4e-experimental-define # Conflicts: # com.unity.netcode.gameobjects/Runtime/Components/Helpers/NetworkObjectBridge.cs
Keeps an implicit bool check where null propagation would call into a destroyed NetworkBehaviour.
| @@ -34,11 +34,6 @@ unified_test_{{ project.name }}_{{ platform.name }}_{{ editor }}: | |||
| UNIFIED_TESTS: "true" | |||
There was a problem hiding this comment.
Will we still need this to run those test?
There was a problem hiding this comment.
Ok, I saw that yes. In theory would be nice if those tests would run by default without any specific variable (which I guess will be true once we bump our minimal dependency on N4E to 7.2.0 right? And then we can remove this job)
There was a problem hiding this comment.
Yes. But it has caveats.
The purpose behind the unified tests is that there are more tests that we "opt out" of running as hybrid than "opt-in". It is possible, but I am marking that down as a follow up PR to this one (to avoid delaying this from merging).
Noel-Bot 🤖 has a plan that is being vetted. If you want can create a Jira ticket as a follow up to this PR with any other conditions to make that possible? Otherwise, it will be added to my "pre-GA release things to do". 👍
| "com.unity.ide.visualstudio": "2.0.26", | ||
| "com.unity.mathematics": "1.4.0", | ||
| "com.unity.multiplayer.tools": "2.2.11", | ||
| "com.unity.netcode": "https://github.cds.internal.unity3d.com/unity/unity.git?path=Workspace/com.unity.netcode#netcode/remove-exp-define-ghost-object", |
There was a problem hiding this comment.
Should we make the same change to minimalproject and Examples/ ?
There was a problem hiding this comment.
Good call out for sure!
Once PR-130027 lands we can update those projects since we will have to update test project to point to trunk anyway.
Once v7.2.0 is published and packman is resolving it, before we publish v3.2.0 we will want to make any changed manifest file(s) point to the version instead of the git URI.
…-n4e-experimental-define # Conflicts: # .yamato/unified-tests.yml
…malproject NGO now applies the NetcodeConfig values it needs at session start, so nothing in either asset is required. Netcode for Entities creates the asset and its project settings entry on first open. The settings file is removed too, since keeping it tracked would make every fresh checkout rewrite it.
…n first open The assets are no longer tracked, so the files N4E writes when a project opens would otherwise show up as untracked changes and could be committed back.
|
@u-pr can you review this again please? |
Review SummaryThe recent changes addressing the hybrid A few items were noted for consideration in this pass (with inline comments attached to the diff):
🤖 Helpful? 👍/👎 |
…l propagation Implicit bool checks instead of ?. on NetworkTransform and NetworkManager, which the Standards formatter does not rewrite.
|
🤖 Acted on 2: |
This branch has not been deployed
2 participants
Purpose of this PR
The unified path no longer needs any scripting define: it compiles in when the project resolves Netcode for Entities 7.2.0 or later, and only runs when a session has hybrid prefabs.
N4E 7.2.0 is the GA release of its GameObject layer and drops
NETCODE_GAMEOBJECT_BRIDGE_EXPERIMENTAL(unity/unity PR 130027). This PR adapts NGO to it.PR Scope:
UNIFIED_NETCODEis an asmdef versionDefine oncom.unity.netcode7.2.0 in the Runtime, Editor, Runtime.Tests and TestProject.Runtime.Tests assemblies. Released N4E 7.0.0 and 7.1.0 keep it off.OUT_OF_BAND_RPC, the bridge-define#errorandUNIFIED_NETCODE_7_0_0are removed (every N4E NGO 3.x supports uses the new namespace).Hybrid mode only for sessions with hybrid prefabs:
UnifiedBootstrapis no longer picked by Entities as the startup bootstrap for every project.NetworkManagercreates it only when a hybrid prefab is registered before the session starts, from aNetworkPrefabsListorAddNetworkPrefab. A hybrid prefab added after a session started without one now logs an error and is not registered; before, it switched the session into hybrid mode and stopped its send queue. A hybridPlayerPrefaboutside the prefab lists now counts toward that decision too; before, it was registered after the transport was chosen.testprojecttakes N4E from unity/unity by git, since 7.2.0 is not published yet.manifest-unified.jsonis deleted, and the unified job no longer edits the manifest or ProjectSettings.The
NetcodeConfigassets are no longer tracked intestprojectandminimalproject, nor theirNetCodeClientAndServerSettings.asset. NGO applies the values it needs at session start, and Netcode for Entities creates both files on first open (a tracked settings file would be rewritten on every fresh checkout), and both projects'.gitignorenow exclude them so they are not committed back.Out of scope:
package.jsonstays oncom.unity.netcode7.1.0 (7.2.0 is unpublished, and depending on it fails package validation). It moves to 7.2.0 once N4E publishes.RpcTestsAutomated(UnifiedHost)throwsObjectDisposedExceptionin OneTimeTearDown when it runs in the full unified suite (pre-existing on develop-3.x.x).Before merge: unity/unity PR 130027 merges first, then
testproject/Packages/manifest.jsongoes from#netcode/remove-exp-define-ghost-objectback to#trunk.Jira ticket:
None
Documentation
Testing & QA
Automated tests:
Covered by existing automated testsCovered by new automated tests:UnifiedHybridPrefabValidationTests.HybridPrefabAddedAfterStartIsRejectedandHybridPlayerPrefabCountsBeforeStart.UNIFIED_TESTS=true, no scripting defines, after merging develop-3.x.x (#4176, #4144)PeerDisconnectCallbackTestscase; 36/36 on rerun)NetworkVariableTests, non-unified job, withUNIFIED_NETCODEcompiled inHybridNetcodeDefaultsTests(EditMode)Up-port
Not needed: unified work is develop-3.x.x only.
Backports
Not needed.