Repository navigation
bugfix(WeaponTemplate): Fix the override handling and resetting of modified weapon templates - #3478
Conversation
WalkthroughThe changes add the ChangesWeapon template override handling
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: 🟡 Moderate · up to Map and solo weapon overrides can use the wrong values or fail to resolve a projectile declared later. Fix override ordering and defer projectile resolution before merging. Pre-merge checks |
 |
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly identifies the main change: fixing WeaponTemplate override handling and reset behavior. |
| Description check | |
The description directly explains weapon-template overrides, reset behavior, memory leaks, the RETAIL_COMPATIBLE_SCRIPTING macro, and map.ini usage. |
- 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.
Comment @coderabbitai help to get the list of available commands.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟠 High
Loading a second INI_LOAD_CREATE_OVERRIDES definition for a weapon silently discards changes made by the first override. findWeaponTemplatePrivate returns the base template, and passing it to newOverride starts the next layer from that base; use the current final override as the source instead.
🤖 Copy this AI Prompt to have your agent fix this:
In file @GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp around line 1852:
Loading a second `INI_LOAD_CREATE_OVERRIDES` definition for a weapon silently discards changes made by the first override. `findWeaponTemplatePrivate` returns the base template, and passing it to `newOverride` starts the next layer from that base; use the current final override as the source instead.
| { | ||
| WeaponTemplate *wt = *it; | ||
| NameKeyType nameKey = wt->getNameKey(); | ||
| if (!wt->deleteOverrides()) |
There was a problem hiding this comment.
🟠 High Object/Weapon.cpp:1791
Map/solo-only weapons remain registered after WeaponStore::reset(), so later games can resolve stale map-specific definitions instead of loading a clean set. newWeaponTemplate entries from the weapon == nullptr branch of parseWeaponTemplateDefinition are not marked as overrides, so deleteOverrides() leaves them in the vector and hash map; remove these standalone entries during reset as well.
🤖 Copy this AI Prompt to have your agent fix this:
In file @GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp around line 1791:
Map/solo-only weapons remain registered after `WeaponStore::reset()`, so later games can resolve stale map-specific definitions instead of loading a clean set. `newWeaponTemplate` entries from the `weapon == nullptr` branch of `parseWeaponTemplateDefinition` are not marked as overrides, so `deleteOverrides()` leaves them in the vector and hash map; remove these standalone entries during reset as well.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| WeaponTemplate *wt = newInstance(WeaponTemplate); | ||
| (*wt) = (*weaponTemplate); | ||
| if (weaponTemplate->m_extraBonus) | ||
| { | ||
| wt->m_extraBonus = newInstance(WeaponBonusSet); | ||
| (*wt->m_extraBonus) = (*weaponTemplate->m_extraBonus); | ||
| } | ||
| weaponTemplate->setNextOverride(wt); |
There was a problem hiding this comment.
With RETAIL_COMPATIBLE_SCRIPTING=0, overriding the same weapon in both map.ini and solo.ini leaves the first override active. The parser passes the base template to newOverride each time. Copying that base also copies its existing m_nextOverride, producing base → second override → first override. getFinalOverride() then returns the first override, ignoring the later changes.
Copy and append from the current final override instead. Apply the same fix in GeneralsMD.
| if (ini->getLoadType() == INI_LOAD_CREATE_OVERRIDES) | ||
| { | ||
| weapon->postProcessLoad(); |
There was a problem hiding this comment.
With RETAIL_COMPATIBLE_SCRIPTING=0, this postProcessLoad() call removes inherited FireOCL and ProjectileDetonationOCL behavior unless the override repeats those fields. Startup already cleared the base template's OCL names while keeping its resolved pointers. newOverride copies those pointers and empty names, then postProcessLoad() replaces the pointers with nullptr.
Even a damage-only override stops the weapon from creating its configured objects. Preserve inherited pointers when resolving unchanged fields, and apply the same fix in GeneralsMD.
Knowledge Base Used: RTS engine core
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
a3548402-be56-40d8-9616-7e3c5598ba33
📒 Files selected for processing (5)
Core/GameEngine/Include/Common/GameDefines.hGenerals/Code/GameEngine/Include/GameLogic/Weapon.hGenerals/Code/GameEngine/Source/GameLogic/Object/Weapon.cppGeneralsMD/Code/GameEngine/Include/GameLogic/Weapon.hGeneralsMD/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
| WeaponTemplate *wt = newInstance(WeaponTemplate); | ||
| (*wt) = (*weaponTemplate); | ||
| if (weaponTemplate->m_extraBonus) | ||
| { | ||
| wt->m_extraBonus = newInstance(WeaponBonusSet); | ||
| (*wt->m_extraBonus) = (*weaponTemplate->m_extraBonus); | ||
| } | ||
| weaponTemplate->setNextOverride(wt); | ||
| wt->markAsOverride(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Repeated overrides build a broken chain. newOverride always copies the base template. That copy includes the base's m_nextOverride, and the new template is then linked directly to the base. When a weapon is overridden twice (for example, by solo.ini and then map.ini), the chain becomes base -> override2 -> override1. getFinalOverride() then returns override1. The newest override is ignored, and it also never received the first override's values.
Generals/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp#L1583-L1591: copy fromfriend_getFinalOverride(), setm_nextOverrideto null, copym_extraBonusfrom that final template, and callsetNextOverride(wt)on the final template.GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp#L1718-L1726: apply the same change.
📍 Affects 2 files
Generals/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp#L1583-L1591(this comment)GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp#L1718-L1726
| #if !RETAIL_COMPATIBLE_SCRIPTING | ||
| if (ini->getLoadType() == INI_LOAD_CREATE_OVERRIDES) | ||
| { | ||
| weapon->postProcessLoad(); | ||
| } | ||
| #endif | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1680,1748p' Generals/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp
sed -n '2180,2225p' Generals/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp
rg -n '#define DEBUG_ASSERTCRASH|DEBUG_ASSERTCRASH\(' Generals/Code/GameEngine/Include/Common Core/GameEngine/Include/Common | head -40Repository: TheSuperHackers/GeneralsGameCode
Length of output: 9936
🏁 Script executed:
set -e
printf '%s\n' '--- Generals references ---'
rg -n -F -- 'newOverride' Generals/Code/GameEngine/Include Generals/Code/GameEngine/Source
rg -n -F -- 'm_weaponTemplateVector' Generals/Code/GameEngine/Include Generals/Code/GameEngine/Source
rg -n -F -- 'void WeaponStore::reset' Generals/Code/GameEngine/Source
rg -n -F -- 'WeaponTemplate::postProcessLoad' Generals/Code/GameEngine/Source
printf '%s\n' '--- Generals definitions ---'
sed -n '350,455p' Generals/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp
sed -n '1600,1695p' Generals/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp
sed -n '1670,1760p' Generals/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp
sed -n '380,470p' Generals/Code/GameEngine/Include/GameLogic/Weapon.h
printf '%s\n' '--- GeneralsMD references ---'
rg -n -F -- 'newOverride' GeneralsMD/Code/GameEngine/Include GeneralsMD/Code/GameEngine/Source
rg -n -F -- 'm_weaponTemplateVector' GeneralsMD/Code/GameEngine/Include GeneralsMD/Code/GameEngine/Source
rg -n -F -- 'void WeaponStore::reset' GeneralsMD/Code/GameEngine/Source
rg -n -F -- 'WeaponTemplate::postProcessLoad' GeneralsMD/Code/GameEngine/Source
printf '%s\n' '--- GeneralsMD definitions ---'
sed -n '365,470p' GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp
sed -n '1720,1885p' GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp
sed -n '390,480p' GeneralsMD/Code/GameEngine/Include/GameLogic/Weapon.h
printf '%s\n' '--- DEBUG behavior ---'
sed -n '175,212p' Core/GameEngine/Include/Common/Debug.h
printf '%s\n' '--- all WeaponStore::postProcessLoad callers ---'
rg -n -F -- '->postProcessLoad()' Generals/Code/GameEngine GeneralsMD/Code/GameEngine | rg 'Weapon|weapon|TheWeaponStore|weaponStore' || trueRepository: TheSuperHackers/GeneralsGameCode
Length of output: 37721
🏁 Script executed:
set -e
printf '%s\n' '--- references ---'
rg -n -F -- 'newOverride' Generals/Code/GameEngine/Include Generals/Code/GameEngine/Source GeneralsMD/Code/GameEngine/Include GeneralsMD/Code/GameEngine/Source
rg -n -F -- 'm_weaponTemplateVector' Generals/Code/GameEngine/Include Generals/Code/GameEngine/Source GeneralsMD/Code/GameEngine/Include GeneralsMD/Code/GameEngine/Source
rg -n -F -- 'void WeaponStore::reset' Generals/Code/GameEngine/Source GeneralsMD/Code/GameEngine/Source
rg -n -F -- 'WeaponTemplate::postProcessLoad' Generals/Code/GameEngine/Source GeneralsMD/Code/GameEngine/Source
printf '%s\n' '--- Generals definitions ---'
sed -n '350,455p' Generals/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp
sed -n '1600,1695p' Generals/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp
sed -n '1670,1760p' Generals/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp
sed -n '380,470p' Generals/Code/GameEngine/Include/GameLogic/Weapon.h
printf '%s\n' '--- GeneralsMD definitions ---'
sed -n '365,470p' GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp
sed -n '1720,1885p' GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp
sed -n '390,480p' GeneralsMD/Code/GameEngine/Include/GameLogic/Weapon.h
printf '%s\n' '--- DEBUG behavior ---'
sed -n '175,212p' Core/GameEngine/Include/Common/Debug.h
printf '%s\n' '--- postProcessLoad callers ---'
rg -n -F -- 'postProcessLoad()' Generals/Code/GameEngine GeneralsMD/Code/GameEngine | rg 'Weapon|weapon|TheWeaponStore' || trueRepository: TheSuperHackers/GeneralsGameCode
Length of output: 38439
🏁 Script executed:
set -e
printf '%s\n' '--- exact Generals override and store blocks ---'
nl -ba Generals/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp | sed -n '1435,1585p'
nl -ba Generals/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp | sed -n '1600,1710p'
nl -ba Generals/Code/GameEngine/Include/GameLogic/Weapon.h | sed -n '430,490p;790,890p'
printf '%s\n' '--- exact GeneralsMD override and store blocks ---'
nl -ba GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp | sed -n '1570,1720p'
nl -ba GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp | sed -n '1740,1845p'
nl -ba GeneralsMD/Code/GameEngine/Include/GameLogic/Weapon.h | sed -n '440,500p;815,915p'
printf '%s\n' '--- reset and map/solo load callers ---'
rg -n -F -- 'TheWeaponStore->reset' Generals/Code GeneralsMD/Code || true
rg -n -F -- 'resetWeaponTemplates' Generals/Code GeneralsMD/Code || true
rg -n -F -- 'loadMapINI' Generals/Code/GameEngine/Source GeneralsMD/Code/GameEngine/Source
rg -n -F -- 'loadMapINI(' Generals/Code/GameEngine/Source GeneralsMD/Code/GameEngine/SourceRepository: TheSuperHackers/GeneralsGameCode
Length of output: 41069
🏁 Script executed:
set -e
printf '%s\n' '--- complete newOverride bodies ---'
nl -ba Generals/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp | sed -n '1568,1600p'
nl -ba GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp | sed -n '1703,1740p'
printf '%s\n' '--- override helper symbols ---'
rg -n -F -- 'getFinalOverride' Generals/Code/GameEngine/Include Generals/Code/GameEngine/Source GeneralsMD/Code/GameEngine/Include GeneralsMD/Code/GameEngine/Source
rg -n -F -- 'deleteOverrides' Generals/Code/GameEngine/Include Generals/Code/GameEngine/Source GeneralsMD/Code/GameEngine/Include GeneralsMD/Code/GameEngine/Source
rg -n -F -- 'setNextOverride' Generals/Code/GameEngine/Include Generals/Code/GameEngine/Source GeneralsMD/Code/GameEngine/Include GeneralsMD/Code/GameEngine/Source
rg -n -F -- 'friend_setNextTemplate' Generals/Code/GameEngine/Include Generals/Code/GameEngine/Source GeneralsMD/Code/GameEngine/Include GeneralsMD/Code/GameEngine/Source
printf '%s\n' '--- map load entrypoints and surrounding lifecycle ---'
nl -ba Generals/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp | sed -n '1140,1205p;2140,2240p'
nl -ba GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp | sed -n '1300,1370p;2445,2545p'
printf '%s\n' '--- subsystem reset dispatch ---'
rg -n -F -- 'reset()' Core/GameEngine/Source Generals/Code/GameEngine/Source/GameLogic GeneralsMD/Code/GameEngine/Source/GameLogic | rg 'Subsystem|reset' || true
rg -n -F -- 'SubsystemInterface' Core/GameEngine/Source Core/GameEngine/Include | head -80Repository: TheSuperHackers/GeneralsGameCode
Length of output: 41757
Defer post-processing for weapon overrides until map loading finishes.
When RETAIL_COMPATIBLE_SCRIPTING is disabled, a [Weapon] override is post-processed before later [Object] blocks are parsed. A forward-referenced projectile therefore caches nullptr and enters DEBUG_ASSERTCRASH. The cached null remains in non-crashing builds.
Calling the existing WeaponStore::postProcessLoad() after map.ini and solo.ini is not sufficient. newOverride() links the override through setNextOverride() but does not add it to m_weaponTemplateVector; the existing loop visits only vector entries. Add a deferred pass that processes each base template's final override, or retain pending override pointers. Do not reprocess only the base templates.
Suggested fix
--- a/Generals/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp
+++ b/Generals/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp
@@
-#if !RETAIL_COMPATIBLE_SCRIPTING
- if (ini->getLoadType() == INI_LOAD_CREATE_OVERRIDES)
- {
- weapon->postProcessLoad();
- }
-#endif
-
//-------------------------------------------------------------------------------------------------
void WeaponStore::postProcessLoad()
@@
}
+void WeaponStore::postProcessOverrideLoad()
+{
+#if !RETAIL_COMPATIBLE_SCRIPTING
+ for (size_t i = 0; i < m_weaponTemplateVector.size(); ++i)
+ {
+ WeaponTemplate* wt = m_weaponTemplateVector[i];
+ WeaponTemplate* finalOverride =
+ static_cast<WeaponTemplate*>(wt->friend_getFinalOverride());
+ if (finalOverride != wt)
+ finalOverride->postProcessLoad();
+ }
+#endif
+}Apply the same change in GeneralsMD, and call postProcessOverrideLoad() after both map.ini and solo.ini have been loaded.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #if !RETAIL_COMPATIBLE_SCRIPTING | |
| if (ini->getLoadType() == INI_LOAD_CREATE_OVERRIDES) | |
| { | |
| weapon->postProcessLoad(); | |
| } | |
| #endif |
Mauller
changed the title
1 participant
Weapon templates now get overridden and get reset properly fixing memory leaks and makes it easier for map makers to adjust those weapons without having to do bypassing and stuff.
All code is wrapped in the new RETAIL_COMPATIBLE_SCRIPTING macro.
This is something Mauller was working on.
Used this map.ini change in both retail and this new build.
Before
https://github.com/user-attachments/assets/68215433-d9c3-49fe-85e3-c527e12f17e2
After
https://github.com/user-attachments/assets/b661a13a-42de-4246-8549-0fa27c8a0823