com.unity.netcode.gameobjects icon indicating copy to clipboard operation
com.unity.netcode.gameobjects copied to clipboard

NetworkObjectReference can't be NULL though that seems like a perfectly valid use case

Open UnkelRambo opened this issue 2 years ago • 22 comments

Is your feature request related to a problem? Please describe. Being able to set a NetworkObjectReference to NULL seems like it should be a valid use-case. As it is, there's no way to do something like tracking a carried object when nothing is carried.

Describe the solution you'd like Pretty straight forward: NetworkObjectReference myObject = null;

This is explicitly disallowed in the NetworkObjectReference constructor (or one of the conversion methods), I believe.

Describe alternatives you've considered I copy+pasted NetworkObjectReference and made a NullableNetworkObjectReference that handles the NULL case correctly.

HOWEVER, this also required me to create a dummy "null network object" to take NetworkID zero, which I'm using to indicate an INVALID (or NULL) object.

Additional context This seems pretty straight forward BUT would require valid NetworkID's to start at 1 instead of 0 (which is what the Source engine did AFAIK.)

UnkelRambo avatar Dec 13 '22 19:12 UnkelRambo

@UnkelRambo You can accomplish what you are trying to do by using its TryGet method like such: (code snippet assumes the property and method are both within a NetworkBehaviour derived class)

        private NetworkObjectReference m_ObjectBeingCarried = new NetworkObjectReference();

        public NetworkObject GetObjectCarried()
        {
            if (m_ObjectBeingCarried.TryGet(out NetworkObject networkObject, NetworkManager))
            {
                return networkObject;
            }
            return null;
        }

Let me know if this helps?

NoelStephensUnity avatar Dec 13 '22 23:12 NoelStephensUnity

@UnkelRambo You can accomplish what you are trying to do by using its TryGet method like such: (code snippet assumes the property and method are both within a NetworkBehaviour derived class)

        private NetworkObjectReference m_ObjectBeingCarried = new NetworkObjectReference();

        public NetworkObject GetObjectCarried()
        {
            if (m_ObjectBeingCarried.TryGet(out NetworkObject networkObject, NetworkManager))
            {
                return networkObject;
            }
            return null;
        }

Let me know if this helps?

This doesn't cover the use case I'm talking about. What I'd expect to do with a NetworkVariable<NetworkObjectReference>> is to be able to assign NULL from the server in cases where the expected object SHOULD be nothing.

For example:

If I pick a the "rock" object up, the client sends an RPC to the server to do the PickUp action. The server sets a NetworkVariable<NetworkObjectReference>> to "rock".

Later, when the client puts the "rock" down, the client sends a server "PutDown" RPC, the server sets that network variable to NULL, etc.

This is important because it lets remote clients who are joining in progress know that SOMETHING is being held, so the client can respond appropriately (setting non-replicated state like animation state.)

This is actually an important piece of MISSING functionality in the Boss Room example submitted as an answer here:

https://forum.unity.com/threads/players-picking-up-objects-is-much-harder-than-it-should-be.1343492/

Again, just to be clear, I would 100% expect to be able to set a NULL replicated object reference. There are plenty of other places in the Netcode codebase where NULL isn't a valid option (which I'll happily submit separate tickets for.) Setting an object parent to NULL in NetworkObject.TrySetParent(), for instance, is explicitly disallowed (another thing I've had an awkward workaround for) but seems like a completely valid use case (and is valid in Unity using 'transform.parent = null')

UnkelRambo avatar Dec 14 '22 00:12 UnkelRambo

Just got the same problem during a UNET migration. At some point a RPC send a GameObject that could be null (in our case something like 'Destroyed( GameObject src)' and having no src is perfectly fine) Currently need to split into 2 rpcs or using a special 'empty' gameobject created for this : no very handy

Since we have a TryGet, creating a NetworkObjectReference from a null object should be perfectly fine

matthieu-blin avatar Dec 14 '22 09:12 matthieu-blin

@UnkelRambo Currently, you are right and NGO does not handle communicating "null" as it really is of type "nothing". However, I do see where having had a pre-defined "null type" (or way to communicate "nothing") might be useful under certain conditions.

Out of curiosity, have you tried something like this?

private NetworkVariable<NetworkObjectReference> m_ItemBeingHeld = new NetworkVariable<NetworkObjectReference>(new NetworkObjectReference());

public override void OnNetworkObjectParentChanged(NetworkObject parentNetworkObject)
{
    if (IsServer)
    {
        if (parentNetworkObject != null)
        {
            m_ItemBeingHeld.Value = new NetworkObjectReference(parentNetworkObject);
        }
        else
        {
            m_ItemBeingHeld.Value = new NetworkObjectReference();
        }

    }
    base.OnNetworkObjectParentChanged(parentNetworkObject);
}

public NetworkObject GetObjectCarried()
{
    if (m_ItemBeingHeld.Value.TryGet(out NetworkObject networkObject, NetworkManager))
    {
        return networkObject;
    }
    return null;
}

This basically does the same thing that you are describing without the use of a null.

You might also look at the NetworkBehaviour.OnSynchronize (added in NGO v1.2.0) to handle synchronizing a specific state of an item a player may or may not be holding for late joining clients. Using a NetworkVariable for this will generate more traffic for connected clients (i.e. you have the parenting message and then you will have a NetworkVariable update that follows).

I also posted a reply, which includes some additional information and a more advanced way to handle parenting, to the forum post you referenced in case you are interested.

NoelStephensUnity avatar Dec 14 '22 14:12 NoelStephensUnity

I'm not really looking for a workaround, I have one already, though I greatly appreciate the suggestion! My workaround is to create a dummy NULL object that's NetID=0, then use NetID 0 to indicate NULL. It works, but my expectation (and confident assumption of this being a broad expectation among developers) of any network ready game engine is that networked NULL object references would be a built-in assumption of functionality.

It's common functionality across Unreal, Source Engine, Quake, proprietary AAA engines I've helped build, etc. and, as @matthieu-blin mentioned, creates issues porting from other Unity networking libraries with support for NULL replicated object references.

UnkelRambo avatar Dec 14 '22 16:12 UnkelRambo

Totally understandable. The feature request is marked for import and we will look into a viable solution. The example provided is really an intended usage pattern and might provide insight to anyone else trying to accomplish something similar. Just making sure that you and potentially other users are not blocked by something like this until we have had a chance to make improvements.
👍

NoelStephensUnity avatar Dec 14 '22 16:12 NoelStephensUnity

Awesome thanks so much, definitely appreciated! Now that I've discovered the Github feedback section here, I'll post a few more pieces of feedback/feature request stuff. Loving Netcode what I'm doing so far for a full-time project 👍

UnkelRambo avatar Dec 14 '22 16:12 UnkelRambo

I agree, and in fact already raised this all the way back in February: https://github.com/Unity-Technologies/com.unity.netcode.gameobjects/issues/1693

daalen avatar Dec 14 '22 19:12 daalen

@daalen Ahh, I am not sure if we had the type:feature label back then so perhaps feedback was the only option... but at that point we were handling other deeper issues...so apologies for what I can only say is a lack of response to your feedback. :(

For future submissions, I would recommend labeling things that you think should be a feature with type:feature...my support time (in-between fixing/implementing) is primarily focused around bugs and feature requests...hopefully I will get a chance to review through all of the feedback labeled issues in the near future.

NoelStephensUnity avatar Dec 14 '22 21:12 NoelStephensUnity

No problem, I will keep that in mind in the future.

daalen avatar Dec 14 '22 21:12 daalen

I am having the same issue, and it really doesn't make sense.

Example code:

public class ExampleNetworkClass : NetworkBehaviour
{ ... }

public class SecondClass : NetworkBehaviour
{
  private ExampleNetworkClass exampleNetworkClass;

  private void Awake()
  {
    exampleNetworkClass = GetComponent<ExampleNetworkClass>(); // which let's assume returns null as there's no component.
  }

  private void Start()
  {
    ExampleServerRpc(new NetworkBehaviourReference(exampleNetworkClass)); // exampleNetworkClass is null.
  }

  private void ExampleServerRpc(NetworkBehaviourReference networkBehaviourReference)
  { ... }
}

The issue at hand is simplified in:

new NetworkBehaviourReference(null); // is not allowed.

... which should be perfectly allowed.

I don't see how it's taken 6 months and it hasn't bothered anyone else.

@UnkelRambo , your null object suggestion is a really nice workaround, though as with you I insist that null reference should be considered perfectly allowed.

powersupersport avatar Jun 15 '23 13:06 powersupersport

I am having the same issue, and it really doesn't make sense.

Example code:

public class ExampleNetworkClass : NetworkBehaviour
{ ... }

public class SecondClass : NetworkBehaviour
{
  private ExampleNetworkClass exampleNetworkClass;

  private void Awake()
  {
    exampleNetworkClass = GetComponent<ExampleNetworkClass>(); // which let's assume returns null as there's no component.
  }

  private void Start()
  {
    ExampleServerRpc(new NetworkBehaviourReference(exampleNetworkClass)); // exampleNetworkClass is null.
  }

  private void ExampleServerRpc(NetworkBehaviourReference networkBehaviourReference)
  { ... }
}

The issue at hand is simplified in:

new NetworkBehaviourReference(null); // is not allowed.

... which should be perfectly allowed.

I don't see how it's taken 6 months and it hasn't bothered anyone else.

@UnkelRambo , your null object suggestion is a really nice workaround, though as with you I insist that null reference should be considered perfectly allowed.

I just upgraded to Netcode 1.4 and this still isn't supported. If I've got time tomorrow I'll fork, fix it, and add a pull request. BIG if, tight deadlines abound ;)

UnkelRambo avatar Jun 15 '23 15:06 UnkelRambo

I absolutely forgot it's open-source. Of course, it would be nice.

powersupersport avatar Jun 15 '23 18:06 powersupersport

I ran into the same problem and did some testing on a fork. I changed the constructors from NetworkObjectReference to something like:

public NetworkObjectReference(NetworkObject networkObject) {
	if (networkObject == null) {
		m_NetworkObjectId = ulong.MaxValue;
		return;
	}

	if (networkObject.IsSpawned == false) {
		throw new ArgumentException($"{nameof(NetworkObjectReference)} can only be created from spawned {nameof(NetworkObject)}s.");
	}

	m_NetworkObjectId = networkObject.NetworkObjectId;
}

public NetworkObjectReference(GameObject gameObject) {
	if (gameObject == null) {
		m_NetworkObjectId = ulong.MaxValue;
		return;
	}
	var networkObject = gameObject.GetComponent<NetworkObject>() ?? throw new ArgumentException($"Cannot create {nameof(NetworkObjectReference)} from {nameof(GameObject)} without a {nameof(NetworkObject)} component.");
	if (networkObject.IsSpawned == false) {
		throw new ArgumentException($"{nameof(NetworkObjectReference)} can only be created from spawned {nameof(NetworkObject)}s.");
	}

	m_NetworkObjectId = networkObject.NetworkObjectId;
}

This seems to work pretty well, although I havent tested it much. I set the m_NetworkObjectId as ulong.MaxValue, since 0 could be assigned to a real object. there still might be an object with id ulong.MaxValue, so to porperly fix it the id generation code sould be edited. The deserialization already returns null if there is no gameObject with the id.

While we are at it, we could do the same for NetworkBehaviourReference:

public NetworkBehaviourReference(NetworkBehaviour networkBehaviour) {
	if (networkBehaviour == null) {
		m_NetworkObjectReference = new NetworkObjectReference((NetworkObject)null);
		m_NetworkBehaviourId = 0;
		return;
	}
	if (networkBehaviour.NetworkObject == null) {
		throw new ArgumentException($"Cannot create {nameof(NetworkBehaviourReference)} from {nameof(NetworkBehaviour)} without a {nameof(NetworkObject)}.");
	}

	m_NetworkObjectReference = networkBehaviour.NetworkObject;
	m_NetworkBehaviourId = networkBehaviour.NetworkBehaviourId;
}

l-landgraf avatar Aug 20 '23 18:08 l-landgraf

Thanks @mrhalleg

zachstronaut avatar Aug 30 '23 20:08 zachstronaut

Random sidenote for somebody: ParrelSync doesn't sync your custom Netcode package that you've made custom edits to... so remember to make a new clone. ;p

zachstronaut avatar Aug 30 '23 20:08 zachstronaut

@zachstronaut Pretty sure it does, but you have to restart the clone editor. It syncs the packages on restart.

powersupersport avatar Aug 30 '23 21:08 powersupersport

NetworkObjectReference and NetworkBehaviourReference should both support null, not just for RPCs but also for people implementing their own INetworkSerializable.

When implementing INetworkSerializable on a struct which contains a gameObject reference (or even worse a NetworkBehaviour) which can be null, storing that null NetworkBehaviourReference isn’t possible and so the NetworkSerialize<> function has to re-implement a method for passing this value by networkObjectID and a bool to determine if this ID is valid. NetworkBehaviours are particularly difficult to pass this way as the NetworkBehaviourID conversion to a NetworkBehaviour is not exposed so for now I’m only able to pass by type of the component and assume that the component is the only one of it’s type on the gameobject.

zachstronaut avatar Sep 13 '23 17:09 zachstronaut

@zachstronaut We are looking into this issue as a potential candidate for the next release after the up-and-coming v1.7.0.

NoelStephensUnity avatar Oct 04 '23 21:10 NoelStephensUnity

@zachstronaut We are looking into this issue as a potential candidate for the next release after the up-and-coming v1.7.0.

Pretty please with sugar on top for the next release! I've had the user supplied patch in place for awhile now on my end.

zachstronaut avatar Oct 10 '23 16:10 zachstronaut

Is this being worked on? This is a pretty important feature. You cannot have player equipment/clothing without the support for null/empty.

pnarimani avatar Jan 24 '24 07:01 pnarimani

@pnarimani the fix supplied above does work, but I sure would love to see this in an official release so I can stop re-patching it in manually!

The fix: https://github.com/Unity-Technologies/com.unity.netcode.gameobjects/issues/2346#issuecomment-1685354716

zachstronaut avatar Jan 29 '24 18:01 zachstronaut

@UnkelRambo Currently, you are right and NGO does not handle communicating "null" as it really is of type "nothing". However, I do see where having had a pre-defined "null type" (or way to communicate "nothing") might be useful under certain conditions.

Out of curiosity, have you tried something like this?

private NetworkVariable<NetworkObjectReference> m_ItemBeingHeld = new NetworkVariable<NetworkObjectReference>(new NetworkObjectReference());

public override void OnNetworkObjectParentChanged(NetworkObject parentNetworkObject)
{
    if (IsServer)
    {
        if (parentNetworkObject != null)
        {
            m_ItemBeingHeld.Value = new NetworkObjectReference(parentNetworkObject);
        }
        else
        {
            m_ItemBeingHeld.Value = new NetworkObjectReference();
        }

    }
    base.OnNetworkObjectParentChanged(parentNetworkObject);
}

public NetworkObject GetObjectCarried()
{
    if (m_ItemBeingHeld.Value.TryGet(out NetworkObject networkObject, NetworkManager))
    {
        return networkObject;
    }
    return null;
}

This basically does the same thing that you are describing without the use of a null.

You might also look at the NetworkBehaviour.OnSynchronize (added in NGO v1.2.0) to handle synchronizing a specific state of an item a player may or may not be holding for late joining clients. Using a NetworkVariable for this will generate more traffic for connected clients (i.e. you have the parenting message and then you will have a NetworkVariable update that follows).

I also posted a reply, which includes some additional information and a more advanced way to handle parenting, to the forum post you referenced in case you are interested.

I hope anyone struggling has seen this. I nearly gave up on my project because I couldn't do something simple like pick up and drop an item in a non rpc way - the auto sync state that network variables provide is too crucial. I finally found this and it worked perfectly. I will adopt the null pattern when it works but, this really works fine.

Thanks @NoelStephensUnity

tones31 avatar Mar 14 '24 02:03 tones31

For anyone still following this: #2874 imports the user submitted solution and this will be available in the next (v1.9.0) update (we are currently working through the promotion steps for this update).

I am closing this issue out at this time. 👍

NoelStephensUnity avatar Apr 05 '24 21:04 NoelStephensUnity