Skip to content

Commit 1966e44

Browse files
EntityGeneration ordering (#19421)
# Objective Recently the `u32` `Entity::generation` was replaced with the new `EntityGeneration` in #19121. This made meanings a lot more clear, and prevented accidental misuse. One common misuse was assuming that `u32`s that were greater than others came after those others. Wrapping makes this assumption false. When `EntityGeneration` was created, it retained the `u32` ordering, which was useless at best and wrong at worst. This pr fixes the ordering implementation, so new generations are greater than older generations. Some users were already accounting for this ordering issue (which was still present in 0.16 and before) by manually accessing the `u32` representation. This made migrating difficult for avian physics; see [here](https://discord.com/channels/691052431525675048/749335865876021248/1377431569228103780). I am generally of the opinion that this type should be kept opaque to prevent accidental misuse. As we find issues like this, the functionality should be added to `EntityGeneration` directly. ## Solution Fix the ordering implementation through `Ord`. Alternatively, we could keep `Ord` the same and make a `cmp_age` method, but I think this is better, even though sorting entity ids may be *marginally* slower now (but more correct). This is a tradeoff. ## Testing I improved documentation for aliasing and ordering, adding some doc tests.
1 parent 2dd0f2c commit 1966e44

File tree

1 file changed

+68
-2
lines changed
  • crates/bevy_ecs/src/entity

1 file changed

+68
-2
lines changed

crates/bevy_ecs/src/entity/mod.rs

Lines changed: 68 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,47 @@ impl SparseSetIndex for EntityRow {
184184
/// Importantly, this can wrap, meaning each generation is not necessarily unique per [`EntityRow`].
185185
///
186186
/// This should be treated as a opaque identifier, and its internal representation may be subject to change.
187-
#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Debug, Display)]
187+
///
188+
/// # Ordering
189+
///
190+
/// [`EntityGeneration`] implements [`Ord`].
191+
/// Generations that are later will be [`Greater`](core::cmp::Ordering::Greater) than earlier ones.
192+
///
193+
/// ```
194+
/// # use bevy_ecs::entity::EntityGeneration;
195+
/// assert!(EntityGeneration::FIRST < EntityGeneration::FIRST.after_versions(400));
196+
/// let (aliased, did_alias) = EntityGeneration::FIRST.after_versions(400).after_versions_and_could_alias(u32::MAX);
197+
/// assert!(did_alias);
198+
/// assert!(EntityGeneration::FIRST < aliased);
199+
/// ```
200+
///
201+
/// Ordering will be incorrect for distant generations:
202+
///
203+
/// ```
204+
/// # use bevy_ecs::entity::EntityGeneration;
205+
/// // This ordering is wrong!
206+
/// assert!(EntityGeneration::FIRST > EntityGeneration::FIRST.after_versions(400 + (1u32 << 31)));
207+
/// ```
208+
///
209+
/// This strange behavior needed to account for aliasing.
210+
///
211+
/// # Aliasing
212+
///
213+
/// Internally [`EntityGeneration`] wraps a `u32`, so it can't represent *every* possible generation.
214+
/// Eventually, generations can (and do) wrap or alias.
215+
/// This can cause [`Entity`] and [`EntityGeneration`] values to be equal while still referring to different conceptual entities.
216+
/// This can cause some surprising behavior:
217+
///
218+
/// ```
219+
/// # use bevy_ecs::entity::EntityGeneration;
220+
/// let (aliased, did_alias) = EntityGeneration::FIRST.after_versions(1u32 << 31).after_versions_and_could_alias(1u32 << 31);
221+
/// assert!(did_alias);
222+
/// assert!(EntityGeneration::FIRST == aliased);
223+
/// ```
224+
///
225+
/// This can cause some unintended side effects.
226+
/// See [`Entity`] docs for practical concerns and how to minimize any risks.
227+
#[derive(Clone, Copy, PartialEq, Eq, Hash, Debug, Display)]
188228
#[cfg_attr(feature = "bevy_reflect", derive(Reflect))]
189229
#[cfg_attr(feature = "bevy_reflect", reflect(opaque))]
190230
#[cfg_attr(feature = "bevy_reflect", reflect(Hash, PartialEq, Debug, Clone))]
@@ -228,16 +268,42 @@ impl EntityGeneration {
228268
}
229269
}
230270

271+
impl PartialOrd for EntityGeneration {
272+
fn partial_cmp(&self, other: &Self) -> Option<core::cmp::Ordering> {
273+
Some(self.cmp(other))
274+
}
275+
}
276+
277+
impl Ord for EntityGeneration {
278+
fn cmp(&self, other: &Self) -> core::cmp::Ordering {
279+
let diff = self.0.wrapping_sub(other.0);
280+
(1u32 << 31).cmp(&diff)
281+
}
282+
}
283+
231284
/// Lightweight identifier of an [entity](crate::entity).
232285
///
233-
/// The identifier is implemented using a [generational index]: a combination of an index and a generation.
286+
/// The identifier is implemented using a [generational index]: a combination of an index ([`EntityRow`]) and a generation ([`EntityGeneration`]).
234287
/// This allows fast insertion after data removal in an array while minimizing loss of spatial locality.
235288
///
236289
/// These identifiers are only valid on the [`World`] it's sourced from. Attempting to use an `Entity` to
237290
/// fetch entity components or metadata from a different world will either fail or return unexpected results.
238291
///
239292
/// [generational index]: https://lucassardois.medium.com/generational-indices-guide-8e3c5f7fd594
240293
///
294+
/// # Aliasing
295+
///
296+
/// Once an entity is despawned, it ceases to exist.
297+
/// However, its [`Entity`] id is still present, and may still be contained in some data.
298+
/// This becomes problematic because it is possible for a later entity to be spawned at the exact same id!
299+
/// If this happens, which is rare but very possible, it will be logged.
300+
///
301+
/// Aliasing can happen without warning.
302+
/// Holding onto a [`Entity`] id corresponding to an entity well after that entity was despawned can cause un-intuitive behavior for both ordering, and comparing in general.
303+
/// To prevent these bugs, it is generally best practice to stop holding an [`Entity`] or [`EntityGeneration`] value as soon as you know it has been despawned.
304+
/// If you must do otherwise, do not assume the [`Entity`] corresponds to the same conceptual entity it originally did.
305+
/// See [`EntityGeneration`]'s docs for more information about aliasing and why it occurs.
306+
///
241307
/// # Stability warning
242308
/// For all intents and purposes, `Entity` should be treated as an opaque identifier. The internal bit
243309
/// representation is liable to change from release to release as are the behaviors or performance

0 commit comments

Comments
 (0)