SDL2 engine base implementation

Question

I would like a little help with the implementation of my SDL2 engine. It's actually not a real engine nor does it strive to be but I don't have a better word to describe it.

The purpose of this project is to serve as a base for other things to be built on, like a visualization of fractals for example, so i would like to make it as optimal as possible.

For now it just creates a window, renderer and handles basic input handling. Before I move on with it I would like to iron out any kinks there might be so any improvement suggestion would come in handy, especially when it comes to performance and future upgradability.

engine.h

#ifndef ENGINE_H
#define ENGINE_H
#pragma once
#include <SDL2/SDL.h>
#include "window.hpp"
#include "renderer.hpp"
class Engine
{
private:
 bool running;
public:
 Window window;
 Renderer renderer;
 Engine();
 ~Engine();
 //Handles input
 void handleInput();
 //draws things onto the screen
 void draw();
 bool isRunning();
};
#endif

engine.cpp

#include "engine.h"
Engine::Engine(): window( 640, 480 ), renderer( window.getWindow() )
{
 if( window.getWindow() == NULL )
 {
 printf("Window could not be created. \n" );
 running = false;
 }
 else
 {
 if( renderer.getRenderer() == NULL )
 {
 printf("Renderer could not be created. \n" );
 running = false;
 }
 running = true;
 printf("Engine has been started successfuly \n" );
 }
}
Engine::~Engine()
{
 running = false;
 window.close();
 renderer.close();
}
void Engine::handleInput()
{
 SDL_Event event;
 while( SDL_PollEvent( &event ) != 0 )
 {
 if( event.type == SDL_QUIT )
 {
 window.close();
 }
 }
}
void Engine::draw()
{
 SDL_RenderClear( renderer.getRenderer() );
 SDL_RenderPresent( renderer.getRenderer() );
}
bool Engine::isRunning()
{
 return window.isOpen() && renderer.isCreated() && running;
}

window.hpp

#ifndef WIDNOW_H
#define WINDOW_H
#pragma once
#include <SDL2/SDL.h>
class Window
{
private:
 const int SCREEN_WIDTH = 0;
 const int SCREEN_HEIGHT = 0;
 bool open = false;
 //Window we'll render to
 SDL_Window* mWindow = NULL;
public:
 Window();
 //Consturctor to initialize window
 Window( int SCREEN_WIDTH, int SCREEN_HEIGHT );
 ~Window();
 void close();
 bool isOpen();
 SDL_Window* getWindow();
};
#endif

window.cpp

#include "window.hpp"
Window::Window()
{
}
//Initialize SDL and create a window
Window::Window( int SCREEN_HEIGHT, int SCREEN_WIDTH ): SCREEN_WIDTH(SCREEN_WIDTH), SCREEN_HEIGHT( SCREEN_HEIGHT)
{
 //If SDL video was not initialize
 if( SDL_Init( SDL_INIT_VIDEO) < 0 )
 {
 printf( "SDL could not initialize! SDL error: %s\n", SDL_GetError() );
 open = false;
 }
 else
 {
 //Create window
 mWindow = SDL_CreateWindow( "Window class", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED, SCREEN_WIDTH, SCREEN_HEIGHT, SDL_WINDOW_SHOWN );
 //If the window was not created
 if( mWindow == NULL )
 {
 printf( "Could not create window. SDL error: %s\n", SDL_GetError() );
 open = false;
 }
 //Set the open flag to true after the operation completed successfuly
 open = true;
 printf("Window was created successfuly.\n" );
 }
}
void Window::close()
{
 open = false;
 SDL_DestroyWindow( mWindow );
 mWindow = NULL;
 SDL_Quit();
}
Window::~Window()
{
 printf("Window destructor was called \n" );
 close();
}
bool Window::isOpen()
{
 return open;
}
SDL_Window* Window::getWindow()
{
 return mWindow;
}

renderer.hpp

#ifndef RENDERER_H
#define RENDERER_H
#pragma once
#include "window.hpp"
class Renderer : public Window
{
private:
 bool created = false;
 SDL_Renderer* mRenderer = NULL;
public:
 Renderer();
 Renderer( SDL_Window* window );
 ~Renderer();
 void close();
 bool isCreated();
 SDL_Renderer* getRenderer();
};
#endif

renderer.cpp

#include "renderer.hpp"
Renderer::Renderer()
{
}
Renderer::Renderer( SDL_Window* window )
{
 //create the renderer
 mRenderer = SDL_CreateRenderer( window, -1, SDL_RENDERER_ACCELERATED );
 //if the rendere was not created
 if( mRenderer == NULL )
 {
 printf("Renderer could not be created. SDL error: %s\n", SDL_GetError() );
 created = false;
 }
 else
 {
 //Set the render draw color to white
 SDL_SetRenderDrawColor( mRenderer, 0xFF, 0xFF, 0xFF, 0xFF );
 created = true;
 printf("Renderer was created successfuly \n" );
 }
}
Renderer::~Renderer()
{
 printf("Renderer desturctor was called \n" );
 close();
}
bool Renderer::isCreated()
{
 return created;
}
void Renderer::close()
{
 SDL_DestroyRenderer( mRenderer );
 mRenderer = NULL;
}
SDL_Renderer* Renderer::getRenderer()
{
 return mRenderer;
}

main.cpp

#include "engine.h"
int main( int argc, char* argv[] )
{
 Engine engine;
 while( engine.isRunning() )
 {
 engine.handleInput();
 engine.draw();
 }
}

• Toby Speight: Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Please check that I haven't misrepresented your code, and correct it if I have.
• Toby Speight: Note that general "best practices" questions are explicitly off-topic here, so the title edit is important to prevent your question being closed!
• Mast: Have you tested this engine? Does it currently produce satisfactory output?
• Asmir Zukić: @Mast ♦ The engine does create a window and renderer and handles exiting the program. I have made some improvements with the help of suggestions from here. There is also a bug where multiple instances of the window are created when inheriting from the engine class. I fixed it by implementing the window and renderer as singletons.

Answer 1

Engine:

Engine::Engine(): window( 640, 480 ), renderer( window.getWindow() )

We should probably set the running variable to false here to prevent accidents. (Or we could do so in the member variable declaration like the other classes do).

if( window.getWindow() == NULL )
{
 printf("Window could not be created. \n" );
 running = false;
}

I think it's reasonable to throw an exception if something so fundamental goes wrong. It's simpler to avoid the complexity of trying to carry on running the program in an abnormal state.

Note that this specific check would then be unnecessary. We could throw the exception from the Window constructor, and have it propagate upwards. We could do the same in the Renderer constructor.

---

Consider adding an Engine::run() function to hold the main loop, so that main can just call engine.run(). But the best way of doing things rather depends on how you add the functionality for your visualization, and how it interacts with Engine. (i.e. will you put that code inside Engine, or will you pass the Engine to that code?)

---

Window:

const int SCREEN_WIDTH = 0;
const int SCREEN_HEIGHT = 0;

I don't think these should be const. We'll probably need to make the window resizeable at some point.

More importantly, we shouldn't store these variables at all. SDL will handle resizing our window, and we don't want to have to keep our own variables up to date. We should simply use SDL_GetWindowSize when we need the current window size.

---

bool open = false;

I don't think we need this variable. We can check that mWindow is not nullptr to see if the window is open.

---

if( SDL_Init( SDL_INIT_VIDEO) < 0 )
...
SDL_Quit();

I'm not sure these should be inside the Window class, since SDL handles more than just the windowing. We also need a valid SDL context for rendering, audio, input etc. So perhaps SDL init and shutdown could be put in a separate SDLContext class that's created before the window.

---

Renderer:

bool created = false;
SDL_Renderer* mRenderer = NULL;

As with Window::open, we don't need the created flag. We can check mRenderer.

Answer 2

Don't use the C macro NULL. C++ has a real null pointer named nullptr. Meanwhile, don't explicitly compare against nullptr, but rather use the convert-to-boolean truth value of the pointer (this is important when you use smart pointers, and is idiomatic for any pointer type). That is, write:

if(p) // NOT if(p!=nullptr)
if(!q) // NOT if(q==nullptr)

ENGINE_H is not unique enough to not clash with anything else. Personally I leave off the guards and use #pragma once and would use an automated program to put in guards if I ever found a platform that needed them. Use a GUID-based identifier if you must have a guard.

You're using printf for logging, but never included the proper header for that. Consider using a standard error for such logging rather than standard output, and don't use printf because at some point you'll want to show class types and strings in particular. I really don't want to use a function that will compile without complaint but crash if it's passed an argument that it can't handle, especially on an error pathway that doesn't get tested in a normal run.

---

Renderer::Renderer()
{
}

Don't explicitly write empty-bodied constructors or destructors! Here, put Renderer() = default; in the class definition.

---

bool Renderer::isCreated()
{
 return created;
}

Put this inline in the class definition, and declare the function as const.

Same with isOpen and other accessors.

• Asmir Zukić: Thank you for the feedback. By the conver-to-bool value do you mean explicitly casting the nullptr to bool, because that throws an error on my machine, or is that the same as something like return mWindow != nullptr.
• JDługosz: @AsmirZukić I clarified with an example. No, do not explicitly cast anything.

Answer 3

Its not much but note that you have a spelling mistake in windows.hpp

//Consturctor to initialize window

should be

//Constructor to initialize window