How can I erase a shared_ptr from vector

Question

Like the codes below, m_vSprites is a vector of shred_ptr, if one of his elements update fail, I would like to erase it from the vector, but my codes crash when I would like to using erase. But I don't why, anyone can help?

The reason I need to use erase is because my application would continually add elements in the vector, but would also continually erase objects from vector if some elements meets their killing conditions. if I didn't erase it, the vector would become huge as the program works!

RECT rcOldSpritePos;
    typedef boost::shared_ptr<Sprite> SmartSprite;
vector<SmartSprite>::iterator siSprite;
for (siSprite = m_vSprites.begin(); siSprite != m_vSprites.end();)
{
    // Save the old sprite position in case we need to restore it
    rcOldSpritePos = (*siSprite)->getPosition();

    if (!((*siSprite)->update()))
    {
        // Kill the sprite
        //siPreviousSprite = siSprite-1;
        siSprite = m_vSprites.erase(siSprite);
    }
            else
            {
                  siSprite++;
                  // See if the sprite collided with any others
          if (checkSpriteCollision(*siSprite))
        // Restore the old sprite position
        (*siSprite)->setPosition(rcOldSpritePos.left, rcOldSpritePos.top);
            }

}

I have changed the codes as someone suggested, but still failed in the erase function, Please anyone have some suggestions?

Some more information how I add an element in the vecotr

Any problem here?

    SmartSprite sprite;
if (0 < enemies.size())
{
    // Pick a random enemy to drop bomb
    size_t nRandEnemy = (rand() % enemies.size());
    RECT rRandEnemy = enemies.at(nRandEnemy)->getPosition();
    sprite.reset(new BombSprite(m_system, rRandEnemy.right-OBJECTSIZE/2,   
            rRandEnemy.bottom));
    m_vSprites.push_back(sprite);
}

My sprite class didn't have any destructor...

one more information is, while I debug, found it crash in erase internal function: _Destroy(_Mylast - 1, _Mylast);

---

problem solved!, the reason is in my Sprite class, I wrap it as a smart pointer, and created another smart pointer as its member variable. now I didn't use the smart pointer member variable, and the system didn't crash again.... I will continue to think about why I can't use a smart pointer inside that class, when erase sprite from vector, does it matter in his member variable? do I still need to delete that member variable if only use raw pointer instead of suing smart pointer?

Answer 1

If the iterator you are about to erase is the vector's begin iterator, then this;

siPreviousSprite = siSprite - 1;

Is undefined behavior. Restructure your loop like this:

for (siSprite = m_vSprites.begin(); siSprite != m_vSprites.end(); )
{

    rcOldSpritePos = (*siSprite)->getPosition();

    if (!((*siSprite)->update()))
    {
        // Kill the sprite
        siSprite = m_vSprites.erase(siSprite);            
    }
    else
    {
        ++siSprite;
    }
}

If there is still a crash, then there must be something wrong with your sprite class.

Answer 2

I'd try with the erase-remove idiom.

You may want to expose a bool attribute from the sprite class, and check if it is true to erase the sprite from the vector. Something like this (code tested with VC10/VS2010 SP1):

#include <algorithm>
#include <iostream>
#include <memory>
#include <ostream>
#include <vector>

using namespace std;


struct Sprite
{
    explicit Sprite(int spriteId)
     : kill(false)
     , id(spriteId)
    {
    }

    int id;
    bool kill;
};


int main()
{
    typedef shared_ptr<Sprite> SmartSprite;  
    typedef vector<SmartSprite> SpriteArray;

    SpriteArray sprites;
    for (int i = 0; i < 5; i++)
        sprites.push_back( make_shared<Sprite>(i*11) );

    for (auto it = sprites.begin(); it != sprites.end(); ++it)
        cout << (*it)->id << " ";
    cout << endl;

    sprites[2]->kill = true;
    sprites[3]->kill = true;

    // Remove sprites with kill flag set    
    sprites.erase
    (
        remove_if
        (
            sprites.begin(),
            sprites.end(),
            [](const SmartSprite& ps){ return ps->kill; }
        ),
        sprites.end()
    );

    for (auto it = sprites.begin(); it != sprites.end(); ++it)
        cout << (*it)->id << " ";
    cout << endl;
}

Output:

0 11 22 33 44
0 11 44

Answer 3

If siSprite == m_vSprites.begin() then siSprite-1 is not legal. The usual way of writing this kind of loop is

for (siSprite = m_vSprites.begin(); siSprite != m_vSprites.end(); )
{
    rcOldSpritePos = (*siSprite)->getPosition();
    if ((*siSprite)->update())
    {
         ++siSprite;
    }
    else
    {
        siSprite = m_vSprites.erase(siSprite);
    }
}

Not sure if this will actually fix your problem because your error could be something completely unrelated.

Answer 4

Erasing potentially leads to new allocation, iterators may break there. It is better to do like this:

for(size_t i=0; i<v.size(); ++i)
  if(!v[i].update())
  {
    v.erase(v.begin()+i);
    --i; // since the elements shifted up.
  }

Otherwise you may want to use queues instead, erasing operation is very expensive on vectors.

EDIT:

I have tested it with a dummy Sprite class and works fine, the error must be within the destructor of the Sprite class.