refactor(network): replace manual size calculations with packed structs #1675

bobtista · 2025-10-07T16:45:50Z

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

xezon

I cannot compile this branch. There are 700 linker errors.

GeneralsMD/Code/GameEngine/Source/GameNetwork/NetPacketStructs.h

GeneralsMD/Code/GameEngine/Source/GameNetwork/NetPacket.cpp

Mauller · 2025-10-07T20:49:53Z

You can still do this for the messages that are variable in length, just make the fixed portion of the message a struct.
Then you just calculate the message size based on the size of the fixed portion and the variable data appended to it.

GeneralsMD/Code/GameEngine/Include/GameNetwork/NetPacketStructs.h

xezon · 2025-10-08T17:46:51Z

GeneralsMD/Code/GameEngine/Source/GameNetwork/NetPacket.cpp

-
-	++msglen; // the NetPacketFieldTypes::Data
-	msglen += sizeof(UnsignedByte); // string msglength
 	UnsignedByte textmsglen = cmdMsg->getText().getLength();


Side note: This truncates 4 bytes to 1 byte 👀

GeneralsMD/Code/GameEngine/Include/GameNetwork/NetPacketStructs.h

GeneralsMD/Code/GameEngine/Source/GameNetwork/NetPacket.cpp

GeneralsMD/Code/GameEngine/Include/GameNetwork/NetCommandMsg.h

GeneralsMD/Code/GameEngine/Source/GameNetwork/NetPacket.cpp

GeneralsMD/Code/GameEngine/Source/GameNetwork/NetCommandMsg.cpp

GeneralsMD/Code/GameEngine/Include/GameNetwork/NetPacketStructs.h

GeneralsMD/Code/GameEngine/Source/GameNetwork/NetPacket.cpp

GeneralsMD/Code/GameEngine/Source/GameNetwork/NetCommandMsg.cpp

GeneralsMD/Code/GameEngine/Include/GameNetwork/NetPacket.h

GeneralsMD/Code/GameEngine/Source/GameNetwork/NetCommandMsg.cpp

…ments more consistent

…dType type, and add struct constructors

…classes - Add NetLoadCompleteCommandMsg and NetTimeOutGameStartCommandMsg classes - Update ConnectionManager.cpp to use specific classes instead of base NetCommandMsg - Remove special case handling from NetPacket.cpp since all types now have classes - Eliminate the switch statement entirely - now just calls virtual function

xezon

This looks very promising.

GeneralsMD/Code/GameEngine/Source/GameNetwork/NetPacket.cpp

GeneralsMD/Code/GameEngine/Include/GameNetwork/NetCommandMsg.h

GeneralsMD/Code/GameEngine/Source/GameNetwork/NetCommandMsg.cpp

GeneralsMD/Code/GameEngine/Source/GameNetwork/NetPacket.cpp

xezon · 2025-10-18T17:03:13Z

Core/GameEngine/Include/GameNetwork/NetPacketStructs.h

+// Frame Info Command Packet
+////////////////////////////////////////////////////////////////////////////////
+
+struct NetPacketFrameCommand {


Would it perhaps make sense to put these packed structs into the non packed message classes?

Such as:

class NetGameCommandMsg : public NetCommandMsg { ... public: #pragma pack(push, 1) struct Packed { ... }; #pragma pack(pop) ... };

This way they would sit together and would perhaps be easier to maintain? Not sure.

Yes we could do that, we'd get to use return sizeof(Packed) instead of return sizeof(NetPacketGameCommand), and we could remove some imports, and it keeps the related code together visually. On the other hand, the packed structs are really about network serialization, not about the message logic. Are there places where we only want to import one vs both? Any file that includes NetCommandMsg.h would now also pull in all the packed struct definitions. Whatever you think is best

Maybe move in later changes when more refactor work is done and we have a clearer picture. Right now seems to be ok.

You mean later in another PR or now in this one?

In a future Pull.

bobtista · 2025-10-21T18:37:41Z

@xezon any changes you want still?

xezon

Did you push all changes? (Some Review Comments were closed without response or action)

GeneralsMD/Code/GameEngine/Include/GameNetwork/NetCommandMsg.h

bobtista · 2025-10-21T21:27:47Z

Did you push all changes? (Some Review Comments were closed without response or action)

I just pushed the latest. I tried to only resolve comments if I addressed them in a commit.

xezon · 2025-10-21T21:33:18Z

For example you marked resolved the virtual function comment without action. Or the obsolete CppMacros.h comment without action. So I wonder what was the resolution for it.

Edit: I see the edits now. Strangely GitHub did not show the changes before. I will look over tomorrow.

xezon

I think it is good. Needs replication in Generals.

bobtista · 2025-10-22T15:54:22Z

I think it is good. Needs replication in Generals.

I just pushed the replication commit. Note, I just copied NetCommandMsg and NetPacket.cpp and corrected comments that mentioned Zero Hour.

xezon · 2025-10-22T16:25:44Z

Copying files from ZH to Generals is risky, unless they are identical. Better use script to replicate:

Batch:

FOR /F "delims=" %%b IN ('git merge-base --fork-point main') DO git diff %%b > changes.patch
git apply -p2 --directory=Generals --reject --whitespace=fix changes.patch
del changes.patch

bobtista · 2025-10-22T16:34:48Z

Copying files from ZH to Generals is risky, unless they are identical. Better use script to replicate:

Batch:
FOR /F "delims=" %%b IN ('git merge-base --fork-point main') DO git diff %%b > changes.patch
git apply -p2 --directory=Generals --reject --whitespace=fix changes.patch
del changes.patch

Ah ok. 118 rejected hunks. I'll try going through them like it's a merge conflict.

xezon · 2025-10-22T16:45:27Z

It drops *.rej files that you can inspect.

bobtista · 2025-10-22T17:37:37Z

It drops *.rej files that you can inspect.

Ok I did it and kept the commit where I'd just copied the files - there are indeed some differences when using the patch approach for those two files

xezon · 2025-10-22T17:47:32Z

Yes. At some places Generals code is a bit different and then needs attention to sort out.

bobtista · 2025-10-22T18:06:20Z

Ok so using this method to compare the original files from ZH to Generals on main:

diff <(git show main:Generals/Code/GameEngine/Source/GameNetwork/NetPacket.cpp) <(git show main:GeneralsMD/Code/GameEngine/Source/GameNetwork/NetPacket.cpp)
2c2
< **    Command & Conquer Generals(tm)
---
> **    Command & Conquer Generals Zero Hour(tm)

The changes should be exactly the same for NetPacket.cpp, NetCommandMsg.h, and NetCommandMsg.cpp
The differences were in cmakefiles and one difference in ConnectionManager.cpp which was preserved. Ah. I made a mistake with the patch command, used the wrong commit. I'll do it again, should be easier and cleaner

Yes. At some places Generals code is a bit different and then needs attention to sort out.

bobtista · 2025-10-22T18:19:37Z

Ok re-doing the patch method with the correct commit, there were zero rejections. Everything applied cleanly, and it should be good to go :)

Generals/Code/GameEngine/Include/GameNetwork/NetPacketStructs.h

Generals/Code/GameEngine/Include/GameNetwork/NetCommandMsg.h

xezon · 2025-10-22T19:54:14Z

Generals/Code/GameEngine/Source/GameNetwork/NetPacket.cpp

-	return 0;
-}
-
-UnsignedInt NetPacket::GetGameCommandSize(NetCommandMsg *msg) {


When I tried to locally replicate to Generals, I saw that the removed code blocks in this file conflicted. Are we certain that the packets are the exact same in both games? No discrepancies?

Hmm...

git apply -p2 --directory=Generals --reject --whitespace=fix changes.patch 2>&1 Checking patch Generals/Code/GameEngine/Include/GameNetwork/NetCommandMsg.h... Checking patch Generals/Code/GameEngine/Source/GameNetwork/ConnectionManager.cpp... Hunk #1 succeeded at 2236 (offset 14 lines). Hunk #2 succeeded at 2249 (offset 14 lines). Checking patch Generals/Code/GameEngine/Source/GameNetwork/NetCommandMsg.cpp... changes.patch:842: new blank line at EOF. + Checking patch Generals/Code/GameEngine/Source/GameNetwork/NetPacket.cpp... Applied patch Generals/Code/GameEngine/Include/GameNetwork/NetCommandMsg.h cleanly. Applied patch Generals/Code/GameEngine/Source/GameNetwork/ConnectionManager.cpp cleanly. Applied patch Generals/Code/GameEngine/Source/GameNetwork/NetCommandMsg.cpp cleanly. Applied patch Generals/Code/GameEngine/Source/GameNetwork/NetPacket.cpp cleanly. warning: 1 line adds whitespace errors. find Generals -name "*.rej" -> no output git status --short MM Generals/Code/GameEngine/Include/GameNetwork/NetCommandMsg.h MM Generals/Code/GameEngine/Source/GameNetwork/ConnectionManager.cpp MM Generals/Code/GameEngine/Source/GameNetwork/NetCommandMsg.cpp MM Generals/Code/GameEngine/Source/GameNetwork/NetPacket.cpp ?? changes.patch

All 4 files applied cleanly - zero rejections
Preserved Generals-specific code (14 lines of disconnect stats logic in ConnectionManager)

Have our mains diverged somehow? Should I pull the latest and rebase?

I did test by removing the top 3 commits where you did replicate work, and then perform a clean replicate with the Script. It then showed conflict.

I took a look at WinMerge and NetPacket.cpp are identical in both games (before this change). So it should not conflict. I will check again tomorrow.

I did also compare NetPacket.cpp in this branch and it is not identical in both games.

Ok I think I found the issue - in commit 0630ab5 on this branch, I changed a comment in both Generals and GeneralsMD, which caused the patch application to complain. So, I did a hard reset to before I made the attempts at duplicating to Generals, then added a commit that reverts the premature change to the Generals file, then the replicate in Generals with the patch went smoothly. I'm force pushing now which just removes the two botched commits where I tried replicating before.

Nice. Yes that explains it.

Btw, seeing major parts of Network are identical between Generals and Zero Hour, how about we unify the Network code first, before applying the new refactors?

I think Rebase after Unify will work, otherwise just copy paste the ZH files over Core files will also work.

What do you think? Would you need help with that?

Unifying then rebasing was giving me headaches, so I started a new branch, unified, then copied the contents of the files to their new homes, and made a cleaner commit history. I think we can close this PR and use this fresh one instead #1733

Generals/Code/GameEngine/Include/GameNetwork/NetCommandMsg.h

xezon reviewed Oct 7, 2025

View reviewed changes

GeneralsMD/Code/GameEngine/Source/GameNetwork/NetPacketStructs.h Outdated Show resolved Hide resolved

GeneralsMD/Code/GameEngine/Source/GameNetwork/NetPacketStructs.h Outdated Show resolved Hide resolved

GeneralsMD/Code/GameEngine/Source/GameNetwork/NetPacket.cpp Show resolved Hide resolved

bobtista force-pushed the bobtista/packedstructpackets branch 2 times, most recently from dbb01a8 to 9d9e804 Compare October 8, 2025 17:25

bobtista mentioned this pull request Oct 8, 2025

refactor(network): use packed structs for packet serialization #1680

Open

2 tasks

bobtista force-pushed the bobtista/packedstructpackets branch from 9d9e804 to db5c043 Compare October 8, 2025 22:08

Mauller reviewed Oct 10, 2025

View reviewed changes

GeneralsMD/Code/GameEngine/Include/GameNetwork/NetPacketStructs.h Outdated Show resolved Hide resolved

xezon reviewed Oct 13, 2025

View reviewed changes

xezon added Major Severity: Minor < Major < Critical < Blocker Network Anything related to network, servers Refactor Edits the code with insignificant behavior changes, is never user facing labels Oct 13, 2025

bobtista force-pushed the bobtista/packedstructpackets branch from 34ab595 to ba6335d Compare October 13, 2025 19:27

xezon reviewed Oct 13, 2025

View reviewed changes

GeneralsMD/Code/GameEngine/Include/GameNetwork/NetCommandMsg.h Outdated Show resolved Hide resolved

bobtista force-pushed the bobtista/packedstructpackets branch 2 times, most recently from 960e7a4 to ad27a6c Compare October 14, 2025 03:29

xezon reviewed Oct 15, 2025

View reviewed changes

xezon reviewed Oct 16, 2025

View reviewed changes

xezon reviewed Oct 17, 2025

View reviewed changes

GeneralsMD/Code/GameEngine/Source/GameNetwork/NetCommandMsg.cpp Outdated Show resolved Hide resolved

bobtista added 12 commits October 17, 2025 12:58

refactor(network): replace manual size calculations with packed structs

797d17b

nit: clean up unused variable

c3cf10e

nit: cleanup redundant comments

8703e6a

refactor: replace C-style casts with static_cast

6572390

refactor: add getByteCount() method to NetChatCommandMsg and use it

102de2b

nit: remove superfluous indent, restore some comments, make other com…

ecead82

…ments more consistent

refactor: add getByteCount() methods to more message classes and use it

a69c685

refactor: move NetPacketFieldTypes to header, use const NetPacketFiel…

a6a6279

…dType type, and add struct constructors

testing: static asserts for all 17 cases

6e72301

clean up tests

9f88e4a

refactor(network): use static cast in a couple more places

03f3adb

refactor(network): use static cast in about 40 more places

02f697e

bobtista added 4 commits October 18, 2025 11:03

Fix build: remove override keyword

06d97b5

Fix remaining NetCommandMsg instantiations in NetPacket.cpp

89c2e1d

Add missing destructor implementations for new command message classes

9ecdf97

xezon reviewed Oct 18, 2025

View reviewed changes

bobtista added 4 commits October 18, 2025 13:21

Add virtual keyword to getPackedByteCount() functions

5e82090

nit: Clean up tabs from empty lines

8838aae

nit: clean up import

4a0d8cc

nit: add comments, split line for readability

4f56498

xezon reviewed Oct 21, 2025

View reviewed changes

GeneralsMD/Code/GameEngine/Include/GameNetwork/NetCommandMsg.h Outdated Show resolved Hide resolved

nit: clean up comments

cd16778

xezon approved these changes Oct 22, 2025

View reviewed changes

xezon added the Approved Pull Request was approved label Oct 22, 2025

xezon reviewed Oct 22, 2025

View reviewed changes

bobtista added 3 commits October 22, 2025 18:42

revert: restore Generals/NetPacket.cpp to clean state

b3a6102

refactor(network): replicate to generals using patch method

41643e5

refactor(network): move NetPacketStructs to Core

1486fca

bobtista force-pushed the bobtista/packedstructpackets branch from c97bc8c to 1486fca Compare October 22, 2025 22:56

restore EA comments that were removed

85f60b6

refactor(network): replace manual size calculations with packed structs #1675

Are you sure you want to change the base?

refactor(network): replace manual size calculations with packed structs #1675

Uh oh!

Conversation

bobtista commented Oct 7, 2025

Uh oh!

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Mauller commented Oct 7, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobtista commented Oct 21, 2025

Uh oh!

xezon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bobtista commented Oct 21, 2025

Uh oh!

xezon commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

bobtista commented Oct 22, 2025

Uh oh!

xezon commented Oct 22, 2025

Uh oh!

bobtista commented Oct 22, 2025

xezon left a comment •

edited

Loading

xezon commented Oct 21, 2025 •

edited

Loading