bugfix(sideslist): Add the general skirmish sides that Generals maps lack by tintinhamans · Pull Request #3484 · TheSuperHackers/GeneralsGameCode

tintinhamans · 2026-10-12T01:17:07Z

Generals maps only have SkirmishAmerica, SkirmishChina and SkirmishGLA. Player::initFromDict turns a general AI into a human player with no AI when no map side matches its Side, so on these maps the generals never move. The 1.03 filter hides Armored Fury and Scorched Earth for this reason.

prepareForMP_or_Skirmish now adds the general sides the way Add Skirmish Players in WorldBuilder does, so the standard skirmish scripts and their teams bind to them. Only for maps saved by the Generals WorldBuilder with the three original factions and no general, and only when all nine fit. Behind !RETAIL_COMPATIBLE_CRC, a general that plays desyncs against retail. The 1.03 filter stays in retail builds.

The map pickers warn when such a map is selected.

Map	Sides added	Warning
Generals map	yes	yes
Generals map with own AI scripts or no room for all generals	no	yes, generals will not work
Zero Hour map	no	no

Retail compatible builds never add sides.

coderabbitai · 2026-10-12T01:17:31Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Last reviewed commit: 9c795ea

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: bbe4b5e0-2bdd-4e23-9915-75fb7491131b

📥 Commits

Reviewing files that changed from the base of the PR and between f8e2cef and 9c795ea.

📒 Files selected for processing: 3

Core/GameEngine/Include/GameLogic/SidesList.h
Core/GameEngine/Source/GameClient/MapUtil.cpp
Core/GameEngine/Source/GameLogic/Map/SidesList.cpp

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.

Walkthrough

The change records polygon-trigger chunk versions, detects legacy maps without Zero Hour skirmish sides, conditionally adds missing sides, and warns during LAN, skirmish, and WOL map selection. It also limits the Armored Fury and Scorched Earth exclusions to Zero Hour builds with retail-compatible CRC mode.

Changes

Map compatibility

Layer / File(s)	Summary
Version-aware skirmish-side preparation `Core/GameEngine/Include/GameLogic/PolygonTrigger.h`, `Core/GameEngine/Source/GameLogic/Map/PolygonTrigger.cpp`, `Core/GameEngine/Include/GameLogic/SidesList.h`, `Core/GameEngine/Source/GameLogic/Map/SidesList.cpp`	Polygon-trigger parsing records the chunk version. In non-retail-compatible builds, side preparation checks faction sets, scripts, trigger version, and capacity before adding eligible missing factions.
Map detection and warnings `Core/GameEngine/Include/GameClient/MapUtil.h`, `Core/GameEngine/Source/GameClient/MapUtil.cpp`	Map inspection reads map side and script data to detect maps without General sides and determine whether sides can be added. The map-list exclusions apply only to Zero Hour builds with retail-compatible CRC mode. Warning text varies according to side eligibility and build configuration.
Map-selection warning calls `GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/{LanMapSelectMenu,SkirmishMapSelectMenu,WOLMapSelectMenu}.cpp`	LAN, skirmish, and WOL map confirmation paths call the warning function with the selected map name.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant SkirmishMapSelectMenuSystem
  participant warnIfMapLacksSkirmishSides
  participant isGeneralsMapWithoutGeneralSides
  participant MessageBox
  SkirmishMapSelectMenuSystem->>warnIfMapLacksSkirmishSides: selected map name
  warnIfMapLacksSkirmishSides->>isGeneralsMapWithoutGeneralSides: inspect map
  isGeneralsMapWithoutGeneralSides-->>warnIfMapLacksSkirmishSides: missing-side result
  warnIfMapLacksSkirmishSides->>MessageBox: display warning

Suggested reviewers: bobtista

Merge Risk: ⚪ Minimal · up to 9c795

The previously identified stale-version mismatch is fixed. No actionable merge-blocking risk remains in the inspected map-selection and side-preparation paths.

Pre-merge checks: 4

Passed checks: 4 passed

Check name	Status	Explanation
Linked Issues check		Issue `#1772` asks why Armored Fury and Scorched Earth are unavailable in Zero Hour and whether they can return. The PR keeps the 1.03 filter only for retail-compatible builds. In non-retail-compatible …
Out of Scope Changes check		The changes stay within issue `#1772`. `SidesList` repairs missing general skirmish sides under the stated map checks. `MapUtil` identifies affected maps and displays warnings. The three map-selection c…
Description check		The description clearly explains the added general skirmish sides, eligibility conditions, warnings, and retail-compatible build behavior.
Title check		The title clearly and concisely identifies the main change: adding missing general skirmish sides to Generals maps.

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

greptile-apps · 2026-10-12T01:25:18Z

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; no actionable issues remain.

Summary

Adds missing Zero Hour general sides to eligible Generals maps in non-retail-compatible builds.

Eligible Generals maps gain the missing Zero Hour sides.
Map pickers warn when a Generals map lacks general sides.
Non-retail builds show Armored Fury and Scorched Earth.

Both earlier, unnumbered findings are addressed: capacity is checked before adding sides, and conflicting player names prevent the repair. No new actionable issues were found.

Review used source inspection only. Builds and visual checks were not run; the changed menus belong to the native Windows game.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  Load[Load map] --> Retail{Retail-compatible build?}
  Retail -- Yes --> Existing[Keep existing sides]
  Retail -- No --> Eligible{Legacy map with original factions only?}
  Eligible -- No --> Existing
  Eligible -- Yes --> Safe{No custom AI scripts, name conflicts, or capacity limits?}
  Safe -- No --> Existing
  Safe -- Yes --> Add[Add every missing general side]
  Add --> Scripts[Bind standard skirmish scripts and teams]
  Existing --> Scripts

_{Reviews (4) · Last reviewed commit: "bugfix(sideslist): Add the general skirm..." · Reviewed by Greptile}

coderabbitai

Actionable comments posted: 1

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: af6ce4e8-183e-4d73-a36a-5926446a2e4a

📥 Commits

Reviewing files that changed from the base of the PR and between 0e6a06c and dc8dcda.

📒 Files selected for processing (9)

Core/GameEngine/Include/GameClient/MapUtil.h
Core/GameEngine/Include/GameLogic/PolygonTrigger.h
Core/GameEngine/Include/GameLogic/SidesList.h
Core/GameEngine/Source/GameClient/MapUtil.cpp
Core/GameEngine/Source/GameLogic/Map/PolygonTrigger.cpp
Core/GameEngine/Source/GameLogic/Map/SidesList.cpp
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/LanMapSelectMenu.cpp
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/SkirmishMapSelectMenu.cpp
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/WOLMapSelectMenu.cpp

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

bobtista · 2026-10-14T20:41:44Z

+		d.setAsciiString(TheKey_teamName, teamName);
+		d.setAsciiString(TheKey_teamOwner, playerName);
+		d.setBool(TheKey_teamIsSingleton, true);
+		addTeam(&d);


I think the added teams get dropped. With no side scripts, prepareForMP_or_Skirmish clears m_skirmishTeamrec and loads the teams from SkirmishScripts.scb. The added sides still help, so maybe only the addTeam part can go.

Oof, realised my test rig missed the extra scb.. adjusted and will retest.

you were right, the scb brings those teams once the sides exist. dropped the addTeam and the team name check, retested and the generals get their AI fine

…lack

tintinhamans force-pushed the arctic/fix/legacy-map-skirmish-sides branch from a654a93 to b488b9c Compare October 12, 2026 01:20

greptile-apps Bot reviewed Oct 12, 2026

View reviewed changes

Comment thread Core/GameEngine/Source/GameLogic/Map/SidesList.cpp Outdated

tintinhamans force-pushed the arctic/fix/legacy-map-skirmish-sides branch from b488b9c to dc8dcda Compare October 12, 2026 01:31

greptile-apps Bot reviewed Oct 12, 2026

View reviewed changes

Comment thread Core/GameEngine/Source/GameLogic/Map/SidesList.cpp Outdated

coderabbitai Bot reviewed Oct 12, 2026

View reviewed changes

Comment thread Core/GameEngine/Source/GameLogic/Map/PolygonTrigger.cpp Outdated

tintinhamans force-pushed the arctic/fix/legacy-map-skirmish-sides branch from dc8dcda to f8e2cef Compare October 12, 2026 01:51

bobtista reviewed Oct 14, 2026

View reviewed changes

tintinhamans added Minor Severity: Minor < Major < Critical < Blocker ZH Relates to Zero Hour Fix Is fixing something, but is not user facing NoRetail This fix or change is not applicable with Retail game compatibility labels Oct 14, 2026

bugfix(sideslist): Add the general skirmish sides that Generals maps …

9c795ea

…lack

tintinhamans force-pushed the arctic/fix/legacy-map-skirmish-sides branch from f8e2cef to 9c795ea Compare October 14, 2026 20:46

Navigation Menu

Repository navigation

bugfix(sideslist): Add the general skirmish sides that Generals maps lack - #3484

bugfix(sideslist): Add the general skirmish sides that Generals maps lack#3484
tintinhamans wants to merge 1 commit into
TheSuperHackers:mainfrom
tintinhamans:arctic/fix/legacy-map-skirmish-sides

tintinhamans commented Oct 12, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 12, 2026 •

edited

Loading

Reviews paused

Uh oh!

greptile-apps Bot commented Oct 12, 2026 •

edited

Loading

Summary

Diagram

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

bobtista Oct 14, 2026

Uh oh!

tintinhamans Oct 14, 2026

Uh oh!

tintinhamans Oct 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

tintinhamans commented Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

greptile-apps Bot commented Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Confidence Score: 5/5

Summary

Diagram

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bobtista Oct 14, 2026

Choose a reason for hiding this comment

Uh oh!

tintinhamans Oct 14, 2026

Choose a reason for hiding this comment

Uh oh!

tintinhamans Oct 14, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tintinhamans commented Oct 12, 2026 •

edited

Loading

coderabbitai Bot commented Oct 12, 2026 •

edited

Loading

greptile-apps Bot commented Oct 12, 2026 •

edited

Loading