Fix command batching issue with adding ChildOf pair while removing name

SanderMertens · Jan 26, 2023 · 5fc3e45 · 5fc3e45
1 parent 7bc505b
commit 5fc3e45
Show file tree

Hide file tree

Showing 7 changed files with 108 additions and 32 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
     },
     {