From c40b20676cbc715f9b257d5e03edda6caeaa24e2 Mon Sep 17 00:00:00 2001 From: Sander Mertens Date: Thu, 6 Feb 2025 18:22:13 -0800 Subject: [PATCH] Fix shift overflow for events for a large (>64) number of components --- distr/flecs.c | 38 +++++--- src/entity.c | 14 +-- src/observable.c | 22 +++-- src/observable.h | 2 +- test/core/project.json | 5 ++ test/core/src/Commands.c | 185 +++++++++++++++++++++++++++++++++++++++ test/core/src/main.c | 27 +++++- 7 files changed, 270 insertions(+), 23 deletions(-) diff --git a/distr/flecs.c b/distr/flecs.c index 99d9d79c8c..40ec18c245 100644 --- a/distr/flecs.c +++ b/distr/flecs.c @@ -2534,7 +2534,7 @@ void flecs_observer_fini( void flecs_emit( ecs_world_t *world, ecs_world_t *stage, - ecs_flags64_t set_mask, + ecs_flags64_t *set_mask, ecs_event_desc_t *desc); bool flecs_default_next_callback( @@ -5181,7 +5181,7 @@ void flecs_notify_on_add( int32_t count, const ecs_table_diff_t *diff, ecs_flags32_t flags, - ecs_flags64_t set_mask, + ecs_flags64_t *set_mask, bool construct, bool sparse) { @@ -9294,8 +9294,10 @@ void flecs_cmd_batch_for_entity( /* Mask to let observable implementation know which components were set. * This prevents the code from overwriting components with an override if - * the batch also contains an IsA pair. */ - ecs_flags64_t set_mask = 0; + * the batch also contains an IsA pair. + * Use 4 elements, so the implementation supports up to 256 components added + * in a single batch. */ + ecs_flags64_t set_mask[4] = {0}; do { cmd = &cmds[cur]; @@ -9391,7 +9393,9 @@ void flecs_cmd_batch_for_entity( } } - set_mask |= (1llu << i); + int32_t index = (i >= 64) + (i >= 128) + (i >= 192); + int32_t shift = i - 64 * index; + set_mask[index] |= (1llu << shift); world->info.cmd.batched_command_count ++; break; @@ -9463,7 +9467,7 @@ void flecs_cmd_batch_for_entity( * This only happens for entities that didn't have the assigned component * yet, as for entities that did have the component already the value will * have been assigned directly to the component storage. */ - if (set_mask) { + if (set_mask[0] | set_mask[1] | set_mask[2] | set_mask[3]) { cur = start; do { cmd = &cmds[cur]; @@ -13244,7 +13248,7 @@ void flecs_emit_forward( void flecs_emit( ecs_world_t *world, ecs_world_t *stage, - ecs_flags64_t set_mask, + ecs_flags64_t *set_mask, ecs_event_desc_t *desc) { flecs_poly_assert(world, ecs_world_t); @@ -13386,10 +13390,22 @@ void flecs_emit( ecs_table_record_t *base_tr = NULL; ecs_entity_t base = 0; bool id_can_override = can_override; - ecs_flags64_t id_bit = 1llu << i; - if (id_bit & set_mask) { - /* Component is already set, so don't override with prefab value */ - id_can_override = false; + + /* Only added components can trigger overriding */ + if (set_mask && event == EcsOnAdd) { + ecs_assert(i < 256, ECS_UNSUPPORTED, + "cannot add more than 256 components in a single operation"); + + int32_t index = (i >= 64) + (i >= 128) + (i >= 192); + int32_t shift = i - 64 * index; + + printf("index = %d, shift = %d\n", index, shift); + + ecs_flags64_t id_bit = 1llu << shift; + if (id_bit & set_mask[index]) { + /* Component is already set, so don't override with prefab value */ + id_can_override = false; + } } /* Check if this id is a pair of an traversable relationship. If so, we diff --git a/src/entity.c b/src/entity.c index 1256a00682..bd0d4160be 100644 --- a/src/entity.c +++ b/src/entity.c @@ -695,7 +695,7 @@ void flecs_notify_on_add( int32_t count, const ecs_table_diff_t *diff, ecs_flags32_t flags, - ecs_flags64_t set_mask, + ecs_flags64_t *set_mask, bool construct, bool sparse) { @@ -4808,8 +4808,10 @@ void flecs_cmd_batch_for_entity( /* Mask to let observable implementation know which components were set. * This prevents the code from overwriting components with an override if - * the batch also contains an IsA pair. */ - ecs_flags64_t set_mask = 0; + * the batch also contains an IsA pair. + * Use 4 elements, so the implementation supports up to 256 components added + * in a single batch. */ + ecs_flags64_t set_mask[4] = {0}; do { cmd = &cmds[cur]; @@ -4905,7 +4907,9 @@ void flecs_cmd_batch_for_entity( } } - set_mask |= (1llu << i); + int32_t index = (i >= 64) + (i >= 128) + (i >= 192); + int32_t shift = i - 64 * index; + set_mask[index] |= (1llu << shift); world->info.cmd.batched_command_count ++; break; @@ -4977,7 +4981,7 @@ void flecs_cmd_batch_for_entity( * This only happens for entities that didn't have the assigned component * yet, as for entities that did have the component already the value will * have been assigned directly to the component storage. */ - if (set_mask) { + if (set_mask[0] | set_mask[1] | set_mask[2] | set_mask[3]) { cur = start; do { cmd = &cmds[cur]; diff --git a/src/observable.c b/src/observable.c index 7471e2f396..cda57a0435 100644 --- a/src/observable.c +++ b/src/observable.c @@ -1126,7 +1126,7 @@ void flecs_emit_forward( void flecs_emit( ecs_world_t *world, ecs_world_t *stage, - ecs_flags64_t set_mask, + ecs_flags64_t *set_mask, ecs_event_desc_t *desc) { flecs_poly_assert(world, ecs_world_t); @@ -1268,10 +1268,22 @@ void flecs_emit( ecs_table_record_t *base_tr = NULL; ecs_entity_t base = 0; bool id_can_override = can_override; - ecs_flags64_t id_bit = 1llu << i; - if (id_bit & set_mask) { - /* Component is already set, so don't override with prefab value */ - id_can_override = false; + + /* Only added components can trigger overriding */ + if (set_mask && event == EcsOnAdd) { + ecs_assert(i < 256, ECS_UNSUPPORTED, + "cannot add more than 256 components in a single operation"); + + int32_t index = (i >= 64) + (i >= 128) + (i >= 192); + int32_t shift = i - 64 * index; + + printf("index = %d, shift = %d\n", index, shift); + + ecs_flags64_t id_bit = 1llu << shift; + if (id_bit & set_mask[index]) { + /* Component is already set, so don't override with prefab value */ + id_can_override = false; + } } /* Check if this id is a pair of an traversable relationship. If so, we diff --git a/src/observable.h b/src/observable.h index 120553622b..c2d1bccdb9 100644 --- a/src/observable.h +++ b/src/observable.h @@ -90,7 +90,7 @@ void flecs_observer_fini( void flecs_emit( ecs_world_t *world, ecs_world_t *stage, - ecs_flags64_t set_mask, + ecs_flags64_t *set_mask, ecs_event_desc_t *desc); bool flecs_default_next_callback( diff --git a/test/core/project.json b/test/core/project.json index c62123ec99..c93325d672 100644 --- a/test/core/project.json +++ b/test/core/project.json @@ -2214,6 +2214,11 @@ "defer_remove_after_set_w_observer", "defer_override_after_remove", "defer_override_after_remove_3_ops", + "defer_override_after_remove_63_commands", + "defer_override_after_remove_64_commands", + "defer_override_after_remove_65_commands", + "defer_override_after_remove_96_commands", + "defer_override_after_remove_255_commands", "flush_stage_to_deferred_world", "add_in_observer_during_merge", "add_in_observer_during_merge_2_commands", diff --git a/test/core/src/Commands.c b/test/core/src/Commands.c index 9883520901..ffc10409ea 100644 --- a/test/core/src/Commands.c +++ b/test/core/src/Commands.c @@ -2672,6 +2672,191 @@ void Commands_defer_override_after_remove_3_ops(void) { ecs_fini(world); } +void Commands_defer_override_after_remove_63_commands(void) { + ecs_world_t *world = ecs_mini(); + + ecs_entity_t components[63]; + for (int i = 0; i < 63; i ++) { + components[i] = ecs_component_init(world, &(ecs_component_desc_t){ + .type.size = ECS_SIZEOF(Position), + .type.alignment = ECS_ALIGNOF(Position) + }); + } + + ECS_COMPONENT(world, Position); + + ecs_add_pair(world, ecs_id(Position), EcsOnInstantiate, EcsInherit); + + ecs_entity_t base = ecs_insert(world, ecs_value(Position, {10, 20})); + ecs_entity_t inst = ecs_new_w_pair(world, EcsIsA, base); + ecs_set(world, inst, Position, {20, 30}); + + ecs_defer_begin(world); + ecs_remove(world, inst, Position); + for (int i = 0; i < 63; i ++) { + ecs_add_id(world, inst, components[i]); + } + ecs_add(world, inst, Position); + ecs_defer_end(world); + + const Position *p = ecs_get(world, inst, Position); + test_assert(p != NULL); + test_int(p->x, 10); + test_int(p->y, 20); + + test_assert(p != ecs_get(world, base, Position)); + + ecs_fini(world); +} + +void Commands_defer_override_after_remove_64_commands(void) { + ecs_world_t *world = ecs_mini(); + + ecs_entity_t components[64]; + for (int i = 0; i < 64; i ++) { + components[i] = ecs_component_init(world, &(ecs_component_desc_t){ + .type.size = ECS_SIZEOF(Position), + .type.alignment = ECS_ALIGNOF(Position) + }); + } + + ECS_COMPONENT(world, Position); + + ecs_add_pair(world, ecs_id(Position), EcsOnInstantiate, EcsInherit); + + ecs_entity_t base = ecs_insert(world, ecs_value(Position, {10, 20})); + ecs_entity_t inst = ecs_new_w_pair(world, EcsIsA, base); + ecs_set(world, inst, Position, {20, 30}); + + ecs_defer_begin(world); + ecs_remove(world, inst, Position); + for (int i = 0; i < 64; i ++) { + ecs_add_id(world, inst, components[i]); + } + ecs_add(world, inst, Position); + ecs_defer_end(world); + + const Position *p = ecs_get(world, inst, Position); + test_assert(p != NULL); + test_int(p->x, 10); + test_int(p->y, 20); + + test_assert(p != ecs_get(world, base, Position)); + + ecs_fini(world); +} + +void Commands_defer_override_after_remove_65_commands(void) { + ecs_world_t *world = ecs_mini(); + + ecs_entity_t components[65]; + for (int i = 0; i < 65; i ++) { + components[i] = ecs_component_init(world, &(ecs_component_desc_t){ + .type.size = ECS_SIZEOF(Position), + .type.alignment = ECS_ALIGNOF(Position) + }); + } + + ECS_COMPONENT(world, Position); + + ecs_add_pair(world, ecs_id(Position), EcsOnInstantiate, EcsInherit); + + ecs_entity_t base = ecs_insert(world, ecs_value(Position, {10, 20})); + ecs_entity_t inst = ecs_new_w_pair(world, EcsIsA, base); + ecs_set(world, inst, Position, {20, 30}); + + ecs_defer_begin(world); + ecs_remove(world, inst, Position); + for (int i = 0; i < 65; i ++) { + ecs_add_id(world, inst, components[i]); + } + ecs_add(world, inst, Position); + ecs_defer_end(world); + + const Position *p = ecs_get(world, inst, Position); + test_assert(p != NULL); + test_int(p->x, 10); + test_int(p->y, 20); + + test_assert(p != ecs_get(world, base, Position)); + + ecs_fini(world); +} + +void Commands_defer_override_after_remove_96_commands(void) { + ecs_world_t *world = ecs_mini(); + + ecs_entity_t components[96]; + for (int i = 0; i < 96; i ++) { + components[i] = ecs_component_init(world, &(ecs_component_desc_t){ + .type.size = ECS_SIZEOF(Position), + .type.alignment = ECS_ALIGNOF(Position) + }); + } + + ECS_COMPONENT(world, Position); + + ecs_add_pair(world, ecs_id(Position), EcsOnInstantiate, EcsInherit); + + ecs_entity_t base = ecs_insert(world, ecs_value(Position, {10, 20})); + ecs_entity_t inst = ecs_new_w_pair(world, EcsIsA, base); + ecs_set(world, inst, Position, {20, 30}); + + ecs_defer_begin(world); + ecs_remove(world, inst, Position); + for (int i = 0; i < 96; i ++) { + ecs_add_id(world, inst, components[i]); + } + ecs_add(world, inst, Position); + ecs_defer_end(world); + + const Position *p = ecs_get(world, inst, Position); + test_assert(p != NULL); + test_int(p->x, 10); + test_int(p->y, 20); + + test_assert(p != ecs_get(world, base, Position)); + + ecs_fini(world); +} + +void Commands_defer_override_after_remove_255_commands(void) { + ecs_world_t *world = ecs_mini(); + + ecs_entity_t components[255]; + for (int i = 0; i < 255; i ++) { + components[i] = ecs_component_init(world, &(ecs_component_desc_t){ + .type.size = ECS_SIZEOF(Position), + .type.alignment = ECS_ALIGNOF(Position) + }); + } + + ECS_COMPONENT(world, Position); + + ecs_add_pair(world, ecs_id(Position), EcsOnInstantiate, EcsInherit); + + ecs_entity_t base = ecs_insert(world, ecs_value(Position, {10, 20})); + ecs_entity_t inst = ecs_new_w_pair(world, EcsIsA, base); + ecs_set(world, inst, Position, {20, 30}); + + ecs_defer_begin(world); + ecs_remove(world, inst, Position); + for (int i = 0; i < 255; i ++) { + ecs_add_id(world, inst, components[i]); + } + ecs_add(world, inst, Position); + ecs_defer_end(world); + + const Position *p = ecs_get(world, inst, Position); + test_assert(p != NULL); + test_int(p->x, 10); + test_int(p->y, 20); + + test_assert(p != ecs_get(world, base, Position)); + + ecs_fini(world); +} + void Commands_flush_stage_to_deferred_world(void) { ecs_world_t *world = ecs_mini(); diff --git a/test/core/src/main.c b/test/core/src/main.c index 0373c84693..3496287d63 100644 --- a/test/core/src/main.c +++ b/test/core/src/main.c @@ -2133,6 +2133,11 @@ void Commands_defer_remove_after_set(void); void Commands_defer_remove_after_set_w_observer(void); void Commands_defer_override_after_remove(void); void Commands_defer_override_after_remove_3_ops(void); +void Commands_defer_override_after_remove_63_commands(void); +void Commands_defer_override_after_remove_64_commands(void); +void Commands_defer_override_after_remove_65_commands(void); +void Commands_defer_override_after_remove_96_commands(void); +void Commands_defer_override_after_remove_255_commands(void); void Commands_flush_stage_to_deferred_world(void); void Commands_add_in_observer_during_merge(void); void Commands_add_in_observer_during_merge_2_commands(void); @@ -10627,6 +10632,26 @@ bake_test_case Commands_testcases[] = { "defer_override_after_remove_3_ops", Commands_defer_override_after_remove_3_ops }, + { + "defer_override_after_remove_63_commands", + Commands_defer_override_after_remove_63_commands + }, + { + "defer_override_after_remove_64_commands", + Commands_defer_override_after_remove_64_commands + }, + { + "defer_override_after_remove_65_commands", + Commands_defer_override_after_remove_65_commands + }, + { + "defer_override_after_remove_96_commands", + Commands_defer_override_after_remove_96_commands + }, + { + "defer_override_after_remove_255_commands", + Commands_defer_override_after_remove_255_commands + }, { "flush_stage_to_deferred_world", Commands_flush_stage_to_deferred_world @@ -11770,7 +11795,7 @@ static bake_test_suite suites[] = { "Commands", NULL, NULL, - 150, + 155, Commands_testcases }, {