1
\$\begingroup\$

I have written simple input manager which is used for me to detect key press, and fire assigned function. For example I can do something like that:

auto own_pointer = std::make_shared<Player>(*this);
inputControl.addKeyToCheck( sf::Keyboard::H, std::function<void( Player& )>( &Player::sayHello ),own_pointer );

and in my loop manager, I check input managers, and if H is pressed, then function sayHello will be fired.

I have written moving my player with this input manager.

auto own_pointer = std::make_shared<Player>(*this);
inputControl.addKeyToCheck( sf::Keyboard::A, std::function<void( Player& )>( &Player::moveLeft ),own_pointer );
inputControl.addKeyToCheck( sf::Keyboard::D, std::function<void( Player& )>( &Player::moveRight ), own_pointer );
inputControl.addKeyToCheck( sf::Keyboard::S, std::function<void( Player& )>( &Player::moveDown ), own_pointer );
inputControl.addKeyToCheck( sf::Keyboard::W, std::function<void( Player& )>( &Player::moveTop ), own_pointer );

where for example moveTop is just

void Player::moveTop()
{
    this->getComponent<Velocity>()->y -= speed * mv::constants::mob::DEFAULT_SPEED;
}

Generally I am so proud of my InputManager, it works everywhere else but there is really bad to use 4 different function to control moving. Have somebody any advice how to deal with that style problem?

Input manager

    /**
    * @brief Class which manage keys' input
    */
    template <class T>
    class InputManager
    {
        /* ===Objects=== */
    public:
    protected:
    private:
        //when you press KEY you execute FunctionWrapper
        std::map < sf::Keyboard::Key, FunctionPointerWrapper_t<T>> keyData;
        /* ===Methods=== */
    public:
        /**
        * @brief Checks if keys have been clicked
        */
        void update();

        /**
        * @bried adds key to database
        */
        bool addKeyToCheck( sf::Keyboard::Key key, std::function<void( T& )> function, std::shared_ptr<T> object );

        /**
        * @brief remove key from database
        */
        bool eraseKey( sf::Keyboard::Key key );
    protected:
    private:
    };

    template <class T>
    void InputManager<T>::update()
    {
        for ( auto&var : keyData )
            if ( sf::Keyboard::isKeyPressed( var.first ) )
                var.second.function( *var.second.object );
    }

    template <class T>
    bool InputManager<T>::addKeyToCheck( sf::Keyboard::Key key, std::function<void( T& )> function, std::shared_ptr<T> object )
    {
        keyData.emplace( key, FunctionPointerWrapper_t<T>( function, object ) );

        return true;
    }

    template <class T>
    bool InputManager<T>::eraseKey( sf::Keyboard::Key key )
    {
        keyData.erase( key );
        return false;
    }
\$\endgroup\$
  • 1
    \$\begingroup\$ I'm not sure this question is a good fit for CodeReview. If you posted the input manager class we could suggest general improvements, some of which might cover what you're asking. However, for a specific question like this you might have better luck on gamedev.stackexchange.com . \$\endgroup\$ – user673679 Feb 13 at 13:59
  • \$\begingroup\$ this question is only about style, so I think that it is good. Ok, I am going to edit question \$\endgroup\$ – VirtualUser Feb 13 at 14:27
  • \$\begingroup\$ Can you provide a usage example of how your input manager is invoked and in what manner? \$\endgroup\$ – Mast Feb 13 at 14:46
  • \$\begingroup\$ @Mast it is just invoked by inputManager.update() in main game loop. \$\endgroup\$ – VirtualUser Feb 13 at 15:42
3
\$\begingroup\$

FunctionPointerWrapper_t seems unnecessary; we can store a std::function<void()> in the map directly. To create this function, we can either use std::bind:

auto player = std::make_shared<Player>(*this);
inputControl.addKeyToCheck( sf::Keyboard::H, std::bind(&Player::sayHello, player) );

or a lambda function:

auto player = std::make_shared<Player>(*this);
inputControl.addKeyToCheck( sf::Keyboard::H, [=] () { player->sayHello(); } );

Note that both of these examples internally create a copy of the shared_ptr.


For cutting down on the number of named functions in the player class, we can route the functionality to a single move function, something like:

void Player::move(Vector2f const& speed) { getComponent<Velocity>() += speed * mv::constants::mob::DEFAULT_SPEED; }

We can then use std::bind, or lambda functions as above:

inputControl.addKeyToCheck( sf::Keyboard::W, std::bind(&Player::move, player, Vector2f(0.f, -1.f)) );
inputControl.addKeyToCheck( sf::Keyboard::W, [=] () { player->move(Vector2f(0.f, -1.f)); });

Where Vector2f(0.f, -1.f) can be replaced by whatever the actual desired type / value is. (We could also do the multiplication outside of the move function, if it's not common between the functions).

We still need to define 4 functions, but there's less overhead in the Player class.

To simplify it further, we'd probably want to define some sort of axis mapping, so we could do something like:

inputControl.addAxis(Axis(sf::Keyboard::W, sf::Keyboard::S, maxSpeed, minSpeed), [=] (float axisValue) { player->moveVertically(axisValue); });

(and perhaps even allow a 2d-axis to be defined mapping 4 keys to one function call) but that might be a fair amount of extra work.


General comments:

  • InputManager::addKeyToCheck and InputManager::eraseKey should probably not return a fixed true / false without checking if they actually succeeded.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.