From 76b8310da5ff3f119dee8a605c7df82647526194 Mon Sep 17 00:00:00 2001 From: urben1680 <55257931+urben1680@users.noreply.github.com> Date: Sun, 8 Jun 2025 00:29:13 +0200 Subject: [PATCH] Replace (`Partial`)`Ord` for `EntityGeneration` with corrected standalone method (#19432) # Objective #19421 implemented `Ord` for `EntityGeneration` along the lines of [the impl from slotmap](https://docs.rs/slotmap/latest/src/slotmap/util.rs.html#8): ```rs /// Returns if a is an older version than b, taking into account wrapping of /// versions. pub fn is_older_version(a: u32, b: u32) -> bool { let diff = a.wrapping_sub(b); diff >= (1 << 31) } ``` But that PR and the slotmap impl are different: **slotmap impl** - if `(1u32 << 31)` is greater than `a.wrapping_sub(b)`, then `a` is older than `b` - if `(1u32 << 31)` is equal to `a.wrapping_sub(b)`, then `a` is older than `b` - if `(1u32 << 31)` is less than `a.wrapping_sub(b)`, then `a` is equal or newer than `b` **previous PR impl** - if `(1u32 << 31)` is greater than `a.wrapping_sub(b)`, then `a` is older than `b` - if `(1u32 << 31)` is equal to `a.wrapping_sub(b)`, then `a` is equal to `b` :warning: - if `(1u32 << 31)` is less than `a.wrapping_sub(b)`, then `a` is newer than `b` :warning: This ordering is also not transitive, therefore it should not implement `PartialOrd`. ## Solution Fix the impl in a standalone method, remove the `Partialord`/`Ord` implementation. ## Testing Given the first impl was wrong and got past reviews, I think a new unit test is justified. --- crates/bevy_ecs/src/entity/map_entities.rs | 10 +- crates/bevy_ecs/src/entity/mod.rs | 101 ++++++++++++++------- 2 files changed, 74 insertions(+), 37 deletions(-) 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()));