Skip to content

refactor: Split preserve retail behavior flags#2691

Open
Stubbjax wants to merge 6 commits intoTheSuperHackers:mainfrom
Stubbjax:split-preserve-retail-behavior-flags
Open

refactor: Split preserve retail behavior flags#2691
Stubbjax wants to merge 6 commits intoTheSuperHackers:mainfrom
Stubbjax:split-preserve-retail-behavior-flags

Conversation

@Stubbjax
Copy link
Copy Markdown

@Stubbjax Stubbjax commented May 6, 2026

This change splits the PRESERVE_RETAIL_BEHAVIOR flag into a separate, dedicated flag for each fix. This allows publishers to more easily toggle controversial fixes without having to manually look through the code and figure out how particular fixes work or are implemented.

@Stubbjax Stubbjax self-assigned this May 6, 2026
@Stubbjax Stubbjax added Gen Relates to Generals ZH Relates to Zero Hour Refactor Edits the code with insignificant behavior changes, is never user facing labels May 6, 2026
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 6, 2026

Greptile Summary

This PR refactors the single PRESERVE_RETAIL_BEHAVIOR compile-time flag into 13 purpose-specific PRESERVE_* flags and one new ALLOW_MONEY_PER_MINUTE_FOR_PLAYER feature flag, making it easier for publishers to selectively toggle controversial fixes. The change is applied symmetrically to both the Generals/ and GeneralsMD/ codebases.

  • GameDefines.h gains 14 new #ifndef-guarded macros; 5 default to 0 (fix active by default, approved by Game Design Committee) and the rest default to 1 (retail behavior preserved). All previous PRESERVE_RETAIL_BEHAVIOR references have been replaced site-by-site with the appropriate specific flag.
  • Semantic alignment is correct for every replacement: the old single flag defaulting to 1 is faithfully replicated by each split flag's default, and flags whose fixes were already committee-approved appropriately default to 0.
  • ALLOW_MONEY_PER_MINUTE_FOR_PLAYER replaces the earlier !PRESERVE_RETAIL_BEHAVIOR guard, correctly keeping the feature disabled by default (0) and decoupling it from retail-behavior preservation semantics.

Confidence Score: 5/5

Safe to merge — the split is a pure compile-time refactoring with no runtime surprises.

Every site that previously referenced PRESERVE_RETAIL_BEHAVIOR has been replaced with its purpose-specific flag, and no residual uses remain across the Generals, GeneralsMD, or Core trees. Default values faithfully reproduce the previous single-flag behaviour, and the five flags whose fixes were committee-approved correctly default to 0. The only finding is a cosmetic ordering nit in the macro block.

No files require special attention. GameDefines.h has a minor alphabetical ordering inconsistency for PRESERVE_OCCUPANT_DETECTION_VIA_DRAG_SELECTION, but it does not affect correctness.

Important Files Changed

Filename Overview
Core/GameEngine/Include/Common/GameDefines.h Core change: replaces PRESERVE_RETAIL_BEHAVIOR with 13 specific PRESERVE_* flags and adds ALLOW_MONEY_PER_MINUTE_FOR_PLAYER; PRESERVE_OCCUPANT_DETECTION_VIA_DRAG_SELECTION is now present but placed out of alphabetical order relative to the other flags
Core/GameEngine/Source/GameClient/SelectionInfo.cpp Replaces PRESERVE_RETAIL_BEHAVIOR with PRESERVE_OCCUPANT_DETECTION_VIA_DRAG_SELECTION at both usage sites; macro is now defined in GameDefines.h
Generals/Code/GameEngine/Include/Common/TunnelTracker.h Swaps PRESERVE_RETAIL_BEHAVIOR for PRESERVE_TUNNEL_HEAL_STACKING in header-conditional method signature; correct replacement
Generals/Code/GameEngine/Source/Common/GlobalData.cpp Replaces !PRESERVE_RETAIL_BEHAVIOR guard with ALLOW_MONEY_PER_MINUTE_FOR_PLAYER (both default to disabled); semantic behavior preserved
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Body/UndeadBody.cpp Replaces PRESERVE_RETAIL_BEHAVIOR with PRESERVE_PREMATURE_BATTLE_BUS_DEATH for Battle Bus lethal damage condition; correct replacement
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/StealthUpdate.cpp Replaces PRESERVE_RETAIL_BEHAVIOR with PRESERVE_STRUCTURE_STEALTH_DURING_REPAIR in stealth-during-healing guard; no equivalent change needed in Generals version
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp Replaces PRESERVE_RETAIL_BEHAVIOR with PRESERVE_UNRELIABLE_FIRESTORMS at both sites for trimOldHistoricDamage and processHistoricDamage; correct replacement

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[PRESERVE_RETAIL_BEHAVIOR] -->|split into| B[PRESERVE_BUILDING_RESUMPTION_DELAY default: 0]
    A -->|split into| C[PRESERVE_CHINOOK_PASSENGER_DUMPING default: 1]
    A -->|split into| D[PRESERVE_HARDCODED_BLACK_LOTUS_CASH_HACK default: 1]
    A -->|split into| E[PRESERVE_MULTI_CRATE_PICKUP default: 0]
    A -->|split into| F[PRESERVE_NO_XP_FROM_FLAME_KILLS default: 0]
    A -->|split into| G[PRESERVE_NO_XP_FROM_OCL_KILLS default: 1]
    A -->|split into| H[PRESERVE_NO_XP_FROM_POISON_KILLS default: 1]
    A -->|split into| I[PRESERVE_OCCUPANT_DETECTION_VIA_DRAG_SELECTION default: 1]
    A -->|split into| J[PRESERVE_PERPETUAL_HORDE_BONUS default: 1]
    A -->|split into| K[PRESERVE_PREMATURE_BATTLE_BUS_DEATH default: 1]
    A -->|split into| L[PRESERVE_RADAR_WARNING_SUPPRESSION default: 1]
    A -->|split into| M[PRESERVE_STRUCTURE_STEALTH_DURING_REPAIR default: 0]
    A -->|split into| N[PRESERVE_TUNNEL_HEAL_STACKING default: 1]
    A -->|split into| O[PRESERVE_UNRELIABLE_FIRESTORMS default: 0]
    P[!PRESERVE_RETAIL_BEHAVIOR] -->|renamed| Q[ALLOW_MONEY_PER_MINUTE_FOR_PLAYER default: 0]
Loading
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
Core/GameEngine/Include/Common/GameDefines.h:74-80
All other flags in this block are defined in alphabetical order, but `PRESERVE_OCCUPANT_DETECTION_VIA_DRAG_SELECTION` (O) is placed after `PRESERVE_UNRELIABLE_FIRESTORMS` (U). It should sit between `PRESERVE_NO_XP_FROM_POISON_KILLS` and `PRESERVE_PERPETUAL_HORDE_BONUS` to maintain the established ordering.

```suggestion
#ifndef PRESERVE_OCCUPANT_DETECTION_VIA_DRAG_SELECTION
#define PRESERVE_OCCUPANT_DETECTION_VIA_DRAG_SELECTION (1)
#endif

#ifndef PRESERVE_UNRELIABLE_FIRESTORMS
#define PRESERVE_UNRELIABLE_FIRESTORMS (0) // The fix for this unfavorable behavior was approved by the Game Design Committee.
#endif
```

Reviews (3): Last reviewed commit: "docs: Add comments" | Re-trigger Greptile

Comment thread Core/GameEngine/Source/GameClient/SelectionInfo.cpp
@Mauller
Copy link
Copy Markdown

Mauller commented May 6, 2026

I think it would be better to split these defines out into their own file of GameplayFixDefines or something similar

But keep preserve retail behaviour as an encompassing switch.

#endif

#ifndef PRESERVE_UNRELIABLE_FIRESTORMS
#define PRESERVE_UNRELIABLE_FIRESTORMS (1)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe set those to 0 that where already approved by the Committee?

Comment thread Core/GameEngine/Include/Common/GameDefines.h Outdated
@Stubbjax
Copy link
Copy Markdown
Author

Stubbjax commented May 7, 2026

I think it would be better to split these defines out into their own file of GameplayFixDefines or something similar

But keep preserve retail behaviour as an encompassing switch.

How useful do you think a master flag would be? Does it outweigh the additional maintenance burden?

#endif

#ifndef PRESERVE_UNRELIABLE_FIRESTORMS
#define PRESERVE_UNRELIABLE_FIRESTORMS (0)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest also add comments that these are approved by Committee, to understand why they are default 0 while others are still 1.

Copy link
Copy Markdown
Author

@Stubbjax Stubbjax May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How should that look? A // TheSuperHackers @info Approved by the Design Committee. comment at the end of each (0)?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe

// Fix for this unfavourable behavior was approved by the Game Design Committee

TheSuperHackers @info is not necessary because the file is ours.

@L3-M
Copy link
Copy Markdown

L3-M commented May 7, 2026

I think after this change, we need to discuss what the workflow would be for controversial features or bug fixes that alter retail behaviour. Do we keep the default as what the author and reviewer think is correct? Or keep the original retail experience as default? Or wait for committee decision and input and implement that as the default?

The cons of the first and third options are that balance changes a lot, has many talks, takes time and many people will disagree.

Over time. This repo was for building the game, not publishing and shipping it. I think it is better to keep the retail behaviour as the default. And let the publishers decide what to enable or not, and whether to follow the committee. This will be helpful too for modders who prefer the retail experience for their mods.

@githubawn
Copy link
Copy Markdown

Much more fun to write it this way:

#ifndef _MSC_VER
    { "AllowMoney", INI::parseBool, nullptr, offsetof(GlobalData, m_allowMoney) },
#elif _MSC_VER >= 1930
    { "DoSomething", INI::parseBool, nullptr, offsetof(GlobalData, m_DoSomething) },
#endif

This _MSC_VER Macro is very powerful, it symbolizes a specific match, one frozen in time. None may touch this. :)

We have the long rumored Retail 1.0 someday. With this Macro style spread across the codebase we might even get to play with the classic Fork with he new tools as well. Like Tracy/SDL3 getting in nowadays.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Gen Relates to Generals Refactor Edits the code with insignificant behavior changes, is never user facing ZH Relates to Zero Hour

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants