Removing class objects from vector (vector subscript out of range)

[lancerhnj]

Hello everyone, I am trying to create a simple coin catching game using SDL and C++. At the moment I am trying to delete to coins as the travel off the bottom of the screen (eventually when colliding with the player, but that's a problem for another day). I am creating a vector which holds the coin objects with a for loop and push_back.

std::vector<Coin> CoinSet;
for (int i=0;i<MAX_COIN;i++)
{
	Coin temp;
	CoinSet.push_back(temp);
}

The problem occurs when I try to remove (or delete) these objects from the vector using pop_back.

//The function I am calling to flag coin if it runs off the edge of the screen
bool Coin::ClearCoin()
{
	if (mPosY - COIN_HEIGHT < SCREEN_HEIGHT)
	{
		return false;
	}
	return true;
}

//In main
//Creating vector
std::vector<Coin> CoinSet;
for (int i=0;i<MAX_COIN;i++)
{
        Coin temp;
	CoinSet.push_back(temp);
}

//Removing objects from vector
//*Problem*
std::cout<<"Size of set"<<CoinSet.size()<<std::endl;
for (int i=0; i<CoinSet.size(); i++) 
{
    CoinSet[i].move(player.collider());
    if (CoinSet[i].ClearCoin() == true)
    {
	CoinSet.pop_back();
    }
    player.move(CoinSet[i].collider()); 
}

As soon as a coin does go off the edge of the screen the flag triggers and that is when the program crashes. I get the error "Vector subscript out of range" The cout that I put above the loop does provide some insight, in that even with only one coin off screen, the size of the set rapidly decreases.

Any help would be greatly appreciated!

[tath]

this is wrong on few level.

std::vector<Coin> CoinSet;
for (int i=0;i<MAX_COIN;i++)
{
	Coin temp;
	CoinSet.push_back(temp);
}

Coin temp create instance, and when you use push_back you create another instance and then you copy temp. You could write push_back(Coin()).

When you pop() instance it will go out of scope and will be deleted.

for (int i=0; i<CoinSet.size(); i++) 
{
    CoinSet[i].move(player.collider());
    if (CoinSet[i].ClearCoin() == true)
    {
	CoinSet.pop_back();
    }
    player.move(CoinSet[i].collider()); 
}

this is wrong. If you pop_back count of elements in vector decrease, and i is stll set for value before poping. So your program is trying to access element that is already poped.

If you have problems, debug your program and try to solve them

[lancerhnj]

Okay what do you suggest? I played around in the debugger and you're right, i does continue working at 10 while the size of CoinSet has been reduced to 9. Im not quite sure where to go? If it were up to me I wouldn't use vectors at all, but as I will be modifying it on the fly so much, an array makes little sense.

[tath]

also pop_back i removing last element, not the one pointed by i. You should use erase to remove specific element.

for ( unsigned i = CoinSet.size(); i != 0 ; i--)
{
	CoinSet[i].move(player.collider());
	player.move(CoinSet[i].collider());

	if (if (CoinSet[i].ClearCoin() == true))
	{
		numbers.erase(CoinSet.end() - i);
	}
}

[TwilightSpectre]

First, just on an efficiency note, removing an element from a vector at any point other than the end is quite expensive (it needs to shift all the values in front along one) - you may want to consider using a std::deque if removals from the front as well will be common.

The other advantage is that with a std::deque you can turn it into a queue (actually, there is an STL class that is a wrapper on std::deque called std::queue), so you can use its pop_front method to remove the first Coin off the front of the list. Though, this is assuming that a Coin 'pushed' onto the queue will always be cleared first, otherwise there isn't much point to this approach.

Also, @tath, what is with line 6? And why do you feel the need to go backwards along the vector? All you've done is make segfaults, remember it starts at element 0 and goes to size() - 1. Maybe you meant this:

for (std::vector<Coin>::size_type i = 0; i < CoinSet.size(); ++i) {
    CoinSet[i].move(player.collider());
    player.move(CoinSet[i].collider()); 
    // The above line - why is the player moving for every coin?

    if (CoinSet[i].ClearCoin()) {
        numbers.erase(CoinSet.start() + i);
    }
}

[lancerhnj]

I feel I should give an update as I've been at work the past few days and unable to. The game now runs well enough, with the coins being erased as soon as the exit the screen (success!), but I am now trying to remove the coins as they come into contact with the player.

for (std::vector<Coin>::iterator i=CoinSet.begin(); i!=CoinSet.end(); i++) 
    {
        if (i->PlayerClear(player.collider()) == true)
	{
		score++;
		cout<<score<<endl;
		CoinSet.erase(i);
	}
    }

This code works perfectly well until "CoinSet.erase(i), which gives the error "vector iterator not incremental". I tried using "CoinSet.clear()" at various points, but to no avail. Any help on this would be great, thanks in advance!