4
\$\begingroup\$

I recently added this ShaderBatch to my engine, which takes care of rendering objects which share the same render technique (shader), like billboards and lights. I would like you to point out if there are any relevant changes I could make to my class.

PS: Is the term "ShaderBatch" appropriate ?

ShaderBatch.h:

/*
 * ShaderTechnique.h
 *
 *  Created on: Aug 9, 2015
 *      Author: mattmatt
 */

#pragma once

#include "alpha/Shader.h"
#include "alpha/Light.h"
#include "alpha/Camera.h"
#include "alpha/Transform.h"
#include <glm/glm.hpp>

class ShaderBatch {
public:
    ShaderBatch();
    virtual ~ShaderBatch(){}

    void SetMatrices(const Camera& camera, const Transform& transform);

    void Begin();
    void End();
protected:
    Shader m_shader;
    Camera m_camera;
    Transform m_transform;
};

class LightBatch : public ShaderBatch{
public:
    LightBatch();
    virtual ~LightBatch(){}

    void SetMaterial(Material material){
        m_material = material;
    }
    void SetPointLight(PhongLight light){
        m_light = light;
    }
    void SetCameraPosition(glm::vec3 pos){
        m_camPosition = pos;
    }

    void Begin();
    void End();

private:
    Material m_material;
    PhongLight m_light;
    glm::vec3 m_camPosition;
};

ShaderBatch.cpp:

#include "ShaderBatch.h"

ShaderBatch::ShaderBatch(){}

void ShaderBatch::SetMatrices(const Camera& camera, const Transform& transform){
    m_camera = camera;
    m_transform = transform;
}

void ShaderBatch::Begin(){
     m_shader->Bind();
         m_camera.SetUniforms(m_shader, m_transform);
}

void ShaderBatch::End(){}

LightBatch::LightBatch()
    : ShaderBatch(),
      m_material(CreateTexture("res/images/Debug.png")),
      m_light(PhongLight()),
      m_camPosition(glm::vec3(0.0, 0.0, 0.0))
{
    ShaderBatch::m_shader = CreateShader("res/shaders/basicShader.glslv", "res/shaders/phongShader.glslf");
}

void LightBatch::Begin(){
    ShaderBatch::Begin();
        m_material.Bind(ShaderBatch::m_shader);
            m_material.SetUniforms(ShaderBatch::m_shader);
            m_light.SetUniforms(m_shader);
}

void LightBatch::End(){}

main.cpp:

#include "alpha.h"

#include <btBulletCollisionCommon.h>
#include <btBulletDynamicsCommon.h>

#include "ShaderBatch.h"

static const unsigned WIDTH = 1000.0;
static const unsigned HEIGHT = 900.0;

int main(int argc, char **args){

    Window window(200, 10, WIDTH, HEIGHT, "Alpha engine - release alpha 0.0", true, false);
    window.SetMouseVisible(false);

    Mesh box1 = CreateMesh("res/models/Terrain.obj");

    Shader shader = CreateShader("res/shaders/basicShader.glslv", "res/shaders/phongShader.glslf");

    LightBatch batch;

    Camera camera(glm::vec3(0, 5.0, 2.5), 70.0f, (float)window.GetWidth() / (float)window.GetHeight(), 0.01, 1000.0f);
    Transform transform;
    InputHandler input = InputHandler();
    PhongLight phongLight;
    phongLight.SetPosition(glm::vec3(-1.0f, 5.0f, 1.0f));
    phongLight.SetIntensity(glm::vec3(1.0f, 1.0f, 1.0f));
    phongLight.SetAttenuation(0.01);

    Material material(CreateTexture("res/images/Terrain.jpg"), CreateTexture("res/images/TerrainNormalMap.jpg"));

    bool isRunning = true;
    unsigned FPS = 0;

    transform.GetPos() -= glm::vec3(.0, 8.5, .0);
    transform.GetScale() *= 7;

    util::InMainLoop(2000, isRunning,
    [&](float delta){
        window.Update();
        input.Update();
        isRunning = !(input.CloseRequested() || input.KeyDown(SDL_SCANCODE_ESCAPE));


        if(!input.KeyDown(SDL_SCANCODE_TAB)){
            util::ApplyFreeFPSLook(camera, input, window, delta, 0.01, 5.0, glm::ivec2(WIDTH / 2, HEIGHT / 2));
            window.SetMouseVisible(false);
        }
        else
            window.SetMouseVisible(true);


        if(input.KeyDown(SDL_SCANCODE_RIGHT))
            phongLight.SetPosition(phongLight.GetPosition() += glm::vec3(0.01, 0.0, 0.0));
        if(input.KeyDown(SDL_SCANCODE_LEFT))
            phongLight.SetPosition(phongLight.GetPosition() -= glm::vec3(0.01, 0.0, 0.0));
        if(input.KeyDown(SDL_SCANCODE_UP))
                    phongLight.SetPosition(phongLight.GetPosition() += glm::vec3(0.0, 0.01, 0.0));
        if(input.KeyDown(SDL_SCANCODE_DOWN))
            phongLight.SetPosition(phongLight.GetPosition() -= glm::vec3(0.0, 0.01, 0.0));


        if(input.KeyDown(SDL_SCANCODE_N))
            material = Material(CreateTexture("res/images/Terrain.jpg"),CreateTexture("res/images/TerrainNormalMap.jpg"));
        if(input.KeyDown(SDL_SCANCODE_P))
            material = Material(CreateTexture("res/images/Terrain.jpg") ,CreateTexture("res/images/DefaultNormalMap.jpg"));
        if(input.KeyDown(SDL_SCANCODE_F))
            material = Material(CreateTexture("res/images/Checkered.png") ,CreateTexture("res/images/TerrainNormalMap.jpg"));
        if(input.KeyDown(SDL_SCANCODE_TAB) && input.KeyDown(SDL_SCANCODE_L))
            glDrawMode(GL_LINES);
        if(input.KeyDown(SDL_SCANCODE_TAB) && input.KeyDown(SDL_SCANCODE_S))
                glDrawMode(GL_TRIANGLES);
        if(input.KeyDown(SDL_SCANCODE_TAB) && input.KeyDown(SDL_SCANCODE_P))
                glDrawMode(GL_POINTS);

    },
    [&](){
            window.Clear(.0f, .0f, .0f, .0f);
            /*
            shader->Bind();
            camera.SetUniforms(shader, transform);
            material.SetUniforms(shader);
            phongLight.SetUniforms(shader);

            material.Bind(shader);
*/
            batch.SetMatrices(camera, transform);
            batch.SetMaterial(material);
            batch.SetPointLight(phongLight);
            batch.Begin();

            box1->Draw();

            window.SwapBuffers();
    },
    FPS
    );
    return EXIT_SUCCESS;///YEHE ! EVERYTHING WENT RIGHT !

}
\$\endgroup\$
2
+50
\$\begingroup\$

Polling versus events

In the update function you're polling the state. This means that every single frame, you're requesting state. In this case, it's about a few keys but if you have support for multiple players and controllers, this method is going to get computationally expensive. Responding on events is a little more complex, but worth considering for larger projects. Have a look at events in sdl. The same happens in the render function, where you set the material every single frame. Why not set the material when the material changes, so in the update? It is weird that a render function changes materials!

Structure

It may be that copy/pasting your code may have changed it a little, but at certain places you have more spaces than others, sometimes the { is on a newline, sometimes it is on the same, the destructor is in the headers while the constructor is not, ...

C versus C++

The style of CreateShader that returns a shader struct is a C-style construct. The C++ way of doing the same would be to give the Shader class a constructor that does the same as CreateShader.

Const references & move operators

If you pass a parameter by value it is copied over. So that means that every time someone calls SetMaterial, a copy of a Material object is created. The same for parameters in setters and constructors. To prevent useless copying, use const references or move operators where possible.

Remove dead/useless code

There is an end function, but it is never called nor implemented. So just remove it, and perhaps rename begin. (There is no use in creating functions because you think it may be useful in the future).

Virtual/Override

Mark functions you are going to overload as such. Mark the function in the base class virtual and add the override keyword in the derived class. This will help the compiler catch errors.

virtual void ShaderBatch::Begin();
virtual void LightBatch::Begin() override;

ShaderBatch

The naming seems to suggest a collection of meshes to be rendered with the same shader. Currently it is used to store the settings. For me personally, it would sound more logical if there is a list of items to render in that batch, and a call to a render function does all logic required to set-up and render all registered items. (e.g. store box1 (that's a bit of an odd name for a terrain, isn't it?) inside of a list inside of the ShaderBatch)

\$\endgroup\$
  • \$\begingroup\$ 1°CreateShader() is actually more complex than you might think : actually, 80% of "classes" are in fact hidden smart pointers(shared_ptr) : the Shader class, for instance is defined as : typdef Shader shared_ptr<CShader>; CreateShader actually returns a new shared_ptr : there is no other way around ! All objects (like CShader) have their assignement operators disabled because of the fact that opengl is big state machine which was written in C (except for opengl 4.5+). \$\endgroup\$ – MattMatt Aug 16 '15 at 18:37
  • \$\begingroup\$ 2° Ok, I'll remove End(); will replace it with Execute(std::function<void>)... \$\endgroup\$ – MattMatt Aug 16 '15 at 18:42
  • \$\begingroup\$ 3° Sorry for the odd name :°) was playing with blender terrain, and did not change the actual name of the var., just the file value... And for the render by list, how would you do it exactly ? I give the ShaderBatch an std::vector at render, and render all primitives by getting their transform components ? But what if there are Lights involved ? Should I then pass those by Get/Sets to member vars ? \$\endgroup\$ – MattMatt Aug 16 '15 at 18:47
  • \$\begingroup\$ Is it ok like his ? \$\endgroup\$ – MattMatt Aug 17 '15 at 12:15
  • \$\begingroup\$ 1) Even if CreateShader returns a shared pointer, why not put it in a resourceManager class that manages all shaders, so that the same shader doesn't get instantiated twice? \$\endgroup\$ – Miklas Aug 18 '15 at 19:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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