bugfix(network): Reset network command id to keep commands in chronological order

[Caball009]

Synchronized network commands are given a unique id so that they can be sorted chronologically. The id needs to be unique per execution frame. It's a uint16 value so it'll overflow quite fast, especially because it's never reset and the starting value is 64000 by default.

When the network runahead decreases it's expected that multiple frames are executed right after each other. I noticed an issue with the overflow when I set the CRC interval to once per frame. When the overflow happened for a client combined with a decrease of the runahead, the order of the CRC messages was no longer guaranteed to be in chronological order for that execution frame. That would cause a mismatch if the overflow didn't happen for all clients at the same time.

With a starting value of 64000 and a CRC message every frame the first overflow happens after ~25 seconds. You can see it mismatch here because of the overflow (this was with a special test build);
https://www.youtube.com/live/V_l-q9Y-DmA?t=621s
https://www.youtube.com/live/V_l-q9Y-DmA?t=9499s

I've added some test code in the first commit. If that's used without this fix, it becomes very apparent that the messages are not in the right order when the command id overflows. On top of that, it's very likely that the disconnect bug (disconnect screen with everyone at ~59XXX msec) occurs frequently.

I intend to do a follow up PR to make the static commandID variable a part of the NetCommandMsg class.

[greptile-apps]

Greptile Summary

This PR fixes a uint16_t command ID overflow bug in the network synchronization layer. The GenerateNextCommandID() function previously used a function-local static initialized to 64000 that was never reset, causing overflow after ~1535 commands and breaking chronological ordering in NetCommandList::addMessage().

• NetworkUtil.cpp: Moves commandID from a function-local static to a file-scoped s_commandID initialized to 0, adds a ResetCommandID() function, and changes the return expression from pre-increment to post-increment (return s_commandID++).
• Network.cpp: Calls ResetCommandID() in Network::init() and adds a periodic reset in Network::update() every MAX_FRAMES_AHEAD (128) frames, gated on m_localStatus == NETLOCALSTATUS_INGAME and a new m_lastFrameResetCommandID member to track the last reset frame.

Confidence Score: 5/5

The change is a targeted, well-scoped fix to a numeric overflow bug in the command ID generator; all clients reset at the same synchronized game frame, so cross-client ordering remains consistent.

The core fix (moving commandID to a file-scoped static, initializing to 0, adding ResetCommandID(), and calling it in init() and periodically in update()) is logically sound. The reset interval (MAX_FRAMES_AHEAD = 128 frames) is large enough to guarantee monotonic IDs within any single execution-frame batch, and because all clients share the same TheGameLogic->getFrame() counter, resets happen synchronously across all peers. No data is at risk, and the only observable behavior change is eliminating the spurious sort inversion that was causing CRC mismatches and false disconnects.

No files require special attention; the changes are contained to the command ID generation and periodic reset logic.

Important Files Changed

Filename	Overview
Core/GameEngine/Source/GameNetwork/NetworkUtil.cpp	Refactors commandID from a function-local static to a file-scoped static s_commandID, resets it to 0 instead of 64000, changes return to post-increment, and adds ResetCommandID().
Core/GameEngine/Source/GameNetwork/Network.cpp	Adds m_lastFrameResetCommandID member, calls ResetCommandID() in init(), and adds a periodic reset in update() every MAX_FRAMES_AHEAD frames; also refactors frame into a local variable to reduce repeated TheGameLogic->getFrame() calls.
Core/GameEngine/Include/GameNetwork/networkutil.h	Adds void ResetCommandID() declaration alongside the existing GenerateNextCommandID() declaration; already uses #pragma once.
Core/GameEngine/Source/GameNetwork/NetCommandList.cpp	Adds an @info comment documenting why the sorting in addMessage() doesn't handle ID overflow directly and why resetting the ID is the preferred retail-compatible fix.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Network::init] --> B[ResetCommandID\ns_commandID = 0\nm_lastFrameResetCommandID = 0]
    B --> C[Game loop starts]
    C --> D[Network::update called\nframe = TheGameLogic::getFrame]
    D --> E{m_localStatus ==\nNETLOCALSTATUS_INGAME?}
    E -- No --> H
    E -- Yes --> F{frame + m_runAhead >\nm_lastExecutionFrame?}
    F -- No --> H
    F -- Yes --> G{frame >=\nm_lastFrameResetCommandID\n+ MAX_FRAMES_AHEAD?}
    G -- No --> H
    G -- Yes --> R[ResetCommandID\ns_commandID = 0\nm_lastFrameResetCommandID = frame]
    R --> H[GetCommandsFromCommandList]
    H --> I[GenerateNextCommandID called\nreturns s_commandID++]
    I --> J[Commands sorted in\nNetCommandList::addMessage\nby type → playerID → commandID]
    J --> K[AllCommandsReady?\nRelay to TheCommandList]
    K --> D

Loading

_{Reviews (5): Last reviewed commit: "Added code to reset command id." | Re-trigger Greptile}

[helmutbuhler]

Resetting the command id just delays the potential mismatch, right? Wouldn't it be better to sort the command ids properly, even if that rarely causes mismatches with retail clients?

[Caball009]

Resetting the command id just delays the potential mismatch, right?

It's reset every N frames, so it's not just delayed.

[xezon]

The id needs to be unique per execution frame.

Can we just reset it to 0 or 1 at the beginning of every new frame? Or does that cause issues? What is the reason for it to reset every 128 frames now?

[xezon]

Is it ok to start it at 0 when retail clients still start at 64000 ? I suspect this means every client can make up his own Id numbers as he pleases ?

Any clue why the original author picked 64000 as a start for it ?

[Caball009]

Is it ok to start it at 0 when retail clients still start at 64000 ?

Yes, this is ok. It's an important argument in favor of resetting the command ID as opposed to fixing the sorting implementation. The latter is not retail-compatible and relatively complicated.

I suspect this means every client can make up his own Id numbers as he pleases ?

Yes, each client can use their own value.

Any clue why the original author picked 64000 as a start for it ?

Not sure. Perhaps they were testing overflow behavior and forgot to change it back. If that's correct, I suppose the question becomes why didn't they notice the issues that come with overflows. They didn't have multiple instances so they'd be more constrained in their ability to stress test it, but I'm only speculating.

[xezon]

I do not understand this condition. Can you explain it?

[helmutbuhler]

When it's reset every 128 frames, isn't it possible that a packet from frame 127 comes in after it's already reset and then gets sorted wrong?

[xezon]

My current understanding is that the ids only matter per frame, not across different frames. But then I do not understand why it needs to be reset every 128 frames instead of every frame.