From 57400994b24d3c39b8c2dd6650d57df97ffabe15 Mon Sep 17 00:00:00 2001 From: NotVeryMoe Date: Tue, 26 Oct 2021 22:26:29 +0800 Subject: [PATCH 01/10] Change entity generation to NonZeroU32, update tests, add new tests --- crates/bevy_ecs/src/entity/mod.rs | 99 +++++++++++++++++++++++++------ crates/bevy_ecs/src/lib.rs | 19 +++--- 2 files changed, 90 insertions(+), 28 deletions(-) diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index c74974d7a4457..4ceb98aa90b83 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -33,6 +33,7 @@ use std::{ convert::TryFrom, fmt, mem, sync::atomic::{AtomicI64, Ordering}, + num::NonZeroU32, }; /// Lightweight unique ID of an entity. @@ -46,7 +47,7 @@ use std::{ /// [`Query::get`](crate::system::Query::get) and related methods. #[derive(Clone, Copy, Hash, Eq, Ord, PartialEq, PartialOrd)] pub struct Entity { - pub(crate) generation: u32, + pub(crate) generation: NonZeroU32, pub(crate) id: u32, } @@ -57,7 +58,7 @@ pub enum AllocAtWithoutReplacement { } impl Entity { - /// Creates a new entity reference with a generation of 0. + /// Creates a new entity reference with a generation of 1. /// /// # Note /// @@ -66,7 +67,10 @@ impl Entity { /// only be used for sharing entities across apps, and only when they have a scheme /// worked out to share an ID space (which doesn't happen by default). pub fn new(id: u32) -> Entity { - Entity { id, generation: 0 } + Entity { + id, + generation: unsafe{ NonZeroU32::new_unchecked(1) } + } } /// Convert to a form convenient for passing outside of rust. @@ -76,17 +80,17 @@ impl Entity { /// /// No particular structure is guaranteed for the returned bits. pub fn to_bits(self) -> u64 { - u64::from(self.generation) << 32 | u64::from(self.id) + u64::from(self.generation.get()) << 32 | u64::from(self.id) } /// 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 fn from_bits(bits: u64) -> Self { - Self { - generation: (bits >> 32) as u32, + pub fn from_bits(bits: u64) -> Option { + Some(Self{ + generation: NonZeroU32::new((bits >> 32) as u32)?, id: bits as u32, - } + }) } /// Return a transiently unique identifier. @@ -104,7 +108,7 @@ impl Entity { /// given id has been reused (id, generation) pairs uniquely identify a given Entity. #[inline] pub fn generation(self) -> u32 { - self.generation + self.generation.get() } } @@ -147,7 +151,10 @@ impl<'a> Iterator for ReserveEntitiesIterator<'a> { generation: self.meta[id as usize].generation, id, }) - .or_else(|| self.id_range.next().map(|id| Entity { generation: 0, id })) + .or_else(|| self.id_range.next().map(|id| Entity { + generation: unsafe{ NonZeroU32::new_unchecked(1) }, + id, + })) } fn size_hint(&self) -> (usize, Option) { @@ -265,7 +272,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: unsafe{ NonZeroU32::new_unchecked(1) }, id: u32::try_from(self.meta.len() as i64 - n).expect("too many entities"), } } @@ -293,7 +300,10 @@ impl Entities { } else { let id = u32::try_from(self.meta.len()).expect("too many entities"); self.meta.push(EntityMeta::EMPTY); - Entity { generation: 0, id } + Entity { + generation: unsafe{ NonZeroU32::new_unchecked(1) }, + id, + } } } @@ -373,7 +383,12 @@ impl Entities { if meta.generation != entity.generation { return None; } - meta.generation += 1; + + meta.generation = unsafe{ NonZeroU32::new_unchecked( + meta.generation.get() + .checked_add(1) + .unwrap_or(1)) + }; let loc = mem::replace(&mut meta.location, EntityMeta::EMPTY.location); @@ -401,7 +416,7 @@ impl Entities { // not reallocated since the generation is incremented in `free` pub fn contains(&self, entity: Entity) -> bool { self.resolve_from_id(entity.id()) - .map_or(false, |e| e.generation() == entity.generation) + .map_or(false, |e| e.generation == entity.generation) } pub fn clear(&mut self) { @@ -442,7 +457,10 @@ impl Entities { // 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(|| Entity { generation: 0, id }) + (idu < self.meta.len() + num_pending).then(|| Entity { + generation: unsafe{ NonZeroU32::new_unchecked(1) }, + id + }) } } @@ -518,13 +536,13 @@ impl Entities { #[derive(Copy, Clone, Debug)] pub struct EntityMeta { - pub generation: u32, + pub generation: NonZeroU32, pub location: EntityLocation, } impl EntityMeta { const EMPTY: EntityMeta = EntityMeta { - generation: 0, + generation: unsafe{ NonZeroU32::new_unchecked(1) }, location: EntityLocation { archetype_id: ArchetypeId::INVALID, index: usize::MAX, // dummy value, to be filled in @@ -549,10 +567,53 @@ mod tests { #[test] fn entity_bits_roundtrip() { let e = Entity { - generation: 0xDEADBEEF, + generation: NonZeroU32::new(0xDEADBEEF).unwrap(), id: 0xBAADF00D, }; - assert_eq!(Entity::from_bits(e.to_bits()), e); + assert_eq!(Entity::from_bits(e.to_bits()).unwrap(), e); + } + + #[test] + fn entity_bad_bits() { + let bits: u64 = 0xBAADF00D; + assert_eq!(Entity::from_bits(bits), None); + } + + #[test] + fn entity_option_size_optimized() { + assert_eq!( + core::mem::size_of::>(), + core::mem::size_of::() + ); + } + + #[test] + fn entities_generation_increment() { + let mut entities = Entities::default(); + + let entity_old = entities.alloc(); + entities.free(entity_old); + let entity_new = entities.alloc(); + + assert_eq!(entity_old.id, entity_new.id ); + assert_eq!(entity_old.generation() + 1, entity_new.generation()); + } + + #[test] + fn entities_generation_wrap() { + let mut entities = Entities::default(); + let mut entity_old = entities.alloc(); + + // Modify generation on entity and entities to cause overflow on free + entity_old.generation = NonZeroU32::new(u32::MAX).unwrap(); + entities.meta[entity_old.id as usize].generation = entity_old.generation; + entities.free(entity_old); + + // Get just free-d entity back + let entity_new = entities.alloc(); + + assert_eq!(entity_old.id, entity_new.id); + assert_eq!(entity_new.generation(), 1); } #[test] diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 6fb347a2a7e8f..b96d15a2ca738 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -52,6 +52,7 @@ mod tests { }; use bevy_tasks::TaskPool; use parking_lot::Mutex; + use std::num::NonZeroU32; use std::{ any::TypeId, sync::{ @@ -1417,7 +1418,7 @@ mod tests { assert_eq!( e4, Entity { - generation: 0, + generation: NonZeroU32::new(1).unwrap(), id: 3, }, "new entity is created immediately after world_a's max entity" @@ -1451,7 +1452,7 @@ mod tests { ); let e4_mismatched_generation = Entity { - generation: 1, + generation: NonZeroU32::new(2).unwrap(), id: 3, }; assert!( @@ -1470,7 +1471,7 @@ mod tests { ); let high_non_existent_entity = Entity { - generation: 0, + generation: NonZeroU32::new(1).unwrap(), id: 6, }; world_b @@ -1484,7 +1485,7 @@ mod tests { ); let high_non_existent_but_reserved_entity = Entity { - generation: 0, + generation: NonZeroU32::new(1).unwrap(), id: 5, }; assert!( @@ -1503,19 +1504,19 @@ mod tests { reserved_entities, vec![ Entity { - generation: 0, + generation: NonZeroU32::new(1).unwrap(), id: 5 }, Entity { - generation: 0, + generation: NonZeroU32::new(1).unwrap(), id: 4 }, Entity { - generation: 0, + generation: NonZeroU32::new(1).unwrap(), id: 7, }, Entity { - generation: 0, + generation: NonZeroU32::new(1).unwrap(), id: 8, }, ], @@ -1567,7 +1568,7 @@ mod tests { let e1 = Entity::new(1); let e2 = world.spawn().id(); let invalid_e2 = Entity { - generation: 1, + generation: NonZeroU32::new(2).unwrap(), id: e2.id, }; From 7ad7eaae14b0bcaf08297df10dc0248c1829a4a3 Mon Sep 17 00:00:00 2001 From: NotVeryMoe Date: Wed, 27 Oct 2021 22:59:06 +0800 Subject: [PATCH 02/10] Remove unsafe code for NonZeroU32 in entity, run rust format, mark entity slots as dead instead of wrapping --- crates/bevy_ecs/src/entity/mod.rs | 121 ++++++++++++++++-------------- 1 file changed, 65 insertions(+), 56 deletions(-) diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index 4ceb98aa90b83..58c74a87689d1 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -32,8 +32,8 @@ use crate::{archetype::ArchetypeId, storage::SparseSetIndex}; use std::{ convert::TryFrom, fmt, mem, - sync::atomic::{AtomicI64, Ordering}, num::NonZeroU32, + sync::atomic::{AtomicI64, Ordering}, }; /// Lightweight unique ID of an entity. @@ -67,9 +67,9 @@ impl Entity { /// only be used for sharing entities across apps, and only when they have a scheme /// worked out to share an ID space (which doesn't happen by default). pub fn new(id: u32) -> Entity { - Entity { - id, - generation: unsafe{ NonZeroU32::new_unchecked(1) } + Entity { + id, + generation: GENERATION_ONE, } } @@ -80,14 +80,14 @@ impl Entity { /// /// No particular structure is guaranteed for the returned bits. pub fn to_bits(self) -> u64 { - u64::from(self.generation.get()) << 32 | u64::from(self.id) + u64::from(self.generation()) << 32 | u64::from(self.id) } /// 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 fn from_bits(bits: u64) -> Option { - Some(Self{ + Some(Self { generation: NonZeroU32::new((bits >> 32) as u32)?, id: bits as u32, }) @@ -148,13 +148,15 @@ impl<'a> Iterator for ReserveEntitiesIterator<'a> { self.id_iter .next() .map(|&id| Entity { - generation: self.meta[id as usize].generation, + generation: self.meta[id as usize].generation.unwrap(), id, }) - .or_else(|| self.id_range.next().map(|id| Entity { - generation: unsafe{ NonZeroU32::new_unchecked(1) }, - id, - })) + .or_else(|| { + self.id_range.next().map(|id| Entity { + generation: GENERATION_ONE, + id, + }) + }) } fn size_hint(&self) -> (usize, Option) { @@ -262,7 +264,7 @@ impl Entities { // Allocate from the freelist. let id = self.pending[(n - 1) as usize]; Entity { - generation: self.meta[id as usize].generation, + generation: self.meta[id as usize].generation.unwrap(), // Safe, meta from pending list, so generation is valid id, } } else { @@ -272,7 +274,7 @@ impl Entities { // As `self.free_cursor` goes more and more negative, we return IDs farther // and farther beyond `meta.len()`. Entity { - generation: unsafe{ NonZeroU32::new_unchecked(1) }, + generation: GENERATION_ONE, id: u32::try_from(self.meta.len() as i64 - n).expect("too many entities"), } } @@ -294,14 +296,14 @@ impl Entities { let new_free_cursor = self.pending.len() as i64; *self.free_cursor.get_mut() = new_free_cursor; Entity { - generation: self.meta[id as usize].generation, + generation: self.meta[id as usize].generation.unwrap(), // Safe, meta from pending list, so generation is valid id, } } else { let id = u32::try_from(self.meta.len()).expect("too many entities"); self.meta.push(EntityMeta::EMPTY); - Entity { - generation: unsafe{ NonZeroU32::new_unchecked(1) }, + Entity { + generation: GENERATION_ONE, id, } } @@ -334,7 +336,7 @@ impl Entities { )) }; - self.meta[entity.id as usize].generation = entity.generation; + self.meta[entity.id as usize].generation = Some(entity.generation); loc } @@ -362,14 +364,14 @@ impl Entities { let current_meta = &mut self.meta[entity.id as usize]; if current_meta.location.archetype_id == ArchetypeId::INVALID { AllocAtWithoutReplacement::DidNotExist - } else if current_meta.generation == entity.generation { + } else if current_meta.generation == Some(entity.generation) { AllocAtWithoutReplacement::Exists(current_meta.location) } else { return AllocAtWithoutReplacement::ExistsWithWrongGeneration; } }; - self.meta[entity.id as usize].generation = entity.generation; + self.meta[entity.id as usize].generation = Some(entity.generation); result } @@ -380,24 +382,21 @@ impl Entities { self.verify_flushed(); let meta = &mut self.meta[entity.id as usize]; - if meta.generation != entity.generation { + if meta.generation != Some(entity.generation) { return None; } - - meta.generation = unsafe{ NonZeroU32::new_unchecked( - meta.generation.get() - .checked_add(1) - .unwrap_or(1)) - }; - let loc = mem::replace(&mut meta.location, EntityMeta::EMPTY.location); + meta.generation = NonZeroU32::new(meta.generation.unwrap().get().wrapping_add(1)); - self.pending.push(entity.id); + if meta.generation.is_some() { + self.pending.push(entity.id); - let new_free_cursor = self.pending.len() as i64; - *self.free_cursor.get_mut() = new_free_cursor; - self.len -= 1; - Some(loc) + let new_free_cursor = self.pending.len() as i64; + *self.free_cursor.get_mut() = new_free_cursor; + self.len -= 1; + } + + Some(mem::replace(&mut meta.location, EntityMeta::EMPTY.location)) } /// Ensure at least `n` allocations can succeed without reallocating. @@ -412,8 +411,8 @@ impl Entities { } /// Returns true if the [`Entities`] contains [`entity`](Entity). - // This will return false for entities which have been freed, even if - // not reallocated since the generation is incremented in `free` + /// 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.id()) .map_or(false, |e| e.generation == entity.generation) @@ -430,7 +429,8 @@ impl Entities { pub fn get(&self, entity: Entity) -> Option { if (entity.id as usize) < self.meta.len() { let meta = &self.meta[entity.id as usize]; - if meta.generation != entity.generation + if meta.generation.is_none() + || meta.generation.unwrap() != entity.generation || meta.location.archetype_id == ArchetypeId::INVALID { return None; @@ -450,16 +450,16 @@ impl Entities { pub fn resolve_from_id(&self, id: u32) -> Option { let idu = id as usize; if let Some(&EntityMeta { generation, .. }) = self.meta.get(idu) { - Some(Entity { generation, id }) + generation.map(|generation| Entity { generation, id }) } 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(|| Entity { - generation: unsafe{ NonZeroU32::new_unchecked(1) }, - id + (idu < self.meta.len() + num_pending).then(|| Entity { + generation: GENERATION_ONE, + id, }) } } @@ -487,13 +487,15 @@ impl Entities { self.meta.resize(new_meta_len, EntityMeta::EMPTY); self.len += -current_free_cursor as u32; for (id, meta) in self.meta.iter_mut().enumerate().skip(old_meta_len) { - init( - Entity { - id: id as u32, - generation: meta.generation, - }, - &mut meta.location, - ); + if let Some(generation) = meta.generation { + init( + Entity { + id: id as u32, + generation, + }, + &mut meta.location, + ); + } } *free_cursor = 0; @@ -506,7 +508,7 @@ impl Entities { init( Entity { id, - generation: meta.generation, + generation: meta.generation.unwrap(), // Safe, meta from pending list, so generation is valid }, &mut meta.location, ); @@ -536,13 +538,13 @@ impl Entities { #[derive(Copy, Clone, Debug)] pub struct EntityMeta { - pub generation: NonZeroU32, + pub generation: Option, pub location: EntityLocation, } impl EntityMeta { const EMPTY: EntityMeta = EntityMeta { - generation: unsafe{ NonZeroU32::new_unchecked(1) }, + generation: Some(GENERATION_ONE), location: EntityLocation { archetype_id: ArchetypeId::INVALID, index: usize::MAX, // dummy value, to be filled in @@ -560,6 +562,14 @@ pub struct EntityLocation { pub index: usize, } +// Constant for the initial generation value, removes the need to unwrap in opt-0 code but doesn't +// require unsafe. When rust 1.57 drops, we can exchange [][1] for panic!(). +const GENERATION_ONE: NonZeroU32 = if let Some(gen) = NonZeroU32::new(1) { + gen +} else { + [][1] +}; + #[cfg(test)] mod tests { use super::*; @@ -582,7 +592,7 @@ mod tests { #[test] fn entity_option_size_optimized() { assert_eq!( - core::mem::size_of::>(), + core::mem::size_of::>(), core::mem::size_of::() ); } @@ -595,24 +605,23 @@ mod tests { entities.free(entity_old); let entity_new = entities.alloc(); - assert_eq!(entity_old.id, entity_new.id ); + assert_eq!(entity_old.id, entity_new.id); assert_eq!(entity_old.generation() + 1, entity_new.generation()); } #[test] - fn entities_generation_wrap() { + fn entities_generation_overflow() { let mut entities = Entities::default(); let mut entity_old = entities.alloc(); // Modify generation on entity and entities to cause overflow on free entity_old.generation = NonZeroU32::new(u32::MAX).unwrap(); - entities.meta[entity_old.id as usize].generation = entity_old.generation; + entities.meta[entity_old.id as usize].generation = Some(entity_old.generation); entities.free(entity_old); - // Get just free-d entity back + // Request new entity, we shouldn't get the one we just freed since it overflowed let entity_new = entities.alloc(); - - assert_eq!(entity_old.id, entity_new.id); + assert!(entity_old.id != entity_new.id); assert_eq!(entity_new.generation(), 1); } From 43e990da71bd0b47a447b58d37e757fbe234341b Mon Sep 17 00:00:00 2001 From: NotVeryMoe Date: Fri, 29 Oct 2021 21:48:13 +0800 Subject: [PATCH 03/10] Add entity generation exhaustion warning --- crates/bevy_ecs/src/entity/mod.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index 58c74a87689d1..c1e60965562f8 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -36,6 +36,8 @@ use std::{ sync::atomic::{AtomicI64, Ordering}, }; +use bevy_utils::tracing::warn; + /// Lightweight unique ID of an entity. /// /// Obtained from [`World::spawn`](crate::world::World::spawn), typically via @@ -394,6 +396,11 @@ impl Entities { let new_free_cursor = self.pending.len() as i64; *self.free_cursor.get_mut() = new_free_cursor; self.len -= 1; + } else { + warn!( + "Entity generation exhausted. Retiring slot for entity #{}", + entity.id + ); } Some(mem::replace(&mut meta.location, EntityMeta::EMPTY.location)) From e3784e528373d07948b24161f99ad4b0632fcbe1 Mon Sep 17 00:00:00 2001 From: NotVeryMoe Date: Sat, 30 Oct 2021 16:42:08 +0800 Subject: [PATCH 04/10] Address PR feedback to improve documentation Change generation.unwrap to generation.expect, add documentation for None generation's in entity meta, add documentation for wrapping_add of generations, revert doc comment change, fix generation comparison --- crates/bevy_ecs/src/entity/mod.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index c1e60965562f8..edbf4876f8305 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -266,7 +266,7 @@ impl Entities { // Allocate from the freelist. let id = self.pending[(n - 1) as usize]; Entity { - generation: self.meta[id as usize].generation.unwrap(), // Safe, meta from pending list, so generation is valid + generation: self.meta[id as usize].generation.expect("Attempt to re-use a retired entity slot, which has no generations remaining"), id, } } else { @@ -298,7 +298,7 @@ impl Entities { let new_free_cursor = self.pending.len() as i64; *self.free_cursor.get_mut() = new_free_cursor; Entity { - generation: self.meta[id as usize].generation.unwrap(), // Safe, meta from pending list, so generation is valid + generation: self.meta[id as usize].generation.expect("Attempt to re-use a retired entity slot, which has no generations remaining"), id, } } else { @@ -388,6 +388,9 @@ impl Entities { return None; } + // wrapping_add will cause the u32 to wrap around to 0 which NonZeroU32::new + // will be converted to None, marking the entity slot as retired and taking + // it out of circulation for re-allocation. meta.generation = NonZeroU32::new(meta.generation.unwrap().get().wrapping_add(1)); if meta.generation.is_some() { @@ -418,8 +421,8 @@ impl Entities { } /// Returns true if the [`Entities`] contains [`entity`](Entity). - /// This will return false for entities which have been freed, even if - /// not reallocated since the generation is incremented in `free` + // 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.id()) .map_or(false, |e| e.generation == entity.generation) @@ -436,8 +439,7 @@ impl Entities { pub fn get(&self, entity: Entity) -> Option { if (entity.id as usize) < self.meta.len() { let meta = &self.meta[entity.id as usize]; - if meta.generation.is_none() - || meta.generation.unwrap() != entity.generation + if meta.generation != Some(entity.generation) || meta.location.archetype_id == ArchetypeId::INVALID { return None; @@ -515,7 +517,7 @@ impl Entities { init( Entity { id, - generation: meta.generation.unwrap(), // Safe, meta from pending list, so generation is valid + generation: meta.generation.expect("Attempt to re-use a retired entity slot, which has no generations remaining"), }, &mut meta.location, ); @@ -545,6 +547,9 @@ impl Entities { #[derive(Copy, Clone, Debug)] pub struct EntityMeta { + /// A generation of `None` marks the slot as retired. This means that the slot + /// is invalid for re-allocation. Without this generations would be repeated, + /// and `Entity` wouldn't be able to act as a unique identifier. pub generation: Option, pub location: EntityLocation, } From 4767f0855f513b4a5e2d52848094bf9f6ab87a4c Mon Sep 17 00:00:00 2001 From: NotVeryMoe Date: Sat, 30 Oct 2021 17:00:28 +0800 Subject: [PATCH 05/10] Ran cargo format --- crates/bevy_ecs/src/entity/mod.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index edbf4876f8305..f241ae7de4839 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -266,7 +266,9 @@ impl Entities { // Allocate from the freelist. let id = self.pending[(n - 1) as usize]; Entity { - generation: self.meta[id as usize].generation.expect("Attempt to re-use a retired entity slot, which has no generations remaining"), + generation: self.meta[id as usize].generation.expect( + "Attempt to re-use a retired entity slot, which has no generations remaining", + ), id, } } else { @@ -298,7 +300,9 @@ impl Entities { let new_free_cursor = self.pending.len() as i64; *self.free_cursor.get_mut() = new_free_cursor; Entity { - generation: self.meta[id as usize].generation.expect("Attempt to re-use a retired entity slot, which has no generations remaining"), + generation: self.meta[id as usize].generation.expect( + "Attempt to re-use a retired entity slot, which has no generations remaining", + ), id, } } else { From 2e201a9e25191a6afd9b19194cf10422674a1f88 Mon Sep 17 00:00:00 2001 From: NotVeryMoe Date: Mon, 20 Dec 2021 22:27:10 +0800 Subject: [PATCH 06/10] Remove workaround for the now stablized const panic --- crates/bevy_ecs/src/entity/mod.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index f241ae7de4839..6b0080e383a49 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -578,12 +578,11 @@ pub struct EntityLocation { pub index: usize, } -// Constant for the initial generation value, removes the need to unwrap in opt-0 code but doesn't -// require unsafe. When rust 1.57 drops, we can exchange [][1] for panic!(). +// Constant for the initial generation value, removes the need to unwrap in opt-0 code but doesn't require unsafe. const GENERATION_ONE: NonZeroU32 = if let Some(gen) = NonZeroU32::new(1) { gen } else { - [][1] + panic!("Failed to unwrap GENERATION_ONE constant") }; #[cfg(test)] From ab84dffa787117851d701e4033584940b4afaada Mon Sep 17 00:00:00 2001 From: NotVeryMoe Date: Tue, 21 Dec 2021 17:52:16 +0800 Subject: [PATCH 07/10] Change to unreachable --- crates/bevy_ecs/src/entity/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index 6b0080e383a49..8b1105c87ab8f 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -582,7 +582,7 @@ pub struct EntityLocation { const GENERATION_ONE: NonZeroU32 = if let Some(gen) = NonZeroU32::new(1) { gen } else { - panic!("Failed to unwrap GENERATION_ONE constant") + unreachable!() }; #[cfg(test)] From 829ae665dd079f635c23b2bee0363f50a821afda Mon Sep 17 00:00:00 2001 From: Natalie Baker Date: Sat, 11 Mar 2023 11:01:05 +0800 Subject: [PATCH 08/10] Update to the latest version, change parent to take Option --- crates/bevy_animation/src/lib.rs | 6 +- crates/bevy_ecs/src/entity/mod.rs | 198 ++++++++++-------- crates/bevy_ecs/src/lib.rs | 19 +- crates/bevy_hierarchy/src/child_builder.rs | 56 ++--- .../bevy_hierarchy/src/components/parent.rs | 30 ++- crates/bevy_hierarchy/src/hierarchy.rs | 2 +- crates/bevy_hierarchy/src/query_extension.rs | 2 +- .../src/valid_parent_check_plugin.rs | 19 +- crates/bevy_render/src/view/visibility/mod.rs | 2 +- crates/bevy_sprite/src/render/mod.rs | 2 +- crates/bevy_transform/src/systems.rs | 4 +- crates/bevy_ui/src/flex/mod.rs | 4 +- crates/bevy_winit/src/accessibility.rs | 4 +- examples/animation/gltf_skinned_mesh.rs | 27 +-- 14 files changed, 203 insertions(+), 172 deletions(-) diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs index 7312a5fcf659f..c95beda114e3a 100644 --- a/crates/bevy_animation/src/lib.rs +++ b/crates/bevy_animation/src/lib.rs @@ -319,14 +319,14 @@ fn verify_no_ancestor_player( player_parent: Option<&Parent>, parents: &Query<(Option>, Option<&Parent>)>, ) -> bool { - let Some(mut current) = player_parent.map(Parent::get) else { return true }; + let Some(mut current) = player_parent.and_then(Parent::try_get) else { return true }; loop { let Ok((maybe_player, parent)) = parents.get(current) else { return true }; if maybe_player.is_some() { return false; } - if let Some(parent) = parent { - current = parent.get(); + if let Some(parent) = parent.and_then(|v| v.try_get()) { + current = parent; } else { return true; } diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index 1b1afe3151fc6..f6c5a51fee359 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -37,6 +37,7 @@ //! [`EntityMut::remove`]: crate::world::EntityMut::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, sync::atomic::Ordering, num::NonZeroU32}; #[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, Deserialize, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)] pub struct Entity { - generation: u32, + generation: NonZeroU32, index: u32, } @@ -129,46 +130,10 @@ pub(crate) enum AllocAtWithoutReplacement { impl Entity { #[cfg(test)] - pub(crate) const fn new(index: u32, generation: u32) -> Entity { + pub(crate) const fn new(index: u32, generation: NonZeroU32) -> Entity { Entity { index, generation } } - /// An entity ID with a placeholder value. This may or may not correspond to an actual entity, - /// and should be overwritten by a new value before being used. - /// - /// ## Examples - /// - /// Initializing a collection (e.g. `array` or `Vec`) with a known size: - /// - /// ```no_run - /// # use bevy_ecs::prelude::*; - /// // Create a new array of size 10 filled with invalid entity ids. - /// let mut entities: [Entity; 10] = [Entity::PLACEHOLDER; 10]; - /// - /// // ... replace the entities with valid ones. - /// ``` - /// - /// Deriving `Reflect` for a component that has an `Entity` field: - /// - /// ```no_run - /// # use bevy_ecs::{prelude::*, component::*}; - /// # use bevy_reflect::Reflect; - /// #[derive(Reflect, Component)] - /// #[reflect(Component)] - /// pub struct MyStruct { - /// pub entity: Entity, - /// } - /// - /// impl FromWorld for MyStruct { - /// fn from_world(_world: &mut World) -> Self { - /// Self { - /// entity: Entity::PLACEHOLDER, - /// } - /// } - /// } - /// ``` - pub const PLACEHOLDER: Self = Self::from_raw(u32::MAX); - /// Creates a new entity ID with the specified `index` and a generation of 0. /// /// # Note @@ -184,7 +149,7 @@ impl Entity { pub const fn from_raw(index: u32) -> Entity { Entity { index, - generation: 0, + generation: GENERATION_ONE, } } @@ -195,16 +160,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() 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 +192,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() } } @@ -263,12 +232,12 @@ impl<'a> Iterator for ReserveEntitiesIterator<'a> { self.index_iter .next() .map(|&index| Entity { - generation: self.meta[index as usize].generation, + generation: self.meta[index as usize].generation.unwrap(), index, }) .or_else(|| { self.index_range.next().map(|index| Entity { - generation: 0, + generation: GENERATION_ONE, index, }) }) @@ -401,7 +370,9 @@ impl Entities { // Allocate from the freelist. let index = self.pending[(n - 1) as usize]; Entity { - generation: self.meta[index as usize].generation, + generation: self.meta[index as usize].generation.expect( + "Attempt to re-use a retired entity slot, which has no generations remaining", + ), index, } } else { @@ -411,7 +382,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: GENERATION_ONE, index: u32::try_from(self.meta.len() as IdCursor - n).expect("too many entities"), } } @@ -433,14 +404,14 @@ impl Entities { let new_free_cursor = self.pending.len() as IdCursor; *self.free_cursor.get_mut() = new_free_cursor; Entity { - generation: self.meta[index as usize].generation, + generation: self.meta[index as usize].generation.unwrap(), index, } } else { let index = u32::try_from(self.meta.len()).expect("too many entities"); self.meta.push(EntityMeta::EMPTY); Entity { - generation: 0, + generation: GENERATION_ONE, index, } } @@ -474,7 +445,7 @@ impl Entities { )) }; - self.meta[entity.index as usize].generation = entity.generation; + self.meta[entity.index as usize].generation = Some(entity.generation); loc } @@ -506,14 +477,14 @@ impl Entities { let current_meta = &self.meta[entity.index as usize]; if current_meta.location.archetype_id == ArchetypeId::INVALID { AllocAtWithoutReplacement::DidNotExist - } else if current_meta.generation == entity.generation { + } else if current_meta.generation == Some(entity.generation) { AllocAtWithoutReplacement::Exists(current_meta.location) } else { return AllocAtWithoutReplacement::ExistsWithWrongGeneration; } }; - self.meta[entity.index as usize].generation = entity.generation; + self.meta[entity.index as usize].generation = Some(entity.generation); result } @@ -524,19 +495,29 @@ impl Entities { self.verify_flushed(); let meta = &mut self.meta[entity.index as usize]; - if meta.generation != entity.generation { + if meta.generation != Some(entity.generation) { return None; } - meta.generation += 1; - let loc = mem::replace(&mut meta.location, EntityMeta::EMPTY.location); + // wrapping_add will cause the u32 to wrap around to 0 which NonZeroU32::new + // will be converted to None, marking the entity slot as retired and taking + // it out of circulation for re-allocation. + meta.generation = NonZeroU32::new(meta.generation.unwrap().get().wrapping_add(1)); + + if meta.generation.is_some() { + self.pending.push(entity.index); - self.pending.push(entity.index); + let new_free_cursor = self.pending.len() as i64; + *self.free_cursor.get_mut() = new_free_cursor; + self.len -= 1; + } else { + warn!( + "Entity generation exhausted. Retiring slot for entity #{}", + entity.index + ); + } - let new_free_cursor = self.pending.len() as IdCursor; - *self.free_cursor.get_mut() = new_free_cursor; - self.len -= 1; - Some(loc) + Some(mem::replace(&mut meta.location, EntityMeta::EMPTY.location)) } /// Ensure at least `n` allocations can succeed without reallocating. @@ -557,7 +538,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. @@ -573,7 +554,7 @@ impl Entities { pub fn get(&self, entity: Entity) -> Option { if (entity.index as usize) < self.meta.len() { let meta = &self.meta[entity.index as usize]; - if meta.generation != entity.generation + if meta.generation != Some(entity.generation) || meta.location.archetype_id == ArchetypeId::INVALID { return None; @@ -605,7 +586,7 @@ impl Entities { pub fn resolve_from_id(&self, index: u32) -> Option { let idu = index as usize; if let Some(&EntityMeta { generation, .. }) = self.meta.get(idu) { - Some(Entity { generation, index }) + generation.map(|generation| Entity { generation, index }) } 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); @@ -613,7 +594,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: GENERATION_ONE, index, }) } @@ -645,13 +626,15 @@ impl Entities { self.meta.resize(new_meta_len, EntityMeta::EMPTY); self.len += -current_free_cursor as u32; for (index, meta) in self.meta.iter_mut().enumerate().skip(old_meta_len) { - init( - Entity { - index: index as u32, - generation: meta.generation, - }, - &mut meta.location, - ); + if let Some(generation) = meta.generation { + init( + Entity { + index: index as u32, + generation, + }, + &mut meta.location, + ); + } } *free_cursor = 0; @@ -664,7 +647,7 @@ impl Entities { init( Entity { index, - generation: meta.generation, + generation: meta.generation.expect("Attempt to re-use a retired entity slot, which has no generations remaining"), }, &mut meta.location, ); @@ -731,7 +714,7 @@ impl Entities { #[repr(C)] struct EntityMeta { /// The current generation of the [`Entity`]. - pub generation: u32, + pub generation: Option, /// The current location of the [`Entity`] pub location: EntityLocation, } @@ -739,7 +722,7 @@ struct EntityMeta { impl EntityMeta { /// meta for **pending entity** const EMPTY: EntityMeta = EntityMeta { - generation: 0, + generation: Some(GENERATION_ONE), location: EntityLocation::INVALID, }; } @@ -783,6 +766,13 @@ impl EntityLocation { }; } +// Constant for the initial generation value, removes the need to unwrap in opt-0 code but doesn't require unsafe. +const GENERATION_ONE: NonZeroU32 = if let Some(gen) = NonZeroU32::new(1) { + gen +} else { + unreachable!() +}; + #[cfg(test)] mod tests { use super::*; @@ -790,10 +780,10 @@ mod tests { #[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] @@ -827,16 +817,58 @@ mod tests { fn entity_const() { const C1: Entity = Entity::from_raw(42); assert_eq!(42, C1.index); - assert_eq!(0, C1.generation); + assert_eq!(0, C1.generation()); - const C2: Entity = Entity::from_bits(0x0000_00ff_0000_00cc); + const C2: Entity = Entity::from_bits(0x0000_00ff_0000_00cc).unwrap(); assert_eq!(0x0000_00cc, C2.index); - assert_eq!(0x0000_00ff, C2.generation); + assert_eq!(0x0000_00ff, C2.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 = Entity::from_bits(0x00dd_00ff_0000_0000).unwrap().generation(); assert_eq!(0x00dd_00ff, C4); } + + #[test] + fn entity_bad_bits() { + let bits: u64 = 0xBAADF00D; + assert_eq!(Entity::from_bits(bits), None); + } + + #[test] + fn entity_option_size_optimized() { + assert_eq!( + core::mem::size_of::>(), + core::mem::size_of::() + ); + } + + #[test] + fn entities_generation_increment() { + let mut entities = Entities::default(); + + let entity_old = entities.alloc(); + entities.free(entity_old); + let entity_new = entities.alloc(); + + assert_eq!(entity_old.id, entity_new.id); + assert_eq!(entity_old.generation() + 1, entity_new.generation()); + } + + #[test] + fn entities_generation_overflow() { + let mut entities = Entities::default(); + let mut entity_old = entities.alloc(); + + // Modify generation on entity and entities to cause overflow on free + entity_old.generation = NonZeroU32::new(u32::MAX).unwrap(); + entities.meta[entity_old.id as usize].generation = Some(entity_old.generation); + entities.free(entity_old); + + // Request new entity, we shouldn't get the one we just freed since it overflowed + let entity_new = entities.alloc(); + assert!(entity_old.id != entity_new.id); + assert_eq!(entity_new.generation(), 1); + } } diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 1c574befe7065..65ed615deeaef 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -72,6 +72,7 @@ mod tests { world::{Mut, World}, }; use bevy_tasks::{ComputeTaskPool, TaskPool}; + use std::num::NonZeroU32; use std::{ any::TypeId, marker::PhantomData, @@ -1544,7 +1545,7 @@ mod tests { let e4 = world_b.spawn(A(4)).id(); assert_eq!( e4, - Entity::new(3, 0), + Entity::new(3, NonZeroU32::new(1).unwrap()), "new entity is created immediately after world_a's max entity" ); assert!(world_b.get::(e1).is_none()); @@ -1575,7 +1576,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, NonZeroU32::new(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" @@ -1591,7 +1592,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, NonZeroU32::new(1).unwrap()); world_b .get_or_spawn(high_non_existent_entity) .unwrap() @@ -1602,7 +1603,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, NonZeroU32::new(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" @@ -1618,10 +1619,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, NonZeroU32::new(1).unwrap()), + Entity::new(4, NonZeroU32::new(1).unwrap()), + Entity::new(7, NonZeroU32::new(1).unwrap()), + Entity::new(8, NonZeroU32::new(1).unwrap()), ], "space between original entities and high entities is used for new entity ids" ); @@ -1670,7 +1671,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(), NonZeroU32::new(2).unwrap()); let values = vec![(e0, (B(0), C)), (e1, (B(1), C)), (invalid_e2, (B(2), C))]; diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index 2d899686d565e..9aee89beac26c 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -28,11 +28,11 @@ fn push_child_unchecked(world: &mut World, parent: Entity, child: Entity) { fn update_parent(world: &mut World, child: Entity, new_parent: Entity) -> Option { let mut child = world.entity_mut(child); if let Some(mut parent) = child.get_mut::() { - let previous = parent.0; - *parent = Parent(new_parent); - Some(previous) + let previous = parent.try_get(); + *parent = Parent::new(new_parent); + previous } else { - child.insert(Parent(new_parent)); + child.insert(Parent::new(new_parent)); None } } @@ -399,7 +399,7 @@ pub struct WorldChildBuilder<'w> { impl<'w> WorldChildBuilder<'w> { /// Spawns an entity with the given bundle and inserts it into the children defined by the [`WorldChildBuilder`] pub fn spawn(&mut self, bundle: impl Bundle + Send + Sync + 'static) -> EntityMut<'_> { - let entity = self.world.spawn((bundle, Parent(self.parent))).id(); + let entity = self.world.spawn((bundle, Parent::new(self.parent))).id(); push_child_unchecked(self.world, self.parent, entity); push_events( self.world, @@ -413,7 +413,7 @@ impl<'w> WorldChildBuilder<'w> { /// Spawns an [`Entity`] with no components and inserts it into the children defined by the [`WorldChildBuilder`] which adds the [`Parent`] component to it. pub fn spawn_empty(&mut self) -> EntityMut<'_> { - let entity = self.world.spawn(Parent(self.parent)).id(); + let entity = self.world.spawn(Parent::new(self.parent)).id(); push_child_unchecked(self.world, self.parent, entity); push_events( self.world, @@ -532,7 +532,7 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { fn remove_parent(&mut self) -> &mut Self { let child = self.id(); - if let Some(parent) = self.take::().map(|p| p.get()) { + if let Some(parent) = self.take::().and_then(|p| p.try_get()) { self.world_scope(|world| { remove_from_children(world, parent, child); push_events(world, [HierarchyEvent::ChildRemoved { child, parent }]); @@ -561,7 +561,7 @@ mod tests { /// Assert the (non)existence and state of the child's [`Parent`] component. fn assert_parent(world: &mut World, child: Entity, parent: Option) { - assert_eq!(world.get::(child).map(|p| p.get()), parent); + assert_eq!(world.get::(child).and_then(|p| p.try_get()), parent); } /// Assert the (non)existence and state of the parent's [`Children`] component. @@ -711,11 +711,11 @@ mod tests { world.get::(parent).unwrap().0.as_slice(), children.as_slice(), ); - assert_eq!(*world.get::(children[0]).unwrap(), Parent(parent)); - assert_eq!(*world.get::(children[1]).unwrap(), Parent(parent)); + assert_eq!(*world.get::(children[0]).unwrap(), Parent::new(parent)); + assert_eq!(*world.get::(children[1]).unwrap(), Parent::new(parent)); - assert_eq!(*world.get::(children[0]).unwrap(), Parent(parent)); - assert_eq!(*world.get::(children[1]).unwrap(), Parent(parent)); + assert_eq!(*world.get::(children[0]).unwrap(), Parent::new(parent)); + assert_eq!(*world.get::(children[1]).unwrap(), Parent::new(parent)); } #[test] @@ -743,11 +743,11 @@ mod tests { world.get::(parent).unwrap().0.clone(), expected_children ); - assert_eq!(*world.get::(child1).unwrap(), Parent(parent)); - assert_eq!(*world.get::(child2).unwrap(), Parent(parent)); + assert_eq!(*world.get::(child1).unwrap(), Parent::new(parent)); + assert_eq!(*world.get::(child2).unwrap(), Parent::new(parent)); - assert_eq!(*world.get::(child1).unwrap(), Parent(parent)); - assert_eq!(*world.get::(child2).unwrap(), Parent(parent)); + assert_eq!(*world.get::(child1).unwrap(), Parent::new(parent)); + assert_eq!(*world.get::(child2).unwrap(), Parent::new(parent)); { let mut commands = Commands::new(&mut queue, &world); @@ -760,8 +760,8 @@ mod tests { world.get::(parent).unwrap().0.clone(), expected_children ); - assert_eq!(*world.get::(child3).unwrap(), Parent(parent)); - assert_eq!(*world.get::(child4).unwrap(), Parent(parent)); + assert_eq!(*world.get::(child3).unwrap(), Parent::new(parent)); + assert_eq!(*world.get::(child4).unwrap(), Parent::new(parent)); let remove_children = [child1, child4]; { @@ -802,8 +802,8 @@ mod tests { world.get::(parent).unwrap().0.clone(), expected_children ); - assert_eq!(*world.get::(child1).unwrap(), Parent(parent)); - assert_eq!(*world.get::(child2).unwrap(), Parent(parent)); + assert_eq!(*world.get::(child1).unwrap(), Parent::new(parent)); + assert_eq!(*world.get::(child2).unwrap(), Parent::new(parent)); { let mut commands = Commands::new(&mut queue, &world); @@ -841,8 +841,8 @@ mod tests { world.get::(parent).unwrap().0.clone(), expected_children ); - assert_eq!(*world.get::(child1).unwrap(), Parent(parent)); - assert_eq!(*world.get::(child2).unwrap(), Parent(parent)); + assert_eq!(*world.get::(child1).unwrap(), Parent::new(parent)); + assert_eq!(*world.get::(child2).unwrap(), Parent::new(parent)); let replace_children = [child1, child4]; { @@ -856,8 +856,8 @@ mod tests { world.get::(parent).unwrap().0.clone(), expected_children ); - assert_eq!(*world.get::(child1).unwrap(), Parent(parent)); - assert_eq!(*world.get::(child4).unwrap(), Parent(parent)); + assert_eq!(*world.get::(child1).unwrap(), Parent::new(parent)); + assert_eq!(*world.get::(child4).unwrap(), Parent::new(parent)); assert!(world.get::(child2).is_none()); } @@ -881,8 +881,8 @@ mod tests { world.get::(parent).unwrap().0.clone(), expected_children ); - assert_eq!(*world.get::(child1).unwrap(), Parent(parent)); - assert_eq!(*world.get::(child2).unwrap(), Parent(parent)); + assert_eq!(*world.get::(child1).unwrap(), Parent::new(parent)); + assert_eq!(*world.get::(child2).unwrap(), Parent::new(parent)); world.entity_mut(parent).insert_children(1, &entities[3..]); let expected_children: SmallVec<[Entity; 8]> = smallvec![child1, child3, child4, child2]; @@ -890,8 +890,8 @@ mod tests { world.get::(parent).unwrap().0.clone(), expected_children ); - assert_eq!(*world.get::(child3).unwrap(), Parent(parent)); - assert_eq!(*world.get::(child4).unwrap(), Parent(parent)); + assert_eq!(*world.get::(child3).unwrap(), Parent::new(parent)); + assert_eq!(*world.get::(child4).unwrap(), Parent::new(parent)); let remove_children = [child1, child4]; world.entity_mut(parent).remove_children(&remove_children); diff --git a/crates/bevy_hierarchy/src/components/parent.rs b/crates/bevy_hierarchy/src/components/parent.rs index 3e573be5a6952..1f2712f765ddf 100644 --- a/crates/bevy_hierarchy/src/components/parent.rs +++ b/crates/bevy_hierarchy/src/components/parent.rs @@ -2,7 +2,6 @@ use bevy_ecs::{ component::Component, entity::{Entity, EntityMap, MapEntities, MapEntitiesError}, reflect::{ReflectComponent, ReflectMapEntities}, - world::{FromWorld, World}, }; use bevy_reflect::Reflect; use std::ops::Deref; @@ -14,24 +13,19 @@ use std::ops::Deref; /// /// [`HierarchyQueryExt`]: crate::query_extension::HierarchyQueryExt /// [`Query`]: bevy_ecs::system::Query -#[derive(Component, Debug, Eq, PartialEq, Reflect)] +#[derive(Component, Debug, Default, Eq, PartialEq, Reflect)] #[reflect(Component, MapEntities, PartialEq)] -pub struct Parent(pub(crate) Entity); +pub struct Parent(Option); impl Parent { - /// Gets the [`Entity`] ID of the parent. - pub fn get(&self) -> Entity { - self.0 + + pub(crate) fn new(entity: Entity) -> Self { + Self(Some(entity)) } -} -// TODO: We need to impl either FromWorld or Default so Parent can be registered as Reflect. -// This is because Reflect deserialize by creating an instance and apply a patch on top. -// However Parent should only ever be set with a real user-defined entity. Its worth looking into -// better ways to handle cases like this. -impl FromWorld for Parent { - fn from_world(_world: &mut World) -> Self { - Parent(Entity::PLACEHOLDER) + /// Gets the [`Entity`] ID of the parent. + pub fn try_get(&self) -> Option { + self.0 } } @@ -39,15 +33,17 @@ impl MapEntities for Parent { fn map_entities(&mut self, entity_map: &EntityMap) -> Result<(), MapEntitiesError> { // Parent of an entity in the new world can be in outside world, in which case it // should not be mapped. - if let Ok(mapped_entity) = entity_map.get(self.0) { - self.0 = mapped_entity; + if let Some(entity) = self.0 { + if let Ok(mapped_entity) = entity_map.get(entity) { + self.0 = Some(mapped_entity); + } } Ok(()) } } impl Deref for Parent { - type Target = Entity; + type Target = Option; fn deref(&self) -> &Self::Target { &self.0 diff --git a/crates/bevy_hierarchy/src/hierarchy.rs b/crates/bevy_hierarchy/src/hierarchy.rs index db1a67b5e88b1..d75f457847d64 100644 --- a/crates/bevy_hierarchy/src/hierarchy.rs +++ b/crates/bevy_hierarchy/src/hierarchy.rs @@ -23,7 +23,7 @@ pub struct DespawnChildrenRecursive { /// Function for despawning an entity and all its children pub fn despawn_with_children_recursive(world: &mut World, entity: Entity) { // first, make the entity's own parent forget about it - if let Some(parent) = world.get::(entity).map(|parent| parent.0) { + if let Some(parent) = world.get::(entity).and_then(|parent| parent.try_get()) { if let Some(mut children) = world.get_mut::(parent) { children.0.retain(|c| *c != entity); } diff --git a/crates/bevy_hierarchy/src/query_extension.rs b/crates/bevy_hierarchy/src/query_extension.rs index 6ee6dc26fc2fd..8e3c1c30c93da 100644 --- a/crates/bevy_hierarchy/src/query_extension.rs +++ b/crates/bevy_hierarchy/src/query_extension.rs @@ -150,7 +150,7 @@ where type Item = Entity; fn next(&mut self) -> Option { - self.next = self.parent_query.get(self.next?).ok().map(|p| p.get()); + self.next = self.parent_query.get(self.next?).ok().and_then(|p| p.try_get()); self.next } } diff --git a/crates/bevy_hierarchy/src/valid_parent_check_plugin.rs b/crates/bevy_hierarchy/src/valid_parent_check_plugin.rs index 557450f9af39d..55fe059216c3a 100644 --- a/crates/bevy_hierarchy/src/valid_parent_check_plugin.rs +++ b/crates/bevy_hierarchy/src/valid_parent_check_plugin.rs @@ -62,15 +62,16 @@ pub fn check_hierarchy_component_has_valid_parent( mut already_diagnosed: Local>, ) { for (entity, parent, name) in &parent_query { - let parent = parent.get(); - if !component_query.contains(parent) && !already_diagnosed.contains(&entity) { - already_diagnosed.insert(entity); - warn!( - "warning[B0004]: {name} with the {ty_name} component has a parent without {ty_name}.\n\ - This will cause inconsistent behaviors! See https://bevyengine.org/learn/errors/#b0004", - ty_name = get_short_name(std::any::type_name::()), - name = name.map_or("An entity".to_owned(), |s| format!("The {s} entity")), - ); + if let Some(parent) = parent.try_get() { + if !component_query.contains(parent) && !already_diagnosed.contains(&entity) { + already_diagnosed.insert(entity); + warn!( + "warning[B0004]: {name} with the {ty_name} component has a parent without {ty_name}.\n\ + This will cause inconsistent behaviors! See https://bevyengine.org/learn/errors/#b0004", + ty_name = get_short_name(std::any::type_name::()), + name = name.map_or("An entity".to_owned(), |s| format!("The {s} entity")), + ); + } } } } diff --git a/crates/bevy_render/src/view/visibility/mod.rs b/crates/bevy_render/src/view/visibility/mod.rs index ca5e8a9df2503..db251aa83779b 100644 --- a/crates/bevy_render/src/view/visibility/mod.rs +++ b/crates/bevy_render/src/view/visibility/mod.rs @@ -338,7 +338,7 @@ fn propagate_recursive( let (visibility, mut computed_visibility, child_parent) = visibility_query.get_mut(entity).map_err(drop)?; assert_eq!( - child_parent.get(), expected_parent, + child_parent.try_get(), Some(expected_parent), "Malformed hierarchy. This probably means that your hierarchy has been improperly maintained, or contains a cycle" ); let visible_in_hierarchy = (parent_visible && visibility == Visibility::Inherited) diff --git a/crates/bevy_sprite/src/render/mod.rs b/crates/bevy_sprite/src/render/mod.rs index 89d7cbbcce871..7b9c9814e2a79 100644 --- a/crates/bevy_sprite/src/render/mod.rs +++ b/crates/bevy_sprite/src/render/mod.rs @@ -600,7 +600,7 @@ pub fn queue_sprites( image_handle_id: HandleId::Id(Uuid::nil(), u64::MAX), colored: false, }; - let mut current_batch_entity = Entity::PLACEHOLDER; + let mut current_batch_entity = Entity::from_raw(u32::MAX); let mut current_image_size = Vec2::ZERO; // Add a phase item for each sprite, and detect when successive items can be batched. // Spawn an entity with a `SpriteBatch` component for each possible batch. diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index be0613213f4ef..0af01fecf031a 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -42,7 +42,7 @@ pub fn propagate_transforms( for (child, actual_parent) in parent_query.iter_many(children) { assert_eq!( - actual_parent.get(), entity, + actual_parent.try_get(), Some(entity), "Malformed hierarchy. This probably means that your hierarchy has been improperly maintained, or contains a cycle" ); // SAFETY: @@ -132,7 +132,7 @@ unsafe fn propagate_recursive( let Some(children) = children else { return }; for (child, actual_parent) in parent_query.iter_many(children) { assert_eq!( - actual_parent.get(), entity, + actual_parent.try_get(), Some(entity), "Malformed hierarchy. This probably means that your hierarchy has been improperly maintained, or contains a cycle" ); // SAFETY: The caller guarantees that `transform_query` will not be fetched diff --git a/crates/bevy_ui/src/flex/mod.rs b/crates/bevy_ui/src/flex/mod.rs index 8fc866fdb1fa5..7bb526b01c099 100644 --- a/crates/bevy_ui/src/flex/mod.rs +++ b/crates/bevy_ui/src/flex/mod.rs @@ -320,8 +320,8 @@ pub fn flex_node_system( let mut new_position = transform.translation; new_position.x = to_logical(layout.location.x + layout.size.width / 2.0); new_position.y = to_logical(layout.location.y + layout.size.height / 2.0); - if let Some(parent) = parent { - if let Ok(parent_layout) = flex_surface.get_layout(**parent) { + if let Some(parent) = parent.and_then(|v| v.try_get()) { + if let Ok(parent_layout) = flex_surface.get_layout(parent) { new_position.x -= to_logical(parent_layout.size.width / 2.0); new_position.y -= to_logical(parent_layout.size.height / 2.0); } diff --git a/crates/bevy_winit/src/accessibility.rs b/crates/bevy_winit/src/accessibility.rs index fd0f4dc1e4645..42ffc00a37e46 100644 --- a/crates/bevy_winit/src/accessibility.rs +++ b/crates/bevy_winit/src/accessibility.rs @@ -117,8 +117,8 @@ fn update_accessibility_nodes( let mut root_children = vec![]; for (entity, node, children, parent) in &nodes { let mut node = (**node).clone(); - if let Some(parent) = parent { - if node_entities.get(**parent).is_err() { + if let Some(parent) = parent.and_then(|v| v.try_get()) { + if node_entities.get(parent).is_err() { root_children.push(entity.to_node_id()); } } else { diff --git a/examples/animation/gltf_skinned_mesh.rs b/examples/animation/gltf_skinned_mesh.rs index 80c0df7aa6b29..73fce0a940f0e 100644 --- a/examples/animation/gltf_skinned_mesh.rs +++ b/examples/animation/gltf_skinned_mesh.rs @@ -52,21 +52,22 @@ fn joint_animation( // Iter skinned mesh entity for skinned_mesh_parent in &parent_query { // Mesh node is the parent of the skinned mesh entity. - let mesh_node_entity = skinned_mesh_parent.get(); - // Get `Children` in the mesh node. - let mesh_node_children = children_query.get(mesh_node_entity).unwrap(); + if let Some(mesh_node_entity) = skinned_mesh_parent.try_get() { + // Get `Children` in the mesh node. + let mesh_node_children = children_query.get(mesh_node_entity).unwrap(); - // First joint is the second child of the mesh node. - let first_joint_entity = mesh_node_children[1]; - // Get `Children` in the first joint. - let first_joint_children = children_query.get(first_joint_entity).unwrap(); + // First joint is the second child of the mesh node. + let first_joint_entity = mesh_node_children[1]; + // Get `Children` in the first joint. + let first_joint_children = children_query.get(first_joint_entity).unwrap(); - // Second joint is the first child of the first joint. - let second_joint_entity = first_joint_children[0]; - // Get `Transform` in the second joint. - let mut second_joint_transform = transform_query.get_mut(second_joint_entity).unwrap(); + // Second joint is the first child of the first joint. + let second_joint_entity = first_joint_children[0]; + // Get `Transform` in the second joint. + let mut second_joint_transform = transform_query.get_mut(second_joint_entity).unwrap(); - second_joint_transform.rotation = - Quat::from_rotation_z(FRAC_PI_2 * time.elapsed_seconds().sin()); + second_joint_transform.rotation = + Quat::from_rotation_z(FRAC_PI_2 * time.elapsed_seconds().sin()); + } } } From 632318586c872f2e7dcb6c5c830793547e868262 Mon Sep 17 00:00:00 2001 From: Natalie Baker Date: Sat, 11 Mar 2023 11:05:47 +0800 Subject: [PATCH 09/10] Fix format --- crates/bevy_ecs/src/entity/mod.rs | 8 +++++--- crates/bevy_hierarchy/src/child_builder.rs | 20 +++++++++++++++---- .../bevy_hierarchy/src/components/parent.rs | 1 - crates/bevy_hierarchy/src/hierarchy.rs | 5 ++++- crates/bevy_hierarchy/src/query_extension.rs | 6 +++++- 5 files changed, 30 insertions(+), 10 deletions(-) diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index f6c5a51fee359..0fd18e4811473 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -45,7 +45,7 @@ use crate::{ storage::{SparseSetIndex, TableId, TableRow}, }; use serde::{Deserialize, Serialize}; -use std::{convert::TryFrom, fmt, mem, sync::atomic::Ordering, num::NonZeroU32}; +use std::{convert::TryFrom, fmt, mem, num::NonZeroU32, sync::atomic::Ordering}; #[cfg(target_has_atomic = "64")] use std::sync::atomic::AtomicI64 as AtomicIdCursor; @@ -503,7 +503,7 @@ impl Entities { // will be converted to None, marking the entity slot as retired and taking // it out of circulation for re-allocation. meta.generation = NonZeroU32::new(meta.generation.unwrap().get().wrapping_add(1)); - + if meta.generation.is_some() { self.pending.push(entity.index); @@ -826,7 +826,9 @@ mod tests { const C3: u32 = Entity::from_raw(33).index(); assert_eq!(33, C3); - const C4: u32 = Entity::from_bits(0x00dd_00ff_0000_0000).unwrap().generation(); + const C4: u32 = Entity::from_bits(0x00dd_00ff_0000_0000) + .unwrap() + .generation(); assert_eq!(0x00dd_00ff, C4); } diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index 9aee89beac26c..ec5a1853db69a 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -711,11 +711,23 @@ mod tests { world.get::(parent).unwrap().0.as_slice(), children.as_slice(), ); - assert_eq!(*world.get::(children[0]).unwrap(), Parent::new(parent)); - assert_eq!(*world.get::(children[1]).unwrap(), Parent::new(parent)); + assert_eq!( + *world.get::(children[0]).unwrap(), + Parent::new(parent) + ); + assert_eq!( + *world.get::(children[1]).unwrap(), + Parent::new(parent) + ); - assert_eq!(*world.get::(children[0]).unwrap(), Parent::new(parent)); - assert_eq!(*world.get::(children[1]).unwrap(), Parent::new(parent)); + assert_eq!( + *world.get::(children[0]).unwrap(), + Parent::new(parent) + ); + assert_eq!( + *world.get::(children[1]).unwrap(), + Parent::new(parent) + ); } #[test] diff --git a/crates/bevy_hierarchy/src/components/parent.rs b/crates/bevy_hierarchy/src/components/parent.rs index 1f2712f765ddf..98fcf95ca8049 100644 --- a/crates/bevy_hierarchy/src/components/parent.rs +++ b/crates/bevy_hierarchy/src/components/parent.rs @@ -18,7 +18,6 @@ use std::ops::Deref; pub struct Parent(Option); impl Parent { - pub(crate) fn new(entity: Entity) -> Self { Self(Some(entity)) } diff --git a/crates/bevy_hierarchy/src/hierarchy.rs b/crates/bevy_hierarchy/src/hierarchy.rs index d75f457847d64..fb5fc3d5db8bf 100644 --- a/crates/bevy_hierarchy/src/hierarchy.rs +++ b/crates/bevy_hierarchy/src/hierarchy.rs @@ -23,7 +23,10 @@ pub struct DespawnChildrenRecursive { /// Function for despawning an entity and all its children pub fn despawn_with_children_recursive(world: &mut World, entity: Entity) { // first, make the entity's own parent forget about it - if let Some(parent) = world.get::(entity).and_then(|parent| parent.try_get()) { + if let Some(parent) = world + .get::(entity) + .and_then(|parent| parent.try_get()) + { if let Some(mut children) = world.get_mut::(parent) { children.0.retain(|c| *c != entity); } diff --git a/crates/bevy_hierarchy/src/query_extension.rs b/crates/bevy_hierarchy/src/query_extension.rs index 8e3c1c30c93da..0adcd5b24eb25 100644 --- a/crates/bevy_hierarchy/src/query_extension.rs +++ b/crates/bevy_hierarchy/src/query_extension.rs @@ -150,7 +150,11 @@ where type Item = Entity; fn next(&mut self) -> Option { - self.next = self.parent_query.get(self.next?).ok().and_then(|p| p.try_get()); + self.next = self + .parent_query + .get(self.next?) + .ok() + .and_then(|p| p.try_get()); self.next } } From 41c3e8015f0d0a218467718eae92412ea5d18b0a Mon Sep 17 00:00:00 2001 From: Natalie Baker Date: Sun, 12 Mar 2023 23:05:04 +0800 Subject: [PATCH 10/10] Fix tests --- crates/bevy_ecs/src/entity/mod.rs | 32 ++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index 0fd18e4811473..086e88d1cac19 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -817,19 +817,25 @@ mod tests { fn entity_const() { const C1: Entity = Entity::from_raw(42); assert_eq!(42, C1.index); - assert_eq!(0, C1.generation()); + assert_eq!(1, C1.generation()); - const C2: Entity = Entity::from_bits(0x0000_00ff_0000_00cc).unwrap(); - assert_eq!(0x0000_00cc, C2.index); - assert_eq!(0x0000_00ff, C2.generation()); + const TMP_1: Option = Entity::from_bits(0x0000_00ff_0000_00cc); + if let Some(c2) = TMP_1 { + assert_eq!(0x0000_00cc, c2.index); + assert_eq!(0x0000_00ff, c2.generation.get()); + } else { + panic!("Entity not constructed from bit pattern: 0x0000_00ff_0000_00cc") + } const C3: u32 = Entity::from_raw(33).index(); assert_eq!(33, C3); - const C4: u32 = Entity::from_bits(0x00dd_00ff_0000_0000) - .unwrap() - .generation(); - assert_eq!(0x00dd_00ff, C4); + const TMP_2: Option = Entity::from_bits(0x00dd_00ff_0000_0000); + if let Some(c4) = TMP_2 { + assert_eq!(0x00dd_00ff, c4.generation.get()); + } else { + panic!("Entity not constructed from bit pattern: 0x00dd_00ff_0000_0000") + } } #[test] @@ -848,29 +854,29 @@ mod tests { #[test] fn entities_generation_increment() { - let mut entities = Entities::default(); + let mut entities = Entities::new(); let entity_old = entities.alloc(); entities.free(entity_old); let entity_new = entities.alloc(); - assert_eq!(entity_old.id, entity_new.id); + assert_eq!(entity_old.index, entity_new.index); assert_eq!(entity_old.generation() + 1, entity_new.generation()); } #[test] fn entities_generation_overflow() { - let mut entities = Entities::default(); + let mut entities = Entities::new(); let mut entity_old = entities.alloc(); // Modify generation on entity and entities to cause overflow on free entity_old.generation = NonZeroU32::new(u32::MAX).unwrap(); - entities.meta[entity_old.id as usize].generation = Some(entity_old.generation); + entities.meta[entity_old.index as usize].generation = Some(entity_old.generation); entities.free(entity_old); // Request new entity, we shouldn't get the one we just freed since it overflowed let entity_new = entities.alloc(); - assert!(entity_old.id != entity_new.id); + assert!(entity_old.index != entity_new.index); assert_eq!(entity_new.generation(), 1); } }