Jump to content
The Dark Mod Forums

Design for a new module system


Recommended Posts

After giving up for a second time attempting to merge RadiantCoreAPI and Radiant together, running into bizarre errors with the totally unrelated PreferenceSystem throwing the dreaded "Module used before being initialised" error, I am wondering if the time has come to look into a new module system which isn't based on incomprehensible templated crap and ridiculous spaghetti code.

 

In my mind, the module system does not need to be that complex -- all it is doing is holding a list of singleton objects (which implement different interfaces) and allow them to be retrieved on demand. I therefore propose the following first draft design:

 

Registering and initialising modules

 

• All modules are registered with a module registry which is owned and operated by the Radiant core (NOT the GlobalRadiant module, since this requires some other modules to be created first e.g. XMLRegistry).

• The module registry defines a RegisterableModule interface which is implemented by all module instances. This interface defines a method getDependencies() which returns a set of named modules that this module relies on, along with a simple getName() method.

• The module registry maintains an std::map<std::string, RegisterableModulePtr> which contains all of the registered modules, indexed by name (NOT by templated type).

• RegisterableModule subclasses must NOT perform initialisation that requires access to other modules in their constructors, but in a separate void initialiseModule() method which is invoked at the appropriate time by the module registry. The contract for this method is that all named modules returned by getDependencies() can be assumed to exist when this method is called, and that the other modules will be available through their GlobalBlah() methods.

• On startup, the Radiant core finds all of the shared libraries and loads them. It passes a reference to the module registry object to the Radiant_RegisterModules() function in the DLL. Using double-dispatch, each DLL registers each of its static contained modules using a public method defined on the module registry (e.g. void ModuleRegistry::registerModule(Registera

bleModulePtr module)). At this point the static module instances will have been constructed (obviously, since the RegisterableModulePtr exists), but not initialised.

• The module name must be unique. If two modules attempt to register themselves with the same name, an exception is thrown.

• During the registration phase, the module registry adds each named RegistrableModulePtr to its internal map. At the end of registration, this map will be full of named modules in no particular order, none of which is initialised.

• We now need to determine the order in which modules must be initialised, taking account of their dependencies. We do this by initially marking all modules as uninitialised (using some internal data structure), then for each uninitialised module, recursively initialise its dependent modules (if there are any, and they are not already initialised) until everything is initialised. A cycle check will be needed here to avoid infinite recursion if a module indirectly depends on itself.

• Module initialisation is complete.

 

Accessing modules

 

• The module registry provides a public method to retrieve a named module: RegisterableModulePtr getModule(const std::string& name). HOWEVER: returning a RegisterableModulePtr to the application is useless, since what we actually want is the interface of the module itself. Therefore, the current GlobalBlah() functions will be redefined to downcast the RegisterableModule into its known type, e.g.:

 

XMLRegistryPtr GlobalRegistry() {
return static_pointer_cast<XMLRegistry> GlobalModuleRegistry().getModule("xmlRegistry");
}

 

• The main Radiant binary will need to export a global function to retrieve the singleton module registry instance. This cannot be part of GlobalRadiant() since this itself is a module.

• Everything to do with GlobalModules, GlobalModuleRefs, SingletonModules, SingletonModuleRefs, StaticRegisterModule, APIConstructors, StaticGlobalSingletonRegistryModuleRef and other bullshit is terminated with extreme prejudice.

 

Shutdown

 

• Reference counting is not needed, and modules are not "unloaded" on shutdown. If a module wants to save persistent data or perform any cleanup, it registers with GlobalRadiant to be notified of application shutdown (using a new interface which is written as part of this design).

 

Any comments? Any important piece of functionality provided by the templated system that I have missed in this design? The only thing I can think of is that there may be some part where multiple modules of a single type are needed, but I'm not sure if we are actually using any such modules (plugins all implement the same interface, but as long as their names are unique this should be OK).

Link to post
Share on other sites
  • Replies 93
  • Created
  • Last Reply

Top Posters In This Topic

I think you've covered that nicely. Currently modules have version tags/strings - will there be any need for this? I could imagine that a version stamp on built DLLs will prevent the Radiant binary from crashing when outdated DLLs remain in the modules/ folder (like we experienced with the Clipper module that I merged back into the core). Maybe we could prevent DarkRadiant from loading incompatible DLLs.

 

Are you going to implement this soon? I would be eager to implement this new system, but I guess so are you. :)

Link to post
Share on other sites
I think you've covered that nicely. Currently modules have version tags/strings - will there be any need for this? I could imagine that a version stamp on built DLLs will prevent the Radiant binary from crashing when outdated DLLs remain in the modules/ folder (like we experienced with the Clipper module that I merged back into the core). Maybe we could prevent DarkRadiant from loading incompatible DLLs.

 

OK, we can have an int getVersion() const method on the RegisterableModule interface, which triggers a logic_error if it does not match the version of the main Radiant binary (which obviously we must remember to increment between versions).

 

Are you going to implement this soon? I would be eager to implement this new system, but I guess so are you. :)

 

Heh, that's something a designer doesn't hear every day: "Please can I do all the boring legwork coding and debugging this major rewrite of large parts of the codebase?" :laugh:

 

I'm quite happy for you to do it, given that you'll get it done in a fraction of the time I would, but if you would rather concentrate on the AI then I'm fine with doing it myself (at a more moderate pace).

Link to post
Share on other sites

I don't know why it's fun, but somehow it is. Perhaps because working with the existing GtkRadiant codebase is much more exhausting than writing something afresh (like this one article said: reading code is much harder than writing code).

 

And I love seeing things form and getting together. Maybe I haven't coded enough to be bored by it, I'm not a professional programmer after all. :)

 

Anyway, I'll probably look into both the AI and DarkRadiant, as I want to keep in touch with both, but don't wait for me when you want to start working on it.

Link to post
Share on other sites

I have a question regarding the new ModuleRegistry interface:

/**
* Initialise all of the modules previously registered with
* registerModule() in the order required by their dependencies. This method
* is invoked once, at application startup, with any subsequent attempts
* to invoke this method throwing a logic_error.
*/
virtual void initialiseModules() = 0;

This might have considerable influence on the way things are initialised at startup.

 

In the current codebase, the XMLRegistry is loaded before all the other modules, because it must be populated with the content of all the XML files (doom3.game, user.xml, etc.). Other modules rely on the contents of the game/* XML tree, so they don't just expect the XMLRegistry to be active, but they also expect it to be filled with the right content.

 

What can we do to resolve this? Should we initialise the XMLRegistry first (maybe via an initialiseModule(const std::string& moduleName) method, pump the XML content into it and then load the other modules (like the current code does) or should we find a way to have the XMLRegistry load its contents on its own (by the time its initialised by the ModuleRegistry)?

 

The latter option requires the Environment class (which stores the directory names) to be active, which we could define as a module as well. But this would result in a cycle dependency, as the ModuleLoader in turn relies on the Environment class.

 

Erm, I think I'm confusing myself, I must think more clearly about that. :) Anyway, my message is that we must consider the DarkRadiant startup phase in more detail, as this must be handled very delicately. Maybe a redesign of the startup phase is also necessary here.

Link to post
Share on other sites

I'm not sure I understand the problem. Anything that requires the XMLRegistry will list it as a dependency, and the ModuleRegistry will ensure (via its dependency resolution) that the XMLRegistry is therefore initialised before any modules that require it. The XML content can be read and parsed in the initialiseModule() of the XMLRegistry.

 

Do you mean that there are parts of the code which require the XMLRegistry but are not part of a module, and therefore would not participate in the dependency resolution?

 

I agree that the startup phase probably needs redesigning, and this may well form part of the modulesystem refactor.

Link to post
Share on other sites

If the XMLRegistry should load the user.xml/doom3.game files in its initialiseModule() routine (which I would prefer), it must know things like the AppPath(), SettingsPath() and such. These paths are setup in the Environment class instance, which is initialised right at the start of the main() routine (it contains platform-specific code). The Registry can't access it directly in the current codebase, so I guess it boils down to moving the Environment instance into the XMLRegistry module itself. The paths are loaded into the XMLRegistry at startup anyway (I already set this up during the first startup redesign), so this could resolve a few things.

 

Still, a small problem remains, as the ModuleLoader itself also needs to know the AppPath(), so it needs to access the Environment class as well. It can't load the XMLRegistry when it doesn't know the AppPath(). I'll have to look how the AppPath is determined. If it's easy to do, I'll just move a method to determine the AppPath() into the libs.

Link to post
Share on other sites

Can the AppPath and other information be provided to the XMLRegistry in its constructor (now that the module's construction will be separate from its initialisation)? This way the main Radiant binary would instantiate the XMLRegistry module with the necessary initialisation paths, and pass it to the ModuleRegistry. When the registry initialised the XMLRegistry module, it would use its stored paths to load the XML data.

 

When you refer to ModuleLoader are you referring to the ModuleRegistry or something else? The ModuleRegistry does not need the app paths because it will be passed RegisterableModulePtrs directly by the main startup code, but of course the startup code itself does need to retain the app path so it can look for DLLs.

Link to post
Share on other sites
Can the AppPath and other information be provided to the XMLRegistry in its constructor (now that the module's construction will be separate from its initialisation)? This way the main Radiant binary would instantiate the XMLRegistry module with the necessary initialisation paths, and pass it to the ModuleRegistry. When the registry initialised the XMLRegistry module, it would use its stored paths to load the XML data.

The XMLRegistry needs to know the AppPath, so yes, it either must be passed to the constructor or to a separate function, but both require the Environment information to be accessible from within the xmlregistry module. Once the AppPath and SettingsPath is known, the XMLRegistry is safe and can be used by other modules.

 

When you refer to ModuleLoader are you referring to the ModuleRegistry or something else? The ModuleRegistry does not need the app paths because it will be passed RegisterableModulePtrs directly by the main startup code, but of course the startup code itself does need to retain the app path so it can look for DLLs.

I was referring to the ModuleLoader (as it is named in the current codebase), which finds and loads the external module (.dll, .so) files. The ModuleLoader will have to find the "register" symbol on the loaded DLL and invoke it, passing the ModuleRegistry& reference to it, as it does now.

 

The ModuleLoader itself needs to know the AppPath: this is the current code for finding the DLLs:

// Load the Radiant modules from the modules/ and plugins/ dir.
ModuleLoader::loadModules(Environment::Instance().getAppPath());

The Environment instance will probably be moved into the XMLRegistry, as the folder info is needed by the XMLRegistry constructor.

 

Thinking about that, can we move the Environment class into a static library (like gtkutil)? It will have to be initialised for both the Radiant binary and the XMLRegistry if I recall correctly, but at least we can use the code from both the binary and the XMLRegistry module.

Link to post
Share on other sites
The XMLRegistry needs to know the AppPath, so yes, it either must be passed to the constructor or to a separate function, but both require the Environment information to be accessible from within the xmlregistry module. Once the AppPath and SettingsPath is known, the XMLRegistry is safe and can be used by other modules.

 

So the XMLRegistry could be initialised by the startup code with both the AppPath and SettingsPath passed as construction parameters?

 

I was referring to the ModuleLoader (as it is named in the current codebase), which finds and loads the external module (.dll, .so) files. The ModuleLoader will have to find the "register" symbol on the loaded DLL and invoke it, passing the ModuleRegistry& reference to it, as it does now.

 

The ModuleLoader itself needs to know the AppPath: this is the current code for finding the DLLs:

// Load the Radiant modules from the modules/ and plugins/ dir.
ModuleLoader::loadModules(Environment::Instance().getAppPath());

 

This ModuleLoader could be invoked straight after constructing hte XMLRegistry (since it is also constructing modules), meaning that the paths would not need to be stored at all. E.g.

 

// Get paths
std::string appPath = getAppPathSomehow();
std::string settingsPath = getSettingsPathSomehow();

// Construct XMLRegistry first
RegisterableModulePtr xmlReg(new XMLRegistry(appPath, settingsPath));
modules::getRegistry().registerModule(xmlReg);

// Load the other DLLs
ModuleLoader::loadModules(appPath);

 

The Environment instance will probably be moved into the XMLRegistry, as the folder info is needed by the XMLRegistry constructor. Thinking about that, can we move the Environment class into a static library (like gtkutil)? It will have to be initialised for both the Radiant binary and the XMLRegistry if I recall correctly, but at least we can use the code from both the binary and the XMLRegistry module.

 

It looks like we can scrap the Environment instance altogether, and just pass the necessary directories to the XMLRegistry as construction parameters. I wouldn't recommend attempting to use a linked library to hold a static instance, there could be linking problems/multiple symbols etc.

Link to post
Share on other sites
So the XMLRegistry could be initialised by the startup code with both the AppPath and SettingsPath passed as construction parameters?

Yes, if the AppPath and SettingsPath are known at that time. (see below.)

 

// Construct XMLRegistry first
RegisterableModulePtr xmlReg(new XMLRegistry(appPath, settingsPath));
modules::getRegistry().registerModule(xmlReg);

// Load the other DLLs
ModuleLoader::loadModules(appPath);

Looking at the code snippet above, I gather that the XMLRegistry code will not be within the plugins/ folder anymore (as it is instantiated by DarkRadiant's core code)? Otherwise the core code wouldn't know how to instantiate an XMLRegistry class, would it?

 

It looks like we can scrap the Environment instance altogether, and just pass the necessary directories to the XMLRegistry as construction parameters.

I don't think we can write it off so quickly. First, we need to define how (and where) these functions work:

std::string appPath = getAppPathSomehow();
std::string settingsPath = getSettingsPathSomehow();

The getAppPathSomehow is exactly what the Environment class is doing at the moment. If we find another method of getting the App path, the Environment class could be kicked indeed.

Link to post
Share on other sites
Looking at the code snippet above, I gather that the XMLRegistry code will not be within the plugins/ folder anymore (as it is instantiated by DarkRadiant's core code)? Otherwise the core code wouldn't know how to instantiate an XMLRegistry class, would it?

 

Excellent point, I was sure there was something I had missed. Clearly the XMLRegistry cannot be manually instantiated if it is in a plugin DLL along with all of the other modules which have not been loaded yet.

 

Since we need to pass the ModuleRegistry instance to all of the DLLs when they are loaded, perhaps we should add a method getApplicationContext() or similar which would return an object providing access to whatever data they might need from the main application (including the app paths etc).

Link to post
Share on other sites
Since we need to pass the ModuleRegistry instance to all of the DLLs when they are loaded, perhaps we should add a method getApplicationContext() or similar which would return an object providing access to whatever data they might need from the main application (including the app paths etc).

Yes, such a getApplicationContext() function sounds reasonable. It's not exactly the competence of the ModuleRegistry to deliver the folder paths, but at least it's accessible from all the modules.

 

I'd still vote for encapsulating the whole folder-determination code (AppPath, SettingsPath) in an Environment or similarly named class, the code contains some ugly IFDEFs which the ModuleRegistry shouldn't be bothered with.

Link to post
Share on other sites
Yes, such a getApplicationContext() function sounds reasonable. It's not exactly the competence of the ModuleRegistry to deliver the folder paths, but at least it's accessible from all the modules.

 

I agree, it's a little out-of-scope, but at least using a Context object makes it clear that is is providing relevant information that all modules might need (and could easily be extended later).

 

We could also pass the ApplicationContext to the Radiant_RegisterModules() function, but I'm not sure how this would work for "static" modules which were part of the main binary; these would appear to have no way of obtaining the context if it is not part of the ModuleRegistry.

 

I'd still vote for encapsulating the whole folder-determination code (AppPath, SettingsPath) in an Environment or similarly named class, the code contains some ugly IFDEFs which the ModuleRegistry shouldn't be bothered with.

 

I was assuming that the determination of paths would be performed by the startup code, and then wrapped in an ApplicationContext object which is passed to the ModuleRegistry as a construction parameter. The registry itself would therefore require nothing more than to hold onto the object and pass it to the modules as required.

 

EDIT: Actually, perhaps the ApplicationContext should be passed as a parameter of initialiseModule() rather than through a separate accessor method -- this would be similar to the way certain web applications get initialised e.g. portlet applications. I guess there is not much in it really either way.

Link to post
Share on other sites

Agreed, let's pass it to initialiseModule(const ApplicationContext& context). The XMLRegistry::initialiseModule() method is invoked anyway before any other modules are accessing it, so the XML contents can be loaded just in time. Only two methods have to be defined on ApplicationContext, namely getAppPath() and getSettingsPath().

 

I'm not sure whether the GameManager poses any problems here, as I can't remember all the call order off the top of my head, but I guess these problems can be resolved with reasonable effort. It's probably worthwile moving the GameManager into a separate module, but let's cross that bridge when time comes.

Link to post
Share on other sites

@OrbWeaver: I just added a few files to the modulesystem branch (an empty class ModuleLoader) when it occurred to me that you might have local changes of this branch as well. Is there anything you have in your local copy that you want me to wait for, before I start to add things?

Link to post
Share on other sites

How to initialise modules

 

This is how I would initialise the modules, taking into account their dependencies.

  • When registering the modules, place them into an std::map<std::string, RegisterableModulePtr> called _uninitialisedModules, indexed by their name returned with getName().
  • Also maintain an std::map<std::string, RegisterableModulePtr> called _initialisedModules, which is initially empty.
  • When the initialiseModules() method is called, the following process occurs:
    • Start iterating over the _uninitialisedModules map.
    • For each module name (i->first), invoke a private method _initialiseModule(const std::string& name) which does the following:
      • If the named module is already in the _initialiseModules map, return immediately (no action required).
      • Otherwise, lookup the named module in _uninitialisedModules and get the set of named dependencies with getDependencies().
      • Iterate over the set of dependencies, recursively invoking _initialiseModule() on each one. If there are no dependencies this step will obviously do nothing.
      • Initialise the named module passed to this call by calling its initialiseModule() public method and placing it into the _initialisedModules map.

As far as I can see this should accomplish the requirement of dependency resolution, avoid infinite recursion (since the recursive _initialiseModule() call will do nothing if the dependency is already satisfied) and does not require any dependency graph information to be constructed first.

Link to post
Share on other sites

I already have a first iteration of such an algorithm in place, I think it works similarly:

  • Incoming modules (by registerModule()) are stored into a ModuleRecord. All ModuleRecords are stored in a ModuleRecordMap, indexed by its name. The record has a bool isInitialised() to check for its status.
  • After all modules have been registered and initialiseModules() is called, the connections between the Modules are established. The dependency lists of each module is traversed and the ModuleRecords are connected with each other (by passing ModuleRecordPtrs). At the end of this step, each ModuleRecord knows the ModuleRecords of its dependencies.
  • Now call ModuleRecord::initialise():
    • First, check for any uninitialised dependencies. If so, a recursion is entered.
    • Cyclic dependency with the root ModuleRecord is detected and a runtime error is thrown if this is the case. This done by passing a const ModuleRecord& reference along the chain.
    • After iterating through the dependencies, initialise the module itself, exit from this recursion.

The changes are already on SVN, so you can take a look at it. I haven't tested it yet, because the symbols are not yet available in the dynamic libraries.

Link to post
Share on other sites
I already have a first iteration of such an algorithm in place

 

Indeed, I saw your changes which I think make the process a little too complicated, hence my algorithm. :)

 

[*]Incoming modules (by registerModule()) are stored into a ModuleRecord. All ModuleRecords are stored in a ModuleRecordMap, indexed by its name. The record has a bool isInitialised() to check for its status.

 

Here an additional wrapper class is used, with a lot of methods, to store the dependencies and initialisation status. By using two maps, the initialisation status is trivially accessed by checking for existence in the _initialisedModules map (a fast operation).

 

[*]After all modules have been registered and initialiseModules() is called, the connections between the Modules are established. The dependency lists of each module is traversed and the ModuleRecords are connected with each other (by passing ModuleRecordPtrs). At the end of this step, each ModuleRecord knows the ModuleRecords of its dependencies.

 

This is the "dependency graph construction" I mentioned: an additional traversal is necessary to convert dependency names into RegisterableModulePtrs. The two-maps solution operates directly on dependency names.

 

[*]Now call ModuleRecord::initialise():

[*]First, check for any uninitialised dependencies. If so, a recursion is entered.

[*]Cyclic dependency with the root ModuleRecord is detected and a runtime error is thrown if this is the case. This done by passing a const ModuleRecord& reference along the chain.

[*]After iterating through the dependencies, initialise the module itself, exit from this recursion.

 

This part is broadly similar, except the two-maps solution gives you cyclic dependency resolution "for free" because the recursive call is a no-op for already-satisfied dependencies. A guess a specific check would be needed in case a module is stupid enough to list itself as a direct dependency, because at this stage it would not already be in the _initialiseModules map.

 

I'm not trying to rubbish your solution, I just like to keep things as simple as possible (I guess I should have come up with a proposed design earlier, since it is a key part of the implementation).

Link to post
Share on other sites

Looks good, I've made a few minor changes which are in the SVN log. Mostly naming/structural stuff, although importantly I have removed the requirement for getModule() to throw an exception if initialiseModules() has not been invoked first, since modules might call getModule() during their own initialisation routine (if they require any access to other modules).

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...