fix: change OwnerClientId before firing OnGainedOwnership() callback #1092
Conversation
com.unity.netcode.gameobjects/Runtime/Messaging/InternalMessageHandler.cs
Show resolved
Hide resolved
|
|
||
| var networkObject = NetworkManager.SpawnManager.SpawnedObjects[networkId]; | ||
| if (networkObject.OwnerClientId == NetworkManager.LocalClientId) | ||
| if (!NetworkManager.SpawnManager.SpawnedObjects.TryGetValue(networkObjectId, out var networkObject)) |
There was a problem hiding this comment.
Nice... trap ahead of time for invalid usage or non-existent NetworkObjects.
| m_DummyPrefab = new GameObject("DummyPrefabPrototype"); | ||
| m_DummyPrefab.AddComponent<NetworkObject>(); | ||
| m_DummyPrefab.AddComponent<NetworkObjectOwnershipComponent>(); | ||
| MultiInstanceHelpers.MakeNetworkedObjectTestPrefab(m_DummyPrefab.GetComponent<NetworkObject>()); |
There was a problem hiding this comment.
For this test you probably don't need to worry about it but I wanted to bring this to your attention.
When we create prefabs using MultiInstanceHelpers.MakeNetworkedObjectTestPrefab I am going to propose we make it a "best practice" to give it some form of GlobalObjectIdHash value other than default (i.e zero). It looks like it will work out for you since you are just dealing with 1 NetworkPrefab...but if you had more than one it would become problematic since we synchronize everything off of the GlobalObjectIdHash value.
Not requesting you change it, but just making you aware of that area of "pain" to watch out for.
There was a problem hiding this comment.
MakeNetworkedObjecttestPrefab kind of does that for us:
public static void MakeNetworkedObjectTestPrefab(NetworkObject networkObject, uint globalObjectIdHash = default)
{
// Set a globalObjectId for prefab
if (globalObjectIdHash != default)
{
networkObject.TempGlobalObjectIdHashOverride = globalObjectIdHash;
}
// Force generation
networkObject.GenerateGlobalObjectIdHash();
// Prevent object from being snapped up as a scene object
networkObject.IsSceneObject = false;
}There was a problem hiding this comment.
default value of uint is?
There was a problem hiding this comment.
0 but we force generate if it's default. if it's not default (0) then we override its hash with whatever we passed in. this code block covers both use cases.
There was a problem hiding this comment.
you were right! addressing with #1094
fixes #1075
Summary
we were calling OnLostOwnership() and OnGainedOwnership() callbacks before we change NetworkObject.OwnerClientId property.
with this PR, we're calling OnLostOwnership() callback, change NetworkObject.OwnerClientId and then call OnGainedOwnership() callback.
this makes it more intiutive and useful (arguably) for developers. now they can handle losing ownership in their OnLostOwnership() callback while still having previous OwnerClientId and then later on, they can also handle latest/new ownership change in OnGainedOwnership() callback to do something with new OwnerClientId.