From 0b4858726c75c80d7aee66c8cd44d7970be3867d Mon Sep 17 00:00:00 2001 From: Eagster <79881080+ElliottjPierce@users.noreply.github.com> Date: Wed, 7 May 2025 14:20:30 -0400 Subject: [PATCH] Make `entity::index` non max (#18704) # Objective There are two problems this aims to solve. First, `Entity::index` is currently a `u32`. That means there are `u32::MAX + 1` possible entities. Not only is that awkward, but it also make `Entity` allocation more difficult. I discovered this while working on remote entity reservation, but even on main, `Entities` doesn't handle the `u32::MAX + 1` entity very well. It can not be batch reserved because that iterator uses exclusive ranges, which has a maximum upper bound of `u32::MAX - 1`. In other words, having `u32::MAX` as a valid index can be thought of as a bug right now. We either need to make that invalid (this PR), which makes Entity allocation cleaner and makes remote reservation easier (because the length only needs to be u32 instead of u64, which, in atomics is a big deal), or we need to take another pass at `Entities` to make it handle the `u32::MAX` index properly. Second, `TableRow`, `ArchetypeRow` and `EntityIndex` (a type alias for u32) all have `u32` as the underlying type. That means using these as the index type in a `SparseSet` uses 64 bits for the sparse list because it stores `Option`. By using `NonMaxU32` here, we cut the memory of that list in half. To my knowledge, `EntityIndex` is the only thing that would really benefit from this niche. `TableRow` and `ArchetypeRow` I think are not stored in an `Option` in bulk. But if they ever are, this would help. Additionally this ensures `TableRow::INVALID` and `ArchetypeRow::INVALID` never conflict with an actual row, which in a nice bonus. As a related note, if we do components as entities where `ComponentId` becomes `Entity`, the the `SparseSet` will see a similar memory improvement too. ## Solution Create a new type `EntityRow` that wraps `NonMaxU32`, similar to `TableRow` and `ArchetypeRow`. Change `Entity::index` to this type. ## Downsides `NonMax` is implemented as a `NonZero` with a binary inversion. That means accessing and storing the value takes one more instruction. I don't think that's a big deal, but it's worth mentioning. As a consequence, `to_bits` uses `transmute` to skip the inversion which keeps it a nop. But that also means that ordering has now flipped. In other words, higher indices are considered less than lower indices. I don't think that's a problem, but it's also worth mentioning. ## Alternatives We could keep the index as a u32 type and just document that `u32::MAX` is invalid, modifying `Entities` to ensure it never gets handed out. (But that's not enforced by the type system.) We could still take advantage of the niche here in `ComponentSparseSet`. We'd just need some unsafe manual conversions, which is probably fine, but opens up the possibility for correctness problems later. We could change `Entities` to fully support the `u32::MAX` index. (But that makes `Entities` more complex and potentially slightly slower.) ## Testing - CI - A few tests were changed because they depend on different ordering and `to_bits` values. ## Future Work - It might be worth removing the niche on `Entity::generation` since there is now a different niche. - We could move `Entity::generation` into it's own type too for clarity. - We should change `ComponentSparseSet` to take advantage of the new niche. (This PR doesn't change that yet.) - Consider removing or updating `Identifier`. This is only used for `Entity`, so it might be worth combining since `Entity` is now more unique. --------- Co-authored-by: atlv Co-authored-by: Zachary Harrold --- assets/scenes/load_scene_example.scn.ron | 6 +- benches/Cargo.toml | 1 + benches/benches/bevy_ecs/world/entity_hash.rs | 5 +- benches/benches/bevy_ecs/world/world_get.rs | 32 +- crates/bevy_ecs/src/entity/map_entities.rs | 14 +- crates/bevy_ecs/src/entity/mod.rs | 385 ++++++++++++++---- crates/bevy_ecs/src/lib.rs | 22 +- crates/bevy_ecs/src/observer/mod.rs | 2 +- crates/bevy_ecs/src/query/state.rs | 14 +- .../relationship_source_collection.rs | 4 +- crates/bevy_ecs/src/storage/sparse_set.rs | 13 +- crates/bevy_ecs/src/storage/table/mod.rs | 7 +- crates/bevy_ecs/src/system/query.rs | 12 +- crates/bevy_remote/src/builtin_methods.rs | 5 +- .../bevy_scene/src/dynamic_scene_builder.rs | 12 +- crates/bevy_scene/src/scene_spawner.rs | 2 +- crates/bevy_scene/src/serde.rs | 56 +-- crates/bevy_transform/src/systems.rs | 6 +- crates/bevy_ui/src/layout/mod.rs | 2 +- crates/bevy_ui/src/layout/ui_surface.rs | 14 +- examples/ecs/error_handling.rs | 2 +- .../make_entity_index_non_max.md | 25 ++ 22 files changed, 447 insertions(+), 194 deletions(-) create mode 100644 release-content/migration-guides/make_entity_index_non_max.md diff --git a/assets/scenes/load_scene_example.scn.ron b/assets/scenes/load_scene_example.scn.ron index e768a7b149..477bee3054 100644 --- a/assets/scenes/load_scene_example.scn.ron +++ b/assets/scenes/load_scene_example.scn.ron @@ -5,7 +5,7 @@ ), }, entities: { - 4294967296: ( + 4294967297: ( components: { "bevy_ecs::name::Name": "joe", "bevy_transform::components::global_transform::GlobalTransform": ((1.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0)), @@ -23,7 +23,7 @@ ), }, ), - 4294967297: ( + 4294967298: ( components: { "scene::ComponentA": ( x: 3.0, @@ -32,4 +32,4 @@ }, ), }, -) \ No newline at end of file +) diff --git a/benches/Cargo.toml b/benches/Cargo.toml index 3f547d80d9..a299a6526c 100644 --- a/benches/Cargo.toml +++ b/benches/Cargo.toml @@ -32,6 +32,7 @@ bevy_platform = { path = "../crates/bevy_platform", default-features = false, fe glam = "0.29" rand = "0.8" rand_chacha = "0.3" +nonmax = { version = "0.5", default-features = false } # Make `bevy_render` compile on Linux with x11 windowing. x11 vs. Wayland does not matter here # because the benches do not actually open any windows. diff --git a/benches/benches/bevy_ecs/world/entity_hash.rs b/benches/benches/bevy_ecs/world/entity_hash.rs index 7e8dfb4a21..3562510c90 100644 --- a/benches/benches/bevy_ecs/world/entity_hash.rs +++ b/benches/benches/bevy_ecs/world/entity_hash.rs @@ -17,9 +17,10 @@ fn make_entity(rng: &mut impl Rng, size: usize) -> Entity { let generation = 1.0 + -(1.0 - x).log2() * 2.0; // this is not reliable, but we're internal so a hack is ok - let bits = ((generation as u64) << 32) | (id as u64); + let id = id as u64 + 1; + let bits = ((generation as u64) << 32) | id; let e = Entity::from_bits(bits); - assert_eq!(e.index(), id as u32); + assert_eq!(e.index(), !(id as u32)); assert_eq!(e.generation(), generation as u32); e } diff --git a/benches/benches/bevy_ecs/world/world_get.rs b/benches/benches/bevy_ecs/world/world_get.rs index 283b984186..e6e2a0bb90 100644 --- a/benches/benches/bevy_ecs/world/world_get.rs +++ b/benches/benches/bevy_ecs/world/world_get.rs @@ -1,9 +1,10 @@ use core::hint::black_box; +use nonmax::NonMaxU32; use bevy_ecs::{ bundle::{Bundle, NoBundleEffect}, component::Component, - entity::Entity, + entity::{Entity, EntityRow}, system::{Query, SystemState}, world::World, }; @@ -53,7 +54,9 @@ pub fn world_entity(criterion: &mut Criterion) { bencher.iter(|| { for i in 0..entity_count { - let entity = Entity::from_raw(i); + let entity = + // SAFETY: Range is exclusive. + Entity::from_raw(EntityRow::new(unsafe { NonMaxU32::new_unchecked(i) })); black_box(world.entity(entity)); } }); @@ -74,7 +77,9 @@ pub fn world_get(criterion: &mut Criterion) { bencher.iter(|| { for i in 0..entity_count { - let entity = Entity::from_raw(i); + let entity = + // SAFETY: Range is exclusive. + Entity::from_raw(EntityRow::new(unsafe { NonMaxU32::new_unchecked(i) })); assert!(world.get::(entity).is_some()); } }); @@ -84,7 +89,9 @@ pub fn world_get(criterion: &mut Criterion) { bencher.iter(|| { for i in 0..entity_count { - let entity = Entity::from_raw(i); + let entity = + // SAFETY: Range is exclusive. + Entity::from_raw(EntityRow::new(unsafe { NonMaxU32::new_unchecked(i) })); assert!(world.get::(entity).is_some()); } }); @@ -106,7 +113,9 @@ pub fn world_query_get(criterion: &mut Criterion) { bencher.iter(|| { for i in 0..entity_count { - let entity = Entity::from_raw(i); + let entity = + // SAFETY: Range is exclusive. + Entity::from_raw(EntityRow::new(unsafe { NonMaxU32::new_unchecked(i) })); assert!(query.get(&world, entity).is_ok()); } }); @@ -131,7 +140,9 @@ pub fn world_query_get(criterion: &mut Criterion) { bencher.iter(|| { for i in 0..entity_count { - let entity = Entity::from_raw(i); + let entity = + // SAFETY: Range is exclusive. + Entity::from_raw(EntityRow::new(unsafe { NonMaxU32::new_unchecked(i) })); assert!(query.get(&world, entity).is_ok()); } }); @@ -142,7 +153,9 @@ pub fn world_query_get(criterion: &mut Criterion) { bencher.iter(|| { for i in 0..entity_count { - let entity = Entity::from_raw(i); + let entity = + // SAFETY: Range is exclusive. + Entity::from_raw(EntityRow::new(unsafe { NonMaxU32::new_unchecked(i) })); assert!(query.get(&world, entity).is_ok()); } }); @@ -169,7 +182,10 @@ pub fn world_query_get(criterion: &mut Criterion) { bencher.iter(|| { for i in 0..entity_count { - let entity = Entity::from_raw(i); + // SAFETY: Range is exclusive. + let entity = Entity::from_raw(EntityRow::new(unsafe { + NonMaxU32::new_unchecked(i) + })); assert!(query.get(&world, entity).is_ok()); } }); diff --git a/crates/bevy_ecs/src/entity/map_entities.rs b/crates/bevy_ecs/src/entity/map_entities.rs index c75817fbc8..6e2c11160c 100644 --- a/crates/bevy_ecs/src/entity/map_entities.rs +++ b/crates/bevy_ecs/src/entity/map_entities.rs @@ -171,7 +171,7 @@ impl> MapEntities for SmallVec { /// fn get_mapped(&mut self, entity: Entity) -> Entity { /// self.map.get(&entity).copied().unwrap_or(entity) /// } -/// +/// /// fn set_mapped(&mut self, source: Entity, target: Entity) { /// self.map.insert(source, target); /// } @@ -228,7 +228,7 @@ impl EntityMapper for SceneEntityMapper<'_> { // this new entity reference is specifically designed to never represent any living entity let new = Entity::from_raw_and_generation( - self.dead_start.index(), + self.dead_start.row(), IdentifierMask::inc_masked_high_by(self.dead_start.generation, self.generations), ); @@ -331,6 +331,7 @@ impl<'m> SceneEntityMapper<'m> { #[cfg(test)] mod tests { + use crate::{ entity::{Entity, EntityHashMap, EntityMapper, SceneEntityMapper}, world::World, @@ -338,14 +339,11 @@ mod tests { #[test] fn entity_mapper() { - const FIRST_IDX: u32 = 1; - const SECOND_IDX: u32 = 2; - let mut map = EntityHashMap::default(); let mut world = World::new(); let mut mapper = SceneEntityMapper::new(&mut map, &mut world); - let mapped_ent = Entity::from_raw(FIRST_IDX); + let mapped_ent = Entity::from_raw_u32(1).unwrap(); let dead_ref = mapper.get_mapped(mapped_ent); assert_eq!( @@ -354,7 +352,7 @@ mod tests { "should persist the allocated mapping from the previous line" ); assert_eq!( - mapper.get_mapped(Entity::from_raw(SECOND_IDX)).index(), + mapper.get_mapped(Entity::from_raw_u32(2).unwrap()).index(), dead_ref.index(), "should re-use the same index for further dead refs" ); @@ -372,7 +370,7 @@ mod tests { let mut world = World::new(); let dead_ref = SceneEntityMapper::world_scope(&mut map, &mut world, |_, mapper| { - mapper.get_mapped(Entity::from_raw(0)) + mapper.get_mapped(Entity::from_raw_u32(0).unwrap()) }); // Next allocated entity should be a further generation on the same index diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index 3126c9d198..02311edf42 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -45,6 +45,7 @@ use bevy_reflect::Reflect; use bevy_reflect::{ReflectDeserialize, ReflectSerialize}; pub use clone_entities::*; +use derive_more::derive::Display; pub use entity_set::*; pub use map_entities::*; @@ -67,6 +68,7 @@ pub mod unique_array; pub mod unique_slice; pub mod unique_vec; +use nonmax::NonMaxU32; pub use unique_array::{UniqueEntityArray, UniqueEntityEquivalentArray}; pub use unique_slice::{UniqueEntityEquivalentSlice, UniqueEntitySlice}; pub use unique_vec::{UniqueEntityEquivalentVec, UniqueEntityVec}; @@ -103,6 +105,86 @@ use bevy_platform::sync::atomic::AtomicIsize as AtomicIdCursor; #[cfg(not(target_has_atomic = "64"))] type IdCursor = isize; +/// This represents the row or "index" of an [`Entity`] within the [`Entities`] table. +/// This is a lighter weight version of [`Entity`]. +/// +/// This is a unique identifier for an entity in the world. +/// This differs from [`Entity`] in that [`Entity`] is unique for all entities total (unless the [`Entity::generation`] wraps), +/// but this is only unique for entities that are active. +/// +/// This can be used over [`Entity`] to improve performance in some cases, +/// but improper use can cause this to identify a different entity than intended. +/// Use with caution. +#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Debug, Display)] +#[cfg_attr(feature = "bevy_reflect", derive(Reflect))] +#[cfg_attr(feature = "bevy_reflect", reflect(opaque))] +#[cfg_attr(feature = "bevy_reflect", reflect(Hash, PartialEq, Debug, Clone))] +#[repr(transparent)] +pub struct EntityRow(NonMaxU32); + +impl EntityRow { + const PLACEHOLDER: Self = Self(NonMaxU32::MAX); + + /// Constructs a new [`EntityRow`] from its index. + pub const fn new(index: NonMaxU32) -> Self { + Self(index) + } + + /// Gets the index of the entity. + #[inline(always)] + pub const fn index(self) -> u32 { + self.0.get() + } + + /// Gets a some bits that represent this value. + /// The bits are opaque and should not be regarded as meaningful. + #[inline(always)] + const fn to_bits(self) -> u32 { + // SAFETY: NonMax is repr transparent. + unsafe { mem::transmute::(self.0) } + } + + /// Reconstruct an [`EntityRow`] previously destructured with [`EntityRow::to_bits`]. + /// + /// Only useful when applied to results from `to_bits` in the same instance of an application. + /// + /// # Panics + /// + /// This method will likely panic if given `u32` values that did not come from [`EntityRow::to_bits`]. + #[inline] + const fn from_bits(bits: u32) -> Self { + Self::try_from_bits(bits).expect("Attempted to initialize invalid bits as an entity row") + } + + /// Reconstruct an [`EntityRow`] previously destructured with [`EntityRow::to_bits`]. + /// + /// Only useful when applied to results from `to_bits` in the same instance of an application. + /// + /// This method is the fallible counterpart to [`EntityRow::from_bits`]. + #[inline(always)] + const fn try_from_bits(bits: u32) -> Option { + match NonZero::::new(bits) { + // SAFETY: NonMax and NonZero are repr transparent. + Some(underlying) => Some(Self(unsafe { + mem::transmute::, NonMaxU32>(underlying) + })), + None => None, + } + } +} + +impl SparseSetIndex for EntityRow { + #[inline] + fn sparse_set_index(&self) -> usize { + self.index() as usize + } + + #[inline] + fn get_sparse_set_index(value: usize) -> Self { + Self::from_bits(value as u32) + } +} + /// Lightweight identifier of an [entity](crate::entity). /// /// The identifier is implemented using a [generational index]: a combination of an index and a generation. @@ -186,10 +268,10 @@ pub struct Entity { // Do not reorder the fields here. The ordering is explicitly used by repr(C) // to make this struct equivalent to a u64. #[cfg(target_endian = "little")] - index: u32, + row: EntityRow, generation: NonZero, #[cfg(target_endian = "big")] - index: u32, + row: EntityRow, } // By not short-circuiting in comparisons, we get better codegen. @@ -244,13 +326,16 @@ impl Hash for Entity { } impl Entity { - /// Construct an [`Entity`] from a raw `index` value and a non-zero `generation` value. + /// Construct an [`Entity`] from a raw `row` value and a non-zero `generation` value. /// Ensure that the generation value is never greater than `0x7FFF_FFFF`. #[inline(always)] - pub(crate) const fn from_raw_and_generation(index: u32, generation: NonZero) -> Entity { + pub(crate) const fn from_raw_and_generation( + row: EntityRow, + generation: NonZero, + ) -> Entity { debug_assert!(generation.get() <= HIGH_MASK); - Self { index, generation } + Self { row, generation } } /// An entity ID with a placeholder value. This may or may not correspond to an actual entity, @@ -287,9 +372,9 @@ impl Entity { /// } /// } /// ``` - pub const PLACEHOLDER: Self = Self::from_raw(u32::MAX); + pub const PLACEHOLDER: Self = Self::from_raw(EntityRow::PLACEHOLDER); - /// Creates a new entity ID with the specified `index` and a generation of 1. + /// Creates a new entity ID with the specified `row` and a generation of 1. /// /// # Note /// @@ -302,8 +387,19 @@ impl Entity { /// `Entity` lines up between instances, but instead insert a secondary identifier as /// a component. #[inline(always)] - pub const fn from_raw(index: u32) -> Entity { - Self::from_raw_and_generation(index, NonZero::::MIN) + pub const fn from_raw(row: EntityRow) -> Entity { + Self::from_raw_and_generation(row, NonZero::::MIN) + } + + /// This is equivalent to [`from_raw`](Self::from_raw) except that it takes a `u32` instead of an [`EntityRow`]. + /// + /// Returns `None` if the row is `u32::MAX`. + #[inline(always)] + pub const fn from_raw_u32(row: u32) -> Option { + match NonMaxU32::new(row) { + Some(row) => Some(Self::from_raw(EntityRow::new(row))), + None => None, + } } /// Convert to a form convenient for passing outside of rust. @@ -314,7 +410,7 @@ impl Entity { /// No particular structure is guaranteed for the returned bits. #[inline(always)] pub const fn to_bits(self) -> u64 { - IdentifierMask::pack_into_u64(self.index, self.generation.get()) + IdentifierMask::pack_into_u64(self.row.to_bits(), self.generation.get()) } /// Reconstruct an `Entity` previously destructured with [`Entity::to_bits`]. @@ -346,10 +442,12 @@ impl Entity { let kind = id.kind() as u8; if kind == (IdKind::Entity as u8) { - return Ok(Self { - index: id.low(), - generation: id.high(), - }); + if let Some(row) = EntityRow::try_from_bits(id.low()) { + return Ok(Self { + row, + generation: id.high(), + }); + } } } @@ -357,18 +455,25 @@ impl Entity { } /// Return a transiently unique identifier. + /// See also [`EntityRow`]. /// - /// No two simultaneously-live entities share the same index, but dead entities' indices may collide + /// No two simultaneously-live entities share the same row, but dead entities' indices may collide /// with both live and dead entities. Useful for compactly representing entities within a /// specific snapshot of the world, such as when serializing. #[inline] - pub const fn index(self) -> u32 { - self.index + pub const fn row(self) -> EntityRow { + self.row } - /// Returns the generation of this Entity's index. The generation is incremented each time an - /// entity with a given index is despawned. This serves as a "count" of the number of times a - /// given index has been reused (index, generation) pairs uniquely identify a given Entity. + /// Equivalent to `self.row().index()`. See [`Self::row`] for details. + #[inline] + pub const fn index(self) -> u32 { + self.row.index() + } + + /// Returns the generation of this Entity's row. The generation is incremented each time an + /// entity with a given row is despawned. This serves as a "count" of the number of times a + /// given row has been reused (row, generation) pairs uniquely identify a given Entity. #[inline] pub const fn generation(self) -> u32 { // Mask so not to expose any flags @@ -471,12 +576,12 @@ impl fmt::Display for Entity { impl SparseSetIndex for Entity { #[inline] fn sparse_set_index(&self) -> usize { - self.index() as usize + self.row().sparse_set_index() } #[inline] fn get_sparse_set_index(value: usize) -> Self { - Entity::from_raw(value as u32) + Entity::from_raw(EntityRow::get_sparse_set_index(value)) } } @@ -486,7 +591,7 @@ pub struct ReserveEntitiesIterator<'a> { meta: &'a [EntityMeta], // Reserved indices formerly in the freelist to hand out. - freelist_indices: core::slice::Iter<'a, u32>, + freelist_indices: core::slice::Iter<'a, EntityRow>, // New Entity indices to hand out, outside the range of meta.len(). new_indices: core::ops::Range, @@ -498,10 +603,16 @@ impl<'a> Iterator for ReserveEntitiesIterator<'a> { fn next(&mut self) -> Option { self.freelist_indices .next() - .map(|&index| { - Entity::from_raw_and_generation(index, self.meta[index as usize].generation) + .map(|&row| { + Entity::from_raw_and_generation(row, self.meta[row.index() as usize].generation) + }) + .or_else(|| { + self.new_indices.next().map(|index| { + // SAFETY: This came from an exclusive range so the max can't be hit. + let row = unsafe { EntityRow::new(NonMaxU32::new_unchecked(index)) }; + Entity::from_raw(row) + }) }) - .or_else(|| self.new_indices.next().map(Entity::from_raw)) } fn size_hint(&self) -> (usize, Option) { @@ -568,7 +679,7 @@ pub struct Entities { /// [`reserve_entity`]: Entities::reserve_entity /// [`reserve_entities`]: Entities::reserve_entities /// [`flush`]: Entities::flush - pending: Vec, + pending: Vec, free_cursor: AtomicIdCursor, } @@ -644,17 +755,21 @@ impl Entities { let n = self.free_cursor.fetch_sub(1, Ordering::Relaxed); if n > 0 { // Allocate from the freelist. - let index = self.pending[(n - 1) as usize]; - Entity::from_raw_and_generation(index, self.meta[index as usize].generation) + let row = self.pending[(n - 1) as usize]; + Entity::from_raw_and_generation(row, self.meta[row.index() as usize].generation) } else { // Grab a new ID, outside the range of `meta.len()`. `flush()` must // eventually be called to make it valid. // // As `self.free_cursor` goes more and more negative, we return IDs farther // and farther beyond `meta.len()`. - Entity::from_raw( - u32::try_from(self.meta.len() as IdCursor - n).expect("too many entities"), - ) + let raw = self.meta.len() as IdCursor - n; + if raw >= u32::MAX as IdCursor { + panic!("too many entities"); + } + // SAFETY: We just checked the bounds + let row = unsafe { EntityRow::new(NonMaxU32::new_unchecked(raw as u32)) }; + Entity::from_raw(row) } } @@ -669,14 +784,17 @@ impl Entities { /// Allocate an entity ID directly. pub fn alloc(&mut self) -> Entity { self.verify_flushed(); - if let Some(index) = self.pending.pop() { + if let Some(row) = self.pending.pop() { let new_free_cursor = self.pending.len() as IdCursor; *self.free_cursor.get_mut() = new_free_cursor; - Entity::from_raw_and_generation(index, self.meta[index as usize].generation) + Entity::from_raw_and_generation(row, self.meta[row.index() as usize].generation) } else { - let index = u32::try_from(self.meta.len()).expect("too many entities"); + let index = u32::try_from(self.meta.len()) + .ok() + .and_then(NonMaxU32::new) + .expect("too many entities"); self.meta.push(EntityMeta::EMPTY); - Entity::from_raw(index) + Entity::from_raw(EntityRow::new(index)) } } @@ -696,13 +814,13 @@ impl Entities { if meta.generation == NonZero::::MIN { warn!( "Entity({}) generation wrapped on Entities::free, aliasing may occur", - entity.index + entity.row() ); } let loc = mem::replace(&mut meta.location, EntityMeta::EMPTY.location); - self.pending.push(entity.index()); + self.pending.push(entity.row()); let new_free_cursor = self.pending.len() as IdCursor; *self.free_cursor.get_mut() = new_free_cursor; @@ -734,7 +852,7 @@ impl Entities { // This will return false for entities which have been freed, even if // not reallocated since the generation is incremented in `free` pub fn contains(&self, entity: Entity) -> bool { - self.resolve_from_id(entity.index()) + self.resolve_from_id(entity.row()) .is_some_and(|e| e.generation() == entity.generation()) } @@ -800,17 +918,17 @@ impl Entities { /// Note: This method may return [`Entities`](Entity) which are currently free /// Note that [`contains`](Entities::contains) will correctly return false for freed /// entities, since it checks the generation - pub fn resolve_from_id(&self, index: u32) -> Option { - let idu = index as usize; + pub fn resolve_from_id(&self, row: EntityRow) -> Option { + let idu = row.index() as usize; if let Some(&EntityMeta { generation, .. }) = self.meta.get(idu) { - Some(Entity::from_raw_and_generation(index, generation)) + Some(Entity::from_raw_and_generation(row, generation)) } else { // `id` is outside of the meta list - check whether it is reserved but not yet flushed. let free_cursor = self.free_cursor.load(Ordering::Relaxed); // If this entity was manually created, then free_cursor might be positive // Returning None handles that case correctly let num_pending = usize::try_from(-free_cursor).ok()?; - (idu < self.meta.len() + num_pending).then_some(Entity::from_raw(index)) + (idu < self.meta.len() + num_pending).then_some(Entity::from_raw(row)) } } @@ -839,8 +957,10 @@ impl Entities { let new_meta_len = old_meta_len + -current_free_cursor as usize; self.meta.resize(new_meta_len, EntityMeta::EMPTY); for (index, meta) in self.meta.iter_mut().enumerate().skip(old_meta_len) { + // SAFETY: the index is less than the meta length, which can not exceeded u32::MAX + let row = EntityRow::new(unsafe { NonMaxU32::new_unchecked(index as u32) }); init( - Entity::from_raw_and_generation(index as u32, meta.generation), + Entity::from_raw_and_generation(row, meta.generation), &mut meta.location, ); } @@ -849,10 +969,10 @@ impl Entities { 0 }; - for index in self.pending.drain(new_free_cursor..) { - let meta = &mut self.meta[index as usize]; + for row in self.pending.drain(new_free_cursor..) { + let meta = &mut self.meta[row.index() as usize]; init( - Entity::from_raw_and_generation(index, meta.generation), + Entity::from_raw_and_generation(row, meta.generation), &mut meta.location, ); } @@ -1064,9 +1184,14 @@ mod tests { #[test] fn entity_bits_roundtrip() { + let r = EntityRow::new(NonMaxU32::new(0xDEADBEEF).unwrap()); + assert_eq!(EntityRow::from_bits(r.to_bits()), r); + // Generation cannot be greater than 0x7FFF_FFFF else it will be an invalid Entity id - let e = - Entity::from_raw_and_generation(0xDEADBEEF, NonZero::::new(0x5AADF00D).unwrap()); + let e = Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(0xDEADBEEF).unwrap()), + NonZero::::new(0x5AADF00D).unwrap(), + ); assert_eq!(Entity::from_bits(e.to_bits()), e); } @@ -1099,18 +1224,18 @@ mod tests { #[test] fn entity_const() { - const C1: Entity = Entity::from_raw(42); + const C1: Entity = Entity::from_raw(EntityRow::new(NonMaxU32::new(42).unwrap())); assert_eq!(42, C1.index()); assert_eq!(1, C1.generation()); const C2: Entity = Entity::from_bits(0x0000_00ff_0000_00cc); - assert_eq!(0x0000_00cc, C2.index()); + assert_eq!(!0x0000_00cc, C2.index()); assert_eq!(0x0000_00ff, C2.generation()); - const C3: u32 = Entity::from_raw(33).index(); + const C3: u32 = Entity::from_raw(EntityRow::new(NonMaxU32::new(33).unwrap())).index(); assert_eq!(33, C3); - const C4: u32 = Entity::from_bits(0x00dd_00ff_0000_0000).generation(); + const C4: u32 = Entity::from_bits(0x00dd_00ff_1111_1111).generation(); assert_eq!(0x00dd_00ff, C4); } @@ -1146,65 +1271,139 @@ mod tests { )] fn entity_comparison() { assert_eq!( - Entity::from_raw_and_generation(123, NonZero::::new(456).unwrap()), - Entity::from_raw_and_generation(123, NonZero::::new(456).unwrap()) + Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(123).unwrap()), + NonZero::::new(456).unwrap() + ), + Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(123).unwrap()), + NonZero::::new(456).unwrap() + ) ); assert_ne!( - Entity::from_raw_and_generation(123, NonZero::::new(789).unwrap()), - Entity::from_raw_and_generation(123, NonZero::::new(456).unwrap()) + Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(123).unwrap()), + NonZero::::new(789).unwrap() + ), + Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(123).unwrap()), + NonZero::::new(456).unwrap() + ) ); assert_ne!( - Entity::from_raw_and_generation(123, NonZero::::new(456).unwrap()), - Entity::from_raw_and_generation(123, NonZero::::new(789).unwrap()) + Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(123).unwrap()), + NonZero::::new(456).unwrap() + ), + Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(123).unwrap()), + NonZero::::new(789).unwrap() + ) ); assert_ne!( - Entity::from_raw_and_generation(123, NonZero::::new(456).unwrap()), - Entity::from_raw_and_generation(456, NonZero::::new(123).unwrap()) + Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(123).unwrap()), + NonZero::::new(456).unwrap() + ), + Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(456).unwrap()), + NonZero::::new(123).unwrap() + ) ); // ordering is by generation then by index assert!( - Entity::from_raw_and_generation(123, NonZero::::new(456).unwrap()) - >= Entity::from_raw_and_generation(123, NonZero::::new(456).unwrap()) + Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(123).unwrap()), + NonZero::::new(456).unwrap() + ) >= Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(123).unwrap()), + NonZero::::new(456).unwrap() + ) ); assert!( - Entity::from_raw_and_generation(123, NonZero::::new(456).unwrap()) - <= Entity::from_raw_and_generation(123, NonZero::::new(456).unwrap()) + Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(123).unwrap()), + NonZero::::new(456).unwrap() + ) <= Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(123).unwrap()), + NonZero::::new(456).unwrap() + ) ); assert!( - !(Entity::from_raw_and_generation(123, NonZero::::new(456).unwrap()) - < Entity::from_raw_and_generation(123, NonZero::::new(456).unwrap())) + !(Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(123).unwrap()), + NonZero::::new(456).unwrap() + ) < Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(123).unwrap()), + NonZero::::new(456).unwrap() + )) ); assert!( - !(Entity::from_raw_and_generation(123, NonZero::::new(456).unwrap()) - > Entity::from_raw_and_generation(123, NonZero::::new(456).unwrap())) + !(Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(123).unwrap()), + NonZero::::new(456).unwrap() + ) > Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(123).unwrap()), + NonZero::::new(456).unwrap() + )) ); assert!( - Entity::from_raw_and_generation(9, NonZero::::new(1).unwrap()) - < Entity::from_raw_and_generation(1, NonZero::::new(9).unwrap()) + Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(9).unwrap()), + NonZero::::new(1).unwrap() + ) < Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(1).unwrap()), + NonZero::::new(9).unwrap() + ) ); assert!( - Entity::from_raw_and_generation(1, NonZero::::new(9).unwrap()) - > Entity::from_raw_and_generation(9, NonZero::::new(1).unwrap()) + Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(1).unwrap()), + NonZero::::new(9).unwrap() + ) > Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(9).unwrap()), + NonZero::::new(1).unwrap() + ) ); assert!( - Entity::from_raw_and_generation(1, NonZero::::new(1).unwrap()) - < Entity::from_raw_and_generation(2, NonZero::::new(1).unwrap()) + Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(1).unwrap()), + NonZero::::new(1).unwrap() + ) > Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(2).unwrap()), + NonZero::::new(1).unwrap() + ) ); assert!( - Entity::from_raw_and_generation(1, NonZero::::new(1).unwrap()) - <= Entity::from_raw_and_generation(2, NonZero::::new(1).unwrap()) + Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(1).unwrap()), + NonZero::::new(1).unwrap() + ) >= Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(2).unwrap()), + NonZero::::new(1).unwrap() + ) ); assert!( - Entity::from_raw_and_generation(2, NonZero::::new(2).unwrap()) - > Entity::from_raw_and_generation(1, NonZero::::new(2).unwrap()) + Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(2).unwrap()), + NonZero::::new(2).unwrap() + ) < Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(1).unwrap()), + NonZero::::new(2).unwrap() + ) ); assert!( - Entity::from_raw_and_generation(2, NonZero::::new(2).unwrap()) - >= Entity::from_raw_and_generation(1, NonZero::::new(2).unwrap()) + Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(2).unwrap()), + NonZero::::new(2).unwrap() + ) <= Entity::from_raw_and_generation( + EntityRow::new(NonMaxU32::new(1).unwrap()), + NonZero::::new(2).unwrap() + ) ); } @@ -1216,12 +1415,16 @@ mod tests { let hash = EntityHash; let first_id = 0xC0FFEE << 8; - let first_hash = hash.hash_one(Entity::from_raw(first_id)); + let first_hash = hash.hash_one(Entity::from_raw(EntityRow::new( + NonMaxU32::new(first_id).unwrap(), + ))); for i in 1..=255 { let id = first_id + i; - let hash = hash.hash_one(Entity::from_raw(id)); - assert_eq!(hash.wrapping_sub(first_hash) as u32, i); + let hash = hash.hash_one(Entity::from_raw(EntityRow::new( + NonMaxU32::new(id).unwrap(), + ))); + assert_eq!(first_hash.wrapping_sub(hash) as u32, i); } } @@ -1232,20 +1435,24 @@ mod tests { let hash = EntityHash; let first_id = 0xC0FFEE; - let first_hash = hash.hash_one(Entity::from_raw(first_id)) >> 57; + let first_hash = hash.hash_one(Entity::from_raw(EntityRow::new( + NonMaxU32::new(first_id).unwrap(), + ))) >> 57; for bit in 0..u32::BITS { let id = first_id ^ (1 << bit); - let hash = hash.hash_one(Entity::from_raw(id)) >> 57; + let hash = hash.hash_one(Entity::from_raw(EntityRow::new( + NonMaxU32::new(id).unwrap(), + ))) >> 57; assert_ne!(hash, first_hash); } } #[test] fn entity_debug() { - let entity = Entity::from_raw(42); + let entity = Entity::from_raw(EntityRow::new(NonMaxU32::new(42).unwrap())); let string = format!("{:?}", entity); - assert_eq!(string, "42v1#4294967338"); + assert_eq!(string, "42v1#8589934549"); let entity = Entity::PLACEHOLDER; let string = format!("{:?}", entity); @@ -1254,7 +1461,7 @@ mod tests { #[test] fn entity_display() { - let entity = Entity::from_raw(42); + let entity = Entity::from_raw(EntityRow::new(NonMaxU32::new(42).unwrap())); let string = format!("{}", entity); assert_eq!(string, "42v1"); diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 44eae20ffb..51bf415400 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -484,10 +484,9 @@ mod tests { results.lock().unwrap().push((e, i)); }); results.lock().unwrap().sort(); - assert_eq!( - &*results.lock().unwrap(), - &[(e1, 1), (e2, 2), (e3, 3), (e4, 4), (e5, 5)] - ); + let mut expected = [(e1, 1), (e2, 2), (e3, 3), (e4, 4), (e5, 5)]; + expected.sort(); + assert_eq!(&*results.lock().unwrap(), &expected); } #[test] @@ -505,10 +504,9 @@ mod tests { .par_iter(&world) .for_each(|(e, &SparseStored(i))| results.lock().unwrap().push((e, i))); results.lock().unwrap().sort(); - assert_eq!( - &*results.lock().unwrap(), - &[(e1, 1), (e2, 2), (e3, 3), (e4, 4), (e5, 5)] - ); + let mut expected = [(e1, 1), (e2, 2), (e3, 3), (e4, 4), (e5, 5)]; + expected.sort(); + assert_eq!(&*results.lock().unwrap(), &expected); } #[test] @@ -1538,8 +1536,8 @@ mod tests { let mut world_a = World::new(); let world_b = World::new(); let mut query = world_a.query::<&A>(); - let _ = query.get(&world_a, Entity::from_raw(0)); - let _ = query.get(&world_b, Entity::from_raw(0)); + let _ = query.get(&world_a, Entity::from_raw_u32(0).unwrap()); + let _ = query.get(&world_b, Entity::from_raw_u32(0).unwrap()); } #[test] @@ -1780,7 +1778,7 @@ mod tests { fn try_insert_batch() { let mut world = World::default(); let e0 = world.spawn(A(0)).id(); - let e1 = Entity::from_raw(1); + let e1 = Entity::from_raw_u32(1).unwrap(); let values = vec![(e0, (A(1), B(0))), (e1, (A(0), B(1)))]; @@ -1804,7 +1802,7 @@ mod tests { fn try_insert_batch_if_new() { let mut world = World::default(); let e0 = world.spawn(A(0)).id(); - let e1 = Entity::from_raw(1); + let e1 = Entity::from_raw_u32(1).unwrap(); let values = vec![(e0, (A(1), B(0))), (e1, (A(0), B(1)))]; diff --git a/crates/bevy_ecs/src/observer/mod.rs b/crates/bevy_ecs/src/observer/mod.rs index 783dafd2d1..2343e66aa1 100644 --- a/crates/bevy_ecs/src/observer/mod.rs +++ b/crates/bevy_ecs/src/observer/mod.rs @@ -1075,7 +1075,7 @@ mod tests { world.add_observer(|_: Trigger, mut res: ResMut| res.observed("add_2")); world.spawn(A).flush(); - assert_eq!(vec!["add_1", "add_2"], world.resource::().0); + assert_eq!(vec!["add_2", "add_1"], world.resource::().0); // Our A entity plus our two observers assert_eq!(world.entities().len(), 3); } diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 326443dd85..81cae32f2c 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -993,7 +993,7 @@ impl QueryState { /// /// assert_eq!(component_values, [&A(0), &A(1), &A(2)]); /// - /// let wrong_entity = Entity::from_raw(365); + /// let wrong_entity = Entity::from_raw_u32(365).unwrap(); /// /// assert_eq!(match query_state.get_many(&mut world, [wrong_entity]).unwrap_err() {QueryEntityError::EntityDoesNotExist(error) => error.entity, _ => panic!()}, wrong_entity); /// ``` @@ -1031,7 +1031,7 @@ impl QueryState { /// /// assert_eq!(component_values, [&A(0), &A(1), &A(2)]); /// - /// let wrong_entity = Entity::from_raw(365); + /// let wrong_entity = Entity::from_raw_u32(365).unwrap(); /// /// assert_eq!(match query_state.get_many_unique(&mut world, UniqueEntityArray::from([wrong_entity])).unwrap_err() {QueryEntityError::EntityDoesNotExist(error) => error.entity, _ => panic!()}, wrong_entity); /// ``` @@ -1087,7 +1087,7 @@ impl QueryState { /// /// assert_eq!(component_values, [&A(5), &A(6), &A(7)]); /// - /// let wrong_entity = Entity::from_raw(57); + /// let wrong_entity = Entity::from_raw_u32(57).unwrap(); /// let invalid_entity = world.spawn_empty().id(); /// /// assert_eq!(match query_state.get_many(&mut world, [wrong_entity]).unwrap_err() {QueryEntityError::EntityDoesNotExist(error) => error.entity, _ => panic!()}, wrong_entity); @@ -1133,7 +1133,7 @@ impl QueryState { /// /// assert_eq!(component_values, [&A(5), &A(6), &A(7)]); /// - /// let wrong_entity = Entity::from_raw(57); + /// let wrong_entity = Entity::from_raw_u32(57).unwrap(); /// let invalid_entity = world.spawn_empty().id(); /// /// assert_eq!(match query_state.get_many_unique(&mut world, UniqueEntityArray::from([wrong_entity])).unwrap_err() {QueryEntityError::EntityDoesNotExist(error) => error.entity, _ => panic!()}, wrong_entity); @@ -1461,7 +1461,7 @@ impl QueryState { /// /// # assert_eq!(component_values, [&A(5), &A(6), &A(7)]); /// - /// # let wrong_entity = Entity::from_raw(57); + /// # let wrong_entity = Entity::from_raw_u32(57).unwrap(); /// # let invalid_entity = world.spawn_empty().id(); /// /// # assert_eq!(match query_state.get_many(&mut world, [wrong_entity]).unwrap_err() {QueryEntityError::EntityDoesNotExist(error) => error.entity, _ => panic!()}, wrong_entity); @@ -1782,7 +1782,7 @@ impl QueryState { /// /// fn my_system(query: Query<&A>) -> Result { /// let a = query.single()?; - /// + /// /// // Do something with `a` /// Ok(()) /// } @@ -1884,7 +1884,7 @@ mod tests { let world_2 = World::new(); let mut query_state = world_1.query::(); - let _panics = query_state.get(&world_2, Entity::from_raw(0)); + let _panics = query_state.get(&world_2, Entity::from_raw_u32(0).unwrap()); } #[test] diff --git a/crates/bevy_ecs/src/relationship/relationship_source_collection.rs b/crates/bevy_ecs/src/relationship/relationship_source_collection.rs index 49e3725f6a..91c7ac3a9d 100644 --- a/crates/bevy_ecs/src/relationship/relationship_source_collection.rs +++ b/crates/bevy_ecs/src/relationship/relationship_source_collection.rs @@ -709,7 +709,7 @@ mod tests { let collection = rel_target.collection(); // Insertions should maintain ordering - assert!(collection.iter().eq(&[b, c, d])); + assert!(collection.iter().eq(&[d, c, b])); world.entity_mut(c).despawn(); @@ -717,7 +717,7 @@ mod tests { let collection = rel_target.collection(); // Removals should maintain ordering - assert!(collection.iter().eq(&[b, d])); + assert!(collection.iter().eq(&[d, b])); } #[test] diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index 6c809df849..69305e8a14 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -659,10 +659,11 @@ mod tests { use super::SparseSets; use crate::{ component::{Component, ComponentDescriptor, ComponentId, ComponentInfo}, - entity::Entity, + entity::{Entity, EntityRow}, storage::SparseSet, }; use alloc::{vec, vec::Vec}; + use nonmax::NonMaxU32; #[derive(Debug, Eq, PartialEq)] struct Foo(usize); @@ -670,11 +671,11 @@ mod tests { #[test] fn sparse_set() { let mut set = SparseSet::::default(); - let e0 = Entity::from_raw(0); - let e1 = Entity::from_raw(1); - let e2 = Entity::from_raw(2); - let e3 = Entity::from_raw(3); - let e4 = Entity::from_raw(4); + let e0 = Entity::from_raw(EntityRow::new(NonMaxU32::new(0).unwrap())); + let e1 = Entity::from_raw(EntityRow::new(NonMaxU32::new(1).unwrap())); + let e2 = Entity::from_raw(EntityRow::new(NonMaxU32::new(2).unwrap())); + let e3 = Entity::from_raw(EntityRow::new(NonMaxU32::new(3).unwrap())); + let e4 = Entity::from_raw(EntityRow::new(NonMaxU32::new(4).unwrap())); set.insert(e1, Foo(1)); set.insert(e2, Foo(2)); diff --git a/crates/bevy_ecs/src/storage/table/mod.rs b/crates/bevy_ecs/src/storage/table/mod.rs index 0f80b77f51..fd8eb410e4 100644 --- a/crates/bevy_ecs/src/storage/table/mod.rs +++ b/crates/bevy_ecs/src/storage/table/mod.rs @@ -823,11 +823,12 @@ mod tests { use crate::{ change_detection::MaybeLocation, component::{Component, ComponentIds, Components, ComponentsRegistrator, Tick}, - entity::Entity, + entity::{Entity, EntityRow}, ptr::OwningPtr, storage::{TableBuilder, TableId, TableRow, Tables}, }; use alloc::vec::Vec; + use nonmax::NonMaxU32; #[derive(Component)] struct W(T); @@ -856,7 +857,9 @@ mod tests { let mut table = TableBuilder::with_capacity(0, columns.len()) .add_column(components.get_info(component_id).unwrap()) .build(); - let entities = (0..200).map(Entity::from_raw).collect::>(); + let entities = (0..200) + .map(|index| Entity::from_raw(EntityRow::new(NonMaxU32::new(index).unwrap()))) + .collect::>(); for entity in &entities { // SAFETY: we allocate and immediately set data afterwards unsafe { diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 71e63cc167..fddbe5376c 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -933,7 +933,7 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { /// /// type Item = &'a Entity; /// type IntoIter = UniqueEntityIter>; - /// + /// /// fn into_iter(self) -> Self::IntoIter { /// // SAFETY: `Friends` ensures that it unique_list contains only unique entities. /// unsafe { UniqueEntityIter::from_iterator_unchecked(self.unique_list.iter()) } @@ -1414,7 +1414,7 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { /// /// assert_eq!(component_values, [&A(0), &A(1), &A(2)]); /// - /// let wrong_entity = Entity::from_raw(365); + /// let wrong_entity = Entity::from_raw_u32(365).unwrap(); /// /// assert_eq!( /// match query.get_many([wrong_entity]).unwrap_err() { @@ -1465,7 +1465,7 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { /// /// assert_eq!(component_values, [&A(0), &A(1), &A(2)]); /// - /// let wrong_entity = Entity::from_raw(365); + /// let wrong_entity = Entity::from_raw_u32(365).unwrap(); /// /// assert_eq!( /// match query.get_many_unique(UniqueEntityArray::from([wrong_entity])).unwrap_err() { @@ -1610,7 +1610,7 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { /// let entities: [Entity; 3] = entities.try_into().unwrap(); /// /// world.spawn(A(73)); - /// let wrong_entity = Entity::from_raw(57); + /// let wrong_entity = Entity::from_raw_u32(57).unwrap(); /// let invalid_entity = world.spawn_empty().id(); /// /// @@ -1684,7 +1684,7 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { /// let entity_set: UniqueEntityArray<3> = entity_set.try_into().unwrap(); /// /// world.spawn(A(73)); - /// let wrong_entity = Entity::from_raw(57); + /// let wrong_entity = Entity::from_raw_u32(57).unwrap(); /// let invalid_entity = world.spawn_empty().id(); /// /// @@ -2234,7 +2234,7 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { /// * `&mut T` -> `&T` /// * `&mut T` -> `Ref` /// * [`EntityMut`](crate::world::EntityMut) -> [`EntityRef`](crate::world::EntityRef) - /// + /// /// [`EntityLocation`]: crate::entity::EntityLocation /// [`&Archetype`]: crate::archetype::Archetype /// diff --git a/crates/bevy_remote/src/builtin_methods.rs b/crates/bevy_remote/src/builtin_methods.rs index ce5fa259a1..b59f08604b 100644 --- a/crates/bevy_remote/src/builtin_methods.rs +++ b/crates/bevy_remote/src/builtin_methods.rs @@ -1490,13 +1490,14 @@ mod tests { "Deserialized value does not match original" ); } + use super::*; #[test] fn serialization_tests() { test_serialize_deserialize(BrpQueryRow { components: Default::default(), - entity: Entity::from_raw(0), + entity: Entity::from_raw_u32(0).unwrap(), has: Default::default(), }); test_serialize_deserialize(BrpListWatchingResponse::default()); @@ -1510,7 +1511,7 @@ mod tests { ..Default::default() }); test_serialize_deserialize(BrpListParams { - entity: Entity::from_raw(0), + entity: Entity::from_raw_u32(0).unwrap(), }); } } diff --git a/crates/bevy_scene/src/dynamic_scene_builder.rs b/crates/bevy_scene/src/dynamic_scene_builder.rs index 5deac09aaa..c9e594107e 100644 --- a/crates/bevy_scene/src/dynamic_scene_builder.rs +++ b/crates/bevy_scene/src/dynamic_scene_builder.rs @@ -509,10 +509,10 @@ mod tests { let mut entities = builder.build().entities.into_iter(); // Assert entities are ordered - assert_eq!(entity_a, entities.next().map(|e| e.entity).unwrap()); - assert_eq!(entity_b, entities.next().map(|e| e.entity).unwrap()); - assert_eq!(entity_c, entities.next().map(|e| e.entity).unwrap()); assert_eq!(entity_d, entities.next().map(|e| e.entity).unwrap()); + assert_eq!(entity_c, entities.next().map(|e| e.entity).unwrap()); + assert_eq!(entity_b, entities.next().map(|e| e.entity).unwrap()); + assert_eq!(entity_a, entities.next().map(|e| e.entity).unwrap()); } #[test] @@ -539,7 +539,7 @@ mod tests { assert_eq!(scene.entities.len(), 2); let mut scene_entities = vec![scene.entities[0].entity, scene.entities[1].entity]; scene_entities.sort(); - assert_eq!(scene_entities, [entity_a_b, entity_a]); + assert_eq!(scene_entities, [entity_a, entity_a_b]); } #[test] @@ -621,9 +621,9 @@ mod tests { .build(); assert_eq!(scene.entities.len(), 3); - assert!(scene.entities[0].components[0].represents::()); + assert!(scene.entities[2].components[0].represents::()); assert!(scene.entities[1].components[0].represents::()); - assert_eq!(scene.entities[2].components.len(), 0); + assert_eq!(scene.entities[0].components.len(), 0); } #[test] diff --git a/crates/bevy_scene/src/scene_spawner.rs b/crates/bevy_scene/src/scene_spawner.rs index dce5ad971e..3bf8ca9f64 100644 --- a/crates/bevy_scene/src/scene_spawner.rs +++ b/crates/bevy_scene/src/scene_spawner.rs @@ -912,7 +912,7 @@ mod tests { app.update(); let world = app.world_mut(); - let spawned_root = world.entity(spawned).get::().unwrap()[0]; + let spawned_root = world.entity(spawned).get::().unwrap()[1]; let children = world.entity(spawned_root).get::().unwrap(); assert_eq!(children.len(), 3); assert!(world.entity(children[0]).get::().is_some()); diff --git a/crates/bevy_scene/src/serde.rs b/crates/bevy_scene/src/serde.rs index ead8933a49..9eff75f901 100644 --- a/crates/bevy_scene/src/serde.rs +++ b/crates/bevy_scene/src/serde.rs @@ -638,24 +638,24 @@ mod tests { ), }, entities: { - 4294967296: ( - components: { - "bevy_scene::serde::tests::Foo": (123), - }, - ), - 4294967297: ( - components: { - "bevy_scene::serde::tests::Bar": (345), - "bevy_scene::serde::tests::Foo": (123), - }, - ), - 4294967298: ( + 8589934589: ( components: { "bevy_scene::serde::tests::Bar": (345), "bevy_scene::serde::tests::Baz": (789), "bevy_scene::serde::tests::Foo": (123), }, ), + 8589934590: ( + components: { + "bevy_scene::serde::tests::Bar": (345), + "bevy_scene::serde::tests::Foo": (123), + }, + ), + 8589934591: ( + components: { + "bevy_scene::serde::tests::Foo": (123), + }, + ), }, )"#; let output = scene @@ -675,18 +675,18 @@ mod tests { ), }, entities: { - 4294967296: ( + 8589934591: ( components: { "bevy_scene::serde::tests::Foo": (123), }, ), - 4294967297: ( + 8589934590: ( components: { "bevy_scene::serde::tests::Foo": (123), "bevy_scene::serde::tests::Bar": (345), }, ), - 4294967298: ( + 8589934589: ( components: { "bevy_scene::serde::tests::Foo": (123), "bevy_scene::serde::tests::Bar": (345), @@ -815,7 +815,7 @@ mod tests { assert_eq!( vec![ - 0, 1, 128, 128, 128, 128, 16, 1, 37, 98, 101, 118, 121, 95, 115, 99, 101, 110, 101, + 0, 1, 255, 255, 255, 255, 31, 1, 37, 98, 101, 118, 121, 95, 115, 99, 101, 110, 101, 58, 58, 115, 101, 114, 100, 101, 58, 58, 116, 101, 115, 116, 115, 58, 58, 77, 121, 67, 111, 109, 112, 111, 110, 101, 110, 116, 1, 2, 3, 102, 102, 166, 63, 205, 204, 108, 64, 1, 12, 72, 101, 108, 108, 111, 32, 87, 111, 114, 108, 100, 33 @@ -856,11 +856,12 @@ mod tests { assert_eq!( vec![ - 146, 128, 129, 207, 0, 0, 0, 1, 0, 0, 0, 0, 145, 129, 217, 37, 98, 101, 118, 121, - 95, 115, 99, 101, 110, 101, 58, 58, 115, 101, 114, 100, 101, 58, 58, 116, 101, 115, - 116, 115, 58, 58, 77, 121, 67, 111, 109, 112, 111, 110, 101, 110, 116, 147, 147, 1, - 2, 3, 146, 202, 63, 166, 102, 102, 202, 64, 108, 204, 205, 129, 165, 84, 117, 112, - 108, 101, 172, 72, 101, 108, 108, 111, 32, 87, 111, 114, 108, 100, 33 + 146, 128, 129, 207, 0, 0, 0, 1, 255, 255, 255, 255, 145, 129, 217, 37, 98, 101, + 118, 121, 95, 115, 99, 101, 110, 101, 58, 58, 115, 101, 114, 100, 101, 58, 58, 116, + 101, 115, 116, 115, 58, 58, 77, 121, 67, 111, 109, 112, 111, 110, 101, 110, 116, + 147, 147, 1, 2, 3, 146, 202, 63, 166, 102, 102, 202, 64, 108, 204, 205, 129, 165, + 84, 117, 112, 108, 101, 172, 72, 101, 108, 108, 111, 32, 87, 111, 114, 108, 100, + 33 ], buf ); @@ -899,12 +900,13 @@ mod tests { assert_eq!( vec![ - 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 0, - 0, 0, 0, 0, 37, 0, 0, 0, 0, 0, 0, 0, 98, 101, 118, 121, 95, 115, 99, 101, 110, 101, - 58, 58, 115, 101, 114, 100, 101, 58, 58, 116, 101, 115, 116, 115, 58, 58, 77, 121, - 67, 111, 109, 112, 111, 110, 101, 110, 116, 1, 0, 0, 0, 0, 0, 0, 0, 2, 0, 0, 0, 0, - 0, 0, 0, 3, 0, 0, 0, 0, 0, 0, 0, 102, 102, 166, 63, 205, 204, 108, 64, 1, 0, 0, 0, - 12, 0, 0, 0, 0, 0, 0, 0, 72, 101, 108, 108, 111, 32, 87, 111, 114, 108, 100, 33 + 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 255, 255, 255, 255, 1, 0, 0, 0, 1, + 0, 0, 0, 0, 0, 0, 0, 37, 0, 0, 0, 0, 0, 0, 0, 98, 101, 118, 121, 95, 115, 99, 101, + 110, 101, 58, 58, 115, 101, 114, 100, 101, 58, 58, 116, 101, 115, 116, 115, 58, 58, + 77, 121, 67, 111, 109, 112, 111, 110, 101, 110, 116, 1, 0, 0, 0, 0, 0, 0, 0, 2, 0, + 0, 0, 0, 0, 0, 0, 3, 0, 0, 0, 0, 0, 0, 0, 102, 102, 166, 63, 205, 204, 108, 64, 1, + 0, 0, 0, 12, 0, 0, 0, 0, 0, 0, 0, 72, 101, 108, 108, 111, 32, 87, 111, 114, 108, + 100, 33 ], serialized_scene ); diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index ecf6651271..62038b37ed 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -798,8 +798,8 @@ mod test { let translation = vec3(1.0, 0.0, 0.0); // These will be overwritten. - let mut child = Entity::from_raw(0); - let mut grandchild = Entity::from_raw(1); + let mut child = Entity::from_raw_u32(0).unwrap(); + let mut grandchild = Entity::from_raw_u32(1).unwrap(); let parent = app .world_mut() .spawn(Transform::from_translation(translation)) @@ -849,7 +849,7 @@ mod test { ); fn setup_world(world: &mut World) -> (Entity, Entity) { - let mut grandchild = Entity::from_raw(0); + let mut grandchild = Entity::from_raw_u32(0).unwrap(); let child = world .spawn(Transform::IDENTITY) .with_children(|builder| { diff --git a/crates/bevy_ui/src/layout/mod.rs b/crates/bevy_ui/src/layout/mod.rs index 1c1c60ea8f..9e6906b1f7 100644 --- a/crates/bevy_ui/src/layout/mod.rs +++ b/crates/bevy_ui/src/layout/mod.rs @@ -1039,7 +1039,7 @@ mod tests { let (mut world, ..) = setup_ui_test_world(); - let root_node_entity = Entity::from_raw(1); + let root_node_entity = Entity::from_raw_u32(1).unwrap(); struct TestSystemParam { root_node_entity: Entity, diff --git a/crates/bevy_ui/src/layout/ui_surface.rs b/crates/bevy_ui/src/layout/ui_surface.rs index a4af6737a7..2df6afa947 100644 --- a/crates/bevy_ui/src/layout/ui_surface.rs +++ b/crates/bevy_ui/src/layout/ui_surface.rs @@ -318,7 +318,7 @@ mod tests { #[test] fn test_upsert() { let mut ui_surface = UiSurface::default(); - let root_node_entity = Entity::from_raw(1); + let root_node_entity = Entity::from_raw_u32(1).unwrap(); let node = Node::default(); // standard upsert @@ -350,7 +350,7 @@ mod tests { #[test] fn test_remove_entities() { let mut ui_surface = UiSurface::default(); - let root_node_entity = Entity::from_raw(1); + let root_node_entity = Entity::from_raw_u32(1).unwrap(); let node = Node::default(); ui_surface.upsert_node(&LayoutContext::TEST_CONTEXT, root_node_entity, &node, None); @@ -366,7 +366,7 @@ mod tests { #[test] fn test_try_update_measure() { let mut ui_surface = UiSurface::default(); - let root_node_entity = Entity::from_raw(1); + let root_node_entity = Entity::from_raw_u32(1).unwrap(); let node = Node::default(); ui_surface.upsert_node(&LayoutContext::TEST_CONTEXT, root_node_entity, &node, None); @@ -381,8 +381,8 @@ mod tests { #[test] fn test_update_children() { let mut ui_surface = UiSurface::default(); - let root_node_entity = Entity::from_raw(1); - let child_entity = Entity::from_raw(2); + let root_node_entity = Entity::from_raw_u32(1).unwrap(); + let child_entity = Entity::from_raw_u32(2).unwrap(); let node = Node::default(); ui_surface.upsert_node(&LayoutContext::TEST_CONTEXT, root_node_entity, &node, None); @@ -402,8 +402,8 @@ mod tests { #[test] fn test_set_camera_children() { let mut ui_surface = UiSurface::default(); - let root_node_entity = Entity::from_raw(1); - let child_entity = Entity::from_raw(2); + let root_node_entity = Entity::from_raw_u32(1).unwrap(); + let child_entity = Entity::from_raw_u32(2).unwrap(); let node = Node::default(); ui_surface.upsert_node(&LayoutContext::TEST_CONTEXT, root_node_entity, &node, None); diff --git a/examples/ecs/error_handling.rs b/examples/ecs/error_handling.rs index b13a018530..d326d7d4aa 100644 --- a/examples/ecs/error_handling.rs +++ b/examples/ecs/error_handling.rs @@ -169,7 +169,7 @@ fn failing_system(world: &mut World) -> Result { fn failing_commands(mut commands: Commands) { commands // This entity doesn't exist! - .entity(Entity::from_raw(12345678)) + .entity(Entity::from_raw_u32(12345678).unwrap()) // Normally, this failed command would panic, // but since we've set the global error handler to `warn` // it will log a warning instead. diff --git a/release-content/migration-guides/make_entity_index_non_max.md b/release-content/migration-guides/make_entity_index_non_max.md new file mode 100644 index 0000000000..d3ba963854 --- /dev/null +++ b/release-content/migration-guides/make_entity_index_non_max.md @@ -0,0 +1,25 @@ +--- +title: Manual Entity Creation +pull_requests: [18704] +--- + +`Entity` no longer stores its index as a plain `u32` but as the new `EntityRow`, which wraps a `NonMaxU32`. +Previously, `Entity::index` could be `u32::MAX`, but that is no longer a valid index. +As a result, `Entity::from_raw` now takes `EntityRow` as a parameter instead of `u32`. `EntityRow` can be constructed via `EntityRow::new`, which takes a `NonMaxU32`. +This also means that the `Ord` implementation of `Entity` has changed: the index order is reversed, so 'newer' entities come before 'older' entities. +If you don't want to add [nonmax](https://docs.rs/nonmax/latest/nonmax/) as a dependency, use `Entity::from_raw_u32` which is identical to the previous `Entity::from_raw`, except that it now returns `Option` where the result is `None` if `u32::MAX` is passed. + +Bevy made this change because it puts a niche in the `EntityRow` type which makes `Option` half the size of `Option`. +This is used internally to open up performance improvements to the ECS. + +Although you probably shouldn't be making entities manually, it is sometimes useful to do so for tests. +To migrate tests, use: + +```diff +- let entity = Entity::from_raw(1); ++ let entity = Entity::from_raw_u32(1).unwrap(); +``` + +If you are creating entities manually in production, don't do that! +Use `Entities::alloc` instead. +But if you must create one manually, either reuse a `EntityRow` you know to be valid by using `Entity::from_raw` and `Entity::row`, or handle the error case of `None` returning from `Entity::from_raw_u32(my_index)`.