Is this is a good renderer implementation?

Hi there! I’m pretty new to OpenGL and I’m currently working on a small game engine to use as a code example to show employers. I’m working in C++ and have written a renderer implementation that provides a method to render a mesh. I’m looking for advice on how to improve the implementation and to point out any glaring mistakes I have made!

Here are the class .h and .cpp files:


#pragma once

#include <iostream>
#include "GL\glew.h"
#include "GL\freeglut.h"
#include "glm\vec4.hpp"
#include "Structures\Mesh.h"
#include "Structures\Texture.h"
#include "Structures\Shader.h"

namespace Core
   // A class that offers functionality to render types of objects given
   // that objects definition and transform. In general it can be used 
   // to render 3D meshes using OpenGL shaders. This is an OpenGL specific
   // implementation for this project.
   class Renderer
      // Creates a new renderer with a specific background colour, this 
      // is the colour displayed behind rendered objects in the view.
      Renderer(const glm::vec4& backgroundColour);

      // Initialises the renderer by setting up the OpenGL environment. 
      virtual void Initialise() const;

      // Sets the renders matrix mode, defaults to projection. 
      virtual void SetupMatrixMode() const;

      // Clears the render to the background colour set when the renderer
      // was constructed
      virtual void Clear() const;

      // Used to facilitate double buffering
      virtual void SwapBuffers() const;

      // Renders a given mesh with a texture and a shader. The projection
      // and view transforms are supplied to specify the position and orientation 
      // of the render view and the model transform specifies the position,
      // orientation and scale of the mesh.
      virtual void RenderMesh(
         const Structures::Mesh& mesh, 
         const Structures::Texture& texture,
         const Structures::Shader& shader,
         const glm::mat4& projectionTransform,
         const glm::mat4& viewTransform,
         const glm::mat4& modelTransform

      glm::vec4 backgroundColour;

      // This variable is supplied to optimise the render code, if the same
      // shader program is needed more than once repeatedly it is not 
      // reassigned to OpenGL unnecessarily. 
      GLuint lastUsedShaderProgram;


 #include "Renderer.h"

namespace Core
   Renderer::Renderer(const glm::vec4& backgroundColour) :
      glutInitDisplayMode(GLUT_DEPTH | GLUT_DOUBLE | GLUT_RGBA); // DEPTH: use depth buffer, DOUBLE: double render to avoid flicker, RGBA: colour channel

   void Renderer::Initialise() const 
      glewInit(); // Must be called after window has been initialized


      glEnable(GL_DEPTH_TEST); // turn on depth testing using the graphics card
      glPolygonMode(GL_FRONT, GL_FILL);

      // Enable alpha blending

      glFrontFace(GL_CW); // Can be used to specify face vertex ordering

   void Renderer::SetupMatrixMode() const 

   void Renderer::Clear() const 
      glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT); // Clear colour and depth bits
      glClearColor(backgroundColour.x, backgroundColour.y, backgroundColour.z, backgroundColour.w); // Clear to black

   void Renderer::SwapBuffers() const 

   void Renderer::RenderMesh(
      const Structures::Mesh& mesh, 
      const Structures::Texture& texture,
      const Structures::Shader& shader,
      const glm::mat4& projectionTransform,
      const glm::mat4& viewTransform,
      const glm::mat4& modelTransform
      // Bind vertices

      // Setup program
      GLuint currentShaderProgram = shader.GetProgram();
      if(currentShaderProgram != lastUsedShaderProgram)
      lastUsedShaderProgram = currentShaderProgram;

      // Apply the texture to the shader texture sampler
      glBindTexture(GL_TEXTURE_2D, texture.GetTextureObject());
      glUniform1i(shader.GetTextureSampler(), 0);

      // Calculate transformation
      glm::mat4 mvpTransform = projectionTransform * viewTransform * modelTransform;

      // Apply transformation
      GLuint shaderMvpTransform = shader.GetMvpTransform();
      glUniformMatrix4fv(shaderMvpTransform, 1, GL_FALSE, &mvpTransform[0][0]);

      // Draw
      glDrawArrays(GL_TRIANGLE_STRIP, 0, mesh.GetVertices().size()); // GL_LINE_LOOP for wire mesh, GL_TRIANGLES for filled mesh

Apologies if this is too much of a code dump! I’ve not included the mesh, texture and shader classes with this as things would start to get a little out of hand.

Your comments are saying what you’re doing rather than why you’re doing it. We know what you’re doing - we can just read the code to find that out.

Your glClear and glClearColor are in the wrong order.

You have homegrown state change filtering on glUseProgram but you don’t have it on textures, buffers, VAOs - that’s going to prompt some questioning.

Your glUniform1i(shader.GetTextureSampler(), 0); only needs to be called once - when the program object is created. I’d suggest using “program” rather than “shader” here to keep terminology consistent; a “shader” in OpenGL is something else. It’s also unclear how expensive (or not) the shader.Get* calls are.

You’re using an odd-looking combination of the old built-in fixed-function matrix stack and a matrix library with uniforms - to avoid confusion you should standardise on one or the other.

The primitive mode in your glDrawArrays call is different to that in the comment.

Much of this is nitpicking, true, but it’s still simple stuff that you should be getting right. The rest of the renderer implementation - it’s neither good nor bad; it’s quite basic and is the bare minimum I’d expect from a “hello triangle” type program.

Thank you mhagain. These are the kind of critical notes I’m looking for and I’ve already learnt a lot from what you’ve told me! I think part of my issue is that I’m piecing together the renderer using examples from several not great tutorials. Could you suggest a good tutorial that explains the sort of details you’ve mentioned?