Fix group cyclic interactions

Skip adding an edge between a pair of plugins if all the following
conditions are met:
- the edge is to be added due to group membership
- adding the edge would introduce a cycle in the absence of any
  other group membership edges.

docs/api/sorting.rst

@@ -26,7 +26,7 @@ Create plugin graph vertices
 Once loaded, a directed graph is created and the plugins are added to it in
 lexicographical order as vertices. Any metadata a plugin has in the masterlist
 and userlist are then merged into its vertex's data store. Plugin group
-dependencies are also resolved and added as additional load after plugins.
+dependencies are also resolved and added as group-derived plugins.
 
 Create plugin graph edges
 ==============================
@@ -46,6 +46,12 @@ For each plugin:
 4. Add edges coming from all the plugin's load after files that are installed
    plugins.
 
+Group-derived interdependencies are then evaluated. Each plugin's group-derived
+plugins are iterated over and individually checked to see if adding an edge from
+the group-derived plugin to the plugin would cause a cycle, and if not the edge
+is recorded. Once all potential edges have been checked, the recorded edges are
+added to the graph.
+
 At this point, all explicit interdependencies have been graphed. Plugin priority
 metadata values must now be propagated down the dependency trees to ensure that
 priority edges are added correctly later in the process. To do this:

docs/metadata/data_structures/group.rst

@@ -19,8 +19,12 @@ This structure can be used to hold group definitions. It is a key-value map.
   The names of groups that this group loads after. Group names are
   case-sensitive. If undefined, the set is empty. The named groups must be
   defined when LOOT sorts plugins, but they don't need to be defined in the same
-  metadata file. If at sort time a group is defined to load after a group that
-  does not exist, a sorting error will occur.
+  metadata file.
+
+  Sorting errors will occur if:
+
+  - A group loads after another group that does not exist.
+  - Group loading is cyclic (e.g. A loads after B and B loads after A).
 
 Merging Groups
 --------------

docs/metadata/data_structures/plugin.rst

@@ -25,10 +25,33 @@ This is the structure that brings all the others together, and forms the main co
   be defined in the same metadata file. If at sort time the group does not
   exist, a sorting error will occur.
 
-  A plugin must load after all the plugins in the groups its group is defined to
-  load after. Group loading is resolved recursively. For example, if group C
-  loads after group B, and group B loads after group A, a plugin in C must load
-  after all the plugins in A even if no plugins in B are installed.
+  The plugin must load after all the plugins in the groups its group is defined
+  to load after, resolving them recursively. An exception exists if doing so
+  would introduce a cyclic dependency between two plugins without any other
+  group loading rules applied.
+
+  For example, if for plugins A.esp, B.esp, C.esp and D.esp:
+
+  - B.esp has A.esp as a master
+  - A.esp is in group A
+  - B.esp and C.esp are in the default group
+  - D.esp is in group D
+  - group A loads after the default group
+  - the default group loads after group D
+
+  Then the load order must be D.esp, C.esp, A.esp, B.esp. Although A.esp's group
+  must load after B.esp's group, this would cause a cycle between A.esp and
+  B.esp, so the requirement is ignored for that pair of plugins.
+
+  However, if for plugins A.esp, B.esp and C.esp in groups of the same names:
+
+  1. group B loads after group A
+  2. group C loads after group B
+  3. A.esp has C.esp as a master
+
+  This will cause a sorting error, as neither group rule introduces a cyclic
+  dependency when combined in isolation with the third rule, but having all
+  three rules applied causes a cycle.
 
 .. describe:: priority
 

src/api/sorting/plugin_sorter.cpp

@@ -131,6 +131,8 @@ std::vector<std::string> PluginSorter::Sort(Game& game) {
   }
   AddSpecificEdges();
 
+  AddGroupEdges();
+
   PropagatePriorities();
 
   if (logger_) {
@@ -268,11 +270,7 @@ void PluginSorter::AddPluginVertices(Game& game) {
         "\" set for plugin \"" + plugin.GetName() + "\" does not exist.");
     }
     else {
-      auto loadAfter = plugin.GetLoadAfterFiles();
-      for (const auto& afterPlugin : groupsIt->second) {
-        loadAfter.insert(File(afterPlugin));
-      }
-      plugin.SetLoadAfterFiles(loadAfter);
+      plugin.SetAfterGroupPlugins(groupsIt->second);
     }
   }
 
@@ -473,6 +471,41 @@ void PluginSorter::AddSpecificEdges() {
   }
 }
 
+void PluginSorter::AddGroupEdges() {
+  std::vector<std::pair<vertex_t, vertex_t>> acyclicEdgePairs;
+  for (const vertex_t& vertex :
+       boost::make_iterator_range(boost::vertices(graph_))) {
+    if (logger_) {
+      logger_->trace("Checking group edges for \"{}\".",
+        graph_[vertex].GetName());
+    }
+    for (const auto& pluginName : graph_[vertex].GetAfterGroupPlugins()) {
+      vertex_t parentVertex;
+      if (GetVertexByName(pluginName, parentVertex)) {
+        if (EdgeCreatesCycle(parentVertex, vertex)) {
+          if (logger_) {
+            logger_->trace("Skipping edge from \"{}\" to \"{}\" as it would "
+              "create a cycle and one or both plugins belongs to the default "
+              "group.",
+              graph_[parentVertex].GetName(),
+              graph_[vertex].GetName());
+          }
+          continue;
+        }
+
+        acyclicEdgePairs.push_back(std::make_pair(parentVertex, vertex));
+      }
+    }
+  }
+
+  if (logger_) {
+    logger_->trace("Adding group edges that don't individually introduce cycles.");
+  }
+  for (const auto& edgePair : acyclicEdgePairs) {
+    AddEdge(edgePair.first, edgePair.second);
+  }
+}
+
 void PluginSorter::AddPriorityEdges() {
   for (const auto& vertex :
        boost::make_iterator_range(boost::vertices(graph_))) {

src/api/sorting/plugin_sorter.h

@@ -61,6 +61,7 @@ private:
 
   void AddPluginVertices(Game& game);
   void AddSpecificEdges();
+  void AddGroupEdges();
   void AddPriorityEdges();
   void AddOverlapEdges();
   void AddTieBreakEdges();

src/api/sorting/plugin_sorting_data.cpp

@@ -51,4 +51,12 @@ bool PluginSortingData::DoFormIDsOverlap(
     const PluginSortingData& plugin) const {
   return plugin_.DoFormIDsOverlap(plugin.plugin_);
 }
+
+std::unordered_set<std::string> PluginSortingData::GetAfterGroupPlugins() const {
+  return afterGroupPlugins_;
+}
+
+void PluginSortingData::SetAfterGroupPlugins(std::unordered_set<std::string> plugins) {
+  afterGroupPlugins_ = plugins;
+}
 }

src/api/sorting/plugin_sorting_data.h

@@ -40,6 +40,9 @@ public:
   size_t NumOverrideFormIDs() const;
   bool DoFormIDsOverlap(const PluginSortingData& plugin) const;
 
+  std::unordered_set<std::string> GetAfterGroupPlugins() const;
+  void SetAfterGroupPlugins(std::unordered_set<std::string> plugins);
+
   using PluginMetadata::GetGlobalPriority;
   using PluginMetadata::GetGroup;
   using PluginMetadata::GetLoadAfterFiles;
@@ -47,10 +50,10 @@ public:
   using PluginMetadata::GetRequirements;
   using PluginMetadata::SetGlobalPriority;
   using PluginMetadata::SetLocalPriority;
-  using PluginMetadata::SetLoadAfterFiles;
 
 private:
   const Plugin& plugin_;
+  std::unordered_set<std::string> afterGroupPlugins_;
 };
 }
 

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

@@ -87,7 +87,10 @@ protected:
       << "      - group1" << endl
       << "  - name: group3" << endl
       << "    after:" << endl
-      << "      - group2" << endl;
+      << "      - group2" << endl
+      << "  - name: group4" << endl
+      << "    after:" << endl
+      << "      - default" << endl;
 
     masterlist.close();
   }
@@ -207,15 +210,18 @@ TEST_P(PluginSorterTest, sortingShouldThrowIfAPluginHasAGroupThatDoesNotExist) {
   EXPECT_THROW(ps.Sort(game_), std::invalid_argument);
 }
 
-TEST_P(PluginSorterTest, sortingShouldThrowIfAGroupIntroducesACycle) {
+TEST_P(PluginSorterTest, sortingShouldThrowIfAddingTwoGroupEdgesIntroducesACycle) {
   ASSERT_NO_THROW(loadInstalledPlugins(game_, false));
 
   GenerateMasterlist();
   game_.GetDatabase()->LoadLists(masterlistPath_.string());
 
-  PluginMetadata plugin(blankDifferentEsm);
+  PluginMetadata plugin(blankMasterDependentEsm);
   plugin.SetGroup("group1");
-  plugin.SetLoadAfterFiles({ File(blankEsm) });
+  game_.GetDatabase()->SetPluginUserMetadata(plugin);
+
+  plugin = PluginMetadata(blankDifferentEsm);
+  plugin.SetGroup("group2");
   game_.GetDatabase()->SetPluginUserMetadata(plugin);
 
   plugin = PluginMetadata(blankEsm);
@@ -226,6 +232,39 @@ TEST_P(PluginSorterTest, sortingShouldThrowIfAGroupIntroducesACycle) {
   EXPECT_THROW(ps.Sort(game_), CyclicInteractionError);
 }
 
+TEST_P(PluginSorterTest, sortingShouldIgnoreAGroupEdgeIfItWouldCauseACycle) {
+  ASSERT_NO_THROW(loadInstalledPlugins(game_, false));
+
+  GenerateMasterlist();
+  game_.GetDatabase()->LoadLists(masterlistPath_.string());
+
+  PluginMetadata plugin(blankEsm);
+  plugin.SetGroup("group4");
+  game_.GetDatabase()->SetPluginUserMetadata(plugin);
+
+  PluginSorter ps;
+  std::vector<std::string> expectedSortedOrder({
+    masterFile,
+    blankDifferentEsm,
+    blankDifferentMasterDependentEsm,
+    blankEsm,
+    blankMasterDependentEsm,
+    blankEsp,
+    blankDifferentEsp,
+    blankMasterDependentEsp,
+    blankDifferentMasterDependentEsp,
+    blankPluginDependentEsp,
+    blankDifferentPluginDependentEsp,
+    });
+
+  if (GetParam() == GameType::fo4 || GetParam() == GameType::tes5se) {
+    expectedSortedOrder.insert(expectedSortedOrder.begin() + 3, blankEsl);
+  }
+
+  std::vector<std::string> sorted = ps.Sort(game_);
+  EXPECT_EQ(expectedSortedOrder, sorted);
+}
+
 TEST_P(PluginSorterTest, sortingShouldEvaluateRelativeGlobalPriorities) {
   ASSERT_NO_THROW(loadInstalledPlugins(game_, false));
   PluginMetadata plugin(blankDifferentMasterDependentEsp);