refactor(netpacket): Simplify NetPacket functions for packet buffer reads

[xezon]

• Follow up for refactor(netpacket): Simplify NetPacket functions for packet buffer writes and size tests #2329

This change simplifies all NetPacket functions for packet buffer reads.

TODO

• Basic Network testing
• Test file transfer
• Test Network matches and communications

[greptile-apps]

Greptile Summary

This PR is a significant structural simplification of the NetPacket read path. The ~700-line flat switch of readXMessage functions is replaced with a small set of composable helpers (NetPacketBuf, NetCommandDataChunk, readObject/readBytes/readStringWithNull) and per-struct readMessage static methods dispatched via the new readMessageData virtual, eliminating parallel maintenance of symmetric read/write logic.

Key improvements in this PR:

• NetPacketBuf provides bounds-safe reads via min() throughout; the old code used raw pointer arithmetic with no truncation protection.
• NetCommandDataChunk gives RAII ownership over heap-allocated network buffers, replacing manual new/delete[] pairs in setData, setFileData, and sendFile.
• m_relay uninitialised field is now zero-initialised in NetCommandRef::NetCommandRef, and a null-guard is added before m_msg->attach().
• readStringWithNull uses strnlen(src.data(), src.size()) — bounded by the remaining packet buffer, not by maxStrLen — so it correctly advances past the full on-wire string even when the string is longer than _MAX_PATH, matching the old strlcpy-return-value semantics.
• Both readStringWithoutNull and readStringWithNull guard getBufferForRead with strLen > 0, preventing the debug assertion crash on empty strings.
• The getCommandList error path uses continue (not break) and i is always advanced by the return value of constructNetCommandRef, so there is no infinite-loop risk on malformed packets.

One concern: the PR's TODO explicitly marks file-transfer testing as incomplete. The NetPacketFileCommandData::readMessage and NetPacketWrapperCommandData::readMessage paths (and the sendFile / processFile round-trip) have not been validated end-to-end yet.

Confidence Score: 3/5

• Core simplification is solid but file-transfer code paths are untested and carry one pre-existing silent-desync risk on unknown argument types.
• The structural refactor is clean and addresses all the major issues raised in previous review threads (relay initialisation, getBufferForRead guard, readStringWithNull advance, argument evaluation order). Error handling on parse failure is correct (continue, not break). The main reasons for the reduced score are: (1) file-transfer testing is explicitly marked incomplete in the TODO, leaving NetPacketFileCommandData::readMessage and NetPacketWrapperCommandData::readMessage unvalidated end-to-end; and (2) the ARGUMENTDATATYPE_UNKNOWN fallthrough in game-command argument parsing causes silent buffer-position desync, which while pre-existing is now consolidated in a single location and may be harder to catch.
• Core/GameEngine/Source/GameNetwork/NetPacketStructs.cpp (game command argument parsing), Core/GameEngine/Source/GameNetwork/ConnectionManager.cpp (file-send path untested)

Important Files Changed

Filename	Overview
Core/GameEngine/Include/GameNetwork/NetCommandMsg.h	Introduces NetCommandDataChunk RAII wrapper for network buffer ownership, adds readMessageData virtual method to the hierarchy, and converts setData/setFileData to consume a NetCommandDataChunk by non-const reference (via release()).
Core/GameEngine/Include/GameNetwork/NetPacketStructs.h	Introduces NetPacketBuf bounds-safe view and readObject/readBytes/readStringWithoutNull/readStringWithNull helpers. The getBufferForRead(0) guard and strnlen-based string advance fix previously flagged issues.
Core/GameEngine/Source/GameNetwork/NetPacket.cpp	Major simplification: removes ~700 lines of per-type readXMessage functions. The new getCommandList loop uses constructNetCommandRef which delegates to SmallNetPacketCommandBase::readMessage and then readMessageData virtual dispatch. Control flow on error correctly uses continue (not break) and i is always advanced by the return value, preventing infinite loops.
Core/GameEngine/Source/GameNetwork/NetPacketStructs.cpp	Implements all readMessage static methods for each command data type. The new SmallNetPacketCommandBase::readMessage centralises field parsing, and constructNetCommandMsg replaces the scattered switch on command type. The Repeat field type in the switch correctly falls through to default since that case is handled by the caller.
Core/GameEngine/Source/GameNetwork/ConnectionManager.cpp	Adopts NetCommandDataChunk for ownership transfer in sendFile. The len is correctly captured before readEntireAndClose() is called, and buffer lifetimes are managed cleanly via RAII. The getFilename → getPortableFilename fix is a correctness improvement.

Sequence Diagram

sequenceDiagram
    participant GCL as NetPacket::getCommandList
    participant CNR as constructNetCommandRef
    participant SBR as SmallNetPacketCommandBase::readMessage
    participant CCM as constructNetCommandMsg
    participant RMD as msg->readMessageData (virtual)
    participant CDD as CommandData::readMessage

    GCL->>GCL: NetPacketBuf buf(m_packet, m_packetLen)
    loop while i < buf.size()
        GCL->>GCL: isRepeat = (m_packet[i] == Repeat)?
        alt not Repeat
            GCL->>CNR: constructNetCommandRef(ref, commandBase, buf.offset(i))
            CNR->>SBR: readMessage(ref, base, buf)
            loop while size < buf.size()
                SBR->>SBR: switch(buf[size]) on field type
                alt CommandType / Relay / Frame / PlayerId / CommandId
                    SBR->>SBR: readObject into base field, advance size
                else Data
                    SBR->>CCM: constructNetCommandMsg(base)
                    CCM-->>SBR: NetCommandMsg* (typed)
                    SBR->>SBR: ref = NEW_NETCOMMANDREF(msg), setRelay
                    SBR-->>CNR: return size
                else Repeat / default
                    SBR-->>CNR: return size+1 (advance past unexpected byte)
                end
            end
            CNR->>RMD: ref->getCommand()->readMessageData(*ref, buf.offset(size))
            RMD->>CDD: CommandData::readMessage(ref, buf)
            CDD-->>RMD: bytes consumed
            RMD-->>CNR: bytes consumed
            CNR-->>GCL: total bytes consumed (i advanced)
            alt ref != nullptr
                GCL->>GCL: retval->addMessage(ref)
                GCL->>GCL: lastCommand = NEW_NETCOMMANDREF(msg)
            else ref == nullptr
                GCL->>GCL: DEBUG_CRASH + continue
            end
        else Repeat
            GCL->>GCL: i += NetPacketRepeatCommand::getSize()
            GCL->>GCL: construct repeated msg from lastCommand
            GCL->>GCL: retval->addMessage(msg)
            GCL->>GCL: lastCommand = NEW_NETCOMMANDREF(msg)
        end
    end
    GCL-->>GCL: return retval (NetCommandList)

Loading

Comments Outside Diff (1)

1. Core/GameEngine/Source/GameNetwork/NetPacketStructs.cpp, line 2569-2613 (link)

   Unhandled argument data type silently desynchronises remaining arguments

   The switch (lastType) has no default branch. If a packet ever carries an ARGUMENTDATATYPE_UNKNOWN (value 0) or any future type not listed here, size is not advanced. sizeBefore equals size, so the guard if (size > sizeBefore) skips addArgument, but --argsLeftForType still runs. Every subsequent argument of a known type is then read from the same buffer position as the unknown type, silently corrupting the entire argument list for that command.

   This behaviour was present in the original readGameMessageArgumentFromPacket, but this PR consolidates all argument reading here and makes it worth surfacing explicitly. Consider adding a default branch that advances size by the expected size of the unknown type, or at minimum a DEBUG_CRASH so malformed packets don't produce silent desync:

   default:
       DEBUG_CRASH(("NetPacketGameCommandData::readMessage: Unknown argument type %d", lastType));
       break;

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: Core/GameEngine/Source/GameNetwork/NetPacketStructs.cpp
   Line: 2569-2613
   
   Comment:
   **Unhandled argument data type silently desynchronises remaining arguments**
   
   The `switch (lastType)` has no `default` branch. If a packet ever carries an `ARGUMENTDATATYPE_UNKNOWN` (value `0`) or any future type not listed here, `size` is not advanced. `sizeBefore` equals `size`, so the guard `if (size > sizeBefore)` skips `addArgument`, but `--argsLeftForType` still runs. Every subsequent argument of a known type is then read from the same buffer position as the unknown type, silently corrupting the entire argument list for that command.
   
   This behaviour was present in the original `readGameMessageArgumentFromPacket`, but this PR consolidates all argument reading here and makes it worth surfacing explicitly. Consider adding a `default` branch that advances `size` by the expected size of the unknown type, or at minimum a `DEBUG_CRASH` so malformed packets don't produce silent desync:
   
   ```cpp
   default:
       DEBUG_CRASH(("NetPacketGameCommandData::readMessage: Unknown argument type %d", lastType));
       break;
   ```
   
   How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: Core/GameEngine/Source/GameNetwork/NetPacketStructs.cpp
Line: 2569-2613

Comment:
**Unhandled argument data type silently desynchronises remaining arguments**

The `switch (lastType)` has no `default` branch. If a packet ever carries an `ARGUMENTDATATYPE_UNKNOWN` (value `0`) or any future type not listed here, `size` is not advanced. `sizeBefore` equals `size`, so the guard `if (size > sizeBefore)` skips `addArgument`, but `--argsLeftForType` still runs. Every subsequent argument of a known type is then read from the same buffer position as the unknown type, silently corrupting the entire argument list for that command.

This behaviour was present in the original `readGameMessageArgumentFromPacket`, but this PR consolidates all argument reading here and makes it worth surfacing explicitly. Consider adding a `default` branch that advances `size` by the expected size of the unknown type, or at minimum a `DEBUG_CRASH` so malformed packets don't produce silent desync:

```cpp
default:
    DEBUG_CRASH(("NetPacketGameCommandData::readMessage: Unknown argument type %d", lastType));
    break;
```

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/NetPacket.cpp
Line: 447-452

Comment:
**Misleading comment about incrementing `i`**

The in-code comment reads "we have to increment i so we don't fall into an infinite loop", but `i` is already advanced by `i += constructNetCommandRef(ref, commandBase, buf.offset(i))` four lines above — *before* the `ref == nullptr` check. The `continue` itself does no additional incrementing. The comment is a remnant of the old `++i; break` pattern and may confuse future readers into thinking the `continue` is doing the work.

```suggestion
			// i was already advanced by constructNetCommandRef above.
			// The continue prevents stale commandBase state from being
			// used to process subsequent commands.
			DEBUG_CRASH(("Unrecognized packet entry, ignoring."));
```

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

_{Last reviewed commit: "Continue on unrecogn..."}

[greptile-apps]

break abandons remaining commands; old code used continue + ++i

The old code had ++i; break; (which in the enclosing switch fell through and the outer while continued with the next byte), making packet parsing resilient to a single unknown byte. The new code breaks out of the outer while loop entirely, so every command that follows a parse failure in the same packet is silently lost.

In a deterministic game engine this asymmetry (sender sends N commands; receiver processes only the first K because of one bad byte) is a desync risk. If the packet-stop semantics are intentional, please add a comment explaining the rationale; otherwise, consider continuing past the failed entry by not breaking the outer loop:

Prompt To Fix With AI

This is a comment left during a code review.
Path: Core/GameEngine/Source/GameNetwork/NetPacket.cpp
Line: 453-460

Comment:
**`break` abandons remaining commands; old code used `continue` + `++i`**

The old code had `++i; break;` (which in the enclosing `switch` fell through and the outer `while` continued with the next byte), making packet parsing resilient to a single unknown byte. The new code `break`s out of the outer `while` loop entirely, so every command that follows a parse failure in the same packet is silently lost.

In a deterministic game engine this asymmetry (sender sends N commands; receiver processes only the first K because of one bad byte) is a desync risk. If the packet-stop semantics are intentional, please add a comment explaining the rationale; otherwise, consider continuing past the failed entry by not breaking the outer loop:

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