[BUG] Singleton issue with custom widgets

[earthsruler]

Hey all,

I was following this tutorial on creating custom CEGUI widgets:

http://www.cegui.org.uk/wiki/index.php/ ... istboxItem

During close if my widget was on the screen it was causing quite a nasty crash.

Ok let me set the scene. We have a GUIManager that is a singleton. During GUIManager::initialisation() among other things we call our version of bindCEGUI which is something like this:

Code: Select all

void bindCEGUI()
{
    CEGUI::WindowFactoryManager& wfMgr = CEGUI::WindowFactoryManager::getSingleton();
    wfMgr.addFactory(&CEGUI_WINDOW_FACTORY(CheckListboxItem));
    wfMgr.addFalagardWindowMapping("WindowsLook/CheckListboxItem", "CEGUI/ItemEntry", "WindowsLook/CheckListboxItem", "Falagard/ItemEntry");
}


For this code to work the WindowFactoryManager has to be created, and therefore this function has to be called after the CEGUI::System has been created.

Our GUIManager is created first to create CEGUI then as part of the initialisation and after CEGUI has been created the custom widget factory singleton is created.

The problem is this. Singletons are deleted in the reverse order in which they are created. When they are destroyed in reverse the custom widget factory goes first, followed by the GUIManager. The GUIManager then tries to destroy all windows in the system through their coresponding factories, however the custom widget factory has already been destroyed resulting in a "pure virtual function call" crash (which was hard to pinpoint).

A workaround we are using at the moment is having a static function in the guimanager called preinitialisation() that calls get singleton on all the custom CEGUI widget factories and is designed to be called before the GUIManager::getSingleton(), however this doesn't seem like a very elegant solution, as it relies on the user to have the correct initialisation sequence to allow custom widgets to work safely.

My suggestion would be to take a more OO approach and provide the window factory manager with an instance of the factory as apposed to a function pointer. Then the order of destruction of singletons would be less of an issue as CEGUI would be maintaining its own copy of the factories and not relying on the user.

I will be interested in hearing other peoples thoughts on the matter:).

ER.

[CrazyEddie]

Hi,

Obviously I could just say "don't destroy the factory until later", although this would be really lame.

I agree that when following the common idiom of destroying things in the reverse order of creation, this is an issue.

We already are passing an object (or a reference to one) and not a function pointer when adding the window factory. The issue is that we take a reference to the object that remains owned by the client code. I tend to agree that having the factory manager copy the object and maintain its own copy would be better (even within CEGUI as it would save having a bunch of static objects around) - the reason this is not done is one of object slicing.

This leaves a couple of options; require a 'clone' member for the factory objects, or use some kind of reference counting / smart pointer.

Opinions?

CE

[earthsruler]

Object slicing would only happen if it was being passed into the factory by value, which could be avoided if the object was created and then a pointer passed to the factory and deletion responsibility given to the factory manager. However this defies the delete-in-the-same-scope-you-create rule. A better way may be to have a templated add function that uses the template parameter to create the new factory object its self on the CEGUI side?

I would generally prefer to use a simple solution like the above as opposed to a smart pointer implementation for two reasons:

1) Adding a library like boost with a proven smart pointer implementation is a costly and verbose dependency which could result in a home grown CEGUI version of smart pointer.

2) Home grown smart pointer classes are difficult to get right and are usually full of subtle bugs that are black holes for debugging time.

So yeah, the template function?

ER.

[CrazyEddie]

So yeah, the template function?

Yes, I like this solution; it's clean and simple. Due to the nature of the change, removing the old function and replacing it with the template one makes it a trunk / 0.7.0 thing rather than a 0.6.x thing - although I will investigate the possibility of having both options for 0.6.2 - with the current function marked as deprecated - if it can be done in such a way that does not break binary compatibility (I think it should not be an issue).

Thanks again

CE.

[CrazyEddie]

I have added a new template addFactory function to WindowFactoryManager - this is in branches/v0-6 as of rev 1854. We're not using this internally at the moment, though will be in trunk once the change is ported over (possibly with some other mods too).

Basically, now when creating your factory, you still declare the factory class as normal - perhaps with our macro:

Code: Select all

// still do this
CEGUI_DECLARE_WINDOW_FACTORY(CheckListboxItem)

However, you no longer call the define macro, or use the helper 'get' macro:

Code: Select all

// don't do this anymore
CEGUI_DEFINE_WINDOW_FACTORY(CheckListboxItem)

CEGUI::WindowFactoryManager& wfMgr = CEGUI::WindowFactoryManager::getSingleton();
wfMgr.addFactory(&CEGUI_WINDOW_FACTORY(CheckListboxItem));

Instead, you just do this:

Code: Select all

// do this instead
CEGUI::WindowFactoryManager& mfMgr = CEGUI::WindowFactoryManager::getSingleton();
wfMgr.addFactory<CheckListboxItemFactory>();

HTH

CE.

[earthsruler]

Thank you so much! This is much better and let me get rid of my filthy preinitialise function hacks.

Just to nit pick ( sorry, im good at that ) you cant add to the factory before the system has been initialised. This introduces a limitation. You can pass a config file to the system to use during initialisation, and that config file can run any lua script. Therefore that script could try to use the custom widget during system initialisation before the window factory has been created.

This is easy to work around, but it is a limitation none the less

And I don't mean to always seem like im poking holes in CEGUI - I really do like the it .

ER.

[CrazyEddie]

And I don't mean to always seem like im poking holes in CEGUI - I really do like the it .

No problemo. Your suggestions are usually backed by a sound reasoning of some kind or other, which makes it easy to see the value in what you're saying.

I'm considering making a couple of changes that will allow pre-registration of factories. The factory objects would be created by CEGUI and held in a list until the WindowFactoryManager singleton is created at which time they will automatically be added to the system for use. This, I believe, would pretty much cover all the bases on this one

CE.

[CrazyEddie]

Ok. branches/v0-6 rev 1861 has made this new member static, so you can now do:

Code: Select all

CEGUI::WindowFactoryManager::addFactory<CheckListboxItemFactory>();

prior to creating the system. The system will complete the set up of the pre-added factories when it creates the WindowFactoryManager singleton - so those will then be available to use in the init script

HTH

CE.

[earthsruler]

Sweet! I will be building CEGUI and adding this into my version today . Thanks again!.

ER.