Skip to content

Commit

Permalink
Fix issue where observers could be triggered multiple times for the s…
Browse files Browse the repository at this point in the history
…ame event

Summary:
This diff fixes a Flecs issue where mulit-component observers could get triggered multiple times for the same event. Two separate problems contributed to this issue:

1. Observers copied the wrong event id used to dedup events. Instead of using the event id passed by `flecs_emit`, observers used the global event id counter. In most cases these are the same, except when nested `flecs_emit` calls happen (which is rare).

2. The event id was copied at the wrong time. When a multi-component observer gets triggered, it still needs to verify if the event matches its query. Observers copied the event id before checking if the query matched, which could cause events to get skipped or delivered multiple times.

Differential Revision: D67219403
  • Loading branch information
SanderMertens authored and facebook-github-bot committed Dec 13, 2024
1 parent 188e29e commit ea58b5b
Show file tree
Hide file tree
Showing 5 changed files with 112 additions and 5 deletions.
4 changes: 2 additions & 2 deletions distr/flecs.c
Original file line number Diff line number Diff line change
Expand Up @@ -14650,8 +14650,6 @@ void flecs_multi_observer_invoke(
return;
}

impl->last_event_id[0] = world->event_id;

ecs_table_t *table = it->table;
ecs_table_t *prev_table = it->other_table;
int8_t pivot_term = it->term_index;
Expand Down Expand Up @@ -14704,6 +14702,8 @@ void flecs_multi_observer_invoke(
}
}

impl->last_event_id[0] = it->event_cur;

/* Patch data from original iterator. If the observer query has
* wildcards which triggered the original event, the component id that
* got matched by ecs_query_has_range may not be the same as the one
Expand Down
4 changes: 2 additions & 2 deletions src/observer.c
Original file line number Diff line number Diff line change
Expand Up @@ -459,8 +459,6 @@ void flecs_multi_observer_invoke(
return;
}

impl->last_event_id[0] = world->event_id;

ecs_table_t *table = it->table;
ecs_table_t *prev_table = it->other_table;
int8_t pivot_term = it->term_index;
Expand Down Expand Up @@ -513,6 +511,8 @@ void flecs_multi_observer_invoke(
}
}

impl->last_event_id[0] = it->event_cur;

/* Patch data from original iterator. If the observer query has
* wildcards which triggered the original event, the component id that
* got matched by ecs_query_has_range may not be the same as the one
Expand Down
2 changes: 2 additions & 0 deletions test/core/project.json
Original file line number Diff line number Diff line change
Expand Up @@ -1690,6 +1690,8 @@
"on_remove_multi_optional",
"on_add_multi_only_optional",
"on_remove_multi_only_optional",
"on_add_multi_observers_w_prefab_instance",
"on_add_overlapping_multi_observers_w_prefab_instance",
"cache_test_1",
"cache_test_2",
"cache_test_3",
Expand Down
95 changes: 95 additions & 0 deletions test/core/src/Observer.c
Original file line number Diff line number Diff line change
Expand Up @@ -9612,3 +9612,98 @@ void Observer_on_remove_multi_only_optional(void) {

ecs_fini(world);
}

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

ECS_COMPONENT(world, Position);
ECS_COMPONENT(world, Velocity);
ECS_COMPONENT(world, Mass);

Probe ctx = {0};

ecs_entity_t prefab = ecs_new_w_id(world, EcsPrefab);
ecs_add(world, prefab, Position);

ecs_observer(world, {
.query.terms = {{ ecs_id(Mass) }, { ecs_id(Velocity) }},
.events = { EcsOnAdd },
.callback = Observer
});


ecs_entity_t o = ecs_observer(world, {
.query.terms = {{ ecs_id(Position) }, { ecs_isa(prefab) }},
.events = { EcsOnAdd },
.callback = Observer,
.ctx = &ctx
});

ecs_entity_t child = ecs_new_w_pair(world, EcsChildOf, prefab);
ecs_add(world, child, Velocity);
ecs_add(world, child, Mass);
ecs_set_name(world, child, "child");
test_int(ctx.invoked, 0);

ecs_entity_t inst = ecs_new_w_pair(world, EcsIsA, prefab);
test_int(ctx.invoked, 1);
test_int(ctx.count, 1);
test_int(ctx.system, o);
test_int(ctx.event, EcsOnAdd);
test_uint(ctx.s[0][0], 0);

test_assert(ecs_has(world, inst, Position));
ecs_entity_t inst_child = ecs_lookup_from(world, inst, "child");
test_assert(inst_child != 0);
test_assert(ecs_has(world, inst_child, Velocity));
test_assert(ecs_has(world, inst_child, Mass));

ecs_fini(world);
}

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

ECS_COMPONENT(world, Position);
ECS_COMPONENT(world, Velocity);

Probe ctx = {0};

ecs_entity_t prefab = ecs_new_w_id(world, EcsPrefab);
ecs_add(world, prefab, Position);

ecs_observer(world, {
.query.terms = {{ ecs_id(Position) }, { ecs_id(Velocity) }},
.events = { EcsOnAdd },
.callback = Observer
});


ecs_entity_t o = ecs_observer(world, {
.query.terms = {{ ecs_id(Position) }, { ecs_isa(prefab) }},
.events = { EcsOnAdd },
.callback = Observer,
.ctx = &ctx
});

ecs_entity_t child = ecs_new_w_pair(world, EcsChildOf, prefab);
ecs_add(world, child, Velocity);
ecs_add(world, child, Position);
ecs_set_name(world, child, "child");
test_int(ctx.invoked, 0);

ecs_entity_t inst = ecs_new_w_pair(world, EcsIsA, prefab);
test_int(ctx.invoked, 1);
test_int(ctx.count, 1);
test_int(ctx.system, o);
test_int(ctx.event, EcsOnAdd);
test_uint(ctx.s[0][0], 0);

test_assert(ecs_has(world, inst, Position));
ecs_entity_t inst_child = ecs_lookup_from(world, inst, "child");
test_assert(inst_child != 0);
test_assert(ecs_has(world, inst_child, Velocity));
test_assert(ecs_has(world, inst_child, Position));

ecs_fini(world);
}
12 changes: 11 additions & 1 deletion test/core/src/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -1629,6 +1629,8 @@ void Observer_on_add_multi_optional(void);
void Observer_on_remove_multi_optional(void);
void Observer_on_add_multi_only_optional(void);
void Observer_on_remove_multi_only_optional(void);
void Observer_on_add_multi_observers_w_prefab_instance(void);
void Observer_on_add_overlapping_multi_observers_w_prefab_instance(void);
void Observer_cache_test_1(void);
void Observer_cache_test_2(void);
void Observer_cache_test_3(void);
Expand Down Expand Up @@ -8643,6 +8645,14 @@ bake_test_case Observer_testcases[] = {
"on_remove_multi_only_optional",
Observer_on_remove_multi_only_optional
},
{
"on_add_multi_observers_w_prefab_instance",
Observer_on_add_multi_observers_w_prefab_instance
},
{
"on_add_overlapping_multi_observers_w_prefab_instance",
Observer_on_add_overlapping_multi_observers_w_prefab_instance
},
{
"cache_test_1",
Observer_cache_test_1
Expand Down Expand Up @@ -11530,7 +11540,7 @@ static bake_test_suite suites[] = {
"Observer",
NULL,
NULL,
229,
231,
Observer_testcases
},
{
Expand Down

0 comments on commit ea58b5b

Please sign in to comment.