Game Design Patterns Command implementation

Question

I started reading the GameProgrammingPatterns book and wanted to implement the command pattern.

namespace bebop::patterns
{

struct Command
{
    using Fn = void (*)(void*);  // Command return type is void
    Fn exec;      // The function to be executed when execute() is called
    void* m_ctx;  // The context for the function

    void execute() const { exec(m_ctx); }
};

template <typename T, typename Method, typename... Args>
struct CommandImpl
{
    CommandImpl(T* obj, Method method, Args&&... args)
        : m_ctx{obj, method, std::make_tuple(std::forward<Args>(args)...)}
    {
        m_command = Command{&trampoline, &m_ctx};
    }

    void execute() const { m_command.execute(); }

    Command command() const { return m_command; }

   private:
    struct Context
    {
        T* obj;
        Method method;
        std::tuple<Args...> args;
    };

    Context m_ctx;

    static void trampoline(void* raw)
    {
        auto* m_ctx = static_cast<Context*>(raw);
        std::apply(
            [m_ctx](auto&&... unpacked_args)
            {
                (m_ctx->obj->*m_ctx->method)(
                    std::forward<decltype(unpacked_args)>(unpacked_args)...);
            },
            m_ctx->args);
    }

    Command m_command;
};

// Deduction guide
template <typename T, typename Method, typename... Args>
auto make_command(T* obj, Method method, Args&&... args)
{
    return CommandImpl<T, Method, std::decay_t<Args>...>(
        obj, method, std::forward<Args>(args)...);
}

}  // namespace bebop::patterns

Decided to implement without virtual functions because we all know that virtual functions add runtime overhead.std::function is adding overhead too, so the only right way to statically do this, without even heap overhead is this.

• To generalize the command function we shall have a function pointer, usually when executing a command action we are not interested in the return result, but the command action function may consume some parameters. So to generalize it we shall have a void(*)(void*) function pointer.
• To have somehow any context about the function we must also store a void* ctx to store the context of the function somewhere.

Now, its gonna be an overhead for an engineer to create the command pointer, the context to create command. So to have some sugar coding lets add a template make_command which receives the pointer to the object, the action of the object, and the parameters that action shall take. But to implement this, we cannot use any captures , std::function or etc... So we must somehow write a trampoline for our case. Thats why templated CommandImpl is used, to create a right command with the trampoline method and right context(with the arguments injected as a tuple). And the templated make_command just returns that implementation.

The usage:

#include <iostream>
#include <memory>
#include <unordered_map>
#include <utility>
#include "Command.hpp"

struct Player
{
    void moveUp() { std::cout << "UP\n"; }
    void moveDown() { std::cout << "DOWN\n"; }
    void moveRight() { std::cout << "RIGHT\n"; }
    void moveLeft() { std::cout << "LEFT\n"; }
    void shoot(int n) { std::cout << "Shoot " << n << "\n"; }
};

struct InputHandler
{
    void bind(char c, bebop::patterns::Command cmd) { bindings[c] = cmd; }

    bebop::patterns::Command* getCommand(char c)
    {
        auto it = bindings.find(c);
        return it != bindings.end() ? &it->second : nullptr;
    }

    template <typename CmdImpl>
    void bindOwned(char c, CmdImpl&& impl)
    {
        using ImplType = std::decay_t<CmdImpl>;
        auto implPtr = std::make_shared<ImplType>(std::forward<CmdImpl>(impl));

        // Extract the Command before erasing type
        bebop::patterns::Command cmd = implPtr->command();

        // Store lifetime-managed pointer
        storage.emplace_back(std::move(implPtr));

        // Bind command (safe — the context pointer is still valid inside
        // CommandImpl)
        bind(c, cmd);
    }

   private:
    std::unordered_map<char, bebop::patterns::Command> bindings;
    std::vector<std::shared_ptr<void>> storage;  // type-erased ownership
};

int main()
{
    Player player;
    InputHandler handler;

    handler.bindOwned('w',
                      bebop::patterns::make_command(&player, &Player::moveUp));

    handler.bindOwned(
        'v', bebop::patterns::make_command(&player, &Player::shoot, 5));

    auto m_command = bebop::patterns::make_command(&player, &Player::shoot, 10);
    m_command.execute();

    std::string s;
    while (std::cin >> s)
    {
        if (s.size() != 1)
        {
            continue;
        }
        if (auto* cmd = handler.getCommand(s[0]))
            cmd->execute();
    }

    return 0;
}

Basically to create a command, as mentioned you shall use make_command templated method, or it's your responsibility to create a context and a method yourself. It's going to be a little overhead for input handler for instance to store the commands, but there will always be one input handler in the whole game... but many commands. What do you think? Is this design good? Or are there any other ways to do this, without virtual overhead, lambda captures and std::function?

Answer 1

This is over-engineered and way too complicated. If you read GameProgrammingPatterns you eventually find that they recommend command be called with the context that is required with it, rather than trying to attach all local context; this obviates a lot of the design decisions made here on its own, but you really should have just used std::function from the start with the design mentality you had here.

Additionally

• there's only so many keys available, and thus no point in using an unordered map (which below a certain amount of values can be slower than small vector anyway).

• you claim that you're not using std::function because of the performance penalties, but then go ahead and just use shared_ptr everywhere in order to re-invent it.

• Shared pointers should only be used when you have asynchronous lifetimes. As in, when you create object A, you cannot deterministically figure out how long A should be alive for, i.e. there's no scope {} that determines A's lifetime from start to end. This happens only in async/concurrent/multithreaded environments.

• All of these extra things you've added have effectively negated any performance gains you think you've had by not using std::function, and made the code complicated for no reason as a result.

• Your use of std::shared_ptr is effectively what a std::function would have to do but worse in virtually all aspects. It's the reason you have to type erase, you're getting no benefits performance or organizationally from doing this, and you're still getting dynamic allocation.

I will demonstrate how much simpler this can be. I'll be assuming you have access to GLFW (because this is for game programming and meant to be used for real input handling), though the concepts I'm using here are the same for other windowing frameworks. I don't even need to use std::function in this one to get the same capabilities you have, and don't allocate on the heap at all (at least from the code written; GLFW/OpenGL might do so behind the scenes). Not that you should avoid heap allocations just because; the stack itself is an allocation after all, just known ahead of time. Note I've also not ran this; it is just a quick example.

#include <glad/gl.h>
#include <GLFW/glfw3.h>
#include <array> 

//see https://www.glfw.org/docs/3.3/group__keys.html

struct Player
{
    void moveUp() { std::cout << "UP\n"; }
    void moveDown() { std::cout << "DOWN\n"; }
    void moveRight() { std::cout << "RIGHT\n"; }
    void moveLeft() { std::cout << "LEFT\n"; }
    void shoot(int n) { std::cout << "Shoot " << n << "\n"; }
};
//forward declaration so we can declare a function that takes this type of object 
struct Context;
using CommandFunction = void(Context& context); 
struct InputHandler{
    //zero initialize the space for all keys. 
    std::array<CommandFunction, GLFW_KEY_LAST + 1> commands = {}; 
};
struct Context{
    Player player;
    InputHandler input_handler; 
}

static void key_callback(GLFWwindow* window, int key, int scancode, int action, int mods){
    //Hypothetically, actions, and mods may be useful, but we will leave that up as an excercise for the reader. 
    Context* context = glfwGetWindowUserPointer(window); 
    //check if we actually filled the given command in for the given key.
    if(context->input_handler.commands[key] != nullptr && action = GLFW_PRESS){
        context->input_handler.commands[key](*context); 
    }
}
int main(){
    //Taken from https://www.glfw.org/docs/3.3/quick_guide.html
    if (!glfwInit()){
        // Initialization failed
    }
    glfwWindowHint(GLFW_CONTEXT_VERSION_MAJOR, 3);
    glfwWindowHint(GLFW_CONTEXT_VERSION_MINOR, 3);
    //just creating a window that can use input. 
    GLFWwindow* window = glfwCreateWindow(640, 480, "My Title", NULL, NULL);
    if (!window){
        // Window or OpenGL context creation failed
    }
    glfwMakeContextCurrent(window);
    gladLoadGL(glfwGetProcAddress);
    Context context; 
    //our callback can now access context. 
    glfwSetWindowUserPointer(window, &context); 
    
    //lambda functions with out captures are valid free functions
    context.input_handler[GLFW_KEY_UP] = [](Context& context){
        context.player.moveUp(); 
    };
    context.input_handler[GLFW_KEY_DOWN] = [](Context& context){
        context.player.moveDown(); 
    };
    context.input_handler[GLFW_KEY_RIGHT] = [](Context& context){
        context.player.moveRight(); 
    };
    context.input_handler[GLFW_KEY_LEFT] = [](Context& context){
        context.player.moveLeft(); 
    };
    context.input_handler[GLFW_KEY_SPACE] = [](Context& context){
        context.player.shoot(10); 
    };
    context.input_handler[GLFW_KEY_Q] = [](Context& context){
        context.player.shoot(5); 
    };
    
    //poll events will process input events, and our callback will automatically get called, dispatching what was in input_handler
    // (callbacks are processed in this thread). 
    while (!glfwWindowShouldClose(window)){
        int width, height;
        glfwGetFramebufferSize(window, &width, &height);
        //we don't really care what opengl is doing here, all we need is a window to consume key inputs. 
        glViewport(0, 0, width, height);
        glClear(GL_COLOR_BUFFER_BIT);
                
        glfwSwapBuffers(window);
        glfwPollEvents();
    }
    
    glfwDestroyWindow(window);
    glfwTerminate();
}

In other situations, you may actually need to capture, and typically because of that, you'd want to change the commands array into an array of std::function<void(Context&)> to deal with those instances. And you may want to attach multiple inputs per key, or have keys mapped to key combinations instead of specific key presses, which may or may not necessitate using a different container than std::array.

Answer 2

Some remarks to add to Krupip's answer:

• Patterns, as the name suggest, are just patterns, and not meant to be implemented on their own. I would expect you to tailor the pattern to your specific problem. Sometimes that makes it even simpler. For example, if all commands are going to act on a Player object, you could just hardcode that into a PlayerCommand.
• The GoF design patterns solve problems using dynamic polymorphism (inheritance), but in C++ you can also use other ways to solve those problems, for example by using static polymorphism (for example, std::variant) or type erasure (for example, std::function).
• C++'s standard library comes with multiple ways to perform an action at a later time. Krupip already mentioned std::function<void(Context&)>, which is all you need to replace your Command. For asynchronous tasks, there is std::packaged_task.
• While the design patterns are useful tools for many situations, sometimes simpler solutions exist. Follow the KISS and YAGNI principles. Krupip shows how you don't even need to store the context in the command objects, but rather just pass them as an argument, and then a regular function pointer suffices.

Answer 3

Another addition to the existing answers.
I also agree to use std::function.
I see your concern that the type erasure of std::function can have some impact on the performance, but on the other hand you implement a type erasing parameter binding on your own, which should have similar impacts, if not even worse.
The complex template implementations in the STL is usually very good and often better than a manual re-implementation of something similar.

I recommend to do the parameter binding via std::bind, this should do basically the same as your void* context but with a better type safety and should be easier to use.

In general, before you assume that some simple implementation might have a worse performance than a complex handwritten one, set up some tests and compare the performance to be sure.
Performance can easily behave very different than expected due to unforeseen side effects.