From 3ea29d581fbb24b61b8829dab6a92fa7219a3d06 Mon Sep 17 00:00:00 2001 From: Ricardo Antunes Date: Sat, 16 Nov 2024 20:52:13 +0100 Subject: [PATCH 1/2] fix(ecs): remove duplicate query matches on self-relations Previously the behavior queries had with symmetric relations on entities was not consistent. For example, if we had a symmetric relation R, and an entity 0, with a relation from 0 to 0, then, when querying over relation R, we would get (0, 0) twice! This only happened when the query was asymmetric, i.e, when the requirements on both targets were different. If they were the same, we would only get (0, 0). This lead to some inconsistencies between the tests and the query code. I also think it was a bit confusing, so I changed it to always return only one match in these cases. --- CHANGELOG.md | 1 + core/src/ecs/query/node/related.cpp | 31 +++++++++--- core/tests/ecs/query/filter.cpp | 76 ++++++++++++++++++++++------- core/tests/ecs/utils/action.cpp | 18 ------- 4 files changed, 84 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 37f21c1639..3a7166f474 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,7 @@ 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**). ## [v0.4.0] - 2024-10-13 diff --git a/core/src/ecs/query/node/related.cpp b/core/src/ecs/query/node/related.cpp index 0b4a7ae719..2197abd02f 100644 --- a/core/src/ecs/query/node/related.cpp +++ b/core/src/ecs/query/node/related.cpp @@ -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 = [&]() { @@ -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 { @@ -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(); } @@ -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; + } } } diff --git a/core/tests/ecs/query/filter.cpp b/core/tests/ecs/query/filter.cpp index 13a099dc8f..0a3eaddaa2 100644 --- a/core/tests/ecs/query/filter.cpp +++ b/core/tests/ecs/query/filter.cpp @@ -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{}); @@ -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); @@ -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); @@ -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; @@ -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); @@ -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; @@ -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()); } @@ -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()); } @@ -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()); } @@ -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()); } @@ -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()); } @@ -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()); @@ -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()); @@ -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; @@ -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); @@ -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); @@ -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()); @@ -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; @@ -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); diff --git a/core/tests/ecs/utils/action.cpp b/core/tests/ecs/utils/action.cpp index 5d4a5886d1..6b38010f1e 100644 --- a/core/tests/ecs/utils/action.cpp +++ b/core/tests/ecs/utils/action.cpp @@ -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()) - { - // 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); - } } } } From 4265115316e63374607c48f4235013eb9fc584bd Mon Sep 17 00:00:00 2001 From: Ricardo Antunes Date: Sun, 17 Nov 2024 18:00:49 +0100 Subject: [PATCH 2/2] fix(ecs): avoid creating new tables while iterating over the table set This was causing undefined behavior, as within the table iteration loop, new tables could be created with calls to propagateDepth --- CHANGELOG.md | 1 + core/src/ecs/world.cpp | 47 ++++++++++++++++++++++++++++-------------- 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a7166f474..2a00aee5ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - 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 diff --git a/core/src/ecs/world.cpp b/core/src/ecs/world.cpp index 7088da797d..a42ce8c495 100644 --- a/core/src/ecs/world.cpp +++ b/core/src/ecs/world.cpp @@ -176,44 +176,59 @@ void World::destroy(Entity entity) mEntityPool.destroy(entity.index); mTables.dense().at(archetype).swapErase(entity.index); + std::vector> 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. + // + // 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) {