2
\$\begingroup\$

I've tried to write an event dispatcher for a GLFW/Glad library. However I am not very used to writing efficient code (spent the majority of my life writing in C# and performance was never really the focus of what I learnt in university).

The code works but I'm unsure if its overcomplicated or simply plain stupid. I would appreciate any and all feedback.

A problem that I am aware of is that you have to dynamic_cast the event to actually get any useful information out of it, which is undesirable. The dynamic_cast wouldn't be an issue if all events were rarely called but this dispatcher would also handle key press events and I reckon that casting several times per frame would be a disaster. Also, you cannot currently unregister/detach a callback due to the way callbacks are registered.

I haven't really thought about inlining either; it's a bit confusing to me since you never ultimately get a say in what gets inlined.


MythDispatcher.h

#ifndef MYTH_FRAMEWORK_DISPATCHER
#define MYTH_FRAMEWORK_DISPATCHER

#include "mpch.h"
#include "MythEvent.h"

namespace MythFramework
{
    using MythCallback = std::function<void(MythEvent&)>;

    class MythDispatcher
    {
    public:
        static void RegisterCallback(EventType eventType, MythCallback callback);
    private:
        MythDispatcher();

        static MythDispatcher& GetInstance();

        MythDispatcher(const MythDispatcher&) = delete;
        MythDispatcher& operator=(const MythDispatcher&) = delete;

        static void Trigger(EventType eventType, MythEvent& eventToDispatch);

        std::map<EventType, std::vector<MythCallback> > callbacks;

        friend class MythWindow;
        friend class MythWinWindow;
    };
}

#endif

MythDispatcher.cpp

#include "MythDispatcher.h"

namespace MythFramework
{
    MythDispatcher::MythDispatcher()
    {
        callbacks = std::map<EventType, std::vector<MythCallback> >();
    }

    void MythDispatcher::RegisterCallback(EventType eventType, MythCallback callback)
    {
        if (GetInstance().callbacks.find(eventType) == GetInstance().callbacks.end())
        {
            GetInstance().callbacks[eventType] = std::vector<MythCallback>();
        }

        GetInstance().callbacks[eventType].push_back(callback);
    }

    MythDispatcher& MythDispatcher::GetInstance()
    {
        static std::unique_ptr<MythDispatcher> instance;
        if (!instance)
        {
            instance.reset(new MythDispatcher());
        }

        return *instance;
    }

    void MythDispatcher::Trigger(EventType eventType, MythEvent& eventToDispatch)
    {
        if (GetInstance().callbacks.find(eventType) == GetInstance().callbacks.end())
            return;

        //Could use auto but i hate auto
        for (std::vector<MythCallback>::iterator it = GetInstance().callbacks[eventType].begin(); it != GetInstance().callbacks[eventType].end(); it++)
        {
            (*it)(eventToDispatch);
        }
    }
}

Extract from window class to show how events are invoked:

        /*
      ######################
          GLFW CALLBACKS    
      ######################
      */

      glfwSetWindowFocusCallback(window, [](GLFWwindow* window, int focus)
      {
          if (focus == GL_TRUE)
          {
              MythDispatcher::Trigger(EventType::AppFocus, MythFocusEvent());
          }
          else
          {
              MythDispatcher::Trigger(EventType::AppLostFocus, MythLostFocusEvent());
          }
      });

      glfwSetWindowPosCallback(window, [](GLFWwindow* window, int x, int y)
      {
          MythDispatcher::Trigger(EventType::AppMove, MythMoveEvent(x, y));
      });

      glfwSetWindowSizeCallback(window, [](GLFWwindow* window, int width, int height)
      {
          MythDispatcher::Trigger(EventType::AppResize, MythResizeEvent(width, height));
      });

Extract from MythApp.h showing how callbacks are registered:

      MythDispatcher::RegisterCallback(EventType::AppExit, [this](MythEvent& e) {
          running = false;
      });

Edit: Github repo

\$\endgroup\$
2
  • \$\begingroup\$ Are there some definitions in mpch.h and MythEvent.h that will enable us to compile the code? It would help us give better reviews if you share enough of those to reproduce your results. \$\endgroup\$ Jan 20, 2022 at 7:25
  • \$\begingroup\$ @TobySpeight I pushed everything to a public repo. You should be able to build it with CMake github.com/ScrappyHaxor/Myth-Framework \$\endgroup\$ Jan 20, 2022 at 7:41

2 Answers 2

3
\$\begingroup\$

We're missing some includes in the header file:

#include <functional>
#include <map>
#include <vector>

And in the implementation:

#include <memory>

I'm not a fan of the singleton, which can be hard to unit-test independently.

However, if that's a done deal, I don't think there's any call for instance methods and data - it can simply be a static class.


Members should be initialised in the constructor's initialiser list, rather than in the body:

MythDispatcher::MythDispatcher()
    : callbacks{}
{}

Better still, we can initialise in-line in the class definition, allowing us to default the constructor:

private:
    std::map<EventType, std::vector<MythCallback> > callbacks = {};
    MythDispatcher() = default;

It's probably better to make Trigger() public, so we don't have to list all the possible classes that might use it as friends. That's a potentially open-ended set, which makes it hard for us to foresee every use case.


In RegisterCallback(), we default-construct a new map entry if one doesn't already exist. But that's what std::map::operator[] already does, so we can just write it thus:

void MythDispatcher::RegisterCallback(EventType eventType, MythCallback callback)
{
    GetInstance().callbacks[eventType].push_back(std::move(callback));
}

In GetInstance(), we start with instance == nullptr. But that's not necessary, as we can initialise with the actual object - it will be populated the first time the function is called. We don't even need the smart-pointer, as it won't be deleted until program end:


In Trigger(), we really ought to overcome your aversion to auto. This spelt-out type (and the repeated lookup) makes for long lines full of clutter:

    //Could use auto but i hate auto
    for (std::vector<MythCallback>::iterator it = GetInstance().callbacks[eventType].begin(); it != GetInstance().callbacks[eventType].end(); it++)
    {
        (*it)(eventToDispatch);
    }

Look, much more legible:

    for (auto& callback: GetInstance().callbacks[eventType]) {
        callback(eventToDispatch);
    }

I'd avoid the multiple GetInstance() and callbacks[eventType] lookups, too:

void MythDispatcher::Trigger(EventType eventType, MythEvent& eventToDispatch)
{
    auto const& callbacks = GetInstance().callbacks;
    auto const it = callbacks.find(eventType);
    if (it == callbacks.end()) {
        return;
    }

    for (auto& callback: it->second) {
        callback(eventToDispatch);
    }
}

Modified code

I've changed the interface so it's no longer a singleton - your application can thus use several dispatchers if it needs to (and the unit tests certainly will need to).

Header

#include <functional>
#include <map>
#include <vector>

namespace MythFramework
{
    using MythCallback = std::function<void(MythEvent&)>;

    class MythDispatcher
    {
        std::map<EventType, std::vector<MythCallback>> callbacks = {};

    public:
        MythDispatcher(const MythDispatcher&) = delete;
        MythDispatcher& operator=(const MythDispatcher&) = delete;

        void RegisterCallback(EventType eventType, MythCallback callback);
        void Trigger(EventType eventType, MythEvent& eventToDispatch);
    };
}

Implementation

#include <utility>

namespace MythFramework
{
    void MythDispatcher::RegisterCallback(EventType eventType, MythCallback callback)
    {
        callbacks[eventType].push_back(std::move(callback));
    }

    void MythDispatcher::Trigger(EventType eventType, MythEvent& eventToDispatch)
    {
        auto const it = callbacks.find(eventType);
        if (it == callbacks.end()) {
            return;
        }

        for (auto& callback: it->second) {
            callback(eventToDispatch);
        }
    }
}

Further inspiration

The Qt library has an event dispatcher in its core, that has been battle-tested over the course of decades. It's worth a good look at how it operates, so you can see what concerns you ought to address in your own implementation.

\$\endgroup\$
2
\$\begingroup\$

In addition to Toby Speight's excellent answer:

Avoid repeating names unnecessarily

Every type you add starts with Myth. However, once you put everything in a namespace, there is no reason to repeat parts of the name of the namespace. I would also recommend renaming MythFramework to just Myth. It then becomes:

namespace Myth
{
    using Callback = std::function<void(Event&)>;

    class Dispatcher {
        ...
    };
};

Also, what is a MythWinWindow?

Love is better than hate

You wrote this code:

//Could use auto but i hate auto
for (std::vector<MythCallback>::iterator it = GetInstance().callbacks[eventType].begin(); it != GetInstance().callbacks[eventType].end(); it++)
{
    (*it)(eventToDispatch);
}

You don't need auto to turn this into a range-based for loop:

for (MythCallback &callback: callbacks[eventType])
{
    callback(eventToDispatch);
}

But please don't hate things. You might not like some aspects of auto, but that does not mean auto also has benefits that might outweigh the drawbacks in some scenarios. By "hating" you make it only harder for you to see those benefits, and your code quality might suffer if you don't use auto in the appropriate places. Maybe it even blinded you from the fact that a range-for could still be used without auto?

Use std::unordered_map

You don't need callbacks to be sorted in any particular order, so consider using a std::unordered_map instead, as it has faster lookups than a std::map.

Note that you could even avoid using an (unordered) map altogether, by making MythDispatcher a template, with the event type being the template parameter. Basically, you will then have one dispatcher per event type.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ I forgot to mention the unordered map! You improved on it by suggesting template instead. The other things I completely missed. \$\endgroup\$ Jan 20, 2022 at 12:02

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.