Removing destroyed NetworkBehaviours from NetworkObject's ChildNetworkBehaviours messes with the order of the other still living sibling NetworkBehaviours

[WhippetsAintDogs]

Description

Hi @NoelStephensUnity,

This commit introduced the bug I'm about to describe:
bc23a35 - fix: Nested NetworkBehaviours don't de-register or Invoke OnNetworkDespawn if destroyed while the parent NetworkObject remains spawned [NCCBUG-137] (#2091)

Let's say I have a NetworkObject representing an encounter with mobs. Each of the mobs are nested GameObjects with NetworkBehaviour(s) owned by the parent encounter. If you kill one of the mobs, the NetworkBehaviour(s) linked to that specific mob is / are going to be destroyed and, since that commit, removed from the encounter ChildNetworkBehaviours list. That will result in any further RPCs targeting one of the still living mob's NetworkBehaviour to fail and throw errors of that kind:

Their NetworkBehaviourId is reliant on their position in the ChildNetworkBehaviours list ; RPCs (RpcMessages.cs) use NetworkObject's GetNetworkBehaviourAtOrderIndex method which does this:

RpcMessages.cs:

        var networkBehaviour = networkObject.GetNetworkBehaviourAtOrderIndex(metadata.NetworkBehaviourId);

NetworkObject.cs:

        internal NetworkBehaviour GetNetworkBehaviourAtOrderIndex(ushort index)
        {
            if (index >= ChildNetworkBehaviours.Count)
            {
                if (NetworkLog.CurrentLogLevel <= LogLevel.Error)
                {
                    NetworkLog.LogError($"Behaviour index was out of bounds. Did you mess up the order of your {nameof(NetworkBehaviour)}s?");
                }

                return null;
            }

            return ChildNetworkBehaviours[index];
        }

Environment

• OS: Windows 10
• Unity Version: 2020.3
• Netcode Version: develop branch
• Netcode Commit: bc23a35

[NoelStephensUnity]

@WhippetsAintDogs To better help you, I will need just a little bit more information about this issue if you have the time?

Does this look like the hierarchy of your NetworkObject with children NetworkBehaviours?

• MobEncounter (GameObject) with NetworkObject component
  • MobEncounter_1 (GameObject) with NetworkBehaviour_1 component
  • MobEncounter_2 (GameObject) with NetworkBehaviour_2 component
  • MobEncounter_3 (GameObject) with NetworkBehaviour_3 component (NetworkBehaviour_1 -3 could be the same component type)

If so, are you deleting each MobEncounter_# GameObject when that mob is killed?

[WhippetsAintDogs]

Yes (with more stuff and levels of nesting, but the general idea is there) ! And yes, we are deleting each MobEncounter_# GameObject when that mob is killed.

[ashwinimurt]

MTT-4718

[NoelStephensUnity]

@WhippetsAintDogs

we are deleting each MobEncounter_# GameObject when that mob is killed.

The error message is there to let you know that somewhere in your scripts you are still sending RPCs to something that no longer exists. So, it sounds like the handling of the deletion of the MobEncounter_# GameObject is working as expected (i.e. not throwing an exception but logging an error if something tries to send a message to it).

If you want to continue deleting the GameObjects and avoid that error message, then you will need to possibly have some way to globally notify all clients (I am assuming a client is sending an RPC to the MobEncounter_# NetworkBehaviour) that the mob no longer exists (i.e. stop the senders from sending to something that no longer exists).

So, you have a few options:

• Disabling the MobEncounter_# GameObject
  • When you disable this you could do a check inside the RPC call to determine if the MobEncounter_# GameObject is disabled and exit early (NetworkBehaviours will still receive RPC calls if the GameObject is disabled).
    • While this does not stop from sending and receiving the RPC calls it is the simplest approach.
• Deleting the GameObject
  • In order to stop sending the messages, the sender(s) should be made aware that this MobEncounter_# GameObject is no longer available/valid.
    • While the SDK does handle deleting GameObjects with NetworkBehaviours, we do not recommend it. You also need to handle synchronizing the clients in a way that the scripts that are still sending the RPCs no longer send the RPCs to the MobEncounter_# NetworkBehaviour.
• Make each MOB its own NetworkObject
  • This does require you to change your project's current design slightly, but if each MobEncounter_# GameObject was its own NetworkObject then when the mob is "finished" you can simply despawn it.

Let me know if any of these options works with your projects needs/goals. If not, then please post additional details so I can further assist you.

[WhippetsAintDogs]

Hello @NoelStephensUnity !

Thanks for your time. I'm well aware of the purpose of this error message and this is not what is happening in my case. I invite you to re-read my original post as I specified that it is RPCs sent to still living mobs that cause this message, not to the destroyed ones. I even described where the problem is located in this codebase : if a mob in the encounter's ChildNetworkBehaviours list is removed because it was destroyed, it will shift the index of all of those that follows in the list, breaking the index / NetworkBehaviourId relationship needed by RpcMessages.cs to behave correctly.

We fixed it on our side by editing NGO's code and preventing the removing of destroyed child NetworkBehaviour from the ChildNetworkBehaviours list that the commit I linked before introduced. So, I do not need assistance, I'm simply reporting it for this project sake.

[NoelStephensUnity]

@WhippetsAintDogs I apologize for misleading you on the deletion of NetworkBehaviours. After creating a replica of your scenario it does seem there is another property being used to get the NetworkBehaviour index and that property is not being updated if a NetworkBehaviour is deleted from the ChildNetworkBehaviours list. Having the NetworkBehaviours re-assign their NetworkBehaviourId when a NetworkBehaviour is deleted should resolve this.

Will open a PR for this issue.
👍

[WhippetsAintDogs]

No worries ! 😄

Thanks @NoelStephensUnity !

[NoelStephensUnity]

@WhippetsAintDogs Attached is the manual test I used to verify the issue as well as how I synchronized connected clients when the GameObject the pseudo mob behaviour was attached to was deleted. 2188-ManualVerification.zip This should "just work" for you if you want to just see it working. There are 3 nested GameObjects with the pseudo mob NetworkBehaviour (clients just ping the server every second) component attached, and if you hit the 1, 2, or 3 key on the server-host then the respective GameObject will be deleted. It is a very small and simple test.

This approach (currently) does require that you send the clients an RPC to handle deleting the GameObject on the client side, and you might be a little fancier than my quick manual test by:

• Sending the ClientRpc first
• Server waits for the clients to delete the GameObject
  • Clients would then delete the associated GameObject and then respond back via ServerRpc with the same reference the server sent (or you could create a unique transaction identifier, etc).
• Once the server knows all clients no longer have that MOB GameObject, then it deletes the associated GameObject locally.
  • This is just to avoid timing issues if there is latency or clients are sending RPCs to the MOB relatively frequently.
• You will also need to keep track of what GameObjects were deleted so when a newly joined client has finished the initial synchronization (OnClientConnectedCallback would work for this if you are using the built in scene management/NetworkSceneManager) you will need to send the newly joined client a list of the deleted MOBs (so they don't try to send RPCs to things that no longer exist as well).

If you want to try an approach like this with your project, you can update your manifest.json file with this: "com.unity.netcode.gameobjects": "https://github.com/Unity-Technologies/com.unity.multiplayer.mlapi.git?path=/com.unity.netcode.gameobjects#fix/networkbehaviours-dont-upate-id-after-a-networkbehaviour-is-deleted-2188",

Which will use the branch that has the current fix.

Let me know if this approach works for your project?

[NoelStephensUnity]

Hello @WhippetsAintDogs, I have been looking at all of the possible ways to do this and after further investigating deleting a NetworkBehaviour, with the current implementation, it could be very problematic for NetworkBehaviours that have NetworkVariable properties.

There are still ongoing discussions about certain areas within the SDK that could be improved and this issue is being added to the list of things that still need to be properly mapped out/discussed in order to determine the best approach. This means a fix for this is not likely going to happen in the next update (or two). 😞

For now, you can accomplish the same type of functionality by making each mob their own NetworkObject and create a pool of NetworkObjects for your mobs.
To switch from your current approach, where you have 1 NetworkObject and several child GameObjects with NetworkBehaviours, it would look something like this:

Object Pool Approach: MobEncounter (NetworkObject + GameObject): This would need to also have a NetworkBehaviour component that implements INetworkPrefabInstanceHandlerand instantiates and manages (n) number of network prefab mob instances. (If you have several unique mob instance, then the simplest approach would be to have a MobEncounter per unique mob type)

When a GenericMob is no longer alive/valid you would:

• Have the server-side despawn the instance
• On both the server and client side, MobEncounter's Destroy(NetworkObject networkObject); method will be invoked and you can either disable it to be re-used later or just simply delete it depending upon your project's design/needs.

Please feel free to post any issues you run into using above approach here and I will assist you.