-4

I'm programming a Breakout game in C++. I'm having a HUGE problem that's preventing me from giving the game multi-ball functionality. I think it has something to do with the destructor. Have a look:

for loop for the balls (Driver.cpp):

for (Ball& b : balls) { // Loops over all balls
    (...)
    // Collision for when you miss
    if (b.getYPos() > HEIGHT) { // If the ball falls below the defined height of the screen
    balls.erase(balls.begin() + b.getID()); // Wipe the ball out of memory to make room (Troublesome line)
    Ball::addToID(-1); // Shift the ball ID to assign to the next ball back one
    (...)
}

And I get this error:

Debug Error!
Program: Breakout.exe
abort() has been called
(Press Retry to debug the application)

Do you know why this mysterious crash is happening? Or more importantly, a fix for it?

Here's a replicable piece of code to help:

Driver.cpp:

#include <vector>
#include <allegro5\allegro.h>
#include "Ball.h"

using namespace std;

vector<Ball> balls(0); // Pay attention to this line

const POS WIDTH = 1600, HEIGHT = 900;


int main {
    while (running) {
        if (al_key_down(&key, ALLEGRO_KEY_SPACE)) { // Spawn the ball
                balls.push_back(Ball(WIDTH / 2, 500, 10, 10)); // Spawn the ball
                balls[Ball::getIDtoAssign()].setYSpeed(5);
            }

            for (Ball& b : balls) { // Pay attention to this loop
                b.draw(); // This line is what's crashing.
                b.move();

                (...)
                // Collision for when you miss
                balls.erase(
                    remove_if(balls.begin(), balls.end(),
                        [=](Ball& b) {
                            // Collision for when you miss
                            return b.getYPos() > HEIGHT; // If the ball falls below the defined height of the screen, wipe the ball out of memory to make room
                        }
                    ),
                    balls.end()
                            );
                }
            }
        }
    }
    return 0;
}

Ball.h:

#pragma once
#include <allegro5\allegro_primitives.h>

using namespace std;

class Ball {
public:
    Ball();
    Ball(float x, float y, float w, float h);
    ~Ball();
    void draw();
    void move();
    float getYPos();
    void setYSpeed(float set);
private:
    float xPos; // Horizontal position
    float yPos; // Vertical position (upside down)
    float width; // Sprite width
    float height; // Sprite height
    float xSpeed; // Horizontal speed
    float ySpeed; // Vertical speed (inverted)
}

Ball.cpp:

#include "Ball.h"

short Ball::ballIDtoAssign = 0;

Ball::Ball() {
    this->xPos = 0;
    this->yPos = 0;
    this->width = 0;
    this->height = 0;
    this->xSpeed = 0;
    this->ySpeed = 0;
}

Ball::Ball(float x, float y, float w, float h) {
    this->xPos = x;
    this->yPos = y;
    this->width = w;
    this->height = h;
    this->xSpeed = 0;
    this->ySpeed = 0;
}

Ball::~Ball() {
    // Destructor
}

void Ball::draw() {
    al_draw_filled_rectangle(xPos, yPos, xPos + width, yPos + height, al_map_rgb(0xFF, 0xFF, 0xFF));
}

void Ball::move() {
    xPos += xSpeed;
    yPos += ySpeed;
}

float Ball::getYPos() {
    return yPos;
}

void Ball::setYSpeed(float set) {
    ySpeed = set;
}

2 Answers2

3

You cannot modify a container while you are iterating through it with a range-for loop. You don't have access to the iterator that the loop uses internally, and erase() will invalidate that iterator.

You can use the container's iterators manually, paying attention to the new iterator that erase() returns, eg:

for(auto iter = balls.begin(); iter != balls.end(); ) { // Loops over all balls
    Ball& b = *iter: 
    ...
    // Collision for when you miss
    if (b.getYPos() > HEIGHT) { // If the ball falls below the defined height of the screen
        ...
        iter = balls.erase(iter); // Wipe the ball out of memory to make room
    }
    else {
        ++iter;
    }
}

Alternatively, use the erase-remove idiom via std::remove_if() instead:

balls.erase(
    std::remove_if(balls.begin(), balls.end(),
        [=](Ball &b){
            // Collision for when you miss
            return b.getYPos() > HEIGHT; // If the ball falls below the defined height of the screen, wipe the ball out of memory to make room
        }
    ),
    balls.end()
);

UPDATE: now that you have posted more of your code, it is clear to see that you are trying to use ID numbers as indexes into the vector, but you are not implementing those IDs correctly, and they are completely unnecessary and should be eliminated.

The Ball::ballID member is never being assigned any value, so in this statement:

balls.erase(balls.begin() + b.getID()); // The troublesome line

Trying to erase() the result of balls.begin() + b.getID() causes undefined behavior since the iterator has an indeterminate value, thus you can end up trying to erase the wrong Ball object, or even an invalid Ball object (which is likely the root cause of your runtime crash).

Also, in this section of code:

balls.push_back(Ball(WIDTH / 2, 500, 10, 10)); // Spawn the ball
balls[Ball::getIDtoAssign()].setYSpeed(5);
Ball::addToID(1);

Since you want to access the Ball object you just pushed, that code can be simplified to this:

balls.back().setYSpeed(5);

And I already gave you code further above to show you how to remove balls from the vector without using IDs.

So, there is need for an ID system at all.

With that said, try something more like this:

Driver.cpp:

#include <vector>
...
#include "Ball.h"
using namespace std;

vector<Ball> balls;
const POS WIDTH = 1600, HEIGHT = 900;

int main {
    ...

    while (running) {
        ...
        
        if (input.type == ALLEGRO_EVENT_TIMER) { // Runs at 60FPS
            ...

            if (al_key_down(&key, ALLEGRO_KEY_SPACE)) { // Spawn the ball
                balls.push_back(Ball(WIDTH / 2, 500, 10, 10)); // Spawn the ball
                balls.back().setYSpeed(5);
            }

            for (auto iter = balls.begin(); iter != balls.end(); ) {
                Ball &b = *iter;
                ...

                if (b.getYPos() > HEIGHT) { // Collision for when you miss
                    iter = balls.erase(iter);
                }
                else {
                    ++iter;
                }
            }

            /* alternatively:

            for (Ball& b : balls) {
                b.draw();
                b.move();
            }

            balls.erase(
                std::remove_if(balls.begin(), balls.end(),
                    [=](Ball &b){
                        // Collision for when you miss
                        return b.getYPos() > HEIGHT; // If the ball falls below the defined height of the screen, wipe the ball out of memory to make room
                    }
                ),
                balls.end()
            );
            */
        }
    }
    return 0;
}

Ball.h:

#pragma once
...

class Ball {
public:
    ...
    // NO ID METHODS!

private:
    ...
    // NO ID MEMBERS!
}

Ball.cpp:

#include "Ball.h"

...

// NO ID MEMBER/METHODS!
Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
  • Thanks for the help, but it still crashes after the change. – Bennett Johnson Jul 21 '21 at 17:46
  • Sadly it's always possible to have two bugs. – user4581301 Jul 21 '21 at 17:58
  • 1
    @BennettJohnson without a [mcve], it is really hard to diagnose your problem. You are likely mismanaging the `Ball` objects elsewhere in the code, but we can't see everything you are doing. Maybe you are running into a double-delete kind of issue. We don't know. – Remy Lebeau Jul 21 '21 at 18:00
  • I do have a double delete issue, but I had a workaround in the form of the ```ballID``` variable. I know why it's happening, and that's because I didn't make the balls in the ```Ball``` vector pointers. Would the double deletion be the reason it still crashes? – Bennett Johnson Jul 21 '21 at 18:18
  • @BennettJohnson "*it's happening ... because I didn't make the balls in the `Ball` vector pointers*" - it is perfectly fine to have a vector of `Ball` objects (`vector`) rather than a vector of `Ball*` pointers (`vector`). The `vector` will destroy the `Ball` objects for you, you should not be destroying them manually at all. Even if you were using pointers instead, you probably should have been using `std::unique_ptr` to manage their deletions for you. Again, it is really hard to tell you what is right or wrong in your case without seeing a [mcve]. – Remy Lebeau Jul 21 '21 at 18:21
  • 1
    @BennettJohnson sounds like you are missing the point of a MINIMAL example. Strip down your real code to just the bare minimum needed to demonstrate the problem. Or create a whole new standalone example that just reproduces the same problem. Then edit your original question to provide that example. – Remy Lebeau Jul 21 '21 at 18:41
  • That stupid character limit prevents [mre]s that are not minimal. The real goal of a MRE is for you to not have to ask a question. The process is a powerful debugging technique. Typically as you work through the code, eliminating everything that is not part of the bug, the reduction in noise around the bug allows you to see the bug and solve the problem without needing any help. If make it to a MRE and still have a question, you either have a problem worth recording or you have overlooked something, and he only way to find out which, and get a solution, is to add the MRE to the question. – user4581301 Jul 21 '21 at 18:41
  • ``` #include #include "Ball.h" using namespace std; vector balls(0); const POS WIDTH = 1600, HEIGHT = 900; for (Ball& b : balls) { // Loops over all balls (MAKE SURE YOU DEREFERENCE!) b.draw(); b.move(); if (b.getYPos() > HEIGHT) { balls.erase(balls.begin() + b.getID()); } ``` – Bennett Johnson Jul 21 '21 at 18:52
  • There's more coming. – Bennett Johnson Jul 21 '21 at 18:52
  • @BennettJohnson please don't put the code in comments! Like I said, EDIT YOUR QUESTION instead. There is an [`Edit`](https://stackoverflow.com/posts/68473725/edit) link underneath it. But I can already tell you that the code snippet you just showed is hardly a [mcve], and it is wrong anyway as it suffers from the same problem I stated in my answer - you CANT modify a container while iterating through it with a `range-for` loop. You stated you had fixed that and are still crashing, so what does THAT new code look like now? PUT IT IN YOUR QUESTION EDIT, please. – Remy Lebeau Jul 21 '21 at 18:53
  • Done. I hope I didn't miss anything when sending in the code! – Bennett Johnson Jul 21 '21 at 19:59
  • That is hardly a [mcve] (emphasis on MINIMAL). There is alot of code in it that is *irrelevant* to your issue and should have been stripped out for demonstration purposes. And this code STILL suffers from the original issue you said you changed (replacing `range-for` with normal `for`), but clearly have not actually made that change. You are making this very hard to help you. – Remy Lebeau Jul 21 '21 at 20:10
  • In any case, your whole ID system is implemented wrong, and it is completely *unnecessary* and should be eliminated. The `Ball::ballID` member is never assigned a value, so using `balls.begin() + b.getID()` is *undefined behavior*. And `balls[Ball::getIDtoAssign()].setYSpeed(5); Ball::addToID(1);` can be simplified to `balls.back().setYSpeed(5);`, so no ID needed. And I already gave you code in my answer to eliminate the need for an ID when removing balls. – Remy Lebeau Jul 21 '21 at 20:11
  • I am not trying to insult you, I'm merely stating facts and asking you to do things you are not doing. In any case, "*you won't tell me what's wrong with my ID system*" - I DID tell you what is wrong with it: "**The `Ball::ballID` member is never assigned a value, so using `balls.begin() + b.getID()` is undefined behavior**." You are using IDs as indexes int the vector, but you are not managing those indexes correctly. And they are completely unnecessary in this situation anyway, so you should get rid of them completely. You don't need them. – Remy Lebeau Jul 21 '21 at 20:22
  • 1
    Also note that if you remove a ball, all balls after it need the their IDs updated. If you don't `balls.begin() + b.getID()` will get the wrong, and possibly nonexistent, ball. cross-referenced book-keeping is hard work. If you can avoid it, do. – user4581301 Jul 21 '21 at 20:28
  • Oh... I think I know what happened. I must've forgot to tell you the Ball ID value was a debug variable. I'll add in your erase-remove right now. – Bennett Johnson Jul 21 '21 at 20:31
  • So, you were using a debug variable to control runtime logic? Ouch! Yeah, don't do that. I have updated my answer. – Remy Lebeau Jul 21 '21 at 20:39
  • I've updated my code to use your erase-remove method, and there are no longer any traces of debug IDs. However, the code still crashes when I drop the balls out of reverse order. BTW, I'd just like to say, thanks for all your help, and thanks for trying not to lose it at me. I'm still a bit new to this kind of stuff, y'know. – Bennett Johnson Jul 21 '21 at 20:49
  • "*the code still crashes when I drop the balls out of reverse order*" - then you have to be doing something else wrong with the balls that you have not shown yet. The code shown so far removes the balls in the same order they are added to the vector. What "reverse order" are you referring to? Please edit your question to show the updated code that is crashing. – Remy Lebeau Jul 21 '21 at 21:21
  • I edited my post to include a comment telling which line the exception is thrown, and which line I think is making it crash. I also took the liberty of streamlining some things by removing all the crazy Allegro commands you didn't need to see. – Bennett Johnson Jul 21 '21 at 21:37
  • Also, it's worth noting that by "reverse order," I mean: For example, if the vector has enough room for 3 balls, they are spawned in as balls 0, 1 and 2. Unless the balls are destroyed in the order 2, 1, 0, the game will crash if it tries to destroy a ball. – Bennett Johnson Jul 21 '21 at 22:05
  • All I see is `b.draw(); // This line is what's crashing.` with no comment explaining why you think it is crashing. But the order in which the balls are removed should not matter, unless you have other code that is still referring to balls that have been destroyed (ie, dangling pointers/references), but I don't see that in this code. The `draw()` code shown can crash only if a ball's `this` pointer is bad, which is not possible in a `range-for` loop of objects, unless other code is corrupting memory. You need to actually debug your code to find the culprit, this back-and-forth is not cutting it – Remy Lebeau Jul 21 '21 at 23:26
-1

OK, so I managed to figure out why the program crashes. It was because I had the erase-remove inside the for loop which can cause all sorts of problems.