[SyncVar] GameObject race condition about mirror

[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.

[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.

[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.

[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]

🎉 This issue has been resolved in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[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.