OpenGL-based library

I've been working, for a month, on an OpenGL library called alpha++. Basically, it is a framework that allows the user to create 3D scenes easily.

I would like to ask your advice on how I could make the user interface simpler. Here is the way I use my classes at the moment in Main.cpp:

Main.cpp

```
/// _______ _ _______ _______ _ _
/// ( ___ ( \ ( ____ |\ /( ___ ) ( ) ( )
/// | ( ) | ( | ( )| ) ( | ( ) | | | | |
/// | (___) | | | (____)| (___) | (___) |__| |__ __| |__
/// | ___ | | | _____| ___ | ___ (__ __(__ __)
/// | ( ) | | | ( | ( ) | ( ) | | | | |
/// | ) ( | (____/| ) | ) ( | ) ( | | | | |
/// |/ \(_______|/ |/ \|/ \| (_) (_)

/*
PLEASE IGNORE : this is a test file for the alpha++ library !
*/

#include
#include
#include
#include "utility.h"
#include "Shader.h"
#include "Window.h"
#include "Vertex.h"
#include "Mesh.h"
#include "Texture.h"
#include "Material.h"
#include "Transform.h"
#include "camera.h"
#include "ModelLoader.h"

#define WIDTH 1000.0
#define HEIGHT 900.0
#define FPS 120

using namespace alpha;

static volatile bool isRunning = false;

struct PhongLight{
glm::vec3 position;
glm::vec3 intensities; /// color of light
};

int main(int argc, char **argv)
{
Window window(300, 100, WIDTH, HEIGHT, "Oppa Win - e dooooow !", true);

///managing textures
Material material;
material.AddTexture("grid_metal", new Texture("res/Metal4.jpg", GL_TEXTURE_2D, GL_LINEAR));
material.AddTexture("modern_metal", new Texture("res/Metal5.jpg", GL_TEXTURE_2D, GL_LINEAR));

Mesh mesh("res/Test.obj");

///Shaders...
Shader shader;
shader.loadFromFile(GL_VERTEX_SHADER, "res/basicShader.glslv");
shader.loadFromFile(GL_FRAGMENT_SHADER, "res/phongShader.glslf");
shader.CreateAndLink();

shader.Bind();
shader.RegisterAttribute("position");
shader.RegisterAttribute

Note: I'll probably be using some C++11 and C++14 features, so if you can't use such features, just ignore them.

No raw new.

material.AddTexture("grid_metal", new Texture("res/Metal4.jpg", GL_TEXTURE_2D, GL_LINEAR));

There is no need to use raw new here.
Either refactor your Texture class to contain a pointer to some TextureImpl class,
or use C++14 std::make_unique to create a std::unique_ptr.

For example:

material.AddTexture("grid_metal", std::make_unique("res/Metal4.jpg", GL_TEXTURE_2D, GL_LINEAR));

Use RAII (or something) for shader binding.

First, why unbind the shader at all?
You can't draw anything without a shader,
and binding a shader automatically unbinds the old shader,
so I see no need for call to shader.UnBind().

But, if you want to guarantee the shader gets unbound, I'd have the library make the guarantee, not the user.

One way would be to pass a function to some Shader::UseWith method that guarantees a call to UnBind.

Example using C++11 lambdas:

void Shader::UseWith(std::function func) {
    this->Bind();
    func();
    this->UnBind();
}

// inside main()
{
    shader.UseWith([&]{
        shader.RegisterAttribute("position");
        // etc.
    });
}

What purpose do RegisterAttribute and RegisterUniform serve?

I assume you're using glGetAttribLocation and glGetUniformLocation,
and storing the results in some std::map?

This is a needless burden on the user of your interface.

Right after you link your shader, you can use glGetActiveAttrib and glGetActiveUniform to enumerate all of your attributes and uniforms.
This would allow you to "register" everything for the shader all in one place.

Making this change would allow you to just remove all calls to RegisterUniform, with no other changes to the interface.

Another issue with RegisterX is that the user could mistakenly call these methods twice with the same parameter.
What happens if the user calls shader.RegisterAttribute("position") twice?

Encapsulate glUniform* and glBindAttribLocation

What's the point of wrapping shader programs in Shader if all it does is compile them and cache uniform locations?

Shader should provide some overloaded methods or method templates to mirror these functions.

Either of these could work:

shader.SetUniform("time", delta);
shader.Uniform("time").Set(delta);

Hide the main loop

Things such as framerate counters, sleeping, and buffer swapping should be hidden behind the interface.

A good way to do this is move the whole main loop into the library, and have it call a user function.

Example:

void Window::MainLoop(std::function userFunc)
{
    ///LOOP REALTED THINGS
    float delta = 0.0f;
    bool isRunning = true;
    unsigned frameRate = 1000 / FPS;
    Uint32 atStart = 0, atEnd = 0, elapsed = 0;

    while (isRunning)
    {
        atStart = SDL_GetTicks();
        Clear(0.0f, 0.0f, 0.0f, 0.0f);

        userFunc(delta); // HERE //

        delta  += 0.002;

        window.SwapBuffers();
        window.Update();

        isRunning = !window.isCloseRequested();

        ///FPS COUNT
        atEnd = SDL_GetTicks();
        elapsed = atEnd - atStart;

        ///SLEEP
        if(elapsed < frameRate)
            SDL_Delay(frameRate - elapsed);
    }
}

// inside main()
{
    window.MainLoop([&](float delta)
    {
        ///Updates
        transform.GetRot() = glm::vec3(.0, delta * 500, .0);

        modelView = transform.GetModelView();
        projectionView = camera.GetViewProjection();
        modelViewProjection =  projectionView * modelView;

        ///Rendering
        shader.Bind();
            // etc.
        shader.UnBind();
    });
}

Getters shouldn't return mutable references.

transform.GetRot() = glm::vec3(.0, delta * 500, .0);

For what purpose? Why not just expose transform.rotation (or whatever it's called)?

If you want to encapsulate it with methods,
then GetRot() should return a const glm::vec3&,
and there should be a SetRot(const glm::vec3&) method to match it.

Other nitpicks

• Don't use #define for constants in C++. static const works fine, or constexpr in C++11.

• Why is isRunning volatile and global? That sounds like a bad idea.

• The name Material seems weird. I would expect something like TextureManager.

• #include instead of #include

• Most of your headers are UpperCase, but "utility.h" and "camera.h" aren't.

• Why is frameRate an unsigned? Just make it an int. Shouldn't it be a double anyway?

Refactoring GetUniformLocation

To help with removing the need for RegisterUniform, I would refactor GetUniformLocation to look something like this:

GLuint Shader::GetUniformLocation(const char* name) {
    auto iter = uniforms.find(name);
    if (iter == uniforms.end()) {
        GLuint loc = glGetUniformLocation(/* program */, name);
        if ( loc == -1 ) {
            std::clog second;
}

StackExchange Code Review Q#93625, answer score: 5