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 #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

Author: DJMcNab

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::<FixedTimestep>())
     }
 
-    fn id(&self) -> SystemId {
-        self.internal_system.id()
-    }
-
     fn new_archetype(&mut self, archetype: &Archetype) {
         self.internal_system.new_archetype(archetype);
     }

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"

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<ArchetypeComponentId>,
     component_access: Access<ComponentId>,
 }
@@ -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::<RunOnce>())
     }
 
-    fn id(&self) -> SystemId {
-        self.system_id
-    }
-
     fn new_archetype(&mut self, _archetype: &Archetype) {}
 
     fn component_access(&self) -> &Access<ComponentId> {

crates/bevy_ecs/src/system/exclusive_system.rs

@@ -1,15 +1,13 @@
 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;
 
 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<dyn FnMut(&mut World) + Send + Sync + 'static>,
     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::<F>().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();

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<ComponentId>,
     pub(crate) archetype_component_access: Access<ArchetypeComponentId>,
@@ -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();

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::<usize>())
-    }
-}
-
 /// 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`].

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<SystemA, SystemB> {
     system_a: SystemA,
     system_b: SystemB,
     name: Cow<'static, str>,
-    id: SystemId,
     component_access: Access<ComponentId>,
     archetype_component_access: Access<ArchetypeComponentId>,
 }
@@ -61,10 +60,6 @@ impl<SystemA: System, SystemB: System<In = SystemA::Out>> 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(),
         }
     }
 }

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<Self> {
+        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::<Vec<_>>();
+        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(|_| ());
+    // }
+}

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