Fix bugs in scene graph manipulation API

assets/sun/sun.asset

@@ -3,8 +3,7 @@
 local AssetHelper = asset.import('assethelper')
 
 asset.SolarSystem = {
-    Name = "SolarSystem",
-    Parent = "Root"
+    Name = "SolarSystem"
 }
 
 asset.SolarSystemBarycenter = {

include/openspace/scene/scene.h

@@ -34,9 +34,11 @@
 
 #include <openspace/util/camera.h>
 #include <openspace/util/updatestructures.h>
+#include <openspace/scene/scenegraphnode.h>
 #include <openspace/scripting/scriptengine.h>
 #include <ghoul/opengl/programobject.h>
 
+
 namespace ghoul { class Dictionary; }
 
 namespace openspace {
@@ -72,9 +74,14 @@ public:
     void clear();
 
     /**
-     * Set the root node of the scene
+     * Attach node to the root
      */
-    void setRoot(std::unique_ptr<SceneGraphNode> root);
+    void attachNode(std::unique_ptr<SceneGraphNode> node);
+
+    /**
+     * Detach node from the root
+     */
+    std::unique_ptr<SceneGraphNode> detachNode(SceneGraphNode& node);
 
     /**
      * Set the camera of the scene
@@ -99,7 +106,12 @@ public:
     /**
      * Return the root SceneGraphNode.
      */
-    SceneGraphNode* root() const;
+    SceneGraphNode* root();
+
+    /**
+    * Return the root SceneGraphNode.
+    */
+    const SceneGraphNode* root() const;
 
     /**
      * Return the scenegraph node with the specified name or <code>nullptr</code> if that
@@ -110,17 +122,17 @@ public:
     /**
      * Add a node and all its children to the scene.
      */
-    void addNode(SceneGraphNode* node, UpdateDependencies updateDeps = UpdateDependencies::Yes);
+    void registerNode(SceneGraphNode* node);
 
     /**
      * Remove a node and all its children from the scene.
      */
-    void removeNode(SceneGraphNode* node, UpdateDependencies updateDeps = UpdateDependencies::Yes);
+    void unregisterNode(SceneGraphNode* node);
 
     /**
-     * Update dependencies.
-     */
-    void updateDependencies();
+    * Mark the node registry as dirty
+    */
+    void markNodeRegistryDirty();
 
     /**
      * Return a vector of all scene graph nodes in the scene.
@@ -152,13 +164,19 @@ public:
 private:
     std::string generateJson() const override;
 
+    /**
+     * Update dependencies.
+     */
+    void updateNodeRegistry();
+
     void sortTopologically();
 
-    std::unique_ptr<SceneGraphNode> _root;
     std::unique_ptr<Camera> _camera;
     std::vector<SceneGraphNode*> _topologicallySortedNodes;
     std::vector<SceneGraphNode*> _circularNodes;
     std::map<std::string, SceneGraphNode*> _nodesByName;
+    bool _dirtyNodeRegistry;
+    SceneGraphNode _rootDummy;
 
     std::mutex _programUpdateLock;
     std::set<ghoul::opengl::ProgramObject*> _programsToUpdate;

include/openspace/scene/scenegraphnode.h

@@ -83,14 +83,15 @@ public:
     void render(const RenderData& data, RendererTasks& tasks);
     void updateCamera(Camera* camera) const;
 
-    void attachChild(std::unique_ptr<SceneGraphNode> child, UpdateScene updateScene = UpdateScene::Yes);
-    std::unique_ptr<SceneGraphNode> detachChild(SceneGraphNode& child, UpdateScene updateScene = UpdateScene::Yes);
-    void setParent(SceneGraphNode& parent, UpdateScene updateScene = UpdateScene::Yes);
+    void attachChild(std::unique_ptr<SceneGraphNode> child);
+    std::unique_ptr<SceneGraphNode> detachChild(SceneGraphNode& child);
+    void clearChildren();
+    void setParent(SceneGraphNode& parent);
 
-    void addDependency(SceneGraphNode& dependency, UpdateScene updateScene = UpdateScene::Yes);
-    void removeDependency(SceneGraphNode& dependency, UpdateScene updateScene = UpdateScene::Yes);
-    void clearDependencies(UpdateScene updateScene = UpdateScene::Yes);
-    void setDependencies(const std::vector<SceneGraphNode*>& dependencies, UpdateScene updateScene = UpdateScene::Yes);
+    void addDependency(SceneGraphNode& dependency);
+    void removeDependency(SceneGraphNode& dependency);
+    void clearDependencies();
+    void setDependencies(const std::vector<SceneGraphNode*>& dependencies);
 
     const std::vector<SceneGraphNode*>& dependencies() const;
     const std::vector<SceneGraphNode*>& dependentNodes() const;

src/engine/openspaceengine.cpp

@@ -573,6 +573,7 @@ void OpenSpaceEngine::loadScene(const std::string& scenePath) {
                 _renderEngine->setScene(nullptr);
                 _renderEngine->setCamera(nullptr);
                 _interactionHandler->setCamera(nullptr);
+                _scene->clear();
             }
 
             _scene = std::make_unique<Scene>();

src/scene/assetloader.cpp

@@ -434,6 +434,10 @@ ghoul::Dictionary AssetLoader::Asset::dataDictionary() {
 }
 
 void AssetLoader::Asset::addDependency(Asset* asset) {
+    // Do nothing if the dependency already exists.
+    if (std::find(_dependencies.begin(), _dependencies.end(), asset) != _dependencies.end()) {
+        return;
+    }
     if (_initialized) {
         asset->initialize();
     }

src/scene/scene.cpp

@@ -32,7 +32,6 @@
 #include <openspace/query/query.h>
 #include <openspace/rendering/renderengine.h>
 #include <openspace/scene/scenegraphnode.h>
-#include <openspace/scene/nodeloader.h>
 #include <openspace/scripting/scriptengine.h>
 #include <openspace/scripting/script_helper.h>
 #include <openspace/util/time.h>
@@ -93,22 +92,23 @@ Scene::Scene()
         },
         JsFilename
     )
-{
-    std::unique_ptr<SceneGraphNode> rootNode = std::make_unique<SceneGraphNode>();
-    rootNode->setName(SceneGraphNode::RootNodeName);
-    setRoot(std::move(rootNode));
+    , _dirtyNodeRegistry(false)
+{   
+    _rootDummy.setName(SceneGraphNode::RootNodeName);
+    _rootDummy.setScene(this);
 }
 
-Scene::~Scene(){
+Scene::~Scene() {
+    clear();
+    _rootDummy.setScene(nullptr);
 }
-    
-void Scene::setRoot(std::unique_ptr<SceneGraphNode> root) {
-    if (_root) {
-        removeNode(_root.get());
-    }
-    _root = std::move(root);
-    _root->setScene(this);
-    addNode(_root.get());
+
+void Scene::attachNode(std::unique_ptr<SceneGraphNode> node) {
+    _rootDummy.attachChild(std::move(node));
+}
+
+std::unique_ptr<SceneGraphNode> Scene::detachNode(SceneGraphNode& node) {
+    return _rootDummy.detachChild(node);
 }
 
 void Scene::setCamera(std::unique_ptr<Camera> camera) {
@@ -119,35 +119,28 @@ Camera* Scene::camera() const {
     return _camera.get();
 }
 
-void Scene::addNode(SceneGraphNode* node, UpdateDependencies updateDeps) {
-    // Add the node and all its children.
-    node->traversePreOrder([this](SceneGraphNode* n) {
-        _topologicallySortedNodes.push_back(n);
-        _nodesByName[n->name()] = n;
-    });
-    
-    if (updateDeps) {
-        updateDependencies();
-    }
+void Scene::registerNode(SceneGraphNode* node) {
+    _topologicallySortedNodes.push_back(node);
+    _nodesByName[node->name()] = node;
+    _dirtyNodeRegistry = true;
 }
 
-void Scene::removeNode(SceneGraphNode* node, UpdateDependencies updateDeps) {
-    // Remove the node and all its children.
-    node->traversePostOrder([this](SceneGraphNode* node) {
-        _topologicallySortedNodes.erase(
-            std::remove(_topologicallySortedNodes.begin(), _topologicallySortedNodes.end(), node),
-            _topologicallySortedNodes.end()
-        );
-        _nodesByName.erase(node->name());
-    });
-    
-    if (updateDeps) {
-        updateDependencies();
-    }
+void Scene::unregisterNode(SceneGraphNode* node) {
+    _topologicallySortedNodes.erase(
+        std::remove(_topologicallySortedNodes.begin(), _topologicallySortedNodes.end(), node),
+        _topologicallySortedNodes.end()
+    );
+    _nodesByName.erase(node->name());
+    _dirtyNodeRegistry = true;
 }
 
-void Scene::updateDependencies() {
+void Scene::markNodeRegistryDirty() {
+    _dirtyNodeRegistry = true;
+}
+
+void Scene::updateNodeRegistry() {
     sortTopologically();
+    _dirtyNodeRegistry = false;
 }
 
 void Scene::sortTopologically() {
@@ -217,6 +210,9 @@ void Scene::sortTopologically() {
 }
 
 void Scene::update(const UpdateData& data) {
+    if (_dirtyNodeRegistry) {
+        updateNodeRegistry();
+    }
     for (SceneGraphNode* node : _topologicallySortedNodes) {
         try {
             LTRACE("Scene::update(begin '" + node->name() + "')");
@@ -244,15 +240,19 @@ void Scene::render(const RenderData& data, RendererTasks& tasks) {
 
 void Scene::clear() {
     LINFO("Clearing current scene graph");
-    _root = nullptr;
+    _rootDummy.clearChildren();
 }
 
 const std::map<std::string, SceneGraphNode*>& Scene::nodesByName() const {
     return _nodesByName;
 }
 
-SceneGraphNode* Scene::root() const {
-    return _root.get();
+SceneGraphNode* Scene::root() {
+    return &_rootDummy;
+}
+
+const SceneGraphNode* Scene::root() const {
+    return &_rootDummy;
 }
     
 SceneGraphNode* Scene::sceneGraphNode(const std::string& name) const {
@@ -269,11 +269,25 @@ const std::vector<SceneGraphNode*>& Scene::allSceneGraphNodes() const {
 
 
 SceneGraphNode* Scene::loadNode(const ghoul::Dictionary& dict) {
+    // First interpret the dictionary
     std::vector<std::string> dependencyNames;
+    const std::string nodeName = dict.value<std::string>(KeyName);
+    const bool hasParent = dict.hasKey(KeyParentName);
+
+    SceneGraphNode* parent = nullptr;
+    if (hasParent) {
+        std::string parentName = dict.value<std::string>(KeyParentName);
+        parent = sceneGraphNode(parentName);
+        if (!parent) {
+            LERROR("Could not find parent '" + parentName + "' for '" + nodeName + "'");
+            return nullptr;
+        }
+    }
 
-    std::string nodeName = dict.value<std::string>(KeyName);
-    std::string parentName = dict.value<std::string>(KeyParentName);
     std::unique_ptr<SceneGraphNode> node = SceneGraphNode::createFromDictionary(dict);
+    if (!node) {
+        LERROR("Could not create node from dictionary: " + nodeName);
+    }
 
     if (dict.hasKey(SceneGraphNode::KeyDependencies)) {
         if (!dict.hasValue<ghoul::Dictionary>(SceneGraphNode::KeyDependencies)) {
@@ -289,12 +303,7 @@ SceneGraphNode* Scene::loadNode(const ghoul::Dictionary& dict) {
         }
     }
 
-    SceneGraphNode* parent = sceneGraphNode(parentName);
-    if (!parent) {
-        LERROR("Could not find parent '" + parentName + "' for '" + nodeName + "'");
-        return nullptr;
-    }
-
+    // Make sure all dependencies are found
     std::vector<SceneGraphNode*> dependencies;
     bool foundAllDeps = true;
     for (const auto& depName : dependencyNames) {
@@ -311,9 +320,16 @@ SceneGraphNode* Scene::loadNode(const ghoul::Dictionary& dict) {
         return nullptr;
     }
 
+    // Now attach the node to the graph
     SceneGraphNode* rawNodePointer = node.get();
-    node->setDependencies(dependencies, SceneGraphNode::UpdateScene::No);
-    parent->attachChild(std::move(node));
+
+    if (parent) {
+        parent->attachChild(std::move(node));
+    } else {
+        attachNode(std::move(node));
+    }
+
+    rawNodePointer->setDependencies(dependencies);
     return rawNodePointer;
 }
 

src/scene/scenegraphnode.cpp

@@ -204,13 +204,13 @@ bool SceneGraphNode::initialize() {
 bool SceneGraphNode::deinitialize() {
     LDEBUG("Deinitialize: " << name());
 
+    setScene(nullptr);
+
     if (_renderable) {
         _renderable->deinitialize();
         _renderable = nullptr;
     }
-    _children.clear();
-
-    // reset variables
+    clearChildren();
     _parent = nullptr;
 
     return true;
@@ -356,47 +356,27 @@ void SceneGraphNode::render(const RenderData& data, RendererTasks& tasks) {
     //    child->render(newData);
 }
 
-void SceneGraphNode::setParent(SceneGraphNode& parent, UpdateScene updateScene) {
+void SceneGraphNode::setParent(SceneGraphNode& parent) {
     ghoul_assert(_parent != nullptr, "Node must be attached to a parent");
-    ghoul_assert(
-        !updateScene || _scene == parent._scene,
-        "For the scene to be updated, this object must belong to the same scene as the parent"
-    );
-    ghoul_assert(
-        !updateScene || _parent->_scene == parent._scene,
-        "Old and new parent cannot belong to separate scenes"
-    );
-
-    parent.attachChild(_parent->detachChild(*this, UpdateScene::No), UpdateScene::No);
-
-    if (_scene && updateScene) {
-        _scene->updateDependencies();
-    }
+    parent.attachChild(_parent->detachChild(*this));
 }
 
-void SceneGraphNode::attachChild(std::unique_ptr<SceneGraphNode> child, UpdateScene updateScene) {
+void SceneGraphNode::attachChild(std::unique_ptr<SceneGraphNode> child) {
     ghoul_assert(child != nullptr, "Child may not be null");
     ghoul_assert(child->parent() == nullptr, "Child may not already have a parent");
 
+    // Create link between parent and child
     child->_parent = this;
-    if (_scene) {
-        child->setScene(_scene);
-    }
-
     SceneGraphNode* childRaw = child.get();
    _children.push_back(std::move(child));
 
-   if (_scene && updateScene) {
-       _scene->addNode(childRaw);
-   }
+    // Set scene of child (and children recursively)
+    childRaw->setScene(_scene);
 }
 
-std::unique_ptr<SceneGraphNode> SceneGraphNode::detachChild(SceneGraphNode& child, UpdateScene updateScene) {
+std::unique_ptr<SceneGraphNode> SceneGraphNode::detachChild(SceneGraphNode& child) {
     ghoul_assert(child._dependentNodes.empty(), "Nodes cannot depend on a node being detached");
     ghoul_assert(child._parent != nullptr, "Node must be attached to a parent");
-    
-    // Update of deps is deffered to the removal of the node from the scene
-    clearDependencies(UpdateScene::No);
 
     auto iter = std::find_if(
         _children.begin(),
@@ -406,29 +386,49 @@ std::unique_ptr<SceneGraphNode> SceneGraphNode::detachChild(SceneGraphNode& chil
         }
     );
 
-    if (_scene && updateScene) {
-        _scene->removeNode(&child);
+    if (iter == _children.end()) {
+        LERROR("Trying to detach a non-existing child");
     }
 
+    traversePreOrder([](SceneGraphNode* node) {
+        node->clearDependencies();
+    });
+
+    // Unset scene of child (and children recursively)
+    if (_scene) {
+        child.setScene(nullptr);
+    }
+
+    // Remove link between parent and child
+    child._parent = nullptr;
     std::unique_ptr<SceneGraphNode> c = std::move(*iter);
     _children.erase(iter);
 
-    if (_scene) {
-        setScene(nullptr);
-    }
     return c;
 }
 
-void SceneGraphNode::addDependency(SceneGraphNode& dependency, UpdateScene updateScene) {
+void SceneGraphNode::clearChildren() {
+    traversePreOrder([](SceneGraphNode* node) {
+        node->clearDependencies();
+    });
+    for (auto& c : _children) {
+        if (_scene) {
+            c->setScene(nullptr);
+        }
+        c->_parent = nullptr;
+    }
+    _children.clear();
+}
+
+void SceneGraphNode::addDependency(SceneGraphNode& dependency) {
     dependency._dependentNodes.push_back(this);
     _dependencies.push_back(&dependency);
-
-    if (_scene && updateScene) {
-        _scene->updateDependencies();
+    if (_scene) {
+        _scene->markNodeRegistryDirty();
     }
 }
 
-void SceneGraphNode::removeDependency(SceneGraphNode& dependency, UpdateScene updateScene) {
+void SceneGraphNode::removeDependency(SceneGraphNode& dependency) {
     dependency._dependentNodes.erase(std::remove_if(
         dependency._dependentNodes.begin(),
         dependency._dependentNodes.end(),
@@ -444,12 +444,12 @@ void SceneGraphNode::removeDependency(SceneGraphNode& dependency, UpdateScene up
         }
     ), _dependencies.end());
 
-    if (_scene && updateScene) {
-        _scene->updateDependencies();
+    if (_scene) {
+        _scene->markNodeRegistryDirty();
     }
 }
 
-void SceneGraphNode::clearDependencies(UpdateScene updateScene) {
+void SceneGraphNode::clearDependencies() {
     for (auto dependency : _dependencies) {
         dependency->_dependentNodes.erase(std::remove_if(
             dependency->_dependentNodes.begin(),
@@ -461,21 +461,21 @@ void SceneGraphNode::clearDependencies(UpdateScene updateScene) {
     }
     _dependencies.clear();
 
-    if (_scene && updateScene) {
-        _scene->updateDependencies();
+    if (_scene) {
+        _scene->markNodeRegistryDirty();
     }
 }
 
-void SceneGraphNode::setDependencies(const std::vector<SceneGraphNode*>& dependencies, UpdateScene updateScene) {
-    clearDependencies(UpdateScene::No);
+void SceneGraphNode::setDependencies(const std::vector<SceneGraphNode*>& dependencies) {
+    clearDependencies();
 
     _dependencies = dependencies;
     for (auto dependency : dependencies) {
         dependency->_dependentNodes.push_back(this);
     }
 
-    if (_scene && updateScene) {
-        _scene->updateDependencies();
+    if (_scene) {
+        _scene->markNodeRegistryDirty();
     }
 }
 
@@ -568,9 +568,26 @@ Scene* SceneGraphNode::scene() {
 }
 
 void SceneGraphNode::setScene(Scene* scene) {
+    // Unregister from previous scene, bottom up
+    traversePostOrder([scene](SceneGraphNode* node) {
+        if (node->_scene) {
+            node->_scene->unregisterNode(node);
+        }
+        node->_scene = nullptr;
+    });
+
+    if (!scene) {
+        return;
+    }
+
+    // Register on new scene, top down
     traversePreOrder([scene](SceneGraphNode* node) {
         node->_scene = scene;
+        if (scene) {
+            scene->registerNode(node);
+        }
     });
+    
 }
 
 std::vector<SceneGraphNode*> SceneGraphNode::children() const {

src/scene/scenegraphnode_doc.inl

@@ -46,13 +46,12 @@ documentation::Documentation SceneGraphNode::Documentation() {
         {
             "Parent",
             new StringAnnotationVerifier(
-                "Must be a name for another scenegraph node, or 'Root'"
+                "If specified, this must be a name for another scenegraph node"
             ),
             "This names the parent of the currently specified scenegraph node. The "
-            "parent must not have been defined earlier, but must exist at loading time, "
-            "or the scenegraph node creation will fail. A special parent 'Root' is "
-            "available that denotes the root of the scenegraph.",
-            Optional::No
+            "parent must already exist in the scene graph. If not specified, the node "
+            "will be attached to the root of the scenegraph.",
+            Optional::Yes
         },
         {
             "Renderable",