8
\$\begingroup\$

I am following an OpenGL tutorial series and it got to the point where the program needed some code abstraction. I followed the tutorial on the abstraction of VBOs, IBOs and the like and when it got time to do the shaders, I decided it would be a good opportunity for me to practice, so I did my best to make it happen.

Seeming as I am fairly new to C++ I thought it would be a great idea to post my code on here to take in some constructive criticism. Make sure to point out what I did bad (because I am sure there will be plenty such examples) but it also would be nice to tell me what I did good so I can reinforce such behavior in the future. There are 4 relevant source files. (Excluding the headers and some helper classes, but I will post them as well.)

I'll start with the main program - Application.cpp:

#include <GL/glew.h>
#include <GLFW/glfw3.h>
#include <iostream>
#include <fstream>
#include <string>
#include <sstream>
#include "Renderer.h"
#include "IndexBuffer.h"
#include "VertexBuffer.h"
#include "VertexArray.h"
#include "BufferLayout.h"
#include "Shader.h"
#include "GLProgram.h"
#include "Uniform.h"

#define _USE_MATH_DEFINES
#include <math.h>

struct Vector4
{
    float x, y, z, w;

    Vector4(float x, float y, float z, float w)
        : x(x), y(y), z(z), w(w)
    {

    }
};

int main(void)
{
    GLFWwindow* window;

    /* Initialize the library */
    if (!glfwInit())
        return -1;

    glfwWindowHint(GLFW_CONTEXT_VERSION_MAJOR, 3);
    glfwWindowHint(GLFW_CONTEXT_VERSION_MINOR, 3);
    glfwWindowHint(GLFW_OPENGL_PROFILE, GLFW_OPENGL_CORE_PROFILE);

    /* Create a windowed mode window and its OpenGL context */
    window = glfwCreateWindow(640, 480, "Hello World", NULL, NULL);             // Window with OpenGL context is created but context is not set to current.
    if (!window)
    {
        glfwTerminate();
        return -1;
    }

    /* Make the window's context current */
    glfwMakeContextCurrent(window);                                             // Setting the window's OpenGL context before initializing GLEW is critical.
    glfwSwapInterval(1);

    if (glewInit() != GLEW_OK)                                                  // GLEW needs to be initialized before attempting to call any GL functions. Beware of the scary NULL pointers.
        std::cout << "GLEW initialization error. Terminating..." << std::endl;                                                  

    std::cout << glGetString(GL_VERSION) << std::endl;                                                      

    float tri2Dpositions[8] = {             // Defined an array containing all our verticies.
        -0.4f, -0.35f,
         0.8f, -0.35f,
         0.4f,  0.35f,
        -0.8f,  0.35f
    };

    unsigned int indicies[6] = {            // Defined indicies of drawing order.
         0, 1, 2,
         2, 3, 0 
    };

    {
        VertexBuffer vbo(tri2Dpositions, sizeof(tri2Dpositions));
        IndexBuffer ibo(indicies, 6);

        BufferLayout layout;
        layout.Push<float>(2);

        VertexArray va;
        va.AddBuffer(vbo, layout);

        Shader testShaders[2] =                                     // Shader abstraction in use.
        {
            Shader(GL_VERTEX_SHADER, "res/shaders/Basic.shader"),
            Shader(GL_FRAGMENT_SHADER, "res/shaders/Basic.shader")
        };

        GLProgram program(testShaders, 2);                          

        float slopeIncrement = 0.04f;
        bool clockwise = true;
        unsigned long count = 0;

        int windowWidth, windowHeight;
        glfwGetWindowSize(window, &windowWidth, &windowHeight);

        float color1[4] = { 0.0f, 1.0f, 0.0f, 1.0f };
        float color2[4] = { 1.0f, 0.0f, 0.0f, 1.0f };
        float windowSize[2] = { windowWidth, windowHeight };
        float slope = 0.0f;
        int switched = false;

        Uniform c1(color1, UniformType::FLOAT4, "u_Color", false);                  // Uniform abstraction in use.
        Uniform c2(color2, UniformType::FLOAT4, "u_Color2", false);
        Uniform WindowSize(windowSize, UniformType::FLOAT2, "u_WindowSize", false);
        Uniform SlopeBounds(&slope, UniformType::FLOAT, "u_SlopeBoundary", false);
        Uniform ColorSwitched(&switched, UniformType::INT, "u_Switched", false);

        program.Bind();

        program.AttachUniform(c1);
        program.AttachUniform(c2);
        program.AttachUniform(WindowSize);
        program.AttachUniform(SlopeBounds);
        program.AttachUniform(ColorSwitched);
        program.RefreshUniforms();

        /* Loop until the user closes the window */
        while (!glfwWindowShouldClose(window))
        {
            /* Render here */
            glClear(GL_COLOR_BUFFER_BIT);

            ibo.Bind();
            va.Bind();

            SlopeBounds.SetData(&slope);
            ColorSwitched.SetData(&switched);
            program.RefreshUniforms();

            GLCall(glDrawElements(GL_TRIANGLES, 6, GL_UNSIGNED_INT, nullptr));      // Draws the currently bound array with specified draw mode using default shaders (if available)
                                                                                    // in the occasion that we aren't binding any custom shaders prior. 
            if (!clockwise)
            {
                slopeIncrement = abs(slopeIncrement);

                if (slope > M_PI)
                {
                    slope = 0;
                    switched = !switched;
                    count++;
                    if (count % 4 == 0) clockwise = !clockwise;
                }
            }
            else if (clockwise)
            {
                slopeIncrement = -abs(slopeIncrement);

                if (slope < 0)
                {
                    slope = M_PI;
                    switched = !switched;
                    count++;
                    if (count % 4 == 0) clockwise = !clockwise;
                }
            }

            slope += slopeIncrement;

            /* Swap front and back buffers */
            glfwSwapBuffers(window);

            /* Poll for and process events */
            glfwPollEvents();
        }
    }

    glfwTerminate();
    return 0;
}

Shader.h:

#pragma once
#include "Renderer.h"
#include <GL/glew.h>
#include <iostream>
#include <string>
#include "Uniform.h"

class Shader
{
private:
    unsigned int m_RendererID;
    unsigned int m_ShaderType;
    bool m_Attachable = false;

    std::string m_Source;

    std::string Parse(const unsigned int type, const std::string& filepath);
    void Compile();
    bool CompileCheck();

public:
    Shader(const unsigned int type, const std::string& filepath);
    ~Shader();

    void Recompile(const std::string& filepath);

    inline unsigned int GetHandle() const
    {
        return m_RendererID;
    }

    inline bool Attachable() const
    {
        return m_Attachable;
    }

    inline unsigned int GetType() const
    {
        return m_ShaderType;
    }

    inline const std::string& GetSource() const
    {
        return m_Source;
    }

    inline bool SameInstance(const Shader& s)
    {
        return (this == &s) ? true : false;
    }
};

Shader.cpp:

#include "Shader.h"
#include <sstream>
#include <fstream>


Shader::Shader(const unsigned int type, const std::string& filepath)
{
    switch (type)
    {
        case GL_VERTEX_SHADER:
            m_ShaderType = GL_VERTEX_SHADER;
            m_Source = Parse(m_ShaderType, filepath);
            break;
        case GL_FRAGMENT_SHADER:
            m_ShaderType = GL_FRAGMENT_SHADER;
            m_Source = Parse(m_ShaderType, filepath);
            break;
        default:
            std::cout << "Unrecognized shader type. Defaulting to GL_VERTEX_SHADER..." << std::endl;
            m_ShaderType = GL_VERTEX_SHADER;
            m_Source = Parse(m_ShaderType, filepath);
    }

    GLCall(m_RendererID = glCreateShader(m_ShaderType));

    Compile();
}

Shader::~Shader()
{
    GLCall(glDeleteShader(m_RendererID));
}

std::string Shader::Parse(const unsigned int type, const std::string& filepath)
{
    std::ifstream stream(filepath);

    std::string line;
    std::stringstream stringStream;
    bool write = false;

    if (type == GL_VERTEX_SHADER)
    {
        while (getline(stream, line))
        {
            if (line.find("#shader vertex") != std::string::npos && write == false)
            {
                write = true;
            }
            else if (line.find("#shader") != std::string::npos && write == true)
            {
                write = false;
            }
            else if (write)
            {
                stringStream << line << "\n";
            }
        }
    }
    else if (type == GL_FRAGMENT_SHADER)
    {
        while (getline(stream, line))
        {
            if (line.find("#shader fragment") != std::string::npos && write == false)
            {
                write = true;
            }
            else if (line.find("#shader") != std::string::npos && write == true)
            {
                write = false;
            }
            else if (write)
            {
                stringStream << line << "\n";
            }
        }
    }
    else
    {
        std::cout << "Couldn't find appropriate shader (Maybe misspelled markup or defaulted type?)... Aborting" << std::endl;
        return nullptr;
    }

    return stringStream.str();
}

void Shader::Compile()
{
    const char* src = m_Source.c_str();

    GLCall(glShaderSource(m_RendererID, 1, &src, nullptr));
    GLCall(glCompileShader(m_RendererID));

    bool state = CompileCheck();
    ASSERT(state);
    m_Attachable = state ? true : false;
}

void Shader::Recompile(const std::string& filepath)
{
    m_Source = Parse(m_ShaderType, filepath);
    const char* src = m_Source.c_str();

    GLCall(glShaderSource(m_RendererID, 1, &src, nullptr));    
    GLCall(glCompileShader(m_RendererID));

    bool state = CompileCheck();
    ASSERT(state);
    m_Attachable = state ? true : false;
}

bool Shader::CompileCheck()
{
    int result;
    GLCall(glGetShaderiv(m_RendererID, GL_COMPILE_STATUS, &result));                                                    

    if (result == GL_FALSE)                         
    {
        int length;
        GLCall(glGetShaderiv(m_RendererID, GL_INFO_LOG_LENGTH, &length));       

        char* message = (char*)(alloca(length * sizeof(char)));           
        GLCall(glGetShaderInfoLog(m_RendererID, length, &length, message)); 

        std::cout << "Failed to compile " << (m_ShaderType == GL_VERTEX_SHADER ? "vertex" : "fragment") << " shader" << std::endl;  
        std::cout << message << std::endl;

        GLCall(glDeleteShader(m_RendererID));  
        return false;
    }

    return true;
}

Uniform.h:

#pragma once
#include <iostream>
#include <vector>
#include <GL\glew.h>
#include "Renderer.h"

enum UniformType
{
    INVALID = -1,
    FLOAT,
    FLOAT2,
    FLOAT3,
    FLOAT4,
    fMAT2x2,
    fMAT3x3,
    fMAT4x4,
    DOUBLE,
    DOUBLE2,
    DOUBLE3,
    DOUBLE4,
    dMAT2x2,
    dMAT3x3,
    dMAT4x4,
    INT,
    INT2,
    INT3,
    INT4,
    iMAT2x2,
    iMAT3x3,
    iMAT4x4
};

class Uniform
{
private:
    void* m_Data;
    UniformType m_Type = INVALID;
    std::string m_UName;
    bool m_Transpose = false;

    template<typename T>
    void ChangeData(void* data, unsigned int count);

    template<>
    void ChangeData<float>(void* data, unsigned int count);

    template<>
    void ChangeData<double>(void* data, unsigned int count);

    template<>
    void ChangeData<int>(void* data, unsigned int count);

public:
    Uniform(void* data, UniformType type, const std::string& identifier, bool transpose);
    ~Uniform();

    void* GetData() const
    {
        if (m_Type < 0)
        {
            std::cout << "Error getting uniform data. Uniform is not initialized properly." << std::endl;
            return nullptr;
        }

        return m_Data;
    }

    inline const std::string& GetName() const
    {
        return m_UName;
    }

    inline const UniformType GetType() const
    {
        return m_Type;
    }

    inline const bool Transpose() const
    {
        return m_Transpose;
    }

    void SetData(void* data);
};

Uniform.cpp:

#include "Uniform.h"

Uniform::Uniform(void * data, UniformType type, const std::string& identifier, bool transpose)
{
    switch (type)
    {
        case FLOAT:
            m_Data = new float;
            m_Type = type;
            SetData(data);
            m_UName = identifier;
            break;
        case FLOAT2:
            m_Data = new float[2];
            m_Type = type;
            SetData(data);
            m_UName = identifier;
            break;
        case FLOAT3:
            m_Data = new float[3];
            m_Type = type;
            SetData(data);
            m_UName = identifier;
            break;
        case FLOAT4:
            m_Data = new float[4];
            m_Type = type;
            SetData(data);
            m_UName = identifier;
            break;
        case DOUBLE:
            m_Data = new double;
            m_Type = type;
            SetData(data);
            m_UName = identifier;
            break;
        case DOUBLE2:
            m_Data = new double[2];
            m_Type = type;
            SetData(data);
            m_UName = identifier;
            break;
        case DOUBLE3:
            m_Data = new double[3];
            m_Type = type;
            SetData(data);
            m_UName = identifier;
            break;
        case DOUBLE4:
            m_Data = new double[4];
            m_Type = type;
            SetData(data);
            m_UName = identifier;
            break;
        case INT:
            m_Data = new int;
            m_Type = type;
            SetData(data);
            m_UName = identifier;
            break;
        case INT2:
            m_Data = new int[2];
            m_Type = type;
            SetData(data);
            m_UName = identifier;
            break;
        case INT3:
            m_Data = new int[3];
            m_Type = type;
            SetData(data);
            m_UName = identifier;
            break;
        case INT4:
            m_Data = new int[4];
            m_Type = type;
            SetData(data);
            m_UName = identifier;
            break;
        case fMAT2x2:
            m_Data = new float[4];
            m_Type = type;
            SetData(data);
            m_UName = identifier;
            m_Transpose = transpose;
            break;
        case fMAT3x3:
            m_Data = new float[9];
            m_Type = type;
            SetData(data);
            m_UName = identifier;
            m_Transpose = transpose;
            break;
        case fMAT4x4:
            m_Data = new float[16];
            m_Type = type;
            SetData(data);
            m_UName = identifier;
            m_Transpose = transpose;
            break;
        case dMAT2x2:
            m_Data = new double[4];
            m_Type = type;
            SetData(data);
            m_UName = identifier;
            m_Transpose = transpose;
            break;
        case dMAT3x3:
            m_Data = new double[9];
            m_Type = type;
            SetData(data);
            m_UName = identifier;
            m_Transpose = transpose;
            break;
        case dMAT4x4:
            m_Data = new double[16];
            m_Type = type;
            SetData(data);
            m_UName = identifier;
            m_Transpose = transpose;
            break;
        case iMAT2x2:
            m_Data = new int[4];
            m_Type = type;
            SetData(data);
            m_UName = identifier;
            m_Transpose = transpose;
            break;
        case iMAT3x3:
            m_Data = new int[9];
            m_Type = type;
            SetData(data);
            m_UName = identifier;
            m_Transpose = transpose;
            break;
        case iMAT4x4:
            m_Data = new int[16];
            m_Type = type;
            SetData(data);
            m_UName = identifier;
            m_Transpose = transpose;
            break;
    }
}

Uniform::~Uniform()
{
    if ((m_Type >= 0 && m_Type <= 3))
    {
        delete (float*)m_Data;
    }
    else if ((m_Type >= 4 && m_Type <= 6))
    {
        delete[] (float*)m_Data;
    }
    else if ((m_Type >= 7 && m_Type <= 10))
    {
        delete (double*)m_Data;
    }
    else if ((m_Type >= 11 && m_Type <= 13))
    {
        delete[] (double*)m_Data;
    }
    else if ((m_Type >= 14 && m_Type <= 16))
    {
        delete (int*)m_Data;
    }
    else if ((m_Type >= 17 && m_Type <= 20))
    {
        delete[] (int*)m_Data;
    }
}

void Uniform::SetData(void* data)
{
    switch (m_Type)
    {
        case FLOAT:
            ChangeData<float>(data, 1);
            break;
        case FLOAT2:
            ChangeData<float>(data, 2);
            break;
        case FLOAT3:
            ChangeData<float>(data, 3);
            break;
        case FLOAT4:
            ChangeData<float>(data, 4);
            break;
        case DOUBLE:
            ChangeData<double>(data, 1);
            break;
        case DOUBLE2:
            ChangeData<double>(data, 2);
            break;
        case DOUBLE3:
            ChangeData<double>(data, 3);
            break;
        case DOUBLE4:
            ChangeData<double>(data, 4);
            break;
        case INT:
            ChangeData<int>(data, 1);
            break;
        case INT2:
            ChangeData<int>(data, 2);
        case INT3:
            ChangeData<int>(data, 3);
            break;
        case INT4:
            ChangeData<int>(data, 4);
            break;
        case fMAT2x2:
            ChangeData<float>(data, 4);
            break;
        case fMAT3x3:
            ChangeData<float>(data, 9);
            break;
        case fMAT4x4:
            ChangeData<float>(data, 16);
            break;
        case dMAT2x2:
            ChangeData<double>(data, 4);
            break;
        case dMAT3x3:
            ChangeData<double>(data, 9);
            break;
        case dMAT4x4:
            ChangeData<double>(data, 16);
            break;
        case iMAT2x2:
            ChangeData<int>(data, 4);
            break;
        case iMAT3x3:
            ChangeData<int>(data, 9);
            break;
        case iMAT4x4:
            ChangeData<int>(data, 16);
            break;
    }
}

template<>
void Uniform::ChangeData<float>(void* data, unsigned int count)
{
    for (unsigned int i = 0; i < count; i++)
    {
        *((float*)m_Data + i) = *((float*)data + i);
    }
}

template<>
void Uniform::ChangeData<double>(void* data, unsigned int count)
{
    for (unsigned int i = 0; i < count; i++)
    {
        *((double*)m_Data + i) = *((double*)data + i);
    }
}

template<>
void Uniform::ChangeData<int>(void* data, unsigned int count)
{
    for (unsigned int i = 0; i < count; i++)
    {
        *((int*)m_Data + i) = *((int*)data + i);
    }
}

GLProgram.h:

#pragma once
#include "Shader.h"
#include <vector>

class GLProgram
{
private:
    unsigned int m_RendererID;
    std::vector<Shader*> m_AttachedShaders;
    std::vector<Uniform*> m_Uniforms;
    int* m_UniformLocations = nullptr;

    void LinkProgram();
    int* GetUniformLocations();
    void ParseUniform(Uniform* uniform);

public:
    GLProgram(Shader shaders[], unsigned int count);
    GLProgram();
    ~GLProgram();

    void Attach(Shader shaders[], unsigned int count);
    void Attach(Shader& shader);
    void Detach(Shader& shader);
    void Reattach();

    void AttachUniform(Uniform& uniform);
    void DeleteUniform(const std::string& identifier);
    void RefreshUniforms();

    void Bind();
    void Unbind();

    inline const std::vector<Shader*> AttachedShaders() const
    {
        return m_AttachedShaders;
    }

    inline const unsigned int GetHandle() const
    {
        return m_RendererID;
    }

    inline int GetUniformLocation(Uniform& uniform) const
    {
        for (unsigned int i = 0; i < m_Uniforms.size(); i++)
        {
            if (&uniform == m_Uniforms[i])
            {
                return *(m_UniformLocations + i);
            }
        }
    }

    inline int GetUniformLocation(std::string& uName) const
    {
        for (unsigned int i = 0; i < m_Uniforms.size(); i++)
        {
            if (m_Uniforms[i]->GetName() == uName)
            {
                return *(m_UniformLocations + i);
            }
        }
    }
};

GLProgram.cpp:

#include "GLProgram.h"

GLProgram::GLProgram(Shader shaders[], unsigned int count)
{
    GLCall(m_RendererID = glCreateProgram());
    Attach(shaders, count);
    Bind();
}

GLProgram::GLProgram()
{
    GLCall(m_RendererID = glCreateProgram());
}

GLProgram::~GLProgram()
{
    GLCall(glDeleteProgram(m_RendererID));
}

void GLProgram::Bind()
{
    GLCall(glUseProgram(m_RendererID));
}

void GLProgram::Unbind()
{
    GLCall(glUseProgram(0));
}

void GLProgram::Attach(Shader& shader)
{
    if (shader.Attachable())
    {
        GLCall(glAttachShader(m_RendererID, shader.GetHandle()));
        m_AttachedShaders.push_back(&shader);
    }
    else
        std::cout << "Shader of type '" << shader.GetType() << "' and handle '" << shader.GetHandle() << "' is not attachable. It was left off the final program with handle '" << m_RendererID << "'" << std::endl;

    LinkProgram();
}

void GLProgram::Attach(Shader shaders[], unsigned int count)
{
    for (unsigned int i = 0; i < count; i++)
    {
        if (shaders[i].Attachable())
        {
            GLCall(glAttachShader(m_RendererID, shaders[i].GetHandle()));
            m_AttachedShaders.push_back(&shaders[i]);
        }
        else
            std::cout << "Shader of type '" << shaders[i].GetType() << "' and handle '" << shaders[i].GetHandle() << "' is not attachable. It was left off the final program with handle '" << m_RendererID << "'" << std::endl;
    }

    LinkProgram();
}

void GLProgram::Detach(Shader& shader)
{
    unsigned int count = m_AttachedShaders.size();

    for (unsigned int i = 0; i < count; i++)
    {
        if (shader.SameInstance(*m_AttachedShaders[i]))
        {
            m_AttachedShaders.erase(m_AttachedShaders.begin() + i);
            GLCall(glDetachShader(m_RendererID, m_AttachedShaders[i]->GetHandle()));
        }
    }

    LinkProgram();
}

void GLProgram::Reattach()
{
    unsigned int count = m_AttachedShaders.size();

    for (unsigned int i = 0; i < count; i++)
    {
        GLCall(glDetachShader(m_RendererID, m_AttachedShaders[i]->GetHandle()));
        GLCall(glAttachShader(m_RendererID, m_AttachedShaders[i]->GetHandle()));
    }

    LinkProgram();
}

void GLProgram::LinkProgram()
{
    GLCall(glLinkProgram(m_RendererID));
    GLCall(glValidateProgram(m_RendererID));

    unsigned int count = m_AttachedShaders.size();

    for (unsigned int i = 0; i < count; i++)
    {
        GLCall(glDeleteShader(m_AttachedShaders[i]->GetHandle()));
    }
}

void GLProgram::AttachUniform(Uniform& uniform)     // Returns location of uniform in program (for modifying uniform data at runtime).
{
    m_Uniforms.push_back(&uniform);
    m_UniformLocations = GetUniformLocations();
    ParseUniform(&uniform);
}

void GLProgram::DeleteUniform(const std::string& identifier)
{
    unsigned int i = 0;

    for (Uniform* u : m_Uniforms)
    {
        if (u->GetName() == identifier)
            m_Uniforms.erase(m_Uniforms.begin() + i);

        i++;
    }

    m_UniformLocations = GetUniformLocations();
}


int* GLProgram::GetUniformLocations()
{
    if (m_UniformLocations != nullptr) free(m_UniformLocations);

    unsigned int count = m_Uniforms.size();
    int* ptr = (int*)malloc(count * sizeof(int));

    for (unsigned int i = 0; i < count; i++)
    {
        GLCall(*(ptr + i) = glGetUniformLocation(m_RendererID, m_Uniforms[i]->GetName().c_str()));
        ASSERT(*(ptr + i) != -1);
    }

    return ptr;
}

void GLProgram::ParseUniform(Uniform* uniform)
{
    unsigned int locationOffset = 0;

    for (Uniform* u : m_Uniforms)
    {
        if (u == uniform)
            break;

        locationOffset++;
    }

    if (locationOffset >= m_Uniforms.size())
    {
        std::cout << "No uniform with identifier '" << uniform->GetName() << "' present. It was not parsed." << std::endl;
        return;
    }

    UniformType type = uniform->GetType();
    void* data = nullptr;

    bool oneValue = type == UniformType::FLOAT || type == UniformType::DOUBLE || type == UniformType::INT;
    bool twoValues = type == UniformType::FLOAT2 || type == UniformType::DOUBLE2 || type == UniformType::INT2;
    bool threeValues = type == UniformType::FLOAT3 || type == UniformType::DOUBLE3 || type == UniformType::INT3;
    bool fourValues = type == UniformType::FLOAT4 || type == UniformType::DOUBLE4 || type == UniformType::INT4;
    bool mat2x2 = type == UniformType::fMAT2x2 || type == UniformType::dMAT2x2 || type == UniformType::iMAT2x2;
    bool mat3x3 = type == UniformType::fMAT3x3 || type == UniformType::dMAT3x3 || type == UniformType::iMAT3x3;
    bool mat4x4 = type == UniformType::fMAT4x4 || type == UniformType::dMAT4x4 || type == UniformType::iMAT4x4;
    bool floatCast = type >= 0 && type <= 6 ? true : false;
    bool doubleCast = type >= 7 && type <= 13 ? true : false;
    bool intCast = type > 13 ? true : false;


    if (oneValue)
    {
        if (floatCast)
        {
            data = uniform->GetData();

            GLCall(glUniform1f(*(m_UniformLocations + locationOffset), *((float*)data)));
        }
        else if (doubleCast)
        {
            data = uniform->GetData();

            GLCall(glUniform1d(*(m_UniformLocations + locationOffset), *((double*)data)));
        }
        else if (intCast)
        {
            data = uniform->GetData();

            GLCall(glUniform1i(*(m_UniformLocations + locationOffset), *((int*)data)));
        }
    }
    else if (twoValues)
    {
        if (floatCast)
        {
            data = uniform->GetData();

            GLCall(glUniform2f(*(m_UniformLocations + locationOffset), *(((float*)data)), *(((float*)data) + 1)));
        }
        else if (doubleCast)
        {
            data = uniform->GetData();

            GLCall(glUniform2d(*(m_UniformLocations + locationOffset), *(((double*)data)), *(((double*)data) + 1)));
        }
        else if (intCast)
        {
            data = uniform->GetData();

            GLCall(glUniform2i(*(m_UniformLocations + locationOffset), *(((int*)data)), *(((int*)data) + 1)));
        }
    }
    else if (threeValues)
    {
        if (floatCast)
        {
            data = uniform->GetData();

            GLCall(glUniform3f(*(m_UniformLocations + locationOffset), *(((float*)data)), *(((float*)data) + 1), *(((float*)data) + 2)));
        }
        else if (doubleCast)
        {
            data = uniform->GetData();

            GLCall(glUniform3d(*(m_UniformLocations + locationOffset), *(((double*)data)), *(((double*)data) + 1), *(((double*)data) + 2)));
        }
        else if (intCast)
        {
            data = uniform->GetData();

            GLCall(glUniform3i(*(m_UniformLocations + locationOffset), *(((int*)data)), *(((int*)data) + 1), *(((int*)data) + 2)));
        }
    }
    else if (fourValues)
    {
        if (floatCast)
        {
            data = uniform->GetData();

            GLCall(glUniform4f(*(m_UniformLocations + locationOffset), *(((float*)data)), *(((float*)data) + 1), *(((float*)data) + 2), *(((float*)data) + 3)));
        }
        else if (doubleCast)
        {
            data = uniform->GetData();

            GLCall(glUniform4d(*(m_UniformLocations + locationOffset), *(((double*)data)), *(((double*)data) + 1), *(((double*)data) + 2), *(((double*)data) + 3)));
        }
        else if (intCast)
        {
            data = uniform->GetData();

            GLCall(glUniform4i(*(m_UniformLocations + locationOffset), *(((int*)data)), *(((int*)data) + 1), *(((int*)data) + 2), *(((int*)data) + 3)));
        }
    }
    else if (mat2x2)
    {
        if (floatCast)
        {
            data = uniform->GetData();

            GLCall(glUniformMatrix2fv(*(m_UniformLocations + locationOffset), 1, uniform->Transpose(), (float*)data));
        }
        else if (doubleCast)
        {
            data = uniform->GetData();

            GLCall(glUniformMatrix2dv(*(m_UniformLocations + locationOffset), 1, uniform->Transpose(), (double*)data));
        }
    }
    else if (mat3x3)
    {
        if (floatCast)
        {
            data = uniform->GetData();

            GLCall(glUniformMatrix3fv(*(m_UniformLocations + locationOffset), 1, uniform->Transpose(), (float*)data));
        }
        else if (doubleCast)
        {
            data = uniform->GetData();

            GLCall(glUniformMatrix3dv(*(m_UniformLocations + locationOffset), 1, uniform->Transpose(), (double*)data));
        }
    }
    else if (mat4x4)
    {
        if (floatCast)
        {
            data = uniform->GetData();

            GLCall(glUniformMatrix4fv(*(m_UniformLocations + locationOffset), 1, uniform->Transpose(), (float*)data));
        }
        else if (doubleCast)
        {
            data = uniform->GetData();

            GLCall(glUniformMatrix4dv(*(m_UniformLocations + locationOffset), 1, uniform->Transpose(), (double*)data));
        }
    }
}

void GLProgram::RefreshUniforms()
{
    for (Uniform* u : m_Uniforms)
    {
        ParseUniform(u);
    }
}

Renderer.h: // Pretty basic GL error handling for now. Contains GLCall macro.

#pragma once
#include <GL/glew.h>

// GL function calls error checking macro.
#ifdef _DEBUG
#define ASSERT(x)\
     if(!(x)) __debugbreak();
#define GLCall(x) GLClearError(); x; ASSERT(GLCheckError(#x, __FILE__, __LINE__))
#else
#define ASSERT(x) ;
#define GLCall(x) x;
#endif

void GLClearError();
bool GLCheckError(const char* functionLog, const char* sourceFile, int line);

Renderer.cpp:

#include "Renderer.h"
#include <GL\glew.h>
#include <iostream>

bool GLCheckError(const char* functionLog, const char* sourceFile, int line)
{
    while (GLenum error = glGetError())
    {
        std::cout << "\n[OpenGL Error " << error << "]: " << functionLog << " : " << sourceFile << " : " << line << std::endl;
        return false;
    }
    return true;
}

void GLClearError()
{
    while (glGetError() != GL_NO_ERROR);
}

And here is the actual shader file containing a simple vertex and fragment shader I am parsing: // Posted this to demonstrate shader file layout.

#shader vertex
#version 330 core

layout(location = 0) in vec4 position;

void main()
{
    gl_Position = position;
}

#shader fragment
#version 330 core

layout(location = 0) out vec4 color;

uniform vec4 u_Color;
uniform vec4 u_Color2;
uniform vec2 u_WindowSize;
uniform float u_SlopeBoundary;
uniform bool u_Switched;

vec2 transformedVec = vec2(gl_FragCoord.x - u_WindowSize.x / 2, gl_FragCoord.y - u_WindowSize.y / 2);
vec2 coordHat = normalize(transformedVec);
float sProductCC = dot(vec2(1, 0), coordHat);
float sProductC = dot(vec2(-1, 0), coordHat);
float angleCC = acos(sProductCC);
float angleC = acos(sProductC);

void main()
{
    if (gl_FragCoord.y > u_WindowSize.y / 2)
    {
        if (!u_Switched)
        {
            if (angleCC < u_SlopeBoundary)
            {
                color = u_Color;
            }
            else
            {
                color = u_Color2;
            }
        }
        else if (angleCC > u_SlopeBoundary)
        {
            color = u_Color;
        }
        else
        {
            color = u_Color2;
        }
    }
    else if (!u_Switched)
    {
        if (angleC < u_SlopeBoundary)
        {
            color = u_Color2;
        }
        else
        {
            color = u_Color;
        }
    }
    else if (angleC > u_SlopeBoundary)
    {
        color = u_Color2;
    }
    else
    {
        color = u_Color;
    }
}

It's a bit of a long one. But excluding the implementation usage in Application.cpp and all the small helper translation units I've posted, the main focus are the files: Uniforms, Shader and GLProgram.

It has been fun writing this as a newbie, but it would be way more fun if I can improve from it. Anyone that would take the time to read this and post some feedback would truly be a sincere help to me.

\$\endgroup\$

2 Answers 2

3
+50
\$\begingroup\$

Shader

Shader::GetSource should probably return a copy of the string (rather than a reference to it). This will prevent any caller from having access to m_Source, and prevent a possible change in value if the returned reference is stored in a reference and m_Source changes after the call.

Using the conditional operator for a bool expression to return true or false is just adding to the size of the code, since a comparison operator will result in the same return value. The definition for SameInstance in Shader.h can be reduced to just

return this == &s;

You're repeating yourself in Shader::Shader. Since all the cases have the same expression for m_Source, this can be removed from the switch and placed after. And m_ShaderType is normally a copy of type, so that can be simplified as well. Finally, you're missing a break in the default case. While not required here (since the default is the last case statement), you can avoid future problems (when a new case is added after the default one) by always including it. Since this is an error condition, adding an ASSERT(0); will help spot these when you are debugging. Put this all together and you end up with:

Shader::Shader(const unsigned int type, const std::string& filepath): m_ShaderType(type)
{
    switch (type)
    {
        case GL_VERTEX_SHADER:
        case GL_FRAGMENT_SHADER:
            break;
        default:
            ASSERT(0);
            std::cout << "Unrecognized shader type. Defaulting to GL_VERTEX_SHADER..." << std::endl;
            m_ShaderType = GL_VERTEX_SHADER;
            break;
    }
    m_Source = Parse(m_ShaderType, filepath);

    GLCall(m_RendererID = glCreateShader(m_ShaderType));

    Compile();
}

While we're on the topic of ASSERT, your ASSERT macro has a problem when used in an if statement where an else can bind improperly. You should make it change into a standalone statement:

#define ASSERT(x) do { if(!(x)) __debugbreak(); } while (0)

In not building debug, it should expand to nothing - not even a semicolon.

Why do you pass type into Shader::Parse? The Shader object already has a type that gives you that info.

Since state is already a bool value, Shader::Compile and Recompile do not need to use the conditional expression: m_Attachable = state;

Uniform

Uniform::GetName: See notes above for Shader::GetSource.

Uniform::GetData checks for a negative m_Type, rather than checking for the explicit INVALID value. This should either just check for m_Type == INVALID, or if you're going to assume that all negative values are invalid, it should also check against the largest legal value as well (which can be added to the UniformType enum).

Uniform::Uniform has lots of code duplication. Start with always assigning m_Type = type (or m_Type(type) in the member initializer list). The same for m_UName and m_Transpose. You should consider always allocating m_Data as an array (m_Data = new float[1]) which will simplify destruction. There's no default error case. This leaves m_Data uninitialized and results in Undefined Behavior (likely a crash) in the destructor. You could make a private CreateData member function to both allocate space then call ChangeData directly with the values (size) you already know, so that the allocator and copier use a single value, rather than having it in multiple places in the code.

In the destructor, you use constants rather than your UniformType enum values. You should always use the enum values. Also many of your deletes are wrong. When you call new float[2], you must use delete []. This why allocating a one element array in the constructor is beneficial. This should use a switch statement instead of the cascading if statements.

SetData is missing a break for the INT2 case.

GLProgram

Consider using std::vector<Shader> rather than passing a pointer and count to the GLProgram constructor and other functions. Some of the member functions could be made const.

Should the destructor clean up various allocated memories (m_UniformLocations)?

In GLProgram::Detach, can there be multiple shaders with the same Instance that need to be detached? If not, break from the for loop after removing the shader. Otherwise you'll access past-the-end of the m_AttachedShaders vector because you remove one but don't reduce the internal count you're using to control the for loop. You could also use iterators rather than an index to search for the instance. Or use std::find with an appropriate comparator.

GLProgram::Reattach can use the for (auto s: m_AttachedShaders) form of the for statement to process all the shaders. LinkProgram would also benefit from this. Should Reattach be calling LinkProgram?

The comment with AttachUniform seems to be incorrect.

DeleteUniform has Undefined Behavior because you modify the m_Uniforms vector while looping with it. Use std::find, break from the loop when you find one, or use iterators if there can be multiple uniforms with the same name that need to be deleted.

GetUniformLocations frees m_UniformLocations but does not NULL out the pointer, not does it store the pointer to the newly allocated memory. When allocating memory for ptr, why are you using malloc and not new? You can use ptr[i] rather than *(ptr + i) to access elements in the for loop.

ParseUniform should use another switch statement rather than creating a bunch of bool variables based on constants (when they should be using the enum values). For any given call, you're only going to be making one GLCall call, so just put that in a switch and don't use multiple nested if statements.

\$\endgroup\$
1
  • \$\begingroup\$ Brilliant feedback! Thank you. I've noted all the major issues especially those regarding memory management. I will fix all that tomorrow. I changed some things around and forgot to delete that one misplaced comment. Lots of stylistic errors in the code as well. I shouldn't have made most of them - in fact, now that I read your remarks about them I am a bit dumbstruck by the amount of wrongly formatted code I was able to conjure :D. I've separated my macros in a different header file and rearranged headers as well. In any case I will wait until tomorrow evening and award you the bounty. :) \$\endgroup\$ Jul 14, 2018 at 21:54
3
\$\begingroup\$

This is a decent start! I think you've done things in a straightforward way that's easy to read and understand. That can sometimes be more valuable than writing something clever or super fast. That said, I think you could make some changes to it that would keep it easy to read and improve its usefulness. I'll outline them below.

Data Types

Due to C-based languages vague descriptions of the built-in types (int, long, float, etc.), when you're working with OpenGL it's best to avoid those types and use the ones that OpenGL specifies. This ensures that everything is the right size. So instead of using int use GLint, and instead of float use GLfloat.

You have Vector4 class in the Application.cpp file. I realize this isn't the focus, but I think this is an important point. In main(), you create vertex data as a straight C-style array of floats. But vertices aren't a 1D array of float. They're a 1D array of vec2, vec3, or vec4 objects. You should create types for these and use them in your code. Vector4 is a good start, but it turns out it's not used anywhere in your code! If you don't want to do the work of making the various classes yourself, you can use an existing library like glm. Also, you should break that main() up into smaller functions. But I digress…

Polymorphism and Templates

One of the great strengths of C++ is its use of polymorphism. Another is its use of templates. I think that your Uniform class is doing too much and that you should use either polymorphism or templates to make the code cleaner.

For example, if you had a base Uniform class that held the name of the uniform, you could subclass it for the various types. For example, you could have an IntUniform class for holding a GLint. You could have a Vec2Uniform class for holding a GLfloat pair. Doing this would eliminate the various long switch statements you have throughout Uniform.cpp (none of which handle a case where an invalid type is passed in).

Another approach would be to make Uniform a template and then have specializations for things like GLint, GLfloat, etc. You would need to have the above mentioned vector structs or classes to handle creating Uniforms for types like vec2, mat4x4, etc.

Avoid C-style arrays and pointers

C-style arrays and pointers can be a source of bugs. C++ has numerous ways to avoid using them, and you should use them. In the GLProgram constructor and Attach() method, you allow a caller to pass in a C-style array of shaders and a count. What happens if the caller screws up the count? What happens if the pointer to the shaders is null? Since you keep them as a std::vector<Shader> internally, why not just have the caller pass in one of those (probably as a const reference to reduce overhead)? With the exception of calls to the C functions that OpenGL supplies, you don't need C arrays anywhere in these classes.

Is a Shader class needed?

In OpenGL shaders tend to be fairly transient things. In most cases the shader string is read from disk, compiled, linked into a program and disposed of. While OpenGL does supply methods of attaching and detaching shaders, in practice I've never seen that used. If you have a specific need for it, then by all means, leave it in. But like I say, I've never used that functionality and I can't see what advantage it provides, so adding a bunch of code to support it seems like a waste.

Additionally, I see a lot of bugs related to Shaders as you currently have them. For example, in GLProgram::LinkProgram() you call glValidateProgram(). But if the program doesn't validate, you'll never know it since you don't ever check the validation state to see if it succeeded. Furthermore, you call glGetError() (in debug) which doesn't give you any information about whether the program is valid. (It instead tells you whether you passed in a program ID that OpenGL actually generated. It says nothing about whether the program it points to is valid.) That's misleading to a user of this code.

Another bug is that GLProgram::LinkProgram() calls glDeleteShader() but it continues to hold onto an array of now invalid shaders. What happens when a caller calls something that tries to attach that shader again, but it's already deleted?

Where Should Uniform Locations Go?

I have mixed feelings about putting an array of uniform locations in GLProgram. (At the least it should be a std::vector<int> rather than an int*.) While an OpenGL program is the object which actually holds the uniform locations, there's no reason you need to mimic that structure in your code. It makes more sense to me for the Uniform object to hold the location. Doing so would completely eliminate the m_UniformLocations array from GLProgram which I think would simplify things. (And consequently it would remove the memory leak that GLProgram has due to you not releasing the uniform locations in the destructor.) (And why are you using malloc and free instead of new and delete in a C++ program?)

Furthermore, I just noticed that your GLProgram::Bind() method only calls glUseProgram(). It doesn't set any of the uniforms. You require the caller to calls RefreshUniforms() which is error prone. I would have GLProgram::Bind() just call it. It would be a rare situation where you want to use a program without setting the uniforms. If you really want to allow that case, I'd have a separate function like GLProgram::BindWithoutUniforms() or something like that.

Overall I think this is pretty good. You're using a lot of the right paradigms. I think you just need to use them more frequently and use older paradigms less frequently.

\$\endgroup\$
3
  • \$\begingroup\$ Thank you so much for the valuable input! For vertex data I actually have separate VertexArray objects that hold vertex buffer layouts and the like, I just did not include it in the code here because it's mainly the work of the tutorial presenter I was following. Although I did assimilate it quite well. I get that C-style pointers and arrays are error prone and not ideal but for some reason I find them interesting to use so I decided to use them. Initially everything in Uniforms was C-style but then I decided to mostly use the 'new' keyword. Haven't tried out smart pointers out yet though. \$\endgroup\$ Jul 14, 2018 at 22:19
  • \$\begingroup\$ I really liked the idea of calling RefreshUniforms() in Bind(). Also I call CompileCheck() in Shader on every compile call to make sure I get the glGetShaderiv log in the console to know if I have a problem in the shader code. Isn't that sufficient or should I do more about it? I will definitely try and refactor uniform location storage because it really is unnecessary allocating such a thing on the heap. I don't know why I did it. For some reason I find memory management fascinating and this is kind of my first program in C++ and I instantly tried using it :D. Guess I got too ahead of myself \$\endgroup\$ Jul 14, 2018 at 22:26
  • \$\begingroup\$ "I get the glGetShaderiv log in the console to know if I have a problem in the shader code. Isn't that sufficient or should I do more about it?" Well, the caller probably wants to know whether it succeeded or not so they can either tell the user they're unable to continue, or, if possible, fix the issue. (Perhaps use a less complicated shader, or a default one?) \$\endgroup\$ Jul 14, 2018 at 23:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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