Skip to content

Commit

Permalink
#1024 Fix recursive cleanup issue for child with multiple relationshi…
Browse files Browse the repository at this point in the history 
…ps to parent
  • Loading branch information
@SanderMertens
SanderMertens committed Aug 10, 2023
1 parent 6fd006a commit 953d9c7
Show file tree
Hide file tree
Showing 8 changed files with 165 additions and 61 deletions.
74 changes: 51 additions & 23 deletions flecs.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2698,7 +2698,7 @@ static
void flecs_register_on_delete_object(ecs_iter_t *it) {
ecs_id_t id = ecs_field_id(it, 1);
flecs_register_id_flag_for_relation(it, EcsOnDeleteTarget,
ECS_ID_ON_DELETE_OBJECT_FLAG(ECS_PAIR_SECOND(id)),
ECS_ID_ON_DELETE_TARGET_FLAG(ECS_PAIR_SECOND(id)),
EcsIdOnDeleteObjectMask,
EcsEntityIsId);
}
Expand Down Expand Up @@ -5469,11 +5469,11 @@ void flecs_targets_mark_for_delete(
if (flags & EcsEntityIsTarget) {
if ((idr = flecs_id_record_get(world, ecs_pair(EcsWildcard, e)))) {
flecs_id_mark_for_delete(world, idr,
ECS_ID_ON_DELETE_OBJECT(idr->flags), true);
ECS_ID_ON_DELETE_TARGET(idr->flags), true);
}
if ((idr = flecs_id_record_get(world, ecs_pair(EcsFlag, e)))) {
flecs_id_mark_for_delete(world, idr,
ECS_ID_ON_DELETE_OBJECT(idr->flags), true);
ECS_ID_ON_DELETE_TARGET(idr->flags), true);
}
}
}
Expand Down Expand Up @@ -5501,12 +5501,38 @@ ecs_entity_t flecs_get_delete_action(
{
ecs_entity_t result = action;
if (!result && delete_target) {
ecs_id_record_t *idr = (ecs_id_record_t*)tr->hdr.cache;
ecs_id_t id = idr->id;

/* If action is not specified and we're deleting a relationship target,
* derive the action from the current record */
ecs_table_record_t *trr = &table->_->records[tr->column];
ecs_id_record_t *idrr = (ecs_id_record_t*)trr->hdr.cache;
result = ECS_ID_ON_DELETE_OBJECT(idrr->flags);
int32_t i = tr->column, count = tr->count;
do {
ecs_type_t *type = &table->type;
ecs_table_record_t *trr = &table->_->records[i];
ecs_id_record_t *idrr = (ecs_id_record_t*)trr->hdr.cache;
result = ECS_ID_ON_DELETE_TARGET(idrr->flags);
if (result == EcsDelete) {
/* Delete takes precedence over Remove */
break;
}

if (count > 1) {
/* If table contains multiple relationships for target they are
* not guaranteed to occupy consecutive elements in the table's
* type vector, so a linear search is needed to find matches. */
for (++ i; i < type->count; i ++) {
if (ecs_id_match(type->array[i], id)) {
break;
}
}

/* We should always have as many matching ids as tr->count */
ecs_assert(i < type->count, ECS_INTERNAL_ERROR, NULL);
}
} while (--count);
}

return result;
}

Expand Down Expand Up @@ -5695,23 +5721,25 @@ bool flecs_on_delete_clear_tables(
for (i = last - 1; i >= first; i --) {
ecs_id_record_t *idr = ids[i].idr;
ecs_entity_t action = ids[i].action;

/* Empty all tables for id */
ecs_table_cache_iter_t it;
if (flecs_table_cache_iter(&idr->cache, &it)) {
ecs_table_record_t *tr;
while ((tr = flecs_table_cache_next(&it, ecs_table_record_t))) {
ecs_table_t *table = tr->hdr.table;

if ((action == EcsRemove) ||
!(table->flags & EcsTableMarkedForDelete))
{
flecs_remove_from_table(world, table);
} else {
ecs_dbg_3(
"#[red]delete#[reset] entities from table %u",
(uint32_t)table->id);
flecs_table_delete_entities(world, table);
/* Empty all tables for id */
{
ecs_table_cache_iter_t it;
if (flecs_table_cache_iter(&idr->cache, &it)) {
ecs_table_record_t *tr;
while ((tr = flecs_table_cache_next(&it, ecs_table_record_t))) {
ecs_table_t *table = tr->hdr.table;

if ((action == EcsRemove) ||
!(table->flags & EcsTableMarkedForDelete))
{
flecs_remove_from_table(world, table);
} else {
ecs_dbg_3(
"#[red]delete#[reset] entities from table %u",
(uint32_t)table->id);
flecs_table_delete_entities(world, table);
}
}
}
}
Expand Down Expand Up @@ -7786,7 +7814,7 @@ bool flecs_remove_invalid(
ecs_id_record_t *idr = flecs_id_record_get(world,
ecs_pair(rel, EcsWildcard));
if (idr) {
ecs_entity_t action = ECS_ID_ON_DELETE_OBJECT(idr->flags);
ecs_entity_t action = ECS_ID_ON_DELETE_TARGET(idr->flags);
if (action == EcsDelete) {
/* Entity should be deleted, don't bother checking
* other ids */
Expand Down
4 changes: 2 additions & 2 deletions flecs.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -353,9 +353,9 @@ extern "C" {
#define ECS_ID_ON_DELETE(flags) \
((ecs_entity_t[]){0, EcsRemove, EcsDelete, 0, EcsPanic}\
[((flags) & EcsIdOnDeleteMask)])
#define ECS_ID_ON_DELETE_OBJECT(flags) ECS_ID_ON_DELETE(flags >> 3)
#define ECS_ID_ON_DELETE_TARGET(flags) ECS_ID_ON_DELETE(flags >> 3)
#define ECS_ID_ON_DELETE_FLAG(id) (1u << ((id) - EcsRemove))
#define ECS_ID_ON_DELETE_OBJECT_FLAG(id) (1u << (3 + ((id) - EcsRemove)))
#define ECS_ID_ON_DELETE_TARGET_FLAG(id) (1u << (3 + ((id) - EcsRemove)))


////////////////////////////////////////////////////////////////////////////////
Expand Down
4 changes: 2 additions & 2 deletions include/flecs/private/api_flags.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,9 @@ extern "C" {
#define ECS_ID_ON_DELETE(flags) \
((ecs_entity_t[]){0, EcsRemove, EcsDelete, 0, EcsPanic}\
[((flags) & EcsIdOnDeleteMask)])
#define ECS_ID_ON_DELETE_OBJECT(flags) ECS_ID_ON_DELETE(flags >> 3)
#define ECS_ID_ON_DELETE_TARGET(flags) ECS_ID_ON_DELETE(flags >> 3)
#define ECS_ID_ON_DELETE_FLAG(id) (1u << ((id) - EcsRemove))
#define ECS_ID_ON_DELETE_OBJECT_FLAG(id) (1u << (3 + ((id) - EcsRemove)))
#define ECS_ID_ON_DELETE_TARGET_FLAG(id) (1u << (3 + ((id) - EcsRemove)))


////////////////////////////////////////////////////////////////////////////////
Expand Down
2 changes: 1 addition & 1 deletion src/bootstrap.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ static
void flecs_register_on_delete_object(ecs_iter_t *it) {
ecs_id_t id = ecs_field_id(it, 1);
flecs_register_id_flag_for_relation(it, EcsOnDeleteTarget,
ECS_ID_ON_DELETE_OBJECT_FLAG(ECS_PAIR_SECOND(id)),
ECS_ID_ON_DELETE_TARGET_FLAG(ECS_PAIR_SECOND(id)),
EcsIdOnDeleteObjectMask,
EcsEntityIsId);
}
Expand Down
72 changes: 50 additions & 22 deletions src/entity.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2119,11 +2119,11 @@ void flecs_targets_mark_for_delete(
if (flags & EcsEntityIsTarget) {
if ((idr = flecs_id_record_get(world, ecs_pair(EcsWildcard, e)))) {
flecs_id_mark_for_delete(world, idr,
ECS_ID_ON_DELETE_OBJECT(idr->flags), true);
ECS_ID_ON_DELETE_TARGET(idr->flags), true);
}
if ((idr = flecs_id_record_get(world, ecs_pair(EcsFlag, e)))) {
flecs_id_mark_for_delete(world, idr,
ECS_ID_ON_DELETE_OBJECT(idr->flags), true);
ECS_ID_ON_DELETE_TARGET(idr->flags), true);
}
}
}
Expand Down Expand Up @@ -2151,12 +2151,38 @@ ecs_entity_t flecs_get_delete_action(
{
ecs_entity_t result = action;
if (!result && delete_target) {
ecs_id_record_t *idr = (ecs_id_record_t*)tr->hdr.cache;
ecs_id_t id = idr->id;

/* If action is not specified and we're deleting a relationship target,
* derive the action from the current record */
ecs_table_record_t *trr = &table->_->records[tr->column];
ecs_id_record_t *idrr = (ecs_id_record_t*)trr->hdr.cache;
result = ECS_ID_ON_DELETE_OBJECT(idrr->flags);
int32_t i = tr->column, count = tr->count;
do {
ecs_type_t *type = &table->type;
ecs_table_record_t *trr = &table->_->records[i];
ecs_id_record_t *idrr = (ecs_id_record_t*)trr->hdr.cache;
result = ECS_ID_ON_DELETE_TARGET(idrr->flags);
if (result == EcsDelete) {
/* Delete takes precedence over Remove */
break;
}

if (count > 1) {
/* If table contains multiple pairs for target they are not
* guaranteed to occupy consecutive elements in the table's type
* vector, so a linear search is needed to find matches. */
for (++ i; i < type->count; i ++) {
if (ecs_id_match(type->array[i], id)) {
break;
}
}

/* We should always have as many matching ids as tr->count */
ecs_assert(i < type->count, ECS_INTERNAL_ERROR, NULL);
}
} while (--count);
}

return result;
}

Expand Down Expand Up @@ -2345,23 +2371,25 @@ bool flecs_on_delete_clear_tables(
for (i = last - 1; i >= first; i --) {
ecs_id_record_t *idr = ids[i].idr;
ecs_entity_t action = ids[i].action;

/* Empty all tables for id */
ecs_table_cache_iter_t it;
if (flecs_table_cache_iter(&idr->cache, &it)) {
ecs_table_record_t *tr;
while ((tr = flecs_table_cache_next(&it, ecs_table_record_t))) {
ecs_table_t *table = tr->hdr.table;

if ((action == EcsRemove) ||
!(table->flags & EcsTableMarkedForDelete))
{
flecs_remove_from_table(world, table);
} else {
ecs_dbg_3(
"#[red]delete#[reset] entities from table %u",
(uint32_t)table->id);
flecs_table_delete_entities(world, table);
{
ecs_table_cache_iter_t it;
if (flecs_table_cache_iter(&idr->cache, &it)) {
ecs_table_record_t *tr;
while ((tr = flecs_table_cache_next(&it, ecs_table_record_t))) {
ecs_table_t *table = tr->hdr.table;

if ((action == EcsRemove) ||
!(table->flags & EcsTableMarkedForDelete))
{
flecs_remove_from_table(world, table);
} else {
ecs_dbg_3(
"#[red]delete#[reset] entities from table %u",
(uint32_t)table->id);
flecs_table_delete_entities(world, table);
}
}
}
}
Expand Down Expand Up @@ -4436,7 +4464,7 @@ bool flecs_remove_invalid(
ecs_id_record_t *idr = flecs_id_record_get(world,
ecs_pair(rel, EcsWildcard));
if (idr) {
ecs_entity_t action = ECS_ID_ON_DELETE_OBJECT(idr->flags);
ecs_entity_t action = ECS_ID_ON_DELETE_TARGET(idr->flags);
if (action == EcsDelete) {
/* Entity should be deleted, don't bother checking
* other ids */
Expand Down
4 changes: 3 additions & 1 deletion test/api/project.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -788,7 +788,9 @@
"nested_delete_with",
"deferred_delete_with_after_create_named",
"deferred_delete_with_childof_after_create_named",
"match_marked_for_deletion"
"match_marked_for_deletion",
"delete_w_low_rel_mixed_cleanup",
"delete_w_low_rel_mixed_cleanup_interleaved_ids"
]
}, {
"id": "Set",
Expand Down
54 changes: 45 additions & 9 deletions test/api/src/OnDelete.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,25 +13,25 @@ void OnDelete_flags() {
test_int(ECS_ID_ON_DELETE(f_delete), EcsDelete);
test_int(ECS_ID_ON_DELETE(f_throw), EcsPanic);

test_int(ECS_ID_ON_DELETE_OBJECT(f_obj_remove), EcsRemove);
test_int(ECS_ID_ON_DELETE_OBJECT(f_obj_delete), EcsDelete);
test_int(ECS_ID_ON_DELETE_OBJECT(f_obj_throw), EcsPanic);
test_int(ECS_ID_ON_DELETE_TARGET(f_obj_remove), EcsRemove);
test_int(ECS_ID_ON_DELETE_TARGET(f_obj_delete), EcsDelete);
test_int(ECS_ID_ON_DELETE_TARGET(f_obj_throw), EcsPanic);

test_int(ECS_ID_ON_DELETE(f_obj_remove), 0);
test_int(ECS_ID_ON_DELETE(f_obj_delete), 0);
test_int(ECS_ID_ON_DELETE(f_obj_throw), 0);

test_int(ECS_ID_ON_DELETE_OBJECT(f_remove), 0);
test_int(ECS_ID_ON_DELETE_OBJECT(f_delete), 0);
test_int(ECS_ID_ON_DELETE_OBJECT(f_throw), 0);
test_int(ECS_ID_ON_DELETE_TARGET(f_remove), 0);
test_int(ECS_ID_ON_DELETE_TARGET(f_delete), 0);
test_int(ECS_ID_ON_DELETE_TARGET(f_throw), 0);

test_int(ECS_ID_ON_DELETE_FLAG(EcsRemove), f_remove);
test_int(ECS_ID_ON_DELETE_FLAG(EcsDelete), f_delete);
test_int(ECS_ID_ON_DELETE_FLAG(EcsPanic), f_throw);

test_int(ECS_ID_ON_DELETE_OBJECT_FLAG(EcsRemove), f_obj_remove);
test_int(ECS_ID_ON_DELETE_OBJECT_FLAG(EcsDelete), f_obj_delete);
test_int(ECS_ID_ON_DELETE_OBJECT_FLAG(EcsPanic), f_obj_throw);
test_int(ECS_ID_ON_DELETE_TARGET_FLAG(EcsRemove), f_obj_remove);
test_int(ECS_ID_ON_DELETE_TARGET_FLAG(EcsDelete), f_obj_delete);
test_int(ECS_ID_ON_DELETE_TARGET_FLAG(EcsPanic), f_obj_throw);
}

void OnDelete_id_default() {
Expand Down Expand Up @@ -3025,3 +3025,39 @@ void OnDelete_match_marked_for_deletion() {

test_assert(true); /* Ensure cleanup was successful */
}

void OnDelete_delete_w_low_rel_mixed_cleanup() {
ecs_world_t *world = ecs_mini();

ecs_entity_t rel = ecs_new_low_id(world);
ecs_entity_t parent = ecs_new_id(world);
ecs_entity_t child = ecs_new_id(world);
ecs_add_pair(world, child, EcsChildOf, parent);
ecs_add_pair(world, child, rel, parent);

ecs_delete(world, parent);

test_assert(!ecs_is_alive(world, parent));
test_assert(!ecs_is_alive(world, child));

ecs_fini(world);
}

void OnDelete_delete_w_low_rel_mixed_cleanup_interleaved_ids() {
ecs_world_t *world = ecs_mini();

ecs_entity_t rel = ecs_new_low_id(world);
ecs_entity_t parent = ecs_new_id(world);
ecs_entity_t other = ecs_new_id(world);
ecs_entity_t child = ecs_new_id(world);
ecs_add_pair(world, child, EcsChildOf, parent);
ecs_add_pair(world, child, rel, other);
ecs_add_pair(world, child, rel, parent);

ecs_delete(world, parent);

test_assert(!ecs_is_alive(world, parent));
test_assert(!ecs_is_alive(world, child));

ecs_fini(world);
}
12 changes: 11 additions & 1 deletion test/api/src/main.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -748,6 +748,8 @@ void OnDelete_nested_delete_with(void);
void OnDelete_deferred_delete_with_after_create_named(void);
void OnDelete_deferred_delete_with_childof_after_create_named(void);
void OnDelete_match_marked_for_deletion(void);
void OnDelete_delete_w_low_rel_mixed_cleanup(void);
void OnDelete_delete_w_low_rel_mixed_cleanup_interleaved_ids(void);

// Testsuite 'Set'
void Set_set_empty(void);
Expand Down Expand Up @@ -5428,6 +5430,14 @@ bake_test_case OnDelete_testcases[] = {
{
"match_marked_for_deletion",
OnDelete_match_marked_for_deletion
},
{
"delete_w_low_rel_mixed_cleanup",
OnDelete_delete_w_low_rel_mixed_cleanup
},
{
"delete_w_low_rel_mixed_cleanup_interleaved_ids",
OnDelete_delete_w_low_rel_mixed_cleanup_interleaved_ids
}
};

Expand Down Expand Up @@ -12694,7 +12704,7 @@ static bake_test_suite suites[] = {
"OnDelete",
NULL,
NULL,
107,
109,
OnDelete_testcases
},
{
Expand Down

0 comments on commit 953d9c7

Please sign in to comment.