track_change_detection: Also track spawns/despawns #16047

SpecificProtagonist · 2024-10-21T21:49:03Z

Objective

Expand track_change_detection feature to also track entity spawns and despawns. Use this to create better error messages.

Solution

Adds Entities::entity_get_spawned_or_despawned_by as well as {all entity reference types}::spawned_by.

This also removes the deprecated get_many_entities_mut & co (and therefore can't land in 0.15) because we don't yet have no Polonius.

Testing

Added a test that checks that the locations get updated and these updates are ordered correctly vs hooks & observers.

Showcase

Access location:

let mut world = World::new();
let entity = world.spawn_empty().id();
println!("spawned by: {}", world.entity(entity).spawned_by());

spawned by: src/main.rs:5:24

Error message (with track_change_detection):

world.despawn(entity);
world.entity(entity);

thread 'main' panicked at src/main.rs:11:11:
Entity 0v1#4294967296 was despawned by src/main.rs:10:11

and without:

thread 'main' panicked at src/main.rs:11:11:
Entity 0v1#4294967296 does not exist (enable `track_change_detection` feature for more details)

Similar error messages now also exists for Query::get, World::entity_mut, EntityCommands creation and everything that causes B0003, e.g.

error[B0003]: Could not insert a bundle (of type `MaterialMeshBundle<StandardMaterial>`) for entity Entity { index: 7, generation: 1 }, which was despawned by src/main.rs:10:11. See: https://bevyengine.org/learn/errors/#b0003

crates/bevy_ecs/Cargo.toml

aevyrie · 2024-10-21T22:13:59Z

crates/bevy_ecs/src/query/error.rs

+            Self::NoSuchEntity(entity, world) => {
+                #[cfg(feature = "track_change_detection")]
+                {
+                    if let Some(location) = world.entities().get_entity_spawned_despawned_by(entity)


This is really cool.

bushrat011899

Really nice change, I think this philosophy should be extended to more areas of Bevy too (e.g., Components). However, there is a safety comment which I believe this change violates. Either the PR needs to be amended to satisfy the invariant, or a SAFETY comment should be added to the field to indicate why it is safe.

crates/bevy_ecs/src/entity/mod.rs

bushrat011899 · 2024-10-24T21:34:33Z

crates/bevy_ecs/src/query/error.rs

+                }
+                #[cfg(not(feature = "track_change_detection"))]
+                {
+                    write!(f, "The entity {entity} does not exist")


Might be nice to include a hint here to enable track_change_detection for a more detailed error message?

Great idea; implemented. Should I note in the feature description (in Cargo.toml) that this feature has a perf impact so people don't ship with it on?

Yeah that's a good idea. And like the tracing filter level features, we should include a note saying library authors should not enable this feature on their published crates, as it removes the ability for end-users to choose themselves.

crates/bevy_ecs/src/world/entity_ref.rs

crates/bevy_ecs/src/world/unsafe_world_cell.rs

crates/bevy_ecs/src/world/entity_ref.rs

# Objective `flush_and_reserve_invalid_assuming_no_entities` was made for the old rendering world (which was reset every frame) and is usused since the 0.15 retained rendering world, but wasn't removed yet. It is pub, but is undocumented apart from the safety comment. ## Solution Remove `flush_and_reserve_invalid_assuming_no_entities` and the safety invariants this method required for `EntityMeta`, `EntityLocation`, `TableId` and `TableRow`. This reduces the amount of unsafe code & safety invariants and makes #16047 easier. ## Alternatives - Document `flush_and_reserve_invalid_assuming_no_entities` and keep it unchanged - Document `flush_and_reserve_invalid_assuming_no_entities` and change it to be based on `EntityMeta::INVALID` ## Migration Guide - exchange `Entities::flush_and_reserve_invalid_assuming_no_entities` for `reserve` and `flush_as_invalid` and notify us if that's insufficient --------- Co-authored-by: Benjamin Brienen <[email protected]>

bushrat011899

Looks great!

NthTensor

Fantastic. I've marked this as ready for review.

alice-i-cecile requested a review from aevyrie October 21, 2024 21:50

alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Oct 21, 2024

alice-i-cecile added this to the 0.16 milestone Oct 21, 2024

alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 21, 2024

aevyrie approved these changes Oct 21, 2024

View reviewed changes

aevyrie mentioned this pull request Oct 24, 2024

Add std::panic::Location field to Entities in debug builds #16070

Closed

bushrat011899 suggested changes Oct 24, 2024

View reviewed changes

BenjaminBrienen approved these changes Oct 31, 2024

View reviewed changes

crates/bevy_ecs/src/world/entity_ref.rs Show resolved Hide resolved

SpecificProtagonist mentioned this pull request Nov 21, 2024

Remove flush_and_reserve_invalid_assuming_no_entities #16460

Merged

SpecificProtagonist added 18 commits December 3, 2024 21:12

track despawns

d66bc02

fix

4769197

fix unused without feature

218b08e

*sigh*

2004f15

fix doctest; adjust feature description

4236af1

cargo run -p build-templated-pages -- update features

a5bee6e

doc links

bb26b83

QueryEntityError: Mention feature

f99b26f

fix outdated doc

02b2a79

function names

5768b81

error message: hint at feature

1b31a81

Improve B0003 error message

59fd3b8

wrap reference in option

fd3d506

core instead of std

e4283df

fix lint

96b391f

reorder despawn vs hooks/observers

a2c9c40

function naming

d32738f

remove safety invariant from EntityLocation

55931b1

typo

85dc8ca

SpecificProtagonist force-pushed the track-entity-spawn-despawn branch from 451ffc3 to 85dc8ca Compare December 3, 2024 20:15

bushrat011899 approved these changes Dec 3, 2024

View reviewed changes

SpecificProtagonist mentioned this pull request Dec 11, 2024

Comprehensive Source Tracking #16775

Open

6 tasks

NthTensor 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 Dec 11, 2024

NthTensor approved these changes Dec 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

track_change_detection: Also track spawns/despawns #16047

track_change_detection: Also track spawns/despawns #16047

SpecificProtagonist commented Oct 21, 2024 •

edited

Loading

aevyrie Oct 21, 2024

bushrat011899 left a comment

bushrat011899 Oct 24, 2024

SpecificProtagonist Oct 25, 2024 •

edited

Loading

bushrat011899 Oct 25, 2024

bushrat011899 left a comment

NthTensor left a comment

track_change_detection: Also track spawns/despawns #16047

Are you sure you want to change the base?

track_change_detection: Also track spawns/despawns #16047

Conversation

SpecificProtagonist commented Oct 21, 2024 • edited Loading

Objective

Solution

Testing

Showcase

aevyrie Oct 21, 2024

Choose a reason for hiding this comment

bushrat011899 left a comment

Choose a reason for hiding this comment

bushrat011899 Oct 24, 2024

Choose a reason for hiding this comment

SpecificProtagonist Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

bushrat011899 Oct 25, 2024

Choose a reason for hiding this comment

bushrat011899 left a comment

Choose a reason for hiding this comment

NthTensor left a comment

Choose a reason for hiding this comment

SpecificProtagonist commented Oct 21, 2024 •

edited

Loading

SpecificProtagonist Oct 25, 2024 •

edited

Loading