Remove invalid entity locations #19433

ElliottjPierce · 2025-05-29T17:48:30Z

Objective

This is the first step of #19430 and is a follow up for #19132.

Now that ArchetypeRow has a niche, we can use Option instead of needing INVALID everywhere.

This was especially concerning since INVALID really was valid!

Using options here made the code clearer and more data-driven.

Solution

Replace all uses of INVALID entity locations (and archetype/table rows) with None.

Testing

CI

ElliottjPierce · 2025-05-29T18:11:28Z

crates/bevy_ecs/src/entity/mod.rs

-            if meta.generation != EntityGeneration::FIRST
-                || meta.location.archetype_id != ArchetypeId::INVALID
-            {
+            if meta.location.is_some() {


I think this is right, but honestly, not even sure why we need the if.

I think this is not right, the tick may also be indirectly returned by entity_get_spawned_or_despawned even if meta.location is None in some cases.

If the the tick is considered valid in such a case, then it needs to be checked upon in this method here too.

It exposes that we somehow lost the ability to reliably detect when the tick is indeed unused and only a default value. But honestly I would not bother given how cold this method here is. Just check all ticks in here and with that be sure all edge cases are covered.

I agree. I'll remove the if.

urben1680 · 2025-05-29T19:53:25Z

crates/bevy_ecs/src/entity/mod.rs

@@ -973,7 +972,7 @@ impl Entities {
    ///  - `location` must be valid for the entity at `index` or immediately made valid afterwards
    ///    before handing control to unknown code.
    #[inline]
-    pub(crate) unsafe fn set(&mut self, index: u32, location: EntityLocation) {
+    pub(crate) unsafe fn set(&mut self, index: u32, location: Option<EntityLocation>) {


It seems that this method is always called with Some location. Do you expect calls with None in the future or another PR?

I added it for completeness but yes.

We used to have alloc_at which let you respawn an entity and reuse its index. There have been requests to support that again. That's one of the next steps in my tracking issue. Basically, we'll set the location to None instead of fully despawning.

urben1680 · 2025-05-29T19:59:31Z

crates/bevy_ecs/src/world/entity_ref.rs

-    pub(crate) fn assert_not_despawned(&self) {
-        if self.location.archetype_id == ArchetypeId::INVALID {
-            self.panic_despawned();
+    pub(crate) fn assert_not_despawned(&self) -> EntityLocation {


Since this and EntityWorldMut::location do the same now, this one can probably be removed and usages switch to the location method.

That's a good idea.

Turns out it's used for other things too, so I've kept it but reverted to how it was.

crates/bevy_ecs/src/world/entity_ref.rs

urben1680 · 2025-05-29T20:27:10Z

crates/bevy_ecs/src/entity/mod.rs

-            if meta.generation != EntityGeneration::FIRST
-                || meta.location.archetype_id != ArchetypeId::INVALID
-            {
+            if meta.location.is_some() {


I think this is not right, the tick may also be indirectly returned by entity_get_spawned_or_despawned even if meta.location is None in some cases.

If the the tick is considered valid in such a case, then it needs to be checked upon in this method here too.

It exposes that we somehow lost the ability to reliably detect when the tick is indeed unused and only a default value. But honestly I would not bother given how cold this method here is. Just check all ticks in here and with that be sure all edge cases are covered.

urben1680 · 2025-05-29T20:31:03Z

crates/bevy_ecs/src/entity/mod.rs

-impl EntityLocation {
-    /// location for **pending entity** and **invalid entity**
-    pub(crate) const INVALID: EntityLocation = EntityLocation {
-        archetype_id: ArchetypeId::INVALID,
-        archetype_row: ArchetypeRow::INVALID,
-        table_id: TableId::INVALID,
-        table_row: TableRow::INVALID,
-    };
-}


feels_good_man.jpg

crates/bevy_ecs/src/entity/mod.rs

chescock

Looks good! Just left some style suggestions.

chescock · 2025-05-30T15:22:53Z

crates/bevy_ecs/src/entity/mod.rs

-            Some(meta.location)
+            (meta.generation == entity.generation)
+                .then_some(meta.location)
+                .flatten()


then_some().flatten() is a little odd. I think that might be Option::filter()?

Hmm, or could this whole thing be replaced with filter().and_then() like

self.meta.get(entity.index() as usize) .filter(|meta| meta.generation == entity.generation) .and_then(|meta| meta.location)

?

(Sorry, I can't get GitHub to let me write that as a suggestion.)

chescock · 2025-05-30T15:27:51Z

crates/bevy_ecs/src/entity/mod.rs

    /// Must not be called while reserved entities are awaiting `flush()`.
-    pub fn free(&mut self, entity: Entity) -> Option<EntityLocation> {
+    pub fn free(&mut self, entity: Entity) -> Option<Option<EntityLocation>> {


Option<Option<T>> can be hard to reason about. Does anyone care about Some(None) vs None, or could we flatten this and return a plain Option<EntityLocation>? It looks like the only place this is called changed from .expect() to .expect().expect(), so it doesn't seem to care.

Otherwise, it might be worth returning a Result with some new enum that gives names to the two kinds of None.

Co-Authored-By: Chris Russell <[email protected]>

ElliottjPierce · 2025-05-30T16:08:49Z

As far as Option<Option<EntityLocation>> goes, I agree that, for now, flattening is the right way to go, especially given the inconsistency between get and free.

But as a heads up, I think IMO we should expose this fully. Maybe a struct MaybeEntityLocation(Option<EntityLocation>);? This will be useful for constructing/destructing entities in the future, but I'll save that for another pr.

ElliottjPierce · 2025-05-30T16:40:35Z

Update: unfortunately tests do seem to need the full location information, so I have added it back.

But I used a type alias to hopefully make things a little more clear.

urben1680 · 2025-05-30T17:23:45Z

An alternative could also be Result<Option<EntityLocation>, EntityDoesNotExistError>, though it is not so nice since Ok(None) also means the entity kinda does not exist.

ElliottjPierce · 2025-05-30T17:32:08Z

An alternative could also be Result<Option<EntityLocation>, EntityDoesNotExistError>, though it is not so nice since Ok(None) also means the entity kinda does not exist.

Option<EntityIdLocation> to Result<EntityIdLocation, EndityDoesNotExistError> is not a bad idea. I'm not opposed to it, but I'm not sure what it would buy us. We still need EntityIdLocation for set, flush, etc. Maybe I'll try it out in a future pr.

NyxAlexandra · 2025-05-30T22:28:39Z

crates/bevy_ecs/src/entity/mod.rs

+///
+/// Setting a location to `None` is often helpful when you want to destruct an entity or yank it from the ECS without allowing another system to reuse the id for something else.
+/// It is also useful for reserving an id; commands will often allocate an `Entity` but not provide it a location until the command is applied.
+pub type EntityIdLocation = Option<EntityLocation>;


I'm not sure if this name is very clear ("location of an entity" versus "location of an entity identifier"). Then again, I'm not sure of a better name (nor am I a maintaner or anything like that).

ElliottjPierce added 2 commits May 29, 2025 13:35

use Option<EntityLocation>

325c77b

remove invalid

ff1344c

migration guide

944a8c7

ElliottjPierce commented May 29, 2025

View reviewed changes

fix migration guide

609e52b

urben1680 suggested changes May 29, 2025

View reviewed changes

urben1680 reviewed May 29, 2025

View reviewed changes

crates/bevy_ecs/src/entity/mod.rs Outdated Show resolved Hide resolved

ElliottjPierce added 5 commits May 29, 2025 22:48

check all change ticks

c8704b4

clean up entity_ref

9f2b155

rename migration guide

0029ddd

improve None location docs

623e914

fixed public to nobody EntityMeta items

9dd16fd

urben1680 approved these changes May 30, 2025

View reviewed changes

chescock approved these changes May 30, 2025

View reviewed changes

alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 30, 2025

ElliottjPierce and others added 2 commits May 30, 2025 12:04

simplify get

698e018

Co-Authored-By: Chris Russell <[email protected]>

flattened free

299e468

Co-Authored-By: Chris Russell <[email protected]>

ElliottjPierce added 2 commits May 30, 2025 12:33

give full location details

c5a01b4

update migration guide

0a8d315

NyxAlexandra reviewed May 30, 2025

View reviewed changes

Uh oh!

Remove invalid entity locations #19433

Are you sure you want to change the base?

Remove invalid entity locations #19433

Conversation

ElliottjPierce commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

urben1680 May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

urben1680 May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

urben1680 May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chescock left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ElliottjPierce commented May 30, 2025

Uh oh!

ElliottjPierce commented May 30, 2025

Uh oh!

urben1680 commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ElliottjPierce commented May 30, 2025

Uh oh!

NyxAlexandra May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ElliottjPierce commented May 29, 2025 •

edited

Loading

urben1680 May 29, 2025 •

edited

Loading

urben1680 May 29, 2025 •

edited

Loading

urben1680 May 29, 2025 •

edited

Loading

urben1680 commented May 30, 2025 •

edited

Loading

NyxAlexandra May 30, 2025 •

edited

Loading