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

Updates lidgren, adds ipv6 support #204

Merged
merged 5 commits into from Jul 9, 2017

Conversation

Projects
None yet
5 participants
@gbasood
Contributor

gbasood commented May 22, 2017

Implements IPv6 support along with the fixes to it from lidgren/lidgren-network-gen3#33 and its discussion.

Also we were a few years behind on our version of lidgren.

@gbasood

This comment has been minimized.

Contributor

gbasood commented May 22, 2017

Currently has two errors I haven't looked into resolving:

'SS14NetServer' does not implement interface member 'ISS14NetServer.RegisterReceivedCallback(SendOrPostCallback)'
'SS14NetServer' does not implement interface member 'ISS14NetServer.SendMessage(NetOutgoingMessage, List<NetConnection>, NetDeliveryMethod, int)'	

This also made a lot of warnings show up on build, not sure what changed in the solution/csproj files to cause that.

@gbasood gbasood force-pushed the gbasood:lidgren-ipv6 branch from c51c2d9 to 42034ae May 23, 2017

@gbasood

This comment has been minimized.

Contributor

gbasood commented May 23, 2017

New problem:

Sending a chat message caused a crash.

@gbasood gbasood changed the title from Updates lidgren, adds ipv6 support/g/Git/ss14csharp/Lidgren.Network to Updates lidgren, adds ipv6 support May 23, 2017

@gbasood

This comment has been minimized.

Contributor

gbasood commented May 23, 2017

fixed above issue
something weird happened between the first compile and this one
long story short, the methods i thought I needed to add to satisfy interface requirements weren't actually doing that and instead were just becoming self-calling infinite recursion-causing methods.

Now we just need two people to test if ipv6 works

@gbasood

This comment has been minimized.

Contributor

gbasood commented May 23, 2017

server gets Connect message properly, tries to send connectresponse
client doesn't seem to get it or handle it, either or

@psykzz

This comment has been minimized.

Contributor

psykzz commented May 23, 2017

Just double checking this to the PR you listed, it looks like your doing things differently
Can you explain the changes?

For example in the PR he converts all IPs to ipv6 regardless. Is that not what your doing here?

@gbasood

This comment has been minimized.

Contributor

gbasood commented May 23, 2017

it's possible that my entire branch is fucked because it doesnt seem like the files match the ones in the original ipv6 fix branch

@gbasood

This comment has been minimized.

Contributor

gbasood commented Jun 6, 2017

This seems to be functional now
I need someone to test it for me on their own
Couple of notes for review:

  • git diff is significantly smaller with whitespace ignored
  • I had to redo the entire branch because I messed up the history at one point and was working on some weird hybrid of old and new lidgren
  • If we continue working with lidgren beyond this we might want to consider setting this up as a submodule
@@ -28,7 +30,8 @@ public void SendMessage(NetOutgoingMessage message, NetConnection client)

public void SendToMany(NetOutgoingMessage message, List<NetConnection> recipients)
{
SendMessage(message, recipients, NetDeliveryMethod.ReliableOrdered, 0);
foreach (NetConnection recipient in recipients)

This comment has been minimized.

@gbasood

gbasood Jun 6, 2017

Contributor

Someone should tell me if this is hacky

This comment has been minimized.

@PJB3005

PJB3005 Jun 6, 2017

Member

Shouldn't this just call the overload you commented on next?


public void SendMessage(NetOutgoingMessage message, List<NetConnection> recipients, NetDeliveryMethod method, int sequenceChannel)
{
foreach (NetConnection recipient in recipients)

This comment has been minimized.

@gbasood

gbasood Jun 6, 2017

Contributor

Someone should tell me if this is hacky

@PJB3005

This comment has been minimized.

Member

PJB3005 commented Jun 14, 2017

@gbasood status on this?

@gbasood

This comment has been minimized.

Contributor

gbasood commented Jun 14, 2017

Right now clients get an exception on trying to access a closed filestream when they send a chat message, and I'd assume when they receive one too
Haven't looked into it much, I thought it wasn't part of my changes until this week

@gbasood gbasood force-pushed the gbasood:lidgren-ipv6 branch from 126db64 to 6b6cc76 Jun 15, 2017

{59250BAF-0000-0000-0000-000000000000}.Release|Mixed Platforms.ActiveCfg = Release|x86
{59250BAF-0000-0000-0000-000000000000}.Release|Mixed Platforms.Build.0 = Release|x86
{59250BAF-0000-0000-0000-000000000000}.Release|x86.ActiveCfg = Release|x86
{59250BAF-0000-0000-0000-000000000000}.Release|x86.Build.0 = Release|x86

This comment has been minimized.

@gbasood

gbasood Jun 15, 2017

Contributor

I have no idea why this was needed
And someone who knows more about configuring this might want to give their input

This comment has been minimized.

@PJB3005

PJB3005 Jun 15, 2017

Member

I can only assume it's VS being dumb.

This comment has been minimized.

@gbasood

gbasood Jun 15, 2017

Contributor

Actually, in the full history, it looks much more sane, so I'm not too worried about it

@PJB3005

This comment has been minimized.

Member

PJB3005 commented Jun 17, 2017

Well whatever, take DO NOT MERGE off the PR when you feel it's ready for review.

@gbasood

This comment has been minimized.

Contributor

gbasood commented Jun 18, 2017

It's as ready as it will be, but it's broken because SFML.
Might need to wait till after robust engine

@gbasood

This comment has been minimized.

Contributor

gbasood commented Jul 9, 2017

No longer crashes
Seems to be ready, not sure what else I could possibly test

@PJB3005 PJB3005 merged commit 7ca66ee into space-wizards:master Jul 9, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@PJB3005

This comment has been minimized.

Member

PJB3005 commented Jul 9, 2017

Time to trigger sonar.

@wafflebot wafflebot bot removed the W: In Progress label Jul 9, 2017

@Acruid Acruid referenced this pull request Aug 26, 2017

Closed

Replace Lidgren.Network? #182

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