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

Handle TriggerTargets that are combinations for components/entities #14563

Closed
wants to merge 2 commits into from
Closed

Handle TriggerTargets that are combinations for components/entities #14563

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d1c3b03
handle TriggerTargets that are combinations for components/entities
cBournhonesque Jul 31, 2024
599476c
Merge branch 'main' into cb/fix-observe-panic
alice-i-cecile Oct 8, 2024
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
52 changes: 52 additions & 0 deletions crates/bevy_ecs/src/observer/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -878,6 +878,58 @@ mod tests {
assert_eq!(vec!["a_2", "a_1"], world.resource::<Order>().0);
}

#[test]
fn observer_multiple_targets() {
let mut world = World::new();
let component_a = world.init_component::<A>();
let component_b = world.init_component::<B>();
world.init_resource::<R>();

// targets (entity_1, A)
let entity_1 = world
.spawn_empty()
.observe(|_: Trigger<EventA, A>, mut res: ResMut<R>| res.0 += 1)
.id();
// targets (entity_2, B)
let entity_2 = world
.spawn_empty()
.observe(|_: Trigger<EventA, B>, mut res: ResMut<R>| res.0 += 10)
.id();
// targets any entity or component
world.observe(|_: Trigger<EventA>, mut res: ResMut<R>| res.0 += 100);
// targets any entity, and components A or B
world.observe(|_: Trigger<EventA, (A, B)>, mut res: ResMut<R>| res.0 += 1000);

// TODO: ideally this flush is not necessary, but right now observe() returns WorldEntityMut
// and therefore does not automatically flush.
world.flush();

// trigger for an entity and a component
world.trigger_targets(EventA, (entity_1, component_a));
world.flush();
assert_eq!(1101, world.resource::<R>().0);
world.resource_mut::<R>().0 = 0;

// trigger for both entities, but no components: trigger once per entity target
world.trigger_targets(EventA, (entity_1, entity_2));
world.flush();
assert_eq!(200, world.resource::<R>().0);
world.resource_mut::<R>().0 = 0;

// trigger for both components, but no entities: trigger once
world.trigger_targets(EventA, (component_a, component_b));
world.flush();
assert_eq!(1100, world.resource::<R>().0);
world.resource_mut::<R>().0 = 0;

// trigger for both entities and both components: trigger once per entity target
// we only get 2211 and not 4211 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!(2211, world.resource::<R>().0);
world.resource_mut::<R>().0 = 0;
}

#[test]
fn observer_dynamic_component() {
let mut world = World::new();
Expand Down
77 changes: 39 additions & 38 deletions crates/bevy_ecs/src/observer/trigger_event.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,8 @@ fn trigger_event<E: Event, Targets: TriggerTargets>(
targets: Targets,
) {
let mut world = DeferredWorld::from(world);
if targets.entities().is_empty() {
let mut entity_targets = targets.entities().peekable();
if entity_targets.peek().is_none() {
// SAFETY: T is accessible as the type represented by self.trigger, ensured in `Self::new`
unsafe {
world.trigger_observers_with_data::<_, E::Traversal>(
Expand All @@ -84,12 +85,12 @@ fn trigger_event<E: Event, Targets: TriggerTargets>(
);
};
} else {
for target in targets.entities() {
for target_entity in entity_targets {
// SAFETY: T is accessible as the type represented by self.trigger, ensured in `Self::new`
unsafe {
world.trigger_observers_with_data::<_, E::Traversal>(
event_type,
*target,
target_entity,
targets.components(),
event_data,
E::AUTO_PROPAGATE,
Expand All @@ -108,88 +109,88 @@ fn trigger_event<E: Event, Targets: TriggerTargets>(
/// [`Observer`]: crate::observer::Observer
pub trait TriggerTargets {
/// The components the trigger should target.
fn components(&self) -> &[ComponentId];
fn components(&self) -> impl Iterator<Item = ComponentId> + Clone;

/// The entities the trigger should target.
fn entities(&self) -> &[Entity];
fn entities(&self) -> impl Iterator<Item = Entity>;
}
Comment on lines 110 to 116
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth noting that this effectively reverts the change to this trait from #13991 which made it object-safe. I don't see any discussion for that original change, but if it's being changed back, it probably warrants some. At the very least, we should probably go back to the original impl ExactSizeIterator<...> return types if possible to keep it consistent with 0.14.0 and minimize potential for breakage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I was hoping to see more discussions on the consequences of this change; I wasn't sure why the change was made in the first place.

Note that impl ExactSizeIterator is unfortunately not possible because calling chain on 2 ExactSizeIterator does not return an ExactSizeIterator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could also just provide our own Iterator type to make this object safe

Copy link
Contributor

Choose a reason for hiding this comment

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

After digging into #14775, returning Iterators would simplify its implementation. So I'm in favor of this change.


impl TriggerTargets for () {
fn components(&self) -> &[ComponentId] {
&[]
fn components(&self) -> impl Iterator<Item = ComponentId> + Clone {
[].into_iter()
}

fn entities(&self) -> &[Entity] {
&[]
fn entities(&self) -> impl Iterator<Item = Entity> {
[].into_iter()
}
}

impl TriggerTargets for Entity {
fn components(&self) -> &[ComponentId] {
&[]
fn components(&self) -> impl Iterator<Item = ComponentId> + Clone {
[].into_iter()
}

fn entities(&self) -> &[Entity] {
core::slice::from_ref(self)
fn entities(&self) -> impl Iterator<Item = Entity> {
core::iter::once(*self)
}
}

impl TriggerTargets for Vec<Entity> {
fn components(&self) -> &[ComponentId] {
&[]
fn components(&self) -> impl Iterator<Item = ComponentId> + Clone {
[].into_iter()
}

fn entities(&self) -> &[Entity] {
self.as_slice()
fn entities(&self) -> impl Iterator<Item = Entity> {
self.iter().copied()
}
}

impl<const N: usize> TriggerTargets for [Entity; N] {
fn components(&self) -> &[ComponentId] {
&[]
fn components(&self) -> impl Iterator<Item = ComponentId> + Clone {
[].into_iter()
}

fn entities(&self) -> &[Entity] {
self.as_slice()
fn entities(&self) -> impl Iterator<Item = Entity> {
self.iter().copied()
}
}

impl TriggerTargets for ComponentId {
fn components(&self) -> &[ComponentId] {
core::slice::from_ref(self)
fn components(&self) -> impl Iterator<Item = ComponentId> + Clone {
core::iter::once(*self)
}

fn entities(&self) -> &[Entity] {
&[]
fn entities(&self) -> impl Iterator<Item = Entity> {
[].into_iter()
}
}

impl TriggerTargets for Vec<ComponentId> {
fn components(&self) -> &[ComponentId] {
self.as_slice()
fn components(&self) -> impl Iterator<Item = ComponentId> + Clone {
self.iter().copied()
}

fn entities(&self) -> &[Entity] {
&[]
fn entities(&self) -> impl Iterator<Item = Entity> {
[].into_iter()
}
}

impl<const N: usize> TriggerTargets for [ComponentId; N] {
fn components(&self) -> &[ComponentId] {
self.as_slice()
fn components(&self) -> impl Iterator<Item = ComponentId> + Clone {
self.iter().copied()
}

fn entities(&self) -> &[Entity] {
&[]
fn entities(&self) -> impl Iterator<Item = Entity> {
[].into_iter()
}
}

impl TriggerTargets for &Vec<Entity> {
fn components(&self) -> &[ComponentId] {
&[]
impl<T1: TriggerTargets, T2: TriggerTargets> TriggerTargets for (T1, T2) {
fn components(&self) -> impl Iterator<Item = ComponentId> + Clone {
self.0.components().chain(self.1.components())
}

fn entities(&self) -> &[Entity] {
self.as_slice()
fn entities(&self) -> impl Iterator<Item = Entity> {
self.0.entities().chain(self.1.entities())
}
}
4 changes: 2 additions & 2 deletions crates/bevy_ecs/src/world/deferred_world.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -522,7 +522,7 @@ impl<'w> DeferredWorld<'w> {
&mut self,
event: ComponentId,
mut entity: Entity,
components: &[ComponentId],
components: impl Iterator<Item = ComponentId> + Clone,
data: &mut E,
mut propagate: bool,
) where
Expand All @@ -533,7 +533,7 @@ impl<'w> DeferredWorld<'w> {
self.reborrow(),
event,
entity,
components.iter().copied(),
components.clone(),
data,
&mut propagate,
);
Expand Down
Loading