Reviewing of a shader class and a wrapper of it

Question

I come here for another review of my shader class. Previously I created a post for the very same purpose. But this time, I tried applying most of the suggestions from that post. And i even made a new wrapper class for it.

Here's my Shader class:

//SHADER.h
#pragma once
//includes

namespace SHADER {

    enum class Shader_Type {
        NONE = -1,
        VERTEX,
        FRAGMENT,
        GEOMETRY,
        TESS_EVAL,
        TESS_CONTROL,
    };

    class Shader {
    private:
        std::map<Shader_Type, std::string> m_Data;
        unsigned int Program_id;


        unsigned int CompileShader(unsigned int type, const std::string& source);
        void CheckCompileErrors(unsigned int shader, unsigned int type);
        void LinkAndValidateProgram();

    public:
        Shader();
        ~Shader();

        Shader(Shader&& other) noexcept;
        Shader& operator=(Shader&& other) noexcept;

        void Bind() const;
        void Unbind() const;
        void Reset();
        void Init(
            const std::string& vertexShader = "", const std::string& fragmentShader = "",
            const std::string& geometryShader = "", const std::string& tess_eval_shader = "",
            const std::string& tess_control_shader = "");


        unsigned int ProgramId() { return Program_id; }
        unsigned int GetUniformLocation(const std::string& name);

    public: //uniform functions

        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, const glm::fvec1& v);
        void SetUniform2fv(const std::string& name, const glm::fvec2& v);
        void SetUniform3fv(const std::string& name, const glm::fvec3& v);
        void SetUniform4fv(const std::string& name, const glm::fvec4& v);

        void SetUniform1iv(const std::string& name, const glm::ivec1& v);
        void SetUniform2iv(const std::string& name, const glm::ivec2& v);
        void SetUniform3iv(const std::string& name, const glm::ivec3& v);
        void SetUniform4iv(const std::string& name, const glm::ivec4& v);

        void SetUniform1dv(const std::string& name, const glm::dvec1& v);
        void SetUniform2dv(const std::string& name, const glm::dvec2& v);
        void SetUniform3dv(const std::string& name, const glm::dvec3& v);
        void SetUniform4dv(const std::string& name, const glm::dvec4& v);

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

}

//SHADER.cpp
#include "SHADER.h"

namespace SHADER {

    Shader::Shader() : Program_id(0) {}

    Shader::~Shader() {
        // glDeleteProgram(Program_id);//i have a personal reason to comment this...
    }

    Shader::Shader(Shader&& other) noexcept {
        *this = std::move(other); // Use move assignment operator
    }

    Shader& Shader::operator=(Shader&& other) noexcept {
        if (this != &other) {
            m_Data = std::move(other.m_Data);
            Program_id = other.Program_id;
        }
        return *this;
    }

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

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

    void Shader::Reset() {
        Program_id = 0;
        m_Data.clear();
    }

    void Shader::Init(const std::string& vertexShader, const std::string& fragmentShader, const std::string& geometryShader, const std::string& tess_eval_shader, const std::string& tess_control_shader)
    {
        m_Data[Shader_Type::VERTEX] = vertexShader;
        m_Data[Shader_Type::FRAGMENT] = fragmentShader;
        m_Data[Shader_Type::GEOMETRY] = geometryShader;
        m_Data[Shader_Type::TESS_EVAL] = tess_eval_shader;
        m_Data[Shader_Type::TESS_CONTROL] = tess_control_shader;

        LinkAndValidateProgram();
        Unbind();
    }

    unsigned int Shader::GetUniformLocation(const std::string& name) {
        int location = glGetUniformLocation(Program_id, name.c_str());
        if (location == -1) {
            MessageBoxA(nullptr, ("Warning! Uniform " + name + " not found or unused!").c_str(), "Uniform Warning", MB_OK);
        }
        return location;
    }

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

        if (!m_Data[Shader_Type::VERTEX].empty()) {
            unsigned int vs = CompileShader(GL_VERTEX_SHADER, m_Data[Shader_Type::VERTEX]);
            glAttachShader(Program_id, vs);
            glDeleteShader(vs);
        }

        if (!m_Data[Shader_Type::FRAGMENT].empty()) {
            unsigned int fs = CompileShader(GL_FRAGMENT_SHADER, m_Data[Shader_Type::FRAGMENT]);
            glAttachShader(Program_id, fs);
            glDeleteShader(fs);
        }

        if (!m_Data[Shader_Type::GEOMETRY].empty()) {
            unsigned int gs = CompileShader(GL_GEOMETRY_SHADER, m_Data[Shader_Type::GEOMETRY]);
            glAttachShader(Program_id, gs);
            glDeleteShader(gs);
        }

        if (!m_Data[Shader_Type::TESS_EVAL].empty()) {
            unsigned int ts = CompileShader(GL_TESS_EVALUATION_SHADER, m_Data[Shader_Type::TESS_EVAL]);
            glAttachShader(Program_id, ts);
            glDeleteShader(ts);
        }

        if (!m_Data[Shader_Type::TESS_CONTROL].empty()) {
            unsigned int ts = CompileShader(GL_TESS_CONTROL_SHADER, m_Data[Shader_Type::TESS_CONTROL]);
            glAttachShader(Program_id, ts);
            glDeleteShader(ts);
        }

        glLinkProgram(Program_id);
        glValidateProgram(Program_id);

        GLint success;
        glGetProgramiv(Program_id, GL_LINK_STATUS, &success);
        if (!success) {
            GLint infoLogLength = 0;
            glGetProgramiv(Program_id, GL_INFO_LOG_LENGTH, &infoLogLength);

            if (infoLogLength > 0) {
                std::vector<GLchar> infoLog(infoLogLength);
                glGetProgramInfoLog(Program_id, infoLogLength, nullptr, infoLog.data());

                std::string errorMessage(infoLog.begin(), infoLog.end());
                MessageBoxA(nullptr, ("Shader Program Linking Error: " + errorMessage).c_str(), "Shader Linking Error", MB_OK);
            }
            else {
                MessageBoxA(nullptr, "Shader Program Linking Error: Unknown error (no details)", "Shader Linking Error", MB_OK);
            }
        }

    }

    unsigned int Shader::CompileShader(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);

        CheckCompileErrors(id, type);
        return id;
    }

    void Shader::CheckCompileErrors(unsigned int shader, unsigned int type) {
        int success;
        glGetShaderiv(shader, GL_COMPILE_STATUS, &success);
        if (!success) {
            GLchar infoLog[1024];
            glGetShaderInfoLog(shader, 1024, nullptr, infoLog);

            std::string shaderType;
            switch (type) {
            case GL_VERTEX_SHADER:          shaderType = "VERTEX"; break;
            case GL_FRAGMENT_SHADER:        shaderType = "FRAGMENT"; break;
            case GL_GEOMETRY_SHADER:        shaderType = "GEOMETRY"; break;
            case GL_TESS_EVALUATION_SHADER: shaderType = "TESSELLATION-EVALUATION"; break;
            case GL_TESS_CONTROL_SHADER:    shaderType = "TESSELLATION-CONTROL"; break;
            default: shaderType = "UNKNOWN"; break;
            }

            MessageBoxA(nullptr, ("Shader Compilation Error (" + shaderType + "): " + std::string(infoLog)).c_str(), "Shader Compilation Error", MB_OK);
            glDeleteShader(shader);
        }
    }

    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, const glm::fvec1& v) { glUniform1fv(GetUniformLocation(name), 1, &v[0]); }
    void Shader::SetUniform2fv(const std::string& name, const glm::fvec2& v) { glUniform2fv(GetUniformLocation(name), 1, &v[0]); }
    void Shader::SetUniform3fv(const std::string& name, const glm::fvec3& v) { glUniform3fv(GetUniformLocation(name), 1, &v[0]); }
    void Shader::SetUniform4fv(const std::string& name, const glm::fvec4& v) { glUniform4fv(GetUniformLocation(name), 1, &v[0]); }

    void Shader::SetUniform1iv(const std::string& name, const glm::ivec1& v) { glUniform1iv(GetUniformLocation(name), 1, &v[0]); }
    void Shader::SetUniform2iv(const std::string& name, const glm::ivec2& v) { glUniform2iv(GetUniformLocation(name), 1, &v[0]); }
    void Shader::SetUniform3iv(const std::string& name, const glm::ivec3& v) { glUniform3iv(GetUniformLocation(name), 1, &v[0]); }
    void Shader::SetUniform4iv(const std::string& name, const glm::ivec4& v) { glUniform4iv(GetUniformLocation(name), 1, &v[0]); }

    void Shader::SetUniform1dv(const std::string& name, const glm::dvec1& v) { glUniform1dv(GetUniformLocation(name), 1, &v[0]); }
    void Shader::SetUniform2dv(const std::string& name, const glm::dvec2& v) { glUniform2dv(GetUniformLocation(name), 1, &v[0]); }
    void Shader::SetUniform3dv(const std::string& name, const glm::dvec3& v) { glUniform3dv(GetUniformLocation(name), 1, &v[0]); }
    void Shader::SetUniform4dv(const std::string& name, const glm::dvec4& v) { glUniform4dv(GetUniformLocation(name), 1, &v[0]); }

    void Shader::SetUniformMat2fv(const std::string& name, const glm::mat2& value, GLboolean transpose) {
        glUniformMatrix2fv(GetUniformLocation(name), 1, transpose, &value[0][0]);
    }

    void Shader::SetUniformMat3fv(const std::string& name, const glm::mat3& value, GLboolean transpose) {
        glUniformMatrix3fv(GetUniformLocation(name), 1, transpose, &value[0][0]);
    }

    void Shader::SetUniformMat4fv(const std::string& name, const glm::mat4& value, GLboolean transpose) {
        glUniformMatrix4fv(GetUniformLocation(name), 1, transpose, &value[0][0]);
    }

}

And here is the wrapper class:

//Shader_Binder.h
#pragma once
#include"includes.h"
#include"SHADER.h"

namespace SHADER
{
    enum class ShaderID
    {
        SH_00 = 0,
        SH_01 = 1, SH_02, SH_03, SH_04, SH_05, SH_06, SH_07, SH_08, SH_09, SH_10,
        SH_11, SH_12, SH_13, SH_14, SH_15, SH_16, SH_17, SH_18, SH_19, SH_20,
        SH_21, SH_22, SH_23, SH_24, SH_25, SH_26, SH_27, SH_28, SH_29, SH_30,
        SH_31, SH_32, SH_33, SH_34, SH_35, SH_36, SH_37, SH_38, SH_39, SH_40,
        SH_41, SH_42, SH_43, SH_44, SH_45, SH_46, SH_47, SH_48, SH_49, SH_50,
    };
#define MAX_SHADER_ID 50
    class Shader_Binder
    {
    private:
        struct Data
        {
            ShaderID id;
            Shader m_shader;
            std::vector<unsigned int> m_VAOs;
            std::vector<unsigned int> m_VBOs;
        };
        ShaderID m_CurrentShader = ShaderID::SH_00; // Track current shader
        unsigned int m_CurrentVAO = 0; // Track current VAO
        unsigned int m_CurrentVBO = 0; // Track current VBO
        std::unordered_map<ShaderID, Data>m_Data;

    public:

        // Static method to get the single instance of the class
        static Shader_Binder& getInstance()
        {
            static Shader_Binder instance; // Guaranteed to be destroyed, instantiated on first use
            return instance;
        }

        // Deleted copy constructor and assignment operator to prevent copying
        Shader_Binder(const Shader_Binder&) = delete;
        Shader_Binder& operator=(const Shader_Binder&) = delete;

        /* Creates a new shader, binds it and returns a ID */
        ShaderID CreateNewShader(const std::string vShader = "", const std::string fShader = "", const std::string gShader = "");
        /* Binds the shader passed in and returns a ID */
        ShaderID CreateNewShader(Shader& shader);

        /* Binds the shader, VAO and VBO*/
        void bind(ShaderID id = ShaderID::SH_00, int vaoid = -1, int vboid = -1);
        /* Unbinds the shader, VAO and VBO*/
        void unbind(ShaderID id = ShaderID::SH_00, int vaoid = -1, int vboid = -1);

        /* Creates a new VAO on the given shader id, binds it and returns the value of the created VAO. */
        unsigned int CreateNewVAO(ShaderID id);
        /* Creates a new VBO on the given shader id, binds it and returns the value of the created VBO. */
        unsigned int CreateNewVBO(ShaderID id);

        /* Returns a pointer to the shader object relative to the id passed in*/
        Shader* getShader(ShaderID id);

        /* Sets the vertex attributes based on the shader bound before calling this function
         * vaoIndex is set to 0 by default ,
         * considering there are no multiple vao's created for the same shader */
        void setVertexAttribs(unsigned int Gl_location_index, int size,
            unsigned int type, GLboolean normalized, int stride, const void* pointer,
            unsigned int vaoIndex = 0);
        /*
        * Assumes GL_DYNAMIC_DRAW as the default data usage.
        * Requires the size of the buffer to be sent, even if the usage is GL_DYNAMIC_DRAW.
        * Assumes the required shaders and others are bound before using.
        * Set the usage to GL_STATIC_DRAW if the data is to be static and other params are passed.
        * Assumes GL_ARRAY_BUFFER as the default target to be used
        * recommended to use again, when needed to update the size of the buffer
        */
        void setBufferData(GLenum usage = GL_DYNAMIC_DRAW, GLsizeiptr size = 0,
            const void* data = nullptr, GLenum target = GL_ARRAY_BUFFER);

        /*
        * Updates the buffer data if the usage was GL_DYNAMIC_DRAW.
        * Assumes the required shaders and others are bound before using.
        * Uses glBufferSubData() for the task
        * Considers GL_ARRAY_BUFFER as the default target
        */
        void UpdateBufferData(GLintptr offset, GLsizeiptr size, const void* data, GLenum target = GL_ARRAY_BUFFER);

        /*unbinds the last used (or whichever currently bound) shader and vao and vbo */
        void reset();
    private:
        bool isIdUsed(ShaderID id);
        Shader_Binder() {}
        ~Shader_Binder() {}

    };
}

typedef SHADER::ShaderID ShaderID;
static SHADER::Shader_Binder& Binder = SHADER::Shader_Binder::getInstance();

//Shader_Binder.cpp
#include "Shader_Binder.h"

namespace SHADER
{
    ShaderID Shader_Binder::CreateNewShader(const std::string vShader, const std::string fShader, const std::string gShader)
    {
        Shader shader;
        shader.Init(vShader, fShader, gShader);
        shader.Bind();
        for (int i = 1; i <= MAX_SHADER_ID; i++)
        {
            if (!isIdUsed(ShaderID(i)))
            {
                m_Data[ShaderID(i)].id = ShaderID(i);
                m_Data[ShaderID(i)].m_shader = std::move(shader);
                return ShaderID(i);
            }
        }
        if (isIdUsed(ShaderID(MAX_SHADER_ID)))
        {
            MessageBoxA(NULL, "Max Amount of shaders used... cannot create any more shaders...", "Shader Limit Reached!!!", MB_OK);
            return ShaderID::SH_00;
        }
    }
    ShaderID Shader_Binder::CreateNewShader(Shader& shader)
    {
        shader.Bind();
        for (int i = 1; i <= MAX_SHADER_ID; i++)
        {
            if (!isIdUsed(ShaderID(i)))
            {
                m_Data[ShaderID(i)].id = ShaderID(i);
                m_Data[ShaderID(i)].m_shader = std::move(shader);
                return ShaderID(i);
            }
        }
        if (isIdUsed(ShaderID(MAX_SHADER_ID)))
        {
            MessageBoxA(NULL, "Max Amount of shaders used... cannot create any more shaders...", "Shader Limit Reached!!!", MB_OK);
            return ShaderID::SH_00;
        }
    }

    void Shader_Binder::bind(ShaderID id, int vaoid, int vboid)
    {
        if (!isIdUsed(id)) return;
        GLenum err;

        // Bind the shader and update tracking
        if (m_CurrentShader != id)
        {
            m_CurrentShader = id;            
            m_Data[m_CurrentShader].m_shader.Bind();
        }
        else
        {
            MessageBoxA(NULL, "Shader id not found or is 0 ... escaping the binding function... ", "Binding error", MB_OK);
            return;
        }
        // Bind the VAO and update tracking if needed
        if (vaoid != -1 && m_CurrentVAO != vaoid)
        {
            GL_CHECK(glBindVertexArray(vaoid));
            m_CurrentVAO = vaoid;
        }

        // Bind the VBO and update tracking if needed
        if (vboid != -1 && m_CurrentVBO != vboid)
        {
            GL_CHECK(glBindBuffer(GL_ARRAY_BUFFER, vboid));
            m_CurrentVBO = vboid;
        }
    }
    void Shader_Binder::unbind(ShaderID id, int vaoid, int vboid)
    {
        if (!isIdUsed(id)) return;

        // Unbind the VAO only if it's the current one
        if (vaoid != -1 && m_CurrentVAO == vaoid)
        {
            //glBindVertexArray(0);
            GL_CHECK(glBindVertexArray(0));
            m_CurrentVAO = 0;
        }

        // Unbind the VBO only if it's the current one
        if (vboid != -1 && m_CurrentVBO == vboid)
        {
            //glBindBuffer(GL_ARRAY_BUFFER, 0);
            GL_CHECK(glBindBuffer(GL_ARRAY_BUFFER, 0));
            m_CurrentVBO = 0;
        }
        // Unbind the shader only if it's actually bound
        if (m_CurrentShader == id)
        {
            m_Data[id].m_shader.Unbind();
            m_CurrentShader = ShaderID::SH_00; // Or some default "unbound" state
        }

    }

    unsigned int Shader_Binder::CreateNewVAO(ShaderID id)
    {
        if (!isIdUsed(id)) return -1;

        GL_CHECK(glGenVertexArrays(1, &m_CurrentVAO));

        m_Data[id].m_VAOs.push_back(m_CurrentVAO);

        GL_CHECK(glBindVertexArray(m_CurrentVAO));
        return m_CurrentVAO;
    }
    unsigned int Shader_Binder::CreateNewVBO(ShaderID id)
    {
        if (!isIdUsed(id)) return -1;

        GL_CHECK(glGenBuffers(1, &m_CurrentVBO));

        m_Data[id].m_VBOs.push_back(m_CurrentVBO);

        GL_CHECK(glBindBuffer(GL_ARRAY_BUFFER, m_CurrentVBO));
        return m_CurrentVBO;
    }


    Shader* Shader_Binder::getShader(ShaderID id)
    {
        return &m_Data.at(id).m_shader;
    }

    void Shader_Binder::setVertexAttribs(unsigned int Gl_location_index, int size, unsigned int type, GLboolean normalized, int stride, const void* pointer, unsigned int vaoIndex)
    {
        GL_CHECK(glEnableVertexAttribArray(Gl_location_index));
        GL_CHECK(glVertexAttribPointer(Gl_location_index, size, type, normalized, stride, pointer));
    }

    void Shader_Binder::setBufferData(GLenum usage, GLsizeiptr size, const void* data, GLenum target)
    {
        GL_CHECK(glBufferData(target, size, data, usage));
    }

    void Shader_Binder::UpdateBufferData(GLintptr offset, GLsizeiptr size, const void* data, GLenum target)
    {
        GL_CHECK(glBufferSubData(target, offset, size, data));
    }

    bool Shader_Binder::isIdUsed(ShaderID id)
    {
        return m_Data.find(id) != m_Data.end();
    }

    void Shader_Binder::reset()
    {
        if (m_CurrentShader != ShaderID::SH_00)
        {
            m_Data[m_CurrentShader].m_shader.Unbind();
            m_CurrentShader = ShaderID::SH_00;
        }

        if (m_CurrentVAO != 0)
        {
            GL_CHECK(glBindVertexArray(0));
            m_CurrentVAO = 0;
        }

        if (m_CurrentVBO != 0)
        {
            GL_CHECK(glBindBuffer(GL_ARRAY_BUFFER, 0));
            m_CurrentVBO = 0;
        }
    }
}

I have yet to learn about tessellation shaders, so i just put it in the shader class and not the wrapper yet.

Can you please review the code and give me suggestions on how i can improve my code for efficient use and in a more convenient way?

Any ideas are accepted, no matter how small. All i want to do is to not have my shader class fall behind the other class that i wrote.

Chris · Accepted Answer · 2024-10-22 17:01:07Z

Looking at Shader_Binder::CreateNewShader and its overloads:

    ShaderID Shader_Binder::CreateNewShader(const std::string vShader, const std::string fShader, const std::string gShader)
    {
        Shader shader;
        shader.Init(vShader, fShader, gShader);
        shader.Bind();
        for (int i = 1; i <= MAX_SHADER_ID; i++)
        {
            if (!isIdUsed(ShaderID(i)))
            {
                m_Data[ShaderID(i)].id = ShaderID(i);
                m_Data[ShaderID(i)].m_shader = std::move(shader);
                return ShaderID(i);
            }
        }
        if (isIdUsed(ShaderID(MAX_SHADER_ID)))
        {
            MessageBoxA(NULL, "Max Amount of shaders used... cannot create any more shaders...", "Shader Limit Reached!!!", MB_OK);
            return ShaderID::SH_00;
        }
    }
    ShaderID Shader_Binder::CreateNewShader(Shader& shader)
    {
        shader.Bind();
        for (int i = 1; i <= MAX_SHADER_ID; i++)
        {
            if (!isIdUsed(ShaderID(i)))
            {
                m_Data[ShaderID(i)].id = ShaderID(i);
                m_Data[ShaderID(i)].m_shader = std::move(shader);
                return ShaderID(i);
            }
        }
        if (isIdUsed(ShaderID(MAX_SHADER_ID)))
        {
            MessageBoxA(NULL, "Max Amount of shaders used... cannot create any more shaders...", "Shader Limit Reached!!!", MB_OK);
            return ShaderID::SH_00;
        }
    }

There's a big chunk of shared logic for finding the first available shader ID that feels ripe for refactoring into a separate function.

You've also got ShaderID(i) repeated five times. You may want to factor that out as well.

In the event a shader ID is not available, are you sure you really want to return the 0 shader ID? Will users of this code know to check for that to find out that something (that seems) erroneous has occurred?

Should this throw an exception?
Should it return a std::optional<ShaderID>?

We can further assume that since your loop includes MAX_SHADER_ID, if control flow continues past the loop, we know that isIdUsed(ShaderID(MAX_SHADER_ID)) is true.

As a result, we don't need to check that.

    ShaderID Shader_Binder::CreateNewShader(Shader& shader)
    {
        shader.Bind();
        for (int i = 1; i <= MAX_SHADER_ID; i++)
        {
            if (!isIdUsed(ShaderID(i)))
            {
                m_Data[ShaderID(i)].id = ShaderID(i);
                m_Data[ShaderID(i)].m_shader = std::move(shader);
                return ShaderID(i);
            }
        }
        
        MessageBoxA(NULL, "Max Amount of shaders used... cannot create any more shaders...", "Shader Limit Reached!!!", MB_OK);
        return ShaderID::SH_00;
    }

If we didn't know this was always going to happen, we'd look at your function and say it doesn't return in every case, leading to the potential for undefined behavior. A compiler that isn't as smart as us humans might still warn you about that with your original code.

You could refactor the conditional in your loop with continue, but that's subjective style.

    ShaderID Shader_Binder::CreateNewShader(Shader& shader)
    {
        shader.Bind();
        for (int i = 1; i <= MAX_SHADER_ID; i++)
        {
            if (isIdUsed(ShaderID(i))) continue;

            m_Data[ShaderID(i)].id = ShaderID(i);
            m_Data[ShaderID(i)].m_shader = std::move(shader);
            return ShaderID(i);
        }
        
        MessageBoxA(NULL, "Max Amount of shaders used... cannot create any more shaders...", "Shader Limit Reached!!!", MB_OK);
        return ShaderID::SH_00;
    }

that is a really nice idea you gave me. ohh and about the error checking, i havent made that properly, but i've made sure to debug everytime that happens, though that happens rarely for me. i'll make sure to remove the repeating lines of code. — Shreeyash Shrestha, Commented Oct 23 at 4:38

Stack Exchange Network

Reviewing of a shader class and a wrapper of it

1 Answer 1

Your Answer

Not the answer you're looking for? Browse other questions tagged
c++
opengl
shaders
or ask your own question.

Linked

Hot Network Questions

Reviewing of a shader class and a wrapper of it

1 Answer 1

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged c++openglshaders or ask your own question.

Linked

Related

Hot Network Questions

Not the answer you're looking for? Browse other questions tagged
c++
opengl
shaders
or ask your own question.