Page MenuHomeWildfire Games

Embed zpl-c's version of enet library
Needs ReviewPublic

Authored by Lefo on Sun, Nov 18, 6:43 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Trac Tickets
#4301
Summary

The main purpose of this patch is to add initial support for IPv6.
With this patch you can host on both IPv4 and IPv6 simultaneously as well as
joining games via both IPv4 and IPv6 protocols.
Since this patch you need your system to support AF_INET6. Also to be able to
use IPv4 you need your system to support IPv4-mapped address feature even when
using IPv4 only.

New option --with-system-enet was added when executing update-workspaces script
so you can switch back to your system's enet library which should allow you to
compile even with no IPv6 support in your system. This will disable any IPv6
support if your system's enet library does not support IPv6.

Lobby still lacks the necessary IPv6 support so for now joining games via lobby
is always IPv4 only.

See https://github.com/zpl-c/enet for more informations about zpl-c's version
of enet library.

v1->v2

  • addressed vladislavbelov's and elexis's requests for syntax adjustments
  • updated premake4 so it should at least compile with system's enet
  • fixed bugs causing crash when peer gets disconnected due to timeout
Test Plan

Tested playing games over IPv6 and also playing over IPv4 with both game compiled with system's enet and the new default embedded enet.
Tested hosting as well as joining other games.
Tested backward compatibility with players without this patch.
Tested banning feature.
Tested hosting and joining games via lobby with both stun enabled and disabled.
So far everything seemed to work as expected.

I did not test the game on any system other than Debian Stretch. But I'm not aware of anything that could break the game on other system.
I did not test compiling game with premake4. But it should work with system's enet library like before.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

Lefo created this revision.Sun, Nov 18, 6:43 PM

Isn't the flag needed in the premake4.lua too?

@fcxSanya I think there is something related to you.

lib/external_libraries/enet.h
61 ↗(On Diff #6992)

I think struct is useless here and below since we compile it with a C++ compiler.

lobby/XmppClient.cpp
1210 ↗(On Diff #6992)

static_cast.

network/StunClient.cpp
240 ↗(On Diff #6992)

It may looks hacky for some readers, I'd prefer the old version of the initialization.

436 ↗(On Diff #6992)

You don't use status anywhere else. So put it inside the if statement.

premake/premake5.lua
184 ↗(On Diff #6992)

Use the common config name notation for defines.

vladislavbelov added a comment.EditedMon, Nov 19, 2:30 PM

It'd be good to attach a diff between the added version and a usual system version of enet.

Itms added a subscriber: Itms.Mon, Nov 19, 7:30 PM

Isn't the flag needed in the premake4.lua too?

I will land D1275 just after the rerelease, so either before this one or after. No need to update premake4 for this patch.

Lefo marked 3 inline comments as done.Wed, Nov 21, 12:18 AM
Lefo added inline comments.
lobby/XmppClient.cpp
1210 ↗(On Diff #6992)

static_cast didn't work:
error: invalid static_cast from type ‘sockaddr_in*’ to type ‘sockaddr*’
So I used reinterpret_cast.

network/StunClient.cpp
240 ↗(On Diff #6992)

What you mean by old version of initialization?
I agree it does not look good.
How about this?
const u16 v4_mapped_prefix[] = {0, 0, 0, 0, 0, 0xFFFF};
bool sender_is_ipv4 = !memcmp(addr.sin6_addr.s6_addr, v4_mapped_prefix, sizeof(v4_mapped_prefix));
It's much longer but seems to be more clear.

436 ↗(On Diff #6992)

Condition of if depends on status so I can't do that.
I could declare status inside if condition and drop the part "!= 0" if that's what you want.
But that would make code inconsistent with getaddrinfo usage above. Unless I adjust it there too.

premake/premake5.lua
184 ↗(On Diff #6992)

You mean replacing WITH_SYSTEM_ENET with something like CONFIG2_SYSTEM_ENET?
But these defines are related to source/lib/config2.h aren't they?
I guess that would require to modify config2.h to default CONFIG2_SYSTEM_ENET to 0 and defines { "WITH_SYSTEM_ENET" } would become defines { "CONFIG2_SYSTEM_ENET=1" }.
enet.h would then need to be adjusted as well to use #if instead of #ifdef.
Is that what you want?

Do I understand it correctly, that IPv4 won't work when IPv6 is enabled?

lobby/XmppClient.cpp
1210 ↗(On Diff #6992)

So, reinterpret_cast then :)

network/StunClient.cpp
240 ↗(On Diff #6992)

Yep, something like that.

436 ↗(On Diff #6992)

I meant, that if (status != 0) equals to if (status), so it could be written so:

if (int status = getaddrinfo(serverAddress, nullptr, &hints, &res))
{
    // ...
}
premake/premake5.lua
184 ↗(On Diff #6992)

I was talking about CONFIG2_SYSTEM_ENET, yes. But I don't mind how it's defined, with =0 or without.

Lefo marked 12 inline comments as done.Thu, Nov 22, 8:18 AM
Lefo added inline comments.
network/StunClient.cpp
436 ↗(On Diff #6992)

Ok, I also updated previously untouched code above to make it consistent with this.

Lefo marked an inline comment as done.Thu, Nov 22, 8:29 AM

Do I understand it correctly, that IPv4 won't work when IPv6 is enabled?

No, it seems you don't. Read the summary.

Lefo updated this revision to Diff 6996.Thu, Nov 22, 8:58 AM
Lefo edited the summary of this revision. (Show Details)
Lefo edited the test plan for this revision. (Show Details)