-2
\$\begingroup\$

I am making this simple game in SFML and I have this vector of pointers std::vector<Brick*> bricks; which stores objects of class Brick.

Classes Brick and Ball derive publicly from class Object, because I have a polymorphic function to check colissions between 2 objects (Ball object is sf::CircleShape and Brick object is sf::RectangleShape).

unsigned Engine::collisionCheck(Game::Object *object1, Game::Object *object2)
{
    /* 1 - top collision, 2 - right, 3 - down, 4 - left */
    if (object1->getDown() == object2->getTop() && inBounds(object1, object2) == 1)
        return 1;
    if (object1->getTop() == object2->getDown() && inBounds(object1, object2) == 1)
        return 3;
    if (object1->getLeft() == object2->getRight() && inBounds(object1, object2) == 2)
        return 2;
    if (object1->getRight() == object2->getLeft() && inBounds(object1, object2) == 2)
        return 4;

    return 0;
}

I want to destroy a brick when it gets hit by a ball so I erase it from the vector.

for (unsigned i = 0; i < bricks.size(); i++) {
        if (collisionCheck(ball, bricks[i]) == 1 || collisionCheck(ball, bricks[i]) == 3) {                     
            double direction = ball->getCenter() - bricks[i]->getCenter();
            direction /= bricks[i]->getWidth() / 2;

            if (direction != 1.0)
                ballMovement.x = direction;

            ballMovement.y *= -1;   

            bricks.erase(bricks.begin() + i);                       
        }

        if (collisionCheck(ball, bricks[i]) == 2 || collisionCheck(ball, bricks[i]) == 4) {
            ballMovement.x *= -1;

            bricks.erase(bricks.begin() + i);           
        }
    }

My question is: should I destroy the object before erasing it, because when I do it my game crashes?

Also, do I need to store an array of pointers instead of objects? Because the polymorphic functions takes pointers as arguments, so I see no other way.

\$\endgroup\$
2
\$\begingroup\$

Bug

for (unsigned i = 0; i < bricks.size(); i++) {

        if (collisionCheck(ball, bricks[i]) == 1) {                     
            bricks.erase(bricks.begin() + i);                       
        }

        if (collisionCheck(ball, bricks[i]) == 2) {
            bricks.erase(bricks.begin() + i);           
        }
    }

So when you you have a size of 1 and first test is true. Then in the second test bricks[i] is out of range of the bricks vector (as you just deleted the only brick).

Ownership

This does not express ownership.

std::vector<Brick*> bricks;

Ownership defines who deletes the bricks. Your code above also leaks those pointers (because you don't define ownership and your bug above causing a crash).

Solutions:

// If you are not going to derive further from brick.
// Then you don't need polymorphic bricks and thus objects work better.
std::vector<Brick> bricks;


// If you need polymorphism and the vector owns the bricks.
std::vector<std::unique_ptr<Brick>> bricks;


// If the bricks are objects somewhere else and you are just
// keeping a reference to them in this vector.
std::vector<std::reference_wrapper<Brick>> bricks;

Questions

My question is: should I destroy the object before erasing it, because when I do it my game crashes?

It crashes because of the bug above. You should decide who owns the object they should destroy the brick. i.e. STOP using pointers.

Also, do I need to store an array of pointers instead of objects?

Depends on if you need polymorphic bricks. BUT you should still not use pointers; use a smart pointer if you do that.

Because the polymorphic functions takes pointers as arguments.

You can always change the functions to take references. Failing that you can simply pass the address of the object at the point you call the function.

Notes on Polymorphic function.

unsigned Engine::collisionCheck(Game::Object *object1, Game::Object *object2)

You really should check those pointers are not null.

unsigned Engine::collisionCheck(Game::Object& object1, Game::Object& object2)

Don't need to check references. The language guarantees they will never point to invalid objects (Assuming your code has no bugs) so no need to test for null.

PS. The * goes by the type in C++.

unsigned Engine::collisionCheck(Game::Object* object1, Game::Object* object2)

Erase Remove Idiom

You should look up the Erase Remove Idiom

In your code it would look like:

bool updateBallIfItHits(Ball* ball, Brick* brick)
{
    // Your code above
}

// Then instead of your loop.
// Use the erase/remove idiom
std::erase(std::remove(std::begin(bricks), std::end(bricks),
                       std::bind(updateBallIfItHits, ball)),
           std::end(bricks));
\$\endgroup\$
  • \$\begingroup\$ I just changed all the pointers to normal objects and it's working of course. Thanks :) I don't know why I used pointers in the first place... really no idea \$\endgroup\$ – user56806 Apr 7 '17 at 6:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy