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

Refactor SlippiNetplayClient to enable attempting connections to multiple endpoints per player #389

Open
wants to merge 4 commits into
base: slippi
Choose a base branch
from

Conversation

jmlee337
Copy link
Contributor

(at this time, the only possible endpoints are external and local)

Because this rewrites much of the connection logic, I tested this in the following cases

  • "normal" singles
  • singles via LAN
  • singles vs symmetric NAT
  • doubles (2 LAN players) vs double symmetric NAT (2 LAN players)

In the immediate term this will fix connections for the case where

  • multiple players are connected to the same VPN AND
  • the selected local address is not correct (due to the VPN client not appearing as a network device)

Before #387 is merged this will also fix connections for the case where

In the future there is the possibility of using this functionality to

  • improve connections for players who are behind the same CGNAT/double NAT
  • enable integrated relay servers which would be deployable by anyone, replace community VPNs, and be easier for both users and administrators

…e applicable

tested
singles
singles via LAN
singles vs symmetric NAT
doubles vs double symmetric NAT
this is necessary because early disconnects are possible (in case of network failures, physical disconnect, etc)
@JLaferri
Copy link
Member

Oh boy. Yeah this changes a lot of critical things. Some of the stuff that got changed were things that I added because there were issues with people disconnecting from each other 30 seconds into a match, for example.

Would be good to do some decent testing and also have a decent revert plan if something breaks.

@jmlee337
Copy link
Contributor Author

I'll have to think more about a testing plan. The main problem I'm thinking of is that this PR alone doesn't offer any improved functionality to entice testers (in 99.9% of cases).

In the meantime can you tell me about the 30 second disconnect? What parts of the original code address that? I can at least try to make sure there's no regressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants