vector deletion problem
Hi
I am using a vector for storing bullets in my game. Every time the player fires his weapon a bullet object is added to this vector.
Of course when a bullet collides with something i.e a wall or another player i need to remove that bullet from the vector.
This is where i have the problem. When i delete the bullet from the vector, my game ends up in an infinite loop!
The bullet class has a boolean attribute called "bulletCollided". This is set to true when the bullet has collided with a wall.
So the following code loops through the vector list and attempts to delete any bullets which have collided:
std::vector<Bullet*> bulletList;
if(bulletList.size() > 0)
{
for(std::vector<Bullet*>::iterator i = bulletList.begin();
i != bulletList.end();
i++)
{
Bullet* bullet = *i;
bullet->Update(map, elapsedTicks);
if(bullet->HasBulletCollided())
{
// bullet has collided so remove it from the list
// delete the bullet object
delete bullet;
// remove from the list
bulletList.erase(i);
}
}
}
Could someone please point out what i am doing wrong here. I am quite new to C++ programming.
Thanks.
Whenever you modify a vector, any iterators pointing to any elements from it are no longer valid. So, to counter this problem, you must do an assignment from the return of the erase method to the iterator. This way, that iterator stays at the same position, relative to the beginning, it was before the erase.
i = bulletList.erase(i);
Since you say that you're new to C++ jatz, you should probably also know that you're going to have to reformat your loop to remove the i++ because
i = bulletList.erase(i);
will have the effect of incrementing you're looking for.
i = bulletList.erase(i);
will have the effect of incrementing you're looking for.
1) Don't store the things by pointer without a reason (Bullets being polymorphic, or being shared between containers, would qualify as reasons; but the first sounds highly dubious given the class name, and in the second case you have much bigger problems with memory management than what you've shown here). Just store them by value. The whole reason for using a vector is so that it can do memory management for you. Why do your own on top of that? It adds more work for you AND slows things down.
2) Don't, generally, include the class' name in a class' member functions. It's redundant; it doesn't provide extra information.
3) Don't check if .size() > 0. Check if ! .empty() instead. It's easier to read (more directly expresses your intent) and could be faster depending on the implementation (it won't be for any sane implementation of std::vector, but it certainly could be for std::list).
4) But don't check for an empty container before iterating over the elements of the container, anyway. Iterating over 0 elements is perfectly valid; it correctly does nothing.
5) To fix the actual problem, you *could* muck around with the iterators in the indicated way, *or* you could just use standard library algorithms, as shown.
Welcome to C++ :)
2) Don't, generally, include the class' name in a class' member functions. It's redundant; it doesn't provide extra information.
3) Don't check if .size() > 0. Check if ! .empty() instead. It's easier to read (more directly expresses your intent) and could be faster depending on the implementation (it won't be for any sane implementation of std::vector, but it certainly could be for std::list).
4) But don't check for an empty container before iterating over the elements of the container, anyway. Iterating over 0 elements is perfectly valid; it correctly does nothing.
5) To fix the actual problem, you *could* muck around with the iterators in the indicated way, *or* you could just use standard library algorithms, as shown.
class BulletUpdater { Map* map; // or whatever type it actually is - weak pointer int elapsedTicks; // or whatever type it actually is public: BulletUpdater(const Map& m, int t) : map(&m), elapsedTicks(t) {} // You *do* accept the Map by reference in Bullet::Update, yes? void operator()(Bullet& b) { b.Update(*map, elapsedTicks); }};// ...std::vector<Bullet> bullets;std::for_each(bullets.begin(), bullets.end(), BulletUpdater(map, elapsedTicks));bullets.erase(std::remove_if(bullets.begin(), bullets.end(), std::mem_fun_ref(&Bullet::HasCollided)), bullets.end());
Welcome to C++ :)
Thanks for helping everyone.
The bullets are now being deleted without any problems.
Just one last thing.
Could someone explain the benefits of Zahlman's code from the previous post over the way i have coded.
In other words what is the benefit of creating the BulletUpdater class and storing that in the vector instead of storing pointers to my Bullet class?
The bullets are now being deleted without any problems.
Just one last thing.
Could someone explain the benefits of Zahlman's code from the previous post over the way i have coded.
In other words what is the benefit of creating the BulletUpdater class and storing that in the vector instead of storing pointers to my Bullet class?
Quote:Original post by jatz
Could someone explain the benefits of Zahlman's code from the previous post over the way i have coded.
In other words what is the benefit of creating the BulletUpdater class and storing that in the vector instead of storing pointers to my Bullet class?
The benefit is that you are using well-tested library functions rather than writing your own potentially buggy code. That's always a good thing to do. However, there are trade-offs and the overall benefits depends of the situation. I think Zahlman's example is a little extreme. The problem with the BugUpdater class is that it adds another level of indirection. The problem with the "bullets.erase" line is that it is too complicated. I would split it into two lines -- the first uses remove_if to sort the array and the second erases dead bullets. Same results, same performance, easier to read.
BTW, BulletUpdater objects aren't being stored. Look up the "for_each" function.
Quote:Original post by jatz
The bullets are now being deleted without any problems.
Just checking... are you absolutely sure?
As was pointed out by ShaunPeoples if you simply only add:
i = bulletList.erase(i);
then you are probably skipping over some bullets. You should only increment the iterator (i++) when you dont erase a bullet.
Erasing from a vector *can be* an expensive operation, have you considered swapping a dead bullet with a live bullet at the end of the vector and then popping it off the back as this would be faster. (which is basically what std::remove_if does except you then simply erase the whole lot at once instead of many indivdiual pops).
Although if youre happy with the performance as-is then its not really a problem [wink]
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement