From 5fc3e45f5eceb9fcad3acba619f3a04208318de1 Mon Sep 17 00:00:00 2001 From: Sander Mertens Date: Wed, 25 Jan 2023 17:21:40 -0800 Subject: [PATCH] Fix command batching issue with adding ChildOf pair while removing name --- flecs.c | 23 ++++++++----------- src/bootstrap.c | 17 +++++---------- src/stage.c | 6 ++--- test/api/project.json | 7 ++++-- test/api/src/DeferredActions.c | 28 ++++++++++++++++++++++++ test/api/src/Hierarchies.c | 40 ++++++++++++++++++++++++++++++++++ test/api/src/main.c | 19 ++++++++++++++-- 7 files changed, 108 insertions(+), 32 deletions(-) diff --git a/flecs.c b/flecs.c index acd106d144..9ad342afcf 100644 --- a/flecs.c +++ b/flecs.c @@ -10113,14 +10113,14 @@ void* flecs_defer_set( /* Find existing component. Make sure it's owned, so that we won't use the * component of a prefab. */ void *existing = NULL; - ecs_table_t *table = NULL; + ecs_table_t *table = NULL, *storage_table; if (idr) { /* Entity can only have existing component if id record exists */ ecs_record_t *r = flecs_entities_get(world, entity); table = r->table; - if (r && table) { + if (r && table && (storage_table = table->storage_table)) { const ecs_table_record_t *tr = flecs_id_record_get_table( - idr, table->storage_table); + idr, storage_table); if (tr) { /* Entity has the component */ ecs_vec_t *column = &table->data.columns[tr->column]; @@ -54028,13 +54028,10 @@ void flecs_on_parent_change(ecs_iter_t *it) { ecs_world_t *world = it->world; ecs_table_t *other_table = it->other_table, *table = it->table; - int32_t col = ecs_search(it->real_world, table, - ecs_pair(ecs_id(EcsIdentifier), EcsName), 0); - bool has_name = col != -1; - bool other_has_name = ecs_search(it->real_world, other_table, - ecs_pair(ecs_id(EcsIdentifier), EcsName), 0) != -1; - - if (!has_name && !other_has_name) { + EcsIdentifier *names = ecs_table_get_pair(it->real_world, + table, EcsIdentifier, EcsName, it->offset); + bool has_name = names != NULL; + if (!has_name) { /* If tables don't have names, index does not need to be updated */ return; } @@ -54046,6 +54043,8 @@ void flecs_on_parent_change(ecs_iter_t *it) { ecs_search(it->real_world, other_table, ecs_pair(EcsChildOf, EcsWildcard), &from_pair); + bool other_has_name = ecs_search(it->real_world, other_table, + ecs_pair(ecs_id(EcsIdentifier), EcsName), 0) != -1; bool to_has_name = has_name, from_has_name = other_has_name; if (it->event == EcsOnRemove) { if (from_pair != ecs_childof(0)) { @@ -54065,10 +54064,6 @@ void flecs_on_parent_change(ecs_iter_t *it) { from_has_name = has_name; } - /* Get the table column with names */ - EcsIdentifier *names = ecs_table_get_pair(it->real_world, - table, EcsIdentifier, EcsName, it->offset); - ecs_hashmap_t *from_index = 0; if (from_has_name) { from_index = flecs_id_name_index_get(world, from_pair); diff --git a/src/bootstrap.c b/src/bootstrap.c index bd5af77371..0068bda397 100644 --- a/src/bootstrap.c +++ b/src/bootstrap.c @@ -444,13 +444,10 @@ void flecs_on_parent_change(ecs_iter_t *it) { ecs_world_t *world = it->world; ecs_table_t *other_table = it->other_table, *table = it->table; - int32_t col = ecs_search(it->real_world, table, - ecs_pair(ecs_id(EcsIdentifier), EcsName), 0); - bool has_name = col != -1; - bool other_has_name = ecs_search(it->real_world, other_table, - ecs_pair(ecs_id(EcsIdentifier), EcsName), 0) != -1; - - if (!has_name && !other_has_name) { + EcsIdentifier *names = ecs_table_get_pair(it->real_world, + table, EcsIdentifier, EcsName, it->offset); + bool has_name = names != NULL; + if (!has_name) { /* If tables don't have names, index does not need to be updated */ return; } @@ -462,6 +459,8 @@ void flecs_on_parent_change(ecs_iter_t *it) { ecs_search(it->real_world, other_table, ecs_pair(EcsChildOf, EcsWildcard), &from_pair); + bool other_has_name = ecs_search(it->real_world, other_table, + ecs_pair(ecs_id(EcsIdentifier), EcsName), 0) != -1; bool to_has_name = has_name, from_has_name = other_has_name; if (it->event == EcsOnRemove) { if (from_pair != ecs_childof(0)) { @@ -481,10 +480,6 @@ void flecs_on_parent_change(ecs_iter_t *it) { from_has_name = has_name; } - /* Get the table column with names */ - EcsIdentifier *names = ecs_table_get_pair(it->real_world, - table, EcsIdentifier, EcsName, it->offset); - ecs_hashmap_t *from_index = 0; if (from_has_name) { from_index = flecs_id_name_index_get(world, from_pair); diff --git a/src/stage.c b/src/stage.c index a3ae022e29..d5154b56c4 100644 --- a/src/stage.c +++ b/src/stage.c @@ -391,14 +391,14 @@ void* flecs_defer_set( /* Find existing component. Make sure it's owned, so that we won't use the * component of a prefab. */ void *existing = NULL; - ecs_table_t *table = NULL; + ecs_table_t *table = NULL, *storage_table; if (idr) { /* Entity can only have existing component if id record exists */ ecs_record_t *r = flecs_entities_get(world, entity); table = r->table; - if (r && table) { + if (r && table && (storage_table = table->storage_table)) { const ecs_table_record_t *tr = flecs_id_record_get_table( - idr, table->storage_table); + idr, storage_table); if (tr) { /* Entity has the component */ ecs_vec_t *column = &table->data.columns[tr->column]; diff --git a/test/api/project.json b/test/api/project.json index 02a4c43a05..6e3cd8e3ee 100644 --- a/test/api/project.json +++ b/test/api/project.json @@ -490,7 +490,9 @@ "lookup_after_clear_from_root", "lookup_after_clear_from_parent", "lookup_after_delete_from_root", - "lookup_after_delete_from_parent" + "lookup_after_delete_from_parent", + "defer_batch_remove_name_w_add_childof", + "defer_batch_remove_childof_w_add_name" ] }, { "id": "Has", @@ -2297,7 +2299,8 @@ "defer_get_mut_no_on_set", "defer_existing_get_mut_no_on_set", "get_mut_override", - "set_override" + "set_override", + "absent_get_mut_for_entity_w_tag" ] }, { "id": "SingleThreadStaging", diff --git a/test/api/src/DeferredActions.c b/test/api/src/DeferredActions.c index bc96ec5c2d..b8a072c878 100644 --- a/test/api/src/DeferredActions.c +++ b/test/api/src/DeferredActions.c @@ -3225,3 +3225,31 @@ void DeferredActions_set_override() { ecs_fini(world); } + +void DeferredActions_absent_get_mut_for_entity_w_tag() { + ecs_world_t *world = ecs_mini(); + + ECS_TAG(world, Tag); + ECS_COMPONENT(world, Position); + + ecs_entity_t e = ecs_new(world, Tag); + test_assert(e != 0); + + ecs_defer_begin(world); + { + Position *p = ecs_get_mut(world, e, Position); + test_assert(p != NULL); + p->x = 10; + p->y = 20; + } + ecs_defer_end(world); + + { + const Position *p = ecs_get(world, e, Position); + test_assert(p != NULL); + test_int(p->x, 10); + test_int(p->y, 20); + } + + ecs_fini(world); +} diff --git a/test/api/src/Hierarchies.c b/test/api/src/Hierarchies.c index bb10ccbd26..ede505665c 100644 --- a/test/api/src/Hierarchies.c +++ b/test/api/src/Hierarchies.c @@ -1942,3 +1942,43 @@ void Hierarchies_lookup_after_delete_from_parent() { ecs_fini(world); } + +void Hierarchies_defer_batch_remove_name_w_add_childof() { + ecs_world_t *world = ecs_mini(); + + ecs_entity_t e = ecs_new_entity(world, "e"); + test_assert(e != 0); + test_str("e", ecs_get_name(world, e)); + test_assert(ecs_lookup_fullpath(world, "e") == e); + + ecs_entity_t parent = ecs_new_id(world); + ecs_defer_begin(world); + ecs_clear(world, e); + ecs_add_pair(world, e, EcsChildOf, parent); + ecs_defer_end(world); + + test_assert(ecs_lookup_fullpath(world, "e") == 0); + test_assert(ecs_has_pair(world, e, EcsChildOf, parent)); + test_assert(ecs_get_name(world, e) == NULL); + + ecs_fini(world); +} + +void Hierarchies_defer_batch_remove_childof_w_add_name() { + ecs_world_t *world = ecs_mini(); + + ecs_entity_t parent = ecs_new_id(world); + ecs_entity_t e = ecs_new_w_pair(world, EcsChildOf, parent); + test_assert(e != 0); + + ecs_defer_begin(world); + ecs_clear(world, e); + ecs_set_name(world, e, "e"); + ecs_defer_end(world); + + test_assert(ecs_get_name(world, e) != NULL); + test_assert(ecs_lookup_fullpath(world, "e") == e); + test_assert(!ecs_has_pair(world, e, EcsChildOf, parent)); + + ecs_fini(world); +} diff --git a/test/api/src/main.c b/test/api/src/main.c index 4e0a1a9d10..d678e3271b 100644 --- a/test/api/src/main.c +++ b/test/api/src/main.c @@ -464,6 +464,8 @@ void Hierarchies_lookup_after_clear_from_root(void); void Hierarchies_lookup_after_clear_from_parent(void); void Hierarchies_lookup_after_delete_from_root(void); void Hierarchies_lookup_after_delete_from_parent(void); +void Hierarchies_defer_batch_remove_name_w_add_childof(void); +void Hierarchies_defer_batch_remove_childof_w_add_name(void); // Testsuite 'Has' void Has_zero(void); @@ -2209,6 +2211,7 @@ void DeferredActions_defer_get_mut_no_on_set(void); void DeferredActions_defer_existing_get_mut_no_on_set(void); void DeferredActions_get_mut_override(void); void DeferredActions_set_override(void); +void DeferredActions_absent_get_mut_for_entity_w_tag(void); // Testsuite 'SingleThreadStaging' void SingleThreadStaging_setup(void); @@ -4125,6 +4128,14 @@ bake_test_case Hierarchies_testcases[] = { { "lookup_after_delete_from_parent", Hierarchies_lookup_after_delete_from_parent + }, + { + "defer_batch_remove_name_w_add_childof", + Hierarchies_defer_batch_remove_name_w_add_childof + }, + { + "defer_batch_remove_childof_w_add_name", + Hierarchies_defer_batch_remove_childof_w_add_name } }; @@ -10914,6 +10925,10 @@ bake_test_case DeferredActions_testcases[] = { { "set_override", DeferredActions_set_override + }, + { + "absent_get_mut_for_entity_w_tag", + DeferredActions_absent_get_mut_for_entity_w_tag } }; @@ -11585,7 +11600,7 @@ static bake_test_suite suites[] = { "Hierarchies", Hierarchies_setup, NULL, - 98, + 100, Hierarchies_testcases }, { @@ -11802,7 +11817,7 @@ static bake_test_suite suites[] = { "DeferredActions", NULL, NULL, - 110, + 111, DeferredActions_testcases }, {