Connect/disconnect Flow Updates

[ShadauxCat]

See the linked RFC document →

RFC proposing improvements to the user ability to react to connect and disconnect events, as well as exposing the existence of a client's peers and connect/disconnect events for peers.

[SamuelBellomo]

It'd be awesome if we could have the ability to provide a payload with our responses.
Right now in Boss Room, we have this very ugly workaround with a custom message to give clients a reason for disconnection.
https://github.com/Unity-Technologies/com.unity.multiplayer.samples.coop/blob/9960a59586a521cf15cb8b477b2aa1c7370f3e13/Assets/BossRoom/Scripts/Server/Net/ServerGameNetPortal.cs#L292
https://github.com/Unity-Technologies/com.unity.multiplayer.samples.coop/blob/9960a59586a521cf15cb8b477b2aa1c7370f3e13/Assets/BossRoom/Scripts/Shared/Net/GameNetPortal.cs#L255

Do you think this future possibility will be in for our fall release?

[ShadauxCat]

I don't think it'd be difficult to add this for the fall release. I was trying to keep this RFC a little slimmer and not try to spec out the ways a client could get willfully disconnected from the server, but if it's valuable to add now, I'm on board with adding that functionality to this RFC. I can't speak to scheduling of implementation though, of course. :)

I'll update the RFC to add the ability for the disconnect to carry a custom UserData buffer that can be passed opaquely into the callback. Do you think that would solve your need here? Something like void Kick(ulong clientId, DisconnectReason reason, NetworkBuffer userDataBuffer = null) - or is there a way that the third parameter could be an INetworkSerializable and get properly constructed on the other end without the user having to deal with buffers directly? (I'm not sure if INetworkSerializables pass any type information with them that could be used to construct it on the other side in a generic way or not.)

[SamuelBellomo]

ok ok, it's really up to you. I just wanted to make sure this didn't fall into cracks :)
we have multiple ways to handle that serialization (that we really should make unique) You can look into how netvars currently do it. Thing is, there's plans with snapshot to look at serialization, you might want to have a quick sync with @jeffreyrainy and @LukeStampfli
Do we need type information though? Both client and server would be implemented by the same team, the person implementing the client would know what data type the server sends in their own custom payload and would be able to deserialize using the appropriate type, no?

[ShadauxCat]

The question I had here was, could we make something like

OnPeerDisconnectedCallback(DisconnectReason reason, INetworkSerializable payload)

And have payload get deserialized 'magically' by the SDK code, which wouldn't know what type payload is expected to be? I'm guessing probably not - which is why I proposed using something like NetworkBuffer, but I do feel that NetworkBuffer is a bit more inaccessible to less experienced teams, so if there was a way to make the payload magically deserialize to the correct type, it'd be a better developer experience.

In this case, unlike RPCs and NetVars, we don't necessarily have any shared understanding on both sides at compile time of what that type will be, due to the generic nature of the callback type signature.

[LukeStampfli]

We currently do not support typed serialization and I don't think it is a feature which we can just quickly add to MLAPI.

What we could do is use generics for this. The API would then look like the following:

OnPeerDisconnectedCallback(DisconnectReason reason, Payload payload)

Payload would be a struct which a) exposes a NetworkBuffer property for direct network buffer access and b) it will have a As<T> function to convert the payload automagically into the right type. So for instance:
payload.As<MyCustomSerializable>(); would extract the payload as a MyCustomSerializable.

The only restriction here would be that the sides writing and reading the payload must use the same generic type. But I'd argue that is something that we want because it is much cleaner then typed serialization.

[ShadauxCat]

100% agree, typed serialization is a little scary. Generics is a better option, gets the ability to use INetworkSerializables so it's accessible but it doesn't add the bandwidth and CPU overhead of sending and decoding type info.

[SamuelBellomo]

I'm wondering if we could get inspired by HTTP error codes here.
for example, they have 4xx error codes which are client errors (asking for the wrong things or the wrong way) --> 400 is malformed request, 401 is unauthorized (where auth failed), 403 is when auth worked, but the user is forbidden from accessing, 404 for ressource not found, etc.
Same for 5xx error codes. We could have 500 for a reason where the server had an exception and the client couldn't connect (if there's any exceptions thrown server side during connection approval for example). Or 503 when the server was supposed to be there, but unavailable, signalling to the client to try again later (maybe using exponential backoff for example)
I don't think they are all applicable, but they might be a great inspiration for an exhaustive list of reasons?

[ShadauxCat]

I think that's valuable. I think I would prefer, personally, to still see them represented as an enum, but having the enum values logically grouped would help with debugging, and we could follow the same prefix as HTTP just to make it more familiar. 400-499 is something wrong with the request (not authorized, malformed connect request, version mismatch, etc), 500-599 is something wrong with the request handler (unhandled exception in authorization callback, time out, etc).

Then we could allow clients to extend it by providing 600-699 as user error codes (leaving us free to expand the ones we provide in 400-599 without any conflicts)

[SamuelBellomo]

oh you're right, I wasn't suggesting we use ints for these error codes. I don't really mind ints vs enums.
It's mostly about the list of errors we can get inspiration from. HTTP has been established for a while, it has a pretty good list of use cases we might want to express too.
Good idea on the user extensible values.

[SamuelBellomo]

Connection approval isn't necessarily about authorization though. You could have a game check to make sure you don't have too many players of a certain type or coming from a certain region (for whatever reason). That wouldn't be "authorization"

[ShadauxCat]

True, in line with the comment about HTTP-based error codes above, we can change the connection approval handler to return a reason for not approving. Possibly also integrate the user data as well with that.

[LukeStampfli]

I'm not sure whether I like the the OnPeerConnected/Disconnected callbacks. The reason for this is because it breaks the enclosure principles of the star topology (a client only knows about the server but not about other clients). While for many games this might be fine, I can see some cases where this would be not desired, that's why I think we should consider to not add this as a core library feature. The problems I see with this are:

• In some larger scale games like Battle Royale or MMOs this would not be desired because a) it adds more bandwidht. b) it allows for cheats / exploits.
• This is not a complete feature. If a new client joins it won't know the list of peers which are already connected. It will just get events for newly connecting/disconnecting peers.

I suggest that if we want to implement this for now, we would rather do it in a high level component (NetworkBehaviour). I don't see a good reason why that's not the better approach. We can do something along the lines of:

public class PeerConnectionHandler : NetworkBehaviour
{
    public OnPeerConnect OnPeerConnectCallback;

    private override void NetworkStart()
    {
        if(IsServer)
        {
            NetworkManager.ClientConnectedCallback += OnClientConnected
        }
    }

    private void OnClientConnected(int clientId)
    {
        Assert.IsTrue(IsServer)
        OnClientConnectedClientRpc(clientId);
    }
    
    [ClientRpc]
    private void OnClientConnectedClientRpc(int clientId)
    {
        OnPeerConnectCallback.Invoke(clientId);
    }
}

And then can tell people to drop this into their scene on a NetworkObject if they want peer connected events.

[ShadauxCat]

This is not a complete feature. If a new client joins it won't know the list of peers which are already connected. It will just get events for newly connecting/disconnecting peers.

Actually the RFC does account for this:

When a new client connects, a CLIENT_CONNECTED message with that clients ID is broadcast to all other connected clients to inform them of the new peer, and when the CONNECTION_APPROVED command is sent to the new peer, a list of existing connected client IDs is included.

Though I can see how that's easy to overlook, and I did neglect to include a parameter in the callback to actually provide that information to the user.

I think the idea of putting this in an optional NetworkBehaviour is good though.