fix: ChangeOwnership sending own clientId and added NetworkSceneHandle struct (#3647)

* chore: Update the OwnershipPermissionsTests to work with rust server

* Remove TestHelpers change

* Ensure we don't send CreateObject messages to the cmb service when there are no valid receivers

* dotnet-fix

* Ensure the DAHost complains if the senderId is included in the ClientIds list

* Refactor and unify the ChangeOwnership message sending

* Update the NetworkObjectOwnershipPropertiesTests to work with rust

* Add placeholder SceneHandle object

* Add serializer for the SceneHandle

* Use SceneHandle in place of int

* Fix ulong to int conversions

* Ensure the spawn manager has the SceneHandle object in scope

* Fix implicit conversions in the tests

* Fix remaining implicit conversions in DistributedAuthorityCodecTests

* Fix more ulong to int conversions

* Simplify the IntegrationTestSceneHandler some

* Update the testproject asmdef so the sceneEventDataTests compile

* fix whitespace

* fix whitespace part 2 electric boogaloo

* Fix codestyle violations

* Fix import ordering

* fix FastBuffer tests

* Remove serialization changes

* Use NetworkSceneHandle wrapper class

* Remove unneeded changes

* Remove accidental int conversion

* Ensure the tests use a specific constructor

* Use the correct syntax

* Fix whitespace errors

* Fix the xml doc

* fix

This resolves the issue with the new edge case where a scene could be unloaded but a spawned object referencing the scene handle could still be spawned (pending being de-spawned within a few frames).

---------

Co-authored-by: Noel Stephens <[email protected]>

Author: EmandM

.yamato/cmb-service-standalone-tests.yml

@@ -34,7 +34,7 @@
 cmb_service_standalone_test_{{ project.name }}_{{ platform.name }}_{{ backend }}_{{ editor }}:
   name : CMB Service Test - NGO {{ project.name }} - [{{ platform.name }}, {{ editor }}, {{ backend }}]
   agent:
-    type: {{ platform.type }}::GPU
+    type: {{ platform.type }}
     image: {{ platform.image }}
     flavor: {{ platform.flavor }}
 

.yamato/desktop-standalone-tests.yml

@@ -36,7 +36,7 @@
 desktop_standalone_build_{{ project.name }}_{{ platform.name }}_{{ backend }}_{{ editor }}:
   name : Standalone Build - NGO {{ project.name }} - [{{ platform.name }}, {{ editor }}, {{ backend }}]
   agent:
-    type: {% if platform.name == "mac" %} {{ platform.type }} {% else %} {{ platform.type }}::GPU {% endif %}
+    type: {% if platform.name == "mac" %} {{ platform.type }} {% else %} {{ platform.type }} {% endif %}
     image: {{ platform.image }}
     flavor: {{ platform.flavor }}
 {% if platform.name == "mac" %}
@@ -70,7 +70,7 @@ desktop_standalone_build_{{ project.name }}_{{ platform.name }}_{{ backend }}_{{
 desktop_standalone_test_{{ project.name }}_{{ platform.name }}_{{ backend }}_{{ editor }}:
   name : Standalone Test - NGO {{ project.name }} - [{{ platform.name }}, {{ editor }}, {{ backend }}]
   agent:
-    type: {% if platform.name == "mac" %} {{ platform.type }} {% else %} {{ platform.type }}::GPU {% endif %}
+    type: {% if platform.name == "mac" %} {{ platform.type }} {% else %} {{ platform.type }} {% endif %}
     image: {{ platform.image }}
     flavor: {{ platform.flavor }}
 {% if platform.name == "mac" %}

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -10,6 +10,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Added
 
+- `NetworkSceneManager` as an internal wrapper for the `SceneManager.Scene.handle` and swapped all places that use an `int` to represent a `Scene.handle` to instead use the `NetworkSceneManager`. (#3647)
 - Clicking on the Help icon in the inspector will now redirect to the relevant documentation. (#3663)
 - Added a `Set` function onto `NetworkList` that takes an optional parameter that forces an update to be processed even if the current value is equal to the previous value. (#3690)
 
@@ -26,6 +27,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Distributed authority clients no longer send themselves in the `ClientIds` list when sending a `ChangeOwnershipMessage`. (#3687)
 - Made a variety of small performance improvements. (#3683)
 
 ### Security

com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs

@@ -1367,12 +1367,12 @@ public bool IsNetworkVisibleTo(ulong clientId)
         ///  the most important part to uniquely identify in-scene
         ///  placed NetworkObjects
         /// </summary>
-        internal int SceneOriginHandle = 0;
+        internal NetworkSceneHandle SceneOriginHandle;
 
         /// <summary>
         /// The server-side scene origin handle
         /// </summary>
-        internal int NetworkSceneHandle = 0;
+        internal NetworkSceneHandle NetworkSceneHandle;
 
         private Scene m_SceneOrigin;
         /// <summary>
@@ -1393,7 +1393,7 @@ internal Scene SceneOrigin
             {
                 // The scene origin should only be set once.
                 // Once set, it should never change.
-                if (SceneOriginHandle == 0 && value.IsValid() && value.isLoaded)
+                if (SceneOriginHandle.IsEmpty() && value.IsValid() && value.isLoaded)
                 {
                     m_SceneOrigin = value;
                     SceneOriginHandle = value.handle;
@@ -1405,13 +1405,13 @@ internal Scene SceneOrigin
         /// Helper method to return the correct scene handle
         /// Note: Do not use this within NetworkSpawnManager.SpawnNetworkObjectLocallyCommon
         /// </summary>
-        internal int GetSceneOriginHandle()
+        internal NetworkSceneHandle GetSceneOriginHandle()
         {
-            if (SceneOriginHandle == 0 && IsSpawned && IsSceneObject != false)
+            if (SceneOriginHandle.IsEmpty() && IsSpawned && IsSceneObject != false)
             {
                 throw new Exception($"{nameof(GetSceneOriginHandle)} called when {nameof(SceneOriginHandle)} is still zero but the {nameof(NetworkObject)} is already spawned!");
             }
-            return SceneOriginHandle != 0 ? SceneOriginHandle : gameObject.scene.handle;
+            return !SceneOriginHandle.IsEmpty() ? SceneOriginHandle : gameObject.scene.handle;
         }
 
         /// <summary>
@@ -2907,7 +2907,7 @@ public struct TransformData : INetworkSerializeByMemcpy
             public NetworkObject OwnerObject;
             public ulong TargetClientId;
 
-            public int NetworkSceneHandle;
+            public NetworkSceneHandle NetworkSceneHandle;
 
             internal int SynchronizationDataSize;
 
@@ -3420,15 +3420,14 @@ internal void SceneChangedUpdate(Scene scene, bool notify = false)
             {
                 // Since the authority is the source of truth for the NetworkSceneHandle,
                 // the NetworkSceneHandle is the same as the SceneOriginHandle.
-                if (NetworkManager.DistributedAuthorityMode)
+                if (NetworkManager.DistributedAuthorityMode && NetworkManager.SceneManager.ClientSceneHandleToServerSceneHandle.ContainsKey(SceneOriginHandle))
                 {
                     NetworkSceneHandle = NetworkManager.SceneManager.ClientSceneHandleToServerSceneHandle[SceneOriginHandle];
                 }
                 else
                 {
                     NetworkSceneHandle = SceneOriginHandle;
                 }
-
             }
             else // Otherwise, the client did not find the client to server scene handle
             if (NetworkManager.LogLevel == LogLevel.Developer)

com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs

@@ -1,4 +1,6 @@
+using System.Linq;
 using System.Runtime.CompilerServices;
+using UnityEngine;
 
 namespace Unity.Netcode
 {
@@ -143,42 +145,47 @@ public bool Deserialize(FastBufferReader reader, ref NetworkContext context, int
         public void Handle(ref NetworkContext context)
         {
             var networkManager = (NetworkManager)context.SystemOwner;
+            var hasObject = networkManager.SpawnManager.SpawnedObjects.TryGetValue(NetworkObjectId, out var networkObject);
 
             // If we are the DAHost then forward this message
             if (networkManager.DAHost)
             {
-                var shouldProcessLocally = HandleDAHostMessageForwarding(ref networkManager, context.SenderId);
+                var shouldProcessLocally = HandleDAHostMessageForwarding(ref networkManager, context.SenderId, hasObject, ref networkObject);
                 if (!shouldProcessLocally)
                 {
                     return;
                 }
             }
 
+            if (!hasObject)
+            {
+                if (networkManager.LogLevel <= LogLevel.Normal)
+                {
+                    NetworkLog.LogError("Ownership change received for an unknown network object. This should not happen.");
+                }
+                return;
+            }
+
             // If ownership is changing (either a straight change or a request approval), then run through the ownership changed sequence
             // Note: There is some extended ownership script at the bottom of HandleOwnershipChange
             // If not in distributed authority mode, ChangeMessageType will always be OwnershipChanging.
             if (ChangeMessageType == ChangeType.OwnershipChanging || ChangeMessageType == ChangeType.RequestApproved || !networkManager.DistributedAuthorityMode)
             {
-                HandleOwnershipChange(ref context);
+                HandleOwnershipChange(ref context, ref networkManager, ref networkObject);
             }
             else if (networkManager.DistributedAuthorityMode)
             {
                 // Otherwise, we handle and extended ownership update
-                HandleExtendedOwnershipUpdate(ref context);
+                HandleExtendedOwnershipUpdate(ref context, ref networkObject);
             }
         }
 
         /// <summary>
         /// Handle the extended distributed authority ownership updates
         /// </summary>
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        private void HandleExtendedOwnershipUpdate(ref NetworkContext context)
+        private void HandleExtendedOwnershipUpdate(ref NetworkContext context, ref NetworkObject networkObject)
         {
-            var networkManager = (NetworkManager)context.SystemOwner;
-
-            // Handle the extended ownership message types
-            var networkObject = networkManager.SpawnManager.SpawnedObjects[NetworkObjectId];
-
             if (ChangeMessageType == ChangeType.OwnershipFlagsUpdate)
             {
                 // Just update the ownership flags
@@ -199,10 +206,8 @@ private void HandleExtendedOwnershipUpdate(ref NetworkContext context)
         /// Handle the traditional change in ownership message type logic
         /// </summary>
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        private void HandleOwnershipChange(ref NetworkContext context)
+        private void HandleOwnershipChange(ref NetworkContext context, ref NetworkManager networkManager, ref NetworkObject networkObject)
         {
-            var networkManager = (NetworkManager)context.SystemOwner;
-            var networkObject = networkManager.SpawnManager.SpawnedObjects[NetworkObjectId];
             var distributedAuthorityMode = networkManager.DistributedAuthorityMode;
 
             // Sanity check that we are not sending duplicated change ownership messages
@@ -260,12 +265,12 @@ private void HandleOwnershipChange(ref NetworkContext context)
         /// </summary>
         /// <param name="networkManager">The current NetworkManager from the NetworkContext</param>
         /// <param name="senderId">The sender of the current message from the NetworkContext</param>
+        /// <param name="hasObject">Whether the local client has this object spawned</param>
+        /// <param name="networkObject">The networkObject we are changing ownership on. Will be null if hasObject is false.</param>
         /// <returns>true if this message should also be processed locally; false if the message should only be forwarded</returns>
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        private bool HandleDAHostMessageForwarding(ref NetworkManager networkManager, ulong senderId)
+        private bool HandleDAHostMessageForwarding(ref NetworkManager networkManager, ulong senderId, bool hasObject, ref NetworkObject networkObject)
         {
-            var clientList = ClientIdCount > 0 ? ClientIds : networkManager.ConnectedClientsIds;
-
             var message = new ChangeOwnershipMessage()
             {
                 NetworkObjectId = NetworkObjectId,
@@ -302,20 +307,44 @@ private bool HandleDAHostMessageForwarding(ref NetworkManager networkManager, ul
             }
             else
             {
+                var clientList = ClientIds;
+                var errorOnSender = true;
+
+                // OwnershipFlagsUpdate doesn't populate the ClientIds list.
+                if (ChangeMessageType == ChangeType.OwnershipFlagsUpdate)
+                {
+                    // if the DAHost can see this object, forward the message to all observers.
+                    // if the DAHost can't see the object, forward the message to everyone.
+                    clientList = hasObject ? networkObject.Observers.ToArray() : networkManager.ConnectedClientsIds.ToArray();
+
+                    // Both clientList arrays will have the local client so we can not throw an error.
+                    errorOnSender = false;
+                }
+
                 foreach (var clientId in clientList)
                 {
                     // Don't forward to self or originating client
-                    if (clientId == networkManager.LocalClientId || clientId == senderId)
+                    if (clientId == networkManager.LocalClientId)
                     {
                         continue;
                     }
 
+                    if (clientId == senderId)
+                    {
+                        if (errorOnSender)
+                        {
+                            Debug.LogError($"client-{senderId} sent a ChangeOwnershipMessage with themself inside the ClientIds list.");
+                        }
+
+                        continue;
+                    }
+
                     networkManager.ConnectionManager.SendMessage(ref message, NetworkDelivery.Reliable, clientId);
                 }
             }
 
             // Return whether to process the message on the DAHost itself (only if object is spawned).
-            return networkManager.SpawnManager.SpawnedObjects.ContainsKey(NetworkObjectId);
+            return hasObject;
         }
     }
 }

com.unity.netcode.gameobjects/Runtime/SceneManagement/DefaultSceneManagerHandler.cs

@@ -20,7 +20,7 @@ internal struct SceneEntry
         }
         public bool IsIntegrationTest() { return false; }
 
-        internal Dictionary<string, Dictionary<int, SceneEntry>> SceneNameToSceneHandles = new Dictionary<string, Dictionary<int, SceneEntry>>();
+        internal Dictionary<string, Dictionary<NetworkSceneHandle, SceneEntry>> SceneNameToSceneHandles = new();
 
         public AsyncOperation LoadSceneAsync(string sceneName, LoadSceneMode loadSceneMode, SceneEventProgress sceneEventProgress)
         {
@@ -47,7 +47,7 @@ public void ClearSceneTracking(NetworkManager networkManager)
         /// <summary>
         /// Stops tracking a specific scene
         /// </summary>
-        public void StopTrackingScene(int handle, string name, NetworkManager networkManager)
+        public void StopTrackingScene(NetworkSceneHandle handle, string name, NetworkManager networkManager)
         {
             if (SceneNameToSceneHandles.ContainsKey(name))
             {
@@ -69,7 +69,7 @@ public void StartTrackingScene(Scene scene, bool assigned, NetworkManager networ
         {
             if (!SceneNameToSceneHandles.ContainsKey(scene.name))
             {
-                SceneNameToSceneHandles.Add(scene.name, new Dictionary<int, SceneEntry>());
+                SceneNameToSceneHandles.Add(scene.name, new Dictionary<NetworkSceneHandle, SceneEntry>());
             }
 
             if (!SceneNameToSceneHandles[scene.name].ContainsKey(scene.handle))
@@ -168,7 +168,7 @@ public Scene GetSceneFromLoadedScenes(string sceneName, NetworkManager networkMa
         /// same application instance is still running, the same scenes are still loaded on the client, and
         /// upon reconnecting the client doesn't have to unload the scenes and then reload them)
         /// </summary>
-        public void PopulateLoadedScenes(ref Dictionary<int, Scene> scenesLoaded, NetworkManager networkManager)
+        public void PopulateLoadedScenes(ref Dictionary<NetworkSceneHandle, Scene> scenesLoaded, NetworkManager networkManager)
         {
             SceneNameToSceneHandles.Clear();
             var sceneCount = SceneManager.sceneCount;
@@ -177,7 +177,7 @@ public void PopulateLoadedScenes(ref Dictionary<int, Scene> scenesLoaded, Networ
                 var scene = SceneManager.GetSceneAt(i);
                 if (!SceneNameToSceneHandles.ContainsKey(scene.name))
                 {
-                    SceneNameToSceneHandles.Add(scene.name, new Dictionary<int, SceneEntry>());
+                    SceneNameToSceneHandles.Add(scene.name, new Dictionary<NetworkSceneHandle, SceneEntry>());
                 }
 
                 if (!SceneNameToSceneHandles[scene.name].ContainsKey(scene.handle))

com.unity.netcode.gameobjects/Runtime/SceneManagement/ISceneManagerHandler.cs

@@ -14,12 +14,12 @@ internal interface ISceneManagerHandler
 
         AsyncOperation UnloadSceneAsync(Scene scene, SceneEventProgress sceneEventProgress);
 
-        void PopulateLoadedScenes(ref Dictionary<int, Scene> scenesLoaded, NetworkManager networkManager = null);
+        void PopulateLoadedScenes(ref Dictionary<NetworkSceneHandle, Scene> scenesLoaded, NetworkManager networkManager = null);
         Scene GetSceneFromLoadedScenes(string sceneName, NetworkManager networkManager = null);
 
         bool DoesSceneHaveUnassignedEntry(string sceneName, NetworkManager networkManager = null);
 
-        void StopTrackingScene(int handle, string name, NetworkManager networkManager = null);
+        void StopTrackingScene(NetworkSceneHandle handle, string name, NetworkManager networkManager = null);
 
         void StartTrackingScene(Scene scene, bool assigned, NetworkManager networkManager = null);