Tron Game Remake in SDL

Recently, just for fun, I've created a clone of the arcade game Tron. Since I am still a novice when it comes to C++, I would really appreciate all the feedback I can get from those with more experience.

Cleanup.h

#ifndef TRON_CLEANUP_H
#define TRON_CLEANUP_H

#include 
#include 

/*
 * Recurse through the list of arguments to clean up, cleaning up
 * the first one in the list each iteration.
 */
template
void cleanup(T *t, Args&&... args){
    //Cleanup the first item in the list
    cleanup(t);
    //Recurse to clean up the remaining arguments
    cleanup(std::forward(args)...);
}
/*
 * These specializations serve to free the passed argument and also provide the
 * base cases for the recursive call above, eg. when args is only a single item
 * one of the specializations below will be called by
 * cleanup(std::forward(args)...), ending the recursion
 * We also make it safe to pass nullptrs to handle situations where we
 * don't want to bother finding out which values failed to load (and thus are null)
 * but rather just want to clean everything up and let cleanup sort it out
 */
template<>
inline void cleanup(SDL_Window *win){
    if (!win){
        return;
    }
    SDL_DestroyWindow(win);
}
template<>
inline void cleanup(SDL_Renderer *ren){
    if (!ren){
        return;
    }
    SDL_DestroyRenderer(ren);
}
template<>
inline void cleanup(SDL_Texture *tex){
    if (!tex){
        return;
    }
    SDL_DestroyTexture(tex);
}
template<>
inline void cleanup(SDL_Surface *surf){
    if (!surf){
        return;
    }
    SDL_FreeSurface(surf);
}

#endif //TRON_CLEANUP_H
Contact GitHub API Training Shop Blog About

Controller.h

#ifndef TRON_CONTROLLER_H
#define TRON_CONTROLLER_H

#include "Includes.h"
#include "Player.h"

class Controller {
public:
    void control();

    Controller(Player *p1, Player *p2);
private:
    SDL_Event current_event;

    Player *p1_pointer = nullptr,
           *p2_pointer = nullptr;
};

#endif //TRON_CONTROLLER_H

Contro

Couple of bugs!

File names typos

You seem to be using a case-insensitive OS. I've had to rename Resources/pixelated.ttf to Pixelated.ttf and cmake/FindSDL2_image.cmake to FindSDL2_Image.cmake.

Input issues

SDL_PollEvents only returns the first event waiting in the event queue. Thus, the code in Controller::control only handles one event per frame. For this reason, the game was at first completely unresponsive. I've quickly changed it to:

while(SDL_PollEvent(¤t_event))
    if (current_event.type == SDL_KEYDOWN) {
        // Rest of the code...

Also, SDL KeySyms are good for text editing, but not great for game controls because they depend on the keyboard's layout. WASD in particular end up mapped to ZQSD on an AZERTY keyboard. Prefer scancodes for that.

The actual review

These sections are in no particular order, they are pretty much my notes while I'm reading through the various files and refactoring as I go.

The magic cleanup function!

I like what you've gone for in Cleanup.h, abstracting the various cleanup functions behind a single one. There are, however, a couple of things that can be improved.

Overload rather than specialize

M. Sutter has explained it here better than I ever could. In short: function template specializations act weird with overload resolution, so it's better to stick with plain overloads when they do the job:

inline void cleanup(SDL_Window *win) {
    if (!win) {
        return;
    }
    SDL_DestroyWindow(win);
}

// And so on.

Pack expansions are cool

Okay, they don't look that great in C++11. But it is definitely a good idea to get used to them, because C++17 brings along fold expressions, which makes the syntax really neat. In cases like this one, where you just want to repeat something on each argument of a pack, recursion with perfect forwarding is not your only option:

template
void cleanup(Args *... args) {

    // C++11, constructing a dummy `int` array
    using ex = int[];
    (void)ex{(cleanup(args), 0)..., 0};

    // C++17
    (void)(cleanup(args), ...);
}

Can't clean this! Now what?

Now is a good time to think about the error case of our cleanup function. What happens when a developer lacking caffeine tries to call cleanup with something that doesn't have a suitable implementation ? Your original code produces this error output, with no hint towards the actual error:

In instantiation of ‘void cleanup(T*, Args&& ...) [with T = int; Args = {}]’:
  required from ‘void cleanup(T*, Args&& ...) [with T = int; Args = {SDL_Window*&}]’
  required from ‘void cleanup(T*, Args&& ...) [with T = SDL_Renderer; Args = {int*&, SDL_Window*&}]’
error: no matching function for call to ‘cleanup()’
     cleanup(std::forward(args)...);
            ^
note: candidate: template void cleanup(T*, Args&& ...)
 void cleanup(T *t, Args&&... args){
      ^
note:   template argument deduction/substitution failed:
note:   candidate expects at least 1 argument, 0 provided
     cleanup(std::forward(args)...);
            ^

Switching from specializations to overloads produces the same output. Using pack-expansion rather than recursion has... unfortunate results in this case: cleanup ends up calling itself recursively, compiling fine but blowing up the stack at runtime. Not ideal. To avoid both of these unpleasant symptoms of an error, we can declare a catch-all function which will be chosen whenever no version of cleanup can take in the argument.

template 
void cleanup(T *) = delete;

Now it fails with a shorter and more helpful error:

error: use of deleted function ‘void cleanup(T*) [with T = int]’

This is enough for a trained developer to recognize the deleted catch-all pattern: he needs to implement cleanup(int*)! But we can do better:

template 
struct zero {
    static constexpr int value = 0;
};

template 
void cleanup(T *) {
    static_assert(zero::value, "No overload of `cleanup` for this type.");
}

The zero::value is here just to make sure the condition of static_assert depends on the template parameter, so it is only checked upon instantiating (trying to call) the function. Now the message appears clearly in the error log, guiding the developer towards solving the issue.

When to call cleanup?

So this cleanup function neatly reduces the syntax for cleaning up a couple of resources, being smart enough to know what to do depending on their types. But when do we actually call it? Turns out, C++ actually has a feature to know when to cleanup stuff: destructors! Rather than carefully calling cleanup at all the right places so nothing is left behind, even in the wildest cases of exception propagation, we can wrap every resource inside an object, whose destructor's job is to clean it up. This habit is known as RAII, probably the worst misnomer ever, but hey -- it's incredibly useful.

So the most visible resources are the window, the renderer and the background, three pointers with specia

StackExchange Code Review Q#151523, answer score: 10