How to remove elements from a vector that uses the vector size in the for loop

user_13705262 0
Viewed 137

I have a game where I shoot bullets at an object and I delete the object that gets hit by the bullet and bullet that are off screen as well.

For example:

std::vector<object> object_list;
for(size_t i = 0; i < object_list.size(); i++)
{

    if(object_list[i].hit())
    {
        object_list.erase(object_list.begin() + i);
    }
    else
        object_list[i].draw();
}

The problem with this is that when i remove an object, the size of the vector decreases so when it check the conditions, it fails and i get an error such as " vector subscript out of range." I could just choose not to render the asteroid by rendering those that haven't been hit, but the problem with that is that the no. of objects increases when hit(splits up) so eventually the program is going to get slower. I've used a similar concept for the off screen bullets but I can't find a way around it. I'm looking for a solution to this or better way of removing elements.

Both object and bullet are classes.

vector c++
7 Answers

You should split for loop in 2 parts:

  1. remove all "hit" elements:
object_list.erase(std::remove_if(object_list.begin(), 
object_list.end(), [](auto&& item) { return item.hit(); }),
object_list.end());
  1. draw remaining:
std::for_each(object_list.begin(), object_list.end(), [](auto&& item) { item.draw(); });

It's safer and more readable.

user_39219450

Same idea as the other answers but this code is a little easier with iterators

for (auto i = object_list.begin(); i != object_list.end(); )
{
    if (i->hit())
    {
        i = object_list.erase(i);
    }
    else
    {
        i->draw();
        ++i;
    }
}

vector::erase returns an iterator to the next element, which you can use to continue the loop.

user_8820030

Functional approach using the range-v3 library (C++20)

[...] I'm looking for a solution to this or better way of removing elements.

Using the ranges::actions::remove_if action from the range-v3 library, you can use a functional programming style approach to mutate the object_list container in-place:

object_list |= ranges::actions::remove_if(
    [](const auto& obj) { return obj.hit(); });

followed by subsequent ranges:for_each invocation to draw the object:

ranges::for_each(object_list, [](const auto& obj){ obj.draw(); });

DEMO.

user_45732470

You could do something like this:

for (size_t i = 0; i < object_list.size(); )
{
    if (object_list[i].hit())
        object_list.erase(object_list.begin() + i)
    else
    {
        object_list[i].draw()
        ++i;
    }
}
user_57432880

Let us say you are at i=5 and that object has been hit, after deleting that element, the obj at i=6 is shifted to i=5, and you haven't checked it, so just add i--; after your erase statement.

Another way to do it would be -

for(size_t i = 0; i < object_list.size();)
{
 if(object_list[i].hit())
 {
     object_list.erase(object_list.begin() + i);
 }
 else
 {
     object_list[i].draw();
     i++;
 }
}

Also, it could possibly be faster to just remove the object from the vector where you execute the code that marks the object as hit, that way you just need to draw all the objects which are left out in the list. Some more background into how you are doing all this would be helpful to decide something specific which would be better :)

user_75589920

The shown code does not fail or give a vector subscript out of range - it just does not consider every object, as it skips over the element after the removed one.

For very short and concise solutions employing concepts from C++11 and later, see the answer by Equod or the one by dfri

For better understanding the issue, and/or if you have to stick to for loops with indices, you basically have two options:

  1. Iterate over the vector in reverse direction (i.e. start at the last element), then items after the current one being shifted is not a problem;
for (int i=object_list.size()-1; i>=0; --i)
{
    if (object_list[i].hit())
    {
        object_list.erase(object_list.begin() + i)
    }
    else
    {
        object_list[i].draw()
    }
}
  1. Or, if the order is important (as I could imagine with items to draw), and you have to iterate from front to back, then only increase the counter i if you have not erased the current element:
for (int i=0; i<object_list.size(); /* No increase here... */ )
{
    if (object_list[i].hit())
    {
        object_list.erase(object_list.begin() + i);
    }
    else
    {
        object_list[i].draw();
        ++i;   // ...just here if we didn't remove the element
    }
}
user_6713660

I suspect that std::vector is not the container you want (but, of course, I don't know the entire code). Each call to erase implies reallocation of the right-part of the vector (and then copies of you objects), it could be very costly. And your actual problem is the symptom of a design problem.

From what I see, std::list is probably better:

std::list<object> objects;
// ...
for(std::list<object>::iterator it = objects.begin(); it != objects.end();)
{
    if(it->hit())
      objects.erase(it++); // No object copied
    else
    {
      it->draw();
      ++it;
    }
}
user_33781790
Related