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

[bobtista]

Created NetPacketStructs.h with packed struct definitions for network packets
Refactored 27 getPackedByteCount() functions to use packed structs: 22 use pure sizeof(struct) calls, 5 use sizeof(struct) as a base for additional calculations.

[xezon]

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

[Mauller]

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.

[xezon]

Side note: This truncates 4 bytes to 1 byte 👀

[xezon]

This looks very promising.

[xezon]

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.

[bobtista]

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

[xezon]

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

[bobtista]

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

[xezon]

In a future Pull.

[bobtista]

@xezon any changes you want still?

[xezon]

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

[bobtista]

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]

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]

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]

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]

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.

[bobtista]

What is the current state of this change?

I tested all but the following:
WrapperCommand
PacketRouterAckCommand
PacketRouterQueryCommand

[xezon]

Sizes of PacketRouterAckCommand, PacketRouterQueryCommand are correct.

[xezon]

Remove inline again

Still needs a bit of work