diff --git a/src/api/sorting/group_sort.cpp b/src/api/sorting/group_sort.cpp index 57e17565..f8ec51ca 100644 --- a/src/api/sorting/group_sort.cpp +++ b/src/api/sorting/group_sort.cpp @@ -130,6 +130,21 @@ std::string joinVector(const std::vector& container) { 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) { + return lhs.GetName() < rhs.GetName(); + }); + + return copy; +} + +std::vector SortNames(std::vector&& groupNames) { + std::sort(groupNames.begin(), groupNames.end()); + + return groupNames; +} + GroupGraph BuildGroupGraph(const std::vector& masterlistGroups, const std::vector& userGroups) { const auto logger = getLogger(); @@ -158,7 +173,12 @@ GroupGraph BuildGroupGraph(const std::vector& masterlistGroups, } const auto vertex = groupVertices.at(groupName); - for (const auto& otherGroupName : group.GetAfterGroups()) { + + // Similar to groups, after groups are sorted by name so that the order + // of a group vertex's in-edges is independent of the order they're + // listed in the group definition. The order of in-edges affects the + // result of calling GetGroupsPath(). + for (const auto& otherGroupName : SortNames(group.GetAfterGroups())) { const auto otherVertex = groupVertices.find(otherGroupName); if (otherVertex == groupVertices.end()) { throw UndefinedGroupError(otherGroupName); @@ -169,15 +189,29 @@ GroupGraph BuildGroupGraph(const std::vector& masterlistGroups, } }; + // Sort groups by name so that they get added to the graph in an order that + // is consistent and independent of the order in which they are defined. + // This is important because the order in which vertices are created affects + // the order in which edges are created and so can affect the outcome of + // sorting. + // It would be surprising if swapping the order in which two groups were + // defined in e.g. the masterlist had an impact on LOOT's sorting behaviour, + // but if a group's name changes that's effectively deleting one group and + // creating another. It would also be surprising that the groups' names can + // have an effect, but the effect is at least constant for a given set of + // groups. + // It might also be surprising that whether a group is defined in the + // masterlist or userlist can have an effect, but it's consistent with the + // handling of edges for all other masterlist and userlist metadata. if (logger) { logger->trace("Adding masterlist groups to groups graph..."); } - addGroups(masterlistGroups, EdgeType::masterlistLoadAfter); + addGroups(SortByName(masterlistGroups), EdgeType::masterlistLoadAfter); if (logger) { logger->trace("Adding user groups to groups graph..."); } - addGroups(userGroups, EdgeType::userLoadAfter); + addGroups(SortByName(userGroups), EdgeType::userLoadAfter); if (logger) { logger->trace("Checking for cycles in the group graph"); diff --git a/src/tests/api/internals/sorting/group_sort_test.h b/src/tests/api/internals/sorting/group_sort_test.h index 7923a68b..a1330875 100644 --- a/src/tests/api/internals/sorting/group_sort_test.h +++ b/src/tests/api/internals/sorting/group_sort_test.h @@ -64,43 +64,17 @@ TEST(BuildGroupGraph, shouldThrowIfAfterGroupsAreCyclic) { } catch (CyclicInteractionError& e) { ASSERT_EQ(3, e.GetCycle().size()); - // 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()); + EXPECT_EQ("a", e.GetCycle()[0].GetName()); + EXPECT_EQ(EdgeType::userLoadAfter, + e.GetCycle()[0].GetTypeOfEdgeToNextVertex()); - EXPECT_EQ("c", e.GetCycle()[1].GetName()); - EXPECT_EQ(EdgeType::userLoadAfter, - e.GetCycle()[1].GetTypeOfEdgeToNextVertex()); + EXPECT_EQ("c", e.GetCycle()[1].GetName()); + EXPECT_EQ(EdgeType::userLoadAfter, + e.GetCycle()[1].GetTypeOfEdgeToNextVertex()); - EXPECT_EQ("b", e.GetCycle()[2].GetName()); - EXPECT_EQ(EdgeType::masterlistLoadAfter, - e.GetCycle()[2].GetTypeOfEdgeToNextVertex()); - } else if (e.GetCycle()[0].GetName() == "b") { - EXPECT_EQ(EdgeType::masterlistLoadAfter, - e.GetCycle()[0].GetTypeOfEdgeToNextVertex()); - - EXPECT_EQ("a", e.GetCycle()[1].GetName()); - EXPECT_EQ(EdgeType::userLoadAfter, - e.GetCycle()[1].GetTypeOfEdgeToNextVertex()); - - EXPECT_EQ("c", e.GetCycle()[2].GetName()); - EXPECT_EQ(EdgeType::userLoadAfter, - e.GetCycle()[2].GetTypeOfEdgeToNextVertex()); - } else { - EXPECT_EQ("c", e.GetCycle()[0].GetName()); - EXPECT_EQ(EdgeType::userLoadAfter, - e.GetCycle()[0].GetTypeOfEdgeToNextVertex()); - - EXPECT_EQ("b", e.GetCycle()[1].GetName()); - EXPECT_EQ(EdgeType::masterlistLoadAfter, - e.GetCycle()[1].GetTypeOfEdgeToNextVertex()); - - EXPECT_EQ("a", e.GetCycle()[2].GetName()); - EXPECT_EQ(EdgeType::userLoadAfter, - e.GetCycle()[2].GetTypeOfEdgeToNextVertex()); - } + EXPECT_EQ("b", e.GetCycle()[2].GetName()); + EXPECT_EQ(EdgeType::masterlistLoadAfter, + e.GetCycle()[2].GetTypeOfEdgeToNextVertex()); } } @@ -301,6 +275,42 @@ TEST(GetGroupsPath, EXPECT_EQ("e", path[3].GetName()); EXPECT_FALSE(path[3].GetTypeOfEdgeToNextVertex().has_value()); } + +TEST(GetGroupsPath, shouldNotDependOnTheAfterGroupDefinitionOrder) { + std::vector> orders{ + // Create a graph with after groups in one order. + {Group("A"), + Group("B", {"A"}), + Group("C", {"A"}), + Group("D", {"B", "C"}), + Group("E", {"D"}), + Group()}, + // Now do the same again, but with a different after group order for D. + {Group("A"), + Group("B", {"A"}), + Group("C", {"A"}), + Group("D", {"C", "B"}), + Group("E", {"D"}), + Group()}}; + + for (const auto& masterlistGroups : orders) { + const auto groupGraph = BuildGroupGraph(masterlistGroups, {}); + auto path = GetGroupsPath(groupGraph, "A", "E"); + + ASSERT_EQ(4, path.size()); + EXPECT_EQ("A", path[0].GetName()); + EXPECT_EQ(EdgeType::masterlistLoadAfter, + path[0].GetTypeOfEdgeToNextVertex().value()); + EXPECT_EQ("B", path[1].GetName()); + EXPECT_EQ(EdgeType::masterlistLoadAfter, + path[1].GetTypeOfEdgeToNextVertex().value()); + EXPECT_EQ("D", path[2].GetName()); + EXPECT_EQ(EdgeType::masterlistLoadAfter, + path[2].GetTypeOfEdgeToNextVertex().value()); + EXPECT_EQ("E", path[3].GetName()); + EXPECT_FALSE(path[3].GetTypeOfEdgeToNextVertex().has_value()); + } +} } } diff --git a/src/tests/api/internals/sorting/plugin_graph_test.h b/src/tests/api/internals/sorting/plugin_graph_test.h index a8cd1f5f..b56f5062 100644 --- a/src/tests/api/internals/sorting/plugin_graph_test.h +++ b/src/tests/api/internals/sorting/plugin_graph_test.h @@ -1295,7 +1295,7 @@ TEST_F(PluginGraphTest, TEST_F( PluginGraphTest, - addGroupEdgesDoesNotDependOnGroupDefinitionOrderIfThereIsASingleLinearPath) { + addGroupEdgesShouldNotDependOnGroupDefinitionOrderIfThereIsASingleLinearPath) { std::vector> masterlistsGroups{ {Group("B"), Group("C", {"B"}), Group("default", {"C"})}, {Group("C", {"B"}), Group("B"), Group("default", {"C"})}}; @@ -1324,16 +1324,23 @@ TEST_F( } } -TEST_F(PluginGraphTest, - addGroupEdgesDependsOnGroupDefinitionOrderIfThereAreMultipleRoots) { - // Create a graph with groups in one order. - { - std::vector masterlistGroups{Group("A"), - Group("B"), - Group("C", {"A", "B"}), - Group("D", {"C"}), - Group()}; - +TEST_F( + PluginGraphTest, + addGroupEdgesShouldNotDependOnGroupDefinitionOrderIfThereAreMultipleRoots) { + std::vector> orders{ + // Create a graph with groups in one order. + {Group("A"), + Group("B"), + Group("C", {"A", "B"}), + Group("D", {"C"}), + Group()}, + // Now do the same again, but with a different group order for A and B. + {Group("B"), + Group("A"), + Group("C", {"A", "B"}), + Group("D", {"C"}), + Group()}}; + for (const auto& masterlistGroups : orders) { groupsMap = GetGroupsMap(masterlistGroups, {}); const auto groupGraph = BuildGroupGraph(masterlistGroups, {}); predecessorGroupsMap = GetPredecessorGroups(groupGraph); @@ -1363,54 +1370,26 @@ TEST_F(PluginGraphTest, EXPECT_THROW(graph.CheckForCycles(), CyclicInteractionError); } - - // Now do the same again, but with a different group order for A and B. - { - std::vector masterlistGroups{Group("B"), - Group("A"), - Group("C", {"A", "B"}), - Group("D", {"C"}), - Group()}; - - groupsMap = GetGroupsMap(masterlistGroups, {}); - const auto groupGraph = BuildGroupGraph(masterlistGroups, {}); - predecessorGroupsMap = GetPredecessorGroups(groupGraph); - - 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(d, a, EdgeType::master); - - graph.AddGroupEdges(groupsMap, predecessorGroupsMap); - - // Should be B.esp -> C.esp -> D.esp -> A.esp - EXPECT_TRUE(graph.EdgeExists(d, a)); - EXPECT_TRUE(graph.EdgeExists(b, c)); - EXPECT_TRUE(graph.EdgeExists(c, d)); - EXPECT_FALSE(graph.EdgeExists(a, b)); - EXPECT_FALSE(graph.EdgeExists(b, a)); - - // FIXME: This edge is unwanted and causes a cycle. - EXPECT_TRUE(graph.EdgeExists(a, c)); - - EXPECT_THROW(graph.CheckForCycles(), CyclicInteractionError); - } } -TEST_F(PluginGraphTest, addGroupEdgesDependsOnBranchingGroupDefinitionOrder) { - // Create a graph with groups in one order. - { - std::vector masterlistGroups{Group("A"), - Group("B", {"A"}), - Group("C", {"A"}), - Group("D", {"B", "C"}), - Group("E", {"D"}), - Group()}; - +TEST_F(PluginGraphTest, + addGroupEdgesShouldNotDependOnBranchingGroupDefinitionOrder) { + std::vector> orders{ + // Create a graph with groups in one order. + {Group("A"), + Group("B", {"A"}), + Group("C", {"A"}), + Group("D", {"B", "C"}), + Group("E", {"D"}), + Group()}, + // Now do the same again, but with a different group order for B and C. + {Group("A"), + Group("C", {"A"}), + Group("B", {"A"}), + Group("D", {"B", "C"}), + Group("E", {"D"}), + Group()}}; + for (const auto& masterlistGroups : orders) { groupsMap = GetGroupsMap(masterlistGroups, {}); const auto groupGraph = BuildGroupGraph(masterlistGroups, {}); predecessorGroupsMap = GetPredecessorGroups(groupGraph); @@ -1428,64 +1407,28 @@ TEST_F(PluginGraphTest, addGroupEdgesDependsOnBranchingGroupDefinitionOrder) { graph.AddGroupEdges(groupsMap, predecessorGroupsMap); // Should be A.esp -> B.esp -> D.esp -> E.esp -> C.esp - EXPECT_TRUE(graph.EdgeExists(e, c)); EXPECT_TRUE(graph.EdgeExists(a, b)); - EXPECT_TRUE(graph.EdgeExists(b, d)); - EXPECT_TRUE(graph.EdgeExists(d, e)); EXPECT_TRUE(graph.EdgeExists(a, c)); - EXPECT_FALSE(graph.EdgeExists(b, c)); - EXPECT_FALSE(graph.EdgeExists(c, b)); - - // FIXME: This edge is unwanted and causes a cycle. - EXPECT_TRUE(graph.EdgeExists(c, d)); - - EXPECT_THROW(graph.CheckForCycles(), CyclicInteractionError); - } - - // Now do the same again, but with a different group order for B and C. - { - std::vector masterlistGroups{Group("A"), - Group("C", {"A"}), - Group("B", {"A"}), - Group("D", {"B", "C"}), - Group("E", {"D"}), - Group()}; - - groupsMap = GetGroupsMap(masterlistGroups, {}); - const auto groupGraph = BuildGroupGraph(masterlistGroups, {}); - predecessorGroupsMap = GetPredecessorGroups(groupGraph); - - 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(groupsMap, predecessorGroupsMap); - - // Should be A.esp -> B.esp -> E.esp -> C.esp -> D.esp - EXPECT_TRUE(graph.EdgeExists(e, c)); - EXPECT_TRUE(graph.EdgeExists(a, b)); + EXPECT_TRUE(graph.EdgeExists(a, d)); + EXPECT_TRUE(graph.EdgeExists(a, e)); EXPECT_TRUE(graph.EdgeExists(b, d)); EXPECT_TRUE(graph.EdgeExists(b, e)); - EXPECT_TRUE(graph.EdgeExists(a, e)); - EXPECT_TRUE(graph.EdgeExists(a, c)); - EXPECT_TRUE(graph.EdgeExists(c, d)); + EXPECT_TRUE(graph.EdgeExists(d, e)); + EXPECT_TRUE(graph.EdgeExists(e, c)); + EXPECT_FALSE(graph.EdgeExists(b, c)); EXPECT_FALSE(graph.EdgeExists(c, b)); + 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(d, e)); + EXPECT_TRUE(graph.EdgeExists(c, d)); EXPECT_THROW(graph.CheckForCycles(), CyclicInteractionError); } } -TEST_F(PluginGraphTest, addGroupEdgesDoesNotDependOnPluginGraphVertexOrder) { +TEST_F(PluginGraphTest, addGroupEdgesShouldNotDependOnPluginGraphVertexOrder) { std::vector masterlistGroups{ Group("A"), Group("B", {"A"}), Group("C", {"B"}), Group()};