C++ Destructor is being called when I don't expect it to be

Question

I added a destructor to some code and it seems to be calling early and causing problems. I added a debug statement to see where it is being called and that made me even more confused. I understand that managing my own memory is not best practice but I wanted to try it out myself.

This is basically my GameObject class:

class GameObject
{
public:
int             xCoord = 0, yCoord = 0, prevX, prevY;
int             status = 0, weight = -1;
int             id = -1;

GameObject(CommandComponent* commands,
    PhysicsComponent* physics,
    GraphicsComponent* graphics)
    : commandComponent(commands),
    physicsComponent(physics),
    graphicsComponent(graphics)
{};

~GameObject()
{
    std::cout << "Destructor called " << id << std::endl;
    delete commandComponent;
    delete physicsComponent;
    delete graphicsComponent;
};

void update(World& world, int command, sf::Time dt)
{
    commandComponent->update(*this, world, command);
    physicsComponent->update(*this, world);
    graphicsComponent->update(*this, dt);
};

void update(World& world, int command)
{
    commandComponent->update(*this, world, command);
    physicsComponent->update(*this, world);
};

sf::Sprite draw()
{
    return *(graphicsComponent->draw());
};

void setCoords(int x, int y)
{
    prevX = xCoord;
    xCoord = x;
    prevY = yCoord;
    yCoord = y;
};

void setId(int newId)
{
    id = newId;
}


private:
    CommandComponent*           commandComponent    = NULL;
    GraphicsComponent*          graphicsComponent   = NULL;
    PhysicsComponent*           physicsComponent    = NULL;

};

This is the createPlayer Method:

    GameObject* createPlayer(sf::Texture* text)
{
    return new GameObject(new PlayerCommandComponent(), new PlayerPhysicsComponent(), new PlayerGraphicsComponent(text));
};

This is a method I invoke to add the new object to a vector based on if it is an active object or an inactive one I also add it to an array :

void World::addObject(GameObject object, int id, int type){
object.setId(id);

if (type == 0)
{
    inactiveObjects.push_back(object);
}
else if (type == 1)
{
    activeObjects.push_back(object);
}
}

Finally this is my test code that creates the Game Objects and calls the above function and where I see the destructors being called from:

void World::test()
{
// Player
std::cout << "Starting to create id 0\n";
addObject((*createPlayer(&(mTextures.get(Textures::PlayerCharacter)))), 0, 1);
activeObjects.at(0).setCoords(3, 3);
activeObjects.at(0).weight = 10;
std::cout << "Created id 0\n";

// Test Objects
std::cout << "Starting to create id 1\n";
addObject((*createPlayer(&(mTextures.get(Textures::PlayerCharacter)))), 1, 1);
activeObjects.at(1).setCoords(3, 4);
activeObjects.at(1).weight = 7;
std::cout << "Created id 1\n";

addObject((*createPlayer(&(mTextures.get(Textures::PlayerCharacter)))), 2, 1);
activeObjects.at(2).setCoords(5, 4);
activeObjects.at(2).weight = 2;

addObject((*createPlayer(&(mTextures.get(Textures::Enemy)))), 3, 1);
activeObjects.at(3).setCoords(6, 6);
activeObjects.at(3).weight = -1;

addObject((*createPlayer(&(mTextures.get(Textures::Enemy)))), 4, 1);
activeObjects.at(4).setCoords(1, 1);
activeObjects.at(4).weight = 0;

std::cout << "Done Creating Test Objects\n";

I guess my main question is how come the Destructors are being called? Im assuming its related to how I'm constructing the object in the createPlayer method, Perhaps it's going out of scope after I return it but I thought using the new keyword would prevent that from happening? I'm puzzled here.

Answer 1

Several things at play here.

GameObject* createPlayer(sf::Texture* text)

returns a dynamically allocated GameObject. This could be done better, read up on std::unique_ptr, but there is nothing strictly wrong here. I mention it mostly to point out std::unique_ptr and set up

addObject((*createPlayer(&(mTextures.get(Textures::PlayerCharacter)))), 0, 1);

because this is where thing start to go wrong. When you find code that uses new and dereferences and discards the the result, you're looking at a memory leak. You've lost the pointer to the dynamically allocated object and without the pointer it is next to impossible to find the allocation again so that you can delete it.

Storing the dereferenced object will invoke either the copy constructor or the assignment operator and at this point you need to consider The Rule of Three: If you need a to define a custom destructor, you probably need to define a custom assignment operator and a copy constructor. This is a standard example of when you need to observe the Rule of Three. What goes wrong is well-explained in the Rule of Three Link, so stop, read, and understand it before going any further. Failure to do this means the rest of this answer will be nigh-useless to you.

You cannot write good, non-trivial C++ code without a firm grip on the Rule of Three and all of its friends.

You can step around the Rule of Three here by changing

void World::addObject(GameObject object, int id, int type)

to

void World::addObject(GameObject * object, int id, int type)

and pass object by reference. This doesn't help much because

inactiveObjects.push_back(object);

is expecting an object, not a pointer.

You can change that as well, but should you? std::vector is at its absolute best when it directly contains an object. Pointers lead to pointer chasing, poor caching behaviour and ultimately suuuhhhfering. Don't store pointers unless you have a compelling reason to do so.

And if you do, manage the pointers with a std::unique_ptr beginning to end.

What I would do:

Jump straight over the Rule of Three and go to The Rule of Five.

1. Exterminate as many dynamically allocated variables as possible so that I don't need to do much work, if any, with point 2. This means no pointers for (or in) commandComponent, physicsComponent and graphicsComponent if possible.
2. Add a move constructor and move assignment operator to GameObject as well as CommandComponent, PhysicsComponent, and GraphicsComponent. Keep all resource management as close to the resource as possible. This allows you to keep higher level classes as ignorant as possible. If GraphicsComponent knows how to copy and move itself, GameObject doesn't need to know how to move it. This allows you to take advantage of The Rule of Zero, and the Rule of Zero should be what you strive for in all of your classes.
3. Use move semantics to get a GameObject, not a GameObject* from createPlayer down to the activeObjects and inactiveObjects vectors.
4. Enjoy the reduced memory management load.