Ok, I think I see what you mean. Everytime an activebullet is removed your entire vector list gets shifted down to fill in the hole of the removed element. In such a case I think you want to check that same position in the vector again as this time around you''ll be checking a different item in the same index. I suppose a simple fix for it is to cancel out the bullet++ if you did the if by maybe putting a bullet-- inside the if. Or you can just move the bullet++ from the for into the else part of the if.




--{You fight like a dairy farmer!}

As for the code within the loop :

(1) release bullet resources
Correct, however it would be better to do this in Bullet''s destructor instead of in a Shutdown() function. Or at least to have your destructor call that function automatically.
(2) delete the bullet object
Correct
(3) erase the bullets position from the vector
Correct

As for the loop itself, yes you do have a problem.
You''ll, be much better off using iterators rather than a loop counter. Iterators are the equivalent of pointers for STL containers. And they are meant, well, to iterate through a container

std::vector<Bullet*>::iterator itor = m_Bullets.begin()while( itor != m_Bullets.end() ){   float x, y;   (*itor)->GetPosition(&x,&y);   if(m_EnemyManager->CheckBulletCOllision(x,y))   {      (*itor)->Shutdown(); // Unnecessary with a proper destructor.      delete *itor;      m_NumberOfActiveBullets--;				      // erase will correctly advance the iterator      itor = m_Bullets.erase(itor);    }   else ++itor;}

Using the return value from erase ensures you are only checking each element in the vector once, regardless of what happens to their indices when one of them is deleted. And since a vector "knows" how many elements it contains, there should be no need for you to keep track of the number of active bullets : it''s simply m_Bullets.size();

Be warned though that, a vector is not the ideal data structure for the current task. Removing elements in the middle of a vector is expensive : as others mentioned, all the following elements must be shifted one place. A linked list would work better and, unless I am mistaken (which I could be), the code above would work with a std::list without modification (beside telling it to use ''list'' instead of ''vector'').

At any rate, one way you can gain speed is only call erase once, as the following example shows:

#include <algorithm>#include <functional>// This class provides the loop''s body as an inline function.// This allows the loop to be faster (than using function// pointers) and to carry more information (than a function// pointer would), since it is a full-fledged object.struct FnReleaseCollidedBullets               // function of one parameter: public std::unary_function<Bullet*&, bool>  // Takes a Bullet* by ref, returns a bool.{private:   EnemyManager* manager;public:   // The EnemyManager object (along with all the other state   // that could be necessary) is provided upon object construction.   // ''explicit'' disables implicit conversions, just to be safe.   explicit FnReleaseCollidedBullets( EnemyManager& manager_ )   : manager( &manager_)   {}    // I have heard some people have had problems with the    // typedefs provided by unary_function, so if it doesn''t    // work for you, replace result_type with bool and    // argument_type with Bullet*&    inline result_type operator()( argument_type bullet ) const    {       float x, y;  // could be private members if we don''t need                    // the function to be reentrant (no threading)       bullet->GetPosition(&x, &y);       if( manager->CheckBulletCollision(x,y) )       {          bullet->Shutdown(); // Unnecessary with a proper destructor.          delete bullet;          bullet = 0;   // That''s why we pass by ref.       }       return bullet == 0;    }};// Now we use it. First check collisions, delete collided// bullets and set the pointer in the container to NULL.std::for_each( m_Bullets.begin(), m_Bullets.end(),               FnReleaseCollidedBullets(m_EnemyManager) );// Now we compact the the vector (remove), by moving non-null// elements over the null elements (in one pass), then we// remove from the vector the extraneous copies that were left// at the end (remove did ''shift'' the elements without reducing// the size of the array, some elements have been duplicated,// erase will remove those duplicates. This is cheap because// we really hold pointers to Bullets.)vector.erase( std::remove( m_Bullets.begin(), m_Bullets.end(), 0 ),              m_Bullets.end() );

It may seem a bit overkill but, once you have created an appropriate set of function classes (like FnRemoveCollidedBullets), most container manipulations become trivial.

Also note that we had to make sure and delete the bullets before throwing their pointers out of the vector. A technique called "smart pointers", which consists in creating objects that act like pointers, but manage deletion of the pointed-to object ''smartly'' (for example by counting how many smart pointers reference a given object, and deleting it when the last reference goes away), would simplify the code even more.

Yes, I''m aware you might not consider the above code simpler than your own. However, I do, as it is much easier (if maybe a bit verbose) to write a function object than to make sure all your loops are always correct

With smart pointers (like boost::shared_ptr), and a proper destructor, so that Shutdown() be unnecessary.

typedef std::shared_ptr<Bullet> spBullet; // smart pointer to Bulletstd::vector< spBullet > m_Bullets;struct FnCheckBulletCollision                 // function of one parameter: public std::unary_function<Bullet*&, bool>  // Takes a Bullet* by ref, returns a bool.{private:   EnemyManager* manager;public:   explicit FnCheckBulletCollision( EnemyManager& manager_ )   : manager( &manager_)   {}   inline result_type operator()( argument_type bullet ) const   {     float x, y;     bullet->GetPosition(&x, &y);     return manager->CheckBulletCollision(x,y);   }}m_Bullets.erase( std::remove_if( m_Bullets.begin(),                                 m_Bullets.end(),                                 FnCheckBulletCollision( m_EnemyManager ),                 m_Bullets.end() );// The function object tests for collision// remove_if overwrites the pointers bullets that have collided// the smart pointers call the destructor for those bullets// the destructor releases the resources// erase removes the leftover bullet pointers at the end of the vector.

Personally, I like that better than writing my own loops
It expresses what you are trying to achieve much better.
Your mileage may vary.

[ Start Here ! | How To Ask Smart Questions | Recommended C++ Books | C++ FAQ Lite | Function Ptrs | CppTips Archive ]
[ Header Files | File Format Docs | LNK2001 | C++ STL Doc | STLPort | Free C++ IDE | Boost C++ Lib | MSVC6 Lib Fixes ]