From 50e039009f992a575c64925641319c40a0025624 Mon Sep 17 00:00:00 2001 From: Sander Mertens Date: Fri, 29 Sep 2023 22:45:23 -0700 Subject: [PATCH] Fix issue where regular entities would be cleaned up after component entities --- flecs.c | 67 +++++++++++++++++++++++++++-------------- flecs.h | 2 +- src/world.c | 67 +++++++++++++++++++++++++++-------------- test/api/project.json | 3 ++ test/api/src/Observer.c | 14 ++++----- test/api/src/OnDelete.c | 62 ++++++++++++++++++++++++++++++++++++++ test/api/src/main.c | 17 ++++++++++- 7 files changed, 178 insertions(+), 54 deletions(-) diff --git a/flecs.c b/flecs.c index 15044a87b2..cd93c9ef52 100644 --- a/flecs.c +++ b/flecs.c @@ -21976,22 +21976,17 @@ void flecs_clean_tables( } static -void flecs_fini_roots(ecs_world_t *world) { - ecs_id_record_t *idr = flecs_id_record_get(world, ecs_pair(EcsChildOf, 0)); - - ecs_run_aperiodic(world, EcsAperiodicEmptyTables); - +void flecs_fini_root_tables( + ecs_world_t *world, + ecs_id_record_t *idr, + bool fini_targets) +{ ecs_table_cache_iter_t it; + bool has_roots = flecs_table_cache_iter(&idr->cache, &it); ecs_assert(has_roots == true, ECS_INTERNAL_ERROR, NULL); (void)has_roots; - /* Delete root entities that are not modules. This prioritizes deleting - * regular entities first, which reduces the chance of components getting - * destructed in random order because it got deleted before entities, - * thereby bypassing the OnDeleteTarget policy. */ - flecs_defer_begin(world, &world->stages[0]); - const ecs_table_record_t *tr; while ((tr = flecs_table_cache_next(&it, ecs_table_record_t))) { ecs_table_t *table = tr->hdr.table; @@ -22002,21 +21997,49 @@ void flecs_fini_roots(ecs_world_t *world) { int32_t i, count = table->data.entities.count; ecs_entity_t *entities = table->data.entities.array; - /* Count backwards so that we're always deleting the last entity in the - * table which reduces moving components around */ - for (i = count - 1; i >= 0; i --) { - ecs_record_t *r = flecs_entities_get(world, entities[i]); - ecs_assert(r != NULL, ECS_INTERNAL_ERROR, NULL); - - ecs_flags32_t flags = ECS_RECORD_TO_ROW_FLAGS(r->row); - if (!(flags & EcsEntityIsTarget)) { - continue; /* Filter out entities that aren't objects */ + if (fini_targets) { + /* Only delete entities that are used as pair target. Iterate + * backwards to minimize moving entities around in table. */ + for (i = count - 1; i >= 0; i --) { + ecs_record_t *r = flecs_entities_get(world, entities[i]); + ecs_assert(r != NULL, ECS_INTERNAL_ERROR, NULL); + if (ECS_RECORD_TO_ROW_FLAGS(r->row) & EcsEntityIsTarget) { + ecs_delete(world, entities[i]); + } + } + } else { + /* Delete remaining entities that are not in use (added to another + * entity). This limits table moves during cleanup and delays + * cleanup of tags. */ + for (i = count - 1; i >= 0; i --) { + ecs_record_t *r = flecs_entities_get(world, entities[i]); + ecs_assert(r != NULL, ECS_INTERNAL_ERROR, NULL); + if (!ECS_RECORD_TO_ROW_FLAGS(r->row)) { + ecs_delete(world, entities[i]); + } } - - ecs_delete(world, entities[i]); } } +} + +static +void flecs_fini_roots( + ecs_world_t *world) +{ + ecs_id_record_t *idr = flecs_id_record_get(world, ecs_pair(EcsChildOf, 0)); + ecs_run_aperiodic(world, EcsAperiodicEmptyTables); + + /* Delete root entities that are not modules. This prioritizes deleting + * regular entities first, which reduces the chance of components getting + * destructed in random order because it got deleted before entities, + * thereby bypassing the OnDeleteTarget policy. */ + flecs_defer_begin(world, &world->stages[0]); + flecs_fini_root_tables(world, idr, true); + flecs_defer_end(world, &world->stages[0]); + + flecs_defer_begin(world, &world->stages[0]); + flecs_fini_root_tables(world, idr, false); flecs_defer_end(world, &world->stages[0]); } diff --git a/flecs.h b/flecs.h index 7f9960ff5a..6a9185a739 100644 --- a/flecs.h +++ b/flecs.h @@ -11213,7 +11213,7 @@ ecs_entity_t ecs_system_init( * Any system may interrupt execution by setting the interrupted_by member in * the ecs_iter_t value. This is particularly useful for manual systems, where * the value of interrupted_by is returned by this operation. This, in - * combination with the param argument lets applications use manual systems + * cominbation with the param argument lets applications use manual systems * to lookup entities: once the entity has been found its handle is passed to * interrupted_by, which is then subsequently returned. * diff --git a/src/world.c b/src/world.c index 9a3751e4ae..91c8156d75 100644 --- a/src/world.c +++ b/src/world.c @@ -592,22 +592,17 @@ void flecs_clean_tables( } static -void flecs_fini_roots(ecs_world_t *world) { - ecs_id_record_t *idr = flecs_id_record_get(world, ecs_pair(EcsChildOf, 0)); - - ecs_run_aperiodic(world, EcsAperiodicEmptyTables); - +void flecs_fini_root_tables( + ecs_world_t *world, + ecs_id_record_t *idr, + bool fini_targets) +{ ecs_table_cache_iter_t it; + bool has_roots = flecs_table_cache_iter(&idr->cache, &it); ecs_assert(has_roots == true, ECS_INTERNAL_ERROR, NULL); (void)has_roots; - /* Delete root entities that are not modules. This prioritizes deleting - * regular entities first, which reduces the chance of components getting - * destructed in random order because it got deleted before entities, - * thereby bypassing the OnDeleteTarget policy. */ - flecs_defer_begin(world, &world->stages[0]); - const ecs_table_record_t *tr; while ((tr = flecs_table_cache_next(&it, ecs_table_record_t))) { ecs_table_t *table = tr->hdr.table; @@ -618,21 +613,49 @@ void flecs_fini_roots(ecs_world_t *world) { int32_t i, count = table->data.entities.count; ecs_entity_t *entities = table->data.entities.array; - /* Count backwards so that we're always deleting the last entity in the - * table which reduces moving components around */ - for (i = count - 1; i >= 0; i --) { - ecs_record_t *r = flecs_entities_get(world, entities[i]); - ecs_assert(r != NULL, ECS_INTERNAL_ERROR, NULL); - - ecs_flags32_t flags = ECS_RECORD_TO_ROW_FLAGS(r->row); - if (!(flags & EcsEntityIsTarget)) { - continue; /* Filter out entities that aren't objects */ + if (fini_targets) { + /* Only delete entities that are used as pair target. Iterate + * backwards to minimize moving entities around in table. */ + for (i = count - 1; i >= 0; i --) { + ecs_record_t *r = flecs_entities_get(world, entities[i]); + ecs_assert(r != NULL, ECS_INTERNAL_ERROR, NULL); + if (ECS_RECORD_TO_ROW_FLAGS(r->row) & EcsEntityIsTarget) { + ecs_delete(world, entities[i]); + } + } + } else { + /* Delete remaining entities that are not in use (added to another + * entity). This limits table moves during cleanup and delays + * cleanup of tags. */ + for (i = count - 1; i >= 0; i --) { + ecs_record_t *r = flecs_entities_get(world, entities[i]); + ecs_assert(r != NULL, ECS_INTERNAL_ERROR, NULL); + if (!ECS_RECORD_TO_ROW_FLAGS(r->row)) { + ecs_delete(world, entities[i]); + } } - - ecs_delete(world, entities[i]); } } +} + +static +void flecs_fini_roots( + ecs_world_t *world) +{ + ecs_id_record_t *idr = flecs_id_record_get(world, ecs_pair(EcsChildOf, 0)); + + ecs_run_aperiodic(world, EcsAperiodicEmptyTables); + /* Delete root entities that are not modules. This prioritizes deleting + * regular entities first, which reduces the chance of components getting + * destructed in random order because it got deleted before entities, + * thereby bypassing the OnDeleteTarget policy. */ + flecs_defer_begin(world, &world->stages[0]); + flecs_fini_root_tables(world, idr, true); + flecs_defer_end(world, &world->stages[0]); + + flecs_defer_begin(world, &world->stages[0]); + flecs_fini_root_tables(world, idr, false); flecs_defer_end(world, &world->stages[0]); } diff --git a/test/api/project.json b/test/api/project.json index 45b4768691..3ed3df57d4 100644 --- a/test/api/project.json +++ b/test/api/project.json @@ -789,6 +789,9 @@ "add_deleted_in_on_remove", "delete_tree_w_query", "fini_cleanup_order", + "fini_cleanup_order_root_id_w_trait", + "fini_cleanup_order_entity_after_singleton", + "fini_cleanup_order_entity_after_component", "on_delete_parent_w_in_use_id_w_remove", "on_delete_parent_w_in_use_id_w_delete", "create_after_delete_with", diff --git a/test/api/src/Observer.c b/test/api/src/Observer.c index a695354b7f..029fa8c37e 100644 --- a/test/api/src/Observer.c +++ b/test/api/src/Observer.c @@ -1838,15 +1838,14 @@ void Observer_unset_on_fini_1(void) { ecs_fini(world); - test_int(ctx.invoked, 1); test_int(ctx.count, 3); test_int(ctx.system, UnSet); test_int(ctx.term_count, 1); test_null(ctx.param); - test_int(ctx.e[0], e1); + test_int(ctx.e[0], e3); test_int(ctx.e[1], e2); - test_int(ctx.e[2], e3); + test_int(ctx.e[2], e1); test_int(ctx.c[0][0], ecs_id(Position)); test_int(ctx.s[0][0], 0); @@ -1887,9 +1886,9 @@ void Observer_unset_on_fini_2(void) { test_int(ctx.term_count, 2); test_null(ctx.param); - test_int(ctx.e[0], e1); + test_int(ctx.e[0], e3); test_int(ctx.e[1], e2); - test_int(ctx.e[2], e3); + test_int(ctx.e[2], e1); test_int(ctx.c[0][0], ecs_id(Position)); test_int(ctx.s[0][0], 0); @@ -1934,15 +1933,14 @@ void Observer_unset_on_fini_3(void) { ecs_fini(world); - test_int(ctx.invoked, 1); test_int(ctx.count, 3); test_int(ctx.system, UnSet); test_int(ctx.term_count, 3); test_null(ctx.param); - test_int(ctx.e[0], e1); + test_int(ctx.e[0], e3); test_int(ctx.e[1], e2); - test_int(ctx.e[2], e3); + test_int(ctx.e[2], e1); test_int(ctx.c[0][0], ecs_id(Position)); test_int(ctx.s[0][0], 0); diff --git a/test/api/src/OnDelete.c b/test/api/src/OnDelete.c index 91a7ee970c..80b49ac97c 100644 --- a/test/api/src/OnDelete.c +++ b/test/api/src/OnDelete.c @@ -2727,6 +2727,67 @@ void OnDelete_fini_cleanup_order(void) { test_int(test_alive_parent_count, 3); } +void OnDelete_fini_cleanup_order_root_id_w_trait(void) { + ecs_world_t *world = ecs_mini(); + + ecs_entity_t r = ecs_new_id(world); + ecs_add_id(world, r, EcsExclusive); + ecs_entity_t t = ecs_new_id(world); + + ecs_entity_t e = ecs_new_id(world); + ecs_add_pair(world, e, r, t); + + ecs_fini(world); + + test_assert(true); // fini should not assert +} + +static ECS_COMPONENT_DECLARE(Position); + +static int on_remove_velocity = 0; +static void ecs_on_remove(Velocity)(ecs_iter_t *it) { + on_remove_velocity ++; + test_assert(ecs_is_alive(it->world, ecs_id(Position))); +} + +void OnDelete_fini_cleanup_order_entity_after_singleton(void) { + ecs_world_t *world = ecs_mini(); + + ECS_COMPONENT_DEFINE(world, Position); + ECS_COMPONENT(world, Velocity); + + ecs_set_hooks(world, Velocity, { + .on_remove = ecs_on_remove(Velocity) + }); + + ecs_singleton_set(world, Position, {10, 20}); + + ecs_entity_t e = ecs_new_id(world); + ecs_add(world, e, Velocity); + + ecs_fini(world); + + test_int(on_remove_velocity, 1); +} + +void OnDelete_fini_cleanup_order_entity_after_component(void) { + ecs_world_t *world = ecs_mini(); + + ECS_COMPONENT_DEFINE(world, Position); + ECS_COMPONENT(world, Velocity); + + ecs_set_hooks(world, Velocity, { + .on_remove = ecs_on_remove(Velocity) + }); + + ecs_entity_t e = ecs_new_id(world); + ecs_add(world, e, Velocity); + + ecs_fini(world); + + test_int(on_remove_velocity, 1); +} + void OnDelete_on_delete_parent_w_in_use_id_w_remove(void) { ecs_world_t *world = ecs_mini(); @@ -3060,3 +3121,4 @@ void OnDelete_delete_w_low_rel_mixed_cleanup_interleaved_ids(void) { ecs_fini(world); } + diff --git a/test/api/src/main.c b/test/api/src/main.c index ef82f4b3ce..6d46aac8fc 100644 --- a/test/api/src/main.c +++ b/test/api/src/main.c @@ -748,6 +748,9 @@ void OnDelete_delete_nested_in_on_remove(void); void OnDelete_add_deleted_in_on_remove(void); void OnDelete_delete_tree_w_query(void); void OnDelete_fini_cleanup_order(void); +void OnDelete_fini_cleanup_order_root_id_w_trait(void); +void OnDelete_fini_cleanup_order_entity_after_singleton(void); +void OnDelete_fini_cleanup_order_entity_after_component(void); void OnDelete_on_delete_parent_w_in_use_id_w_remove(void); void OnDelete_on_delete_parent_w_in_use_id_w_delete(void); void OnDelete_create_after_delete_with(void); @@ -5467,6 +5470,18 @@ bake_test_case OnDelete_testcases[] = { "fini_cleanup_order", OnDelete_fini_cleanup_order }, + { + "fini_cleanup_order_root_id_w_trait", + OnDelete_fini_cleanup_order_root_id_w_trait + }, + { + "fini_cleanup_order_entity_after_singleton", + OnDelete_fini_cleanup_order_entity_after_singleton + }, + { + "fini_cleanup_order_entity_after_component", + OnDelete_fini_cleanup_order_entity_after_component + }, { "on_delete_parent_w_in_use_id_w_remove", OnDelete_on_delete_parent_w_in_use_id_w_remove @@ -12884,7 +12899,7 @@ static bake_test_suite suites[] = { "OnDelete", NULL, NULL, - 110, + 113, OnDelete_testcases }, {