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

Implement IPv6 support for server and direct connect #17571

Closed
wants to merge 1 commit into from

Conversation

jrb0001
Copy link
Contributor

@jrb0001 jrb0001 commented Jan 8, 2020

No description provided.

Copy link
Member

@pchote pchote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quick initial review as requested. It will be a while before I can put time into thinking about the potential implications of the changes here.

OpenRA.Game/Game.cs Outdated Show resolved Hide resolved
OpenRA.Game/Game.cs Outdated Show resolved Hide resolved
OpenRA.Game/Game.cs Outdated Show resolved Hide resolved
OpenRA.Game/Game.cs Outdated Show resolved Hide resolved
OpenRA.Game/Network/Connection.cs Outdated Show resolved Hide resolved
OpenRA.Game/Network/Connection.cs Outdated Show resolved Hide resolved
OpenRA.Game/Network/Connection.cs Outdated Show resolved Hide resolved
OpenRA.Game/Network/Connection.cs Show resolved Hide resolved
OpenRA.Game/Server/Server.cs Outdated Show resolved Hide resolved
OpenRA.Game/Server/Server.cs Outdated Show resolved Hide resolved
@teinarss
Copy link
Contributor

teinarss commented Jan 9, 2020

@jrb0001
Copy link
Contributor Author

jrb0001 commented Jan 9, 2020

Not all OSes support it. Also multiple sockets are much more convenient if you want explicit bind addresses. I decided against adding an option for overriding the bind addresses in this PR because that would need fundamental announcement system changes which are out of scope.

Copy link
Member

@pchote pchote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent some time poking at this tonight, sorry for the long delay.

A few crashes and minor style nits:

OpenRA.Game/Server/Server.cs Show resolved Hide resolved
OpenRA.Game/Network/Connection.cs Outdated Show resolved Hide resolved
OpenRA.Game/Network/Connection.cs Outdated Show resolved Hide resolved
OpenRA.Game/Support/LaunchArguments.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Widgets/Logic/ConnectionLogic.cs Outdated Show resolved Hide resolved
OpenRA.Game/Network/Connection.cs Outdated Show resolved Hide resolved
OpenRA.Game/Network/Connection.cs Outdated Show resolved Hide resolved
OpenRA.Game/Network/Connection.cs Outdated Show resolved Hide resolved
OpenRA.Game/Server/Server.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Widgets/Logic/ConnectionLogic.cs Outdated Show resolved Hide resolved
@pchote
Copy link
Member

pchote commented Feb 23, 2020

The connection failed dialog now repeats itself:

Screenshot 2020-02-23 at 16 25 35

Screenshot 2020-02-23 at 16 31 23

@pchote
Copy link
Member

pchote commented Feb 23, 2020

Everything else seems to work as expected, so I expect 👍 once the issues above are resolved.

@jrb0001
Copy link
Contributor Author

jrb0001 commented Feb 25, 2020

Fixed everything except the error dialog:

  • I don't see a way to remove the redundancy without removing a line from the dialog. @pchote doesn't want that (discussion on IRC).
  • Reverting back to the old error handling isn't an option for me. The lines on the dialog are still redundant and the generic "Server is not responding" is just wrong in many different cases.

@pchote
Copy link
Member

pchote commented Feb 25, 2020

As I said on IRC, my issue with changing the error dialog in this PR is:

  • The error messages is that the screenshots above, which cover the two most common cases that players see, is a step backwards in terms of polish.
  • The connection dialog implementation is old and fragile, and I am not prepared to risk regressions given the short timescale we have to branch and ship the playtest.

It seems silly to delay IPv6 support over an unrelated scope creep, but if you're not willing for us to change this I don't see much other choice. Merging such a big feature without time to bake on bleed would probably not have been wise anyway.

Moving to next + 1 gives this more time to bake and resolves my second point so please go ahead.

@pchote pchote modified the milestones: Next Release, Next+1 Feb 25, 2020
@pchote
Copy link
Member

pchote commented Mar 18, 2020

ping @jrb0001.

@pchote
Copy link
Member

pchote commented Apr 11, 2020

@jrb0001 do you mind if I take over this and push to your branch to update this PR?

I would like to get this merged before it becomes too late in the Next + 1 cycle.

@jrb0001 jrb0001 mentioned this pull request Apr 11, 2020
@jrb0001
Copy link
Contributor Author

jrb0001 commented Apr 11, 2020

@pchote I don't think I will find time and motivation to work on it in the near future so go ahead.

@pchote
Copy link
Member

pchote commented May 2, 2020

@jrb0001 can you please check the "Allow edits by maintainers" checkbox at the bottom of the right-hand sidebar? Your repo is currently rejecting my ability to update this PR.

@jrb0001
Copy link
Contributor Author

jrb0001 commented May 3, 2020

@pchote That checkbox is enabled, no idea why it would reject your push.

@pchote
Copy link
Member

pchote commented May 3, 2020

paul@Ragnarok:OpenRA+ipv6-server$ git push jrb0001 +ipv6-server 
ERROR: Permission to jrb0001/OpenRA.git denied to pchote.
fatal: Could not read from remote repository.

Probably easiest at this point to file a new pr, so i'll close this one.

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

Successfully merging this pull request may close these issues.

None yet

4 participants