From 2b46b92fa78095c8d103542570ec4d2286a3d74c Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Sat, 1 Mar 2025 10:32:34 +0000 Subject: [PATCH] Fix group edges not getting added When a vertex is unfinishable during a groups graph DFS, that means all the plugins in the path leading up to it are also unfinishable, and the comment I'd written in the code acknowledged that but the impl didn't. --- src/api/sorting/plugin_graph.cpp | 92 ++++++++++++++----- .../api/internals/sorting/plugin_graph_test.h | 38 ++++++++ 2 files changed, 108 insertions(+), 22 deletions(-) diff --git a/src/api/sorting/plugin_graph.cpp b/src/api/sorting/plugin_graph.cpp index b9ef787e..8d7ae96a 100644 --- a/src/api/sorting/plugin_graph.cpp +++ b/src/api/sorting/plugin_graph.cpp @@ -24,6 +24,8 @@ #include "plugin_graph.h" +#include + #include #include #include @@ -174,6 +176,18 @@ std::unordered_map> GetGroupsPlugins( } } + const auto logger = getLogger(); + if (logger && logger->should_log(spdlog::level::debug)) { + logger->debug("Found the following plugins in groups:"); + for (const auto [key, value] : groupsPlugins) { + std::vector pluginNames; + for (const auto vertex : value) { + pluginNames.push_back("\"" + graph.GetPlugin(vertex).GetName() + "\""); + } + logger->debug("\t{}: {}", key, fmt::join(pluginNames, ", ")); + } + } + return groupsPlugins; } @@ -241,19 +255,27 @@ public: } void forward_or_cross_edge(GroupGraphEdge edge, const GroupGraph& graph) { - // Mark the source vertex as unfinishable, because none of the plugins in - // in the path so far can have edges added to plugins past the target - // vertex. - unfinishableVertices_.insert(boost::source(edge, graph)); + // Mark the source vertex and all edges in the current stack as + // unfinishable, because none of the plugins in in the path so far can have + // edges added to plugins past the target vertex. + for (const auto& edgeInPath : edgeStack_) { + MarkSourceAsUnfinishable(edgeInPath.first, graph); + } + + MarkSourceAsUnfinishable(edge, graph); } - void finish_vertex(GroupGraphVertex vertex, const GroupGraph&) { + void finish_vertex(GroupGraphVertex vertex, const GroupGraph& graph) { // Now that this vertex's DFS-tree has been fully explored, mark it as // finished so that it won't have edges added from its plugins again in a // different DFS that uses the same finished vertices set. if (vertex != vertexToIgnoreAsSource_ && unfinishableVertices_.count(vertex) == 0) { - finishedVertices_->insert(vertex); + const auto inserted = finishedVertices_->insert(vertex).second; + if (inserted && logger_) { + logger_->debug("Recorded groups graph vertex \"{}\" as finished", + graph[vertex]); + } } // Since this vertex has been fully explored, pop the edge stack to remove @@ -311,24 +333,26 @@ private: for (const auto& toVertex : toPluginVertices) { const auto& toPlugin = pluginGraph_->GetPlugin(toVertex); - if (!pluginGraph_->IsPathCached(fromPluginVertex, toVertex) && - !pluginGraph_->PathExists(toVertex, fromPluginVertex)) { - const auto involvesUserMetadata = groupPathInvolvesUserMetadata || - fromPlugin.IsGroupUserMetadata() || - toPlugin.IsGroupUserMetadata(); + if (pluginGraph_->IsPathCached(fromPluginVertex, toVertex)) { + continue; + } - const auto edgeType = involvesUserMetadata ? EdgeType::userGroup - : EdgeType::masterlistGroup; + const auto involvesUserMetadata = groupPathInvolvesUserMetadata || + fromPlugin.IsGroupUserMetadata() || + toPlugin.IsGroupUserMetadata(); + const auto edgeType = involvesUserMetadata ? EdgeType::userGroup + : EdgeType::masterlistGroup; + + if (!pluginGraph_->PathExists(toVertex, fromPluginVertex)) { pluginGraph_->AddEdge(fromPluginVertex, toVertex, edgeType); - } else { - if (logger_) { - logger_->debug( - "Skipping group edge from \"{}\" to \"{}\" as it would " - "create a cycle.", - fromPlugin.GetName(), - toPlugin.GetName()); - } + } else if (logger_) { + logger_->debug( + "Skipping a \"{}\" edge from \"{}\" to \"{}\" as it would " + "create a cycle.", + describeEdgeType(edgeType), + fromPlugin.GetName(), + toPlugin.GetName()); } } } @@ -344,6 +368,23 @@ private: } } + void MarkSourceAsUnfinishable(const GroupGraphEdge edge, + const GroupGraph& graph) { + const auto source = boost::source(edge, graph); + const auto inserted = unfinishableVertices_.insert(source).second; + + if (logger_ && inserted) { + const auto target = boost::source(edge, graph); + + logger_->debug( + "Found groups graph forward or cross \"{}\" edge going from \"{}\" " + "to \"{}\", treating the source as unfinishable", + describeEdgeType(graph[edge]), + graph[source], + graph[target]); + } + } + PluginGraph* pluginGraph_{nullptr}; const std::unordered_map>* groupsPlugins_{ nullptr}; @@ -362,6 +403,13 @@ void DepthFirstVisit( const GroupGraph& graph, const boost::graph_traits::vertex_descriptor& startingVertex, GroupsPathVisitor& visitor) { + const auto logger = getLogger(); + if (logger) { + logger->trace( + "Starting depth-first search of the groups graph starting from \"{}\"", + graph[startingVertex]); + } + 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)); @@ -1047,7 +1095,7 @@ void PluginGraph::AddOverlapEdges() { AddEdge(fromVertex, toVertex, edgeType); } else if (logger) { logger->debug( - "Skipping {} edge from \"{}\" to \"{}\" as it would " + "Skipping \"{}\" edge from \"{}\" to \"{}\" as it would " "create a cycle.", describeEdgeType(edgeType), GetPlugin(fromVertex).GetName(), diff --git a/src/tests/api/internals/sorting/plugin_graph_test.h b/src/tests/api/internals/sorting/plugin_graph_test.h index f045271c..834a02e2 100644 --- a/src/tests/api/internals/sorting/plugin_graph_test.h +++ b/src/tests/api/internals/sorting/plugin_graph_test.h @@ -1562,6 +1562,44 @@ TEST_F(PluginGraphTest, addGroupEdgesDoesNotStartSearchingWithTheLongestPath) { EXPECT_NO_THROW(graph.CheckForCycles()); } +TEST_F(PluginGraphTest, + addGroupEdgesShouldMarkVerticesAsUnfinishableIfAVertexInTheirSubtreeIsUnfinishable) { + std::vector masterlistGroups{Group("A"), + Group("B", {"A"}), + Group("C", {"B"}), + Group("D", {"C"}), + Group()}; + std::vector userGroups {Group("BU1", {"B"}), Group("BU2", {"BU1"}), Group("C", {"BU2"})}; + groupGraph = BuildGroupGraph(masterlistGroups, userGroups); + + PluginGraph graph; + + const auto a = graph.AddVertex(CreatePluginSortingData("A.esp", "A")); + const auto b = graph.AddVertex(CreatePluginSortingData("B.esp", "B")); + const auto bu1 = graph.AddVertex(CreatePluginSortingData("BU1.esp", "BU1")); + const auto bu2 = graph.AddVertex(CreatePluginSortingData("BU2.esp", "BU2")); + const auto c = graph.AddVertex(CreatePluginSortingData("C.esp", "C")); + const auto d = graph.AddVertex(CreatePluginSortingData("D.esp", "D")); + + graph.AddGroupEdges(groupGraph); + + // Should be A.esp -> B.esp -----------------------> C.esp -> D.esp + // -> BU1.esp -> BU2.esp -> + EXPECT_TRUE(graph.EdgeExists(a, b)); + EXPECT_TRUE(graph.EdgeExists(a, c)); + EXPECT_TRUE(graph.EdgeExists(a, d)); + EXPECT_TRUE(graph.EdgeExists(b, c)); + EXPECT_TRUE(graph.EdgeExists(b, d)); + EXPECT_TRUE(graph.EdgeExists(b, bu1)); + EXPECT_TRUE(graph.EdgeExists(b, bu2)); + EXPECT_TRUE(graph.EdgeExists(bu1, bu2)); + EXPECT_TRUE(graph.EdgeExists(bu1, c)); + EXPECT_TRUE(graph.EdgeExists(bu1, d)); + EXPECT_TRUE(graph.EdgeExists(bu2, c)); + EXPECT_TRUE(graph.EdgeExists(bu2, d)); + EXPECT_TRUE(graph.EdgeExists(c, d)); +} + TEST_F(PluginGraphTest, addOverlapEdgesShouldNotAddEdgesBetweenNonOverlappingPlugins) { PluginGraph graph;