• 1

• 11.9k
• 1
• 20
• 46

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.