SDL Input Managment

Question

I've created an SDL input manager for my 3D OpenGL applications. It's not finished yet, but I would like to know if I'm doing it the right way.

I really tried to optimise and compact my code, as it is for an OpenGL library.

InputHandler.h

#pragma once
#include <SDL2/SDL.h>
#include <vector>
#include <glm/glm.hpp>
#include "SDL_Handle.h"

#define TheInputHandler InputHandler::Instance() //Yes... Macros aren't good... but we can't ude typdef, or anything else :)

class InputHandler
{
    public:
        static InputHandler& Instance(){
            static InputHandler instance;
            return instance;
        }

        void Update();
        inline bool ExitRequested(){ return _hasExit; }
        inline glm::uvec2 GetMousePosition(){ return _mousePosition; }
        inline bool KeyDown(SDL_Scancode key){
            return _keyStates != 0 ? _keyStates[key] : false;
        }

        ///disable assignemant operators
        InputHandler(InputHandler const&) = delete;
        InputHandler& operator=(InputHandler const&) = delete;
    private:
        InputHandler();/// singleton pattern
        virtual ~InputHandler();
        SDL_Handle _handle;
        bool _hasExit = false;
        glm::uvec2 _mousePosition;
        const Uint8* _keyStates = nullptr;
};

InputHandler.cpp

InputHandler::InputHandler(){
    _handle = SDL_Handle(); /// init SDL : uses RAII to avoid initializing twice !
}
InputHandler::~InputHandler(){}

void InputHandler::Update(){
    _keyStates = SDL_GetKeyboardState(0);
    SDL_Event event;
    while(SDL_PollEvent(&event))
    {
        switch(event.type)
        {
            case SDL_QUIT:
                _hasExit = true;
                break;
            case SDL_MOUSEMOTION:
                _mousePosition = glm::uvec2(event.motion.x, event.motion.y);
                break;
        }
    }
}

Answer 1

Right so you're using a Singleton pattern because you find it reasonable that there will only ever be one input handler. You correctly implement the singleton pattern in C++, which is good. The bad thing is that the pattern in of itself is bad.

A better design is to make InputHandler an interface with virtual functions. Then create a concrete instance SDLInputHandler which implements InputHandler. Then, wherever you need to parse inputs you pass a pointer to a InputHandler. In your main class you can simply have an instance of SDLInputHandler which you pass pointers to.

An added benefit of this is that you can implement an InputHandler that can generate a fixed sequence of inputs, this is very useful for testing your code or for playing back a sequence of user inputs, like a recorded game for example.

If you at this point are thinking:

No way, Jose! I'm not going to be passing that pointer all over my application!

Then that indicates a bigger issue you're having in your overall design. The parts of your application that are directly dealing with input should be quite limited and small. If you indeed are having this phenomena, you should really look into the Command Pattern. It will make your life easier.

edit

If I'm not mistaken, all names that start with an underscore are reserved for the implementation and all your member variable names are technically not allowed.

Answer 2

The most important thing about the singleton pattern is to learn when NOT to use it.

Maybe you only need one input manager in your application. Perhaps the one-instance requirement is justifiable. But think about this: suppose you were to add input support for a gamepad/controller. With this architecture, everything would have to go inside InputHandler. Mouse handling, keyboard, gampepad. It would become a massive singleton that tries to handle every possible input device combination. Rather, if you had other classes, such as GamePadInput and MouseKeyboardInput your code would be much better organized and easier to maintain.

Before settling with a all-in-one global point of access, consider other approaches first. Read the link above. @EmilyL's answer also covers that in good detail.

---

The global point of access:

I've seen this used many times in combination with the lazy-initialization singleton:

#define TheInputHandler InputHandler::Instance()

I personally don't like it. Not because of the macro usage per-se, but because it obfuscates the code. When I call TheInputHandler it will seem like I'm referencing a global variable. However, if I have to track down the declaration, surprise, surprise! It is actually calling a function.

I like code to be as explicit as possible. From my experience, it makes for more robust code. If sticking to a singleton, I would not provide that macro. Don't lie to users of your class. Let them know they are calling into a singleton class.

Other options include: making every method and data static, to avoid the need for a Instance() function, or declaring TheInputHandler as an actual global variable.

---

Two superfluous things in your class:

• inline for methods that already declared inline (no pun intended) inside the class body in the header file. In this case they keyword adds nothing and may be removed.

• A virtual private destructor for a class that is not inherited from. The very fact that the destructor is private will ensure that no instance of the class is ever directly deleted, so even if you were to inherit from it, the virtual destructor would still be unnecessary. Also see When to use virtual destructors? on SO.