1) Use code tags when posting code. The code you posted is almost unreadable.
2) Your post is plain C++, i.e. there is no use of MFC, ATL, or other technologies relevant to the usage of the Visual C++ compiler. It is straight C++, with a few types such as "Sprite" that we don't know about. Therefore it would have been relevant to post in the non-Visual C++ forum.
For everyone's benefit, here is your code, formatted using code tags:
Code:
bool innerHit = false;
for (std::vector<Sprite*>::iterator outerIter = sprites.begin(); outerIter != sprites.end() && (!sprites.empty()); outerIter++)
{
Sprite* spriteOne = *outerIter;
for (std::vector<Sprite*>::reverse_iterator innerIter = sprites.rbegin(); innerIter != sprites.rend() && (!sprites.empty()); innerIter++)
{
Sprite* spriteTwo = *innerIter;
if (spriteOne->getRect() != spriteTwo->getRect() && spriteOne->getRect().overlaps(spriteTwo->getRect()) && spriteOne != spriteTwo)
{
spriteOne->collisionReaction(spriteTwo);
spriteTwo->collisionReaction(spriteOne);
spriteOne->collisionDestroy(sprites);
spriteTwo->collisionDestroy(sprites);
innerHit = true;
}
if (innerHit)
break;
}
}
There is no need to write loops like this if you know what the ultimate goal is when it comes to proper usage of containers. The way you rewrite this entire sequence is to use
algorithms from the STL that alters the container in a safe way.
Here is the problem. It calls
Code:
std::vector<Sprite*>::erase()
which is a very big issue.
Let's start at the beginning:
Code:
for (std::vector<Sprite*>::iterator outerIter = sprites.begin(); outerIter != sprites.end() && (!sprites.empty()); outerIter++)
The above has the issue of being inefficient and very hard to read. First, it would be a lot easier if you used typedef:
Code:
typedef std::vector<Sprite*> SpriteVector;
//...
for (SpriteVector::iterator outerIter = sprites.begin(); outerIter != sprites.end() && (!sprites.empty()); ++outerIter)
Note how we typedef'ed the vector, and we use pre-increment in the outerIter loop variable.
Now we see that you are erasing items in the vector while you're looping (see the item in red). Whenever I see this, I can bet you had to write this loop and your inner loop (which has the same sort of checking) 2, 3, 4, or more times to get working in some manner, all due to bugs being discovered. The goal, if possible, is to not write loops that erase items in a container while looping within the container. This is especially the case for a simple container such as vector. There are very few times when you need to erase elements in a vector while looping over the vector.
The way you go about doing erasures in a vector is to arrange the items in the vector so that the "erased" items appear at the end of the container, with an iterator pointing to the beginning of the erased items so that you can safely call erase() in a separate step starting at this iterator. Again, usage of remove_if() / erase or std::partition() and erase() is recommended.
The easiest way to overcome this is in your Sprite class, have a simple bool member variable that sets whether this sprite has been destroyed (make sure you initialize this variable when you create a sprite to
false). Then just set this boolean to true in your collisionDestroy method --
do not erase the item in the vector -- in effect, you're delaying the real erasure of the items for later. In your other loops above, you skip over the ones that have this bool variable set to true -- that is much easier than figuring out what is empty and potentially accessing an invalid iterator. Now, you are no longer erasing items in the middle of the loop which means your loop constraints need no longer check for invalid iterators, since they will always be valid.
Code:
void Enemy::collisionDestroy(std::vector<Sprite*>& sprites)
{
for (SpriteVector::iterator iter = sprites.begin(); iter != sprites.end(); ++iter)
{
Enemy* tmp = dynamic_cast<Enemy*>(*iter);
if (this == tmp && collisionType == 3 || collisionType == 1)
{
tmp->hasCollided = true;
break;
}
}
}
I changed the code so that we no longer erase, but just set the boolean. But this can also be rewritten, again, using algorithms (and why did you need to cast to Enemy??)
Code:
void Enemy::collisionDestroy(SpriteVector& sprites)
{
if ( collisionType == 1 || collisionType == 3)
{
SpriteVector::iterator it = find(sprites.begin(), sprites.end(), this);
if ( it != sprites.end() )
it->hasCollided = true;
}
}
If the goal was to find the sprite that equaled "this", and then erase it, then the above code should work.
Then in a separate step afterwards, outside of all of these loops, you use the remove_if() / vector::erase idiom.
Code:
#include <algorithm>
bool hasCollided(Sprite* sprite)
{
return sprite->isCollided(); // this is just a public "getter" for the hasCollided flag that was set earlier.
}
//...
void Enemy::RemoveCollided()
{
sprites.erase(remove_if(sprites.begin(), sprites.end(), hasCollided), sprites.end());
}
Problem solved, or at least, made easier. The above checks the vector and erases all the ones that have the "hasCollided" flag set to true. Basically, all I did was come up with a scheme to invalidate the item (setting a simple bool member), and then at the end, erase the invalidated items.
(Note that I am using C++ 98 features, and not C++ 11. If I knew you were using C++ 11 (or Visual Studio 2013), then the RemoveCollided would make use of lamda's in the remove_if() call instead of a separate function such as hasCollided).
Regards,
Paul McKenzie