refactor(netpacket): Simplify NetPacket functions for packet buffer writes and size tests

[xezon]

Merge with Rebase

• Follow up for refactor(netpacket): Simplify packet serialization code by using packed structs #1680

This change has 5 commits with refactors/fixes to greatly simplify all Net Packet buffer writes and size tests. All original networking behavior should be correctly preserved.

The only thing left to refactor for a follow up change are the Net Packet read functions.

TODO

• Test Multiplayer with VC6 clients + Retail clients
• Test File Transfer
• Test basic 2 player network matches
• Add pull id to commits
• Replicate in Generals
• Verify each commit compiles on its own

[greptile-apps]

Greptile Summary

This PR successfully refactors the network packet serialization system by replacing ~600 lines of repetitive FillBufferWithXXX functions with a clean template-based approach. The refactoring introduces NetPacketStructs.h/cpp with packed structs and helper functions, enabling polymorphic packet writing through virtual copyBytesForNetPacket() calls. Key improvements include:

• Eliminated code duplication: Removed 25+ similar buffer-filling functions from NetPacket.cpp
• Improved maintainability: Packet structure definitions now live in one place (NetPacketStructs.h)
• Enhanced const correctness: Added const qualifiers throughout (getCommand(), getArgumentDataType(), etc.)
• Fixed field ordering bug: Corrected Frame/PlayerId/CommandId field order in ConstructNetCommandMsgFromRawData() to match actual packet structure
• Consolidated Ack classes: Introduced NetAckCommandMsg base class to eliminate duplicate code across NetAckBothCommandMsg, NetAckStage1CommandMsg, NetAckStage2CommandMsg

The refactoring preserves all original networking behavior while making the codebase significantly cleaner and more maintainable. No issues found.

Confidence Score: 5/5

• This PR is safe to merge with minimal risk
• The refactoring is well-executed with clear improvements to code quality. The author confirmed basic 2-player network testing passed, and noted file transfer + VC6/Retail client testing remains in the TODO. The changes are purely structural refactoring that preserves existing behavior through virtual function dispatch. The fixed field ordering bug in packet reading improves correctness. All changes compile individually per the TODO checklist.
• No files require special attention - all changes follow consistent patterns

Important Files Changed

Filename	Overview
Core/GameEngine/Include/GameNetwork/NetPacketStructs.h	New header file introducing templated packet structure system with helper functions for network serialization. Well-organized with packed structs for network compatibility.
Core/GameEngine/Source/GameNetwork/NetPacketStructs.cpp	New implementation file providing copyBytes functions for all network command types. Systematically replaces old buffer filling code with cleaner template-based approach.
Core/GameEngine/Source/GameNetwork/NetPacket.cpp	Removed ~600 lines of repetitive FillBufferWithXXX functions, replaced with single virtual function call. Field ordering in ConstructNetCommandMsgFromRawData fixed to match packet structure.
Core/GameEngine/Include/GameNetwork/NetCommandMsg.h	Replaced getPackedByteCount with getSizeForNetPacket/getSizeForSmallNetPacket and copyBytesForNetPacket/copyBytesForSmallNetPacket virtual functions. Introduced NetCommandMsgT template base class for consistency.
Core/GameEngine/Source/GameNetwork/NetCommandMsg.cpp	Removed getPackedByteCount implementations, replaced with getSmallNetPacketSelect. Consolidated duplicated code in Ack command classes by introducing NetAckCommandMsg base class.

Class Diagram

%%{init: {'theme': 'neutral'}}%%
classDiagram
    class NetCommandMsg {
        <<abstract>>
        +getSizeForNetPacket()
        +copyBytesForNetPacket()
        +getSizeForSmallNetPacket()
        +copyBytesForSmallNetPacket()
        +getSmallNetPacketSelect()
    }
    
    class NetCommandMsgT~NetPacketType, SmallNetPacketType~ {
        +getSizeForNetPacket()
        +copyBytesForNetPacket()
        +getSizeForSmallNetPacket()
        +copyBytesForSmallNetPacket()
    }
    
    class NetGameCommandMsg {
        +constructGameMessage()
        +getSmallNetPacketSelect()
    }
    
    class NetAckCommandMsg {
        +getCommandID()
        +getOriginalPlayerID()
        +getSmallNetPacketSelect()
    }
    
    class NetAckBothCommandMsg
    class NetAckStage1CommandMsg
    class NetAckStage2CommandMsg
    
    class NetPacketCommandTemplate~Base, Data~ {
        +getSize()
        +copyBytes()
    }
    
    class SmallNetPacketCommandTemplate~Base, Data~ {
        +getSize()
        +copyBytes()
    }
    
    class NetPacketGameCommand
    class SmallNetPacketGameCommand
    
    NetCommandMsg <|-- NetCommandMsgT
    NetCommandMsgT <|-- NetGameCommandMsg
    NetCommandMsgT <|-- NetAckCommandMsg
    NetAckCommandMsg <|-- NetAckBothCommandMsg
    NetAckCommandMsg <|-- NetAckStage1CommandMsg
    NetAckCommandMsg <|-- NetAckStage2CommandMsg
    
    NetPacketCommandTemplate ..> NetGameCommandMsg : uses
    SmallNetPacketCommandTemplate ..> NetGameCommandMsg : uses
    NetPacketGameCommand --|> NetPacketCommandTemplate
    SmallNetPacketGameCommand --|> SmallNetPacketCommandTemplate

Loading

_{Last reviewed commit: 86faf0d}

[greptile-apps]

_{12 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[stephanmeesters]

Would it make sense to use macros? For some of the repeating stuff like:

size_t NetDisconnectPlayerCommandMsg::getSizeForNetPacket() const {
	return NetPacketDisconnectPlayerCommand::getSize(*this);
}

size_t NetDisconnectPlayerCommandMsg::copyBytesForNetPacket(UnsignedByte* buffer, const NetCommandRef& ref) const {
	return NetPacketDisconnectPlayerCommand::copyBytes(buffer, ref);
}

size_t NetDisconnectPlayerCommandMsg::getSizeForSmallNetPacket(const Select* select) const {
	return SmallNetPacketDisconnectPlayerCommand::getSize(*this, select);
}

size_t NetDisconnectPlayerCommandMsg::copyBytesForSmallNetPacket(UnsignedByte* buffer, const NetCommandRef& ref, const Select* select) const {
	return SmallNetPacketDisconnectPlayerCommand::copyBytes(buffer, ref, select);
}

virtual size_t getSizeForNetPacket() const;
virtual size_t copyBytesForNetPacket(UnsignedByte* buffer, const NetCommandRef& ref) const;
virtual size_t getSizeForSmallNetPacket(const Select* select = nullptr) const;
virtual size_t copyBytesForSmallNetPacket(UnsignedByte* buffer, const NetCommandRef& ref, const Select* select = nullptr) const;
virtual Select getSmallNetPacketSelect() const;

[xezon]

Preferably a template. I will think about it.

[xezon]

Replicated to Generals.

[xezon]

Would it make sense to use macros? For some of the repeating stuff like:

Simplified in fixup commit with template.

[bobtista]

buffer + size? That's what NetPacketFileCommandData::copyBytes does

[xezon]

Indeed. This was strong bug. Fixed.

[bobtista]

Would
size += network::writeBytes(buffer + size, (const UnsignedByte*)cmdMsg->getText().str(), textLength * sizeof(WideChar));
be good? writeStringWithoutNull always writes the full string, and we truncate to textLength - so we may record the size wrong for messages longer than 255 characters, right?

[xezon]

Indeed. This was very unsafe. Fixed.

[bobtista]

const?

[xezon]

Indeed. This was bug, preventing virtual override in NetAckCommandMsg. Fixed.