Skip to content

fix: owner changing ownership causing identical previous and current ids when using a distributed authority network topology #3347

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

Merged
1 change: 1 addition & 0 deletions com.unity.netcode.gameobjects/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ Additional documentation and release notes are available at [Multiplayer Documen

- Fixed issue when using a distributed authority network topology and many clients attempt to connect simultaneously the session owner could max-out the maximum in-flight reliable messages allowed, start dropping packets, and some of the connecting clients would fail to fully synchronize. (#3350)
- Fixed issue when using a distributed authority network topology and scene management was disabled clients would not be able to spawn any new network prefab instances until synchronization was complete. (#3350)
- Fixed issue where an owner that changes ownership, when using a distributed authority network topology, could yield identical previous and current owner identifiers. This could also cause `NetworkTransform` to fail to change ownership which would leave the previous owner still subscribed to network tick events. (#3347)
- Fixed issue where the `MaximumInterpolationTime` could not be modified from within the inspector view or runtime. (#3337)
- Fixed `ChangeOwnership` changing ownership to clients that are not observers. This also happened with automated object distribution. (#3323)
- Fixed issue where `AnticipatedNetworkVariable` previous value returned by `AnticipatedNetworkVariable.OnAuthoritativeValueChanged` is updated correctly on the non-authoritative side. (#3306)
Expand Down
11 changes: 10 additions & 1 deletion com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1185,14 +1185,23 @@ internal void MarkVariablesDirty(bool dirty)
}
}

internal void MarkOwnerReadVariablesDirty()
/// <summary>
/// For owner read permissions, when changing ownership we need to do a full synchronization
/// of all NetworkVariables that are owner read permission based since the owner is the only
/// instance that knows what the most current values are.
/// </summary>
internal void MarkOwnerReadDirtyAndCheckOwnerWriteIsDirty()
{
for (int j = 0; j < NetworkVariableFields.Count; j++)
{
if (NetworkVariableFields[j].ReadPerm == NetworkVariableReadPermission.Owner)
{
NetworkVariableFields[j].SetDirty(true);
}
if (NetworkVariableFields[j].WritePerm == NetworkVariableWritePermission.Owner)
{
NetworkVariableFields[j].OnCheckIsDirtyState();
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,6 @@ internal void NetworkBehaviourUpdate(bool forceSend = false)
foreach (var dirtyobj in m_DirtyNetworkObjects)
{
dirtyobj.PostNetworkVariableWrite(forceSend);
// Once done processing, we set the previous owner id to the current owner id
dirtyobj.PreviousOwnerId = dirtyobj.OwnerClientId;
}
m_DirtyNetworkObjects.Clear();
}
Expand Down
31 changes: 29 additions & 2 deletions com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2567,12 +2567,39 @@ internal void MarkVariablesDirty(bool dirty)
}
}

internal void MarkOwnerReadVariablesDirty()
/// <summary>
/// Used when changing ownership, this will mark any owner read permission base NetworkVariables as dirty
/// and will check if any owner write permission NetworkVariables are dirty (primarily for collections) so
/// the new owner will get a full state update prior to changing ownership.
/// </summary>
/// <remarks>
/// We have to pass in the original owner and previous owner to "reset" back to the current state of this
/// NetworkObject in order to preserve the same ownership change flow. By the time this is invoked, the
/// new and previous owner ids have already been set.
/// </remarks>
/// <param name="originalOwnerId">the owner prior to beginning the change in ownership change.</param>
/// <param name="originalPreviousOwnerId">the previous owner prior to beginning the change in ownership change.</param>
internal void SynchronizeOwnerNetworkVariables(ulong originalOwnerId, ulong originalPreviousOwnerId)
{
var currentOwnerId = OwnerClientId;
OwnerClientId = originalOwnerId;
PreviousOwnerId = originalPreviousOwnerId;
for (int i = 0; i < ChildNetworkBehaviours.Count; i++)
{
ChildNetworkBehaviours[i].MarkOwnerReadVariablesDirty();
ChildNetworkBehaviours[i].MarkOwnerReadDirtyAndCheckOwnerWriteIsDirty();
}

// Now set the new owner and previous owner identifiers back to their original new values
// before we run the NetworkBehaviourUpdate. For owner read only permissions this order of
// operations is **particularly important** as we need to first (above) mark things as dirty
// from the context of the original owner and then second (below) we need to send the messages
// which requires the new owner to be set for owner read permission NetworkVariables.
OwnerClientId = currentOwnerId;
PreviousOwnerId = originalOwnerId;

// Force send a state update for all owner read NetworkVariables and any currently dirty
// owner write NetworkVariables.
NetworkManager.BehaviourUpdater.NetworkBehaviourUpdate(true);
}

// NGO currently guarantees that the client will receive spawn data for all objects in one network tick.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -381,11 +381,8 @@ private void HandleOwnershipChange(ref NetworkContext context)

if (originalOwner == networkManager.LocalClientId && !networkManager.DistributedAuthorityMode)
{
// Mark any owner read variables as dirty
networkObject.MarkOwnerReadVariablesDirty();
// Immediately queue any pending deltas and order the message before the
// change in ownership message.
networkManager.BehaviourUpdater.NetworkBehaviourUpdate(true);
// Fully synchronize NetworkVariables with either read or write ownership permissions.
networkObject.SynchronizeOwnerNetworkVariables(originalOwner, networkObject.PreviousOwnerId);
}

// Always invoke ownership change notifications
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,13 @@ public bool CheckDirtyState(bool forceCheck = false)
return isDirty;
}

/// <inheritdoc/>
internal override void OnCheckIsDirtyState()
{
CheckDirtyState();
base.OnCheckIsDirtyState();
}

internal ref T RefValue()
{
return ref m_InternalValue;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,15 @@ internal ulong OwnerClientId()
return m_NetworkBehaviour.NetworkObject.OwnerClientId;
}

/// <summary>
/// Primarily to check for collections dirty states when doing
/// a fully owner read/write NetworkVariable update.
/// </summary>
internal virtual void OnCheckIsDirtyState()
{

}

/// <summary>
/// Writes the dirty changes, that is, the changes since the variable was last dirty, to the writer
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,15 @@ internal void RemoveOwnership(NetworkObject networkObject)

internal void ChangeOwnership(NetworkObject networkObject, ulong clientId, bool isAuthorized, bool isRequestApproval = false)
{
if (clientId == networkObject.OwnerClientId)
{
if (NetworkManager.LogLevel <= LogLevel.Developer)
{
Debug.LogWarning($"[{nameof(NetworkSpawnManager)}][{nameof(ChangeOwnership)}] Attempting to change ownership to Client-{clientId} when the owner is already {networkObject.OwnerClientId}! (Ignoring)");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is already happening on line 524 of this function, though the check is a little different:

if (networkObject.OwnerClientId == clientId && networkObject.PreviousOwnerId == clientId)

We should probably consolidate the two checks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue was that there are several places that invoke NetworkSpawnManager.ChangeOwnership:
image
I decided to put a check at the funnel point. However, at some point in the future we might look at all of the invocations to determine if it makes sense to consolidate the various checks for this condition... for now we know that we will at least see warnings...maybe I should make that an error so it will fail any PRs that introduce this condition?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the bigger thing is that the check is happening twice in the same function. So the same code path is going to check the same check twice.

}
return;
}

// For client-server:
// If ownership changes faster than the latency between the client-server and there are NetworkVariables being updated during ownership changes,
// then notify the user they could potentially lose state updates if developer logging is enabled.
Expand Down Expand Up @@ -530,6 +539,10 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId, bool
return;
}

// Save the previous owner to work around a bit of a nasty issue with assuring NetworkVariables are serialized when ownership changes
var originalPreviousOwnerId = networkObject.PreviousOwnerId;
var originalOwner = networkObject.OwnerClientId;

// Used to distinguish whether a new owner should receive any currently dirty NetworkVariable updates
networkObject.PreviousOwnerId = networkObject.OwnerClientId;

Expand All @@ -545,13 +558,10 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId, bool
// Always notify locally on the server when a new owner is assigned
networkObject.InvokeBehaviourOnGainedOwnership();

if (networkObject.PreviousOwnerId == NetworkManager.LocalClientId)
// If we are the original owner, then we want to synchronize owner read & write NetworkVariables.
if (originalOwner == NetworkManager.LocalClientId)
{
// Mark any owner read variables as dirty
networkObject.MarkOwnerReadVariablesDirty();
// Immediately queue any pending deltas and order the message before the
// change in ownership message.
NetworkManager.BehaviourUpdater.NetworkBehaviourUpdate(true);
networkObject.SynchronizeOwnerNetworkVariables(originalOwner, originalPreviousOwnerId);
}

var size = 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
using System.Collections.Generic;
using System.Linq;
using NUnit.Framework;
using Unity.Netcode.Components;
using Unity.Netcode.TestHelpers.Runtime;
using UnityEngine;
using UnityEngine.TestTools;
Expand All @@ -25,6 +26,12 @@ public override void OnGainedOwnership()
OnGainedOwnershipFired = true;
}

protected override void OnOwnershipChanged(ulong previous, ulong current)
{
Assert.True(previous != current, $"[{nameof(OnOwnershipChanged)}][Invalid Parameters] Invoked and the previous ({previous}) equals the current ({current})!");
base.OnOwnershipChanged(previous, current);
}

public override void OnNetworkSpawn()
{
if (!SpawnedInstances.ContainsKey(NetworkManager.LocalClientId))
Expand All @@ -39,6 +46,12 @@ public void ResetFlags()
OnLostOwnershipFired = false;
OnGainedOwnershipFired = false;
}

[Rpc(SendTo.Authority)]
public void ChangeOwnershipRpc(RpcParams rpcParams = default)
{
NetworkObject.ChangeOwnership(rpcParams.Receive.SenderClientId);
}
}


Expand Down Expand Up @@ -71,6 +84,7 @@ protected override void OnServerAndClientsCreated()
{
m_OwnershipPrefab = CreateNetworkObjectPrefab("OnwershipPrefab");
m_OwnershipPrefab.AddComponent<NetworkObjectOwnershipComponent>();
m_OwnershipPrefab.AddComponent<NetworkTransform>();
if (m_DistributedAuthority)
{
m_OwnershipPrefab.GetComponent<NetworkObject>().SetOwnershipStatus(NetworkObject.OwnershipStatus.Transferable);
Expand Down Expand Up @@ -465,7 +479,87 @@ public IEnumerator TestOwnedObjectCounts()

yield return WaitForConditionOrTimeOut(ServerHasCorrectClientOwnedObjectCount);
AssertOnTimeout($"Server does not have the correct count for all clients spawned {k_NumberOfSpawnedObjects} {nameof(NetworkObject)}s!");
}

/// <summary>
/// Validates that when changing ownership NetworkTransform does not enter into a bad state
/// because the previous and current owner identifiers are the same. For client-server this
/// ends up always being the server, but for distributed authority the authority changes when
/// ownership changes.
/// </summary>
[UnityTest]
public IEnumerator TestAuthorityChangingOwnership()
{
var authorityManager = (NetworkManager)null;
var allNetworkManagers = m_ClientNetworkManagers.ToList();
allNetworkManagers.Add(m_ServerNetworkManager);

if (m_DistributedAuthority)
{
var authorityId = Random.Range(1, NumberOfClients) - 1;
authorityManager = m_ClientNetworkManagers[authorityId];
m_OwnershipObject = SpawnObject(m_OwnershipPrefab, authorityManager);
m_OwnershipNetworkObject = m_OwnershipObject.GetComponent<NetworkObject>();
}
else
{
authorityManager = m_ServerNetworkManager;
m_OwnershipObject = SpawnObject(m_OwnershipPrefab, m_ServerNetworkManager);
m_OwnershipNetworkObject = m_OwnershipObject.GetComponent<NetworkObject>();
}
var ownershipNetworkObjectId = m_OwnershipNetworkObject.NetworkObjectId;
bool WaitForClientsToSpawnNetworkObject()
{
foreach (var clientNetworkManager in m_ClientNetworkManagers)
{
if (!clientNetworkManager.SpawnManager.SpawnedObjects.ContainsKey(ownershipNetworkObjectId))
{
return false;
}
}
return true;
}

yield return WaitForConditionOrTimeOut(WaitForClientsToSpawnNetworkObject);
AssertOnTimeout($"Timed out waiting for all clients to spawn the {m_OwnershipNetworkObject.name} {nameof(NetworkObject)} instance!");

var currentTargetOwner = (ulong)0;
bool WaitForAllInstancesToChangeOwnership()
{
foreach (var clientNetworkManager in m_ClientNetworkManagers)
{
if (!clientNetworkManager.SpawnManager.SpawnedObjects.ContainsKey(ownershipNetworkObjectId))
{
return false;
}
if (clientNetworkManager.SpawnManager.SpawnedObjects[ownershipNetworkObjectId].OwnerClientId != currentTargetOwner)
{
return false;
}
}
return true;
}

// Change ownership a few times and as long as the previous and current owners are not the same when
// OnOwnershipChanged is invoked then the test passed.
foreach (var networkManager in allNetworkManagers)
{
if (networkManager == authorityManager)
{
continue;
}
var clonedObject = networkManager.SpawnManager.SpawnedObjects[ownershipNetworkObjectId];

if (clonedObject.OwnerClientId == networkManager.LocalClientId)
{
continue;
}

var testComponent = clonedObject.GetComponent<NetworkObjectOwnershipComponent>();
testComponent.ChangeOwnershipRpc();
yield return WaitForAllInstancesToChangeOwnership();
AssertOnTimeout($"Timed out waiting for all instances to change ownership to Client-{networkManager.LocalClientId}!");
}
}
}
}