From 5ba2b9adcf5e686273cf024acf1ad8ddfb4f8e18 Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Thu, 30 Sep 2021 20:54:47 +0000 Subject: [PATCH] Unique WorldId (#2827) # Objective Fixes these issues: - `WorldId`s currently aren't necessarily unique - I want to guarantee that they're unique to safeguard my librarified version of https://github.com/bevyengine/bevy/discussions/2805 - There probably hasn't been a collision yet, but they could technically collide - `SystemId` isn't used for anything - It's no longer used now that `Locals` are stored within the `System`. - `bevy_ecs` depends on rand ## Solution - Instead of randomly generating `WorldId`s, just use an incrementing atomic counter, panicing on overflow. - Remove `SystemId` - We do need to allow Locals for exclusive systems at some point, but exclusive systems couldn't access their own `SystemId` anyway. - Now that these don't depend on rand, move it to a dev-dependency ## Todo Determine if `WorldId` should be `u32` based instead --- crates/bevy_core/src/time/fixed_timestep.rs | 6 +- crates/bevy_ecs/Cargo.toml | 2 +- crates/bevy_ecs/src/schedule/run_criteria.rs | 8 +-- .../bevy_ecs/src/system/exclusive_system.rs | 14 +---- crates/bevy_ecs/src/system/function_system.rs | 11 +--- crates/bevy_ecs/src/system/system.rs | 14 ----- crates/bevy_ecs/src/system/system_chaining.rs | 8 +-- crates/bevy_ecs/src/world/identifier.rs | 56 +++++++++++++++++++ crates/bevy_ecs/src/world/mod.rs | 19 +++---- 9 files changed, 72 insertions(+), 66 deletions(-) create mode 100644 crates/bevy_ecs/src/world/identifier.rs diff --git a/crates/bevy_core/src/time/fixed_timestep.rs b/crates/bevy_core/src/time/fixed_timestep.rs index 3655da70db419..a22c874f92930 100644 --- a/crates/bevy_core/src/time/fixed_timestep.rs +++ b/crates/bevy_core/src/time/fixed_timestep.rs @@ -4,7 +4,7 @@ use bevy_ecs::{ component::ComponentId, query::Access, schedule::ShouldRun, - system::{ConfigurableSystem, IntoSystem, Local, Res, ResMut, System, SystemId}, + system::{ConfigurableSystem, IntoSystem, Local, Res, ResMut, System}, world::World, }; use bevy_utils::HashMap; @@ -148,10 +148,6 @@ impl System for FixedTimestep { Cow::Borrowed(std::any::type_name::()) } - fn id(&self) -> SystemId { - self.internal_system.id() - } - fn new_archetype(&mut self, archetype: &Archetype) { self.internal_system.new_archetype(archetype); } diff --git a/crates/bevy_ecs/Cargo.toml b/crates/bevy_ecs/Cargo.toml index 00010a15252e0..8f01bc61db490 100644 --- a/crates/bevy_ecs/Cargo.toml +++ b/crates/bevy_ecs/Cargo.toml @@ -24,11 +24,11 @@ fixedbitset = "0.4" fxhash = "0.2" thiserror = "1.0" downcast-rs = "1.2" -rand = "0.8" serde = "1" [dev-dependencies] parking_lot = "0.11" +rand = "0.8" [[example]] name = "events" diff --git a/crates/bevy_ecs/src/schedule/run_criteria.rs b/crates/bevy_ecs/src/schedule/run_criteria.rs index 56c43452d3e60..4104daab56d7c 100644 --- a/crates/bevy_ecs/src/schedule/run_criteria.rs +++ b/crates/bevy_ecs/src/schedule/run_criteria.rs @@ -3,7 +3,7 @@ use crate::{ component::ComponentId, query::Access, schedule::{BoxedRunCriteriaLabel, GraphNode, RunCriteriaLabel}, - system::{BoxedSystem, IntoSystem, System, SystemId}, + system::{BoxedSystem, IntoSystem, System}, world::World, }; use std::borrow::Cow; @@ -398,7 +398,6 @@ where pub struct RunOnce { ran: bool, - system_id: SystemId, archetype_component_access: Access, component_access: Access, } @@ -407,7 +406,6 @@ impl Default for RunOnce { fn default() -> Self { Self { ran: false, - system_id: SystemId::new(), archetype_component_access: Default::default(), component_access: Default::default(), } @@ -422,10 +420,6 @@ impl System for RunOnce { Cow::Borrowed(std::any::type_name::()) } - fn id(&self) -> SystemId { - self.system_id - } - fn new_archetype(&mut self, _archetype: &Archetype) {} fn component_access(&self) -> &Access { diff --git a/crates/bevy_ecs/src/system/exclusive_system.rs b/crates/bevy_ecs/src/system/exclusive_system.rs index 7050355b6b8fe..de951ee6e4a60 100644 --- a/crates/bevy_ecs/src/system/exclusive_system.rs +++ b/crates/bevy_ecs/src/system/exclusive_system.rs @@ -1,6 +1,6 @@ use crate::{ archetype::ArchetypeGeneration, - system::{check_system_change_tick, BoxedSystem, IntoSystem, SystemId}, + system::{check_system_change_tick, BoxedSystem, IntoSystem}, world::World, }; use std::borrow::Cow; @@ -8,8 +8,6 @@ use std::borrow::Cow; pub trait ExclusiveSystem: Send + Sync + 'static { fn name(&self) -> Cow<'static, str>; - fn id(&self) -> SystemId; - fn run(&mut self, world: &mut World); fn initialize(&mut self, world: &mut World); @@ -20,7 +18,6 @@ pub trait ExclusiveSystem: Send + Sync + 'static { pub struct ExclusiveSystemFn { func: Box, name: Cow<'static, str>, - id: SystemId, last_change_tick: u32, } @@ -29,10 +26,6 @@ impl ExclusiveSystem for ExclusiveSystemFn { self.name.clone() } - fn id(&self) -> SystemId { - self.id - } - fn run(&mut self, world: &mut World) { // The previous value is saved in case this exclusive system is run by another exclusive // system @@ -67,7 +60,6 @@ where ExclusiveSystemFn { func: Box::new(self), name: core::any::type_name::().into(), - id: SystemId::new(), last_change_tick: 0, } } @@ -83,10 +75,6 @@ impl ExclusiveSystem for ExclusiveSystemCoerced { self.system.name() } - fn id(&self) -> SystemId { - self.system.id() - } - fn run(&mut self, world: &mut World) { let archetypes = world.archetypes(); let new_generation = archetypes.generation(); diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index c37653c078567..90b36671e8eb1 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -3,8 +3,8 @@ use crate::{ component::ComponentId, query::{Access, FilteredAccessSet}, system::{ - check_system_change_tick, ReadOnlySystemParamFetch, System, SystemId, SystemParam, - SystemParamFetch, SystemParamState, + check_system_change_tick, ReadOnlySystemParamFetch, System, SystemParam, SystemParamFetch, + SystemParamState, }, world::{World, WorldId}, }; @@ -13,7 +13,6 @@ use std::{borrow::Cow, marker::PhantomData}; /// The metadata of a [`System`]. pub struct SystemMeta { - pub(crate) id: SystemId, pub(crate) name: Cow<'static, str>, pub(crate) component_access_set: FilteredAccessSet, pub(crate) archetype_component_access: Access, @@ -30,7 +29,6 @@ impl SystemMeta { archetype_component_access: Access::default(), component_access_set: FilteredAccessSet::default(), is_send: true, - id: SystemId::new(), last_change_tick: 0, } } @@ -340,11 +338,6 @@ where self.system_meta.name.clone() } - #[inline] - fn id(&self) -> SystemId { - self.system_meta.id - } - #[inline] fn new_archetype(&mut self, archetype: &Archetype) { let param_state = self.param_state.as_mut().unwrap(); diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index eabf476730340..25ccf598eb7fb 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -8,18 +8,6 @@ use crate::{ }; use std::borrow::Cow; -/// A [`System`] identifier. -#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)] -pub struct SystemId(pub usize); - -impl SystemId { - /// Creates a new random `SystemId`. - #[allow(clippy::new_without_default)] - pub fn new() -> Self { - SystemId(rand::random::()) - } -} - /// An ECS system that can be added to a [Schedule](crate::schedule::Schedule) /// /// Systems are functions with all arguments implementing [SystemParam](crate::system::SystemParam). @@ -38,8 +26,6 @@ pub trait System: Send + Sync + 'static { type Out; /// Returns the system's name. fn name(&self) -> Cow<'static, str>; - /// Returns the system's [`SystemId`]. - fn id(&self) -> SystemId; /// Register a new archetype for this system. fn new_archetype(&mut self, archetype: &Archetype); /// Returns the system's component [`Access`]. diff --git a/crates/bevy_ecs/src/system/system_chaining.rs b/crates/bevy_ecs/src/system/system_chaining.rs index 736bf066331ea..e2f789ee728e7 100644 --- a/crates/bevy_ecs/src/system/system_chaining.rs +++ b/crates/bevy_ecs/src/system/system_chaining.rs @@ -2,7 +2,7 @@ use crate::{ archetype::{Archetype, ArchetypeComponentId}, component::ComponentId, query::Access, - system::{IntoSystem, System, SystemId}, + system::{IntoSystem, System}, world::World, }; use std::borrow::Cow; @@ -48,7 +48,6 @@ pub struct ChainSystem { system_a: SystemA, system_b: SystemB, name: Cow<'static, str>, - id: SystemId, component_access: Access, archetype_component_access: Access, } @@ -61,10 +60,6 @@ impl> System for ChainSystem self.name.clone() } - fn id(&self) -> SystemId { - self.id - } - fn new_archetype(&mut self, archetype: &Archetype) { self.system_a.new_archetype(archetype); self.system_b.new_archetype(archetype); @@ -143,7 +138,6 @@ where system_b, archetype_component_access: Default::default(), component_access: Default::default(), - id: SystemId::new(), } } } diff --git a/crates/bevy_ecs/src/world/identifier.rs b/crates/bevy_ecs/src/world/identifier.rs new file mode 100644 index 0000000000000..6e2b8efc0f222 --- /dev/null +++ b/crates/bevy_ecs/src/world/identifier.rs @@ -0,0 +1,56 @@ +use std::sync::atomic::{AtomicUsize, Ordering}; + +#[derive(Copy, Clone, PartialEq, Eq, Debug)] +// We use usize here because that is the largest `Atomic` we want to require +/// A unique identifier for a [`super::World`]. +// Note that this *is* used by external crates as well as for internal safety checks +pub struct WorldId(usize); + +/// The next [`WorldId`]. +static MAX_WORLD_ID: AtomicUsize = AtomicUsize::new(0); + +impl WorldId { + /// Create a new, unique [`WorldId`]. Returns [`None`] if the supply of unique + /// [`WorldId`]s has been exhausted + /// + /// Please note that the [`WorldId`]s created from this method are unique across + /// time - if a given [`WorldId`] is [`Drop`]ped its value still cannot be reused + pub fn new() -> Option { + MAX_WORLD_ID + // We use `Relaxed` here since this atomic only needs to be consistent with itself + .fetch_update(Ordering::Relaxed, Ordering::Relaxed, |val| { + val.checked_add(1) + }) + .map(WorldId) + .ok() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn world_ids_unique() { + let ids = std::iter::repeat_with(WorldId::new) + .take(50) + .map(Option::unwrap) + .collect::>(); + for (i, &id1) in ids.iter().enumerate() { + // For the first element, i is 0 - so skip 1 + for &id2 in ids.iter().skip(i + 1) { + assert_ne!(id1, id2, "WorldIds should not repeat"); + } + } + } + + // We cannot use this test as-is, as it causes other tests to panic due to using the same atomic variable. + // #[test] + // #[should_panic] + // fn panic_on_overflow() { + // MAX_WORLD_ID.store(usize::MAX - 50, Ordering::Relaxed); + // std::iter::repeat_with(WorldId::new) + // .take(500) + // .for_each(|_| ()); + // } +} diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index d246248872adb..441da301bda80 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -25,15 +25,9 @@ use std::{ sync::atomic::{AtomicU32, Ordering}, }; -#[derive(Copy, Clone, PartialEq, Eq, Debug)] -pub struct WorldId(u64); - -impl Default for WorldId { - fn default() -> Self { - WorldId(rand::random()) - } -} +mod identifier; +pub use identifier::WorldId; /// Stores and exposes operations on [entities](Entity), [components](Component), resources, /// and their associated metadata. /// @@ -94,7 +88,7 @@ pub struct World { impl Default for World { fn default() -> Self { Self { - id: Default::default(), + id: WorldId::new().expect("More `bevy` `World`s have been created than is supported"), entities: Default::default(), components: Default::default(), archetypes: Default::default(), @@ -113,12 +107,17 @@ impl Default for World { impl World { /// Creates a new empty [World] + /// # Panics + /// + /// If [`usize::MAX`] [`World`]s have been created. + /// This guarantee allows System Parameters to safely uniquely identify a [`World`], + /// since its [`WorldId`] is unique #[inline] pub fn new() -> World { World::default() } - /// Retrieves this world's unique ID + /// Retrieves this [`World`]'s unique ID #[inline] pub fn id(&self) -> WorldId { self.id