Lifetime issue instance of class || How to build my copy constructor?

Question

My problem lies in the creation of instances of a class in my project. This is how I create my instances and try to store them:

    for (const auto & entry : fs::directory_iterator(path))
    {
        auto x = path + "/" + std::filesystem::path(entry).filename().string();
        auto y = x.c_str();
        Texture text(y);
        inGameTextures.push_back(text);
    }

The goal is to load all images as textures from a specific directory and store all of them in a list.

I found out that this is not working, the first loaded images always being overwritten by the later ones. That and the fact that the textures are not showing on screen at all must have to do with the lifetime of the instances, as I am creating all instances from a function. I found out that I might need a copy constructor for my class, so I tried to add a basic copy and move constructor to my header:

    Texture(const char* path);
    Texture(const Texture& other) :
    texture(other.texture){};
    Texture& operator=(const Texture & that)
    {texture = that.texture;};
    Texture(Texture &&) noexcept = default;
    Texture& operator=(Texture &&) noexcept = default;
    ~Texture();

Of course this is not working either, but I can't really get behind the setup of copy cunstructors and what exactly I need it to copy for it to work correctly. I was hoping someone could give me a hint what's wrong with these copy/move constructors and how to build mine correctly, and for any hints what else is wrong with my code, since none of it is really working.

For you to explore what I tried to do in my 'Texture' class, here is my header:

class Texture {
    private:
        unsigned int texture = 0;
    public:
        Texture(const char* path);
        Texture(const Texture& other) :
        texture(other.texture){};
        Texture& operator=(const Texture & that)
        {texture = that.texture;};
        Texture(Texture &&) noexcept = default;
        Texture& operator=(Texture &&) noexcept = default;
        ~Texture();

        int LoadImage(const char* path);
        void CustomizeTexture();

        struct data
        {
            std::string name;
            std::string documentExtension;
            glm::vec3 position;
            int ID;
        };

        static data& GetTextureData();
        static void SetData(const char* path);
        void SetID(int ID);
        static float RandomPositionValue();

        //int GetWidth() const { return width; };
        //int GetHeight() const { return height; };
    };

And here is what my .cpp essentially does:

Texture::data textureData;

Texture::Texture(const char* path) :
texture(0)
{
    textureData.ID = LoadImage(path);
    SetData(path);
}

Sorry for newbie mistakes, hope someone can help me anyways :) Thanks in advance! :)

Answer 1

As commenters have already pointed out, the way your manage resources for Texture is broken. You are providing a copy constructor and destructor manually, but the move operations remain defaulted, which is pointless, since the defaulted move operations can bypass your usual manual management.

Since the texture ID returned by glGenTextures isn't copyable anyway (there exists no glCopyTexture), modeling unique ownership makes more sense:

// note: prefer GLuint over unsigned int.
//       GLuint is a 32-bit unsigned integer so the closest thing would be
//       std::uint32_t, wheras unsigned int can be 16 bits or any greater width.
GLuint texture;

Texture(Texture &&other) noexcept
  : texture(std::exchange(other.texture, 0))
{}

Texture& operator=(Texture &&other) noexcept {
    reset();
    texture = std::exchange(other.texture, 0);
    return *this;
}

~Texture() {
    reset();
}

void reset() noexcept {
    // I'm 50% sure that you cannot delete the zero object, so let's check
    if (texture != 0) {
        glDeleteTextures(1, &texture);
        texture = 0;
    }
}

// optional, these are implicitly deleted
Texture(Texture const&) = delete;
Texture& operator=(Texture const&) = delete;

Unfortunately, we cannot delegate the work to std::unique_ptr because GLuint is not a NullablePointer.

Other than the resource management issues, it's hard to say what could be wrong. Maybe it's this, or maybe it isn't. You haven't provided a minimal reproducible example.

Note on copy-and-swap

Instead of a move assignment operator, you could also write a by-value copy assignment operator:

friend void swap(Texture& a, Texture& b) noexcept {
    std::swap(a.texture, b.texture);
}

Texture& operator=(Texture other) noexcept {
    swap(*this, other);
    return *this;
}

See also: What is the copy-and-swap idiom?