Skip to content

Commit

Permalink
Sort groups metadata by name before building the groups graph
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@Ortham
Ortham committed Jan 16, 2025
1 parent de3a17f commit a94b559
Show file tree
Hide file tree
Showing 3 changed files with 119 additions and 99 deletions.
42 changes: 38 additions & 4 deletions src/api/sorting/group_sort.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,21 @@ class CycleDetector : public boost::dfs_visitor<> {
std::vector<Vertex> trail;
};

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();
Expand All @@ -106,7 +121,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);
Expand All @@ -117,15 +137,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);

return graph;
}
Expand Down Expand Up @@ -220,4 +254,4 @@ std::vector<Vertex> GetGroupsPath(const std::vector<Group>& masterlistGroups,

return path;
}
}
}
35 changes: 35 additions & 0 deletions src/tests/api/internals/sorting/group_sort_test.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,41 @@ TEST(GetGroupsPath, shouldThrowIfMasterlistGroupLoadsAfterAUserlistGroup) {
EXPECT_THROW(GetGroupsPath(groups, userGroups, "a", "e"),
UndefinedGroupError);
}

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) {
auto path = GetGroupsPath(masterlistGroups, {}, "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());
}
}
}
}

Expand Down
141 changes: 46 additions & 95 deletions src/tests/api/internals/sorting/plugin_graph_test.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,7 @@ class PluginGraphTest : public ::testing::Test {
PluginSortingData CreatePluginSortingData(const std::string& name) {
const auto plugin = GetPlugin(name);

return PluginSortingData(
plugin, PluginMetadata(), PluginMetadata(), {});
return PluginSortingData(plugin, PluginMetadata(), PluginMetadata(), {});
}

PluginSortingData CreatePluginSortingData(const std::string& name,
Expand All @@ -165,8 +164,7 @@ class PluginGraphTest : public ::testing::Test {
masterlistMetadata.SetGroup(group);
}

return PluginSortingData(
plugin, masterlistMetadata, userMetadata, {});
return PluginSortingData(plugin, masterlistMetadata, userMetadata, {});
}

plugingraph::TestPlugin* GetPlugin(const std::string& name) {
Expand Down Expand Up @@ -1276,7 +1274,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"})}};
Expand Down Expand Up @@ -1304,15 +1302,21 @@ 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()};

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) {
groupGraph = BuildGroupGraph(masterlistGroups, {});

PluginGraph graph;
Expand All @@ -1337,50 +1341,27 @@ TEST_F(PluginGraphTest,

EXPECT_NO_THROW(graph.CheckForCycles());
}

// 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()};

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(d, a, EdgeType::master);

graph.AddGroupEdges(groupGraph);

// 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));
EXPECT_FALSE(graph.EdgeExists(a, c));

EXPECT_NO_THROW(graph.CheckForCycles());
}
}

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) {
groupGraph = BuildGroupGraph(masterlistGroups, {});

PluginGraph graph;
Expand All @@ -1396,56 +1377,26 @@ TEST_F(PluginGraphTest, addGroupEdgesDependsOnBranchingGroupDefinitionOrder) {
graph.AddGroupEdges(groupGraph);

// 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));

EXPECT_NO_THROW(graph.CheckForCycles());
}

// 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()};

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(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, d));
EXPECT_FALSE(graph.EdgeExists(c, e));
EXPECT_FALSE(graph.EdgeExists(d, c));

EXPECT_NO_THROW(graph.CheckForCycles());
}
}

TEST_F(PluginGraphTest, addGroupEdgesDoesNotDependOnPluginGraphVertexOrder) {
TEST_F(PluginGraphTest, addGroupEdgesShouldNotDependOnPluginGraphVertexOrder) {
std::vector<Group> masterlistGroups{
Group("A"), Group("B", {"A"}), Group("C", {"B"}), Group()};

Expand Down

0 comments on commit a94b559

Please sign in to comment.