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

Jump to bottom

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

wants to merge 2 commits into from

Conversation

cBournhonesque
Copy link
Contributor

@cBournhonesque cBournhonesque commented Jul 31, 2024 •
edited
Loading

Objective

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)

@cBournhonesque cBournhonesque added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jul 31, 2024
@cBournhonesque cBournhonesque added this to the 0.15 milestone Jul 31, 2024
@cBournhonesque cBournhonesque added S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Jul 31, 2024
@cBournhonesque cBournhonesque requested a review from NthTensor July 31, 2024 21:31
@alice-i-cecile
Copy link
Member

The triggering rules for observers are quite confusing:

I'd like to see more docs explicitly explaining this.

jrobsonchase
jrobsonchase reviewed Aug 1, 2024
View reviewed changes
Copy link
Contributor

@jrobsonchase jrobsonchase left a comment

Choose a reason for hiding this comment

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

I had a similar change in the works that added impls for various combinations of (<entities>, <components>), but this is more general!

crates/bevy_ecs/src/observer/trigger_event.rs
Comment on lines 102 to 108
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>;
}
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.

@alice-i-cecile alice-i-cecile added D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes and removed D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Aug 1, 2024
@cBournhonesque
Copy link
Contributor Author

The triggering rules for observers are quite confusing:

I'd like to see more docs explicitly explaining this.

This should be independent from this PR I think, right?
Here i'm just adding the possibility to have more flexible trigger targets

@alice-i-cecile alice-i-cecile 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 Oct 8, 2024
@NthTensor
Copy link
Contributor

Kind of wondering if we shouldn't cut this from 0.15 for stability.

@alice-i-cecile alice-i-cecile modified the milestones: 0.15, 0.16 Oct 8, 2024
@cBournhonesque cBournhonesque added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Oct 8, 2024
@alice-i-cecile alice-i-cecile removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Oct 14, 2024
@BenjaminBrienen
Copy link
Contributor

Adopted in #16326

@BenjaminBrienen BenjaminBrienen mentioned this pull request Nov 10, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrobsonchase jrobsonchase jrobsonchase left review comments

@ItsDoot ItsDoot ItsDoot approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@NthTensor NthTensor Awaiting requested review from NthTensor

@james-j-obrien james-j-obrien Awaiting requested review from james-j-obrien

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Adopt-Me The original PR author has no intent to complete this work. Pick me up!
Projects
None yet
Milestone
0.16
Development

Successfully merging this pull request may close these issues.

Observers missing API for triggers targetting components on a given Entity
6 participants
@cBournhonesque @alice-i-cecile @NthTensor @BenjaminBrienen @jrobsonchase @ItsDoot