diff --git a/src/api/sorting/plugin_sorter.cpp b/src/api/sorting/plugin_sorter.cpp index 67b21169..0532f0e5 100644 --- a/src/api/sorting/plugin_sorter.cpp +++ b/src/api/sorting/plugin_sorter.cpp @@ -394,6 +394,17 @@ bool shouldIgnorePlugin( return false; } +bool shouldIgnoreGroupEdge( + const PluginSortingData& fromPlugin, + const PluginSortingData& toPlugin, + const std::map>& + groupPluginsToIgnore) { + return shouldIgnorePlugin( + fromPlugin.GetGroup(), toPlugin.GetName(), groupPluginsToIgnore) || + shouldIgnorePlugin( + toPlugin.GetGroup(), fromPlugin.GetName(), groupPluginsToIgnore); +} + void ignorePlugin(const std::string& pluginName, const std::unordered_set& groups, std::map>& @@ -488,29 +499,38 @@ void PluginSorter::AddGroupEdges() { for (const auto& pluginName : graph_[vertex].GetAfterGroupPlugins()) { vertex_t parentVertex; if (GetVertexByName(pluginName, parentVertex)) { - bool ignore = shouldIgnorePlugin(graph_[vertex].GetGroup(), - graph_[parentVertex].GetName(), - groupPluginsToIgnore); + if (EdgeCreatesCycle(parentVertex, vertex)) { + auto& fromPlugin = graph_[parentVertex]; + auto& toPlugin = graph_[vertex]; - if (ignore || EdgeCreatesCycle(parentVertex, vertex)) { if (logger_) { logger_->trace( "Skipping edge from \"{}\" to \"{}\" as it would " "create a cycle.", - graph_[parentVertex].GetName(), - graph_[vertex].GetName()); + fromPlugin.GetName(), + toPlugin.GetName()); } - // Get the groups of the two plugins, and find all groups that are - // in paths that sit between those two groups. Group-derived edges - // from the plugin at parentVertex to any plugins in those groups - // should then be ignored as they also create cycles. - auto groupsInPaths = getGroupsInPaths(groups_, - graph_[parentVertex].GetGroup(), - graph_[vertex].GetGroup()); - ignorePlugin(graph_[parentVertex].GetName(), - groupsInPaths, - groupPluginsToIgnore); + // The default group is a special case, as it's given to plugins + // with no metadata. If a plugin in the default group causes + // a cycle due to its group, ignore that plugin's group for all + // groups in the group graph paths between default and the other + // plugin's group. + std::string pluginToIgnore; + if (toPlugin.GetGroup() == Group().GetName()) { + pluginToIgnore = toPlugin.GetName(); + } else if (fromPlugin.GetGroup() == Group().GetName()) { + pluginToIgnore = fromPlugin.GetName(); + } else { + // If neither plugin is in the default group, it's impossible + // to decide which group to ignore, so ignore neither of them. + continue; + } + + auto groupsInPaths = getGroupsInPaths( + groups_, fromPlugin.GetGroup(), toPlugin.GetGroup()); + + ignorePlugin(pluginToIgnore, groupsInPaths, groupPluginsToIgnore); continue; } @@ -525,17 +545,19 @@ void PluginSorter::AddGroupEdges() { "Adding group edges that don't individually introduce cycles."); } for (const auto& edgePair : acyclicEdgePairs) { - bool ignore = shouldIgnorePlugin(graph_[edgePair.second].GetGroup(), - graph_[edgePair.first].GetName(), - groupPluginsToIgnore); + auto& fromPlugin = graph_[edgePair.first]; + auto& toPlugin = graph_[edgePair.second]; + bool ignore = + shouldIgnoreGroupEdge(fromPlugin, toPlugin, groupPluginsToIgnore); + if (!ignore) { AddEdge(edgePair.first, edgePair.second); } else if (logger_) { logger_->trace( "Skipping edge from \"{}\" to \"{}\" as it would " "create a multi-group cycle.", - graph_[edgePair.first].GetName(), - graph_[edgePair.second].GetName()); + fromPlugin.GetName(), + toPlugin.GetName()); } } } diff --git a/src/tests/api/internals/sorting/plugin_sorter_test.h b/src/tests/api/internals/sorting/plugin_sorter_test.h index 08a77179..f84fe791 100644 --- a/src/tests/api/internals/sorting/plugin_sorter_test.h +++ b/src/tests/api/internals/sorting/plugin_sorter_test.h @@ -83,6 +83,13 @@ protected: boost::filesystem::ofstream masterlist(masterlistPath_); masterlist << "groups:" << endl + << " - name: earliest" << endl + << " - name: earlier" << endl + << " after:" << endl + << " - earliest" << endl + << " - name: default" << endl + << " after:" << endl + << " - earlier" << endl << " - name: group1" << endl << " - name: group2" << endl << " after:" << endl @@ -248,27 +255,24 @@ TEST_P(PluginSorterTest, TEST_P( PluginSorterTest, - sortingShouldIgnoreGroupsThatContradictAnotherGroupInCombinationWithMoreSpecificMetadata) { + sortingShouldIgnoreGroupEdgesInvolvedInABackCycleOfAGroupEdgeFromADefaultGroupPlugin) { ASSERT_NO_THROW(loadInstalledPlugins(game_, false)); GenerateMasterlist(); game_.GetDatabase()->LoadLists(masterlistPath_.string()); PluginMetadata plugin(blankEsp); - plugin.SetGroup("group1"); - game_.GetDatabase()->SetPluginUserMetadata(plugin); plugin = PluginMetadata(blankDifferentMasterDependentEsp); - plugin.SetGroup("group1"); plugin.SetLoadAfterFiles(std::set({File(blankMasterDependentEsp)})); game_.GetDatabase()->SetPluginUserMetadata(plugin); plugin = PluginMetadata(blankDifferentEsp); - plugin.SetGroup("group2"); + plugin.SetGroup("group1"); game_.GetDatabase()->SetPluginUserMetadata(plugin); plugin = PluginMetadata(blankMasterDependentEsp); - plugin.SetGroup("group3"); + plugin.SetGroup("group2"); game_.GetDatabase()->SetPluginUserMetadata(plugin); PluginSorter ps; @@ -294,6 +298,69 @@ TEST_P( EXPECT_EQ(expectedSortedOrder, sorted); } +TEST_P( + PluginSorterTest, + sortingShouldIgnoreGroupEdgesInvolvedInABackCycleOfAGroupEdgeToADefaultGroupPlugin) { + ASSERT_NO_THROW(loadInstalledPlugins(game_, false)); + + GenerateMasterlist(); + game_.GetDatabase()->LoadLists(masterlistPath_.string()); + + PluginMetadata plugin(blankMasterDependentEsm); + plugin.SetGroup("earliest"); + game_.GetDatabase()->SetPluginUserMetadata(plugin); + + plugin = PluginMetadata(blankDifferentEsm); + plugin.SetGroup("earlier"); + game_.GetDatabase()->SetPluginUserMetadata(plugin); + + PluginSorter ps; + std::vector expectedSortedOrder({ + blankEsm, + blankMasterDependentEsm, + blankDifferentEsm, + masterFile, + blankDifferentMasterDependentEsm, + blankEsp, + blankDifferentEsp, + blankMasterDependentEsp, + blankDifferentMasterDependentEsp, + blankPluginDependentEsp, + blankDifferentPluginDependentEsp, + }); + + if (GetParam() == GameType::fo4 || GetParam() == GameType::tes5se) { + expectedSortedOrder.insert(expectedSortedOrder.begin() + 5, blankEsl); + } + + std::vector sorted = ps.Sort(game_); + EXPECT_EQ(expectedSortedOrder, sorted); +} + +TEST_P( + PluginSorterTest, + sortingShouldThrowForAGroupEdgeThatCausesAMultiGroupCycleBetweenTwoNonDefaultGroups) { + ASSERT_NO_THROW(loadInstalledPlugins(game_, false)); + + GenerateMasterlist(); + game_.GetDatabase()->LoadLists(masterlistPath_.string()); + + PluginMetadata plugin(blankMasterDependentEsm); + plugin.SetGroup("earliest"); + game_.GetDatabase()->SetPluginUserMetadata(plugin); + + plugin = PluginMetadata(blankDifferentEsm); + plugin.SetGroup("earlier"); + game_.GetDatabase()->SetPluginUserMetadata(plugin); + + plugin = PluginMetadata(blankEsm); + plugin.SetGroup("group4"); + game_.GetDatabase()->SetPluginUserMetadata(plugin); + + PluginSorter ps; + EXPECT_THROW(ps.Sort(game_), CyclicInteractionError); +} + TEST_P( PluginSorterTest, sortingShouldNotIgnorePluginsInTheSameGroupAsTheTargetPluginOfAGroupEdgeThatCausesACycleInIsolation) {