Mirror doesn't consistently use custom reader/writerFunc for NetworkBehaviour-inheriting types

[lumeriith]

Describe the bug SyncVars of type deriving from NetworkBehaviours point to wrong NetworkBehaviour on clients, while RPCs work fine.

How to reproduce the issue, step by step

1. Setup a basic scene with Network Manager with empty GameObject with NetworkIdentity attached on it as player object. (NetworkManagerHUD and everything)
2. Let Tester.cs have following code:

public class Tester : NetworkBehaviour
{
    [SyncVar] public Tester syncvar; // If the type is NetworkBehaviour instead of Tester, this works fine.
    public Tester rpc; // RPCs work fine as far as I have tested.

    private void Update()
    {
        if (!NetworkServer.active) return;
        if (Input.GetKeyDown(KeyCode.Space))
        {
            var testers = FindObjectsOfType<Tester>();
            var target = testers[Random.Range(0, testers.Length)];
            syncvar = target;
            RpcSetTester(target);
        }
    }

    [ClientRpc]
    private void RpcSetTester(Tester tester)
    {
        rpc = tester;
    }
}

1. Place 3 GameObjects with Tester attached on the scene.
2. Run the project twice, run one as a host, one as a client.
3. Pressing Space on host changes every Tester's syncvar and rpc.
4. Client's syncvar and rpc should point to a same Tester that the server has set, but syncvar points to wrong Tester, while rpc one works fine.

Repro Project Don't mind the project name, I thought this was caused by something else before. CustomReaderWriterRepro.zip

Expected behavior The default reader/writers Mirror implements supports serializing/deserializing NetworkBehaviours. Hence both variables points to the same, correct Tester instance.

Screenshots Left: Host, Right: Client

Desktop:

• OS: Windows 10
• Build target: Standalone
• Unity version: 2019.2.21f
• Mirror branch: v35.1.0 (latest as of now)

Additional context Writing a custom reader/writer extension causes the Deserialization of Tester to fail, specifically SyncVar syncvar. When syncing SyncVars, server uses the custom writer correctly, but the client uses the default one implemented for NetworkBehaviour. This leads to EndOfStream error or a warning from Mirror telling you that it just read wrong number of bytes, and of course wrong data is synced.

public static class CustomReaderWriterExtensions
{
    public static void WriteTester(this NetworkWriter writer, Tester tester)
    {
        Debug.Log("Using custom Writer function");
        if (tester == null) writer.WriteUInt32(uint.MaxValue);
        else writer.WriteUInt32(tester.netId);
    }

    public static Tester ReadTester(this NetworkReader reader)
    {
        Debug.Log("Using custom Reader function");
        uint netId = reader.ReadUInt32();
        return netId == uint.MaxValue ? null : NetworkIdentity.spawned[netId].GetComponent<Tester>();
    }
}

Left: Host, Right: Client

1. After the client is connected to the host
2. Pressing Space on host results in this (SyncVar fails, RPC one is synced just fine)

OnDeserialize failed Exception=System.IO.EndOfStreamException (see below) object=Tester 0 component=Tester sceneId=7A7BE716D0A73C77 length=20. Possible Reasons:
  * Do Tester's OnSerialize and OnDeserialize calls write the same amount of data(20 bytes)? 
  * Was there an exception in Tester's OnSerialize/OnDeserialize code?
  * Are the server and client the exact same project?
  * Maybe this OnDeserialize call was meant for another GameObject? The sceneIds can easily get out of sync if the Hierarchy was modified only in the client OR the server. Try rebuilding both.

Exception System.IO.EndOfStreamException: ReadByte out of range:NetworkReader pos=25 len=25 buffer=00-14-00-00-00-00-00-00-00-00-00-00-00-01-00-00-00-00-00-00-00-02-00-00-00
  at Mirror.NetworkReader.ReadByte () [0x0001b] in D:\GitHub\CustomReaderWriterReproClone\Assets\Mirror\Runtime\NetworkReader.cs:50 
  at Mirror.NetworkReaderExtensions.ReadNetworkBehaviourSyncVar (Mirror.NetworkReader reader) [0x00013] in D:\GitHub\CustomReaderWriterReproClone\Assets\Mirror\Runtime\NetworkReader.cs:319 
  at Tester.DeserializeSyncVars (Mirror.NetworkReader reader, System.Boolean initialState) [0x00057] in <e1209480b5c4434d96b4f33e16b6da8f>:0 
  at Mirror.NetworkBehaviour.OnDeserialize (Mirror.NetworkReader reader, System.Boolean initialState) [0x0001c] in D:\GitHub\CustomReaderWriterReproClone\Assets\Mirror\Runtime\NetworkBehaviour.cs:584 
  at Mirror.NetworkIdentity.OnDeserializeSafely (Mirror.NetworkBehaviour comp, Mirror.NetworkReader reader, System.Boolean initialState) [0x00019] in D:\GitHub\CustomReaderWriterReproClone\Assets\Mirror\Runtime\NetworkIdentity.cs:915 
UnityEngine.Debug:LogError(Object)
Mirror.NetworkIdentity:OnDeserializeSafely(NetworkBehaviour, NetworkReader, Boolean) (at Assets/Mirror/Runtime/NetworkIdentity.cs:920)
Mirror.NetworkIdentity:OnDeserializeAllSafely(NetworkReader, Boolean) (at Assets/Mirror/Runtime/NetworkIdentity.cs:952)
Mirror.NetworkClient:OnUpdateVarsMessage(UpdateVarsMessage) (at Assets/Mirror/Runtime/NetworkClient.cs:1122)
Mirror.<>c__DisplayClass44_0`1:<RegisterHandler>b__0(NetworkConnection, UpdateVarsMessage) (at Assets/Mirror/Runtime/NetworkClient.cs:338)
Mirror.<>c__DisplayClass5_0`2:<WrapHandler>b__0(NetworkConnection, NetworkReader, Int32) (at Assets/Mirror/Runtime/MessagePacking.cs:116)
Mirror.NetworkConnection:UnpackAndInvoke(NetworkReader, Int32) (at Assets/Mirror/Runtime/NetworkConnection.cs:156)
Mirror.NetworkConnection:TransportReceive(ArraySegment`1, Int32) (at Assets/Mirror/Runtime/NetworkConnection.cs:191)
Mirror.NetworkClient:OnDataReceived(ArraySegment`1, Int32) (at Assets/Mirror/Runtime/NetworkClient.cs:276)
kcp2k.KcpTransport:<Awake>b__13_2(ArraySegment`1) (at Assets/Mirror/Runtime/Transport/KCP/MirrorTransport/KcpTransport.cs:60)
kcp2k.KcpClient:<Connect>b__6_1(ArraySegment`1) (at Assets/Mirror/Runtime/Transport/KCP/kcp2k/highlevel/KcpClient.cs:45)
kcp2k.KcpConnection:TickIncoming_Authenticated(UInt32) (at Assets/Mirror/Runtime/Transport/KCP/kcp2k/highlevel/KcpConnection.cs:326)
kcp2k.KcpConnection:TickIncoming() (at Assets/Mirror/Runtime/Transport/KCP/kcp2k/highlevel/KcpConnection.cs:367)
kcp2k.KcpClient:TickIncoming() (at Assets/Mirror/Runtime/Transport/KCP/kcp2k/highlevel/KcpClient.cs:89)
kcp2k.KcpTransport:ClientEarlyUpdate() (at Assets/Mirror/Runtime/Transport/KCP/MirrorTransport/KcpTransport.cs:117)
Mirror.NetworkClient:NetworkEarlyUpdate() (at Assets/Mirror/Runtime/NetworkClient.cs:1204)
Mirror.NetworkLoop:NetworkEarlyUpdate() (at Assets/Mirror/Runtime/NetworkLoop.cs:182)

[Jengal34]

Same problem here. trying to sync (with hook) a var from a type derived class from NetworkBehaviour the host change value before the client join then the client join, it will receive a SyncHooked(null,null) instead of SyncHooked(null,newValue) (no error just wrong SyncFunction params)

[lumeriith]

At the time I wrote this issue about two bugs:

1. NetworkBehaviour SyncVars work fine, but Tester SyncVars (which inherits from NetworkBehaviour) show different value on clients.
2. (described on Additional Context section) Writing custom reader/writer of NetworkBehaviours makes deserialization fail because of server and client using different reader/writerFunc in certain unknown conditions.

I did further testing and found out that the first bug is purely editor bug.

Server Code

    [SyncVar]
    public MonsterBase recentMonster;
    [SyncVar]
    public NetworkBehaviour recentBehaviour;

...

    private IEnumerator SetRandomMonsterRoutine()
    {
        while (true)
        {
            var monsters = FindObjectsOfType<MonsterBase>();
            var mon = monsters[Random.Range(0, monsters.Length)];
            recentMonster = mon;
            recentBehaviour = mon;
            yield return new WaitForSeconds(1f);
        }
    }

Server/Client Inspector side-by-side

recentBehaviour field shows the correct value, but recentMonster field doesn't, even though they're set to same value on server. In fact, recentMonster field is delayed by exactly one SyncVar set, showing what the value was the last time. However as soon as I add a code that reads from recentMonster, you can see it starts to show correct values on editor.

The first bug became a really trivial one, the focus should be on the second bug. I will edit the title accordingly to this find.

[Jengal34]

Since I add some info to your bug, I think I figured out "my" problem. At the really start, the var is synched but I guess the other player component is not created for the new client. So the hook occur saying "value change from null (a real null) to null (I have a netID but no gameComponent with it)" And as soon as the game component is created, the hook is no more invoked. I feel like it s a bug. but not sure it s yours.

[MrGadget1024]

Since I add some info to your bug, I think I figured out "my" problem.

You're issue is a race condition if you're trying to ref something on a different object that may not yet exist on the client, which is why we caution strongly against game objects / NB's in SyncVars because we can't guarantee spawn order (or if the other thing will ever spawn due to network visibility). That's not the same as this bug report.

[imerr]

NB SyncVar Serialization uses writer.GetWriteFunc which looks up the right method by type: https://github.com/MirrorNetworking/Mirror/blob/master/Assets/Mirror/Editor/Weaver/Processors/NetworkBehaviourProcessor.cs#L446 or defaults to the standard Write method for NetworkBehaviour, which should be NetworkWriterExtensions.WriteNetworkBehaviour if I'm seeing this right.

NB SyncVar Deserialization uses a deserializer for the generated backing field of type "NetworkBehaviourSyncVar" to serialize it: https://github.com/MirrorNetworking/Mirror/blob/master/Assets/Mirror/Core/NetworkBehaviour.cs#L913

Thats where the mismatch is coming from with custom serializers

The fix seems to be to not use the convenient reader, but to manually call .Read<T> and then "copy over" the netId/componentIndex, but that doesn't work currently. It'll fail with a "No reader found for . Use a type supported by mirror or define a custom reader extension" since the weaver never sees Read being called as the helper method is generic and those are currently not looked at (can they even?) But we can easily force the weaver to generate the reader mapping for that type, no worries

Next issue: ordering 🎉 It seem, when a NB syncvar references itself, the NB isn't actually registered as spawned until after the syncvars are populated, so the NB lookup fails Otherwise it seems to work great https://share.dl.je/2023/02/2023-02-17_19-44-50_xDvKrVo66G.mp4 see how if it spawns in with it set to itself the lookup doesnt work, but NI does

            // we need to use reader.Read<T> here, since people may define custom
            // readers for their network behaviours
            T readNB = reader.Read<T>();

            netIdField = new NetworkBehaviourSyncVar();
            if (readNB)
            {
                netIdField.netId = readNB.netId;
                netIdField.componentIndex = readNB.ComponentIndex;
            }

instead of the old

netIdField = reader.ReadNetworkBehaviourSyncVar();

Weaver patch to force generation of typed reader: Goes here https://github.com/MirrorNetworking/Mirror/blob/master/Assets/Mirror/Editor/Weaver/Processors/NetworkBehaviourProcessor.cs#L589

                // force generation of a reader, if it doesn't exist for this type yet
                // we need to do this in order to be able to use Read<T> in the deserialization helper
                // We need to use Read<T> in case a custom reader/writer pair is defined for the type
                // Previously the serialization code would always use the custom writer,
                // while the deserialization would only use a fixed NB reader in the deserialization
                // helper method we call below
                // https://github.com/MirrorNetworking/Mirror/issues/2680
                MethodReference readFunc = readers.GetReadFunc(syncVar.FieldType, ref WeavingFailed);
                if (readFunc == null)
                {
                    Log.Error($"Failed to generate a read method for a NetworkBehaviour subclass.", syncVar);
                    WeavingFailed = true;
                    return;
                }