vector deletion problem

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.

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

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?

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]