FishNet icon indicating copy to clipboard operation
FishNet copied to clipboard

Unconsumed GoalDataQueue in NetworkTransform causes a quick jerk when client loses Ownership

Open TsFreddie opened this issue 2 years ago • 3 comments

General Unity version: 2022.3.8f1 Fish-Networking version: 3.11.6R

Description If switching ownership from Client A to Client B while Client A is actively sending transform data, when Client B loses ownership, the object will momentarily goes back to the old position from Client A before update to the correct location.

Replication Here's a simple test script for reproducing

using FishNet.Component.Ownership;
using FishNet.Component.Transforming;
using FishNet.Connection;
using FishNet.Object;
using UnityEngine;

public class TestBehaviour : NetworkBehaviour
{
    private Vector3 _targetPosition;

    void Update()
    {
        if (Input.GetMouseButton(0))
        {
            if (!IsOwner) RequestOwnership();
            if (Camera.main != null)
                _targetPosition = Camera.main.ScreenToWorldPoint(Input.mousePosition);
            _targetPosition.z = 0;
        }

        if (Input.GetMouseButtonDown(1))
        {
            if (IsOwner) GiveUpOwnership();
        }

        if (IsOwner)
        {
            transform.position = _targetPosition;
            GetComponent<NetworkTransform>().ForceSend();
        }
    }

    [ServerRpc(RequireOwnership = false)]
    public void RequestOwnership(NetworkConnection sender = null)
    {
        GiveOwnership(sender);
    }

    [ServerRpc]
    public void GiveUpOwnership()
    {
        RemoveOwnership();
    }
}

Steps to reproduce the behavior:

  1. Build the project and launch three instances. make one of them the server and the other two clients
  2. Left click anywhere on Client A to grab ownership and update the object's position
  3. Left click anywhere else on Client B to grab ownership from Client A and update the object's position
  4. Right click on Client B to release ownership and observe on Client B

Other Notes

ForceSend was called constantly to simulate constant movement without actually having to control two client at the same time.

Adding a _goalDataQueue.Clear(); at the end of NetworkTransform's SetDefaultGoalData() solved the problem for me, but I'm not sure if keeping the goalDataQueue is intended.

TsFreddie avatar Oct 31 '23 10:10 TsFreddie

Here's a video for more context:

https://github.com/FirstGearGames/FishNet/assets/3797859/d2ae2ea1-7b14-4396-a7e4-7a183c49ce38

TsFreddie avatar Nov 03 '23 03:11 TsFreddie

The queue most likely is not needed for owner but is for spectators. Your solution should be okay, I just need to find if that is the best place to put it.

We do not clear goal datas on ownership transfer for spectators because we still want the object to move to the positions of the previous owner before moving to the positions of the new owner. But as a new owner it would make sense to clear since you are now the authoritative client.

Can you please clarify if you are seeing the jump on owner, or on spectators?

FirstGearGames avatar Nov 14 '23 18:11 FirstGearGames

The queue most likely is not needed for owner but is for spectators. Your solution should be okay, I just need to find if that is the best place to put it.

We do not clear goal datas on ownership transfer for spectators because we still want the object to move to the positions of the previous owner before moving to the positions of the new owner. But as a new owner it would make sense to clear since you are now the authoritative client.

Can you please clarify if you are seeing the jump on owner, or on spectators?

Just on owner, well technically on spectator who just lost the ownership.

For additional info, when ownership is gained, NT skips consuming goaldata which may not be empty if previous owner or server has been sending right before the owner transfer.

When the owner lose ownership, NT resumes consuming goaldata which still contains very outdated positions which causes the jump.

TsFreddie avatar Nov 14 '23 23:11 TsFreddie

I made this change... added

        /// <summary>
        /// Tries to clear the GoalDatas queue during an ownership change.
        /// </summary>
        private void TryClearGoalDatas_OwnershipChange(NetworkConnection prevOwner, bool asServer)
        {
            if (_clientAuthoritative)
            {
                //If not server
                if (!asServer)
                {
                    //If owner now then clear as the owner controls the object now and shouldnt use past datas.
                    if (base.IsOwner)
                        _goalDataQueue.Clear();
                }
                //as Server.
                else
                {
                    //If new owner is valid then clear to allow new owner datas.
                    if (base.Owner.IsValid)
                        _goalDataQueue.Clear();
                }
            }
            /* Server authoritative never clears because the
             * clients do not control this object thus should always
             * follow the queue. */
        }

And now call it from server/client OnOwnershipChange passing in true/false as appropriate.

TryClearGoalDatas_OwnershipChange(prevOwner, false);

FirstGearGames avatar Apr 04 '24 00:04 FirstGearGames

Resolved in 4.2.0

FirstGearGames avatar Apr 04 '24 00:04 FirstGearGames