Review of simple Input Manager.

[Whittler]

Hello!
I am during my first project in SFML in which I decided to create some kind of game engine that I can use in my future games. I made an action manager which has its members marked as static.

.hpp file:

#include "Input/input.hpp"

#include <unordered_map>

namespace WhitE {

class ActionManager
{
public:
        static void addAction(const std::string&, sf::Keyboard::Key);
        static void addAction(const std::string&, std::vector<sf::Keyboard::Key>);

        static void deleteAction(const std::string& actionName);
        static bool isActionPressed(const std::string& actionName);
        static bool isMouseButtonPressed(sf::Mouse::Button);

private:
        inline static Input mInput;
        inline static std::unordered_map<std::string, std::vector<sf::Keyboard::Key>> mKeyboardMap;
};

}
 

.cpp file:

#include "Input/actionManager.hpp"
#include "Logger/logs.hpp"

namespace WhitE {

void ActionManager::addAction(const std::string& actionName, sf::Keyboard::Key key)
{
        mKeyboardMap.insert(std::make_pair(actionName, std::vector<sf::Keyboard::Key> {key}));

        WE_LOG_INFO(actionName + " action was added to the manager.");
}

void ActionManager::addAction(const std::string& actionName, std::vector<sf::Keyboard::Key> keys)
{
        mKeyboardMap.insert(std::make_pair(actionName, keys));
}

void ActionManager::deleteAction(const std::string& actionName)
{
        if (mKeyboardMap.find(actionName) != mKeyboardMap.end())
                mKeyboardMap.erase(actionName);
        else
                WE_LOG_WARNING("WhitE: Key of map cannot be found!");
}

bool ActionManager::isActionPressed(const std::string& actionName)
{
        auto keys = mKeyboardMap.at(actionName);
        for (const auto& key : keys)
        {
                if (mInput.isKeyPressed(key))
                        return true;
        }
}

bool ActionManager::isMouseButtonPressed(sf::Mouse::Button button)
{
        return mInput.isMouseButtonPressed(button) ? true : false;
}

}
 

And then I've got a simple class which handles real-time input

#include "Input/actionManager.hpp"

#include <SFML/Window.hpp>

namespace WhitE {

class Input
{
private:
        bool isKeyPressed(sf::Keyboard::Key key) { return sf::Keyboard::isKeyPressed(key); }
        bool isMouseButtonPressed(sf::Mouse::Button button) { return sf::Mouse::isButtonPressed(button); }

        friend class ActionManager;
};

}
 

My question is if it even makes sense? In case of checking input I'd need to type "if(ActionManager::isActionPressed("someAction")) doSomeStuff" which is not the most convinient style but I have no idea how it could be rearranged. Any help would be appreciated.

[h3rname]

It's like what I'm doing for my first project for APKNite. Basically, classes are just a way for us stupid humans organize code and understand it better. I think it's running in the right way.

[Elias Daler]

The scheme looks okay. You can also report an error if action name that's passed to one of the functions is not correct, so that misspelled actions are easy to spot in code, e.g.

if(ActionManager::isActionPressed("someAction")) doSomeStuff(); // ok
if(ActionManager::isActionPressed("someActoin")) doSomeStuff(); // throws/logs error

Also, I'd strongly advice against using "static" classes which are really Singletons. I prefer Service Locator pattern a lot more.

[Nexus]

Some ideas:

• If you use an enum class instead of strings for actions, your actions are type-safe (no mistypes possible)
• Service locator is an option, but it's often misused as a lazy design pattern. Input is not something you need to access from anywhere in the application, and you should strive to decouple it from game logic. If you design class relations carefully, you can instantiate the Input class once, and delegate its actions (or results thereof) to other components, like the one which steers your player.
• You might also be interested in the Thor.Input module I wrote (docs, tutorial)

[Elias Daler]

Some ideas:

• If you use an enum class instead of strings for actions, your actions are type-safe (no mistypes possible)

Agreed, but this only works if the actions are defined statically. In my game I load them and input mapping from JSON, so it's not always a viable option.

• Service locator is an option, but it's often misused as a lazy design pattern. Input is not something you need to access from anywhere in the application, and you should strive to decouple it from game logic. If you design class relations carefully, you can instantiate the Input class once, and delegate its actions (or results thereof) to other components, like the one which steers your player.

Agreed. I struggled with that, but managed to put all input logic into handleInput functions which take InputManager by reference. At times it's still useful to access InputManager globally, but one should generally avoid doing so. That being said, I think that Service Locator is less harmful than Singleton. [/list]

[Nexus]

Agreed, but this only works if the actions are defined statically. In my game I load them and input mapping from JSON, so it's not always a viable option.

If the actions are dynamic, would you even have string literals in your code?

If just the binding is dynamic (i.e. there is always an action "attack", but how it's mapped is defined in a config/script file), then you could verify those upon loading, and fail on incorrectly mapped strings.

Serializing enums is a pain in C++ indeed, but it's also possible to have a list of string constants instead of an enum. Which of course is only useful if you use them in multiple and/or different places.

That being said, I think that Service Locator is less harmful than Singleton.

I guess it depends on how you access the service locator. If the locator is global or a singleton itself, then you win nothing, all you have is an extra indirection.

Singletons/all-static classes are bad because of the same reasons global variables are bad, and those apply as well to a global service locator