Incorrect value for `ulong previous` in `OnOwnershipChanged` on owner side, triggering issue in `NetworkTransform`

[FureurTime]

Description

In Distributed Authority, there is an issue with the ownership change of NetworkTransform that seems to happen only on the Owner side.
The error message is : Non-owner Client-1 is being updated by network tick still!!!!

If the previous owner gets back the ownership, the error stops.

I use a component that detect collision with the object and request the Owner to change the ownership if certain conditions are met, like this:

public void CheckCollisionFor(GameObject otherGameObject)
{
    if(!HasAuthority && /*some other condition*/)
        RequestOwnershipRpc(NetworkManager.LocalClientId);
}

[Rpc(SendTo.Authority)]
void RequestOwnershipRpc(ulong clientId)
{
    if(HasAuthority && /*some other condition*/)
        networkObject.ChangeOwnership(clientId);
}

But I don't know if the issue only happen when it's the Owner that use NetworkObject.ChangeOwnership() to another client.

The first collision always trigger the issue, but the next collisions seems to be random and not always.

By digging into the code of NetworkTransform, I found the issue is because InternalInitialization is not called but should be called.
InternalInitialization is supposed to update bool CanCommitToTransform, but since it's not called, the value stays to true and triggers the error.

The issue is here, but more generally on all OnOwnershipChanged I think.

NetworkTransform.cs

protected override void OnOwnershipChanged(ulong previous, ulong current)
{
    // If we were the previous owner or the newly assigned owner then reinitialize
    Debug.Log($"[NetworkTransform] ({gameObject.name}) OnOwnershipChanged   previous={previous}, current={current} : {m_CachedNetworkManager.LocalClientId}", this);
    if (current == m_CachedNetworkManager.LocalClientId || previous == m_CachedNetworkManager.LocalClientId)
    {
        InternalInitialization(true);
    }
    base.OnOwnershipChanged(previous, current);
}

On the first call of OnOwnershipChanged after being spawned the output is :

• On the owner of the object: previous = 2 current = 2 : m_CachedNetworkManager.LocalClientId = 1
• On the client that triggers the ownership request : previous = 1 current = 2 : m_CachedNetworkManager.LocalClientId = 2

The Owner doesn't have the correct previous client id, which should be him.
And in the case of NetworkTransform, it doesn't call InternalInitialization to update the CanCommitToTransform variable.

Environment

• OS: Windows 11
• Unity Version: 6000.0.32f1
• Netcode Version: 2.2.0

[NoelStephensUnity]

@FureurTime
I would not use an RPC to handle the request ownership change as we have added some additional features to help with all of this.
What I would do is:

• Mark the NetworkObject as having RequestRequired ownership permissions.
• You can subscribe to NetworkObject.OnOwnershipRequested to have the current owner/authority handle incoming requests to determine if the client requesting ownership can take ownership. (API Link)
• You should also subscribe to NetworkObject.OnOwnershipRequestResponse to handle any situation where the ownership request is denied. (API Link)

So, your NetworkBehaviour should look something like this:

public class MyOwnershipRequestBehavior : NetworkBehaviour
{
    /// <summary>
    /// When handling collision, you should pick "who is going to check".
    /// Due to latency, if you have both the owner and non-owners check you 
    /// can run into a scenario where you run into a race condition where 
    /// both the owner and non-owner are trying to change ownership.
    /// It is better to handle this either on the owner or non-owner side.
    /// For this example, I am following your approach where the non-owner 
    /// is checking for the collision.
    /// </summary>
    /// <param name="otherGameObject"></param>
    public void CheckCollisionFor(GameObject otherGameObject)
    {
        if (!HasAuthority && /*some other condition*/)
        {
            if (NetworkObject.IsOwnershipRequestRequired)
            {
                NetworkObject.RequestOwnership();
            }
            else if (NetworkObject.IsOwnershipTransferable) 
            {
                NetworkObject.ChangeOwnership(NetworkManager.LocalClientId);
            }
        }
    }

    protected override void OnNetworkPostSpawn()
    {
        if (HasAuthority) 
        {
            NetworkObject.OnOwnershipRequested = OwnershipRequested;
        }
        else
        {
            NetworkObject.OnOwnershipRequestResponse = OwnershipRequestResponse;
        }

        base.OnNetworkPostSpawn();
    }

    protected virtual bool ShouldGrantOwnership(ulong clientRequesting)
    {
        // You can add any additional logic here if you want to determine if the request is approved.
        return true;
    }

    private bool OwnershipRequested(ulong clientRequesting)
    {
        var approved = ShouldGrantOwnership(clientRequesting);
        if (approved)
        {
            Debug.Log($"[{name}] Client-{clientRequesting} ownership approved.");
            // Remove the handler for authority request handling
            NetworkObject.OnOwnershipRequested = null;
            // Add the handler for the non-authority request response handling
            NetworkObject.OnOwnershipRequestResponse = OwnershipRequestResponse;
        }
        return approved;
    }

    /// <summary>
    /// When using the <see cref="NetworkObject.OwnershipStatus.RequestRequired"/> permission, 
    /// this will only be invoked if the ownership request is approved by the client authority/owner.
    /// </summary>
    protected override void OnOwnershipChanged(ulong previous, ulong current)
    {
        if (current == NetworkManager.LocalClientId)
        {
            // Remove the handler for non-authority request response handling
            NetworkObject.OnOwnershipRequestResponse = null;
            // Add the handler for the  authority request handling (this c
            NetworkObject.OnOwnershipRequested = OwnershipRequested;

            // You can continue to have normal change in ownership script here
        }
        base.OnOwnershipChanged(previous, current);
    }

    private void OwnershipRequestResponse(NetworkObject.OwnershipRequestResponseStatus ownershipRequestResponse)
    {
        if (ownershipRequestResponse == NetworkObject.OwnershipRequestResponseStatus.Approved)
        {
            // Add any additional logic here to handle the scenario where perhaps the ownership request was denied
            Debug.LogWarning($"[Ownership Request Failed] Client-{OwnerClientId} denied ownership request because: {ownershipRequestResponse}");
        }
    }
}

I will try to replicate your issue, however I would recommend taking the above example script and see if this works out better for you.

If you find this does resolve your issue, then could you post a quick response to let me know?

[FureurTime]

I know there is NetworkObject.RequestOwnership(), but I already use it for some objects to grab them, and I don't want interference between these components on NetworkObject.Ownership or with the delegate.
If I send the request to the owner with an RPC, it is to avoid race conditions and do checks that are local to the owner.

Using your exemple will surely fix the issue, but create more on my other components.

I temporarily fixed the NetworkTransform issue by adding current == previous to the condition.

[NoelStephensUnity]

@FureurTime
I see. Well there are a few things that could be happening that could be causing this issue depending upon what is invoking this method:

public void CheckCollisionFor(GameObject otherGameObject)
{
    if(!HasAuthority && /*some other condition*/)
        RequestOwnershipRpc(NetworkManager.LocalClientId);
}

If this is based on a collision, then you could be sending multiple RPCs during FixedUpdate.
Here is a project I am about to push to the NGO v2 Examples folder where I include the ability to take ownership of a single spawned object that clients can take turns controlling:
PlayerSpawningAndApplicationControl.zip

You will want to look over the TransferPlayerControlBootStrap scene:

Within the ExtendedNetworkManager properties, you can switch between "Host" and "Live Service" when using a distributed authority network topology.

The SingleSpawnHandler prefab is what spawns the control that lets you spawn the single NetworkObject instance you can then change ownership during runtime/playmode (the UI provides the keys to use).

The prefab spawned has an OwnershipChangeNonAuthorityHandler script that you can press "O" (default KeyCode) to take ownership on the non-authority side using an RPC. Using this I was not able to replicate your issue.

Of course, this is all non-rigidbody based and is not a collision related event... which I will look into putting together another project that uses a Rigidbody and attempts to replicate your issue...

Take a look at the project (just to confirm using an RPC under this scenario) does not replicate your issue.

If you could provide any additional details or perhaps provide a project that replicates the issues it would help me narrow down what the issue is you are experiencing.

[FureurTime]

@NoelStephensUnity
I couldn't reproduce the bug on your project.
I created my own debug project but I couldn't reproduce the bug either.
On my main project I deleted the 'Library' folder to be sure to reset NGO, but the problem was still there.
I manipulated several parameters on the debug project, like the network and physics parameters, but without being able to reproduce the bug.
The component code itself is pretty much the same, with just a few tweaks to make it compile on this project, but without changing its behavior.
The issue is maybe on other component of my object, I will need to investigate it further.

OwnershipCollisionDebug.zip

[NoelStephensUnity]

@FureurTime
Ahhh... those are sneaky issues. Thank you for the replication project. I will dive into it as well and see if I can find where this issue is coming from.
👍

[NoelStephensUnity]

@FureurTime
Not sure if this helps your actual project, but I wasn't sure if you had tried using the trigger approach?
You can also setup all sorts of things using layers and tags to help reduce the "noise" as well as have multiple Rigidbody components on the children of the bodies. I added a child GameObject to the player with a Rigidbody that has no mass and no friction but has a collider and a trigger which extend beyond the normal bounds of the player and only include the "Ownership" layer to be considered.

TransferOnTrigger.zip

Of course, I don't use an RPC to transfer ownership when the trigger is entered...I just take ownership of it... you might need to make that an RPC depending on what you are trying to accomplish.

• I disabled all of the collision script in the above version so you can see it working without any collisions needed.
• I noticed you were using the CharacterController's OnControllerColliderHit which could be part of your primary project's issue,

Just fiddle with the above and perhaps try the approach using a similar trigger approach (i.e. also including the child object on the player that has a Rigidbody and use that as opposed to the CharacterController to handle detecting triggers and see if that resolves your issue?