Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix non-deterministic ECS stress test failures #1367

Merged
merged 2 commits into from
Nov 23, 2024
Merged

Fix non-deterministic ECS stress test failures #1367

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3ea29d5
fix(ecs): remove duplicate query matches on self-relations
RiscadoA Nov 16, 2024
4265115
fix(ecs): avoid creating new tables while iterating over the table set
RiscadoA Nov 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Duplicated destructor call in AnyVector which caused double free crashes on multiple samples (**@RiscadoA**).
- Compiler Error when using -O3 flag (#1351, **@SrGesus**).
- Flipped documentation of SystemBuilder::before and SystemBuilder::after (#1371, **@RiscadoA**).
- Inconsistent behavior on ECS queries on symmetric self-relations (**@RiscadoA**).
- Undefined behavior on ECS entity removal due to creating tables while iterating over tables (#1363, **@RiscadoA**).

## [v0.4.0] - 2024-10-13

Expand Down
31 changes: 25 additions & 6 deletions core/src/ecs/query/node/related.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@ bool QueryRelatedNode::next(World& world, TargetMask pins, Iterator& iterator) c
SparseRelationTableId tableId;
SparseRelationTable* table;
bool isReverse;
bool isDuplicate;

// Utility function which updates the table pointer based on the current table index.
auto setTable = [&]() {
Expand All @@ -202,12 +203,14 @@ bool QueryRelatedNode::next(World& world, TargetMask pins, Iterator& iterator) c
tableId = mTables[tableIndex];
table = &relationTables.at(tableId);
isReverse = false;
isDuplicate = false;
}
else if (tableIndex < mTables.size() + mReverseTables.size())
{
tableId = mReverseTables[tableIndex - mTables.size()].id;
table = &relationTables.at(tableId);
isReverse = true;
isDuplicate = mReverseTables[tableIndex - mTables.size()].isDuplicate;
}
else
{
Expand All @@ -225,7 +228,7 @@ bool QueryRelatedNode::next(World& world, TargetMask pins, Iterator& iterator) c

// Utility function which checks if the current row stores a relation from an entity to itself.
auto skipIfIdentityRow = [&]() {
if (isReverse && !mIncludeDuplicates && row < table->size() && table->from(row) == table->to(row))
if (isReverse && isDuplicate && row < table->size() && table->from(row) == table->to(row))
{
advanceRow();
}
Expand Down Expand Up @@ -303,12 +306,28 @@ bool QueryRelatedNode::next(World& world, TargetMask pins, Iterator& iterator) c
{
// If we've tried all normal tables, try the ones facing the reverse direction.
// If mIncludeDuplicates is false, skip duplicate tables.
while (tableIndex < mTables.size() + mReverseTables.size() &&
(row >= world.tables().sparseRelation().at(mReverseTables[tableIndex - mTables.size()].id).size() ||
(!mIncludeDuplicates && mReverseTables[tableIndex - mTables.size()].isDuplicate)))
while (tableIndex < mTables.size() + mReverseTables.size())
{
++tableIndex;
row = 0;
SparseRelationTable& table =
world.tables().sparseRelation().at(mReverseTables[tableIndex - mTables.size()].id);
if (row >= table.size() || (!mIncludeDuplicates && mReverseTables[tableIndex - mTables.size()].isDuplicate))
{
++tableIndex;
row = 0;
continue;
}

// We're iterating over the reverse tables. If we find a self-relation, and we have already iterated over
// the same table in the forward direction, we should skip it. E.g. if we have a relation (ent1, ent1), we
// should only iterate over it once.
if (table.from(row) == table.to(row) && mReverseTables[tableIndex - mTables.size()].isDuplicate)
{
++row;
}
else
{
break;
}
}
}

Expand Down
47 changes: 31 additions & 16 deletions core/src/ecs/world.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,44 +176,59 @@ void World::destroy(Entity entity)
mEntityPool.destroy(entity.index);
mTables.dense().at(archetype).swapErase(entity.index);

std::vector<std::pair<DataTypeId, SparseRelationTable*>> delayedPropagateDepth{};
for (const auto& [type, index] : mTables.sparseRelation())
{
// For each table where the entity's archetype is the 'from' archetype.
if (index.from().contains(archetype))
{
for (const auto& table : index.from().at(archetype))
for (const auto& tableId : index.from().at(archetype))
{
// Erase all occurrences of the entity in the 'from' column.
mTables.sparseRelation().at(table).eraseFrom(entity.index);
mTables.sparseRelation().at(tableId).eraseFrom(entity.index);
}
}

// For each table where the entity's archetype is the 'to' archetype.
if (index.to().contains(archetype))
{
for (const auto& table : index.to().at(archetype))
for (const auto& tableId : index.to().at(archetype))
{
auto& table = mTables.sparseRelation().at(tableId);

if (mTypes.isTreeRelation(type))
{
// If the relation is tree-like, then we need to update the depth of the corresponding 'from'
// entities.
for (auto row = mTables.sparseRelation().at(table).firstTo(entity.index);
row != mTables.sparseRelation().at(table).size();
row = mTables.sparseRelation().at(table).nextTo(row))
{
uint32_t fromIndex;
uint32_t toIndex;
mTables.sparseRelation().at(table).indices(row, fromIndex, toIndex);
this->propagateDepth(fromIndex, type, 0);
}
// entities. We delay this to after the loop has finished, as we're iterating over all tables and
// propagateDepth() may create new tables, which can lead to UB.
//
RiscadoA marked this conversation as resolved.
Show resolved Hide resolved
// We won't erase the relation here, as we'll need to iterate over the table below.
delayedPropagateDepth.emplace_back(type, &table);
}
else
{
// Erase all occurrences of the entity in the 'to' column.
table.eraseTo(entity.index);
}

// Erase all occurrences of the entity in the 'to' column.
mTables.sparseRelation().at(table).eraseTo(entity.index);
}
}
}

// Process the delayed depth propagation.
for (auto [type, table] : delayedPropagateDepth)
{
for (auto row = table->firstTo(entity.index); row != table->size(); row = table->nextTo(row))
{
uint32_t fromIndex;
uint32_t toIndex;
table->indices(row, fromIndex, toIndex);
this->propagateDepth(fromIndex, type, 0);
}

// Now it's safe to erase the relations.
table->eraseTo(entity.index);
}

// Run commands from observers after entity is destroyed
if (observed)
{
Expand Down
76 changes: 58 additions & 18 deletions core/tests/ecs/query/filter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ TEST_CASE("ecs::QueryFilter")
world.relate(e4P, e3I, EmptyRelation{});

world.relate(e1, e1, SymmetricRelation{});
world.relate(e2I, e2I, SymmetricRelation{});
world.relate(e2I, e3I, SymmetricRelation{});
world.relate(e4P, e2I, SymmetricRelation{});

Expand Down Expand Up @@ -846,6 +847,9 @@ TEST_CASE("ecs::QueryFilter")
REQUIRE(it->entities[1] == e1);
++it;
REQUIRE(it->entities[0] == e2I);
REQUIRE(it->entities[1] == e2I);
++it;
REQUIRE(it->entities[0] == e2I);
REQUIRE(it->entities[1] == e3I);
++it;
REQUIRE(it->entities[0] == e2I);
Expand All @@ -871,6 +875,9 @@ TEST_CASE("ecs::QueryFilter")
auto view = filter.view().pin(0, e2I);
auto it = view.begin();
REQUIRE(it->entities[0] == e2I);
REQUIRE(it->entities[1] == e2I);
++it;
REQUIRE(it->entities[0] == e2I);
REQUIRE(it->entities[1] == e3I);
++it;
REQUIRE(it->entities[0] == e2I);
Expand Down Expand Up @@ -906,6 +913,9 @@ TEST_CASE("ecs::QueryFilter")
{
auto view = filter.view().pin(1, e2I);
auto it = view.begin();
REQUIRE(it->entities[0] == e2I);
REQUIRE(it->entities[1] == e2I);
++it;
REQUIRE(it->entities[0] == e3I);
REQUIRE(it->entities[1] == e2I);
++it;
Expand Down Expand Up @@ -938,6 +948,16 @@ TEST_CASE("ecs::QueryFilter")
REQUIRE(it == view.end());
}

SUBCASE("on e2I and e2I")
{
auto view = filter.view().pin(0, e2I).pin(1, e2I);
auto it = view.begin();
REQUIRE(it->entities[0] == e2I);
REQUIRE(it->entities[1] == e2I);
++it;
REQUIRE(it == view.end());
}

SUBCASE("on e2I and e4P")
{
auto view = filter.view().pin(0, e2I).pin(1, e4P);
Expand Down Expand Up @@ -995,9 +1015,6 @@ TEST_CASE("ecs::QueryFilter")
REQUIRE(it->entities[0] == e1);
REQUIRE(it->entities[1] == e1);
++it;
REQUIRE(it->entities[0] == e1);
REQUIRE(it->entities[1] == e1);
++it;
REQUIRE(it->entities[0] == e4P);
REQUIRE(it->entities[1] == e2I);
++it;
Expand All @@ -1013,9 +1030,6 @@ TEST_CASE("ecs::QueryFilter")
REQUIRE(it->entities[0] == e1);
REQUIRE(it->entities[1] == e1);
++it;
REQUIRE(it->entities[0] == e1);
REQUIRE(it->entities[1] == e1);
++it;
REQUIRE(it == view.end());
}

Expand Down Expand Up @@ -1045,9 +1059,6 @@ TEST_CASE("ecs::QueryFilter")
REQUIRE(it->entities[0] == e1);
REQUIRE(it->entities[1] == e1);
++it;
REQUIRE(it->entities[0] == e1);
REQUIRE(it->entities[1] == e1);
++it;
REQUIRE(it == view.end());
}

Expand Down Expand Up @@ -1116,9 +1127,6 @@ TEST_CASE("ecs::QueryFilter")
REQUIRE(it->entities[0] == e2I);
REQUIRE(it->entities[1] == e4P);
++it;
REQUIRE(it->entities[0] == e1);
REQUIRE(it->entities[1] == e1);
++it;
REQUIRE(it == view.end());
}

Expand All @@ -1131,9 +1139,6 @@ TEST_CASE("ecs::QueryFilter")
REQUIRE(it->entities[0] == e1);
REQUIRE(it->entities[1] == e1);
++it;
REQUIRE(it->entities[0] == e1);
REQUIRE(it->entities[1] == e1);
++it;
REQUIRE(it == view.end());
}

Expand Down Expand Up @@ -1163,9 +1168,6 @@ TEST_CASE("ecs::QueryFilter")
REQUIRE(it->entities[0] == e1);
REQUIRE(it->entities[1] == e1);
++it;
REQUIRE(it->entities[0] == e1);
REQUIRE(it->entities[1] == e1);
++it;
REQUIRE(it == view.end());
}

Expand Down Expand Up @@ -1230,6 +1232,9 @@ TEST_CASE("ecs::QueryFilter")
auto view = filter.view();
auto it = view.begin();
REQUIRE(it->entities[0] == e2I);
REQUIRE(it->entities[1] == e2I);
++it;
REQUIRE(it->entities[0] == e2I);
REQUIRE(it->entities[1] == e3I);
++it;
REQUIRE(it == view.end());
Expand All @@ -1242,6 +1247,9 @@ TEST_CASE("ecs::QueryFilter")
auto view = filter.view().pin(0, e2I);
auto it = view.begin();
REQUIRE(it->entities[0] == e2I);
REQUIRE(it->entities[1] == e2I);
++it;
REQUIRE(it->entities[0] == e2I);
REQUIRE(it->entities[1] == e3I);
++it;
REQUIRE(it == view.end());
Expand All @@ -1264,6 +1272,9 @@ TEST_CASE("ecs::QueryFilter")
{
auto view = filter.view().pin(1, e2I);
auto it = view.begin();
REQUIRE(it->entities[0] == e2I);
REQUIRE(it->entities[1] == e2I);
++it;
REQUIRE(it->entities[0] == e3I);
REQUIRE(it->entities[1] == e2I);
++it;
Expand All @@ -1283,6 +1294,16 @@ TEST_CASE("ecs::QueryFilter")

SUBCASE("both targets pinned")
{
SUBCASE("e2I and e2I")
{
auto view = filter.view().pin(0, e2I).pin(1, e2I);
auto it = view.begin();
REQUIRE(it->entities[0] == e2I);
REQUIRE(it->entities[1] == e2I);
++it;
REQUIRE(it == view.end());
}

SUBCASE("e2I and e3I")
{
auto view = filter.view().pin(0, e2I).pin(1, e3I);
Expand Down Expand Up @@ -1319,6 +1340,9 @@ TEST_CASE("ecs::QueryFilter")
auto view = filter.view();
auto it = view.begin();
REQUIRE(it->entities[0] == e2I);
REQUIRE(it->entities[1] == e2I);
++it;
REQUIRE(it->entities[0] == e2I);
REQUIRE(it->entities[1] == e3I);
++it;
REQUIRE(it->entities[0] == e3I);
Expand All @@ -1334,6 +1358,9 @@ TEST_CASE("ecs::QueryFilter")
auto view = filter.view().pin(0, e2I);
auto it = view.begin();
REQUIRE(it->entities[0] == e2I);
REQUIRE(it->entities[1] == e2I);
++it;
REQUIRE(it->entities[0] == e2I);
REQUIRE(it->entities[1] == e3I);
++it;
REQUIRE(it == view.end());
Expand All @@ -1356,6 +1383,9 @@ TEST_CASE("ecs::QueryFilter")
{
auto view = filter.view().pin(1, e2I);
auto it = view.begin();
REQUIRE(it->entities[0] == e2I);
REQUIRE(it->entities[1] == e2I);
++it;
REQUIRE(it->entities[0] == e3I);
REQUIRE(it->entities[1] == e2I);
++it;
Expand All @@ -1375,6 +1405,16 @@ TEST_CASE("ecs::QueryFilter")

SUBCASE("both targets pinned")
{
SUBCASE("e2I and e2I")
{
auto view = filter.view().pin(0, e2I).pin(1, e2I);
auto it = view.begin();
REQUIRE(it->entities[0] == e2I);
REQUIRE(it->entities[1] == e2I);
++it;
REQUIRE(it == view.end());
}

SUBCASE("e2I and e3I")
{
auto view = filter.view().pin(0, e2I).pin(1, e3I);
Expand Down
18 changes: 0 additions & 18 deletions core/tests/ecs/utils/action.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -834,24 +834,6 @@ void SingleRelationQueryAction::test(World& world, ExpectedWorld& expected)
// Make sure that the matched relation actually exists.
REQUIRE(it != range.second);
entities.erase(it);

if (entity == toEntity && !excludeDuplicates && mRelation.has<SymmetricTrait>())
{
// Then it should have been matched twice.
range = entities.equal_range(entity);
it = range.first;
for (; it != range.second; ++it)
{
if (it->second == toEntity)
{
break;
}
}

// Make sure that the matched relation actually exists.
REQUIRE(it != range.second);
entities.erase(it);
}
}
}
}
Expand Down
Loading