current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

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 <stdlib.h>
#include <glm/glm.hpp>
#include <glm/gtc/type_ptr.hpp>
#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("texCoord");
        shader.RegisterAttribute("normal");
        glBindAttribLocation(shader.GetProgramID(), shader.GetAttribLocation("position"), "position");
        glBindAttribLocation(shader.GetProgramID(), shader.GetAttribLocation("texCoord"), "texCoord");
        glBindAttribLocation(shader.GetProgramID(), shader.GetAttribLocation("normal"), "normal");
        shader.RegisterUniform("modelView");
        shader.RegisterUniform("MVP");
        shader.RegisterUniform("time");
        shader.RegisterUniform("lightPosition");
        shader.RegisterUniform("lightIntensities");
        ///shader.RegisterUniform("lightDirection"); used for basic Fragment Shader
    shader.UnBind();

    Camera camera(glm::vec3(0, 0, -4), 70.0f, (float)window.GetWidth() / (float)window.GetHeight(), 0.01, 1000.0f);
    Transform transform;

    /// ///////////////////UNIFORMS //////////////////////// //
    glm::mat4 modelView;
    glm::mat4 projectionView;
    glm::mat4 modelViewProjection;
    glm::vec3 lightDirection = glm::vec3(0.5, 0.5, 0.5);
    glm::vec2 screenResolution = glm::vec2(WIDTH, HEIGHT);
    PhongLight phongLight;
    phongLight.position = glm::vec3(4.0, 4.0, -4.0);
    phongLight.intensities = glm::vec3(0.5, 0.5, 0.5);

    ///LOOP REALTED THINGS
    float delta = 0.0f;
    isRunning = true;
    unsigned frameRate = 1000 / FPS;
    Uint32 atStart = 0, atEnd = 0, elapsed = 0;

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

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

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

        ///Rendering
        shader.Bind();
            material.GetTexture("grid_metal")->Bind(0);
                glUniformMatrix4fv(shader.GetUniformLocation("MVP"), 1, GL_FALSE, glm::value_ptr(modelViewProjection));
                modelView = glm::mat4(1);///Light point doe not move !
                glUniformMatrix4fv(shader.GetUniformLocation("modelView"), 1, GL_FALSE, glm::value_ptr(modelView));
                glUniform1f(shader.GetUniformLocation("time"), delta);
                ///glUniform3fv(shader.GetUniformLocation("lightDirection"), 1, glm::value_ptr(lightDirection));
               /// used for the basic fragment shader !
                glUniform3fv(shader.GetUniformLocation("lightPosition"), 1, glm::value_ptr(phongLight.position));
                glUniform3fv(shader.GetUniformLocation("lightIntensities"), 1, glm::value_ptr(phongLight.intensities));

            mesh.Draw();
        shader.UnBind();

        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);

    }
    return EXIT_SUCCESS;///YEHE ! EVERYTHING WENT RIGHT !

}
share|improve this question
    
You are misusing the volatile qualifier. Read about it here. –  glampert Jun 15 at 21:25
    
Yes, I know ! But my main loop is not finished yet, and I want to run the main loop on a seperate thread ( like in java ) ! –  MattMatt Jun 16 at 15:41

1 Answer 1

up vote 5 down vote accepted

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<Texture>("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<void()> 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<std::string, GLuint>?

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<void(float)> 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 <cstdlib> instead of #include <stdlib.h>
  • 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 << "Warning: Uniform " << name << " is inactive!" << std::endl;
        }
        iter = uniforms.insert({name, loc}).first;
    }
    return iter->second;
}
share|improve this answer
    
1) Never mind about C++11 and 14 : I use C++11 features very frequently ! –  MattMatt Jun 15 at 16:40
    
2) if the user calls register twice, an ALPHA_REGISTERED_TWICE exception is thrown ! –  MattMatt Jun 15 at 16:42
    
4) Register actually gives the user the guarantee that the uniforms exists : if they do not, an ALPHA_UNIFORM_NOT_FOUND exception gets thrown ! –  MattMatt Jun 15 at 16:46
    
5) Should I then encapsulate glUniform and glAttribute into two diff. structures ? Then there would be a method which retrn that structure, and I can update it ! –  MattMatt Jun 15 at 16:49
    
3) I know : i just hacked things together for the main loop ! I know that my loop is really really bad : for instance, the updates a relative to the FPS count ! –  MattMatt Jun 15 at 17:01

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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