-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Netplay: Add support for IPv6 #13201
base: master
Are you sure you want to change the base?
Conversation
7c6535d
to
715f770
Compare
715f770
to
0d03ddf
Compare
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.
By the second comment I realized that I should probably just ask as a general question: Is this still ok with IPv4 systems? My ISP still hasn't rolled out IPv6 to me, and while my Windows is capable of it, I simply have it disabled because I don't have anything else to talk v6 to.
That said, if something (either ENet or the socket APIs directly; specifically the sock_*
vs. sock6_*
stuff that isn't from ENet) handles this for us and we don't have to worry about it; just ignore my review comments and move on 👍
@@ -18,7 +18,7 @@ void WakeupThread(ENetHost* host) | |||
address.port = host->address.port; | |||
else | |||
enet_socket_get_address(host->socket, &address); | |||
address.host = 0x0100007f; // localhost | |||
address.host = in6addr_loopback; // localhost or ::1 |
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.
I see the 6 in there, but I'm not sure if this is an IPv4 or IPv6 address in the end (vs. it simply being an IPv6-capable oblivious thing, no matter what is used in the end). Is this smart enough to do the right thing on a system that doesn't have IPv6 (and also, on a system that only has IPv6, if those exist)?
sockaddr_in6 sin6 = {}; | ||
|
||
sin.sin_family = AF_INET; | ||
sin.sin_port = ENET_HOST_TO_NET_16(peer->host->address.port); | ||
sin.sin_addr.s_addr = peer->host->address.host; | ||
sin6.sin6_family = AF_INET6; | ||
sin6.sin6_port = ENET_HOST_TO_NET_16(peer->host->address.port); | ||
memcpy(&sin6.sin6_addr, &peer->host->address.host, sizeof(struct in6_addr)); |
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.
Does this still work fine for IPv4?
@BhaaLseN I think you're right that this might not work on systems/ISPs that only support IPv4. hmm. tbf I was doomed to begin with because this fork of enet also handles everything in IPv6. Just for the sake of it, since you have IPv6 disabled, could you test out the build (from the CI artifact for your platform) on your system? |
Honestly, the right way to do this would be to rip out Dophins networking stack and replace enet with something else, like webrtc or https://github.com/ValveSoftware/GameNetworkingSockets maybe? |
Yeah, I didn't have time to try it out until now, and it can't look up the central server for traversal. Seems this is now IPv6 only, and not really a good fit at a glance with IPv4 still out there. |
This PR swaps out enet with a fork of enet that supports IPv6. No UI changes are done.
This is an experiment. I've only tested it on Linux (both direct connection and traversal server).
I've tried out two forks of enet:
enet6 has API changes (adds an address type to functions that accepts an EnetAddress), while freeminer/enet handles everything in IPv6 (thus, IPv4 too via IPv4 mapped IPv6 address). I felt like freeminer/enet was much cleaner and thus this PR uses freeminer/enet.
(contact: snooze6214 on discord)