-
Notifications
You must be signed in to change notification settings - Fork 434
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Calls to ChangeOwnership(NetworkManager.ServerClientId)
and RemoveOwnership()
doesn't have same effect
#2511
Comments
@PitouGames
So, there are different contexts to consider. 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. |
@NoelStephensUnity
I think there is an issue with the implementation: If the two calls are different, why calling I know it doesn't really make sens to do this in a game, but I miss the logic.
While:
At the end, the publicly accessible An other issue I found with the I also spoted one other little thing compared to what you said:
It works for dynamicaly spawned objects, but this is not true for In-Scene placed NetworkObject:
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 |
Have a look at the following tests:
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).
Now think about:
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").
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?
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: 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. |
@NoelStephensUnity 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 😉 |
@PitouGames |
Description
While working on #2507, I remarked that calling
serverObject.ChangeOwnership(NetworkManager.ServerClientId)
doesn't have the same effect asserverObject.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
com.unity.netcode.gameobjects/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs
Line 259 in a418eab
Shouldn't
RemoveOwnership()
just callsChangeOwnership(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
The text was updated successfully, but these errors were encountered: