#1600 Remove ordering guarantee between batched OnAdd/on_set on different components

Author: SanderMertens

distr/flecs.c

@@ -9299,51 +9299,6 @@ bool flecs_remove_invalid(
     return true;
 }
 
-static
-ecs_table_t* flecs_cmd_batch_add_diff(
-    ecs_world_t *world,
-    ecs_table_t *dst,
-    ecs_table_t *cur,
-    ecs_table_t *prev)
-{
-    int32_t p = 0, p_count = prev->type.count;
-    int32_t c = 0, c_count = cur->type.count;
-    ecs_id_t *p_ids = prev->type.array;
-    ecs_id_t *c_ids = cur->type.array;
-
-    for (; c < c_count;) {
-        ecs_id_t c_id = c_ids[c];
-        ecs_id_t p_id = p_ids[p];
-
-        if (p_id < c_id) {
-            /* Previous id no longer exists in new table, so it was removed */
-            dst = ecs_table_remove_id(world, dst, p_id);
-        }
-        if (c_id < p_id) {
-            /* Current id didn't exist in previous table, so it was added */
-            dst = ecs_table_add_id(world, dst, c_id);
-        }
-
-        c += c_id <= p_id;
-        p += p_id <= c_id;
-        if (p == p_count) {
-            break;
-        }
-    }
-
-    /* Remainder */
-    for (; p < p_count; p ++) {
-        ecs_id_t p_id = p_ids[p];
-        dst = ecs_table_remove_id(world, dst, p_id);
-    }
-    for (; c < c_count; c ++) {
-        ecs_id_t c_id = c_ids[c];
-        dst = ecs_table_add_id(world, dst, c_id);
-    }
-
-    return dst;
-}
-
 static
 void flecs_cmd_batch_for_entity(
     ecs_world_t *world,
@@ -9414,43 +9369,6 @@ void flecs_cmd_batch_for_entity(
             table = flecs_find_table_add(world, table, id, diff);
             world->info.cmd.batched_command_count ++;
             break;
-        case EcsCmdModified: {
-            /* If a modified was inserted for an existing component, the value
-             * of the component could have been changed. If this is the case,
-             * call on_set hooks before the OnAdd/OnRemove observers are invoked
-             * when moving the entity to a different table.
-             * This ensures that if OnAdd/OnRemove observers access the modified
-             * component value, the on_set hook has had the opportunity to
-             * run first to set any computed values of the component. */
-            int32_t row = ECS_RECORD_TO_ROW(r->row);
-            ecs_id_record_t *idr = flecs_id_record_get(world, cmd->id);
-            const ecs_table_record_t *tr = 
-                flecs_id_record_get_table(idr, start_table);
-            if (tr) {
-                const ecs_type_info_t *ti = idr->type_info;
-                ecs_iter_action_t on_set;
-                if ((on_set = ti->hooks.on_set)) {
-                    ecs_table_t *prev_table = r->table;
-                    ecs_defer_begin(world);
-                    flecs_invoke_hook(world, start_table, tr, 1, row, 
-                        &entity,cmd->id, ti, EcsOnSet, on_set);
-                    ecs_defer_end(world);
-
-                    /* Don't run on_set hook twice, but make sure to still
-                     * invoke observers. */
-                    cmd->kind = EcsCmdModifiedNoHook;
-
-                    /* If entity changed tables in hook, add difference to
-                     * destination table, so we don't lose the side effects
-                     * of the hook. */
-                    if (r->table != prev_table) {
-                        table = flecs_cmd_batch_add_diff(
-                            world, table, r->table, prev_table);
-                    }
-                }
-            }
-            break;
-        }
         case EcsCmdSet:
         case EcsCmdEnsure: {
             ecs_id_t *ids = diff->added.array;
@@ -9505,6 +9423,7 @@ void flecs_cmd_batch_for_entity(
         case EcsCmdEvent:
         case EcsCmdSkip:
         case EcsCmdModifiedNoHook:
+        case EcsCmdModified:
             break;
         }
 
@@ -16418,7 +16337,7 @@ bool flecs_defer_modified(
     ecs_id_t id)
 {
     if (flecs_defer_cmd(stage)) {
-        ecs_cmd_t *cmd = flecs_cmd_new_batched(stage, entity);
+        ecs_cmd_t *cmd = flecs_cmd_new(stage);
         if (cmd) {
             cmd->kind = EcsCmdModified;
             cmd->id = id;
@@ -67971,8 +67890,6 @@ ecs_var_id_t flecs_query_find_var_id(
             if (query->pub.flags & EcsQueryHasTableThisVar) {
                 return 0;
             } else {
-                printf("VARNONE\n");
-                flecs_dump_backtrace(stdout);
                 return EcsVarNone;
             }
         }

src/entity.c

@@ -4694,51 +4694,6 @@ bool flecs_remove_invalid(
     return true;
 }
 
-static
-ecs_table_t* flecs_cmd_batch_add_diff(
-    ecs_world_t *world,
-    ecs_table_t *dst,
-    ecs_table_t *cur,
-    ecs_table_t *prev)
-{
-    int32_t p = 0, p_count = prev->type.count;
-    int32_t c = 0, c_count = cur->type.count;
-    ecs_id_t *p_ids = prev->type.array;
-    ecs_id_t *c_ids = cur->type.array;
-
-    for (; c < c_count;) {
-        ecs_id_t c_id = c_ids[c];
-        ecs_id_t p_id = p_ids[p];
-
-        if (p_id < c_id) {
-            /* Previous id no longer exists in new table, so it was removed */
-            dst = ecs_table_remove_id(world, dst, p_id);
-        }
-        if (c_id < p_id) {
-            /* Current id didn't exist in previous table, so it was added */
-            dst = ecs_table_add_id(world, dst, c_id);
-        }
-
-        c += c_id <= p_id;
-        p += p_id <= c_id;
-        if (p == p_count) {
-            break;
-        }
-    }
-
-    /* Remainder */
-    for (; p < p_count; p ++) {
-        ecs_id_t p_id = p_ids[p];
-        dst = ecs_table_remove_id(world, dst, p_id);
-    }
-    for (; c < c_count; c ++) {
-        ecs_id_t c_id = c_ids[c];
-        dst = ecs_table_add_id(world, dst, c_id);
-    }
-
-    return dst;
-}
-
 static
 void flecs_cmd_batch_for_entity(
     ecs_world_t *world,
@@ -4809,43 +4764,6 @@ void flecs_cmd_batch_for_entity(
             table = flecs_find_table_add(world, table, id, diff);
             world->info.cmd.batched_command_count ++;
             break;
-        case EcsCmdModified: {
-            /* If a modified was inserted for an existing component, the value
-             * of the component could have been changed. If this is the case,
-             * call on_set hooks before the OnAdd/OnRemove observers are invoked
-             * when moving the entity to a different table.
-             * This ensures that if OnAdd/OnRemove observers access the modified
-             * component value, the on_set hook has had the opportunity to
-             * run first to set any computed values of the component. */
-            int32_t row = ECS_RECORD_TO_ROW(r->row);
-            ecs_id_record_t *idr = flecs_id_record_get(world, cmd->id);
-            const ecs_table_record_t *tr = 
-                flecs_id_record_get_table(idr, start_table);
-            if (tr) {
-                const ecs_type_info_t *ti = idr->type_info;
-                ecs_iter_action_t on_set;
-                if ((on_set = ti->hooks.on_set)) {
-                    ecs_table_t *prev_table = r->table;
-                    ecs_defer_begin(world);
-                    flecs_invoke_hook(world, start_table, tr, 1, row, 
-                        &entity,cmd->id, ti, EcsOnSet, on_set);
-                    ecs_defer_end(world);
-
-                    /* Don't run on_set hook twice, but make sure to still
-                     * invoke observers. */
-                    cmd->kind = EcsCmdModifiedNoHook;
-
-                    /* If entity changed tables in hook, add difference to
-                     * destination table, so we don't lose the side effects
-                     * of the hook. */
-                    if (r->table != prev_table) {
-                        table = flecs_cmd_batch_add_diff(
-                            world, table, r->table, prev_table);
-                    }
-                }
-            }
-            break;
-        }
         case EcsCmdSet:
         case EcsCmdEnsure: {
             ecs_id_t *ids = diff->added.array;
@@ -4900,6 +4818,7 @@ void flecs_cmd_batch_for_entity(
         case EcsCmdEvent:
         case EcsCmdSkip:
         case EcsCmdModifiedNoHook:
+        case EcsCmdModified:
             break;
         }
 

src/query/compiler/compiler_term.c

@@ -19,8 +19,6 @@ ecs_var_id_t flecs_query_find_var_id(
             if (query->pub.flags & EcsQueryHasTableThisVar) {
                 return 0;
             } else {
-                printf("VARNONE\n");
-                flecs_dump_backtrace(stdout);
                 return EcsVarNone;
             }
         }

src/stage.c

@@ -175,7 +175,7 @@ bool flecs_defer_modified(
     ecs_id_t id)
 {
     if (flecs_defer_cmd(stage)) {
-        ecs_cmd_t *cmd = flecs_cmd_new_batched(stage, entity);
+        ecs_cmd_t *cmd = flecs_cmd_new(stage);
         if (cmd) {
             cmd->kind = EcsCmdModified;
             cmd->id = id;

test/core/project.json

@@ -2261,7 +2261,6 @@
                 "ensure_override",
                 "set_override",
                 "absent_ensure_for_entity_w_tag",
-                "on_set_hook_before_on_add_for_existing_component",
                 "defer_2_sets_w_observer_same_component",
                 "defer_2_sets_w_observer_other_component",
                 "on_remove_after_deferred_clear_and_add",

test/core/src/Commands.c

@@ -3742,7 +3742,6 @@ void Commands_absent_ensure_for_entity_w_tag(void) {
 
 static int set_position_invoked = 0;
 static int set_velocity_invoked = 0;
-static int add_tag_invoked = 0;
 
 static void set_position_hook(ecs_iter_t *it) {
     set_position_invoked ++;
@@ -3752,45 +3751,6 @@ static void set_velocity_hook(ecs_iter_t *it) {
     set_velocity_invoked ++;
 }
 
-static void add_tag(ecs_iter_t *it) {
-    test_int(set_position_invoked, 1);
-    add_tag_invoked ++;
-}
-
-void Commands_on_set_hook_before_on_add_for_existing_component(void) {
-    ecs_world_t *world = ecs_mini();
-
-    ECS_COMPONENT(world, Position);
-    ECS_TAG(world, TagA);
-    ECS_TAG(world, TagB);
-
-    ecs_set_hooks(world, Position, {
-        .on_set = set_position_hook
-    });
-
-    ecs_observer(world, {
-        .query.terms[0].id = TagA,
-        .events = { EcsOnAdd },
-        .callback = add_tag
-    });
-
-    ecs_entity_t e = ecs_new_w(world, Position);
-
-    ecs_defer_begin(world);
-    ecs_add(world, e, TagB); /* 2 add commands, to trigger batching */
-    ecs_modified(world, e, Position);
-    ecs_add(world, e, TagA);
-
-    test_int(set_position_invoked, 0);
-    test_int(add_tag_invoked, 0);
-    ecs_defer_end(world);
-
-    test_assert(set_position_invoked != 0);
-    test_int(add_tag_invoked, 1);
-
-    ecs_fini(world);
-}
-
 void Commands_defer_2_sets_w_observer_same_component(void) {
     ecs_world_t *world = ecs_mini();
 

test/core/src/main.c

@@ -2180,7 +2180,6 @@ void Commands_defer_existing_ensure_no_on_set(void);
 void Commands_ensure_override(void);
 void Commands_set_override(void);
 void Commands_absent_ensure_for_entity_w_tag(void);
-void Commands_on_set_hook_before_on_add_for_existing_component(void);
 void Commands_defer_2_sets_w_observer_same_component(void);
 void Commands_defer_2_sets_w_observer_other_component(void);
 void Commands_on_remove_after_deferred_clear_and_add(void);
@@ -10831,10 +10830,6 @@ bake_test_case Commands_testcases[] = {
         "absent_ensure_for_entity_w_tag",
         Commands_absent_ensure_for_entity_w_tag
     },
-    {
-        "on_set_hook_before_on_add_for_existing_component",
-        Commands_on_set_hook_before_on_add_for_existing_component
-    },
     {
         "defer_2_sets_w_observer_same_component",
         Commands_defer_2_sets_w_observer_same_component
@@ -11850,7 +11845,7 @@ static bake_test_suite suites[] = {
         "Commands",
         NULL,
         NULL,
-        156,
+        155,
         Commands_testcases
     },
     {