Fix requests from code review

include/openspace/engine/openspaceengine.h

@@ -98,7 +98,7 @@ public:
     void encode();
     void decode();
 
-    void scheduleLoadScene(const std::string& scenePath);
+    void scheduleLoadScene(std::string scenePath);
 
     void enableBarrier();
     void disableBarrier();
@@ -199,7 +199,7 @@ private:
     // Others
     std::unique_ptr<properties::PropertyOwner> _globalPropertyNamespace;
     
-    bool _switchScene;
+    bool _scheduledSceneSwitch;
     std::string _scenePath;
 
     struct {

include/openspace/rendering/renderengine.h

@@ -25,17 +25,18 @@
 #ifndef __OPENSPACE_CORE___RENDERENGINE___H__
 #define __OPENSPACE_CORE___RENDERENGINE___H__
 
-#include <openspace/scripting/scriptengine.h>
-
+#include <openspace/performance/performancemanager.h>
 #include <openspace/properties/optionproperty.h>
 #include <openspace/properties/propertyowner.h>
-#include <openspace/properties/triggerproperty.h>
 #include <openspace/properties/scalar/boolproperty.h>
 #include <openspace/properties/scalar/intproperty.h>
-#include <openspace/rendering/screenspacerenderable.h>
-#include <openspace/rendering/renderer.h>
+#include <openspace/properties/triggerproperty.h>
+
 #include <openspace/rendering/raycastermanager.h>
-#include <openspace/performance/performancemanager.h>
+#include <openspace/rendering/renderer.h>
+#include <openspace/rendering/screenspacerenderable.h>
+
+#include <openspace/scripting/scriptengine.h>
 
 #include <openspace/util/syncdata.h>
 

include/openspace/scene/sceneloader.h

@@ -25,14 +25,14 @@
 #ifndef __OPENSPACE_CORE___SCENELOADER___H__
 #define __OPENSPACE_CORE___SCENELOADER___H__
 
-#include <memory>
-#include <string>
+#include <openspace/scene/scenegraphnode.h>
+#include <openspace/util/camera.h>
 
 #include <ghoul/misc/dictionary.h>
 #include <ghoul/lua/ghoul_lua.h>
 
-#include <openspace/scene/scenegraphnode.h>
-#include <openspace/util/camera.h>
+#include <memory>
+#include <string>
 
 namespace openspace {
 
@@ -40,35 +40,6 @@ class Scene;
 
 class SceneLoader {
 public:
-    struct LoadedNode {
-        LoadedNode(
-            const std::string& nodeName,
-            const std::string& parentName,
-            const std::vector<std::string>& deps,
-            std::unique_ptr<SceneGraphNode> n
-        )
-            : name(nodeName)
-            , parent(parentName)
-            , dependencies(deps)
-            , node(std::move(n)) {}
-
-        std::string name;
-        std::string parent;
-        std::vector<std::string> dependencies;
-        std::unique_ptr<SceneGraphNode> node;
-    };
-
-    struct LoadedCamera {
-        LoadedCamera(
-            const std::string& parentName,
-            std::unique_ptr<Camera> c
-        )
-            : parent(parentName)
-            , camera(std::move(c)) {}
-        std::string parent;
-        std::unique_ptr<Camera> camera;
-    };
-
     SceneLoader() = default;
     ~SceneLoader() = default;
 
@@ -88,6 +59,35 @@ public:
     SceneGraphNode* importNodeDictionary(Scene& scene, const ghoul::Dictionary& dictionary);
 
 private:
+    struct LoadedNode {
+        LoadedNode(
+            const std::string& nodeName,
+            const std::string& parentName,
+            const std::vector<std::string>& deps,
+            std::unique_ptr<SceneGraphNode> n
+            )
+            : name(nodeName)
+            , parent(parentName)
+            , dependencies(deps)
+            , node(std::move(n)) {}
+
+        std::string name;
+        std::string parent;
+        std::vector<std::string> dependencies;
+        std::unique_ptr<SceneGraphNode> node;
+    };
+
+    struct LoadedCamera {
+        LoadedCamera(
+            const std::string& parentName,
+            std::unique_ptr<Camera> c
+            )
+            : parent(parentName)
+            , camera(std::move(c)) {}
+        std::string parent;
+        std::unique_ptr<Camera> camera;
+    };
+
     /**
      * Load a scene graph node from a dictionary
      */

modules/newhorizons/rendering/renderableplaneprojection.cpp

@@ -265,7 +265,7 @@ void RenderablePlaneProjection::updatePlane(const Image& img, double currentTime
     if (!_moving) {
         SceneGraphNode* thisNode = OsEng.renderEngine().scene()->sceneGraphNode(_name);
         SceneGraphNode* newParent = OsEng.renderEngine().scene()->sceneGraphNode(_target.node);
-        if (thisNode != nullptr && newParent != nullptr) {
+        if (thisNode && newParent) {
             thisNode->setParent(*newParent);
         }   
     }

src/engine/openspaceengine.cpp

@@ -135,7 +135,7 @@ OpenSpaceEngine::OpenSpaceEngine(
     , _parallelConnection(new ParallelConnection)
     , _windowWrapper(std::move(windowWrapper))
     , _globalPropertyNamespace(new properties::PropertyOwner(""))
-    , _switchScene(false)
+    , _scheduledSceneSwitch(false)
     , _scenePath("")
     , _runTime(0.0)
     , _shutdown({false, 0.f, 0.f})
@@ -509,13 +509,12 @@ void OpenSpaceEngine::initialize() {
     LINFO("Finished initializing");
 }
 
-void OpenSpaceEngine::scheduleLoadScene(const std::string& scenePath) {
-    _switchScene = true;
-    _scenePath = scenePath;
+void OpenSpaceEngine::scheduleLoadScene(std::string scenePath) {
+    _scheduledSceneSwitch = true;
+    _scenePath = std::move(scenePath);
 }
 
 void OpenSpaceEngine::loadScene(const std::string& scenePath) {
-    
     windowWrapper().setBarrier(false);
     windowWrapper().setSynchronization(false);
     OnExit(
@@ -576,42 +575,43 @@ void OpenSpaceEngine::loadScene(const std::string& scenePath) {
     }
 
     // Write keyboard documentation.
-    const std::string KeyboardShortcutsType =
-        ConfigurationManager::KeyKeyboardShortcuts + "." +
-        ConfigurationManager::PartType;
+    {
+        const std::string KeyboardShortcutsType =
+            ConfigurationManager::KeyKeyboardShortcuts + "." +
+            ConfigurationManager::PartType;
 
-    const std::string KeyboardShortcutsFile =
-        ConfigurationManager::KeyKeyboardShortcuts + "." +
-        ConfigurationManager::PartFile;
+        const std::string KeyboardShortcutsFile =
+            ConfigurationManager::KeyKeyboardShortcuts + "." +
+            ConfigurationManager::PartFile;
 
-    std::string type, file;
-    bool hasType = configurationManager().getValue(KeyboardShortcutsType, type);
-    bool hasFile = configurationManager().getValue(KeyboardShortcutsFile, file);
+        std::string type, file;
+        const bool hasType = configurationManager().getValue(KeyboardShortcutsType, type);
+        const bool hasFile = configurationManager().getValue(KeyboardShortcutsFile, file);
 
-    if (hasType && hasFile) {
-        interactionHandler().writeKeyboardDocumentation(type, file);
+        if (hasType && hasFile) {
+            file = absPath(file);
+            interactionHandler().writeKeyboardDocumentation(type, file);
+        }
     }
 
     // If a PropertyDocumentationFile was specified, generate it now.
-    const std::string KeyPropertyDocumentationType =
-        ConfigurationManager::KeyPropertyDocumentation + '.' +
-        ConfigurationManager::PartType;
+    {    
+        const std::string KeyPropertyDocumentationType =
+            ConfigurationManager::KeyPropertyDocumentation + '.' +
+            ConfigurationManager::PartType;
 
-    const std::string KeyPropertyDocumentationFile =
-        ConfigurationManager::KeyPropertyDocumentation + '.' +
-        ConfigurationManager::PartFile;
+        const std::string KeyPropertyDocumentationFile =
+            ConfigurationManager::KeyPropertyDocumentation + '.' +
+            ConfigurationManager::PartFile;
 
-    hasType = configurationManager().hasKey(KeyPropertyDocumentationType);
-    hasFile = configurationManager().hasKey(KeyPropertyDocumentationFile);
+        std::string type, file;
+        const bool hasType = configurationManager().getValue(KeyPropertyDocumentationType, type);
+        const bool hasFile = configurationManager().getValue(KeyPropertyDocumentationFile, file);
 
-    if (hasType && hasFile) {
-        std::string propertyDocumentationType;
-        OsEng.configurationManager().getValue(KeyPropertyDocumentationType, propertyDocumentationType);
-        std::string propertyDocumentationFile;
-        OsEng.configurationManager().getValue(KeyPropertyDocumentationFile, propertyDocumentationFile);
-
-        propertyDocumentationFile = absPath(propertyDocumentationFile);
-        scene->writePropertyDocumentation(propertyDocumentationFile, propertyDocumentationType, scenePath);
+        if (hasType && hasFile) {
+            file = absPath(file);
+            scene->writePropertyDocumentation(file, type, scenePath);
+        }
     }
 
     _renderEngine->setGlobalBlackOutFactor(0.0);
@@ -934,9 +934,9 @@ void OpenSpaceEngine::preSynchronization() {
     LTRACE("OpenSpaceEngine::preSynchronization(begin)");
     FileSys.triggerFilesystemEvents();
 
-    if (_switchScene) {
+    if (_scheduledSceneSwitch) {
         loadScene(_scenePath);
-        _switchScene = false;
+        _scheduledSceneSwitch = false;
     }
 
     if (_isFirstRenderingFirstFrame) {

src/engine/settingsengine.cpp

@@ -70,8 +70,9 @@ SettingsEngine::SettingsEngine()
 void SettingsEngine::initialize() {
     // Load all matching files in the Scene
     // TODO: match regex with either with new ghoul readFiles or local code
-    std::string sceneDir = "${SCENE}";
-    std::vector<std::string> scenes = ghoul::filesystem::Directory(sceneDir).readFiles();
+    const std::string sceneDir = "${SCENE}";
+    const std::vector<std::string> scenes = ghoul::filesystem::Directory(sceneDir).readFiles();
+
     for (std::size_t i = 0; i < scenes.size(); ++i) {
         std::size_t found = scenes[i].find_last_of("/\\");
         _scenes.addOption(static_cast<int>(i), scenes[i].substr(found + 1));
@@ -79,11 +80,11 @@ void SettingsEngine::initialize() {
 
     // Set interaction to change ConfigurationManager and schedule the load
     _scenes.onChange(
-        [this]() {
+        [this, sceneDir]() {
         std::string sceneFile = _scenes.getDescriptionByValue(_scenes);
         OsEng.configurationManager().setValue(
             ConfigurationManager::KeyConfigScene, sceneFile);
-        OsEng.scheduleLoadScene("${SCENE}/" + sceneFile);
+        OsEng.scheduleLoadScene(sceneDir + "/" + sceneFile);
     }
     );
 }

src/scene/scene.cpp

@@ -103,7 +103,7 @@ Camera* Scene::camera() const {
     return _camera.get();
 }
 
-void Scene::addNode(SceneGraphNode* node, Scene::UpdateDependencies updateDeps) {
+void Scene::addNode(SceneGraphNode* node, UpdateDependencies updateDeps) {
     // Add the node and all its children.
     node->traversePreOrder([this](SceneGraphNode* n) {
         _topologicallySortedNodes.push_back(n);
@@ -115,7 +115,7 @@ void Scene::addNode(SceneGraphNode* node, Scene::UpdateDependencies updateDeps)
     }
 }
 
-void Scene::removeNode(SceneGraphNode* node, Scene::UpdateDependencies updateDeps) {
+void Scene::removeNode(SceneGraphNode* node, UpdateDependencies updateDeps) {
     // Remove the node and all its children.
     node->traversePostOrder([this](SceneGraphNode* node) {
         _topologicallySortedNodes.erase(
@@ -135,7 +135,11 @@ void Scene::updateDependencies() {
 }
 
 void Scene::sortTopologically() {
-    std::copy(_circularNodes.begin(), _circularNodes.end(), std::back_inserter(_topologicallySortedNodes));
+    _topologicallySortedNodes.insert(
+        _topologicallySortedNodes.end(),
+        std::make_move_iterator(_circularNodes.begin()),
+        std::make_move_iterator(_circularNodes.end())
+    );
     _circularNodes.clear();
 
     ghoul_assert(_topologicallySortedNodes.size() == _nodesByName.size(), "Number of scene graph nodes is inconsistent");
@@ -168,7 +172,7 @@ void Scene::sortTopologically() {
         nodes.push_back(node);
         zeroInDegreeNodes.pop();
 
-        for (auto& n : node->dependentNodes()) {
+        for (SceneGraphNode* n : node->dependentNodes()) {
             auto it = inDegrees.find(n);
             it->second -= 1;
             if (it->second == 0) {
@@ -176,7 +180,7 @@ void Scene::sortTopologically() {
                 inDegrees.erase(it);
             }
         }
-        for (auto& n : node->children()) {
+        for (SceneGraphNode* n : node->children()) {
             auto it = inDegrees.find(n);
             it->second -= 1;
             if (it->second == 0) {
@@ -189,7 +193,7 @@ void Scene::sortTopologically() {
         LERROR("The scene contains circular dependencies. " << inDegrees.size() << " nodes will be disabled.");
     }
 
-    for (auto& it : inDegrees) {
+    for (auto it : inDegrees) {
         _circularNodes.push_back(it.first);
     }
     
@@ -212,7 +216,7 @@ void Scene::initialize() {
 }
 
 void Scene::update(const UpdateData& data) {
-    for (auto& node : _topologicallySortedNodes) {
+    for (SceneGraphNode* node : _topologicallySortedNodes) {
         try {
             LTRACE("Scene::update(begin '" + node->name() + "')");
             node->update(data);
@@ -225,7 +229,7 @@ void Scene::update(const UpdateData& data) {
 }
 
 void Scene::evaluate(Camera* camera) {
-    for (auto& node : _topologicallySortedNodes) {
+    for (SceneGraphNode* node : _topologicallySortedNodes) {
         try {
             LTRACE("Scene::evaluate(begin '" + node->name() + "')");
             node->evaluate(camera);
@@ -238,7 +242,7 @@ void Scene::evaluate(Camera* camera) {
 }
 
 void Scene::render(const RenderData& data, RendererTasks& tasks) {
-    for (auto& node : _topologicallySortedNodes) {
+    for (SceneGraphNode* node : _topologicallySortedNodes) {
         try {
             LTRACE("Scene::render(begin '" + node->name() + "')");
             node->render(data, tasks);

src/scene/scenegraphnode.cpp

@@ -459,7 +459,7 @@ void SceneGraphNode::removeDependency(SceneGraphNode& dependency, UpdateScene up
 }
 
 void SceneGraphNode::clearDependencies(UpdateScene updateScene) {
-    for (auto& dependency : _dependencies) {
+    for (auto dependency : _dependencies) {
         dependency->_dependentNodes.erase(std::remove_if(
             dependency->_dependentNodes.begin(),
             dependency->_dependentNodes.end(),
@@ -479,7 +479,7 @@ void SceneGraphNode::setDependencies(const std::vector<SceneGraphNode*>& depende
     clearDependencies(UpdateScene::No);
 
     _dependencies = dependencies;
-    for (auto& dependency : dependencies) {
+    for (auto dependency : dependencies) {
         dependency->_dependentNodes.push_back(this);
     }
 

src/scene/sceneloader.cpp

@@ -34,7 +34,6 @@
 #include <ghoul/misc/onscopeexit.h>
 
 #include <unordered_set>
-#include <memory>
 
 namespace {
     const std::string _loggerCat = "SceneLoader";

src/scene/scenemanager.cpp

@@ -22,11 +22,12 @@
  * OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.                                         *
  ****************************************************************************************/
 
-#include <openspace/scene/scenemanager.h>
+#include <openspace/scene/scene.h>
 #include <openspace/scene/sceneloader.h>
+#include <openspace/scene/scenemanager.h>
+
 #include <algorithm>
 #include <memory>
-#include <openspace/scene/scene.h>
 
 namespace openspace {