SyncVar does not sync if it's modified by reference

PeturDarri commented 2 years ago

Describe the bug If you pass a SyncVar field by reference (either with ref or out) and modify the value, the value will change locally but won't be synced to other clients.

[IMPORTANT] How can we reproduce the issue, step by step:

public class Enemy : NetworkBehaviour
{
    [SyncVar]
    public int health = 100;

    [Server]
    private void Update()
    {
        Randomize(out health);
    }

    private void Randomize(out int variable)
    {
        variable = Random.Range(0, 100);
    }
}

Pass a SyncVar field by reference (out or ref).
Assign a new value to the variable on the server.
Observe that clients have not received the new value.

Expected behavior I expect the value of the field to be synced or for there to be an error when I attempt to pass a SyncVar by reference.

Desktop (please complete the following information):

OS: Windows
Build target: Standalone
Unity version: 2020.3
Mirror branch: master

Additional context This is also problematic for ref returns:

public class Enemy : NetworkBehaviour
{
    [SyncVar]
    public int health = 100;

    public ref int HealthProperty => ref health;

    [Server]
    private void Update()
    {
        HealthProperty = Random.Range(0, 100);
    }
}

imerr commented 2 years ago

This is somewhat expected SyncVars are "magic" in the sense that the weaver replaces code after compilation with a property and only detects changes via setting to that property for your example that might look something like this

    public int Networkhealth
    {
        get
        {
            return health;
        }
        [param: In]
        set
        {
            if (!NetworkBehaviour.SyncVarEqual(value, ref health))
            {
                int oldValue = health;
                SetSyncVar(value, ref health, 1uL);
                if (NetworkServer.localClientActive && !GetSyncVarHookGuard(1uL))
                {
                    SetSyncVarHookGuard(1uL, value: true);
                    OnChanged(oldValue, value);
                    SetSyncVarHookGuard(1uL, value: false);
                }
            }
        }
    }

I'm not sure it is possible to support the ref usage in this case

There should probably be an error raised from the weaver if you attempt to ref a syncvar, but I'm not sure how easy that would be to implement

PeturDarri commented 2 years ago

I expected this as well. I actually don't use Mirror myself but I was aware Mirror used IL weaving and noticed SyncVars are just regular fields so I suspected this would be problematic. I expected an error because I've seen people complain about weaver errors in Mirror, but when I didn't see one, I submitted this issue.

I don't think it's necessary to make this work, it should just raise an error. It should be pretty trivial to spot whenever a SyncVar field is being passed by reference. The IL weaver is already looking for ldflda instructions related to SyncVar fields for another purpose.

in and ref readonly are safe in this case though, the IL weaver should permit those.

MrGadget1024 commented 2 years ago

I'd think that'd work but spamming values into it in Update won't sync every change because it's too fast...it sends nothing at all?

PeturDarri commented 2 years ago

I'd think that'd work but spamming values into it in Update won't sync every change because it's too fast...it sends nothing at all?

This is a made up example, kept simple to keep the focus on the true cause of the problem. I've confirmed the issue by decompiling the assembly after Mirror's IL weaver has modified it. It clearly bypasses the generated syncing code:

public class Enemy : NetworkBehaviour
{
    [SyncVar]
    public int Health = 100;

    [Server]
    public void RandomizeHealth()
    {
        Health = Random.Range(0, 100);
    }

    [Server]
    public void RandomizeHealthRef()
    {
        ref var healthRef = ref Health;

        healthRef = Random.Range(0, 100);
    }
}

decompiled becomes:

public class Enemy : NetworkBehaviour
{
        [SyncVar]
    public int Health = 100;

    [Server]
    public void RandomizeHealth()
    {
        if (!NetworkServer.active)
        {
            Debug.LogWarning("[Server] function 'System.Void Enemy::RandomizeHealth()' called when server was not active");
            return;
        }

                // Health was replaced with NetworkHealth, which is correct.
        this.NetworkHealth = Random.Range(0, 100);
    }

    [Server]
    public void RandomizeHealthRef()
    {
        if (!NetworkServer.active)
        {
            Debug.LogWarning("[Server] function 'System.Void Enemy::RandomizeHealthRef()' called when server was not active");
            return;
        }

                // NetworkHealth is nowhere to be seen here, which means GeneratedSyncVarSetter is never called.
        ref int healthRef = ref this.Health;
        healthRef = Random.Range(0, 100);
    }

    private void MirrorProcessed()
    {
    }

    public int NetworkHealth
    {
        get
        {
            return this.Health;
        }
        [param: In]
        set
        {
            base.GeneratedSyncVarSetter<int>(value, ref this.Health, 1UL, null);
        }
    }

    public override bool SerializeSyncVars(NetworkWriter writer, bool forceAll)
    {
        bool result = base.SerializeSyncVars(writer, forceAll);
        if (forceAll)
        {
            writer.WriteInt(this.Health);
            return true;
        }
        writer.WriteULong(base.syncVarDirtyBits);
        if ((base.syncVarDirtyBits & 1UL) != 0UL)
        {
            writer.WriteInt(this.Health);
            result = true;
        }
        return result;
    }

    public override void DeserializeSyncVars(NetworkReader reader, bool initialState)
    {
        base.DeserializeSyncVars(reader, initialState);
        if (initialState)
        {
            base.GeneratedSyncVarDeserialize<int>(ref this.Health, null, reader.ReadInt());
            return;
        }
        long num = (long)reader.ReadULong();
        if ((num & 1L) != 0L)
        {
            base.GeneratedSyncVarDeserialize<int>(ref this.Health, null, reader.ReadInt());
        }
    }
}

Obviously no one is using ref locals like this, but it highlights the problem in a simple way. The same underlying issue applies to ref and out parameters and ref returns.

MrGadget1024 commented 2 years ago

I'll leave this open for @vis2k to look at to determine level of effort to solve.