iTickable corrupted pointer in ProcessList

#1 ITickable is (or used to be) a bit... tricky. These threads might be useful for you Lukas:

www.garagegames.com/community/forums/viewthread/58289
www.garagegames.com/community/forums/viewthread/58368
www.garagegames.com/community/forums/viewthread/39389

The last thread in particular is interesting.

#2 @Chris thanks! I think the last thread got my pointed in the right direction.

A little more digging shows that it happens right after another iTickable object is deleted.
More accurately I have an "Impact" object which calls a callback a number of times and then deletes itself.
It's advanceTime method looks like this:

void DOTImpact::advanceTime( F32 timeDelta ) 
{
   TimeSpent += timeDelta * 1000;
   U32 TimeSinceTick = TimeSpent - (TicksSpent * TickMS);
   while( TimeSinceTick >= TickMS )
   {
      if(TicksSpent >= TickCount){
         deleteObject();
         break;
      }
      doImpact(SourceObject, TargetObject);
      TicksSpent++;

      TimeSinceTick = TimeSpent - (TicksSpent * TickMS);
   }
}

when deleteObject() gets called, the engine will crash before Tween::advanceTime gets called another time, will do some more digging into this.

Edit: as a matter of fact it happens right after the DOTImpact object is destroyed, it exits the call to advanceTime, grabs the next object in the iterator and tries to call advanceTime on that one and crashes.

#3 Okay so this is what I think happens:
The DOTImpact is deleted, the next object is now indexed at the current object, so the iterator will now try and grab the next object. Normally this would result in a skipped list, but since the DOTImpact is the second last object in the list, and the last object is skipped, i != getProcessList().end() never becomes true so it will keep iterating, just linearly grabbing the next address in memory, which is bad.

Shouldn't be so hard to fix tho.

#4 That definitely sounds about right. One of those fun 'gotchas!' about ITickable.

#5 Okay this was what I did to fix it:

//for( ProcessListIterator i = getProcessList().begin(); i != getProcessList().end(); i++ ){
   for(int i = 0; i < getProcessList().size(); ){
      ITickable* iTick = getProcessList()[i];
      iTick->advanceTime( dt );
      // Check if we should advance the iterator. Don't if the processed  
      // object has deleted itself.  
      if(iTick == getProcessList()[i])
         ++i;
      // Special case if the object was the last in the list
      else if(i == getProcessList().size())
         --i;
   }

Basically the fix from David Wyand with an added special case.

Edit: pull requested

#6 Cool that you are optimizing Twillex! :)

While I don't follow the problem specifically, I imagine a cleaner solution is to work backwards in the list. This is a general way to avoid issues of the object being processed being removed (as I have to do at least a couple of times in Twillex).

In other words, would it fix your problem if you had

for (int i = getProcessList().size() - 1; i >= 0; i--) {
   // process here as if everything were normal
}

Whether and object removed itself or not, and whether the elements after it are slid down, each item will get one call as desired.

P.S. The solution you have above may fail sometimes on the last element of the list? What if it deleted itself and then you called getProcessList()[i]?

#7 @Charlie already made the fix and it's a Pull Request on GitHub, just needs to get merged ;)
If the list were processed backwards it makes some complications with processing order that may bring issues later on.
Or it takes changes to how the iTickable's is queued, and I prefer to keep the fix as simple as possible :)
Trying to figure out whether another case in the if clause is needed for testing if the object itself is deleted.. Somehow it haven't given me any errors yet.
Edit: added an if-clause for if the i gets bigger than the ProcessList size.