From 20813aed641225362c368193cf576745c94e288d Mon Sep 17 00:00:00 2001 From: Zachary Harrold Date: Tue, 25 Feb 2025 10:57:34 +1100 Subject: [PATCH] Handle `TriggerTargets` that are combinations for components/entities (#18024) # Objective * Fixes https://github.com/bevyengine/bevy/issues/14074 * Applies CI fixes for #16326 It is currently not possible to issues a trigger that targets a specific list of components AND a specific list of entities ## Solution We can now use `((A, B), (entity_1, entity_2))` as a trigger target, as well as the reverse ## Testing Added a unit test. The triggering rules for observers are quite confusing: Triggers once per entity target For each entity target, an observer system triggers if any of its components matches the trigger target components (but it triggers at most once, since we use an internal counter to make sure that an observer can run at most once per entity target) (copied from #14563) (copied from #16326) ## Notes All credit to @BenjaminBrienen and @cBournhonesque! Just applying a small fix to this PR so it can be merged. --------- Co-authored-by: Benjamin Brienen Co-authored-by: Christian Hughes Co-authored-by: Alice Cecile --- benches/benches/bevy_ecs/observers/simple.rs | 8 +- crates/bevy_ecs/src/observer/mod.rs | 237 ++++++++++++++----- crates/bevy_ecs/src/storage/sparse_set.rs | 6 +- crates/bevy_ecs/src/world/deferred_world.rs | 4 +- 4 files changed, 195 insertions(+), 60 deletions(-) diff --git a/benches/benches/bevy_ecs/observers/simple.rs b/benches/benches/bevy_ecs/observers/simple.rs index bf2dd236d6..85207624e8 100644 --- a/benches/benches/bevy_ecs/observers/simple.rs +++ b/benches/benches/bevy_ecs/observers/simple.rs @@ -1,6 +1,10 @@ use core::hint::black_box; -use bevy_ecs::{entity::Entity, event::Event, observer::Trigger, world::World}; +use bevy_ecs::{ + event::Event, + observer::{Trigger, TriggerTargets}, + world::World, +}; use criterion::Criterion; use rand::{prelude::SliceRandom, SeedableRng}; @@ -46,6 +50,6 @@ fn empty_listener_base(trigger: Trigger) { black_box(trigger); } -fn send_base_event(world: &mut World, entities: &Vec) { +fn send_base_event(world: &mut World, entities: impl TriggerTargets) { world.trigger_targets(EventBase, entities); } diff --git a/crates/bevy_ecs/src/observer/mod.rs b/crates/bevy_ecs/src/observer/mod.rs index d0aa364606..0c40a7f5e3 100644 --- a/crates/bevy_ecs/src/observer/mod.rs +++ b/crates/bevy_ecs/src/observer/mod.rs @@ -5,6 +5,7 @@ mod runner; pub use entity_observer::ObservedBy; pub use runner::*; +use variadics_please::all_tuples; use crate::{ archetype::ArchetypeFlags, @@ -177,92 +178,108 @@ impl<'w, E, B: Bundle> DerefMut for Trigger<'w, E, B> { /// will run. pub trait TriggerTargets { /// The components the trigger should target. - fn components(&self) -> &[ComponentId]; + fn components(&self) -> impl Iterator + Clone + '_; /// The entities the trigger should target. - fn entities(&self) -> &[Entity]; + fn entities(&self) -> impl Iterator + Clone + '_; } -impl TriggerTargets for () { - fn components(&self) -> &[ComponentId] { - &[] +impl TriggerTargets for &T { + fn components(&self) -> impl Iterator + Clone + '_ { + (**self).components() } - fn entities(&self) -> &[Entity] { - &[] + fn entities(&self) -> impl Iterator + Clone + '_ { + (**self).entities() } } impl TriggerTargets for Entity { - fn components(&self) -> &[ComponentId] { - &[] + fn components(&self) -> impl Iterator + Clone + '_ { + [].into_iter() } - fn entities(&self) -> &[Entity] { - core::slice::from_ref(self) - } -} - -impl TriggerTargets for Vec { - fn components(&self) -> &[ComponentId] { - &[] - } - - fn entities(&self) -> &[Entity] { - self.as_slice() - } -} - -impl TriggerTargets for [Entity; N] { - fn components(&self) -> &[ComponentId] { - &[] - } - - fn entities(&self) -> &[Entity] { - self.as_slice() + fn entities(&self) -> impl Iterator + Clone + '_ { + core::iter::once(*self) } } impl TriggerTargets for ComponentId { - fn components(&self) -> &[ComponentId] { - core::slice::from_ref(self) + fn components(&self) -> impl Iterator + Clone + '_ { + core::iter::once(*self) } - fn entities(&self) -> &[Entity] { - &[] + fn entities(&self) -> impl Iterator + Clone + '_ { + [].into_iter() } } -impl TriggerTargets for Vec { - fn components(&self) -> &[ComponentId] { - self.as_slice() +impl TriggerTargets for Vec { + fn components(&self) -> impl Iterator + Clone + '_ { + self.iter().flat_map(T::components) } - fn entities(&self) -> &[Entity] { - &[] + fn entities(&self) -> impl Iterator + Clone + '_ { + self.iter().flat_map(T::entities) } } -impl TriggerTargets for [ComponentId; N] { - fn components(&self) -> &[ComponentId] { - self.as_slice() +impl TriggerTargets for [T; N] { + fn components(&self) -> impl Iterator + Clone + '_ { + self.iter().flat_map(T::components) } - fn entities(&self) -> &[Entity] { - &[] + fn entities(&self) -> impl Iterator + Clone + '_ { + self.iter().flat_map(T::entities) } } -impl TriggerTargets for &Vec { - fn components(&self) -> &[ComponentId] { - &[] +impl TriggerTargets for [T] { + fn components(&self) -> impl Iterator + Clone + '_ { + self.iter().flat_map(T::components) } - fn entities(&self) -> &[Entity] { - self.as_slice() + fn entities(&self) -> impl Iterator + Clone + '_ { + self.iter().flat_map(T::entities) } } +macro_rules! impl_trigger_targets_tuples { + ($(#[$meta:meta])* $($trigger_targets: ident),*) => { + #[expect(clippy::allow_attributes, reason = "can't guarantee violation of non_snake_case")] + #[allow(non_snake_case, reason = "`all_tuples!()` generates non-snake-case variable names.")] + $(#[$meta])* + impl<$($trigger_targets: TriggerTargets),*> TriggerTargets for ($($trigger_targets,)*) + { + fn components(&self) -> impl Iterator + Clone + '_ { + let iter = [].into_iter(); + let ($($trigger_targets,)*) = self; + $( + let iter = iter.chain($trigger_targets.components()); + )* + iter + } + + fn entities(&self) -> impl Iterator + Clone + '_ { + let iter = [].into_iter(); + let ($($trigger_targets,)*) = self; + $( + let iter = iter.chain($trigger_targets.entities()); + )* + iter + } + } + } +} + +all_tuples!( + #[doc(fake_variadic)] + impl_trigger_targets_tuples, + 0, + 15, + T +); + /// A description of what an [`Observer`] observes. #[derive(Default, Clone)] pub struct ObserverDescriptor { @@ -673,7 +690,8 @@ impl World { caller: MaybeLocation, ) { let mut world = DeferredWorld::from(self); - if targets.entities().is_empty() { + let mut entity_targets = targets.entities().peekable(); + if entity_targets.peek().is_none() { // SAFETY: `event_data` is accessible as the type represented by `event_id` unsafe { world.trigger_observers_with_data::<_, E::Traversal>( @@ -686,12 +704,12 @@ impl World { ); }; } else { - for target in targets.entities() { + for target_entity in entity_targets { // SAFETY: `event_data` is accessible as the type represented by `event_id` unsafe { world.trigger_observers_with_data::<_, E::Traversal>( event_id, - *target, + target_entity, targets.components(), event_data, E::AUTO_PROPAGATE, @@ -1209,6 +1227,119 @@ mod tests { assert_eq!(vec!["a_2", "a_1"], world.resource::().0); } + #[test] + fn observer_multiple_targets() { + #[derive(Resource, Default)] + struct R(i32); + + let mut world = World::new(); + let component_a = world.register_component::(); + let component_b = world.register_component::(); + world.init_resource::(); + + // targets (entity_1, A) + let entity_1 = world + .spawn_empty() + .observe(|_: Trigger, mut res: ResMut| res.0 += 1) + .id(); + // targets (entity_2, B) + let entity_2 = world + .spawn_empty() + .observe(|_: Trigger, mut res: ResMut| res.0 += 10) + .id(); + // targets any entity or component + world.add_observer(|_: Trigger, mut res: ResMut| res.0 += 100); + // targets any entity, and components A or B + world.add_observer(|_: Trigger, mut res: ResMut| res.0 += 1000); + // test all tuples + world.add_observer(|_: Trigger, mut res: ResMut| res.0 += 10000); + world.add_observer( + |_: Trigger, mut res: ResMut| { + res.0 += 100000; + }, + ); + world.add_observer( + |_: Trigger, + mut res: ResMut| res.0 += 1000000, + ); + + // WorldEntityMut does not automatically flush. + world.flush(); + + // trigger for an entity and a component + world.trigger_targets(EventA, (entity_1, component_a)); + world.flush(); + // only observer that doesn't trigger is the one only watching entity_2 + assert_eq!(1111101, world.resource::().0); + world.resource_mut::().0 = 0; + + // trigger for both entities, but no components: trigger once per entity target + world.trigger_targets(EventA, (entity_1, entity_2)); + world.flush(); + // only the observer that doesn't require components triggers - once per entity + assert_eq!(200, world.resource::().0); + world.resource_mut::().0 = 0; + + // trigger for both components, but no entities: trigger once + world.trigger_targets(EventA, (component_a, component_b)); + world.flush(); + // all component observers trigger, entities are not observed + assert_eq!(1111100, world.resource::().0); + world.resource_mut::().0 = 0; + + // trigger for both entities and both components: trigger once per entity target + // we only get 2222211 because a given observer can trigger only once per entity target + world.trigger_targets(EventA, ((component_a, component_b), (entity_1, entity_2))); + world.flush(); + assert_eq!(2222211, world.resource::().0); + world.resource_mut::().0 = 0; + + // trigger to test complex tuples: (A, B, (A, B)) + world.trigger_targets( + EventA, + (component_a, component_b, (component_a, component_b)), + ); + world.flush(); + // the duplicate components in the tuple don't cause multiple triggers + assert_eq!(1111100, world.resource::().0); + world.resource_mut::().0 = 0; + + // trigger to test complex tuples: (A, B, (A, B), ((A, B), (A, B))) + world.trigger_targets( + EventA, + ( + component_a, + component_b, + (component_a, component_b), + ((component_a, component_b), (component_a, component_b)), + ), + ); + world.flush(); + // the duplicate components in the tuple don't cause multiple triggers + assert_eq!(1111100, world.resource::().0); + world.resource_mut::().0 = 0; + + // trigger to test the most complex tuple: (A, B, (A, B), (B, A), (A, B, ((A, B), (B, A)))) + world.trigger_targets( + EventA, + ( + component_a, + component_b, + (component_a, component_b), + (component_b, component_a), + ( + component_a, + component_b, + ((component_a, component_b), (component_b, component_a)), + ), + ), + ); + world.flush(); + // the duplicate components in the tuple don't cause multiple triggers + assert_eq!(1111100, world.resource::().0); + world.resource_mut::().0 = 0; + } + #[test] fn observer_dynamic_component() { let mut world = World::new(); diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index 671cd7c161..bb79382e06 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -722,10 +722,10 @@ mod tests { assert_eq!(sets.len(), 0); assert!(sets.is_empty()); - init_component::(&mut sets, 1); + register_component::(&mut sets, 1); assert_eq!(sets.len(), 1); - init_component::(&mut sets, 2); + register_component::(&mut sets, 2); assert_eq!(sets.len(), 2); // check its shape by iter @@ -739,7 +739,7 @@ mod tests { vec![(ComponentId::new(1), 0), (ComponentId::new(2), 0),] ); - fn init_component(sets: &mut SparseSets, id: usize) { + fn register_component(sets: &mut SparseSets, id: usize) { let descriptor = ComponentDescriptor::new::(); let id = ComponentId::new(id); let info = ComponentInfo::new(id, descriptor); diff --git a/crates/bevy_ecs/src/world/deferred_world.rs b/crates/bevy_ecs/src/world/deferred_world.rs index 654d35266e..581f1af25e 100644 --- a/crates/bevy_ecs/src/world/deferred_world.rs +++ b/crates/bevy_ecs/src/world/deferred_world.rs @@ -674,7 +674,7 @@ impl<'w> DeferredWorld<'w> { &mut self, event: ComponentId, mut target: Entity, - components: &[ComponentId], + components: impl Iterator + Clone, data: &mut E, mut propagate: bool, caller: MaybeLocation, @@ -686,7 +686,7 @@ impl<'w> DeferredWorld<'w> { self.reborrow(), event, target, - components.iter().copied(), + components.clone(), data, &mut propagate, caller,