Signals & Slots implementation using Templates

Question

I have written this code to handle events in my game engine. I guess the naming was inspired by Qt Signals & Slots, though I have never used Qt before. It might be similar.

Anyways, I think it would help me if someone more experienced than me looked at this piece of code and gave me some feedback. Honestly I feel a little proud about this, because it manages something I have had a lot of trouble with very effectively (as far as my tests have gone), but I would like to know if anyone can spot opportunities for optimization or tell me about inefficient patterns in this implementation.

There is an explanation to give a good place to start:

//Events.h    
#pragma once
#include <vector>
#include <unordered_map>
#include <initializer_list>
/*
---
This is a structure which allows you to call functions of any type upon certain events. 
You define an event to wait for by creating an EventSlot<EventType>. It stores the signature of the event and receives a unique signal_index. 
Use EventSlot<EventType>::instance_count() and ::get_instance(index) to compare the stored events of each slot with your incoming events
If an Event matches an EventSlot, the Slot will set its assigned signal to 'signaled'. 
These signals can be watched by FuncSlots. FuncSlots store the function pointers and arguments of any function you want to call.
They are assigned a set of signalIDs and call their function once they find one of their signals 'signaled'.
---Usage------------------------------------------------------------------------------------------------------------------------------------------------------------------------
- create EventSlots/Signals -
        - (it is recommended to call EventSlot<EventType>::reserve_slots(slotCount); before defining slots)
        - EventSlot<KeyEvent> w_press_slot(KeyEvent(GLFW_KEY_W, 1, 0)); //creates a signal which becomes 'signaled' when KeyEvent(GLFW_KEY_W, 1, 0) appears in the eventBuffer
- create FuncSlots to call functions -
        - FuncSlot<void> moveForwardSlot(moveForward);
        - or: FuncSlot<void, vec3> moveForwardSlot(move, 0.0f, 0.0f, -1.0f);
        - or: FuncSlot<void> moveRightSlot( [](){ move(1.0f, 0.0f, 0.0f); });
- assign Signals to FuncSlots -
        - moveForwardSlot.listen({ w_press_slot });
        - moveRightSlot.listen({ d_press_slot });
        - stopXSlot.listen({ d_release_slot, a_release_slot });
---
- Optionally you can set signals to lock other signals from being signaled. This way you can synchronize many, possibly conflicting Events.
        - signal_lock(w_press_slot.signal, { s_press_slot.signal, s_release_slot.signal });
        //when 'W' is pressed, the signals of s_press and s_release are locked. they will not change anymore and any events matching these slots will be ignored.
        //You will need a signal to explicitly unlock locked signals to use them again.
        - signal_unlock(w_release_slot.signal, { s_press_slot.signal, s_release_slot.signal });
        //You have to make sure that all locks will be released at some point (unless you have specific intentions ofc), this is why you should generally use
        set_up_lock to set up both locking and unlocking signals at once.
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
*/
namespace App {
    namespace Input {

        struct Signal {
            Signal(int pOn, int pStay)
                :on(pOn), stay(pStay) {}
            Signal()
                :on(0), stay(0) {}

            void set(int set = 1) {
                on = set;
            }
            void set_stay(int set = 1) {
                stay = set;
            }
            void reset() {
                on = stay;
            }
            int on;
            int stay;//the value to assign to 'on' when resetting it
        };
        extern std::vector<Signal> allSignalSlots;
        extern std::vector<int> allSignalLocks; // 1 = locked 


        //this might be kinda slow. it should store an array of indices for each "LockSignal" which is a signal which either locks or unlocks the array of other signals
        extern std::unordered_map<unsigned int, std::vector<unsigned int>> signalLockBindings;
        extern std::unordered_map<unsigned int, std::vector<unsigned int>> signalUnlockBindings;



        static void signal_lock(unsigned int pLockSignal, std::initializer_list<unsigned int> pTargetSignals) {
            auto it = signalLockBindings.find(pLockSignal);
            if (it == signalLockBindings.end()) {
                signalLockBindings.insert(std::pair<unsigned int, std::vector<unsigned int>>(pLockSignal, std::vector<unsigned int>(pTargetSignals)));
                return;
            }
            it->second.insert(it->second.end(), pTargetSignals.begin(), pTargetSignals.end());
        }
        // set pLockSignal to set the locks of pTargetSignals to pLock
        static void signal_unlock(unsigned int pUnlockSignal, std::initializer_list<unsigned int> pTargetSignals) {
            auto it = signalUnlockBindings.find(pUnlockSignal);
            if (it == signalUnlockBindings.end()) {
                signalUnlockBindings.insert(std::pair<unsigned int, std::vector<unsigned int>>(pUnlockSignal, std::vector<unsigned int>(pTargetSignals)));
                return;
            }
            it->second.insert(it->second.end(), pTargetSignals.begin(), pTargetSignals.end());
        }
        static void set_up_lock(unsigned int pLockSignal, unsigned int pUnlockSignal, std::initializer_list<unsigned int> pTargetSignals) {
            signal_lock(pLockSignal, pTargetSignals);
            signal_unlock(pUnlockSignal, pTargetSignals);
        }


        template<typename R, typename... Args>
        class FuncSlot {
        private:
            R(*fun)(Args...);
            std::tuple<Args...> args;
            unsigned int slot_index;

        public:
            std::vector<unsigned int> signal_bindings;
            static std::vector<FuncSlot<R, Args...>> instances;

            FuncSlot() : fun(nullptr), args(std::tuple<Args...>()), signal_bindings(std::vector<unsigned>())
            {}

            FuncSlot(R(*pF)(Args...), Args... pArgs)
                :fun(pF), args(std::forward_as_tuple(pArgs)...), signal_bindings(), slot_index(instances.size()) {

                instances.push_back(*this);
            }
            static void clear() {
                instances.clear();
            }
            static void reserve_slots(unsigned int pCount) {
                instances.reserve(pCount);
            }
            void listen(std::initializer_list<unsigned int> pSignals)
            {
                instances[slot_index].signal_bindings = std::vector<unsigned>(pSignals);
            }
            void listen(FuncSlot<R, Args...>& pSlot, std::initializer_list<unsigned int> pSignals)
            {
                instances[pSlot.slot_index].signal_bindings = std::vector<unsigned>(pSignals);
            }
            R invoke() const {
                return std::apply(fun, args);
            }
            R callFunc(Args... pArgs) const {
                return fun(pArgs...);
            }

        };
        template<typename R, typename... Args>
        std::vector<FuncSlot<R, Args...>> FuncSlot<R, Args...>::instances = std::vector<FuncSlot<R, Args...>>();

        template<class EventType>
        class EventSlot {
        public:
            EventSlot(EventType pEvent) :signal_index(allSignalSlots.size()), evnt(pEvent) {
                                allSignalSlots.push_back(Signal());
                                allSignalLocks.push_back(0);
                instances.push_back(*this);
            }
                        static void reserve_slots(unsigned int pCount) {
                                allSignalSlots.reserve(allSignalSlots.size() + pCount);
                                allSignalLocks.reserve(allSignalLocks.size() + pCount);
                instances.reserve(pCount);
            }
            static unsigned int instance_count() {
                return instances.size();
            }
                        static EventSlot<EventType> get_instance(unsigned int index) {
                                return instances[index];
                        }
            static void clear() {
                instances.clear();
            }

            EventType evnt;
            unsigned signal_index;//this event´s signal slot
        private:

            static std::vector<EventSlot<EventType>> instances;

        };
        template<class EventType>
        std::vector<EventSlot<EventType>> EventSlot<EventType>::instances = std::vector<EventSlot<EventType>>();

    }
}

(all the .cpp file contains are the definitions for the externs)

---

Here is an example usage:

EDIT: I have now added some actual snippets that I use in my implementation.

First, all the events to wait for and the functions to call have to be defined:

void setupSignalsAndSlots()
{
    EventSlot<KeyEvent>::reserve_slots(2);//reserve EventSlots for EventType KeyEvent
    EventSlot<KeyEvent> esc_press_slot(KeyEvent(GLFW_KEY_ESCAPE, KeyCondition(1, 0)));
    EventSlot<KeyEvent> c_press_slot(KeyEvent(GLFW_KEY_C, KeyCondition(1, 0)));

    EventSlot<MouseKeyEvent>::reserve_slots(2);
    EventSlot<MouseKeyEvent> rmb_press_slot(MouseKeyEvent(GLFW_MOUSE_BUTTON_2, KeyCondition(1, 0)));
    EventSlot<MouseKeyEvent> rmb_release_slot(MouseKeyEvent(GLFW_MOUSE_BUTTON_2, KeyCondition(0, 0)));

    FuncSlot<void>::reserve_slots(2);
    FuncSlot<void> quitFunc(quit);//define functions
    FuncSlot<void> toggleMouseTrack(toggleTrackMouse);

    quitFunc.listen({ esc_press_slot.signal_index });
    toggleMouseTrack.listen({ c_press_slot.signal_index, rmb_press_slot.signal_index, rmb_release_slot.signal_index });

    //you could do:
    //set_up_lock(rmb_press_slot.signal_index, rmb_release_slot.signal_index, {c_press_slot.signal_index});
}

---

During the frame loop a function like this determines the signals to set by comparing the buffer of occurred events to the EventSlots of the respective type. The Signals which should be signaled are buffered in a buffer of signal indices.

void App::Input::checkKeyEvents()
{
    size_t keyEventCount = keyEventBuffer.size();
    unsigned int signalOffset = signalBuffer.size();
    signalBuffer.resize(signalOffset + keyEventCount);
    unsigned int signalCount = 0;


    for (unsigned int e = 0; e < keyEventCount; ++e) {
        KeyEvent& kev = keyEventBuffer[e];
        for (unsigned int ks = 0; ks < EventSlot<KeyEvent>::instance_count(); ++ks) {
            EventSlot<KeyEvent>& slot = EventSlot<KeyEvent>::get_instance(ks);
            if (slot.evnt == kev) {
                signalBuffer[signalOffset + signalCount++] = slot.signal_index;
                break;
            }
        }
    }
    keyEventBuffer.clear();
    signalBuffer.resize(signalOffset + signalCount);
}

---

Whether the to-be-set signals will actually be set, depends on whether they are unlocked. this function sets the signals which are not locked and stores the rejected ones in a buffer in order to check them again the next frame.

void checkSignals()
{
    //set signals if they are not locked
    rejectedSignals.insert(rejectedSignals.end(), signalBuffer.begin(), signalBuffer.end());
    signalBuffer.clear();
    std::vector<unsigned int> rejected;
    rejected.reserve(rejectedSignals.size());
    unsigned int passed = 0;
    for (unsigned int& sig : rejectedSignals) {
        if (!allSignalBlocks[sig]) {
            allSignalSlots[sig].on = 1;
        }
        else {
            rejected.push_back(sig);
        }
    }
    rejectedSignals = rejected;
}

---

Afterwards, this function makes each FuncSlot check its signal_bindings and call its function if a signal is set. Also, it locks/unlocks signals based on the signals which are set. Afterwards, all signals are reset to their stay value.

void callFunctions()
{
    for (FuncSlot<void>& inst : FuncSlot<void>::instances) {
        for (unsigned s : inst.signal_bindings) {
            if (allSignalSlots[s].on) {
                inst.invoke();
                break;
            }
        }
    }
    //any other function template here

    //reset signals and lock signals
    for(auto& to_lock : signalBlockBindings){
        if (allSignalSlots[to_lock.first].on) {
            for (unsigned int l = 0; l < to_lock.second.size(); ++l) {
                allSignalBlocks[to_lock.second[l]] = 1;
            }
        }
    }
    for (auto& to_unlock : signalUnblockBindings) {
        if (allSignalSlots[to_unlock.first].on) {
            for (unsigned int l = 0; l < to_unlock.second.size(); ++l) {
                allSignalBlocks[to_unlock.second[l]] = 0;
            }
        }
    }

    //reset signals
    //some signals will be set off, others (rules) will stay on untill they are explicitly turned off
    for (unsigned int s = 0; s < allSignalSlots.size(); ++s) {
        allSignalSlots[s].reset();
    }
}

Answer 1

Implementation details

• The implementation repeats a lot of internal types - sometimes even differently (unsigned instead of unsigned int for signal indices). Why not introduce type aliases that are defined once and then used everywhere?

  using SignalIndex = unsigned int;
  /*...*/
  class FuncSlot {
      using FunctionType = R(*)(Args...);
      /* ... */
  };
  

• In both signal_lock and signal_unlock the implementation first tries to check whether there is an existing entry in the unordered_map - just to insert one anyways if it is missing. This could be simplified to:

  void signal_lock(SignalIndex pLockSignal, std::initializer_list<SignalIndex> pTargetSignals) {
      // creates an empty vector if not already there
      auto& locks = signalLockBindings[pLockSignal];
      locks.insert(std::end(locks), std::begin(pTargetSignals), std::end(pTargetSignals));
  }
  

• In both EventSlot and FuncSlot there is a constructor containing instances.push_back(*this). This is bad for 2 reasons:

  • If any of those constructors were ever to be called from a derived class, this copy operation would slice the object. This sets you on the fast track to undefined behavior, even disregarding potential problems of constructors throwing exceptions.
  • It creates an independent copy - which is then only used internally. The originally constructed object is completely unrelated, so any changes there will not be reflected in the internal copy.

• The signal locking mechanism is borderline broken. In the code examples, signals are set with no regards to whether that signal might be locked. Also, any checks whether a signal should lock/unlock other signals are graciously overlooked. The locking mechanic should be consistent in all cases! So it should either be enforced in all cases, or it should be replaced by an explicit opt-in abstraction.

• The comment documentation mentions that one can create FuncSlot object with a lambda. This isn't always the case - only if the lambda doesn't capture anything! And if we are already on topic, why not use std::function<R(Args...)> instead of that cumbersome function pointer type? As a bonus, it would also allow to accept lambdas with captures and functors (objects overloading operator()).

Naming

I get it, naming is hard.

• Especially after you mentioned the Qt signals and slots in the beginning of your post, I personally got really confused with EventSlot - as that represents an event trigger, which in Qt terminology is a signal.
• While it gets the intention somewhat across, locking might be the wrong name for that mechanic. First, that name has already lots of uses in normal programming jargon (with regards to multithreading), so it is a bit bloated and might confuse someone expecting the usual meaning. Second, even in normal English usage, signals don't get locked, they get blocked - which might be a far better description of that system.

Design stuff

• The encapsulation (if there is any to speak of) is leaking hard. The reliance on public data members is everywhere, and even in the code example, Signal::set isn't even used in favor of setting Signal::on directly. This will very likely cause problems if the underlying implementation ever needs to be changed.
• The implementation is not thread safe. I know, it might not be intended to be, but more often than not, event-based systems are used to communicate between different systems - which might run on different threads. (Just something to consider for future extensions)
• The abstractions are not well defined. EventSlot has multiple responsibilities (providing a trigger signal AND providing an event object), and any usage of this library will need to go back and forth across abstraction levels (needs to work with lower level Signal in order to achieve anything with FuncSlot or EventSlot.