diff --git a/include/loot/enum/edge_type.h b/include/loot/enum/edge_type.h index a50e5b5b..e81a7d5f 100644 --- a/include/loot/enum/edge_type.h +++ b/include/loot/enum/edge_type.h @@ -41,7 +41,8 @@ enum struct EdgeType : unsigned int { userRequirement, masterlistLoadAfter, userLoadAfter, - group, + masterlistGroup, + userGroup, overlap, tieBreak, }; diff --git a/src/api/sorting/group_sort.cpp b/src/api/sorting/group_sort.cpp index fbdeb764..c1f0cea9 100644 --- a/src/api/sorting/group_sort.cpp +++ b/src/api/sorting/group_sort.cpp @@ -44,21 +44,44 @@ typedef boost::graph_traits::vertex_descriptor vertex_t; typedef boost::graph_traits::edge_descriptor edge_t; typedef boost::associative_property_map> edge_map_t; -class AfterGroupsVisitor : public boost::dfs_visitor<> { +// This visitor is responsible for recording a vertex's successor vertices, +// and whether they are reachable without user metadata or not. However, a +// vertex is only recorded the first time it's discovered, vertices and +// edges are iterated over in their insertion order, and masterlist metadata +// is inserted first, so it depends on the structure and sources of the data +// which path is encountered first. +// E.g. for vertices A, B and C, if C -> B and C -> A are masterlist edges +// added in that order and B -> A is a userlist edge then C -> B -> A will +// be visited first and A will then have been finished so C -> A won't be +// recorded, so it'll look the relationship between C and A involves user +// metadata when it isn't necessary. +class PredecessorGroupsVisitor : public boost::dfs_visitor<> { public: - AfterGroupsVisitor(std::unordered_set& visitedGroups) : + PredecessorGroupsVisitor(std::vector& visitedGroups) : visitedGroups_(visitedGroups) {} - void tree_edge(edge_t edge, const GroupGraph& graph) { - auto target = boost::target(edge, graph); - visitedGroups_.insert(graph[target]); + + void discover_vertex(vertex_t vertex, const GroupGraph& graph) { + pathStack_.push_back(vertex); + + if (pathStack_.size() > 1) { + bool pathInvolvesUserMetadata = false; + for (size_t i = 0; i < pathStack_.size() - 1; i += 1) { + const auto edge = boost::edge(pathStack_[i], pathStack_[i + 1], graph); + + pathInvolvesUserMetadata |= + graph[edge.first] == EdgeType::userLoadAfter; + } + + visitedGroups_.push_back( + PredecessorGroup{graph[vertex], pathInvolvesUserMetadata}); + } } - std::unordered_set get_visited_groups() const { - return visitedGroups_; - } + void finish_vertex(vertex_t, const GroupGraph&) { pathStack_.pop_back(); } private: - std::unordered_set& visitedGroups_; + std::vector& visitedGroups_; + std::vector pathStack_; }; class CycleDetector : public boost::dfs_visitor<> { @@ -102,19 +125,20 @@ private: std::vector trail; }; -std::string joinVector(const std::vector& set) { +std::string joinVector(const std::vector& container) { std::string output; - for (const auto& element : set) { + for (const auto& element : container) { output += element + ", "; } return output.substr(0, output.length() - 2); } -std::string joinUnorderedSet(const std::unordered_set& set) { +std::string joinVector(const std::vector& container) { std::string output; - for (const auto& element : set) { - output += element + ", "; + for (const auto& element : container) { + const auto via = element.pathInvolvesUserMetadata ? "user" : "masterlist"; + output += element.name + " (via " + via + " metadata), "; } return output.substr(0, output.length() - 2); @@ -180,9 +204,9 @@ GroupGraph BuildGraph(const std::vector& masterlistGroups, return graph; } -std::unordered_map> -GetTransitiveAfterGroups(const std::vector& masterlistGroups, - const std::vector& userGroups) { +std::unordered_map> +GetPredecessorGroups(const std::vector& masterlistGroups, + const std::vector& userGroups) { GroupGraph graph = BuildGraph(masterlistGroups, userGroups); auto logger = getLogger(); @@ -196,12 +220,12 @@ GetTransitiveAfterGroups(const std::vector& masterlistGroups, } boost::depth_first_search(graph, boost::visitor(CycleDetector())); - std::unordered_map> + std::unordered_map> transitiveAfterGroups; for (const vertex_t& vertex : boost::make_iterator_range(boost::vertices(graph))) { - std::unordered_set visitedGroups; - const AfterGroupsVisitor afterGroupsVisitor(visitedGroups); + std::vector visitedGroups; + const PredecessorGroupsVisitor afterGroupsVisitor(visitedGroups); // Create a color map. std::vector colorVec(boost::num_vertices(graph)); @@ -216,7 +240,7 @@ GetTransitiveAfterGroups(const std::vector& masterlistGroups, if (logger) { logger->debug("Group \"{}\" transitively loads after groups \"{}\"", graph[vertex], - joinUnorderedSet(visitedGroups)); + joinVector(visitedGroups)); } } diff --git a/src/api/sorting/group_sort.h b/src/api/sorting/group_sort.h index ce7efa3c..16c22be6 100644 --- a/src/api/sorting/group_sort.h +++ b/src/api/sorting/group_sort.h @@ -27,17 +27,21 @@ #include #include -#include #include #include "loot/metadata/group.h" #include "loot/vertex.h" namespace loot { +struct PredecessorGroup { + std::string name; + bool pathInvolvesUserMetadata{false}; +}; + // Map entries are a group name and names of transitive load after groups. -std::unordered_map> -GetTransitiveAfterGroups(const std::vector& masterlistGroups, - const std::vector& userGroups); +std::unordered_map> +GetPredecessorGroups(const std::vector& masterlistGroups, + const std::vector& userGroups); std::vector GetGroupsPath(const std::vector& masterlistGroups, const std::vector& userGroups, diff --git a/src/api/sorting/plugin_graph.cpp b/src/api/sorting/plugin_graph.cpp index ca4aef12..6987e894 100644 --- a/src/api/sorting/plugin_graph.cpp +++ b/src/api/sorting/plugin_graph.cpp @@ -211,8 +211,10 @@ std::string describeEdgeType(EdgeType edgeType) { return "Masterlist Load After"; case EdgeType::userLoadAfter: return "User Load After"; - case EdgeType::group: - return "Group"; + case EdgeType::masterlistGroup: + return "Masterlist Group"; + case EdgeType::userGroup: + return "User Group"; case EdgeType::overlap: return "Overlap"; case EdgeType::tieBreak: @@ -715,16 +717,17 @@ void PluginGraph::AddGroupEdges( logger->trace("Adding edges based on plugin group memberships..."); } - std::vector> acyclicEdgePairs; + // Tuple fields are from, to, and edge type. + std::vector> acyclicEdges; std::map> groupPluginsToIgnore; for (const vertex_t& vertex : boost::make_iterator_range(GetVertices())) { const auto& toPlugin = GetPlugin(vertex); - for (const auto& pluginName : toPlugin.GetAfterGroupPlugins()) { + for (const auto& plugin : toPlugin.GetPredecessorGroupPlugins()) { // After group plugin names are taken from other PluginSortingData names, // so exact string comparisons can be used. - const auto parentVertex = GetVertexByExactName(pluginName); + const auto parentVertex = GetVertexByExactName(plugin.name); if (!parentVertex.has_value()) { continue; } @@ -775,22 +778,31 @@ void PluginGraph::AddGroupEdges( continue; } - acyclicEdgePairs.push_back(std::make_pair(parentVertex.value(), vertex)); + const auto edgeType = plugin.pathInvolvesUserMetadata + ? EdgeType::userGroup + : EdgeType::masterlistGroup; + + acyclicEdges.push_back( + std::make_tuple(parentVertex.value(), vertex, edgeType)); } } - for (const auto& edgePair : acyclicEdgePairs) { - const auto& fromPlugin = GetPlugin(edgePair.first); - const auto& toPlugin = GetPlugin(edgePair.second); + for (const auto& edge : acyclicEdges) { + const auto fromVertex = std::get<0>(edge); + const auto toVertex = std::get<1>(edge); + const auto edgeType = std::get<2>(edge); + const auto& fromPlugin = GetPlugin(fromVertex); + const auto& toPlugin = GetPlugin(toVertex); const bool ignore = ShouldIgnoreGroupEdge(fromPlugin, toPlugin, groupPluginsToIgnore); if (!ignore) { - AddEdge(edgePair.first, edgePair.second, EdgeType::group); + AddEdge(fromVertex, toVertex, edgeType); } else if (logger) { logger->debug( - "Skipping group edge from \"{}\" to \"{}\" as it would " + "Skipping {} edge from \"{}\" to \"{}\" as it would " "create a multi-group cycle.", + describeEdgeType(edgeType), fromPlugin.GetName(), toPlugin.GetName()); } diff --git a/src/api/sorting/plugin_sort.cpp b/src/api/sorting/plugin_sort.cpp index 3d73189c..410e8999 100644 --- a/src/api/sorting/plugin_sort.cpp +++ b/src/api/sorting/plugin_sort.cpp @@ -32,6 +32,39 @@ #include "loot/exception/undefined_group_error.h" namespace loot { +std::unordered_map> +GetPredecessorGroupPlugins( + const std::unordered_map>>& + groupPlugins, + const std::unordered_map>& + predecessorGroupsMap) { + std::unordered_map> + predecessorGroupsPlugins; + for (const auto& group : predecessorGroupsMap) { + std::vector predecessorGroupPlugins; + + for (const auto& predecessorGroup : group.second) { + const auto pluginsIt = groupPlugins.find(predecessorGroup.name); + if (pluginsIt != groupPlugins.end()) { + // If the path from the predecessor group to this one involves user + // metadata, the plugins' paths all involve user metadata, otherwise + // only those plugins that belong to the predecessor group due to user + // metadata have a path involving user metadata. + for (const auto& groupPlugin : pluginsIt->second) { + predecessorGroupPlugins.push_back(PredecessorGroupPlugin{ + groupPlugin.first, + predecessorGroup.pathInvolvesUserMetadata || groupPlugin.second}); + } + } + } + + predecessorGroupsPlugins.insert({group.first, predecessorGroupPlugins}); + } + + return predecessorGroupsPlugins; +} + int ComparePlugins(const PluginSortingData& plugin1, const PluginSortingData& plugin2) { if (plugin1.GetLoadOrderIndex().has_value() && @@ -131,33 +164,33 @@ std::vector GetPluginsSortingData( pluginsSortingData.push_back(pluginSortingData); } - std::unordered_map> groupPlugins; + // Each element of the vector is a pair of a plugin name and if it's in the + // group due to user metadata. + std::unordered_map>> + groupPlugins; for (const auto& plugin : pluginsSortingData) { const auto groupName = plugin.GetGroup(); + const auto groupPlugin = + std::make_pair(plugin.GetName(), plugin.IsGroupUserMetadata()); const auto groupIt = groupPlugins.find(groupName); + if (groupIt == groupPlugins.end()) { - groupPlugins.emplace(groupName, - std::vector({plugin.GetName()})); + groupPlugins.emplace( + groupName, std::vector>({groupPlugin})); } else { - groupIt->second.push_back(plugin.GetName()); + groupIt->second.push_back(groupPlugin); } } // Map sets of transitive group dependencies to sets of transitive plugin // dependencies. - auto groups = GetTransitiveAfterGroups(game.GetDatabase().GetGroups(false), - game.GetDatabase().GetUserGroups()); - for (auto& group : groups) { - std::unordered_set transitivePlugins; - for (const auto& afterGroup : group.second) { - const auto pluginsIt = groupPlugins.find(afterGroup); - if (pluginsIt != groupPlugins.end()) { - transitivePlugins.insert(pluginsIt->second.begin(), - pluginsIt->second.end()); - } - } - group.second = transitivePlugins; - } + const auto predecessorGroupsMap = GetPredecessorGroups( + game.GetDatabase().GetGroups(false), game.GetDatabase().GetUserGroups()); + + // Replace the transitive after group names with the names of the plugins in + // those groups. + const auto predecessorGroupsPlugins = + GetPredecessorGroupPlugins(groupPlugins, predecessorGroupsMap); // Add all transitive plugin dependencies for a group to the plugin's load // after metadata. @@ -170,11 +203,22 @@ std::vector GetPluginsSortingData( plugin.GetGroup()); } - const auto groupsIt = groups.find(plugin.GetGroup()); - if (groupsIt == groups.end()) { + const auto groupsIt = predecessorGroupsPlugins.find(plugin.GetGroup()); + if (groupsIt == predecessorGroupsPlugins.end()) { throw UndefinedGroupError(plugin.GetGroup()); + } + + if (plugin.IsGroupUserMetadata()) { + // If the current plugin is a member of its group due to user metadata, + // then all predecessor plugins are such due to user metadata. + auto predecessorGroupPlugins = groupsIt->second; + for (auto& predecessorGroupPlugin : predecessorGroupPlugins) { + predecessorGroupPlugin.pathInvolvesUserMetadata = true; + } + + plugin.SetPredecessorGroupPlugins(predecessorGroupPlugins); } else { - plugin.SetAfterGroupPlugins(groupsIt->second); + plugin.SetPredecessorGroupPlugins(groupsIt->second); } } diff --git a/src/api/sorting/plugin_sorting_data.cpp b/src/api/sorting/plugin_sorting_data.cpp index b9930221..e09e4a29 100644 --- a/src/api/sorting/plugin_sorting_data.cpp +++ b/src/api/sorting/plugin_sorting_data.cpp @@ -57,18 +57,13 @@ PluginSortingData::PluginSortingData( const GameType gameType, const std::vector& loadedPlugins) : plugin_(plugin), + group_(userMetadata.GetGroup().value_or( + masterlistMetadata.GetGroup().value_or(Group::DEFAULT_NAME))), masterlistLoadAfter_(masterlistMetadata.GetLoadAfterFiles()), userLoadAfter_(userMetadata.GetLoadAfterFiles()), masterlistReq_(masterlistMetadata.GetRequirements()), - userReq_(userMetadata.GetRequirements()) { - if (userMetadata.GetGroup()) { - group_ = userMetadata.GetGroup().value(); - } else if (masterlistMetadata.GetGroup()) { - group_ = masterlistMetadata.GetGroup().value(); - } else { - group_ = Group().GetName(); - } - + userReq_(userMetadata.GetRequirements()), + groupIsUserMetadata_(userMetadata.GetGroup().has_value()) { if (plugin == nullptr) { return; } @@ -148,14 +143,18 @@ bool PluginSortingData::DoAssetsOverlap(const PluginSortingData& plugin) const { std::string PluginSortingData::GetGroup() const { return group_; } -std::unordered_set PluginSortingData::GetAfterGroupPlugins() - const { - return afterGroupPlugins_; +bool PluginSortingData::IsGroupUserMetadata() const { + return groupIsUserMetadata_; } -void PluginSortingData::SetAfterGroupPlugins( - std::unordered_set plugins) { - afterGroupPlugins_ = plugins; +std::vector +PluginSortingData::GetPredecessorGroupPlugins() const { + return predecessorGroupPlugins_; +} + +void PluginSortingData::SetPredecessorGroupPlugins( + std::vector plugins) { + predecessorGroupPlugins_ = plugins; } const std::vector& PluginSortingData::GetMasterlistLoadAfterFiles() diff --git a/src/api/sorting/plugin_sorting_data.h b/src/api/sorting/plugin_sorting_data.h index 969ef9c1..f2e12002 100644 --- a/src/api/sorting/plugin_sorting_data.h +++ b/src/api/sorting/plugin_sorting_data.h @@ -31,6 +31,11 @@ #include "loot/metadata/plugin_metadata.h" namespace loot { +struct PredecessorGroupPlugin { + std::string name; + bool pathInvolvesUserMetadata{false}; +}; + class PluginSortingData { public: explicit PluginSortingData() = default; @@ -59,9 +64,10 @@ public: bool DoAssetsOverlap(const PluginSortingData& plugin) const; std::string GetGroup() const; + bool IsGroupUserMetadata() const; - std::unordered_set GetAfterGroupPlugins() const; - void SetAfterGroupPlugins(std::unordered_set plugins); + std::vector GetPredecessorGroupPlugins() const; + void SetPredecessorGroupPlugins(std::vector plugins); const std::vector& GetMasterlistLoadAfterFiles() const; const std::vector& GetUserLoadAfterFiles() const; @@ -73,7 +79,7 @@ public: private: const PluginSortingInterface* plugin_{nullptr}; std::string group_; - std::unordered_set afterGroupPlugins_; + std::vector predecessorGroupPlugins_; std::vector masterlistLoadAfter_; std::vector userLoadAfter_; @@ -82,6 +88,7 @@ private: std::optional loadOrderIndex_; size_t overrideRecordCount_{0}; + bool groupIsUserMetadata_{0}; }; } diff --git a/src/tests/api/internals/sorting/group_sort_test.h b/src/tests/api/internals/sorting/group_sort_test.h index 3762d2b1..dccafe6f 100644 --- a/src/tests/api/internals/sorting/group_sort_test.h +++ b/src/tests/api/internals/sorting/group_sort_test.h @@ -32,34 +32,101 @@ along with LOOT. If not, see #include "loot/exception/undefined_group_error.h" namespace loot { +bool operator==(const PredecessorGroup& lhs, const PredecessorGroup& rhs) { + return lhs.pathInvolvesUserMetadata == rhs.pathInvolvesUserMetadata && + lhs.name == rhs.name; +} + namespace test { -TEST(GetTransitiveAfterGroups, shouldMapGroupsToTheirTransitiveAfterGroups) { +TEST(GetPredecessorGroups, shouldMapGroupsToTheirPredecessorGroups) { std::vector groups({Group("a"), Group("b", {"a"}), Group("c", {"b"})}); - auto mapped = GetTransitiveAfterGroups(groups, {}); + auto predecessors = GetPredecessorGroups(groups, {}); - EXPECT_TRUE(mapped["a"].empty()); - EXPECT_EQ(std::unordered_set({"a"}), mapped["b"]); - EXPECT_EQ(std::unordered_set({"a", "b"}), mapped["c"]); + EXPECT_TRUE(predecessors["a"].empty()); + EXPECT_EQ(std::vector({{"a"}}), predecessors["b"]); + EXPECT_EQ(std::vector({{"b"}, {"a"}}), predecessors["c"]); } -TEST(GetTransitiveAfterGroups, shouldThrowIfAnAfterGroupDoesNotExist) { +TEST(GetPredecessorGroups, + shouldRecordIfADirectSuccessorIsDefinedInUserMetadata) { + std::vector masterlistGroups({Group("a")}); + std::vector userlistGroups({Group("b", {"a"})}); + + auto predecessors = GetPredecessorGroups(masterlistGroups, userlistGroups); + + EXPECT_EQ(std::vector({{"a", true}}), predecessors["b"]); +} + +TEST(GetPredecessorGroups, + shouldRecordIfADirectPredecessorIsLinkedDueToUserMetadata) { + std::vector masterlistGroups({Group("a"), Group("b")}); + std::vector userlistGroups({Group("b", {"a"})}); + + auto predecessors = GetPredecessorGroups(masterlistGroups, userlistGroups); + + EXPECT_EQ(std::vector({{"a", true}}), predecessors["b"]); +} + +TEST(GetPredecessorGroups, + shouldRecordIfAnIndirectSuccessorIsDefinedInUserMetadata) { + std::vector masterlistGroups({Group("a"), Group("b", {"a"})}); + std::vector userlistGroups({Group("c", {"b"})}); + + auto predecessors = GetPredecessorGroups(masterlistGroups, userlistGroups); + + EXPECT_EQ(std::vector({{"a"}}), predecessors["b"]); + EXPECT_EQ(std::vector({{"b", true}, {"a", true}}), + predecessors["c"]); +} + +TEST(GetPredecessorGroups, + shouldRecordIfAnIndirectPredecessorIsLinkedDueToUserMetadata) { + std::vector masterlistGroups( + {Group("a"), Group("b"), Group("c", {"b"})}); + std::vector userlistGroups({Group("b", {"a"})}); + + auto predecessors = GetPredecessorGroups(masterlistGroups, userlistGroups); + + EXPECT_EQ(std::vector({{"a", true}}), predecessors["b"]); + EXPECT_EQ(std::vector({{"b"}, {"a", true}}), + predecessors["c"]); +} + +TEST(GetPredecessorGroups, + shouldNotLeakUserMetadataInvolvementToSeparatePaths) { + // This arrangement of groups ensures that a masterlist-sourced edge is + // followed after a userlist-sourced edge along a different path, to check + // encountering a userlist-sourced edge along one path does not poison + // discovery of other paths. + std::vector masterlistGroups( + {Group("a"), Group("b"), Group("c"), Group("d", {"b", "c"})}); + std::vector userlistGroups({Group("b", {"a"})}); + + auto predecessors = GetPredecessorGroups(masterlistGroups, userlistGroups); + + EXPECT_EQ(std::vector({{"b"}, {"a", true}, {"c"}}), + predecessors["d"]); +} + +TEST(GetPredecessorGroups, shouldThrowIfAnAfterGroupDoesNotExist) { std::vector groups({Group("b", {"a"})}); - EXPECT_THROW(GetTransitiveAfterGroups(groups, {}), UndefinedGroupError); + EXPECT_THROW(GetPredecessorGroups(groups, {}), UndefinedGroupError); } -TEST(GetTransitiveAfterGroups, shouldThrowIfAfterGroupsAreCyclic) { +TEST(GetPredecessorGroups, shouldThrowIfAfterGroupsAreCyclic) { std::vector groups({Group("a"), Group("b", {"a"})}); std::vector userGroups({Group("a", {"c"}), Group("c", {"b"})}); try { - GetTransitiveAfterGroups(groups, userGroups); + GetPredecessorGroups(groups, userGroups); FAIL(); - } catch (CyclicInteractionError &e) { + } catch (CyclicInteractionError& e) { ASSERT_EQ(3, e.GetCycle().size()); - // Vertices can be added in any order, so which group is first is undefined. + // Vertices can be added in any order, so which group is first is + // undefined. if (e.GetCycle()[0].GetName() == "a") { EXPECT_EQ(EdgeType::userLoadAfter, e.GetCycle()[0].GetTypeOfEdgeToNextVertex()); diff --git a/src/tests/api/internals/sorting/plugin_sort_test.h b/src/tests/api/internals/sorting/plugin_sort_test.h index 621dd76a..ef85bb1e 100644 --- a/src/tests/api/internals/sorting/plugin_sort_test.h +++ b/src/tests/api/internals/sorting/plugin_sort_test.h @@ -341,20 +341,20 @@ TEST_P( 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::group, e.GetCycle()[1].GetTypeOfEdgeToNextVertex()); + 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::group, e.GetCycle()[3].GetTypeOfEdgeToNextVertex()); + 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::group, e.GetCycle()[0].GetTypeOfEdgeToNextVertex()); + 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::group, e.GetCycle()[2].GetTypeOfEdgeToNextVertex()); + EXPECT_EQ(EdgeType::userGroup, e.GetCycle()[2].GetTypeOfEdgeToNextVertex()); } } }