-
Notifications
You must be signed in to change notification settings - Fork 438
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
Conversation
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.
There was a problem hiding this 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); |
There was a problem hiding this comment.
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 :) )
There was a problem hiding this comment.
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.
com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ConnectionApprovedMessage.cs
Outdated
Show resolved
Hide resolved
{ | ||
if (m_UnderlyingTarget == null) | ||
{ | ||
if (behaviour.NetworkManager.ConnectionManager.PeerClientIds.Contains(NetworkManager.ServerClientId)) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs
Outdated
Show resolved
Hide resolved
- Replaced OnPeerConnectedCallback and OnPeerDisconnectCallback with unified OnConnectionEvent
- 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
There was a problem hiding this 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!
Awesome!
(Some minor nit-picky suggestions on very minor adjustments made.)
com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ConnectionApprovedMessage.cs
Show resolved
Hide resolved
com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ConnectionApprovedMessage.cs
Outdated
Show resolved
Hide resolved
com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs
Outdated
Show resolved
Hide resolved
com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs
Outdated
Show resolved
Hide resolved
com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs
Outdated
Show resolved
Hide resolved
@@ -62,7 +62,6 @@ public void MyNativeListServerRpc(NativeList<ulong> clientId, ServerRpcParams pa | |||
} | |||
#endif | |||
|
|||
|
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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. 😔
- Improved test coverage
VERY excited for this, thank you @ShadauxCat |
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
The first thing I noticed is that When I ran the SampleScene as Host with no clients and tried to use my new RPC, I got the following error:
Would it be better to open a bug ticket for this? |
I believe RuntimeAccessModifiersILPP is missing the following lines in the if condition in ProcessNetworkBehaviour()
|
@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. |
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 asNetworkManager.OnPeerConnectedCallback
andNetworkManager.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
Rpc
.Server
,NotServer
,Owner
,NotOwner
,Me
,NotMe
,ClientsAndHost
, andEveryone
. Runtime overrides are available for any of these targets, as well as for sending to a specific ID or groups of IDs.ServerRpc
andClientRpc
.ServerRpc
andClientRpc
remain in their existing forms for backward compatibility, butRpc
will be the recommended and most supported option.NetworkManager.OnConnectionEvent
as a unified connection event callback to notify clients and servers of all client connections and disconnections within the sessionNetworkManager.ServerIsHost
andNetworkBehaviour.ServerIsHost
to allow a client to tell if it is connected to a host or to a dedicated serverNetworkManager.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 modeTesting and Documentation