From ea58b5b77b8586115c667a9ab75c08df681f9e1c Mon Sep 17 00:00:00 2001 From: Sander Mertens Date: Fri, 13 Dec 2024 13:53:22 -0800 Subject: [PATCH] Fix issue where observers could be triggered multiple times for the same 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 --- distr/flecs.c | 4 +- src/observer.c | 4 +- test/core/project.json | 2 + test/core/src/Observer.c | 95 ++++++++++++++++++++++++++++++++++++++++ test/core/src/main.c | 12 ++++- 5 files changed, 112 insertions(+), 5 deletions(-) diff --git a/distr/flecs.c b/distr/flecs.c index bf47e83f5..e3887454d 100644 --- a/distr/flecs.c +++ b/distr/flecs.c @@ -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; @@ -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 diff --git a/src/observer.c b/src/observer.c index 4eb7433e7..5f071dea4 100644 --- a/src/observer.c +++ b/src/observer.c @@ -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; @@ -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 diff --git a/test/core/project.json b/test/core/project.json index 3b2436d1e..7bd3bb55d 100644 --- a/test/core/project.json +++ b/test/core/project.json @@ -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", diff --git a/test/core/src/Observer.c b/test/core/src/Observer.c index ba78d3f98..cf6c547de 100644 --- a/test/core/src/Observer.c +++ b/test/core/src/Observer.c @@ -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); +} diff --git a/test/core/src/main.c b/test/core/src/main.c index 956cb0439..edcd9d77d 100644 --- a/test/core/src/main.c +++ b/test/core/src/main.c @@ -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); @@ -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 @@ -11530,7 +11540,7 @@ static bake_test_suite suites[] = { "Observer", NULL, NULL, - 229, + 231, Observer_testcases }, {