FirstGearGames / FishNet

FishNet: Unity Networking Evolved.
Other
1.38k stars 149 forks source link

LastPacketTick is never reset between matches resulting in incorrect client tick #748

Closed JadenH closed 3 months ago

JadenH commented 3 months ago

General Unity version: 2022.3.27f1 Fish-Networking version: 4.3.8 Discord link: https://discord.com/channels/424284635074134018/1270893274621546528

Description Clients have an invalid Tick value after their first match. This looks to be due to the LastPacketTick.Reset never being called on the client after disconnecting from the first match.

The LastPacketTick is used in the ParseTimingUpdate to calculate the nextTick and difference:

/* Use the last ordered remote tick rather than
 * lastPacketTick. This will help with out of order
 * packets where the timing update sent before
 * the remote tick but arrived after. By using ordered
 * remote tick we are comparing against however many
 * ticks really passed rather than the difference
 * between the out of order/late packet. */
uint lastPacketTick = LastPacketTick.RemoteTick;
//Set Tick based on difference between localTick and clientTick, added onto lastPacketTick.
uint prevTick = Tick;
uint nextTick = (LocalTick - clientTick) + lastPacketTick;
long difference = ((long)nextTick - (long)prevTick);
Tick = nextTick;

ClientManager updates the LastPacketTick in ClientManager.cs:

tm.LastPacketTick.Update(reader.ReadTickUnpacked(), EstimatedTick.OldTickOption.Discard, false);

Which makes its way to Update in EstimatedTick.cs:

public bool Update(TimeManager tm, uint remoteTick, OldTickOption oldTickOption = OldTickOption.Discard, bool resetValue = true)
{
    _updateTimeManager = tm;
    //Always set LastRemoteTick even if out of order.
    LastRemoteTick = remoteTick;
    //If cannot update with old values return.
    if (oldTickOption != OldTickOption.SetRemoteTick && remoteTick <= RemoteTick)
        return false;

    //nm is assumed set here.
    LocalTick = tm.LocalTick;
    if (resetValue)
        _valueLocalTick = LocalTick;
    RemoteTick = remoteTick;

    return true;
}

Since RemoteTick is set to a value from the previous match, it's greater than the new remoteTick received in the new match. oldTickOption is set to EstimatedTick.OldTickOption.Discard, causing the method to return early and not update the LocalTick or RemoteTick.

There are no references to LastPacketTick.Reset in FishNet. EstimatedTick.cs contains a Reset method that should likely be called upon client disconnect, the same way PacketTick and LocalTick are reset at NetworkConnection.ResetState:449.

Replication Steps to reproduce the behavior:

  1. Start a host client
  2. Start a client and connect to the host
  3. Wait some time (to allow the tick to increment to a large number)
  4. Close the host (don't stop or shutdown the executable)
  5. Observe that the client is disconnected (don't stop or shutdown the executable)
  6. Start the host client once again
  7. Connect the client to the host again
  8. Observe that the client Tick value continues from the previous connection and is now ahead of the server (host) tick.

Note: Issue resolves once the server tick surpasses the incorrect client tick.

Expected behavior Client tick should be approximately equal to the server tick.

Screenshots The screenshot shows some debug UI at the top right-hand side of the screen, showing the value of TimeManager.Tick, and the values within LastPacketTick. image

JadenH commented 3 months ago

This might not be the best long-term fix, but I resolved this issue by resetting and initializing LastPacketTick when the server/client starts and stops in TimeManager.cs:

/// <summary>
/// Called after the local client connection state changes.
/// </summary>
private void ClientManager_OnClientConnectionState(ClientConnectionStateArgs obj)
{
    if (obj.ConnectionState != LocalConnectionState.Started)
    {
        _pingStopwatch.Stop();
        ClientUptime = 0f;

        //Only reset ticks if also not server.
        if (!NetworkManager.IsServerStarted)
        {
            LocalTick = 0;
            Tick = 0;
            SetTickRate(TickRate);
        }
    }
    //Started.
    else
    {
        _pingStopwatch.Restart();
    }

    if (obj.ConnectionState == LocalConnectionState.Started)
    {
        LastPacketTick.Initialize(this);
    }

    if (obj.ConnectionState == LocalConnectionState.Stopped)
    {
        LastPacketTick.Reset();
    }
}

/// <summary>
/// Called after the local server connection state changes.
/// </summary>
private void ServerManager_OnServerConnectionState(ServerConnectionStateArgs obj)
{
    //If no servers are running.
    if (!NetworkManager.ServerManager.AnyServerStarted())
    {
        ServerUptime = 0f;
        Tick = 0;
    }

    if (obj.ConnectionState == LocalConnectionState.Started)
    {
        LastPacketTick.Initialize(this);
    }

    if (obj.ConnectionState == LocalConnectionState.Stopped)
    {
        LastPacketTick.Reset();
    }
}
JadenH commented 3 months ago

I tried updating to 4.4.1 and this issue is still present.

FirstGearGames commented 3 months ago

Yes, if we do not mark something as resolved we've not yet implemented it. Your fix is very likely the appropriate route to take though. I'll check it out and get the fix in.

FirstGearGames commented 3 months ago

Checked out the code, you will want to make sure LPT does not reset if the client or server is connected. The solution is to reset if server state is not started, or if client state is not started and server isnt started.

        private void ClientManager_OnClientConnectionState(ClientConnectionStateArgs obj)
        {
            if (obj.ConnectionState != LocalConnectionState.Started)
            {
                if (!NetworkManager.IsServerStarted)
                    LastPacketTick.Reset();