0

I have a vector of class objects (bullets) and it works for the most part. But as soon as I try to delete the bullets it loops back and then causes a breakpoint. "basic game.exe has triggered a breakpoint." I have tried iterating backwards and forwards, but it always gets stuck.

I'm using SFML, and the objects are rectangle with positions, rotations and sizes.

    for (it = bullets.end(); it != bullets.begin(); it--)
    {
        it->draw(game); 
        it->move();
        if (it->bullet.getPosition().x > 800)
        {
            bullets.erase(it);
        }
    }

I'm a noob at coding, so if you need other infomation ill try and provide it.

Rama
  • 3,222
  • 2
  • 11
  • 26
Mitchell
  • 3
  • 1
  • 3
    erasing an element from a `std::vector` will invalidate all its iterators (in this case `it`), invoking undefined behaviour ( because you are attempting to use `it-1` in the next loop cycle, `it->draw(game); // access invalid iterator` ). – George Jan 27 '17 at 14:55
  • What is valid though is bullets.erase(it--); but then you'll have to reconsider your for loop – UKMonkey Jan 27 '17 at 14:58
  • 1
    Even first `it->draw(game);` is UB (as `it` is `end()`). – Jarod42 Jan 27 '17 at 14:59
  • 1
    You probably want to take a look at the [erase/remove idiom](https://en.wikipedia.org/wiki/Erase%E2%80%93remove_idiom). – G.M. Jan 27 '17 at 15:00

2 Answers2

4

When you call erase() on a vector, the iterators become invalid. Instead, consider trying this:

 for (auto it = bullets.begin(); it != bullets.end();)
 {
    it->draw(game); 
    it->move();
    if (it->bullet.getPosition().x > 800)
    {
      it = bullets.erase(it);
    }
    else
    {
      it++;
    }
  }
xEric_xD
  • 506
  • 1
  • 5
  • 12
  • i changed my code to this and it worked. thanks, but can you explain why this works and mine doesn't? – Mitchell Jan 27 '17 at 15:07
  • @Mitchell After you call erase the iterator becomes invalid. erase() returns an iterator to the element that follows the erased element, therefore validating the iterator again, which is what happens in the code above – xEric_xD Jan 27 '17 at 15:12
  • @Kevin sure, I misread and missed that the return of `erase()` is used to reset `it`. Downvode => upvote. – YSC Jan 27 '17 at 15:25
3

You may fix your loop with

for (auto& bullet : bullets) {
    bullet.draw(game);
    bullet.move();
}
bullets.erase(std::remove_if(bullets.begin(), bullets.end(),
                             [](const auto& bullet) {
                                 return bullet.getPosition().x > 800;
                             }),
              bullets.end());
Jarod42
  • 203,559
  • 14
  • 181
  • 302