Wrong element is removed after leaving method which used std::erase succesfully

Question

I am working on a graphics engine. The engine has a std::vector of drawables. A Drawable is an object which contains a Model and a DrawableObject, which in turn holds a shader program and a bunch vertices from a 2D or 3D model. Adding new Drawables goes well, the problem occurs when I try to remove a Drawable. The last Drawable will always be removed and the second to last will have its values changed.

Code

Drawable.h

class Drawable
{
public:
    Drawable& operator=(const Drawable& other)
    { 
        Drawable tmp(other);
        std::swap(model, other.model);
        std::swap(drawableObject, other.drawableObject);
        return *this;
    }

    Drawable(domain::Model& model, DrawableObject& drawableObject) :
        model(model),
        drawableObject(drawableObject)
    {}

    domain::Model& model;
    DrawableObject& drawableObject;
};

game.cpp

void Game::init_game()
{
    human = domain::Model(glm::vec3(0, 0, -3));
    moveables.push_back(&human);
    room = domain::Model(glm::vec3(0, 0, -10));
    props.push_back(&room);
    cube = domain::Model(glm::vec3(0, 0, 0));
    props.push_back(&cube);
}

void Game::init_graphics_engine()
{
    // ... load graphics models

    // add drawables
    graphicsEngine->add(cube, Drawable::CUBE);
    graphicsEngine->add(human, Drawable::HUMAN);
    graphicsEngine->add(room, Drawable::ROOM);
    graphicsEngine->add(topDownScene->cursor, Drawable::MARKER);
}

graphics_engine/engine.cpp

void Engine::add(domain::Model& model, unsigned int object)
{
    auto drawableObject = drawableObjects[object];
    // make sure not to add a model that already is represented
    auto it = std::find_if(drawables.begin(), drawables.end(), [&model](Drawable& drawable) {return &drawable.model == &model;});
    if(drawableObject && it == drawables.end())
        drawables.push_back(Drawable(model, *drawableObject));
}

void Engine::remove(domain::Model& model)
{
    auto predicate = [&model](Drawable& drawable) 
    {
        return &drawable.model == &model;
    };
    drawables.erase(std::remove_if(drawables.begin(), drawables.end(), predicate), drawables.end());
}

Scenes

This is what the scene looks like when I start the application:

This is what the scene looks like after attempting to erase the small 'human' cube in the middle:

The code removes the last Drawable, which is the white marker, instead of the 'human' cube, and changed the z position of the room. This almost always happens, it removes the last element and changed the z of the second to last. It only works if I add the 'human' cube last in the init method.

Breakpoints

Before removing the object:

After removing the object:

This is correct.

Leaving the remove method and taking a look in the render loop:

Somehow changed.

Wait, your copy assignment operator modifies the right-hand side? And what's up with `tmp`? — NPE, Aug 13 '17 at 14:40
@NPE [copy-and-swap idiom](https://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom). — LogicStuff, Aug 13 '17 at 14:49
@LogicStuff: That's what I am guessing it's *meant* to be, but it isn't if you read the code closely (unless I am failing to see something, which is of course also a possibility). — NPE, Aug 13 '17 at 15:01
It's also confusing using reference member variables. OP is removing using the address of the model, which cannot change, as it is a reference. Remove will invoke the assignment operator, but the address of the model isn't going to change, as the swap is swapping the contents of the models. — Dave S, Aug 13 '17 at 16:18
You are using the copy and swap wrong I think, std::swap(model, other.model); — Giel, Aug 13 '17 at 16:30

score 0 · Accepted Answer · answered Aug 13 '17 at 18:55

I changed the class members to pointers. Now it works. The comments were right, I didn't do anything with the tmp variable.

class Drawable
{
public:
    Drawable& operator=(const Drawable& other)
    { 
        this->model = other.model;
        this->drawableObject = other.drawableObject;
        return *this;
    }

    Drawable(domain::Model* model, std::shared_ptr<DrawableObject> drawableObject) :
        model(model),
        drawableObject(drawableObject)
    {}

    domain::Model* model;
    std::shared_ptr<DrawableObject> drawableObject;
};

Wrong element is removed after leaving method which used std::erase succesfully

Code

Scenes

Breakpoints

1 Answers1