fix(particlesys): Decouple particle systems from logic crc#2742
Conversation
|
| Filename | Overview |
|---|---|
| Core/GameEngine/Include/Common/RandomValue.h | Introduces RandomValueClass abstract base, LogicRandomValueClass, ClientRandomValueClass, and helper macros; missing virtual destructor on the base class. |
| Core/GameEngine/Source/Common/RandomValue.cpp | Thin virtual-method implementations delegating to existing free functions; straightforward and correct. |
| GeneralsMD/Code/GameEngine/Include/Common/Geometry.h | Same signature change as Generals; mirrors expected parallel structure between the two targets. |
| GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Damage/TransitionDamageFX.cpp | Same changes as Generals counterpart; dummy CRC call correctly placed inside if(pSystemT) guard. |
| GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/EMPUpdate.cpp | Mirrors Generals EMPUpdate changes; RETAIL_COMPATIBLE_CRC block scoped correctly. |
| GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/SpecialAbilityUpdate.cpp | Mirrors Generals SpecialAbilityUpdate changes. |
Class Diagram
%%{init: {'theme': 'neutral'}}%%
classDiagram
class RandomValueClass {
<<abstract>>
+GetRandomValueInt(lo, hi, file, line) Int
+GetRandomValueReal(lo, hi, file, line) Real
}
class LogicRandomValueClass {
<<final>>
+GetRandomValueInt(lo, hi, file, line) Int
+GetRandomValueReal(lo, hi, file, line) Real
}
class ClientRandomValueClass {
<<final>>
+GetRandomValueInt(lo, hi, file, line) Int
+GetRandomValueReal(lo, hi, file, line) Real
}
RandomValueClass <|-- LogicRandomValueClass : delegates to GameLogicRandom
RandomValueClass <|-- ClientRandomValueClass : delegates to GameClientRandom
class GeometryInfo {
+makeRandomOffsetWithinFootprint(pt, random)
}
class TransitionDamageFX {
+onBodyDamageStateChange(...)
}
class EMPUpdate {
+doDisableAttack()
}
class SpecialAbilityUpdate {
+triggerAbilityEffect()
}
GeometryInfo ..> RandomValueClass : uses
TransitionDamageFX ..> LogicRandomValueClass : CRC dummy
TransitionDamageFX ..> ClientRandomValueClass : actual particle pos
EMPUpdate ..> LogicRandomValueClass : CRC dummy
EMPUpdate ..> ClientRandomValueClass : actual particle pos/delay
SpecialAbilityUpdate ..> LogicRandomValueClass : CRC dummy
SpecialAbilityUpdate ..> ClientRandomValueClass : actual particle pos
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/RandomValue.h:38-42
`RandomValueClass` has virtual methods but no virtual destructor. While the two concrete types are always used as stack temporaries here and pose no immediate risk, the absence of a virtual destructor is undefined behaviour if any caller ever destroys a derived instance through a base-class pointer. A protected non-virtual destructor is the idiomatic way to express "not for polymorphic deletion" without the overhead of a virtual destructor.
```suggestion
struct RandomValueClass
{
virtual Int GetRandomValueInt( Int lo, Int hi, const char *file, Int line ) const = 0;
virtual Real GetRandomValueReal( Real lo, Real hi, const char *file, Int line ) const = 0;
protected:
~RandomValueClass() = default;
};
```
Reviews (3): Last reviewed commit: "Replicate in Generals" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| #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.
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.
There was a problem hiding this comment.
Do we need a comment for that? Otherwise it might be tempting to combine both checks in a similar effort as #2724 I don't have a strong opinion on it either way FWIW.
There was a problem hiding this comment.
I dont think a comment is needed. Moving logic out of conditions will always the carry risk of logic mismatch and this one is not special.
|
Replicated in Generals with conflict in SpecialAbilityUpdate because of whitespace differences. |
2 participants
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