OpenGL: Freeing allocated memory

Question

Here's a snippet of my code. I have a Button structure made up by 2 Figure structures, which contains vertices, colors and the relative VBOs and VAO.

I have a function to allocate the memory and one to free it, but I'm not sure if it's missing something.

#include "GL/glew.h"
#include "GL/freeglut.h"
#define GLM_ENABLE_EXPERIMENTAL
#include "glm/glm.hpp"
#include "glm/gtc/matrix_transform.hpp"
#include "glm/gtx/transform.hpp"
#include "glm/gtc/type_ptr.hpp"
// Structures
typedef struct {
 std::vector<glm::vec2> vertices;
 std::vector<glm::vec4> colors;
 GLuint VAO, VBO_V, VBO_C;
 int drawMode;
} Figure;
typedef struct {
 glm::vec2 pos;
 glm::vec2 size;
 glm::vec4 color;
 glm::vec4 colorHover;
 Figure bg;
 Figure border;
 void (*onClick)(void);
} Button;
// Creation
Button* createButton(float posx, float posy, float width, float height,
 glm::vec4 color, glm::vec4 colorHover, glm::vec4 colorBorder, void (*onClick)(void))
{
 Button* button = new Button;
 button->pos.x = posx;
 button->pos.y = posy;
 button->size.x = width;
 button->size.y = height;
 button->color = color;
 button->colorHover = colorHover;
 button->onClick = onClick;
 // Background Figure
 button->bg.drawMode = GL_TRIANGLES;
 button->bg.vertices.push_back({ -1.0f, 1.0f });
 button->bg.vertices.push_back({ -1.0f, -1.0f });
 button->bg.vertices.push_back({ 1.0f, 1.0f });
 button->bg.vertices.push_back({ 1.0f, 1.0f });
 button->bg.vertices.push_back({ 1.0f, -1.0f });
 button->bg.vertices.push_back({ -1.0f, -1.0f });
 for (int i = 0; i < button->bg.vertices.size(); i++)
 {
 button->bg.colors.push_back(button->color);
 }
 // Generate VAO 
 glGenVertexArrays(1, &button->bg.VAO);
 glBindVertexArray(button->bg.VAO);
 // VBO for vertices
 glGenBuffers(1, &button->bg.VBO_V);
 glBindBuffer(GL_ARRAY_BUFFER, button->bg.VBO_V);
 glBufferData(GL_ARRAY_BUFFER, button->bg.vertices.size() * sizeof(glm::vec2), button->bg.vertices.data(), GL_STATIC_DRAW);
 glVertexAttribPointer(0, 2, GL_FLOAT, GL_FALSE, 0, (void*)0);
 glEnableVertexAttribArray(0);
 // VBO for colors
 glGenBuffers(1, &button->bg.VBO_C);
 glBindBuffer(GL_ARRAY_BUFFER, button->bg.VBO_C);
 // Check: cambiare a GL_DYNAMIC_DRAW?
 glBufferData(GL_ARRAY_BUFFER, button->bg.colors.size() * sizeof(glm::vec4), button->bg.colors.data(), GL_DYNAMIC_DRAW);
 glVertexAttribPointer(1, 4, GL_FLOAT, GL_FALSE, 0, (void*)0);
 glEnableVertexAttribArray(1);
 // Border Figure
 button->border.drawMode = GL_LINE_LOOP;
 button->border.vertices.push_back({ -1.0f, 1.0f });
 button->border.vertices.push_back({ -1.0f, -1.0f });
 button->border.vertices.push_back({ 1.0f, -1.0f });
 button->border.vertices.push_back({ 1.0f, 1.0f });
 for (int i = 0; i < button->border.vertices.size(); i++)
 {
 button->border.colors.push_back({ 1.0f, 1.0f, 1.0f, 1.0f });
 }
 glGenVertexArrays(1, &button->border.VAO);
 glBindVertexArray(button->border.VAO);
 glGenBuffers(1, &button->border.VBO_V);
 glBindBuffer(GL_ARRAY_BUFFER, button->border.VBO_V);
 glBufferData(GL_ARRAY_BUFFER, button->border.vertices.size() * sizeof(glm::vec2), button->border.vertices.data(), GL_STATIC_DRAW);
 glVertexAttribPointer(0, 2, GL_FLOAT, GL_FALSE, 0, (void*)0);
 glGenBuffers(1, &button->border.VBO_C);
 glBindBuffer(GL_ARRAY_BUFFER, button->border.VBO_C);
 glBufferData(GL_ARRAY_BUFFER, button->border.colors.size() * sizeof(glm::vec4), button->border.colors.data(), GL_STATIC_DRAW);
 glVertexAttribPointer(1, 4, GL_FLOAT, GL_FALSE, 0, (void*)0);
 return button;
}
void destroyButton(Button* button)
{
 glDeleteBuffers(1, &button->bg.VBO_V);
 glDeleteBuffers(1, &button->bg.VBO_C);
 glBindVertexArray(0); // Unbind any currently bound VAO
 glDeleteVertexArrays(1, &button->bg.VAO);
 button->bg.vertices.clear();
 button->bg.colors.clear();
 glDeleteBuffers(1, &button->border.VBO_V);
 glDeleteBuffers(1, &button->border.VBO_C);
 glBindVertexArray(0); // Unbind any currently bound VAO
 glDeleteVertexArrays(1, &button->border.VAO);
 button->border.vertices.clear();
 button->border.colors.clear();
 delete button;
}
int main(int argc, char** argv)
{
 // glut/OpenGL init code
 // [...]
 Button* myButton = createButton(100.0f, 100.0f, 150.0f, 40.0f,
 { 0.0f, 0.0f, 0.7f, 1.0f }, { 0.0f, 0.0f, 1.0f, 1.0f }, { 1.0f, 1.0f, 1.0f, 1.0f }, nullptr);
 destroyButton(myButton);
 
 return 0;
}

Is that the correct approach to free the allocated resources, or am I missing something? Is it ok to delete VAO after the VBOs?

---

Note

These snippets are part of a quite big project...

If somebody needs a MRE, please add a comment: I can add the initialization code and whatever is needed as soon as I get to use my device and test the code

• Blindman67: Buffers marked as GL_STATIC_DRAW (indicating you will not be changing them) are copies of the RAM vectors you fill them from using glBufferData The associated vectors in Figure .vertices .colors should only have a life inside the createButton method. They are just unused copies of the GL buffers.
• mikyll98: @Blindman67 Thank you so much for the advise :) let me know if I understood this correctly: as a general approach, in the case of these vectors corresponding to "static VBOs", it would be better to limit their life to just the createButton() function, and therefore not save their value in the Figure structure at all, right?
• Blindman67: Yes no need to hold copies of GL buffers if you never use them (after glBufferData call)
• Davislor: It would be possible—as an absurd kludge—to define a custom deleter for std::unique_ptr<Button, ButtonDeleter>. If for some reason you’re not allowed to just add a destructor to the class.

Answer 1

Ugh, manual memory management - you want to avoid that.

You should add a destructor to your Button class to release the memory it owns, rather than requiring the caller to do that.

And createButton() really looks like it should be one of the constructors.

• mikyll98: Thank you for your precious answer :) In this project, for some too long and weird reasons to be written down, even if the project is in C++, we never used any kind of class, only structures (kind of C-like). I understand it's not the best practice, and we'll probably be better off refactoring the whole project. However, in case we want to manually manage the memory, do you know what else we would need to do?
• Toby Speight: My first reaction was "this looks so much like C, why is it written in C++?" You could replace new with malloc() and delete with free(), and then you'd have quite reasonable C code. But it's terrible as C++.
• mikyll98: So, supposing we have Button* myButton = (Button*) malloc(sizeof(Button)); for the creation, a free(myButton) (after the VBOs and VAOs delete) would be enough, right?
• Toby Speight: Yes, though I would write that Button *myButton = malloc(sizeof *myButton) as a good practice. Just make sure that every allocation is paired with exactly one deallocation (including pairing createButton() with destroyButton() etc.) and you won't go far wrong. And then check your execution using Valgrind.
• Rish: C++ does not have implicit conversions from void*. Also, it might work for Button (which I presume is a POD), but malloc does not construct the object; you'd need placement new or std::construct_at. Similarly, free does not call the destructor, which delete does.

Answer 2

Maybe the buffer deletion is technically correct.

But this design is headed in pretty much the opposite direction of how people usually try to use OpenGL (or other rendering APIs) when they need to render a bunch of trivial geometry. So far you haven't shown how you will render the buttons, but based on the code so far I predict that goes something like this: for one button, bind the VAO and any relevant textures and uniforms that you may have, draw the button, separately draw the border (or maybe you do this first but that's not the point), then move on to the next button. At least two draw calls per button (maybe more if you add more stuff to the button?).

The "usual" (as far as I know) way to draw GUI elements with OpenGL (or other typical rendering APIs) is to have each GUI element write its relevant data into a buffer, and then render everything at once as if all visible GUI elements together form one big (dynamic) mesh. OK, maybe not all at once, but the goal is to batch as much stuff together as can go together. Not everything can always go together, you may need several batches so you can draw things with a different render pipeline state, eg different textures or uniforms or blend modes, etc. So there are good reasons to separate some parts into different batches that are drawn separately, but "it's a different GUI element" is not sufficient reason.

That's not to say that that is the only (or best) way, certainly there are alternatives, and you don't always have to do the best thing anyway. But giving each button its own (multiple) VBOs is IMO the wrong direction to go in.