ClientRPCs invoked within other ClientRPCs are only executed on the host player

[RikuTheFuffs]

Description

ClientRPCs invoked within other ClientRPCs are only executed on the host player.

The code below calls a ClientRpc nested within another ClientRpc. It makes sure that only the host calls the nested Rpc, because only the server can do that:

private void Update()
   {
       if (Input.GetKeyDown(KeyCode.T))
       {
           TestNestedRPCCall();
       }
   }

   private void TestNestedRPCCall()
   {
       Debug.LogError("TestNestedRPCCall");
       FirstMethodClientRpc();
   }

   [ClientRpc]
   private void FirstMethodClientRpc()
   {
       Debug.LogError("FirstMethodClientRpc");
       if (IsHost)
           SecondMethodClientRpc();
   }

   [ClientRpc]
   private void SecondMethodClientRpc()
   {
       Debug.LogError("SecondMethodClientRpc");
   }

This will result in logs:

• On Host: TestNestedRPCCall --> FirstMethodClientRpc --> SecondMethodClientRpc
• On Client: FirstMethodClientRpc

Solution I'd like

If it is intended to work like this, I would really like there to be a warning for this, as it is easily missed in more complex code. Otherwise, I'd expect the SecondMethodClientRpc() to be executed on all clients as well.

Additional context:

from @fatimah-f :

"when NGO invokes an Rpc, __RpcExecStage is set to either Server or Client meaning the execution context is from "server's eyes" or "client's eyes". there's a similar code running locally too when user invokes an Rpc, and during that if RpcExecStage is not set to None (in other words, if it's either Server or Client instead of None), it treats it like a local NGO -> RPC execution, not like User -> RPC execution.
A potential solution would be to have 2 __RpcExecStage defined: incoming and outgoing — instead of storing the stage on 1 variable. that way, while incoming stage would be set to .Server, outgoing would still be set to .None and the nested RPC call would take outgoing stage into consideration to either send the Rpc call over the network, or execute its body, or skip the function call."

[zachstronaut]

Got burned by this today! Glad I thought to look in github issues. I don't think there's a warning about it in the docs?

[zachstronaut]

One fix is to introduce another ServerRpc that itself just calls your second ClientRpc you were hoping to execute... verbose and a nuisance, but easy enough of a fix.

[zachstronaut]

@RikuTheFuffs this ticket seems like a bug actually not a feature, especially since there's no documentation for this problem and it results in unexpected and inconsistent execution. I'm wonder if this ticket hasn't been triaged since Nov 2022 because it is in the feature category?

[NoelStephensUnity]

@zachstronaut
The ClientRpc documentation does make mention of only the server invoking a ClientRpc and does make mention of the special case where a host invoking a ClientRpc as a broadcast will also invoke the ClientRpc locally. However, I see several places where we could improve the text copy of that documentation and make a very direct statement that:
"A client remote procedure call (ClientRpc) can only be invoked by a server. When running as a host, since the host is both a client and a server, invoking a ClientRpc without specifying the targeted clients (i.e. a broadcast to all clients) will also result in the ClientRpc being invoked locally on the host."

As a side note:
Remote procedure calls were a highly discussed/debated topic in the early MLAPI/NGO days regarding the original implementation (MLAPI) and whether any adjustments should be made. The final deciding factor was to keep the remote procedure call invocation rules true to their widely accepted definition where they are a procedure/method that is executed on another computer or network. In the context of a "client" remote procedure call, it didn't make sense to allow one to invoke it locally on a client instance but it did make sense to allow a "host" invoke it locally since it was both a server and client instance. (Microsoft even follows this pattern)

With that all said...I 100% completely understand how previous implementations (some more current with different engine technologies) allowed one to invoke an RPC locally and that it can be a more convenient way to "defer" the invocation of a method much like one can by creating a coroutine that simply invokes another method at a later time during the current frame or even at some point in the next frame (depending upon how long you yield). Just invoking an RPC in a "generic sense" without having to worry about the context (i.e. is this a server, client, or a host?) is much simpler than having to make a coroutine method that invokes another common method that both the coroutine and an RPC invoke and then having to determine if you need to start the coroutine or if you can just invoke the RPC itself (just typing that made my fingers twitch at the complexity over just invoking an "RPC" locally).

So, previous decisions made are the "technology debt" we are sorting through. In the next update (v1.7.0), you will see a several issues resolved that when you look at the "final PR" it seems like an easy fix but if you trace back the history of changes, that led to those relatively smaller sets of adjustments, you will see a larger set of changes that happened over a longer period of time. As an example, turning domain reloading off was something that couldn't "just be fixed" when we first started hearing about the pain associated with not being able to turn domain reloading off...but...over time...many things were adjusted such that PR #2720 ended up being the last few items that needed adjusting in order to be able to turn domain reloading off.

With all of that being said... 😸
We are constantly looking at "close targets" and "final destinations" while trying to map a path to achieve both. Currently, there is still an outstanding issue with RPC invocation that can end up in an infinite/recursive invocation loop (i.e. a host invoking a ClientRpc that invokes a ServerRpc that invokes the same ClientRpc). Having a more "generic" RPC that is defined by an attribute that has attribute properties that further define who can invoke the RPC coupled with an RPC invocation queue that defers the invocation (away from the current callstack) is something we have definitely been contemplating.

For now, all I can say is we are aware of these feature requests and are constantly reviewing NGO's evolution along other future netcode strategies in hopes that we can continue to provide you an improved netcode development experience... sometimes we get pulled in many directions...so certain areas might seem like there is little to no progress.

While I can't provide you with an ETA on when a "better RPC approach" will be available... I can say that "we do hear you" and we are driving towards constantly improving the netcode development experience. 👍

[zachstronaut]

@NoelStephensUnity Thanks for the lengthy reply on RPCs and roadmaps. Related, have you seen: #2718

[NoelStephensUnity]

Related, have you seen: #2718

Just reviewed over it and most of what has been discussed internally regarding RPC adjustments would be along those lines.
ETA for these updates could be a few updates from now...but it is something being mulled over currently.