From c8381617a891ba4d2646f65433cc498713914d94 Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Sun, 2 Feb 2025 16:36:54 +0000 Subject: [PATCH] Make plugin loading and sorting more granular - Don't clear the cache in LoadPlugins() - Don't load plugins in SortPlugins(), and make it take a vector of strings, not paths. - Add a ClearLoadedPlugins() method to clear the loaded plugins cache. - Remove IdentifyMainMasterFile() Instead of calling IdentifyMainMasterFile(), callers can use LoadPlugins() to initially load all plugin headers only, then omit the main master file when calling LoadPlugins() to fully load plugins. --- docs/api/changelog.rst | 16 ++++-- include/loot/game_interface.h | 50 ++++++++++--------- include/loot/metadata/plugin_metadata.h | 2 +- src/api/game/game.cpp | 29 +++-------- src/api/game/game.h | 8 ++- src/api/game/game_cache.cpp | 10 ++-- src/api/sorting/plugin_sort.cpp | 47 +++++++++-------- src/tests/api/interface/game_interface_test.h | 23 +++++++-- src/tests/api/internals/game/game_test.h | 41 ++++++++++++--- .../metadata/condition_evaluator_test.h | 1 - .../api/internals/sorting/plugin_sort_test.h | 25 +++++++++- .../sorting/plugin_sorting_data_test.h | 1 - 12 files changed, 158 insertions(+), 95 deletions(-) diff --git a/docs/api/changelog.rst b/docs/api/changelog.rst index 7afc1e64..4b7946a5 100644 --- a/docs/api/changelog.rst +++ b/docs/api/changelog.rst @@ -26,6 +26,8 @@ Added paths to ensure that they are resolved correctly. - Ghosted plugins are not supported for OpenMW. +- :cpp:any:`loot::GameInterface::ClearLoadedPlugins()` + Fixed ----- @@ -38,9 +40,13 @@ Fixed Changed ------- -- :cpp:any:`loot::GameInterface::IdentifyMainMasterFile()` now takes a - ``const std::filesystem::path&`` instead of a - ``const std::string&``. +- :cpp:any:`loot::GameInterface::LoadPlugins()` no longer clears the data of + previously-loaded plugins, though if any of the given paths have filenames + that match previously-loaded plugins, the previously-loaded data will be + still be replaced. +- :cpp:any:`loot::GameInterface::SortPlugins()` now takes a vector of filenames + instead of a vector of strings, and no longer loads the given plugins. It + instead expects the plugins to have already been loaded. - The application of plugin groups as part of the sorting process has been overhauled. As well as fixing several known bugs, the new approach avoids causing cyclic interaction errors, handles groups more consistently and is @@ -64,6 +70,10 @@ Changed Removed ------- +- ``loot::GameInterface::IdentifyMainMasterFile()``: callers should instead + call :cpp:any:`loot::GameInterface::LoadPlugins()` with the main master file + to load only its headers, and omit the main master file when calling + :cpp:any:`loot::GameInterface::LoadPlugins()` to fully load plugins. - Prebuilt Linux release binaries are no longer provided, as the binaries that were previously provided were not very portable beyond the Linux distribution versions that they were built on. diff --git a/include/loot/game_interface.h b/include/loot/game_interface.h index 1252ecd0..a0115027 100644 --- a/include/loot/game_interface.h +++ b/include/loot/game_interface.h @@ -103,27 +103,42 @@ public: /** * @brief Parses plugins and loads their data. - * @details Any previously-loaded plugin data is discarded when this function - * is called. + * @details If a given plugin filename (or one that is case-insensitively + * equal) has already been loaded, its previously-loaded data + * data is discarded, invalidating any existing shared pointers to + * that plugin's PluginInterface object. + * + * If the game is Morrowind, OpenMW or Starfield, it's only valid to + * fully load a plugin if its masters are already loaded or included + * in the same input vector. * @param pluginPaths * The plugin paths to load. Relative paths are resolved relative to * the game's plugins directory, while absolute paths are used as * given. Each plugin filename must be unique within the vector. * @param loadHeadersOnly * If true, only the plugins' headers are loaded. If false, all records - * in the plugins are parsed, apart from the main master file if it has - * been identified by a previous call to ``IdentifyMainMasterFile()``. + * in the plugins are parsed. */ virtual void LoadPlugins( const std::vector& pluginPaths, bool loadHeadersOnly) = 0; + /** + * @brief Clears the plugins loaded by previous calls to `LoadPlugins()`. + * @details This invalidates any PluginInterface pointers retrieved using + * `GetPlugin()` or `GetLoadedPlugins()`. + */ + virtual void ClearLoadedPlugins() = 0; + /** * @brief Get data for a loaded plugin. * @param pluginName * The filename of the plugin to get data for. * @returns A shared pointer to a const PluginInterface implementation. The - * pointer is null if the given plugin has not been loaded. + * pointer is null if the given plugin has not been loaded. The + * pointer remains valid until the `ClearLoadedPlugins()` function + * is called, this GameInterface is destroyed, or until a plugin with + * a case-insensitively equal filename is loaded. */ virtual const PluginInterface* GetPlugin( const std::string& pluginName) const = 0; @@ -131,9 +146,10 @@ public: /** * @brief Get a set of const references to all loaded plugins' PluginInterface * objects. - * @returns A set of const PluginInterface references. The references remain - * valid until the ``LoadPlugins()`` or ``SortPlugins()`` functions - * are next called or this GameInterface is destroyed. + * @returns A set of shared pointers to const PluginInterface. The pointers + * remain valid until the `ClearLoadedPlugins()` function is called, + * this GameInterface is destroyed, or until a plugin with a + * case-insensitively equal filename is loaded. */ virtual std::vector GetLoadedPlugins() const = 0; @@ -143,16 +159,6 @@ public: * @{ */ - /** - * @brief Identify the game's main master file. - * @details When sorting, LOOT always only loads the headers of the game's - * main master file as a performance optimisation. - * - * A relative path is resolved relative to the game's plugins - * directory, while an absolute path is used as given. - */ - virtual void IdentifyMainMasterFile(const std::filesystem::path& masterFile) = 0; - /** * @brief Calculates a new load order for the game's installed plugins * (including inactive plugins) and outputs the sorted order. @@ -161,15 +167,13 @@ public: * applied to the load order used by the game. This function does * not load or evaluate the masterlist or userlist. * @param pluginPaths - * The plugin paths to sort, in their current load order. Relative - * paths are resolved relative to the game's plugins directory, while - * absolute paths are used as given. Each plugin filename must be - * unique within the vector. + * The plugins to sort, in their current load order. All given plugins + * must have been loaded using `LoadPlugins()`. * @returns A vector of the given plugin filenames in their sorted load * order. */ virtual std::vector SortPlugins( - const std::vector& pluginPaths) = 0; + const std::vector& pluginFilenames) = 0; /** * @} diff --git a/include/loot/metadata/plugin_metadata.h b/include/loot/metadata/plugin_metadata.h index f1a58122..da821a90 100644 --- a/include/loot/metadata/plugin_metadata.h +++ b/include/loot/metadata/plugin_metadata.h @@ -211,7 +211,7 @@ public: /** * Check if the plugin name is a regular expression. - * @return True if the plugin name contains any of the characters ``:\*?|``, + * @return True if the plugin name contains any of the characters `:\*?|`, * false otherwise. */ LOOT_API bool IsRegexPlugin() const; diff --git a/src/api/game/game.cpp b/src/api/game/game.cpp index 52a0dca2..c9aca43b 100644 --- a/src/api/game/game.cpp +++ b/src/api/game/game.cpp @@ -255,9 +255,6 @@ void Game::LoadPlugins(const std::vector& pluginPaths, "\" is not a valid plugin"); } - // Clear the existing plugin and archive caches. - cache_.ClearCachedPlugins(); - // Search for and cache archives. CacheArchives(); @@ -275,12 +272,8 @@ void Game::LoadPlugins(const std::vector& pluginPaths, const auto resolvedPluginPath = ResolvePluginPath(GetType(), DataPath(), pluginPath); - const bool loadHeader = - loadHeadersOnly || - loot::equivalent(resolvedPluginPath, masterFilePath_); - cache_.AddPlugin( - Plugin(GetType(), cache_, resolvedPluginPath, loadHeader)); + Plugin(GetType(), cache_, resolvedPluginPath, loadHeadersOnly)); } catch (const std::exception& e) { if (logger) { logger->error( @@ -304,6 +297,10 @@ void Game::LoadPlugins(const std::vector& pluginPaths, conditionEvaluator_->RefreshLoadedPluginsState(GetLoadedPlugins()); } +void Game::ClearLoadedPlugins() { + cache_.ClearCachedPlugins(); +} + const PluginInterface* Game::GetPlugin(const std::string& pluginName) const { return cache_.GetPlugin(pluginName); } @@ -317,21 +314,9 @@ std::vector Game::GetLoadedPlugins() const { return interfacePointers; } -void Game::IdentifyMainMasterFile(const std::filesystem::path& masterFile) { - masterFilePath_ = ResolvePluginPath(GetType(), DataPath(), masterFile); -} - std::vector Game::SortPlugins( - const std::vector& pluginPaths) { - LoadPlugins(pluginPaths, false); - - std::vector loadOrder; - for (const auto& pluginPath : pluginPaths) { - loadOrder.push_back(pluginPath.filename().u8string()); - } - - // Sort plugins into their load order. - return loot::SortPlugins(*this, loadOrder); + const std::vector& pluginFilenames) { + return loot::SortPlugins(*this, pluginFilenames); } void Game::LoadCurrentLoadOrderState() { diff --git a/src/api/game/game.h b/src/api/game/game.h index 5c0c48a2..6e580325 100644 --- a/src/api/game/game.h +++ b/src/api/game/game.h @@ -70,15 +70,15 @@ public: void LoadPlugins(const std::vector& pluginPaths, bool loadHeadersOnly) override; + void ClearLoadedPlugins() override; + const PluginInterface* GetPlugin( const std::string& pluginName) const override; std::vector GetLoadedPlugins() const override; - void IdentifyMainMasterFile(const std::filesystem::path& masterFile) override; - std::vector SortPlugins( - const std::vector& pluginPaths) override; + const std::vector& pluginFilenames) override; void LoadCurrentLoadOrderState() override; @@ -103,8 +103,6 @@ private: std::shared_ptr conditionEvaluator_; ApiDatabase database_; - std::filesystem::path masterFilePath_; - std::vector additionalDataPaths_; }; } diff --git a/src/api/game/game_cache.cpp b/src/api/game/game_cache.cpp index 3a0c2490..b2b7868f 100644 --- a/src/api/game/game_cache.cpp +++ b/src/api/game/game_cache.cpp @@ -93,12 +93,14 @@ void GameCache::AddPlugin(Plugin&& plugin) { lock_guard lock(mutex_); auto normalizedName = NormalizeFilename(plugin.GetName()); + auto pluginPointer = std::make_shared(std::move(plugin)); const auto it = plugins_.find(normalizedName); - if (it != end(plugins_)) - plugins_.erase(it); - - plugins_.emplace(normalizedName, std::make_shared(std::move(plugin))); + if (it != end(plugins_)) { + it->second = pluginPointer; + } else { + plugins_.emplace(normalizedName, pluginPointer); + } } std::set GameCache::GetArchivePaths() const { diff --git a/src/api/sorting/plugin_sort.cpp b/src/api/sorting/plugin_sort.cpp index 09bb0fe2..80f475cf 100644 --- a/src/api/sorting/plugin_sort.cpp +++ b/src/api/sorting/plugin_sort.cpp @@ -34,33 +34,23 @@ namespace loot { std::vector GetPluginsSortingData( const DatabaseInterface& db, - const std::vector loadedPluginInterfaces, - const std::vector& loadOrder) { + const std::vector& loadOrder) { std::vector pluginsSortingData; - pluginsSortingData.reserve(loadedPluginInterfaces.size()); + pluginsSortingData.reserve(loadOrder.size()); std::vector comparableLoadOrder; - for (const auto& pluginName : loadOrder) { - comparableLoadOrder.push_back(ToComparableFilename(pluginName)); + for (const auto& plugin : loadOrder) { + comparableLoadOrder.push_back(ToComparableFilename(plugin->GetName())); } - for (const auto& pluginInterface : loadedPluginInterfaces) { - if (!pluginInterface) { - continue; - } - - const auto plugin = dynamic_cast(pluginInterface); - - if (!plugin) { - throw std::logic_error( - "Tried to case a PluginInterface pointer to a Plugin pointer."); - } + for (const auto& plugin : loadOrder) { + const auto pluginFilename = plugin->GetName(); const auto masterlistMetadata = - db.GetPluginMetadata(plugin->GetName(), false, true) - .value_or(PluginMetadata(plugin->GetName())); - const auto userMetadata = db.GetPluginUserMetadata(plugin->GetName(), true) - .value_or(PluginMetadata(plugin->GetName())); + db.GetPluginMetadata(pluginFilename, false, true) + .value_or(PluginMetadata(pluginFilename)); + const auto userMetadata = db.GetPluginUserMetadata(pluginFilename, true) + .value_or(PluginMetadata(pluginFilename)); const auto pluginSortingData = PluginSortingData( plugin, masterlistMetadata, userMetadata, comparableLoadOrder); @@ -313,8 +303,7 @@ std::vector SortPlugins( return {}; } - // Sort the plugins according to into their existing load order, or - // lexicographical ordering for pairs of plugins without load order positions. + // Sort the plugins according to the lexicographical order of their names. // This ensures a consistent iteration order for vertices given the same input // data. The vertex iteration order can affect what edges get added and so // the final sorting result, so consistency is important. @@ -387,8 +376,18 @@ std::vector SortPlugins( std::vector SortPlugins( Game& game, const std::vector& loadOrder) { - auto pluginsSortingData = GetPluginsSortingData( - game.GetDatabase(), game.GetLoadedPlugins(), loadOrder); + std::vector plugins; + for (const auto& pluginFilename : loadOrder) { + const auto plugin = game.GetCache().GetPlugin(pluginFilename); + if (plugin == nullptr) { + throw std::invalid_argument("The plugin \"" + pluginFilename + + "\" has not been loaded."); + } + + plugins.push_back(plugin); + } + + auto pluginsSortingData = GetPluginsSortingData(game.GetDatabase(), plugins); const auto logger = getLogger(); if (logger) { diff --git a/src/tests/api/interface/game_interface_test.h b/src/tests/api/interface/game_interface_test.h index ce03d7a3..4545d3dd 100644 --- a/src/tests/api/interface/game_interface_test.h +++ b/src/tests/api/interface/game_interface_test.h @@ -118,7 +118,7 @@ TEST_P(GameInterfaceTest, isValidPluginShouldReturnFalseForAnEmptyFile) { TEST_P( GameInterfaceTest, - loadPluginsWithHeadersOnlyTrueShouldLoadTheHeadersOfAllInstalledPlugins) { + loadPluginsWithHeadersOnlyTrueShouldLoadTheHeadersOfAllGivenPlugins) { handle_->LoadPlugins(pluginsToLoad, true); if (GetParam() == GameType::starfield) { EXPECT_EQ(6, handle_->GetLoadedPlugins().size()); @@ -184,7 +184,17 @@ TEST_P(GameInterfaceTest, loadPluginsWithANonAsciiPluginShouldLoadIt) { EXPECT_EQ(blankEsmCrc, plugin->GetCRC().value()); } -TEST_P(GameInterfaceTest, getPluginThatIsNotCachedShouldReturnAnEmptyOptional) { +TEST_P(GameInterfaceTest, clearLoadedPluginsShouldClearThePluginsCache) { + handle_->LoadPlugins({std::filesystem::u8path(blankEsm)}, true); + const auto pointer = handle_->GetPlugin(blankEsm); + ASSERT_NE(nullptr, pointer); + + handle_->ClearLoadedPlugins(); + + EXPECT_EQ(nullptr, handle_->GetPlugin(blankEsm)); +} + +TEST_P(GameInterfaceTest, getPluginThatIsNotCachedShouldReturnANullPointer) { EXPECT_FALSE(handle_->GetPlugin(blankEsm)); } @@ -232,7 +242,14 @@ TEST_P(GameInterfaceTest, sortPluginsShouldSucceedIfPassedValidArguments) { } handle_->LoadCurrentLoadOrderState(); - std::vector actualOrder = handle_->SortPlugins(pluginsToLoad); + handle_->LoadPlugins(pluginsToLoad, false); + + std::vector pluginsToSort; + for (const auto& plugin : pluginsToLoad) { + pluginsToSort.push_back(plugin.filename().u8string()); + } + + std::vector actualOrder = handle_->SortPlugins(pluginsToSort); EXPECT_EQ(expectedOrder, actualOrder); } diff --git a/src/tests/api/internals/game/game_test.h b/src/tests/api/internals/game/game_test.h index 67ad5ad9..a6b822af 100644 --- a/src/tests/api/internals/game/game_test.h +++ b/src/tests/api/internals/game/game_test.h @@ -218,7 +218,7 @@ TEST_P( TEST_P( GameTest, - loadPluginsWithHeadersOnlyTrueShouldLoadTheHeadersOfAllInstalledPlugins) { + loadPluginsWithHeadersOnlyTrueShouldLoadTheHeadersOfGivenPlugins) { Game game = Game(GetParam(), gamePath, localPath); EXPECT_NO_THROW(loadInstalledPlugins(game, true)); @@ -266,7 +266,7 @@ TEST_P(GameTest, } TEST_P(GameTest, - loadPluginsWithHeadersOnlyFalseShouldFullyLoadAllInstalledPlugins) { + loadPluginsWithHeadersOnlyFalseShouldFullyLoadAllGivenPlugins) { Game game = Game(GetParam(), gamePath, localPath); EXPECT_NO_THROW(loadInstalledPlugins(game, false)); @@ -285,6 +285,32 @@ TEST_P(GameTest, EXPECT_EQ(blankEsmCrc, plugin->GetCRC().value()); } +TEST_P(GameTest, loadPluginsShouldNotClearThePluginsCache) { + Game game = Game(GetParam(), gamePath, localPath); + + game.LoadPlugins({std::filesystem::u8path(blankEsm)}, true); + const auto pointer = game.GetPlugin(blankEsm); + ASSERT_NE(nullptr, pointer); + + game.LoadPlugins({std::filesystem::u8path(blankEsp)}, true); + + EXPECT_EQ(pointer, game.GetPlugin(blankEsm)); +} + +TEST_P(GameTest, loadPluginsShouldReplaceCacheEntriesForTheGivenPlugins) { + Game game = Game(GetParam(), gamePath, localPath); + + game.LoadPlugins({std::filesystem::u8path(blankEsm)}, true); + const auto pointer = game.GetPlugin(blankEsm); + ASSERT_NE(nullptr, pointer); + + game.LoadPlugins({std::filesystem::u8path(blankEsm)}, false); + + const auto newPointer = game.GetPlugin(blankEsm); + ASSERT_NE(nullptr, newPointer); + EXPECT_NE(pointer, newPointer); +} + TEST_P( GameTest, loadPluginsShouldFindAndCacheArchivesForLoadDetectionWhenLoadingPlugins) { @@ -408,15 +434,16 @@ TEST_P( } } -TEST_P(GameTest, sortPluginsShouldHandlePluginPathsThatAreNotJustFilenames) { +TEST_P(GameTest, clearLoadedPluginsShouldClearThePluginsCache) { Game game = Game(GetParam(), gamePath, localPath); - const auto absolutePath = dataPath / std::filesystem::u8path(blankEsm); + game.LoadPlugins({std::filesystem::u8path(blankEsm)}, true); + const auto pointer = game.GetPlugin(blankEsm); + ASSERT_NE(nullptr, pointer); - const auto newLoadOrder = - game.SortPlugins(std::vector({absolutePath})); + game.ClearLoadedPlugins(); - EXPECT_EQ(std::vector{blankEsm}, newLoadOrder); + EXPECT_EQ(nullptr, game.GetPlugin(blankEsm)); } TEST_P(GameTest, shouldShowBlankEsmAsActiveIfItHasNotBeenLoaded) { diff --git a/src/tests/api/internals/metadata/condition_evaluator_test.h b/src/tests/api/internals/metadata/condition_evaluator_test.h index 219e653c..eb4098a6 100644 --- a/src/tests/api/internals/metadata/condition_evaluator_test.h +++ b/src/tests/api/internals/metadata/condition_evaluator_test.h @@ -68,7 +68,6 @@ protected: plugins.push_back(blankEsl); } - game_.IdentifyMainMasterFile(std::filesystem::u8path(masterFile)); game_.LoadCurrentLoadOrderState(); game_.LoadPlugins(plugins, true); } diff --git a/src/tests/api/internals/sorting/plugin_sort_test.h b/src/tests/api/internals/sorting/plugin_sort_test.h index 890ed793..4e889996 100644 --- a/src/tests/api/internals/sorting/plugin_sort_test.h +++ b/src/tests/api/internals/sorting/plugin_sort_test.h @@ -51,8 +51,14 @@ protected: } } - game.IdentifyMainMasterFile(std::filesystem::u8path(masterFile)); game.LoadCurrentLoadOrderState(); + + if (!headersOnly) { + const auto gameMasterPlugin = plugins.front(); + game.LoadPlugins({gameMasterPlugin}, true); + plugins.erase(plugins.begin()); + } + game.LoadPlugins(plugins, headersOnly); } @@ -1141,6 +1147,23 @@ TEST_P( }), sorted); } + +TEST_P(PluginSortTest, sortingShouldOnlySortTheGivenPlugins) { + loadInstalledPlugins(game_, false); + + std::vector plugins{blankEsp, blankDifferentEsp}; + std::vector sorted = SortPlugins(game_, plugins); + + EXPECT_EQ(plugins, sorted); +} + +TEST_P(PluginSortTest, sortingShouldThrowIfAGivenPluginIsNotLoaded) { + game_.ClearLoadedPlugins(); + + std::vector plugins{blankEsp, blankDifferentEsp}; + + EXPECT_THROW(SortPlugins(game_, plugins), std::invalid_argument); +} } } diff --git a/src/tests/api/internals/sorting/plugin_sorting_data_test.h b/src/tests/api/internals/sorting/plugin_sorting_data_test.h index 790a74ca..f508746b 100644 --- a/src/tests/api/internals/sorting/plugin_sorting_data_test.h +++ b/src/tests/api/internals/sorting/plugin_sorting_data_test.h @@ -46,7 +46,6 @@ protected: } } - game.IdentifyMainMasterFile(std::filesystem::u8path(masterFile)); game.LoadCurrentLoadOrderState(); game.LoadPlugins(plugins, headersOnly); }