Jump to content
The Dark Mod Forums

Design for a new module system


Recommended Posts

  • Replies 93
  • Created
  • Last Reply

Top Posters In This Topic

I've found one thing yesterday: not all shortcuts in the menus are displayed, only a part of them. The reason is that the ObjectivesEditor and S/R Editor plugins are registering their menu items at startup, which triggers a partial GTK menu construction, before all the shortcuts are loaded.

 

I'll have to redesign this part a bit to sort this out. Either I load the shortcuts into a map which allows lookup or I defer the GTK menu construction somehow. I can't remember the exact reasons, but I think I had to construct the parent menu before I could insert new menu items, hence the immediate GTK widget creation.

 

Once that's done, we can merge back the changes into the trunk.

Link to post
Share on other sites

Fixed. I added an updateAccelerators() routine to the MenuManager, which lets the menu items update their labels according to the information found in the EventManager.

 

I also added such a call at the end of the CommandList dialog, so that the menu items are always up to date even when the user re-assigns them at runtime.

 

Are we ready for merging the changes back into the trunk or do you want to further test it beforehand?

Link to post
Share on other sites
Are we ready for merging the changes back into the trunk or do you want to further test it beforehand?

 

As long as things are reasonably stable, merging is fine. The idea behind branches is to prevent changes from getting into the trunk which totally break the application or prevent it from compiling, but that part is over for these changes I think.

Link to post
Share on other sites

I think it's stable enough - it's possible that one or the other thing behaves unexpectedly, but nothing showstopping. I will have to go through the clean startup phase once more to check whether first-time installations are working as they should.

 

I'll start merging the changes back in. :)

 

edit: done.

Link to post
Share on other sites

There is some information here:

 

http://www.williamwilling.com/blog/?p=74

 

about how to redirect std::cout and std::cerr to a different location. Therefore, to use std::cout instead of GlobalOutputStream() it would be necessary to

 

1. Convert TextOutputStream into a subclass of std::streambuf, just like TextInputStream currently is.

2. Have the startup code set up TextOutputStreams for both the output and error stream, and set them as the streambufs for both std::cout and std::cerr.

3. Determine whether there were any problems using std::cout and std::cerr from the DLLs, i.e. are these actually the same stream objects, or would they get created anew with the default console output?

 

I'll have to look into the DLL symbol export issues on Windows, hopefully with the correct combination of DLL export declarations it should be possible to share global functions in this way. EDIT: Never mind, I forgot what a dungheap Windows is. A minute and a half of faffing around just to start DarkRadiant, never mind compile and debug (the latter of which doesn't even work). We can just stick with your wrapper class arrangement for now, and perhaps your experiments with MSVC might cast some light on the symbol export issues.

Link to post
Share on other sites
What I have is this: The S/R Editor is deriving from gtkutil::PersistentTransientWindow now. On destruction (which happens after the main() routine has ended), the destructor ~TransientWindow is called. This destructor checks whether the _window widget is existing, and invokes a destroy() call. Now comes the interesting part: The destroy call invokes the virtual void _preDestroy() method, which is defined on both TransientWindow and PersistentTransientWindow. However, only the TransientWindow::_preDestroy() is called and the overriding one of PersTransWindow is not called, although it's been declared virtual by TransientWindow.

I know now what the problem here is (after reading another chapter in Effective C++). The PersistentWindow::_preDestroy() method can not be called after the ~PersistentTransientWindow destructor has run. The virtual table and the RTTI is reset during the destruction of all the derived classes, and at the time the ~TransientWindow destructor is reached, the class acts like it has no subclasses (as they are already destroyed) and therefore all the overridden virtual methods just don't exist. It's logical of course, but I wouldn't have thought of that.

 

There is an entire chapter dedicated to this problem (called Don't call virtual methods in constructors/destructors), and the example there almost perfectly describes what we have here.

 

I suggest as a workaround of this problem to define a ~PersistentTransientWindow, which calls the base class TransientWindow::destroy() - at this time the subclass PersistentTransientWindow is still existing, and the overridden methods still exist.

Link to post
Share on other sites
I suggest as a workaround of this problem to define a ~PersistentTransientWindow, which calls the base class TransientWindow::destroy() - at this time the subclass PersistentTransientWindow is still existing, and the overridden methods still exist.

 

That's exactly how it used to work, while I was engaged in the refactor I added this for debugging purposes but then removed it when the changes were finished.

 

I didn't realise there was a similar call in the TransientWindow base class; I must have added this without really thinking about it (I do remember reading about the virtual methods in destructors problem now you mention it).

Link to post
Share on other sites

The crash on shutdown in Windows is now gone (it took me over twelve hours to nail that one down!). There was a problem with the Overlay class, the GroupDialog and I think (not sure) with the TransientWindow hierarchy. It definitely sucks not to have any debugger in Windows, especially if the crash occurs after the main() routine has ended.

Link to post
Share on other sites

OrbWeaver: do we want to make the Inspectors separate modules? I was thinking that, in an ideal system, the SurfaceInspector could be exchanged by a newer version without breaking anything? Now that the RadiantEventListener interface is roughly in place (and can easily be extended), the migration to a stand-alone (static) module is not that hard to perform, I reckon.

Link to post
Share on other sites

I don't see much benefit in static modules except as an intermediate migration step, but they could certainly be made into DLL-based plugins provided that there was a way for them to access the necessary data, such as the currently-selected texture (which didn't involve adding nasty ad-hoc functions to the GlobalRadiant interface).

 

Plugin DLLs would have the advantage that the feature could be removed just by deleting the DLL, either because it was buggy or because the user wanted a "streamlined" release, or perhaps because a different version of the tool would be needed for another game.

 

On a related note, does using MSVC provide any insight into the problem with symbols not being exported from the binary to DLLs? I think the RegistryReference thing is a bit of hack (which I don't understand particularly well), and in an ideal world would not be necessary but may be unavoidable. Does the MSVC debugger work successfully?

Link to post
Share on other sites

Yes, the debugger works quite ok. It always complains about memory leaks after shutdown though. (There are some compiler flags which allow to locate the place where the leaked resources were allocated, although I couldn't get it to compile, it throws errors somewhere in the VC standard headers.) Sometimes it's crashing on strange things, and of course, I had to disable the graph_tree_model crapness to get DarkRadiant running.

 

The RegistryReference is just a wrapped pointer, which gets initialised once for each DLL. Hm, thinking about that, we can probably replace the whole thing by a

static IModuleRegistry* RegistryReference;

:)

 

I guess I first wanted to store an actual IModuleRegistry& reference and intended to initialise the reference in the constructor, but I threw away that idea.

 

I already played around a bit with the __declspec(export) stuff, but couldn't get it to work. Sparhawk mentioned that this is not possible to import symbols from the main binary into the DLL, because the DLLs are designed to be slaves controlled by the main EXE (although I can't make so much sense out of it).

Link to post
Share on other sites
On a related note, does using MSVC provide any insight into the problem with symbols not being exported from the binary to DLLs? I think the RegistryReference thing is a bit of hack (which I don't understand particularly well), and in an ideal world would not be necessary but may be unavoidable.

I found this page here regarding our problem: http://www.codeguru.com/cpp/w-p/dll/article.php/c3649/

 

Basically, it says we should export a function from the EXE similar to our RegisterModule function in the DLL and have the DLL lookup the symbol with FindProcAdress. This is not very elegant in our case, because the GetProcAddress is Win32-specific. We would again need a bunch of IFDEFs and possible more code than we do now.

 

I'll look around some more, but I'm not too confident that there is an easier solution than our current one.

Link to post
Share on other sites

What exactly do you try to do? The most elegant and portable solution would be to write a class, which has the pointers, and the exe fills them. This is how Id did it as well in their SDK, and I don't see a much better way to do this.

Gerhard

Link to post
Share on other sites

We have the EXE which provides a method:

IModuleRegistry& getRegistry();

The function body resides in the main binary (the exe), but the DLLs want to access it too. This works fine under Linux (all the main binary symbols are visible to the .so files), but the Windows linker throws up on this.

 

I already wrote such a static class holding a pointer which gets filled in the DLL initialisation routine, but we're hoping to getting it to work like in Linux (without the class).

Link to post
Share on other sites
The RegistryReference is just a wrapped pointer, which gets initialised once for each DLL. Hm, thinking about that, we can probably replace the whole thing by a

static IModuleRegistry* RegistryReference;

I just tried to replace the pointer holder with this, but then it occurred to me that this won't work as the static pointer will get replicated along translation units. This won't work, so I'll leave the "singleton" RegistryReference class instance.

Link to post
Share on other sites
  • 1 month later...

I performed a quick profile using the Global*() accessors, like GlobalSceneGraph(). I accessed that method 1 million times and measured the time it took.

 

The current implementation of GlobalSceneGraph() is this:

inline scene::Graph& GlobalSceneGraph() {
boost::shared_ptr<scene::Graph> _sceneGraph(
	boost::static_pointer_cast<scene::Graph>(
		module::GlobalModuleRegistry().getModule(MODULE_SCENEGRAPH)
	)
);
return *_sceneGraph;
}

Calling this 1,000,000 times equals to 0.54 seconds on my system.

 

Changing the contained pointer to static, like this:

inline scene::Graph& GlobalSceneGraph() {
static boost::shared_ptr<scene::Graph> _sceneGraph(
	boost::static_pointer_cast<scene::Graph>(
		module::GlobalModuleRegistry().getModule(MODULE_SCENEGRAPH)
	)
);
return *_sceneGraph;
}

Drops the time down to 0.01 seconds (sometimes 0.00, i.e. below display accuracy).

 

So I'd vote for changing the pointer to static, even if the static pointer is replicated to each module. It's presumably better to have 23 static shared_ptr's instead of doing a lookup in a stringmap each time.

Link to post
Share on other sites
So I'd vote for changing the pointer to static, even if the static pointer is replicated to each module. It's presumably better to have 23 static shared_ptr's instead of doing a lookup in a stringmap each time.

 

As long as it works and doesn't cause issues elsewhere, this should be no problem (although in reality any code which calls the GlobalBlah() accessor function a million times should really be rewritten to call the accessor once and cache the result).

Link to post
Share on other sites

Ok, scratch that, it causes issues. Holding shared_ptrs to modules is not a good idea considering the fact that many modules are contained in DLLs. Storing shared_ptrs to these classes until binary shutdown leads right into crashes, as the DLLs are unloaded earlier than the radiant binary, leading to destruction issues.

 

I've messed with that issue before, I should've thought of that earlier.

Link to post
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.


×
×
  • Create New...