Support for `With<T: Bundle>` #6516

Zeenobit · 2022-11-07T21:43:50Z

Objective

As far as I'm aware, there is currently no way to use a Bundle as a filter type. This applies to named bundles (i.e. using #[derive(Bundle)]), as well as unnamed bundles, such as (A, B, C, ...).

In other words, this transformation does not exist:

(A, B, C, ...) -> (With<A>, With<B>, With<C>, ...)

This makes the implementation of some generic systems unergonomic because for systems operating on an unknown number of generic components (i.e. a bundle), the user would have to define the filter query and the bundle separately. Basically the workaround is this:

#[derive(Bundle, Default)]
struct SomeBundle {
  a: A,
  b: B,
  ...
}

fn get_or_spawn<T: Bundle + Default, F: WorldQuery>(query: Query<Entity, F>, mut commands: Commands) -> Entity {
  if let Some(entity) = query.iter().next() {
    entity
  } else {
    commands.spawn(T::default()).id()
  }  
}

This is also bad because there is effectively no guarantee that F and the components within T match.

Solution

I propose a way to make this more ergonomic by using an associated type Filter on Bundle, which is automatically constructed by ECS macros. This type can then be used to create filtered queries for any generic Bundle or Component.

In short, this PR allows the same system above to be written as:

fn get_or_spawn<T: Bundle + Default>(query: Query<Entity, With<T>>, mut commands: Commands) -> Entity {
  ...
}

This implementation also works with nested bundles:

{ With<(A, B, C)> = With<((A, B), C) = With<(A, (B, C))> } -> (With<A>, With<B>, With<C>)

This would also be a step towards a cleaner implementation of Entity Kinds (#1634) as described in @JoJoJet 's comment:
#1634 (comment)

Because instead of having to write Entity<(With<A>, With, With<C>, ...)>, we'd be able to write Entity<With<(A, B, C, ...)>>, and use With<T> to derive the WorldQuery needed for working with it.

Changelog

Added Filter associated type to Bundle

DJMcNab · 2022-11-07T21:47:24Z

bevy/crates/bevy_ecs/src/bundle.rs

Lines 39 to 56 in 0b2a0e9

    
           /// Importantly, bundles are only their constituent set of components. 
        
           /// You **should not** use bundles as a unit of behaviour. 
        
           /// The behaviour of your app can only be considered in terms of components, as systems, 
        
           /// which drive the behaviour of a `bevy` application, operate on combinations of 
        
           /// components. 
        
           /// 
        
           /// This rule is also important because multiple bundles may contain the same component type, 
        
           /// calculated in different ways &mdash; adding both of these bundles to one entity 
        
           /// would create incoherent behaviour. 
        
           /// This would be unexpected if bundles were treated as an abstraction boundary, as 
        
           /// the abstraction would be unmaintainable for these cases. 
        
           /// For example, both `Camera3dBundle` and `Camera2dBundle` contain the `CameraRenderGraph` 
        
           /// component, but specifying different render graphs to use. 
        
           /// If the bundles were both added to the same entity, only one of these two bundles would work. 
        
           /// 
        
           /// For this reason, There is intentionally no [`Query`] to match whether an entity 
        
           /// contains the components of a bundle. 
        
           /// Queries should instead only select the components they logically operate on.

Zeenobit · 2022-11-07T21:50:35Z

bevy/crates/bevy_ecs/src/bundle.rs

Lines 39 to 56 in 0b2a0e9

/// Importantly, bundles are only their constituent set of components.

/// You **should not** use bundles as a unit of behaviour.

/// The behaviour of your app can only be considered in terms of components, as systems,

/// which drive the behaviour of a `bevy` application, operate on combinations of

/// components.

///

/// This rule is also important because multiple bundles may contain the same component type,

/// calculated in different ways — adding both of these bundles to one entity

/// would create incoherent behaviour.

/// This would be unexpected if bundles were treated as an abstraction boundary, as

/// the abstraction would be unmaintainable for these cases.

/// For example, both `Camera3dBundle` and `Camera2dBundle` contain the `CameraRenderGraph`

/// component, but specifying different render graphs to use.

/// If the bundles were both added to the same entity, only one of these two bundles would work.

///

/// For this reason, There is intentionally no [`Query`] to match whether an entity

/// contains the components of a bundle.

/// Queries should instead only select the components they logically operate on.

I fully understand the intent behind bundle. This is more to help with implementing generic systems that operate on bundles, not to query bundles. This also doesn't deal with overlapping issues, because With<A> is the same as (With<A>, With<A>). This doesn't allow one to access components of a bundle.

This just allows you to go from (A, B) to (With<A>, With). The fact that we have Bundle implemented for (A, B) allows this to be implemented pretty easily as I've done it.

joseph-gio · 2022-11-07T22:32:40Z

This just allows you to go from (A, B) to (With<A>, With). The fact that we have Bundle implemented for (A, B) allows this to be implemented pretty easily as I've done it.

Not sure how I feel about bundle filters yet, but I think a more natural way of doing it would be to "just" allow With<(A, B)>, i.e. use bundles for With<>.

Zeenobit · 2022-11-07T22:39:45Z

This just allows you to go from (A, B) to (With<A>, With). The fact that we have Bundle implemented for (A, B) allows this to be implemented pretty easily as I've done it.

Not sure how I feel about bundle filters yet, but I think a more natural way of doing it would be to "just" allow With<(A, B)>, i.e. use bundles for With<>.

I like the With<(A, B)> approach. Any hints as to how it could be implemented like that?
Also, given that SomeBundle { A, B } is equivalent to (A, B), would this imply that With<SomeBundle> would also be allowed?

Edit: What about Without<(A, B)>? Would we also want to support that? Same for Added and Changed, but I can't come up with a use case for that.

joseph-gio · 2022-11-07T22:54:05Z

Also, given that SomeBundle { A, B } is equivalent to (A, B), would this imply that With<SomeBundle> would also be allowed?

Yeah, that's why I'm hesitant. Not sure if I'm a fan of allowing With<> for named bundles.

I like the With<(A, B)> approach. Any hints as to how it could be implemented like that?

One way:

Rename With<> -> WithComponent<>, maybe make it #[doc(hidden)]
Define a new With<> filter type, which accepts bundles.
- It can just be a thin wrapper around <T as Bundle>::Filter -- maybe even a type alias?

alice-i-cecile · 2022-11-08T00:01:31Z

Can't we just make With and friends accept a bundle, rather than a component? Which should still work for components, because single components are now bundles.

crates/bevy_ecs/src/lib.rs

alice-i-cecile · 2022-11-08T00:14:12Z

crates/bevy_ecs/src/query/filter.rs

+/// }
+/// # bevy_ecs::system::assert_is_system(compliment_entity_system);
+/// ```
+pub type With<T> = <T as Bundle>::Filter;


Oof, I really don't like this added complexity.

Were you suggesting to just use Bundle as the trait bound for With in your other comment?

Zeenobit · 2022-11-08T00:14:50Z

Can't we just make With and friends accept a bundle, rather than a component? Which should still work for components, because single components are now bundles.

I'm not a big fan of allowing this pattern for anything but With, precisely for the reasons mentioned in @DJMcNab 's comment further up. But I don't see why it wouldn't be possible.

crates/bevy_ecs/src/component.rs

joseph-gio · 2022-11-08T00:17:11Z

An alternative way of doing With<T: Bundle> is to mimic the implementation of WithBundle<>, which has been removed: https://docs.rs/bevy_ecs/0.5.0/src/bevy_ecs/query/filter.rs.html#244

It's probably less efficient, but much less complex

Zeenobit · 2022-11-08T00:29:00Z

An alternative way of doing With<T: Bundle> is to mimic the implementation of WithBundle<>, which has been removed: https://docs.rs/bevy_ecs/0.5.0/src/bevy_ecs/query/filter.rs.html#244

It's probably less efficient, but much less complex

Less efficient in terms of implementation or runtime performance? If it's the latter, I don't think it'd be smart to compromise the performance of With<T: Component> just so we can use With<T: Bundle>. To me, With<(A, B)> should ideally be a zero cost abstraction over (With<A>, With).

Zeenobit · 2022-11-08T00:57:45Z

The other issue we may need to consider is that if this is being used as a stepping stone for entity kinds (#1634), then we would need to provide some ability to "upcast" an Entity<(A, B)> to an Entity<A> and finally an Entity<()>.

I don't think we'd be able to achieve that without using type information at the macro level, because it all has to happen at compile time. So the complexity added here may be necessary for that.

Zeenobit · 2022-11-08T02:52:19Z

Thinking more on this, if instead of With<T: Bundle>, we implement entity kinds as Entity<T: Bundle>, and then implement WorldQuery for Entity<T> as a derivative of Entity and WithBundle<T> (from the snippet JoJo mentioned), then ...

Any query Query<Entity<(A, B, C)>> would be functionally equivalent to Query<Entity, With<(A, B, C)>>, which makes the latter form redundant.

This means the implementation of WithBundle can remain private (its function only accessible via a kinded entity query), and With would still be restricted to T: Component.

Because of this, a better incremental approach may be to just re-introduce WithBundle as a filter again. Any information on why this was removed initially would be helpful.

I would still be hesitant in using WithBundle if it would add a runtime overhead.

joseph-gio · 2022-11-08T04:29:42Z

Because of this, a better incremental approach may be to just re-introduce WithBundle as a filter again. Any information on why this was removed initially would be helpful.

Seems like it was removed due to #2620

Zeenobit · 2022-11-08T05:38:53Z

Given the discussion in #2620, I understand the objection to WithBundle and supporting With<T: Bundle> by extension.

So then would it be correct to assume kinded entities would also be an "anti-feature" as a result, because it's effectively the same problem/solution with a different syntax?

With the way bundles are implemented as-is in Bevy (i.e. (A, B) = Foo { A, B }), if we want to support Entity<With<(A, B)>> or Entity<(A, B)>, it means we have to support Entity<With<Foo>> or Entity<Foo>.

DJMcNab · 2022-11-08T06:42:39Z

Not necessarily; we could implement a trait over T: Component and tuples of other implementations for things which can be used within With.

That way With<A> and with With<(A, B)> would work, but not With<Foo>

Zeenobit · 2022-11-08T06:56:12Z

Not necessarily; we could implement a trait over T: Component and tuples of other implementations for things which can be used within With.

That way With<A> and with With<(A, B)> would work, but not With<Foo>

I don't quite see the benefit in the increased complexity of this approach (i.e. adding a new tuple of components that are like bundles in syntax, but not quite in usage), when the user could just do this to get what they want anyways:

type Foo = (A, B);
fn system(query: Query<(), With<Foo>>) { }

Putting artificial barriers in front of the user or taking power away from them because we don't trust in their ability to use the engine correctly, or because it may lead to poor OOP habits is a very pessimistic view of the user. :(

Edit: I'm not even sure if your suggestion would be possible without causing ambiguity in the compiler. I already thought about doing something similar initially. The only way I can think of doing that without causing ambiguous cases would be do have a IS_NAMED_BUNDLE flag or marker type on bundles that's populated by the macros.

joseph-gio · 2022-11-08T13:46:58Z

I don't quite see the benefit in the increased complexity of this approach (i.e. adding a new tuple of components that are like bundles in syntax, but not quite in usage)

It's not increasing complexity, so much as moving it to a different trait.

Here's a very rough example of how it could work: joseph-gio@6271a94

Now, this approach doesn't pave the way for entity kinds, but I don't think it needs to. We can always cross that bridge when we come to it.

Zeenobit · 2022-11-08T14:34:47Z

The implementation does make sense. And it does make the query_with_named_bundle_filter test fail as expected. While this is simpler than I imagined, I still don't think there is a gain in having a dedicated AnonymousBundle type. I think that's where the complexity lies, in terms of API, not the implementation.

And again, this is all to prevent the user from using named bundles as filters, when the user could just define their bundles like this:

type MyBundle = (A, B, C);

Which is even less verbose than:

#[derive(Bundle)]
struct MyBundle {
  a: A,
  b: B,
}

So to me, we'd just be adding this artificial barrier for the user because we decided that anonymous and named bundles are mostly the same, but not quite.

Instead, if we do allow Bundle filters without restriction on named bundles, we'd empower the user to decide when to use named bundles as filters, or otherwise, as needed.

If we don't want to allow named Bundle filters, then I propose we abandon the feature entirely, because it just feels like artificial complexity (however small) to enable that distinction.

Edit: Alternatively, we could also approach the problem by removing named bundles, and making all bundles be anonymous, with "named" ones being simple type declarations. Even that would be a better solution in my opinion. If we don't want to allow the user to use the names of bundles for anything other than to spawn/insert them, why do we have names for bundles to begin with?

joseph-gio · 2022-11-08T16:03:08Z

And again, this is all to prevent the user from using named bundles as filters, when the user could just define their bundles like this:
type MyBundle = (A, B, C);

That doesn't quite work. Type aliases can't be used as constructors, which means you can't do commands.spawn(MyBundle(a, b, c)). Named bundles also have the benefit of letting you define custom new functions, which you can't do for anonymous bundles.

You might be right about complexity, but I'm not really opinionated enough to comment on it.

Zeenobit · 2022-11-08T16:25:22Z

That doesn't quite work. Type aliases can't be used as constructors, which means you can't do commands.spawn(MyBundle(a, b, c)). Named bundles also have the benefit of letting you define custom new functions, which you can't do for anonymous bundles.

Until the user does this:

pub fn new_my_bundle(...) -> MyBundle { ... }

With your changes, this also compiles/passes just fine:

#[derive(Component, Default)]
struct Foo;

#[derive(Component, Default)]
struct Bar;

// #[derive(Bundle, Default)]
// struct FooBundle {
//     foo: Foo,
//     bar: Bar,
// }

type FooBundle2 = (Foo, Bar);

let mut world = World::new();
let e = world.spawn(FooBundle2::default()).id();

let mut q = world.query_filtered::<Entity, With<FooBundle2>>();

assert_eq!(q.single(&world), e);

At this point I'm leaning towards just abandoning the idea because I don't think it justifies this arbitrary distinction between Bundle and AnonymousBundle.

Let me know if you'd like me to update the PR with AnonymousBundle. But otherwise I'm gonna leave it open in case we want to revisit it in future.

alice-i-cecile · 2022-11-08T18:05:30Z

I don't think it justifies this arbitrary distinction between Bundle and AnonymousBundle.

Agree here, this distinction is fiddly and just going to frustrate users.

Zeenobit added 4 commits November 7, 2022 15:34

Add Component::Filter and Bundle::Filter

2a1fe0c

Add query_with_incompatible_bundle_filter test

6d896c3

Add named bundle filter tests

0b2a0e9

Fix formatting

7ef4791

alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR labels Nov 7, 2022

Allow using Bundle as With type parameter

2d45a77

Zeenobit changed the title ~~Associated Component/Bundle Filter Type~~ Support for With<Bundle> Nov 8, 2022

alice-i-cecile reviewed Nov 8, 2022

View reviewed changes

crates/bevy_ecs/src/lib.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Nov 8, 2022

View reviewed changes

crates/bevy_ecs/src/lib.rs Show resolved Hide resolved

alice-i-cecile reviewed Nov 8, 2022

View reviewed changes

joseph-gio reviewed Nov 8, 2022

View reviewed changes

crates/bevy_ecs/src/component.rs Outdated Show resolved Hide resolved

Zeenobit changed the title ~~Support for With<Bundle>~~ Support for With<T: Bundle> Nov 8, 2022

Zeenobit added 4 commits November 7, 2022 19:33

Remove Component::Filter

dd1264c

Fix bundle filter tests

d33f999

Hide Bundle::Filter from docs

2ed18f8

Remove redundant as ExtractComponent

2dcc4d9

Zeenobit closed this by deleting the head repository Dec 13, 2022

Uh oh!

Support for With<T: Bundle> #6516

Support for With<T: Bundle> #6516

Uh oh!

Conversation

Zeenobit commented Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Changelog

Uh oh!

DJMcNab commented Nov 7, 2022

Uh oh!

Zeenobit commented Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joseph-gio commented Nov 7, 2022

Uh oh!

Zeenobit commented Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joseph-gio commented Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alice-i-cecile commented Nov 8, 2022

Uh oh!

Uh oh!

Uh oh!

alice-i-cecile Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

Zeenobit Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

Zeenobit commented Nov 8, 2022

Uh oh!

Uh oh!

joseph-gio commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zeenobit commented Nov 8, 2022

Uh oh!

Zeenobit commented Nov 8, 2022

Uh oh!

Zeenobit commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joseph-gio commented Nov 8, 2022

Uh oh!

Zeenobit commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DJMcNab commented Nov 8, 2022

Uh oh!

Zeenobit commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joseph-gio commented Nov 8, 2022

Uh oh!

Zeenobit commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joseph-gio commented Nov 8, 2022

Uh oh!

Zeenobit commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alice-i-cecile commented Nov 8, 2022

Uh oh!

Uh oh!

Support for `With<T: Bundle>` #6516

Support for `With<T: Bundle>` #6516

Zeenobit commented Nov 7, 2022 •

edited

Loading

Zeenobit commented Nov 7, 2022 •

edited

Loading

Zeenobit commented Nov 7, 2022 •

edited

Loading

joseph-gio commented Nov 7, 2022 •

edited

Loading

joseph-gio commented Nov 8, 2022 •

edited

Loading

Zeenobit commented Nov 8, 2022 •

edited

Loading

Zeenobit commented Nov 8, 2022 •

edited

Loading

Zeenobit commented Nov 8, 2022 •

edited

Loading

Zeenobit commented Nov 8, 2022 •

edited

Loading

Zeenobit commented Nov 8, 2022 •

edited

Loading