• Advertisement
Sign in to follow this  

vector deletion problem

This topic is 3867 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

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.

Share this post


Link to post
Share on other sites
Advertisement
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);

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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.


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

Share this post


Link to post
Share on other sites
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?

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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]

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement