Add line error checking; rename shader folder env variable; refactor ellipse

src/ellipse.cpp

@@ -26,13 +26,12 @@
 #include <glm/glm.hpp>
 
 namespace {  // anonymous namespace
-nzl::Program create_program() {
-  const std::string vertex_shader_source =
-      nzl::slurp(nzl::get_env_var("SHADERS_PATH") + "/simple_shader.vert");
-
-  const std::string fragment_shader_source =
-      nzl::slurp(nzl::get_env_var("SHADERS_PATH") + "/simple_shader.frag");
+const std::string vertex_shader_source =
+    nzl::slurp(nzl::get_env_var("MXD_SHADER_ROOT") + "/simple_shader.vert");
 
+const std::string fragment_shader_source =
+    nzl::slurp(nzl::get_env_var("MXD_SHADER_ROOT") + "/simple_shader.frag");
+nzl::Program create_program() {
   nzl::Shader vert_shader(nzl::Shader::Stage::Vertex, vertex_shader_source);
   vert_shader.compile();
 
@@ -66,79 +65,90 @@ std::vector<glm::vec3> gen_points(float rX, float rY, float number_of_points) {
 
 namespace nzl {
 
-struct Ellipse::IDContainer {
-  glm::vec3 m_color;
-  unsigned int m_vao_id;
-  unsigned int m_vbo_id;
-  int m_number_of_points{0};
-  nzl::Program m_program;
+struct Ellipse::EllipseImp {
+  EllipseImp(float s_ma, float s_mi, float number_of_points);
 
-  IDContainer(glm::vec3 color, float rX, float rY, float number_of_points)
-      : m_color{color}, m_program{create_program()} {
-    glGenVertexArrays(1, &m_vao_id);
+  ~EllipseImp();
+  nzl::Program program;
+  glm::vec3 color;
+  unsigned int vao_id;
+  unsigned int vbo_id;
+  int number_of_points{0};
+};
 
-    glBindVertexArray(m_vao_id);
+nzl::Ellipse::EllipseImp::EllipseImp(float s_ma, float s_mi,
+                                     float number_of_points)
+    : program{create_program()} {
+  glGenVertexArrays(1, &vao_id);
 
-    glGenBuffers(1, &m_vbo_id);
+  glBindVertexArray(vao_id);
 
-    glBindBuffer(GL_ARRAY_BUFFER, m_vbo_id);
+  glGenBuffers(1, &vbo_id);
 
-    std::vector<glm::vec3> points{gen_points(rX, rY, number_of_points)};
+  glBindBuffer(GL_ARRAY_BUFFER, vbo_id);
 
-    // Add the first point again, so the first point is connected to the last.
-    points.push_back(points.front());
+  std::vector<glm::vec3> points{gen_points(s_ma, s_mi, number_of_points)};
 
-    m_number_of_points = points.size();
+  // Add the first point again, so the first point is connected to the last
 
-    glBufferData(GL_ARRAY_BUFFER, points.size() * 3 * sizeof(float),
-                 points.data(), GL_STATIC_DRAW);
+  points.push_back(points.front());
 
-    glEnableVertexAttribArray(0);
+  number_of_points = points.size();
 
-    glVertexAttribPointer(0, 3, GL_FLOAT, GL_FALSE, 3 * sizeof(float),
-                          (void*)0);
+  glBufferData(GL_ARRAY_BUFFER, points.size() * 3 * sizeof(float),
+               points.data(), GL_STATIC_DRAW);
 
-    glDisableVertexAttribArray(0);
+  glEnableVertexAttribArray(0);
 
-    glBindBuffer(GL_ARRAY_BUFFER, 0);
+  glVertexAttribPointer(0, 3, GL_FLOAT, GL_FALSE, 3 * sizeof(float), (void*)0);
 
-    glBindVertexArray(0);
-  }
+  glDisableVertexAttribArray(0);
 
-  ~IDContainer() {
-    glDeleteVertexArrays(1, &m_vao_id);
-    glDeleteBuffers(1, &m_vbo_id);
-  }
-};
+  glBindBuffer(GL_ARRAY_BUFFER, 0);
 
-Ellipse::Ellipse(float rX, float rY, int number_of_points,
-                 glm::vec3 color) noexcept
-    : m_id_container{
-          std::make_shared<IDContainer>(color, rX, rY, number_of_points)} {}
+  glBindVertexArray(0);
+
+  nzl::check_gl_errors();
+}
+
+nzl::Ellipse::EllipseImp::~EllipseImp() {
+  glDeleteVertexArrays(1, &vao_id);
+  glDeleteBuffers(1, &vbo_id);
+  nzl::check_gl_errors();
+}
 
-glm::vec3 Ellipse::color() const noexcept { return m_id_container->m_color; }
+// -----------------------------------------------------------------------------
+//         The section below forwards API calls to the implementation
+// -----------------------------------------------------------------------------
 
-void Ellipse::set_color(glm::vec3 color) noexcept {
-  m_id_container->m_color = color;
+Ellipse::Ellipse(float s_ma, float s_mi, int number_of_points, glm::vec3 color)
+    : m_pimpl{std::make_shared<EllipseImp>(s_ma, s_mi, number_of_points)} {
+  m_pimpl->color = color;
 }
 
-nzl::Program Ellipse::get_program() const noexcept {
-  return m_id_container->m_program;
+glm::vec3 Ellipse::color() const noexcept { return m_pimpl->color; }
+
+void Ellipse::set_color(glm::vec3 color) noexcept { m_pimpl->color = color; }
+
+const nzl::Program& Ellipse::get_program() const noexcept {
+  return m_pimpl->program;
 }
 
 void Ellipse::do_render(TimePoint t [[maybe_unused]]) {
-  m_id_container->m_program.use();
-  m_id_container->m_program.set("color", m_id_container->m_color);
+  m_pimpl->program.use();
+  m_pimpl->program.set("color", m_pimpl->color);
 
-  glBindVertexArray(m_id_container->m_vao_id);
+  glBindVertexArray(m_pimpl->vao_id);
 
   glEnableVertexAttribArray(0);
 
-  glDrawArrays(GL_LINE_STRIP, 0, m_id_container->m_number_of_points);
+  glDrawArrays(GL_LINE_STRIP, 0, m_pimpl->number_of_points);
 
   glDisableVertexAttribArray(0);
 
   glBindVertexArray(0);
+
+  nzl::check_gl_errors();
 }
 
 }  // namespace nzl

src/ellipse.hpp

@@ -25,11 +25,11 @@ namespace nzl {
 class Ellipse : public Geometry {
  public:
   /// @brief Creates an ellipse.
-  /// @param rX Radius in the x direction.
-  /// @param rY Radius in the y direction.
+  /// @param s_ma Length of semi-major axis.
+  /// @param s_mi Length of semi-minor axis.
   /// @param number_of_points Number of points to be generated for the ellipse.
   /// @param color Color the ellipse will be drawn with.
-  Ellipse(float rX, float rY, int number_of_points, glm::vec3 color) noexcept;
+  Ellipse(float s_ma, float s_mi, int number_of_points, glm::vec3 color);
 
   /// @brief Return the ellipse's color.
   glm::vec3 color() const noexcept;
@@ -40,11 +40,11 @@ class Ellipse : public Geometry {
   void set_color(glm::vec3 color) noexcept;
 
   /// @brief Returns the program used by the ellipse.
-  nzl::Program get_program() const noexcept;
+  const nzl::Program& get_program() const noexcept;
 
  private:
-  struct IDContainer;
-  std::shared_ptr<IDContainer> m_id_container{nullptr};
+  struct EllipseImp;
+  std::shared_ptr<EllipseImp> m_pimpl;
 
   void do_render(TimePoint t) override;
 };

src/ellipse.t.cpp

@@ -37,6 +37,12 @@ TEST(Ellipse, ConstructorAndParameterAccess) {
   EXPECT_FLOAT_EQ(ellipse.color().y, 0.0f);
   EXPECT_FLOAT_EQ(ellipse.color().z, 0.0f);
 
+  ellipse.set_color(glm::vec3(0.4f, 0.3f, 0.1f));
+
+  EXPECT_FLOAT_EQ(ellipse.color().x, 0.4f);
+  EXPECT_FLOAT_EQ(ellipse.color().y, 0.3f);
+  EXPECT_FLOAT_EQ(ellipse.color().z, 0.1f);
+
   EXPECT_NE(ellipse.get_program().id(), 0);
 
   EXPECT_EQ(glGetError(), 0);

src/geometry.t.cpp

@@ -16,9 +16,6 @@
 // Google Test Framework
 #include <gtest/gtest.h>
 
-TEST(Geometry, Failing) {
-  ASSERT_TRUE(false) << "You must add unit tests for geometry.hpp";
-}
 
 int main(int argc, char** argv) {
   ::testing::InitGoogleTest(&argc, argv);

src/line.cpp

@@ -28,17 +28,12 @@
 namespace {  // anonymous namespace
 
 const std::string vertex_shader_source =
-    nzl::slurp(nzl::get_env_var("SHADERS_PATH") + "/simple_shader.vert");
+    nzl::slurp(nzl::get_env_var("MXD_SHADER_ROOT") + "/simple_shader.vert");
 
 const std::string fragment_shader_source =
-    nzl::slurp(nzl::get_env_var("SHADERS_PATH") + "/simple_shader.frag");
+    nzl::slurp(nzl::get_env_var("MXD_SHADER_ROOT") + "/simple_shader.frag");
 
-/// @TODO This is too ugly. Program needs a full refactoring.
 auto make_program() {
-  /// @TODO The Shader/Program interface feels very awkward. For instance: why
-  /// do I need to pass a vector of shaders? Why do I need to compile the
-  /// shaders and then compile the program? Shouldn't the program compile the
-  /// shaders if it needs to? What happens if I forget to compile the shader?
   std::vector<nzl::Shader> shaders;
   shaders.emplace_back(nzl::Shader::Stage::Vertex, vertex_shader_source);
   shaders.emplace_back(nzl::Shader::Stage::Fragment, fragment_shader_source);
@@ -58,8 +53,8 @@ namespace nzl {
 /// Put all you need in the Implementation.
 struct nzl::Line::LineImp {
   LineImp();
-  ~LineImp() noexcept;
-  nzl::Program program;  /// @TODO Why not provide a default constructor?
+  ~LineImp();
+  nzl::Program program;
   unsigned int vao_id;
   unsigned int vbo_id;
   int number_of_points{0};
@@ -69,8 +64,6 @@ struct nzl::Line::LineImp {
 };
 
 nzl::Line::LineImp::LineImp() : program{make_program()} {
-  /// @TODO Add error checking! 10 minutes spent adding good error checking and
-  /// error messages will save you 10 hours debugging the program in the future.
   glGenVertexArrays(1, &vao_id);
   glBindVertexArray(vao_id);
   glGenBuffers(1, &vbo_id);
@@ -81,12 +74,13 @@ nzl::Line::LineImp::LineImp() : program{make_program()} {
   glDisableVertexAttribArray(0);
   glBindBuffer(GL_ARRAY_BUFFER, 0);
   glBindVertexArray(0);
-}
 
-nzl::Line::LineImp::~LineImp() noexcept {
-  /// @TODO: Add error checking!
+  nzl::check_gl_errors();
+}
+nzl::Line::LineImp::~LineImp() {
   glDeleteVertexArrays(1, &vao_id);
   glDeleteBuffers(1, &vbo_id);
+  nzl::check_gl_errors();
 }
 
 void nzl::Line::LineImp::load_points(glm::vec3 points[], int size) {
@@ -95,6 +89,7 @@ void nzl::Line::LineImp::load_points(glm::vec3 points[], int size) {
   glBufferData(GL_ARRAY_BUFFER, size * 3 * sizeof(float), points,
                GL_STATIC_DRAW);
   glBindBuffer(GL_ARRAY_BUFFER, 0);
+  nzl::check_gl_errors();
 }
 
 // -----------------------------------------------------------------------------
@@ -111,11 +106,11 @@ Line::Line(glm::vec3 color, std::vector<glm::vec3>& points) : Line(color) {
   load_points(points);
 }
 
-void Line::load_points(std::vector<glm::vec3>& points) noexcept {
+void Line::load_points(std::vector<glm::vec3>& points) {
   m_pimpl->load_points(points.data(), points.size());
 }
 
-void Line::load_points(glm::vec3 points[], int size) noexcept {
+void Line::load_points(glm::vec3 points[], int size) {
   m_pimpl->load_points(points, size);
 }
 
@@ -134,12 +129,13 @@ void Line::do_render(TimePoint t [[maybe_unused]]) {
   program.use();
   program.set("color", m_pimpl->color);
 
-  /// @TODO Add error checking!
   glBindVertexArray(m_pimpl->vao_id);
   glEnableVertexAttribArray(0);
   glDrawArrays(GL_LINE_STRIP, 0, m_pimpl->number_of_points);
   glDisableVertexAttribArray(0);
   glBindVertexArray(0);
+
+  nzl::check_gl_errors();
 }
 
 }  // namespace nzl

src/line.hpp

@@ -27,27 +27,32 @@ namespace nzl {
 class Line : public Geometry {
  public:
   /// @brief Creates line with white color.
+  /// @throws std::runtime_error if an OpenGL error is found.
   Line();
 
   /// @brief Creates line.
   /// @param color Line color.
+  /// @throws std::runtime_error if an OpenGL error is found.
   Line(glm::vec3 color);
 
   /// @brief Creates line and loads points onto line.
   /// @param color Line color.
   /// @param points Points to be loaded into the VBO.
+  /// @throws std::runtime_error if an OpenGL error is found.
   Line(glm::vec3 color, std::vector<glm::vec3>& points);
 
   /// @brief Loads points into the line's VBO.
   /// @param points Points to be loaded into the VBO.
+  /// @throws std::runtime_error if an OpenGL error is found.
   /// @note Affects all copies of this object.
-  void load_points(std::vector<glm::vec3>& points) noexcept;
+  void load_points(std::vector<glm::vec3>& points);
 
   /// @brief Loads points into the line's VBO.
   /// @param points Points to be loaded into the VBO.
   /// @param size Size of the array.
+  /// @throws std::runtime_error if an OpenGL error is found.
   /// @note Affects all copies of this object.
-  void load_points(glm::vec3 points[], int size) noexcept;
+  void load_points(glm::vec3 points[], int size);
 
   /// @brief Returns the line's color.
   glm::vec3 color() const noexcept;

src/line.t.cpp

@@ -37,6 +37,12 @@ TEST(Line, ConstructorAndParameterAccess) {
   EXPECT_FLOAT_EQ(line.color().y, 0.5f);
   EXPECT_FLOAT_EQ(line.color().z, 0.3f);
 
+  line.set_color(glm::vec3(1.0f, 0.3f, 0.1f));
+
+  EXPECT_FLOAT_EQ(line.color().x, 1.0f);
+  EXPECT_FLOAT_EQ(line.color().y, 0.3f);
+  EXPECT_FLOAT_EQ(line.color().z, 0.1f);
+
   EXPECT_NE(line.get_program().id(), 0);
 
   EXPECT_EQ(glGetError(), 0);
@@ -105,6 +111,34 @@ TEST(Line, DrawAndReplacePoints) {
   nzl::terminate();
 }
 
+TEST(Line, DrawFromArray) {
+  nzl::initialize();
+  nzl::Window win(800, 600, "Test Window");
+  win.hide();
+  win.make_current();
+
+  nzl::Line line;
+
+  glm::vec3 arr[3];
+
+  arr[0] = glm::vec3(-1.0f, 1.0f, 0.0f);
+  arr[1] = glm::vec3(1.0f, -1.0f, 0.0f);
+  arr[2] = glm::vec3(0.5f, 1.0f, 0.0f);
+  line.load_points(arr, 3);
+
+  for (int i = 0; i < 3; i++) {
+    glClearColor(0.0f, 0.0f, 0.0f, 1.0f);
+    glClear(GL_COLOR_BUFFER_BIT);
+
+    line.render(nzl::TimePoint());
+
+    win.swap_buffers();
+  }
+  EXPECT_EQ(glGetError(), 0);
+
+  nzl::terminate();
+}
+
 int main(int argc, char** argv) {
   ::testing::InitGoogleTest(&argc, argv);
   return RUN_ALL_TESTS();