Erasing AnimatedSprite from vector triggers crash

[kepler0]

Hello,
I've been looking for an answer for the past few days, and it's been frustrating.

I have a vector array of Enemy classes:

vector<Enemy> enemies;

while (window.isOpen()) {

                        if (spawned == false) {
                               
                                for (int i = 0; i < 5; i++) {
                                        Enemy enemyNew(enemy);
                                        enemyNew.setPosition(sf::Vector2f(100 + (i * 160), 100));
                                        enemyNew.setHealth(enemyHealth);
                                        enemies.push_back(enemyNew);
                                };

                                spawned = true;
                        }
}
 

 The Enemy class has a private AnimatedSprite variable from https://github.com/SFML/SFML/wiki/Source:-AnimatedSprite, now, once a bullet hits an Enemy, the enemy loses health, blah blah, dies. Once the enemy is dead it sets a boolean to false.

In the main window loop, it erases the Enemy from the vector if that boolean is false.

                        enemies.erase(std::remove_if(enemies.begin(), enemies.end(), predEnemy), enemies.end());

However, once I kill an enemy, and it is erased from the vector, it returns

vector subscript out of range

and leads me to the following function in Animation.cpp (from the AnimatedSprite wiki):

const sf::IntRect& Animation::getFrame(std::size_t n) const
{
        return m_frames[n];
}

So I have identified it isn't a fault in my code. Also, this happens at random times, so if I launched into the game, waited a couple seconds, killed the enemy, then slowly killed all the other enemies aswell, there would be no crash. But as soon as I start firing rapidly it triggers the error. I assume this has to do with the frames of the Animation etc.

Also, no, the bullet is not animated.

Thanks in advance.

[eXpl0it3r]

Use your debugger. When you get a crash, find out why it crashes by looking at the call stack, checking currently active variables - especially index counters. Set break points to dig down into your code to see what's going on. Print out values if you want to keep track of the - especially container sizes and index counters.

It's most likely an index going out of range or a reference getting invalidated, e.g. by you erasing the object from the vector, but the other class still tries to manipulate the object.

With the given code snippets and description text it's quite hard to conclude anything.

[kepler0]

Well, Visual Studio isn't telling me much. It shows the 'n' variable, it changes every time it crashes, because it's on different frames. I've tried stopping and pausing the animation as soon as the the boolean is set to false. It makes no difference, I'm not sure what to do, the call stack seems okay. I think the problem is I have a vector of classes with the AnimatedSprite as a private variable, instead a vector of AnimatedSprites directly, maybe if I make the AnimatedSprite variable a public variable it could help, I'm not sure, still new to this stuff.

[Hapax]

As mentioned by eXpl0it3r:

It's most likely an index going out of range or a reference getting invalidated, e.g. by you erasing the object from the vector, but the other class still tries to manipulate the object.

Therefore, the problem is likely to be at the point of erasure. Remember that when erasing, iterators shouldn't be increased; they should instead be set to the result of erasure.

[kepler0]

Well I'm not erasing using a for loop... I used std::remove_if with the predicate function which returns true if the boolean that is set to false when the enemy is dead, is false. Each program tick it checks this and erases if true (remove-erase idiom). Using a for loop made no difference to this. I've also tried setting the animation to an empty one after the enemy dies but just before it is erased, so there are no frames running, but still not helping. :/

[Hapax]

Are you sure the predicate is correct?

Is it possible that you are using a constant for the number of enemies - or at least a supposedly up-to-date variable that isn't updated after the erasure - and then trying to index the item based upon that value?

It's just becoming guesswork here...

[kepler0]

Well this is my predicate:

main.cpp:

static bool predEnemy(Enemy enemy1 ) {
        if (enemy1.getShouldDraw() == false) {
                return true;
        }
        else {
                return false;
        };
}

Making it non-static doesn't make a difference.

I'm not exactly sure what you mean, but erasing an element from a vector automatically should resize the vector.

[Laurent]

Can you show the definition of your Enemy class?

[kepler0]

Enemy.h

class Enemy {
public:

        Enemy(sf::Texture &spritesheet) {
                enemy.setScale(2.5, 2.5);

                enemy.setFrameTime(sf::seconds(0.1));
                enemy.setLooped(true);

                moveAnimation.setSpriteSheet(spritesheet);
                moveAnimation.addFrame(sf::IntRect(0, 0, 32, 32));
                moveAnimation.addFrame(sf::IntRect(32, 0, 32, 32));
                moveAnimation.addFrame(sf::IntRect(64, 0, 32, 32));
               
                shouldDraw = true;

                health = 7;
        }

        void draw(sf::RenderWindow &window) {
                if (shouldDraw == true) {
                        window.draw(enemy);
                };
        }

        sf::Vector2f getPosition() {
                return enemy.getPosition();
        }

        void moveToPlayer(AnimatedSprite player, float speed, sf::Time frameTime) {
                sf::Vector2f direction = player.getPosition() - enemy.getPosition();
                float magnitude = sqrt((direction.x * direction.x) + (direction.y * direction.y));
                sf::Vector2f unitVector(direction.x / magnitude, direction.y / magnitude);

                enemy.move(unitVector * speed * frameTime.asSeconds());
        }

        void setColor(sf::Color color) {
                enemy.setColor(color);
        }

        void setHealth(int setHealth) {
                health = setHealth;
        }

        void update(sf::Time deltaTime, AnimatedSprite player, sf::Vector2f movement, gamemode &c, std::vector<Enemy> enemies) {
                if (shouldDraw == true) {
                        enemy.update(deltaTime);
                        enemy.play(moveAnimation);
                }
                else {};

                if (enemy.getGlobalBounds().contains(player.getPosition()) && shouldDraw == true) {
                        c = death;
                }
                else {
                       
                };

                if (health <= 0) {
                        enemy.stop();
                        shouldDraw = false;
                };
        }

        void stopAnimation() {
                enemy.stop();
        }

        sf::FloatRect getGlobalBounds() {
                return enemy.getGlobalBounds();
        }

        bool getShouldDraw() {
                return shouldDraw;
        }

        void minusHealth() {
                health--;
        }

        void setShouldDraw(bool sd) {
                shouldDraw = sd;
        }

        void setPosition(sf::Vector2f pos) {
                enemy.setPosition(pos);
        }

private:
        AnimatedSprite enemy;
        bool shouldDraw;
        int health;
        Animation moveAnimation;
};

[Laurent]

This is the same problem as with sf::Sprite and sf::Texture. AnimatedSprite stores a raw pointer to its associated Animation, so when the latter moves in memory the former gets a dangling pointer that results in undefined behaviour. When does Animation instances move in memory? When you move/copy the owner Enemy, which happens a lot since you store them by value in a std::vector. Every push_back, erase and remove may possibly trigger this undefined behaviour.

[kepler0]

I don't understand some of the keywords mentioned, but the only time I use the Animation, is to add frames, and set it as the animation for the AnimatedSprite. I'm not exactly sure what goes on in Animation.cpp/AnimatedSprite.cpp, but I think I have somewhat of an idea of what is going on.

[Laurent]

I don't understand some of the keywords mentioned

Just ask yourself: what happens when an Enemy instance is copied; how are the members of the new copy initialized (you can focus on the animation stuff only ).

The solution to your problem is simple: since copying an Enemy is incorrectly implemented, you either have to:
- implement copy (and assignment) correctly
- disallow copy

In this case, the second option is probably the most relevant, since "copying an enemy" doesn't make much sense in your game's logic; here you only have copies because of the underlying operations that std::vector performs behind the scenes, you don't actually needs these copies to happen.

How to disallow copy? First make sure that the compiler can spot any direct or indirect attempt to make a copy of an Enemy instance, by explicitly marking the copy constructor and assignment operator as disabled:

class Enemy
{
     Enemy(const Enemy&) = delete;
     Enemy& operator=(const Enemy&) = delete;

    ...
};

Then how do you make std::vector happy? Well, if your class was movable, and you were careful about what you do with your Enemy instances, it wouldn't be an issue at all. And doing that would probably bo a good idea, but right now you probably want to solve your problem with a quicker solution, that doesn't involve studying and modifying Animation & AnimatedSprite.

So, the simplest solution is to store pointers to Enemy:

std::vector<std::unique_ptr<Enemy>> enemies;

[kepler0]

Thank you for not just plainly posting the answer, but instead walking through the steps. However, this requires me to go through each time I reference enemies in main.cpp and change it to a unique_ptr. Also the .at() function now only accepts a size_t instead of an integer, but after reading upon unique_ptr, it seems very clever.