From 51be4b1788650b753c8474a4f8f261dfe46ead9b Mon Sep 17 00:00:00 2001 From: Alexander Bock Date: Fri, 19 Sep 2014 00:29:36 +0200 Subject: [PATCH] Make use of new Dictionary::getValueSafe method Clean up code in various places --- ext/ghoul | 2 +- src/engine/openspaceengine.cpp | 67 +++++---- src/properties/property.cpp | 14 +- src/rendering/planets/planetgeometry.cpp | 17 +-- src/rendering/planets/renderableplanet.cpp | 16 ++- .../planets/simplespheregeometry.cpp | 49 ++++--- src/rendering/renderable.cpp | 21 +-- src/rendering/renderablefieldlines.cpp | 127 +++++++++++------- src/scenegraph/scenegraphnode.cpp | 2 + 9 files changed, 172 insertions(+), 143 deletions(-) diff --git a/ext/ghoul b/ext/ghoul index 7906906863..d0cc759daa 160000 --- a/ext/ghoul +++ b/ext/ghoul @@ -1 +1 @@ -Subproject commit 79069068634b5a582d7fed49eb857dcc490471d3 +Subproject commit d0cc759daaedbd199b91aad5ce398b58f2084557 diff --git a/src/engine/openspaceengine.cpp b/src/engine/openspaceengine.cpp index b1571563fe..6d0a9d518c 100644 --- a/src/engine/openspaceengine.cpp +++ b/src/engine/openspaceengine.cpp @@ -160,7 +160,6 @@ bool OpenSpaceEngine::registerBasePathFromConfigurationFile(const std::string& f bool OpenSpaceEngine::findConfiguration(std::string& filename) { - //return FileSys.fileExists(filename); std::string currentDirectory = FileSys.absolutePath(FileSys.currentDirectory()); size_t occurrences = std::count(currentDirectory.begin(), currentDirectory.end(), ghoul::filesystem::FileSystem::PathSeparator); @@ -249,13 +248,10 @@ bool OpenSpaceEngine::create(int argc, char** argv, LDEBUG("Loading configuration from disk"); ghoul::Dictionary& configuration = _engine->configurationManager(); ghoul::lua::loadDictionaryFromFile(configurationFilePath, configuration); - const bool hasKey = configuration.hasKey(constants::openspaceengine::keyPaths); - const bool hasValue = configuration.hasValue(constants::openspaceengine::keyPaths); - if (hasKey && hasValue) { - ghoul::Dictionary pathsDictionary; - configuration.getValue(constants::openspaceengine::keyPaths, pathsDictionary); + ghoul::Dictionary pathsDictionary; + const bool success = configuration.getValueSafe(constants::openspaceengine::keyPaths, pathsDictionary); + if (success) OpenSpaceEngine::registerPathsFromDictionary(pathsDictionary); - } else { LFATAL("Configuration file does not contain paths token '" << constants::openspaceengine::keyPaths << "'"); return false; @@ -264,10 +260,9 @@ bool OpenSpaceEngine::create(int argc, char** argv, // Determining SGCT configuration file LDEBUG("Determining SGCT configuration file"); - std::string sgctConfigurationPath = _sgctDefaultConfigFile; - if (configuration.hasKey(constants::openspaceengine::keyConfigSgct)) - configuration.getValue(constants::openspaceengine::keyConfigSgct, sgctConfigurationPath); - + std::string sgctConfigurationPath = _sgctDefaultConfigFile; + configuration.getValueSafe(constants::openspaceengine::keyConfigSgct, + sgctConfigurationPath); // Prepend the outgoing sgctArguments with the program name // as well as the configuration file that sgct is supposed to use @@ -296,8 +291,8 @@ bool OpenSpaceEngine::initialize() glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT); GLFWwindow* win = sgct::Engine::instance()->getActiveWindowPtr()->getWindowHandle(); glfwSwapBuffers(win); - int samples = sqrt(sgct::Engine::instance()->getActiveWindowPtr()->getNumberOfAASamples()); - LDEBUG("samples: " << samples); + //int samples = sqrt(sgct::Engine::instance()->getActiveWindowPtr()->getNumberOfAASamples()); + //LDEBUG("samples: " << samples); int x1, xSize, y1, ySize; sgct::Engine::instance()->getActiveWindowPtr()->getCurrentViewportPixelCoords(x1, y1, xSize, ySize); @@ -323,11 +318,8 @@ bool OpenSpaceEngine::initialize() SysCap.detectCapabilities(); SysCap.logCapabilities(); - std::string timeKernel = ""; - using constants::openspaceengine::keyConfigTimekernel; - if (OsEng.configurationManager().hasKeyAndValue(keyConfigTimekernel)) { - OsEng.configurationManager().getValue(keyConfigTimekernel, timeKernel); - } + std::string timeKernel; + OsEng.configurationManager().getValueSafe(constants::openspaceengine::keyConfigTimekernel, timeKernel); // initialize OpenSpace helpers SpiceManager::initialize(); @@ -347,25 +339,27 @@ bool OpenSpaceEngine::initialize() // Load scenegraph SceneGraph* sceneGraph = new SceneGraph; _renderEngine->setSceneGraph(sceneGraph); - if (!OsEng.configurationManager().hasValue( - constants::openspaceengine::keyConfigScene)) { - LFATAL("Configuration needs to point to the scene file"); + + std::string sceneDescriptionPath; + bool success = OsEng.configurationManager().getValueSafe( + constants::openspaceengine::keyConfigScene, sceneDescriptionPath); + if (!success) { + LFATAL("The configuration does not contain a scene file under key '" << + constants::openspaceengine::keyConfigScene << "'"); return false; } - std::string sceneDescriptionPath; - bool success = _configurationManager->getValue( - constants::openspaceengine::keyConfigScene, sceneDescriptionPath); - - if (!FileSys.fileExists(sceneDescriptionPath)) { - LFATAL("Could not find '" << sceneDescriptionPath << "'"); + if (!FileSys.fileExists(sceneDescriptionPath)) { + LFATAL("Could not find scene description '" << sceneDescriptionPath << "'"); return false; } std::string scenePath; - success = _configurationManager->getValue(constants::openspaceengine::keyPathScene, scenePath); + success = _configurationManager->getValueSafe( + constants::openspaceengine::keyPathScene, scenePath); if (!success) { - LFATAL("Could not find SCENEPATH key in configuration file"); + LFATAL("Could not find key '" << constants::openspaceengine::keyPathScene << + "' in configuration file '" << sceneDescriptionPath << "'"); return false; } @@ -385,13 +379,12 @@ bool OpenSpaceEngine::initialize() _engine->_interactionHandler->connectDevices(); // Run start up scripts - using ghoul::Dictionary; - using constants::openspaceengine::keyStartupScript; - const bool hasScript = _engine->configurationManager().hasKeyAndValue(keyStartupScript); - if (hasScript) { - Dictionary scripts; - _engine->configurationManager().getValue(keyStartupScript, scripts); - + //using ghoul::Dictionary; + //using constants::openspaceengine::keyStartupScript; + ghoul::Dictionary scripts; + success = _engine->configurationManager().getValueSafe( + constants::openspaceengine::keyStartupScript, scripts); + if (success) { for (size_t i = 0; i < scripts.size(); ++i) { std::stringstream stream; // Dictionary-size is 0-based; script numbers are 1-based @@ -405,7 +398,7 @@ bool OpenSpaceEngine::initialize() std::string scriptPath; scripts.getValue(key, scriptPath); - const std::string absoluteScriptPath = absPath(scriptPath); + std::string&& absoluteScriptPath = absPath(scriptPath); _engine->scriptEngine().runScriptFile(absoluteScriptPath); } } diff --git a/src/properties/property.cpp b/src/properties/property.cpp index b225be2861..0b5004a281 100644 --- a/src/properties/property.cpp +++ b/src/properties/property.cpp @@ -30,6 +30,7 @@ namespace openspace { namespace properties { namespace { + const std::string _loggerCat = "Property"; const std::string _metaDataKeyGuiName = "guiName"; const std::string _metaDataKeyGroup = "group"; const std::string _metaDataKeyVisible = "isVisible"; @@ -47,6 +48,11 @@ const std::string Property::ViewOptions::PowerScaledScalar = "powerScaledScalar" Property::Property(std::string identifier, std::string guiName) : _identifier(std::move(identifier)) { + if (_identifier.empty()) + LWARNING("Property identifier is empty"); + if (guiName.empty()) + LWARNING("Property GUI name is empty"); + setVisible(true); _metaData.setValue(_metaDataKeyGuiName, std::move(guiName)); } @@ -80,8 +86,8 @@ int Property::typeLua() const { } const std::string& Property::guiName() const { - std::string result = ""; - _metaData.getValue(_metaDataKeyGuiName, result); + std::string result; + _metaData.getValueSafe(_metaDataKeyGuiName, result); return std::move(result); } @@ -90,8 +96,8 @@ void Property::setGroupIdentifier(std::string groupId) { } std::string Property::groupIdentifier() const { - std::string result = ""; - _metaData.getValue(_metaDataKeyGroup, result); + std::string result; + _metaData.getValueSafe(_metaDataKeyGroup, result); return std::move(result); } diff --git a/src/rendering/planets/planetgeometry.cpp b/src/rendering/planets/planetgeometry.cpp index ca6e361414..547984a8fe 100644 --- a/src/rendering/planets/planetgeometry.cpp +++ b/src/rendering/planets/planetgeometry.cpp @@ -36,17 +36,14 @@ namespace planetgeometry { PlanetGeometry* PlanetGeometry::createFromDictionary(const ghoul::Dictionary& dictionary) { - std::string name; - dictionary.getValue(constants::scenegraphnode::keyName, name); - - if (!dictionary.hasValue(constants::planetgeometry::keyType)) { - LERROR("PlanetGeometry for '" << name << "' did not contain a '" - << constants::planetgeometry::keyType << "' key"); + std::string geometryType; + const bool success = dictionary.getValueSafe( + constants::planetgeometry::keyType, geometryType); + if (!success) { + LERROR("PlanetGeometry did not contain a correct value of the key '" + << constants::planetgeometry::keyType << "'"); return nullptr; - } - std::string geometryType; - dictionary.getValue(constants::planetgeometry::keyType, geometryType); - + } ghoul::TemplateFactory* factory = FactoryManager::ref().factory(); diff --git a/src/rendering/planets/renderableplanet.cpp b/src/rendering/planets/renderableplanet.cpp index f6dd119ab2..4f0700a763 100644 --- a/src/rendering/planets/renderableplanet.cpp +++ b/src/rendering/planets/renderableplanet.cpp @@ -47,18 +47,22 @@ RenderablePlanet::RenderablePlanet(const ghoul::Dictionary& dictionary) , _texture(nullptr) , _geometry(nullptr) { + std::string name; + bool success = dictionary.getValue(constants::scenegraphnode::keyName, name); + assert(success); + std::string path; dictionary.getValue(constants::scenegraph::keyPathModule, path); - if (dictionary.hasKey(constants::renderableplanet::keyGeometry)) { - ghoul::Dictionary geometryDictionary; - dictionary.getValue(constants::renderableplanet::keyGeometry, geometryDictionary); + ghoul::Dictionary geometryDictionary; + success = dictionary.getValueSafe( + constants::renderableplanet::keyGeometry, geometryDictionary); + if (success) { + geometryDictionary.setValue(constants::scenegraphnode::keyName, name); geometryDictionary.setValue(constants::scenegraph::keyPathModule, path); - geometryDictionary.setValue(constants::scenegraphnode::keyName, name()); - _geometry = planetgeometry::PlanetGeometry::createFromDictionary(geometryDictionary); - } + } // TODO: textures need to be replaced by a good system similar to the geometry as soon // as the requirements are fixed (ab) diff --git a/src/rendering/planets/simplespheregeometry.cpp b/src/rendering/planets/simplespheregeometry.cpp index 5c544d49fd..e016f37f4c 100644 --- a/src/rendering/planets/simplespheregeometry.cpp +++ b/src/rendering/planets/simplespheregeometry.cpp @@ -47,38 +47,37 @@ SimpleSphereGeometry::SimpleSphereGeometry(const ghoul::Dictionary& dictionary) , _segments("segments", "Segments", 20, 1, 1000) , _planet(nullptr) { - if (!dictionary.hasValue(constants::simplespheregeometry::keyRadius)) { - std::string name; - dictionary.getValue(constants::scenegraphnode::keyName, name); - LERROR("SimpleSphereGeometry of '" << name << "' did not provide a key '" - << constants::simplespheregeometry::keyRadius - << "'"); - } - else { - glm::vec2 radius; - dictionary.getValue(constants::simplespheregeometry::keyRadius, radius); - _radius = radius; - } + using constants::scenegraphnode::keyName; + using constants::simplespheregeometry::keyRadius; + using constants::simplespheregeometry::keySegments; - if (!dictionary.hasValue(constants::simplespheregeometry::keySegments)) { - std::string name; - dictionary.getValue(constants::scenegraphnode::keyName, name); + // The name is passed down from the SceneGraphNode + std::string name; + bool success = dictionary.getValue(keyName, name); + assert(success); + + glm::vec2 radius; + success = dictionary.getValueSafe(keyRadius, radius); + if (!success) { LERROR("SimpleSphereGeometry of '" << name << "' did not provide a key '" - << constants::simplespheregeometry::keySegments - << "'"); - } - else { - float segments; - dictionary.getValue(constants::simplespheregeometry::keySegments, segments); - _segments = static_cast(segments); - } + << keyRadius << "'"); + } + else + _radius = radius; + + int segments; + success = dictionary.getValueSafe(keySegments, segments); + if (!success) { + LERROR("SimpleSphereGeometry of '" << name << "' did not provide a key '" + << keySegments << "'"); + } + else + _segments = segments; addProperty(_radius); _radius.onChange(std::bind(&SimpleSphereGeometry::createSphere, this)); addProperty(_segments); _segments.onChange(std::bind(&SimpleSphereGeometry::createSphere, this)); - - //createSphere(); } SimpleSphereGeometry::~SimpleSphereGeometry() diff --git a/src/rendering/renderable.cpp b/src/rendering/renderable.cpp index a81a6b1b33..7c497be81c 100644 --- a/src/rendering/renderable.cpp +++ b/src/rendering/renderable.cpp @@ -37,16 +37,19 @@ namespace openspace { Renderable* Renderable::createFromDictionary(const ghoul::Dictionary& dictionary) { + // The name is passed down from the SceneGraphNode std::string name; - dictionary.getValue(constants::scenegraphnode::keyName, name); + bool success = dictionary.getValue(constants::scenegraphnode::keyName, name); + assert(success); - if (!dictionary.hasValue(constants::renderable::keyType)) { + std::string renderableType; + success = dictionary.getValueSafe(constants::renderable::keyType, renderableType); + if (!success) { LERROR("Renderable '" << name << "' did not have key '" << constants::renderable::keyType << "'"); return nullptr; - } - std::string renderableType; - dictionary.getValue(constants::renderable::keyType, renderableType); + } + ghoul::TemplateFactory* factory = FactoryManager::ref().factory(); Renderable* result = factory->create(renderableType, dictionary); @@ -64,11 +67,9 @@ Renderable::Renderable(const ghoul::Dictionary& dictionary) setName("renderable"); // get path if available - _relativePath = ""; - if(dictionary.hasKey(constants::scenegraph::keyPathModule)) { - dictionary.getValue(constants::scenegraph::keyPathModule, _relativePath); - _relativePath += "/"; - } + const bool success = dictionary.getValueSafe(constants::scenegraph::keyPathModule, _relativePath); + if (success) + _relativePath += ghoul::filesystem::FileSystem::PathSeparator; addProperty(_enabled); } diff --git a/src/rendering/renderablefieldlines.cpp b/src/rendering/renderablefieldlines.cpp index c750ddcd9a..1817400413 100644 --- a/src/rendering/renderablefieldlines.cpp +++ b/src/rendering/renderablefieldlines.cpp @@ -27,70 +27,97 @@ #include #include +#include + namespace { - std::string _loggerCat = "RenderableFieldlines"; + const std::string _loggerCat = "RenderableFieldlines"; + + const std::string keyFieldlines = "Fieldlines"; + const std::string keyFilename = "File"; + const std::string keyHints = "Hints"; + const std::string keyShaders = "Shaders"; + const std::string keyVertexShader = "VertexShader"; + const std::string keyFragmentShader = "FragmentShader"; } namespace openspace { -RenderableFieldlines::RenderableFieldlines(const ghoul::Dictionary& dictionary) : - Renderable(dictionary), _VAO(0), _programUpdateOnSave(false),_update(false) { +RenderableFieldlines::RenderableFieldlines(const ghoul::Dictionary& dictionary) + : Renderable(dictionary) + , _VAO(0) + , _programUpdateOnSave(false) + , _update(false) +{ + std::string name; + bool success = dictionary.getValue(constants::scenegraphnode::keyName, name); + assert(success); - if(dictionary.hasKey("Fieldlines")) { - ghoul::Dictionary fieldlines; - if(dictionary.getValue("Fieldlines", fieldlines)) { - for(auto key: fieldlines.keys()) { - ghoul::Dictionary fieldline; - if(fieldlines.getValue(key, fieldline)) { - if (fieldline.hasKey("File")) { - std::string file = ""; - if (fieldline.getValue("File", file)) { - file = findPath(file); - if (file != "") { + ghoul::Dictionary fieldlines; + success = dictionary.getValueSafe(keyFieldlines, fieldlines); + if (!success) { + LERROR("RenderableFieldlines '" << name << "' did not contain a '" << + keyFieldlines << "' key"); + return; + } + for (const std::string& key : fieldlines.keys()) { + ghoul::Dictionary fieldline; + success = fieldlines.getValueSafe(key, fieldline); - // read hints into dictionary - ghoul::Dictionary hintsDictionary; - if(fieldline.hasKey("Hints")) - fieldline.getValue("Hints", hintsDictionary); + if (!success) { + LERROR("Key '" << key << "' in '" << keyFieldlines << + "' of the RenderableFieldlines '" << name << + "' does not have a table as value"); + continue; + } - _filenames.push_back(file); - _hintsDictionaries.push_back(hintsDictionary); - } else - LERROR("File not found!"); - } - } - } - } + std::string fileName; + fieldline.getValueSafe(keyFilename, fileName); + fileName = findPath(fileName); + if (fileName.empty()) + LERROR("File not found!"); + else { + ghoul::Dictionary hintsDictionary; + fieldline.getValueSafe(keyHints, hintsDictionary); + + _filenames.push_back(fileName); + _hintsDictionaries.push_back(hintsDictionary); } } - std::string vshaderpath = ""; - std::string fshaderpath = ""; - - if (dictionary.hasKey("Shaders")) { - ghoul::Dictionary shaderDictionary; - if(dictionary.getValue("Shaders", shaderDictionary)) { - if (shaderDictionary.hasKey("VertexShader")) - shaderDictionary.getValue("VertexShader", vshaderpath); - - if (shaderDictionary.hasKey("FragmentShader")) - shaderDictionary.getValue("FragmentShader", fshaderpath); - - vshaderpath = findPath(vshaderpath); - fshaderpath = findPath(fshaderpath); - - _vertexSourceFile = new ghoul::filesystem::File(vshaderpath, false); - _fragmentSourceFile = new ghoul::filesystem::File(fshaderpath, false); - - - ShaderCreator sc = OsEng.shaderBuilder(); - _fieldlinesProgram = sc.buildShader("FieldlinesProgram", vshaderpath, fshaderpath); - } + ghoul::Dictionary shaderDictionary; + success = dictionary.getValueSafe(keyShaders, shaderDictionary); + if (!success) { + LERROR("RenderableFieldlines '" << name << "' does not contain a '" << + keyShaders << "' table"); + return; } - if(dictionary.hasKey("UpdateOnSave")) { - dictionary.getValue("UpdateOnSave", _programUpdateOnSave); + std::string vshaderpath; + success = shaderDictionary.getValueSafe(keyVertexShader, vshaderpath); + if (!success) { + LERROR("RenderableFieldlines '" << name << "' does not have a '" << + keyVertexShader << "'"); + return; } + vshaderpath = findPath(vshaderpath); + + std::string fshaderpath; + success = shaderDictionary.getValueSafe(keyFragmentShader, fshaderpath); + if (!success) { + LERROR("RenderableFieldlines '" << name << "' does not have a '" << + keyFragmentShader << "'"); + return; + } + fshaderpath = findPath(fshaderpath); + + _vertexSourceFile = new ghoul::filesystem::File(vshaderpath, false); + _fragmentSourceFile = new ghoul::filesystem::File(fshaderpath, false); + + + ShaderCreator sc = OsEng.shaderBuilder(); + _fieldlinesProgram = sc.buildShader("FieldlinesProgram", vshaderpath, fshaderpath); + + dictionary.getValueSafe("UpdateOnSave", _programUpdateOnSave); setBoundingSphere(PowerScaledScalar::CreatePSS(5)); // FIXME a non-magic number perhaps } diff --git a/src/scenegraph/scenegraphnode.cpp b/src/scenegraph/scenegraphnode.cpp index 4c203b8e36..b096b8a1eb 100644 --- a/src/scenegraph/scenegraphnode.cpp +++ b/src/scenegraph/scenegraphnode.cpp @@ -74,6 +74,8 @@ SceneGraphNode* SceneGraphNode::createFromDictionary(const ghoul::Dictionary& di if (dictionary.hasValue(keyRenderable)) { ghoul::Dictionary renderableDictionary; dictionary.getValue(keyRenderable, renderableDictionary); + + renderableDictionary.setValue(keyName, name); renderableDictionary.setValue(keyPathModule, path); result->_renderable = Renderable::createFromDictionary(renderableDictionary);