ClientNetworkTransform does not allow for server to commit after changing ownership

AlexEnnerfelt commented 1 year ago

Description

If a NetworkObject that has a ClientNetworkTransform (implemented as per the documentation) the Host/Server can commit the transform while it has not yet changed ownership. When another client takes ownership, the ability to commit transform is removed from the Server (so far so good) If the server then has the ownership returned to it, it will still not be allowed to commit transform.

Reproduce Steps

Set up NetworkObject with ClientNetworkTransform
Change ownership to client 1 (server loses right to commit transform)
Change ownership to client 0 (the Host)
The host still can't commit transform

Actual Outcome

The Host cannot commit transform for an owned object with ClientNetworkTransform when it has previously been owned by another client.

Expected Outcome

The Host should be treated as a client and regain right to commit transform on a ClientNetworkTransform despite having previously been owned by another client.

Screenshots

If applicable, add screenshots to help explain your problem.

Environment

OS: Wibndows 11
Unity Version: 2022.2.19
Netcode Version: 1.4.0
Netcode Commit: latest release in the unity registry

Additional Context

Add any other context about the problem here. Logs, code snippets would be useful here but please also consider attaching a minimal Unity project that reproduces the issue.

AlexEnnerfelt commented 1 year ago

Just realized that I was getting warnings that the change was happening while the some behaviours were disabled. I had to refactor a bit, but it does strike me as a problem that the ownerclientid changed in the editor, but it's not triggering certain thing in the behaviours.

AlexEnnerfelt commented 1 year ago

Is there any guidance on how to deal with Networkbehaviours that were not active during ownership assignment, like say for example the NetworkTransform

AlexEnnerfelt commented 1 year ago

I reverted back to Netcode 1.3.1 and all of my Networktransform issues went away

NoelStephensUnity commented 1 year ago

@AlexEnnerfelt I would like to investigate this scenario further but have some questions about the issue you ran into. The following project allows you to change the ownership of a "follower" object that follows the player that owns the object. The players automatically just move in a circular pattern when connected (for simplicity sakes) and hitting the "space bar" on the host side changes the owner. I am able to switch between owners and all NetworkTransforms used are ownership authoritative and it appears switching ownership between multiple instances (had three local instances running with one as a host) and couldn't replicate the issue you described: OwnershipChanges.zip

Could you further describe what it was that caused it to stop updating after ownership changed the second time back to the host? Also, if you can describe why the host side NetworkTransform was disabled and perhaps when that happened?

It will help me track down the issue you ran into.

AlexEnnerfelt commented 1 year ago

It seems like it was caused by that the server does not invoke the OnOwnershipGained when ownership is changed to server it invoked OnOwnershipRemoved. My solution that made ClientNetworkTransform work again is:

public class ExtendedClientNetworkTransform : NetworkTransform
    {
        /// <summary>
        /// Used to determine who can write to this transform. Owner client only.
        /// This imposes state to the server. This is putting trust on your clients. Make sure no security-sensitive features use this transform.
        /// </summary>
        protected override bool OnIsServerAuthoritative()
        {
            return false;
        }

        public override void OnGainedOwnership()
        {
            base.OnGainedOwnership();
            if (IsServer && OwnerClientId != NetworkManager.LocalClientId)
            {
                OnLostOwnership();
            }
        }

        public override void OnLostOwnership()
        {
            base.OnLostOwnership();
            if (IsServer && OwnerClientId == NetworkManager.LocalClientId)
            {
                OnGainedOwnership();
            }
        }
    }

Credit David Gilbert on the Discord

NoelStephensUnity commented 1 year ago

@AlexEnnerfelt Ahhh... interesting... Was this happening on the player object specifically or just some other dynamically spawned or in-scene placed NetworkObject?

Laumania commented 1 year ago

Just wanted to tip in on this one too, as I have spend some hours now trying to figure what I messed up. Turns out @AlexEnnerfelt fix makes my game work again too. I created a little video showing the issue and that it works after the fix (however, first time the clone didn't pick up the code change, so it didn't work but I figure it out and didn't wanted to redo the video :)

https://www.youtube.com/watch?v=JxuNejhd8EM

Don't know if this help you @NoelStephensUnity just wanted to let you know that the fix from above actually worked for me too.

davidyrgilbert commented 1 year ago

Oh I remember posting this one on Discord but never got around to creating an issue.

The reason this happens is actually quite clear if you go through the ownership change code (which IMO is quite broken/does not work as you'd think):

The first strange thing is that the server is always invoking InvokeBehaviourOnLostOwnership here , even if the server actually gained ownership.

The second issue is that if you execute RemoveOwnership, the logic early-exits here, never actually calling any other callback besides aforementioned Lost on the server. My hack that @AlexEnnerfelt thankfully posted actually uses this issue to abuse the lost ownership callback as an ownership gain callback. It works very well after testing everything for a while, but cannot be the intended behaviour.

davidyrgilbert commented 1 year ago

@AlexEnnerfelt Ahhh... interesting... Was this happening on the player object specifically or just some other dynamically spawned or in-scene placed NetworkObject?

This happens on both dynamically spawned and in-scene placed NetworkObjects. I never tried changing ownership of the player object, but I imagine it will happen too. It should also happen on dedicated servers and hosts, at least it was for me.

Just putting Debug.Logs in the Gained/Lost callbacks should already show that things are off (or at least not named correctly).

NoelStephensUnity commented 1 year ago

@davidyrgilbert

The first strange thing is that the server is always invoking InvokeBehaviourOnLostOwnership here , even if the server actually gained ownership.

This is actually not broken for a client-server architecture where the server maintains the root authority for everything. The server-side is being notified of "who lost ownership" and then "who gained ownership" because that information is pertinent from an authority perspective (i.e. server should know both who lost and who gained ownership even if the server itself is gaining ownership). So, it really isn't that strange that the server is notified when ownership is both lost and gained since there are scenarios where you actually might want to know which client lost ownership.

What I do think can be confusing is the entire concept of "IsServer or IsClient" and whether the code written is taking that into consideration or if each relative callback handler only takes into consideration the type of handler being invoked (i.e. lost ownership and gained ownership handlers only handle the specific change in ownership but don't check IsServer).

In an effort to semi-contradict my previous statements, the second (and third to follow) thing I do think is a fundamental confusing design is that ownership messages are only sent to current and new owners and not broadcasted (but all ownership notifications are "broadcasted" internally on the server-side which makes it a bit more confusing for host mode). Taking all of that into consideration, it would make more sense (the "third thing"), if ownership changes were broadcasted, to treat it like NetworkVariable change notifications where the handler was just "OnOwnershipChanged" and it included the previous and new client identifiers (and the message was just broadcasted to the server and all clients...with the exception of the host where that would just be invoked once).

However, the current design decision was made a very long time ago and the primary concern is that any adjustments to the "server authority approach that only informs the previous and new owners (with the caveat that the server knows about all ownership changes)" would be considered a breaking change since it changes the fundamental "behavior".

Sooo...if an opportunity arises where such things get the green light to adjust I will most definitely push for a more uniform broadcasted notification of ownership change that includes the previous and new owner.

Until then the following is the ownership notification "logic" to follow:

Server or Host: Notified of all ownership changes (where ownership lost is invoked even if the server is the new owner)
Clients: Only notified when ownership is gained and when ownership is lost but will never be notified of both (unless it is the client of a host instance)

🤷

davidyrgilbert commented 1 year ago

Thanks for the explanation, it makes a bit more sense now. Broken was the wrong word I guess, more like unintuitive for people not familiar with the inner workings.

What I still think is strange is that the server, when removing ownership, becomes again (as authority), the owner of the object as intended. However, it only notifies the corresponding object (on the server) of ownership lost, not that it actually gained ownership. So the server actually does not track ownership gained in that sense, which leads to above issue.

I fully agree with your third thing, that would be the actual ideal case. For now I'm perfectly happy with the hack, as that seems to have no side effects and actually works smoothly.

Thanks again for all the input and explanations!

NoelStephensUnity commented 1 year ago

The first strange thing is that the server is always invoking InvokeBehaviourOnLostOwnership here , even if the server actually gained ownership.

The second issue is that if you execute RemoveOwnership, the logic early-exits here, never actually calling any other callback besides aforementioned Lost on the server.

To understand that code you sort of need to see the "context" of invocation in order to grok what it is doing.

In all cases, the UpdateOwnershipTable method is always invoked from a server context.

It is invoked from within the ChangeOwnership method which is a "server only" method. I just explained the whole reason behind the "server gets notified of everything".

It is invoked from within the RemoveOwnership method which is a "server only" method and it exits early because there is no need to remove the dictionary entry for that NetworkObject's ownership (remember the context is that the table is a server only thing) and at that one place that you pointed out just prior to reaching that point it checked to see if the dictionary entry existed...since it is a "remove ownership" action (i.e. basically a change ownership with the new owner being the server) there is nothing else to do (i.e. no new entry in the table to add nor does it need to remove the current entry...and just above the code you pointed to it already made the adjustment to that ownership table entry... so...exit early...task completed).

That code I actually did write and it does a few things that are "confusing until you think about the context" and then it starts to make more sense... the table helped to condense much more confusing code down into a tightly coupled singular place to keep track of ownership on the server side...prior to that chunk of code it was (if you can believe) more confusing than it is already today. 😹

NoelStephensUnity commented 1 year ago

Broken was the wrong word I guess, more like unintuitive for people not familiar with the inner workings.

By all means feel free to vent about certain areas in the code base... I am a realist and as such the "reality" is that it is not perfect in many ways and has things about it that are frustrating... If I had the power to just push updates regularly then I would do so, but we are bound to the internal, more refined, update and release process which in some ways do limit the extent of what we can fix at any given time (depending where we are in the green lit release cycle of things).

The good news is this is an open source project and for those that are capable I would encourage creating a fork and making modifications specific to your project's needs and goals... for some the package is all that is needed...for others it makes more sense to fork and modify to meet the needs of your project. This is another reason why making "foundational" changes is something we are trying to stay away from as some foundational changes can make it more difficult for those who already have a fork for their project.

But always feel free to voice your opinions and frustrations... someone told me something about a wheel and squeaking at some point in my life...

😸

NoelStephensUnity commented 1 year ago

What I still think is strange is that the server, when removing ownership, becomes again (as authority), the owner of the object as intended. However, it only notifies the corresponding object (on the server) of ownership lost, not that it actually gained ownership. So the server actually does not track ownership gained in that sense, which leads to above issue.

Yeah... so... think of "ownership" as a "remote client only thing"... the mud in the water is when you are running a host since the "host-client" identifier is the same as the "host-server"...and since you can invoke ChangeOwnership and RemoveOwnership on the host instance without any errors but never notified that ownership has "changed"...because the identifiers are the same...it can become a bit perplexing at first.

This really goes back to a decision that was made awhile back (another legacy architectural decision) that the "Host-Client" would have the same identifier as the "Host-Server"...which then you think about ownership today and it seems like the "Host-Client" should have been given a unique identifier in order to keep the consistency of ownership specific to "clients"... where objects with "no owner" would be "owned" by the server...so having the ability to have a "host-client" own a NetworkObject might have made things a bit less confusing (not sure at this point whether it would or not TBH).

The way I keep it all straight is just accept that a Host is both a "client and a server" (which in reality that is what it is) and so I just view the two as "one" and know that a "Host-Client" owns everything that the "Host-Server" owns...so really since default ownership is always given to the server you can think of any change in ownership being specific to only remote clients...and just remembering that can also help keep one aligned with the NGO ownership model.

Daniel4144 commented 1 year ago

I just had the same issue - after returning the ownership of a dynamically spawned object, the host cannot move the object.

The solution of @AlexEnnerfelt also worked for me.

(Netcode Version 1.4.0 with Unity 2021.3.24f1)

NoelStephensUnity commented 1 year ago

@Daniel4144 @davidyrgilbert @AlexEnnerfelt I am currently looking into this issue and am not seeing the issue when using NetworkObject.ChangeOwnership... but I am seeing the issue when using NetworkObject.RemoveOwnership. My assumption is that you are all using NetworkObject.RemoveOwnership? (Have a pending fix just making sure this is the issue at hand)

Laumania commented 1 year ago

@NoelStephensUnity I can at least confirm that I had the issue with using RemoveOwnership (before I applied the fix previously mentioned).

NoelStephensUnity commented 1 year ago

@Laumania Thank you for confirming this.

So a PR will be posted with an over-all fix for server ownership notifications:

NetworkObject.RemoveOwnership
- Server-host is always notified that it gained ownership unless remove ownership is invoked when it is already the owner.
- This was the core issue as it was not notifying the server-host when it gained ownership via this method
NetworkObject.ChangeOwnership
- Server-host is notified when ownership is transferred to a remote client and the server-host was the original owner.
- Server-host is always notified when ownership changes
- This means when the server-host is owner and ownership is transferred it will trigger the lost ownership and gained ownership notifications.

There was no need to actually change the NetworkTransform after these adjustments, and the changes applied will assure the server-host is notified when ownership changes. The only time it will receive two notifications is if the server-host was the original owner and ownership is transferred to a remote client.

NoelStephensUnity commented 1 year ago

In the next update (most likely a patch) this issue will be resolved and you will no longer need the work around for ownership to properly transfer back to the server-host when removing ownership from a remote client.

Daniel4144 commented 1 year ago

@NoelStephensUnity I was also using the NetworkObject.RemoveOwnership when I had the issue.

NoelStephensUnity commented 1 year ago

Correction on the change: NetworkObject.RemoveOwnership

Now just invokes NetworkObject.ChangeOwnership defaulting to the server client identifier

NetworkObject.ChangeOwnership

Server-host is notified of ownership being lost and gained.
- This is to keep the existing behavior of ownership notifications.

davidyrgilbert commented 1 year ago

Perfekt, I think that should solve the issues completely. Thanks a lot for the detailed response and fixes!

Unity-Technologies / com.unity.netcode.gameobjects