Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Track callsite for observers & hooks #15607

Merged
merged 13 commits into from
Jan 22, 2025
Merged

Track callsite for observers & hooks #15607

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
bd68936
track callsites for observers & hooks
SpecificProtagonist Dec 17, 2024
b2fbe41
remove superfluous lifetimes in doctest
SpecificProtagonist Dec 17, 2024
385b43d
resolve conflicts
SpecificProtagonist Jan 8, 2025
3544c71
resolve conflict from updated main
SpecificProtagonist Jan 9, 2025
7e062b5
Merge branch 'main' into track-observer-callsites
SpecificProtagonist Jan 9, 2025
f6151f6
ci
SpecificProtagonist Jan 10, 2025
7f25444
doclink
SpecificProtagonist Jan 10, 2025
b5f5197
resolve conflicts
SpecificProtagonist Jan 19, 2025
4918e71
hierarchy
SpecificProtagonist Jan 19, 2025
0f1c177
fix test
SpecificProtagonist Jan 19, 2025
8933b95
Make trigger caller publicly accessible
SpecificProtagonist Jan 19, 2025
f34cc9f
address review comments
SpecificProtagonist Jan 22, 2025
0c26359
Merge branch 'main' into track-observer-callsites
alice-i-cecile Jan 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions crates/bevy_core_pipeline/src/oit/mod.rs
View file
Open in desktop
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
Open in desktop
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,
Comment on lines +1069 to +1070
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up PR: I think this pattern is pretty annoying to deal with. It's verbose and off-putting (IMO). I think we should investigate if obfuscating this behind a Context type which internally contains this feature gate is easier to use without sacrificing performance.

As much as I'm a no_std advocate, another alternative could be to use thread-local storage to manage this information without explicitly passing it as a function argument.

);
}
}
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading