ddnet
Show 0.7 servers in LAN serverbrowser
It already works. But I would like to know your opinion on code. One thing which is not implemented yet is removing connless packets from m_ConnlessPackets after some time. Maybe you have better ideas how to do it =]
If you think there's no way to make this code mergeable, lemme know I'll close the pr xd
closes #8920
Checklist
- [x] Tested the change ingame
- [ ] Provided screenshots if it is a visual change
- [ ] Tested in combination with possibly related configuration options
- [ ] Written a unit test (especially base/) or added coverage to integration test
- [ ] Considered possible null pointers and out of bounds array indexing
- [ ] Changed no physics that affect existing maps
- [ ] Tested the change with ASan+UBSan or valgrind's memcheck (optional)
I also reworked the code which was comparing addresses and sending server browser info packet, Because of this now ipv6 servers are also shown in serverbrowser
First on a high level. Why does this need a new parameter on the CNetClient::Recv function? By that point, the client should already know whether the server is a 0.7 or a 0.6 server.
First on a high level. Why does this need a new parameter on the
CNetClient::Recvfunction? By that point, the client should already know whether the server is a 0.7 or a 0.6 server.
The parameter is true if a packet was of type NET_CTRLMSG_TOKEN
I think the NET_CTRLMSG_TOKEN stuff should be handled by the CNetClient, not outside of it. Basically, you should send a serverbrowser info request message and the CNetClient will handle the requesting of tokens and sending of the message.
Robyt3 wrote that he thinks those functions should be in serverbrowser https://github.com/ddnet/ddnet/pull/8992#discussion_r1844296563
I guess the token stuff specifically should be handled inside CNetClient, but the serverinfo specifics should not be added there.
Where should BrowserInfo7(https://github.com/ddnet/ddnet/pull/8992/files#diff-64f65b9589678af8ccfe6558a3ea6712dff9e74becd3d0e22fcac74758546776R2352-R2366) function be, in CNetClient or CServerBrowser?
Where should
BrowserInfo7(https://github.com/ddnet/ddnet/pull/8992/files#diff-64f65b9589678af8ccfe6558a3ea6712dff9e74becd3d0e22fcac74758546776R2352-R2366) function be, in CNetClient or CServerBrowser?
I'd say only SERVERBROWSE_GETINFO and sizeof(SERVERBROWSE_GETINFO) belong in CServerBrowser. Everything else belongs in CNetClient, i.e. generating the token based on the address and adding the token to the end of the message (use a unsigned char aBuffer[NET_MAX_PACKETSIZE]; instead of CPacker for this).
i.e. generating the token based on the address and adding the token to the end of the message (use a
unsigned char aBuffer[NET_MAX_PACKETSIZE];instead ofCPackerfor this).
I don't understand how to use bytes array instead of CPacker, CVariableInt::Pack has to be used for browser token. @ChillerDragon can you do this pr, I'm too stupid for this xd
I don't understand how to use bytes array instead of
CPacker,CVariableInt::Packhas to be used for browser token.
Ah, I didn't notice that. I guess it's fine to use CPacker then.
I'm planning to add a method to CNetClient which will queue 0.7 packets and will send them as soon as the token is fetched. But how to represent the packet's data itself? Should it be something like unsigned char [N][NET_MAX_PACKETSIZE](that's a lot of bytes) or do you have a better idea?
I'm planning to add a method to
CNetClientwhich will queue 0.7 packets and will send them as soon as the token is fetched. But how to represent the packet's data itself?
Yes. You can even add that functionality to the existing send function, it'll be used whenever you set the NETTYPE_TW7 constant.
But how to represent the packet's data itself? Should it be something like
unsigned char [N][NET_MAX_PACKETSIZE](that's a lot of bytes) or do you have a better idea?
You can probably take a look at how the 0.7 code does it. https://github.com/teeworlds/teeworlds/blob/c56fa9e6a20cfc9d7d16502e18c7d7633acdf492/src/engine/shared/network_server.cpp#L267
https://github.com/teeworlds/teeworlds/blob/c56fa9e6a20cfc9d7d16502e18c7d7633acdf492/src/engine/shared/network.h#L308-L309 What's better than linked lists?
You can probably store it in a vector and hope that there aren't going to be many connless messages sent at once.
I tested this together with some other fixes for #9176, but it looks like the translated server info produced by this PR isn't valid in some cases yet. For example on @ChillerDragon's server the translated server info has type SERVERINFO_VANILLA but this causes the server info to be discarded by the client because of the Info.m_MaxPlayers > VANILLA_MAX_CLIENTS check.
I tested this together with some other fixes for #9176, but it looks like the translated server info produced by this PR isn't valid in some cases yet. For example on @ChillerDragon's server the translated server info has type
SERVERINFO_VANILLAbut this causes the server info to be discarded by the client because of theInfo.m_MaxPlayers > VANILLA_MAX_CLIENTScheck.
What server info type should it have? SERVERINFO_64_LEGACY?
What server info type should it have?
SERVERINFO_64_LEGACY?
Probably a new SERVERINFO_7 or so.
What server info type should it have?
SERVERINFO_64_LEGACY?Probably a new
SERVERINFO_7or so.
I don't fully understand how it should work. Should there also be SERVERBROWSE_INFO_7 which would be set in CClient::PreprocessConnlessPacket7 or only CClient::ProcessConnlessPacket method should be changed?
Ah, I misunderstood the question. Yes, SERVERINFO_EXTENDED or SERVERINFO_64_LEGACY, preferably the first one unless it doesn't work. We'd probably drop SERVERINFO_64_LEGACY at some point.