Vector Collision

Question

I am quite green regarding vectors, and this is my first time actually using them for collision checking. This is for my project, and I am stumped on how to implement the collision. The current Collision check and response codes I have seem to be ... bad design.

This is my code:

for(auto it = ArrayofEntities.begin(); it != ArrayofEntities.end(); it++)
    {
        CEntity * go = (*it);

        for(auto i = ArrayofEntities.begin(); i != ArrayofEntities.end();)
        {
            //Collision for entities. Collision Event returns the iterator after an element is erased.
            CEntity * other = (*i);
            if (go != other)
            {
                if (!theCollision.CheckCollision(go, other, false, false, false, false)) //Checks if it has collided go with other
                {
                    i = go->CollisionEvent(*other, ArrayofEntities);    //Run collision code, setting i to the iterator which is returned.

                    //break;
                }
                else
                {
                    i++;
                }
            }
            else
            {
                i++;
            }
        }
}

CEntity is the base class for all the entities.

My CheckCollision just returns a true or false on the collision, and my collision event runs the collision and returns an iterator (because I might have to destroy things in the vector).

My collision event is below

vector<CEntity*>::iterator  bullet::CollisionEvent(CEntity &other, vector<CEntity*> & theArray)
{
case ZOMBIE:
    {
        other.hp -= power * 0.01;//Effect


        int Counter, index, bulletindex;
        auto it = theArray.begin();
        //Find the bullet and the other in the array.
        for (it = theArray.begin(), Counter = 0; it != theArray.end();it++, Counter++)
        {
            CEntity *go = NULL;
            go = (*it);
            if (go == &other)
            {
                index = Counter;
            }
            if(go->ID == BULLET && go->GetX() == GetX() && go->GetY() == GetY())
            {
                bulletindex = Counter;
            }
        }

        this->~bullet();//Delete the bullet
        theArray.erase(theArray.begin() + bulletindex);


        if(other.hp <= 0)
        {
            other.~CEntity();
            it = theArray.erase(theArray.begin() + index); //delete from array.


            return it;
        }

        it = theArray.begin() + index;
        return it;

    }

}

I have basically done this like how I would do an array. Just check it against itself. The error it gives is "Vector Iterator not Incrementable", on the first for loop after the collision event has been run.

So my question: 1) What am I doing wrong? 2) Is my thinking to do this like checking arrays wrong?

This is my school project, so I have full control of the codes.

I would prefer to have a quick fix over a complete rewriting of all the collision codes, but if it really comes down to it, I will rewrite my codes.

You're invalidating your iterators of you entity collection if that `erase` ever fires. And frankly something like this: `this->~bullet()` has a *horrid* smell. Likewise with `other.~CEntity()`. *Why* ? — WhozCraig, Aug 29 '14 at 07:57
It has a horrid smell because it is a horrid one. I need some guidance on how to do this. I think I have fallen into the trap of trying too hard to make this work. — user2418426, Aug 29 '14 at 07:58
These are all dynamic objects, right? as in the content are all entity derivations? Perhaps a vector of smart pointers may be wise, and as I look at it, I *think* my assessment of your iterators may be *partially* premature. The last one worries me, but you seem to know that `erase()` returns the next iterator so there is no reason to increment it afterward for a proper enumeration. And the assessment of invalidating iterators is definitely still valid for the most-outer iteration loop. — WhozCraig, Aug 29 '14 at 08:00
When I was making this out, my thought process was just to make the array of entities check against itself and than if collided, they activate their collision code. But as I try to do this more and more, the more ugly the code is coming out. — user2418426, Aug 29 '14 at 08:00
Yes, all the objects are under the same entity. What does a smart pointer mean? I know what is a pointer; I've done polymorphism, but I have never really understood what is a smart pointer. — user2418426, Aug 29 '14 at 08:02
[**Intelligent pointers**](http://en.cppreference.com/w/cpp/memory). I think all you need for this to work are some smart pointers and better management of your container iterators. — WhozCraig, Aug 29 '14 at 08:04
I wouldn't store raw pointers in your container. I would probably use `boost::shared_ptr`. That way you don't have to worry about releasing memory when a container entry is removed. Also, you seem to be always doing forward iteration with removal from the middle of the array: consider `list` instead of `vector`, it's more efficient for the removals. Finally, it's probably not good the way you're handling the iterator after erasing through it. See [this answer](http://stackoverflow.com/questions/2874441/deleting-elements-from-stl-set-while-iterating). — Andy Brown, Aug 29 '14 at 08:19
Ok, but what are the problems I am having? And how would smart pointers help me? None of my pointers here have a new or delete to them. " better management of your container iterators" -- What does this mean? How would I achieve this? — user2418426, Aug 29 '14 at 08:21

score 2 · Accepted Answer · answered Aug 29 '14 at 08:19

If you look at the implementation of std::remove_if, you'll see that they've solved the issue of iterator invalidation in another way. instead of erasing elements, they move them to the end of the array.

This may be the easiest solution for you as well. Keep an iterator which points after the last "live" entirty. It starts out at .end but as bullets hit things, you swap the entities to the back of your range and decrement that last-live iterator.

Then, when you're done looping over your array, you clean up with a single call to .erase.

And yes, you should use either std::unique_ptr<CEntity> or std::shared_ptr<CEntity> in the collection. In that way, .erase won't just erase the pointer but also the object pointed to.

+1 `std::remove_if` would certainly seem a logical path to take for this, as it would shove all matching conditions to the end of the sequence, sift non-matches down, and give a clean location from which to wipe out with that single erase and clean up all the destroyed entities. Great suggestion. — WhozCraig, Aug 29 '14 at 08:25

Vector Collision

1 Answers1