Skip to content

fix: ClientNetworkTransform ownership change with half precision synchronization issues [MTT-7271] #2696

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

Closed
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,20 @@ public struct NetworkDeltaPosition : INetworkSerializable
internal Vector3 DeltaPosition;
internal int NetworkTick;

internal bool SynchronizeBase;

internal bool CollapsedDeltaIntoBase;

/// <summary>
/// The serialization implementation of <see cref="INetworkSerializable"/>
/// </summary>
public void NetworkSerialize<T>(BufferSerializer<T> serializer) where T : IReaderWriter
{
HalfVector3.NetworkSerialize(serializer);
if (SynchronizeBase)
{
serializer.SerializeValue(ref CurrentBasePosition);
}
}

/// <summary>
Expand Down Expand Up @@ -122,6 +130,7 @@ public Vector3 GetDeltaPosition()
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public void UpdateFrom(ref Vector3 vector3, int networkTick)
{
CollapsedDeltaIntoBase = false;
NetworkTick = networkTick;
DeltaPosition = (vector3 + PrecisionLossDelta) - CurrentBasePosition;
for (int i = 0; i < HalfVector3.Length; i++)
Expand All @@ -136,6 +145,7 @@ public void UpdateFrom(ref Vector3 vector3, int networkTick)
CurrentBasePosition[i] += HalfDeltaConvertedBack[i];
HalfDeltaConvertedBack[i] = 0.0f;
DeltaPosition[i] = 0.0f;
CollapsedDeltaIntoBase = true;
}
}
}
Expand Down Expand Up @@ -164,6 +174,8 @@ public NetworkDeltaPosition(Vector3 vector3, int networkTick, bool3 axisToSynchr
DeltaPosition = Vector3.zero;
HalfDeltaConvertedBack = Vector3.zero;
HalfVector3 = new HalfVector3(vector3, axisToSynchronize);
SynchronizeBase = false;
CollapsedDeltaIntoBase = false;
UpdateFrom(ref vector3, networkTick);
}

Expand Down
468 changes: 390 additions & 78 deletions com.unity.netcode.gameobjects/Components/NetworkTransform.cs

Large diffs are not rendered by default.

17 changes: 17 additions & 0 deletions com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs
Original file line number Diff line number Diff line change
Expand Up @@ -532,6 +532,23 @@ internal void InternalOnGainedOwnership()
OnGainedOwnership();
}

/// <summary>
/// Invoked on all clients, override this method to be notified of any
/// ownership changes (even if the instance was niether the previous or
/// newly assigned current owner).
/// </summary>
/// <param name="previous">the previous owner</param>
/// <param name="current">the current owner</param>
protected virtual void OnOwnershipChanged(ulong previous, ulong current)
{

}

internal void InternalOnOwnershipChanged(ulong previous, ulong current)
{
OnOwnershipChanged(previous, current);
}

/// <summary>
/// Gets called when we loose ownership of this object
/// </summary>
Expand Down
15 changes: 15 additions & 0 deletions com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs
Original file line number Diff line number Diff line change
Expand Up @@ -753,6 +753,21 @@ internal void InvokeBehaviourOnGainedOwnership()
}
}

internal void InvokeOwnershipChanged(ulong previous, ulong next)
{
for (int i = 0; i < ChildNetworkBehaviours.Count; i++)
{
if (ChildNetworkBehaviours[i].gameObject.activeInHierarchy)
{
ChildNetworkBehaviours[i].InternalOnOwnershipChanged(previous, next);
}
else
{
Debug.LogWarning($"{ChildNetworkBehaviours[i].gameObject.name} is disabled! Netcode for GameObjects does not support disabled NetworkBehaviours! The {ChildNetworkBehaviours[i].GetType().Name} component was skipped during ownership assignment!");
}
}
}

internal void InvokeBehaviourOnNetworkObjectParentChanged(NetworkObject parentNetworkObject)
{
for (int i = 0; i < ChildNetworkBehaviours.Count; i++)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ public void Handle(ref NetworkContext context)
}
}

networkObject.InvokeOwnershipChanged(originalOwner, OwnerClientId);

networkManager.NetworkMetrics.TrackOwnershipChangeReceived(context.SenderId, networkObject, context.MessageSize);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId)
throw new SpawnStateException("Object is not spawned");
}

var previous = networkObject.OwnerClientId;
// Assign the new owner
networkObject.OwnerClientId = clientId;

Expand Down Expand Up @@ -286,6 +287,12 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId)
NetworkManager.NetworkMetrics.TrackOwnershipChangeSent(client.Key, networkObject, size);
}
}

// After we have sent the change ownership message to all client observers, invoke the ownership changed notification.
/// !!Important!!
/// This gets called specifically *after* sending the ownership message so any additional messages that need to proceed an ownership
/// change can be sent from NetworkBehaviours that override the <see cref="NetworkBehaviour.OnOwnershipChanged"></see>
networkObject.InvokeOwnershipChanged(previous, clientId);
}

internal bool HasPrefab(NetworkObject.SceneObject sceneObject)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,11 @@ public void TestSyncAxes([Values] SynchronizationType synchronizationType, [Valu
var gameObject = new GameObject($"Test-{nameof(NetworkTransformStateTests)}.{nameof(TestSyncAxes)}");
var networkObject = gameObject.AddComponent<NetworkObject>();
var networkTransform = gameObject.AddComponent<NetworkTransform>();

var manager = new GameObject($"Test-{nameof(NetworkManager)}.{nameof(TestSyncAxes)}");
var networkManager = manager.AddComponent<NetworkManager>();
networkObject.NetworkManagerOwner = networkManager;

networkTransform.enabled = false; // do not tick `FixedUpdate()` or `Update()`

var initialPosition = Vector3.zero;
Expand Down Expand Up @@ -269,6 +274,7 @@ public void TestSyncAxes([Values] SynchronizationType synchronizationType, [Valu

if (syncPosX || syncPosY || syncPosZ || syncRotX || syncRotY || syncRotZ || syncScaX || syncScaY || syncScaZ)
{
Assert.IsTrue(networkTransform.NetworkManager != null, "NetworkManager is NULL!");
Assert.IsTrue(networkTransform.ApplyTransformToNetworkState(ref networkTransformState, 0, networkTransform.transform));
}
}
Expand Down Expand Up @@ -714,6 +720,7 @@ public void TestSyncAxes([Values] SynchronizationType synchronizationType, [Valu
}

Object.DestroyImmediate(gameObject);
Object.DestroyImmediate(manager);
}


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,7 @@ private void AllChildrenLocalTransformValuesMatch(bool useSubChild)
var childLocalRotation = childInstance.transform.localRotation.eulerAngles;
var childLocalScale = childInstance.transform.localScale;
// Adjust approximation based on precision
if (m_Precision == Precision.Half)
if (m_Precision == Precision.Half || m_RotationCompression == RotationCompression.QuaternionCompress)
{
m_CurrentHalfPrecision = k_HalfPrecisionPosScale;
}
Expand All @@ -486,7 +486,7 @@ private void AllChildrenLocalTransformValuesMatch(bool useSubChild)
}

// Adjust approximation based on precision
if (m_Precision == Precision.Half)
if (m_Precision == Precision.Half || m_RotationCompression == RotationCompression.QuaternionCompress)
{
m_CurrentHalfPrecision = k_HalfPrecisionRot;
}
Expand Down Expand Up @@ -523,6 +523,7 @@ public void ParentedNetworkTransformTest([Values] Precision precision, [Values]
{
// Set the precision being used for threshold adjustments
m_Precision = precision;
m_RotationCompression = rotationCompression;

// Get the NetworkManager that will have authority in order to spawn with the correct authority
var isServerAuthority = m_Authority == Authority.ServerAuthority;
Expand Down Expand Up @@ -577,6 +578,9 @@ public void ParentedNetworkTransformTest([Values] Precision precision, [Values]

// Allow one tick for authority to update these changes
TimeTravelToNextTick();
success = WaitForConditionOrTimeOutWithTimeTravel(PositionRotationScaleMatches);

Assert.True(success, "All transform values did not match prior to parenting!");

// Parent the child under the parent with the current world position stays setting
Assert.True(serverSideChild.TrySetParent(serverSideParent.transform, worldPositionStays), "[Server-Side Child] Failed to set child's parent!");
Expand All @@ -588,6 +592,7 @@ public void ParentedNetworkTransformTest([Values] Precision precision, [Values]
success = WaitForConditionOrTimeOutWithTimeTravel(AllChildObjectInstancesHaveChild);
Assert.True(success, "Timed out waiting for all instances to have parented a child!");

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Both minor tweaks to this test will help prevent random edge case precision validation errors.

TimeTravelToNextTick();
// This validates each child instance has preserved their local space values
AllChildrenLocalTransformValuesMatch(false);

Expand Down Expand Up @@ -688,8 +693,9 @@ protected override void OnNewClientCreated(NetworkManager networkManager)
}

private Precision m_Precision = Precision.Full;
private RotationCompression m_RotationCompression = RotationCompression.None;
private float m_CurrentHalfPrecision = 0.0f;
private const float k_HalfPrecisionPosScale = 0.03f;
private const float k_HalfPrecisionPosScale = 0.041f;
private const float k_HalfPrecisionRot = 0.725f;

protected override float GetDeltaVarianceThreshold()
Expand Down