#5231: Remove the static ModuleRegistry::Instance accessor, the modul…

…e registry is hosted by the core module now. This revision crashes at startup due to the statically created OpenGLRenderSystem trying to access the module registry before there is one available.
codereader · Apr 29, 2020 · 4f9a655 · 4f9a655
1 parent 47bde40
commit 4f9a655
Show file tree

Hide file tree

Showing 17 changed files with 53 additions and 52 deletions.
diff --git a/include/iradiant.h b/include/iradiant.h
@@ -26,6 +26,11 @@ class IRadiant
      */
     virtual applog::ILogWriter& getLogWriter() = 0;
 
+    /**
+     * Returns the central module registry instance.
+     */
+    virtual IModuleRegistry& getModuleRegistry() = 0;
+
     virtual ~IRadiant() {}
 };
 

diff --git a/radiant/Radiant.cpp b/radiant/Radiant.cpp
@@ -65,6 +65,11 @@ class Radiant :
 		return applog::LogWriter::Instance();
 	}
 
+	IModuleRegistry& getModuleRegistry() override
+	{
+		return *_moduleRegistry;
+	}
+
 private:
 	void createLogFile()
 	{

diff --git a/radiant/RadiantApp.cpp b/radiant/RadiantApp.cpp
@@ -50,14 +50,14 @@ bool RadiantApp::OnInit()
 	// Initialise the context (application path / settings path, is
 	// OS-specific)
 	_context.initialise(wxApp::argc, wxApp::argv);
-	module::ModuleRegistry::Instance().setContext(_context);
 
 	try
 	{
 		_coreModule.reset(new module::CoreModule(_context));
 
 		auto* radiant = _coreModule->get();
 
+		module::RegistryReference::Instance().setRegistry(radiant->getModuleRegistry());
 		module::initialiseStreams(radiant->getLogWriter());
 	}
 	catch (module::CoreModule::FailureException & ex)
@@ -151,7 +151,7 @@ void RadiantApp::onStartupEvent(wxCommandEvent& ev)
 
     try
     {
-        module::ModuleRegistry::Instance().loadAndInitialiseModules();
+		module::GlobalModuleRegistry().loadAndInitialiseModules();
     }
     catch (const std::exception& e)
     {

diff --git a/radiant/log/PIDFile.h b/radiant/log/PIDFile.h
@@ -1,8 +1,8 @@
 #pragma once
 
+#include "imodule.h"
 #include "wxutil/dialog/MessageBox.h"
 #include "settings/PreferenceSystem.h"
-#include "modulesystem/ModuleRegistry.h"
 
 #include "os/file.h"
 #include "string/replace.h"
@@ -23,13 +23,13 @@ class PIDFile
 public:
 	PIDFile(const std::string& filename)
 	{
-		module::ModuleRegistry& registry = module::ModuleRegistry::Instance();
+		IModuleRegistry& registry = module::GlobalModuleRegistry();
 		_filename = registry.getApplicationContext().getSettingsPath() + filename;
 
 		FILE* pid = fopen(_filename.c_str(), "r");
 
 		// Check for an existing radiant.pid file
-		if (pid != NULL)
+		if (pid != nullptr)
 		{
 			fclose(pid);
 

diff --git a/radiant/map/StartupMapLoader.cpp b/radiant/map/StartupMapLoader.cpp
@@ -8,7 +8,6 @@
 #include "iradiant.h"
 #include "Map.h"
 #include "ui/mru/MRU.h"
-#include "modulesystem/ModuleRegistry.h"
 
 #include "os/path.h"
 #include "os/file.h"
@@ -31,7 +30,7 @@ void StartupMapLoader::onRadiantStartup()
 	std::string mapToLoad = "";
 
     const ApplicationContext::ArgumentList& args(
-        module::ModuleRegistry::Instance().getApplicationContext().getCmdLineArgs()
+        module::GlobalModuleRegistry().getApplicationContext().getCmdLineArgs()
     );
 
     for (const std::string& candidate : args)

diff --git a/radiant/model/ModelFormatManager.cpp b/radiant/model/ModelFormatManager.cpp
@@ -1,5 +1,6 @@
 #include "ModelFormatManager.h"
 
+#include "imodule.h"
 #include "i18n.h"
 #include "itextstream.h"
 #include "ifiletypes.h"
@@ -29,7 +30,7 @@ void ModelFormatManager::initialiseModule(const ApplicationContext& ctx)
 
 	_nullModelLoader.reset(new NullModelLoader);
 
-	module::ModuleRegistry::Instance().signal_allModulesInitialised().connect(
+	module::GlobalModuleRegistry().signal_allModulesInitialised().connect(
 		sigc::mem_fun(this, &ModelFormatManager::postModuleInitialisation)
 	);
 }

diff --git a/radiant/modulesystem/ModuleLoader.cpp b/radiant/modulesystem/ModuleLoader.cpp
@@ -26,6 +26,10 @@ namespace
 	typedef void(*RegisterModulesFunc)(IModuleRegistry& registry);
 }
 
+ModuleLoader::ModuleLoader(IModuleRegistry& registry) :
+	_registry(registry)
+{}
+
 // Functor operator, gets invoked on directory traversal
 void ModuleLoader::processModuleFile(const fs::path& file)
 {
@@ -36,7 +40,7 @@ void ModuleLoader::processModuleFile(const fs::path& file)
 	rConsole() << "ModuleLoader: Loading module '" << fullName << "'" << std::endl;
 
 	// Create the encapsulator class
-	DynamicLibraryPtr library = std::make_shared<DynamicLibrary>(fullName);
+	auto library = std::make_shared<DynamicLibrary>(fullName);
 
 	// greebo: Try to find our entry point and invoke it and add the library to the list
 	// on success. If the load fails, the shared pointer won't be added and
@@ -52,9 +56,7 @@ void ModuleLoader::processModuleFile(const fs::path& file)
 	}
 
 	// Library was successfully loaded, lookup the symbol
-	DynamicLibrary::FunctionPointer funcPtr(
-		library->findSymbol(SYMBOL_REGISTER_MODULE)
-	);
+	auto funcPtr = library->findSymbol(SYMBOL_REGISTER_MODULE);
 
 	if (funcPtr == nullptr)
 	{
@@ -65,14 +67,14 @@ void ModuleLoader::processModuleFile(const fs::path& file)
 	}
 
 	// Brute-force conversion of the pointer to the desired type
-	RegisterModulesFunc regFunc = reinterpret_cast<RegisterModulesFunc>(funcPtr);
+	auto regFunc = reinterpret_cast<RegisterModulesFunc>(funcPtr);
 
 	try
 	{
 		// Call the symbol and pass a reference to the ModuleRegistry
 		// This method might throw a ModuleCompatibilityException in its
 		// module::performDefaultInitialisation() routine.
-		regFunc(ModuleRegistry::Instance());
+		regFunc(_registry);
 
 		// Add the library to the static list (for later reference)
 		_dynamicLibraryList.push_back(library);

diff --git a/radiant/modulesystem/ModuleLoader.h b/radiant/modulesystem/ModuleLoader.h
@@ -9,10 +9,11 @@
 namespace module
 {
 
-/** Module loader functor class. This class is used to traverse a directory and
- * load each module into the GlobalModuleServer.
+/**
+ * Module loader functor class. This class is used to traverse a directory and
+ * load each module, calling the RegisterModule function they need to expose.
  *
- * Invoke the static method loadModules() to load the DLLs from DarkRadiant's
+ * Invoke the loadModules() method to load the DLLs from DarkRadiant's
  * default module folders (e.g. modules/ and plugins/).
  */
 class ModuleLoader
@@ -21,8 +22,12 @@ class ModuleLoader
 	// This list contains all the allocated dynamic libraries
 	DynamicLibraryList _dynamicLibraryList;
 
+	IModuleRegistry& _registry;
+
 public:
-	// Static loader algorithm, searches plugins/ and modules/ for .dll/.so files
+	ModuleLoader(IModuleRegistry& registry);
+
+	// Load algorithm, searches plugins/ and modules/ for .dll/.so files
 	void loadModules(const std::string& root);
 
 	// Frees the list of DLLs

diff --git a/radiant/modulesystem/ModuleRegistry.cpp b/radiant/modulesystem/ModuleRegistry.cpp
@@ -16,7 +16,8 @@ namespace module
 ModuleRegistry::ModuleRegistry() :
 	_modulesInitialised(false),
 	_modulesShutdown(false),
-    _context(nullptr)
+    _context(nullptr),
+	_loader(*this)
 {
 	rMessage() << "ModuleRegistry instantiated." << std::endl;
 
@@ -271,16 +272,4 @@ std::string ModuleRegistry::getModuleList(const std::string& separator)
 	return returnValue;
 }
 
-ModuleRegistry& ModuleRegistry::Instance()
-{
-	static ModuleRegistry _registry;
-	return _registry;
-}
-
-IModuleRegistry& getRegistry()
-{
-	// Retrieve the singleton instance and deliver it
-	return ModuleRegistry::Instance();
-}
-
 } // namespace module
diff --git a/radiant/modulesystem/ModuleRegistry.h b/radiant/modulesystem/ModuleRegistry.h
@@ -9,8 +9,12 @@
 namespace module 
 {
 
-/** greebo: This is the actual implementation of the ModuleRegistry as defined in imodule.h.
- *          See the imodule.h file for a detailed documentation of the public methods.
+/** 
+ * greebo: Implementation of the IModuleRegistry interface defined in imodule.h.
+ * It stores and manages the lifecycle of all modules in DarkRadiant.
+ * 
+ * Use the registerModule() method to add new modules, which will be initialised
+ * during the startup phase, resolving the module dependencies on the go.
  */
 class ModuleRegistry :
 	public IModuleRegistry
@@ -79,9 +83,6 @@ class ModuleRegistry :
         _context = &context;
     }
 
-	// Contains the singleton instance
-	static ModuleRegistry& Instance();
-
 	// Returns a list of modules
 	std::string getModuleList(const std::string& separator = "\n");
 

diff --git a/radiant/modulesystem/StaticModule.h b/radiant/modulesystem/StaticModule.h
@@ -70,7 +70,7 @@ class StaticModule
 	inline std::shared_ptr<ModuleType> getModule()
     {
 		return std::static_pointer_cast<ModuleType>(
-			ModuleRegistry::Instance().getModule(_moduleName)
+            GlobalModuleRegistry().getModule(_moduleName)
 		);
 	}
 };

diff --git a/radiant/render/OpenGLRenderSystem.cpp b/radiant/render/OpenGLRenderSystem.cpp
@@ -47,7 +47,7 @@ OpenGLRenderSystem::OpenGLRenderSystem() :
 {
 	// For the static default rendersystem, the MaterialManager is not existent yet,
 	// hence it will be attached in initialiseModule().
-    if (module::ModuleRegistry::Instance().moduleExists(MODULE_SHADERSYSTEM))
+    if (module::GlobalModuleRegistry().moduleExists(MODULE_SHADERSYSTEM))
 	{
 		_materialDefsLoaded = GlobalMaterialManager().signal_DefsLoaded().connect(
 			sigc::mem_fun(*this, &OpenGLRenderSystem::realise));
@@ -62,7 +62,7 @@ OpenGLRenderSystem::OpenGLRenderSystem() :
 
     // If the openGL module is already initialised and a shared context is created
     // trigger a call to extensionsInitialised().
-    if (module::ModuleRegistry::Instance().moduleExists(MODULE_OPENGL) && 
+    if (module::GlobalModuleRegistry().moduleExists(MODULE_OPENGL) &&
         GlobalOpenGL().wxContextValid())
 	{
         extensionsInitialised();

diff --git a/radiant/settings/LanguageManager.cpp b/radiant/settings/LanguageManager.cpp
@@ -155,7 +155,7 @@ void LanguageManager::init(const ApplicationContext& ctx)
 	LanguageManagerPtr instancePtr(new LanguageManager);
 
 	// Hand that over to the module registry
-    module::ModuleRegistry::Instance().registerModule(instancePtr);
+    module::GlobalModuleRegistry().registerModule(instancePtr);
 
 	// Initialise the module manually
 	instancePtr->initFromContext(ctx);

diff --git a/radiant/ui/about/AboutDialog.cpp b/radiant/ui/about/AboutDialog.cpp
@@ -89,13 +89,7 @@ void AboutDialog::populateWindow()
 
 	findNamedObject<wxTextCtrl>(this, "AboutDialogOpenGLExtensions")->SetValue(openGLExtensions);
 
-	// DarkRadiant modules
-
-	std::string modules = module::ModuleRegistry::Instance().getModuleList(", ");
-	findNamedObject<wxTextCtrl>(this, "AboutDialogDarkRadiantModules")->SetValue(modules);
-
-	findNamedObject<wxButton>(this, "AboutDialogOkButton")->Connect(
-		wxEVT_BUTTON, wxCommandEventHandler(AboutDialog::_onClose), NULL, this);
+	findNamedObject<wxButton>(this, "AboutDialogOkButton")->Bind(wxEVT_BUTTON, &AboutDialog::_onClose, this);
 
 	// Make all headers bold
 	wxFont bold = findNamedObject<wxStaticText>(this, "AboutDialogHeader1")->GetFont().Bold();

diff --git a/radiant/ui/einspector/PropertyEditorFactory.cpp b/radiant/ui/einspector/PropertyEditorFactory.cpp
@@ -39,7 +39,7 @@ void PropertyEditorFactory::registerClasses()
 	_peMap["mat"] = PropertyEditorPtr(new TexturePropertyEditor());
 	_peMap["skin"] = PropertyEditorPtr(new SkinPropertyEditor());
 
-	if (module::ModuleRegistry::Instance().moduleExists(MODULE_SOUNDMANAGER))
+	if (module::GlobalModuleRegistry().moduleExists(MODULE_SOUNDMANAGER))
 	{
 		_peMap["sound"] = PropertyEditorPtr(new SoundPropertyEditor());
 	}

diff --git a/radiant/ui/ortho/OrthoContextMenu.cpp b/radiant/ui/ortho/OrthoContextMenu.cpp
@@ -337,7 +337,7 @@ void OrthoContextMenu::callbackAddSpeaker()
     ISoundShaderPtr soundShader;
 
     // If we have an active sound module, query the desired shader from the user
-    if (module::ModuleRegistry::Instance().moduleExists(MODULE_SOUNDMANAGER))
+    if (module::GlobalModuleRegistry().moduleExists(MODULE_SOUNDMANAGER))
     {
         IResourceChooser* chooser = GlobalDialogManager().createSoundShaderChooser();
 

diff --git a/radiant/ui/splash/Splash.cpp b/radiant/ui/splash/Splash.cpp
@@ -70,7 +70,7 @@ Splash::Splash() :
 	wxFrame(nullptr, wxID_ANY, wxT("DarkRadiant"), wxDefaultPosition, wxDefaultSize, wxCENTRE),
 	_progressBar(nullptr)
 {
-    const ApplicationContext& ctx = module::ModuleRegistry::Instance().getApplicationContext();
+    const ApplicationContext& ctx = module::GlobalModuleRegistry().getApplicationContext();
 	std::string fullFileName(ctx.getBitmapsPath() + SPLASH_FILENAME);
 
 	wxBoxSizer* sizer = new wxBoxSizer(wxVERTICAL);
@@ -88,7 +88,7 @@ Splash::Splash() :
 	Show();
 
     // Subscribe to the post-module init event to destroy ourselves
-    module::ModuleRegistry::Instance().signal_allModulesInitialised().connect(
+    module::GlobalModuleRegistry().signal_allModulesInitialised().connect(
         sigc::hide_return(sigc::mem_fun(this, &Splash::Destroy)));
 }
 
@@ -128,7 +128,7 @@ Splash& Splash::Instance()
 void Splash::OnAppStartup()
 {
 	// Connect the module progress callback
-	module::ModuleRegistry::Instance().signal_moduleInitialisationProgress().connect(
+	module::GlobalModuleRegistry().signal_moduleInitialisationProgress().connect(
 		sigc::mem_fun(Instance(), &Splash::setProgressAndText));
 }