Cleanup of the Initializer class (#3631)

* Combine single- and multithreaded initializer to only one initializer
* Change the default to 0 to represent initializing the nodes on the main thread. Increase the default number of threads

---------

Co-authored-by: Malin E <malin.ejdbo@gmail.com>

include/openspace/scene/sceneinitializer.h

@@ -33,35 +33,24 @@ namespace openspace {
 
 class SceneGraphNode;
 
+/**
+ * This class is responsible for initializing nodes, in a multithreaded fashion if needed,
+ * that are passed into it. The constructor takes the number of extra separate threads
+ * that are used to initialize nodes. Passing `0` for the number of threads results in
+ * nodes being initialized on the main thread instead.
+ */
 class SceneInitializer {
 public:
-    virtual ~SceneInitializer() = default;
-    virtual void initializeNode(SceneGraphNode* node) = 0;
-    virtual std::vector<SceneGraphNode*> takeInitializedNodes() = 0;
-    virtual bool isInitializing() const = 0;
-};
+    SceneInitializer(unsigned int nThreads = 0);
 
-class SingleThreadedSceneInitializer final : public SceneInitializer {
-public:
-    void initializeNode(SceneGraphNode* node) override;
-    std::vector<SceneGraphNode*> takeInitializedNodes() override;
-    bool isInitializing() const override;
-
-private:
-    std::vector<SceneGraphNode*> _initializedNodes;
-};
-
-class MultiThreadedSceneInitializer final : public SceneInitializer {
-public:
-    explicit MultiThreadedSceneInitializer(unsigned int nThreads);
-
-    void initializeNode(SceneGraphNode* node) override;
-    std::vector<SceneGraphNode*> takeInitializedNodes() override;
-    bool isInitializing() const override;
+    void initializeNode(SceneGraphNode* node);
+    std::vector<SceneGraphNode*> takeInitializedNodes();
+    bool isInitializing() const;
 
 private:
     std::vector<SceneGraphNode*> _initializedNodes;
     std::unordered_set<SceneGraphNode*> _initializingNodes;
+    const unsigned int _nThreads;
     ThreadPool _threadPool;
     mutable std::mutex _mutex;
 };

src/engine/openspaceengine.cpp

@@ -803,13 +803,12 @@ void OpenSpaceEngine::loadAssets() {
     std::unique_ptr<SceneInitializer> sceneInitializer;
     if (global::configuration->useMultithreadedInitialization) {
         const unsigned int nThreads = std::max(
-            std::thread::hardware_concurrency() / 4,
-            4u
+            std::thread::hardware_concurrency() / 2, 2u
         );
-        sceneInitializer = std::make_unique<MultiThreadedSceneInitializer>(nThreads);
+        sceneInitializer = std::make_unique<SceneInitializer>(nThreads);
     }
     else {
-        sceneInitializer = std::make_unique<SingleThreadedSceneInitializer>();
+        sceneInitializer = std::make_unique<SceneInitializer>();
     }
 
     _scene = std::make_unique<Scene>(std::move(sceneInitializer));

src/scene/sceneinitializer.cpp

@@ -32,82 +32,80 @@
 
 namespace openspace {
 
-void SingleThreadedSceneInitializer::initializeNode(SceneGraphNode* node) {
-    node->initialize();
-    _initializedNodes.push_back(node);
-}
-
-std::vector<SceneGraphNode*> SingleThreadedSceneInitializer::takeInitializedNodes() {
-    std::vector<SceneGraphNode*> nodes = std::move(_initializedNodes);
-    return nodes;
-}
-
-bool SingleThreadedSceneInitializer::isInitializing() const {
-    return false;
-}
-
-MultiThreadedSceneInitializer::MultiThreadedSceneInitializer(unsigned int nThreads)
-    : _threadPool(nThreads)
+SceneInitializer::SceneInitializer(unsigned int nThreads)
+    : _nThreads(nThreads)
+    , _threadPool(nThreads) // The threadpool handles 0 threads gracefully
 {}
 
-void MultiThreadedSceneInitializer::initializeNode(SceneGraphNode* node) {
+void SceneInitializer::initializeNode(SceneGraphNode* node) {
     ZoneScoped;
 
-    auto initFunction = [this, node]() {
-        ZoneScopedN("MultiThreadedInit");
+    if (_nThreads == 0) {
+        // If we initialize nodes on the main thread, we don't need to update any loading
+        // screen as those changes won't be visible anyway as it is rendered on the same
+        // main thread
 
-        LoadingScreen* loadingScreen = global::openSpaceEngine->loadingScreen();
+        ZoneScopedN("SingleThreadedInit");
+        node->initialize();
+        _initializedNodes.push_back(node);
+    }
+    else {
+        auto initFunction = [this, node]() {
+            ZoneScopedN("MultiThreadedInit");
+
+            LoadingScreen* loadingScreen = global::openSpaceEngine->loadingScreen();
+
+            LoadingScreen::ProgressInfo progressInfo;
+            progressInfo.progress = 1.f;
+            if (loadingScreen) {
+                loadingScreen->updateItem(
+                    node->identifier(),
+                    node->guiName(),
+                    LoadingScreen::ItemStatus::Initializing,
+                    progressInfo
+                );
+            }
+
+            try {
+                node->initialize();
+            }
+            catch (const ghoul::RuntimeError& e) {
+                LERRORC(e.component, e.message);
+            }
+            const std::lock_guard g(_mutex);
+            _initializedNodes.push_back(node);
+            _initializingNodes.erase(node);
+
+            if (loadingScreen) {
+                loadingScreen->updateItem(
+                    node->identifier(),
+                    node->guiName(),
+                    LoadingScreen::ItemStatus::Finished,
+                    progressInfo
+                );
+            }
+            };
 
         LoadingScreen::ProgressInfo progressInfo;
-        progressInfo.progress = 1.f;
+        progressInfo.progress = 0.f;
+
+        LoadingScreen* loadingScreen = global::openSpaceEngine->loadingScreen();
         if (loadingScreen) {
             loadingScreen->updateItem(
                 node->identifier(),
                 node->guiName(),
-                LoadingScreen::ItemStatus::Initializing,
+                LoadingScreen::ItemStatus::Started,
                 progressInfo
             );
         }
 
-        try {
-            node->initialize();
-        }
-        catch (const ghoul::RuntimeError& e) {
-            LERRORC(e.component, e.message);
-        }
         const std::lock_guard g(_mutex);
-        _initializedNodes.push_back(node);
-        _initializingNodes.erase(node);
-
-        if (loadingScreen) {
-            loadingScreen->updateItem(
-                node->identifier(),
-                node->guiName(),
-                LoadingScreen::ItemStatus::Finished,
-                progressInfo
-            );
-        }
-    };
-
-    LoadingScreen::ProgressInfo progressInfo;
-    progressInfo.progress = 0.f;
-
-    LoadingScreen* loadingScreen = global::openSpaceEngine->loadingScreen();
-    if (loadingScreen) {
-        loadingScreen->updateItem(
-            node->identifier(),
-            node->guiName(),
-            LoadingScreen::ItemStatus::Started,
-            progressInfo
-        );
+        _initializingNodes.insert(node);
+        _threadPool.enqueue(initFunction);
     }
-
-    const std::lock_guard g(_mutex);
-    _initializingNodes.insert(node);
-    _threadPool.enqueue(initFunction);
 }
 
-std::vector<SceneGraphNode*> MultiThreadedSceneInitializer::takeInitializedNodes() {
+std::vector<SceneGraphNode*> SceneInitializer::takeInitializedNodes() {
     // Some of the scene graph nodes might still be in the initialization queue and we
     // should wait for those to finish or we end up in some half-initialized state since
     // other parts of the application already know about their existence
@@ -120,7 +118,7 @@ std::vector<SceneGraphNode*> MultiThreadedSceneInitializer::takeInitializedNodes
     return nodes;
 }
 
-bool MultiThreadedSceneInitializer::isInitializing() const {
+bool SceneInitializer::isInitializing() const {
     const std::lock_guard g(_mutex);
     return !_initializingNodes.empty();
 }

tests/test_assetloader.cpp

@@ -42,7 +42,7 @@
 TEST_CASE("AssetLoader: Assertion", "[assetloader]") {
     using namespace openspace;
 
-    const Scene scene = Scene(std::make_unique<SingleThreadedSceneInitializer>());
+    const Scene scene = Scene(std::make_unique<SceneInitializer>());
     ghoul::lua::LuaState* state = global::scriptEngine->luaState();
     AssetManager assetLoader(state, absPath("${TESTDIR}/AssetLoaderTest/"));
 
@@ -53,7 +53,7 @@ TEST_CASE("AssetLoader: Assertion", "[assetloader]") {
 TEST_CASE("AssetLoader: Basic Export Import", "[assetloader]") {
     using namespace openspace;
 
-    const Scene scene = Scene(std::make_unique<SingleThreadedSceneInitializer>());
+    Scene scene = Scene(std::make_unique<SceneInitializer>());
     ghoul::lua::LuaState* state = global::scriptEngine->luaState();
     AssetManager assetLoader(state, absPath("${TESTDIR}/AssetLoaderTest/"));
 
@@ -63,7 +63,7 @@ TEST_CASE("AssetLoader: Basic Export Import", "[assetloader]") {
 TEST_CASE("AssetLoader: Asset Functions", "[assetloader]") {
     using namespace openspace;
 
-    const Scene scene = Scene(std::make_unique<SingleThreadedSceneInitializer>());
+    const Scene scene = Scene(std::make_unique<SceneInitializer>(1u));
     ghoul::lua::LuaState* state = global::scriptEngine->luaState();
     AssetManager assetLoader(state, absPath("${TESTDIR}/AssetLoaderTest/"));