Eventmanager Plugin

[greebo]

I overhauled the Radiant command system in the last three days (mostly because I got fed up with all the ToggleItem::addCallbackCaller stuff) and introduced the new EventManager plugin, that handles all the Command/Toggle stuff.

 

There are (currently) five types of Events available:

 

- Commands (fire a single callback when invoked)

- Toggles (fire a callback when invoked and maintain an internal true/false state)

- WidgetToggles (show/hide the connected widgets when invoked, derives from Toggle)

- RegistryToggles (inverts a certain RegistryKey when invoked, derives from Toggle)

- KeyEvent (fires a keyUp and a keyDown event when the key/modifier is pressed, this is used for the Camera movements at the moment)

(more can easily added when necessary, but these should cover most applications for now.)

 

Each of these Event can be attached to an Accelerator (i.e. key/modifier combo), which can be enabled/disabled. All the connected shortcuts are stored in input.xml now, so the shortcuts.ini is soon becoming obsolete.

 

Each Event can be attached to a GtkWidget (e.g. MenuItem, ToggleToolButton, etc.). Just call the connectWidget() method with the GtkWidget* as argument and the rest is done by the plugin. The Events figure out themselves which GtkWidgets are supported and which are not. As the most common Widgets for commands are ToolButtons and MenuItems, both are supported (the ToolbarCreator already makes use of the new methods).

 

I already migrated all of the native Radiant Commands and most of the Toggles, but this is WIP. The plan is also to move the EventMapper into this plugin, so that both mouse and keyboard events are handled by the new EventManager.

 

When I will be finished with this, most of the commands.h/commands.cpp and old gtkutil::accelerator stuff will be gone. This also applies to the ToggleItem and ToggleShown objects in gtkutil and some other small things. For now, both systems co-exist, the code cleanup is still to come.

 

Note: If there occur any problems, please delete your local input.xml, I had some rare cases where the input.xml messed up the system and made Radiant fail to load, so beware for this.

[OrbWeaver]

Sounds good, compiling now.

 

If you are doing any refactoring involving the Callback system, you might like to check out boost::function (http://www.boost.org/doc/html/function.html), which is the Boost equivalent, and seems more flexible in many ways (for example, you can pass both a functor object and a function pointer to a boost::functor as long as they have the same interface). It would probably be quite difficult to replace the entire callback system with boost::function, but if you ever think "hey, the callback system is pretty ugly, I wonder if there is a Boost equivalent", there is.

[OrbWeaver]

I get a segfault on startup, from SaveEventVisitor.h

 

void visit(const std::string& eventName, IEvent* event) {
	// Sanity check 
	if (event != NULL && eventName != "") {
		// Try to find an accelerator connected to this event
		Accelerator* accelerator = dynamic_cast<Accelerator*>(_eventManager->findAccelerator(event));

		std::string keyStr = "";
		std::string modifierStr = "";

		if (accelerator != NULL) {
			keyStr = gdk_keyval_name(accelerator->getKey()); // <-----  * SEGFAULT HERE *
			modifierStr = _eventManager->getModifierStr(accelerator->getModifiers());
		}

 

Incidentally, what is the function of dynamic_cast here? I changed accelerator to type IAccelerator*, removed the cast and it compiled fine (although still crashed).

[greebo]

Hang on, I will look into this. I guess the dynamic_cast is a leftover from previous debugging, when I hadn't implemented the getKey() methods yet (I think, I'm not entirely sure).

[OrbWeaver]

A few initial comments on the code:

 

1. Prefer references over pointers where possible, especially when passing objects as parameters to functions. This wipes out a whole class of problems related to NULL pointers. For example, IEventVisitor::visit() should probably take an IEvent&, not an IEvent*. This also makes the syntax nicer because you don't have to use -> and &.

2. Where you have to allocate stuff on the heap[1], it is almost always best to use a boost::shared_ptr<> to hold the result, rather than a raw pointer. If you only ever learn one Boost class in your lifetime, shared_ptr should be it. This wipes out most of the other class of problems with NULL pointers that were not covered by (1). In particular, if you use shared pointers you will never have to call delete, ever. (Note: shared pointers should be passed by value, not by reference).

3. Consider enhancing xmlutil::Node with methods to add children, text and other stuff. This will avoid having to mess around with libxml2's ugly API, and (once again) removes a source of pointer nastiness. Ideally the Node class should not have a method to return its internal pointer, as this is implementation detail which should be hidden.

 

Summary: pointers are evil.

 

[1] which I am not entirely sure you do, in this instance. Command and Accelerator seem to be small objects which could be inserted into an STL container by value, although I haven't examined in enough detail to see if there is in fact some reason why this is not possible (perhaps Callback is non-copyable?).

[greebo]

Ok, will try to change this (and I think I already did try it), but I ran into a few problems with instantiating the classes that derive from abstract base classes. It's most probably me not knowing how to work around the errors - I will post back here when I encounter them.

[OrbWeaver]

Ok, will try to change this (and I think I already did try it), but I ran into a few problems with instantiating the classes that derive from abstract base classes. It's most probably me not knowing how to work around the errors - I will post back here when I encounter them.

 

You may run into problems if you try to pass an ABC by value -- if you pass a derived class instead of the base class, the derived class will be "sliced" into the base class, which is not what you want.

 

Using references does not have this problem however.

[greebo]

I cannot reproduce this crash on my system. Could this be one of these problems that only producte crashes under Linux but not under Win32?

 

I will try to refactor this thing using references and commit the changes as I progress, maybe the problem will become obvious to me then or disappears at all.

[OrbWeaver]

I cannot reproduce this crash on my system. Could this be one of these problems that only producte crashes under Linux but not under Win32?

 

Possibly, I will try a full rebuild and see if I can find anything useful with GDB.

 

I will try to refactor this thing using references and commit the changes as I progress, maybe the problem will become obvious to me then or disappears at all.

 

That may well happen, there was a segfault in the shaders plugin which miraculously disappeared as soon as I removed raw pointers and replaced them with boost::shared_ptr.

[greebo]

Okay, I have the following problem (this will be quite some post, so thanks in advance for reading this ):

 

I have this abstract base class of an event, namely IEvent:

class IEvent
{
public:
// Handles the incoming keyUp / keyDown calls
virtual void keyUp() = 0;
virtual void keyDown() = 0;

// Enables/disables this event
virtual void setEnabled(const bool enabled) = 0;

// Connect a GtkWidget to this event (the event must support the according widget). 
virtual void connectWidget(GtkWidget* widget) = 0; 

// Exports the current state to the widgets
virtual void updateWidgets() = 0;

virtual bool empty() const = 0;
};

Note the method empty() which states if the event is pointing to anything or not. I have a base class Event that derives from IEvent and forms the base of all other Events (Toggle, Command, and so on).

 

The Event implements the method virtual bool empty() and returns true by default:

virtual bool empty() const {
	std::cout << "Event::empty() called\n";
	// This is the base class for an event, it's empty by default
	return true;
}

 

I have one more class Toggle which is deriving from Event and overloads the method empty() so that it returns "not empty".

bool Toggle::empty() const {
std::cout << "Toggle::empty() called\n";
return false;
}

 

Now to my problem: the EventManager provides a method to lookup an Event by passing its name:

// Returns the pointer to the command specified by the <given> commandName
virtual IEvent& findEvent(const std::string& name) = 0;

As this method is defined in the ABC, the method just returns an IEvent& reference, as the actual type of the event should be hidden within the EventManager plugin.

 

If I lookup the command "ToggleCamera", I retrieve an IEvent& and I want to check, whether this event is empty or not. I do the following:

IEvent& event = GlobalEventManager().findEvent("ToggleCamera");

if (!event.empty()) {
	event.connectWidget(GTK_WIDGET(camwnd->getParent()));
	event.updateWidgets();
}

My problem is, by calling event.empty() the method in the Event class Event::empty() is called instead of Toggle::empty(), as I would like it to.

 

I'm a bit stuck here, because I don't know how to call the empty() method of the actual Event without trying to cast the reference on every possible event type. I didn't have this problem with pointers, however (if I returned a pointer IEvent* and called event->empty(), the overloaded method got called, as intended).

 

Any advice? Should I elaborate some more?

[OrbWeaver]

How are the IEvent's allocated, and who "owns" them? Presumably they are stored in some data structure like a std::map or similar - what is the declaration for this map? If you are storing them by value (std::map<std::string, IEvent>) then you will lose the dynamic binding due to the slicing problem I mentioned - in this case you would need to allocate them on the heap (but use shared_ptr, not raw pointers).

[greebo]

I have this map:

typedef std::map<const std::string, Event> EventMap;

So obviously the instances are getting reduced to an Event on storing?

 

Well then, I already read a bit about boost::shared_ptr and I will try to refactor it (again).

[OrbWeaver]

I have this map:

typedef std::map<const std::string, Event> EventMap;

So obviously the instances are getting reduced to an Event on storing?

 

Yup, that's the problem.

 

Although references are preferable to pointers for function parameters, you cannot really use them in a map because there is no way to allocate a persistent object by reference - new always returns a pointer which you have to keep hold of in order to delete the object. This is where you should use shared_ptr.

 

Well then, I already read a bit about boost::shared_ptr and I will try to refactor it (again).

 

For the most part, you can use a shared_ptr exactly like a real pointer, except that you don't have to worry about deleting it. For example:

 

{
boost::shared_ptr<IEvent> event(new IEvent());

event->doSomething(); // same indirection semantics as pointer

} // object deleted at end of scope, unless you have stored it somewhere else

[greebo]

Is there an equivalent of dynamically casting a shared_ptr boost::shared_ptr onto something else, say boost::shared_ptr? I would like to find out about what type of class this shared_ptr is pointing at.

 

Lately, I was using dynamic_cast(IEvent* event) to find out, what do I do here? Do I have to implement some sort of getType() method?

 

This does not work (with typedef boost::shared_ptr TogglePtr):

TogglePtr foundToggle = dynamic_cast<TogglePtr>(findEvent(name));

[OrbWeaver]

shared_ptr<Toggle> = dynamic_pointer_cast<Toggle>(findEvent(name));

 

I do suggest considering, however, whether the need for dynamic_cast indicates that the interface needs adjusting. I am not necessarily saying it is wrong to use dynamic_cast -- there are certainly many legitimate uses for it -- but in general the idea of an ABC is that it encapsulates all of the public interface that a hierarchy of classes needs to provide, without having to worry (or even know) which particular subclass you are dealing with at any given moment.

 

For example, instead of doing

 

if (dynamic_cast<ChildA>(baseClassPtr)
 // do Child-A specific stuff here
else if (dynamic_cast<ChildB>(baseClassPtr))
 // do Child-B specific stuff here

 

it is far better to define a virtual function Base::doSpecificStuff which is implemented by each subclass, so that you can just call baseClassPtr->doSpecificStuff() and get the correct behaviour in every case.

[greebo]

Well, it may be that my IEvent ABC is kept a bit too general then. The above example was the only case I needed to distinguish between a Toggle and an IEvent anyway. I will work around this.

[greebo]

I just saw that there is a dynamic_cast for boost::shared_ptrs:

shared_ptr<T> dynamic_pointer_cast(shared_ptr<U> const & r);

 

But I will nevertheless work around the above behaviour.

 

Currently I'm googling for this problem:

IEventPtr(new Command(name, callback));

which tries to allocate a new Command object (deriving from Event deriving from IEvent), and turn it into a boost::shared_ptr. However, I'm getting this error:

boost.w32/include/boost/shared_ptr.hpp:211: error: invalid conversion from `IEvent* const' to `Event*'

Maybe this problems has to do with the const although I don't know exactly how I can work around it.

 

Would it be safe to use this:

IEvent* event = new Command(name, callback);
_events[name] = IEventPtr(event);

or is it mandatory to place the new into the IEventPtr constructor?

 

edit: Nevermind, the above doesn't work either.

 

edit2: Even more nevermind, I think I found the problem...

[OrbWeaver]

Would it be safe to use this:

IEvent* event = new Command(name, callback);
_events[name] = IEventPtr(event);

or is it mandatory to place the new into the IEventPtr constructor?

 

Yes, that is perfectly safe to do, although obviously a little more verbose (also putting new directly into the constructor prevents you from inadvertently accessing the raw pointer later in the function). Glad you've found the problem though.

[greebo]

I forgot to remove the typdef from the plugin after moving the IEventPtr definition into the include files.

 

I have a working version now that will committed as I type. Please do test, if DarkRadiant still crashes. I will continue to work in the other changes you suggested.

[New Horizon]

Hmmm, crashes when I try to create an AI.

 

Entities and models are alright.

[greebo]

I haven't touched that code, so perhaps there is something else broken? I had this problem a while ago when creating AI, which was caused by an old module object (.dll/.so) left in the modules/ folder. Are you compiling under Linux?

[OrbWeaver]

There were a couple of crashes in the shaders plugin which were triggered by AI creation (as well as loading certain texture directories), but they should be fixed now. Have you done a complete rebuild?

[New Horizon]

There were a couple of crashes in the shaders plugin which were triggered by AI creation (as well as loading certain texture directories), but they should be fixed now. Have you done a complete rebuild?

 

Yup, did an update and a rebuild. Just updated to the latest changes and it's still crashing on me. Hmmmm. Maybe a stray file kicking around on my end for some reason? I'll try a clean update and then rebuld.

[greebo]

Be sure to clean your install/modules/ folder to remove any legacy .dll/.so files before rebuilding.

[OrbWeaver]

Well my error is fixed