Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Added a universal RPC attribute [MTT-7482] [MTT-7578] [MTT-7579] [MTT-7580] [MTT-7581] #2762

Merged
merged 16 commits into from
Nov 27, 2023

Conversation

ShadauxCat
Copy link
Collaborator

@ShadauxCat ShadauxCat commented Nov 14, 2023

The new attribute is more configurable at both compile time and runtime than the existing ClientRpc and ServerRpc attributes, and also includes support for clients sending to themselves, and for optionally delaying the invocation of local RPCs until the start of the next frame to enable mutually recursive client/server RPCs to function on a host the same as on other clients (as well as mutually recursive Owner/NotOwner RPCs and other similar patterns).

This attribute supersedes ClientRpc and ServerRpc and will be the new default recommendation for creating RPCs. ClientRpc and ServerRpc will eventually be deprecated in favor of the new [Rpc] attribute.

By necessity of the feature, this also adds NetworkManager.PeerClientIds to allow clients to know the IDs of other connected clients, as well as NetworkManager.OnPeerConnectedCallback and NetworkManager.OnPeerDisconnectCallback events that are fired when this list is updated via messages from the server.

resolves #2718
resolves #2615
resolves #2504
resolves #2465
resolves #2448
resolves #2331
resolves #2326

Changelog

  • Added: Added a new RPC attribute, which is simply Rpc.
    • This is a generic attribute that can perform the functions of both Server and Client RPCs, as well as enabling client-to-client RPCs. Includes several default targets: Server, NotServer, Owner, NotOwner, Me, NotMe, ClientsAndHost, and Everyone. Runtime overrides are available for any of these targets, as well as for sending to a specific ID or groups of IDs.
    • This attribute also includes the ability to defer RPCs that are sent to the local process to the start of the next frame instead of executing them immediately, treating them as if they had gone across the network. The default behavior is to execute immediately.
    • This attribute effectively replaces ServerRpc and ClientRpc. ServerRpc and ClientRpc remain in their existing forms for backward compatibility, but Rpc will be the recommended and most supported option.
  • Added: NetworkManager.OnConnectionEvent as a unified connection event callback to notify clients and servers of all client connections and disconnections within the session
  • Added: NetworkManager.ServerIsHost and NetworkBehaviour.ServerIsHost to allow a client to tell if it is connected to a host or to a dedicated server
  • Changed: NetworkManager.ConnectedClientsIds is now accessible on the client side and will contain the list of all clients in the session, including the host client if the server is operating in host mode

Testing and Documentation

  • Includes unit tests.
  • Includes integration tests.
  • Includes documentation for previously-undocumented public API entry points.

The new attribute is more configurable at both compile time and runtime than the existing ClientRpc and ServerRpc attributes, and also includes support for clients sending to themselves, and for optionally delaying the invocation of local RPCs until the start of the next frame to enable mutually recursive client/server RPCs to function on a host the same as on other clients (as well as mutually recursive Owner/NotOwner RPCs and other similar patterns).

This attribute supersedes ClientRpc and ServerRpc and will be the new default recommendation for creating RPCs. ClientRpc and ServerRpc will eventually be deprecated in favor of the new [Rpc] attribute.
@ShadauxCat ShadauxCat requested a review from a team as a code owner November 14, 2023 19:29
@ShadauxCat ShadauxCat changed the title feat: Added a universal RPC attribute feat: Added a universal RPC attribute [MTT-7482] [MTT-7578] [MTT-7579] [MTT-7580] [MTT-7581] Nov 14, 2023
Copy link
Contributor

@samlucas-unity samlucas-unity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the comments are things which need changing. Mostly asking for context or minor suggestions.

@@ -113,14 +113,14 @@ public virtual void ProcessTriggers(IDeferredNetworkMessageManager.TriggerType t
// processed before the object is fully spawned. This must be the last thing done in the spawn process.
if (triggers.TryGetValue(key, out var triggerInfo))
{
triggers.Remove(key);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the move? (for context not that I think it shouldn't move :) )

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the handler for a trigger tried to add a new trigger of the same type, the Remove() after executing the trigger would just delete it and the trigger would never actually get executed.

{
if (m_UnderlyingTarget == null)
{
if (behaviour.NetworkManager.ConnectionManager.PeerClientIds.Contains(NetworkManager.ServerClientId))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whats the difference here between Everyone and NotServer. Is the default to not send to self? Why does Everyone not exclude the local client (NotMe)? Oh is this for a host game then so in a dedicated server the ServerClientId will be null or is it just the server id. Then this check if the server counts as a peer as in a host game so in that case the server is a client Oh its in the name ClientAndHost, so this is only to Clients and in a locally hosted game (not dedicated server) that host too.

Its clear now, maybe slap a comment on the if check to say its checking for host session?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, if the server is a host, then ClientsAndHost means everyone. If the server is a dedicated server, then the server gets excluded. This is basically "send to everyone that runs any kind of client logic", which hosts do but dedicated servers don't.

- This is a generic attribute that can perform the functions of both Server and Client RPCs, as well as enabling client-to-client RPCs. Includes several default targets: `Server`, `NotServer`, `Owner`, `NotOwner`, `Me`, `NotMe`, `ClientsAndHost`, and `Everyone`. Runtime overrides are available for any of these targets, as well as for sending to a specific ID or groups of IDs.
- This attribute also includes the ability to defer RPCs that are sent to the local process to the start of the next frame instead of executing them immediately, treating them as if they had gone across the network. The default behavior is to execute immediately.
- This attribute effectively replaces `ServerRpc` and `ClientRpc`. `ServerRpc` and `ClientRpc` remain in their existing forms for backward compatibility, but `Rpc` will be the recommended and most supported option.
- Added `NetworkManager.OnConnectionEvent` as a unified connection event callback to notify clients and servers of all client connections and disconnections within the session (#2762)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:godmode:
❤️

Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks Good To Me!
:godmode:
Awesome!
(Some minor nit-picky suggestions on very minor adjustments made.)

@@ -62,7 +62,6 @@ public void MyNativeListServerRpc(NativeList<ulong> clientId, ServerRpcParams pa
}
#endif


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could remove the file from this PR by adding the CR/LF back. (nit-picky of me)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I save it in Rider, it just removes the CR/LF again. 😔

@ShadauxCat ShadauxCat merged commit 39d0a2e into develop Nov 27, 2023
@ShadauxCat ShadauxCat deleted the feat/universal_rpc_attribute branch November 27, 2023 21:38
@zachstronaut
Copy link

VERY excited for this, thank you @ShadauxCat

@zachstronaut
Copy link

I wanted to test this, so opened this repository's testproject in Unity 2022.3.10f1 on Windows.

I tried adding a generic [Rpc] to GrabbableBall.cs

    [Rpc(SendTo.Everyone)]
    private void TestRpc()
    {
        Debug.Log("TestRpc");
    }

The first thing I noticed is that [Rpc] without arguments is not available.

When I ran the SampleScene as Host with no clients and tried to use my new RPC, I got the following error:

MethodAccessException: Method `Unity.Netcode.NetworkBehaviour.__beginSendRpc(uint,Unity.Netcode.RpcParams,Unity.Netcode.RpcAttribute/RpcAttributeParams,Unity.Netcode.SendTo,Unity.Netcode.RpcDelivery)' is inaccessible from method `GrabbableBall.TestRpc()'
GrabbableBall.TestRpc () (at Assets/Scripts/GrabbableBall.cs:113)
GrabbableBall.Update () (at Assets/Scripts/GrabbableBall.cs:64)

Would it be better to open a bug ticket for this?

@zachstronaut
Copy link

I believe RuntimeAccessModifiersILPP is missing the following lines in the if condition in ProcessNetworkBehaviour()

methodDefinition.Name == nameof(NetworkBehaviour.__beginSendRpc) ||
methodDefinition.Name == nameof(NetworkBehaviour.__endSendRpc) ||

@ShadauxCat
Copy link
Collaborator Author

@zachstronaut You are spot on. I'll get a PR with that fix open tomorrow. Thanks for catching that.

I'm also going to be integrating this into BossRoom tomorrow, which should help catch any other issues like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment