PretendoNetwork / nex-protocols-common-go

Reusable implementations of NEX methods found in many servers
GNU Affero General Public License v3.0
19 stars 8 forks source link

New nex-go #26

Closed jonbarrow closed 7 months ago

jonbarrow commented 1 year ago

Draft since lots of things are changing still, but wanted to get this into more peoples hands

The goal of this PR is to bring this library up to date with the new nex-go rewrite in the works https://github.com/PretendoNetwork/nex-go/pull/42. Eventually I would also like to refactor all the methods implemented here which assume a PID is a uint32. A PID is not always a uint32, it can be a uint64 on modern clients (the Switch)

Currently this library does still assume a client is a legacy client when reading PIDs, but we now have the ability to support modern clients here

jonbarrow commented 11 months ago

Most common implementations now use the new protocol interfaces. The exceptions to this are Secure Connection and Matchmaking

Secure Connection does technically use the interface, but there are places where it just always assumes a PRUDPServer and casts, which makes it not compatible with the HPPServer

Matchmaking does not use the interface at all, which I would like to fix at some point as it's now the only common implementation to not do it this way

It might get verbose, but we can make use of Go's type assertions to work around these casts. For example in Matchmaking

server.OnClientRemoved(func(client *nex.PRUDPClient) {
    fmt.Println("Leaving")
    common_globals.RemoveClientFromAllSessions(client)
})

Could become

if server, ok := server.(*nex.PRUDPServer); ok {
    server.OnClientRemoved(func(client *nex.PRUDPClient) {
        fmt.Println("Leaving")
        common_globals.RemoveClientFromAllSessions(client)
    })
}

So that the OnClientRemoved call only happens if server asserts to a PRUDPServer. We would need many of these assertions though, for both the server and the client interfaces

DaniElectra commented 11 months ago

I think we should be fine right now by assigning the interfaces since HPP isn't expected to work with this. Once we implement websockets, we can remove the casts and use a common interface for PRUDP and websockets for these cases

DaniElectra commented 11 months ago

btw I have noticed that there is some inconsistent naming for the matchmaking protocols. For example, on nex-protocols-go we use match-making and match-making-ext but on the common protocols they are named matchmaking and matchmaking-ext. I think we should rename one of them for better consistency

jonbarrow commented 11 months ago

I think we should be fine right now by assigning the interfaces since HPP isn't expected to work with this

I would like at some point to make HPP compatible, since it's my understanding that HPP is supposed to be a drop-in replacement for NEX, just over HTTP. Allowing for access to all protocols/methods normally accessible in the NEX server. But I agree that it's fine for right now. Our use case does not require this, it's mostly a "something nice we should do in the future" thing

btw I have noticed that there is some inconsistent naming for the matchmaking protocols. For example, on nex-protocols-go we use match-making and match-making-ext but on the common protocols they are named matchmaking and matchmaking-ext. I think we should rename one of them for better consistency

I agree. I had not noticed this. I'll update common since the protocols lib is correct

DaniElectra commented 11 months ago

I'm thinking if we should rename the common protocol creation methods too, like for the raw protocols where we changed them to NewProtocol. This is an example for creating a common secure protocol:

secureProtocol := secure.NewProtocol(globals.SecureServer)
common_secure.NewCommonSecureConnectionProtocol(secureProtocol)

Maybe we could rename them to something like NewCommonProtocol to simplify?

secureProtocol := secure.NewProtocol(globals.SecureServer)
common_secure.NewCommonProtocol(secureProtocol)
jonbarrow commented 11 months ago

Yes that looks a lot better imo

jonbarrow commented 10 months ago

@DaniElectra I updated everything to support the new type system. It's basically untested though, just did enough to get Go to stop showing errors/warnings

HPP support also needs to be re-added. Right now it assumes a PRUDPConnection. In most cases though this is just to remove a type assertion for the server variable, because there's TONS of places where we just do blind assertions without checking (which WILL lead to panics if the wrong client interacts with some protocols)

A solution to the PRUDPConnection assumption would be to just check the type of packet.Sender(). If it's a PRUDPConnection then use the connections endpoint to get the server like it is now, otherwise if it's a HPPClient use the server on the client