From 858a82ba50e99ca7d8d95a9cc368e9931a294dff Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Sat, 18 Jan 2025 22:28:14 +0000 Subject: [PATCH] Prioritise DFSes from the group graph roots with the longest paths This means that when there's a potential cycle, it's more likely that an earlier group will have its effect applied than a later group. --- src/api/sorting/plugin_graph.cpp | 101 +++++++++++++++-- .../api/internals/sorting/plugin_graph_test.h | 106 ++++++++++++++++++ 2 files changed, 196 insertions(+), 11 deletions(-) diff --git a/src/api/sorting/plugin_graph.cpp b/src/api/sorting/plugin_graph.cpp index fdd45b1c..a6d829e7 100644 --- a/src/api/sorting/plugin_graph.cpp +++ b/src/api/sorting/plugin_graph.cpp @@ -38,12 +38,86 @@ #include "loot/exception/cyclic_interaction_error.h" #include "loot/exception/undefined_group_error.h" +namespace { +using loot::GroupGraph; +typedef boost::graph_traits::vertex_descriptor GroupGraphVertex; + +bool IsRootVertex(const GroupGraphVertex& vertex, const GroupGraph& graph) { + return boost::in_degree(vertex, graph) == 0; +} + +class GroupsPathLengthVisitor : public boost::dfs_visitor<> { +public: + explicit GroupsPathLengthVisitor(size_t& maxPathLength) : + maxPathLength_(&maxPathLength) {} + + typedef boost::graph_traits::edge_descriptor GroupGraphEdge; + + void discover_vertex(GroupGraphVertex, const GroupGraph&) { + currentPathLength_ += 1; + if (currentPathLength_ > *maxPathLength_) { + *maxPathLength_ = currentPathLength_; + } + } + void finish_vertex(GroupGraphVertex, const GroupGraph&) { + currentPathLength_ -= 1; + } + +private: + size_t currentPathLength_{0}; + size_t* maxPathLength_{nullptr}; +}; + +void DepthFirstVisit( + const GroupGraph& graph, + const boost::graph_traits::vertex_descriptor& startingVertex, + GroupsPathLengthVisitor& visitor) { + 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, startingVertex, visitor, colorMap); +} + +// Sort the group vertices so that root vertices come first, in order of +// decreasing path length, but otherwise preserving the existing +// (lexicographical) ordering. +std::vector GetSortedGroupVertices( + const GroupGraph& groupGraph) { + const auto [gvit, gvitend] = boost::vertices(groupGraph); + std::vector groupVertices{gvit, gvitend}; + + // Calculate the max path lengths for root vertices. + std::unordered_map maxPathLengths; + for (const auto& groupVertex : groupVertices) { + if (IsRootVertex(groupVertex, groupGraph)) { + size_t maxPathLength = 0; + GroupsPathLengthVisitor visitor(maxPathLength); + + DepthFirstVisit(groupGraph, groupVertex, visitor); + + maxPathLengths.emplace(groupVertex, maxPathLength); + } + } + + // Now sort the group vertices. + std::stable_sort( + groupVertices.begin(), + groupVertices.end(), + [&](const GroupGraphVertex& lhs, const GroupGraphVertex& rhs) { + return IsRootVertex(lhs, groupGraph) && + (!IsRootVertex(rhs, groupGraph) || + (maxPathLengths[lhs] > maxPathLengths[rhs])); + }); + + return groupVertices; +} +} + namespace loot { typedef boost::graph_traits::edge_descriptor edge_t; typedef boost::graph_traits::edge_iterator edge_it; -typedef boost::graph_traits::vertex_descriptor GroupGraphVertex; - class CycleDetector : public boost::dfs_visitor<> { public: void tree_edge(edge_t edge, const RawPluginGraph& graph) { @@ -115,12 +189,11 @@ boost::graph_traits::vertex_descriptor GetDefaultVertex( throw std::logic_error("Could not find default group in group graph"); } -class GroupsVisitor : public boost::dfs_visitor<> { +class GroupsPathVisitor : public boost::dfs_visitor<> { public: typedef boost::graph_traits::edge_descriptor GroupGraphEdge; - typedef boost::graph_traits::vertex_descriptor GroupGraphVertex; - explicit GroupsVisitor( + explicit GroupsPathVisitor( PluginGraph& pluginGraph, std::unordered_set& finishedVertices, const std::unordered_map>& @@ -130,7 +203,7 @@ public: finishedVertices_(&finishedVertices), logger_(getLogger()) {} - explicit GroupsVisitor( + explicit GroupsPathVisitor( PluginGraph& pluginGraph, std::unordered_set& finishedVertices, const std::unordered_map>& @@ -292,7 +365,7 @@ private: void DepthFirstVisit( const GroupGraph& graph, const boost::graph_traits::vertex_descriptor& startingVertex, - GroupsVisitor& visitor) { + GroupsPathVisitor& visitor) { 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)); @@ -914,19 +987,25 @@ void PluginGraph::AddGroupEdges(const GroupGraph& groupGraph) { // Get the default group's vertex because it's needed for the DFSes. const auto defaultVertex = GetDefaultVertex(groupGraph); + // The vertex sort order prioritises resolving potential cycles in + // favour of earlier-loading groups. It does not guarantee that the + // longest paths will be walked first, because a root vertex may be in + // more than one path and the vertex sort order here does not influence + // which path the DFS takes. + const auto groupVertices = GetSortedGroupVertices(groupGraph); + // Now loop over the vertices in the groups graph. // Keep a record of which vertices have already been fully explored to avoid // adding edges from their plugins more than once. std::unordered_set finishedVertices; - for (const auto& groupVertex : - boost::make_iterator_range(boost::vertices(groupGraph))) { + for (const auto& groupVertex : groupVertices) { // 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( + GroupsPathVisitor visitor( *this, finishedVertices, groupsPlugins, defaultVertex); DepthFirstVisit(groupGraph, groupVertex, visitor); @@ -934,7 +1013,7 @@ void PluginGraph::AddGroupEdges(const GroupGraph& groupGraph) { // Now do one last DFS starting from the default group and not ignoring its // plugins. - GroupsVisitor visitor(*this, finishedVertices, groupsPlugins); + GroupsPathVisitor visitor(*this, finishedVertices, groupsPlugins); DepthFirstVisit(groupGraph, defaultVertex, visitor); } diff --git a/src/tests/api/internals/sorting/plugin_graph_test.h b/src/tests/api/internals/sorting/plugin_graph_test.h index f5ea9fa3..9a3e89e4 100644 --- a/src/tests/api/internals/sorting/plugin_graph_test.h +++ b/src/tests/api/internals/sorting/plugin_graph_test.h @@ -1458,6 +1458,112 @@ TEST_F(PluginGraphTest, addGroupEdgesShouldNotDependOnPluginGraphVertexOrder) { } } +TEST_F( + PluginGraphTest, + addGroupEdgesShouldStartSearchingFromRootGroupsBeforeGoingInLexicographicalOrder) { + std::vector masterlistGroups{Group("D"), + Group("A", {"D"}), + Group("B", {"A"}), + Group("C", {"B"}), + Group()}; + groupGraph = BuildGroupGraph(masterlistGroups, {}); + + PluginGraph graph; + + const auto a = graph.AddVertex(CreatePluginSortingData("A.esp", "A")); + const auto b = graph.AddVertex(CreatePluginSortingData("B.esp", "B")); + const auto c = graph.AddVertex(CreatePluginSortingData("C.esp", "C")); + const auto d = graph.AddVertex(CreatePluginSortingData("D.esp", "D")); + + graph.AddEdge(c, d, EdgeType::master); + + graph.AddGroupEdges(groupGraph); + + // Should be C.esp -> D.esp -> A.esp -> B.esp + // Processing groups lexicographically would give: + // A.esp -> B.esp -> C.esp -> D.esp + EXPECT_TRUE(graph.EdgeExists(c, d)); + EXPECT_TRUE(graph.EdgeExists(d, a)); + EXPECT_TRUE(graph.EdgeExists(d, b)); + EXPECT_TRUE(graph.EdgeExists(a, b)); + + EXPECT_FALSE(graph.EdgeExists(a, c)); + EXPECT_FALSE(graph.EdgeExists(a, d)); + EXPECT_FALSE(graph.EdgeExists(b, a)); + EXPECT_FALSE(graph.EdgeExists(b, c)); + EXPECT_FALSE(graph.EdgeExists(b, d)); + EXPECT_FALSE(graph.EdgeExists(d, c)); + + EXPECT_NO_THROW(graph.CheckForCycles()); +} + +TEST_F(PluginGraphTest, + addGroupEdgesShouldStartSearchingFromTheRootGroupWithTheLongestPath) { + std::vector masterlistGroups{Group("D"), + Group("B", {"D"}), + Group("C", {"B"}), + Group("A"), + Group("E", {"C", "A"}), + Group("F", {"E"}), + Group()}; + groupGraph = BuildGroupGraph(masterlistGroups, {}); + + PluginGraph graph; + + const auto a = graph.AddVertex(CreatePluginSortingData("A.esp", "A")); + const auto b = graph.AddVertex(CreatePluginSortingData("B.esp", "B")); + const auto c = graph.AddVertex(CreatePluginSortingData("C.esp", "C")); + const auto d = graph.AddVertex(CreatePluginSortingData("D.esp", "D")); + const auto e = graph.AddVertex(CreatePluginSortingData("E.esp", "E")); + const auto f = graph.AddVertex(CreatePluginSortingData("F.esp", "F")); + + graph.AddEdge(f, b, EdgeType::master); + + graph.AddGroupEdges(groupGraph); + + // Should be D.esp -> B.esp -> C.esp -> E.esp + // A.esp -> F.esp ----------> B.esp + // A.esp -------------------------------------> E.esp + EXPECT_TRUE(graph.EdgeExists(d, b)); + EXPECT_TRUE(graph.EdgeExists(b, c)); + EXPECT_TRUE(graph.EdgeExists(c, e)); + EXPECT_TRUE(graph.EdgeExists(a, f)); + EXPECT_TRUE(graph.EdgeExists(a, e)); + EXPECT_TRUE(graph.EdgeExists(f, b)); + + EXPECT_NO_THROW(graph.CheckForCycles()); +} + +TEST_F(PluginGraphTest, addGroupEdgesDoesNotStartSearchingWithTheLongestPath) { + std::vector masterlistGroups{Group("A"), + Group("B", {"A"}), + Group("C", {"A"}), + Group("D", {"C"}), + Group("E", {"B", "D"}), + Group()}; + groupGraph = BuildGroupGraph(masterlistGroups, {}); + + PluginGraph graph; + + const auto a = graph.AddVertex(CreatePluginSortingData("A.esp", "A")); + const auto b = graph.AddVertex(CreatePluginSortingData("B.esp", "B")); + const auto c = graph.AddVertex(CreatePluginSortingData("C.esp", "C")); + const auto d = graph.AddVertex(CreatePluginSortingData("D.esp", "D")); + const auto e = graph.AddVertex(CreatePluginSortingData("E.esp", "E")); + + graph.AddEdge(e, c, EdgeType::master); + + graph.AddGroupEdges(groupGraph); + + // Should be A.esp -> B.esp -> E.esp -> C.esp -> D.esp + EXPECT_TRUE(graph.EdgeExists(a, b)); + EXPECT_TRUE(graph.EdgeExists(b, e)); + EXPECT_TRUE(graph.EdgeExists(e, c)); + EXPECT_TRUE(graph.EdgeExists(c, d)); + + EXPECT_NO_THROW(graph.CheckForCycles()); +} + TEST_F(PluginGraphTest, addOverlapEdgesShouldNotAddEdgesBetweenNonOverlappingPlugins) { PluginGraph graph;