FirstGearGames / FishNet

FishNet: Unity Networking Evolved.
Other
1.39k stars 150 forks source link

The value of _valueSetAfterInitialized in SyncVar.cs is always false. #683

Closed KeithSwanger closed 5 months ago

KeithSwanger commented 6 months ago

General Fish-Networking version: 4.3.3R

Description In SyncVar.cs, _valueSetAfterInitialized will never get a chance to be set to true

This manifested itself in my project as late-joining clients never getting SyncVars of previously spawned objects synced when they joined.

Replication Take a look at these two methods in SyncVar.cs

public void SetInitialValues(T value)
{
    _initialValue = value;
    UpdateValues(value);

    if (base.IsInitialized)
        _valueSetAfterInitialized = true;
}
internal void SetValue(T nextValue, bool calledByUser, bool sendRpc = false)
{
...
    if (!base.IsInitialized)
    {
        SetInitialValues(nextValue);
        return;
    }
...
}

Looking at these two methods, you can see that _valueSetAfterInitialized will never get a chance to be set to true, as SetInitialValues is only ever called when base.IsInitialized is false.

This means that SyncVar.WriteFull() will never sync SyncVars of non-value types.

Potential Fix Perhaps move the assignment into the SetValue method instead:

public void SetInitialValues(T value)
{
    _initialValue = value;
    UpdateValues(value);
}
internal void SetValue(T nextValue, bool calledByUser, bool sendRpc = false)
{
...
    if (!base.IsInitialized)
    {
        SetInitialValues(nextValue);
        return;
    }
    else
    {
    _valueSetAfterInitialized = true;
    }
...
}
ooonush commented 6 months ago

I also got a sync error after this change. When the server changes the value before the object is spawned, clients do not receive this change.

ooonush commented 6 months ago

If the server changes the initial value before initialization, WriteFull does return on line 351. Isn't there some way to write Unit Tests so that bugs like this don't occur? It's just not the first time I've had a game crash in my project after a FishNet update :(

image

SnaiperoG3D commented 6 months ago

God damn, i have same issue i guess, i just updated to 4.3.3 before release my last game version, and got late joiners bug

SnaiperoG3D commented 6 months ago

Confirm, comment if state lines on 350 fix the issue with late joiners

FirstGearGames commented 6 months ago

If the server changes the initial value before initialization, WriteFull does return on line 351. Isn't there some way to write Unit Tests so that bugs like this don't occur? It's just not the first time I've had a game crash in my project after a FishNet update :(

It's near impossible to provide unit tests for scenarios we don't know could cause problems. When we made the changes they were tested and all tests passed. If you can let me know the condition which causes this as code, I can include it in the existing tests.

FirstGearGames commented 6 months ago

I'm going to get this resolved and released by tomorrow. I'm going to also open up bounties for help on creating tests for situations which cannot simply be done through normal unit tests.

FirstGearGames commented 6 months ago

Thank you @KeithSwanger for finding this issue as well the solution. Setting to true inside SetValue should work without issue. This is resolved in 4.3.4.

PS: it should also be set as shown inside SetInitialValues as it was in the release, but as well in SetValue.