Reviewing my shader class for efficient use

Question

Im just trying to setup my shader function in a class in a way that would make me work with it easier and in a more understandable way.

In Shader.h:

#pragma once
#include"Includes.h"
#include<unordered_map>

namespace GAME
{
    enum class Shader_Type
    {
        NONE = -1,
        VERTEX = 0,
        FRAGMENT = 1,
        GEOMETRY = 2
    };

    class Shader
    {
    private:
        std::map<Shader_Type, std::string>m_Data;
        Shader_Type type;
        std::unordered_map<std::string, int> m_UniformLocationCache;
    public:
        unsigned int Program_id;
    private:
        unsigned int Compiler(unsigned int type, const std::string& source);
        void Check_errors(unsigned int* id, unsigned int type);
        void Create_shader();

    public:

        Shader();
        ~Shader();

        Shader(const Shader& e);
        Shader& operator=(const Shader& e);
        Shader(Shader&& e)noexcept;
        Shader& operator=(Shader&& e)noexcept;
        Shader* operator-> () { return this; }

        operator Shader* () { return this; }
        Shader* Clone() { return new Shader(*this); }


        void Bind() const;
        void Unbind() const;

        void Reset();
        void Init(std::string vertexShader = "", std::string fragmentshader = "", std::string Geometryshader = "");

        unsigned int GetUniformLocation(const std::string& name);



        //uniforms

        void Setuniform1f(const std::string& name, float v1);
        void Setuniform2f(const std::string& name, float v1, float v2);
        void Setuniform3f(const std::string& name, float v1, float v2, float v3);
        void Setuniform4f(const std::string& name, float v1, float v2, float v3, float v4);

        void Setuniform1i(const std::string& name, int v1);
        void Setuniform2i(const std::string& name, int v1, int v2);
        void Setuniform3i(const std::string& name, int v1, int v2, int v3);
        void Setuniform4i(const std::string& name, int v1, int v2, int v3, int v4);

        void Setuniform1d(const std::string& name, double v1);
        void Setuniform2d(const std::string& name, double v1, double v2);
        void Setuniform3d(const std::string& name, double v1, double v2, double v3);
        void Setuniform4d(const std::string& name, double v1, double v2, double v3, double v4);



        void Setuniform1fv(const std::string& name, int count, glm::fvec1 v);
        void Setuniform2fv(const std::string& name, int count, glm::fvec2 v);
        void Setuniform3fv(const std::string& name, int count, glm::fvec3 v);
        void Setuniform4fv(const std::string& name, int count, glm::fvec4 v);

        void Setuniform1iv(const std::string& name, int count, glm::ivec1 v);
        void Setuniform2iv(const std::string& name, int count, glm::ivec2 v);
        void Setuniform3iv(const std::string& name, int count, glm::ivec3 v);
        void Setuniform4iv(const std::string& name, int count, glm::ivec4 v);

        void Setuniform1dv(const std::string& name, int count, glm::dvec1 v);
        void Setuniform2dv(const std::string& name, int count, glm::dvec2 v);
        void Setuniform3dv(const std::string& name, int count, glm::dvec3 v);
        void Setuniform4dv(const std::string& name, int count, glm::dvec4 v);



        void SetUniformMat2fv(const std::string& name, int count, GLboolean transpose, glm::mat2 value);
        void SetUniformMat3fv(const std::string& name, int count, GLboolean transpose, glm::mat3 value);
        void SetUniformMat4fv(const std::string& name, int count, GLboolean transpose, glm::mat4 value);



    };


}

#include"Includes.h" is the header where I kept all includes of gl and other required classes

In Shader.cpp:

#include "Shader.h"
namespace GAME
{
    Shader::Shader()
        :Program_id(0), type(Shader_Type::NONE)
    {
    }
    Shader::~Shader()
    {
    }

    Shader::Shader(const Shader& e)
    {
        this->m_Data = e.m_Data;
        this->m_UniformLocationCache = e.m_UniformLocationCache;
        this->Program_id = e.Program_id;
        this->type = Shader_Type(e.type);
    }

    Shader& Shader::operator=(const Shader& e)
    {
        this->m_Data = e.m_Data;
        this->m_UniformLocationCache = e.m_UniformLocationCache;
        this->Program_id = e.Program_id;
        this->type = Shader_Type(e.type);
        return *this;
    }

    Shader::Shader(Shader&& e) noexcept
    {
        this->m_Data = std::move(e.m_Data);
        this->m_UniformLocationCache = std::move(e.m_UniformLocationCache);
        this->Program_id =std::move( e.Program_id);
        this->type = std::move(Shader_Type(e.type));
    }

    Shader& Shader::operator=(Shader&& e) noexcept
    {
        this->m_Data = std::move(e.m_Data);
        this->m_UniformLocationCache = std::move(e.m_UniformLocationCache);
        this->Program_id = std::move(e.Program_id);
        this->type = std::move(Shader_Type(e.type));
        return *this;
    }

    void Shader::Bind() const
    {
        glUseProgram(Program_id);
    }
    void Shader::Unbind() const
    {
        glUseProgram(0);
    }


    void Shader::Reset()
    {
        this->type = Shader_Type::NONE;
        this->Program_id = 0;
        this->m_UniformLocationCache.clear();
        this->m_Data.clear();
    }
    void Shader::Init(std::string vertexShader, std::string fragmentshader, std::string Geometryshader)
    {
        m_Data[Shader_Type::VERTEX] = vertexShader;
        m_Data[Shader_Type::FRAGMENT] = fragmentshader;
        m_Data[Shader_Type::GEOMETRY] = Geometryshader;

        Create_shader();
        Unbind();
    }

    unsigned int Shader::GetUniformLocation(const std::string& name)
    {
        if (m_UniformLocationCache.find(name) != m_UniformLocationCache.end())
        {
            return m_UniformLocationCache[name];
        }
        int location = glGetUniformLocation(Program_id, name.c_str());
        if (location == -1)
        {
            std::cout << "Warning! Uniform " << name << " is unused or not found or does no exist!" << std::endl;
        }
        else
            m_UniformLocationCache[name] = location;
        return location;
    }

    void Shader::Create_shader()
    {
        Program_id = glCreateProgram();

        if (m_Data[Shader_Type::VERTEX] != "")
        {
            unsigned int vs = Compiler(GL_VERTEX_SHADER, m_Data[Shader_Type::VERTEX]);
            glAttachShader(Program_id, vs);
            glDeleteShader(vs);
        }
        if (m_Data[Shader_Type::FRAGMENT] != "")
        {
            unsigned int fs = Compiler(GL_FRAGMENT_SHADER, m_Data[Shader_Type::FRAGMENT]);
            glAttachShader(Program_id, fs);
            glDeleteShader(fs);
        }
        if (m_Data[Shader_Type::GEOMETRY] != "")
        {
            unsigned int gs = Compiler(GL_GEOMETRY_SHADER, m_Data[Shader_Type::GEOMETRY]);
            glAttachShader(Program_id, gs);
            glDeleteShader(gs);
        }

        glLinkProgram(Program_id);
        glValidateProgram(Program_id);

    }

    unsigned int Shader::Compiler(unsigned int type, const std::string& source)
    {
        unsigned int id = glCreateShader(type);
        const char* src = source.c_str();
        glShaderSource(id, 1, &src, nullptr);
        glCompileShader(id);

        Check_errors(&id, type);

        return id;
    }

    void Shader::Check_errors(unsigned int* id, unsigned int type)
    {
        int result;
        glGetShaderiv(*id, GL_COMPILE_STATUS, &result);
        if (result == GL_FALSE)
        {
            int length;
            glGetShaderiv(*id, GL_INFO_LOG_LENGTH, &length);
            char* message = (char*)alloca(length * sizeof(char));
            glGetShaderInfoLog(*id, length, &length, message);
            std::cout << "Failed to compile ";
            switch (type)
            {
            case GL_VERTEX_SHADER:
                std::cout << "vertex";
                break;
            case GL_FRAGMENT_SHADER:
                std::cout << "fragment";
                break;
            case GL_GEOMETRY_SHADER:
                std::cout << "geometry";
                break;
            default:
                std::cout << "unknown";
                break;
            }
            std::cout << " shader!" << std::endl;
            std::cout << " Error:" << message << std::endl;
            glDeleteShader(*id);
            id = 0;
        }

    }





    void Shader::Setuniform1f(const std::string& name, float v1)
    {
        glUniform1f(GetUniformLocation(name), v1);
    }
    void Shader::Setuniform2f(const std::string& name, float v1, float v2)
    {
        glUniform2f(GetUniformLocation(name), v1, v2);
    }
    void Shader::Setuniform3f(const std::string& name, float v1, float v2, float v3)
    {
        glUniform3f(GetUniformLocation(name), v1, v2, v3);
    }
    void Shader::Setuniform4f(const std::string& name, float v1, float v2, float v3, float v4)
    {
        glUniform4f(GetUniformLocation(name), v1, v2, v3, v4);
    }

    void Shader::Setuniform1i(const std::string& name, int v1)
    {
        glUniform1i(GetUniformLocation(name), v1);
    }
    void Shader::Setuniform2i(const std::string& name, int v1, int v2)
    {
        glUniform2i(GetUniformLocation(name), v1, v2);
    }
    void Shader::Setuniform3i(const std::string& name, int v1, int v2, int v3)
    {
        glUniform3i(GetUniformLocation(name), v1, v2, v3);
    }
    void Shader::Setuniform4i(const std::string& name, int v1, int v2, int v3, int v4)
    {
        glUniform4i(GetUniformLocation(name), v1, v2, v3, v4);
    }

    void Shader::Setuniform1d(const std::string& name, double v1)
    {
        glUniform1d(GetUniformLocation(name), v1);
    }
    void Shader::Setuniform2d(const std::string& name, double v1, double v2)
    {
        glUniform2d(GetUniformLocation(name), v1, v2);
    }
    void Shader::Setuniform3d(const std::string& name, double v1, double v2, double v3)
    {
        glUniform3d(GetUniformLocation(name), v1, v2, v3);
    }
    void Shader::Setuniform4d(const std::string& name, double v1, double v2, double v3, double v4)
    {
        glUniform4d(GetUniformLocation(name), v1, v2, v3, v4);
    }

    void Shader::Setuniform1fv(const std::string& name, int count, glm::fvec1 v)
    {
        glUniform1fv(GetUniformLocation(name), count, &v[0]);
    }
    void Shader::Setuniform2fv(const std::string& name, int count, glm::fvec2 v)
    {
        glUniform2fv(GetUniformLocation(name), count, &v[0]);
    }
    void Shader::Setuniform3fv(const std::string& name, int count, glm::fvec3 v)
    {
        glUniform3fv(GetUniformLocation(name), count, &v[0]);
    }
    void Shader::Setuniform4fv(const std::string& name, int count, glm::fvec4 v)
    {
        glUniform4fv(GetUniformLocation(name), count, &v[0]);
    }

    void Shader::Setuniform1iv(const std::string& name, int count, glm::ivec1 v)
    {
        glUniform1iv(GetUniformLocation(name), count, &v[0]);
    }
    void Shader::Setuniform2iv(const std::string& name, int count, glm::ivec2 v)
    {
        glUniform2iv(GetUniformLocation(name), count, &v[0]);
    }
    void Shader::Setuniform3iv(const std::string& name, int count, glm::ivec3 v)
    {
        glUniform3iv(GetUniformLocation(name), count, &v[0]);
    }
    void Shader::Setuniform4iv(const std::string& name, int count, glm::ivec4 v)
    {
        glUniform4iv(GetUniformLocation(name), count, &v[0]);
    }

    void Shader::Setuniform1dv(const std::string& name, int count, glm::dvec1 v)
    {
        glUniform1dv(GetUniformLocation(name), count, &v[0]);
    }
    void Shader::Setuniform2dv(const std::string& name, int count, glm::dvec2 v)
    {
        glUniform2dv(GetUniformLocation(name), count, &v[0]);
    }
    void Shader::Setuniform3dv(const std::string& name, int count, glm::dvec3 v)
    {
        glUniform3dv(GetUniformLocation(name), count, &v[0]);
    }
    void Shader::Setuniform4dv(const std::string& name, int count, glm::dvec4 v)
    {
        glUniform4dv(GetUniformLocation(name), count, &v[0]);
    }

    void Shader::SetUniformMat2fv(const std::string& name, int count, GLboolean transpose, glm::mat2 value)
    {
        glUniformMatrix2fv(GetUniformLocation(name), count, transpose, &value[0][0]);
    }
    void Shader::SetUniformMat3fv(const std::string& name, int count, GLboolean transpose, glm::mat3 value)
    {
        glUniformMatrix3fv(GetUniformLocation(name), count, transpose, &value[0][0]);
    }
    void Shader::SetUniformMat4fv(const std::string& name, int count, GLboolean transpose, glm::mat4 value)
    {
        glUniformMatrix4fv(GetUniformLocation(name), count, transpose, &value[0][0]);
    }

}

Is this enough for using in normal programming or do i need to make some changes to make it more usable and efficient? Any helpful review would be appreciated. Thanks in advance!!

Answer 1

Various suggestions:

• Use the OpenGL types (e.g. GLuint, GLint) for the variables that need them. It's more portable, and the purpose of the variables is clearer.

• Don't store the shader source inside the shader. We don't need it after compiling the shader.

• It doesn't look like the type member variable is actually used (and it doesn't make sense as programs are made up of shader objects of various different types).

• The Program_id shouldn't be public, as the user shouldn't be able to change it directly. Add a getter function for it.

• We should destroy the shader program with glDeleteProgram when we're done with it (i.e. in the destructor).

• Copying a shader program doesn't really make sense from an OpenGL point of view... there's no way to copy OpenGL shaders or shader objects. So I'd suggest =deleteing the copy constructor and copy assignment operator, and only supporting move operations.

• We don't need operator-> or operator* or the Clone() function (and they don't really make sense).

• I don't think there's any point in caching uniform locations in a map keyed by a std::string. The driver will be doing more or less the exact same look-up when glGetUniformLocation() is called.

• Consider having separate "ShaderObject" and "ShaderProgram" classes. This might make loading assets easier later on (shader objects can be used by multiple programs), and it's closer to the OpenGL model.

---

Some other things about the code:

    if (m_UniformLocationCache.find(name) != m_UniformLocationCache.end())
    {
        return m_UniformLocationCache[name];
    }

This does two searches. Once when calling .find(), and the other in [name]. We should use the return value of the first find call instead.

        char* message = (char*)alloca(length * sizeof(char));

This isn't portable, as alloca isn't a standard library function and might not exist (or might be called something different) on a platform. Maybe creating a string of the appropriate length would be better: auto message = std::string(length, '\0');

    glLinkProgram(Program_id);

After linking the shader program, we can also detach the shader objects from the program with glDetachShader. This doesn't affect the linked program, and allows the shader objects to be cleaned up completely by the GL.