Some cleanup of ModuleEngine

Some cleanup of SettingsEngine
Some cleanup of SyncEngine

include/openspace/engine/moduleengine.h

@@ -25,14 +25,23 @@
 #ifndef __OPENSPACE_CORE___MODULEENGINE___H__
 #define __OPENSPACE_CORE___MODULEENGINE___H__
 
-#include <openspace/util/openspacemodule.h>
-#include <openspace/scripting/scriptengine.h>
-
 #include <memory>
 #include <vector>
 
+namespace ghoul {
+namespace systemcapabilities {
+
+struct Version;
+
+} // namespace systemcapabilities
+} // namespace ghoul
+
 namespace openspace {
 
+namespace scripting { struct LuaLibrary; }
+
+class OpenSpaceModule;
+
 /**
  * The ModuleEngine is the central repository for registering and accessing
  * OpenSpaceModule for the current application run. By initializing (#initialize) the
@@ -80,8 +89,7 @@ public:
      * version of all registered modules' OpenGL versions.
      * \return The combined minimum OpenGL version
      */
-    ghoul::systemcapabilities::OpenGLCapabilitiesComponent::Version
-        requiredOpenGLVersion() const;
+    ghoul::systemcapabilities::Version requiredOpenGLVersion() const;
 
     /**
     * Returns the Lua library that contains all Lua functions available to affect the

include/openspace/engine/settingsengine.h

@@ -43,26 +43,19 @@ public:
 
     void initialize();
     
-    void setModules(std::vector<OpenSpaceModule*> modules);
+    void setModules(const std::vector<OpenSpaceModule*>& modules);
 
     bool busyWaitForDecode();
     bool logSGCTOutOfOrderErrors();
     bool useDoubleBuffering();
 
 private:
-    void initEyeSeparation();
-    void initSceneFiles();
-    void initBusyWaitForDecode();
-    void initLogSGCTOutOfOrderErrors();
-    void initUseDoubleBuffering();
-
     properties::FloatProperty _eyeSeparation;
     properties::OptionProperty _scenes;
     properties::BoolProperty _busyWaitForDecode;
     properties::BoolProperty _logSGCTOutOfOrderErrors;
     properties::BoolProperty _useDoubleBuffering;
     properties::BoolProperty _spiceUseExceptions;
-
 };
 
 } // namespace openspace

include/openspace/engine/syncengine.h

@@ -25,80 +25,82 @@
 #ifndef __OPENSPACE_CORE___SYNCENGINE___H__
 #define __OPENSPACE_CORE___SYNCENGINE___H__
 
+#include <openspace/util/syncbuffer.h>
+
+#include <ghoul/misc/boolean.h>
+
 #include <vector>
 #include <memory>
 
 namespace openspace {
 
 class Syncable;
-class SyncBuffer;
 
 /**
-* Manages a collection of <code>Syncable</code>s and ensures they are synchronized
-* over SGCT nodes. Encoding/Decoding order is handles internally.
-*/
+ * Manages a collection of <code>Syncable</code>s and ensures they are synchronized
+ * over SGCT nodes. Encoding/Decoding order is handles internally.
+ */
 class SyncEngine {
 public:
+    using IsMaster = ghoul::Boolean;
 
     /**
-    * Dependency injection: a SyncEngine relies on a SyncBuffer to encode the sync data.
-    */
-    SyncEngine(SyncBuffer* syncBuffer);
-
+     * Creates a new SyncEngine which a buffer size of \p syncBufferSize
+     * \pre syncBufferSize must be bigger than 0
+     */
+    SyncEngine(unsigned int syncBufferSize);
 
     /**
-    * Encodes all added Syncables in the injected <code>SyncBuffer</code>. 
-    * This method is only called on the SGCT master node
-    */
+     * Encodes all added Syncables in the injected <code>SyncBuffer</code>. 
+     * This method is only called on the SGCT master node
+     */
     void encodeSyncables();
 
     /**
-    * Decodes the <code>SyncBuffer</code> into the added Syncables.
-    * This method is only called on the SGCT slave nodes
-    */
+     * Decodes the <code>SyncBuffer</code> into the added Syncables.
+     * This method is only called on the SGCT slave nodes
+     */
     void decodeSyncables();
 
     /**
-    * Invokes the presync method of all added Syncables
-    */
-    void presync(bool isMaster);
+     * Invokes the presync method of all added Syncables
+     */
+    void preSynchronization(IsMaster isMaster);
 
     /**
-    * Invokes the postsync method of all added Syncables
-    */
-    void postsync(bool isMaster);
+     * Invokes the postsync method of all added Syncables
+     */
+    void postSynchronization(IsMaster isMaster);
     
-
-
     /**
-    * Add a Syncable to be synchronized over the SGCT cluster
-    */
+     * Add a Syncable to be synchronized over the SGCT cluster.
+     * \pre syncable must not be nullptr
+     */
     void addSyncable(Syncable* syncable);
 
     /**
-    * Add multiple Syncables to be synchronized over the SGCT cluster
-    */
+     * Add multiple Syncables to be synchronized over the SGCT cluster
+     * \pre syncables must not contain any nullptr
+     */
     void addSyncables(const std::vector<Syncable*>& syncables);
 
     /**
-    * Remove a Syncable from being synchronized over the SGCT cluster
-    */
+     * Remove a Syncable from being synchronized over the SGCT cluster
+     */
     void removeSyncable(Syncable* syncable);
 
 private:
-    
     /** 
-    * Vector of Syncables. The vectors ensures consistent encode/decode order
-    */
+     * Vector of Syncables. The vectors ensures consistent encode/decode order
+     */
     std::vector<Syncable*> _syncables;
 
     /**
-    * Databuffer used in encoding/decoding
-    */
-    std::unique_ptr<SyncBuffer> _syncBuffer;
+     * Databuffer used in encoding/decoding
+     */
+    SyncBuffer _syncBuffer;
 };
 
-
 } // namespace openspace
 
 #endif // __OPENSPACE_CORE___SYNCENGINE___H__

include/openspace/util/openspacemodule.h

@@ -29,7 +29,7 @@
 
 #include <openspace/scripting/lualibrary.h>
 
-#include <ghoul/systemcapabilities/openglcapabilitiescomponent.h>
+#include <ghoul/systemcapabilities/version.h>
 
 #include <string>
 #include <vector>
@@ -87,8 +87,7 @@ public:
      * overwritten, it returns an OpenGL version of <code>3.3</code>.
      * \return The minimum required OpenGL version of this OpenSpaceModule
      */
-    virtual ghoul::systemcapabilities::OpenGLCapabilitiesComponent::Version
-        requiredOpenGLVersion() const;
+    virtual ghoul::systemcapabilities::Version requiredOpenGLVersion() const;
 
 protected:
     /**

src/engine/moduleengine.cpp

@@ -25,6 +25,7 @@
 #include <openspace/engine/moduleengine.h>
 
 #include <openspace/moduleregistration.h>
+#include <openspace/scripting/lualibrary.h>
 #include <openspace/util/openspacemodule.h>
 
 #include <ghoul/logging/logmanager.h>
@@ -34,7 +35,7 @@
 #include "moduleengine_lua.inl"
 
 namespace {
-    const std::string _loggerCat = "ModuleEngine";
+    const char* _loggerCat = "ModuleEngine";
 }
 
 namespace openspace {
@@ -67,7 +68,8 @@ void ModuleEngine::registerModule(std::unique_ptr<OpenSpaceModule> module) {
     );
     if (it != _modules.end()) {
         throw ghoul::RuntimeError(
-            "Module name '" + module->name() + "' was registered before", "ModuleEngine"
+            "Module name '" + module->name() + "' was registered before",
+            "ModuleEngine"
         );
     }
     
@@ -85,11 +87,8 @@ std::vector<OpenSpaceModule*> ModuleEngine::modules() const {
     return result;
 }
 
-ghoul::systemcapabilities::OpenGLCapabilitiesComponent::Version
-ModuleEngine::requiredOpenGLVersion() const
-{
-    using Version = ghoul::systemcapabilities::OpenGLCapabilitiesComponent::Version;
-    Version version = { 0,0 };
+ghoul::systemcapabilities::Version ModuleEngine::requiredOpenGLVersion() const {
+    ghoul::systemcapabilities::Version version = { 0, 0 };
 
     for (const auto& m : _modules) {
         version = std::max(version, m->requiredOpenGLVersion());

src/engine/openspaceengine.cpp

@@ -51,6 +51,7 @@
 #include <openspace/scene/scene.h>
 #include <openspace/scene/translation.h>
 #include <openspace/util/factorymanager.h>
+#include <openspace/util/openspacemodule.h>
 #include <openspace/util/time.h>
 #include <openspace/util/timemanager.h>
 #include <openspace/util/spicemanager.h>
@@ -128,7 +129,7 @@ OpenSpaceEngine::OpenSpaceEngine(std::string programName,
     , _scriptEngine(new scripting::ScriptEngine)
     , _scriptScheduler(new scripting::ScriptScheduler)
     , _networkEngine(new NetworkEngine)
-    , _syncEngine(std::make_unique<SyncEngine>(new SyncBuffer(4096)))
+    , _syncEngine(std::make_unique<SyncEngine>(4096))
     , _commandlineParser(new ghoul::cmdparser::CommandlineParser(
         programName, ghoul::cmdparser::CommandlineParser::AllowUnknownCommands::Yes
       ))
@@ -448,9 +449,9 @@ void OpenSpaceEngine::initialize() {
     SysCap.logCapabilities(verbosity);
 
     // Check the required OpenGL versions of the registered modules
-    ghoul::systemcapabilities::OpenGLCapabilitiesComponent::Version version =
+    ghoul::systemcapabilities::Version version =
         _engine->_moduleEngine->requiredOpenGLVersion();
-    LINFO("Required OpenGL version: " << version.toString());
+    LINFO("Required OpenGL version: " << std::to_string(version));
 
     if (OpenGLCap.openGLVersion() < version) {
         throw ghoul::RuntimeError(
@@ -845,7 +846,7 @@ void OpenSpaceEngine::preSynchronization() {
     
     bool master = _windowWrapper->isMaster();
     
-    _syncEngine->presync(master);
+    _syncEngine->preSynchronization(SyncEngine::IsMaster(master));
     if (master) {
         double dt = _windowWrapper->averageDeltaTime();
         _timeManager->preSynchronization(dt);
@@ -877,7 +878,7 @@ void OpenSpaceEngine::postSynchronizationPreDraw() {
     LTRACE("OpenSpaceEngine::postSynchronizationPreDraw(begin)");
     
     bool master = _windowWrapper->isMaster();
-    _syncEngine->postsync(master);
+    _syncEngine->postSynchronization(SyncEngine::IsMaster(master));
 
     if (_shutdown.inShutdown) {
         if (_shutdown.timer <= 0.f) {

src/engine/settingsengine.cpp

@@ -40,13 +40,13 @@
 
 
 namespace {
-    const std::string _loggerCat = "SettingsEngine";
+    const char* _loggerCat = "SettingsEngine";
 }
 
 namespace openspace {
 
 SettingsEngine::SettingsEngine()
-    : _eyeSeparation("eyeSeparation", "Eye Separation" , 0.f, 0.f, 10.f)
+    : _eyeSeparation("eyeSeparation", "Eye Separation", 0.f, 0.f, 10.f)
     , _scenes("scenes", "Scene", properties::OptionProperty::DisplayType::Dropdown)
     , _busyWaitForDecode("busyWaitForDecode", "Busy Wait for decode", false)
     , _logSGCTOutOfOrderErrors("logSGCTOutOfOrderErrors", "Log SGCT out-of-order", false)
@@ -55,79 +55,26 @@ SettingsEngine::SettingsEngine()
 {
     setName("Global Properties");
 
-    _spiceUseExceptions.onChange([this]{
+    _spiceUseExceptions.onChange([this] {
         if (_spiceUseExceptions) {
             SpiceManager::ref().setExceptionHandling(SpiceManager::UseException::Yes);
-        }
-        else {
+        } else {
             SpiceManager::ref().setExceptionHandling(SpiceManager::UseException::No);
         }
     });
     addProperty(_spiceUseExceptions);
+    addProperty(_eyeSeparation);
+    addProperty(_busyWaitForDecode);
+    addProperty(_logSGCTOutOfOrderErrors);
+    addProperty(_useDoubleBuffering);
+    addProperty(_scenes);
 }
 
 void SettingsEngine::initialize() {
-    initEyeSeparation();
-    initSceneFiles();
-    initBusyWaitForDecode();
-    initLogSGCTOutOfOrderErrors();
-    initUseDoubleBuffering();
-}
-    
-void SettingsEngine::setModules(std::vector<OpenSpaceModule*> modules) {
-    for (OpenSpaceModule* m : modules) {
-        addPropertySubOwner(m);
-    }
-}
-
-void SettingsEngine::initEyeSeparation() {
-    addProperty(_eyeSeparation);
-
     // Set interaction to change the window's (SGCT's) eye separation
     _eyeSeparation.onChange(
-        [this]() { OsEng.windowWrapper().setEyeSeparationDistance(_eyeSeparation); });
-}
-
-void SettingsEngine::initBusyWaitForDecode() {
-    addProperty(_busyWaitForDecode);
-    _busyWaitForDecode.onChange(
-        [this]() { 
-        LINFO((_busyWaitForDecode.value() ? "Busy wait for decode" : "Async decode"));
-    });
-}
-
-bool SettingsEngine::busyWaitForDecode() {
-    return _busyWaitForDecode.value();
-}
-
-void SettingsEngine::initLogSGCTOutOfOrderErrors() {
-    addProperty(_logSGCTOutOfOrderErrors);
-    _logSGCTOutOfOrderErrors.onChange(
-        [this]() {
-        LINFO("Turn " << (_logSGCTOutOfOrderErrors.value() ? "on" : "off") << " SGCT out of order logging");
-    });
-}
-
-bool SettingsEngine::logSGCTOutOfOrderErrors() {
-    return _logSGCTOutOfOrderErrors.value();
-}
-
-
-void SettingsEngine::initUseDoubleBuffering() {
-    addProperty(_useDoubleBuffering);
-    _useDoubleBuffering.onChange(
-        [this]() {
-        LINFO("Turn " << (_useDoubleBuffering.value() ? "on" : "off") << " double buffering");
-    });
-}
-
-
-bool SettingsEngine::useDoubleBuffering() {
-    return _useDoubleBuffering.value();
-}
-
-void SettingsEngine::initSceneFiles() {
-    addProperty(_scenes);
+        [this]() { OsEng.windowWrapper().setEyeSeparationDistance(_eyeSeparation); }
+    );
 
     // Load all matching files in the Scene
     // TODO: match regex with either with new ghoul readFiles or local code
@@ -135,18 +82,36 @@ void SettingsEngine::initSceneFiles() {
     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(i, scenes[i].substr(found+1));
+        _scenes.addOption(i, scenes[i].substr(found + 1));
     }
 
     // Set interaction to change ConfigurationManager and schedule the load
     _scenes.onChange(
         [this]() {
-            std::string sceneFile = _scenes.getDescriptionByValue(_scenes);
-            OsEng.configurationManager().setValue(
-                ConfigurationManager::KeyConfigScene, sceneFile);
-            OsEng.renderEngine().scene()->scheduleLoadSceneFile(sceneFile);
-        }
+        std::string sceneFile = _scenes.getDescriptionByValue(_scenes);
+        OsEng.configurationManager().setValue(
+            ConfigurationManager::KeyConfigScene, sceneFile);
+        OsEng.renderEngine().scene()->scheduleLoadSceneFile(sceneFile);
+    }
     );
 }
 
+void SettingsEngine::setModules(const std::vector<OpenSpaceModule*>& modules) {
+    for (OpenSpaceModule* m : modules) {
+        addPropertySubOwner(m);
+    }
 }
+
+bool SettingsEngine::busyWaitForDecode() {
+    return _busyWaitForDecode.value();
+}
+
+bool SettingsEngine::logSGCTOutOfOrderErrors() {
+    return _logSGCTOutOfOrderErrors.value();
+}
+
+bool SettingsEngine::useDoubleBuffering() {
+    return _useDoubleBuffering.value();
+}
+
+} // namespace openspace

src/engine/syncengine.cpp

@@ -23,73 +23,67 @@
  ****************************************************************************************/
 
 #include <openspace/engine/syncengine.h>
+
 #include <openspace/util/syncdata.h>
-#include <openspace/util/syncbuffer.h>
-#include <ghoul/logging/logmanager.h>
+
+#include <ghoul/misc/assert.h>
 
 #include <algorithm>
-#include <string>
-
-
-namespace {
-    const std::string _loggerCat = "SyncEngine";
-}
-
 
 namespace openspace {
 
-    SyncEngine::SyncEngine(SyncBuffer* syncBuffer) 
-        : _syncBuffer(syncBuffer)
-    {
-
-    }
-
-
-    void SyncEngine::presync(bool isMaster) {
-        for (const auto& syncable : _syncables) {
-            syncable->presync(isMaster);
-        }
-    }
-
-    // should be called on sgct master
-    void SyncEngine::encodeSyncables() {
-        for (const auto& syncable : _syncables) {
-            syncable->encode(_syncBuffer.get());
-        }
-        _syncBuffer->write();
-    }
-
-    //should be called on sgct slaves
-    void SyncEngine::decodeSyncables() {
-        _syncBuffer->read();
-        for (const auto& syncable : _syncables) {
-            syncable->decode(_syncBuffer.get());
-        }
-    }
-
-    void SyncEngine::postsync(bool isMaster) {
-        for (const auto& syncable : _syncables) {
-            syncable->postsync(isMaster);
-        }
-    }
-
-
-
-    void SyncEngine::addSyncable(Syncable*  syncable) {
-        _syncables.push_back(syncable);
-    }
-
-    void SyncEngine::addSyncables(const std::vector<Syncable*>& syncables) {
-        for (const auto& syncable : syncables) {
-            addSyncable(syncable);
-        }
-    }
-
-    void SyncEngine::removeSyncable(Syncable* syncable) {
-        _syncables.erase(
-            std::remove(_syncables.begin(), _syncables.end(), syncable),
-            _syncables.end()
-        );
-    }
-
+SyncEngine::SyncEngine(unsigned int syncBufferSize) 
+    : _syncBuffer(syncBufferSize)
+{
+    ghoul_assert(syncBufferSize > 0, "syncBufferSize must be bigger than 0");
 }
+
+// should be called on sgct master
+void SyncEngine::encodeSyncables() {
+    for (Syncable* syncable : _syncables) {
+        syncable->encode(&_syncBuffer);
+    }
+    _syncBuffer.write();
+}
+
+//should be called on sgct slaves
+void SyncEngine::decodeSyncables() {
+    _syncBuffer.read();
+    for (Syncable* syncable : _syncables) {
+        syncable->decode(&_syncBuffer);
+    }
+}
+
+void SyncEngine::preSynchronization(IsMaster isMaster) {
+    for (Syncable* syncable : _syncables) {
+        syncable->presync(isMaster);
+    }
+}
+
+void SyncEngine::postSynchronization(IsMaster isMaster) {
+    for (Syncable* syncable : _syncables) {
+        syncable->postsync(isMaster);
+    }
+}
+
+void SyncEngine::addSyncable(Syncable* syncable) {
+    ghoul_assert(syncable, "synable must not be nullptr");
+
+    _syncables.push_back(syncable);
+}
+
+void SyncEngine::addSyncables(const std::vector<Syncable*>& syncables) {
+    for (Syncable* syncable : syncables) {
+        ghoul_assert(syncable, "syncables must not contain any nullptr");
+        addSyncable(syncable);
+    }
+}
+
+void SyncEngine::removeSyncable(Syncable* syncable) {
+    _syncables.erase(
+        std::remove(_syncables.begin(), _syncables.end(), syncable),
+        _syncables.end()
+    );
+}
+
+} // namespace openspace

src/rendering/renderengine.cpp

@@ -186,7 +186,7 @@ void RenderEngine::initialize() {
     if (confManager.hasKeyAndValue<std::string>(KeyRenderingMethod)) {
         renderingMethod = confManager.value<std::string>(KeyRenderingMethod);
     } else {
-        using Version = ghoul::systemcapabilities::OpenGLCapabilitiesComponent::Version;
+        using Version = ghoul::systemcapabilities::Version;
 
         // The default rendering method has a requirement of OpenGL 4.3, so if we are
         // below that, we will fall back to frame buffer operation
@@ -556,7 +556,9 @@ void RenderEngine::postDraw() {
     }
 
     if (_performanceManager) {
-        _performanceManager->storeScenePerformanceMeasurements(scene()->allSceneGraphNodes());
+        _performanceManager->storeScenePerformanceMeasurements(
+            scene()->allSceneGraphNodes()
+        );
     }
 }
 

src/scripting/systemcapabilitiesbinding.cpp

@@ -111,7 +111,7 @@ int hasOpenGLVersion(lua_State* L) {
     int major = std::stoi(v[0]);
     int minor = std::stoi(v[1]);
     int release = v.size() == 3 ? std::stoi(v[2]) : 0;
-    OpenGLCapabilitiesComponent::Version version = { major, minor, release };
+    Version version = { major, minor, release };
     
     bool supported = OpenGLCap.openGLVersion() >= version;
     
@@ -121,7 +121,7 @@ int hasOpenGLVersion(lua_State* L) {
 }
     
 int openGLVersion(lua_State* L) {
-    lua_pushstring(L, OpenGLCap.openGLVersion().toString().c_str());
+    lua_pushstring(L, std::to_string(OpenGLCap.openGLVersion()).c_str());
     return 1;
 }
     

src/util/openspacemodule.cpp

@@ -73,9 +73,7 @@ scripting::LuaLibrary OpenSpaceModule::luaLibrary() const {
     return {};
 }
 
-ghoul::systemcapabilities::OpenGLCapabilitiesComponent::Version
-OpenSpaceModule::requiredOpenGLVersion() const
-{
+ghoul::systemcapabilities::Version OpenSpaceModule::requiredOpenGLVersion() const {
     return { 3, 3 };
 }