Skip to content

fix(particlesys): Add or simplify null-checks to createParticleSystem calls #2724

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
stephanmeesters wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(particlesys): Add or simplify null-checks to createParticleSystem calls #2724

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
845a0a3
fix(particlesys): Add or optimize null-checks to createParticleSystem…
stephanmeesters May 17, 2026
6dd0b8f
Process review comments
stephanmeesters May 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 7 additions & 11 deletions Core/GameEngine/Source/GameClient/Drawable/Update/BeaconClientUpdate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,25 +97,21 @@ static ParticleSystem* createParticleSystem( Drawable *draw )

DEBUG_ASSERTCRASH(particleTemplate, ("Could not find particle system %s", templateName.str()));

if (particleTemplate)
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() );
}
}
Comment on lines +100 to 116
Copy link
Copy Markdown

@greptile-apps greptile-apps Bot May 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

Copy link
Copy Markdown
Author

@stephanmeesters stephanmeesters May 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should investigate if we can remove this failsafe

Copy link
Copy Markdown

@xezon xezon May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the failsafe path can be the sole default path. What is the benefit of the preset house color beacons? Do they look different?

Copy link
Copy Markdown

@greptile-apps greptile-apps Bot May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two paths differ as follows:

  • Primary path: loads a pre-baked BeaconSmoke{RRGGBB} template (e.g. BeaconSmokeFF0000). The color is authored directly into the particle system INI definition.
  • Failsafe path: loads the white BeaconSmokeFFFFFF template and then calls tintAllColors(obj->getIndicatorColor()), which multiplies all particle colors by the house color at runtime.

Visually, if the only difference between the per-color templates and the white one is the baked color value, the two paths would produce identical results — tinting white by 0xRRGGBB gives 0xRRGGBB. So yes, the failsafe could be the sole path with no visual difference, and you'd be able to delete all the BeaconSmoke{color} INI entries except the white one.

The only scenario where they'd differ is if some per-color templates have non-white base colors or other differing properties (emission rate, size, alpha curves, etc.) — in which case those distinctions would be lost. Worth checking the INI data to confirm they're all identical except for color before removing the primary path.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.

Copy link
Copy Markdown

@xezon xezon May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest revert the change to this one.

}
Expand Down
8 changes: 3 additions & 5 deletions Core/GameEngine/Source/GameClient/System/ParticleSys.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2061,9 +2061,9 @@ Bool ParticleSystem::update( Int localPlayerIndex )
if (m_attachedSystemName.isEmpty() == false)
{
const ParticleSystemTemplate *tmp = TheParticleSystemManager->findTemplate( m_attachedSystemName );
if (tmp)
ParticleSystem *sys = TheParticleSystemManager->createParticleSystem( tmp, TRUE );
if (sys)
{
ParticleSystem *sys = TheParticleSystemManager->createParticleSystem( tmp, TRUE );
sys->setControlParticle( p );
p->controlParticleSystem( sys );
}
Expand Down Expand Up @@ -2867,9 +2867,7 @@ ParticleSystem *ParticleSystemTemplate::createSlaveSystem( Bool createSlaves ) c
if (m_slaveTemplate == nullptr && m_slaveSystemName.isEmpty() == false)
m_slaveTemplate = TheParticleSystemManager->findTemplate( m_slaveSystemName );

ParticleSystem *slave = nullptr;
if (m_slaveTemplate)
slave = TheParticleSystemManager->createParticleSystem( m_slaveTemplate, createSlaves );
ParticleSystem *slave = TheParticleSystemManager->createParticleSystem( m_slaveTemplate, createSlaves );

return slave;
}
Expand Down
5 changes: 3 additions & 2 deletions Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DTankTruckDraw.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,9 +247,10 @@ void W3DTankTruckDraw::createWheelEmitters()
{
if (m_truckEffectIDs[i] == INVALID_PARTICLE_SYSTEM_ID)
{
if (const ParticleSystemTemplate *sysTemplate = TheParticleSystemManager->findTemplate(*effectNames[i]))
const ParticleSystemTemplate *sysTemplate = TheParticleSystemManager->findTemplate(*effectNames[i]);
ParticleSystem *particleSys = TheParticleSystemManager->createParticleSystem( sysTemplate );
if (particleSys)
{
ParticleSystem *particleSys = TheParticleSystemManager->createParticleSystem( sysTemplate );
particleSys->attachToObject(getDrawable()->getObject());
// important: mark it as do-not-save, since we'll just re-create it when we reload.
particleSys->setSaveable(FALSE);
Expand Down
5 changes: 3 additions & 2 deletions Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DTruckDraw.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,9 +170,10 @@ void W3DTruckDraw::createWheelEmitters()
{
if (m_truckEffectIDs[i] == INVALID_PARTICLE_SYSTEM_ID)
{
if (const ParticleSystemTemplate *sysTemplate = TheParticleSystemManager->findTemplate(*effectNames[i]))
const ParticleSystemTemplate *sysTemplate = TheParticleSystemManager->findTemplate(*effectNames[i]);
ParticleSystem *particleSys = TheParticleSystemManager->createParticleSystem( sysTemplate );
if (particleSys)
{
ParticleSystem *particleSys = TheParticleSystemManager->createParticleSystem( sysTemplate );
particleSys->attachToObject(getDrawable()->getObject());
// important: mark it as do-not-save, since we'll just re-create it when we reload.
particleSys->setSaveable(FALSE);
Expand Down
9 changes: 3 additions & 6 deletions GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Behavior/AutoHealBehavior.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,13 +294,10 @@ void AutoHealBehavior::pulseHealObject( Object *obj )
obj->attemptHealingFromSoleBenefactor( data->m_healingAmount, getObject(), data->m_healingDelay );


if( data->m_unitHealPulseParticleSystemTmpl )
ParticleSystem *system = TheParticleSystemManager->createParticleSystem( data->m_unitHealPulseParticleSystemTmpl );
if( system )
{
ParticleSystem *system = TheParticleSystemManager->createParticleSystem( data->m_unitHealPulseParticleSystemTmpl );
if( system )
{
system->setPosition( obj->getPosition() );
}
system->setPosition( obj->getPosition() );
}

m_soonestHealFrame = TheGameLogic->getFrame() + data->m_healingDelay;// In case onDamage tries to wake us up early
Expand Down
4 changes: 2 additions & 2 deletions GeneralsMD/Code/GameEngine/Source/GameLogic/Object/ObjectCreationList.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -974,9 +974,9 @@ class GenericObjectCreationNugget : public ObjectCreationNugget
if (!m_particleSysName.isEmpty())
{
const ParticleSystemTemplate *tmp = TheParticleSystemManager->findTemplate(m_particleSysName);
if (tmp)
ParticleSystem *sys = TheParticleSystemManager->createParticleSystem(tmp);
if (sys)
{
ParticleSystem *sys = TheParticleSystemManager->createParticleSystem(tmp);
sys->attachToObject(obj);
}
}
Expand Down
14 changes: 5 additions & 9 deletions GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/AIUpdate/ChinookAIUpdate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1199,15 +1199,11 @@ UpdateSleepTime ChinookAIUpdate::update()
if ( GameClientRandomValueReal( 0.0f, chopperElevation ) < 5.0f )
{
const ParticleSystemTemplate *tmp = TheParticleSystemManager->findTemplate( getChinookAIUpdateModuleData()->m_rotorWashParticleSystem );
ParticleSystem *system;
if( tmp )
{
system = TheParticleSystemManager->createParticleSystem( tmp );
if( system )
{
system->setPosition( &pos );
}
}
ParticleSystem *system = TheParticleSystemManager->createParticleSystem( tmp );
if( system )
{
system->setPosition( &pos );
}
}

}
Expand Down
9 changes: 4 additions & 5 deletions GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/EMPUpdate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,12 +449,11 @@ UpdateSleepTime LeafletDropBehavior::update()
// start shoveling out those leaflets, boys.
const LeafletDropBehaviorModuleData *data = getLeafletDropBehaviorModuleData();
const ParticleSystemTemplate *tmp = data->m_leafletFXParticleSystem;
if (tmp)
ParticleSystem *sys = TheParticleSystemManager->createParticleSystem(tmp);
if (sys)
{
ParticleSystem *sys = TheParticleSystemManager->createParticleSystem(tmp);
if (sys)
sys->attachToObject(getObject());
}
sys->attachToObject(getObject());
Comment thread
stephanmeesters marked this conversation as resolved.
}

m_fxFired = TRUE; // hey, at least we tried.
}
Expand Down
40 changes: 18 additions & 22 deletions GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/HelicopterSlowDeathUpdate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,40 +229,36 @@ void HelicopterSlowDeathBehavior::beginSlowDeath( const DamageInfo *damageInfo )
locomotor->setMaxBraking( modData->m_maxBraking );

// attach particle system to bone if present
if( modData->m_attachParticleSystem )
ParticleSystem *pSys = TheParticleSystemManager->createParticleSystem( modData->m_attachParticleSystem );
if( pSys )
{
ParticleSystem *pSys = TheParticleSystemManager->createParticleSystem( modData->m_attachParticleSystem );
if( pSys )

// where do the offset attachment to
if( modData->m_attachParticleBone.isEmpty() == FALSE )
{
Drawable *draw = getObject()->getDrawable();

// where do the offset attachment to
if( modData->m_attachParticleBone.isEmpty() == FALSE )
if( draw )
{
Drawable *draw = getObject()->getDrawable();
Coord3D pos;

if( draw )
{
Coord3D pos;

if( draw->getPristineBonePositions( modData->m_attachParticleBone.str(), 0, &pos, nullptr, 1 ) )
pSys->setPosition( &pos );

}
if( draw->getPristineBonePositions( modData->m_attachParticleBone.str(), 0, &pos, nullptr, 1 ) )
pSys->setPosition( &pos );

}
else
{

// use location coord specified ... it will be zero if not given which is center of obj anyway
pSys->setPosition( &modData->m_attachParticleLoc );

}
}
else
{

// attach the particle system to the object
pSys->attachToObject( getObject() );
// use location coord specified ... it will be zero if not given which is center of obj anyway
pSys->setPosition( &modData->m_attachParticleLoc );

}

// attach the particle system to the object
pSys->attachToObject( getObject() );

}


Expand Down
18 changes: 6 additions & 12 deletions GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/LaserUpdate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,27 +357,21 @@ void LaserUpdate::initLaser( const Object *parent, const Object *target, const C
if( data->m_particleSystemName.isNotEmpty() )
{
const ParticleSystemTemplate *tmp = TheParticleSystemManager->findTemplate( data->m_particleSystemName );
if( tmp )
system = TheParticleSystemManager->createParticleSystem( tmp );
if( system )
{
system = TheParticleSystemManager->createParticleSystem( tmp );
if( system )
{
m_particleSystemID = system->getSystemID();
}
m_particleSystemID = system->getSystemID();
}
}

//If we don't have a particle system for the target effect, create it.
if( data->m_targetParticleSystemName.isNotEmpty() )
{
const ParticleSystemTemplate *tmp = TheParticleSystemManager->findTemplate( data->m_targetParticleSystemName );
if( tmp )
system = TheParticleSystemManager->createParticleSystem( tmp );
if( system )
{
system = TheParticleSystemManager->createParticleSystem( tmp );
if( system )
{
m_targetParticleSystemID = system->getSystemID();
}
m_targetParticleSystemID = system->getSystemID();
}
}
}
Expand Down
23 changes: 8 additions & 15 deletions GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/ParticleUplinkCannonUpdate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -934,15 +934,11 @@ void ParticleUplinkCannonUpdate::createConnectorFlare( IntensityTypes intensity
if( str.isNotEmpty() )
{
const ParticleSystemTemplate *tmp = TheParticleSystemManager->findTemplate( str );
ParticleSystem *system;
if( tmp )
ParticleSystem *system = TheParticleSystemManager->createParticleSystem( tmp );
if( system )
{
system = TheParticleSystemManager->createParticleSystem( tmp );
if( system )
{
m_connectorSystemID = system->getSystemID();
system->setPosition( &m_connectorNodePosition );
}
m_connectorSystemID = system->getSystemID();
system->setPosition( &m_connectorNodePosition );
}
}
}
Expand Down Expand Up @@ -970,14 +966,11 @@ void ParticleUplinkCannonUpdate::createLaserBaseFlare( IntensityTypes intensity
if( str.isNotEmpty() )
{
const ParticleSystemTemplate *tmp = TheParticleSystemManager->findTemplate( str );
if( tmp )
ParticleSystem *system = TheParticleSystemManager->createParticleSystem( tmp );
if( system )
{
ParticleSystem *system = TheParticleSystemManager->createParticleSystem( tmp );
if( system )
{
m_laserBaseSystemID = system->getSystemID();
system->setPosition( &m_laserOriginPosition );
}
m_laserBaseSystemID = system->getSystemID();
system->setPosition( &m_laserOriginPosition );
}
}
}
Expand Down
39 changes: 18 additions & 21 deletions GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/SlavedUpdate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -621,30 +621,27 @@ void SlavedUpdate::setRepairState( RepairStates repairState )
if( !data->m_weldingSysName.isEmpty() )
{
const ParticleSystemTemplate *tmp = TheParticleSystemManager->findTemplate( data->m_weldingSysName );
if( tmp )
ParticleSystem *weldingSys = TheParticleSystemManager->createParticleSystem(tmp);
if( weldingSys )
{
ParticleSystem *weldingSys = TheParticleSystemManager->createParticleSystem(tmp);
if( weldingSys )
Coord3D pos;
//Get the bone position
if( draw->getPristineBonePositions( data->m_weldingFXBone.str(), 0, &pos, nullptr, 1 ) )
{
Coord3D pos;
//Get the bone position
if( draw->getPristineBonePositions( data->m_weldingFXBone.str(), 0, &pos, nullptr, 1 ) )
{
pos.add( obj->getPosition() );
}
else
{
pos.set( obj->getPosition() );
}

weldingSys->setPosition( &pos );
Real time = (Real)(m_framesToWait * LOGICFRAMES_PER_SECOND);
weldingSys->setLifetimeRange( time, time );

AudioEventRTS soundToPlay = TheAudio->getMiscAudio()->m_repairSparks;
soundToPlay.setPosition( &pos );
TheAudio->addAudioEvent( &soundToPlay );
pos.add( obj->getPosition() );
}
else
{
pos.set( obj->getPosition() );
}

weldingSys->setPosition( &pos );
Real time = (Real)(m_framesToWait * LOGICFRAMES_PER_SECOND);
weldingSys->setLifetimeRange( time, time );

AudioEventRTS soundToPlay = TheAudio->getMiscAudio()->m_repairSparks;
soundToPlay.setPosition( &pos );
TheAudio->addAudioEvent( &soundToPlay );
}
}

Expand Down
Loading
Loading