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

LAN games discovery #8906 #13132

Merged
merged 1 commit into from May 14, 2017

Conversation

Projects
None yet
4 participants
@rob-v
Contributor

rob-v commented Apr 15, 2017

@rob-v rob-v referenced this pull request Apr 15, 2017

Closed

LAN discovery #8906

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Apr 15, 2017

Contributor

Updated - LAN game advertisement moved to MasterServerPinger, works also for Started games.

Contributor

rob-v commented Apr 15, 2017

Updated - LAN game advertisement moved to MasterServerPinger, works also for Started games.

@atlimit8

This comment has been minimized.

Show comment
Hide comment
@atlimit8

atlimit8 Apr 16, 2017

Member

This PR fails continuous integration tests. I don't think it will compile for others. Try making a clean clone of your branch in a different location and building that.

See https://travis-ci.org/OpenRA/OpenRA/builds/222360145 and https://ci.appveyor.com/project/OpenRA/openra/build/1.0.15108

Member

atlimit8 commented Apr 16, 2017

This PR fails continuous integration tests. I don't think it will compile for others. Try making a clean clone of your branch in a different location and building that.

See https://travis-ci.org/OpenRA/OpenRA/builds/222360145 and https://ci.appveyor.com/project/OpenRA/openra/build/1.0.15108

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Apr 16, 2017

Contributor

@atlimit8 Can it be related to new .dll? Phrohdoh created issue for that library to create nuget package. Then I need to update some .sh/makefile files.

Contributor

rob-v commented Apr 16, 2017

@atlimit8 Can it be related to new .dll? Phrohdoh created issue for that library to create nuget package. Then I need to update some .sh/makefile files.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Apr 16, 2017

Contributor

Updated - fixed .csproj formatting.

Contributor

rob-v commented Apr 16, 2017

Updated - fixed .csproj formatting.

@pchote

The overall approach looks good, just a couple of suggestions/requests to make the logic simpler and more maintainable.

@rob-v rob-v closed this Apr 19, 2017

@rob-v rob-v reopened this Apr 19, 2017

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Apr 19, 2017

Contributor

Updated.

Contributor

rob-v commented Apr 19, 2017

Updated.

@pchote

pchote requested changes Apr 23, 2017 edited

This works really well, good job! I have a few more requests below, but then I think this should be good to merge. We should also get 👍's from people on Windows and Linux before we merge.

I notice a few "System.Net.Sockets.SocketOptionName 0x17 is not supported at IP level" being written to stdout by, i assume, beaconlib, but that doesn't appear to stop it from working.

Finally, it would be nice for polish if we could change the "Country" column in the server browser to "Location", and then have local games show as "Local Network" instead of "Unknown Loca...".
The easiest way will be to check if the server IP matches the reserved internal IP ranges, but in any case this is out of scope for this PR and should be left to a followup.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Apr 23, 2017

Contributor

Updated as requested but with some additional changes to be reviewed.
I replaced ProcessId by Guid to ensure it is unique. Due to this change to replace then Id by -1, I used this to identify 'Local Network' games. We could also add GameServer.IsLANGame flag rather than checking the IP range.

Contributor

rob-v commented Apr 23, 2017

Updated as requested but with some additional changes to be reviewed.
I replaced ProcessId by Guid to ensure it is unique. Due to this change to replace then Id by -1, I used this to identify 'Local Network' games. We could also add GameServer.IsLANGame flag rather than checking the IP range.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Apr 23, 2017

Member

Ah, well actualy the reason why I suggested to check the IP range and also do it in a separate PR was so that it could also fix the other cases where GeoIP fails on local networks, like the client tooltips in the lobby.

Member

pchote commented Apr 23, 2017

Ah, well actualy the reason why I suggested to check the IP range and also do it in a separate PR was so that it could also fix the other cases where GeoIP fails on local networks, like the client tooltips in the lobby.

@pchote

pchote approved these changes Apr 23, 2017

Looks good otherwise 👍

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Apr 23, 2017

Contributor

Updated, thanks.

Contributor

rob-v commented Apr 23, 2017

Updated, thanks.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Apr 30, 2017

Contributor

Needs a rebase.

Contributor

reaperrr commented Apr 30, 2017

Needs a rebase.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Apr 30, 2017

Contributor

Rebased.

Contributor

rob-v commented Apr 30, 2017

Rebased.

@pchote pchote added this to the Next Release milestone Apr 30, 2017

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote May 14, 2017

Member

Needs another rebase, sorry.

@OpenRA/engine-hackers: this has now been stalled waiting for a second 👍 for nearly three weeks. This really isn't hard to review.

Member

pchote commented May 14, 2017

Needs another rebase, sorry.

@OpenRA/engine-hackers: this has now been stalled waiting for a second 👍 for nearly three weeks. This really isn't hard to review.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 14, 2017

Contributor

Seems this was already rebased. Looks good to me and works fine on Windows, but before we merge this, is the AppVeyor fail a one-time thing or does that need som kind of fix @pchote ?

Contributor

reaperrr commented May 14, 2017

Seems this was already rebased. Looks good to me and works fine on Windows, but before we merge this, is the AppVeyor fail a one-time thing or does that need som kind of fix @pchote ?

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote May 14, 2017

Member

It looks like nuget is having server troubles right now. I don't think its anything wrong with this PR.

Member

pchote commented May 14, 2017

It looks like nuget is having server troubles right now. I don't think its anything wrong with this PR.

@reaperrr reaperrr merged commit ffc3f6e into OpenRA:bleed May 14, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rob-v rob-v deleted the rob-v:LANGamesDiscovery branch May 18, 2017

@reaperrr reaperrr modified the milestones: Next Release, Next + 1 Sep 1, 2017

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