fix(particlesys): Decouple particle systems from logic crc#2742
Conversation
|
| Filename | Overview |
|---|---|
| Core/GameEngine/Include/Common/RandomValue.h | Introduces RandomValueClass hierarchy (base + Logic/Client impls) and helper macros; clean design with correct const-ref temporary binding for default parameters. |
| Core/GameEngine/Source/Common/RandomValue.cpp | Implements the four virtual dispatch methods; each simply delegates to the appropriate global random function. Straightforward and correct. |
| GeneralsMD/Code/GameEngine/Include/Common/Geometry.h | Adds RandomValue.h include and a const-ref default-parameter overload to makeRandomOffsetWithinFootprint; backward-compatible change. |
| GeneralsMD/Code/GameEngine/Source/Common/System/Geometry.cpp | Replaces all GameLogicRandomValueReal calls with the polymorphic RandomValueReal macro; logic is unchanged for existing callers using the default LogicRandomValueClass. |
| GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Damage/TransitionDamageFX.cpp | Switches particle position to ClientRandomValueClass and adds a RETAIL_COMPATIBLE_CRC dummy call; however the dummy call is placed before the if(pSystem) guard instead of inside it, mismatching the original execution order. |
| GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/EMPUpdate.cpp | Three CRC-preservation dummy calls placed unconditionally before createParticleSystem; originals were inside if(sys), so CRC can diverge when sys is null. |
| GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/SpecialAbilityUpdate.cpp | Same RETAIL_COMPATIBLE_CRC scope placement issue as EMPUpdate.cpp and TransitionDamageFX.cpp; dummy call runs before the if(sys) null check. |
Class Diagram
%%{init: {'theme': 'neutral'}}%%
classDiagram
class RandomValueClass {
<<abstract>>
+GetRandomValueInt(lo, hi, file, line) Int
+GetRandomValueReal(lo, hi, file, line) Real
}
class LogicRandomValueClass {
+GetRandomValueInt(lo, hi, file, line) Int
+GetRandomValueReal(lo, hi, file, line) Real
}
class ClientRandomValueClass {
+GetRandomValueInt(lo, hi, file, line) Int
+GetRandomValueReal(lo, hi, file, line) Real
}
RandomValueClass <|-- LogicRandomValueClass
RandomValueClass <|-- ClientRandomValueClass
class GeometryInfo {
+makeRandomOffsetWithinFootprint(pt, random) void
}
GeometryInfo ..> RandomValueClass : uses
note for LogicRandomValueClass "delegates to GetGameLogicRandomValue()"
note for ClientRandomValueClass "delegates to GetGameClientRandomValue()"
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
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/EMPUpdate.cpp:307-315
**CRC dummy calls placed outside the `if (sys)` null-check**
In the original code all three logic-random calls (`makeRandomOffsetWithinFootprint`, `GameLogicRandomValue(3, victimHeight)`, `GameLogicRandomValue(1, 100)`) were inside `if (sys)`. The new `RETAIL_COMPATIBLE_CRC` block runs them unconditionally, before `createParticleSystem` is even called. If `createParticleSystem` returns null the original code would not have advanced the logic RNG at all, but the CRC block advances it anyway, producing a divergent RNG state and breaking the replay CRC that the block is supposed to preserve.
The same structural mistake is present in `TransitionDamageFX.cpp` (the `getLocalEffectPos` dummy call before `if (pSystem)`) and `SpecialAbilityUpdate.cpp` (the `makeRandomOffsetWithinFootprint` dummy call before `if (sys)`). All three blocks should be moved inside their respective `if (sys/pSystem)` guards to exactly mirror the original code path.
Reviews (2): Last reviewed commit: "Fix macro arguments" | Re-trigger Greptile
| #if RETAIL_COMPATIBLE_CRC | ||
| // TheSuperHackers @fix The particle system is now decoupled from the logic crc | ||
| // and the legacy logic random values are preserved here. | ||
| { | ||
| Coord3D offs = {0,0,0}; | ||
| curVictim->getGeometryInfo().makeRandomOffsetWithinFootprint( offs, LogicRandomValueClass() ); | ||
| GameLogicRandomValue(3, victimHeight); | ||
| GameLogicRandomValue(1, 100); | ||
| } |
There was a problem hiding this comment.
CRC dummy calls placed outside the
if (sys) null-check
In the original code all three logic-random calls (makeRandomOffsetWithinFootprint, GameLogicRandomValue(3, victimHeight), GameLogicRandomValue(1, 100)) were inside if (sys). The new RETAIL_COMPATIBLE_CRC block runs them unconditionally, before createParticleSystem is even called. If createParticleSystem returns null the original code would not have advanced the logic RNG at all, but the CRC block advances it anyway, producing a divergent RNG state and breaking the replay CRC that the block is supposed to preserve.
The same structural mistake is present in TransitionDamageFX.cpp (the getLocalEffectPos dummy call before if (pSystem)) and SpecialAbilityUpdate.cpp (the makeRandomOffsetWithinFootprint dummy call before if (sys)). All three blocks should be moved inside their respective if (sys/pSystem) guards to exactly mirror the original code path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/EMPUpdate.cpp
Line: 307-315
Comment:
**CRC dummy calls placed outside the `if (sys)` null-check**
In the original code all three logic-random calls (`makeRandomOffsetWithinFootprint`, `GameLogicRandomValue(3, victimHeight)`, `GameLogicRandomValue(1, 100)`) were inside `if (sys)`. The new `RETAIL_COMPATIBLE_CRC` block runs them unconditionally, before `createParticleSystem` is even called. If `createParticleSystem` returns null the original code would not have advanced the logic RNG at all, but the CRC block advances it anyway, producing a divergent RNG state and breaking the replay CRC that the block is supposed to preserve.
The same structural mistake is present in `TransitionDamageFX.cpp` (the `getLocalEffectPos` dummy call before `if (pSystem)`) and `SpecialAbilityUpdate.cpp` (the `makeRandomOffsetWithinFootprint` dummy call before `if (sys)`). All three blocks should be moved inside their respective `if (sys/pSystem)` guards to exactly mirror the original code path.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
This is fine for as long as it is inside the if (tmp) branch.
Merge after fix(particlesys): Simplify ParticleSystemManagerDummy setup #2740This change decouples particle systems from logic crc and is an alternative to #2717.
The implementation adds a tiny runtime cost for virtual table lookups, but it provides an easy to use runtime switch for logic and client random values. Using it requires no refactors, template functions or static functions.
TODO