Search code examples
c++vectoreraseremove-if

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


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:

scene on startup

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

enter image description here

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:

enter image description here

After removing the object:

enter image description here

This is correct.

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

enter image description here

Somehow changed.


Solution

  • 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;
    };