3
\$\begingroup\$

I am writing my own game engine, and I wanted to share with my own window class written in C++ using GLFW as the windowing API. Is there someone who can help me better this class? I'm not a complete beginner in C++ but not an expert.

Window.h:

#pragma once

#include <GL\glew.h>
#include <GLFW\glfw3.h>
#include <iostream>

namespace Engine
{
    namespace graphics
    {
        class Window
        {
        private:

            int m_Width, m_Height;
            const char* m_Title;
            bool m_VSync;
            bool Init();

        public:

            GLFWwindow* m_Window;
            Window(const char* title, int width, int height, bool vsync);
            ~Window();
            void Clear(float x, float y, float z);
            void Update();
            bool Closed() { return glfwWindowShouldClose(m_Window); }
        };
    }
}

Window.cpp:

#include "Window.h"

namespace Engine
{
    namespace graphics
    {
        static void key_callback(GLFWwindow* window, int key, int scancode, int action, int mods)
        {
            if (key == GLFW_KEY_ESCAPE && action == GLFW_PRESS)
                glfwSetWindowShouldClose(window, GLFW_TRUE);
        }

        void framebuffer_size_callback(GLFWwindow* window, int width, int height)
        {
            glViewport(0, 0, width, height);
        }

        bool Window::Init()
        {
            if (!glfwInit())
            {
                std::cout<<"ERROR::GLFW::INIT : GLFW failed to initialize"<<std::endl;
                return false;
            }

            glfwWindowHint(GLFW_CONTEXT_VERSION_MAJOR, 3);
            glfwWindowHint(GLFW_CONTEXT_VERSION_MINOR, 3);

            m_Window = glfwCreateWindow(m_Width, m_Height, m_Title, NULL, NULL);
            glfwSetKeyCallback(m_Window, key_callback);
            glfwSetFramebufferSizeCallback(m_Window, framebuffer_size_callback);

            glfwMakeContextCurrent(m_Window);

            if (glewInit() != GLEW_OK)
            {
                glfwTerminate();
                std::cout << "ERROR::GLEW::INIT : GLEW failed to initialize"<<std::endl;
                return false;
            }

            if (m_VSync)
            {
                glfwSwapInterval(1);
                std::cout << "WINDOW::OPTIONS::VSYNC::ON" << std::endl;
            }
            else
            {
                std::cout << "WINDOW::OPTIONS::VSYNC::OFF" << std::endl;
            }
            return true;
        }

        Window::Window(const char* title, int width, int height,bool vsync)
        {
            m_Title = title;
            m_Width = width;
            m_Height = height;
            m_VSync = vsync;
            Init();
        }

        void Window::Clear(float x, float y, float z)
        {
            glClearColor(x, y, z, 1.0f);
            glEnable(GL_DEPTH_TEST);
            glClear(GL_COLOR_BUFFER_BIT |GL_DEPTH_BUFFER_BIT);
        }

        void Window::Update()
        {
            int width, height;

            glfwGetFramebufferSize(m_Window, &width, &height);

            glViewport(0, 0, width, height);

            glfwSwapBuffers(m_Window);
            glfwPollEvents();
        }

        Window::~Window()
        {
            glfwTerminate();
            glfwDestroyWindow(m_Window);
        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ As @Adriano already posted a great review, I thought I'd add a small performance modification. In 'Window::Update()', either 'width' and 'height' should be static or at least outside the function. You function is probably called many times, so you don't want to create an instance that doesn't need to be created every time. \$\endgroup\$ – Oliver Yasuna Oct 26 '17 at 19:27
  • \$\begingroup\$ the reason why width and height are in the update function is because whenever the window gets resized, width and height change, so they adjust the viewport using glViewport. I know there is also a callback for GLFW called something like windowResizeCallback, that does the same thing(and this is the way you should be doing it... Ops :) ) I am definitely going to add this in the future! thanks for helping! \$\endgroup\$ – user152094 Oct 28 '17 at 9:25
  • \$\begingroup\$ I understand that, but for performance reasons, see my comment above. \$\endgroup\$ – Oliver Yasuna Oct 28 '17 at 19:46
2
\$\begingroup\$

Window::Init() performs OpenGL initialization and creates the GL window, I do not completely agree with this: I'd avoid to perform expensive tasks inside ctor (which should, IMO, just initialize the object in a consistent state). Initialization may be done lazily when required (better, I think) or made explicit with a public Initialize() method (which has the disadvantage to be another function to call and the overhead of the checks to determine if it has been called or not but where you can use virtual functions without worries).

Your actual Window::Init() returns a bool which is simply unused. However when initialization fails the object is not in a valid state and any other function call may simply crash. Calling code has to any clue and it can't gracefully exit. Feel free to throw an exception (even easier if you have a separate Initialize() method) instead of returning a bool:

if (!glfwInit())
    throw std::runtime_error("GLFW failed to initialize.");

Also note that if you log something to console then errors should go to std::cerr instead of std::cout:

std::cerr << "ERROR::GLFW::INIT : GLFW failed to initialize" << std::endl;

If you properly handle error cases (instead of returning an unused flag to signal an error) then you do not need to repeat clean-up code in your initialization function because dtor will take care of it:

Window::~Window()
{
    if (m_IsInitialized)
        glfwTerminate();

    if (m_Window != nullptr)
        glfwDestroyWindow(m_Window);
}

About m_Window: why it's public? There is almost never a good reason to have a public field. Note that if, for any reason, you have to expose this pointer to the external world then I strongly suggest to use std::shared_ptr, you may also go one step further and implement a custom delete function that will call glfwDestroyWindow() (I think it's much easier to add few assertions in this case then track an access violation because memory has been released somewhere). Also this opens the door to more complex scenarios (for example using std::move() to transfer window ownership to another Window instance).

Jumping back to ctor, you can use an initializer list:

Window::Window(const char* title, int width, int height, bool vsync)
    : m_Title(title),
      m_Width(width),
      m_Height(height),
      m_VSync(vsync)
{
}

It does not really matter for PODs however you may (and should) declare all those members as const (and * const) then you must use this syntax.

You're not validating inputs, that's a bad thing! Is, for example, a negative width allowed? Throw the right exception as soon as possible and you won't need to hunt a bug later.

if (width < 0)
    throw std::invalid_argument("Width cannot be negatie.");
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy