3
\$\begingroup\$

I've written a Controller for the keyboard so that I can map certain keys to callbacks within a game. A key can only call one callback, but a callback can be called by many keys.

Currently, a Controller has two std::maps - one for when the key(s) is pressed/held, and one for when the key(s) are released. These maps are SDL_Scancode to std::string (Which I may change later to an enum rather than a string? But strings are easier to read so maybe not.) The Controller also has a ControllerContext object, which is just a wrapper around two std::maps of std::string to std::function<void(void)>: one map for callbacks to happen when a key is pressed, another for callbacks to happen when a key is released.

I feel like some kind of change could be made to make the code more readable, and I am unsure of if I should have the context stored in a shared_ptr, or if I should just keep a raw pointer within the Controller.

This is how it would work:

//An event union provided by SDL
//https://www.libsdl.org/release/SDL-1.2.15/docs/html/sdlevent.html
SDL_Event e;
//A controller
Controller controller_;
ControllerContext context_;

//Build the context
controller_.SetContext(&context);

//Poll all events in the event queue, feeding them to the structure e
while(SDL_PollEvent(&e))
{
   switch(e.type)
       case SDL_KEYDOWN:
           controller_.OnKeyPress(e.key);
           break;
       case SDL_KEYUP:
           controller_.OnKeyRelease(e.key);
           break;
       default: 
           break;
}

keyboardcontroller.h

#ifndef SHMUPPY_HEADER_KEYBOARDCONTROLLER_H_
#define SHMUPPY_HEADER_KEYBOARDCONTROLLER_H_

#include <map>
#include <SDL.h>
#include <iostream>
#include <exception>
#include <string>
#include <memory>


struct ControllerContext;

class Controller
{
public:
    Controller();

    int OnKeyPress(const SDL_Event &e);

    int OnKeyRelease(const SDL_Event &e);

    void SetContext(std::shared_ptr<ControllerContext> context_);

protected:
    std::map<SDL_Scancode, std::string> keypress_action_map_;
    std::map<SDL_Scancode, std::string> keyrelease_action_map_;
    std::shared_ptr<ControllerContext> controller_context_;
};


#endif

keyboardcontroller.cpp

#include "KeyboardController.h"
#include "ControllerContext.h"


Controller::Controller()
{
    //Some test initializations
    keypress_action_map_[SDL_SCANCODE_LEFT] = "Left";
    keypress_action_map_[SDL_SCANCODE_DOWN] = "Down";
    keypress_action_map_[SDL_SCANCODE_RIGHT] = "Right";
    keypress_action_map_[SDL_SCANCODE_UP] = "Up";


    keyrelease_action_map_[SDL_SCANCODE_LEFT] = "Left";
    keyrelease_action_map_[SDL_SCANCODE_DOWN] = "Down";
    keyrelease_action_map_[SDL_SCANCODE_RIGHT] = "Right";
    keyrelease_action_map_[SDL_SCANCODE_UP] = "Up";
}

int Controller::OnKeyPress(const SDL_Event& e)
{
    try
    {
        //std::cout << keypress_action_map_.at(e.key.keysym.scancode) << std::endl;
        controller_context_->keypress_events_.at(keypress_action_map_.at(e.key.keysym.scancode))();
        return 1;
    }
    catch (std::exception ex)
    {
        return 0;
    }
}

int Controller::OnKeyRelease(const SDL_Event& e)
{
    try
    {
        std::cout << keyrelease_action_map_.at(e.key.keysym.scancode) << std::endl;
        return 1;
    }
    catch (std::exception ex)
    {
        return 0;
    }
}


void Controller::SetContext(std::shared_ptr<ControllerContext> context_)
{
    controller_context_ = context_;
}

ControllerContext.h

#ifndef SHMUPPY_HEADER_CONTROLLER_CONTEXT_H
#define SHMUPPY_HEADER_CONTROLLER_CONTEXT_H


#include <map>
#include <functional>

struct ControllerContext
{
    std::map<std::string, std::function<void(void)>> keypress_events_;
    std::map<std::string, std::function<void(void)>> keyrelease_events_;
};


#endif
\$\endgroup\$

3 Answers 3

3
\$\begingroup\$

Edit: Tested implementations below.

As jason says, returning either 0 or 1, looks like boolean. Change int to bool and return true or false.

Drop the std::string layer. It doesn't add anything over enum. And enum are just as readable and there is no way typos can appear with enums.

I don't have any experience with SDL myself, but I've written a keyboard -> action controller in an attempt at a game.

I'd also like to drop the std::map in favor of a std::vector. The trick is to use continuous enums and use those as indices for the std::vector.

Something like this:

enum class KeyboardEvents : byte
{
    ON_RELEASE,
    ON_PRESS,

    ORDINAL
};

typedef std::function<void(void)> keyboardFunc_t;

std::vector<keyboardFunc_t> keyboardEvents(KeyboardEvents::ORDINAL);

void bindControls()
{
    auto onReleaseLambda = [&]()
    {
        // Do stuff
    };
    auto onPressLambda = [&]()
    {
        // Do stuff
    };

    // Bind with lambdas
    keyboardEvents[KeyboardEvents::ON_RELEASE] = onReleaseLambda;
    keyboardEvents[KeyboardEvents::ON_PRESS] = onPressLambda;

    // Alternatively, use std::bind
    keyboardEvents[KeyboardEvents::ON_RELEASE] = std::bind(&ClassName::onRelease, classInstancePtr);
    keyboardEvents[KeyboardEvents::ON_PRESS] = std::bind(&ClassName::onPress, classInstancePtr);
}

I just wrote this on the top off my head. I'm not sure if it'll compile, but you get the idea.

I would also prefer lambdas in favor of std::bind.

As for storing std::shared_ptr<ControllerContext> controller_context_; in a raw or shared_ptr, I'd say neither. But DO use std::unique_ptr. Using a unique_ptr will make sure your controller class is not default copyable, which is a good thing when dealing with limited resources.

Like this std::unique_ptr<ControllerContext> controller_context_;

Edit: From here and on:

Note, don't look too closely on main.cpp, it was mainly copied from somewhere to get SDL2 running.

ControllerContext.h:

#ifndef SHMUPPY_HEADER_CONTROLLER_CONTEXT_H
#define SHMUPPY_HEADER_CONTROLLER_CONTEXT_H


#include <vector>
#include <functional>

struct ControllerContext
{

    ControllerContext()
        : keyPressEvents(SDL_NUM_SCANCODES)
        , keyReleaseEvents(SDL_NUM_SCANCODES)
    {
        keyPressEvents.resize(SDL_NUM_SCANCODES);
        keyReleaseEvents.resize(SDL_NUM_SCANCODES);
    }

    typedef std::function<void(void)>   KeyboardFunc_t;
    typedef std::vector<KeyboardFunc_t> KeyEvents;

    KeyEvents   keyPressEvents;
    KeyEvents   keyReleaseEvents;
};


typedef std::unique_ptr<ControllerContext>  ControllerContext_ptr;

#endif

keyboardcontroller.h:

#ifndef SHMUPPY_HEADER_KEYBOARDCONTROLLER_H_
#define SHMUPPY_HEADER_KEYBOARDCONTROLLER_H_

#include <map>
#include <SDL.h>
#include <iostream>
#include <exception>
#include <string>
#include <memory>


#include "ControllerContext.h"



class Controller
{
public:
    Controller();

    bool OnKeyPress(const SDL_Event &e);

    bool OnKeyRelease(const SDL_Event &e);

    // Clear by setting empty func
    void SetReleaseCallback(SDL_Scancode code, const ControllerContext::KeyboardFunc_t & func);
    void SetPressCallback(SDL_Scancode code, const ControllerContext::KeyboardFunc_t & func);

    // Clear both press and release
    void ClearCallback(SDL_Scancode code);

protected:
    ControllerContext_ptr controller_context_;
};


#endif

keyboardcontroller.cpp:

#include "KeyboardController.h"
#include "ControllerContext.h"


Controller::Controller()
    : controller_context_(new ControllerContext())
{
    //Some test initializations
    auto leftPress = [&]()
    {
        // Left press lambda
        int debug=0;
    };
    auto leftRelease = [&]()
    {
        // Left release lambda
        int debug=0;
    };

    SetPressCallback(SDL_SCANCODE_LEFT, leftPress);
    SetReleaseCallback(SDL_SCANCODE_LEFT, leftRelease);
}

bool Controller::OnKeyPress(const SDL_Event &e)
{
    const auto & scancode = e.key.keysym.scancode;

    const auto & pressFunc = controller_context_->keyPressEvents[scancode];

    if (pressFunc)
    {
        // Call callback if valid
        pressFunc();
        return true;
    }

    return false;
}

bool Controller::OnKeyRelease(const SDL_Event &e)
{
    const auto & scancode = e.key.keysym.scancode;

    const auto & pressFunc = controller_context_->keyReleaseEvents[scancode];

    if (pressFunc)
    {
        // Call callback if valid
        pressFunc();

        return true;
    }

    return false;
}

void Controller::SetReleaseCallback(SDL_Scancode code, const ControllerContext::KeyboardFunc_t & func)
{
    controller_context_->keyReleaseEvents[code] = func; 
}

void Controller::SetPressCallback(SDL_Scancode code, const ControllerContext::KeyboardFunc_t & func)
{
    controller_context_->keyPressEvents[code] = func;
}

void Controller::ClearCallback(SDL_Scancode code)
{
    controller_context_->keyPressEvents[code] = ControllerContext::KeyboardFunc_t();
    controller_context_->keyReleaseEvents[code] = ControllerContext::KeyboardFunc_t();
}

main.cpp:

#include <SDL.h>
#include <iostream>

class TestMain
{
    public:
        void handlePressW()
        {
            // W press
            int debug=0;
        }
        void handleReleaseW()
        {
            // W release
            int debug=0;
        }
};

#include "keyboardcontroller.h"

int _stdcall WinMain(struct HINSTANCE__ *hInstance, struct HINSTANCE__ *hPrevInstance, char *lpszCmdLine, int nCmdShow)
{
    if (SDL_Init(SDL_INIT_EVERYTHING) < 0)
    {
        /* Handle problem */
        fprintf(stderr, "%s\n", SDL_GetError());
    }
    SDL_Window* window = SDL_CreateWindow("Window caption", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED, 852, 480, 0);
    if (window == NULL)
    {
        /* Handle problem */
        fprintf(stderr, "%s\n", SDL_GetError());
        SDL_Quit();
    }
    SDL_Renderer* renderer = SDL_CreateRenderer(window, -1, 0);
    if (renderer == NULL)
    {
        /* Handle problem */
        fprintf(stderr, "%s\n", SDL_GetError());
        SDL_Quit();
    }


    Controller keycontroller;

    // Bind testmain methods
    TestMain testmain;
    keycontroller.SetPressCallback(SDL_SCANCODE_W, std::bind(&TestMain::handlePressW, testmain));
    keycontroller.SetReleaseCallback(SDL_SCANCODE_W, std::bind(&TestMain::handleReleaseW, testmain));


    bool running = true;
    while (running)
    {
        /* Clear the buffer of color, setting it to black */
        SDL_RenderClear(renderer);

        /* Draw the buffer into the window */
        SDL_RenderPresent(renderer);

        /* Handle input and events: */
        SDL_Event sdl_event;
        while (SDL_PollEvent(&sdl_event) > 0)
        {
            switch (sdl_event.type)
            {
                case(SDL_KEYDOWN):
                {
                    keycontroller.OnKeyPress(sdl_event);
                    break;
                }

                case(SDL_KEYUP):
                {
                    keycontroller.OnKeyRelease(sdl_event);
                    break;
                }


                case(SDL_QUIT):
                    running = false;
                    break;
            }

        }

        /* Run your logic. For example: */
    }

    SDL_Quit();

    return 0;
}
\$\endgroup\$
6
  • \$\begingroup\$ Hi, and welcome to Code Review. You've passed though the first-post review queue with flying colours! \$\endgroup\$
    – rolfl
    Commented Jun 13, 2015 at 12:42
  • \$\begingroup\$ @rolfl: Thank you very much. I just hope this will be of any help to OP and/or any other. \$\endgroup\$
    – Kent
    Commented Jun 13, 2015 at 14:10
  • \$\begingroup\$ @Kent it is helpful, but I am having a hard time understanding some of it. For example, you suggest using a unique_ptr for the ControllerContext object, but in your bindControls() method you're bypassing the ControllerContext object and setting the methods straight to a vector of functions. I'm assuming that both of those get stuck into my ControllerContext object? I also don't quite understand the enum class. What is ORDINAL? The number of keys on the keyboard? And by using the enums ON_RELEASE and ON_PRESS, only two methods can be assigned to the vector? \$\endgroup\$ Commented Jun 13, 2015 at 16:51
  • 1
    \$\begingroup\$ @DanielMartin: That code was more pseudolike code to show how I'd do it. The ORDINAL in the enum is number of actions you have, though when I come to think of it, actions should be KEY_A, KEY_E and so on. I'll update my code with real code soon. \$\endgroup\$
    – Kent
    Commented Jun 13, 2015 at 19:04
  • 1
    \$\begingroup\$ @DanielMartin: I've added some more code. \$\endgroup\$
    – Kent
    Commented Jun 13, 2015 at 22:26
2
\$\begingroup\$

Your OnKeyPress and OnKeyRelease methods return 1 or 0. That looks suspiciously as true or false, in which case bool would be the correct return type, with true and false as the returned values.

I don't understand the need for this mapping chain logic: SDL_Scancode -> std::string -> std::function<void(void)>. It seems you could cut out the string in the middle, and use SDL_Scancode -> std::function<void(void)> directly.

\$\endgroup\$
1
  • \$\begingroup\$ The chaining of SDL_Scancode -> std::string -> std::function<void(void)> was to get a layout of the controller, but support different actions across different states. For example, if I set my space key -> "Action", then in the game state I could do "Action"->ShootMethod, in the pause state I could do "Action"->Accept, etc etc. This way the player could set up the controller to behave the same way across states, instead of the controller only behaving one way for one state(Action button only does actions in gameplay, but action button does nothing if I bring up the pause menu? Lame) \$\endgroup\$ Commented Jun 13, 2015 at 16:37
1
\$\begingroup\$

This is silly:

int Controller::OnKeyRelease(const SDL_Event& e) 
{
    try
    {
        std::cout << keyrelease_action_map_.at(e.key.keysym.scancode) << std::endl;
        return 1;
    }
    catch (std::exception ex)
    {
        return 0;
    } 
}

If you don't want to use exceptions, then don't call at(). Catching the exception to return an error code is a clear sign that at() is not what you want. You should use find() instead:

bool Controller::OnKeyRelease(const SDL_Event& e)
{
    const auto entry = keyrelease_action_map_.find(e.key.keysym.scancode);
    if (entry == std::end(keyrelease_action_map_))
    {
        return false;
    }

    std::cout << entry->second << std::endl;
    return true;
}

That being said, I would actually prefer the exception approach in this case, keeping at() but letting the error propagate to the caller. Well, not necessarily. If a value not being present in the map is an exceptional thing, then propagating the exception is adequate. If a value is only optionally in the map, then testing with find() would be the adequate approach, with the optional return false with it.

If you do keep the return code to signal error, then at the very least use a boolean. Integer error codes are very prone to confusion and there isn't a single agreement on meaning. E.g.: on Unix, some system calls return 0 on success and -1 on error, while others threat the value as boolean and return 0 or 1. Madness, I say!

\$\endgroup\$
5
  • \$\begingroup\$ That being said, I would actually prefer the exception approach in this case, keeping at() but letting the error propagate to the caller. Something like this, where the try-catch is wrapped around the call to the function, rather than within the function itself ? ideone.com/ci6gAp \$\endgroup\$ Commented Jun 13, 2015 at 18:28
  • \$\begingroup\$ @DanielMartin, certainly not like that. Handle the exception if there is a way to recover from the error, otherwise let the program terminate. \$\endgroup\$
    – glampert
    Commented Jun 13, 2015 at 18:37
  • \$\begingroup\$ In both examples what is being done is to convert the exception to an error code, so that it can be ignored. This is not error handling and a violation of the fail fast principle by not letting the error propagate and terminate the program. \$\endgroup\$
    – glampert
    Commented Jun 13, 2015 at 18:48
  • \$\begingroup\$ I see, I just misunderstood you then. I'll switch to using find(), since I don't want the program to terminate if they press a button on the keyboard but there is no method attached to it. Although I am curious what you mean to let the error propagate to the caller. \$\endgroup\$ Commented Jun 13, 2015 at 19:13
  • 1
    \$\begingroup\$ @DanielMartin Ah, okay, so I also misinterpreted your code. I thought that a key not being in the map was a more serious error, judging by your use of exceptions. If a key is optionally on the map, then find is really what you should be using. at is for when you want to either get a value or fail with a hard error (the exception). \$\endgroup\$
    – glampert
    Commented Jun 14, 2015 at 2:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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