App initialisation and simple game loop

Question

Summary

I come from a Java background but I am trying to make a game in C++. This is my attempt at a state management system, which should allow me to easily switch between "states" (menu, game, scoreboard, etc.).

The idea is that:

• When the program starts, I create an Application.
• The Application contains the game loop, which runs until the program exits.
• Each frame, the application updates and renders the current state.
• State is an abstract class, and Rival is a concrete subclass.

Feedback

I would really love any feedback. Specifically, the areas I am most concerned about are:

• Inheritance: I have not really used this before in C++. My understanding is that by using a unique_ptr my state gets stored on the heap, and this avoids the problem of object slicing. My State methods are all pure virtual, and overridden by the subclass. Am I missing anything?
• Ownership: The Application owns the current State; Rival owns the current Scenario. My understanding is that when the Application exits (or changes to a new state), the current state will be destroyed / freed. When Rival gets freed, the current Scenario will subsequently be freed. Have I got that right?
• Heap vs Stack: I understand that the stack is faster to access, but it is quite small and not particularly suitable for long-lived objects (they are freed when they go out of scope), polymorphic objects or variable-size objects. For this reason the State and Scenario live on the heap, but everything else lives on the stack. Does this sound ok?

NOTE: I am not too worried about the technical aspects of the game loop itself at this point (fixed or variable time step, sleep duration, etc.) - I just want to make sure the code is clean, free from bugs / memory leaks, and follows best practices where possible. I would be grateful if you could try to include an explanation along with any suggestions, so that I can learn WHY and not just WHAT.

Code

I have tried to omit any details which are not relevant to this particular mechanism, but the full code can be found here.

Main.cpp

#include "pch.h"

#include <iostream>
#include <stdexcept>

#include "Application.h"
#include "Rival.h"
#include "Scenario.h"
#include "ScenarioBuilder.h"
#include "ScenarioReader.h"
#include "Window.h"

/**
 * Entry point for the application.
 */
int main() {

    try {

        // Create our Window
        Rival::Window window(800, 600, "Rival Realms");
        window.use();

        // Create our Application
        Rival::Application app(window);

        // Load some scenario
        Rival::ScenarioReader reader(Rival::Resources::mapsDir + "example.sco");
        Rival::ScenarioBuilder scenarioBuilder(reader.readScenario());
        std::unique_ptr<Rival::Scenario> scenario = scenarioBuilder.build();

        // Create our initial state
        std::unique_ptr<Rival::State> initialState =
                std::make_unique<Rival::Rival>(app, std::move(scenario));

        // Run the game!
        app.start(std::move(initialState));

    } catch (const std::runtime_error& e) {
        std::cerr << "Unhandled error during initialization or gameplay\n";
        std::cerr << e.what() << "\n";
        return 1;
    }

    return 0;
}

Application.h

#ifndef APPLICATION_H
#define APPLICATION_H

#include <memory>

#include "Resources.h"
#include "State.h"
#include "Window.h"

namespace Rival {

    class Application {

    public:
        bool vsyncEnabled;

        Application(Window& window);

        /**
         * Runs the Application until the user exits.
         */
        void start(std::unique_ptr<State> state);

        /**
         * Exits the Application cleanly.
         */
        void exit();

        Window& getWindow();
        Resources& getResources();

    private:
        Window& window;
        Resources res;
        std::unique_ptr<State> state;
    };

}  // namespace Rival

#endif  // APPLICATION_H

Application.cpp

#include "pch.h"
#include "Application.h"

#include <SDL.h>

namespace Rival {

    bool vsyncEnabled = true;

    Application::Application(Window& window)
        : window(window) {

        // Try to enable vsync
        if (SDL_GL_SetSwapInterval(1) < 0) {
            printf("Unable to enable vsync! SDL Error: %s\n", SDL_GetError());
            vsyncEnabled = false;
        }
    }

    void Application::start(std::unique_ptr<State> initialState) {

        // Event handler
        SDL_Event e;

        state = std::move(initialState);
        bool exiting = false;
        Uint32 nextUpdateDue = SDL_GetTicks();

        // Game loop
        while (!exiting) {
            Uint32 frameStartTime = SDL_GetTicks();

            // Is the next update due?
            if (vsyncEnabled || nextUpdateDue <= frameStartTime) {

                // Handle events on the queue
                while (SDL_PollEvent(&e) != 0) {
                    if (e.type == SDL_QUIT) {
                        exiting = true;
                    } else if (e.type == SDL_KEYDOWN) {
                        state->keyDown(e.key.keysym.sym);
                    } else if (e.type == SDL_MOUSEWHEEL) {
                        state->mouseWheelMoved(e.wheel);
                    }
                }

                // Update the game logic, as many times as necessary to keep it
                // in-sync with the refresh rate.
                //
                // For example:
                //  - For a 30Hz monitor, this will run twice per render.
                //  - For a 60Hz monitor, this will run once per render.
                //  - For a 120Hz monitor, this will run every other render.
                //
                // If vsync is disabled, this should run once per render.
                while (nextUpdateDue <= frameStartTime) {
                    state->update();
                    nextUpdateDue += TimerUtils::timeStepMs;
                }

                // Render the game, once per iteration.
                // With vsync enabled, this matches the screen's refresh rate.
                // Otherwise, this matches our target FPS.
                state->render();

                // Update the window with our newly-rendered game.
                // If vsync is enabled, this will block execution until the
                // next swap interval.
                window.swapBuffers();

            } else {
                // Next update is not yet due.
                // Sleep for the shortest possible time, so as not to risk
                // overshooting!
                SDL_Delay(1);
            }
        }

        // Free resources and exit SDL
        exit();
    }

    void Application::exit() {
        SDL_Quit();
    }

    Window& Application::getWindow() {
        return window;
    }

    Resources& Application::getResources() {
        return res;
    }

}  // namespace Rival

State.h

#ifndef STATE_H
#define STATE_H

#include <SDL.h>

namespace Rival {

    // Forward declaration to avoid circular reference
    class Application;

    class State {

    public:
        /**
         * Handles keyDown events.
         */
        virtual void keyDown(const SDL_Keycode keyCode) = 0;

        /**
         * Handles mouse wheel events.
         */
        virtual void mouseWheelMoved(const SDL_MouseWheelEvent evt) = 0;

        /**
         * Updates the logic.
         *
         * It is assumed that a fixed amount of time has elapsed between calls
         * to this method, equal to TimerUtils::timeStepMs.
         */
        virtual void update() = 0;

        /**
         * Renders the current frame.
         */
        virtual void render() = 0;
    };

}  // namespace Rival

#endif  // STATE_H

Rival.h

#ifndef RIVAL_H
#define RIVAL_H

#include <SDL.h>
#include <memory>

#include "Application.h"
#include "Scenario.h"
#include "State.h"
#include "Window.h"

namespace Rival {

    class Rival : public State {

    public:
        Rival(Application& app, std::unique_ptr<Scenario> scenario);

        // Inherited from State
        void keyDown(const SDL_Keycode keyCode) override;
        void mouseWheelMoved(const SDL_MouseWheelEvent evt) override;
        void render() override;
        void update() override;

    private:
        Application& app;
        Window& window;
        Resources& res;
        std::unique_ptr<Scenario> scenario;
    };

}  // namespace Rival

#endif  // RIVAL_H

Rival.cpp

#include "pch.h"
#include "Rival.h"

namespace Rival {

    Rival::Rival(Application& app, std::unique_ptr<Scenario> scenarioToMove)
        : app(app),
          window(app.getWindow()),
          res(app.getResources()),
          scenario(std::move(scenarioToMove)) {}

    void Rival::update() {
        // ...
    }

    void Rival::render() {
        // ...
    }

    void Rival::keyDown(const SDL_Keycode keyCode) {
        // ...
    }

    void Rival::mouseWheelMoved(const SDL_MouseWheelEvent evt) {
        // ...
    }

}  // namespace Rival
```

Answer 1

Answers to your questions

Inheritance: I have not really used this before in C++. My understanding is that by using a unique_ptr my state gets stored on the heap, and this avoids the problem of object slicing. My State methods are all pure virtual, and overridden by the subclass. Am I missing anything?

Object slicing happens when you copy a derived class variable into a base class variable. Using any kind of pointer prevents a copy from being made. However, you probably want to use a pointer (or reference) anyway, even if there was no object slicing.

Ownership: The Application owns the current State; Rival owns the current Scenario. My understanding is that when the Application exits (or changes to a new state), the current state will be destroyed / freed. When Rival gets freed, the current Scenario will subsequently be freed. Have I got that right?

Yes, as soon as a class is destroyed, all its member variables are destroyed as well. If a member variable is a std::unique_ptr, this will ensure delete is called on the pointer.

Heap vs Stack: I understand that the stack is faster to access, but it is quite small and not particularly suitable for long-lived objects (they are freed when they go out of scope), polymorphic objects or variable-size objects. For this reason the State and Scenario live on the heap, but everything else lives on the stack. Does this sound ok?

The main thread of an application typically has megabytes of stack space on a desktop computer, so I wouldn't worry about it that much. For regular variables, even if their type is that of a large class, it will mostly be fine, but if you start allocating arrays on the stack you have to be careful. The lifetime depends on the lifetime of the scope, but that can be very long; for example the variables allocated on the stack frame of main() will basically live for as long as the program lives.

As for faster access: the only issue with variables on the heap is that they are access via a pointer, so at some point the pointer has to be dereferenced. This may or may not be an issue for performance. I wouldn't worry about it in the early stages of your program, it is something you can worry about later if you are doing performance tuning, and only then if a profiler tells you that this is actually a problem.

It should be fine to declare a State and Scenario variable on the stack of main():

// Load some scenario
Rival::ScenarioReader reader(Rival::Resources::mapsDir + "example.sco");
Rival::ScenarioBuilder scenarioBuilder(reader.readScenario());
Rival::Scenario scenario = scenarioBuilder.build();

// Create our initial state
Rival::Rival initialState(scenario);

// Run the game!
app.start(initialState);

This requires the constructor of Rival::Rival and Application::start() to take a plain reference as an argument. This means those objects also no longer own the scenario and state. But it should be fine, those variables will now be destroyed when main() exits.

Don't catch exceptions if you can't do anything about them

In main(), you catch any std::runtime_error(), but the only thing you do is to print an error and exit with a non-zero exit code. This is exactly what will already happen if you don't catch exceptions there, so it is a pointless excercise. Perhaps Java taught you that you have to catch 'm all, but that's not the case in C++. Just let fatal exceptions you can't deal with fall through.

Aside from that, if you do want to have a generic exception catcher, then you should catch std::exception instead, it's the base class of std::runtime_error and will also catch other types of exceptions.

Not everything needs to be a class

Again, I think this comes from your background in Java, where all functions must live inside a class. This is not the case in C++. In particular, class Application is just something that you construct once, call start() on, and then it exits and you're done. For such a one-shot operation, you can just use a single function. Since Application primarily implements the main loop of your application, I would just create a single function called main_loop():

void main_loop(Window& window, State& initialState) {
    bool vsyncEnabled = SDL_GL_SetSwapInterval(1) == 0;

    if (!vsyncEnabled) {
            printf("Unable to enable vsync! SDL Error: %s\n", SDL_GetError());
    }

    SDL_Event e;
    bool exiting = false;
    Uint32 nextUpdateDue = SDL_GetTicks();

    // Game loop
    while (!exiting) {
        ...
    }
}

And then in main():

Rival::Window window(800, 600, "Rival Realms");
...
Rival::State initialState(scenario);

// Run the game!
main_loop(window, initialState);

Do you need inheritance at all?

Is there a reason why you have made the pure virtual base classes Rival::State? If you only have one derived class Rival::Rival, it really does not do anything, except you have to now keep the members of the base class and the derived class in sync, which is work for you, and now access to the state will have to go via a vtable, which might impact performance. Even if you think you might need it in the future, the YAGNI principle applies here: if you don't need it now, don't write it.

Don't call SDL_Quit() too early

In your original code, after exitting the main loop, you call Application::exit(), which in turn calls SDL_Quit(). However, as far as I can tell, nothing in class Application ever initialized SDL, so it should also not deinitialize it. In particular, the destructor of the variable window in main() will be called afterwards, so that might still rely on SDL being initialized properly.

Consider moving event handling into its own function

In the main loop, you have a switch() statement handling all possible SDL events. Consider moving this part into its own function, so that the main loop looks as simple as possible:

while (!exiting) {
    handle_events(); // or maybe state.handle_events()?
    state.update();
    state.render();
    window.swapBuffers();
}

This will keep the main loop short, and give you a clear high-level overview of what you are doing for each frame you render.

Avoid busy-waiting and arbitrary delays

If you want to wait for some time to pass or an event to happen, never implement a busy wait, nor a loop that calls SDL_Delay(1). That is just going to waste CPU cycles, and while the SDL_Delay(1) statement will certainly use less cycles, waiting for just a millisecond will likely prevent the processor from going into a low power state while you are waiting for the next update. This means it will have a higher temperature, which could cause thermal throttling to kick, and for users on a battery-operated device, they will drain their batteries faster.

If you know that nextUpdateDue > frameStartTime, just call SDL_Delay(nextUpdateDue - frameStartTime).

Answer 2

My understanding is that by using a unique_ptr my state gets stored on the heap, and this avoids the problem of object slicing

Well... technically, no. When you have a pointer to a base class pointing to a derived class, non virtual methods still get sliced. However, since we allocate on the heap and pass around a pointer to the base class, that information isn't "lost", just "hidden".

My understanding is that when the Application exits (or changes to a new state), the current state will be destroyed / freed. When Rival gets freed, the current Scenario will subsequently be freed. Have I got that right?

Yes (since you are using a smart pointer), but it's always best practice to define a destructor for your class (even if it is just default destructor). Especially in the State class, you need to define a virtual destructor, otherwise the compiler won't know it has to call the derived class' destructor as well. If your derived class' destructor does something non-trivial, it won't be called when the object is destroyed and that can lead to all sorts of nastiness.

I understand that the stack is faster to access, but it is quite small and not particularly suitable for long-lived objects

Not really. Allocating on the stack is faster than on the heap because it involves a bit more bookkeeping, but unless you're allocating thousands of objects on the heap every frame, it's rarely is an issue.

Now onto the code:

1. Why the call to window.use()? Just call it in the constructor. As an aside, window.use is kind of a weird name of what the method is doing, something like window.init might be more suitable.

2. Unlike Java, C++ doesn't require you to use classes for everything. ScenarioReader and ScenarioBuilder can be rewritten as free-standing functions, something like this: auto scenario_desc = readScenario(scenarioPath); auto scenario = buildScenario(scenario_desc); . Even better, you can put them inside a common namespace (something like ScenarioUtils and group them.

3. Application::exit should be moved inside the destructor, so it is automatically called on destruction, or if an exception occurred.

4. Application::getWindow should return a const Window& if you don't want the calling code to be able to modify Window. Also, make sure to use const modifiers whenever possible.