feat(cratecollide): Add INI field to allow a crate to be picked up multiple times in one frame

[Stubbjax]

• Follow up for bugfix(cratecollide): Prevent crates from being collected multiple times in a single frame #2279

This change is a follow-up to #2279 and adds an AllowMultiPickup field to the CrateCollide module. When assigned a value of Yes, a crate can be collected by multiple colliding objects in a single frame, effectively replicating the original retail behaviour. The field defaults to a value of No when unassigned.

This feature was requested as some mods rely on the original behaviour.

Usage:

Behavior = MoneyCrateCollide ModuleTag_X
  AllowMultiPickup = Yes ; Allows collection by multiple objects on the same frame
End

[greptile-apps]

Greptile Summary

This PR adds an AllowMultiPickup configuration field to the CrateCollide module, allowing mod creators to control whether crates can be picked up by multiple objects in a single frame. The implementation:

• Adds the m_allowMultiPickup boolean field to CrateCollideModuleData in both Generals and GeneralsMD codebases
• Defaults to true when PRESERVE_RETAIL_BEHAVIOR is enabled (the current default), replicating original game behavior
• Adds INI parser support via the AllowMultiPickup configuration key
• Modifies the isValidToExecute() validation to check !md->m_allowMultiPickup alongside the destroyed state check

This is a follow-up to PR #2279, which prevented the multi-pickup bug by checking if a crate is destroyed. This PR allows mod creators to opt back into the original behavior when needed, while maintaining the bugfix as the default for new configurations.

Confidence Score: 5/5

• This PR is safe to merge with minimal risk
• The changes are minimal, well-scoped, and follow established patterns in the codebase. The implementation correctly adds a new configuration field with proper initialization, INI parsing, and logical integration. The default value preserves retail behavior, and the changes are consistently applied across both Generals and GeneralsMD codebases. The logic is straightforward and doesn't introduce any security vulnerabilities or breaking changes.
• No files require special attention

Important Files Changed

Filename	Overview
Generals/Code/GameEngine/Source/GameLogic/Object/Collide/CrateCollide/CrateCollide.cpp	Initialized m_allowMultiPickup based on PRESERVE_RETAIL_BEHAVIOR, added INI parser entry, updated logic to allow multi-pickup when flag is enabled
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Collide/CrateCollide/CrateCollide.cpp	Initialized m_allowMultiPickup based on PRESERVE_RETAIL_BEHAVIOR, added INI parser entry, updated logic (identical to Generals version)

Flowchart

flowchart TD
    A[Object collides with crate] --> B{isValidToExecute check}
    B --> C{Is crate destroyed?}
    C -->|No| D[Continue validation checks]
    C -->|Yes| E{Is AllowMultiPickup enabled?}
    E -->|Yes| D
    E -->|No| F[Return FALSE - prevent pickup]
    D --> G{Other validation checks pass?}
    G -->|Yes| H[Execute crate behavior]
    G -->|No| F
    H --> I[Destroy crate object]
    
    style E fill:#e1f5ff
    style I fill:#ffe1e1

Loading

_{Last reviewed commit: 247bb19}

[Skyaero42]

This looks fine to me.

[xezon]

m_allowMultiPickup = (PRESERVE_RETAIL_BEHAVIOR != 0);

The init value needs to be true when preserving retail behavior, so that legacy INI files work as usual, meaning Mods do not need to be adapted to work correctly.

[Stubbjax]

Okay, though I'm a little concerned about setting a precedent here. Is a non-retail compatible CRC build expected to be fully compatible with all mods? How do we determine or predict which bugs a particular mod might rely on? Do we need to retroactively apply this consideration across previous fixes?

[xezon]

Ideally, for feature-like things, if PRESERVE_RETAIL_BEHAVIOR is 1. We have not been that strict historically, but it is very welcome if we know that Mods break with our changes. For this particular change, hanfield has told us that it break a Mod and therefore it is wise to handle it gracefully.

We can retroactively apply it to other changes we made if we know that it is helping Mods run normally.

[Stubbjax]

In that case we'll be effectively forgoing the fix for active publishers currently preserving retail behaviour such as Generals Online in favour of supporting abandoned mods played on a non-RETAIL_COMPATIBLE_CRC client (which seems like a rather unlikely combination). Is this a reasonable compromise?

[xezon]

The problem is, if a Mod adds these new INI entries to make their Mod work normally with the new executable, then it will crash on boot with retail executable, because the INI parser will throw on unknown INI fields.

[Stubbjax]

Won't that be the case for any mod that makes use of a new INI field, irrespective of any preprocessor directives?

[xezon]

If PRESERVE_RETAIL_BEHAVIOR is 1 it allows Mod creator to build INI Mod without strict new Executable requirement. Once we require set new INI fields to restore Retail behavior, they cannot easily go back to make the INI Mod work with the retail executable. This is a disadvantage, for example for testing if something was already broken with the retail executable.