Crash in SimGroup::onRemove()

#1 Alright... I believe I've fixed this one and it was an actual bug in the engine.

I had added some code which I had found in the latest version of TSE which looked like it finally dealt with the issue of ParticleEmitters not being properly cleaned up when a mission ended...

void ParticleEmitter::deleteWhenEmpty()
{

   SimGroup *cleanup = dynamic_cast<SimGroup *>( Sim::findObject( "MissionCleanup") );

   if( cleanup != NULL )
   {
      cleanup->addObject( this );
   }
   else
   {
      Con::errorf( "No mission cleanup group?" );

      // if no mission cleanup group then it's probably been removed in shutdown
      deleteObject();
   }

   mDeleteWhenEmpty = true;
}

The code looked fairly benign, but was in fact the root cause of the above problems. As soon as I added the code, I started experiencing problems in the SimGroup destructor... it was complaining about SimObjects being removed without their onRemove functions having been called. A quick glance at SimGroup::~SimGroup showed that any SimObjects that remained in the SimGroup were just being directly 'delete'd... I thought I was being smart and got the SimGroup desctructor to instead do things 'properly' and use deleteObject() on the remaining SimObjects so that their onRemove function would be properly called. This seemed to fix things... on the surface... but didn't get to the root of the problem.

The problem with the above code is that when the MissionCleanup group gets deleted it deletes all of the objects within it... So, when it reaches a ParticleEmitterNode, for example, it will delete it which makes the ParticleEmitterNode call ParticleEmitter::deleteWhenEmpty on the ParticleEmitter it owns which adds the ParticleEmitter to the MissionCleanup group (which has now already begun its removal process)... Needless to say, adding an object to a Group that has already begun its removal process is apparently not good...

So, all that to say that the above code is on the right track but missing a simple check:

void ParticleEmitter::deleteWhenEmpty()
{

   SimGroup* cleanup = dynamic_cast<SimGroup *>( Sim::findObject( "MissionCleanup") );

   if (cleanup != NULL && [b]!cleanup->isRemoved()[/b])
      cleanup->addObject( this );
   else
   {
      Con::errorf( "No mission cleanup group?" );

      // if no mission cleanup group then it's probably been removed in shutdown
      deleteObject();
   }

   mDeleteWhenEmpty = true;
}

I actually added a second check just because I'm kind of paranoid like that...

void ParticleEmitter::deleteWhenEmpty()
{
   [b]if (!mDeleteWhenEmpty)
  {[/b]
      SimGroup* cleanup = dynamic_cast<SimGroup *>( Sim::findObject( "MissionCleanup") );

      if (cleanup != NULL && [b]!cleanup->isRemoved()[/b])
         cleanup->addObject( this );
      else
      {
         Con::errorf( "No mission cleanup group?" );

         // if no mission cleanup group then it's probably been removed in shutdown
         deleteObject();
      }

      mDeleteWhenEmpty = true;
   [b]}[/b]
}

The above check ensures that we don't attempt to delete this ParticleEmitter more than once.


Now... all that being said, I would actually like to propose a more concrete fix than this: It seems like adding an object to a group that has already been marked as Removed would ALWAYS be a bad thing. Should this check not be added to the SimGroup::addObject() function? Maybe also the SimSet::addObject() function too? Then, in the ParticleEmitter::deleteWhenEmpty() function we could just check if the addObject() call was successful and, if not, just delete the Emitter right there. Come to think of it, an Assert would probably be the best method, since we really shouldn't be adding anything to any SimGroups that have been marked as Removed.

Hope this helps someone in the same position as me!

Cheers,

Stephane

#2 Thanks for the thorough write, Stephane (and for mailing me to make sure I see this), I've forwarded it to the appropriate folk to get it reviewed and integrated!