Simple game loop using SDL

Question

This is a basic game loop written in C++ using SDL. All feedback is appreciated especially feedback related to design and extensibility.

Main.cpp:

#include "Game.h"
int main(int argc, char* argv[])
{
 Game game;
 game.Start();
 return 0;
}

Game.h:

#pragma once
#include <SDL.h>
class Game
{
private:
 SDL_Window* m_Window;
 SDL_Renderer* m_Renderer;
 bool m_Running;
 float m_DeltaTime;
public:
 void Start();
 void Stop();
private:
 void GameLoop();
 void HandleEvents();
 void Update(float deltaTime);
 void Render();
};

Timer.h:

#pragma once
class Timer
{
private:
 int m_StartTime;
 int m_PausedTime;
 bool m_Started;
 bool m_Paused;
public:
 Timer()
 : m_StartTime(0), m_PausedTime(0), m_Started(false), m_Paused(false) {}
 void Start();
 void Stop();
 void Pause();
 void Unpause();
 int GetTicks();
 inline bool IsStarted() const { return m_Started; }
 inline bool IsPaused() const { return m_Paused; }
};

Game.cpp:

#include "Game.h"
#include <iostream>
#include <SDL_image.h>
#include <SDL_ttf.h>
#include "Timer.h"
void Game::Start()
{
 m_Running = true;
 if (SDL_Init(SDL_INIT_EVERYTHING) != 0)
 {
 std::cout << "ERROR: Failed to initialise SDL!" << std::endl;
 Stop();
 }
 if (IMG_Init(IMG_INIT_PNG) < 0)
 {
 std::cout << "ERROR: Failed to initialise SDL_image!" << std::endl;
 Stop();
 }
 if (TTF_Init() == -1)
 {
 std::cout << "ERROR: Failed to initialise SDL_ttf!" << std::endl;
 Stop();
 }
 m_Window = SDL_CreateWindow("RPG", SDL_WINDOWPOS_CENTERED, SDL_WINDOWPOS_CENTERED, 800, 600, 0);
 if (!m_Window)
 {
 std::cout << "ERROR: Failed to create SDL_Window!" << std::endl;
 Stop();
 }
 SDL_GL_SetAttribute(SDL_GL_CONTEXT_MAJOR_VERSION, 3);
 SDL_GL_SetAttribute(SDL_GL_CONTEXT_MINOR_VERSION, 3);
 m_Renderer = SDL_CreateRenderer(m_Window, -1, SDL_RENDERER_PRESENTVSYNC);
 if (!m_Renderer)
 {
 std::cout << "ERROR: Failed to create SDL_Renderer!" << std::endl;
 Stop();
 }
 SDL_SetRenderDrawColor(m_Renderer, 0, 0, 0, 255);
 GameLoop();
}
void Game::Stop()
{
 SDL_DestroyWindow(m_Window);
 SDL_DestroyRenderer(m_Renderer);
 m_Window = nullptr;
 m_Renderer = nullptr;
 IMG_Quit();
 SDL_Quit();
 m_Running = false;
}
void Game::GameLoop()
{
 Timer deltaTimer;
 deltaTimer.Start();
 while (m_Running)
 {
 HandleEvents();
 Update(m_DeltaTime);
 m_DeltaTime = static_cast<float>(deltaTimer.GetTicks()) / 1000.0f;
 deltaTimer.Start();
 Render();
 }
 Stop();
}
void Game::HandleEvents()
{
 SDL_Event event;
 SDL_PollEvent(&event);
 switch (event.type)
 {
 case SDL_QUIT:
 m_Running = false;
 break;
 }
}
void Game::Update(float deltaTime)
{
}
void Game::Render()
{
 SDL_RenderClear(m_Renderer);
 static TTF_Font* font = TTF_OpenFont("assets/fonts/caliban.ttf", 12);
 static SDL_Color colour = { 255, 255, 255, 255 };
 SDL_Surface* textSurface = TTF_RenderText_Blended(font, std::string("FPS: " + std::to_string(1.0f / m_DeltaTime)).c_str(), colour);
 SDL_Texture* textTexture = SDL_CreateTextureFromSurface(m_Renderer, textSurface);
 SDL_Rect textBounds = { 5, 5, textSurface->w, textSurface->h };
 SDL_RenderCopy(m_Renderer, textTexture, nullptr, &textBounds);
 SDL_FreeSurface(textSurface);
 SDL_DestroyTexture(textTexture);
 SDL_RenderPresent(m_Renderer);
}

Timer.cpp:

#include "Timer.h"
#include <SDL.h>
void Timer::Start()
{
 m_Started = true;
 m_Paused = false;
 m_StartTime = SDL_GetTicks();
 m_PausedTime = 0;
}
void Timer::Stop()
{
 m_Started = false;
 m_Paused = false;
 m_StartTime = 0;
 m_PausedTime = 0;
}
void Timer::Pause()
{
 if (m_Started && !m_Paused)
 {
 m_Paused = true;
 m_PausedTime = SDL_GetTicks() - m_StartTime;
 m_StartTime = 0;
 }
}
void Timer::Unpause()
{
 if (m_Started && m_Paused)
 {
 m_Paused = false;
 m_StartTime = SDL_GetTicks() - m_PausedTime;
 m_PausedTime = 0;
 }
}
int Timer::GetTicks()
{
 if (m_Started)
 return m_Paused ? m_PausedTime : SDL_GetTicks() - m_StartTime;
 return 0;
}

• hoffmale: Are you using any specific C++ version? I would give some different advice for C++98 than for C++11 (or newer)...
• user162687: I am using Visual Studio so this could be any C++ version however I plan to use C++11/14 in the future.
• frankenapps: Would you like to clarify under which license this code can be used by others (if this is permitted)? I would like to build a simple open source game on top of this, so it would be good to know for me...
• user162687: @frankenapps feel free to use the code for any purpose. This code is quite old (for me) and I would write it differently if I was to do it again.
• Lee: IoanThomas, I know this is an old thread, and you mentioned a year ago you would do it differently if you were to do it again... May I ask how you might do it differently? I read hoffmale's answer. Are there things you would differently than that?

Answer 1

Design

Single Responsibility Principle (SRP)

Your current design, while serving its purpose, is rather restricted regarding future development.

Game is trying to do too much. It currently is responsible for:

• handling user input
• rendering
• possibly advancing the underlying simulation (Game::Update is currently empty)

Instead, each of those concerns should be separated, leaving Game to manage the interactions between them. This would also allow for much tighter control over different systems (e.g. you might want to render as fast as possible, but only advance the underlying simulation every 0.05s). This would also allow for easier reuse in different contexts (e.g. developing an editor to create game levels by only using the renderer, without simulation updates).

Phrased differently, Games only purpose should be running the game loop, i.e. delegating the actual logic to other components and calling them in order.

Example class layouts:

class Renderer {
 SDLWindow sdl_window; // RAII wrapper for SDL_Window*
 SDLRenderer sdl_renderer; // RAII wrapper for SDL_Renderer*
public:
 void Render(const Simulation& current_state);
};
class Simulation {
 // TODO: insert game state
public:
 void Update();
};
class Controls {
public:
 bool HandleInput(Renderer& renderer, Simulation& simulation);
};
class Game {
 Simulation sim;
 Renderer renderer;
 Controls controls;
public:
 inline void Run() {
 while(controls.HandleInputs(renderer, sim)) {
 sim.Update();
 renderer.Render();
 }
 }
};

While that might seem a bit overkill for a (very) simple example, it will help in the future!

Note: This is just a basic outline. I would probably make use of the state pattern in Controls and Simulation to easily enable different modes (e.g. main menu vs. ingame). In some cases, it might make sense to integrate input handling into the Simulation itself (e.g. region based control scheme change in a platformer). It all depends on your game!

Correctness

Resource handling

Any exception thrown during Game::Start will leak resources. While these resources are usually reclaimed by the operating system, you shouldn't rely on this. (Did you ever have trouble with a file being locked after a program crashed? Something like this might happen here, too!)

A better approach would wrap resources like SDL_Window or SDL_renderer in a RAII class, so that they get correctly released, even in case of an exception.

Uninitialized value

The value of Game::m_deltaTime during the first call to Game::Update in Game::GameLoop is uninitialized. This is very likely not intended.

Implementation

For Timer:

• You might want to take a look at the <chrono> standard library header, especially std::chrono::high_resolution_clock. This should provide a higher accuracy than SDL_GetTicks. Especially in the current state, where nothing happens each frame, a simple thing like calculating FPS might cause a division by zero because the timer resolution is too low.

For Game (disregarding above design changes):

• All members are uninitialized upon construction.
• Game::Stop doesn't check if the member variables/SDL systems are actually initialized or if m_Running is true. This might cause lots of issues, especially when the member variables are still in an uninitialized state!
• The instructions in Game::Start might be more appropriate in Games constructor (as they initialize/acquire resources). Similarly, most instructions in Game::Stop would be more appropriate in the destructor.
• I'm personally not a fan of the tail calls in Game::Start and Game::GameLoop. I'd rather extract the sequence of calls to Game::Start, Game::GameLoop and Game::Stop into a Game::Run member function.
• Game::Stop should not be public in its current state. While the game is running, the original caller of Game::Start is still executing that call and unable to call Game::Stop so only internal calls to Game::Stop could be made anyways. The only other possible caller of Game::Stop would be another thread - but Game, especially Game::Stop, is not thread safe and might release resources still used by another thread!
• I'm not a fan of the Timer usage in Game::GameLoop. Not only is it impossible to use the timer pause/unpause functionality this way (as likely intended to pause the game), different systems very likely require different pause states (e.g. the GUI should still respond, even if the game is paused). Also, the repeated calls to Timer::Start seem like a hacky workaround that is throwing away useful information; it would be just as easy to cache the result of the last call to Timer::GetTicks in a local variable.
• Variable naming: Most variables are more or less given descriptive names. But then, in Game::Render, all local variables have very general names. textSurface doesn't contain any generic text, it contains the FPS counter (and similar for textTexture and textBounds). colour might be better described by what it represents (white) or, even better, what it's intended usage is: text_color (system_text_color?) or maybe foreground_color. font might be borderline acceptable, but even there a more descriptive name like system_font or similar would help to better communicate its intended usage.
• If you have to use console output for error reporting, prefer std::cerr over std::cout. Even though they might look the same for a human observer on the console window, they can easily be distiguished from normal console output by other programs.
• There are a lot of "magic numbers" (constants without obvious meaning). While I can rather confidently estimate that "assets/fonts/caliban.ttf" is the path to a font file, I just have to guess what the default background color is: "fully opaque black" (RGBA)? "bright transparent blue" (ARGB)? Or maybe something completely different?

• hoffmale: If interested in modern C++ facilities that help with correctness and memory management, this CppCon talk by Herb Sutter might be interesting.
• user162687: Thanks for the answer. I didn't know that even a simple setup like this already has quite tight coupling. I will definitely try to put separate logic into different systems and I will have a look at the talk you suggested.
• hoffmale: @IoanThomas: Well, if this would be the final product, the coupling could be considered ok. However, since this is just the base for future progress, if not adressed now it will soon be too deeply rooted to easily refactor out.