1
\$\begingroup\$

I've made a vector which is used by a universally accessible namespace called 'BULLET_HANDLER'. This vector stores and updates all bullets that are produced from the 'shootBullet' function. The problem I'm facing is that after a while of firing bullets, the vector does not draw or update the bullets and does not push back anymore bullets. It also doesn't crash the game and the game continues normally but none of the objects can fire bullets.

Here is the code for the Bullet Handler:

#include "BULLET_HANDLER.h"

static vector<Bullet*> BULLET_ARRAY;

void BULLET_HANDLER::shootBullet(Bullet *BULLET) {
  BULLET_ARRAY.push_back(BULLET);
}

void BULLET_HANDLER::Update() {

  for (unsigned int i = 0; i < BULLET_ARRAY.size(); i++) {
    if (BULLET_ARRAY.size() > 0) {
      if (
        BULLET_ARRAY[i]->getX() <= 0 - BULLET_ARRAY[i]->getW() || 
        BULLET_ARRAY[i]->getX() >= WINDOW_WIDTH) {

        BULLET_ARRAY.erase(BULLET_ARRAY.begin() + i);
      }
    }

    BULLET_ARRAY[i]->Update();
  }
}

void BULLET_HANDLER::Draw(SDL_Surface *Window_Surface) {

  for (unsigned int i = 0; i < BULLET_ARRAY.size(); i++) {
    BULLET_ARRAY[i]->Draw(Window_Surface);
  }
}

void BULLET_HANDLER::clearBulletArray() {
  BULLET_ARRAY.clear();
}
\$\endgroup\$
0
5
\$\begingroup\$

You're shooting yourself in the foot:

for (unsigned int i = 0; i < BULLET_ARRAY.size(); i++) {
  if (BULLET_ARRAY.size() > 0) {
    if (
      BULLET_ARRAY[i]->getX() <= 0 - BULLET_ARRAY[i]->getW() || 
      BULLET_ARRAY[i]->getX() >= WINDOW_WIDTH) {

      BULLET_ARRAY.erase(BULLET_ARRAY.begin() + i);
    }
  }

  BULLET_ARRAY[i]->Update();
}

You modify the collection while you iterate over it: if you delete an item while you iterate, the BULLET_ARRAY.size() will change over the course of the loop and at some point you'll never be able to reach the end of your vector.

There are a couple of ways to get around that: use a list** instead of vector. In your case when you erase an element from the list, an iterator to the next item is returned, so you can easily erase elements as you inspect them. If you still want to use a vector, you'll have to find a way to erase the elements in another way.

Also, please note that even though you call erase(BULLET_ARRAY.begin() + i), this does not mean that the underlying bullet is deleted (because you use pointers), so we can assume one of two things: 1) you know about it and you take that into consideration somewhere else in the code or 2) you have a memory leak there. If you assume that the vector is the owner of the bullet, you should consider looking into std::unique_ptrs for your memory management. If the ownership of the bullets is shared, look into std::shared_ptrs.

Finally, I suggest you learn to use the debugger and verify case-by-case situations in your code to make sure that what you get is what you expect.


** The list is less memory-access efficient (due to cache misses) than the vector. In your specific case, I don't think you have the requirement for that much of a memory efficiency, so I still suggest you use the list.

\$\endgroup\$
1
\$\begingroup\$

My answer is merely a small addition to the excellent answer by Alexandre. Your problem really is modifying a vector while iterating over it. The solutions mentioned by Alexander are great, but I just wanted to share a trick I've been using for removing elements from vectors while iterating over them.

Basically, you would keep a separate container (this can be an array if that'll do, but a vector would probably be the easiest), and place your "dead" bullets into this other list. Once you are done with your iterating, you would then just loop over the bullets in the dead container, and remove those from the other container.

std::vector<Bullet*> bullets;
std::vector<Bullet*> deadBullets;

for (Bullet* bullet : bullets)
{
    if (bullet->IsAlive())
    {
        bullet->Update();
    }
    else 
    {
        deadBullets.push_back(bullet);
    }
}

for (Bullet* deadBullet : deadBullets)
{
    removeFromVector(deadBullet, deadBullets);
}

Apparently to some people it is not obvious how to remove items from an std::vector, so for those (even though this is not actually related to the question here, it's merely a small implementation detail) you can take a loot at the SO answer here.

Now, this might have a small impact on your memory usage, but all in all this is a very clear and clean solution, and is easily extendable if you want to start adding new bullets to the list. Just add another vector addedBullets, and iterate over those after the loop, adding those to the bullet container.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.