Fix texture leak in TexturePoolHolder

[brunoabinader]

Fixes a misused operator bool() plus usage of an undefined pointer in {GL,TexturePool}Holder code.

👀 @kkaefer @jfirebaugh

Fixes #5136.

[jfirebaugh]

Why the switch from range-based for to for_each?

[brunoabinader]

You are right, I can achieve the same thing capturing a non-const reference in a range-based for.

[jfirebaugh]

The intended invariant is that if {GLObjectStore,TexturePoolHolder} is owning a resource, its objectStore is not null. It shouldn't be necessary to check both the resource and objectStore.

I think the actual bug lies in the implementations of the move constructors and operator=s. They need to transfer objectStore along with the resource.

[jfirebaugh]

Here's how I would do if implementing this pattern from scratch:

• Pull in a polyfill for C++XX unique_resource (https://github.com/okdshin/unique_resource)
• using UniqueProgram = std::unique_resource<GLuint, ProgramDeleter>;

struct ProgramDeleter {
    GLObjectStore* store = nullptr;
    void operator()(GLuint id) {
        assert(store);
        store->abandonedPrograms.push_back(id);
    }
};

UniqueProgram GLObjectStore::createProgram() {
    return std::make_unique_resource(MBGL_CHECK_ERROR(glCreateProgram()), ProgramDeleter(this));
}

[brunoabinader]

@jfirebaugh my previous implementation used std::unique_ptr but as suggested by @kkaefer in 02b3771#commitcomment-16093745 we ditched that to save on lesser redirections. I'm 👍 on revisiting this if we all agree on the implementation pros/cons.

[jfirebaugh]

The return needs to revert back to a conditional.

[brunoabinader]

Ah yes, meant continue there.

[jfirebaugh]

Why did you change o.id = 0 to id = 0? Isn't the former correct?

[brunoabinader]

We'll proceed with @jfirebaugh's suggestion from #5141 (comment) in a separate PR.

[haberbyte]

Here's how I would do if implementing this pattern from scratch:

* Pull in a polyfill for C++XX `unique_resource` (https://github.com/okdshin/unique_resource)

* `using UniqueProgram = std::unique_resource<GLuint, ProgramDeleter>;`

struct ProgramDeleter {
    GLObjectStore* store = nullptr;
    void operator()(GLuint id) {
        assert(store);
        store->abandonedPrograms.push_back(id);
    }
};

UniqueProgram GLObjectStore::createProgram() {
    return std::make_unique_resource(MBGL_CHECK_ERROR(glCreateProgram()), ProgramDeleter(this));
}

Hey @jfirebaugh, i came across this recently and was wondering if you would still do it this way.
I am wondering if the same thing could be accomplished by simply using std::unique_ptr, either wrapping each type in its own class or even by passing a deleter to the unique_ptr, e.g.

struct TextureDeleter {
    Context* context;
    TextureDeleter(Context* c) : context(c) {}
    void operator()(TextureID*) const;
};

Why is the unique_resource.hpp needed? Would love to understand this, i am probably missing something!

[kkaefer]

@haberbyte unique_resource is similar, but not identical to unique_ptr: It manages resources that are represented as integers (e.g. as returned by OpenGL API calls) instead of pointers. You could create a wrapper class for every resource type that stores the integer handle, but that means that you'd have to dereference a pointer every time you want to access the resource (i.e. you essentially store a pointer to an integer). In reality though, the performance benefit of storing the integer directly is probably pretty small.

[haberbyte]

Ah, that makes sense, thank you! Unfortunate, that it is not even part of C++17 🤷‍♂