[SyncVar] GameObject race condition

[miwarnec]

Just gathering my findings here.

The bug: Player.cs has [SyncVar] GameObject pet; Pet.cs has [SyncVar] GameObject owner;

In other words: circular references.

Now this player+pet are spawned at another client, here is what happens:

1. Player component is spawned. pet is set to that pet, which doesn't exist yet (hence null)
2. Pet component is spawned. owner is set to that player, which exists (see 1.). hence not null.

so instead of Player knowing Pet and Pet knowing Player, only Pet knows Player because it was spawned after Player. Player was spawned before Pet, so it doesn't know Pet.

What weaver does right now: for every [SyncVar] GameObject player; it creates:

1. a uint ___playerNetId;
2. a Networkplayer property that does this:

   public GameObject Networkplayer
   {
   get
   {
     return this.player;
   }
   [param: In] set
   {
     this.SetSyncVarGameObject(value, ref this.player, 8388608UL, ref this.____playerNetId);
   }
   }

   The get just returns the original variable. The set sets calls SetSyncVarGameObject which sets dirty and then sets the original variable.

3. replaces all player = ... (writes) in code with NetworkPlayer = ....
4. In PreStartClient() assigns Networkplayer = ClientScene.FindLocalObject(__playerNetId)

Solution 1 What it should do is to also replace all ... = player (reads) with ... = Networkplayer and in Networkplayer get return ClientScene.FindLocalObject(___playerNetId)

In other words: for every [SyncVar] GameObject test variable it should wrap get/set around the __testNetId field. The netId is just an int, it doesn't matter when we receive it. The 'get' will simply return objects[netId] which is null if not spawned yet, or != null if spawned yet.

Solution 2 Spawn everything first. Then assign. Perhaps it's already meant to do that but it doesn't for some reason. The NetworkPlayer field is assigned in 'PreStartClient()' after all.

[miwarnec]

Some more findings: PreStartClient() is called in NetworkIdentity.OnStartClient().

So this is not reliable enough. if Player is spawned, then 10s later Pet is spawned (=both in separate spawn packets) then OnStartClient of Player won't know Pet. No way around it except wrapping [SyncVar] NetIdentity with netIds.

[miwarnec]

just for my notes. the plan:

1. NetworkVAR.get: return clientscene.findlocalobject
2. remove PreStartClient? not needed anymore then?
3. replace 'VAR' reads with 'NetworkVAR.get'

[miwarnec]

More findings: OnDeserialize reads the netId if initialstate, and ReadGameObject() otherwise. Not 100% sure about the reason yet, but looks like we can remove Read/WriteGameObject() from NetworkReader/Writer if this is fixed too.

[miwarnec]

This is what it should look like in the end.

Might need a helper function so we don't have to do all of that with the weaver.

[paulpach]

Be careful with removing Read/Write game object. I believe this is used when you pass a gameobject in a Cmd or Rpc.

[miwarnec]

Some more research.. the above method only works if the GameObject was spawned already. This will result in NullReferenceExceptions if we try to access it before it was spawned, for example:

So this might not be the best solution yet after all.

[miwarnec]

Will test this solution in uMMORPG for a while now. This should work: There is one _ownerNetId which is still persistent, and the result is now cached in _owner which also makes it work before Spawn is called, because it's assigned in the setter too.

[paulpach]

Cache is not good. What happens if you reassign the object? the client would keep using the old object.

[miwarnec]

Cache is not good. What happens if you reassign the object? the client would keep using the old object.

Good point. It seems like 'return NetworkIdentity.all[netId]' would be the easiest solution here. Right now NetworkIdentities are only stored in a dict after OnStartServer or ApplySpawnPayload. Not sure why yet.

[paulpach]

@vis2k any progress on this? I believe this affects me too

[miwarnec]

still working on some edge cases, like assigning player.pet to summoned pet before networkserver.spawned was called, in which case netid is still 0.

[miwarnec]

considering to simply show a warning in that case. [SyncVar] GameObject/NetworkIdentity is really just a wrapper around netid then, so using null values or values with netid=0 will set it to 0, that's it

[miwarnec]

this works in uMMORPG so far:

porting it to Weaver now.

[miwarnec]

Test setup in case it's needed again later:

using UnityEngine;
using Mirror;

public class Test : NetworkBehaviour
{
    // the syncvar
    [SyncVar] GameObject test;

    // a function that uses it
    void Update()
    {
        Debug.Log(test.name);

        // read and write once
        GameObject read = test;
        test = gameObject;
    }
}

Currently generated:

using Mirror;
using System.Runtime.InteropServices;
using UnityEngine;

public class Test : NetworkBehaviour
{
  [SyncVar]
  private GameObject test;
  private uint ___testNetId;

  private void Update()
  {
    Debug.Log((object) ((Object) this.test).get_name());
    GameObject test = this.test;
    this.Networktest = ((Component) this).get_gameObject();
  }

  private void MirrorProcessed()
  {
  }

  public GameObject Networktest
  {
    get
    {
      return this.test;
    }
    [param: In] set
    {
      this.SetSyncVarGameObject(value, ref this.test, 1UL, ref this.___testNetId);
    }
  }

  public override bool OnSerialize(NetworkWriter writer, bool forceAll)
  {
    bool flag = base.OnSerialize(writer, forceAll);
    if (forceAll)
    {
      writer.Write(this.test);
      return true;
    }
    writer.WritePackedUInt64(this.syncVarDirtyBits);
    if (((long) this.syncVarDirtyBits & 1L) != 0L)
    {
      writer.Write(this.test);
      flag = true;
    }
    return flag;
  }

  public override void OnDeserialize(NetworkReader reader, bool initialState)
  {
    base.OnDeserialize(reader, initialState);
    if (initialState)
    {
      this.___testNetId = reader.ReadPackedUInt32();
    }
    else
    {
      if (((long) reader.ReadPackedUInt64() & 1L) == 0L)
        return;
      this.test = reader.ReadGameObject();
    }
  }

  public override void PreStartClient()
  {
    if (this.___testNetId == 0U)
      return;
    this.Networktest = ClientScene.FindLocalObject(this.___testNetId);
  }
}

[miwarnec]

progress so far: +removed prestartclient +Networktest.get returns GameObject based on netId +NetworkTest.set SetSyncVarGameObject assigns netId internally +onserialize/ondeserialize use netIds for GameObjects -all read access to 'test' still need to be replaced with Networktest.get

using Mirror;
using System.Runtime.InteropServices;
using UnityEngine;

public class Test : NetworkBehaviour
{
    [SyncVar]
    private GameObject test;

    private uint ___testNetId;

    public GameObject Networktest
    {
        get
        {
            return NetworkBehaviour.GetSyncVarGameObject(___testNetId);
        }
        [param: In]
        set
        {
            this.SetSyncVarGameObject(value, ref test, 1uL, ref ___testNetId);
        }
    }

    public Test()
        : this()
    {
    }

    private void Update()
    {
        Debug.Log((object)test.get_name());
        GameObject val = test;
        Networktest = this.get_gameObject();
    }

    private void MirrorProcessed()
    {
    }

    public override bool OnSerialize(NetworkWriter writer, bool forceAll)
    {
        bool result = this.OnSerialize(writer, forceAll);
        if (forceAll)
        {
            writer.WritePackedUInt32(___testNetId);
            return true;
        }
        writer.WritePackedUInt64(this.get_syncVarDirtyBits());
        if ((this.get_syncVarDirtyBits() & 1L) != 0L)
        {
            writer.WritePackedUInt32(___testNetId);
            result = true;
        }
        return result;
    }

    public override void OnDeserialize(NetworkReader reader, bool initialState)
    {
        this.OnDeserialize(reader, initialState);
        if (initialState)
        {
            ___testNetId = reader.ReadPackedUInt32();
            return;
        }
        long num = (long)reader.ReadPackedUInt64();
        if ((num & 1L) != 0L)
        {
            ___testNetId = reader.ReadPackedUInt32();
        }
    }
}

[miwarnec]

:tada: This issue has been resolved in version 1.0.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

[abdelfattahradwan]

@vis2k with all due respect, are you sure this has been fixed?! because this still happens even in the latest version as of writing this message.

[SoftwareGuy]

@Abdel-Fattah-Radwan Please don't necrobump old tickets, this ticket was from 2018/2019.

Please open a new ticket and reference this one. Thanks.