[SOLVED]Deleting dynamically allocated objects within vector

[hurstshifter]

Hey Everyone,

I'm working on the collision detection portion of a small game and am stuck on one part. Here is the code with a description (in english) below. Sorry if the code is a bit messy.

Code: Select all

void all_collision(Player*& bluePlayer, Player*& redPlayer)
{
	std::vector<Pizza*>::iterator itr;

	for(itr = bluePlayer->pizzas.begin(); itr < bluePlayer->pizzas.end(); ++itr)
	{
		if( collision_check( (*itr)->getrect(), redPlayer->getrect() ) )
		{
			delete (*itr);
			itr = bluePlayer->pizzas.erase(itr);
		}
	}

	for(itr = redPlayer->pizzas.begin(); itr < redPlayer->pizzas.end(); ++itr)
	{
		if( collision_check( (*itr)->getrect(), bluePlayer->getrect() ) )
		{
			delete (*itr);
			itr = redPlayer->pizzas.erase(itr);
		}
	}

}

This function takes 2 Player* references in as its arguments. Each Player object holds a vector pizzas of type <Pizza*>. Pizzas are allocated through another function, listed below.

Code: Select all

void Player::throw_pizza()
{
	Pizza* temp = new Pizza(pizzaSurface);
	temp->collisionBox.x = collisionBox.x;
	temp->collisionBox.y = collisionBox.y;

	pizzas.push_back(temp);
}

My problem comes when the first function all_collision() goes to delete one of the Pizzas. (This is my very first attempt at dealing with dynamically allocated objects within an array so I'm not sure if the allocation is wrong, the removal, or BOTH ) What happens is the game throws a runtime error as seen below.



The interesting thing I discovered is that this only happens on the last Pizza (pizzas are projectiles in this game) that is deallocated. If I throw 5 pizzas, the first 4 are removed just fine. My only guess so far is that I am perhaps setting the final iterator itr to a null value and that is causing the final iteration of the for loop to shit itself. Any help here would be appreciated.

[CC Ricers]

When the last item of a vector is erased, then erase() would return bluePlayer->pizzas.end() and that's what itr is set to. This should already equal the end of the loop but I never found the < comparison operator to be that clear when looping with iterators. Maybe the loop is trying to read past the end.

Instead, of

Code: Select all

itr < bluePlayer->pizzas.end();

use

Code: Select all

itr != bluePlayer->pizzas.end();

You could also try breaking out of the loop if itr == bluePlayer->pizzas.end().

Alternatively you can use boost::shared_ptr instead of allocating with new, but rewrite the code carefully if you do. It will throw a different set of errors if you mix raw pointers with shared pointers when they deal with the same data.

[hurstshifter]

Thanks for the suggestions CC. Checking if itr == pizzas.end() and breaking if so did work.

I guess my question now would be if this is typical practice to ensure that the iterator does != end() before it hits the last condition check of the loop? If not, I need to find out why this is happening.

[Live-Dimension]

An error is much more nasty and much more troublesome then the slightest non-noticeable performance loss. If there is even a CHANCE, no matter how big, that doing said expression can cause an error, you have to deal with that possibility. That's typical practice. Hell, I'd seriously consider checking on every loop as to avoid this bug.

edit: Short answer - Yes!

[hurstshifter]

Thanks Live. I think I should rephrase my question a little though.

I realize that it's not ok to risk the chance of running into a fatal runtime error such as this. I will definitely continue to do it the way that it has been fixed if that is the proper way to handle iterating through vectors. But what I really want to know is if this is truly the way to handle iterating through a vector and removing all the elements. I want to make sure I am doing it the right way, and if adding an if(itr == vector.end() ) break; is the proper way to handle this, then great. But I feel like the for loop itself should handle this once it reaches the end (which is the condition that the loop is looking for).

[K-Bal]

You could also do:

Code: Select all

for(it = list.begin(); it != list.end(); )
{
  if(condition)
  {
    it = list.erase(it);
  }
  else
    ++it;
}