From 92ba6a11ff6beffe164e62e5b18e02363e823094 Mon Sep 17 00:00:00 2001 From: Natalie Baker Date: Sat, 23 Sep 2023 16:15:13 +0800 Subject: [PATCH 1/7] Change Entity::generation from u32 to NonZeroU32, fix tests, fix deserialization --- crates/bevy_ecs/src/entity/map_entities.rs | 22 ++++- crates/bevy_ecs/src/entity/mod.rs | 99 +++++++++++++++------- crates/bevy_ecs/src/lib.rs | 18 ++-- crates/bevy_scene/src/serde.rs | 32 +++---- 4 files changed, 112 insertions(+), 59 deletions(-) diff --git a/crates/bevy_ecs/src/entity/map_entities.rs b/crates/bevy_ecs/src/entity/map_entities.rs index 122e418179301..6d17c33f7ba18 100644 --- a/crates/bevy_ecs/src/entity/map_entities.rs +++ b/crates/bevy_ecs/src/entity/map_entities.rs @@ -1,3 +1,5 @@ +use std::num::NonZeroU32; + use crate::{entity::Entity, world::World}; use bevy_utils::HashMap; @@ -70,7 +72,17 @@ impl<'m> EntityMapper<'m> { // this new entity reference is specifically designed to never represent any living entity let new = Entity { - generation: self.dead_start.generation + self.generations, + // TODO: wrapping add will make u32::MAX + 1 == u32::MAX + 2 + // SAFETY: We use u32::max to ensure that we wrap to 1, meaning it's safe to construct NonZeroU32 without checking + generation: unsafe { + NonZeroU32::new_unchecked( + self.dead_start + .generation + .get() + .wrapping_add(self.generations) + .max(1), + ) + }, index: self.dead_start.index, }; self.generations += 1; @@ -147,7 +159,7 @@ mod tests { let mut world = World::new(); let mut mapper = EntityMapper::new(&mut map, &mut world); - let mapped_ent = Entity::new(FIRST_IDX, 0); + let mapped_ent = Entity::new(FIRST_IDX, 1).unwrap(); let dead_ref = mapper.get_or_reserve(mapped_ent); assert_eq!( @@ -156,7 +168,9 @@ mod tests { "should persist the allocated mapping from the previous line" ); assert_eq!( - mapper.get_or_reserve(Entity::new(SECOND_IDX, 0)).index(), + mapper + .get_or_reserve(Entity::new(SECOND_IDX, 1).unwrap()) + .index(), dead_ref.index(), "should re-use the same index for further dead refs" ); @@ -174,7 +188,7 @@ mod tests { let mut world = World::new(); let dead_ref = EntityMapper::world_scope(&mut map, &mut world, |_, mapper| { - mapper.get_or_reserve(Entity::new(0, 0)) + mapper.get_or_reserve(Entity::new(0, 1).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 3a49c99aedcf6..e91c78e4a644f 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -37,6 +37,7 @@ //! [`EntityWorldMut::remove`]: crate::world::EntityWorldMut::remove mod map_entities; +use bevy_utils::tracing::warn; pub use map_entities::*; use crate::{ @@ -44,7 +45,7 @@ use crate::{ storage::{SparseSetIndex, TableId, TableRow}, }; use serde::{Deserialize, Serialize}; -use std::{convert::TryFrom, fmt, mem, sync::atomic::Ordering}; +use std::{convert::TryFrom, fmt, mem, num::NonZeroU32, sync::atomic::Ordering}; #[cfg(target_has_atomic = "64")] use std::sync::atomic::AtomicI64 as AtomicIdCursor; @@ -117,7 +118,7 @@ type IdCursor = isize; /// [`World`]: crate::world::World #[derive(Clone, Copy, Eq, Hash, Ord, PartialEq, PartialOrd)] pub struct Entity { - generation: u32, + generation: NonZeroU32, index: u32, } @@ -129,8 +130,12 @@ pub(crate) enum AllocAtWithoutReplacement { impl Entity { #[cfg(test)] - pub(crate) const fn new(index: u32, generation: u32) -> Entity { - Entity { index, generation } + pub(crate) const fn new(index: u32, generation: u32) -> Result { + if let Some(generation) = NonZeroU32::new(generation) { + Ok(Entity { index, generation }) + } else { + Err("Failed to construct Entity, check that generation > 0") + } } /// An entity ID with a placeholder value. This may or may not correspond to an actual entity, @@ -184,7 +189,7 @@ impl Entity { pub const fn from_raw(index: u32) -> Entity { Entity { index, - generation: 0, + generation: NonZeroU32::MIN, } } @@ -195,16 +200,20 @@ impl Entity { /// /// No particular structure is guaranteed for the returned bits. pub const fn to_bits(self) -> u64 { - (self.generation as u64) << 32 | self.index as u64 + (self.generation.get() as u64) << 32 | self.index as u64 } /// Reconstruct an `Entity` previously destructured with [`Entity::to_bits`]. /// /// Only useful when applied to results from `to_bits` in the same instance of an application. - pub const fn from_bits(bits: u64) -> Self { - Self { - generation: (bits >> 32) as u32, - index: bits as u32, + pub const fn from_bits(bits: u64) -> Option { + if let Some(generation) = NonZeroU32::new((bits >> 32) as u32) { + Some(Self { + generation, + index: bits as u32, + }) + } else { + None } } @@ -223,7 +232,7 @@ impl Entity { /// given index has been reused (index, generation) pairs uniquely identify a given Entity. #[inline] pub const fn generation(self) -> u32 { - self.generation + self.generation.get() } } @@ -241,8 +250,9 @@ impl<'de> Deserialize<'de> for Entity { where D: serde::Deserializer<'de>, { + use serde::de::Error; let id: u64 = serde::de::Deserialize::deserialize(deserializer)?; - Ok(Entity::from_bits(id)) + Entity::from_bits(id).ok_or(D::Error::custom("Invalid generation bits")) } } @@ -289,7 +299,7 @@ impl<'a> Iterator for ReserveEntitiesIterator<'a> { }) .or_else(|| { self.index_range.next().map(|index| Entity { - generation: 0, + generation: NonZeroU32::MIN, index, }) }) @@ -437,7 +447,7 @@ impl Entities { // As `self.free_cursor` goes more and more negative, we return IDs farther // and farther beyond `meta.len()`. Entity { - generation: 0, + generation: NonZeroU32::MIN, index: u32::try_from(self.meta.len() as IdCursor - n).expect("too many entities"), } } @@ -466,7 +476,7 @@ impl Entities { let index = u32::try_from(self.meta.len()).expect("too many entities"); self.meta.push(EntityMeta::EMPTY); Entity { - generation: 0, + generation: NonZeroU32::MIN, index, } } @@ -553,7 +563,18 @@ impl Entities { if meta.generation != entity.generation { return None; } - meta.generation += 1; + + // NOTE: checked_add has more optimal instructions than NonZeroU32::checked_add or u32::wrapping_add + let new_generation = meta.generation.get().checked_add(1).unwrap_or(1); + // SAFETY: We use checked add to ensure that we wrap to 1, meaning it's safe to construct NonZeroU32 without checking + meta.generation = unsafe { NonZeroU32::new_unchecked(new_generation) }; + + if meta.generation == NonZeroU32::MIN { + warn!( + "Entity({}) generation wrapped on Entities::free, aliasing may occur", + entity.index + ); + } let loc = mem::replace(&mut meta.location, EntityMeta::EMPTY.location); @@ -583,7 +604,7 @@ impl Entities { // not reallocated since the generation is incremented in `free` pub fn contains(&self, entity: Entity) -> bool { self.resolve_from_id(entity.index()) - .map_or(false, |e| e.generation() == entity.generation) + .map_or(false, |e| e.generation() == entity.generation()) } /// Clears all [`Entity`] from the World. @@ -635,7 +656,13 @@ impl Entities { let meta = &mut self.meta[index as usize]; if meta.location.archetype_id == ArchetypeId::INVALID { - meta.generation += generations; + // TODO: wrapping add will make u32::MAX + 1 == u32::MAX + 2 + let new_generation = meta.generation.get().wrapping_add(generations).max(1); + if new_generation < meta.generation.get() { + warn!("Entity({index}) generation wrapped on Entities::reserve_generations, aliasing may occur"); + } + // SAFETY: We use u32::max to ensure that we wrap to 1, meaning it's safe to construct NonZeroU32 without checking + meta.generation = unsafe { NonZeroU32::new_unchecked(new_generation) }; true } else { false @@ -659,7 +686,7 @@ impl Entities { // Returning None handles that case correctly let num_pending = usize::try_from(-free_cursor).ok()?; (idu < self.meta.len() + num_pending).then_some(Entity { - generation: 0, + generation: NonZeroU32::MIN, index, }) } @@ -777,7 +804,7 @@ impl Entities { #[repr(C)] struct EntityMeta { /// The current generation of the [`Entity`]. - pub generation: u32, + pub generation: NonZeroU32, /// The current location of the [`Entity`] pub location: EntityLocation, } @@ -785,7 +812,7 @@ struct EntityMeta { impl EntityMeta { /// meta for **pending entity** const EMPTY: EntityMeta = EntityMeta { - generation: 0, + generation: NonZeroU32::MIN, location: EntityLocation::INVALID, }; } @@ -833,13 +860,21 @@ impl EntityLocation { mod tests { use super::*; + #[test] + fn entity_niche_optimization() { + assert_eq!( + std::mem::size_of::(), + std::mem::size_of::>() + ); + } + #[test] fn entity_bits_roundtrip() { let e = Entity { - generation: 0xDEADBEEF, + generation: NonZeroU32::new(0xDEADBEEF).unwrap(), index: 0xBAADF00D, }; - assert_eq!(Entity::from_bits(e.to_bits()), e); + assert_eq!(Entity::from_bits(e.to_bits()), Some(e)); } #[test] @@ -872,17 +907,21 @@ mod tests { #[test] fn entity_const() { const C1: Entity = Entity::from_raw(42); - assert_eq!(42, C1.index); - assert_eq!(0, C1.generation); + 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_00ff, C2.generation); + const C2: Option = Entity::from_bits(0x0000_00ff_0000_00cc); + assert_eq!(Some(0x0000_00cc), C2.map(|v| v.index())); + assert_eq!(Some(0x0000_00ff), C2.map(|v| v.generation())); const C3: u32 = Entity::from_raw(33).index(); assert_eq!(33, C3); - const C4: u32 = Entity::from_bits(0x00dd_00ff_0000_0000).generation(); + const C4: u32 = if let Some(entity) = Entity::from_bits(0x00dd_00ff_0000_0000) { + entity.generation() + } else { + panic!("Could not construct Entity from bits, check generation > 1") + }; assert_eq!(0x00dd_00ff, C4); } @@ -908,6 +947,6 @@ mod tests { // The very next entity allocated should be a further generation on the same index let next_entity = entities.alloc(); assert_eq!(next_entity.index(), entity.index()); - assert!(next_entity.generation > entity.generation + GENERATIONS); + assert!(next_entity.generation() > entity.generation() + GENERATIONS); } } diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 06001b584dbdc..460f4ca3e492d 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -1561,7 +1561,7 @@ mod tests { let e4 = world_b.spawn(A(4)).id(); assert_eq!( e4, - Entity::new(3, 0), + Entity::new(3, 1).unwrap(), "new entity is created immediately after world_a's max entity" ); assert!(world_b.get::(e1).is_none()); @@ -1592,7 +1592,7 @@ mod tests { "spawning into existing `world_b` entities works" ); - let e4_mismatched_generation = Entity::new(3, 1); + let e4_mismatched_generation = Entity::new(3, 2).unwrap(); assert!( world_b.get_or_spawn(e4_mismatched_generation).is_none(), "attempting to spawn on top of an entity with a mismatched entity generation fails" @@ -1608,7 +1608,7 @@ mod tests { "failed mismatched spawn doesn't change existing entity" ); - let high_non_existent_entity = Entity::new(6, 0); + let high_non_existent_entity = Entity::new(6, 1).unwrap(); world_b .get_or_spawn(high_non_existent_entity) .unwrap() @@ -1619,7 +1619,7 @@ mod tests { "inserting into newly allocated high / non-continuous entity id works" ); - let high_non_existent_but_reserved_entity = Entity::new(5, 0); + let high_non_existent_but_reserved_entity = Entity::new(5, 1).unwrap(); assert!( world_b.get_entity(high_non_existent_but_reserved_entity).is_none(), "entities between high-newly allocated entity and continuous block of existing entities don't exist" @@ -1635,10 +1635,10 @@ mod tests { assert_eq!( reserved_entities, vec![ - Entity::new(5, 0), - Entity::new(4, 0), - Entity::new(7, 0), - Entity::new(8, 0), + Entity::new(5, 1).unwrap(), + Entity::new(4, 1).unwrap(), + Entity::new(7, 1).unwrap(), + Entity::new(8, 1).unwrap(), ], "space between original entities and high entities is used for new entity ids" ); @@ -1687,7 +1687,7 @@ mod tests { let e0 = world.spawn(A(0)).id(); let e1 = Entity::from_raw(1); let e2 = world.spawn_empty().id(); - let invalid_e2 = Entity::new(e2.index(), 1); + let invalid_e2 = Entity::new(e2.index(), 2).unwrap(); let values = vec![(e0, (B(0), C)), (e1, (B(1), C)), (invalid_e2, (B(2), C))]; diff --git a/crates/bevy_scene/src/serde.rs b/crates/bevy_scene/src/serde.rs index c91d8084ff2db..5d9b564ed8656 100644 --- a/crates/bevy_scene/src/serde.rs +++ b/crates/bevy_scene/src/serde.rs @@ -529,18 +529,18 @@ mod tests { ), }, entities: { - 0: ( + 4294967296: ( components: { "bevy_scene::serde::tests::Foo": (123), }, ), - 1: ( + 4294967297: ( components: { "bevy_scene::serde::tests::Foo": (123), "bevy_scene::serde::tests::Bar": (345), }, ), - 2: ( + 4294967298: ( components: { "bevy_scene::serde::tests::Foo": (123), "bevy_scene::serde::tests::Bar": (345), @@ -566,18 +566,18 @@ mod tests { ), }, entities: { - 0: ( + 4294967296: ( components: { "bevy_scene::serde::tests::Foo": (123), }, ), - 1: ( + 4294967297: ( components: { "bevy_scene::serde::tests::Foo": (123), "bevy_scene::serde::tests::Bar": (345), }, ), - 2: ( + 4294967298: ( components: { "bevy_scene::serde::tests::Foo": (123), "bevy_scene::serde::tests::Bar": (345), @@ -687,10 +687,10 @@ mod tests { assert_eq!( vec![ - 0, 1, 0, 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 + 0, 1, 128, 128, 128, 128, 16, 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 ], serialized_scene ); @@ -727,11 +727,11 @@ mod tests { assert_eq!( vec![ - 146, 128, 129, 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, 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 ], buf ); @@ -768,7 +768,7 @@ 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, 0, 0, 0, 0, 1, 0, 0, 0, + 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, From 460a4ed6de83e72ae66e3efd5e8a92c74b5ace38 Mon Sep 17 00:00:00 2001 From: Natalie Baker Date: Mon, 25 Sep 2023 14:52:04 +0800 Subject: [PATCH 2/7] Change method of wrapping generation count --- crates/bevy_ecs/src/entity/map_entities.rs | 16 ++----- crates/bevy_ecs/src/entity/mod.rs | 51 +++++++++++++++++----- 2 files changed, 43 insertions(+), 24 deletions(-) diff --git a/crates/bevy_ecs/src/entity/map_entities.rs b/crates/bevy_ecs/src/entity/map_entities.rs index 6d17c33f7ba18..f86d96cb63dbc 100644 --- a/crates/bevy_ecs/src/entity/map_entities.rs +++ b/crates/bevy_ecs/src/entity/map_entities.rs @@ -1,8 +1,8 @@ -use std::num::NonZeroU32; - use crate::{entity::Entity, world::World}; use bevy_utils::HashMap; +use super::inc_generation_by; + /// Operation to map all contained [`Entity`] fields in a type to new values. /// /// As entity IDs are valid only for the [`World`] they're sourced from, using [`Entity`] @@ -72,17 +72,7 @@ impl<'m> EntityMapper<'m> { // this new entity reference is specifically designed to never represent any living entity let new = Entity { - // TODO: wrapping add will make u32::MAX + 1 == u32::MAX + 2 - // SAFETY: We use u32::max to ensure that we wrap to 1, meaning it's safe to construct NonZeroU32 without checking - generation: unsafe { - NonZeroU32::new_unchecked( - self.dead_start - .generation - .get() - .wrapping_add(self.generations) - .max(1), - ) - }, + generation: inc_generation_by(self.dead_start.generation, self.generations), index: self.dead_start.index, }; self.generations += 1; diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index e91c78e4a644f..443baedebfc24 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -564,10 +564,7 @@ impl Entities { return None; } - // NOTE: checked_add has more optimal instructions than NonZeroU32::checked_add or u32::wrapping_add - let new_generation = meta.generation.get().checked_add(1).unwrap_or(1); - // SAFETY: We use checked add to ensure that we wrap to 1, meaning it's safe to construct NonZeroU32 without checking - meta.generation = unsafe { NonZeroU32::new_unchecked(new_generation) }; + meta.generation = inc_generation_by(meta.generation, 1); if meta.generation == NonZeroU32::MIN { warn!( @@ -656,13 +653,7 @@ impl Entities { let meta = &mut self.meta[index as usize]; if meta.location.archetype_id == ArchetypeId::INVALID { - // TODO: wrapping add will make u32::MAX + 1 == u32::MAX + 2 - let new_generation = meta.generation.get().wrapping_add(generations).max(1); - if new_generation < meta.generation.get() { - warn!("Entity({index}) generation wrapped on Entities::reserve_generations, aliasing may occur"); - } - // SAFETY: We use u32::max to ensure that we wrap to 1, meaning it's safe to construct NonZeroU32 without checking - meta.generation = unsafe { NonZeroU32::new_unchecked(new_generation) }; + meta.generation = inc_generation_by(meta.generation, generations); true } else { false @@ -856,10 +847,48 @@ impl EntityLocation { }; } +pub(crate) const fn inc_generation_by(lhs: NonZeroU32, rhs: u32) -> NonZeroU32 { + let (lo, hi) = lhs.get().overflowing_add(rhs); + let ret = lo + hi as u32; + // SAFETY: + // - Adding the overflow flag will offet overflows to start at 1 instead of 0 + // - The sum of `NonZeroU32::MAX` + `u32::MAX` + 1 (overflow) == `NonZeroU32::MAX` + // - If the operation doesn't overflow, no offsetting takes place + unsafe { NonZeroU32::new_unchecked(ret) } +} + #[cfg(test)] mod tests { use super::*; + #[test] + fn inc_generation_by_is_safe() { + // Correct offsets without overflow + assert_eq!( + inc_generation_by(NonZeroU32::MIN, 1), + NonZeroU32::new(2).unwrap() + ); + + assert_eq!( + inc_generation_by(NonZeroU32::MIN, 10), + NonZeroU32::new(11).unwrap() + ); + + // Check that overflows start at 1 correctly + assert_eq!(inc_generation_by(NonZeroU32::MAX, 1), NonZeroU32::MIN); + + assert_eq!( + inc_generation_by(NonZeroU32::MAX, 2), + NonZeroU32::new(2).unwrap() + ); + + // Ensure we can't overflow twice + assert_eq!( + inc_generation_by(NonZeroU32::MAX, u32::MAX), + NonZeroU32::MAX + ); + } + #[test] fn entity_niche_optimization() { assert_eq!( From b1aac2a9abc8ca71e165abe001248a6ab5fadd54 Mon Sep 17 00:00:00 2001 From: Natalie Baker Date: Mon, 25 Sep 2023 14:53:38 +0800 Subject: [PATCH 3/7] Add comment --- crates/bevy_ecs/src/entity/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index 443baedebfc24..93d745a11eb5c 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -847,6 +847,7 @@ impl EntityLocation { }; } +/// Offsets a generation value by the specified amount, wrapping to 1 instead of 0 pub(crate) const fn inc_generation_by(lhs: NonZeroU32, rhs: u32) -> NonZeroU32 { let (lo, hi) = lhs.get().overflowing_add(rhs); let ret = lo + hi as u32; From d3b893b72c4af609a736dcc86574e59595769545 Mon Sep 17 00:00:00 2001 From: Natalie Baker Date: Wed, 15 Nov 2023 11:07:38 +0800 Subject: [PATCH 4/7] Fix rustdoc generation comment, swap to from_raw where appropriate --- crates/bevy_ecs/src/entity/map_entities.rs | 8 +++----- crates/bevy_ecs/src/entity/mod.rs | 2 +- crates/bevy_ecs/src/lib.rs | 14 +++++++------- 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/crates/bevy_ecs/src/entity/map_entities.rs b/crates/bevy_ecs/src/entity/map_entities.rs index f86d96cb63dbc..19f34830277bd 100644 --- a/crates/bevy_ecs/src/entity/map_entities.rs +++ b/crates/bevy_ecs/src/entity/map_entities.rs @@ -149,7 +149,7 @@ mod tests { let mut world = World::new(); let mut mapper = EntityMapper::new(&mut map, &mut world); - let mapped_ent = Entity::new(FIRST_IDX, 1).unwrap(); + let mapped_ent = Entity::from_raw(FIRST_IDX); let dead_ref = mapper.get_or_reserve(mapped_ent); assert_eq!( @@ -158,9 +158,7 @@ mod tests { "should persist the allocated mapping from the previous line" ); assert_eq!( - mapper - .get_or_reserve(Entity::new(SECOND_IDX, 1).unwrap()) - .index(), + mapper.get_or_reserve(Entity::from_raw(SECOND_IDX)).index(), dead_ref.index(), "should re-use the same index for further dead refs" ); @@ -178,7 +176,7 @@ mod tests { let mut world = World::new(); let dead_ref = EntityMapper::world_scope(&mut map, &mut world, |_, mapper| { - mapper.get_or_reserve(Entity::new(0, 1).unwrap()) + mapper.get_or_reserve(Entity::from_raw(0)) }); // 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 7296636639996..50b445cf94565 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -181,7 +181,7 @@ impl Entity { /// ``` pub const PLACEHOLDER: Self = Self::from_raw(u32::MAX); - /// Creates a new entity ID with the specified `index` and a generation of 0. + /// Creates a new entity ID with the specified `index` and a generation of 1. /// /// # Note /// diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 460f4ca3e492d..68dd44e9bca64 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -1561,7 +1561,7 @@ mod tests { let e4 = world_b.spawn(A(4)).id(); assert_eq!( e4, - Entity::new(3, 1).unwrap(), + Entity::from_raw(3), "new entity is created immediately after world_a's max entity" ); assert!(world_b.get::(e1).is_none()); @@ -1608,7 +1608,7 @@ mod tests { "failed mismatched spawn doesn't change existing entity" ); - let high_non_existent_entity = Entity::new(6, 1).unwrap(); + let high_non_existent_entity = Entity::from_raw(6); world_b .get_or_spawn(high_non_existent_entity) .unwrap() @@ -1619,7 +1619,7 @@ mod tests { "inserting into newly allocated high / non-continuous entity id works" ); - let high_non_existent_but_reserved_entity = Entity::new(5, 1).unwrap(); + let high_non_existent_but_reserved_entity = Entity::from_raw(5); assert!( world_b.get_entity(high_non_existent_but_reserved_entity).is_none(), "entities between high-newly allocated entity and continuous block of existing entities don't exist" @@ -1635,10 +1635,10 @@ mod tests { assert_eq!( reserved_entities, vec![ - Entity::new(5, 1).unwrap(), - Entity::new(4, 1).unwrap(), - Entity::new(7, 1).unwrap(), - Entity::new(8, 1).unwrap(), + Entity::from_raw(5), + Entity::from_raw(4), + Entity::from_raw(7), + Entity::from_raw(8), ], "space between original entities and high entities is used for new entity ids" ); From b5e9ec83df58125ecabd1ef930feffe1359b3985 Mon Sep 17 00:00:00 2001 From: Natalie Baker Date: Wed, 15 Nov 2023 11:55:16 +0800 Subject: [PATCH 5/7] Fix CI compile failure --- benches/benches/bevy_utils/entity_hash.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/benches/benches/bevy_utils/entity_hash.rs b/benches/benches/bevy_utils/entity_hash.rs index fa83ee3950d47..d492c5c33ccec 100644 --- a/benches/benches/bevy_utils/entity_hash.rs +++ b/benches/benches/bevy_utils/entity_hash.rs @@ -13,16 +13,16 @@ fn make_entity(rng: &mut impl Rng, size: usize) -> Entity { // -logâ‚‚(1-x) gives an exponential distribution with median 1.0 // That lets us get values that are mostly small, but some are quite large // * For ids, half are in [0, size), half are unboundedly larger. - // * For generations, half are in [0, 2), half are unboundedly larger. + // * For generations, half are in [1, 3), half are unboundedly larger. let x: f64 = rng.gen(); let id = -(1.0 - x).log2() * (size as f64); let x: f64 = rng.gen(); - let gen = -(1.0 - x).log2() * 2.0; + let gen = 1 + -(1.0 - x).log2() * 2.0; // this is not reliable, but we're internal so a hack is ok let bits = ((gen as u64) << 32) | (id as u64); - let e = Entity::from_bits(bits); + let e = Entity::from_bits(bits).unwrap(); assert_eq!(e.index(), id as u32); assert_eq!(e.generation(), gen as u32); e @@ -57,7 +57,7 @@ fn entity_set_build_and_lookup(c: &mut Criterion) { let set = EntityHashSet::from_iter(entities.iter().copied()); bencher.iter(|| entities.iter() .copied() - .map(|e| Entity::from_bits(e.to_bits() + 1)) + .map(|e| Entity::from_bits(e.to_bits() + 1).unwrap()) .filter(|e| set.contains(e)).count()); }, ); @@ -67,7 +67,7 @@ fn entity_set_build_and_lookup(c: &mut Criterion) { let set = EntityHashSet::from_iter(entities.iter().copied()); bencher.iter(|| entities.iter() .copied() - .map(|e| Entity::from_bits(e.to_bits() + (1 << 32))) + .map(|e| Entity::from_bits(e.to_bits() + (1 << 32)).unwrap()) .filter(|e| set.contains(e)).count()); }, ); From de0fe3a9e47406a3d2296c72e7c3742da6ff9280 Mon Sep 17 00:00:00 2001 From: Natalie Baker Date: Wed, 15 Nov 2023 12:06:18 +0800 Subject: [PATCH 6/7] Fix CI compile failure, for real this time --- benches/benches/bevy_utils/entity_hash.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/benches/benches/bevy_utils/entity_hash.rs b/benches/benches/bevy_utils/entity_hash.rs index d492c5c33ccec..19930a6695abc 100644 --- a/benches/benches/bevy_utils/entity_hash.rs +++ b/benches/benches/bevy_utils/entity_hash.rs @@ -18,7 +18,7 @@ fn make_entity(rng: &mut impl Rng, size: usize) -> Entity { let x: f64 = rng.gen(); let id = -(1.0 - x).log2() * (size as f64); let x: f64 = rng.gen(); - let gen = 1 + -(1.0 - x).log2() * 2.0; + let gen = 1.0 + -(1.0 - x).log2() * 2.0; // this is not reliable, but we're internal so a hack is ok let bits = ((gen as u64) << 32) | (id as u64); From a61c5258bc229284d3fd2416d2626103773df579 Mon Sep 17 00:00:00 2001 From: Natalie Baker Date: Tue, 5 Dec 2023 09:40:02 +0800 Subject: [PATCH 7/7] Swap to seperate failable from_bits impl to better match identifier PR --- benches/benches/bevy_utils/entity_hash.rs | 6 +-- crates/bevy_ecs/src/entity/mod.rs | 45 +++++++++++++++-------- 2 files changed, 33 insertions(+), 18 deletions(-) diff --git a/benches/benches/bevy_utils/entity_hash.rs b/benches/benches/bevy_utils/entity_hash.rs index 6f469df88f02f..44cf80ba9a4ec 100644 --- a/benches/benches/bevy_utils/entity_hash.rs +++ b/benches/benches/bevy_utils/entity_hash.rs @@ -22,7 +22,7 @@ fn make_entity(rng: &mut impl Rng, size: usize) -> Entity { // this is not reliable, but we're internal so a hack is ok let bits = ((gen as u64) << 32) | (id as u64); - let e = Entity::from_bits(bits).unwrap(); + let e = Entity::from_bits(bits); assert_eq!(e.index(), id as u32); assert_eq!(e.generation(), gen as u32); e @@ -52,7 +52,7 @@ fn entity_set_build_and_lookup(c: &mut Criterion) { entities .iter() .copied() - .map(|e| Entity::from_bits(e.to_bits() + 1).unwrap()) + .map(|e| Entity::from_bits(e.to_bits() + 1)) .filter(|e| set.contains(e)) .count() }); @@ -66,7 +66,7 @@ fn entity_set_build_and_lookup(c: &mut Criterion) { entities .iter() .copied() - .map(|e| Entity::from_bits(e.to_bits() + (1 << 32)).unwrap()) + .map(|e| Entity::from_bits(e.to_bits() + (1 << 32))) .filter(|e| set.contains(e)) .count() }); diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index df580c1bc45ec..8c5e1c1ca3122 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -264,19 +264,38 @@ impl Entity { (self.generation.get() as u64) << 32 | self.index as u64 } - /// Reconstruct an `Entity` previously destructured with [`Entity::to_bits`], will return - /// `None` if the parsed generation is an invalid value (ie. generation 0). + /// Reconstruct an `Entity` previously destructured with [`Entity::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 `u64` values that did not come from [`Entity::to_bits`] + #[inline] + pub const fn from_bits(bits: u64) -> Self { + // Construct an Identifier initially to extract the kind from. + let id = Self::try_from_bits(bits); + + match id { + Ok(entity) => entity, + Err(_) => panic!("Attempted to initialise invalid bits as an entity"), + } + } + + /// Reconstruct an `Entity` previously destructured with [`Entity::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 [`Entity::from_bits`]. #[inline(always)] - pub const fn from_bits(bits: u64) -> Option { + pub const fn try_from_bits(bits: u64) -> Result { if let Some(generation) = NonZeroU32::new((bits >> 32) as u32) { - Some(Self { + Ok(Self { generation, index: bits as u32, }) } else { - None + Err("Invalid generation bits") } } @@ -315,7 +334,7 @@ impl<'de> Deserialize<'de> for Entity { { use serde::de::Error; let id: u64 = serde::de::Deserialize::deserialize(deserializer)?; - Entity::from_bits(id).ok_or(D::Error::custom("Invalid generation bits")) + Entity::try_from_bits(id).map_err(D::Error::custom) } } @@ -966,7 +985,7 @@ mod tests { generation: NonZeroU32::new(0xDEADBEEF).unwrap(), index: 0xBAADF00D, }; - assert_eq!(Entity::from_bits(e.to_bits()), Some(e)); + assert_eq!(Entity::from_bits(e.to_bits()), e); } #[test] @@ -1002,18 +1021,14 @@ mod tests { assert_eq!(42, C1.index()); assert_eq!(1, C1.generation()); - const C2: Option = Entity::from_bits(0x0000_00ff_0000_00cc); - assert_eq!(Some(0x0000_00cc), C2.map(|v| v.index())); - assert_eq!(Some(0x0000_00ff), C2.map(|v| v.generation())); + const C2: Entity = Entity::from_bits(0x0000_00ff_0000_00cc); + assert_eq!(0x0000_00cc, C2.index()); + assert_eq!(0x0000_00ff, C2.generation()); const C3: u32 = Entity::from_raw(33).index(); assert_eq!(33, C3); - const C4: u32 = if let Some(entity) = Entity::from_bits(0x00dd_00ff_0000_0000) { - entity.generation() - } else { - panic!("Could not construct Entity from bits, check generation > 1") - }; + const C4: u32 = Entity::from_bits(0x00dd_00ff_0000_0000).generation(); assert_eq!(0x00dd_00ff, C4); }