Togglable bit flag for Identifier

Author: Bluefinger

crates/bevy_ecs/Cargo.toml

@@ -23,7 +23,7 @@ bevy_utils = { path = "../bevy_utils", version = "0.14.0-dev" }
 bevy_ecs_macros = { path = "macros", version = "0.14.0-dev" }
 petgraph = "0.6"
 
-bitflags = "2.3"
+bitflags = "2.4"
 concurrent-queue = "2.4.0"
 fixedbitset = "0.4.2"
 rustc-hash = "1.1"

crates/bevy_ecs/src/entity/map_entities.rs

@@ -86,7 +86,7 @@ impl EntityMapper for SceneEntityMapper<'_> {
         // this new entity reference is specifically designed to never represent any living entity
         let new = Entity::from_raw_and_generation(
             self.dead_start.index(),
-            IdentifierMask::inc_masked_high_by(self.dead_start.generation, self.generations),
+            IdentifierMask::inc_entity_generation_by(self.dead_start.generation, self.generations),
         );
 
         // Prevent generations counter from being a greater value than HIGH_MASK.

crates/bevy_ecs/src/entity/mod.rs

@@ -47,12 +47,7 @@ use bevy_utils::tracing::warn;
 
 use crate::{
     archetype::{ArchetypeId, ArchetypeRow},
-    identifier::{
-        error::IdentifierError,
-        kinds::IdKind,
-        masks::{IdentifierMask, HIGH_MASK},
-        Identifier,
-    },
+    identifier::{error::IdentifierError, masks::IdentifierMask, Identifier},
     storage::{SparseSetIndex, TableId, TableRow},
 };
 use serde::{Deserialize, Serialize};
@@ -217,12 +212,20 @@ pub(crate) enum AllocAtWithoutReplacement {
 
 impl Entity {
     /// Construct an [`Entity`] from a raw `index` value and a non-zero `generation` value.
-    /// Ensure that the generation value is never greater than `0x7FFF_FFFF`.
+    /// Ensure that the masked generation value is never greater than `0x3FFF_FFFF`.
     #[inline(always)]
     pub(crate) const fn from_raw_and_generation(index: u32, generation: NonZeroU32) -> Entity {
-        debug_assert!(generation.get() <= HIGH_MASK);
+        // Create an intermediate Identifier to do debug verification
+        // This will get optimised away as Identifier and Entity have identical layouts
+        let id = Identifier::from_parts(index, generation);
 
-        Self { index, generation }
+        // If the PLACEHOLDER bit flag is set, then it is not a valid generation value for Entity.
+        debug_assert!(!id.is_placeholder());
+
+        Self {
+            index: id.low(),
+            generation: id.high(),
+        }
     }
 
     /// An entity ID with a placeholder value. This may or may not correspond to an actual entity,
@@ -286,7 +289,13 @@ impl Entity {
     /// No particular structure is guaranteed for the returned bits.
     #[inline(always)]
     pub const fn to_bits(self) -> u64 {
-        IdentifierMask::pack_into_u64(self.index, self.generation.get())
+        self.to_identifier().to_bits()
+    }
+
+    /// Convert to an [`Identifier`].
+    #[inline(always)]
+    pub const fn to_identifier(self) -> Identifier {
+        Identifier::from_parts(self.index, self.generation)
     }
 
     /// Reconstruct an `Entity` previously destructured with [`Entity::to_bits`].
@@ -298,12 +307,19 @@ impl Entity {
     /// 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 {
+        #[inline(never)]
+        #[cold]
+        #[track_caller]
+        const fn invalid_entity() -> ! {
+            panic!("Attempted to initialise invalid bits as an entity");
+        }
+
         // 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"),
+            Err(_) => invalid_entity(),
         }
     }
 
@@ -315,9 +331,7 @@ impl Entity {
     #[inline(always)]
     pub const fn try_from_bits(bits: u64) -> Result<Self, IdentifierError> {
         if let Ok(id) = Identifier::try_from_bits(bits) {
-            let kind = id.kind() as u8;
-
-            if kind == (IdKind::Entity as u8) {
+            if !id.is_placeholder() {
                 return Ok(Self {
                     index: id.low(),
                     generation: id.high(),
@@ -343,8 +357,7 @@ impl Entity {
     /// given index has been reused (index, generation) pairs uniquely identify a given Entity.
     #[inline]
     pub const fn generation(self) -> u32 {
-        // Mask so not to expose any flags
-        IdentifierMask::extract_value_from_high(self.generation.get())
+        self.to_identifier().masked_high()
     }
 }
 
@@ -353,14 +366,19 @@ impl TryFrom<Identifier> for Entity {
 
     #[inline]
     fn try_from(value: Identifier) -> Result<Self, Self::Error> {
+        // Every Entity is an Identifier, but not every Identifier
+        // is an Entity.
         Self::try_from_bits(value.to_bits())
     }
 }
 
 impl From<Entity> for Identifier {
     #[inline]
     fn from(value: Entity) -> Self {
-        Identifier::from_bits(value.to_bits())
+        // Entity's layout is exactly the same as Identifier, therefore
+        // this will remove any panic! path as we know Entity -> Identifier
+        // conversions will never panic.
+        value.to_identifier()
     }
 }
 
@@ -684,12 +702,12 @@ impl Entities {
             return None;
         }
 
-        meta.generation = IdentifierMask::inc_masked_high_by(meta.generation, 1);
+        meta.generation = IdentifierMask::inc_entity_generation_by(meta.generation, 1);
 
         if meta.generation == NonZeroU32::MIN {
             warn!(
                 "Entity({}) generation wrapped on Entities::free, aliasing may occur",
-                entity.index
+                entity.index()
             );
         }
 
@@ -775,7 +793,8 @@ impl Entities {
 
         let meta = &mut self.meta[index as usize];
         if meta.location.archetype_id == ArchetypeId::INVALID {
-            meta.generation = IdentifierMask::inc_masked_high_by(meta.generation, generations);
+            meta.generation =
+                IdentifierMask::inc_entity_generation_by(meta.generation, generations);
             true
         } else {
             false

crates/bevy_ecs/src/identifier/bits.rs

@@ -0,0 +1,64 @@
+//! Module for defining the flag bits present in the [`super::Identifier`] format.
+
+use std::fmt::Debug;
+
+use bitflags::bitflags;
+
+bitflags! {
+    /// Flag bits defined for [`super::Identifier`].
+    pub struct IdentifierFlagBits: u32 {
+        /// Flag for determining whether an [`super::Identifier`] is a
+        /// [`super::IdKind::Placeholder`] or [`super::IdKind::Entity`].
+        const IS_PLACEHOLDER = 0b1000_0000_0000_0000_0000_0000_0000_0000;
+        /// Flag for determining whether the [`super::Identifier`] is in a
+        /// `toggeable` state or not.
+        const IS_TOGGLABLE = 0b0100_0000_0000_0000_0000_0000_0000_0000;
+
+        const _ = !0;
+    }
+}
+
+impl PartialEq for IdentifierFlagBits {
+    #[inline]
+    fn eq(&self, other: &Self) -> bool {
+        self.0 == other.0
+    }
+}
+
+impl Debug for IdentifierFlagBits {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.debug_tuple("IdentifierFlagBits").field(&self.0).finish()
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn smoke_test() {
+        // Both flag bits are set
+        let bits: u32 = 0xC0FF_EEEE;
+
+        let flags = IdentifierFlagBits::from_bits_retain(bits);
+
+        assert!(flags.contains(IdentifierFlagBits::IS_PLACEHOLDER));
+        assert!(flags.contains(IdentifierFlagBits::IS_TOGGLABLE));
+
+        // Only IS_PLACEHOLDER flag bit set
+        let bits: u32 = 0x80FF_F00F;
+
+        let flags = IdentifierFlagBits::from_bits_retain(bits);
+
+        assert!(flags.contains(IdentifierFlagBits::IS_PLACEHOLDER));
+        assert!(!flags.contains(IdentifierFlagBits::IS_TOGGLABLE));
+
+        // Only IS_TOGGLABLE flag bit set
+        let bits: u32 = 0x40FF_F00F;
+
+        let flags = IdentifierFlagBits::from_bits_retain(bits);
+
+        assert!(!flags.contains(IdentifierFlagBits::IS_PLACEHOLDER));
+        assert!(flags.contains(IdentifierFlagBits::IS_TOGGLABLE));
+    }
+}

crates/bevy_ecs/src/identifier/kinds.rs

@@ -1,3 +1,6 @@
+//! Module for defining the different [`super::Identifier`] kinds, which
+//! have different semantics with regards to their shared layout.
+
 /// The kinds of ID that [`super::Identifier`] can represent. Each
 /// variant imposes different usages of the low/high segments
 /// of the ID.
@@ -7,5 +10,5 @@ pub enum IdKind {
     /// An ID variant that is compatible with [`crate::entity::Entity`].
     Entity = 0,
     /// A future ID variant.
-    Placeholder = 0b1000_0000,
+    Placeholder = 1,
 }