• Announcements

    • khawk

      Download the Game Design and Indie Game Marketing Freebook   07/19/17

      GameDev.net and CRC Press have teamed up to bring a free ebook of content curated from top titles published by CRC Press. The freebook, Practices of Game Design & Indie Game Marketing, includes chapters from The Art of Game Design: A Book of Lenses, A Practical Guide to Indie Game Marketing, and An Architectural Approach to Level Design. The GameDev.net FreeBook is relevant to game designers, developers, and those interested in learning more about the challenges in game development. We know game development can be a tough discipline and business, so we picked several chapters from CRC Press titles that we thought would be of interest to you, the GameDev.net audience, in your journey to design, develop, and market your next game. The free ebook is available through CRC Press by clicking here. The Curated Books The Art of Game Design: A Book of Lenses, Second Edition, by Jesse Schell Presents 100+ sets of questions, or different lenses, for viewing a game’s design, encompassing diverse fields such as psychology, architecture, music, film, software engineering, theme park design, mathematics, anthropology, and more. Written by one of the world's top game designers, this book describes the deepest and most fundamental principles of game design, demonstrating how tactics used in board, card, and athletic games also work in video games. It provides practical instruction on creating world-class games that will be played again and again. View it here. A Practical Guide to Indie Game Marketing, by Joel Dreskin Marketing is an essential but too frequently overlooked or minimized component of the release plan for indie games. A Practical Guide to Indie Game Marketing provides you with the tools needed to build visibility and sell your indie games. With special focus on those developers with small budgets and limited staff and resources, this book is packed with tangible recommendations and techniques that you can put to use immediately. As a seasoned professional of the indie game arena, author Joel Dreskin gives you insight into practical, real-world experiences of marketing numerous successful games and also provides stories of the failures. View it here. An Architectural Approach to Level Design This is one of the first books to integrate architectural and spatial design theory with the field of level design. The book presents architectural techniques and theories for level designers to use in their own work. It connects architecture and level design in different ways that address the practical elements of how designers construct space and the experiential elements of how and why humans interact with this space. Throughout the text, readers learn skills for spatial layout, evoking emotion through gamespaces, and creating better levels through architectural theory. View it here. Learn more and download the ebook by clicking here. Did you know? GameDev.net and CRC Press also recently teamed up to bring GDNet+ Members up to a 20% discount on all CRC Press books. Learn more about this and other benefits here.
Sign in to follow this  
Followers 0
Shadeirou

[C++] Looking for feedback on my event system

45 posts in this topic

I've been trying to decide how to format my event messaging system for a while now, but could never settle on a way to do it. My goals for this system were to make it fast, intuitive, and easily expandable (meaning I can easily add listeners for whatever event type I want). I ended up settling on the observer pattern to handle the way in which objects get informed of an event because it seems fast, but more so because it conforms to the design pattern of the rest of my engine.

So here is what I have. To me, it still seems like a strange way of doing it, as it seems template heavy, but it has performed well in my tests and is pretty easy to use. I would appreciate any suggestions, criticisms, improvements, other designs, etc.

Event classes:
[code]
// Description: All events derive from this base class. This can be used to
// store events of different types together.
//
// Inheritance: None
//
// Example Usage:
// std::vector<EventBase*> events;
// events.push_back(new KeyDownEvent());
// events.push_back(new KeyUpEvent());
class EventBase {
public:
// ctors/dtor: =============================================================
EventBase() : handled_(false) {}
virtual ~EventBase() {}

// public functions: =======================================================
void SetHandled(bool handled) { handled_ = handled; }
bool IsHandled() const { return handled_; }

protected:
// protected static functions: =============================================
static unsigned int GenerateUniqueID();

private:
// vars: ===================================================================
bool handled_;
};

unsigned int EventBase::GenerateUniqueID() {
static unsigned int id = 0;
++id;
return id;
}

// Description: Generates a unique integer ID for an event that derives from
// this class. All concrete events should derive from this class.
//
// Inheritance: EventBase -> Event<EventT>
//
// Example Usage:
// class SomeEvent : public Event<SomeEvent> {
// public:
// SomeEvent(int data) : data_(data) {}
//
// void SetData(int data) { data_ = data; }
// int GetData() const { return data_; }
//
// private:
// int data_;
// };
template<typename EventT>
class Event : public EventBase {
public:
// public static constants: ================================================
static const unsigned int kID;
};

template<typename EventT>
const unsigned int Event<EventT>::kID = Event<EventT>::GenerateUniqueID();
[/code]
Listener classes:
[code]
// Description: All event listeners derive from this base class. This can be
// used to store listeners of different types together.
//
// Inheritance: None
//
// Example Usage:
// std::vector<EventListenerBase_Interface*> listeners;
// listeners.push_back(new KeyDownListener());
// listeners.push_back(new KeyUpListener());
class EventListenerBase_Interface {
public:
// ctors/dtor: =============================================================
virtual ~EventListenerBase_Interface() {}
};

// Description: Generates a unique OnEvent method for each event input for
// EventT. All event listeners should derived from this class.
//
// Inheritance: EventListenerBase_Interface -> EventListener_Interface<EventT>
//
// Example Usage:
// class SomeEventListener
// : public EventListener_Interface<KeyDownEvent>,
// public EventListener_Interface<KeyUpEvent> {
// public:
// SomeEventListener() {}
//
// void OnEvent(KeyDownEvent* event) {
// // do something
// }
//
// void OnEvent(KeyUpEvent* event) {
// // do something
// }
// };
template<typename EventT>
class EventListener_Interface : public EventListenerBase_Interface {
public:
// dtors: ==================================================================
virtual ~EventListener_Interface() {}

// public functions: =======================================================
virtual void OnEvent(EventT* event) = 0;
};
[/code]
EventDispatcher:
[code]

// Description: Used to register events listeners and dispatch events.
//
// Inheritance: None
//
// Example Usage:
// EventDispatcher dispatcher;
// SomeListener listener;
//
// dispatcher.Register<SomeEvent>(SomeEvent::kID, &listener);
// dispatcher.Dispatch(SomeEvent::kID, new SomeEvent());
class EventDispatcher {
public:
// typedefs: ===============================================================
typedef std::vector<EventListenerBase_Interface*> ListenerVector;
typedef std::map<int, ListenerVector> ListenerMap;

// ctors/dtor: =============================================================
EventDispatcher() {}

// public functions: =======================================================
// - dispatch event
template<typename EventT>
void Dispatch(int type, EventT* event);

// - register listeners
template<typename EventT>
void Register(int type, EventListener_Interface<EventT>* listener);

private:
// vars: ===================================================================
ListenerMap listeners_;
};

template<typename EventT>
void EventDispatcher::Dispatch(int type, EventT* event) {
if (event == NULL) { return; } // disallow a NULL event

ListenerMap::iterator itr = listeners_.find(type);
if (itr == listeners_.end()) { return; } // event type not found

// dispatch event to the listeners
for (unsigned int x = 0; x < itr->second.size(); ++x) {
static_cast<EventListener_Interface<EventT>*>(
itr->second.at(x))->OnEvent(event);
}
}

template<typename EventT>
void EventDispatcher::Register(int type, EventListener_Interface<EventT>* listener) {
if (listener == NULL) { return; } // disallow a NULL listener
listeners_[type].push_back(listener);

}
[/code]
Again, feedback would be greatly appreciated. I would prefer not to use RTTI, but I'm open to suggests.

Thanks.

EDIT: De-virtualized [i]SetHandled [/i]and [i]IsHandled [/i]in EventBase and removed the ctor and dtor for Event at Ftn's suggestion.
EDIT: Altered [i]Register[/i] in EventDispatcher in the way __sprite suggested.
0

Share this post


Link to post
Share on other sites
I have only some nitpicking that could possible simplify the public interfaces.

These does not need to be virtual. I think it would be simpler if they weren't.
[code]
virtual void SetHandled(bool handled) { handled_ = handled; }
virtual bool IsHandled() const { return handled_; }
[/code]

Unnecessary code.
[code]
Event() {}
virtual ~Event() {}
[/code]

I find the "Interface" word in the class names redundant.
If the event parameter in EventListener would be a reference, you could omit all checks for null event.
Likewise in EventDispatcher Register method, if the listener parameter would be a reference, it would indicate to the user that the EventDispatcher will not call delete to given listeners.
1

Share this post


Link to post
Share on other sites
[quote name='Ftn' timestamp='1315387706' post='4858538']
These does not need to be virtual. I think it would be simpler if they weren't.
[code]
virtual void SetHandled(bool handled) { handled_ = handled; }
virtual bool IsHandled() const { return handled_; }
[/code]
[/quote]You're absolutely right. Those functions are no longer virtual.


[quote name='Ftn' timestamp='1315387706' post='4858538']
Unnecessary code.
[code]
Event() {}
virtual ~Event() {}
[/code]
[/quote]Right again, the constructor and destructor have been removed. I typically write a virtual destructor for any class with virtual functions, but, since this one doesn't have any, it's not necessary.


[quote name='Ftn' timestamp='1315387706' post='4858538']
I find the "Interface" word in the class names redundant.
[/quote]I agree, but I believe in making code as explicit as possible. I generally only use multiple inheritance when at most one of the base classes has implementation and all others are pure interface classes, so I tag pure interface classes with the "Interface" suffix to make it easier to keep track of that policy.


[quote name='Ftn' timestamp='1315387706' post='4858538']
If the event parameter in EventListener would be a reference, you could omit all checks for null event.
Likewise in EventDispatcher Register method, if the listener parameter would be a reference, it would indicate to the user that the EventDispatcher will not call delete to given listeners.
[/quote]My problem with references as parameters is that references act like values in syntax but behave like pointers when it comes to semantics. On top of that, it goes back to my want for code to be explicit; If I call Register<SomeEvent>(SomeEvent::kID, listener) it is not obvious that the function can change [i]listener [/i]or keep track of it via references or pointers. In fact, I would assume, if I only looked at that line of code, that it would either be passed as a constant reference or copied and, therefore, could not be manipulated. Now, if I call Register<SomeEvent>(SomeEvent::kID, &listener) it is clear to me that [i]listener [/i]can be manipulated. With that in mind, I wanted the OnEvent function to be able to change the event passed into it so it could set it as handled if need be. If I'm not using reference parameters, pointers are the only way left. That's my take on it anyway.

Thanks for your suggestions.
0

Share this post


Link to post
Share on other sites
I think your Register function can be simplified to just this?

[code]

template<typename EventT>
void EventDispatcher::Register(int type, EventListener_Interface<EventT>* listener) {
if (listener == NULL) { return; } // disallow a NULL listener

// I think this is equivalent to what you're doing atm... (insert default constructed item if the thing isn't there)
listeners_[type].second.push_back(listener);
}
[/code]

If you want to make sure your listeners are unique, you could use a set instead of a vector. That might be faster than checking if something's already registered yourself (the commented out code).

Personally I'd use references instead of pointers. I don't really see how pointers are clearer -> you have to check that they're valid everywhere, and if you're looking up the parameter (because you don't know that it's a reference / pointer already), you'd immediately see that it's a non-const reference anyway.

Also, I'm curious to know what "handled" actually gives you. I'm not sure that things passing in the event should care about whether the event is handled or not (and we can't tell whether that's by one listener or more than one). Do you have a specific use in mind for it?
1

Share this post


Link to post
Share on other sites
[quote name='__sprite' timestamp='1315401429' post='4858602']
I think your Register function can be simplified to just this?
[/quote]Aha! Thanks, I wasn't aware that map::operator[] inserted a new value for that key if the key wasn't found. That makes that function a hell of a lot smaller and a [i]tiny[/i] bit faster.


[quote name='__sprite' timestamp='1315401429' post='4858602']
If you want to make sure your listeners are unique, you could use a set instead of a vector. That might be faster than checking if something's already registered yourself (the commented out code).
[/quote]Your right, but it also makes dispatching the events a much slower. I tried iterating through a set instead of a vector to dispatch events and it was almost four times slower in many separate tests. Since EventDispatcher should be dispatching events much more often than registering listeners, it is probably best I use vectors and just allow duplicates. Of course, as long as I'm paying attention I shouldn't be registering a listener more than once anyway.


[quote name='__sprite' timestamp='1315401429' post='4858602']
Personally I'd use references instead of pointers. I don't really see how pointers are clearer -> you have to check that they're valid everywhere, and if you're looking up the parameter (because you don't know that it's a reference / pointer already), you'd immediately see that it's a non-const reference anyway.
[/quote]Yeah, I think it's just a person preference thing for me really. That and I tend to follow the Google C++ Style Guide and it frowns on reference parameters.


[quote name='__sprite' timestamp='1315401429' post='4858602']
Also, I'm curious to know what "handled" actually gives you. I'm not sure that things passing in the event should care about whether the event is handled or not (and we can't tell whether that's by one listener or more than one). Do you have a specific use in mind for it?
[/quote]I have an idea in mind that I have not implemented as of the time I'm writing this. Basically, I want OnEvent to have the option to set the event as being handled. If it does this, I would have a check in the DispatchEvent function that would break out of the loop if the event was handled, preventing it from being passed to other listeners. It's kind of a work in progress for me at the moment. Don't know if it is going to be in the final version or not.

Thanks for your suggestions. I was really over thinking my register function [img]http://public.gamedev.net/public/style_emoticons/default/smile.gif[/img].
0

Share this post


Link to post
Share on other sites
You might want to look into the boost implementation of the signal&slot concept.
Apart from that, on what level do you need communication? Are your entities(/components) communicating with each other or are unique compile-time-known systems sending and receiving messages?
0

Share this post


Link to post
Share on other sites
[quote name='GorbGorb' timestamp='1315406540' post='4858648']
You might want to look into the boost implementation of the signal&slot concept.
Apart from that, on what level do you need communication? Are your entities(/components) communicating with each other or are unique compile-time-known systems sending and receiving messages?
[/quote]
Well, that's a good question. At the moment I'm only theorizing about the practical applications of this system, so I don't have a specific answer other than to say it's probably more the latter. I know that I'm going to have a few systems set up for general use that use an event dispatcher, such as a SDL_Event dispatcher that will poll events in the main loop and dispatch to listeners that need information about key presses, mouse clicks, etc. Another example would be a central game event system where listeners could trigger an event that gets sent to an event dispatcher and dispatched back out to all the listeners (Player triggers AlarmEvent -> AlarmEvent dispatched through EventDispatcher to all Players -> Players handle AlarmEvent). As for listeners/entities communicating with each other, that's not really the intent of this system, unless you were referring to something like one of the previous examples.
0

Share this post


Link to post
Share on other sites
I asked because I've been working on a framework for inter system communication that dispatches messages with constant time overhead, independent from the number of listeners. Unfortunately, it needs a good c++11 compiler (dependency on variadic template arguments and SFINAE) and still a lot of testing.
Here is my concept:
I put all my systems in a special container, e.g.

[code]
system_manager< SystemA , SystemB , ... , SystemX > manager;
[/code]
Each system can derive from a 'sending' base, where the system has to specify which messages it wants to send, e.g.
[code]

class SystemA
: public sending< MessageA , MessageB >
{
void f()
{
send( MA() );
}
void handle( const MessageX &)
{
//code for handling MessageX and all messages derived from or convertible to MessageX
}
};

[/code]

In system_manager I then check which messages each system has to send (with template meta programming), overwrite a virtual function [code] void send( const MessageType &)[/code] for each MessageType that dispatches the message to all systems in the manager at the same time (with ordinary function calls & SFINAE, so with compiler optimizations pretty much just 1 virtual function call to send messages to any number of systems).
If you want to actually make a game I don't recommend implementing that though, it took me ages to make it work.

EDIT: Just talked about me, sry about that.
I guess your system will work the way you posted it. But why do you think you need a centralized message system? Both the sender and the receiver have to know which message they want to send/listen to, so it should be easy to have a special dispatcher for each event. That way, both registering and dispatching will be faster, because you don't have to do a map lookup.
0

Share this post


Link to post
Share on other sites
[quote name='GorbGorb' timestamp='1315411616' post='4858690']
EDIT: Just talked about me, sry about that.
[/quote]
No worries, I like to see other peoples' designs too and it's one of the things I ask for in my original post [img]http://public.gamedev.net/public/style_emoticons/default/wink.gif[/img].


[quote name='GorbGorb' timestamp='1315411616' post='4858690']
I guess your system will work the way you posted it. But why do you think you need a centralized message system? Both the sender and the receiver have to know which message they want to send/listen to, so it should be easy to have a special dispatcher for each event. That way, both registering and dispatching will be faster, because you don't have to do a map lookup.
[/quote]
You have a point, but this system came about as a result of having to write multiple classes and/or functions for each event class/structure that I wrote. I wanted a generic dispatcher that could handle any number of events, event types, and listeners, but that is not to say it is necessary. Also, I may have overplayed the "centralized" part a bit. I don't mean to have only one EventDispatcher that every listener registers to, but rather to inherit from EventDispatcher for specific event groups, e.g. SDLEventDispatcher to dispatch SDL events (SDL_KEYDOWN, SDL_MOUSEBUTTONDOWN, SDL_QUIT, etc.) or PlayerEventDispatcher to dispatch player events (player moved, player took damage, player died, etc.). Creating a special dispatcher for each event seems like overkill to me because SDL alone has at least 16 different event types, not to mention any I come up with. That's 16 different dispatchers at the very least that I would have to instantiate and maintain, whereas a centralized one for SDL events would do the same and probably wouldn't be that worse off due to the map lookup.
0

Share this post


Link to post
Share on other sites
If your compiler supports variadic templates and you are ok to have all message types compiled into your dispatcher you could do something like this (just a demo):


[code]
template< class MessageType >
class Handler
{
public:
virtual void handle( const MessageType & ) = 0 ;
};

template< class ...MessageTypes >
class Dispatcher;

template<>
class Dispatcher<>
{
public:
void dispatch();
void add_listener();
//these functions are just dummies to be able to do 'using Base::dispatch' in derived classes
};

template< class Head , class ...Tail >
class Dispatcher< Head , Tail... >
: public Dispatcher< Tail... >
{
private:
std::vector< Handler< Head >* > handlers;
public:
void dispatch( const Head &Message )
{
//iterate over all handlers and call 'handle'
}
using Dispatcher< Tail... >::dispatch; //to avoid name hiding
void add_listener( Handler< Head > &HeadHandler )
{
//add HeadHandler to the vector of handlers
}
using Dispatcher< Tail... >::add_listener;
};

//using the Dispatcher:

typedef Dispatcher< MessageA , MessageB , MessageC > SDL_Events;

[/code]

This would add the dependency on variadic templates (I know gcc supports it, but msvc++ 2010 not, for example).
In addition, you have to recompile all modules that depend on SDL_Events if you want to add new messages.
On the other hand, you save the map lookup. It depends on your requirements how much performance you are willing
to sacrifize for modularity.
0

Share this post


Link to post
Share on other sites
[quote]
[code]virtual void OnEvent(EventT* event) = 0;
void Dispatch(int type, EventT* event);
[/code]
[/quote]

Aren't these supposed to be
[code]virtual void OnEvent(Event<EventT>* event) = 0;
void Dispatch(int type, Event<EventT>* event);[/code]
?

Also, I don't know if somebody has brought this up, but there's a flaw in your system that it seems each type of event is tied to a type of listeners. This means I can't have two different subsystems generating the same type of events but listening to different events.
0

Share this post


Link to post
Share on other sites
[font="arial, verdana, tahoma, sans-serif"][size="2"][quote name='GorbGorb' timestamp='1315420915' post='4858748']
If your compiler supports variadic templates and you are ok to have all message types compiled into your dispatcher you could do something like this (just a demo):

[code removed]

This would add the dependency on variadic templates (I know gcc supports it, but msvc++ 2010 not, for example).
In addition, you have to recompile all modules that depend on SDL_Events if you want to add new messages.
On the other hand, you save the map lookup. It depends on your requirements how much performance you are willing
to sacrifize for modularity.
[/quote][/size][/font]
This is an interesting method, but I'm not sure I want to rely on variadic templates just yet. If I'm not mistaken, it's being introduced in the new C++ standard? I would prefer to wait until that becomes standard and more compilers support it. I am using GCC, however, so I will probably mess around with that now and keep it in my toolbox for later as the overhead from the map lookup is one of my biggest issues with my system. Thanks.


[quote name='alnite' timestamp='1315428839' post='4858797']
Aren't these supposed to be
[code]virtual void OnEvent(Event<EventT>* event) = 0;
void Dispatch(int type, Event<EventT>* event);[/code]
?
[/quote]
Not on the first one, unless I wanted to convert the event to type EventT in each OnEvent so the function could access the data of the actual event (EventT) and not just the base class (Event<EventT>). If I did do conversions each time in the OnEvent functions, I would still have to write a separate function for each event type, so I don't see the benefit in changing OnEvent(EventT*) to OnEvent(Event<EventT>*).

On the other hand, I could absolutely change Dispatch(int, EventT*) to Dispatch(int, Event<EventT>*) and cast the event to an EventT once before dispatching it. The trade-off, then, is that the dispatcher can no longer handle events that don't inherit from that class, so it would be an enforced rule that the Dispatcher could only dispatch event types inherited from my class. And if I enforce that, I may as well take out the "int type" part from each function because I could just get that type via Event<EventT>::kID in the function itself. That would simplify the calls from Register<EventT>(type, &listener) and Dispatch(type, &event) to just Register<EventT>(&listener) and Dispatch(&event), which seems more intuitive to me. Interesting.


[quote name='alnite' timestamp='1315428839' post='4858797']
Also, I don't know if somebody has brought this up, but there's a flaw in your system that it seems each type of event is tied to a type of listeners. This means I can't have two different subsystems generating the same type of events but listening to different events.
[/quote]
I'm not sure I know entirely what you mean. Yes, each event type Event<EventT> is tied to a listener type EventListener_Interface<EventT>, but the classes that inherit from EventListener_Interface<EventT> can handle any number of event types in their own unique ways:
[code]
class Listener1 : public EventListener_Interface<KeyDownEvent>, public EventListener_Interface<KeyUpEvent> {
public:
void OnEvent(KeyDownEvent* event) {
// do something unique to this class

}

void OnEvent(KeyUpEvent* event) {
// do something unique to this class
}
};

class Listener2 : public EventListener_Interface<KeyDownEvent> {
public:
void OnEvent(KeyDownEvent* event) {
// do something unique to this class

}
};

int main() {
EventDispatcher disp;
Listener1 l1;
Listener2 l2;

disp.Register<KeyDownEvent>(&l1);
disp.Register<KeyUpEvent>(&l1);
disp.Register<KeyDownEvent>(&l2);

dist.Dispatch(new KeyDownEvent(some, key, data));
}
[/code]

If I completely missed your point, I apologize.
0

Share this post


Link to post
Share on other sites
Personally I dislike the whole Java-esque 'inheriate to recieve messages' setup. I contend that a delegate pattern is far more flexible and probably just as fast.

However as I've already written a series of massively long posts on it recently I shant go down that route again.
0

Share this post


Link to post
Share on other sites
[quote name='phantom' timestamp='1315476036' post='4858974']
Personally I dislike the whole Java-esque 'inheriate to recieve messages' setup. I contend that a delegate pattern is far more flexible and probably just as fast.

However as I've already written a series of massively long posts on it recently I shant go down that route again.
[/quote]
I'm not closed off to the idea of using a delegate pattern, I just haven't seen one done well enough to make me use it. I did just skim over the... uh... book of an argument between you and speciesUnknown on this subject (and bookmarked it for later in-depth reading), but I would appreciate any insights you have on the subject. I'm not looking for an argument, but rather some guidance as, honestly, I just want something that works well, is flexible, and allows me to get the job done.
0

Share this post


Link to post
Share on other sites
[quote name='Shadeirou' timestamp='1315481078' post='4858999']
I'm not looking for an argument, but rather some guidance as, honestly, I just want something that works well, is flexible, and allows me to get the job done.
[/quote]

1) You are given a class, let's say ThirdPartyFoo. You want to notify its bar() when something happens. Suddenly, you are inheriting third-party code that might not have been meant to inherit.

2) The classes are provided to you by a factory, all you get is base interface IBase which has one pure virtual method foo(). Nothing to inherit, you don't even have class definitions

3) You want to call free function. If using SDL, or many C-like libraries, there won't be many classes or methods, so nothing to inherit.

The solution to above is always a wrapper class. Preferably an anonymous class. This works great in Java or C#, even in other languages - because they are memory managed.

In C or C++, you are suddenly stuck with a temporary anonymous object whose lifecycles is a total mess between event system and instance it operates on.

But it gets better. Since the callback definitions are polymorphic by very nature, they cannot be passed around by value. References cannot be stored. So suddenly there's a bunch of pointers, possibly wrapped in smart pointers, making things very complicated and verbose.

---

Delegates are basically boost::function. A value class, passed and copied arbitrarily. They are capable of dispatch to free or member functions, can deal with any type of polymorphism and require minimal bookkeeping.

In short (ignoring signature overhead):[code]struct Event {
void add(boost::function<void()> callback);
void dispatch() {
for_each(events.begin(), events.end(), ...);
}
private:
vector<boost::function<void()>> events;
};[/code]And that's it.

All the problems solved by ::function, works with any and every situation possible and takes basically 5 lines to implement.

It's even possible to abandon the Event class and just keep vectors around.

PS: ::function will become standard, there are variadic templates, etc.... The above just works reliably today across all platforms and compilers made over last 10 or more years.
1

Share this post


Link to post
Share on other sites
Generally I'm against the function pointers.

Function pointers can be seen as anonymous interface with single function. All interfaces could be done by function pointers. Sure they are flexible, but if it gets out of hand, the program can be very hard to reason about. If some call fails, it might get tricky to pinpoint the target site, with debugger or by searching source code.

This case is a single function interface though. Luckily interface vs function pointer solutions are orthogonal, you can do both.

[code]
template<typename EventT>
class EventListener
{
public:
virtual ~EventListener_Interface() {}
virtual void OnEvent(EventT* event) = 0;
};

template<typename EventT>
class FunctionPointerEventListener : public EventListener<EventT>
{
std::function<void(EventT*)> m_callback;

public:
FunctionPointerEventListener(std::function<void(EventT*)> callback)
: m_callback(callback)
{
}

void OnEvent(EventT* event)
{
m_callback(event);
}
};
[/code]
0

Share this post


Link to post
Share on other sites
[quote name='Ftn' timestamp='1315516949' post='4859218']
Generally I'm against the function pointers.[/quote]

Which works well, until one needs to use third-party libraries.

It's not really a preference, but it simply cannot be done any other way.

[quote]All interfaces could be done by function pointers.[/quote]
A typical Java observer interface has 9 methods. With exception of demos, only one of them will ever be used.

See [url="http://download.oracle.com/javase/1.4.2/docs/api/java/awt/event/WindowListener.html"]this[/url].

A prime example of redundant bloat. Single function version:[code]void windowCallback(WindowEvent e, ChangeType t);[/code]And no, the other version is not more OO, it doesn't help with polymorphic aspects at all. All such code ends up using instanceof, which is worse than a switch that would be required for second version.

[quote]Sure they are flexible, but if it gets out of hand, the program can be very hard to reason about.[/quote]I'll counter with Java. Almost no listener is ever implemented beyond one single method. So one is suddenly burdened with abstract helpers and adapters, causing no end to code bloat.

[quote]If some call fails, it might get tricky to pinpoint the target site, with debugger or by searching source code.[/quote]
What's the difference between boost::function<void()> failing and ICallback * failing? Both are just pointers.

If using debugger, callstack points out where something failed.


Anyone using Qt however will be wary of function pointers. Because Qt is basically a port of Java idioms and standard library combined with intrinsic VM-like resource management and *all* of C++ stuff. A bloat of epic proportions that makes boost look like Hello World application.
0

Share this post


Link to post
Share on other sites
[font="arial, verdana, tahoma, sans-serif"][size="2"][quote name='Ftn' timestamp='1315516949' post='4859218']
This case is a single function interface though. Luckily interface vs function pointer solutions are orthogonal, you can do both.
[/quote]
[/size][/font][font="arial, verdana, tahoma, sans-serif"][size="2"]I don't know why I would implement both interfaces and function pointers. It seems to me that using function pointers would make the FunctionPointerEventListener unnecessary and would just add an extra layer of unnecessary complexity, as well as the overhead of a virtual function call and function pointer call.[/size][/font]



[quote name='Ftn' timestamp='1315516949' post='4859218']
[color="#1C2837"][size="2"]If some call fails, it might get tricky to pinpoint the target site, with debugger or by searching source code[/size][/color]
[/quote]
How is that different from an interface? Shouldn't the debugger point out where the call failed in both the function pointer and the interface?


[quote name='Antheus' timestamp='1315518684' post='4859236']
[quote name='Ftn' timestamp='1315516949' post='4859218']
Generally I'm against the function pointers.[/quote]

Which works well, until one needs to use third-party libraries.

It's not really a preference, but it simply cannot be done any other way.
[/quote]
I'm not sure I agree. It can be done using interfaces, but it, again, adds an extra layer of complexity. For instance, if I wanted a function from another library to be called when an event happened I would make a new class that inherits from the base event listener class and calls that function in OnEvent.


[quote name='Antheus' timestamp='1315518684' post='4859236']
[quote]All interfaces could be done by function pointers.[/quote]
A typical Java observer interface has 9 methods. With exception of demos, only one of them will ever be used.

See [url="http://download.oracle.com/javase/1.4.2/docs/api/java/awt/event/WindowListener.html"]this[/url].

...

I'll counter with Java. Almost no listener is ever implemented beyond one single method. So one is suddenly burdened with abstract helpers and adapters, causing no end to code bloat.
[/quote]
In my current implementation I only inherit if and when the object is one that needs to be notified when an event of the inherited listener interface type happens. If an object of that class is not registered, however, I guess I would end up with a bunch of unused OnEvent functions...

I am starting to lean toward using a delegate pattern and function pointers for this.
0

Share this post


Link to post
Share on other sites
So I converted my system to using function pointers as opposed to interfaces, but the only drawback seems to be that it is a bit slower. I tested each system with 500 listeners and 50000 events and blank member functions for the objects passed. On average, the old interface system dispatched all events to all listeners in ~134 ms while the function pointer system dispatched all events in ~208 ms.

Here's my updated system:[code]
class EventDispatcher {
public:
// typedefs: ===============================================================
typedef std::vector<void*> ListenerVector;
typedef std::map<int, ListenerVector> ListenerMap;

// ctors/dtor: =============================================================
EventDispatcher() {}
virtual ~EventDispatcher() {}

// public functions: =======================================================
// - listeners_ access
template<typename EventT>
void Register(int type, void (*func)(EventT*));

template<typename EventT, typename ClassT>
void Register(int type, void (ClassT::*func)(EventT*), ClassT* obj);

// - dispatch event
template<typename EventT>
void Dispatch(int type, EventT* event);

private:
// vars: ===================================================================
ListenerMap listeners_;
};

template<typename EventT>
void EventDispatcher::Register(int type, void (*func)(EventT*)) {
listeners_[type].push_back(new std::function<void(EventT*)>(func));
}

template<typename EventT, typename ClassT>
void EventDispatcher::Register(int type, void (ClassT::*func)(EventT*), ClassT* obj) {
listeners_[type].push_back(
new std::function<void(EventT*)>(
std::bind(func, obj, std::placeholders::_1)));
}

template<typename EventT>
void EventDispatcher::Dispatch(int type, EventT* event) {
if (event == NULL) { return; } // disallow a NULL event

ListenerMap::iterator itr = listeners_.find(type);
if (itr == listeners_.end()) { return; } // type not found

// dispatch event to the listeners
for (unsigned int x = 0; x < itr->second.size(); ++x) {
// I know, I know, this is fugly, but it didn't appear to affect
// performance to static_cast the void* to std::function* in my tests.
(*static_cast<std::function<void(EventT*)>*>(itr->second.at(x)))(event);
}
}[/code]

TestListener/TestEvents:[code]

class TestEvent1 : public inp::Event<TestEvent1> {
public:
TestEvent1(int x, int y, int z) : x_(x), y_(y), z_(z) {}

int x_, y_, z_;
};

class TestEvent2 : public inp::Event<TestEvent2> {
public:
TestEvent2(const std::string& str) : str_(str) {}

std::string str_;
};

class TestListener
: public inp::EventListener_Interface<TestEvent1>,
public inp::EventListener_Interface<TestEvent2> {
public:
TestListener();

void OnEvent(TestEvent1* event);
void OnEvent(TestEvent2* event);
void OnTestEvent1(TestEvent1* event);
};


TestListener::TestListener() {}

void TestListener::OnEvent(TestEvent1* event) {
}

void TestListener::OnEvent(TestEvent2* event) {
}

void TestListener::OnTestEvent1(TestEvent1* event) {
}[/code]

And here is the code I used to test it:[code]
int listeners = 500, events = 50000;
lib::Timer timer;

// TEST FUNCTION-BASED EVENTDISPATCHER ---------------------------------------
EventDispatcher func_disp;

timer.Start();
for (int x = 0; x < listeners; ++x) {
func_disp.Register(TestEvent1::kID,
&TestListener::OnTestEvent1,
new TestListener);
}
cout << "fn: " << timer.GetTicks() << "\n";

timer.Start();
for (int x = 0; x < events; ++x) {
func_disp.Dispatch(TestEvent1::kID, new TestEvent1(1, 2, 3));
}
cout << "fn: " << timer.GetTicks() << "\n\n";

// TEST INTERFACE-BASED EVENTDISPATCHER --------------------------------------
inp::EventDispatcher interface_disp;

timer.Start();
for (int x = 0; x < listeners; ++x) {
interface_disp.Register<TestEvent1>(new TestListener);
}
cout << "in: " << timer.GetTicks() << "\n";

timer.Start();
for (int x = 0; x < events; ++x) {
interface_disp.Dispatch(new TestEvent1(1, 2, 3));
}
cout << "in: " << timer.GetTicks() << "\n";[/code]

So, is there something that I'm doing wrong for it to be slower than the old system, or is that just the nature of the beast?
0

Share this post


Link to post
Share on other sites
Ok, although I don't use std::function too often, I noticed some things to improve:
- you are leaking memory
- why don't you just let users register a std::function object directly?

In addition, you could hide some boilerplate (event id , event base ) with this technique:

[code]
unsigned int next_id()
{
unsigned int previous_id = 0;
return ++previous_id; //or return previous_id++ , depending on wheter you need a NULL value
}
template< class Type >
class type_id
{
public:
static unsigned int value;
};
template< class Type >
unsigned int type_id< Type >::value = next_id(); //unique value for each type
[/code]
After that, the register and disptach functions would just look like this:
[code]
template< class EventType >
void register( std::function< void( const EventType &) > callback )
{
unsigned int id = type_id< EventType >::value;
//...
}
template< class EventType >
void dispatch( const EventType &event )
{
unsigned int id = type_id< EventType >::value;
//...
}
[/code]
That way, you can use any type you want (e.g. an int or an std::string) as an EventType and save a lot of boilerplate and bugs because you can't accidentally use a wrong event id.

To save the map lookup, you could use a vector whose size is as big as the number of EventType s you have instead of a map.
(to do is, check in register and dispatch whether your vector is large enough and resize it if needed).
Then you can just access the listener_vector for any type T with
[code]
listener_vectors[ type_id< T >::value ];
[/code]
which has O(1), but probably consumes more memory if you have multiple dispatchers.
1

Share this post


Link to post
Share on other sites
[quote name='GorbGorb' timestamp='1315567449' post='4859437']
Ok, although I don't use std::function too often, I noticed some things to improve:
- you are leaking memory
- why don't you just let users register a std::function object directly?
[/quote]
Where? You mean in the test code? If so, I know, I was just testing so I didn't care [img]http://public.gamedev.net/public/style_emoticons/default/smile.gif[/img]. If the dispatcher is leaking memory, however, that is another story.


[quote name='GorbGorb' timestamp='1315567449' post='4859437']
In addition, you could hide some boilerplate (event id , event base ) with this technique:

[code]
unsigned int next_id()
{
unsigned int previous_id = 0;
return ++previous_id; //or return previous_id++ , depending on wheter you need a NULL value
}
template< class Type >
class type_id
{
public:
static unsigned int value;
};
template< class Type >
unsigned int type_id< Type >::value = next_id(); //unique value for each type
[/code]
[/quote]
Indeed! Very nice, that is a much better way of doing it. I salute you sir.

[quote name='GorbGorb' timestamp='1315567449' post='4859437']

To save the map lookup, you could use a vector whose size is as big as the number of EventType s you have instead of a map.
(to do is, check in register and dispatch whether your vector is large enough and resize it if needed).
Then you can just access the listener_vector for any type T with
[code]
listener_vectors[ type_id< T >::value ];
[/code]
which has O(1), but probably consumes more memory if you have multiple dispatchers.
[/quote]
I thought of this, but was unsure if I should do it this way or not. It would surely consume more memory, but hopefully to a lesser extent if I use a vector of vector pointers and set the thus far unused ones to NULL until they are needed for the first time.


0

Share this post


Link to post
Share on other sites
You have some std::vector< void* > s. You fill them with 'new std::function<...>'. Where do you delete this memory?
A solution would be to have something like a
[code]
template< class EventType >
class listener_vector
: public std::vector< std::function< void( const EventType & ) > > ,
public virtual_base //has virtual destructor
{
};
[/code]
and store a vector< virtual_base* > instead.
Concerning the memory issue: You could make the dispatcher a singleton (although I dislike singletons). If there is just one instance of the dispatcher, there won't be a memory overhead when using vector instead of map. Since dispatching is independent from the number of events, it won't hurt your performance.
0

Share this post


Link to post
Share on other sites
I've not had a chance to look closely at the code however what might be hurting you here is the dispatcher having to go via a void pointer for every intrested party.

You can probably solve this by introducing a class which holds std::function/boost::function objects for a certain type and using that in your primary map and then having that class perform the dispatch.

Sure, it adds another layer BUT I suspect this would be less painful than doing multiple casts from void to the correct type at dispatch time (it would certainly help the compiler).

I'll take a closer look once I get home and see if I can whip up some psuedo code to demo what I mean if you haven't already come up with something :)
0

Share this post


Link to post
Share on other sites
[quote name='GorbGorb' timestamp='1315572674' post='4859464']
You have some std::vector< void* > s. You fill them with 'new std::function<...>'. Where do you delete this memory?
[/quote]
Ah, yes. This was just a quick version I made up for testing. I'm first getting the design I want and testing it for efficiency, then going back and adding in all the necessary parts, such as deleting those, once I figure out which I plan on using. Currently I have 5 different versions of this class, so I'm skipping a lot of steps [img]http://public.gamedev.net/public/style_emoticons/default/unsure.gif[/img].

[quote name='GorbGorb' timestamp='1315572674' post='4859464']


Concerning the memory issue: You could make the dispatcher a singleton (although I dislike singletons). If there is just one instance of the dispatcher, there won't be a memory overhead when using vector instead of map. Since dispatching is independent from the number of events, it won't hurt your performance.
[/quote]
I thought about this as well, but I feel the same way about singletons. A class with static members and functions would work the same way, but I'm not sure I'm ready to go the completely centralized messaging system route just yet. It may actually counteract some of the reasons I'm switching to a delegate pattern. For instance, if I have a button class that inherits or uses an event dispatcher to dispatch a ButtonDownEvent (or whatever) I can register specific functions to that button (e.g. void OnMenuButtonDown(const ButtonDownEvent&) registered to Button menu_button) and messages would get dispatched to only that button's functions, saving a good bit of time. If I use a central messaging system, I would have to register functions to the general ID for ButtonDownEvent. Then, whenever any button was pressed, all functions registered to that event type would get the message and each would have to have a filter for the correct button. A way around that would be to alter my design and add some kind of pairing between the event type and its sender, which wouldn't be too difficult, but it would increase the number of registered event types from 1 (UniqueTypeID<ButtonDownEvent>::kID) to the amount of objects created with registered functions, which would probably be all of them.
0

Share this post


Link to post
Share on other sites
[quote name='phantom' timestamp='1315573870' post='4859471']
I've not had a chance to look closely at the code however what might be hurting you here is the dispatcher having to go via a void pointer for every intrested party.

You can probably solve this by introducing a class which holds std::function/boost::function objects for a certain type and using that in your primary map and then having that class perform the dispatch.

Sure, it adds another layer BUT I suspect this would be less painful than doing multiple casts from void to the correct type at dispatch time (it would certainly help the compiler).

I'll take a closer look once I get home and see if I can whip up some psuedo code to demo what I mean if you haven't already come up with something :)
[/quote]
I would appreciate it [img]http://public.gamedev.net/public/style_emoticons/default/laugh.gif[/img]. I'll post the one that is running fastest for me at the moment.
[code]
// NOTE: Test version. Needs a lot of clean up code and extra features.
class EventDispatcher {
public:
// typedefs: ===============================================================
typedef std::function<void(inp::EventBase*)> ListenerCallback;
typedef std::vector<ListenerCallback> ListenerVector;
typedef std::map<int, ListenerVector> ListenerMap;

// ctors/dtor: =============================================================
EventDispatcher() {}
virtual ~EventDispatcher() {}

// public functions: =======================================================
// - listeners_ access
void Register(int type, void (*func)(inp::EventBase*));

template<typename ClassT>
void Register(int type, void (ClassT::*func)(inp::EventBase*), ClassT* obj);

// - dispatch event
void Dispatch(int type, inp::EventBase* event);

private:
// vars: ===================================================================
ListenerMap listeners_;
};

// public functions ============================================================
// - listeners access ----------------------------------------------------------
void EventDispatcher::Register(int type, void (*func)(inp::EventBase*)) {
listeners_[type].push_back(ListenerCallback(func));
}

template<typename ClassT>
void EventDispatcher::Register(int type, void (ClassT::*func)(inp::EventBase*), ClassT* obj) {
listeners_[type].push_back(
ListenerCallback(std::bind(func, obj, std::placeholders::_1)));
}

// - dispatch event ------------------------------------------------------------
void EventDispatcher::Dispatch(int type, inp::EventBase* event) {
if (event == NULL) { return; } // disallow a NULL event

ListenerMap::iterator itr = listeners_.find(type);
if (itr == listeners_.end()) { return; } // type not found

// dispatch event to the listeners
for (unsigned int x = 0; x < itr->second.size(); ++x) {
itr->second.at(x)(event);
}
}[/code]
It seems that by passing a function with the EventBase as a parameter and then typecasting in that function I can increase speed a bit, but it is not necessarily the way I want to do it.

[s]Also, how do you +1 people these days? I have people I must give credit to, such as GorbGorb, and can't seem to find it ><.[/s] Found it.
0

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!


Register a new account

Sign in

Already have an account? Sign in here.


Sign In Now
Sign in to follow this  
Followers 0