refactor(particlesys): Add or optimize null-checks to createParticleSystem calls#2724
Conversation
|
| Filename | Overview |
|---|---|
| Core/GameEngine/Source/GameClient/Drawable/Update/BeaconClientUpdate.cpp | Null-check moved from template pointer to createParticleSystem return value; the else/failsafe branch now triggers on any null return instead of only a missing template. |
| Core/GameEngine/Source/GameClient/System/ParticleSys.cpp | Removed redundant template null-guards before createParticleSystem; safe because createParticleSystem already returns nullptr for null input. |
| Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DTankTruckDraw.cpp | Replaced if-scoped template declaration pattern with separate declaration + null-check on createParticleSystem result; no behavioral change. |
| Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DTruckDraw.cpp | Same refactor as W3DTankTruckDraw; redundant template null-guard removed, null-check on return value preserved. |
| GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Behavior/AutoHealBehavior.cpp | Template null-guard removed; createParticleSystem result checked instead; logic and behavior unchanged. |
| GeneralsMD/Code/GameEngine/Source/GameLogic/Object/ObjectCreationList.cpp | Straightforward null-check migration from template pointer to createParticleSystem return value; safe. |
| GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/AIUpdate/ChinookAIUpdate.cpp | Double if-guard collapsed to one; introduces inconsistent indentation in the replacement block. |
| GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/EMPUpdate.cpp | Redundant template check removed; createParticleSystem return value is still null-checked; safe. |
| GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/HelicopterSlowDeathUpdate.cpp | Outer template null-guard removed and nesting reduced; logic is equivalent, code is cleaner. |
| GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/LaserUpdate.cpp | Two separate particle system initialisation blocks simplified; template guard removed, return-value check retained. |
| GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/ParticleUplinkCannonUpdate.cpp | Two connector/laser-base flare creation sites simplified; template guards replaced with return-value checks. |
| GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/SlavedUpdate.cpp | Welding particle system creation flattened by one nesting level; behavior unchanged. |
| GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/StealthDetectorUpdate.cpp | Three particle system creation sites simplified; IR-grid, ping, and beacon template guards replaced with return-value checks. |
| GeneralsMD/Code/GameEngine/Source/GameLogic/ScriptEngine/ScriptEngine.cpp | Parent-template null-guard removed before createParticleSystem; return-value check is preserved; safe. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Find particle template\nfindTemplate] --> B{template != nullptr?}
B -- OLD: explicit check --> C[createParticleSystem template]
B -- OLD: null --> F[Skip / failsafe]
C --> D{system != nullptr?}
D -- yes --> E[Use system]
D -- no --> G[Silently do nothing]
A2[Find particle template\nfindTemplate] --> C2[NEW: createParticleSystem null-safe]
C2 --> D2{system != nullptr?}
D2 -- yes --> E2[Use system]
D2 -- no --> F2[Skip / failsafe]
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
Core/GameEngine/Source/GameClient/Drawable/Update/BeaconClientUpdate.cpp:100-116
**Failsafe `else` now triggers on a broader condition**
Originally the `else` (fallback to `BeaconSmokeFFFFFF`) ran only when `particleTemplate == nullptr`, i.e. when the color-keyed template was not found. After this change it runs whenever `createParticleSystem` returns `nullptr` — which includes any future code path (e.g. particle-cap enforcement) where the manager declines to create a system for a *valid* template. In that scenario the failsafe would silently emit a tinted-white beacon instead of simply producing nothing, which is the unintended behavior the comment describes as "someone has monkeyed with the particle system names".
### Issue 2 of 2
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/AIUpdate/ChinookAIUpdate.cpp:1202-1206
**Inconsistent indentation introduced**
The `if( system )` block uses a different indent level (two hard tabs) than the `system` declaration immediately above it (one tab + six spaces), mismatching the surrounding code style and making the block's nesting visually ambiguous.
```suggestion
ParticleSystem *system = TheParticleSystemManager->createParticleSystem( tmp );
if( system )
{
system->setPosition( &pos );
}
```
Reviews (1): Last reviewed commit: "fix(particlesys): Add or optimize null-c..." | Re-trigger Greptile
| system = TheParticleSystemManager->createParticleSystem( particleTemplate ); | ||
| if (system) | ||
| { | ||
| system = TheParticleSystemManager->createParticleSystem( particleTemplate ); | ||
| if (system) | ||
| system->attachToDrawable( draw ); | ||
| system->attachToDrawable( draw ); | ||
| } | ||
| else// This is a failsafe... if someone has monkeyed with the particle system names, or the MP house colors | ||
| {// THis this will whip up a new particle system to match the house color provided | ||
| templateName.format("BeaconSmokeFFFFFF"); | ||
| const ParticleSystemTemplate *failsafeTemplate = TheParticleSystemManager->findTemplate( templateName ); | ||
| DEBUG_ASSERTCRASH(failsafeTemplate, ("Doh, this is bad \n I Could not even find the white particle system to make a failsafe system out of.")); | ||
| if (failsafeTemplate) | ||
| system = TheParticleSystemManager->createParticleSystem( failsafeTemplate ); | ||
| if (system) | ||
| { | ||
| system = TheParticleSystemManager->createParticleSystem( failsafeTemplate ); | ||
| if (system) | ||
| { | ||
| system->attachToDrawable( draw ); | ||
| system->tintAllColors( obj->getIndicatorColor() ); | ||
| } | ||
| system->attachToDrawable( draw ); | ||
| system->tintAllColors( obj->getIndicatorColor() ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Failsafe
else now triggers on a broader condition
Originally the else (fallback to BeaconSmokeFFFFFF) ran only when particleTemplate == nullptr, i.e. when the color-keyed template was not found. After this change it runs whenever createParticleSystem returns nullptr — which includes any future code path (e.g. particle-cap enforcement) where the manager declines to create a system for a valid template. In that scenario the failsafe would silently emit a tinted-white beacon instead of simply producing nothing, which is the unintended behavior the comment describes as "someone has monkeyed with the particle system names".
Prompt To Fix With AI
This is a comment left during a code review.
Path: Core/GameEngine/Source/GameClient/Drawable/Update/BeaconClientUpdate.cpp
Line: 100-116
Comment:
**Failsafe `else` now triggers on a broader condition**
Originally the `else` (fallback to `BeaconSmokeFFFFFF`) ran only when `particleTemplate == nullptr`, i.e. when the color-keyed template was not found. After this change it runs whenever `createParticleSystem` returns `nullptr` — which includes any future code path (e.g. particle-cap enforcement) where the manager declines to create a system for a *valid* template. In that scenario the failsafe would silently emit a tinted-white beacon instead of simply producing nothing, which is the unintended behavior the comment describes as "someone has monkeyed with the particle system names".
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Should investigate if we can remove this failsafe
createParticleSystemis null checkedTodo