From 997631667fa5b800ce59a301626ae14237d4df4d Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Wed, 4 Jan 2023 23:29:19 +0000 Subject: [PATCH] Overhaul how group edges are added during sorting The new logic is conceptually simpler, with fewer special cases to handle. Unlike the old implementation, the new approach avoids cycles. It basically does depth-first searches through the group graph, adding edges from each group's plugins to the plugins in the group's successors. This involved reversing the direction of edges in the group graph, as this switches the logic from trying to find predecessors to trying to successors: the new direction matches that of plugin graph edges, so it's less confusing. I can't think of a situation in which the iteration order of plugins within a group matters. I tested it manually with my test load order of ~1600 plugins, and saw no difference when the order was randomly shuffled. I've added a test case but it's a relatively simple scenario and there may be a more complex scenario where it would matter that I haven't thought of. A buffer is used to hold the plugins in the previous groups in the current path, because that's noticeably faster than just recording the path and looking up the plugins for each group in the path. It does duplicate the group vectors, but that's an insignificant amount of memory used. The new approach has a negative performance impact, with sorting now 15% slower than before. A more efficient solution could be to implement a custom DFS algorithm that doesn't stop when it reaches a vertex it's already visited (which would be fine since the graph has already been validated to be acyclic), as then only the root vertices would need to be searched from. --- docs/api/sorting.rst | 35 +- src/api/sorting/group_sort.cpp | 115 +---- src/api/sorting/group_sort.h | 11 +- src/api/sorting/plugin_graph.cpp | 439 ++++++++---------- src/api/sorting/plugin_sort.cpp | 28 +- .../api/internals/sorting/group_sort_test.h | 90 +--- .../api/internals/sorting/plugin_graph_test.h | 59 +-- 7 files changed, 270 insertions(+), 507 deletions(-) diff --git a/docs/api/sorting.rst b/docs/api/sorting.rst index c5d67cc8..8b6ea489 100644 --- a/docs/api/sorting.rst +++ b/docs/api/sorting.rst @@ -90,18 +90,31 @@ plugins and the rest of the plugins in the graph. Group edges ----------- -For each plugin, the plugins that are members of groups that the current -plugin's group loads after are iterated over and individually checked to see if -adding an edge from the other group's plugin to the current plugin would cause a -cycle. If not, the edge is queued for addition. If it would cause a cycle and -one of the plugins is in the default group and the other group's plugin is -master-flagged or the current plugin is not master flagged, then the plugin in -the default group is recorded as one to skip adding edges to or from when the -prospective edge involves any of the groups in the path from the other plugin -to the current plugin. +First a graph of groups is created to represent which groups must load after +which other groups, with groups being added in lexicographical order and +masterlist groups before userlist groups. Once all the groups have been added, +a depth-first search is performed starting from each group in the order they +were added. -Once all the plugins have been iterated over, all the queued edges are added, -skipping those edges identified in the earlier loop. +At the start of each search, the starting group is used as the first element in +a stack that will represent the current path through the graph. On each new edge +encountered, the target group is appended to the stack and edges are +added going from the plugins in the edge's source group to the plugins in the +edge's target group, unless the edge to be added would cause a cycle, or unless +the source group is the ``default`` group, in which case its plugins are +ignored. The same is done for all the groups currently recorded in the stack. +Once all a group's out-edges (going to groups that load directly after it) have +been processed, the group is removed from the stack. + +Once all the groups have been iterated over, one final depth-first search is +performed, this time starting from the ``default`` group and *not* skipping +edges from its plugins. + +In this way all plugins have edges added from them to all the plugins in the +groups that load after their group, unless the edge would cause a cycle. The +order in which groups are defined can affect which edges are skipped, but the +order of groups' "load after" metadata does not, and neither does the order in +which plugins in each group are looped over. At this point the plugin graph is checked for cycles, and an error is thrown if any are encountered, so that metadata (or indeed plugin data) that cause them diff --git a/src/api/sorting/group_sort.cpp b/src/api/sorting/group_sort.cpp index 78fd5649..02d02075 100644 --- a/src/api/sorting/group_sort.cpp +++ b/src/api/sorting/group_sort.cpp @@ -38,46 +38,6 @@ typedef boost::graph_traits::vertex_descriptor vertex_t; typedef boost::graph_traits::edge_descriptor edge_t; typedef boost::associative_property_map> edge_map_t; -// 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: - PredecessorGroupsVisitor(std::vector& visitedGroups) : - visitedGroups_(visitedGroups) {} - - 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}); - } - } - - void finish_vertex(vertex_t, const GroupGraph&) { pathStack_.pop_back(); } - -private: - std::vector& visitedGroups_; - std::vector pathStack_; -}; - class CycleDetector : public boost::dfs_visitor<> { public: void tree_edge(edge_t edge, const GroupGraph& graph) { @@ -120,16 +80,6 @@ private: std::vector trail; }; -std::string joinVector(const std::vector& container) { - std::string output; - 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); -} - std::vector SortByName(const std::vector& groups) { auto copy = groups; std::sort(copy.begin(), copy.end(), [](const auto& lhs, const auto& rhs) { @@ -184,7 +134,7 @@ GroupGraph BuildGroupGraph(const std::vector& masterlistGroups, throw UndefinedGroupError(otherGroupName); } - boost::add_edge(vertex, otherVertex->second, edgeType, graph); + boost::add_edge(otherVertex->second, vertex, edgeType, graph); } } }; @@ -221,40 +171,6 @@ GroupGraph BuildGroupGraph(const std::vector& masterlistGroups, return graph; } -std::unordered_map> -GetPredecessorGroups(const GroupGraph& graph) { - auto logger = getLogger(); - if (logger) { - logger->trace("Sorting groups according to their load after data"); - } - - std::unordered_map> - transitiveAfterGroups; - for (const vertex_t& vertex : - boost::make_iterator_range(boost::vertices(graph))) { - std::vector visitedGroups; - const PredecessorGroupsVisitor afterGroupsVisitor(visitedGroups); - - // Create a color map. - std::vector colorVec(boost::num_vertices(graph)); - const auto colorMap = boost::make_iterator_property_map( - colorVec.begin(), - boost::get(boost::vertex_index, graph), - colorVec.at(0)); - - boost::depth_first_visit(graph, vertex, afterGroupsVisitor, colorMap); - transitiveAfterGroups[graph[vertex]] = visitedGroups; - - if (logger) { - logger->debug("Group \"{}\" transitively loads after groups \"{}\"", - graph[vertex], - joinVector(visitedGroups)); - } - } - - return transitiveAfterGroups; -} - vertex_t GetVertexByName(const GroupGraph& graph, const std::string& name) { for (const auto& vertex : boost::make_iterator_range(boost::vertices(graph))) { @@ -263,7 +179,7 @@ vertex_t GetVertexByName(const GroupGraph& graph, const std::string& name) { } } - auto logger = getLogger(); + const auto logger = getLogger(); if (logger) { logger->error("Can't find group with name \"{}\"", name); } @@ -293,7 +209,7 @@ std::vector GetGroupsPath(const GroupGraph& graph, std::vector predecessors(boost::num_vertices(graph)); std::vector distance(predecessors.size(), (std::numeric_limits::max)()); - distance.at(toVertex) = 0; + distance.at(fromVertex) = 0; bellman_ford_shortest_paths( graph, @@ -301,13 +217,13 @@ std::vector GetGroupsPath(const GroupGraph& graph, .predecessor_map(boost::make_iterator_property_map( predecessors.begin(), get(boost::vertex_index, graph))) .distance_map(distance.data()) - .root_vertex(toVertex)); + .root_vertex(fromVertex)); - std::vector path; - vertex_t currentVertex = fromVertex; - while (currentVertex != toVertex) { - auto nextVertex = predecessors.at(currentVertex); - if (nextVertex == currentVertex) { + std::vector path{Vertex(graph[toVertex])}; + vertex_t currentVertex = toVertex; + while (currentVertex != fromVertex) { + const auto precedingVertex = predecessors.at(currentVertex); + if (precedingVertex == currentVertex) { if (logger) { logger->error( "Unreachable vertex {} encountered while looking for vertex {}", @@ -317,18 +233,19 @@ std::vector GetGroupsPath(const GroupGraph& graph, return std::vector(); } - const auto pair = boost::edge(nextVertex, currentVertex, graph); + const auto pair = boost::edge(precedingVertex, currentVertex, graph); if (!pair.second) { throw std::runtime_error("Unexpectedly couldn't find edge between \"" + - graph[currentVertex] + "\" and \"" + - graph[nextVertex] + "\""); + graph[precedingVertex] + "\" and \"" + + graph[currentVertex] + "\""); } - auto vertex = Vertex(graph[currentVertex], graph[pair.first]); + const auto vertex = Vertex(graph[precedingVertex], graph[pair.first]); path.push_back(vertex); - currentVertex = nextVertex; + currentVertex = precedingVertex; } - path.push_back(Vertex(graph[currentVertex])); + + std::reverse(path.begin(), path.end()); return path; } diff --git a/src/api/sorting/group_sort.h b/src/api/sorting/group_sort.h index 72ca8b15..fa27ed3d 100644 --- a/src/api/sorting/group_sort.h +++ b/src/api/sorting/group_sort.h @@ -36,23 +36,14 @@ namespace loot { typedef boost::adjacency_list GroupGraph; -struct PredecessorGroup { - std::string name; - bool pathInvolvesUserMetadata{false}; -}; - GroupGraph BuildGroupGraph(const std::vector& masterlistGroups, const std::vector& userGroups); -// Map entries are a group name and names of transitive load after groups. -std::unordered_map> -GetPredecessorGroups(const GroupGraph& groupGraph); - std::vector GetGroupsPath(const GroupGraph& groupGraph, const std::string& fromGroupName, const std::string& toGroupName); diff --git a/src/api/sorting/plugin_graph.cpp b/src/api/sorting/plugin_graph.cpp index f76e0d1f..6a8c4242 100644 --- a/src/api/sorting/plugin_graph.cpp +++ b/src/api/sorting/plugin_graph.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -82,41 +83,6 @@ private: std::vector trail; }; -struct PredecessorGroupPlugin { - vertex_t vertex{0}; - bool groupPathInvolvesUserMetadata{false}; -}; - -std::unordered_map> -GetPredecessorGroupsPlugins( - const std::unordered_map>& groupsPlugins, - 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 = groupsPlugins.find(predecessorGroup.name); - if (pluginsIt != groupsPlugins.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, predecessorGroup.pathInvolvesUserMetadata}); - } - } - } - - predecessorGroupsPlugins.insert({group.first, predecessorGroupPlugins}); - } - - return predecessorGroupsPlugins; -} - std::unordered_map> GetGroupsPlugins( const PluginGraph& graph) { std::unordered_map> groupsPlugins; @@ -134,157 +100,199 @@ std::unordered_map> GetGroupsPlugins( } } - // Sort plugins by their names. This is necessary to ensure that - // plugin precedessor group plugins are listed in a consistent - // order, which is important because that is the order in which - // group edges are added and differences could cause different - // sorting results. - for (auto& groupPlugins : groupsPlugins) { - std::sort(groupPlugins.second.begin(), - groupPlugins.second.end(), - [&](const vertex_t& lhs, const vertex_t& rhs) { - return graph.GetPlugin(lhs).GetName() < - graph.GetPlugin(rhs).GetName(); - }); - } - return groupsPlugins; } -std::vector GetPredecessorGroupPlugins( - const std::string& groupName, - const std::unordered_map>& - predecessorGroupsPlugins) { - const auto groupsIt = predecessorGroupsPlugins.find(groupName); - if (groupsIt == predecessorGroupsPlugins.end()) { - throw UndefinedGroupError(groupName); - } - - return groupsIt->second; -} - -bool ShouldIgnorePlugin( - const std::string& group, - const std::string& pluginName, - const std::map>& - groupPluginsToIgnore) { - const auto pluginsToIgnore = groupPluginsToIgnore.find(group); - if (pluginsToIgnore != groupPluginsToIgnore.end()) { - return pluginsToIgnore->second.count(pluginName) > 0; - } - - 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 IgnorePluginGroupEdges( - const std::string& pluginName, - const std::unordered_set& groups, - std::map>& - groupPluginsToIgnore) { - for (const auto& group : groups) { - const auto pluginsToIgnore = groupPluginsToIgnore.find(group); - if (pluginsToIgnore != groupPluginsToIgnore.end()) { - pluginsToIgnore->second.insert(pluginName); - } else { - groupPluginsToIgnore.emplace( - group, std::unordered_set({pluginName})); +boost::graph_traits::vertex_descriptor GetDefaultVertex( + const GroupGraph& graph) { + for (const auto& vertex : + boost::make_iterator_range(boost::vertices(graph))) { + if (graph[vertex] == Group::DEFAULT_NAME) { + return vertex; } } + + throw std::logic_error("Could not find default group in group graph"); } -class IntermediateGroupsFinder : public boost::dfs_visitor<> { +class GroupsVisitor : public boost::dfs_visitor<> { public: typedef boost::graph_traits::edge_descriptor GroupGraphEdge; typedef boost::graph_traits::vertex_descriptor GroupGraphVertex; - explicit IntermediateGroupsFinder( - const GroupGraphVertex& targetGroup, - std::unordered_set& groupNamesInPath) : - targetGroup_(targetGroup), groupNamesInPath_(&groupNamesInPath) {} + explicit GroupsVisitor( + PluginGraph& pluginGraph, + const std::unordered_map>& + groupsPlugins) : + pluginGraph_(&pluginGraph), + groupsPlugins_(&groupsPlugins), + logger_(getLogger()) {} + + explicit GroupsVisitor( + PluginGraph& pluginGraph, + const std::unordered_map>& + groupsPlugins, + const GroupGraphVertex vertexToIgnoreAsSource) : + pluginGraph_(&pluginGraph), + groupsPlugins_(&groupsPlugins), + vertexToIgnoreAsSource_(vertexToIgnoreAsSource), + logger_(getLogger()) {} void tree_edge(GroupGraphEdge edge, const GroupGraph& graph) { - // If this target vertex is the visitor's target group, add all the names of - // the groups in the current path stack to the names set. Also add the names - // if the target vertex name appears in the set of names in the path, as - // that indicates that the current path merges with an already-walked path - // so also ends up at the target vertex. + const auto source = boost::source(edge, graph); const auto target = boost::target(edge, graph); - if (target == targetGroup_ || - groupNamesInPath_->count(graph[target]) != 0) { - for (const auto& vertex : pathStack_) { - if (vertex != startVertex_) { - groupNamesInPath_->insert(graph[vertex]); + + // Add the edge to the stack so that adding edges can take into account + // whether its edge to the source group involves user metadata. + edgeStack_.push_back(edge); + + // Find the plugins in the target group. + const auto targetPlugins = FindPluginsInGroup(target, graph); + + // Create a new buffer to hold plugins that have one or more of their edges + // to target group plugins skipped. + std::vector newBuffer; + + // Function to add edges from all the given plugins to all the target group + // plugins, and record any plugins that have edges added or at least one + // edge skipped. + const auto addEdges = [&](const std::vector& fromPlugins, + const size_t sourceGroupEdgeStackIndex) { + const auto groupPathInvolvesUserMetadata = + PathToGroupInvolvesUserMetadata(sourceGroupEdgeStackIndex, graph); + for (const auto& plugin : fromPlugins) { + AddEdges(plugin, targetPlugins, groupPathInvolvesUserMetadata); + } + }; + + // Add edges for plugins buffered when adding edges from the previous + // groups in the path being walked. + for (size_t i = 0; i < pluginsBuffers_.size(); i += 1) { + // Each plugins buffer holds the plugins in the source group for the edge + // at the same index. + addEdges(pluginsBuffers_[i], i); + } + + // For each source plugin, add an edge to each target plugin, unless the + // source group should be ignored (i.e. because the visitor has been + // configured to ignore the default group's plugins as sources). + if (source != vertexToIgnoreAsSource_) { + newBuffer = FindPluginsInGroup(source, graph); + // Current edge is the last one in the stack. + addEdges(newBuffer, edgeStack_.size() - 1); + } + + // Add the new buffer to the stack. + pluginsBuffers_.push_back(newBuffer); + } + + void forward_or_cross_edge(GroupGraphEdge edge, const GroupGraph& graph) { + tree_edge(edge, graph); + + // A forward or cross edge doesn't visit its target vertex, so pop it and + // its buffer back off the stacks. + PopStacks(); + } + + void finish_vertex(GroupGraphVertex, const GroupGraph&) { PopStacks(); } + +private: + bool PathToGroupInvolvesUserMetadata(const size_t sourceGroupEdgeStackIndex, + const GroupGraph& graph) const { + if (sourceGroupEdgeStackIndex >= edgeStack_.size()) { + // Can't find group, this should be impossible. + throw std::logic_error("Given index is past the end of the path stack"); + } + + // The target group is always the most recent group in the stack, so we + // don't need to look for it. + + bool pathInvolvesUserMetadata = false; + + const auto begin = std::next(edgeStack_.begin(), sourceGroupEdgeStackIndex); + + // The path involves user metadata if any edge between two groups in the + // path came from user metadata. + for (auto it = begin; it != edgeStack_.end(); ++it) { + pathInvolvesUserMetadata |= graph[*it] == EdgeType::userLoadAfter; + } + + return pathInvolvesUserMetadata; + } + + std::vector FindPluginsInGroup(const GroupGraphVertex vertex, + const GroupGraph& graph) { + const auto targetPluginsIt = groupsPlugins_->find(graph[vertex]); + return targetPluginsIt == groupsPlugins_->end() ? std::vector() + : targetPluginsIt->second; + } + + void AddEdges(const vertex_t& fromVertex, + const std::vector& toPlugins, + const bool groupPathInvolvesUserMetadata) { + if (toPlugins.empty()) { + return; + } + + const auto& fromPlugin = pluginGraph_->GetPlugin(fromVertex); + + for (const auto& toVertex : toPlugins) { + const auto& toPlugin = pluginGraph_->GetPlugin(toVertex); + + if (!pluginGraph_->PathExists(toVertex, fromVertex)) { + const auto involvesUserMetadata = groupPathInvolvesUserMetadata || + fromPlugin.IsGroupUserMetadata() || + toPlugin.IsGroupUserMetadata(); + + const auto edgeType = involvesUserMetadata ? EdgeType::userGroup + : EdgeType::masterlistGroup; + + pluginGraph_->AddEdge(fromVertex, toVertex, edgeType); + } else { + if (logger_) { + logger_->debug( + "Skipping group edge from \"{}\" to \"{}\" as it would " + "create a cycle.", + fromPlugin.GetName(), + toPlugin.GetName()); } } } } - void forward_or_cross_edge(GroupGraphEdge edge, const GroupGraph& graph) { - tree_edge(edge, graph); + void PopStacks() { + if (!edgeStack_.empty()) { + edgeStack_.pop_back(); + } + + if (!pluginsBuffers_.empty()) { + pluginsBuffers_.pop_back(); + } } - void start_vertex(GroupGraphVertex vertex, const GroupGraph&) { - startVertex_ = vertex; - } + PluginGraph* pluginGraph_{nullptr}; + const std::unordered_map>* groupsPlugins_{ + nullptr}; + std::optional vertexToIgnoreAsSource_; + std::shared_ptr logger_; - void discover_vertex(GroupGraphVertex vertex, const GroupGraph&) { - pathStack_.push_back(vertex); - } + // This represents the path to the current target vertex in the group graph. + std::vector edgeStack_; - void finish_vertex(GroupGraphVertex, const GroupGraph&) { - pathStack_.pop_back(); - } - -private: - GroupGraphVertex targetGroup_{0}; - std::unordered_set* groupNamesInPath_{nullptr}; - - GroupGraphVertex startVertex_{0}; - std::vector pathStack_; + // This represents the plugins carried forward from each vertex in the path + // to the current target vertex in the group path. + std::vector> pluginsBuffers_; }; -std::unordered_map GetGroupVertexMap( - const GroupGraph& graph) { - std::unordered_map map; - for (const vertex_t& vertex : - boost::make_iterator_range(boost::vertices(graph))) { - map.emplace(graph[vertex], vertex); - } - - return map; -} - -std::unordered_set FindGroupsInAllPaths( - const GroupGraph& groupGraph, - const GroupGraphVertex& fromGroup, - const GroupGraphVertex& toGroup) { - std::vector colorVec( - boost::num_vertices(groupGraph)); +void DepthFirstVisit( + const GroupGraph& graph, + const boost::graph_traits::vertex_descriptor& startingVertex, + GroupsVisitor& visitor) { + std::vector colorVec(boost::num_vertices(graph)); const auto colorMap = boost::make_iterator_property_map( - colorVec.begin(), - boost::get(boost::vertex_index, groupGraph), - colorVec.at(0)); + colorVec.begin(), boost::get(boost::vertex_index, graph), colorVec.at(0)); - std::unordered_set foundGroups; - - // The starting vertex is the last group in the path, since the group graph's - // edges point from the group that loads after to the group it loads after. - IntermediateGroupsFinder visitor(fromGroup, foundGroups); - - boost::depth_first_visit(groupGraph, toGroup, visitor, colorMap); - - return foundGroups; + boost::depth_first_visit(graph, startingVertex, visitor, colorMap); } std::string describeEdgeType(EdgeType edgeType) { @@ -640,7 +648,6 @@ const PluginSortingData& PluginGraph::GetPlugin(const vertex_t& vertex) const { void PluginGraph::CheckForCycles() const { const auto logger = getLogger(); - // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDelete) if (logger) { logger->trace("Checking plugin graph for cycles..."); } @@ -884,6 +891,8 @@ void PluginGraph::AddHardcodedPluginEdges( } void PluginGraph::AddGroupEdges(const GroupGraph& groupGraph) { + typedef boost::graph_traits::vertex_descriptor GroupGraphVertex; + const auto logger = getLogger(); if (logger) { logger->trace("Adding edges based on plugin group memberships..."); @@ -891,105 +900,29 @@ void PluginGraph::AddGroupEdges(const GroupGraph& groupGraph) { // First build a map from groups to the plugins in those groups. const auto groupsPlugins = GetGroupsPlugins(*this); - const auto groupVertexMap = GetGroupVertexMap(groupGraph); - const auto predecessorGroupsMap = GetPredecessorGroups(groupGraph); - const auto predecessorGroupsPlugins = - GetPredecessorGroupsPlugins(groupsPlugins, predecessorGroupsMap); - // Tuple fields are from, to, and edge type. - std::vector> acyclicEdges; - std::map> groupPluginsToIgnore; + // Get the default group's vertex because it's needed for the DFSes. + const auto defaultVertex = GetDefaultVertex(groupGraph); - for (const vertex_t& vertex : boost::make_iterator_range(GetVertices())) { - const auto& toPlugin = GetPlugin(vertex); + // Now loop over the vertices in the groups graph. + for (const auto& groupVertex : + boost::make_iterator_range(boost::vertices(groupGraph))) { + // Run a DFS from each vertex in the group graph, adding edges except from + // plugins in the default group. This could be run only on the root + // vertices, except that the DFS only visits each vertex once, so a branch + // and merge inside a given root's DAG would result in plugins from one of + // the branches not being carried forwards past the point at which the + // branches merge. + GroupsVisitor visitor(*this, groupsPlugins, defaultVertex); - const auto predecessorGroupPlugins = GetPredecessorGroupPlugins( - toPlugin.GetGroup(), predecessorGroupsPlugins); - - for (const auto& plugin : predecessorGroupPlugins) { - // After group plugin names are taken from other PluginSortingData names, - // so exact string comparisons can be used. - const auto parentVertex = plugin.vertex; - const auto& fromPlugin = GetPlugin(parentVertex); - - if (PathExists(vertex, parentVertex)) { - if (logger) { - logger->debug( - "Skipping group edge from \"{}\" to \"{}\" as it would " - "create a cycle.", - fromPlugin.GetName(), - toPlugin.GetName()); - } - - // If the earlier plugin is not a master and the later plugin is, - // don't ignore the plugin with the default group for all - // intermediate plugins, as some of those plugins may be masters - // that wouldn't be involved in the cycle, and any of those - // plugins that are not masters would have their own cycles - // detected anyway. - if (!fromPlugin.IsMaster() && toPlugin.IsMaster()) { - continue; - } - - // 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; - } - - const auto fromGroup = groupVertexMap.at(fromPlugin.GetGroup()); - const auto toGroup = groupVertexMap.at(toPlugin.GetGroup()); - - const auto groupsInPaths = - FindGroupsInAllPaths(groupGraph, fromGroup, toGroup); - - IgnorePluginGroupEdges( - pluginToIgnore, groupsInPaths, groupPluginsToIgnore); - - continue; - } - - const auto edgeInvolvesUserMetadata = - plugin.groupPathInvolvesUserMetadata || - fromPlugin.IsGroupUserMetadata() || toPlugin.IsGroupUserMetadata(); - const auto edgeType = edgeInvolvesUserMetadata - ? EdgeType::userGroup - : EdgeType::masterlistGroup; - - acyclicEdges.push_back(std::make_tuple(parentVertex, vertex, edgeType)); - } + DepthFirstVisit(groupGraph, groupVertex, visitor); } - 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); + // Now do one last DFS starting from the default group and not ignoring its + // plugins. + GroupsVisitor visitor(*this, groupsPlugins); - if (!ignore) { - AddEdge(fromVertex, toVertex, edgeType); - } else if (logger) { - logger->debug( - "Skipping {} edge from \"{}\" to \"{}\" as it would " - "create a multi-group cycle.", - describeEdgeType(edgeType), - fromPlugin.GetName(), - toPlugin.GetName()); - } - } + DepthFirstVisit(groupGraph, defaultVertex, visitor); } void PluginGraph::AddOverlapEdges() { diff --git a/src/api/sorting/plugin_sort.cpp b/src/api/sorting/plugin_sort.cpp index 10d23bee..bc804bcc 100644 --- a/src/api/sorting/plugin_sort.cpp +++ b/src/api/sorting/plugin_sort.cpp @@ -29,6 +29,7 @@ #include "api/helpers/logging.h" #include "api/sorting/group_sort.h" #include "api/sorting/plugin_graph.h" +#include "loot/exception/undefined_group_error.h" namespace loot { std::vector GetPluginsSortingData( @@ -77,6 +78,23 @@ std::vector GetPluginsSortingData( return pluginsSortingData; } +void ValidatePluginGroups(const std::vector& plugins, + const GroupGraph& graph) { + std::unordered_set groupNames; + + for (const vertex_t& vertex : + boost::make_iterator_range(boost::vertices(graph))) { + groupNames.insert(graph[vertex]); + } + + for (const auto& plugin : plugins) { + const auto pluginGroup = plugin.GetGroup(); + if (groupNames.count(pluginGroup) == 0) { + throw UndefinedGroupError(pluginGroup); + } + } +} + bool IsInRange(const std::vector::const_iterator& begin, const std::vector::const_iterator& end, const std::string& name) { @@ -263,14 +281,13 @@ std::vector SortPlugins( graph.AddSpecificEdges(); graph.AddHardcodedPluginEdges(hardcodedPlugins); - graph.AddGroupEdges(groupGraph); - // Check for cycles now because from this point on edges are only added if - // they don't cause cycles, and adding tie-break edges is by far the slowest - // part of the process, so if there is a cycle checking now will provide - // quicker feedback than checking later. + // they don't cause cycles, and adding overlap and tie-break edges is + // relatively slow, so checking now provides quicker feedback if there is an + // issue. graph.CheckForCycles(); + graph.AddGroupEdges(groupGraph); graph.AddOverlapEdges(); graph.AddTieBreakEdges(); @@ -318,6 +335,7 @@ std::vector SortPlugins( }); const auto groupGraph = BuildGroupGraph(masterlistGroups, userGroups); + ValidatePluginGroups(pluginsSortingData, groupGraph); // Some parts of sorting are O(N^2) for N plugins, and master flags cause // O(M*N) edges to be added for M masters and N non-masters, which can be diff --git a/src/tests/api/internals/sorting/group_sort_test.h b/src/tests/api/internals/sorting/group_sort_test.h index a1330875..41181920 100644 --- a/src/tests/api/internals/sorting/group_sort_test.h +++ b/src/tests/api/internals/sorting/group_sort_test.h @@ -32,11 +32,6 @@ 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(BuildGroupGraph, shouldThrowIfAnAfterGroupDoesNotExist) { std::vector groups({Group("b", {"a"})}); @@ -65,15 +60,15 @@ TEST(BuildGroupGraph, shouldThrowIfAfterGroupsAreCyclic) { ASSERT_EQ(3, e.GetCycle().size()); EXPECT_EQ("a", e.GetCycle()[0].GetName()); - EXPECT_EQ(EdgeType::userLoadAfter, + EXPECT_EQ(EdgeType::masterlistLoadAfter, e.GetCycle()[0].GetTypeOfEdgeToNextVertex()); - EXPECT_EQ("c", e.GetCycle()[1].GetName()); + EXPECT_EQ("b", e.GetCycle()[1].GetName()); EXPECT_EQ(EdgeType::userLoadAfter, e.GetCycle()[1].GetTypeOfEdgeToNextVertex()); - EXPECT_EQ("b", e.GetCycle()[2].GetName()); - EXPECT_EQ(EdgeType::masterlistLoadAfter, + EXPECT_EQ("c", e.GetCycle()[2].GetName()); + EXPECT_EQ(EdgeType::userLoadAfter, e.GetCycle()[2].GetTypeOfEdgeToNextVertex()); } } @@ -120,83 +115,6 @@ TEST(BuildGroupGraph, } } -TEST(GetPredecessorGroups, shouldMapGroupsToTheirPredecessorGroups) { - std::vector groups({Group("a"), Group("b", {"a"}), Group("c", {"b"})}); - - const auto groupGraph = BuildGroupGraph(groups, {}); - auto predecessors = GetPredecessorGroups(groupGraph); - - EXPECT_TRUE(predecessors["a"].empty()); - EXPECT_EQ(std::vector({{"a"}}), predecessors["b"]); - EXPECT_EQ(std::vector({{"b"}, {"a"}}), predecessors["c"]); -} - -TEST(GetPredecessorGroups, - shouldRecordIfADirectSuccessorIsDefinedInUserMetadata) { - std::vector masterlistGroups({Group("a")}); - std::vector userlistGroups({Group("b", {"a"})}); - - const auto groupGraph = BuildGroupGraph(masterlistGroups, userlistGroups); - auto predecessors = GetPredecessorGroups(groupGraph); - - EXPECT_EQ(std::vector({{"a", true}}), predecessors["b"]); -} - -TEST(GetPredecessorGroups, - shouldRecordIfADirectPredecessorIsLinkedDueToUserMetadata) { - std::vector masterlistGroups({Group("a"), Group("b")}); - std::vector userlistGroups({Group("b", {"a"})}); - - const auto groupGraph = BuildGroupGraph(masterlistGroups, userlistGroups); - auto predecessors = GetPredecessorGroups(groupGraph); - - 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"})}); - - const auto groupGraph = BuildGroupGraph(masterlistGroups, userlistGroups); - auto predecessors = GetPredecessorGroups(groupGraph); - - 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"})}); - - const auto groupGraph = BuildGroupGraph(masterlistGroups, userlistGroups); - auto predecessors = GetPredecessorGroups(groupGraph); - - 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"})}); - - const auto groupGraph = BuildGroupGraph(masterlistGroups, userlistGroups); - auto predecessors = GetPredecessorGroups(groupGraph); - - EXPECT_EQ(std::vector({{"b"}, {"a", true}, {"c"}}), - predecessors["d"]); -} - TEST(GetGroupsPath, shouldThrowIfTheFromGroupDoesNotExist) { std::vector groups({Group("a"), Group("b", {"a"})}); std::vector userGroups({Group("a", {"c"}), Group("c")}); diff --git a/src/tests/api/internals/sorting/plugin_graph_test.h b/src/tests/api/internals/sorting/plugin_graph_test.h index 11b9d3c2..f5ea9fa3 100644 --- a/src/tests/api/internals/sorting/plugin_graph_test.h +++ b/src/tests/api/internals/sorting/plugin_graph_test.h @@ -581,7 +581,7 @@ TEST_F(PluginGraphTest, addGroupEdgesShouldSkipAnEdgeThatWouldCauseACycle) { TEST_F( PluginGraphTest, - addGroupEdgesDoesNotSkipAnEdgeThatCausesACycleInvolvingOtherNonDefaultGroups) { + addGroupEdgesShouldSkipAnEdgeThatWouldCauseACycleInvolvingOtherNonDefaultGroups) { PluginGraph graph; const auto a = graph.AddVertex(CreatePluginSortingData("A.esp", "A")); @@ -596,20 +596,7 @@ TEST_F( EXPECT_TRUE(graph.EdgeExists(c, a)); EXPECT_TRUE(graph.EdgeExists(a, b)); - // FIXME: This should not cause a cycle. - try { - graph.CheckForCycles(); - FAIL(); - } catch (CyclicInteractionError& e) { - ASSERT_EQ(3, e.GetCycle().size()); - EXPECT_EQ(graph.GetPlugin(a).GetName(), e.GetCycle()[0].GetName()); - EXPECT_EQ(EdgeType::masterlistGroup, - e.GetCycle()[0].GetTypeOfEdgeToNextVertex()); - EXPECT_EQ(graph.GetPlugin(b).GetName(), e.GetCycle()[1].GetName()); - EXPECT_EQ(EdgeType::userGroup, e.GetCycle()[1].GetTypeOfEdgeToNextVertex()); - EXPECT_EQ(graph.GetPlugin(c).GetName(), e.GetCycle()[2].GetName()); - EXPECT_EQ(EdgeType::master, e.GetCycle()[2].GetTypeOfEdgeToNextVertex()); - } + EXPECT_NO_THROW(graph.CheckForCycles()); } TEST_F( @@ -662,10 +649,7 @@ TEST_F( EXPECT_TRUE(graph.EdgeExists(c, d3)); EXPECT_TRUE(graph.EdgeExists(b, d3)); - - // FIXME: This edge should be added but isn't, it's a limitation of the - // current implementation. - EXPECT_FALSE(graph.EdgeExists(c, d1)); + EXPECT_TRUE(graph.EdgeExists(c, d1)); EXPECT_NO_THROW(graph.CheckForCycles()); } @@ -729,11 +713,9 @@ TEST_F( EXPECT_TRUE(graph.EdgeExists(b1, c2)); EXPECT_TRUE(graph.EdgeExists(b2, c2)); EXPECT_TRUE(graph.EdgeExists(a2, c1)); + EXPECT_FALSE(graph.EdgeExists(b2, c1)); - // FIXME: This edge is unwanted and causes a cycle. - EXPECT_TRUE(graph.EdgeExists(b2, c1)); - - EXPECT_THROW(graph.CheckForCycles(), CyclicInteractionError); + EXPECT_NO_THROW(graph.CheckForCycles()); } TEST_F( @@ -800,13 +782,11 @@ TEST_F( EXPECT_TRUE(graph.EdgeExists(a2, c1)); EXPECT_TRUE(graph.EdgeExists(a2, d1)); EXPECT_TRUE(graph.EdgeExists(a2, d2)); + EXPECT_FALSE(graph.EdgeExists(b2, c1)); EXPECT_FALSE(graph.EdgeExists(d1, d2)); EXPECT_FALSE(graph.EdgeExists(d2, d1)); - // FIXME: This edge is unwanted and causes a cycle. - EXPECT_TRUE(graph.EdgeExists(b2, c1)); - - EXPECT_THROW(graph.CheckForCycles(), CyclicInteractionError); + EXPECT_NO_THROW(graph.CheckForCycles()); } TEST_F( @@ -825,6 +805,7 @@ TEST_F( // Should be D.esp -> B.esp -> C.esp EXPECT_TRUE(graph.EdgeExists(b, c)); EXPECT_TRUE(graph.EdgeExists(d, b)); + EXPECT_FALSE(graph.EdgeExists(c, d)); EXPECT_NO_THROW(graph.CheckForCycles()); } @@ -915,11 +896,9 @@ TEST_F( EXPECT_TRUE(graph.EdgeExists(d, b)); EXPECT_TRUE(graph.EdgeExists(b, c)); EXPECT_TRUE(graph.EdgeExists(c, e)); + EXPECT_FALSE(graph.EdgeExists(e, f)); - // FIXME: This edge is unwanted and causes a cycle. - EXPECT_TRUE(graph.EdgeExists(e, f)); - - EXPECT_THROW(graph.CheckForCycles(), CyclicInteractionError); + EXPECT_NO_THROW(graph.CheckForCycles()); } TEST_F( @@ -1362,11 +1341,9 @@ TEST_F( EXPECT_TRUE(graph.EdgeExists(b, d)); EXPECT_FALSE(graph.EdgeExists(a, b)); EXPECT_FALSE(graph.EdgeExists(b, a)); + EXPECT_FALSE(graph.EdgeExists(c, d)); - // FIXME: This edge is unwanted and causes a cycle. - EXPECT_TRUE(graph.EdgeExists(c, d)); - - EXPECT_THROW(graph.CheckForCycles(), CyclicInteractionError); + EXPECT_NO_THROW(graph.CheckForCycles()); } } @@ -1414,13 +1391,11 @@ TEST_F(PluginGraphTest, EXPECT_FALSE(graph.EdgeExists(b, c)); EXPECT_FALSE(graph.EdgeExists(c, b)); + EXPECT_FALSE(graph.EdgeExists(c, d)); EXPECT_FALSE(graph.EdgeExists(c, e)); EXPECT_FALSE(graph.EdgeExists(d, c)); - // FIXME: This edge is unwanted and causes a cycle. - EXPECT_TRUE(graph.EdgeExists(c, d)); - - EXPECT_THROW(graph.CheckForCycles(), CyclicInteractionError); + EXPECT_NO_THROW(graph.CheckForCycles()); } } @@ -1477,11 +1452,9 @@ TEST_F(PluginGraphTest, addGroupEdgesShouldNotDependOnPluginGraphVertexOrder) { ASSERT_TRUE(graph.EdgeExists(a2, c)); ASSERT_FALSE(graph.EdgeExists(a1, a2)); ASSERT_FALSE(graph.EdgeExists(a2, a1)); + ASSERT_FALSE(graph.EdgeExists(b, c)); - // FIXME: This edge is unwanted and causes a cycle. - ASSERT_TRUE(graph.EdgeExists(b, c)); - - EXPECT_THROW(graph.CheckForCycles(), CyclicInteractionError); + ASSERT_NO_THROW(graph.CheckForCycles()); } }