Trying to make a game with C++, using lists to store bullets and enemies, but they are not erased

Question

I've been trying to make a pretty simple space shooter game with C++, and I have been having a lot of trouble trying to use lists to store enemies and bullets.

Basically I followed the post here almost exactly to store my bullets:

SDL Bullet Movement

I've done something similar to store my enemies.

However, when I call bullets.erase(it++), for some reason the bullet is not erased.

When the bullet movement is run for the next frame, it tries to re delete the bullet and segfaults the program. When I comment out the erase line, it runs fine, but the bullets are then never erased from the list...

Is there any reason why the elements of the list aren't being deleted? I also set it up to print the number of elements in the list for every iteration, and it does not go down after deleting.

Thanks!

EDIT:

Here's the specific code I'm using to store my enemies and having them act:

std::list<Grunt*> doGrunts(std::list<Grunt*> grunts)
{
  for(std::list<Grunt*>::iterator it = grunts.begin(); it != grunts.end();)
    {

      if((*it)->getHull() == 0)
        {                                       
          delete * it;                                             
          grunts.erase(it++);
        }
      else
        {

          (**it).doUnit(grunts, it);
          ++it;
        }
     }
  }

Grunt is my enemy class, and the list grunts is a global variable. Is that my problem? When I pass the global into the function it becomes local? I assumed lists would be a reference type so thought this wouldn't be a problem.

Sorry if this was a dumb question, I'm very new to C++, this is the first major thing I'm working on.

Answer 1

container.erase calls the destructor of the contained object if it has one. pointers like Grunt* do not have destructors. You must manually delete the pointer THEN call erase.

You also have to be careful of invalidation of iterators, incrementing an invalidated pointer will cause a BLOCK_HEAD failure.

pseudo-code:

for each iterator "it" in container //(DO NOT increment iterator in for loop block statement!)
    if it->position is off_screen {
        delete *it;
        *it = NULL; //Prevents seg fault due to double-delete.
        container.erase(it++);
    } else {
        ++it;
    }

SUPER EDIT

In light of the code you posted:

1) Yes, a copy of grunts is created because the list is being passed by value. It is also being shadowed because you named the local variable the same as the global one. 2) Why are you passing a GLOBAL variable around? Either A) just use it, or B) make it non-global and pass by reference.

Answer 2

You need to use resource-managing classes. Deleting your own memory is extremely error-prone. The only viable solution in the long term is to use classes like std::shared_ptr, std::unique_ptr, or simply storing your bullets by value. Why on earth would you use a linked list of pointers? That's quite unnecessary.

Answer 3

If it is an iterator for the std::list<Bullet*> bullets, calling bullets.erase(*it) should be invalid since *it is a Bullet* not an std::list<Bullet*>::iterator and erase requires an iterator. If you want to remove elements by referring to their value you could try the remove function.

EDIT: Also, with an std::list you do not have to worry about invalidating iterators using list::erase. I am not sure if the same guarantee is given for list::remove.

Answer 4

There are two things that I can see in that code that could cause problems.

First, you aren't checking for NULL Grunt pointers. You dereference (*it)->GetHull, and if (*it) in this case is NULL you'll get your segfault.

Second, when you call erase, you could be moving memory about in the list. It's not really safe to use an iterator (i.e. a pointer into the list structure) after the list is modified. But specifically in this case, you should NOT be incrementing it, as that will skip an element. Like this

(G1)->(G2)->it->end -> erase (it) -> (G1)->(G2)->[it]end (maybe) -> it++ -> ??? the iterator's already at the end!

I can't say for sure if that's happening, but you shouldn't do it anyway, as you'll skip over elements of the list.

Answer 5

For the record, the std::list::erase() method returns the next iterator in the list, making incrementing a counter redundant, and in my opinion, a safer method for erasing container elements.

Assuming grunts is a std::list<Grunt*>:

auto iter = grunts.begin();
while(iter != grunts.end())
{
  delete *iter; // No need to set NULL here, if you're going to remove from list.
  iter = grunts.erase(iter);
}

Furthermore, you should start collecting a nice set of helper macros for stuff like deleting pointers. This will save you some headaches later.

#define SAFE_DELETE(x) if(x){delete x; x=NULL;} // Keep in a common header somewhere..
Grunt* pGrunt = new Grunt();
Grunt* pBadGrunt = NULL;
SAFE_DELETE(pGrunt); // pGrunt is now free'd and NULL
SAFE_DELETE(pBadGrunt); // Nothing happens, pointer is still NULL