Skip to content

Commit

Permalink
Track callsite for observers & hooks (#15607)
Browse files Browse the repository at this point in the history 
# Objective

Fixes #14708

Also fixes some commands not updating tracked location.


## Solution

`ObserverTrigger` has a new `caller` field with the
`track_change_detection` feature;
hooks take an additional caller parameter (which is `Some(…)` or `None`
depending on the feature).

## Testing

See the new tests in `src/observer/mod.rs`

---

## Showcase

Observers now know from where they were triggered (if
`track_change_detection` is enabled):
```rust
world.observe(move |trigger: Trigger<OnAdd, Foo>| {
    println!("Added Foo from {}", trigger.caller());
});
```

## Migration

- hooks now take an additional `Option<&'static Location>` argument

---------

Co-authored-by: Alice Cecile <[email protected]>
  • Loading branch information
@SpecificProtagonist @alice-i-cecile
SpecificProtagonist and alice-i-cecile authored Jan 22, 2025
1 parent 5c43890 commit f32a6fb
Show file tree
Hide file tree
Showing 26 changed files with 789 additions and 133 deletions.
7 changes: 5 additions & 2 deletions crates/bevy_core_pipeline/src/oit/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,13 @@ impl Component for OrderIndependentTransparencySettings {
type Mutability = Mutable;

fn register_component_hooks(hooks: &mut ComponentHooks) {
hooks.on_add(|world, entity, _| {
hooks.on_add(|world, entity, _, caller| {
if let Some(value) = world.get::<OrderIndependentTransparencySettings>(entity) {
if value.layer_count > 32 {
warn!("OrderIndependentTransparencySettings layer_count set to {} might be too high.", value.layer_count);
warn!("{}OrderIndependentTransparencySettings layer_count set to {} might be too high.",
caller.map(|location|format!("{location}: ")).unwrap_or_default(),
value.layer_count
);
}
}
});
Expand Down
90 changes: 68 additions & 22 deletions crates/bevy_ecs/src/bundle.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1058,12 +1058,16 @@ impl<'w> BundleInserter<'w> {
ON_REPLACE,
entity,
archetype_after_insert.iter_existing(),
#[cfg(feature = "track_location")]
caller,
);
}
deferred_world.trigger_on_replace(
archetype,
entity,
archetype_after_insert.iter_existing(),
#[cfg(feature = "track_location")]
caller,
);
}
}
Expand Down Expand Up @@ -1236,12 +1240,16 @@ impl<'w> BundleInserter<'w> {
new_archetype,
entity,
archetype_after_insert.iter_added(),
#[cfg(feature = "track_location")]
caller,
);
if new_archetype.has_add_observer() {
deferred_world.trigger_observers(
ON_ADD,
entity,
archetype_after_insert.iter_added(),
#[cfg(feature = "track_location")]
caller,
);
}
match insert_mode {
Expand All @@ -1251,12 +1259,16 @@ impl<'w> BundleInserter<'w> {
new_archetype,
entity,
archetype_after_insert.iter_inserted(),
#[cfg(feature = "track_location")]
caller,
);
if new_archetype.has_insert_observer() {
deferred_world.trigger_observers(
ON_INSERT,
entity,
archetype_after_insert.iter_inserted(),
#[cfg(feature = "track_location")]
caller,
);
}
}
Expand All @@ -1267,12 +1279,16 @@ impl<'w> BundleInserter<'w> {
new_archetype,
entity,
archetype_after_insert.iter_added(),
#[cfg(feature = "track_location")]
caller,
);
if new_archetype.has_insert_observer() {
deferred_world.trigger_observers(
ON_INSERT,
entity,
archetype_after_insert.iter_added(),
#[cfg(feature = "track_location")]
caller,
);
}
}
Expand Down Expand Up @@ -1348,6 +1364,7 @@ impl<'w> BundleSpawner<'w> {
/// # Safety
/// `entity` must be allocated (but non-existent), `T` must match this [`BundleInfo`]'s type
#[inline]
#[track_caller]
pub unsafe fn spawn_non_existent<T: DynamicBundle>(
&mut self,
entity: Entity,
Expand Down Expand Up @@ -1395,24 +1412,32 @@ impl<'w> BundleSpawner<'w> {
archetype,
entity,
bundle_info.iter_contributed_components(),
#[cfg(feature = "track_location")]
caller,
);
if archetype.has_add_observer() {
deferred_world.trigger_observers(
ON_ADD,
entity,
bundle_info.iter_contributed_components(),
#[cfg(feature = "track_location")]
caller,
);
}
deferred_world.trigger_on_insert(
archetype,
entity,
bundle_info.iter_contributed_components(),
#[cfg(feature = "track_location")]
caller,
);
if archetype.has_insert_observer() {
deferred_world.trigger_observers(
ON_INSERT,
entity,
bundle_info.iter_contributed_components(),
#[cfg(feature = "track_location")]
caller,
);
}
};
Expand Down Expand Up @@ -1681,6 +1706,7 @@ mod tests {
use crate as bevy_ecs;
use crate::{component::ComponentId, prelude::*, world::DeferredWorld};
use alloc::vec;
use core::panic::Location;

#[derive(Component)]
struct A;
Expand All @@ -1689,19 +1715,39 @@ mod tests {
#[component(on_add = a_on_add, on_insert = a_on_insert, on_replace = a_on_replace, on_remove = a_on_remove)]
struct AMacroHooks;

fn a_on_add(mut world: DeferredWorld, _: Entity, _: ComponentId) {
fn a_on_add(
mut world: DeferredWorld,
_: Entity,
_: ComponentId,
_: Option<&'static Location<'static>>,
) {
world.resource_mut::<R>().assert_order(0);
}

fn a_on_insert<T1, T2>(mut world: DeferredWorld, _: T1, _: T2) {
fn a_on_insert<T1, T2>(
mut world: DeferredWorld,
_: T1,
_: T2,
_: Option<&'static Location<'static>>,
) {
world.resource_mut::<R>().assert_order(1);
}

fn a_on_replace<T1, T2>(mut world: DeferredWorld, _: T1, _: T2) {
fn a_on_replace<T1, T2>(
mut world: DeferredWorld,
_: T1,
_: T2,
_: Option<&'static Location<'static>>,
) {
world.resource_mut::<R>().assert_order(2);
}

fn a_on_remove<T1, T2>(mut world: DeferredWorld, _: T1, _: T2) {
fn a_on_remove<T1, T2>(
mut world: DeferredWorld,
_: T1,
_: T2,
_: Option<&'static Location<'static>>,
) {
world.resource_mut::<R>().assert_order(3);
}

Expand Down Expand Up @@ -1734,10 +1780,10 @@ mod tests {
world.init_resource::<R>();
world
.register_component_hooks::<A>()
.on_add(|mut world, _, _| world.resource_mut::<R>().assert_order(0))
.on_insert(|mut world, _, _| world.resource_mut::<R>().assert_order(1))
.on_replace(|mut world, _, _| world.resource_mut::<R>().assert_order(2))
.on_remove(|mut world, _, _| world.resource_mut::<R>().assert_order(3));
.on_add(|mut world, _, _, _| world.resource_mut::<R>().assert_order(0))
.on_insert(|mut world, _, _, _| world.resource_mut::<R>().assert_order(1))
.on_replace(|mut world, _, _, _| world.resource_mut::<R>().assert_order(2))
.on_remove(|mut world, _, _, _| world.resource_mut::<R>().assert_order(3));

let entity = world.spawn(A).id();
world.despawn(entity);
Expand All @@ -1761,10 +1807,10 @@ mod tests {
world.init_resource::<R>();
world
.register_component_hooks::<A>()
.on_add(|mut world, _, _| world.resource_mut::<R>().assert_order(0))
.on_insert(|mut world, _, _| world.resource_mut::<R>().assert_order(1))
.on_replace(|mut world, _, _| world.resource_mut::<R>().assert_order(2))
.on_remove(|mut world, _, _| world.resource_mut::<R>().assert_order(3));
.on_add(|mut world, _, _, _| world.resource_mut::<R>().assert_order(0))
.on_insert(|mut world, _, _, _| world.resource_mut::<R>().assert_order(1))
.on_replace(|mut world, _, _, _| world.resource_mut::<R>().assert_order(2))
.on_remove(|mut world, _, _, _| world.resource_mut::<R>().assert_order(3));

let mut entity = world.spawn_empty();
entity.insert(A);
Expand All @@ -1778,8 +1824,8 @@ mod tests {
let mut world = World::new();
world
.register_component_hooks::<A>()
.on_replace(|mut world, _, _| world.resource_mut::<R>().assert_order(0))
.on_insert(|mut world, _, _| {
.on_replace(|mut world, _, _, _| world.resource_mut::<R>().assert_order(0))
.on_insert(|mut world, _, _, _| {
if let Some(mut r) = world.get_resource_mut::<R>() {
r.assert_order(1);
}
Expand All @@ -1800,22 +1846,22 @@ mod tests {
world.init_resource::<R>();
world
.register_component_hooks::<A>()
.on_add(|mut world, entity, _| {
.on_add(|mut world, entity, _, _| {
world.resource_mut::<R>().assert_order(0);
world.commands().entity(entity).insert(B);
})
.on_remove(|mut world, entity, _| {
.on_remove(|mut world, entity, _, _| {
world.resource_mut::<R>().assert_order(2);
world.commands().entity(entity).remove::<B>();
});

world
.register_component_hooks::<B>()
.on_add(|mut world, entity, _| {
.on_add(|mut world, entity, _, _| {
world.resource_mut::<R>().assert_order(1);
world.commands().entity(entity).remove::<A>();
})
.on_remove(|mut world, _, _| {
.on_remove(|mut world, _, _, _| {
world.resource_mut::<R>().assert_order(3);
});

Expand All @@ -1832,27 +1878,27 @@ mod tests {
world.init_resource::<R>();
world
.register_component_hooks::<A>()
.on_add(|mut world, entity, _| {
.on_add(|mut world, entity, _, _| {
world.resource_mut::<R>().assert_order(0);
world.commands().entity(entity).insert(B).insert(C);
});

world
.register_component_hooks::<B>()
.on_add(|mut world, entity, _| {
.on_add(|mut world, entity, _, _| {
world.resource_mut::<R>().assert_order(1);
world.commands().entity(entity).insert(D);
});

world
.register_component_hooks::<C>()
.on_add(|mut world, _, _| {
.on_add(|mut world, _, _, _| {
world.resource_mut::<R>().assert_order(3);
});

world
.register_component_hooks::<D>()
.on_add(|mut world, _, _| {
.on_add(|mut world, _, _, _| {
world.resource_mut::<R>().assert_order(2);
});

Expand Down
18 changes: 10 additions & 8 deletions crates/bevy_ecs/src/component.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,14 @@ use bevy_ptr::{OwningPtr, UnsafeCellDeref};
#[cfg(feature = "bevy_reflect")]
use bevy_reflect::Reflect;
use bevy_utils::{HashMap, HashSet, TypeIdMap};
#[cfg(feature = "track_location")]
use core::panic::Location;
use core::{
alloc::Layout,
any::{Any, TypeId},
cell::UnsafeCell,
fmt::Debug,
marker::PhantomData,
mem::needs_drop,
panic::Location,
};
use disqualified::ShortName;
use thiserror::Error;
Expand Down Expand Up @@ -304,6 +303,7 @@ pub use bevy_ecs_macros::require;
/// # use bevy_ecs::world::DeferredWorld;
/// # use bevy_ecs::entity::Entity;
/// # use bevy_ecs::component::ComponentId;
/// # use core::panic::Location;
/// #
/// #[derive(Component)]
/// #[component(on_add = my_on_add_hook)]
Expand All @@ -315,12 +315,12 @@ pub use bevy_ecs_macros::require;
/// // #[component(on_replace = my_on_replace_hook, on_remove = my_on_remove_hook)]
/// struct ComponentA;
///
/// fn my_on_add_hook(world: DeferredWorld, entity: Entity, id: ComponentId) {
/// fn my_on_add_hook(world: DeferredWorld, entity: Entity, id: ComponentId, caller: Option<&Location>) {
/// // ...
/// }
///
/// // You can also omit writing some types using generics.
/// fn my_on_insert_hook<T1, T2>(world: DeferredWorld, _: T1, _: T2) {
/// fn my_on_insert_hook<T1, T2>(world: DeferredWorld, _: T1, _: T2, caller: Option<&Location>) {
/// // ...
/// }
/// ```
Expand Down Expand Up @@ -497,8 +497,10 @@ pub enum StorageType {
SparseSet,
}

/// The type used for [`Component`] lifecycle hooks such as `on_add`, `on_insert` or `on_remove`
pub type ComponentHook = for<'w> fn(DeferredWorld<'w>, Entity, ComponentId);
/// The type used for [`Component`] lifecycle hooks such as `on_add`, `on_insert` or `on_remove`.
/// The caller location is `Some` if the `track_caller` feature is enabled.
pub type ComponentHook =
for<'w> fn(DeferredWorld<'w>, Entity, ComponentId, Option<&'static Location<'static>>);

/// [`World`]-mutating functions that run as part of lifecycle events of a [`Component`].
///
Expand Down Expand Up @@ -535,12 +537,12 @@ pub type ComponentHook = for<'w> fn(DeferredWorld<'w>, Entity, ComponentId);
/// let mut tracked_component_query = world.query::<&MyTrackedComponent>();
/// assert!(tracked_component_query.iter(&world).next().is_none());
///
/// world.register_component_hooks::<MyTrackedComponent>().on_add(|mut world, entity, _component_id| {
/// world.register_component_hooks::<MyTrackedComponent>().on_add(|mut world, entity, _component_id, _caller| {
/// let mut tracked_entities = world.resource_mut::<TrackedEntities>();
/// tracked_entities.0.insert(entity);
/// });
///
/// world.register_component_hooks::<MyTrackedComponent>().on_remove(|mut world, entity, _component_id| {
/// world.register_component_hooks::<MyTrackedComponent>().on_remove(|mut world, entity, _component_id, _caller| {
/// let mut tracked_entities = world.resource_mut::<TrackedEntities>();
/// tracked_entities.0.remove(&entity);
/// });
Expand Down
1 change: 1 addition & 0 deletions crates/bevy_ecs/src/entity/clone_entities.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,7 @@ pub struct EntityCloner {

impl EntityCloner {
/// Clones and inserts components from the `source` entity into `target` entity using the stored configuration.
#[track_caller]
pub fn clone_entity(&mut self, world: &mut World) {
// SAFETY:
// - `source_entity` is read-only.
Expand Down
6 changes: 4 additions & 2 deletions crates/bevy_ecs/src/hierarchy.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ use crate::{
};
use alloc::{format, string::String, vec::Vec};
use bevy_ecs_macros::VisitEntitiesMut;
use core::ops::Deref;
use core::slice;
use core::{ops::Deref, panic::Location};
use disqualified::ShortName;
use log::warn;

Expand Down Expand Up @@ -270,6 +270,7 @@ pub fn validate_parent_has_component<C: Component>(
world: DeferredWorld,
entity: Entity,
_: ComponentId,
caller: Option<&'static Location<'static>>,
) {
let entity_ref = world.entity(entity);
let Some(child_of) = entity_ref.get::<ChildOf>() else {
Expand All @@ -282,8 +283,9 @@ pub fn validate_parent_has_component<C: Component>(
// TODO: print name here once Name lives in bevy_ecs
let name: Option<String> = None;
warn!(
"warning[B0004]: {name} with the {ty_name} component has a parent without {ty_name}.\n\
"warning[B0004]: {}{name} with the {ty_name} component has a parent without {ty_name}.\n\
This will cause inconsistent behaviors! See: https://bevyengine.org/learn/errors/b0004",
caller.map(|c| format!("{c}: ")).unwrap_or_default(),
ty_name = ShortName::of::<C>(),
name = name.map_or_else(
|| format!("Entity {}", entity),
Expand Down
Loading

0 comments on commit f32a6fb

Please sign in to comment.