loot/libloot
0
0
Fork 0
mirror of https://github.com/loot/libloot.git synced 2026-07-27 14:16:01 -07:00
Code Issues Packages Projects Releases Wiki Activity

Sort groups metadata by name before building the groups graph

Browse Source 
So that the graph vertices and edges have a consistent order independent of the order that the metadata is defined in.

This is less of a concern than for plugins because the order of the metadata is something that the author controls, but having the metadata order matter could be surprising and doesn't seem useful.
This commit is contained in:
Oliver Hamlet
2025-01-17 23:13:12 +00:00
parent defe94f2e3
commit 885c6fd2be
3 changed files with 127 additions and 140 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/api/sorting/group_sort.cpp
+37 -3
View File
@@ -130,6 +130,21 @@ std::string joinVector(const std::vector<PredecessorGroup>& container) {
return output.substr(0, output.length() - 2);
}
std::vector<Group> SortByName(const std::vector<Group>& 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<std::string> SortNames(std::vector<std::string>&& groupNames) {
std::sort(groupNames.begin(), groupNames.end());
return groupNames;
}
GroupGraph BuildGroupGraph(const std::vector<Group>& masterlistGroups,
const std::vector<Group>& userGroups) {
const auto logger = getLogger();
@@ -158,7 +173,12 @@ GroupGraph BuildGroupGraph(const std::vector<Group>& 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<Group>& 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");
src/tests/api/internals/sorting/group_sort_test.h
+45 -35
View File
@@ -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<std::vector<Group>> 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());
}
}
}
}
src/tests/api/internals/sorting/plugin_graph_test.h
+45 -102
View File
@@ -1295,7 +1295,7 @@ TEST_F(PluginGraphTest,
TEST_F(
PluginGraphTest,
addGroupEdgesDoesNotDependOnGroupDefinitionOrderIfThereIsASingleLinearPath) {
addGroupEdgesShouldNotDependOnGroupDefinitionOrderIfThereIsASingleLinearPath) {
std::vector<std::vector<Group>> 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<Group> masterlistGroups{Group("A"),
Group("B"),
Group("C", {"A", "B"}),
Group("D", {"C"}),
Group()};
TEST_F(
PluginGraphTest,
addGroupEdgesShouldNotDependOnGroupDefinitionOrderIfThereAreMultipleRoots) {
std::vector<std::vector<Group>> 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<Group> 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<Group> masterlistGroups{Group("A"),
Group("B", {"A"}),
Group("C", {"A"}),
Group("D", {"B", "C"}),
Group("E", {"D"}),
Group()};
TEST_F(PluginGraphTest,
addGroupEdgesShouldNotDependOnBranchingGroupDefinitionOrder) {
std::vector<std::vector<Group>> 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<Group> 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<Group> masterlistGroups{
Group("A"), Group("B", {"A"}), Group("C", {"B"}), Group()};