Window class for a game engine

Question

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);
        }
    }
}

Answer 1

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.");