4
\$\begingroup\$

I am new to C++, so please take me easy. I want to make a low-level game engine only using C++, OpenGL and GLFW. This is a continuation of Event System using C++ ; I added the suggestions from there. Here's a diagram of the improved event system:

enter image description here

I also added an InputManager:

enter image description here

I know I could implement a FSM for the InputManager, but for the moment, i do not need it -instead of the bool textInputMode. I would appreciate very much some feedback.

Event.h:

#pragma once

#include <string>

enum class EventCategory
{
    None,
    Input,
    Window,
    Mouse,
    Keyboard,
    Application,
};

enum class EventType
{
    None,
    WindowResize, WindowLostFocus, WindowGainedFocus, 
    WindowClose, WindowCursorEnter, WindowCursorLeave,
    MouseMove, MousePress, MouseRelease, MouseScroll,
    KeyboardPress, KeyboardRepeat, KeyboardRelease, CharPress,
};

struct EventDescription
{
    const EventCategory category;
    const EventType type;
    const std::string name;
};

class Event
{
public:
    virtual ~Event() = default;

    bool handle() { handled = true; }
    bool isHandled() const { return handled; }

    EventCategory getCategory() const { return getDescription().category; }
    EventType getType()         const { return getDescription().type;     }
    std::string getTypeName()   const { return getDescription().name;     }

    virtual EventDescription getDescription() const = 0;

private:
    bool handled = false;
};                                  

class NoneEvent : public Event
{
public:
    EventDescription getDescription() const override { return { EventCategory::None ,EventType::None, "None" }; }
};

KeyboardEvent.h:

#pragma once

#include "Event.h"

#include <Stdint.h>

class KeyboardEvent : public Event
{
protected:
    KeyboardEvent(const int keyCode, const int scancode, const int mods)
        : keyCode(keyCode), scancode(scancode), mods(mods) { }

public:
    virtual ~KeyboardEvent() = default;
    EventDescription getDescription() const override = 0;

    int getKeyCode() const { return keyCode; }
    int getScanCode() const { return scancode; }
    int getMods() const { return mods; }

private:
    const int keyCode;
    const int scancode;
    const int mods;
};

class KeyboardPressEvent : public KeyboardEvent
{
public:
    KeyboardPressEvent(const int keyCode, const int scancode, const int mods)
        : KeyboardEvent(keyCode, scancode, mods) { }

    EventDescription getDescription() const override 
        { return { EventCategory::Keyboard, EventType::KeyboardPress, "KeyboardPress"}; }
};

class KeyboardRepeatEvent : public KeyboardEvent
{
    KeyboardRepeatEvent(const int keyCode, const int scancode, const int mods)
        : KeyboardEvent(keyCode, scancode, mods) { }

    EventDescription getDescription() const override 
        { return { EventCategory::Keyboard, EventType::KeyboardRepeat, "KeyboardRepeat" }; }
};

class KeyboardReleaseEvent : public KeyboardEvent
{
public:
    KeyboardReleaseEvent(const int keyCode, const int scancode, const int mods)
        : KeyboardEvent(keyCode, scancode, mods) { }

    EventDescription getDescription() const override 
        { return { EventCategory::Keyboard, EventType::KeyboardRelease, "KeyboardRelease" }; }

};

class CharPressEvent : public Event
{
public:
    CharPressEvent(const int codePoint)
        : codePoint(codePoint) { }

    int getCodePoint() const { return codePoint; }

    EventDescription getDescription() const override 
        { return { EventCategory::Keyboard, EventType::CharPress, "CharPress" }; }

private:
    const int codePoint;
};

MouseEvent.h:

#pragma once

#include "Event.h"

class MouseEvent : public Event 
{
public:
    virtual ~MouseEvent() = default;
    EventDescription getDescription() const override = 0;
};

// ===================== MOTION UPCOMING ==============================

class MouseMotionEvent : public MouseEvent
{
public:
    double getMouseX() const { return mouseX; }
    double getMouseY() const { return mouseY; }

    virtual ~MouseMotionEvent() = default;
    EventDescription getDescription() const override = 0;

protected:
    MouseMotionEvent(const double mouseX, const double mouseY)
        : mouseX(mouseX), mouseY(mouseY) { }

private:
    const double mouseX, mouseY;
};

class MouseMoveEvent : public MouseMotionEvent
{
public:
    MouseMoveEvent(const double mouseX, const double mouseY)
        : MouseMotionEvent(mouseX, mouseY) { }

    virtual ~MouseMoveEvent() = default;
    EventDescription getDescription() const override 
        { return { EventCategory::Mouse, EventType::MouseMove, "MouseMove" }; }
};

class MouseScrollEvent : public MouseMotionEvent
{
public:
    MouseScrollEvent(const double mouseX, const double mouseY)
        : MouseMotionEvent(mouseX, mouseY) { }

    virtual ~MouseScrollEvent() = default;
    EventDescription getDescription() const override 
        { return { EventCategory::Mouse, EventType::MouseScroll, "MouseScroll" }; }
};

// ===================== BUTTON UPCOMING ==============================

class MouseButtonEvent : public MouseEvent
{
protected:
    MouseButtonEvent(const int button, const int mods)
        : button(button), mods(mods) { }

public:
    int getButton() const { return button; }
    int getMods() const { return mods; }

    virtual ~MouseButtonEvent() = default;
    EventDescription getDescription() const override = 0;

private:
    const int button, mods;
};

class MousePressEvent : public MouseButtonEvent
{
public: 
    MousePressEvent(const int button, const int mods)
        : MouseButtonEvent(button, mods) { }

    virtual ~MousePressEvent() = default;
    EventDescription getDescription() const override 
        { return { EventCategory::Mouse, EventType::MousePress, "MousePress" }; }
};

class MouseReleaseEvent : public MouseButtonEvent
{
public: 
    MouseReleaseEvent(const int button, const int mods)
        : MouseButtonEvent(button, mods) { }

    virtual ~MouseReleaseEvent() = default;
    EventDescription getDescription() const override 
        { return { EventCategory::Mouse, EventType::MouseRelease, "MouseRelease" }; }
};

WindowEvent.h:

#pragma once

#include "Event.h"

class WindowEvent : public Event 
{
public: 
    virtual ~WindowEvent() = default;
    EventDescription getDescription() const override = 0;
};

class WindowResizeEvent : public WindowEvent 
{
public: 
    WindowResizeEvent(const int width, const int height) 
        : width(width), height(height) { }

    virtual ~WindowResizeEvent() = default;
    EventDescription getDescription() const override 
        { return { EventCategory::Window, EventType::WindowResize, "WindowResize" }; }

    int getWidth() const { return width; }
    int getHeight() const { return height; }

private:
    const int width, height;
};

class WindowCloseEvent : public WindowEvent
{
public:
    virtual ~WindowCloseEvent() = default;
    EventDescription getDescription() const override 
        { return { EventCategory::Window, EventType::WindowClose, "WindowClose" }; }
};

class WindowLostFocusEvent : public WindowEvent 
{
public: 
    virtual ~WindowLostFocusEvent() = default;
    EventDescription getDescription() const override 
        { return { EventCategory::Window, EventType::WindowLostFocus, "WindowLostFocus" }; }
};

class WindowGainedFocusEvent : public WindowEvent 
{
public: 
    virtual ~WindowGainedFocusEvent() = default;
    EventDescription getDescription() const override 
        { return { EventCategory::Window, EventType::WindowGainedFocus, "WindowGainedFocus" }; }
};

class WindowCursorEnterEvent : public WindowEvent
{
public:
    virtual ~WindowCursorEnterEvent() = default;
    EventDescription getDescription() const override 
        { return { EventCategory::Window, EventType::WindowCursorEnter, "WindowCursorEnter" }; }
};

class WindowCursorLeaveEvent : public WindowEvent
{
public:
    virtual ~WindowCursorLeaveEvent() = default;
    EventDescription getDescription() const override 
        { return { EventCategory::Window, EventType::WindowCursorLeave, "WindowCursorLeave" }; }
};

EventQueue.h:

#pragma once

#include <queue>
#include <memory>

#include "MouseEvent.h"
#include "KeyboardEvent.h"
#include "WindowEvent.h"

class EventQueue
{
public:
    using EventPtr = std::unique_ptr<Event>;

    void push(EventPtr event) { eventQueue.push(std::move(event)); }
    
    EventPtr pop()
    {
        if (eventQueue.empty())
            return nullptr;
        EventPtr event = std::move(eventQueue.front());
        eventQueue.pop();
        return event;
    }

    bool isEmpty() const { return eventQueue.empty(); }

private:
    std::queue<EventPtr> eventQueue;
};

EventListenerID.h:

#pragma once

class EventListenerID
{
public:
    explicit EventListenerID(const size_t id) : id(id) { }
    
    size_t get() const { return id; }

private:
    const size_t id;
};

EventListener.h:

#pragma once

#include <functional>

#include "MouseEvent.h"
#include "KeyboardEvent.h"
#include "WindowEvent.h"
#include "EventListenerID.h"

class BaseEventListener
{
public:
    virtual ~BaseEventListener() = default;
    virtual void dispatchEvent(const Event&) const = 0;
    virtual bool isEventType(const Event& event) const = 0;
    virtual EventListenerID getID() const = 0;
};

template<typename EventType>
class EventListener : public BaseEventListener
{
public:
    using EventCallBackFn = std::function<void(const EventType&)>;

    explicit EventListener(const EventCallBackFn& callBack, const EventListenerID& id) : callBack(callBack), id(id) { }
    void dispatchEvent(const Event& event) const override { callBack(static_cast<const EventType&>(event));}

    bool isEventType(const Event& event) const override { return dynamic_cast<const EventType*>(&event) != nullptr; }

    EventListenerID getID() const override { return id; }

private:
    const EventCallBackFn callBack;
    const EventListenerID id;
};

EventListenerRegister.h:

#pragma once

#include <vector>
#include <memory>
#include <algorithm>
#include <stdexcept>

#include "EventListener.h"

class EventListenerRegister
{
public:
    using ListenerList = std::vector<std::unique_ptr<BaseEventListener>>;

    template<typename EventType>
    EventListenerID registerListenerFor(const typename EventListener<EventType>::EventCallBackFn& callBack)
    {
        static int currentID = 0;
        EventListenerID listenerID(currentID);
        ++currentID;

        std::unique_ptr<BaseEventListener> listener = std::make_unique<EventListener<EventType>>(callBack, listenerID);
        listeners.push_back(std::move(listener));

        return listenerID;
    }

    void unregisterListener(EventListenerID& listenerID)
    {
        listeners.erase(
            std::remove_if(listeners.begin(), listeners.end(), [&listenerID](const auto& listener) {
                return listenerID.get() == listener->getID().get();
                }),
            listeners.end()
        );
    }

    auto begin()        { return listeners.begin(); }
    auto begin() const  { return listeners.begin(); }
    auto end()          { return listeners.end();   }
    auto end()   const  { return listeners.end();   }

private:
    ListenerList listeners;
};

EventBus.h:

#pragma once

#include <memory>

#include "EventListenerRegister.h"
#include "EventQueue.h"

class EventBus
{
public:
    template<typename EventType>
    EventListenerID registerListenerFor(const typename EventListener<EventType>::EventCallBackFn& callBack)
    {
        return listeners.registerListenerFor<EventType>(callBack);
    }

    void unregisterListener(EventListenerID& listenerID)
    {
        listeners.unregisterListener(listenerID);
    }

    void publishEvent(EventQueue& eventQueue) const
    {
        while(const auto& event = eventQueue.pop()) {
            for (const auto& listener : listeners) {
                if (listener->isEventType(*event.get()))
                    listener->dispatchEvent(*event.get());

                if (event->isHandled())
                    break;
            }
        }
    }

private:
    EventListenerRegister listeners;
};

EventManager.h:

#pragma once

#include "EventBus.h"
#include "EventQueue.h"

class EventManager
{
public:
    template<typename EventType>
    requires std::derived_from<EventType, Event>
    EventListenerID registerListenerFor(const typename EventListener<EventType>::EventCallBackFn& callBack) 
    { 
        return eventBus.registerListenerFor<EventType>(callBack);
    }

    void unregisterListener(EventListenerID& listenerID)
    { 
        eventBus.unregisterListener(listenerID);
    }

    template<typename EventType, typename... Args>
    requires std::derived_from<EventType, Event>
    void pushEvent(Args&&... args) 
    { 
        eventQueue.push(std::make_unique<EventType>(std::forward<Args>(args)...)); 
    }

    void publishEvents()
    {
        eventBus.publishEvent(eventQueue);
    }

private:
    EventBus eventBus;
    EventQueue eventQueue;
};

MouseState.h:

#pragma once

#include <unordered_map>

class MouseState
{
public:
    struct Position
    {
        double x, y;
    };

    void keyDown(const int buttonCode) { buttons[buttonCode] = true; }
    void keyRelease(const int buttonCode) { buttons[buttonCode] = false; }
    void setPosition(Position position) { this->position = position; }

    bool isButtonDown(const int buttonCode) const { return buttons[buttonCode]; }
    Position getPosition() const { return position; }
private:
    mutable std::unordered_map<int, bool> buttons;
    Position position;
};

KeyboardState.h:

#pragma once

#include <unordered_map>

#include "Events/EventManager.h"

class KeyboardState
{
public:
    enum class ButtonState
    {
        Pressed,
        Repeat,
        Released
    };

    void keyPress(const int keyCode) { keyStates[keyCode] = ButtonState::Pressed; }
    void keyRepeat(const int keyCode) { keyStates[keyCode] = ButtonState::Repeat; }
    void keyRelease(const int keyCode) { keyStates[keyCode] = ButtonState::Released;  }

    ButtonState getKeyState(const int keyCode) const { return keyStates[keyCode]; }
    bool isKeyPressed(const int keyCode) const { return keyStates[keyCode] == ButtonState::Pressed; }
    bool isKeyRepeated(const int keyCode) const { return keyStates[keyCode] == ButtonState::Repeat; }
    bool isKeyReleased(const int keyCode) const { return keyStates[keyCode] == ButtonState::Released; }

private:
    mutable std::unordered_map<int, ButtonState> keyStates;
};

CharState.h:

#pragma once

#include <string>

class CharState
{
public:
    void charDown(const int codePoint) { charsDown += static_cast<char>(codePoint); } // TODO: make charsDown correlate to a codePoint
    void clear() { charsDown.clear(); }

    std::string getCharsDown() const { return charsDown; }

private:
    std::string charsDown;
};

InputManager.h:

#pragma once

#include "MouseState.h"
#include "KeyboardState.h"
#include "CharState.h"

class InputManager
{
public:
    InputManager(EventManager& eventManager);

    bool isKeyPressed(const int keyCode) const { return keyboardState.isKeyPressed(keyCode); }
    bool isKeyRepeated(const int keyCode) const { return keyboardState.isKeyRepeated(keyCode); }
    bool isKeyReleased(const int keyCode) const { return keyboardState.isKeyReleased(keyCode); }
    KeyboardState::ButtonState getButtonState(const int keyCode) const { return keyboardState.getKeyState(keyCode); }

    std::string getCharsDown() const { return charState.getCharsDown(); }
    void clearCharState() { charState.clear(); }

    bool isButtonPressed(const int keyCode) const { return mouseState.isButtonDown(keyCode); }
    MouseState::Position getMousePosition() const { return mouseState.getPosition(); }

    void setTextInputMode(const bool enabled) { textInputMode = enabled; }
    
private:
    template<typename EventType>
    void registerListener(EventManager& eventManager, void (InputManager::* handler)(const EventType&))
    {
        eventManager.registerListenerFor<EventType>
            ([this, handler](const EventType& event) { (this->*handler)(event); });
    }

    void handleKeyboardPressEvent(const KeyboardPressEvent& event);
    void handleKeyboardRepeatEvent(const KeyboardRepeatEvent& event);
    void handleKeyboardReleaseEvent(const KeyboardReleaseEvent& event);
    void handleCharPressEvent(const CharPressEvent& event);
    void handleMouseButtonPressEvent(const MousePressEvent& event);
    void handleMouseButtonReleaseEvent(const MouseReleaseEvent& event);
    void handleMouseMoveEvent(const MouseMoveEvent& event);

private:
    KeyboardState keyboardState;
    CharState charState;
    MouseState mouseState;

    bool textInputMode = false;
};

InputManager.cpp:

#include "InputManager.h"
#include <iostream>

InputManager::InputManager(EventManager& eventManager)
{
    registerListener(eventManager, &InputManager::handleKeyboardPressEvent);
    registerListener(eventManager, &InputManager::handleKeyboardRepeatEvent);
    registerListener(eventManager, &InputManager::handleKeyboardReleaseEvent);
    registerListener(eventManager, &InputManager::handleCharPressEvent);
    registerListener(eventManager, &InputManager::handleMouseButtonPressEvent);
    registerListener(eventManager, &InputManager::handleMouseButtonReleaseEvent);
    registerListener(eventManager, &InputManager::handleMouseMoveEvent);
}

void InputManager::handleKeyboardPressEvent(const KeyboardPressEvent& event)
{
    keyboardState.keyPress(event.getKeyCode());
}

void InputManager::handleKeyboardRepeatEvent(const KeyboardRepeatEvent& event)
{
    keyboardState.keyRepeat(event.getKeyCode());
}

void InputManager::handleKeyboardReleaseEvent(const KeyboardReleaseEvent& event)
{
    keyboardState.keyRelease(event.getKeyCode());
}

void InputManager::handleCharPressEvent(const CharPressEvent& event)
{
    if(textInputMode)
        charState.charDown(event.getCodePoint());
}

void InputManager::handleMouseButtonPressEvent(const MousePressEvent& event)
{
    mouseState.keyDown(event.getButton());
}

void InputManager::handleMouseButtonReleaseEvent(const MouseReleaseEvent& event)
{
    mouseState.keyRelease(event.getButton());
}

void InputManager::handleMouseMoveEvent(const MouseMoveEvent& event)
{
    mouseState.setPosition({ event.getMouseX(), event.getMouseY() });
}
\$\endgroup\$
1

1 Answer 1

Reset to default
3
\$\begingroup\$

About diagrams

While there was a time that UML diagrams were very popular, there are a lot of issues with them. The biggest issue with diagrams is that even if they start out to correctly describe the code, they will extremely quickly no longer represent the code. Unless you are meticulous in keeping the code and diagrams in sync, the diagrams are at best a suggestion of what the code actually does.

Consider your second diagram: it says that KeyboardState listens to KeyEvent, however if I look at your code that is not the case. Instead, it is InputManager that listens to KeyEvents.

Note that you could make it so KeyboardState registers callbacks, and make the callbacks member functions of KeyboardState.

InputManager never unregisters its callbacks

InputManager registers callbacks, but it never unregisters them. Consider this code:

EventManager eventManager;
{
    InputManager inputManager(eventManager);
}
eventManager.pushEvent<KeyboardPressEvent>(…);

You should store the EventListenerIDs in InputManager, and add a destructor that calls unregisterListener() on the IDs. Ideally, this omission would have been caught at compilation time.

There are various ways to make the code safer. First, you can annotate the return type of registerListenerFor() with [[nodiscard]]. Second, you could make EventListenerID itself have a destructor that unregisters the callback if the ID object is destroyed.

About the use of const and mutable

It's good to see you use const. This makes the code safer and more efficient. However, there are places where using const is not helpful and can even be detrimental. First, in these places it's very good to use const:

  • const function annotations
  • const reference/pointer parameters
  • const reference/pointer return value
  • const reference/pointer member variables

Then there are some places where const isn't doing much:

  • const non-reference/pointer return values (it's good to see you've removed those)
  • const non-reference/pointer parameters; these do nothing for the caller, and they very rarely prevent any bugs. I'm personally not sure it has enough benefits to warrant the extra noise it introduces.
  • const auto non-reference/pointer range-for variables; auto can deduce const itself, so it's better to make sure the container you iterate over is const.
  • const reference/pointers to temporaries (see below).

Finally, there are some places where const can be harmful:

  • const non-reference/pointer member variables. These prevent the assignment operator from working, which can be a problem. Instead of having a non-const object that contains a const member variable, it's often better to have a const object.

Conversely, don't have a const object with a mutable member variable if you can just have a non-const object with a regular member variable. Normally, if I'd see a const KeyboardState keyboardState, I'd expect that nothing can modify the state it holds. I see you use this only for the std::unordered_maps, probably because you want to be able to use operator[] on it in const functions. But you should use [.find()][3] instead, that will work in const contexts without needing mutable.

In publishEvent() you have this code:

while(const auto& event = eventQueue.pop()) {
    …
    listener->dispatchEvent(*event.get());
    …
}

While it works, it doesn't look very nice. eventQueue.pop() returns a std::unique_ptr by value, but event is a const reference. What luckily happens is that the lifetime of the temporary value returned by eventQueue.pop() is extended by storing reference to it. But this is completely unnecessary; you could just have written:

while(auto event = eventQueue.pop())

Getting access to objects stored in smart pointers

*event.get() is equivalent to *(event.get()): you first ask for a pointer to the object stored in the std::unique_ptr, and then you dereference that pointer. However, you can get access to the object directly by just writing *event:

listener->dispatchEvent(*event);

Handling events

There is a problem in your code. You expect listeners to call handle() on events to indicate they are handled and should not be passed to other listeners anymore. However, the callback functions get a const reference to the Events they handle. So they can never call handle() on the event. You could pass non-const references or make the member variable handled mutable, however there is a better way to solve this: make the callbacks return a bool or enum to indicate whether they have handled the event or not. This should then also be returned by dispatchEvent() of course. Then in publishEvent you can do something like:

for (const auto& listener : listeners) {
    if (listener->isEventType(*event) {
        if (listener->dispatchEvent(*event))
            break;
    }
}

Storing key and mouse button states

Using std::unordered_map for storing the state of keys and mouse buttons is overkill, and not very efficient. Consider that for every entry in those maps, a separate memory allocation will be made to store the key/value pair. Instead, consider using statically sized arrays. GLFW defines GLFW_KEY_LAST for the highest possible keycode, and GLFW_MOUSE_BUTTON_LAST for the highest possible mouse button.

Consider using const char* to store compile-time constant strings

Your struct EventDescription stores name as a std::string. That works, but it has some unnecessary overhead. Since you only use it to assing strings known at compile time, consider using const char* instead:

struct EventDescription
{
    EventCategory category;
    EventType type;
    const char* name;
}

And of course let getTypeName() return a const char* as well. A caller can store the result in a std::string anyway if so desired.

\$\endgroup\$
4
  • 1
    \$\begingroup\$ A problem with the id's unregstering themsleves, in the destructor the id calls unregisterListener, unregisterListener then deletes the listenerID AGAIN \$\endgroup\$ Commented Jun 30, 2023 at 10:34
  • 1
    \$\begingroup\$ Yes, there should be a distinction between the ID class returned to the caller of registerListener(), and the ID as stored in the BaseEventListener class. The latter should not call any destructor. \$\endgroup\$ Commented Jun 30, 2023 at 10:40
  • \$\begingroup\$ The EventListenerKey contains a sahred pointer to the listenerID. I added reference counting through shared pointers with a custom lambda destructor for EventListenerID, now I know how to do that too thanks! Also, are you suggesting I should move the registration to the statates themselves? 'Note that you could make it so KeyboardState registers callbacks, and make the callbacks member functions of KeyboardState.' \$\endgroup\$ Commented Jun 30, 2023 at 20:24
  • 1
    \$\begingroup\$ You don't have to move the registration to the *State classes themselves, but the graph you showed said for example that KeyboardState Listens to KeyEvent. That seems to imply to me that KeyboardState registers one of its own member functions as a callback. But what you currently have also works fine. \$\endgroup\$ Commented Jun 30, 2023 at 20:46

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.