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

@@ -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
 ===================
 

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``.

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<vertex_t> 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);

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<PluginSortingData> GetPluginsSortingData(
     const GameType gameType,
     const DatabaseInterface& db,
@@ -206,12 +164,13 @@ std::vector<std::string> 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<std::string> SortPlugins(
 
   return newLoadOrder;
 }
-}
+}

src/tests/api/internals/sorting/plugin_graph_test.h

@@ -46,9 +46,7 @@ public:
     return std::optional<std::string>();
   }
 
-  std::vector<std::string> GetMasters() const override {
-    return std::vector<std::string>();
-  }
+  std::vector<std::string> GetMasters() const override { return masters_; }
 
   std::vector<Tag> GetBashTags() const override { return std::vector<Tag>(); }
 
@@ -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<std::string> masters_;
   std::set<const PluginInterface*> recordsOverlapWith;
   std::set<const PluginSortingInterface*> assetsOverlapWith;
   size_t overrideRecordCount_{0};

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<std::string> plugins({
         masterFile,
         blankEsm,
@@ -116,10 +117,38 @@ protected:
     }
   }
 
+  PluginSortingData CreatePluginSortingData(
+      const std::string& name,
+      const std::vector<std::string>& 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<plugingraph::TestPlugin>(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<std::string, std::shared_ptr<plugingraph::TestPlugin>> 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<std::string> loadOrder{
+      p1->GetName(), p2->GetName(), p3->GetName()};
+
+  const std::vector<std::string> expectedSortedOrder{
+      p3->GetName(), p1->GetName(), p2->GetName()};
+
+  // Now sort the plugins.
+  {
+    std::vector<PluginSortingData> 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<PluginSortingData> 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());
   }
 }