forestia/bevy
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Replace (Partial)Ord for EntityGeneration with corrected standalone method (#19432)

Browse Source 
# 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` ⚠️
- if `(1u32 << 31)` is less than `a.wrapping_sub(b)`, then `a` is newer
than `b` ⚠️

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.
This commit is contained in:
urben1680 2025-06-08 00:29:13 +02:00 committed by GitHub
parent de79d3f363
commit 76b8310da5
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 74 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
crates/bevy_ecs/src/entity/map_entities.rs
View File

@ -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]

101
crates/bevy_ecs/src/entity/mod.rs
View File

@ -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<core::cmp::Ordering> {
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()));