From a4d84d87038feb771a080863a34054fd8a63a916 Mon Sep 17 00:00:00 2001 From: GPayne Date: Thu, 16 Sep 2021 23:11:02 -0600 Subject: [PATCH] Code review changes on new profile initialization --- include/openspace/engine/configuration.h | 2 - include/openspace/engine/openspaceengine.h | 111 ++++++++--------- include/openspace/scene/profile.h | 12 -- include/openspace/scene/scene.h | 22 ++-- include/openspace/util/time.h | 4 +- include/openspace/util/timemanager.h | 2 +- src/engine/openspaceengine.cpp | 131 ++++++++------------- src/scene/profile.cpp | 4 - src/scene/profile_lua.inl | 8 -- src/scene/scene.cpp | 10 +- src/util/time.cpp | 4 +- src/util/timemanager.cpp | 18 ++- 12 files changed, 131 insertions(+), 197 deletions(-) diff --git a/include/openspace/engine/configuration.h b/include/openspace/engine/configuration.h index 1ece821be8..9748891b38 100644 --- a/include/openspace/engine/configuration.h +++ b/include/openspace/engine/configuration.h @@ -45,7 +45,6 @@ struct Configuration { std::string windowConfiguration = "${CONFIG}/single.xml"; std::string asset; - std::string profileOutPrefixName; std::string profile; std::vector readOnlyProfiles; std::vector globalCustomizationScripts; @@ -132,7 +131,6 @@ struct Configuration { HTTPProxy httpProxy; // Values not read from the openspace.cfg file - bool usingProfile = false; std::string sgctConfigNameInitialized; static documentation::Documentation Documentation; diff --git a/include/openspace/engine/openspaceengine.h b/include/openspace/engine/openspaceengine.h index 03c87cfd9b..aaadc9cd18 100644 --- a/include/openspace/engine/openspaceengine.h +++ b/include/openspace/engine/openspaceengine.h @@ -102,61 +102,6 @@ public: void writeStaticDocumentation(); void createUserDirectoriesIfNecessary(); - /** - * Sets the camera position using the time contents of a profile. The function will - * set an absolute position or a go-to-geolocation command using the globebrowsing - * module. - * \param p The Profile to be read. - */ - void setFromProfile_camera(const Profile& p); - - /** - * Sets the delta times using the delta time array from a profile. - * - * \param p The Profile to be read. - */ - void setFromProfile_deltaTimes(const Profile& p); - - /** - * Reads a list of modules from a profile, and executes scripts based on whether or - * not the corresponding module is loaded. - * - * \param p The Profile to be read. - */ - void setFromProfile_modules(const Profile& p); - - /** - * Registers actions from the contents of a profile. - * - * \param p The Profile to be read. - */ - void setFromProfile_actions(const Profile& p); - - /** - * Registers keybindings from the contents of a profile. - * - * \param p The Profile to be read. - */ - void setFromProfile_keybindings(const Profile& p); - - /** - * Reads list of nodes from profile to be marked as interesting nodes. - * If any nodes are listed, a script to mark these will be queued with the - * script engine. - * - * \param p The Profile to be read. - */ - void setFromProfile_markInterestingNodes(const Profile& p); - - /** - * Reads list of "additional scripts" that are added to the profile to be run - * at the end of the initialization. Any openspace lua commands are allowed, - * and will be added to the script queue. - * - * \param p The Profile to be read. - */ - void setFromProfile_additionalScripts(const Profile& p); - /** * Returns the Lua library that contains all Lua functions available to affect the * application. @@ -164,16 +109,13 @@ public: static scripting::LuaLibrary luaLibrary(); private: - void loadAsset_init(const std::string assetName); - void loadAsset_postInit(const std::string assetName); + void loadAsset(const std::string& assetName); void loadFonts(); void runGlobalCustomizationScripts(); void configureLogging(); std::string generateFilePath(std::string openspaceRelativePath); void resetPropertyChangeFlagsOfSubowners(openspace::properties::PropertyOwner* po); - void loadInitAssetSection(std::string& initAssetOutput, - std::string profileSectionName); std::unique_ptr _scene; std::unique_ptr _assetManager; @@ -183,7 +125,6 @@ private: bool _hasScheduledAssetLoading = false; std::string _scheduledAssetPathToLoad; - bool _hasInitializedProfile = false; glm::vec2 _mousePosition = glm::vec2(0.f); @@ -196,9 +137,57 @@ private: // The first frame might take some more time in the update loop, so we need to know to // disable the synchronization; otherwise a hardware sync will kill us after 1 minute - bool _isFirstRenderingFirstFrame = true; + bool _isRenderingFirstFrame = true; }; +/** + * Sets the camera position using the time contents of a profile. The function will + * set an absolute position or a go-to-geolocation command using the globebrowsing + * module. + * \param p The Profile to be read. + */ +void setCameraFromProfile(const Profile& p); + +/** + * Reads a list of modules from a profile, and executes scripts based on whether or + * not the corresponding module is loaded. + * + * \param p The Profile to be read. + */ +void setModulesFromProfile(const Profile& p); + +/** + * Registers actions from the contents of a profile. + * + * \param p The Profile to be read. + */ +void setActionsFromProfile(const Profile& p); + +/** + * Registers keybindings from the contents of a profile. + * + * \param p The Profile to be read. + */ +void setKeybindingsFromProfile(const Profile& p); + +/** + * Reads list of nodes from profile to be marked as interesting nodes. + * If any nodes are listed, a script to mark these will be queued with the + * script engine. + * + * \param p The Profile to be read. + */ +void setMarkInterestingNodesFromProfile(const Profile& p); + +/** + * Reads list of "additional scripts" that are added to the profile to be run + * at the end of the initialization. Any openspace lua commands are allowed, + * and will be added to the script queue. + * + * \param p The Profile to be read. + */ +void setAdditionalScriptsFromProfile(const Profile& p); + } // namespace openspace // Lua functions - exposed for testing diff --git a/include/openspace/scene/profile.h b/include/openspace/scene/profile.h index 86631e45ff..eeb79b8adf 100644 --- a/include/openspace/scene/profile.h +++ b/include/openspace/scene/profile.h @@ -163,20 +163,8 @@ public: * \return The Lua library that contains all Lua functions available for profiles */ static scripting::LuaLibrary luaLibrary(); - - std::string file_subName_assets = ""; - std::string assetFileExtension = ".asset"; }; -/** - * Function to retrieve the individual additional script lines included in the profile - * - * \param p The profile that should be processed - * - * \return vector of strings containing the individual script lines - */ -std::vector& additionalScripts(Profile& p); - } // namespace openspace #endif // __OPENSPACE_CORE___PROFILE___H__ diff --git a/include/openspace/scene/scene.h b/include/openspace/scene/scene.h index 8eed3fd26c..e199f1f425 100644 --- a/include/openspace/scene/scene.h +++ b/include/openspace/scene/scene.h @@ -243,17 +243,7 @@ public: * * \param p The Profile to be read. */ - void setFromProfile_properties(const Profile& p); - - /** - * Accepts string version of a property value from a profile, converts it to the - * appropriate type, and then pushes the value onto the lua state. - * - * \param L the lua state to push value to - * \param value string representation of the value with which to set property - */ - void property_pushValueFromProfileToLuaState(ghoul::lua::LuaState& L, - const std::string& value); + void setPropertiesFromProfile(const Profile& p); private: /** @@ -289,6 +279,16 @@ private: ghoul::MemoryPool<4096> _memoryPool; }; +/** + * Accepts string version of a property value from a profile, converts it to the + * appropriate type, and then pushes the value onto the lua state. + * + * \param L the lua state to push value to + * \param value string representation of the value with which to set property + */ +void propertyPushValueFromProfileToLuaState(ghoul::lua::LuaState& L, + const std::string& value); + } // namespace openspace #endif // __OPENSPACE_CORE___SCENE___H__ diff --git a/include/openspace/util/time.h b/include/openspace/util/time.h index 82adf87f02..7b19cb26ba 100644 --- a/include/openspace/util/time.h +++ b/include/openspace/util/time.h @@ -146,14 +146,14 @@ public: * \param setTime a string containing time adjustment as described in documentation * for luascriptfunctions::time_advancedTime */ - void setFromProfile_timeRelative(const std::string& setTime); + void setTimeRelativeFromProfile(const std::string& setTime); /** * Sets an absolute time from profile. * \param setTime a string containing time to set, which must be a valid * ISO 8601-like date string of the format YYYY-MM-DDTHH:MN:SS */ - void setFromProfile_timeAbsolute(const std::string& setTime); + void setTimeAbsoluteFromProfile(const std::string& setTime); /** * Returns the Lua library that contains all Lua functions available to change the diff --git a/include/openspace/util/timemanager.h b/include/openspace/util/timemanager.h index 9ecd2f1f4c..87b80f3b80 100644 --- a/include/openspace/util/timemanager.h +++ b/include/openspace/util/timemanager.h @@ -85,7 +85,7 @@ public: * * \param p The Profile to be read. */ - void setFromProfile_time(const Profile& p); + void setTimeFromProfile(const Profile& p); bool isPaused() const; diff --git a/src/engine/openspaceengine.cpp b/src/engine/openspaceengine.cpp index 7a04e3ea61..b5405f0fe4 100644 --- a/src/engine/openspaceengine.cpp +++ b/src/engine/openspaceengine.cpp @@ -305,8 +305,6 @@ void OpenSpaceEngine::initialize() { + ".profile"; std::string inputUserProfile = absPath("${USER_PROFILES}").string() + "/" + global::configuration->profile + ".profile"; - std::string outputProfilePrefix = outputScenePath + "/" - + global::configuration->profile; if (std::filesystem::is_regular_file(inputUserProfile)) { inputProfile = inputUserProfile; @@ -335,9 +333,6 @@ void OpenSpaceEngine::initialize() { std::istreambuf_iterator() ); *global::profile = Profile(content); - - global::configuration->profileOutPrefixName = outputProfilePrefix; - global::configuration->usingProfile = true; } } @@ -689,10 +684,10 @@ void OpenSpaceEngine::scheduleLoadSingleAsset(std::string assetPath) { _scheduledAssetPathToLoad = std::move(assetPath); } -void OpenSpaceEngine::loadAsset_init(const std::string assetName) { +void OpenSpaceEngine::loadAsset(const std::string& assetName) { ZoneScoped - LTRACE("OpenSpaceEngine::loadAsset_init(begin)"); + LTRACE("OpenSpaceEngine::loadAsset(begin)"); global::windowDelegate->setBarrier(false); global::windowDelegate->setSynchronization(false); @@ -752,7 +747,7 @@ void OpenSpaceEngine::loadAsset_init(const std::string assetName) { if (!assetName.empty()) { _assetManager->add(assetName); } - for (auto a : global::profile->assets) { + for (const std::string& a : global::profile->assets) { _assetManager->add(a); } @@ -866,16 +861,7 @@ void OpenSpaceEngine::loadAsset_init(const std::string assetName) { _writeDocumentationTask = std::async(&OpenSpaceEngine::writeSceneDocumentation, this); - LTRACE("OpenSpaceEngine::loadAsset_init(end)"); -} - -void OpenSpaceEngine::loadAsset_postInit(const std::string assetName) { - ZoneScoped - - LTRACE("OpenSpaceEngine::loadAsset_postInit(begin)"); - _assetManager->add(assetName); - _assetManager->update(); - LTRACE("OpenSpaceEngine::loadAsset_postInit(end)"); + LTRACE("OpenSpaceEngine::loadAsset(end)"); } void OpenSpaceEngine::deinitialize() { @@ -1110,27 +1096,24 @@ void OpenSpaceEngine::preSynchronization() { if (_hasScheduledAssetLoading) { LINFO(fmt::format("Loading asset: {}", absPath(_scheduledAssetPathToLoad))); global::profile->ignoreUpdates = true; - loadAsset_init(_scheduledAssetPathToLoad); + loadAsset(_scheduledAssetPathToLoad); global::profile->ignoreUpdates = false; resetPropertyChangeFlagsOfSubowners(global::rootPropertyOwner); _hasScheduledAssetLoading = false; _scheduledAssetPathToLoad.clear(); } - else if (!_hasInitializedProfile) { + else if (_isRenderingFirstFrame) { global::profile->ignoreUpdates = true; - loadAsset_init(""); - global::renderEngine->scene()->setFromProfile_properties(*global::profile); - global::timeManager->setFromProfile_time(*global::profile); - setFromProfile_deltaTimes(*global::profile); - setFromProfile_actions(*global::profile); - setFromProfile_keybindings(*global::profile); - setFromProfile_modules(*global::profile); - setFromProfile_markInterestingNodes(*global::profile); + loadAsset(""); + global::renderEngine->scene()->setPropertiesFromProfile(*global::profile); + global::timeManager->setTimeFromProfile(*global::profile); + global::timeManager->setDeltaTimeSteps(global::profile->deltaTimes); + setActionsFromProfile(*global::profile); + setKeybindingsFromProfile(*global::profile); + setModulesFromProfile(*global::profile); + setMarkInterestingNodesFromProfile(*global::profile); global::profile->ignoreUpdates = false; resetPropertyChangeFlagsOfSubowners(global::rootPropertyOwner); - } - - if (_isFirstRenderingFirstFrame) { global::windowDelegate->setSynchronization(false); } @@ -1175,29 +1158,13 @@ void OpenSpaceEngine::preSynchronization() { func(); } - if (!_hasInitializedProfile) { - setFromProfile_camera(*global::profile); - setFromProfile_additionalScripts(*global::profile); - _hasInitializedProfile = true; + if (_isRenderingFirstFrame) { + setCameraFromProfile(*global::profile); + setAdditionalScriptsFromProfile(*global::profile); } LTRACE("OpenSpaceEngine::preSynchronization(end)"); } -void OpenSpaceEngine::loadInitAssetSection(std::string& initAssetOutput, - std::string profileSectionName) -{ - std::string filename = fmt::format( - "{}_{}{}", - global::configuration->profileOutPrefixName, - profileSectionName, - global::profile->assetFileExtension - ); - std::ifstream in(filename); - initAssetOutput.append(std::string(std::istreambuf_iterator(in), - std::istreambuf_iterator())); - LINFO(fmt::format("Loading profile subsection {}", profileSectionName)); -} - void OpenSpaceEngine::postSynchronizationPreDraw() { ZoneScoped TracyGpuZone("postSynchronizationPreDraw") @@ -1223,7 +1190,6 @@ void OpenSpaceEngine::postSynchronizationPreDraw() { _shutdown.timer -= static_cast(global::windowDelegate->averageDeltaTime()); } - const bool updated = _assetManager->update(); if (updated) { if (_writeDocumentationTask.valid()) { @@ -1337,10 +1303,10 @@ void OpenSpaceEngine::postDraw() { func(); } - if (_isFirstRenderingFirstFrame) { + if (_isRenderingFirstFrame) { global::windowDelegate->setSynchronization(true); resetPropertyChangeFlags(); - _isFirstRenderingFirstFrame = false; + _isRenderingFirstFrame = false; } global::memoryManager->PersistentMemory.housekeeping(); @@ -1584,7 +1550,7 @@ void OpenSpaceEngine::toggleShutdownMode() { } } -void OpenSpaceEngine::setFromProfile_camera(const Profile& p) { +void setCameraFromProfile(const Profile& p) { std::visit( overloaded{ [](const Profile::CameraNavState& navStateProfile) { @@ -1609,6 +1575,10 @@ void OpenSpaceEngine::setFromProfile_camera(const Profile& p) { global::navigationHandler->setNavigationStateNextFrame(nav); }, [](const Profile::CameraGoToGeo& geo) { + //Instead of direct calls to navigation state code, lua commands with + //globebrowsing goToGeo are used because this prevents a module + //dependency in this core code. Eventually, goToGeo will be incorporated + //in the OpenSpace core and this code will change. std::string geoScript = fmt::format("openspace.globebrowsing.goToGeo" "([[{}]], {}, {}", geo.anchor, geo.latitude, geo.longitude); if (geo.altitude.has_value()) { @@ -1625,17 +1595,12 @@ void OpenSpaceEngine::setFromProfile_camera(const Profile& p) { ); } -void OpenSpaceEngine::setFromProfile_deltaTimes(const Profile& p) { - global::timeManager->setDeltaTimeSteps(p.deltaTimes); -} - -void OpenSpaceEngine::setFromProfile_modules(const Profile& p) -{ +void setModulesFromProfile(const Profile& p) { for (Profile::Module mod : p.modules) { const std::vector& m = global::moduleEngine->modules(); const auto it = std::find_if(m.begin(), m.end(), [&mod](const OpenSpaceModule* moduleSearch) { - return (moduleSearch->identifier().compare(mod.name) == 0); + return (moduleSearch->identifier() == mod.name); }); if (it != m.end()) { if (mod.loadedInstruction.has_value()) { @@ -1656,39 +1621,35 @@ void OpenSpaceEngine::setFromProfile_modules(const Profile& p) } } -void OpenSpaceEngine::setFromProfile_actions(const Profile& p) -{ +void setActionsFromProfile(const Profile& p) { for (Profile::Action a : p.actions) { if (a.identifier.empty()) { LERROR("Identifier must to provided to register action"); } if (global::actionManager->hasAction(a.identifier)) { - LERROR(fmt::format("Action for identifier '{}' already existed & registered", - a.identifier)); + LERROR( + fmt::format("Action for identifier '{}' already existed & registered", + a.identifier) + ); } if (a.script.empty()) { - LERROR(fmt::format("Identifier '{}' doesn't provide a Lua command to execute", - a.identifier)); + LERROR( + fmt::format("Identifier '{}' doesn't provide a Lua command to execute", + a.identifier) + ); } interaction::Action action; action.identifier = a.identifier; action.command = a.script; - if (!a.name.empty()) { - action.name = a.name; - } - if (!a.documentation.empty()) { - action.documentation = a.documentation; - } - if (!a.guiPath.empty()) { - action.guiPath = a.guiPath; - } + action.name = a.name; + action.documentation = a.documentation; + action.guiPath = a.guiPath; action.synchronization = interaction::Action::IsSynchronized(a.isLocal); global::actionManager->registerAction(std::move(action)); } } -void OpenSpaceEngine::setFromProfile_keybindings(const Profile& p) -{ +void setKeybindingsFromProfile(const Profile& p) { for (Profile::Keybinding k : p.keybindings) { if (k.action.empty()) { LERROR("Action must not be empty"); @@ -1697,15 +1658,19 @@ void OpenSpaceEngine::setFromProfile_keybindings(const Profile& p) LERROR(fmt::format("Action '{}' does not exist", k.action)); } if (k.key.key == openspace::Key::Unknown) { - LERROR(fmt::format("Could not find key '{}'", - std::to_string(static_cast(k.key.key)))); + LERROR( + fmt::format( + "Could not find key '{}'", + std::to_string(static_cast(k.key.key)) + ) + ); } global::keybindingManager->bindKey(k.key.key, k.key.modifier, k.action); } } -void OpenSpaceEngine::setFromProfile_markInterestingNodes(const Profile& p) { - for (std::string nodeName : p.markNodes) { +void setMarkInterestingNodesFromProfile(const Profile& p) { + for (const std::string& nodeName : p.markNodes) { SceneGraphNode* node = global::renderEngine->scene()->sceneGraphNode(nodeName); if (node) { node->addTag("GUI.Interesting"); @@ -1713,7 +1678,7 @@ void OpenSpaceEngine::setFromProfile_markInterestingNodes(const Profile& p) { } } -void OpenSpaceEngine::setFromProfile_additionalScripts(const Profile& p) { +void setAdditionalScriptsFromProfile(const Profile& p) { for (const std::string& a : p.additionalScripts) { global::scriptEngine->queueScript( a, diff --git a/src/scene/profile.cpp b/src/scene/profile.cpp index 3a84d304e0..938335bcdc 100644 --- a/src/scene/profile.cpp +++ b/src/scene/profile.cpp @@ -738,8 +738,4 @@ scripting::LuaLibrary Profile::luaLibrary() { }; } -std::vector& additionalScripts(Profile& p) { - return p.additionalScripts; -} - } // namespace openspace diff --git a/src/scene/profile_lua.inl b/src/scene/profile_lua.inl index a66f8dc1c2..eb51290048 100644 --- a/src/scene/profile_lua.inl +++ b/src/scene/profile_lua.inl @@ -36,14 +36,6 @@ namespace openspace::luascriptfunctions { int saveSettingsToProfile(lua_State* L) { - if (!global::configuration->usingProfile) { - return ghoul::lua::luaError( - L, - "Program was not started with a profile, so cannot use this " - "save-current-settings feature" - ); - } - ghoul::lua::checkArgumentsAndThrow(L, { 0, 2 }, "lua::saveSettingsToProfile"); auto [saveFilePath, overwrite] = ghoul::lua::values, std::optional>(L); diff --git a/src/scene/scene.cpp b/src/scene/scene.cpp index b4503b2ede..fc698333f6 100644 --- a/src/scene/scene.cpp +++ b/src/scene/scene.cpp @@ -485,7 +485,7 @@ std::chrono::steady_clock::time_point Scene::currentTimeForInterpolation() { } void Scene::addPropertyInterpolation(properties::Property* prop, float durationSeconds, - ghoul::EasingFunction easingFunction) + ghoul::EasingFunction easingFunction) { ghoul_precondition(prop != nullptr, "prop must not be nullptr"); ghoul_precondition(durationSeconds > 0.f, "durationSeconds must be positive"); @@ -603,7 +603,7 @@ const std::vector& Scene::interestingTimes() const { return _interestingTimes; } -void Scene::setFromProfile_properties(const Profile& p) { +void Scene::setPropertiesFromProfile(const Profile& p) { ghoul::lua::LuaState L(ghoul::lua::LuaState::IncludeStandardLibrary::Yes); for (const Profile::Property& prop : p.properties) { @@ -616,7 +616,7 @@ void Scene::setFromProfile_properties(const Profile& p) { ghoul::lua::push(L, uriOrRegex); ghoul::lua::push(L, 0.0); // Later functions expect the value to be at the last position on the stack - property_pushValueFromProfileToLuaState(L, prop.value); + propertyPushValueFromProfileToLuaState(L, prop.value); applyRegularExpression( L, @@ -631,8 +631,8 @@ void Scene::setFromProfile_properties(const Profile& p) { } } -void Scene::property_pushValueFromProfileToLuaState(ghoul::lua::LuaState& L, - const std::string& value) +void propertyPushValueFromProfileToLuaState(ghoul::lua::LuaState& L, + const std::string& value) { if (!luascriptfunctions::convertStringToLuaAndPush_bool(L, value)) { if (!luascriptfunctions::convertStringToLuaAndPush_float(L, value)) { diff --git a/src/util/time.cpp b/src/util/time.cpp index cbdb353bf5..2653ee2b31 100644 --- a/src/util/time.cpp +++ b/src/util/time.cpp @@ -122,7 +122,7 @@ void Time::ISO8601(char* buffer) const { SpiceManager::ref().dateFromEphemerisTime(_time, buffer, S, Format); } -void Time::setFromProfile_timeRelative(const std::string& setTime) { +void Time::setTimeRelativeFromProfile(const std::string& setTime) { ghoul::lua::LuaState L(ghoul::lua::LuaState::IncludeStandardLibrary::Yes); luascriptfunctions::time_currentWallTime(L); @@ -131,7 +131,7 @@ void Time::setFromProfile_timeRelative(const std::string& setTime) { luascriptfunctions::time_setTime(L); } -void Time::setFromProfile_timeAbsolute(const std::string& setTime) { +void Time::setTimeAbsoluteFromProfile(const std::string& setTime) { ghoul::lua::LuaState L(ghoul::lua::LuaState::IncludeStandardLibrary::Yes); ghoul::lua::push(L, setTime); diff --git a/src/util/timemanager.cpp b/src/util/timemanager.cpp index 8fbe3ae004..89f898dbd9 100644 --- a/src/util/timemanager.cpp +++ b/src/util/timemanager.cpp @@ -868,14 +868,20 @@ double TimeManager::previousApplicationTimeForInterpolation() const { return _previousApplicationTime; } -void TimeManager::setFromProfile_time(const Profile& p) { +void TimeManager::setTimeFromProfile(const Profile& p) { Time t; - if (p.time->type == Profile::Time::Type::Relative) { - t.setFromProfile_timeRelative(p.time->value); - } - else if (p.time->type == Profile::Time::Type::Absolute) { - t.setFromProfile_timeAbsolute(p.time->value); + switch (p.time.value().type) { + case Profile::Time::Type::Relative: + t.setTimeRelativeFromProfile(p.time.value().value); + break; + + case Profile::Time::Type::Absolute: + t.setTimeAbsoluteFromProfile(p.time.value().value); + break; + + default: + throw ghoul::MissingCaseException(); } }