Simple OpenGL / C++ Game Engine

Question

I come from a C# background and recently have been trying to improve my C++ and OpenGL. I've hacked together a basic 3d engine and am looking for feedback as to how to improve it.

One bit I'm most unsure about is the render loop for multiple objects, I've set it up like this and would love some feedback on it. For instance I've no idea if my use of pointers is optimal.

scene.hpp:

namespace r3d
{
 class scene
 {
 public:
 scene(int width, int height);
 void update();
 void exit();
 void add_object(r3d::game_object *);
 void add_light(r3d::light *);
 GLFWwindow * window;
 r3d::camera * main_camera;
 };
}

This is the main loop in my scene.cpp file:

void scene::update()
{
 time->update();
 // render scene loop
 glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);
 behaviour * script;
 for(auto it = game_objects.begin(); it != game_objects.end(); ++it)
 {
 if(!(*it)->enabled) { continue; }
 script = (behaviour *)(*it)->get_component(constants::BEHAVIOUR);
 if(script)
 {
 script->update();
 }
 }
 mesh_renderer * render_obj;
 for(auto it = game_objects.begin(); it != game_objects.end(); ++it)
 {
 if(!(*it)->enabled) { continue; }
 render_obj = (mesh_renderer *)(*it)->get_component(constants::RENDER_OBJECT);
 // use our shader
 render_obj->material->shader->bind();
 // set camera uniforms
 main_camera->set_uniforms(render_obj->material->shader, (*it)->get_transform());
 // set light uniforms
 lights.front()->set_uniforms(render_obj->material->shader);
 // bind texture
 render_obj->material->bind();
 // bind buffers and draw elements
 render_obj->render();
 }
 glfwSwapBuffers(window);
 glfwPollEvents();
}

shader.hpp:

namespace r3d
{
 class shader
 {
 public:
 enum id { UNLIT_VERT_COLOR, UNLIT_TEXTURE, DIFFUSE };
 shader(id);
 shader(std::string, std::string);
 void bind();
 id shader_id;
 GLuint program;
 GLuint mvp_matrix;
 GLuint view_matrix;
 GLuint model_matrix;
 GLuint texture_sampler;
 GLuint light_world_pos;
 GLuint light_color;
 GLuint light_intensity;
 private:
 void init(std::string, std::string);
 };
}

shader.cpp:

shader::shader(std::string vert, std::string frag)
{
 init(vert, frag);
}
void shader::init(std::string vert, std::string frag)
{
 program = load_shader(vert.c_str(), frag.c_str());
 // Get handles for our uniforms
 mvp_matrix = glGetUniformLocation(program, "MVP");
 view_matrix = glGetUniformLocation(program, "V");
 model_matrix = glGetUniformLocation(program, "M");
 texture_sampler = glGetUniformLocation(program, "Sampler");
 light_world_pos = glGetUniformLocation(program, "LightPosition_worldspace");
 light_color = glGetUniformLocation(program, "LightColor");
 light_intensity = glGetUniformLocation(program, "LightPower");
 glUseProgram(program);
}
void shader::bind()
{
 glUseProgram(program);
}

camera.hpp:

namespace r3d
{
 class camera : public game_object
 {
 public:
 camera();
 camera(int w, int h) : width(w), height(h) {};
 void set_uniforms(r3d::shader *, glm::mat4);
 float fov = 45;
 float near = 0.1f;
 float far = 100.0f;
 int width = 1024;
 int height = 768;
 private:
 glm::mat4 projection;
 glm::mat4 view;
 };
}

camera.cpp:

using namespace r3d;
void camera::set_uniforms(r3d::shader * shader, glm::mat4 model)
{
 view = glm::lookAt(position, position + forward, up);
 projection = glm::perspective(glm::radians(fov), (float)width / (float)height, near, far);
 glm::mat4 mvp = projection * view * model;
 glUniformMatrix4fv(shader->mvp_matrix, 1, GL_FALSE, &mvp[0][0]);
 glUniformMatrix4fv(shader->model_matrix, 1, GL_FALSE, &model[0][0]);
 glUniformMatrix4fv(shader->view_matrix, 1, GL_FALSE, &view[0][0]);
}

light.hpp:

namespace r3d
{
 class light
 {
 public:
 light();
 light(glm::vec3, glm::vec3, float);
 void set_uniforms(r3d::shader *);
 glm::vec3 position;
 glm::vec3 color;
 float intensity;
 };
}

light.cpp:

light::light(glm::vec3 position, glm::vec3 color, float intensity)
{
 this->position = position;
 this->color = color;
 this->intensity = intensity;
}
void light::set_uniforms(r3d::shader * shader)
{
 glUniform3f(shader->light_world_pos, position.x, position.y, position.z);
 glUniform3f(shader->light_color, color.x, color.y, color.z);
 glUniform1f(shader->light_intensity, intensity);
}

mesh_renderer.hpp:

namespace r3d
{
 class mesh_renderer : public component
 {
 public:
 mesh_renderer();
 mesh_renderer(std::string, r3d::material *);
 void render();
 std::vector<unsigned short> indices;
 std::vector<glm::vec3> vertices;
 std::vector<glm::vec2> uvs;
 std::vector<glm::vec3> normals;
 GLuint vertex_array_object;
 GLuint vertex_buffer;
 GLuint uv_buffer;
 GLuint normal_buffer;
 GLuint indices_buffer;
 r3d::material * material;
 };
}

mesh_renderer.cpp:

using namespace r3d;
mesh_renderer::mesh_renderer(std::string model_path, r3d::material * material)
{
 name = "mesh_renderer";
 this->material = material;
 // Read file
 load_mesh(model_path.c_str(), indices, vertices, uvs, normals);
 // Vertex Array Object
 glGenVertexArrays(1, &vertex_array_object);
 glBindVertexArray(vertex_array_object);
 // Vertex buffer
 glGenBuffers(1, &vertex_buffer);
 glBindBuffer(GL_ARRAY_BUFFER, vertex_buffer);
 glBufferData(GL_ARRAY_BUFFER, vertices.size() * sizeof(glm::vec3), &vertices[0], GL_STATIC_DRAW);
 // UV buffer
 glGenBuffers(1, &uv_buffer);
 glBindBuffer(GL_ARRAY_BUFFER, uv_buffer);
 glBufferData(GL_ARRAY_BUFFER, uvs.size() * sizeof(glm::vec2), &uvs[0], GL_STATIC_DRAW);
 // Normal buffer
 glGenBuffers(1, &normal_buffer);
 glBindBuffer(GL_ARRAY_BUFFER, normal_buffer);
 glBufferData(GL_ARRAY_BUFFER, normals.size() * sizeof(glm::vec3), &normals[0], GL_STATIC_DRAW);
 // Indices buffer
 glGenBuffers(1, &indices_buffer);
 glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, indices_buffer);
 glBufferData(GL_ELEMENT_ARRAY_BUFFER, indices.size() * sizeof(unsigned short), &indices[0], GL_STATIC_DRAW);
 // Remove bindings
 glBindBuffer(GL_ARRAY_BUFFER, 0);
 glBindVertexArray(0);
 printf("Add mesh_renderer: %s [indices: %lu]\n", model_path.c_str(), indices.size());
}
void mesh_renderer::render()
{
 // vao
 glBindVertexArray(vertex_array_object);
 // vertex buffer
 glEnableVertexAttribArray(0);
 glBindBuffer(GL_ARRAY_BUFFER, vertex_buffer);
 glVertexAttribPointer(0, 3, GL_FLOAT, GL_FALSE, 0, (void*)0);
 // uv buffer
 glEnableVertexAttribArray(1);
 glBindBuffer(GL_ARRAY_BUFFER, uv_buffer);
 glVertexAttribPointer(1, 2, GL_FLOAT, GL_FALSE, 0, (void*)0);
 // normal buffer
 glEnableVertexAttribArray(2);
 glBindBuffer(GL_ARRAY_BUFFER, normal_buffer);
 glVertexAttribPointer(2, 3, GL_FLOAT, GL_FALSE, 0, (void*)0);
 // index buffer
 glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, indices_buffer);
 glDrawElements(GL_TRIANGLES, indices.size(), GL_UNSIGNED_SHORT, (void*)0);
 glDisableVertexAttribArray(0);
 glDisableVertexAttribArray(1);
 glDisableVertexAttribArray(2);
 glBindVertexArray(0);
}

Full source code is here if anyone wants to look at/review any further:

https://github.com/ryanw3bb/r3d

Any feedback or comments would be hugely appreciated! :)

• hoffmale: It would be really helpful for reviewers if you included the relevant header files, too. As it stands now, we can only guess the types of member variables, which hinders any meaningful analysis.
• zooooom: Cheers, headers added
• user34073: This post is being discussed on meta

Answer 1

class scene
{
 public:
 scene(int width, int height);
 void update();
 void exit();
 void add_object(r3d::game_object *);
 void add_light(r3d::light *);
 GLFWwindow * window;
 r3d::camera * main_camera;
};

It’s odd to indent more levels for the public. I mention this because at first glance I thought it was nested namespaces.

You have members that are raw pointers, but have not declared any destructor, copy constructor, assignment operator. What happens if you write

scene x {2,3};
scene y = x;

?

Look up "Rule of zero/three/five". Always consider the value semantics of your classes. This is something different from what you are used to in C#!

---

The style in C++ is to put the * or & with the type, not the identifier. This is called out specifically near the beginning of Stroustrup’s first book, and is an intentional difference from C style.

---

for(auto it = game_objects.begin(); it != game_objects.end(); ++it)
{
 if(!(*it)->enabled) { continue; }
 script = (behaviour *)(*it)->get_component(constants::BEHAVIOUR);
 if(script)
 {
 script->update();
 }
}

First, use the range-for loop. This should be familiar to you from C# :

for (auto item : game_objects) {

Now I can’t tell if you really want to copy the items or not, so you might mean const auto& there instead?

Don’t define your variables until you are ready to initialize them.
Then, use the define/test idiom so that the variable script is only in scope to use where it is valid!

 if (auto script = get_behavior(item)) {
 script->update();
 }

Notice how I avoided the ghastly furball you had in the original by making a named function to hide that and make sure the cast and parameter are matched, in one place in the code.

For that, you should use component storage that doesn’t require casting! Why can’t you simply have named (typed!) members in the game_object? If you must decouple knowledge of your types here from the game_object code, look into strongly typed ways of doing that. E.g. store a std::any, use a system like the localle objects in the standard library (though that is rather dated), ... I could offer more specific advice if I knew the details.

Where you do cast, never use C-style casts.

auto get_behavior (game_object_t& item)
{
 return static_cast<behavior*>(item->get_component(constants::BEHAVIOR);
}

I don’t know if you need static or reinterpret cast, but assuming you are storing a base class in the collection, you are downcasting to take it out. You really should use dynamic_cast, but that is slow. I suggest though using conditional compilation to turn on dynamic_cast when you are debugging, to validate what you think is happening.

---

 shader(std::string, std::string);
 void init(std::string, std::string);

I wonder why you are taking strings by value. Given your C# background, perhaps by mistake?

The one-line body should be in the header in-line in the class. But as written, init is really the constructor, so move the contents to the constructor and initialize the members rather than default initializing and then assigning over them.

I see that you don’t need a copy of the string at all, and would actually prefer not creating a string to pass at all if you have a plain literal. So I’ll change that to string_view.

 shader::shader (string_view vert, string_view frag)
 : program{ load_shader(vert.c_str(),frag.c_str()) }, // N.B. must be initialized first! ※(注記)
 mvp_matrix{ glGetUniformLocation(program,"MVP") },
 ⋮
 {
 glUseProgram(program);
 }

I don’t see any definition of shader::shader(id) but I suppose it would share most of the same code, right? Use a delegating constructor to have one call the other or both call a private form.

※(注記) Members are initialized in the order in which they are declared in the class, not the order presented in the constructor’s init list.

---

Good luck to you!

Moving to C++, I suggest you learn about value semantics. Constructors, assignment, etc.; both how they are used by the code and how you write them. You will find important differences from things called constructors in C# and Java (and Delphi for that matter).

Links for you:

• https://en.cppreference.com/w/cpp/language/rule_of_three
• https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md

Answer 2

This looks really interesting! If you end up using it for a game, let us know so we can try it out!

Namespaces

You've chosen the namespace r3d. Just be aware that the Red Camera company uses r3d as their file extension and they have an SDK for reading in files from their format. Not sure if they use C++ namespaces, but thought you should know.

Architecture

I notice in scene, you have loops where you're doing:

for (auto it = game_objects.begin(); it != game_objects.end(); ++it)
{
 if (!(*it)->enabled) { continue; }
 //... etc.
}

I'm guessing you use this elsewhere, too. If this is a common task in your engine, it would probably be worth having different types of iterators, such as an "enabled objects" iterator. You're also casting the results to specific types, so it might even be worthwhile to have "enabled behaviors" iterators and "enabled mesh renderer" iterators. Or that might be overkill. It all depends on how you're going to use them and how frequently you need to differentiate them.

Some of your architecture feels inside-out to me. For example, you have a set_uniforms() method on camera and light. Neither cameras nor lights have uniforms (in your classes, in OpenGL, or logically). Shaders have uniforms. So you should be calling render_obj->material->shader->set_uniforms(). (Or perhaps it would be better to have a set_lighting_uniforms() method and a set_camera_uniforms() method.)

I'm also unclear on why some things are left to the scene::update() method and some are in mesh_renderer::render(). For example, you call shader::bind() from scene::update(), but directly call glBindVertexArray(), and glBindBuffer() in mesh_renderer::render(). I would think the render method should set up the fragment and vertex shaders, set their uniforms, set up the various arrays and buffers and call glDrawElements().

Avoid Magic Numbers

You use glGetUniformLocation() to get the locations of each uniform in shader::init(). But you're using hard-coded constants for the vertex attributes. You could use glGetAttributeLocation() to get the locations of your attributes. Then they would have names in your C++ code and be more readable. It would make it easier to spot errors if, for example, you accidentally sent vertices to your normal buffer or vice-versa. Or, if you don't want to do that, at least use named constants instead of hardcoding 0, 1, and 2 to mean vertices, texture coordinates, and normals.

Likewise, when passing in the number of coordinates for each attribute, I would use the type names, like this:

glVertexAttribPointer(vertex_attribute, sizeof(glm::vec3) / sizeof(GLfloat), GL_FLOAT, GL_FALSE, 0, (void*)0);

or better:

glVertexAttribPointer(vertex_attribute, sizeof(vertices[0]) / sizeof(vertices[0].x), GL_FLOAT, GL_FALSE, 0, (void*)0);

This ensures the number of coordinates per element is always correct without hardcoding the value.

Member Access

You've made almost all of your instance variables public. This is generally a bad idea. It means that any code at all can just reach in and change those values. It makes debugging much harder as when you do find an object with a bad value, it becomes impossible to track down where it was set. Most of your instance variables should probably be private. It looks like many of your instance variables are only used within the class in which they're declared, so it probably wouldn't cause much disruption to do this.

Odds and Ends

Your camera class has a width and height. This is odd as a virtual camera has no inherent width and height. It does have an aspect ratio, though, and I see that's all you use width and height for. I would have the constructor just save the ratio rather than separately saving the width and height. This would also avoid the cast in camera::set_uniforms(). (That is, it would move it to the constructor.)

I would also probably store the field of view in radians so it's not converted on every frame. (Not that it's likely to be a performance bottleneck. Just why save the data in a different format than the one you actually need?)

You appear to only be supporting point lights. Directional and spot lights are not too difficult to implement and will give you additional creative options.

• JDługosz: ⟪it would probably be worth having different types of iterators, such as an "enabled objects" iterator.⟫ Indeed! Better yet, make a set of composable range adaptors that can be attached to any collection of game items in a range-for loop. E.g. for(auto& item : game_objects | enabled) { ⋯