Skip to content

Fix shift overflow for events for a large (>64) number of components #1556

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
SanderMertens merged 1 commit into from
Feb 7, 2025
Merged

Fix shift overflow for events for a large (>64) number of components #1556

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 27 additions & 11 deletions distr/flecs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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];
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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];
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down
14 changes: 9 additions & 5 deletions src/entity.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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];
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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];
Expand Down
22 changes: 17 additions & 5 deletions src/observable.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/observable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
5 changes: 5 additions & 0 deletions test/core/project.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
185 changes: 185 additions & 0 deletions test/core/src/Commands.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
Loading