Design for a new module system

[OrbWeaver]

Sounds like good progress nevertheless. I can use GDB to investigate any hard segfaults if necessary.

[greebo]

Thanks, I'm developing under Ubuntu 7.10 the last few days anyway. Otherwise it would've taken me much longer, compiling under Linux is so much faster.

[greebo]

We're getting closer: most modules are functional now, Brushes are showing up again, no sudden crashes at startup, shortcuts are working, and so on.

 

What's missing: Models are not loaded yet (I guess this is due to a wrong module lookup, I just threw a quick mockup in there to make it compile), and the S/R Editor is not showing for some reason. There's probably more, but nothing showstopping so far.

[OrbWeaver]

I wonder if the S/R editor problem has anything to do with the PersistentTransientWindow changes -- I keep remembering that I forgot to test that originally, so if you haven't used it since then it may not in fact be related to the module refactor.

[greebo]

I haven't used it since weeks, I must admit, so it may be possible. Can't be sure at this point of course.

 

Models are showing up again now, I registered the filetypes accidentally using uppercase, the FileType registry couldn't find the modules.

[greebo]

Apart from the S/R Editor not showing up the other major problem is the globalOutputStream(), which only works for the main binary at the moment.

 

OrbWeaver, do you have a design suggestion for this? The old system initialised the reference to the singleton stream instance in the extern Radiant_RegisterModule() routine: the old ModuleServer had a method getOutputStream() and getErrorStream(), which was stored into the globalOutputStream holder.

 

Do you have a better idea than to do it this way? Apart from that it would also be nice to have a redirectable stream for debugging purposes, like "redirect all globalOutputStream() to std::cout").

[OrbWeaver]

Ideally I would like to get rid of those streams altogether; it would be preferable to just use std::cout (after all, that is it's purpose) but possibly redirect it so that it also writes to the Radiant console.

 

I think this is related to the module::getRegistry() problem which I noticed in the commit log, I suspected there would be problems due to the incorrect exporting of symbols to DLLs. I think this should be solvable using appropriate linker flags however, e.g. -Wl,--export-dynamic (although I thought that was already set).

 

If necessary though, we do have a mechanism for the main binary to pass important objects to modules, in the form of the ApplicationContext. This could perhaps hold the streams, which in future should be std::ostream not TextOutputStream or any other homegrown rubbish.

[greebo]

Yes, the windows DLLs don't seem to have access to all the core binary symbols, they must be passed along to and stored within the module. I can't remember in detail, but I think export-dynamic is already set, IIRC that solved our problems with the class types when using dynamic_cast.

 

I think the idea of having two streams (output and error) isn't all that bad, because it makes catching errors in the radiant console output very easy for the user. Also, I like the idea of streaming both the console and to the radiant.log file at the same time, as log files makes our support life potentially easier.

 

The ApplicationContext can be indeed be useful in this case, I think that's the right way to pass it to the module. I'll implement it this way.

[OrbWeaver]

Yes, the windows DLLs don't seem to have access to all the core binary symbols, they must be passed along to and stored within the module. I can't remember in detail, but I think export-dynamic is already set, IIRC that solved our problems with the class types when using dynamic_cast.

 

There must be a way of doing this though -- perhaps it needs to DARKRADIANT_DLLEXPORT macro or whatever it is? Windows would be pretty crap (!) if it doesn't allow some means of exporting symbols from an EXE to a loaded DLL.

 

I think the idea of having two streams (output and error) isn't all that bad, because it makes catching errors in the radiant console output very easy for the user. Also, I like the idea of streaming both the console and to the radiant.log file at the same time, as log files makes our support life potentially easier.

 

I agree with having two streams, and using them for simultaneous logging in this way. What I don't like is the use of Radiant stream objects when these should just be std::ostreams. This is obviously not a problem for the modulesystem though, but some future refactoring work.

 

The ApplicationContext can be indeed be useful in this case, I think that's the right way to pass it to the module. I'll implement it this way.

 

Sounds like the best option in this particular situation.

[greebo]

I'll do some research and implement a solution to get the streams back working at the same time.

 

Btw: I just noticed that scons can do multiple compile multiple jobs simultaneously, which means that I can use both processor cores at the same time! I wish I would have watched out for this earlier.

[OrbWeaver]

Btw: I just noticed that scons can do multiple compile multiple jobs simultaneously, which means that I can use both processor cores at the same time! I wish I would have watched out for this earlier.

 

Yes, I use scons -j2 on my Pentium 4; I don't know whether HyperThreading makes much difference but it's worth a try. With two cores you might try -j4.

[OrbWeaver]

Here is some potentially-useful information about exporting symbols.

 

http://www.redhat.com/docs/manuals/enterpr...nker/win32.html

 

If, however, -export-all-symbols is not given explicitly on the command line, then the default auto-export behavior will be disabled if either of the following are true:

* A DEF file is used.

* Any symbol in any object file was marked with the __declspec(dllexport) attribute.

 

So it looks like using the __declspec(dllexport) idiom may be worth trying, however note the following:

 

This complicates the structure of library header files, because when included by the library itself the header must declare the variables and functions as dllexport, but when included by client code the header must declare them as dllimport. There are a number of idioms that are typically used to do this; often client code can omit the __declspec() declaration completely. See -enable-auto-import and automatic data imports for more imformation.

 

This would certainly apply in our situation, since the imodule.h header is included by both DLL and EXE.

[greebo]

Yes, I already stumbled upon exactly that site (search terms: win32 exporting symbols DLL). I tried it, but I couldn't get it to work yet. From what I understand of it so far, the __declspec(dllimport) seems to be used for accessing DLL functions from within the main binary and the __declspec(dllexport) function is to be used to mark DLL functions for export to the main binary.

 

What we need though, is to access functions of the main binary from within the DLL, which works fine in POSIX, but not in Win32 for some reason. This needs more reading, I guess.

 

Before that, I had a go at -Wl,-export-all-symbols, but this was to no avail.

[sparhawk]

I don't see how you can access symbols from the main exe fom within a DLL, as they are unrelated and the direction is usually from the DLL to the exe and not the other way around.

What you can do is, to reserve some pointers in the DLL, then when the DLL is loaded, you call an initialisation routine, which fills them. This is also how Id did it with the game dll in Doom 3.

I don't think that there is another way, because an exe is not expected to export any symbols. Where should it export them to anyway?

[greebo]

I don't see how you can access symbols from the main exe fom within a DLL, as they are unrelated and the direction is usually from the DLL to the exe and not the other way around.

What you can do is, to reserve some pointers in the DLL, then when the DLL is loaded, you call an initialisation routine, which fills them. This is also how Id did it with the game dll in Doom 3.

I ended up doing exactly this, as was the deprecated code. Curiously, it works fine in Linux without initialising any pointers or references (I can access the symbol module::getRegistry() from a dynmic library, the linker doesn't even raise an eyebrow).

[sparhawk]

Is this static? If so it might be alerady initialized by the loader when the SO is initialized. Can't say though, without seeing the code.

[greebo]

You mean the symbol? Yes, it's a static function in the module:: namespace. Obviously it's handled differently in POSIX systems.

 

@OrbWeaver: I run into a wall while trying to fix the GTK warning on shutdown. I can reproduce them by 1) Starting DarkRadiant, 2) Create a light, 3) Open S/R, 4) Close the dialog and Exit DarkRadiant.

 

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 presume that the GTK warnings relate to the PersistentTransientWindow::_preDestroy method not being called, although I have no clue why this happens. Any ideas? The version in SVN is my current version, just set a breakpoint in ~StimResponseEditor and step through to see what happens.

 

edit: I could "fix" it by explicitly calling destroy() in the StimResponseEditor::shutdown() method, which works (the overridden virtual methods get invoked correctly). Obviously the situation at destruction time (~StimResponseEditor) is a different one than in shutdown(). Whatever, I'll probably understand it someday what the heck was going on here.

[OrbWeaver]

I don't see how you can access symbols from the main exe fom within a DLL, as they are unrelated and the direction is usually from the DLL to the exe and not the other way around.

 

An EXE and a DLL are more or less the same though, except that an EXE has a main() function and possibly some other metadata. You can even call functions directly in a DLL using rundll32. I don't see any reason why symbols could not be exported to DLLs under Windows, since it works in Linux, but I guess Windows is different and maybe there is some architectural reason why this is impossible.

 

Greebo: have you tried using the nm utility (part of MinGW) to view the exported symbols from the darkradiant.exe binary? You would probably use something like nm -DC darkradiant.exe to view all of the dynamic symbols.

 

I presume that the GTK warnings relate to the PersistentTransientWindow::_preDestroy method not being called, although I have no clue why this happens. Any ideas? The version in SVN is my current version, just set a breakpoint in ~StimResponseEditor and step through to see what happens.

 

I don't know what is happening with the virtuals off the top of my head, but in any case the PersistentTransientWindow destructor should not be calling destroy, because this should happen first. This is one of the fixups I needed to make for all of the other windows, which I obviously forgot for the S/R editor.

 

I guess this is what we need the RadiantListener event interface for, so that the S/R dialog can register an interest in knowing when the application is shut down. For the time being the other dialogs are shut down manually by the main code, but this can obviously not happen for plugins.

[sparhawk]

An EXE and a DLL are more or less the same though, except that an EXE has a main() function and possibly some other metadata. You can even call functions directly in a DLL using rundll32. I don't see any reason why symbols could not be exported to DLLs under Windows, since it works in Linux, but I guess Windows is different and maybe there is some architectural reason why this is impossible.

 

Don't know if I ever tried this, but I can make a test app and see if that works but I doubt it. It doesn't make sense IMO, because a DLL is under the EXE if you view it hierarchically. Don't know if it works in Linux, I guess if you tried it is true.

[greebo]

I guess this is what we need the RadiantListener event interface for, so that the S/R dialog can register an interest in knowing when the application is shut down. For the time being the other dialogs are shut down manually by the main code, but this can obviously not happen for plugins.

Yes, this is the root of the problem. The S/R module shutdown happens after the MainFrame has been deleted and the reference to the main window is no longer valid, but referred to by the PersistentWindow _preDestroy() methods.

 

I'll implement the RadiantListener interface now. What's your suggestion about it? Something like this?

class IRadiant {
public:
  class EventListener {
  public:
  virtual void onRadiantShutdown() {};
  virtual void onRadiantStartup() {};
  };
  typedef boost::shared_ptr<EventListener> EventListenerPtr;

  virtual void addListener(EventListenerPtr listener) = 0;
  virtual void removeListener(EventListenerPtr listener) = 0;
};

Or should the interface of EventListener be more flexible, like:

class EventListener {
  public:
  virtual void onRadiantEvent(const std::string& namedEvent) = 0;
  };

[OrbWeaver]

The first one is almost exactly what I was thinking, although instead of IRadiant::EventListener I would just call it RadiantEventListener for simplicity's sake, and I think the methods should be pure virtual rather than default empty. This is modelled on the Java Swing libraries, which have interfaces like MouseEventListener which defines methods like mouseEntered(MouseEvent e) and mouseMoved(MouseEvent e) (except in our case we don't need the parameter, since a shutdown just means a shutdown).

[greebo]

Do we really want to have the events pure virtual? I can imagine that the interface might grow over time and not every EventListener wants to listen for all events?

[OrbWeaver]

I guess that is reasonable, of course in Java an interface cannot provide default implementations so perhaps this is just a restriction rather than a design decision.

[greebo]

Ok, the assertions are sorted out now, I was barking up the wrong tree. I had a g_object_unref call in the ClassEditor, which was called twice on the same GtkListStore. It's still not perfect, because it has to be called once, but I haven't implemented that for now, just commented out the line. I spent over an hour searching for that crap...

 

The RadiantEventListener interface is defined and implemented by the S/R Editor. We're almost there, the only thing that's missing is the proper implementation of the ApplicationContext class.

[greebo]

We're touching the ground again, I just removed the old Environment class and moved the functionality to the ApplicationContextImpl. The only thing that's missing is to refactor the inspector classes to use the RadiantEventListener interface, but that's a different story.

 

@OrbWeaver: can you give it a go and test it briefly on your machine if you find anything broken? I'll switch back to Windows now to see if I got that OS-specific part of the ApplicationContext class right.