fix(network): correctly map PlayerIndex to SlotIndex in CRC validation

[fbraz3]

Description

A recent bugfix intended to prevent spurious mismatches from disconnected players has inadvertently disabled CRC mismatch detection for all active players. This allows games to completely desync (e.g., destroyed buildings on one client but not on the other) without the game ever raising the "Mismatch" error screen or halting the match.

This was discovered during a GeneralsX test running a LAN game between Mac and Linux. The main base on one side had been completely destroyed, while on the other side it still had about 30% life remaining, yet no mismatch error was triggered and the game continued to run smoothly in a completely desynchronized state.

Root Cause

The issue stems from the commit by Caball009 on 14/06/2026 in GameLogic::processCommandList (GameLogic.cpp):

if (!TheNetwork->isPlayerConnected(it->first))
    continue;

In the m_cachedCRCs map, the key (it->first) is the Player Index (e.g., 3, 4). However, the TheNetwork->isPlayerConnected() function expects a Network Slot Index (e.g., 0, 1).

Because the PlayerIndex does not match the SlotIndex, the function checks the connection status of invalid or empty network slots, returning FALSE for all human players. Consequently, the loop continues for every CRC, skipping the referenceCRC != crc check entirely.

Changes

To correctly determine if the player is connected, we must first map their PlayerIndex back to their SlotIndex by matching the network display name, similar to how it is handled when receiving the CRC in GameLogicDispatch::onLogicCrc.

This fix correctly maps the PlayerIndex to the SlotIndex before evaluating isPlayerConnected(), restoring proper CRC mismatch detection for active players while retaining the intent of ignoring disconnected players.

[greptile-apps]

Greptile Summary

This PR fixes a regression introduced by Caball009's disconnect-guard commit where PlayerIndex was mistakenly passed directly to isPlayerConnected(), which expects a SlotIndex — causing the guard to always skip CRC comparison and silently allow full game desyncs. The fix adds a PlayerIndex → SlotIndex mapping table (m_slotIndices) populated at game start via resolveSlotIndices(), then looks up the correct slot before calling isPlayerConnected().

• PlayerList::resolveSlotIndices() iterates occupied TheGameInfo slots, resolves each to the player with internal name \"player{N}\", and caches the mapping at game start (called from newGame()). A TheGameInfo null-guard prevents crashes in single-player campaigns where TheGameInfo is not set.
• GameLogic::processCommandList (both Generals and Zero Hour) now calls getSlotIndex(playerIndex) and uses the resulting slot index to guard isPlayerConnected(), restoring mismatch detection for active players while correctly skipping disconnected ones.
• Slot lookup approach: resolveSlotIndices uses the internal \"player{N}\" naming convention rather than the display-name comparison used in onLogicCrc; these converge on the same result given the game's slot-naming invariant, but the assumption is implicit.

Confidence Score: 4/5

The primary desync regression is correctly restored; the remaining concern is that slot lookup via internal "player{N}" names is an implicit convention that isn't enforced or verified at startup, so a mismatch between the naming convention and actual player setup would silently revert to the broken behaviour.

The core mapping fix in processCommandList is sound — Byte is typedef'd to signed char, so the -1 sentinel sign-extends correctly to Int and the slotIndex >= 0 guard works as intended. resolveSlotIndices() correctly guards against null TheGameInfo. The subtle gap is that the "player{N}" naming assumption in resolveSlotIndices() is never validated against what onLogicCrc uses (display-name matching): if any map or mode breaks that convention, slot indices silently go unresolved and the disconnect guard is bypassed again.

PlayerList.cpp (both Generals and GeneralsMD) — specifically resolveSlotIndices(), which relies on the implicit "player{slotIndex}" internal-name convention to build the mapping table.

Important Files Changed

Filename	Overview
Generals/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp	Core fix: replaces direct PlayerIndex pass to isPlayerConnected with getSlotIndex lookup first; logic is correct since Byte is signed char and -1 sentinel sign-extends cleanly to Int
Generals/Code/GameEngine/Source/Common/RTS/PlayerList.cpp	Adds resolveSlotIndices() with TheGameInfo null-guard, sets up PlayerIndex→SlotIndex mapping using "player{N}" naming convention; approach differs from onLogicCrc's display-name match but relies on the same underlying slot-naming invariant
Generals/Code/GameEngine/Include/Common/PlayerList.h	Adds three new public methods and Byte m_slotIndices[MAX_PLAYER_COUNT] member; declaration is clean and consistent with existing style
GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp	Mirror of the Generals fix for Zero Hour; identical change, same correctness assessment
GeneralsMD/Code/GameEngine/Source/Common/RTS/PlayerList.cpp	Mirror of the Generals PlayerList changes for Zero Hour; same resolveSlotIndices logic, same considerations apply
GeneralsMD/Code/GameEngine/Include/Common/PlayerList.h	Mirror header for Zero Hour; identical additions to the Generals header, no issues

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant N as Network
    participant GLD as GameLogicDispatch
    participant GL as GameLogic::processCommandList
    participant PL as PlayerList
    participant GI as TheGameInfo

    Note over PL,GI: Game start – PlayerList::newGame()
    PL->>GI: getSlot(i) for each slot
    GI-->>PL: "GameSlot* (occupied/empty)"
    PL->>PL: "findPlayerWithNameKey("player{i}")"
    PL->>PL: setSlotIndex(playerIndex, slotIndex)

    Note over N,GL: During game – CRC validation
    N->>GLD: onLogicCrc(msg)
    GLD->>N: "getPlayerName(i) == displayName?"
    GLD->>N: isPlayerConnected(slotIndex)
    GLD->>GL: store CRC in m_cachedCRCs[playerIndex]

    GL->>GL: processCommandList (iterate m_cachedCRCs)
    GL->>PL: getSlotIndex(playerIndex)
    PL-->>GL: slotIndex (or -1 if unmapped)
    alt "slotIndex >= 0"
        GL->>N: isPlayerConnected(slotIndex)
        alt disconnected
            GL->>GL: continue (skip CRC comparison)
        else connected
            GL->>GL: compare CRC vs referenceCRC
            alt mismatch
                GL->>GL: "sawCRCMismatch = TRUE"
            end
        end
    else "slotIndex == -1 (unmapped)"
        GL->>GL: compare CRC vs referenceCRC (conservative)
    end

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant N as Network
    participant GLD as GameLogicDispatch
    participant GL as GameLogic::processCommandList
    participant PL as PlayerList
    participant GI as TheGameInfo

    Note over PL,GI: Game start – PlayerList::newGame()
    PL->>GI: getSlot(i) for each slot
    GI-->>PL: "GameSlot* (occupied/empty)"
    PL->>PL: "findPlayerWithNameKey("player{i}")"
    PL->>PL: setSlotIndex(playerIndex, slotIndex)

    Note over N,GL: During game – CRC validation
    N->>GLD: onLogicCrc(msg)
    GLD->>N: "getPlayerName(i) == displayName?"
    GLD->>N: isPlayerConnected(slotIndex)
    GLD->>GL: store CRC in m_cachedCRCs[playerIndex]

    GL->>GL: processCommandList (iterate m_cachedCRCs)
    GL->>PL: getSlotIndex(playerIndex)
    PL-->>GL: slotIndex (or -1 if unmapped)
    alt "slotIndex >= 0"
        GL->>N: isPlayerConnected(slotIndex)
        alt disconnected
            GL->>GL: continue (skip CRC comparison)
        else connected
            GL->>GL: compare CRC vs referenceCRC
            alt mismatch
                GL->>GL: "sawCRCMismatch = TRUE"
            end
        end
    else "slotIndex == -1 (unmapped)"
        GL->>GL: compare CRC vs referenceCRC (conservative)
    end

Loading

_{Reviews (6): Last reviewed commit: "fix(network): Pre-compute player slot in..." | Re-trigger Greptile}

[greptile-apps]

Silent disconnection guard bypass when slot lookup fails

When slotIndex remains -1 (either because getNthPlayer returns null, or because no network slot's getPlayerName() matches the player's display name), the condition slotIndex >= 0 && !isPlayerConnected(slotIndex) short-circuits to false, so the continue is never taken. That means a disconnected player whose name has already been cleared by the network layer (common on disconnect) will still have their stale cached CRC included in the comparison, potentially producing the same spurious mismatch that Caball009's original commit was trying to avoid.

The same pattern appears identically in GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp line 2688–2689.

If the intent is to treat "unresolvable slot" conservatively (always check), the current behaviour is defensible. But if getPlayerName() can return empty after disconnection, you may want to also continue when slotIndex == -1 — or at least add a comment explaining why proceeding is safe in that case.

Prompt To Fix With AI

This is a comment left during a code review.
Path: Generals/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp
Line: 2362-2363

Comment:
**Silent disconnection guard bypass when slot lookup fails**

When `slotIndex` remains `-1` (either because `getNthPlayer` returns null, or because no network slot's `getPlayerName()` matches the player's display name), the condition `slotIndex >= 0 && !isPlayerConnected(slotIndex)` short-circuits to `false`, so the `continue` is never taken. That means a disconnected player whose name has already been cleared by the network layer (common on disconnect) will still have their stale cached CRC included in the comparison, potentially producing the same spurious mismatch that Caball009's original commit was trying to avoid.

The same pattern appears identically in `GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp` line 2688–2689.

If the intent is to treat "unresolvable slot" conservatively (always check), the current behaviour is defensible. But if `getPlayerName()` can return empty after disconnection, you may want to also `continue` when `slotIndex == -1` — or at least add a comment explaining why proceeding is safe in that case.

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

[xezon]

What about this comment?

[greptile-apps]

The new @bugfix comment only explains the index mapping fix, but doesn't address what happens when slotIndex == -1 — which is the edge case my review flagged. A reader looking at:

if (slotIndex >= 0 && !TheNetwork->isPlayerConnected(slotIndex))
    continue;

has no way to know whether falling through when slotIndex == -1 is intentional ("always validate CRC if we can't resolve the slot") or an oversight ("might miss the disconnected-player guard").

The comment should be extended to make the intent explicit. For example:

Suggested change

	if (slotIndex >= 0 && !TheNetwork->isPlayerConnected(slotIndex))
	continue;
	// TheSuperHackers @bugfix Caball009 14/06/2026 Check if player is still connected,
	// to avoid spurious mismatches at low CRC intervals, e.g. every frame.
	// TheSuperHackers @bugfix felipebraz 05/07/2026 Map PlayerIndex to SlotIndex since isPlayerConnected expects a SlotIndex.
	// If slotIndex cannot be resolved (player not found or name cleared), fall through and validate the CRC anyway.

Alternatively, if the intent is actually to skip unresolvable slots (matching the conservative spirit of Caball009's original fix), the guard should be:

if (slotIndex < 0 || !TheNetwork->isPlayerConnected(slotIndex))
    continue;

Which of those two behaviours is intended?

[Caball009]

Good catch.

It would be better if the conversion from player index to slot index was extracted to a separate function. It'd also be useful in other places in the code. An example is the recorder code, so I don't think it should rely on anything from the network. I think it can be done once, before the start of the match.

Perhaps something like this:

//class PlayerList
//Byte m_slotIndices[MAX_PLAYER_COUNT];
//memset(m_slotIndices, -1, sizeof(m_slotIndices));

void PlayerList::setSlotIndex(PlayerIndex playerIndex, Byte slotIndex)
{
	if (playerIndex >= 0 && playerIndex < ARRAY_SIZE(m_slotIndices))
	{
		m_slotIndices[playerIndex] = slotIndex;
	}
}

Byte PlayerList::getSlotIndex(PlayerIndex playerIndex) const
{
	if (playerIndex >= 0 && playerIndex < ARRAY_SIZE(m_slotIndices))
	{
		return m_slotIndices[playerIndex];
	}

	return -1;
}

void resolveSlotIndices()
{
	AsciiString playerName;

	for (int i = 0; i < MAX_SLOTS; ++i)
	{
		const GameSlot* slot = TheGameInfo->getSlot(i);
		if (!slot || !slot->isOccupied())
			continue;

		playerName.format("player%d", i);

		Player* player = ThePlayerList->findPlayerWithNameKey(TheNameKeyGenerator->nameToKey(playerName));
		if (player)
		{
			ThePlayerList->setSlotIndex(player->getPlayerIndex(), i);
		}
	}
}

The change to GameLogic::processCommandList becomes trivial then:

const int slotIndex = ThePlayerList->getSlotIndex(it->first);
if (slotIndex >= 0 && !TheNetwork->isPlayerConnected(slotIndex))
	continue;

[fbraz3]

thanks for the feedback. I've updated the code as suggested.

[fbraz3]

@Caball009 build pipeline fixed, please review.

[fbraz3]

Thanks @greptile-apps! Fixed the null dereference bug in resolveSlotIndices().

[greptile-apps]

The null guard is already visible in the diff and the thread is marked resolved — looks good. The if (!TheGameInfo) return; early-out in resolveSlotIndices() correctly covers the single-player campaign case where TheGameInfo stays nullptr.

There's still the open P2 comment about the slotIndex == -1 bypass case in GameLogic.cpp — worth addressing if you haven't already.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.

[xezon]

Style: nearby functions have a line comment above them

[xezon]

1 excess blank line

[xezon]

Better: std::fill

memset only works here because the indices are 1 byte each.

[xezon]

This is not supposed to be public, right?

[xezon]

This is not supposed to be public, right?

[xezon]

Excess blank line

[xezon]

Any reason why the PlayerList class owns the slot indices as opposed to the GameInfo class owning the player indices?

I am not sure right now which one is better, but my first instinct would have been to tie it to GameInfo class, because that is closer to the slots. Do we have any opinions on this?

[Caball009]

I think PlayerList is preferable because there are a couple of places where it's not clear to me if we could easily access GameInfo.

You can search the code base for player%d to find them; one example:

GeneralsGameCode/Core/GameEngine/Source/GameNetwork/NetCommandMsg.cpp

Lines 156 to 158 in 480a9a7

	AsciiString name;
	name.format("player%d", getPlayerID());
	retval->friend_setPlayerIndex( ThePlayerList->findPlayerWithNameKey(TheNameKeyGenerator->nameToKey(name))->getPlayerIndex());