2
\$\begingroup\$

I am trying to create an event system that would be quite easy to use. The idea is that a user would only need to create a custom function and tell which event should fire that function. It works as is, but I was wondering if there is a more efficient way to accomplish this of which I'm not aware.

The main classes of the system

Event
EventListener
EventManager
EventType (size_t enum class)

Event

class Event
{
    friend class EventListener;
public:
    virtual EventType GetEventType() const = 0;

    virtual ~Event() { }

    inline bool IsHandled() const
    {
        return m_Handled;
    }
protected:
    bool m_Handled = false;
};

EventListener

using EventBehavior = std::function<bool(const Event& e)>;
    class EventListener
    {       
    public:
        EventListener()
        {
            EventManager::AddListener(this);
        }
        template<class T>
        void Listen(EventBehavior& behavior)
        {
            ASSERT(std::is_base_of<Event, T>, "Can't listen to non Event!");
            m_ListeningTo[(size_t)T::GetStaticType()] = true;
            m_RegisteredEvents[(size_t)T::GetStaticType()] = behavior;
        }
        template<class T>
        void StopListening()
        {
            ASSERT(std::is_base_of<Event, T>, "Can't stop listening to non Event!");
            m_ListeningTo[(size_t)T::GetStaticType()] = false;
        }

        void React(Event& event)
        {
            if (m_ListeningTo[(size_t)event.GetEventType()])
            {
                event.m_Handled = m_RegisteredEvents[(size_t)event.GetEventType()](event);
            }
        }
    private:
        std::bitset<MaxEvents> m_ListeningTo;
        std::array<EventBehavior, MaxEvents> m_RegisteredEvents;
    };

EventManager

    class EventManager
    {
    public:
        static void Post(Event* event)
        {
             m_EventBuffer.push_back(event);
        }
        static void Dispatch()
        {
             for (unsigned int i = 0; i < m_EventBuffer.size(); i++)
             {
                  for (EventListener* listener : m_Listeners)
                  {
                       if (!m_EventBuffer[i]->IsHandled())
                       {
                            listener->React(*m_EventBuffer[i]);
                       }
                       else if(m_EventBuffer[i]->IsHandled())
                       {
                            delete m_EventBuffer[i];
                            m_EventBuffer.erase(m_EventBuffer.begin() + i);
                            break;
                       }
                  }
             }
        }
        static void AddListener(EventListener* listener)
        {
            m_Listeners.push_back(listener);
        }
        static void ClearBuffer()
        {
            for (unsigned int i = 0; i < m_EventBuffer.size(); i++)
            {
                delete m_EventBuffer[i];
            }
            m_EventBuffer.clear();
        }
    private:
        static std::vector<Event*> m_EventBuffer;
        static std::vector<EventListener*> m_Listeners;
    };

Example event

//Base class for all key events
class KeyEvent : public Event
{
public:
    inline int GetKeyCode() const { return m_KeyCode; }
protected:
    KeyEvent(int keycode)
        : m_KeyCode(keycode), m_Mods(0) {}

    KeyEvent(int keycode, int mods)
        : m_KeyCode(keycode), m_Mods(mods) {}

    int m_KeyCode;
    int m_Mods;
};

class KeyPressedEvent : public KeyEvent
{
public:
    KeyPressedEvent(int keycode, int repeatCount)
        : KeyEvent(keycode, 0), m_RepeatCount(repeatCount) {}

    KeyPressedEvent(int keycode, int repeatCount, int scancode, int mods)
        : KeyEvent(keycode, mods), m_RepeatCount(repeatCount) {}

    inline int GetRepeatCount() const { return m_RepeatCount; }

    //EventType is a size_t enum class 
    static EventType GetStaticType() { return EventType::KeyPressed; }
    virtual EventType GetEventType() const override { return GetStaticType(); }
private:
    int m_RepeatCount;
};

The way the user uses the system

bool keyPressed(const Event& event)
{
    const KeyPressedEvent& kpe = static_cast<const KeyPressedEvent&>(event);
    //Do something
    return true;
}

class Sandbox : private EventListener
{
public:
    Sandbox()
    {
        this->Listen<KeyPressedEvent>(EventBehavior(keyPressed));
    }

    ~Sandbox()
    {

    }
};

My main questions

  1. Is there a way to pass the Listen method a function that would accept KeyPressedEvent and thus removing the need for dynamic_cast (I realize this will require a code change and I am glad to do that as long as the way the user would use the system would remain the same)
  2. Is the current system efficient as it is or is it a complete mess?
  3. What would be some similar alternatives (I tried making the EventBehavior a template, but then ran into problems trying to store it inside the array)?
\$\endgroup\$
  • \$\begingroup\$ I'd take a look into some library like POCO. They do have callbacks/observers taking correct type, so you don't have to do dynamic cast inside of callback, but it'd be somewhere in the dispatcher anyways. \$\endgroup\$ – KIIV Dec 17 '18 at 8:51
  • \$\begingroup\$ pretty sure you have a bug where you delete handled events. after you delete the ith item, you shouldn't increment your index or you will skip elements \$\endgroup\$ – sudo rm -rf slash Dec 17 '18 at 22:37
  • \$\begingroup\$ @sudorm-rfslash Oh yeah, I missed that part. Thanks :) \$\endgroup\$ – Gytautas Dec 17 '18 at 22:44
1
\$\begingroup\$

General observations.

Personally (so you can ignore) I prefer to be able to easily distinguish between types and objects. To this end type names always have an initial uppercase letter while objects (and functions) always have an initial lowercase letter. It is a relatively common convention (but not absolute).

Very Good. Nothing major. Think I found one minor bug. Some issues around the ownership of pointers that should be tightened up plus a few questions you should ask yourself.

Code Review

I don't add inline unless it is absolutely needed.

inline bool IsHandled() const

In the class it is not needed.

Avoid protected on variables:

protected:
    bool m_Handled = false;

It does not really provide any protection from accidental abuse (which is what the public/protected/private is about. You are giving your user an entry point to abuse your class.

If you add something in the constructor:

    EventListener()
    {
        EventManager::AddListener(this);
    }

I would normally expect you to remove it in the destructor. Or have a very clear comment on why we don't need to remove it.

I would pass by R-Value reference here:

    template<class T>
    void Listen(EventBehavior& behavior)

I would wrtie like this:

    template<class T>
    void Listen(EventBehavior&& behavior)
    {
        // STUFF

        // By using the && above and the std::move()
        // here we are doing a move assignment (rather than
        // copy assignment). This "can" be more effecient than
        // a copy (depending on the type of "EventBehavior").
        m_RegisteredEvents[(size_t)T::GetStaticType()] = std::move(behavior);
    }

Rather than force a copy you could potentially move it into the destination array.

Why the old school assert?

        ASSERT(std::is_base_of<Event, T>, "Can't listen to non Event!");

This assert is run time only. Also it is only enabled when the appropriate macro flags are set correctly. C++ has static_assert() a much better compiler time assert.

So when you set a "Behavior" you override the existing one. What happens if there was already one set (could we not chain them)? Why do we need to set m_ListeningTo to true (could we not have a default null behavior that just always returns false; this would make a check against true unnecessary!). Just some thoughts.

        m_ListeningTo[(size_t)T::GetStaticType()] = true;
        m_RegisteredEvents[(size_t)T::GetStaticType()] = behavior;

Prefer to use C++ casts:

        (size_t)event.GetEventType()

These C casts are very dangerous; there is no compiler checking.

        static_cast<std::size_t>(event.GetEventType())

Also I see the above cast everywhere. It would probably be best to give this its own function so it is done in exactly one place (that way if you change the behavior you only need to do it once).

Passing by pointer.

    static void Post(Event* event)

Please don't do that. Who is the owner? When I call this function am I supposed to pass a value created with new or the address of an object? I can't tell without reading the code in detail. If you want to force ownership transfer use std::unique_ptr if you want to pass objects use a reference. Pointers should be reserved for internal use where you can easily know the semantics and there is no questions.

Think this is a bug!

         for (unsigned int i = 0; i < m_EventBuffer.size(); i++)
         {
              for (EventListener* listener : m_Listeners)
              {
                  // STUFF
                        m_EventBuffer.erase(m_EventBuffer.begin() + i);
                        break;

                  // OK you just erased an item (and broke out the inner loop).
                  // But the outer loop has now moved all elements down one
                  // position and you are about to increment `i`.
                  //
                  // Does this not mean you are going to skip one of the events?
              }
         }

Seems like an un-needed test after the else?

                   if (!m_EventBuffer[i]->IsHandled()) {
                       // ACTION 1
                   }
                   else if(m_EventBuffer[i]->IsHandled()) {
                       // ACTION 2
                   }

Ether it was handled or it was not handled previously.

Again a pointer.

    static void AddListener(EventListener* listener)

But this time (opposite from last time) you are not passing ownership. If you are not taking ownership pass by reference so we know ownership is not being passed. You can internally take a pointer from the reference. But the caller needs to know that ownership is not being taken.

Why not just use the new range based for?

        for (unsigned int i = 0; i < m_EventBuffer.size(); i++)
        {
            delete m_EventBuffer[i];
        }

Like this:

        for (auto item: m_EventBuffer)
        {
            delete item;
        }

This looks like it should be a dynamic_cast.

bool keyPressed(const Event& event)
{
    const KeyPressedEvent& kpe = static_cast<const KeyPressedEvent&>(event);
    //Do something
    return true;
}

There is no guarantee that the Event is a KeyPressedEvent. You want to do a dynamic_cast to make sure the code checks at runtime that this is the correct type. This will help during testing as it will help you identify bugs. Also will static_cast work if there is multiple inheritance at play?

\$\endgroup\$
  • \$\begingroup\$ I can't find anything on std::assert. Or do you mean <cassert>? \$\endgroup\$ – bruglesco Dec 18 '18 at 3:47
  • \$\begingroup\$ Thanks for reviewing my code. I fixed the bug. And made the listener be passed as reference as well as remove the listener from the manager on destruction. But I have one question. What do you mean by passing an R-Value reference. \$\endgroup\$ – Gytautas Dec 18 '18 at 11:21
  • 1
    \$\begingroup\$ Ops. I meant static_assert \$\endgroup\$ – Martin York Dec 18 '18 at 16:38
  • \$\begingroup\$ You can catch an R-Value with: void Listen(EventBehavior&& behavior) notice the double &&. This allows you to std::move() the object to its destination which allows the move constructor to be used. This prevents a copy construction. \$\endgroup\$ – Martin York Dec 18 '18 at 16:40
  • \$\begingroup\$ Added example of how to use R-Value reference and move semantics. \$\endgroup\$ – Martin York Dec 18 '18 at 16:43

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.