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.

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

Replace Trigger's default bundle filtering behavior from an OR to an AND #15325

Open
ItsDoot opened this issue Sep 20, 2024 · 2 comments
Open
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior D-Complex Quite challenging from either a design or technical perspective. Ask for help!

Comments

@ItsDoot
Copy link
Contributor

ItsDoot commented Sep 20, 2024

What problem does this solve or what need does it fill?

The fact that the B in Trigger<E, B: Bundle> currently means OR instead of AND is confusing enough that it even surprised a maintainer. Everywhere else in bevy, tuples (which bundles can be) mean AND, so we should change Trigger's default behavior.

What solution would you like?

The following code should work (but doesn't currently):

fn my_observer(trigger: Trigger<Foo, (A, B)>) {

}

// This *should not* trigger the above observer:
world.trigger(Foo, world.component::<A>());
// But *this* should:
world.trigger(Foo, (world.component::<A>(), world.component::<B>());

Additional context

Moment of confusion on discord.

@ItsDoot ItsDoot added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Sep 20, 2024
@alice-i-cecile
Copy link
Member

We should add a second generic, defaulting to AND, to allow users to get the same behavior back if they want it.

@james-j-obrien
Copy link
Contributor

james-j-obrien commented Sep 20, 2024

Note that the implementation here is difficult as the observer lookup structure does not support this in any way so care will have to be taken not to regress performance when addressing this. I wouldn't consider it straightforward.

@ItsDoot ItsDoot added D-Complex Quite challenging from either a design or technical perspective. Ask for help! and removed D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Sep 20, 2024
@ItsDoot ItsDoot changed the title Replace Trigger's bundle filtering behavior from an OR to an AND. Replace Trigger's default bundle filtering behavior from an OR to an AND Sep 20, 2024
@janhohenheim janhohenheim removed the S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! label Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior D-Complex Quite challenging from either a design or technical perspective. Ask for help!
Projects
None yet
Development

No branches or pull requests

4 participants