C++ OpenGL Debug Utility (Completed)

Question

So I had posted C++ OpenGL Debug Utility before to get some feedback on it, this is a more complete version of the class. (Again, looking for feedback/critiquing of any sort. {As I'm in the early stages of learning C++ and don't want to pick up any bad practices / poor coding styles.})

#pragma once
#include <vector>
#include <GL/glew.h>
#include <glm/glm.hpp>
#include "Camera.h"
class GLDebug final {
 static const glm::vec4 clip_space_cube[];
 struct Point {
 glm::vec3 point;
 glm::vec3 color;
 };
 struct Line {
 glm::vec3 point_0;
 glm::vec3 point_1;
 glm::vec3 color;
 };
 std::vector<Point> m_points;
 std::vector<Line> m_lines;
public:
 static const glm::vec3 color_white;
 static const glm::vec3 color_red;
 static const glm::vec3 color_green;
 static const glm::vec3 color_blue;
 void drawPoint(const glm::vec3& point, const glm::vec3& color = color_white);
 void drawLine(const glm::vec3& point_0, const glm::vec3& point_1, const glm::vec3& color = color_white);
 void drawMat4(const glm::mat4& matrix);
 void drawFrustum(Camera& camera);
 void onRender(Camera& camera);
};

.cpp

#include "GLDebug.h"
#include <glm/gtc/type_ptr.hpp>
#include <glm/gtc/matrix_transform.hpp>
const glm::vec4 GLDebug::clip_space_cube[] = {
 glm::vec4( 1.0f, 1.0f, 0.0f, 1.0f),
 glm::vec4(-1.0f, 1.0f, 0.0f, 1.0f),
 glm::vec4(-1.0f,-1.0f, 0.0f, 1.0f),
 glm::vec4( 1.0f,-1.0f, 0.0f, 1.0f),
 glm::vec4( 1.0f, 1.0f, 1.0f, 1.0f),
 glm::vec4(-1.0f, 1.0f, 1.0f, 1.0f),
 glm::vec4(-1.0f,-1.0f, 1.0f, 1.0f),
 glm::vec4( 1.0f,-1.0f, 1.0f, 1.0f)
};
const glm::vec3 GLDebug::color_white = glm::vec3(1.0f);
const glm::vec3 GLDebug::color_red = glm::vec3(1.0f, 0.0f, 0.0f);
const glm::vec3 GLDebug::color_green = glm::vec3(0.0f, 1.0f, 0.0f);
const glm::vec3 GLDebug::color_blue = glm::vec3(0.0f, 0.0f, 1.0f);
void GLDebug::drawPoint(const glm::vec3& point, const glm::vec3& color) {
 m_points.push_back({ point, color });
}
void GLDebug::drawLine(const glm::vec3& point_0, const glm::vec3& point_1, const glm::vec3& color) {
 m_lines.push_back({ point_0, point_1, color });
}
void GLDebug::drawMat4(const glm::mat4& matrix) {
 glm::vec4 temp0;
 glm::vec4 temp1;
 temp0 = matrix * glm::vec4(0.0f, 0.0f, 0.0f, 1.0f);
 drawPoint(temp0);
 temp1 = matrix * glm::vec4(1.0f, 0.0f, 0.0f, 1.0f);
 drawLine(temp0, temp1, color_red);
 temp1 = matrix * glm::vec4(0.0f, 1.0f, 0.0f, 1.0f);
 drawLine(temp0, temp1, color_green);
 temp1 = matrix * glm::vec4(0.0f, 0.0f, 1.0f, 1.0f);
 drawLine(temp0, temp1, color_blue);
}
void GLDebug::drawFrustum(Camera& camera) {
 drawMat4(glm::inverse(camera.getViewMatrix()));
 const glm::mat4 invViewProj = glm::inverse(camera.getViewProjMatrix());
 glm::vec4 transformedClipCube[8];
 for (int i = 0; i < 8; i++) {
 transformedClipCube[i] = invViewProj * clip_space_cube[i];
 transformedClipCube[i] /= transformedClipCube[i].w;
 }
 drawLine(transformedClipCube[0], transformedClipCube[1]);
 drawLine(transformedClipCube[1], transformedClipCube[2]);
 drawLine(transformedClipCube[2], transformedClipCube[3]);
 drawLine(transformedClipCube[3], transformedClipCube[0]);
 drawLine(transformedClipCube[4], transformedClipCube[5]);
 drawLine(transformedClipCube[5], transformedClipCube[6]);
 drawLine(transformedClipCube[6], transformedClipCube[7]);
 drawLine(transformedClipCube[7], transformedClipCube[4]);
 drawLine(transformedClipCube[0], transformedClipCube[4]);
 drawLine(transformedClipCube[1], transformedClipCube[5]);
 drawLine(transformedClipCube[2], transformedClipCube[6]);
 drawLine(transformedClipCube[3], transformedClipCube[7]);
}
void GLDebug::onRender(Camera& camera) {
 if (m_points.empty() && m_lines.empty()) return;
 glDisable(GL_DEPTH_TEST);
 glMatrixMode(GL_PROJECTION);
 glLoadMatrixf(glm::value_ptr(camera.getProjMatrix()));
 glMatrixMode(GL_MODELVIEW);
 glLoadMatrixf(glm::value_ptr(camera.getViewMatrix()));
 glPointSize(5.0f);
 glBegin(GL_POINTS);
 for (const auto& point : m_points) {
 glColor3fv(glm::value_ptr(point.color));
 glVertex3fv(glm::value_ptr(point.point));
 }
 glEnd();
 glBegin(GL_LINES);
 for (const auto& line : m_lines) {
 glColor3fv(glm::value_ptr(line.color));
 glVertex3fv(glm::value_ptr(line.point_0));
 glColor3fv(glm::value_ptr(line.color));
 glVertex3fv(glm::value_ptr(line.point_1));
 }
 glEnd();
 m_points.clear();
 m_lines.clear();
 glEnable(GL_DEPTH_TEST);
}

Some of the more specific things I'd like feedback on:

• Is my use of "static const" member variables proper? (Is there a better way to go about them.)
• In the "drawMat4" function definition, is it ok to have have "temp" variables defined as they are? (Not concerned as much about the naming, just how they are used. In my old Java project, I had some class level static variables I would use as "temp" to prevent memory re-allocation. {I was targeting Android and without reusing an existing object, it would GC often from new object allocations which would cause stutter.} I was going to use more static class member variables for the "temp" here however I'd like to hear others thoughts before doing so. {Is there a proper C++ way of doing what I described?})
• Eventually I would like to make my project multi-threaded, are there any preparations I should make early on to ensure the transition goes smoothly? (Like modifying key words etc? {In Java there are synchronized, volatile, etc.})

Thanks in advance to all advice!

• Malachi: Welcome to Code Review! Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers .

Answer 1

I’ve seen it stated in conference presentations that constexpr should generally replace uses of static const. Even if you can’t do that because the initializer isn't constexpr, note that you no longer have to put the definitions into a CPP file separate from the class definition.

Is there a reason why drawLine and drawPoint are not inlined in the class definition?

• Hex: Thanks for the response, I've inline'd my draw point/line functions. As for the constexpr I was able to apply it to static constexpr size_t cube_vertex_count = 8; but nothing else appears to like it. I also tried moving the initialization of the clip_cube_vertices,color_white, etc from the cpp to the h but with no luck. (Also those don't like constexpr ether)
• JDługosz: Why doesn’t static const glm::vec3 color_white = glm::vec3(1.0f); inside the body of the class not work?
• Hex: Gives me the error "a member of type const glm::vec3 cannot have an in-class initializer" I googled around but couldn't find much. (Sorry if this error is something obvious and dumb I'm doing on my part, like I mentioned, I've just started getting into C++)
• JDługosz: @HexCrown Hmm, appears that only integer or enum static data member can be initialized that way. You need to add inline to enable this.
• JDługosz: @HexCrown The keywords can appear in either order. Sounds like you are compiling against an old dialect — make sure C++17 (or "latest" ) is selected.

Answer 2

• glBegin / glEnd have been deprecated for a long time and aren't even available on some platforms.
• avoid using c-style arrays, use std::array instead (this can be applied to clip_space_cube)
• avoid using magic numbers, 8 in the following block should be a static constant like cube_vertex_count (which also could be used for array size):

for (int i = 0; i < 8; i++) {
 transformedClipCube[i] = invViewProj * clip_space_cube[i];
 transformedClipCube[i] /= transformedClipCube[i].w;
}

• Hex: Hi, thanks for the feedback, I'll change to std::array and use a static const for the size. As for the glBegin/glEnd, this method of render is not one I will be using for anything other than debugging, with regards to OpenGL, I use VAOs/VBOs with glDrawElements for actual mesh rendering, that said, the amount of boilerplate to set those up is quite extensive, needing shaders, etc. Is there an alternative with similar low boilerplate to glBegin/glEnd that is equally as convenient to use but more current?
• user7860670: @HexCrown cube_vertex_count should rather be a static constant at class scope, and used to define space_cube type using clip_space_cube_t = ::std::array<glm::vec4, cube_vertex_count>; static const clip_space_cube_t clip_space_cube;
• Hex: You lost me after the part about moving cube_vertex_count to class scope.
• yuri: @HexCrown using is just a modern alternative to typedef
• user7860670: @HexCrown There is no need to define that constant in the .cpp as long as no reference or pointer to it is used. In modern c++ It is possible to define static inline variables and constexpr constants only in header file.