Input handling system using the command pattern

Question

The online book Game Programming Patterns briefly describes the use of the command pattern to handle input in a game. I've attempted to write my own "one size fits all" input handling system based on the command pattern, as shown below.

• Is this a correct implementation and use of the command pattern?
• Would it be "better" (more efficient, more maintainable) to use another pattern (e.g: observer pattern)?
• How do I adapt it to handle input from multiple controllers?

input_handler.hpp

#ifndef INPUT_HANDLER_HPP
#define INPUT_HANDLER_HPP

#include <map>
#include <vector>
#include <SDL2/SDL.h>
#include "character.hpp"
#include "input_constants.hpp"

class Command
{
    public:
        virtual ~Command() {}
        virtual void execute(Character *character) = 0;
        virtual InputType get_input_type() = 0;
};

class InputHandler
{
    private:
        // Pointers to all commands
        Command *move_up;
        Command *move_down;
        Command *move_left;
        Command *move_right;
        Command *jump;

        std::map <int, Command*> commands;

        // Gameplay context 
        std::map <int, State> state_map;
        std::map <int, Action> action_map;

        bool input_mapping();
        void dispatcher(std::vector<Command*> &command_queue);

        void keydown(SDL_Event &event);
        void keyup(SDL_Event &event);

        bool is_held(int key);
        bool was_pressed(int key);

    public:
        InputHandler();
       ~InputHandler();
        bool fill(std::vector<Command*> &command_queue);
        void configure(int key, Command *command);
};

class MoveUp : public Command
{
    public:
        void execute(Character *character) { character->move_up(); }
        InputType get_input_type() { return STATE; }
};

class MoveLeft : public Command
{
    public:
        void execute(Character *character) { character->move_left(); }
        InputType get_input_type() { return STATE; }
};

class MoveRight : public Command
{
    public:
        void execute(Character *character) { character->move_right(); }
        InputType get_input_type() { return STATE; }
};

class MoveDown : public Command
{
    public:
        void execute(Character *character) { character->move_down(); }
        InputType get_input_type() { return STATE; }
};

class Jump : public Command
{
    public:
        void execute(Character *character) { character->jump(); }
        InputType get_input_type() { return ACTION; }
};

#endif  // INPUT_HANDLER_HPP

input_handler.cpp

#include "input_handler.hpp"

InputHandler::InputHandler()
{

    // Create pointers to all commands (to apply the flyweight pattern)
    move_up = new MoveUp();
    move_down = new MoveDown();
    move_left = new MoveLeft();
    move_right = new MoveRight();
    jump = new Jump();

    // Player 1
    commands[SDLK_UP]       = move_up;
    commands[SDLK_LEFT]     = move_left;
    commands[SDLK_DOWN]     = move_down;
    commands[SDLK_RIGHT]    = move_right;
    commands[SDLK_SPACE]    = jump;

    // Player 2
    //commands[SDLK_w]        = move_up;
    //commands[SDLK_a]        = move_left;
    //commands[SDLK_s]        = move_down;
    //commands[SDLK_d]        = move_right;
    //commands[SDLK_LSHIFT]   = jump;
}

void InputHandler::configure(int key, Command *command)
{
    commands[key] = command;    // key points to newly assigned command
}

bool InputHandler::fill(std::vector<Command*> &command_queue)
{
    bool exit = input_mapping();    // converts raw input datum to an action and/or state

    if (exit) return true;
    else {
        dispatcher(command_queue);  // fills command queue
        action_map.clear();         // clears key presses
        return false;
    }
}

bool InputHandler::input_mapping()
{
    SDL_Event event;
    while (SDL_PollEvent(&event))
        if (event.type == SDL_QUIT) return true;
        else if (event.type == SDL_KEYDOWN) {
            if (event.key.keysym.sym == SDLK_ESCAPE) return true;
            keydown(event);
        }
        else if (event.type == SDL_KEYUP)
            keyup(event);

    return false;
}

void InputHandler::dispatcher(std::vector<Command*> &command_queue)
{
    std::map<int, Command*>::iterator iter;
    for (iter = commands.begin(); iter != commands.end(); iter++) {
        if (is_held(iter->first) && iter->second->get_input_type() == STATE)
            command_queue.push_back(iter->second);
        else if (was_pressed(iter->first) && iter->second->get_input_type() == ACTION)
            command_queue.push_back(iter->second);
    }
}

void InputHandler::keydown(SDL_Event &event)
{
    if (state_map[event.key.keysym.sym] == RELEASED)
        action_map[event.key.keysym.sym] = EXECUTE;
    state_map[event.key.keysym.sym] = PRESSED;
}

void InputHandler::keyup(SDL_Event &event)
{
    state_map[event.key.keysym.sym] = RELEASED;
}

bool InputHandler::is_held(int key)
{
    return state_map[key];
}

bool InputHandler::was_pressed(int key)
{
    return action_map[key];
}

InputHandler::~InputHandler()
{
    // Delete all command pointers    
    std::map<int, Command*>::iterator iter;
    for (iter = commands.begin(); iter != commands.end(); iter++)
        delete iter->second;
}

input_constants.hpp

#ifndef INPUT_CONSTANTS_HPP
#define INPUT_CONSTANTS_HPP

enum InputType
{
    ACTION,
    STATE,
    RANGE
};

enum Action
{
    EXECUTE = true,
    STOP = false
};

enum State
{
    PRESSED = true,
    RELEASED = false
};

#endif // INPUT_CONSTANTS_HPP

game.hpp (only relevant lines)

#include "input_handler.hpp"

class Game
{
    private:
        bool exit;
        InputHandler *input_handler;
        Character *character;
        void update();

    public:
        void execute();
};

game.cpp (only relevant lines)

#include "game.hpp"

Game::Game(int screen_width, int screen_height)
{
    exit = false;

    // Initialize input handler
    input_handler = new InputHandler();

    // Create character
    character = new Character("Rouge", 100, 300);

    // Command queue
    std::vector<Command*> command_queue;
}

void Game::execute()
{
    while(!exit) {
        // Handle input
        exit = input_handler->fill(command_queue);
        update ();
    }
}

void Game::update()
{
    // Update character state
    while (!command_queue.empty()) {
        command_queue.back()->execute(character);
        command_queue.pop_back();
    }
}

Answer 1

Is this a correct implementation and use of the command pattern?

Yes, I think it is good enough.

Would it be better (more efficient, more maintainable) to use another pattern?

Performance wise it is hard to tell if other solutions would be faster. This one is quite maintainable and scalable, but I am biased in this case, since I really appreciate the Command pattern.

One performance optimization that you might do in the future if to get rid of the std::maps that you are using right now. You could very well use arrays, since your map keys are just integer constants. If you take to trouble of ensuring that the enum constants are sequential, you can replace:

std::map<int, Command*> commands;
std::map<int, State>    state_map;
std::map<int, Action>   action_map;

With plain arrays:

Command* commands[MAX_COMMAND_INDEX];
State    state_map[MAX_STATE_INDEX];
Action   action_map[MAX_ACTION_INDEX];

Or better still, with a C++11 array:

std::array<Command*, MAX_ACTION_INDEX> commands;
std::array<State,    MAX_ACTION_INDEX> state_map;
std::array<Action,   MAX_ACTION_INDEX> action_map;

General improvements:

You could use smart pointers to enforce a safer object ownership policy. The C++11 std::shared_ptr would be a good choice:

typedef std::shared_ptr<Command> CommandPtr;

This ensures a Command never gets destroyed when it is passed around and frees you from the burden of manually deleting them in the destructor of InputHandler.

In fact, you should use smart pointers for most if not all objects, this includes the Character pointers:

typedef std::shared_ptr<Character> CharacterPtr;

So that in virtual void execute(CharacterPtr character) of Command the character pointer is always valid while to function is being executed, even if the original pointer's referenced is lost in some other thread.

Better naming:

The public function:

void configure(int key, Command *command);

of InputHandler is responsible for the binding of a given key to a command, thus, a better name would be bind:

void bind(int key, CommandPtr command);

fill is also a vague name for a function that is responsible for generating the input commands for a game frame:

bool fill(std::vector<Command*> &command_queue);

I would recommend renaming it to:

bool generate_input(std::vector<CommandPtr> &command_queue);

or even more explicit:

bool generate_input_commands(std::vector<CommandPtr> &command_queue);

Don't be afraid of using long names. The less margin you leave for ambiguity, the better.

bool input_mapping();

Is also a somewhat poor name. Take the description commend you've written on its call site and try to come-up with a very descriptive name of what the function does:

// converts raw input datum to an action and/or state

You could rename it to:

bool convert_inputs_to_actions();

bool map_inputs_to_actions();

bool do_input_to_action_mapping();

And the list goes on.

void dispatcher(std::vector<Command*> &command_queue);

This one actually fills the command queue. The name you've chosen is good for a type (E.g.: A Dispatcher class) but not quite proper for a function. You could rename it to:

void dispatch_commands(std::vector<CommandPtr> &command_queue);

Or more simply:

void fill_command_queue(std::vector<CommandPtr> &command_queue);

Mind your style of control flow layout:

This one might be disregarded as personal taste, but I think it is worth mentioning.

I'd like to recommend that you mind your style of placing the return statements on the same line of an if clause. For example, in this block:

while (SDL_PollEvent(&event))
    if (event.type == SDL_QUIT) return true;
    else if (event.type == SDL_KEYDOWN) {
        if (event.key.keysym.sym == SDLK_ESCAPE) return true;
        keydown(event);
    }
    else if (event.type == SDL_KEYUP)
        keyup(event);

I find the mixture of styles and bracing hard to read. It would be a lot easier on the eyes if you were to reformat as:

while (SDL_PollEvent(&event))
    if (event.type == SDL_QUIT)
        return true;
    else if (event.type == SDL_KEYDOWN) {
        if (event.key.keysym.sym == SDLK_ESCAPE)
            return true;
        keydown(event);
    }
    else if (event.type == SDL_KEYUP)
        keyup(event);

But even better and safer if you added uniform bracing on all statements, even in the single line ones:

while (SDL_PollEvent(&event)) {
    if (event.type == SDL_QUIT) {
        return true;
    } else if (event.type == SDL_KEYDOWN) {
        if (event.key.keysym.sym == SDLK_ESCAPE) {
            return true;
        }
        keydown(event);
    } else if (event.type == SDL_KEYUP) {
        keyup(event);
    }
}

With uniform bracing, if you need to add more than one command to an if result there is no risk of doing something stupid like:

if (x == 42)
    do_something();
    do_some_other_thing();

The correct indentation fools you into thinking that both lines belong to the result of the first if. If the programmer had bracing form the beginning, he would be immune to this sort or errors.

How to adapt it to handle input from multiple controllers?

You have hard-coded some game specific logic into the constructor of InputHandler. The most straightforward and possible best fix would be subclassing.

You can define one or more virtual methods that must be implemented by the client of your code:

class InputHandler
{
    // all the previous stuff...

protected:

    // Classes extending InputHandler will define their
    // own mappings inside this method.
    virtual void setup_input_mappings() = 0;
};

So that then you could have one InputHandler type for each hardware/platform you wish to support:

class InputHandlerXBOXController : public InputHandler
{
    void setup_input_mappings()
    {
        commands[XB_Y]  = move_up;
        commands[XB_X]  = move_left;
        commands[XB_A]  = move_down;
        commands[XB_B]  = move_right;
        commands[XB_L2] = jump;
    }
};

You can use this same concept of Template Methods to insulate the rest of InputHandler from SDL, for example.

Answer 2

I don't think you implemented the command pattern in the proper way.

The idea behind command pattern is to encapsulate the logic of receiver method execution, from the receiver. A good example for that would be a model class. You don't wan't that the model to change it's own state. So good way to achieve this is to encapsulate all the settings methods of the model in commands.

And your command indeed does that, how ever it also does that:

virtual InputType get_input_type() = 0;

This defiantly does not belong there.

You are using your command state to determinate your client(InputHandler) behavior. You supposed to use a model for that.

Think about better way to initialize your command, and also consider moving command_queue to model as well, so your client or controller will only do logic and will not deal with application states, even if it's not a business logic.