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 is a substantial refactor of the network packet serialisation layer, replacing ~50 per-type addXxxCommand / isRoomForXxxMessage / FillBufferWithXxx free functions with a unified polymorphic design built around two CRTP template hierarchies (NetPacketCommandTemplate / SmallNetPacketCommandTemplate), a new SmallNetPacketCommandBaseSelect bitfield for dynamic field selection, and a NetCommandMsgT<Fixed, Small> mixin that wires each message class to its packet structs. The refactor significantly reduces code duplication and is reported as passing multiplayer compatibility tests against retail clients.

Key points:

• The new unified addCommand() path correctly performs the size check before updating any m_last* state, preserving the atomicity invariant.
• updateLastCommandId is captured from the pre-mask select.useCommandId, so m_lastCommandID is always advanced for commands that use IDs — correctly fixing the sequential-compression invariant.
• Three getSmallNetPacketSelect() implementations (NetDisconnectFrameCommandMsg, NetDisconnectScreenOffCommandMsg, NetFrameResendRequestCommandMsg) return useFrame=1, but their corresponding fixed-base structs have the frame field commented out. This may silently write an executionFrame field for these command types that was never written by the old code, representing a behavioural change worth verifying.
• The virtual overrides inside NetCommandMsgT are implicitly private (class default), which is functionally harmless for polymorphic use but inconsistent with the public declarations in NetCommandMsg and can generate compiler warnings.

Confidence Score: 3/5

• This PR is safe to merge with minor concerns — the refactor is well-structured and has passed multiplayer compatibility testing, but three getSmallNetPacketSelect() values may silently change the wire format for disconnect/resend command types.
• Extensive manual testing (VC6 + retail clients, file transfer, 2-player matches) has been completed and passed. The core addCommand atomicity and sequential-ID compression invariants are correctly implemented. The concern about useFrame=1 on three commands that previously had no frame in their fixed-base structs lowers confidence slightly, as it represents an unverified behavioural change in the normal packet path for those command types. The private access specifier on NetCommandMsgT's virtual overrides is a style issue with no runtime impact.
• Core/GameEngine/Source/GameNetwork/NetCommandMsg.cpp — the getSmallNetPacketSelect() implementations for NetDisconnectFrameCommandMsg, NetDisconnectScreenOffCommandMsg, and NetFrameResendRequestCommandMsg should be verified against the original per-command write logic to confirm useFrame=1 is intentional.

Important Files Changed

Filename	Overview
Core/GameEngine/Include/GameNetwork/NetPacketStructs.h	Major new file: introduces SmallNetPacketCommandBaseSelect bitfield, SmallNetPacketCommandBase with dynamic field selection, NetPacketCommandTemplate/SmallNetPacketCommandTemplate CRTP templates, network write helpers (writePrimitive, writeObject, etc.), and per-command Base/Data struct pairs. Represents the core abstraction of the refactor. No critical issues found; the write helpers are correctly placed before #pragma pack(push,1).
Core/GameEngine/Source/GameNetwork/NetPacketStructs.cpp	New implementation file: contains all copyBytes / getSize logic for every packet command type. Uses nullptr correctly, const-correctness is good. Each Base::copyBytes correctly mirrors its CommandBase struct's included/excluded fields.
Core/GameEngine/Include/GameNetwork/NetCommandMsg.h	Introduces NetCommandMsgT CRTP template, NetAckCommandMsg base class consolidating the three ack types, and replaces getPackedByteCount() with getSizeForNetPacket/SmallNetPacket virtual interface. The virtual overrides in NetCommandMsgT are implicitly private (access specifier omitted), which is functionally correct but unusual and potentially causes compiler warnings.
Core/GameEngine/Source/GameNetwork/NetCommandMsg.cpp	Replaces per-type getPackedByteCount() with getSmallNetPacketSelect() for every command type. Three implementations (NetDisconnectFrameCommandMsg, NetDisconnectScreenOffCommandMsg, NetFrameResendRequestCommandMsg) return useFrame=1 despite the corresponding fixed-base structs having frame commented out, which may silently change the wire format for those command types in the normal packet path.
Core/GameEngine/Source/GameNetwork/NetPacket.cpp	Core packet logic consolidated into a single addCommand() method. Size check now correctly precedes all state updates (m_last*), and updateLastCommandId captures the pre-masking useCommandId to ensure m_lastCommandID is always advanced for command types that use IDs. Atomic state update behavior is preserved.

Class Diagram

%%{init: {'theme': 'neutral'}}%%
classDiagram
    class NetCommandMsg {
        +getSizeForNetPacket() size_t*
        +copyBytesForNetPacket() size_t*
        +getSizeForSmallNetPacket() size_t*
        +copyBytesForSmallNetPacket() size_t*
        +getSmallNetPacketSelect() Select*
    }

    class NetCommandMsgT~NetPacketType,SmallNetPacketType~ {
        -getSizeForNetPacket() size_t
        -copyBytesForNetPacket() size_t
        -getSizeForSmallNetPacket() size_t
        -copyBytesForSmallNetPacket() size_t
    }

    class NetAckCommandMsg {
        +getSmallNetPacketSelect() Select
        +getCommandID() UnsignedShort
        +getOriginalPlayerID() UnsignedByte
    }

    class NetPacketCommandTemplate~Base,Data~ {
        +getSize(msg) size_t
        +copyBytes(buffer, ref) size_t
    }

    class SmallNetPacketCommandTemplate~Base,Data~ {
        +getSize(msg, select) size_t
        +copyBytes(buffer, ref, select) size_t
    }

    class SmallNetPacketCommandBase {
        +getSize(select) size_t
        +copyBytes(buffer, ref, select) size_t
    }

    class SmallNetPacketCommandBaseSelect {
        +useCommandType : 1
        +useRelay : 1
        +useFrame : 1
        +usePlayerId : 1
        +useCommandId : 1
    }

    NetCommandMsg <|-- NetCommandMsgT
    NetCommandMsgT <|-- NetAckCommandMsg
    NetCommandMsgT <|-- NetGameCommandMsg
    NetCommandMsgT <|-- NetFrameCommandMsg
    NetAckCommandMsg <|-- NetAckBothCommandMsg
    NetAckCommandMsg <|-- NetAckStage1CommandMsg
    NetAckCommandMsg <|-- NetAckStage2CommandMsg

    SmallNetPacketCommandTemplate --> SmallNetPacketCommandBase : Base
    SmallNetPacketCommandTemplate --> SmallNetPacketCommandBaseSelect : uses

    NetCommandMsgT ..> NetPacketCommandTemplate : getSizeForNetPacket
    NetCommandMsgT ..> SmallNetPacketCommandTemplate : getSizeForSmallNetPacket

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: Core/GameEngine/Include/GameNetwork/NetCommandMsg.h
Line: 77-99

Comment:
**Private virtual overrides in `NetCommandMsgT`**

The virtual method overrides inside `NetCommandMsgT` are implicitly `private` (since `class` uses private-by-default access), even though the corresponding functions in `NetCommandMsg` are declared `public`. While this is valid C++ — polymorphic dispatch still works through a `NetCommandMsg*` — it creates an access mismatch that can confuse readers and may trigger compiler warnings (e.g., MSVC's diagnostic for accessibility narrowing in virtual overrides).

Consider adding an explicit `public:` access specifier before the four overrides:

```suggestion
template<typename NetPacketType, typename SmallNetPacketType>
class NetCommandMsgT : public NetCommandMsg
{
public:
	virtual size_t getSizeForNetPacket() const
	{
		return NetPacketType::getSize(*this);
	}

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

	virtual size_t getSizeForSmallNetPacket(const Select* select = nullptr) const
	{
		return SmallNetPacketType::getSize(*this, select);
	}

	virtual size_t copyBytesForSmallNetPacket(UnsignedByte* buffer, const NetCommandRef& ref, const Select* select = nullptr) const
	{
		return SmallNetPacketType::copyBytes(buffer, ref, select);
	}
};
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: Core/GameEngine/Source/GameNetwork/NetCommandMsg.cpp
Line: 1159-1178

Comment:
**`useFrame=1` inconsistent with corresponding fixed-base struct for these command types**

`NetDisconnectFrameCommandMsg::getSmallNetPacketSelect()` returns `useFrame=1`, but `NetPacketDisconnectFrameCommandBase::CommandBase` (used by the non-small `getSizeForNetPacket`/`copyBytesForNetPacket` path) has the frame field commented out — indicating that the original protocol for this command type never wrote `executionFrame`.

With `useFrame=1` in the small-packet path, `executionFrame` **will** be written to the packet whenever the execution frame advances between commands (which happens on every new game frame). This is a behavioural deviation from the pre-refactor code. Because the generic receiver parser accepts frame tags anywhere, it won't crash, but the `executionFrame` now carried on wire for these messages was not there before.

The same issue applies to:
- `NetDisconnectScreenOffCommandMsg::getSmallNetPacketSelect()` at `Core/GameEngine/Source/GameNetwork/NetCommandMsg.cpp:1188`
- `NetFrameResendRequestCommandMsg::getSmallNetPacketSelect()` at `Core/GameEngine/Source/GameNetwork/NetCommandMsg.cpp:1217`

If these command types genuinely do not need `executionFrame` in the packet (as the fixed-base structs suggest), all three should use `select.useFrame = 0`:

```suggestion
NetCommandMsg::Select NetDisconnectFrameCommandMsg::getSmallNetPacketSelect() const {
	Select select;
	select.useCommandType = 1;
	select.useRelay = 1;
	select.useFrame = 0;
	select.usePlayerId = 1;
	select.useCommandId = 1;
	return select;
}
```

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: 35fa72a}

[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.

[xezon]

Goldfish:

We did already a quicktest with map transfer and 5 min in game without issues.

I was on this patch 1 other guy on retail and then the rest on the superhackers. worked great no issues to be seen. played a like 3 games with it

[bobtista]

This looks great. Much cleaner and more readable :) LGTM

[Mauller]

Looks good and works without issue when fish tested it from what i heard