Simple rendering system in OpenGL

Question

Recently, I added this 'batch' based rendering system in my game engine, which takes care of rendering Spatials (those are actually wrappers for a mesh and a transform) by giving it an environment parameter (taking care of uniform values, like light, fog, environmentMaps, ...). I would like to ask your advice on how I could improve it.

#pragma once
#include <cstdarg>
#include <algorithm>
#include "ResourceManager.h"
#include "../render/Light.h"
#include "../render/Material.h"
#include "../render/Camera.h"
#include "Spatial.h"
class Environment
{
public:
 Environment()
 {
 }
 Environment& Add(const Spiky::PointLight& light)
 {
 m_pointLights.push_back(light);
 return *this;
 }
 Environment& Add(const Spiky::DirectionalLight& light)
 {
 m_directLights.push_back(light);
 return *this;
 }
 std::vector<Spiky::PointLight> GetPointLights()
 {
 return m_pointLights;
 }
 std::vector<Spiky::DirectionalLight> GetDirectionalLights()
 {
 return m_directLights;
 }
private:
 //ShadowMap shadowMap; // those will be implemented later...
 //Clear color;
 //Fog fog;
 std::vector<Spiky::PointLight> m_pointLights;
 std::vector<Spiky::DirectionalLight> m_directLights;
};
struct Renderable
{
 Spiky::Mesh mesh;
 Environment environment;
 const glm::mat4 worldTransform;
 Spiky::Material material;
 GLenum primitiveType;
 Spiky::Shader shader;
 //glm::mat4[] bones;
 Renderable(Spiky::Mesh mesh, const Environment& environment, const glm::mat4& world_transform,
 const Spiky::Material& material, GLenum primitive_type = GL_TRIANGLES, Spiky::Shader shader = Spiky::ResourceManager::GetShader("__STANDART_SHADER"))
 : mesh(mesh),
 environment(environment),
 worldTransform(world_transform),
 material(material),
 primitiveType(primitive_type),
 shader(shader)
 {
 auto uniformBlockIndex = glGetUniformBlockIndex(shader->GetProgram(), "Viewport");
 glUniformBlockBinding(shader->GetProgram(), uniformBlockIndex, 0);
 }
};
class ModelBatch
{
public:
 ModelBatch()
 : 
 m_UBO_Viewport(GL_NONE)
 {
 glGenBuffers(1, &m_UBO_Viewport);
 glBindBuffer(GL_UNIFORM_BUFFER, m_UBO_Viewport);
 glBufferData(GL_UNIFORM_BUFFER, 3 * sizeof(glm::mat4) + sizeof(glm::vec2), nullptr, GL_STATIC_DRAW);
 glBindBuffer(GL_UNIFORM_BUFFER, 0);
 glBindBufferRange(GL_UNIFORM_BUFFER, 0, m_UBO_Viewport, 0, sizeof(glm::mat4));
 }
 void Begin(const Spiky::Camera& camera)
 {
 //Clear all render targets
 m_renderTargets.clear();
 m_camera = camera;
 //Update UBOs --> Common Uniforms
 glBindBuffer(GL_UNIFORM_BUFFER, m_UBO_Viewport);
 glBufferSubData(GL_UNIFORM_BUFFER, 0, sizeof(glm::mat4), glm::value_ptr(camera.Projection()));
 glBufferSubData(GL_UNIFORM_BUFFER, sizeof(glm::mat4), sizeof(glm::mat4), glm::value_ptr(camera.View()));
 glBufferSubData(GL_UNIFORM_BUFFER, 2 * sizeof(glm::mat4), sizeof(glm::mat4), glm::value_ptr(camera.Combined()));
 glBufferSubData(GL_UNIFORM_BUFFER, 3 * sizeof(glm::mat4), sizeof(glm::vec2), glm::value_ptr(glm::vec2(1000.0, 900.0)));
 glBindBuffer(GL_UNIFORM_BUFFER, 0);
}
void Render(const Spiky::Spatial& spatial, const Environment& environment)
{
 m_renderTargets.push_back(Renderable(spatial.GetMesh(), environment,
 spatial.GetTransform().GetModel(), spatial.GetMaterial()));
}
void End() const
{
 for (auto target : m_renderTargets)
 {
 target.shader->Bind(); 
 target.shader->UpdateUniform3fv("cameraPosition", glm::value_ptr(m_camera.GetPosition()));
 target.shader->UpdateUniformMatrix4f("model", target.worldTransform);
 target.shader->UpdateUniformMatrix3f("normalMatrix",
 glm::inverseTranspose(glm::mat3(target.worldTransform))
 );
 target.environment.GetPointLights()[0].SetUniforms(target.shader);
 target.environment.GetPointLights()[0].SetUniforms(target.shader);
 target.material.SetUniforms(target.shader);
 target.material.Bind(target.shader);
 target.mesh->Render();
 }
}
private:
 Spiky::Camera m_camera;
 std::vector<Renderable> m_renderTargets;
 GLuint m_UBO_Viewport;
};

main()

#include "../render/render.h"
#include "../core/core.h"
#include "../physics/physics.h"
using namespace Spiky;
int main(int argc, char** args)
{
 Window window(200, 50, 1024, 900, "Spiky engine - release alpha 0.0", true, false);
 window.SetMouseVisible(false);
 Environment environment;
 ModelBatch batch;
 //more Init code...
 //here is the render loop : 
 util::InMainLoop(200, isRunning,
 [&](float delta)
 {
 //Update code...
 },
 [&]()
 {
 window.SwapBuffers();
 window.Clear(0.0f, 0.0f, 0.0f, 0.0f);
 //here, it performs render : 
 batch.Begin(camera);
 batch.Render(woodFloor, environment);
 batch.End();
 },
 FPS
 );
 return EXIT_SUCCESS;
}

Answer 1

The expression target.environment.GetPointLights()[0].SetUniforms(target.shader); appears twice in a row.

Is this on purpose? Can you be sure the vector of point lights always has at least one element?

If it doesn't, your code is invalid. If there are multiple lights, won't they set the same uniforms? There's not enough information about Spiky::PointLight to know this.

Also, a small note about the C++11 auto keyword: In the for loop for (auto target : m_renderTargets), you're creating a copy of each of your targets and working on that copy. Try using auto& or even better const Renderable&.

In my experience, the few keystrokes you save by typing auto are not worth the potential pain of fixing the possible compile errors when the type is not what you expect. Writing the actual type will produce a compilation error at that line if the type stored in the vector is not what you expected and also benefits the reader, who no longer has to refer back to the class definition to remember what type you're trying to use.

• SirPython: Welcome to Code Review! Good job on your first answer.
• Mat: Strange advice about auto in the range-based for. auto const& would be better assuming const is desired (which it probably isn't here, so auto&. You'll get compiler errors in the body if the collection passed in has the wrong type of objects (highly unlikely).
• MattMatt: target.environment.GetPointLights()[0].SetUniforms(target.shader); is actually just a test line of code.. I was just experimenting with some GLSL stuff
• Filipp: auto& should work just fine because it infers the type from dereferencing the iterator. That range for loop is inside a const function, therefore m_renderTargets is a const vector. Calling begin() on that vector returns a const iterator and dereferencing it returns a const reference. In my experience, auto& should do the right thing. That said, it would still be better to be more explicit. const auto& or auto const& would be better, but writing out the concrete type is best.