Skip to content

Commit 3e9923c

Browse files
fix: ClientRpcs ignores NetworkObject's observer list [MTT-2878] (#1836)
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. Removing the [MethodImpl(MethodImplOptions.AggressiveInlining)] attribute. removing namespace. 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 Adjusting RpcObserverTests error message check since we changed it to a Networklog. 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. Co-authored-by: Albin Corén <2108U9@gmail.com>
1 parent b72456c commit 3e9923c

File tree

4 files changed

+358
-3
lines changed

4 files changed

+358
-3
lines changed

com.unity.netcode.gameobjects/CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
2323
### Fixed
2424
- Fixed NetworkBehaviour dependency verification check for an existing NetworkObject not searching from root parent transform relative GameObject. (#1841)
2525
- Fixed issue where entries were not being removed from the NetworkSpawnManager.OwnershipToObjectsTable. (#1838)
26+
- Fixed ClientRpcs would always send to all connected clients by default as opposed to only sending to the NetworkObject's Observers list by default. (#1836)
2627
- Fixed clarity for NetworkSceneManager client side notification when it receives a scene hash value that does not exist in its local hash table. (#1828)
2728
- Fixed client throws a key not found exception when it times out using UNet or UTP. (#1821)
2829
- Fixed network variable updates are no longer limited to 32,768 bytes when NetworkConfig.EnsureNetworkVariableLengthSafety is enabled. The limits are now determined by what the transport can send in a message. (#1811)

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

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,6 @@ internal void __endSendClientRpc(ref FastBufferWriter bufferWriter, uint rpcMeth
158158
// We check to see if we need to shortcut for the case where we are the host/server and we can send a clientRPC
159159
// to ourself. Sadly we have to figure that out from the list of clientIds :(
160160
bool shouldSendToHost = false;
161-
162161
if (clientRpcParams.Send.TargetClientIds != null)
163162
{
164163
foreach (var targetClientId in clientRpcParams.Send.TargetClientIds)
@@ -168,6 +167,12 @@ internal void __endSendClientRpc(ref FastBufferWriter bufferWriter, uint rpcMeth
168167
shouldSendToHost = true;
169168
break;
170169
}
170+
171+
// Check to make sure we are sending to only observers, if not log an error.
172+
if (NetworkManager.LogLevel >= LogLevel.Error && !NetworkObject.Observers.Contains(targetClientId))
173+
{
174+
NetworkLog.LogError(GenerateObserverErrorMessage(clientRpcParams, targetClientId));
175+
}
171176
}
172177

173178
rpcWriteSize = NetworkManager.SendMessage(ref clientRpcMessage, networkDelivery, in clientRpcParams.Send.TargetClientIds);
@@ -181,14 +186,29 @@ internal void __endSendClientRpc(ref FastBufferWriter bufferWriter, uint rpcMeth
181186
shouldSendToHost = true;
182187
break;
183188
}
189+
190+
// Check to make sure we are sending to only observers, if not log an error.
191+
if (NetworkManager.LogLevel >= LogLevel.Error && !NetworkObject.Observers.Contains(targetClientId))
192+
{
193+
NetworkLog.LogError(GenerateObserverErrorMessage(clientRpcParams, targetClientId));
194+
}
184195
}
185196

186197
rpcWriteSize = NetworkManager.SendMessage(ref clientRpcMessage, networkDelivery, clientRpcParams.Send.TargetClientIdsNativeArray.Value);
187198
}
188199
else
189200
{
190-
shouldSendToHost = IsHost;
191-
rpcWriteSize = NetworkManager.SendMessage(ref clientRpcMessage, networkDelivery, NetworkManager.ConnectedClientsIds);
201+
var observerEnumerator = NetworkObject.Observers.GetEnumerator();
202+
while (observerEnumerator.MoveNext())
203+
{
204+
// Skip over the host
205+
if (IsHost && observerEnumerator.Current == NetworkManager.LocalClientId)
206+
{
207+
shouldSendToHost = true;
208+
continue;
209+
}
210+
rpcWriteSize = NetworkManager.MessagingSystem.SendMessage(ref clientRpcMessage, networkDelivery, observerEnumerator.Current);
211+
}
192212
}
193213

194214
// If we are a server/host then we just no op and send to ourself
@@ -228,6 +248,12 @@ internal void __endSendClientRpc(ref FastBufferWriter bufferWriter, uint rpcMeth
228248
#endif
229249
}
230250

251+
internal string GenerateObserverErrorMessage(ClientRpcParams clientRpcParams, ulong targetClientId)
252+
{
253+
var containerNameHoldingId = clientRpcParams.Send.TargetClientIds != null ? nameof(ClientRpcParams.Send.TargetClientIds) : nameof(ClientRpcParams.Send.TargetClientIdsNativeArray);
254+
return $"Sending ClientRpc to non-observer! {containerNameHoldingId} contains clientId {targetClientId} that is not an observer!";
255+
}
256+
231257
/// <summary>
232258
/// Gets the NetworkManager that owns this NetworkBehaviour instance
233259
/// See note around `NetworkObject` for how there is a chicken / egg problem when we are not initialized
Lines changed: 317 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,317 @@
1+
using System.Collections;
2+
using System.Collections.Generic;
3+
using System.Linq;
4+
using NUnit.Framework;
5+
using UnityEngine;
6+
using UnityEngine.TestTools;
7+
using Unity.Netcode.TestHelpers.Runtime;
8+
using Unity.Netcode;
9+
using Unity.Collections;
10+
11+
namespace TestProject.RuntimeTests
12+
{
13+
/// <summary>
14+
/// Integration test to validate ClientRpcs will only
15+
/// send to observers of the NetworkObject
16+
/// </summary>
17+
[TestFixture(HostOrServer.Host)]
18+
[TestFixture(HostOrServer.Server)]
19+
public class RpcObserverTests : NetcodeIntegrationTest
20+
{
21+
protected override int NumberOfClients => 9;
22+
23+
private GameObject m_TestPrefab;
24+
25+
private GameObject m_ServerPrefabInstance;
26+
private RpcObserverObject m_ServerRpcObserverObject;
27+
28+
private NativeArray<ulong> m_NonObserverArrayError;
29+
private bool m_ArrayAllocated;
30+
31+
public RpcObserverTests(HostOrServer hostOrServer) : base(hostOrServer) { }
32+
33+
protected override void OnOneTimeSetup()
34+
{
35+
m_NetworkTransport = NetcodeIntegrationTestHelpers.InstanceTransport.UTP;
36+
}
37+
38+
protected override void OnServerAndClientsCreated()
39+
{
40+
m_TestPrefab = CreateNetworkObjectPrefab($"{nameof(RpcObserverObject)}");
41+
m_TestPrefab.AddComponent<RpcObserverObject>();
42+
}
43+
44+
protected override IEnumerator OnServerAndClientsConnected()
45+
{
46+
m_ServerPrefabInstance = SpawnObject(m_TestPrefab, m_ServerNetworkManager);
47+
m_ServerRpcObserverObject = m_ServerPrefabInstance.GetComponent<RpcObserverObject>();
48+
return base.OnServerAndClientsConnected();
49+
}
50+
51+
[UnityTest]
52+
public IEnumerator ClientRpcObserverTest()
53+
{
54+
// Wait for all clients to report they have spawned an instance of our test prefab
55+
yield return WaitForConditionOrTimeOut(m_ServerRpcObserverObject.AllClientsSpawned);
56+
57+
var nonObservers = new List<ulong>();
58+
59+
// We start out with all clients being observers of the test prefab
60+
// Test that all clients receive the RPC
61+
yield return RunRpcObserverTest(nonObservers);
62+
63+
// This hides the test prefab from one client and then runs the test
64+
// and repeats until all clients are no longer observers
65+
foreach (var clientId in m_ServerNetworkManager.ConnectedClientsIds)
66+
{
67+
if (clientId == m_ServerNetworkManager.LocalClientId)
68+
{
69+
continue;
70+
}
71+
// Hide it from the client
72+
m_ServerRpcObserverObject.NetworkObject.NetworkHide(clientId);
73+
nonObservers.Add(clientId);
74+
// Provide 1 tick for the client to hide the NetworkObject
75+
yield return s_DefaultWaitForTick;
76+
77+
// Run the test
78+
yield return RunRpcObserverTest(nonObservers);
79+
}
80+
81+
// ****** Verify that sending to non-observer(s) generates error ******
82+
var clientRpcParams = new ClientRpcParams();
83+
// Verify that we get an error message when we try to send to a non-observer using TargetClientIds
84+
clientRpcParams.Send.TargetClientIds = new List<ulong>() { nonObservers[0] };
85+
m_ServerNetworkManager.LogLevel = LogLevel.Error;
86+
LogAssert.Expect(LogType.Error, "[Netcode] " + m_ServerRpcObserverObject.GenerateObserverErrorMessage(clientRpcParams, nonObservers[0]));
87+
m_ServerRpcObserverObject.ObserverMessageClientRpc(clientRpcParams);
88+
yield return s_DefaultWaitForTick;
89+
90+
m_NonObserverArrayError = new NativeArray<ulong>(clientRpcParams.Send.TargetClientIds.ToArray(), Allocator.Persistent);
91+
m_ArrayAllocated = true;
92+
93+
// Now clean the TargetClientIds to prepare for the next TargetClientIdsNativeArray error check
94+
clientRpcParams.Send.TargetClientIds = null;
95+
96+
// Now verify that we get an error message when we try to send to a non-observer using TargetClientIdsNativeArray
97+
clientRpcParams.Send.TargetClientIdsNativeArray = m_NonObserverArrayError;
98+
LogAssert.Expect(LogType.Error, "[Netcode] " + m_ServerRpcObserverObject.GenerateObserverErrorMessage(clientRpcParams, nonObservers[0]));
99+
m_ServerRpcObserverObject.ObserverMessageClientRpc(clientRpcParams);
100+
yield return s_DefaultWaitForTick;
101+
102+
// Validate we can still just send to the host-client when no clients are connected
103+
if (m_UseHost)
104+
{
105+
m_ServerRpcObserverObject.ResetTest();
106+
107+
foreach (var clientId in nonObservers)
108+
{
109+
m_ServerNetworkManager.DisconnectClient(clientId);
110+
}
111+
112+
yield return s_DefaultWaitForTick;
113+
114+
m_ServerRpcObserverObject.ObserverMessageClientRpc();
115+
116+
yield return s_DefaultWaitForTick;
117+
118+
Assert.True(m_ServerRpcObserverObject.HostReceivedMessage, "Host failed to receive the ClientRpc when no clients were connected!");
119+
Assert.False(m_ServerRpcObserverObject.NonObserversReceivedRPC(nonObservers), $"Non-observers ({m_ServerRpcObserverObject.GetClientIdsAsString(nonObservers)}) received the RPC message!");
120+
}
121+
122+
m_ServerNetworkManager.LogLevel = LogLevel.Normal;
123+
}
124+
125+
/// <summary>
126+
/// This will send an RPC to all observers of the test prefab and check
127+
/// to see if the observer clients receive the message and also confirms
128+
/// that the non-observer clients did not receive the message.
129+
/// </summary>
130+
private IEnumerator RunRpcObserverTest(List<ulong> nonObservers)
131+
{
132+
m_ServerRpcObserverObject.ResetTest();
133+
134+
m_ServerRpcObserverObject.ObserverMessageClientRpc();
135+
136+
yield return WaitForConditionOrTimeOut(m_ServerRpcObserverObject.AllObserversReceivedRPC);
137+
Assert.False(s_GlobalTimeoutHelper.TimedOut, $"Timed out waiting for all clients to receive message!\n" +
138+
$"Clients that received the message:{m_ServerRpcObserverObject.GetClientIdsAsString()}");
139+
Assert.False(m_ServerRpcObserverObject.NonObserversReceivedRPC(nonObservers), $"Non-observers ({m_ServerRpcObserverObject.GetClientIdsAsString(nonObservers)}) received the RPC message!");
140+
141+
// Always verify the host received the RPC
142+
if (m_UseHost)
143+
{
144+
Assert.True(m_ServerRpcObserverObject.HostReceivedMessage, $"Host failed to receive the ClientRpc with the following observers: {m_ServerRpcObserverObject.GetClientIdsAsString()}!");
145+
}
146+
}
147+
148+
protected override IEnumerator OnTearDown()
149+
{
150+
// Make sure to dispose of the native array
151+
if (m_ArrayAllocated)
152+
{
153+
m_ArrayAllocated = false;
154+
m_NonObserverArrayError.Dispose();
155+
}
156+
return base.OnTearDown();
157+
}
158+
}
159+
160+
/// <summary>
161+
/// Test prefab component used with RpcObserverTests
162+
/// </summary>
163+
public class RpcObserverObject : NetworkBehaviour
164+
{
165+
public readonly List<ulong> ObserversThatReceivedRPC = new List<ulong>();
166+
167+
public static readonly List<ulong> ClientInstancesSpawned = new List<ulong>();
168+
169+
protected bool m_NotifyClientReceivedMessage;
170+
public bool HostReceivedMessage { get; internal set; }
171+
172+
public string GetClientIdsAsString(List<ulong> clientIds = null)
173+
{
174+
if (clientIds == null)
175+
{
176+
clientIds = ObserversThatReceivedRPC;
177+
}
178+
var clientIdsAsString = string.Empty;
179+
foreach (var clientId in clientIds)
180+
{
181+
clientIdsAsString += $"({clientId})";
182+
}
183+
return clientIdsAsString;
184+
}
185+
186+
/// <summary>
187+
/// Returns true if all connected clients have spawned the test prefab
188+
/// </summary>
189+
public bool AllClientsSpawned()
190+
{
191+
if (!IsServer)
192+
{
193+
return false;
194+
}
195+
196+
foreach (var clientId in NetworkManager.ConnectedClientsIds)
197+
{
198+
if (clientId == NetworkManager.LocalClientId)
199+
{
200+
continue;
201+
}
202+
203+
if (!ClientInstancesSpawned.Contains(clientId))
204+
{
205+
return false;
206+
}
207+
}
208+
return true;
209+
}
210+
211+
/// <summary>
212+
/// Returns true if all observers have received the RPC
213+
/// </summary>
214+
public bool AllObserversReceivedRPC()
215+
{
216+
if (!IsServer)
217+
{
218+
return false;
219+
}
220+
221+
foreach (var clientId in NetworkObject.Observers)
222+
{
223+
if (clientId == NetworkManager.LocalClientId)
224+
{
225+
continue;
226+
}
227+
if (!ObserversThatReceivedRPC.Contains(clientId))
228+
{
229+
return false;
230+
}
231+
}
232+
return true;
233+
}
234+
235+
/// <summary>
236+
/// Returns true if any clientId in the nonObservers list received the RPC
237+
/// </summary>
238+
/// <param name="nonObservers">list of clientIds that should not have received the RPC message</param>
239+
public bool NonObserversReceivedRPC(List<ulong> nonObservers)
240+
{
241+
foreach (var clientId in nonObservers)
242+
{
243+
// return false if a non-observer received the RPC
244+
if (ObserversThatReceivedRPC.Contains(clientId))
245+
{
246+
return true;
247+
}
248+
}
249+
return false;
250+
}
251+
252+
/// <summary>
253+
/// Clears the received
254+
/// </summary>
255+
public void ResetTest()
256+
{
257+
ObserversThatReceivedRPC.Clear();
258+
HostReceivedMessage = false;
259+
}
260+
261+
/// <summary>
262+
/// Called from server-host once per test run
263+
/// </summary>
264+
[ClientRpc]
265+
public void ObserverMessageClientRpc(ClientRpcParams clientRpcParams = default)
266+
{
267+
if (IsHost)
268+
{
269+
HostReceivedMessage = true;
270+
}
271+
else
272+
{
273+
m_NotifyClientReceivedMessage = true;
274+
}
275+
}
276+
277+
/// <summary>
278+
/// Called by each observer client that received the ObserverMessageClientRpc message
279+
/// The sender id is added to the ObserversThatReceivedRPC list
280+
/// </summary>
281+
[ServerRpc(RequireOwnership = false)]
282+
public void ObserverMessageServerRpc(ServerRpcParams serverRpcParams = default)
283+
{
284+
ObserversThatReceivedRPC.Add(serverRpcParams.Receive.SenderClientId);
285+
}
286+
287+
public override void OnNetworkSpawn()
288+
{
289+
if (IsClient)
290+
{
291+
ClientInstancesSpawned.Add(NetworkManager.LocalClientId);
292+
}
293+
}
294+
295+
public override void OnNetworkDespawn()
296+
{
297+
if (IsClient)
298+
{
299+
ClientInstancesSpawned.Remove(NetworkManager.LocalClientId);
300+
}
301+
}
302+
303+
private void Update()
304+
{
305+
if (IsServer)
306+
{
307+
return;
308+
}
309+
310+
if (m_NotifyClientReceivedMessage)
311+
{
312+
m_NotifyClientReceivedMessage = false;
313+
ObserverMessageServerRpc();
314+
}
315+
}
316+
}
317+
}

0 commit comments

Comments
 (0)