Skip to content

Commit

Permalink
Fix issue where on_set hook was invoked without deferred mode & could…
Browse files Browse the repository at this point in the history 
… be called twice
  • Loading branch information
@SanderMertens
SanderMertens committed Nov 28, 2023
1 parent e1b9798 commit f3bc0c9
Show file tree
Hide file tree
Showing 6 changed files with 96 additions and 10 deletions.
24 changes: 20 additions & 4 deletions flecs.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1167,6 +1167,7 @@ typedef enum ecs_cmd_kind_t {
EcsCmdEmplace,
EcsCmdMut,
EcsCmdModified,
EcsCmdModifiedNoHook,
EcsCmdAddModified,
EcsCmdPath,
EcsCmdDelete,
Expand Down Expand Up @@ -4438,6 +4439,8 @@ void flecs_invoke_hook(

ecs_iter_t it = { .field_count = 1};
it.entities = entities;

// ecs_assert(ecs_is_deferred(world), ECS_INTERNAL_ERROR, NULL);

flecs_iter_init(world, &it, flecs_iter_cache_all);
it.world = world;
Expand Down Expand Up @@ -6435,7 +6438,8 @@ static
void flecs_modified_id_if(
ecs_world_t *world,
ecs_entity_t entity,
ecs_id_t id)
ecs_id_t id,
bool owned)
{
ecs_check(world != NULL, ECS_INVALID_PARAMETER, NULL);
ecs_check(ecs_is_alive(world, entity), ECS_INVALID_PARAMETER, NULL);
Expand All @@ -6455,7 +6459,7 @@ void flecs_modified_id_if(
}

ecs_type_t ids = { .array = &id, .count = 1 };
flecs_notify_on_set(world, table, ECS_RECORD_TO_ROW(r->row), 1, &ids, true);
flecs_notify_on_set(world, table, ECS_RECORD_TO_ROW(r->row), 1, &ids, owned);

flecs_table_mark_dirty(world, table, id);
flecs_defer_end(world, stage);
Expand Down Expand Up @@ -8008,8 +8012,14 @@ void flecs_cmd_batch_for_entity(
const ecs_type_info_t *ti = ptr.ti;
ecs_iter_action_t on_set;
if ((on_set = ti->hooks.on_set)) {
ecs_defer_begin(world);
flecs_invoke_hook(world, start_table, 1, row, &entity,
ptr.ptr, cmd->id, ptr.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;
}
}
}
Expand Down Expand Up @@ -8047,6 +8057,7 @@ void flecs_cmd_batch_for_entity(
case EcsCmdDisable:
case EcsCmdEvent:
case EcsCmdSkip:
case EcsCmdModifiedNoHook:
break;
}

Expand Down Expand Up @@ -8129,6 +8140,7 @@ void flecs_cmd_batch_for_entity(
case EcsCmdRemove:
case EcsCmdEmplace:
case EcsCmdModified:
case EcsCmdModifiedNoHook:
case EcsCmdAddModified:
case EcsCmdPath:
case EcsCmdDelete:
Expand Down Expand Up @@ -8256,12 +8268,16 @@ bool flecs_defer_end(
world->info.cmd.get_mut_count ++;
break;
case EcsCmdModified:
flecs_modified_id_if(world, e, id);
flecs_modified_id_if(world, e, id, true);
world->info.cmd.modified_count ++;
break;
case EcsCmdModifiedNoHook:
flecs_modified_id_if(world, e, id, false);
world->info.cmd.modified_count ++;
break;
case EcsCmdAddModified:
flecs_add_id(world, e, id);
flecs_modified_id_if(world, e, id);
flecs_modified_id_if(world, e, id, true);
world->info.cmd.add_count ++;
world->info.cmd.modified_count ++;
break;
Expand Down
23 changes: 19 additions & 4 deletions src/entity.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1039,6 +1039,8 @@ void flecs_invoke_hook(

ecs_iter_t it = { .field_count = 1};
it.entities = entities;

// ecs_assert(ecs_is_deferred(world), ECS_INTERNAL_ERROR, NULL);

flecs_iter_init(world, &it, flecs_iter_cache_all);
it.world = world;
Expand Down Expand Up @@ -3036,7 +3038,8 @@ static
void flecs_modified_id_if(
ecs_world_t *world,
ecs_entity_t entity,
ecs_id_t id)
ecs_id_t id,
bool owned)
{
ecs_check(world != NULL, ECS_INVALID_PARAMETER, NULL);
ecs_check(ecs_is_alive(world, entity), ECS_INVALID_PARAMETER, NULL);
Expand All @@ -3056,7 +3059,7 @@ void flecs_modified_id_if(
}

ecs_type_t ids = { .array = &id, .count = 1 };
flecs_notify_on_set(world, table, ECS_RECORD_TO_ROW(r->row), 1, &ids, true);
flecs_notify_on_set(world, table, ECS_RECORD_TO_ROW(r->row), 1, &ids, owned);

flecs_table_mark_dirty(world, table, id);
flecs_defer_end(world, stage);
Expand Down Expand Up @@ -4609,8 +4612,14 @@ void flecs_cmd_batch_for_entity(
const ecs_type_info_t *ti = ptr.ti;
ecs_iter_action_t on_set;
if ((on_set = ti->hooks.on_set)) {
ecs_defer_begin(world);
flecs_invoke_hook(world, start_table, 1, row, &entity,
ptr.ptr, cmd->id, ptr.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;
}
}
}
Expand Down Expand Up @@ -4648,6 +4657,7 @@ void flecs_cmd_batch_for_entity(
case EcsCmdDisable:
case EcsCmdEvent:
case EcsCmdSkip:
case EcsCmdModifiedNoHook:
break;
}

Expand Down Expand Up @@ -4730,6 +4740,7 @@ void flecs_cmd_batch_for_entity(
case EcsCmdRemove:
case EcsCmdEmplace:
case EcsCmdModified:
case EcsCmdModifiedNoHook:
case EcsCmdAddModified:
case EcsCmdPath:
case EcsCmdDelete:
Expand Down Expand Up @@ -4857,12 +4868,16 @@ bool flecs_defer_end(
world->info.cmd.get_mut_count ++;
break;
case EcsCmdModified:
flecs_modified_id_if(world, e, id);
flecs_modified_id_if(world, e, id, true);
world->info.cmd.modified_count ++;
break;
case EcsCmdModifiedNoHook:
flecs_modified_id_if(world, e, id, false);
world->info.cmd.modified_count ++;
break;
case EcsCmdAddModified:
flecs_add_id(world, e, id);
flecs_modified_id_if(world, e, id);
flecs_modified_id_if(world, e, id, true);
world->info.cmd.add_count ++;
world->info.cmd.modified_count ++;
break;
Expand Down
1 change: 1 addition & 0 deletions src/private_types.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,7 @@ typedef enum ecs_cmd_kind_t {
EcsCmdEmplace,
EcsCmdMut,
EcsCmdModified,
EcsCmdModifiedNoHook,
EcsCmdAddModified,
EcsCmdPath,
EcsCmdDelete,
Expand Down
3 changes: 2 additions & 1 deletion test/api/project.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2588,7 +2588,8 @@
"observer_while_defer_suspended",
"on_add_hook_while_defer_suspended",
"on_set_hook_while_defer_suspended",
"on_remove_hook_while_defer_suspended"
"on_remove_hook_while_defer_suspended",
"on_set_hook_batched_is_deferred"
]
}, {
"id": "SingleThreadStaging",
Expand Down
48 changes: 48 additions & 0 deletions test/api/src/Commands.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3655,3 +3655,51 @@ void Commands_on_remove_hook_while_defer_suspended(void) {

ecs_fini(world);
}

static int on_set_is_deferred_invoked = 0;
static int dummy_observer_invoked = 0;

static
void OnSetIsDeferred(ecs_iter_t *it) {
on_set_is_deferred_invoked ++;
test_int(it->count, 1);
ecs_table_t *t = ecs_get_table(it->world, it->entities[0]);
ecs_remove(it->world, it->entities[0], Position);
test_assert(t == ecs_get_table(it->world, it->entities[0]));
}

static
void DummyObserver(ecs_iter_t *it) {
dummy_observer_invoked ++;
}

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

ECS_COMPONENT_DEFINE(world, Position);
ECS_TAG(world, Tag);

ecs_set_hooks(world, Position, {
.on_set = OnSetIsDeferred
});

ECS_OBSERVER(world, DummyObserver, EcsOnSet, Position);

ecs_entity_t e = ecs_new(world, 0);
ecs_add(world, e, Position);

test_int(on_set_is_deferred_invoked, 0);
test_int(dummy_observer_invoked, 0);

ecs_defer_begin(world);
ecs_add(world, e, Tag);
ecs_modified(world, e, Position);
test_int(on_set_is_deferred_invoked, 0);
test_int(dummy_observer_invoked, 0);
ecs_defer_end(world);

test_int(dummy_observer_invoked, 1);
test_int(on_set_is_deferred_invoked, 1);

ecs_fini(world);
}
7 changes: 6 additions & 1 deletion test/api/src/main.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2498,6 +2498,7 @@ void Commands_observer_while_defer_suspended(void);
void Commands_on_add_hook_while_defer_suspended(void);
void Commands_on_set_hook_while_defer_suspended(void);
void Commands_on_remove_hook_while_defer_suspended(void);
void Commands_on_set_hook_batched_is_deferred(void);

// Testsuite 'SingleThreadStaging'
void SingleThreadStaging_setup(void);
Expand Down Expand Up @@ -12359,6 +12360,10 @@ bake_test_case Commands_testcases[] = {
{
"on_remove_hook_while_defer_suspended",
Commands_on_remove_hook_while_defer_suspended
},
{
"on_set_hook_batched_is_deferred",
Commands_on_set_hook_batched_is_deferred
}
};

Expand Down Expand Up @@ -13269,7 +13274,7 @@ static bake_test_suite suites[] = {
"Commands",
NULL,
NULL,
122,
123,
Commands_testcases
},
{
Expand Down

0 comments on commit f3bc0c9

Please sign in to comment.