fix: ClientRpcs ignores NetworkObject's observer list [MTT-2878] #1836

NoelStephensUnity · 2022-03-25T18:45:21Z

This fixes the issue where NetworkBehaviour.__endSendClientRpc ignored the associated NetworkObject's Observers list and would send to all connected clients (including non-observers which would not have that NetworkObject instance).

Changelog

Fixed: ClientRpcs would always send to all connected clients by default as opposed to only sending to the NetworkObject's Observers list by default.

Testing and Documentation

Includes an integration test.

First pass fix for bug MTT-2878 where ClientRPCs do not take into account the NetworkObject's observers list

Adding observer checks when using ClientRpcParams.Send.TargetClientIds or ClientRpcParams.Send.TargetClientIdsNativeArray to let users know that they are sending a ClientRpc to a client that is not an observer of the NetworkObject.

This test verifies that only client observers of a NetworkObject receive the ClientRpc message.

Created GenerateObserverErrorMessage for generating the "sending to a non-observer" error message.

updated the test to check for the "sending to a non-observer" error message

Added check to see if only the host was supposed to receive the rpc when it was the only observer and check to see if the host received the rpc when no clients were connected.

updated comments and assert message.

com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs

Removing the [MethodImpl(MethodImplOptions.AggressiveInlining)] attribute.

removing namespace.

TwoTenPvP

Minor changes, overall looks neat!

com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs

addressing Albin's catches on how we log an error as well as assuring the array is disposed.

Setting the log level required to LogLevel.Error

Greater than or equal to Error...that is. >.<

TwoTenPvP

After the changes discussed here and in Slack. 👌🏻

Adjusting RpcObserverTests error message check since we changed it to a Networklog.

mattwalsh-unity · 2022-03-28T18:37:35Z

com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs

@@ -158,7 +158,7 @@ internal void __endSendClientRpc(ref FastBufferWriter bufferWriter, uint rpcMeth
            // We check to see if we need to shortcut for the case where we are the host/server and we can send a clientRPC
            // to ourself. Sadly we have to figure that out from the list of clientIds :(
            bool shouldSendToHost = false;
-
+            var logLevel = NetworkManager.LogLevel;


Though I'm a big fan of temp variables for clarity, I'm not sure this actually does so here.

com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs

mattwalsh-unity · 2022-03-28T20:36:08Z

com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs

+                                shouldSendToHost = true;
+                                continue;
+                            }
+                            observerClientIds[observerCount++] = observerEnumerator.Current;


Rather than go to the trouble of allocating and building up the client ids, why not simply here in the while loop do:

NetworkManager.SendMessage(ref clientRpcMessage, networkDelivery, observerEnumerator.Current)

After our internal discussion, I changed my approach to avoid the temporary memory allocation.

Removed var logLevel. Re-organized the error logging check such that it will only perform the HashSet.Contains call when LogLevel.Error level or higher.

removing the allocation, avoiding cost of using NetworkManager.SendMessage, and now adding each observer client's message to the outbound queue one at a time.

NoelStephensUnity added 6 commits March 25, 2022 11:37

fix

e241738

First pass fix for bug MTT-2878 where ClientRPCs do not take into account the NetworkObject's observers list

update

51a9b43

Adding observer checks when using ClientRpcParams.Send.TargetClientIds or ClientRpcParams.Send.TargetClientIdsNativeArray to let users know that they are sending a ClientRpc to a client that is not an observer of the NetworkObject.

test

a59afb8

This test verifies that only client observers of a NetworkObject receive the ClientRpc message.

update

99720d8

Created GenerateObserverErrorMessage for generating the "sending to a non-observer" error message.

test

d59ce62

updated the test to check for the "sending to a non-observer" error message

Merge branch 'develop' into fix/mtt-2878-clientrpc-ignores-observers

563be64

NoelStephensUnity requested a review from 0xFA11 March 25, 2022 18:47

NoelStephensUnity added 3 commits March 25, 2022 13:49

Update CHANGELOG.md

64338b8

update

22a9c40

Added check to see if only the host was supposed to receive the rpc when it was the only observer and check to see if the host received the rpc when no clients were connected.

style

4020022

updated comments and assert message.

NoelStephensUnity requested a review from ShadauxCat March 25, 2022 19:35

becksebenius-unity reviewed Mar 25, 2022

View reviewed changes

com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs Outdated Show resolved Hide resolved

NoelStephensUnity and others added 3 commits March 25, 2022 15:15

update

925941c

Removing the [MethodImpl(MethodImplOptions.AggressiveInlining)] attribute.

style

b544fdd

removing namespace.

Merge branch 'develop' into fix/mtt-2878-clientrpc-ignores-observers

600bf98

NoelStephensUnity requested a review from mattwalsh-unity March 28, 2022 15:34

TwoTenPvP suggested changes Mar 28, 2022

View reviewed changes

TwoTenPvP and others added 4 commits March 28, 2022 16:21

Merge branch 'develop' into fix/mtt-2878-clientrpc-ignores-observers

d3dbefd

update

06a8937

addressing Albin's catches on how we log an error as well as assuring the array is disposed.

update

2b40268

Setting the log level required to LogLevel.Error

fix

5adeff9

Greater than or equal to Error...that is. >.<

NoelStephensUnity requested a review from TwoTenPvP March 28, 2022 16:53

TwoTenPvP approved these changes Mar 28, 2022

View reviewed changes

NoelStephensUnity and others added 3 commits March 28, 2022 12:12

Merge branch 'develop' into fix/mtt-2878-clientrpc-ignores-observers

fb16e5b

fix

cfcd8df

Adjusting RpcObserverTests error message check since we changed it to a Networklog.

Merge branch 'develop' into fix/mtt-2878-clientrpc-ignores-observers

d2ad8a4

NoelStephensUnity requested a review from becksebenius-unity March 28, 2022 18:37

Merge branch 'develop' into fix/mtt-2878-clientrpc-ignores-observers

fb0d58a

mattwalsh-unity suggested changes Mar 28, 2022

View reviewed changes

update

42265d0

Removed var logLevel. Re-organized the error logging check such that it will only perform the HashSet.Contains call when LogLevel.Error level or higher.

update

e2e0779

removing the allocation, avoiding cost of using NetworkManager.SendMessage, and now adding each observer client's message to the outbound queue one at a time.

NoelStephensUnity requested a review from mattwalsh-unity March 28, 2022 23:44

mattwalsh-unity approved these changes Mar 29, 2022

View reviewed changes

mattwalsh-unity merged commit 3e9923c into develop Mar 29, 2022

mattwalsh-unity deleted the fix/mtt-2878-clientrpc-ignores-observers branch March 29, 2022 00:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: ClientRpcs ignores NetworkObject's observer list [MTT-2878] #1836

fix: ClientRpcs ignores NetworkObject's observer list [MTT-2878] #1836

Uh oh!

NoelStephensUnity commented Mar 25, 2022 •

edited

Loading

Uh oh!

Uh oh!

TwoTenPvP left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TwoTenPvP left a comment

Uh oh!

mattwalsh-unity Mar 28, 2022

Uh oh!

Uh oh!

mattwalsh-unity Mar 28, 2022

Uh oh!

NoelStephensUnity Mar 28, 2022

Uh oh!

Uh oh!

fix: ClientRpcs ignores NetworkObject's observer list [MTT-2878] #1836

fix: ClientRpcs ignores NetworkObject's observer list [MTT-2878] #1836

Uh oh!

Conversation

NoelStephensUnity commented Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog

Testing and Documentation

Uh oh!

Uh oh!

TwoTenPvP left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TwoTenPvP left a comment

Choose a reason for hiding this comment

Uh oh!

mattwalsh-unity Mar 28, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattwalsh-unity Mar 28, 2022

Choose a reason for hiding this comment

Uh oh!

NoelStephensUnity Mar 28, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NoelStephensUnity commented Mar 25, 2022 •

edited

Loading