SFML community forums

Help => Graphics => Topic started by: vro on July 31, 2011, 04:01:00 am

Title: anything look wrong with this code?
Post by: vro on July 31, 2011, 04:01:00 am

I am using the Collision example/code from the wiki.
My game runs fine 90% of the time, but every once in a while it crashes on a random collision between a projectile and an asteroid.
When I debug I get a seg fault, and it points to the code below. Specifically the line with the getSprite() calls (it's just a "getter" function in the projectile and asteroid classes that returns a sf::sprite object)

Am I doing something wrong with the vector iter? Like I said the game usually runs without crashing, but the seg fault has to do something with my vector of projectile objects, trying to reach something that's not there anymore.

Code: [Select]


for (vector<Projectile*>::iterator iterP = pProjectileVector->begin(); iterP != pProjectileVector->end(); iterP++)
{
    for (vector<Asteroid*>::iterator iterA = pAsteroidVector->begin(); iterA != pAsteroidVector->end(); iterA++)
    {
        if (DetectCollision.BoundingBoxTest((*iterP)->getSprite(), (*iterA)->getSprite()))
        {
            delete *iterP;
            pProjectileVector->erase(iterP);
            delete *iterA;
            pAsteroidVector->erase(iterA);
            iterP--;
            iterA--;
            Asteroids.setAsteroidCount(Asteroids.getAsteroidCount()-1);
        }
    }
}


Title: anything look wrong with this code?
Post by: JAssange on July 31, 2011, 04:18:13 am

Iterators are invalid once you modify their container.

Title: anything look wrong with this code?
Post by: vro on July 31, 2011, 04:22:35 am

that's why I decrement the iterator when a projectile collides.

if I am doing it wrong then how is it done correctly?

lot's of people have suggested to use a vector for this sort of thing

Title: anything look wrong with this code?
Post by: Disch on July 31, 2011, 04:23:02 am

Once you erase() something from a vector, all existing iterators of that vector become invalid.

That means:

Code: [Select]


        if (DetectCollision.BoundingBoxTest((*iterP)->getSprite(), (*iterA)->getSprite()))
        {
//...
            pProjectileVector->erase(iterP);  // iterP is no longer valid after this call
//...
            iterP--;  // this is meaningless.  Next loop iteration, iterP will be bad/invalid
//  same problem with iterA


iterP--; is a bad idea anyway.  A general rule of thumb is that if you need to have something that "undoes" the for loop increment, then you probably shouldn't be using a for loop.

To get a valid iterator after an erase, use erase's return value.

This code should work (untested):

Code: [Select]


vector<Projectile*>::iterator iterP = pProjectileVector->begin();
while( iterP != pProjectileVector->end() )
{
    vector<Asteroid*>::iterator iterA = pAsteroidVector->begin();
    while( (iterA != pAsteroidVector->end()) && (iterP != pProjectileVector->end()) ) // have to check iterP in this loop, too
      // since it could reach the end of the vector as well
    {
        if (DetectCollision.BoundingBoxTest((*iterP)->getSprite(), (*iterA)->getSprite()))
        {
            delete *iterP;
            iterP = pProjectileVector->erase(iterP);  // iterP =
            delete *iterA;
            iterA = pAsteroidVector->erase(iterA);  // iterA =
            Asteroids.setAsteroidCount(Asteroids.getAsteroidCount()-1);
        }
else  // no collision
{
            ++iterA;
   }
    }

if(iterP != pProjectileVector->end())
   ++iterP;
}


Title: anything look wrong with this code?
Post by: vro on July 31, 2011, 04:28:44 am

thank you disch, makes perfect sense now.

I read at cplusplusreference that an iterator wasn't valid once you erased, but I thought it just meant it didn't automatically adjust the iterator for you.

as you can see i'm still learning the stl :/

thanks again

Title: anything look wrong with this code?
Post by: Nexus on July 31, 2011, 10:25:07 am

By the way, you should take a look at the STL algorithms std::remove() and std::remove_if(). They are far more efficient than a for-loop with erase() calls, namely O(n) vs. O(n²).