I'm quite far into the development of a game using SDL, OpenGL and C++ and am looking for ways to optimize the way the game switches between GLSL shaders for lots of different objects of different types. This is much more of a C++ question than an OpenGL question. However, I still want to provide as much context as I can, as I feel some justification is needed as to why the proposed Shader class I need, needs to be created / deleted the way that it is.

The first four sections are my justifications, journey & attempts leading up to this point, however my question can likely be answered by just the final section alone and I've intentionally written it as a bit of a tldr.

The necessity for a Shader class:

I've seen many implementations online of OpenGL shaders being created, compiled and deleted all in the same function when game objects are created during gameplay. This has proven to be inefficient and far too slow in particular sections of my game. Thus, I've required a system that creates and compiles shaders during a load-time and then intermittently uses/swaps between them during game-time before being deleted later.

This has lead to the creation of a class(Shader) that manages OpenGL shaders. Each instance of the class should manage one unique OpenGL shader each and contains some complex behavior around the shader type, where it's loaded in from, where it's used, the uniform variables it takes, etc.

With this said, the most important role of this class is to store the GLuint variable id that is returned from glCreateShader(), and manage all OpenGL calls that relate to the OpenGL shader with this id. I understand that this is effectively futile given the global nature of OpenGL(as anywhere in the program could technically call glDeleteShader() with the matching id and break the class), however for the purposes of intentionally encapsulating all OpenGL calls to very specific areas throughout the entire codebase this system will drastically reduce code-complexity.

Where the problems start...

The most "automatic" way to manage this GLuint id, would be to invoke glCreateShader() on the object's construction and glDeleteShader() on the object's destruction. This guarantees(within OpenGL limits) that the OpenGL shader will exist for the entire lifetime of the C++ Shader object and eliminates the need to call some void createShader() and deleteShader() functions.

This is all well and good, however problems soon arise when considering what happens if this object is copied. What if a copy of this object is destructed? That means that glDeleteShader() will be called and effectively break all copies of the shader object.

What about simple mistakes like accidentally invoking std::vector::push_back() in a vector of Shaders? Various std::vector methods can invoke the constructor / copy constructor / destructor of their type, which can result in the same problem as above.

Okay then... how about we do create some void createShader() and deleteShader() methods even if it's messy? Unfortunately this just defers the above problem, as once again any calls that modify the OpenGL shader will desynchronize / outright break all copies of a shader class with the same id. I've limited the OpenGL calls to glCreateShader() and glDeleteShader() in this example to keep things simple, however I should note that there are many other OpenGL calls in the class that would make creating various instance/static variables that keep track of instance copies far too complicated to justify doing it this way.

The last point I want to make before jumping into the class design below is that for a project as large as a raw C++, OpenGL and SDL Game, I'd prefer if any potential OpenGL mistakes I make generate compiler errors versus graphical issues that are harder to track down. This can be reflected in the class design below.

The first version of the Shader class:

It is for the above reasons that I have elected to:

• Make the constructor private.
• Provide a public static create function that returns a pointer to a new Shader object in place of a constructor.
• Make the copy constructor private.
• Make the operator= private (Although this might not be necessary).
• Make the destructor private.
• Put calls to glCreateShader() in the constructor and glDeleteShader() in the destructor, to have OpenGL shaders exist for the lifetime of this object.
• As the create function invokes the new keyword(and returns the pointer to it), the place with the outside call to Shader::create() must then invoke delete manually (more on this in a second).

To my understanding, the first two bullet points utilize a factory pattern and will generate a compiler error should a non-pointer type of the class be attempted to be created. The third, fourth and fifth bullet points then prevent the object from being copied. The seventh bullet point then ensures that the OpenGL Shader will exist for the same lifetime of the C++ Shader object.

Smart Pointers and the main problem:

The only thing I'm not a huge fan of in the above, is the new/delete calls. They also make the glDeleteShader() calls in the destructor of the object feel inappropriate given the encapsulation that the class is trying to achieve. Given this, I opted to:

• change the create function to return a std::unique_ptr of the Shader type instead of a Shader pointer.

The create function then looked like this:

std::unique_ptr<Shader> Shader::create() {
    return std::make_unique<Shader>();
}

But then a new problem arose... std::make_unique unfortunately requires that the constructor is public, which interferes with the necessities described in the previous section. Fortunately, I found a solution by changing it to:

std::unique_ptr<Shader> Shader::create() {
    return std::unique_ptr<Shader>(new Shader());
}

But... now std::unique_ptr requires that the destructor is public! This is... better but unfortunately, this means that the destructor can be manually called outside of the class, which in turn means the glDeleteShader() function can be called from outside the class.

Shader* p = Shader::create();
p->~Shader(); // Even though it would be hard to do this intentionally, I don't want to be able to do this.
delete p;

The final class:

For the sake of simplicity, I have removed the majority of instance variables, function/constructor arguments & other attributes but here's what the final proposed class (mostly)looks like:

class GLSLShader {

public:
    ~GLSLShader() { // OpenGL delete calls for id }; // want to make this private.

    static std::unique_ptr<GLSLShader> create() { return std::unique_ptr<GLSLShader>(new GLSLShader()); };

private:
    GLSLShader() { // OpenGL create calls for id };

    GLSLShader(const GLSLShader& glslShader);
    GLSLShader& operator=(const GLSLShader&);

    GLuint id;

};

I'm happy with everything in this class, aside from the fact that the destructor is public. I've put this design to the test and the performance increase is very noticeable. Even though I can't imagine I'd ever accidentally manually call the destructor on a Shader object, I don't like that it is publicly exposed. I also feel that I might accidentally miss something, like the std::vector::push_back consideration in the second section.

I've found two potential solutions to this problem. I'd like some advice on these or other solutions.

1. Make std::unique_ptr or std::make_unique a friend of the Shader class. I've been reading threads such as this one, however this is to make the constructor accessible, rather than the destructor. I also don't quite understand the downsides / extra considerations needed with making std::unique_ptr or std::make_unique a friend (The top answer to that thread + comments)?

2. Not use smart pointers at all. Is there perhaps a way to have my static create() function return a raw pointer(using the new keyword), that is automatically deleted inside the class / when the Shader goes out of scope and the destructor is called?

Thank you very much for your time.

This is a context challenge.

You are solving the wrong problem.

GLuint id, would be to invoke glCreateShader() on the object's construction and glDeleteShader()

Fix the problem here.

The Rule of Zero is that you make your resource wrappers manage lifetimes, and you don't do it in business logic types. We can write a wrapper around a GLuint that knows how to clean itself up and is move-only, preventing double destruction, by hijacking std::unique_ptr to store an integer instead of a pointer.

Here we go:

// "pointers" in unique ptrs must be comparable to nullptr.
// So, let us make an integer qualify:
template<class Int>
struct nullable{
  Int val=0;
  nullable()=default;
  nullable(Int v):val(v){}
  friend bool operator==(std::nullptr_t, nullable const& self){return !static_cast<bool>(self);}
  friend bool operator!=(std::nullptr_t, nullable const& self){return static_cast<bool>(self);}
  friend bool operator==(nullable const& self, std::nullptr_t){return !static_cast<bool>(self);}
  friend bool operator!=(nullable const& self, std::nullptr_t){return static_cast<bool>(self);}
  operator Int()const{return val;}
};

// This both statelessly stores the deleter, and
// tells the unique ptr to use a nullable<Int> instead of an Int*:
template<class Int, void(*deleter)(Int)>
struct IntDeleter{
  using pointer=nullable<Int>;
  void operator()(pointer p)const{
    deleter(p);
  }
};

// Unique ptr's core functionality is cleanup on destruction
// You can change what it uses for a pointer. 
template<class Int, void(*deleter)(Int)>
using IntResource=std::unique_ptr<Int, IntDeleter<Int,deleter>>;

// Here we statelessly remember how to destroy this particular
// kind of GLuint, and make it an RAII type with move support:
using GLShaderResource=IntResource<GLuint,glDeleteShader>;

now that type knows it is a shader and cleans itself up it non-null.

GLShaderResource id(glCreateShader());
SomeGLFunction(id.get());

apologies for any typos.

Stuff that in your class, and copy ctors are blocked, move ctors do the right thing, dtors clean up automatically, etc.

struct GLSLShader {
  // public!
  ~GLSLShader() = default;
  GLSLShader() { // OpenGL create calls for id };
private: // does this really need to be private?
  GLShaderResource id;
};

so much simpler.

std::vector<GLSLShader> v;

and that just works. Our GLShaderResource is semi-regular (move only regular type, no sort support), and vector is happy with those. Rule of 0 means that GLSLShader, which owns it, is also semi-regular and supports RAII -- resource allocation is initialization -- which in turn means it cleans up after itself properly when stored in std containers.

A type being "Regular" means it "behaves like an int" -- like the prototypical value type. C++'s standard library, and much of C++, likes it when you are using regular or semi-regular types.

Note that this is basically zero overhead; sizeof(GLShaderResource) is the same as GLuint and nothing goes on the heap. We have a pile of compile-time type machinery wrapping a simple 32 bit integers; that compile-time type machinery generates code, but doesn't make the data more complex than 32 bits.

Live example.

The overhead includes:

1. Some calling conventions make passing a struct wrapping only an int be passed differently than an int.

2. On destruction, we check every one of these to see if it is 0 to decide if we want to call glDeleteShader; compilers can sometimes prove that something is guaranteed zero and skip that check. But it won't tell you if it did manage to pull that off. (OTOH, humans are notoriously bad at proving that they kept track of all resources, so a few runtime checks aren't the worst thing).

3. If you are doing a completely unoptimized build, there are going to be a few extra instructions when you call a OpenGL function. But after any non-zero level of inlineing by the compiler they will disappear.

4. The type isn't "trivial" (a term in the C++ standard) in a few ways (copyable, destroyable, constructible), which makes doing things like memset illegal under the C++ standard; you can't treat it like raw memory in a few low level ways.

---

A problem!

Many OpenGL implementations have pointers for glDeleteShader/glCreateShader etc, and the above relies on them being actual functions not pointers or macros or whatever.

There are two easy workarounds. The first is to add a & to the deleter arguments above (two spots). This has the problem that it only works when they are actually pointers now, and not when they are actual functions.

Making code that works in both cases is a bit tricky, but I think almost every GL implementation uses function pointers, so you should be good unless you want to make a "library quality" implementation. In that case, you can write some helper types that create constexpr function pointers that call the function pointer (or not) by name.

---

Finally, apparently some destructors require extra parameters. Here is a sketch.

using GLuint=std::uint32_t;

GLuint glCreateShaderImpl() { return 7; }
auto glCreateShader = glCreateShaderImpl;
void glDeleteShaderImpl(GLuint x) { std::cout << x << " deleted\n"; }
auto glDeleteShader = glDeleteShaderImpl;

std::pair<GLuint, GLuint> glCreateTextureWrapper() { return {7,1024}; }

void glDeleteTextureImpl(GLuint x, GLuint size) { std::cout << x << " deleted size [" << size << "]\n"; }
auto glDeleteTexture = glDeleteTextureImpl;

template<class Int>
struct nullable{
  Int val=0;
  nullable()=default;
  nullable(Int v):val(v){}
  nullable(std::nullptr_t){}
  friend bool operator==(std::nullptr_t, nullable const& self){return !static_cast<bool>(self);}
  friend bool operator!=(std::nullptr_t, nullable const& self){return static_cast<bool>(self);}
  friend bool operator==(nullable const& self, std::nullptr_t){return !static_cast<bool>(self);}
  friend bool operator!=(nullable const& self, std::nullptr_t){return static_cast<bool>(self);}
  operator Int()const{return val;}
};

template<class Int, auto& deleter>
struct IntDeleter;

template<class Int, class...Args, void(*&deleter)(Int, Args...)>
struct IntDeleter<Int, deleter>:
  std::tuple<std::decay_t<Args>...>
{
  using base = std::tuple<std::decay_t<Args>...>;
  using base::base;
  using pointer=nullable<Int>;
  void operator()(pointer p)const{
    std::apply([&p](std::decay_t<Args> const&...args)->void{
        deleter(p, args...);
    }, static_cast<base const&>(*this));
  }
};

template<class Int, void(*&deleter)(Int)>
using IntResource=std::unique_ptr<Int, IntDeleter<Int,deleter>>;

using GLShaderResource=IntResource<GLuint,glDeleteShader>;

using GLTextureResource=std::unique_ptr<GLuint,IntDeleter<GLuint, glDeleteTexture>>;

int main() {
    auto res = GLShaderResource(glCreateShader());
    std::cout << res.get() << "\n";
    auto tex = std::make_from_tuple<GLTextureResource>(glCreateTextureWrapper());
    std::cout << tex.get() << "\n";
}