bugfix(gamelogic): Modules now cease updating when disabled by non-whitelisted disabled types

[Stubbjax]

Closes TheSuperHackers/GeneralsGamePatch#147

This change fixes an issue where modules continue updating when disabled by a type that is not whitelisted.

For example, the stealth detector module has DISABLED_HELD whitelisted. This allows stealth detection to still run if an object is held, which in most cases is the result of containment (though CanDetectWhileContained typically blocks this). Any other disabling type is expected to shut the stealth detector down, such as DISABLED_EMP or DISABLED_SUBDUED. The problem is that the module only looks for whether the DISABLED_HELD bit is set and, if so, any other disabled type such as DISABLED_EMP is not considered.

This can also be seen elsewhere, such as with powered USA base defences. BaseRegenerateUpdate whitelists DISABLED_UNDERPOWERED. This means a Patriot Missile Battery will still repair itself when out of power. Conversely, if a Patriot Missile Battery is disabled by a Microwave Tank (DISABLED_SUBDUED), it will not repair itself. But if an underpowered Patriot Missile Battery is out of power and disabled by a Microwave Tank, then it will repair itself.

Before

The EMP Patriot will not repair while subdued, but will while the power is also out

REPAIR.mp4

After

A disabled (including unmanned) Overlord's Gattling Cannon no longer detects stealth units

DISABLE_DETECTION.mp4

[greptile-apps]

Greptile Summary

Fixes a bug where a module would continue updating if any whitelisted disabled type was active, even when a non-whitelisted type (e.g., DISABLED_EMP) was simultaneously set. The old anyIntersectionWith check is replaced with testForAll(dis) — ensuring all active disabled flags are a subset of the module's whitelist — and the original behavior is preserved under #if RETAIL_COMPATIBLE_CRC for CRC-compatibility with the retail game. The fix is applied symmetrically to both normal and sleepy update loops in both the Generals and Zero Hour codebases.

Confidence Score: 5/5

Safe to merge — the fix is logically correct, handles all edge cases, and the retail CRC path is preserved unchanged.

The testForAll(dis) semantics are verified against BitFlags.h: it returns (W & D) == D, which correctly yields true when dis is empty (object not disabled) and false when any non-whitelisted bit is set. The change is symmetric across both game variants and both update loops. No P1 or P0 issues found.

No files require special attention.

Important Files Changed

Filename	Overview
Generals/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp	Wraps the module update condition in a RETAIL_COMPATIBLE_CRC guard, introducing a new exclusive disabled-types check via testForAll in non-retail builds; logic is correct and handles the zero-disabled case via vacuous truth.
GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp	Identical fix applied to the Zero Hour variant; both #ifdef ALLOW_NONSLEEPY_UPDATES and sleepy-update paths are correctly patched.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[GameLogic::update - iterate modules] --> B{RETAIL_COMPATIBLE_CRC?}
    B -- Yes --> C["Old check: !dis.any() || dis.anyIntersectionWith(whitelist)"]
    B -- No --> D["New check: whitelist.testForAll(dis)
    = all active disabled bits ⊆ whitelist"]
    C --> E{Condition true?}
    D --> E
    E -- Yes --> F[Run module update]
    E -- No --> G[Skip update / use default sleep]
    
    subgraph testForAll semantics
        H["dis = 0 (not disabled)"] --> I["(W & 0)==0 → true ✓"]
        J["dis = HELD, W = {HELD}"] --> K["(HELD & HELD)==HELD → true ✓"]
        L["dis = EMP, W = {HELD}"] --> M["(HELD & EMP)==EMP → false ✓"]
        N["dis = HELD|EMP, W = {HELD}"] --> O["HELD == HELD|EMP → false ✓ (bug fixed)"]
    end

Loading

_{Reviews (3): Last reviewed commit: "refactor: Remove redundant checks" | Re-trigger Greptile}

[Skyaero42]

Looks good to me.

[Caball009]

Before merging this I'd like to test if this code overlaps / conflicts with possible fixes for the gattling cannon barrels rotating despite insufficient energy.

No conflict issue that I can see.

[xezon]

Do we need to label this is Controversial?

[xezon]

Is this certainly correct?

I would have expected:

const Bool isDisabled = dis.any() && dis.testForAny(u->getDisabledTypesToProcess());
if (!isDisabled)
...

[Stubbjax]

The getDisabledTypesToProcess function returns the disabled statuses that do not block the module's update.

For example, BaseRegenerateUpdate::getDisabledTypesToProcess returns DISABLED_UNDERPOWERED. This means the module is allowed to continue performing its repair update even if the player has low power.

Similarly, PoisonedBehaviour::getDisabledTypesToProcess returns DISABLEDMASK_ALL because nothing should disable the poisoned update. This is highlighted by the accompanying comment "we should still poison disabled things".

The function serves as a whitelist of disabled types that do not affect the update of the respective module. Perhaps a better name would be getAllowedDisabledTypes or getIgnoredDisabledTypes.

[xezon]

Ok got it. But why is this condition still correct then?

So what we need to test is: (dis & ~allowedDisabledTypes) == 0

Is u->getDisabledTypesToProcess().testForAll(dis) equivalent to that? It is so complicated to read. Can we write that easier?

[xezon]

So after cleaning up BitFlags function and getting a better understanding of what each of these functions do, I think

if (!dis.any() || u->getDisabledTypesToProcess().testForAll(dis))
{
  // do the update ...
}

should be

const DisabledMaskType disallowedDisabledTypes = u->getDisabledTypesToProcess().flip();
if (dis.testForNone(disallowedDisabledTypes))
{
  // do the update ...
}

Can you double check?

[Stubbjax]

Isn't that the same thing but in reverse? What u->getDisabledTypesToProcess().testForAll(dis)) does is essentially check that all bits set in dis (the current status) are also set in the allowed mask. If dis has any bits that are not in the allowed mask, then testForAll returns false and the update is skipped.

[xezon]

Right. But then you can omit !dis.any() as well. It is implicitly true when dis is empty. The first version is somehow much more complicated to understand for me in this context, because it uses the test-against flags as our primary, which requires mental gymnastics.

[xezon]

Condition can probably be a bit simplified.

[Stubbjax]

Condition can probably be a bit simplified.

What do you suggest?

[xezon]

Can you try remove !dis.any() ? Does it work?

[Stubbjax]

Can you try remove !dis.any() ? Does it work?

It does work. The check is indeed redundant, but it does make the intent explicit and possibly a bit clearer. Should I remove it?

[xezon]

When not needed, remove.