fix(logic): Add nullptr check to thisPlayer in GameLogic::logicMessageDispatcher

[Caball009]

This PR changes the nullptr assertion on thisPlayer in GameLogic::logicMessageDispatcher to an early return. Although there are a couple of places in this function that check explicitly for it, a lot of code assumes that this pointer is not null, so it makes sense to check that up front.

Commit 1 adds the check.
Commit 2 removes code that's become redundant (implicitly).
Commit 3 changes msg->getPlayerIndex to thisPlayer->getPlayerIndex for the sole purpose of consistency with code that already uses the latter.

TODO:

• Replicate in Generals.

[Mauller]

on 645 we have

case GameMessage::MSG_ENABLE_RETALIATION_MODE:
{
	//Logically turns on or off retaliation mode for a specified player.
	Int playerIndex = msg->getArgument( 0 )->integer;
	Bool enableRetaliation = msg->getArgument( 1 )->boolean;

	Player *player = ThePlayerList->getNthPlayer( playerIndex );
	if( player )
	{
		player->setLogicalRetaliationModeEnabled( enableRetaliation );
	}
	break;
}

I wonder if this needs clarifying that player here is a targetPlayer and not the same as thisPlayer

I am guessing that Int playerIndex = msg->getArgument( 0 )->integer; returns a different value compared to msg->getPlayerID

[stephanmeesters]

I wonder if this needs clarifying that player here is a targetPlayer and not the same as thisPlayer

I am guessing that Int playerIndex = msg->getArgument( 0 )->integer; returns a different value compared to msg->getPlayerID

I have a PR planned for this one which makes the change that player = thisPlayer, which seemed to pass 1k replays, but wanted to test that a bit more

[greptile-apps]

Greptile Summary

This PR hardens GameLogic::logicMessageDispatcher by replacing an assertion on thisPlayer (which would silently continue past a null pointer in release builds) with a proper early-return guard paired with a DEBUG_CRASH. Once the guard is in place, all the individual per-case player == nullptr checks become redundant and are removed, and two callsites are updated from msg->getPlayerIndex() to the equivalent thisPlayer->getPlayerIndex() for style consistency.

Key changes:

• Early return guard (line 353–357): thisPlayer == nullptr now causes a DEBUG_CRASH + return, preventing execution from continuing with a null pointer in release builds — a clear safety improvement over the old DEBUG_ASSERTCRASH.
• Removed redundant local lookups in MSG_CREATE_SELECTED_GROUP, MSG_REMOVE_FROM_SELECTED_GROUP, MSG_DESTROY_SELECTED_GROUP, MSG_CREATE_TEAM*, MSG_SELECT_TEAM*, MSG_ADD_TEAM*, and MSG_PURCHASE_SCIENCE — all correctly replaced with the already-validated thisPlayer.
• Consistency cleanup: msg->getPlayerIndex() replaced with thisPlayer->getPlayerIndex() where thisPlayer is now guaranteed valid.
• The PR description notes an open TODO to replicate this fix in the vanilla Generals directory, which is tracked in the PR checklist.

Confidence Score: 4/5

• This PR is safe to merge; the early-return guard is a clear correctness improvement and all redundant checks are correctly removed.
• The change is logically sound: the early-return replaces an assertion that would silently continue with a null pointer in release builds. Every removed per-case null check was genuinely redundant after the guard. The thisPlayer->getPlayerIndex() substitution is equivalent since thisPlayer is non-null at those points. The only outstanding item is the open TODO to replicate the same fix in the vanilla Generals directory.
• No files require special attention, though the matching Generals/ counterpart file should receive the same fix per the PR checklist.

Important Files Changed

Filename	Overview
GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogicDispatch.cpp	Replaces the DEBUG_ASSERTCRASH on thisPlayer with an early return guard, then removes all redundant per-case null checks for player that had been doing the same lookup. Minor consistency cleanup replacing msg->getPlayerIndex() with thisPlayer->getPlayerIndex() in a couple of places.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[logicMessageDispatcher called] --> B[Resolve thisPlayer via msg->getPlayerIndex]
    B --> C{thisPlayer == nullptr?}
    C -- Yes --> D[DEBUG_CRASH + return early]
    C -- No --> E[Initialize currentlySelectedGroup if in-game]
    E --> F{switch msg->getType}
    F --> G[MSG_CREATE_SELECTED_GROUP\nthisPlayer->getPlayerMask]
    F --> H[MSG_REMOVE_FROM_SELECTED_GROUP\nthisPlayer->getPlayerMask]
    F --> I[MSG_DESTROY_SELECTED_GROUP\nthisPlayer->setCurrentlySelectedAIGroup]
    F --> J[MSG_CREATE_TEAM*\nthisPlayer->processCreateTeamGameMessage]
    F --> K[MSG_SELECT_TEAM*\nthisPlayer->processSelectTeamGameMessage]
    F --> L[MSG_ADD_TEAM*\nthisPlayer->processAddTeamGameMessage]
    F --> M[MSG_LOGIC_CRC\nm_cachedCRCs indexed by thisPlayer->getPlayerIndex]
    F --> N[MSG_PURCHASE_SCIENCE\nthisPlayer->attemptToPurchaseScience]
    F --> O[... other cases ...]

Loading

_{Last reviewed commit: c0d0f24}