diff --git a/crates/bevy_ecs/src/entity/map_entities.rs b/crates/bevy_ecs/src/entity/map_entities.rs index 3dac2fa749..647bde983a 100644 --- a/crates/bevy_ecs/src/entity/map_entities.rs +++ b/crates/bevy_ecs/src/entity/map_entities.rs @@ -358,7 +358,10 @@ mod tests { // Next allocated entity should be a further generation on the same index let entity = world.spawn_empty().id(); assert_eq!(entity.index(), dead_ref.index()); - assert!(entity.generation() > dead_ref.generation()); + assert!(entity + .generation() + .cmp_approx(&dead_ref.generation()) + .is_gt()); } #[test] @@ -373,7 +376,10 @@ mod tests { // Next allocated entity should be a further generation on the same index let entity = world.spawn_empty().id(); assert_eq!(entity.index(), dead_ref.index()); - assert!(entity.generation() > dead_ref.generation()); + assert!(entity + .generation() + .cmp_approx(&dead_ref.generation()) + .is_gt()); } #[test] diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index 617f945bb7..0e22fddbc8 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -185,29 +185,6 @@ impl SparseSetIndex for EntityRow { /// /// This should be treated as a opaque identifier, and its internal representation may be subject to change. /// -/// # Ordering -/// -/// [`EntityGeneration`] implements [`Ord`]. -/// Generations that are later will be [`Greater`](core::cmp::Ordering::Greater) than earlier ones. -/// -/// ``` -/// # use bevy_ecs::entity::EntityGeneration; -/// assert!(EntityGeneration::FIRST < EntityGeneration::FIRST.after_versions(400)); -/// let (aliased, did_alias) = EntityGeneration::FIRST.after_versions(400).after_versions_and_could_alias(u32::MAX); -/// assert!(did_alias); -/// assert!(EntityGeneration::FIRST < aliased); -/// ``` -/// -/// Ordering will be incorrect for distant generations: -/// -/// ``` -/// # use bevy_ecs::entity::EntityGeneration; -/// // This ordering is wrong! -/// assert!(EntityGeneration::FIRST > EntityGeneration::FIRST.after_versions(400 + (1u32 << 31))); -/// ``` -/// -/// This strange behavior needed to account for aliasing. -/// /// # Aliasing /// /// Internally [`EntityGeneration`] wraps a `u32`, so it can't represent *every* possible generation. @@ -235,6 +212,9 @@ impl EntityGeneration { /// Represents the first generation of an [`EntityRow`]. pub const FIRST: Self = Self(0); + /// Non-wrapping difference between two generations after which a signed interpretation becomes negative. + const DIFF_MAX: u32 = 1u32 << 31; + /// Gets some bits that represent this value. /// The bits are opaque and should not be regarded as meaningful. #[inline(always)] @@ -266,18 +246,48 @@ impl EntityGeneration { let raw = self.0.overflowing_add(versions); (Self(raw.0), raw.1) } -} -impl PartialOrd for EntityGeneration { - fn partial_cmp(&self, other: &Self) -> Option { - Some(self.cmp(other)) - } -} - -impl Ord for EntityGeneration { - fn cmp(&self, other: &Self) -> core::cmp::Ordering { - let diff = self.0.wrapping_sub(other.0); - (1u32 << 31).cmp(&diff) + /// Compares two generations. + /// + /// Generations that are later will be [`Greater`](core::cmp::Ordering::Greater) than earlier ones. + /// + /// ``` + /// # use bevy_ecs::entity::EntityGeneration; + /// # use core::cmp::Ordering; + /// let later_generation = EntityGeneration::FIRST.after_versions(400); + /// assert_eq!(EntityGeneration::FIRST.cmp_approx(&later_generation), Ordering::Less); + /// + /// let (aliased, did_alias) = EntityGeneration::FIRST.after_versions(400).after_versions_and_could_alias(u32::MAX); + /// assert!(did_alias); + /// assert_eq!(EntityGeneration::FIRST.cmp_approx(&aliased), Ordering::Less); + /// ``` + /// + /// Ordering will be incorrect and [non-transitive](https://en.wikipedia.org/wiki/Transitive_relation) + /// for distant generations: + /// + /// ```should_panic + /// # use bevy_ecs::entity::EntityGeneration; + /// # use core::cmp::Ordering; + /// let later_generation = EntityGeneration::FIRST.after_versions(3u32 << 31); + /// let much_later_generation = later_generation.after_versions(3u32 << 31); + /// + /// // while these orderings are correct and pass assertions... + /// assert_eq!(EntityGeneration::FIRST.cmp_approx(&later_generation), Ordering::Less); + /// assert_eq!(later_generation.cmp_approx(&much_later_generation), Ordering::Less); + /// + /// // ... this ordering is not and the assertion fails! + /// assert_eq!(EntityGeneration::FIRST.cmp_approx(&much_later_generation), Ordering::Less); + /// ``` + /// + /// Because of this, `EntityGeneration` does not implement `Ord`/`PartialOrd`. + #[inline] + pub const fn cmp_approx(&self, other: &Self) -> core::cmp::Ordering { + use core::cmp::Ordering; + match self.0.wrapping_sub(other.0) { + 0 => Ordering::Equal, + 1..Self::DIFF_MAX => Ordering::Greater, + _ => Ordering::Less, + } } } @@ -1422,7 +1432,10 @@ mod tests { // The very next entity allocated should be a further generation on the same index let next_entity = entities.alloc(); assert_eq!(next_entity.index(), entity.index()); - assert!(next_entity.generation() > entity.generation().after_versions(GENERATIONS)); + assert!(next_entity + .generation() + .cmp_approx(&entity.generation().after_versions(GENERATIONS)) + .is_gt()); } #[test] @@ -1609,6 +1622,24 @@ mod tests { } } + #[test] + fn entity_generation_is_approximately_ordered() { + use core::cmp::Ordering; + + let old = EntityGeneration::FIRST; + let middle = old.after_versions(1); + let younger_before_ord_wrap = middle.after_versions(EntityGeneration::DIFF_MAX); + let younger_after_ord_wrap = younger_before_ord_wrap.after_versions(1); + + assert_eq!(middle.cmp_approx(&old), Ordering::Greater); + assert_eq!(middle.cmp_approx(&middle), Ordering::Equal); + assert_eq!(middle.cmp_approx(&younger_before_ord_wrap), Ordering::Less); + assert_eq!( + middle.cmp_approx(&younger_after_ord_wrap), + Ordering::Greater + ); + } + #[test] fn entity_debug() { let entity = Entity::from_raw(EntityRow::new(NonMaxU32::new(42).unwrap()));