CODEWITHSUNDEEP
c++vector
furrylion
furrylion

Reputation: 31

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

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.

Upvotes: 3

Views: 587

Answers (7)

Caduchon
Caduchon

Reputation: 5201

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;
    }
}

Upvotes: 0

codeling
codeling

Reputation: 11369

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
    }
}

Upvotes: 0

dfrib
dfrib

Reputation: 73186

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.

Upvotes: 3

Equod
Equod

Reputation: 556

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.

Upvotes: 5

john
john

Reputation: 87959

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.

Upvotes: 2

Ayush
Ayush

Reputation: 131

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 :)

Upvotes: 0

catnip
catnip

Reputation: 25388

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;
    }
}

Upvotes: 2

Related Questions