2
\$\begingroup\$

I am looking for advice on my input component. There is no real problem with it yet, but I am just getting started on everything and want to know if there is any obvious problems with it, plus I was looking for advice on a design question of it.

The design question refers to whether or not I should treat alt, ctrl, and shift like mod keys or separate keys. When I thought about it at first, it made sense. This way gives more shortcut key possibilities (i.e. macros in games like Ultima Online) or I assumed how games like Counter Strike control run/walk (if shift is held with move key, and it affects how much speed the move gives) but the more I think about it, in Counter Strike, it could be the modifying key is just another key affecting a total speed. So I'm at a point in which I wonder if my efforts to make the modded key functions aren't really the right way to go.

Second, in my class, I use std::map<key,function<void()>> where key is a struct that contains mods/scancode values. Then it finds the associated function from the map and calls it if it exists. I don't know if it is wise to search a map every time the key is pressed down, held, and released. Is this something I should concern myself with at this point in development? Should I assume it works fine, move on, and find it slow later to change it then?

TL;DR care about modded keys in input class? Are there any obvious problems with my design?

#ifndef __INPUT_H__
#define __INPUT_H__

#include <SDL.h>
#include <iostream>
#include <map>
#include <vector>
#include <functional>

#include "Vector2f.h"
#include "MouseButton.h"

struct key {
    SDL_Scancode    code;
    Uint16          kmod;

    key() {
        code = SDL_SCANCODE_UNKNOWN;
        kmod = KMOD_NONE;
    }

    key(SDL_Scancode kcode, Uint16 kkmod) {
        code = kcode;
        kmod = kkmod;
    }

    key(const key& source) {
        code = source.code;
        kmod = source.kmod;
    }

    bool operator<(const key& rhs) const {
        return std::tie( code,kmod ) < std::tie( rhs.code, rhs.kmod );
    }

    bool operator==(const key& rhs) const {
        return std::tie( code,kmod ) == std::tie( rhs.code, rhs.kmod );
    }
};

class Input {

private:

    std::vector<key> HeldKeys;

    std::map<key,std::function<void()>> OnDownFuncs;
    std::map<key,std::function<void()>> OnHoldFuncs;
    std::map<key,std::function<void()>> OnUpFuncs;

    MouseButton LClick; //unimplemented yet
    MouseButton RClick; //unimplemented yet

    const int NUM_KEYCODES;
public:


    Input() : NUM_KEYCODES(256) {

        HeldKeys.clear();

    }

    void update();  
    void KeyDown(SDL_KeyboardEvent key);
    void KeyUp(SDL_KeyboardEvent key);
    void MouseDown(int key);
    void MouseUp(int key);
    void RegOnKeyDown(SDL_Scancode key, Uint16 kmod, std::function<void()> func);
    void RegOnKeyHold(SDL_Scancode key, Uint16 kmod, std::function<void()> func);
    void RegOnKeyUp(SDL_Scancode key, Uint16 kmod, std::function<void()> func);
};

#endif

Input.cpp

#include "Input.h"



void Input::update() {

    for( std::vector<key>::iterator it = HeldKeys.begin(); it != HeldKeys.end(); it++ ) {
        std::map<key,std::function<void()>>::iterator func = OnHoldFuncs.find(*it);
        if( func != OnHoldFuncs.end() )
            func->second();
    }

}

void Input::KeyDown(SDL_KeyboardEvent pkey) {
    if( pkey.repeat == 1 ) 
        return;

    key nkey(pkey.keysym.scancode,pkey.keysym.mod);                        

    std::map<key,std::function<void()>>::iterator func = OnDownFuncs.find(nkey);
    if( func != OnDownFuncs.end() )
        func->second();
    else
        std::cout << "unknown key " << pkey.keysym.scancode << " is pressed" << std::endl;

    func = OnHoldFuncs.find(nkey);
    if( func != OnHoldFuncs.end() )
        HeldKeys.push_back(nkey);

    return;
}

void Input::KeyUp(SDL_KeyboardEvent pkey) {

    key nkey(pkey.keysym.scancode,pkey.keysym.mod);

    std::map<key,std::function<void()>>::iterator func = OnUpFuncs.find(nkey);
    if( func != OnUpFuncs.end() )
        func->second();
    else
        std::cout << "unknown key " << pkey.keysym.scancode << " is released" << std::endl;

    for( std::vector<key>::iterator it = HeldKeys.begin(); it != HeldKeys.end(); it++ ) {
        if(*it == nkey) {
            HeldKeys.erase(it);
            break;
        }
    }

}

void Input::MouseDown(int key) {

}

void Input::MouseUp(int key) {

}



void Input::RegOnKeyDown(SDL_Scancode code, Uint16 kmod, std::function<void()> func) {
    key tkey; 
    tkey.code = code;
    tkey.kmod = kmod;

    OnDownFuncs.insert(std::pair<key,std::function<void()>>(tkey,func));

    return;
}

void Input::RegOnKeyHold(SDL_Scancode code, Uint16 kmod, std::function<void()> func) {
    key tkey; 
    tkey.code = code;
    tkey.kmod = kmod;

    OnHoldFuncs.insert(std::pair<key,std::function<void()>>(tkey,func));

    return;
}

void Input::RegOnKeyUp(SDL_Scancode code, Uint16 kmod, std::function<void()> func) {
    key tkey; 
    tkey.code = code;
    tkey.kmod = kmod;

    OnUpFuncs.insert(std::pair<key,std::function<void()>>(tkey,func));

    return;
}
\$\endgroup\$
3
\$\begingroup\$

A little bit of background

There are two basic device input models that most applications use. Games will most certainly use one of these two or both. I'll briefly elaborate on them to give you some ideas.

Polled input:

Your application constantly queries the system/device to check if new input has occurred. Usually the devices have a buffer/queue of the recent input that accumulate until your program checks them. In such a setup, you would have functions like isKeyDown(Key) and isKeyUp(key) that your applications calls whenever it needs to receive some input. For this approach, it doesn't really matter much if you tie keys to modifiers, since you can simply check for both the key and modifier at the same time. Assume we have a fictional Player on a first person game where we move forward with W and begin to run with Shift:

if (isKeyDown(Key::W) && isKeyDown(Key::Shift))
{
    player.setState(Player::Running);
}

This approach is very straightforward and easy to implement. This is more often then not the approach used in video games because of its simplicity.

Event or interrupt based input:

In this setup, the application registers some callback with the system/device that will be fired whenever a new input arrives. There are variants of this when the application might need to call some library function to generate the events, e.g.: SDL_PollEvent(), this is a sort of hybrid between polled and event-based input.

Your approach is clearly event based, since the users of Input register callback functions that get run whenever you have new input. For this approach, having the key modifier makes more sense, since each event can only refer to one key at a time. Taking the W/Shift example from before, it will either fire a W or a Shift event callback in sequence, but it can't fire both callbacks simultaneously, so you need the modifier to tell if Shift was down at the same time of W.

This approach is more complicated and takes more code to set up. The advantage is being able to remap keys to callbacks very easily, even at runtime if needed be. Though you can also do that with the simpler polled input, only you have to approach the problem at a higher level instead (I.e. perform the mapping inside the Player class).


Okay, enough chat, lets take a look at your current implementation and see what can be objectively improved. Points roughly in order of appearance in the code:

  • Don't use identifiers in your code containing double underscore (like __INPUT_H__), that naming convention is reserved for compiler and standard library uses.

  • Prefer using PascalCase for types and camelCase for variable and function names, to distinguish both kinds of things. It would be better to name key as Key, with the big K, so you can for example declare: Key key = ....

  • Key struct doesn't need a copy constructor. It has only Plain Old Data (POD) members, so the compiler will provide a default bit-wise copy constructor and assignment operator.

  • Always initialize your member data in the constructor's initializer list. There is no difference in the generated code for native/POD types, but it will avoid redundant work for complex objects. If you init a member string, for instance, with the assignment operator, it will first go thru the default constructor to then re-assign it in the sequence. Keep your code uniform and always use the initializer list to avoid duplicate work when dealing with objects.

    Key() 
        : code(SDL_SCANCODE_UNKNOWN)
        , kmod(KMOD_NON)
    { }
    
  • operator < in Key is arguable. I don't recall even comparing a key for ordering. However, I believe you added this to make it compatible with the standard map, so in that case it is understandable. Another option might be providing the map with a comparator functor/struct that you can make private, so Key doesn't expose the unexpected operator.

  • For both Key operators, using std::tie seems to only obfuscate the code. I actually had to lookup in the reference to remember what std::tie was for. This isn't a function used often I'm afraid, so most programmers are likely unfamiliar with it. In this case, since you only deal with two variables, using plain comparisons would be much better, in my opinion:

    bool operator == (const key& rhs) const 
    {
        return code == rhs.code && kmod == rhs.kmod;
    }
    // Code gets shorter too!
    
  • I find it better to place the public interface of a class first in the header file, to give it more emphasis. That's usually the part you and users of your code will be looking at more often. protected and private are implementation details, so shouldn't need to stand out as much.

  • NUM_KEYCODES is not a "real" constant, but just a const member integer, so all instances of Input will have an instance of that variable. To make it a true compile-time constant, declare it as static and initialize it inline:

    // inside class Input 
    static const int NUM_KEYCODES = 256;
    

    Note that you can (and arguably should) use constexpr if your target is C++11 and above.

  • No need to HeldKeys.clear() in the constructor. The vector is initially empty!

  • return at then of a void function is unnecessary and only makes for more verbosity.

  • The necessity of HeldKeys is not quite clear to me. Aren't the key maps enough? If you could get rid of it, it would save you some memory and extra indirections.

  • Use auto to cleanup things like std::map<key,std::function<void()>>::iterator, or at lest typedef std::map<key,std::function<void()>> if you can't use C++11.


I'm running out of time now, so I can't dig further into your implementation and architecture, so I'll leave you with a reference implementation you can look at to get more ideas from. Take a look at the Key Input class used by the Doom 3 game. It is somewhat similar to yours, but uses a polling approach instead.

\$\endgroup\$
  • \$\begingroup\$ Yeah, the copy constructor was because I was having problems trying to put a key into a vector and having a run time error so I tried implementing my own copy constructor. Turned out to be a different problem. The < operator is indeed for the use of key as well, the key, in std::map functions. \$\endgroup\$ – Chemistpp Jul 23 '15 at 21:56
  • \$\begingroup\$ The HeldKeys vector is in case a held key needs to do something very update iteration. Though, I see one point you make is that I can write the key down functions in such a way to handle doing something until it is released. The held keys can be kind of redundant in that way. Thanks again, the rest of this input I'll take into account and revamp it once. Much appreciated. \$\endgroup\$ – Chemistpp Jul 23 '15 at 21:56

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.