bugfix(network): Increase message buffer and max packet sizes to reduce connection issues#2277
Conversation
Greptile Overview
|
| Filename | Overview |
|---|---|
| Core/GameEngine/Include/Common/GameDefines.h | Added RETAIL_COMPATIBLE_NETWORKING flag to control non-retail networking improvements |
| Core/GameEngine/Include/GameNetwork/NetworkDefs.h | Increased MAX_MESSAGES from 128 to 256, introduced larger UDP payload sizes (1100 bytes) conditionally, and refactored constants with better naming |
| Core/GameEngine/Source/GameNetwork/Transport.cpp | Updated send/receive operations to use MAX_NETWORK_MESSAGE_LEN and added clarifying comments about payload sizing logic |
Flowchart
flowchart TD
A[UDP Payload Size] -->|Retail| B[512 bytes total MTU]
A -->|Non-Retail| C[1100 bytes UDP payload]
B --> D[512 - 36 IP/UDP headers = 476 bytes]
C --> E[1100 bytes max UDP payload]
D --> F[MAX_PACKET_SIZE = 476<br/>RETAIL_GAME_PACKET_SIZE]
E --> G[MAX_PACKET_SIZE = 1094<br/>1100 - 6 header bytes]
F --> H[TransportMessageHeader: 6 bytes<br/>CRC + Magic]
G --> H
H --> I[Game Data Payload]
I --> J[MAX_MESSAGES buffer<br/>128 → 256 doubled]
J --> K{RETAIL_COMPATIBLE_NETWORKING?}
K -->|Yes| L[Use 476 byte packets<br/>256 message buffers]
K -->|No| M[Use 1094 byte packets<br/>256 message buffers<br/>~2.3x more data per packet]
Last reviewed commit: 671170a
188298b to
535b4db
Compare
| static const Int MAX_MESSAGE_LEN = MAX_UDP_PAYLOAD; | ||
| #endif | ||
|
|
||
| static const Int MAX_MESSAGES = 256; |
There was a problem hiding this comment.
Increases buffer footprint
Bumping MAX_MESSAGES from 128 to 256 doubles the size of Transport::m_inBuffer/m_outBuffer (and m_delayedInBuffer in debug) because these arrays are sized by MAX_MESSAGES (see Core/GameEngine/Include/GameNetwork/Transport.h:73-78). This is a behavioral change that can increase memory pressure (and potentially stack/heap usage depending on how Transport is instantiated) even when RETAIL_COMPATIBLE_NETWORKING is kept enabled; please confirm this is intended and safe for all builds, or gate it behind the same retail-compat switch.
Prompt To Fix With AI
This is a comment left during a code review.
Path: Core/GameEngine/Include/GameNetwork/NetworkDefs.h
Line: 71:71
Comment:
**Increases buffer footprint**
Bumping `MAX_MESSAGES` from 128 to 256 doubles the size of `Transport::m_inBuffer`/`m_outBuffer` (and `m_delayedInBuffer` in debug) because these arrays are sized by `MAX_MESSAGES` (see `Core/GameEngine/Include/GameNetwork/Transport.h:73-78`). This is a behavioral change that can increase memory pressure (and potentially stack/heap usage depending on how `Transport` is instantiated) even when `RETAIL_COMPATIBLE_NETWORKING` is kept enabled; please confirm this is intended and safe for all builds, or gate it behind the same retail-compat switch.
How can I resolve this? If you propose a fix, please make it concise.
xezon
left a comment
There was a problem hiding this comment.
The presentation can be polished more.
535b4db to
66fef26
Compare
|
Tweaked and addressed comments |
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: Core/GameEngine/Source/GameNetwork/NetPacket.cpp
Line: 960:964
Comment:
**Incorrect memcpy length**
`NetPacket::NetPacket(TransportMessage *msg)` copies `MAX_PACKET_SIZE` bytes (`memcpy(m_packet, msg->data, MAX_PACKET_SIZE)`) even though `msg->length` may be smaller. With this PR increasing `MAX_PACKET_SIZE` to 1200 in non-retail builds, this will read past the end of the received UDP payload for most packets and can cause out-of-bounds reads / crashes. This should copy `msg->length` (or `m_packetLen`) bytes instead of `MAX_PACKET_SIZE`.
How can I resolve this? If you propose a fix, please make it concise. |
66fef26 to
022692a
Compare
Dismissed because the Mauller wants to do another pass
|
After some talking with X64 i will need to adjust the packet size (data that goes onto the wire) down to 1100, the extra 100 bytes is to help with TURN relay headers and extra reliability layers That GO uses. this still gives 2.2-2.3x the packet size of retail. This means that data on the wire, UDP payload, needs to be a max of 1100, but game messages can only be a max of 1094 due to the 6 bytes of game packet header that i overlooked originally. We can always adjust these values in future when refactoring more of the networking. |
022692a to
265fab3
Compare
|
Updated now with the tweaks |
|
In future, we can also introduce packet data compression to help reduce the traffic pressure. |
Yes it would be worth investigating. I want to overhaul the entire packet structure to get something more standardised between message types with a common header containing an enumeration for message type, then relevant payload data etc. |
xezon
left a comment
There was a problem hiding this comment.
The Pull title says tweak but the comments have 2 bugfix. Should be bugfix in title?
…e connection issues
265fab3 to
671170a
Compare
I changed it to say bugfix now, initially i was considering this a tweak but after more testing it does appear to be a considerable bugfix towards connection issues. |
Closes: #203
This PR increases the send and receive buffer sizes, this can help to alleviate network pressure that can lead to the DC menu appearing when the game is waiting for more network data or has to wait for a resend.
The PR also implements a non retail change to increase the amount of data stored per UDP packet.
The retail game was designed with Dialup limitations in mind, which tended to limit the MTU side to 576 bytes.
Internally they chose 512 bytes as their packet size limit and then incorrectly calculated a lower max UDP payload than necessary on top of this.
The fix allows the game to use 1100 Bytes as the UDP payload for the greatest compatibility between different types of network connections. This should take into account overhead of PPPOE, VPN's and IPV6 on supporting networks without causing fragmentation.
As 6 bytes is required for the current network packet header, game data packets can only be up to 1094 bytes in size.
At 1094 bytes per game packet we are now handling ~2.3x the amount of data per packet compared to retail, this should allow a reduction of network traffic by up to 2/3's depending on how game commands are being generated.
When testing this on GO with a full 8 player Mayhem game, we did not see a single DC screen popup.
All players were creating large armies and using QQA repeatedly to move them.