Calls to `ChangeOwnership(NetworkManager.ServerClientId)` and `RemoveOwnership()` doesn't have same effect

[PitouGames]

Description

While working on #2507, I remarked that calling serverObject.ChangeOwnership(NetworkManager.ServerClientId) doesn't have the same effect as serverObject.RemoveOwnership(). Moreover, some automated tests use the first one, others the second.

The difference is subtil, but server side, when removing the ownership, the object entry is deleted in NetworkSpawnManager.OwnershipToObjectsTable, while giving ownership back to the server keeps it.
Since the ownership is given back to the server in both case, I think we should let the entry in the table. The entry should be deleted only on despawn.

com.unity.netcode.gameobjects/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs

Line 197 in a418eab

internal void RemoveOwnership(NetworkObject networkObject)

VS

com.unity.netcode.gameobjects/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs

Line 259 in a418eab

internal void ChangeOwnership(NetworkObject networkObject, ulong clientId)

Shouldn't RemoveOwnership() just calls ChangeOwnership(NetworkManager.ServerClientId) instead of having a "copy/paste but not identical" implementation?
If there is a difference made on purpose between those two calls, documentation needs to be updated to explain it.

Environment

• OS: Windows 11
• Unity Version: 2020.3.40f1 (the automated testing project of this repo)
• Netcode Version: develop
• Netcode Commit: 449bf94

[NoelStephensUnity]

@PitouGames
The two methods actually are not identical.
Remove ownership removes any previous non-server client ownership entries for the NetworkObject while also broadcasting the removal of ownership to all clients (whether visible or not).
Change ownership implies that there is a transfer of ownership which includes some additional factoring:

• All NetworkVariables belonging to the associated NetworkBehaviours are marked as dirty
• The NetworkObject is marked to be updated for NetworkVariable synchronization
  • Primarily for owner permission based NetworkVariables
• Only the clients that have visibility of the NetworkObject are updated.

So, there are different contexts to consider.
If you are dealing with a host which is both a client and a server, then there are scenarios where invoking ChangeOwnership are more pertinent than RemoveOwnership. Likewise, if you are running a server only then it might not make sense to use ChangeOwnership while passing the server id as opposed to simply invoking the RemoveOwnership method.

The "quirky" element here is when running a host since the host-server and host-client are the same instance, and it is understandable that it can be a bit confusing.

The distinctive differences being that RemoveOwnership removes ownership entries completely and notifies all clients (ignoring visibility) and ChangeOwnership transfers ownership between clients while also taking into consideration NetworkVariables and NetworkObject visibility.

The two distinct behaviors are intentional and by design.
Does this help to clarify the differences between the two methods?

[PitouGames]

@NoelStephensUnity
Thank you for the answer. Now I'm 100% sure it's intentionnal, but I still didn't fully understand the difference and why there should be one.

• The only documentation on ownership I found is https://docs-multiplayer.unity3d.com/netcode/current/basics/networkobject#ownership, which is really basic and doesn't cover this question. Maybe you should add a dedicated and more advanced page on ownership?

• There is no automated tests (or I didn't found them) that illustrate an verify this intentional difference.

---

I think there is an issue with the implementation:

If the two calls are different, why calling RemoveOwnership(); has no effect when called after a ChangeOwnership(NetworkManager.ServerClientId);? (or in reverse order)

I know it doesn't really make sens to do this in a game, but I miss the logic.

1. Call ChangeOwnership(1); put the NetworkObject in the NetworkSpawnManager.OwnershipToObjectsTable[1] dictionnary
2. Call ChangeOwnership(NetworkManager.ServerClientId) move the NetworkObject to the NetworkSpawnManager.OwnershipToObjectsTable[0] dictionnary
3. Call RemoveOwnership() doesn't remove the NetworkObject in the NetworkSpawnManager.OwnershipToObjectsTable[0] dictionnary and has no effect at all because the call is ignored since server is already the owner

While:

1. Call ChangeOwnership(1); put the NetworkObject in the NetworkSpawnManager.OwnershipToObjectsTable[1] dictionnary
2. Call RemoveOwnership() only remove from NetworkObject in the NetworkSpawnManager.OwnershipToObjectsTable[1]. It doesn't add it to NetworkSpawnManager.OwnershipToObjectsTable[0] dictionnary

At the end, the publicly accessible NetworkSpawnManager.OwnershipToObjectsTable isn't in the same state, and I don't think is normal.

---

An other issue I found with the NetworkSpawnManager.OwnershipToObjectsTable: when an object spawn (with NetworkShow for example), it is added in the OwnershipToObjectsTable on the client that spawned the object, even if it shouldn't be tracked in the table because the object is owned by an other client (not the server nor himself).

---

I also spoted one other little thing compared to what you said:

RemoveOwnership removes ownership entries completely and notifies all clients (ignoring visibility)

It works for dynamicaly spawned objects, but this is not true for In-Scene placed NetworkObject:

1. Call NetworkObject.ChangeOwnership(1);
2. Call NetworkObject.NetworkHide(1);. It doesn't disable (despawn) the in-scene placed object
3. Call NetworkObject.RemoveOwnership();. Client gets a warning "[Netcode] Deferred messages were received for a trigger of type OnSpawn with key 1, but that trigger was not received within within 1 second(s)."

Now client and host both see at the same time that they own the object, RemoveOwnership didn't removed the ownership on the client. As soon as NetworkObject.NetworkShow(1); is called, the situation gets back to normal.

[NoelStephensUnity]

@PitouGames

There is no automated tests (or I didn't found them) that illustrate an verify this intentional difference.

Have a look at the following tests:
NetworkObjectNetworkClientOwnedObjectsTests.cs
NetworkObjectOwnershipTests.cs

Call RemoveOwnership() doesn't remove the NetworkObject in the NetworkSpawnManager.OwnershipToObjectsTable[0] dictionnary and has no effect at all because [the call is ignored since server is already the owner]

A few reasons behind this... but one way to look at that is memory allocation...would it make sense to completely remove ownership of a NetworkObject from a table? Now, before you answer this in your head...you need to think about all possible cases and not the specific cases you might have in mind. A user might want to change ownership frequently (i.e. a ball being passed back and fourth and each time the ball is "caught" ownership changes).
Scenario:
Suppose you have:

• A server (dedicated)
  • Ball (Starts off owned by server but neither client have ownership)
• Client A (remote)
• Client B (remote)

Now think about:

• Client A picks up the ball first
  • Ownership changes to Client A (table entry created)
• Client A tries to throw the ball at a target
  • Ownership changes back to "neither Client A or Client B"
• Client B intercepts the ball (i.e. catches it)
  • Ownership changes to Client B

Now repeat that process over and over... or perhaps you could have many players with many balls...the idea being that it is "ok" for "no one" to have ownership in the table when you "remove ownership" (the server is always the "default owner").
Now suppose you have a host...where it represents both the "server" and "a client". How would you be able to distinguish between the host-client having ownership versus "no one" (i.e. the "server") having ownership?
You would use:

• RemoveOwnership to make "no-one" have ownership (remains in table)
• ChangeOwnership to make the "host-client" have ownership

RemoveOwnership actually does remove the NetworkObject from the owner key entry by setting the isRemoving parameter to "true" unless it is the server and the server is already the owner. Really, changing the ownership to the server using ChangeOwnership and then trying to have the server remove ownership from itself is sort of a quirky logic to think about. You are assigning ownership to yourself (i.e. server is the only instance that can change ownership) and then you want to "remove ownership"....in the end the NetworkObject ownership on the server and all connected clients remains the same: the server owns the object...so I think I see your point about "why keep the entry"...but then in all reality the next question to ask is "why not keep the entry if it makes no difference in ownership"?

Really, RemoveOwnership is a bit quirky...so I totally understand some of the confusion...but in the end the biggest thing to look at would be NetworkSpawnManager.OwnershipToObjectsTable. Based on what you are describing, when using this public ownership to object table how does this get impacted?
You assign ownership to the server. (OwnershipToObjectsTable will reflect that the object belongs to the server)
You then invoke remove ownership but nothing seems to change...the server is still the owner...so should the OwnershipToObjectsTable still reflect that the server owns the object? (the answer is yes).
Perhaps thinking of it in this perspective might help shed some light in that (admittedly seemingly quirky logic). 😸

Call NetworkObject.ChangeOwnership(1);
Call NetworkObject.NetworkHide(1);. It doesn't disable (despawn) the in-scene placed object
Call NetworkObject.RemoveOwnership();. Client gets a warning "[Netcode] Deferred messages were received for a trigger of type OnSpawn with key 1, but that trigger was not received within within 1 second(s)."

This actually could be a bug... but I would suggest reading over spawning and despawning in-scene placed NetworkObjects and the rest of that section to get a better idea of the differences.

Really, in-scene placed NetworkObjects were originally intended to be used as "management" oriented NetworkBehaviours that would more be used to dynamically spawn NetworkObjects than to be used like a dynamically spawned NetworkObject. I could write pages as to why this ended up this way... but the summary is:
In-Scene Placed NetworkObjects are unique because they are instantiated by the scene itself and you can load the same scene repeatedly (additively) where the GlobalObjectIdHash remains the same (i.e. each in-scene placed NetworkObject has a unique GlobalObjectIdHash value assigned to it when placed in a scene)...so you run into this scenario where you want to synchronize late joining players...but you have the same scene loaded 10 times that all have an in-scene placed NetworkObject with the same GlobalObjectIdHash...and you need to be able to distinguish between each in-scene placed NetworkObject in order to synchronize them (initial synchronization of late joining players)...which the only way to do this (with the current back-end architecture) is to associate each instance with the scene handle (which is unique as well)...

Needless to say...I would recommend (until some of the internal architecture is updated) that if you are going to be doing the above combination you might avoid using an in-scene placed NetworkObject and would recommend using the non-pooled "hybrid approach". This will avoid some of the limitations of in-scene placed NetworkObjects (apologies for those...we are aware and discussing/working towards an improved internal/back-end architecture).

Regarding the change of ownership and then hiding in-scene placed NetworkObjects... that is a good call out and we should improve our object-visibility documentation as well as investigate a potential fix for that specific issue.

[PitouGames]

@NoelStephensUnity
Thank you for the very detailed answer, and sorry for the delay of my response.

Concerning the automated tests, I want to highlight that you can replace RemoveOwnership with:

internal void RemoveOwnership(NetworkObject networkObject)
{
    ChangeOwnership(networkObject, NetworkManager.ServerClientId);
}

and no test fails, showing that this intentionnal behaviour difference isn't properly tested. That's the main reason I have created this issue, I couldn't know if the difference was intentionnal or not (lack of tests and documentation).

For all the others thing we spooted along the path, it shouldn't cause any trouble as long as the dev don't mess up with the two functions as I did for testing. But it's good to know it's there 😉

[NoelStephensUnity]

@PitouGames
This issue is resolved in PR #2618 and will be included in the next patch update.
👍