Improve checks for overlapping entity ids

Differential Revision: D63159488 Pull Request resolved: #1372
SanderMertens · Sep 24, 2024 · 343ac35 · 343ac35
1 parent b1bf3a5
commit 343ac35
Show file tree

Hide file tree

Showing 6 changed files with 101 additions and 9 deletions.
diff --git a/distr/flecs.c b/distr/flecs.c
@@ -19145,8 +19145,6 @@ void ecs_set_entity_range(
 {
     flecs_poly_assert(world, ecs_world_t);
     ecs_check(!id_end || id_end > id_start, ECS_INVALID_PARAMETER, NULL);
-    ecs_check(!id_end || id_end > flecs_entities_max_id(world),
-        ECS_INVALID_PARAMETER, NULL);
 
     if (id_start == 0) {
       id_start = flecs_entities_max_id(world) + 1;
@@ -19156,8 +19154,6 @@ void ecs_set_entity_range(
     uint32_t end = (uint32_t)id_end;
 
     flecs_entities_max_id(world) = start - 1;
-    ecs_check(!id_end || id_end > flecs_entities_max_id(world),
-        ECS_INVALID_PARAMETER, NULL);
 
     world->info.min_id = start;
     world->info.max_id = end;
@@ -35241,6 +35237,11 @@ uint64_t flecs_entity_index_new_id(
 
     /* Create new id */
     uint32_t id = (uint32_t)++ index->max_id;
+
+    /* Make sure id hasn't been issued before */
+    ecs_assert(!flecs_entity_index_exists(index, id), ECS_INVALID_OPERATION, 
+        "new entity %u id already in use (likely due to overlapping ranges)", (uint32_t)id);
+
     ecs_vec_append_t(index->allocator, &index->dense, uint64_t)[0] = id;
 
     ecs_entity_index_page_t *page = flecs_entity_index_ensure_page(index, id);
@@ -35272,6 +35273,11 @@ uint64_t* flecs_entity_index_new_ids(
     int32_t i, to_add = new_count - dense_count;
     for (i = 0; i < to_add; i ++) {
         uint32_t id = (uint32_t)++ index->max_id;
+
+        /* Make sure id hasn't been issued before */
+        ecs_assert(!flecs_entity_index_exists(index, id), ECS_INVALID_OPERATION, 
+            "new entity %u id already in use (likely due to overlapping ranges)", (uint32_t)id);
+
         int32_t dense = dense_count + i;
         ecs_vec_get_t(&index->dense, uint64_t, dense)[0] = id;
         ecs_entity_index_page_t *page = flecs_entity_index_ensure_page(index, id);

diff --git a/src/storage/entity_index.c b/src/storage/entity_index.c
@@ -249,6 +249,11 @@ uint64_t flecs_entity_index_new_id(
 
     /* Create new id */
     uint32_t id = (uint32_t)++ index->max_id;
+
+    /* Make sure id hasn't been issued before */
+    ecs_assert(!flecs_entity_index_exists(index, id), ECS_INVALID_OPERATION, 
+        "new entity %u id already in use (likely due to overlapping ranges)", (uint32_t)id);
+
     ecs_vec_append_t(index->allocator, &index->dense, uint64_t)[0] = id;
 
     ecs_entity_index_page_t *page = flecs_entity_index_ensure_page(index, id);
@@ -280,6 +285,11 @@ uint64_t* flecs_entity_index_new_ids(
     int32_t i, to_add = new_count - dense_count;
     for (i = 0; i < to_add; i ++) {
         uint32_t id = (uint32_t)++ index->max_id;
+
+        /* Make sure id hasn't been issued before */
+        ecs_assert(!flecs_entity_index_exists(index, id), ECS_INVALID_OPERATION, 
+            "new entity %u id already in use (likely due to overlapping ranges)", (uint32_t)id);
+
         int32_t dense = dense_count + i;
         ecs_vec_get_t(&index->dense, uint64_t, dense)[0] = id;
         ecs_entity_index_page_t *page = flecs_entity_index_ensure_page(index, id);

diff --git a/src/world.c b/src/world.c
@@ -1602,8 +1602,6 @@ void ecs_set_entity_range(
 {
     flecs_poly_assert(world, ecs_world_t);
     ecs_check(!id_end || id_end > id_start, ECS_INVALID_PARAMETER, NULL);
-    ecs_check(!id_end || id_end > flecs_entities_max_id(world),
-        ECS_INVALID_PARAMETER, NULL);
 
     if (id_start == 0) {
       id_start = flecs_entities_max_id(world) + 1;
@@ -1613,8 +1611,6 @@ void ecs_set_entity_range(
     uint32_t end = (uint32_t)id_end;
 
     flecs_entities_max_id(world) = start - 1;
-    ecs_check(!id_end || id_end > flecs_entities_max_id(world),
-        ECS_INVALID_PARAMETER, NULL);
 
     world->info.min_id = start;
     world->info.max_id = end;

diff --git a/test/core/project.json b/test/core/project.json
@@ -1942,6 +1942,9 @@
                 "entity_range_check_after_delete",
                 "entity_range_offset_0",
                 "entity_range_set_limit_to_lower",
+                "entity_range_set_limit_to_lower_than_offset",
+                "entity_range_overlapping_new_id",
+                "entity_range_overlapping_new_bulk_id",
                 "dim",
                 "phases",
                 "phases_w_merging",

diff --git a/test/core/src/World.c b/test/core/src/World.c
@@ -292,6 +292,68 @@ void World_entity_range_set_limit_to_lower(void) {
     ecs_fini(world);
 }
 
+void World_entity_range_set_limit_to_lower_than_offset(void) {
+    ecs_world_t *world = ecs_mini();
+
+    const ecs_world_info_t *info = ecs_get_world_info(world);
+    test_assert(info != NULL);
+
+    ecs_set_entity_range(world, 2000, 3000);
+
+    test_uint(info->max_id, 3000);
+
+    ecs_set_entity_range(world, 0, 1000);
+
+    test_uint(info->max_id, 1000);
+
+    ecs_fini(world);
+}
+
+void World_entity_range_overlapping_new_id(void) {
+    install_test_abort();
+
+    ecs_world_t *world = ecs_mini();
+
+    const ecs_world_info_t *info = ecs_get_world_info(world);
+    test_assert(info != NULL);
+
+    ecs_set_entity_range(world, 2000, 3000);
+    test_uint(info->max_id, 3000);
+
+    ecs_entity_t e1 = ecs_new(world);
+    test_assert(e1 == 2000);
+
+    ecs_set_entity_range(world, 1999, 0);
+
+    ecs_entity_t e2 = ecs_new(world);
+    test_assert(e2 == 1999);
+
+    test_expect_abort();
+    ecs_new(world);
+}
+
+void World_entity_range_overlapping_new_bulk_id(void) {
+    install_test_abort();
+
+    ecs_world_t *world = ecs_mini();
+
+    ECS_COMPONENT(world, Position);
+
+    const ecs_world_info_t *info = ecs_get_world_info(world);
+    test_assert(info != NULL);
+
+    ecs_set_entity_range(world, 2000, 3000);
+    test_uint(info->max_id, 3000);
+
+    ecs_entity_t e1 = ecs_new(world);
+    test_assert(e1 == 2000);
+
+    ecs_set_entity_range(world, 1999, 0);
+
+    test_expect_abort();
+    ecs_bulk_new(world, Position, 2);
+}
+
 void World_get_tick(void) {
     ecs_world_t *world = ecs_init();
 

diff --git a/test/core/src/main.c b/test/core/src/main.c
@@ -1867,6 +1867,9 @@ void World_entity_range_out_of_range_check_disabled(void);
 void World_entity_range_check_after_delete(void);
 void World_entity_range_offset_0(void);
 void World_entity_range_set_limit_to_lower(void);
+void World_entity_range_set_limit_to_lower_than_offset(void);
+void World_entity_range_overlapping_new_id(void);
+void World_entity_range_overlapping_new_bulk_id(void);
 void World_dim(void);
 void World_phases(void);
 void World_phases_w_merging(void);
@@ -9471,6 +9474,18 @@ bake_test_case World_testcases[] = {
         "entity_range_set_limit_to_lower",
         World_entity_range_set_limit_to_lower
     },
+    {
+        "entity_range_set_limit_to_lower_than_offset",
+        World_entity_range_set_limit_to_lower_than_offset
+    },
+    {
+        "entity_range_overlapping_new_id",
+        World_entity_range_overlapping_new_id
+    },
+    {
+        "entity_range_overlapping_new_bulk_id",
+        World_entity_range_overlapping_new_bulk_id
+    },
     {
         "dim",
         World_dim
@@ -11194,7 +11209,7 @@ static bake_test_suite suites[] = {
         "World",
         World_setup,
         NULL,
-        62,
+        65,
         World_testcases
     },
     {