bugfix(network): Map player index to slot index for CRC validation

[fbraz3]

• Follow-up for PR: bugfix(crc): Fix spurious mismatches for disconnected players at low CRC intervals #2796

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 from PR #2796 where isPlayerConnected() was being called with a player index instead of a slot index, causing the check to always return FALSE and skip all CRC comparisons — allowing games to fully desync without triggering the mismatch error screen. The fix introduces a m_slotIndices cache in PlayerList (populated in newGame()) that maps player indices to network slot indices, then uses getSlotIndex() in processCommandList before calling isPlayerConnected().

• PlayerList gains resolveSlotIndices() (called from newGame()), getSlotIndex(), and setSlotIndex() backed by a new m_slotIndices[MAX_PLAYER_COUNT] array initialized in init() and loadPostProcess().
• GameLogic::processCommandList now maps it->first (player index) to a slot index before calling isPlayerConnected(), restoring correct CRC mismatch detection for active players while still skipping disconnected ones.
• The same fix is mirrored in the GeneralsMD (Zero Hour) tree.

Confidence Score: 3/5

The core fix is sound, but loadPostProcess() resets the slot-index cache without rebuilding it, leaving the mapping broken for any game-load path.

The player-index to slot-index mapping introduced in newGame() is correct and directly addresses the desync regression. However, loadPostProcess() fills m_slotIndices with -1 but never calls resolveSlotIndices(), so any save/load path in a network context would leave every slot index as -1, silently making the fix inoperative for loaded games.

Generals/Code/GameEngine/Source/Common/RTS/PlayerList.cpp and its GeneralsMD mirror — specifically loadPostProcess() which resets but does not rebuild the slot-index cache.

Important Files Changed

Filename	Overview
Generals/Code/GameEngine/Source/Common/RTS/PlayerList.cpp	Adds resolveSlotIndices(), getSlotIndex(), and setSlotIndex() to maintain a player-index to slot-index cache; loadPostProcess() resets the cache but never rebuilds it.
GeneralsMD/Code/GameEngine/Source/Common/RTS/PlayerList.cpp	Mirror of Generals changes with an additional mixed-indentation inconsistency in loadPostProcess() (spaces vs tabs).
Generals/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp	Correctly maps player index to slot index via getSlotIndex() before calling isPlayerConnected(), restoring CRC mismatch detection for active players.
GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp	Identical fix to Generals version — same mapping logic correctly applied.
Generals/Code/GameEngine/Include/Common/PlayerList.h	Adds getSlotIndex() public declaration and private helpers/member array for the new slot-index cache.
GeneralsMD/Code/GameEngine/Include/Common/PlayerList.h	Mirror of Generals header changes — identical additions.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant GL as GameLogic
    participant PL as PlayerList
    participant GI as GameInfo
    participant NET as TheNetwork

    Note over GL,NET: newGame() path
    PL->>GI: getSlot(i) for each slot
    GI-->>PL: GameSlot (occupied)
    PL->>PL: findPlayerWithNameKey("player%d")
    PL->>PL: setSlotIndex(playerIndex, slotIndex)

    Note over GL,NET: processCommandList() CRC check (fixed)
    GL->>PL: "getSlotIndex(it->first)"
    PL-->>GL: slotIndex (0..N) or -1
    alt "slotIndex >= 0"
        GL->>NET: isPlayerConnected(slotIndex)
        NET-->>GL: false (disconnected)
        GL->>GL: continue (skip CRC)
    else "slotIndex == -1 (unresolved)"
        GL->>GL: fall through to compare CRC
    end

    Note over GL,NET: loadPostProcess() path (gap)
    PL->>PL: fill m_slotIndices with -1
    Note over PL: resolveSlotIndices() NOT called
    GL->>PL: "getSlotIndex(it->first)"
    PL-->>GL: -1 (always)
    GL->>GL: fall through, guard never fires

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 GL as GameLogic
    participant PL as PlayerList
    participant GI as GameInfo
    participant NET as TheNetwork

    Note over GL,NET: newGame() path
    PL->>GI: getSlot(i) for each slot
    GI-->>PL: GameSlot (occupied)
    PL->>PL: findPlayerWithNameKey("player%d")
    PL->>PL: setSlotIndex(playerIndex, slotIndex)

    Note over GL,NET: processCommandList() CRC check (fixed)
    GL->>PL: "getSlotIndex(it->first)"
    PL-->>GL: slotIndex (0..N) or -1
    alt "slotIndex >= 0"
        GL->>NET: isPlayerConnected(slotIndex)
        NET-->>GL: false (disconnected)
        GL->>GL: continue (skip CRC)
    else "slotIndex == -1 (unresolved)"
        GL->>GL: fall through to compare CRC
    end

    Note over GL,NET: loadPostProcess() path (gap)
    PL->>PL: fill m_slotIndices with -1
    Note over PL: resolveSlotIndices() NOT called
    GL->>PL: "getSlotIndex(it->first)"
    PL-->>GL: -1 (always)
    GL->>GL: fall through, guard never fires

Loading

_{Reviews (13): Last reviewed commit: "fix(network): resolve PR #2857 code revi..." | 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]

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());

[xezon]

All the various comment were marked as addressed but no update was pushed. Where is this pull request going?

[fbraz3]

All the various comment were marked as addressed but no update was pushed. Where is this pull request going?

Not fixed on 3107035 ?

[Caball009]

That commit is not part of the branch for this PR.

[Caball009]

@fbraz3 Please update this branch with your changes.

[fbraz3]

@fbraz3 Please update this branch with your changes.

I was working on GeneralsX since the weekend, and it took me all my spare time.

I will work on this for tomorrow and will let you guys know.

[Caball009]

That's good. I think it's very important that this gets fixed ASAP, definitely before the next weekly release.

[fbraz3]

@Caball009 @xezon pr updated, please check.

[Caball009]

Make sure that the changes for Zero Hour are the same as Generals. It's best to add the code for Generals last.

[fbraz3]

Make sure that the changes for Zero Hour are the same as Generals. It's best to add the code for Generals last.

done

[greptile-apps]

Want your agent to iterate on Greptile's feedback? Try greploops.

[fbraz3]

@Caball009 just adapted your script to bash

for b in $(git merge-base --fork-point main); do
      git diff $b > changes.patch;
      git apply -p2 --directory=Generals --reject --whitespace=fix changes.patch;
      rm -rf changes.patch;
done

[greptile-apps]

Byte is unsigned on ARM, making the -1 sentinel become 255

Byte is defined as typedef char Byte in BaseTypeCore.h. On ARM (Apple Silicon Mac — explicitly listed as a tested platform), char is unsigned by default, so return -1 returns unsigned char(255). When GameLogic.cpp stores this in const Int slotIndex, it zero-extends to 255 rather than sign-extending to -1. The guard slotIndex >= 0 then evaluates to true (255 ≥ 0), and isPlayerConnected(255) is called. ConnectionManager::isPlayerConnected directly indexes m_connections[playerID] (with a DEBUG_ASSERTCRASH for playerID < MAX_SLOTS): in debug builds this crashes the assertion, in release builds it is an out-of-bounds memory read on any slot lookup that failed to resolve.

The same applies to GeneralsMD/Code/GameEngine/Source/Common/RTS/PlayerList.cpp.

The return type (and array element type) should be Int to make the sentinel value unambiguous across platforms, or the array should use signed char explicitly.

Prompt To Fix With AI

This is a comment left during a code review.
Path: Generals/Code/GameEngine/Source/Common/RTS/PlayerList.cpp
Line: 497-504

Comment:
**`Byte` is unsigned on ARM, making the -1 sentinel become 255**

`Byte` is defined as `typedef char Byte` in `BaseTypeCore.h`. On ARM (Apple Silicon Mac — explicitly listed as a tested platform), `char` is unsigned by default, so `return -1` returns `unsigned char(255)`. When `GameLogic.cpp` stores this in `const Int slotIndex`, it zero-extends to `255` rather than sign-extending to `-1`. The guard `slotIndex >= 0` then evaluates to `true` (255 ≥ 0), and `isPlayerConnected(255)` is called. `ConnectionManager::isPlayerConnected` directly indexes `m_connections[playerID]` (with a `DEBUG_ASSERTCRASH` for `playerID < MAX_SLOTS`): in debug builds this crashes the assertion, in release builds it is an out-of-bounds memory read on any slot lookup that failed to resolve.

The same applies to `GeneralsMD/Code/GameEngine/Source/Common/RTS/PlayerList.cpp`.

The return type (and array element type) should be `Int` to make the sentinel value unambiguous across platforms, or the array should use `signed char` explicitly.

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

[fbraz3]

what about this greptile warning?

[Caball009]

If this is an issue for ARM, that needs to be handled separately. There's no reason to change from Byte to Int just for this.

[greptile-apps]

The technical risk is real and specific to this PR: Apple Silicon Mac is explicitly listed in the PR description as a tested platform, and getSlotIndex returning -1 as Byte will produce 255 on that platform. Since 255 >= 0 is true, the sentinel fails silently and isPlayerConnected(255) gets called — an out-of-bounds access.

That said, you're right that the fix doesn't have to be Int. A minimal in-scope fix would be to use signed char explicitly for the array and return type, which preserves the Byte-like size while making the signedness unambiguous across platforms. Either way, handling it in a follow-up PR is your call — just worth noting that the sentinel is broken on the very platform this bugfix was discovered on, so it'd be good to track.

[Caball009]

This project doesn't target ARM, nor is it the responsibility of this PR to take this into account.

[bobtista]

Agreed, can add it as a separate issue and PR, though in the interest of keeping reviewers focused on releasing 1.0, I'd set this part aside for a later date. I've also come across weird things where the original code would use weirdly wrapped numbers in my bgfx fork.

[xezon]

You can solve this simply by adding a bool PlayerList::hasSlotIndex(Int playerIndex) function. Then use that instead of comparing the returned index.

Or provide a InvalidSlotIndex = -1 value and compare against that.

I see you changed Byte to Int already, which is also ok.

[greptile-apps]

The return type was indeed changed to Int in the current diff, which resolves the original ARM/unsigned concern — good catch there.

On xezon's suggestions:

• hasSlotIndex(playerIndex) would encapsulate the validity check inside PlayerList, which is slightly cleaner.
• InvalidSlotIndex = -1 constant would make the magic number self-documenting.

Both are reasonable style improvements, but the current slotIndex >= 0 check is readable and correct as written with Int. It's a matter of preference — the fix is sound either way.

[xezon]

The bot complaints that the slot mapping will not work in save load.

[xezon]

Maybe call it assignSlotIndices ?

[xezon]

Maybe make GameInfo an argument

assignSlotIndices(const GameInfo& gameInfo)

It makes the intent of the function even clearer from the outside.

if (TheGameInfo)
  assignSlotIndices(TheGameInfo)

[xezon]

You can solve this simply by adding a bool PlayerList::hasSlotIndex(Int playerIndex) function. Then use that instead of comparing the returned index.

Or provide a InvalidSlotIndex = -1 value and compare against that.

I see you changed Byte to Int already, which is also ok.

[xezon]

I can confirm that this is called after newGame, overwriting its assignments.

[Caball009]

Is it necessary to do anything in loadPostProcess?

[fbraz3]

Hey guys, what are the remaining tasks required to complete this PR?