Jump to content

  • Log In with Google      Sign In   
  • Create Account


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


Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

  • You cannot reply to this topic
45 replies to this topic

#1 Shadeirou   Members   -  Reputation: 105

Like
0Likes
Like

Posted 06 September 2011 - 08:32 PM

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:
// 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();
Listener classes:
// 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;
};
EventDispatcher:

// 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);

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

Thanks.

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

Sponsor:

#2 Codarki   Members   -  Reputation: 462

Like
1Likes
Like

Posted 07 September 2011 - 03:28 AM

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.
virtual void SetHandled(bool handled) { handled_ = handled; }
virtual bool IsHandled() const { return handled_; }

Unnecessary code.
Event() {}
virtual ~Event() {}

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.

#3 Shadeirou   Members   -  Reputation: 105

Like
0Likes
Like

Posted 07 September 2011 - 05:08 AM

These does not need to be virtual. I think it would be simpler if they weren't.

virtual void SetHandled(bool handled) { handled_ = handled; }
virtual bool IsHandled() const { return handled_; }

You're absolutely right. Those functions are no longer virtual.


Unnecessary code.

Event() {}
virtual ~Event() {}

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.


I find the "Interface" word in the class names redundant.

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.


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.

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 listener 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 listener 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.

#4 __sprite   Members   -  Reputation: 461

Like
1Likes
Like

Posted 07 September 2011 - 07:17 AM

I think your Register function can be simplified to just this?


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);
}

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?

#5 Shadeirou   Members   -  Reputation: 105

Like
0Likes
Like

Posted 07 September 2011 - 08:19 AM

I think your Register function can be simplified to just this?

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 tiny bit faster.


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).

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.


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.

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.


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?

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 Posted Image.

#6 GorbGorb   Members   -  Reputation: 112

Like
0Likes
Like

Posted 07 September 2011 - 08:42 AM

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?

#7 Shadeirou   Members   -  Reputation: 105

Like
0Likes
Like

Posted 07 September 2011 - 09:22 AM

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?

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.

#8 GorbGorb   Members   -  Reputation: 112

Like
0Likes
Like

Posted 07 September 2011 - 10:06 AM

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.

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

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
	}
};


In system_manager I then check which messages each system has to send (with template meta programming), overwrite a virtual function
void send( const MessageType &)
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.

#9 Shadeirou   Members   -  Reputation: 105

Like
0Likes
Like

Posted 07 September 2011 - 11:54 AM

EDIT: Just talked about me, sry about that.

No worries, I like to see other peoples' designs too and it's one of the things I ask for in my original post Posted Image.


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.

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.

#10 GorbGorb   Members   -  Reputation: 112

Like
0Likes
Like

Posted 07 September 2011 - 12:41 PM

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):


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;


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.

#11 alnite   Crossbones+   -  Reputation: 2038

Like
0Likes
Like

Posted 07 September 2011 - 02:53 PM

virtual void OnEvent(EventT* event) = 0;
void Dispatch(int type, EventT* event);


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

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.

#12 Shadeirou   Members   -  Reputation: 105

Like
0Likes
Like

Posted 07 September 2011 - 07:09 PM

[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.

#13 phantom   Moderators   -  Reputation: 6785

Like
0Likes
Like

Posted 08 September 2011 - 04:00 AM

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.

#14 Shadeirou   Members   -  Reputation: 105

Like
0Likes
Like

Posted 08 September 2011 - 05:24 AM

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.

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.

#15 Antheus   Members   -  Reputation: 2393

Like
1Likes
Like

Posted 08 September 2011 - 08:21 AM

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.


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):
struct Event {
  void add(boost::function<void()> callback);
  void dispatch() {
	for_each(events.begin(), events.end(), ...);
  }
private:
  vector<boost::function<void()>> events;
};
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.

#16 Codarki   Members   -  Reputation: 462

Like
0Likes
Like

Posted 08 September 2011 - 03:22 PM

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.

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);
  }
};


#17 Antheus   Members   -  Reputation: 2393

Like
0Likes
Like

Posted 08 September 2011 - 03:51 PM

Generally I'm against the function pointers.


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.

All interfaces could be done by function pointers.

A typical Java observer interface has 9 methods. With exception of demos, only one of them will ever be used.

See this.

A prime example of redundant bloat. Single function version:
void windowCallback(WindowEvent e, ChangeType t);
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.

Sure they are flexible, but if it gets out of hand, the program can be very hard to reason about.

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.

If some call fails, it might get tricky to pinpoint the target site, with debugger or by searching source code.

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.

#18 Shadeirou   Members   -  Reputation: 105

Like
0Likes
Like

Posted 08 September 2011 - 07:43 PM

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

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.



If some call fails, it might get tricky to pinpoint the target site, with debugger or by searching source code

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?



Generally I'm against the function pointers.


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.

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.


All interfaces could be done by function pointers.

A typical Java observer interface has 9 methods. With exception of demos, only one of them will ever be used.

See this.

...

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.

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.

#19 Shadeirou   Members   -  Reputation: 105

Like
0Likes
Like

Posted 09 September 2011 - 02:02 AM

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:
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);
  }
}

TestListener/TestEvents:

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) {
}

And here is the code I used to test it:
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";

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?

#20 GorbGorb   Members   -  Reputation: 112

Like
1Likes
Like

Posted 09 September 2011 - 05:24 AM

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:

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
After that, the register and disptach functions would just look like this:
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;
  //...
}
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
listener_vectors[ type_id< T >::value ];
which has O(1), but probably consumes more memory if you have multiple dispatchers.




Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.



PARTNERS