Self-removing animator II

[binford]

Hi there,

This is not really a request for help, because I solved the issue already. I just would like to get some comments in order to find out whether my solution is a good one.

The Task:
Im currently programming a CnC-like RTS game. I needed an animator that followed some waypoints on a virtual two dimensional map. After the animation is done, the animator should be removed automatically. I think the reaseon for this is obvious, if you just keep the animator attached it wastes memory(not so bad) and cpu time during each animation loop(very bad).

My first approach was to write a custom animator and hold a reference to its SceneNode within the animator. After the animation was done, I tried to call

Code: Select all

mSceneNode->removeAnimator( this );

which resulted in a segfault. Im not really sure why this happens, but my explanation is this: removeAnimator deletes the animator instantly and returns. Then, the programm tries to continue after mSceneNode->removeAnimator( this ); but the memory isnt allocated anymore. Is this true?

Ok my solution for this involved changing the irrlicht engine. In ISceneNodeAnimator.h i introduced a bool flag.

Code: Select all

class ISceneNodeAnimator : public io::IAttributeExchangingObject 
{ 
public: 
ISceneNodeAnimator() : IsDead( false ) {} 

//! destructor 
virtual ~ISceneNodeAnimator() {} 
virtual ESCENE_NODE_ANIMATOR_TYPE getType() 
{ 
   return ESNAT_UNKNOWN; 
} 

bool isDead() const { return IsDead; } 

protected: 
   bool IsDead; 
};

...and in ISceneNode.h::OnAnimate()

Code: Select all

for (; ait != Animators.end(); ++ait) 
{ 
   if( (*ait)->isDead() ) 
   { 
      (*ait)->drop(); 
      ait = Animators.erase( ait ); 

      if( ait == Animators.end() ) 
         break; 
   } 
   else 
   { 
      (*ait)->animateNode(this, timeMs); 
   } 
} 

What do you guys think? Is this a good solution? Might this even be considered for one of the next releases or is this a too special case?

PS: My first post to this issue can be found here: http://irrlicht.sourceforge.net/phpBB2/ ... p?p=110906

[IPv6]

Another solution should be list with animators scheduled to be removed
List should be processed at endScene point...

[vitek]

The reason you get a segmentation fault is because you delete the animator while the scene node is iterating over its animators. If there is one animator, and you remove it, the following loop will screw up...

Code: Select all

core::list<ISceneNodeAnimator*>::Iterator ait = Animators.begin();
for (; ait != Animators.end(); ++ait)
   (*ait)->animateNode(this, timeMs);

If there was one animator, and it called removeAnimator() on itself, the iterator ait would be incremented past the end. The condition ait != Animators.end() would succeed, and animateNode() would be called on an invalid animator pointer. If there were more than one animator and you remove one that is not the first, it will skip the next one.

Your code is bad in that it skips any animator that follows a dead animator. You erase the dead one, so now the iterator points to the one after the dead one. Then the iterator is incremented on the next loop.

Personally, I would not add an isDead() and the additional member variable. They are unnecessary. Yo ucould have the animateNode() method return a bool that indicates whether or not that animator is done. Then the loop would look something like this...

Code: Select all

for (; ait != Animators.end(); ) 
{ 
   if ((*ait)->animateNode(this, timeMs))
   {
     (*ait)->drop();
     Animators.erase(ait);
   } 
   else
   {
     ++ait;
   }
} 

Either that, or I would use the code I gave you here. If you don't like the global functions, you could optionally make addToDeletionQueue() a member of the scene manager, and add a getSceneManager() to the scene node. Then the animator could schedule itself for deletion using the scene manager, and the scene manager would flush the deletion queue in drawAll().

Travis

[binford]

Hmm sorry for my ignorance, but I still dont get it.

If there was one animator, and it called removeAnimator() on itself, the iterator ait would be incremented past the end.

Suppose we have only one iterator in the list. If this animator removes itself

Code: Select all

core::list<ISceneNodeAnimator*>::Iterator ait = Animators.begin();
for (; ait != Animators.end(); ++ait)
   (*ait)->animateNode(this, timeMs);  // within animateNode the object deletes itself
                                       // this makes (*ait) point to a non-existing list element
                                       // in the end, we're incrementing an undefined iterator, right?

Sorry for my hairsplitting, I just want to really understand whats going on there. I think the app doesent crash because we're calling animateNode() on an non-existing element. I think it crashes even earlier because we're trying to increment an invalid iterator. (If you look at the iterator class, you'll se that calling iterator++ means dereferencing the non-existent list node and trying to access "next")

Thx for your suggestions for solving that, but I think changing the signature of animateNode would be not a good idea, because you would have to change the signature in ISceneNodeAnimator and therefore in all derived classes.

And your right! My code is indeed rubbish. I think I'll change it to

Code: Select all

for (; ait != Animators.end(); ++ait)
{
   if( (*ait)->isDead() )
   {
      (*ait)->drop();
      ait = Animators.erase( ait );

      if( ait == Animators.end() )
         break;
   }
   (*ait)->animateNode(this, timeMs);

}

This all leads my to two requests for (very minor) improvements:
1. It would be nice if irrlichts iterator classes would have the < and > operator. This would make the code more robust because you could use a safer break condition in a for loop.

Code: Select all

for( Iterator curPos = mConatiner.begin(); curPos < mContainer.end(); curPos++ )

2. I think it would be cool if irrlicht would throw some more exceptions. For example if you're iterating beyond the bounds of a container. (Does throwing Exception involve a performance loss?)

[vitek]

Yes, you're right. I didn't look at the implementation of core::list<T>::erase() and incorrectly assumed that it updated the input iterator, but it does not. It returns a new iterator.

So when you call erase(), you should assign ait to the result of that function call, just like you were already doing. This will keep the iterator valid, and will skip you to the next element. You just need to be sure not to increment the iterator again on the next loop or you will skip the element after the one that was just erased. The code that you've posted above will work, except with the small caveat that the animator will stick around until the render after the animator became useless.

I think that changing the signature of animateNode() is cleaner, and yes it would break the existing interface, requiring you to update the other animator types. That said, I've already provided code that doesn't involve modifying Irrlicht at all and you've rejected that twice...

As for adding operator< and operator> to the list iterator, I think that is a bad idea. Normally, as in the case of std::list<>, the list iterators are bidirectional iterators, and bidirectional iterators define operator== and operator!= only. Since an iterator is approximately equivalent to a pointer, those operators are easily implemented when the container stores elements in contiguous memory blocks. This is not the case for a list. You would need to store some ordering information inside the list node since the address of the list node cannot be reliably used do determine ordering.

Adding those operators would make it easier to write bug-free code, but it would unnecessarily complicate the list iterator, and it would break 'compatibility' with the list concept as it implies that the elements in a list are ordered, which they are not required to be.

Travis

[cyberwan]

Hi, I'm very interested about this topic, because I have exactly the same problem.. I have tested the proposed solutions, of course they work, but they're not easy to use.. I think that changing the signature of animateNode() is a good idea, even if it breaks compatibility.. That problem can be resolved by adding a simple "return false" (or "return true" ? that would appear more logical to me...).
Is it any chance that it would be included to irrlicht ?

[cyberwan]

Hello ? Could anyone take a decision about this ?

[IPv6]

I think that changing the signature of animateNode() is a good idea, even if it breaks compatibility..

Sorry, but breaking backward compatibility is no way a good idea. Animators is big part of any project build on irrlicht, forcing people to change their sources for rare (for most of irrlicht users i think) case is very bad thing. It is really annoying to dig into working code with every major update, with a great chance of adding new errors into tested code (game for example). for example i have a lot if custom animators and scene nodes and breaking this kind of things will make all of them unusable

Personally i`m using "scheduler" approach for this problem. Just one array and small piece of code before endScene is really nothing when compared to other code and game logic. Your approach suffers from several problem.
- What if animator adds new animator to the same node and then remove itself? added animator will be skipped for one frame on that node. some times it matters.
- sometimes there is need to rearrange the order of animators, attached to the node (yes, sounds crazy ). I don`t know how to do that kind of things if your approach will be taken

[cyberwan]

Sorry, but breaking backward compatibility is no way a good idea.

OK, I agree with that, but maybe it's a good idea to keep in mind for Irrlicht 2.0 ?

- What if animator adds new animator to the same node and then remove itself? added animator will be skipped for one frame on that node. some times it matters.

I don't really understand what you mean, is it adding a new animator from an animator being animated ? My first thought about that is that it may not work, even with the actual version..

- sometimes there is need to rearrange the order of animators, attached to the node (yes, sounds crazy ). I don`t know how to do that kind of things if your approach will be taken

I really don't see the link with adding a bool to the method. Actually, I don't see how you're rearranging the order of animators.. is it during the animation ?? dangerous operations..