From 58df21f11caa9590ef7d41181440f563bfd83115 Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Sun, 8 Jan 2023 21:18:17 +0000 Subject: [PATCH] Revert back to using lexicographical vertex order For vertices in the plugin graph. This is so that the group and overlap edges are evaluated in an order that does not depend on the current load order. Tie-breaking still uses the current load order. This is necessary because if the group and overlap edges that get added depend on the current load order, sorting and applying changes the current load order, so sorting again may give different results even even though no plugin data or metadata has changed. --- docs/api/changelog.rst | 11 ++ docs/api/sorting.rst | 30 ++--- src/api/sorting/plugin_graph.cpp | 58 ++++++++- src/api/sorting/plugin_sort.cpp | 51 +------- .../api/internals/sorting/plugin_graph_test.h | 7 +- .../api/internals/sorting/plugin_sort_test.h | 111 ++++++++++++++---- 6 files changed, 180 insertions(+), 88 deletions(-) diff --git a/docs/api/changelog.rst b/docs/api/changelog.rst index 320da5cb..57b3a77c 100644 --- a/docs/api/changelog.rst +++ b/docs/api/changelog.rst @@ -2,6 +2,17 @@ Version History *************** +0.19.1 - Unreleased +=================== + +Fixed +----- + +- Sorting and applying and then sorting again will no longer give a different + result for the second sort. libloot v0.19.0 changed the order in which group + and overlap edges were processed to be the current load order: it has now + reverted back to the lexicographical order of plugin filenames. + 0.19.0 - 2023-01-07 =================== diff --git a/docs/api/sorting.rst b/docs/api/sorting.rst index e59aa1e2..9baf89f2 100644 --- a/docs/api/sorting.rst +++ b/docs/api/sorting.rst @@ -21,17 +21,7 @@ in the masterlist and userlist. Create plugin graph vertices ============================ -Once the plugins have been loaded, they are sorted into their current load -order: - -* If both plugins have positions in the current load order, the function - preserves their existing relative order. -* If one plugin has a position and the other does not, the plugin with a - position goes before the plugin without a position. -* If neither plugin has a load order position, a case-insensitive - lexicographical comparison of their filenames without file extensions is used - to decide their order. If they are equal, a case-insensitive lexicographical - comparison of their file extensions is used. +Once the plugins have been loaded, they are sorted into lexicographical order. After that, two graphs are created, and the plugins are added to them as vertices in their sorted order. Plugins that have their master flag set go in @@ -127,10 +117,20 @@ Tie-break edges --------------- Finally, tie-break edges are added to ensure that sorting is consistent. The -graph's vertices are iterated over in their insertion order (i.e. the current -load order). Each loop looks at the current vertex and the next one following it -(e.g. the first iteration is for vertices 0 and 1, the second is for 1 and 2, -etc.). +graph's vertices are sorted into their current load order: + +* If both plugins have positions in the current load order, the function + preserves their existing relative order. +* If one plugin has a position and the other does not, the plugin with a + position goes before the plugin without a position. +* If neither plugin has a load order position, a case-insensitive + lexicographical comparison of their filenames without file extensions is used + to decide their order. If they are equal, a case-insensitive lexicographical + comparison of their file extensions is used. + +Once sorted, they are iterated over. Each loop looks at the current vertex and +the next one following it (e.g. the first iteration is for vertices 0 and 1, the +second is for 1 and 2, etc.). For each (``current``, ``next``) pair of vertices, try to find a path from ``next`` to ``current``. diff --git a/src/api/sorting/plugin_graph.cpp b/src/api/sorting/plugin_graph.cpp index 9af0c66f..f1509cb0 100644 --- a/src/api/sorting/plugin_graph.cpp +++ b/src/api/sorting/plugin_graph.cpp @@ -528,6 +528,48 @@ bool FindPath(RawPluginGraph& graph, return false; } +int ComparePlugins(const PluginSortingData& plugin1, + const PluginSortingData& plugin2) { + if (plugin1.GetLoadOrderIndex().has_value() && + !plugin2.GetLoadOrderIndex().has_value()) { + return -1; + } + + if (!plugin1.GetLoadOrderIndex().has_value() && + plugin2.GetLoadOrderIndex().has_value()) { + return 1; + } + + if (plugin1.GetLoadOrderIndex().has_value() && + plugin2.GetLoadOrderIndex().has_value()) { + if (plugin1.GetLoadOrderIndex().value() < + plugin2.GetLoadOrderIndex().value()) { + return -1; + } else { + return 1; + } + } + + // Neither plugin has a load order position. Compare plugin basenames to + // get an ordering. + const auto name1 = plugin1.GetName(); + const auto name2 = plugin2.GetName(); + const auto basename1 = name1.substr(0, name1.length() - 4); + const auto basename2 = name2.substr(0, name2.length() - 4); + + const int result = CompareFilenames(basename1, basename2); + + if (result != 0) { + return result; + } else { + // Could be a .esp and .esm plugin with the same basename, + // compare their extensions. + const auto ext1 = name1.substr(name1.length() - 4); + const auto ext2 = name2.substr(name2.length() - 4); + return CompareFilenames(ext1, ext2); + } +} + bool PathsCache::IsPathCached(const vertex_t& fromVertex, const vertex_t& toVertex) const { const auto descendents = pathsCache_.find(fromVertex); @@ -1145,9 +1187,19 @@ void PluginGraph::AddTieBreakEdges() { return std::make_reverse_iterator(std::next(newLoadOrderIt)); }; - // Vertices were already sorted into their existing load order when they - // were added to the graph. - const auto [vitstart, vitend] = GetVertices(); + // First get the graph vertices and sort them into the current load order. + const auto [it, itend] = GetVertices(); + std::vector vertices(it, itend); + + std::sort(vertices.begin(), + vertices.end(), + [this](const vertex_t& lhs, const vertex_t& rhs) { + return ComparePlugins(GetPlugin(lhs), GetPlugin(rhs)) < 0; + }); + + // Now iterate over the vertices in their sorted order. + const auto vitstart = vertices.begin(); + const auto vitend = vertices.end(); for (auto vit = vitstart; vit != vitend; ++vit) { const auto currentVertex = *vit; const auto nextVertexIt = std::next(vit); diff --git a/src/api/sorting/plugin_sort.cpp b/src/api/sorting/plugin_sort.cpp index a7f17575..856946a9 100644 --- a/src/api/sorting/plugin_sort.cpp +++ b/src/api/sorting/plugin_sort.cpp @@ -31,48 +31,6 @@ #include "api/sorting/plugin_graph.h" namespace loot { -int ComparePlugins(const PluginSortingData& plugin1, - const PluginSortingData& plugin2) { - if (plugin1.GetLoadOrderIndex().has_value() && - !plugin2.GetLoadOrderIndex().has_value()) { - return -1; - } - - if (!plugin1.GetLoadOrderIndex().has_value() && - plugin2.GetLoadOrderIndex().has_value()) { - return 1; - } - - if (plugin1.GetLoadOrderIndex().has_value() && - plugin2.GetLoadOrderIndex().has_value()) { - if (plugin1.GetLoadOrderIndex().value() < - plugin2.GetLoadOrderIndex().value()) { - return -1; - } else { - return 1; - } - } - - // Neither plugin has a load order position. Compare plugin basenames to - // get an ordering. - const auto name1 = plugin1.GetName(); - const auto name2 = plugin2.GetName(); - const auto basename1 = name1.substr(0, name1.length() - 4); - const auto basename2 = name2.substr(0, name2.length() - 4); - - const int result = CompareFilenames(basename1, basename2); - - if (result != 0) { - return result; - } else { - // Could be a .esp and .esm plugin with the same basename, - // compare their extensions. - const auto ext1 = name1.substr(name1.length() - 4); - const auto ext2 = name2.substr(name2.length() - 4); - return CompareFilenames(ext1, ext2); - } -} - std::vector GetPluginsSortingData( const GameType gameType, const DatabaseInterface& db, @@ -206,12 +164,13 @@ std::vector SortPlugins( // 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. - // Load order is used because this simplifies the logic when adding tie-break - // edges. + // This order needs to be independent of any state (e.g. the current load + // order) so that sorting and applying the result doesn't then produce a + // different result if you then sort again. std::sort(pluginsSortingData.begin(), pluginsSortingData.end(), [](const auto& lhs, const auto& rhs) { - return ComparePlugins(lhs, rhs) < 0; + return lhs.GetName() < rhs.GetName(); }); // Create some shared data structures. @@ -286,4 +245,4 @@ std::vector SortPlugins( return newLoadOrder; } -} \ No newline at end of file +} diff --git a/src/tests/api/internals/sorting/plugin_graph_test.h b/src/tests/api/internals/sorting/plugin_graph_test.h index 1d595e62..7179ab59 100644 --- a/src/tests/api/internals/sorting/plugin_graph_test.h +++ b/src/tests/api/internals/sorting/plugin_graph_test.h @@ -46,9 +46,7 @@ public: return std::optional(); } - std::vector GetMasters() const override { - return std::vector(); - } + std::vector GetMasters() const override { return masters_; } std::vector GetBashTags() const override { return std::vector(); } @@ -91,6 +89,8 @@ public: otherPlugin->assetsOverlapWith.count(this) != 0; } + void AddMaster(const std::string& master) { masters_.push_back(master); } + void AddOverlappingRecords(const PluginInterface& plugin) { recordsOverlapWith.insert(&plugin); } @@ -107,6 +107,7 @@ public: private: std::string name_; + std::vector masters_; std::set recordsOverlapWith; std::set assetsOverlapWith; size_t overrideRecordCount_{0}; diff --git a/src/tests/api/internals/sorting/plugin_sort_test.h b/src/tests/api/internals/sorting/plugin_sort_test.h index ef85bb1e..85ecdf1b 100644 --- a/src/tests/api/internals/sorting/plugin_sort_test.h +++ b/src/tests/api/internals/sorting/plugin_sort_test.h @@ -28,6 +28,7 @@ along with LOOT. If not, see #include "api/sorting/plugin_sort.h" #include "loot/exception/cyclic_interaction_error.h" #include "loot/exception/undefined_group_error.h" +#include "tests/api/internals/sorting/plugin_graph_test.h" #include "tests/common_game_test_fixture.h" namespace loot { @@ -40,7 +41,7 @@ protected: masterlistPath_(metadataFilesPath / "userlist.yaml"), cccPath_(dataPath.parent_path() / getCCCFilename()) {} - void loadInstalledPlugins(Game &game, bool headersOnly) { + void loadInstalledPlugins(Game& game, bool headersOnly) { std::vector plugins({ masterFile, blankEsm, @@ -116,10 +117,38 @@ protected: } } + PluginSortingData CreatePluginSortingData( + const std::string& name, + const std::vector& loadOrder) { + const auto plugin = GetPlugin(name); + + return PluginSortingData(plugin, + PluginMetadata(), + PluginMetadata(), + loadOrder, + GameType::tes4, + {}); + } + + plugingraph::TestPlugin* GetPlugin(const std::string& name) { + auto it = testPlugins_.find(name); + + if (it != testPlugins_.end()) { + return it->second.get(); + } + + const auto plugin = std::make_shared(name); + + return testPlugins_.insert_or_assign(name, plugin).first->second.get(); + } + Game game_; const std::string blankEslEsp; const std::filesystem::path masterlistPath_; const std::filesystem::path cccPath_; + +private: + std::map> testPlugins_; }; // Pass an empty first argument, as it's a prefix for the test instantation, @@ -149,6 +178,58 @@ TEST_P(PluginSortTest, } } +TEST_P(PluginSortTest, + sortingShouldNotChangeTheResultIfGivenItsOwnOutputLoadOrder) { + // Can't test with the test plugin files, so use the other SortPlugins() + // overload to provide stubs. + const auto p1 = GetPlugin("1.esp"); + const auto p2 = GetPlugin("2.esp"); + const auto p3 = GetPlugin("3.esp"); + + p1->AddMaster(p3->GetName()); + + p1->AddOverlappingRecords(*p2); + p1->AddOverlappingRecords(*p3); + p2->AddOverlappingRecords(*p3); + p1->SetOverrideRecordCount(3); + p2->SetOverrideRecordCount(2); + p3->SetOverrideRecordCount(1); + + // Define the initial load order. + std::vector loadOrder{ + p1->GetName(), p2->GetName(), p3->GetName()}; + + const std::vector expectedSortedOrder{ + p3->GetName(), p1->GetName(), p2->GetName()}; + + // Now sort the plugins. + { + std::vector pluginsSortingData{ + CreatePluginSortingData(p1->GetName(), loadOrder), + CreatePluginSortingData(p2->GetName(), loadOrder), + CreatePluginSortingData(p3->GetName(), loadOrder)}; + + auto sorted = SortPlugins( + std::move(pluginsSortingData), GetParam(), {Group()}, {}, {}); + ASSERT_EQ(expectedSortedOrder, sorted); + + loadOrder = sorted; + } + + // Now do it again but supplying the sorted load order as the current load + // order. + { + std::vector pluginsSortingData{ + CreatePluginSortingData(p1->GetName(), loadOrder), + CreatePluginSortingData(p2->GetName(), loadOrder), + CreatePluginSortingData(p3->GetName(), loadOrder)}; + + auto sorted = SortPlugins( + std::move(pluginsSortingData), GetParam(), {Group()}, {}, {}); + ASSERT_EQ(expectedSortedOrder, sorted); + } +} + TEST_P(PluginSortTest, sortingShouldResolveGroupsAsTransitiveLoadAfterSets) { ASSERT_NO_THROW(loadInstalledPlugins(game_, false)); @@ -336,26 +417,14 @@ TEST_P( SortPlugins(game_, game_.GetLoadOrder()); FAIL(); } catch (CyclicInteractionError &e) { - if (GetParam() == GameType::fo4) { - ASSERT_EQ(4, e.GetCycle().size()); - EXPECT_EQ("Blank.esm", e.GetCycle()[0].GetName()); - EXPECT_EQ(EdgeType::master, e.GetCycle()[0].GetTypeOfEdgeToNextVertex()); - EXPECT_EQ("Blank - Master Dependent.esm", e.GetCycle()[1].GetName()); - EXPECT_EQ(EdgeType::userGroup, e.GetCycle()[1].GetTypeOfEdgeToNextVertex()); - EXPECT_EQ("Blank - Different.esm", e.GetCycle()[2].GetName()); - EXPECT_EQ(EdgeType::master, e.GetCycle()[2].GetTypeOfEdgeToNextVertex()); - EXPECT_EQ("Blank - Different Master Dependent.esm", - e.GetCycle()[3].GetName()); - EXPECT_EQ(EdgeType::userGroup, e.GetCycle()[3].GetTypeOfEdgeToNextVertex()); - } else { - ASSERT_EQ(3, e.GetCycle().size()); - EXPECT_EQ(masterFile, e.GetCycle()[0].GetName()); - EXPECT_EQ(EdgeType::userGroup, e.GetCycle()[0].GetTypeOfEdgeToNextVertex()); - EXPECT_EQ("Blank.esm", e.GetCycle()[1].GetName()); - EXPECT_EQ(EdgeType::master, e.GetCycle()[1].GetTypeOfEdgeToNextVertex()); - EXPECT_EQ("Blank - Master Dependent.esm", e.GetCycle()[2].GetName()); - EXPECT_EQ(EdgeType::userGroup, e.GetCycle()[2].GetTypeOfEdgeToNextVertex()); - } + ASSERT_EQ(3, e.GetCycle().size()); + EXPECT_EQ("Blank - Different Master Dependent.esm", + e.GetCycle()[0].GetName()); + EXPECT_EQ(EdgeType::userGroup, e.GetCycle()[0].GetTypeOfEdgeToNextVertex()); + EXPECT_EQ("Blank.esm", e.GetCycle()[1].GetName()); + EXPECT_EQ(EdgeType::master, e.GetCycle()[1].GetTypeOfEdgeToNextVertex()); + EXPECT_EQ("Blank - Master Dependent.esm", e.GetCycle()[2].GetName()); + EXPECT_EQ(EdgeType::userGroup, e.GetCycle()[2].GetTypeOfEdgeToNextVertex()); } }