refactor(network): replace manual size calculations with packed structs #1675
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
xezon
reviewed
Oct 7, 2025
GeneralsMD/Code/GameEngine/Source/GameNetwork/NetPacketStructs.h
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Source/GameNetwork/NetPacketStructs.h
Outdated
Show resolved
Hide resolved
|
You can still do this for the messages that are variable in length, just make the fixed portion of the message a struct. |
bobtista
force-pushed
the
bobtista/packedstructpackets
branch
2 times, most recently
from
dbb01a8 to
9d9e804
Compare
2 tasks
bobtista
force-pushed
the
bobtista/packedstructpackets
branch
from
9d9e804 to
db5c043
Compare
Mauller
reviewed
Oct 10, 2025
GeneralsMD/Code/GameEngine/Include/GameNetwork/NetPacketStructs.h
Outdated
Show resolved
Hide resolved
xezon
reviewed
Oct 13, 2025
|
|
||
| ++msglen; // the NetPacketFieldTypes::Data | ||
| msglen += sizeof(UnsignedByte); // string msglength | ||
| UnsignedByte textmsglen = cmdMsg->getText().getLength(); |
There was a problem hiding this comment.
Side note: This truncates 4 bytes to 1 byte 👀
GeneralsMD/Code/GameEngine/Include/GameNetwork/NetPacketStructs.h
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Include/GameNetwork/NetPacketStructs.h
Outdated
Show resolved
Hide resolved
xezon
added
Major
bobtista
force-pushed
the
bobtista/packedstructpackets
branch
from
34ab595 to
ba6335d
Compare
xezon
reviewed
Oct 13, 2025
bobtista
force-pushed
the
bobtista/packedstructpackets
branch
from
35cac52 to
d54e7dc
Compare
…dType type, and add struct constructors
bobtista
force-pushed
the
bobtista/packedstructpackets
branch
from
960e7a4 to
ad27a6c
Compare
xezon
reviewed
Oct 15, 2025
| */ | ||
| size_t NetDisconnectChatCommandMsg::getByteCount() const | ||
| { | ||
| return m_text.getLength() * sizeof(UnsignedShort); |
There was a problem hiding this comment.
I think UnicodeString also has a getByteCount function by now. Use it.
GeneralsMD/Code/GameEngine/Include/GameNetwork/NetPacketStructs.h
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Include/GameNetwork/NetPacketStructs.h
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| UnsignedInt NetPacket::GetGameCommandSize(NetCommandMsg *msg) { | ||
| NetGameCommandMsg *cmdMsg = (NetGameCommandMsg *)msg; |
| GameMessage *gmsg = cmdMsg->constructGameMessage(); | ||
| GameMessageParser *parser = newInstance(GameMessageParser)(gmsg); | ||
|
|
||
| msglen += sizeof(GameMessage::Type); |
There was a problem hiding this comment.
Can all this junk also be shoved into a getByteCount function?
xezon
reviewed
Oct 16, 2025
| */ | ||
| size_t NetChatCommandMsg::getByteCount() const | ||
| { | ||
| return m_text.getLength() * sizeof(UnsignedShort) + sizeof(m_playerMask); |
| */ | ||
| size_t NetDisconnectChatCommandMsg::getByteCount() const | ||
| { | ||
| return m_text.getByteCount(); |
There was a problem hiding this comment.
How does the reader recognize the length of this text? Surely this must be either null terminated or bundled with a count? I wonder if this is an original bug.
There was a problem hiding this comment.
Should this be addressed in another PR or here?
A potential fix:
{
return sizeof(UnsignedByte) + m_text.getByteCount();
}
|
|
||
| GameMessageParserArgumentType *arg = parser->getFirstArgumentType(); | ||
| while (arg != NULL) { | ||
| msglen += 2 * sizeof(UnsignedByte); // for the type and number of args of that type declaration. |
There was a problem hiding this comment.
Maybe this can be written in 2 lines. Does "type" have a typedef?
There was a problem hiding this comment.
How about something like:
constexpr size_t ARG_TYPE_SIZE = sizeof(UnsignedByte);
constexpr size_t ARG_COUNT_SIZE = sizeof(UnsignedByte);
...
msglen += ARG_TYPE_SIZE + ARG_COUNT_SIZE;
Or
msglen += sizeof(UnsignedByte); // argument type
msglen += sizeof(UnsignedByte); // argument count
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Created NetPacketStructs.h with packed struct definitions for network packets
Replaced 9 manual size calculation functions with sizeof(struct) calls
In the next PR we could use these structs for actual serialization, replacing manual memcpy calls
TODO:
Replicate in Generals