5
\$\begingroup\$

I have been trying to create an event queue system using SDL. I've been trying to use this for Pong. The game is not even finished yet, but I stopped work to examine how my event system is organized.

  • Event_Queue - This is the heart of the system. All events go through here and all listeners come through here.
  • Event - This holds a name string, a type string, and a Boost.Any object to be able to package any type of data along with it. Boost.Any will act as a wrapper for another general data structure, but in the case that it isn't general enough, another can be easily adopted.
  • Event_Receivable - Interface between Event_Receiver and object; I think it's unclean but I don't know of a better way than inheritance to do this.
  • Event_Receiver - The object that is tacked onto an object to be able to receive objects. It is also a type of interface--logic will be handled within the object, preferably through a Logic module, and is intended to be hooked up to a scripting API later.
  • Event_Sender - This is supposed to send events onto the Event_Queue. This will probably be improved so that I can just use std::move or something, but this is supposed to always be the outlet from where Events are fired.

Does this constitute a complete event system? Are there any flaws besides the glaring ignorance regarding copy, references, and move?

Event_Queue.h:

class Event_Queue
{
private:

    SDL_Event input;

    std::queue<Event> event_queue;
    std::vector<Event_Receiver*> receiver_list;
public:
    void add_receiver(Event_Receiver* receiver_ptr);
    bool remove_receiver(Event_Receiver* receiver_ptr);

    void push_event(Event e);

    void poll_input();
    void poll_event();

};

Event_Receivable.h:

class Event_Receivable
{
public:
    virtual ~Event_Receivable() = 0;
    virtual void on_notify(Event* e) = 0;

};

Event_Receiver.h:

class Event_Receiver
{
public:
    Event_Receiver(Event_Receivable* owner_obj);
    ~Event_Receiver();

    void receive_event(Event* e);
    void notify_owner(Event* e);
    void set_owner(Event_Receivable* owner_obj) {owner = owner_obj;}

    Event_Receivable* get_owner_obj() {return owner;}

private:
    Event_Receivable* owner;

};

Event.h:

#include "Common.h"
#include "boost/any.hpp"

/*
 * Event is used extensively within the program.
 * It is the basis for all messages in the event system.
 * Objects will send and receive events.
 *
 * It uses boost::any in order to be able to
 * attach any kind of data to be read.
 *
 * Event is meant to be extremely flexible in order to
 * be extensible to many different types of games.
 */
class Event

    {
    private:
        std::string name;
        std::string type;
        std::vector<boost::any> info;
    public:
        Event(std::string name_str, std::string type_str)
        {
            name = name_str;
            type = type_str;
        }

        void set_name(std::string str) {name = str;}
        void set_type(std::string str) {type = str;}
        void attach_data(boost::any data) {info.push_back(data);}

        std::string get_name() {return name;}
        std::string get_type() {return type;}
        boost::any get_info() {return info;}
    };

Event_Sender.h:

class Event_Sender
{
public:
    void send_event(Event e);
    void send_event(Event e, Event_Queue q);

    void set_event_queue(Event_Queue e_queue) {event_queue = &e_queue;}

    Event_Queue* get_event_queue() {return event_queue;}

private:
    Event_Queue* event_queue;
};

GitHub

This code already does compile correctly with dependencies sorted out, but I need feedback on the system.

\$\endgroup\$
2
\$\begingroup\$

It looks good to me. The Event_Receivable thing looks like a vanilla observer pattern, which involves inheritance. I don't really have comments on the architecture, but I recommend a look at this Game Programming Patterns chapter if you haven't already.

Code review

A few considerations on the design:

  1. The first thing that jumps to the eye here is the heavy use of raw pointers. The event queue, for instance, has a vector of Event_Receiver pointers which it does not own. It would be a disaster if one of those objects were to be deleted while the pointer is still in the queue's vector. Event_Receiver also keeps a raw pointer to its owner. I think you should take a step back and rethink this. It can save you a lot of headache on pointer debugging in the future, not to mention the potential for memory leaks, which can be a serious problem in a game. If you can use C++11, then consider the standard smart pointers. It seems that a combination of shared_ptrs and weak_ptrs would be suitable here.

  2. You already know this, but when you do decide how to handle copy/assignment, take a look at the Rule of Three. The safest course of action until then would be to make the types that hold naked pointers, or arrays of pointers, non-copyable, by deleting the copy constructor and assignment operator. That might actually suit your needs and it is very easy to do.

  3. You are using strings to define the event type and name/id; not sure if this is a good approach. The only reason I would chose to represent those as strings would be if I had the need to create event types dynamically during runtime. The two main issues with using strings are 1) performance: it is much faster comparing an integer/enum-constant than it is comparing a string; 2) when declaring/referencing an event in the code, if you mistype a string, you will only find out about the error after a little runtime debugging. That problem goes away with compile-time constants. Your choice should really be based on how flexible you need the system to be, then choose your trade-off.

Miscellaneous:

  1. Your get* methods are missing const at the end, which would make it impossible to call one of them on a const object. Since they don't mutate member data, make them const correct. E.g.:

    boost::any get_info() const { return info; }
                          ^^^^^
    

    Still talking about the getters, I'm not sure how costly it is to copy, for instance, a boost::any, but if the caller will mostly just be inspecting the value, then consider returning complex objects by const reference. E.g.:

    const boost::any & get_info() const { return info; }
    ^^^^^            ^            ^^^^^
    
  2. Looking in the repository link, I see that you have some very long-winded for loops in Event_Queue, such as this one from Event_Queue::poll_input():

    for(std::vector<Event_Receiver*>::iterator it = receiver_list.begin(); it != receiver_list.end(); ++it)
    

    Such huge line can be made a lot simpler by using a range-based for loop (C++11), or at least the use of auto for the iterators.

  3. Event_Sender seems to be missing a constructor, which would leave its member pointer uninitialized until it is set somewhere else. This is very dangerous and can be a source of bugs. Always initialize your variables to a known value. Don't leave room for chance.

  4. Looking at the implementation of Event_Receiver, the destructor is empty, so you can just remove it and let the compiler supply a default.

  5. You have a mixed style of public/private class sections. Some have private at the start, others have it at the end. I'd suggest that you make it uniform through all classes. My personal preference is to place the public section of a class first in the header file. The rationale is that people using and reading my code will care much more about the public methods and data, so that's what they want to see first when reading the header file. Private/protected sections of a class are of interest to the programmer maintaining the code, so it makes sense that they don't need that much visibility.

  6. Use the constructor initializer list to init member data. Using the assignment operator might result in redundant initialization if the compiler is not able to optimize the code (calls the default constructor of a type + operator =).

    Event(std::string name_str, std::string type_str)
        : name(std::move(name_str))
        , type(std::move(type_str))
    { }
    

    That's also a perfect place for a move, to avoid copying things over and over. Without move the best would have been taking name_str and type_str by const reference.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.