Convenient event handling mechanism

[AMDmi3]

We need c++11 style event handling mechanism instead of legacy switch-loop

[AMDmi3]

How I see it.

With SDL2:

while (SDL_PollEvent(&event)) {
    if (event.type == SDL_KEYDOWN) {
        switch (event.key.keysym.sym) {
        case SDLK_LEFT:     
            HandleLeft();           
            break;                  
        case SDLK_RIGHT:    
            HandleRight();           
            break;                  
        }
    }
}

With SDL2pp:

SDL2pp::EventHandler eh;
eh.HandleKeyDown(SDLK_LEFT, [&](){ HandleLeft(); });
eh.HandleKeyDown(SDLK_RIGHT, [&](){ HandleRight(); });
eh.PollAll();

In more complex cases it may be less readable though.

[ooxi]

I'm not that familiar with C++11 but I guess [&](){ HandleRight(); } is an lambda capture? I would propose being able to pass the event union (for example SDL_KeyboardEvent in this case) to the lambda in order to be able to process additional information, for example repeat information.

[AMDmi3]

Yes, that's lambda. Sure, corresponding event structure will be passed, I just also want to provide a way to not pass it when it's needed. Lambda syntax is pretty hard to read even without arguments :(

[Vraiment]

If you have ever used XNA they have several static classes in the Input namespace that you can access like, and you can do stuff like:
var mouseState = Mouse.GetState();

I think it would be far more usable (but less C++11ish) to have something of this kind:

struct Events {
    Events() {
        SDL_Event sdlEvent;
        while (SDL_PollEvent(&sdlEvent)) {
            // logic to store all the events
            // in other words a giant switch
        }
    }
    
    bool isKeyDown(SDL_Keycode key) const {
        return keys.find(key) != keys.end();
    }
    
    // ... more const methods to check for events ...
    
private:
    std::set<SDL_Keycode> keys;
    // ... more members to store the event info ...
};

It doesn't have to be that monolithic, other idea is there could be one struct/class for each event type:

struct Events {
    Events() {
        SDL_Event sdlEvent;
        std::vector<SDL_Event> keyboardEvents;
        while (SDL_PollEvent(&sdlEvent)) {
            if (SDL_KEYDOWN == sdlEvent.type || SDL_KEYUP == sdlEvent.type) {
                keyboardEvents.push_back(sdlEvent);
            } else if
            // logic for the rest of the events
        }

        keyboard = Keyboard(keyboardEvents);
    }

    struct Keyboard {
        Keybard(std::vector<SDL_Event> keyEvents) {
            // logic to fill in the keys set
        }
        
        bool isKeyDown(SDL_Keycode key) const {
            return keys.find(key) != keys.end();
        }

    private:
        std::set<SDL_Keycode> keys;
    };

    Keyboard getKeyboard() const {
        return keyboard;
    }

    // More structs for the rest of the events

private:
    Keyboard keyboard;
};

That way you can pass around the polled events instead of just listening to a fixed set of callbacks.

[Vraiment]

I have written a small proposal of how it could work, you can see it at my repo in the event_manager branch is not complete but it gives you general idea

[AMDmi3]

Honestly this doesn't look too useful to me. You store events in a vector - what's the purpose of it? You seem to try to recreate keyboard state from the events - why is this needed while there's SDL_GetKeyboardState() which doesn't even need wrapping?

My idea was oo-wrapping around clumsy event handling code SDL project have, e.g. mainly an event.type switch. The base idea is a virtual class which handles all the events, e.g.

class EventHandler {
protected:
    virtual void OnKeyboard(keysum, keycode, ...) {
    }

    virtual void OnMouseButton(point, state, clicks, ...) {
    }
    ...

public:
    static int PollOneEvent() {
        SDL_Event event;
        if (!SDL_PollEvent(&event))
            return 0;

        switch (event.type) {
        case SDL_MOUSEBUTTON:
            OnMouseButton(event.x, event.y, ...);
            break;
            ....
        }

        return 1;        
    }

    static int PollAllEvents() {
        while (PollOneEvent()) {}
    }
};

Now, users can encapsulate event handling code in EventHandler derived class and forget an ugly switch.

From here, it's possible to build more, both high and low level abstractions which allow users to avoid even more boilerplate code.

• (higher level) EventLoop class which adds just a couple of virtual methods, but allows to drop even more boilerplate code, implementing almost a whole application as a single class:

class EventLoop: public EventHandler {
protected:
    virtual void Frame() = 0;

public:
    void Run() {
        while (true) {
            PollAllEvents();
            ProcessFrame();
        }
    }
}

Usage:

class MyGame : SDL2pp::EventLoop {
    void OnKeydown() {
       // set player speed
    }
    
    void Frame() {
       // update game state
       // render scene
    }
}

int main() {
    MyGame().Run();
}

• (lower level) EventHandlerRegistry class which could maintain per-eventype lists of handler functors and could be used in imperative code, without reorganizing it into a class:

int main() {
    EventHandlerRegistory ehr;
    ...
    ehr.AddMouseMotionHandler([&](){
        // set player speed
    });

    while (true) {
        ehr.PollAllEvents();
        // update game state
        // render scene
    }
}

[Vraiment]

I didn't know SDL_GetKeyboardState() existed, but yeah the idea was to provide functionality like that for every event type (I used keyboard as an example), but your proposals sound far better, just one thing:

On the "callback" base approach (EventHandlerRegistry) should there be a way to "unregister" callbacks? Having AddMouseMotionHandler to me implies you can RemoveMouseMotionHandler.
The other way to go would be to have a single callback per event per EventHandlerRegistry (ie: having methods like SetMouseMotionHandler instead).

[AMDmi3]

On the "callback" base approach (EventHandlerRegistry) should there be a way to "unregister" callbacks?

Yes! I though about it but forgot to add it to an example. There should probably should just be a single RemoveHandler method, as long as handler IDs returned by Add*Handler are global.

[Vraiment]

I have uploaded another branch with my proposal for the change (this one is pretty bare bones compared to the other one), you can find it in my repo and is called event_handler_2.0, relevant files are: EventHandler.hh/EventHandler.cc.

Stuff I know will rise questions, and my reasoning to do it that way:

• Why have an "OnUnkownEvent" method?: This is just a placeholder while the rest of the events are implemented, should be eliminated after that? Not sure, in case SDL adds a new event this is a nice way to allow a user to catch an event without the need to update SDLpp but if later we update (which we should) the user won't receive the event where it expects it and would need to update again its code base.

• Why having two versions of "PollOneEvent"?/Why return a vector of events from "PollAllEvents"?: I think the user should have the chance to go through the actual events that were polled aside from implementing the methods (which should be the preferred way) but I can't think of a good reason for this other than retrieving a single event (the vector of events doesn't seem very useful), thoughts on this? Should we drop it?

• Why not passing the actual values to the events instead of the event structure?: Again I think the users should have the ability to access the full information of the event, if we pass just some values (ex: the mouse position) other important info (ex: like the window id or the timestamp) won't be accessible to the user.

[AMDmi3]

Why have an "OnUnkownEvent" method?

Doesn't rise any questions, this is a good idea.

I think the user should have the chance to go through the actual events that were polled aside from implementing the methods

This would be a bad design and we should not tolerate that. E.g. if event handling logic in encapsulated in a class, it should not leak outside. But yes, it should be flexible, and I've had a couple of additional ideas which may help improve this:

1. Having multiple handlers for a single event. For example, since SDL_MouseButtonEvent has a flag which indicates button state, user may want a generic mouse button event handler, so we might consider having

virtual void OnMouseButton(SDL_MouseButtonEvent event);
virtual void OnMouseButtonDown(SDL_MouseButtonEvent event);
virtual void OnMouseButtonUp(SDL_MouseButtonEvent event);

and the former is called in both cases. Likewise, we may have

virtual void OnAnyEvent(SDL_Event event);

This adds a bit of confusion, but at the same time it adds the flexibility, which IMO overweights. The call order IMO should be from more generic to less generic (e.g. OnAnyEvent, then OnMouseButton, then OnMouseButtonDown).

2. make HandleEvent() public. It would allow one to use this class with events supplied from outside, allowing using it with custom event extracting logic. The class will remain clear and concentrated on event dispatching. "Do one thing and do it good".

Again I think the users should have the ability to access the full information of the event

I agree. It's very tempting to have event info passed through arguments, but I guess it's just too inflexible.

[AMDmi3]

Now some comments on the code (I guess it's time to add CONTIBUTING.md to the repo...):

Please const SDL_QuitEvent&, as more effective and safe (in theory, we could allow modifying events, but let's not. Usage patterns of this are not clear to me yet, and another design would be required, e.g. one which allows explicit specification of handler order):

OnQuit(SDL_QuitEvent event);

void, as explained above:

Optional<SDL_Event> PollOneEvent();
std::vector<SDL_Event> PollAllEvents();

switch, please:

if (SDL_QUIT == event.type) { OnQuit(event.quit); }
else if (SDL_WINDOWEVENT == event.type) { OnWindowEvent(event.window); }

also I'm an ardent opponent of reversing comparison order in conditions: this makes programmers intent less apparent and the code less readable, while "= instead of ==" problem is easily detected by static analyzers and compilers for a long time.

[Vraiment]

So I've been giving it a long thought because it bothers me the idea of calling several no-op functions which on top would be confusing for the users but after your posts I realized it, we are doing it wrong, we are coupling event polling and event handling.

Basically the polling functions are vey simple wrappers for the SDL_PollEvent() function and don't necessarily needs to know about event handling, consider:

template<typename T>
bool PollEvent(T eventHandler);

template<typename T>
int PollAllEvents(T eventHandler);

Where T is an object that can be called with SDL_Event as argument: free functions/lambdas/classes or structs with operator(SDL_Event) member functions. We can argue about about the readability of the operator(SDL_Event) vs a named member function (like HandleEvent(SDL_Event)) but that we can have that discussion later.

Now, this only solves half of the problem (polling) and we need a solution for the actual event handling to save the users the horrible switch event.type { ... } preferably preventing the no-op function calling. This can be done by using templates to generate a "compile time dynamic if".

We need an easy templated way to say: for any given SDL_EventType call all of the given functions that accept the specific event structure, example:

class PlayerEventHandler { ... };
void HandleMenuInput(SDL_KeyEvent);
...
auto playerEventHandler = getPlayerEventHandler();

bool quit = false;
auto quitEventHandler = [](SDL_QuitEvent) { quit = true; };

auto eventHandler<SDL_KEYDOWN, SDL_KEYUP, SDL_QUIT>(playerEventHandler, HandleMenuInput, quitEventHandler);

while(!quit) {
    PollAllEvents(eventHandler);
}

That way the user can explicitly say what are the only events that should be handled and we can use lambdas in addition to classes/structs.

[Kiritow]

I have been watching this issue for days. And I hope my looper class will help you.

class Looper
{
public:
    Looper();
    /// If Callback does not return 0, then stop transferring this event.
    LooperID add(_SDLEventType_ event_type,const std::function<int(Looper&,Event&)>& event_callback);
    LooperID add(_SDLEventType_ event_type,const std::function<int(Event&)>& event_callback);
    LooperID add(_SDLEventType_ event_type,const std::function<int(Looper&)>& event_callback);
    LooperID add(_SDLEventType_ event_type,const std::function<int()>& event_callback);

    /// If Callback has no return value (void), then we assume it returns 0.
    LooperID add(_SDLEventType_ event_type,const std::function<void(Looper&,Event&)>& event_callback);
    LooperID add(_SDLEventType_ event_type,const std::function<void(Event&)>& event_callback);
    LooperID add(_SDLEventType_ event_type,const std::function<void(Looper&)>& event_callback);
    LooperID add(_SDLEventType_ event_type,const std::function<void()>& event_callback);

    LooperID operator + (const std::pair<_SDLEventType_,std::function<int(Looper&,Event&)>>& event_callback);
    LooperID operator + (const std::pair<_SDLEventType_,std::function<int(Event&)>>& event_callback);
    LooperID operator + (const std::pair<_SDLEventType_,std::function<int(Looper&)>>& event_callback);
    LooperID operator + (const std::pair<_SDLEventType_,std::function<int()>>& event_callback);

    LooperID operator + (const std::pair<_SDLEventType_,std::function<void(Looper&,Event&)>>& event_callback);
    LooperID operator + (const std::pair<_SDLEventType_,std::function<void(Event&)>>& event_callback);
    LooperID operator + (const std::pair<_SDLEventType_,std::function<void(Looper&)>>& event_callback);
    LooperID operator + (const std::pair<_SDLEventType_,std::function<void()>>& event_callback);

    bool remove(const LooperID& looperid);

    bool operator - (const LooperID& looperid);

    void dispatch();
    void run();
    Event GetLastEvent();
    void needupdate();
    void stop();
    void reset();

    std::function<void()> updater;

protected:
	LooperID _getNextID(const _SDLEventType_ & event_type);
    Event _e;
    bool _running,_update;
    std::map<_SDLEventType_,std::list<std::pair<int,std::function<int(Looper&,Event&)>>>> _evmap;
    int _loop_cnt;
};

This class is a wrapper for event in my SDL project. It is used like this:

Looper lp;
/// Add Event Handler of SDL_QUIT
lp.add(SDL_QUIT,[](Looper& lp)
       {
           lp.stop();
       });
/// Set screen updater
lp.updater=[&]()
{
    /// Update Screen.
};
/// Start the looper
lp.run();

[Vraiment]

I like how you are using the operator+() as in .NET's event callbacks. I can't exactly understand how your Looper class works with stuff like dispatch(). What I wonder is how do you generate the LooperID for each callback?

[Kiritow]

Here is my implement of Looper::dispatch()

void Looper::dispatch()
{
    for(auto callbackPack:_evmap[_e.type])
    {
        if(callbackPack.second(*this,_e)) break;
    }
}

You can view this for the whole Looper class implement .