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

Component Bundle Queries #2252

Closed
aevyrie opened this issue May 25, 2021 · 11 comments
Closed

Component Bundle Queries #2252

aevyrie opened this issue May 25, 2021 · 11 comments
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible C-Performance A change motivated by improving speed, memory usage or compile times C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged

Comments

@aevyrie
Copy link
Member

aevyrie commented May 25, 2021

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

Reduce the verbosity of queries by using bundles, which are already used for this purpose during entity archetype construction.

What solution would you like?

The ability to add a struct that derives Bundle to a query, and have it return with its fields accessible as components. E.g.:

#[derive(Bundle)]
struct Transform {
    translation: Translation,
    rotation: Rotation,
    scale: Scale,
}

// can still use this common case
Query<&Transform>

// but also, able to query fields granularly with change detection
Query<&Transform, Changed<Rotation>>
//...

// Can use this to group other sets of components as query-able bundles
#[derive(Bundle)]
struct Dog {
    legs: Legs
    fluffy_tail: Tail
    wet_nose: Nose
}
// These two queries are functionally equivalent, but the `Dog` query returns a list of `Dog` structs
Query<&Dog>
Query<(&Legs, &Tail, &Nose)>

What alternative(s) have you considered?

Most of this can already be achieved with some verbosity, as shown in the dog example above. However, it would also allow us to split apart common components like Transform into their parts with field level change detection, without losing the ergonomics of the Transform type.

Additional context

Similar to #786

@aevyrie aevyrie added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels May 25, 2021
@alice-i-cecile
Copy link
Member

It seems clear that when operating in this way, With and Without should use AND logic.

However for Added / Changed / Removed (see #2148), the intuitive behavior is actually different.

For Added, it should trigger if and only if the entity now satisfies the With criteria, and did not before. Removed is then the opposite.

For Changed, it should trigger if any of the components are changed, which is a both intuitive and a nice ergonomic win for Or-logic query filtering in the most common cases.

@alice-i-cecile 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 C-Performance A change motivated by improving speed, memory usage or compile times and removed S-Needs-Triage This issue needs to be labelled labels May 25, 2021
@alice-i-cecile
Copy link
Member

alice-i-cecile commented May 25, 2021

This is almost certainly blocked on #1843, as otherwise we have no way to differentiate whether you want to query for a bundle as a bundle or a component. This may result in overlapping impl errors for attempts to have types as both a bundle and a component, but in my book that's a serious win for clarity (even if bevy_rapier uses this pattern right now).

Note that the granular fields (and ability to split components apart) actually allow this to enable nontrivial functionality and performance enhancements. We can get more granular change detection, which is very useful for some expensive algorithms, and also reduce the data that must be accessed when only handling one part of a larger component.

This would also be very useful for things along the lines of the massive Style component, particularly with archetype invariants (#1481) to guard against misconfiguration.

To break down the desired behavior a bit, I think it should work like so when iterated over.

let query = Query<&Transform>

for (translation, rotation, scale) in query.iter(){

}
let query = Query<(&Life, &Transform)>

for (life, transform) in query.iter(){
  transform.rotation += 0.2;
}

And maybe even:

let query = Query<(&Life, &Transform)>

for (life, (translation, rotation, scale)) in query.iter(){
  rotation += 0.2;
}

Note that this syntax is actually likely to result in extremely minimal code breakage for existing users of Transform. Combining this with #1843 would result in serious ergonomic gains, as querying for a bundle would result in an actual effect (if sometimes wasteful), and users could use rust-analyzer's type inference to guide them on what is likely to actually result when the query is unpacked.

@aevyrie
Copy link
Member Author

aevyrie commented May 25, 2021

To break down the desired behavior a bit, I think it should work like so when iterated over.

let query = Query<&Transform>

for (translation, rotation, scale) in query.iter(){

}

I think I'd actually prefer it if the query returned the struct, though I'm not sure which is easier to implement. One reason to return the struct is because you can then implement methods for the bundle, it would also mean this isn't (as much of) a breaking change, because querying a Transform would still return a Transform.

@alice-i-cecile
Copy link
Member

Ah, there was a fourth case I forget to mention:

let query = Query<&Transform>

for transform in query.iter(){

}

As I'm trying to hint at by example, the idea is that the compiler either keeps the struct intact or automatically unpacks it based on the number of arguments it has to match against.

@alice-i-cecile
Copy link
Member

As part of this change, we could remove WithBundle, cleaning up our API in a more consistent way.

@alice-i-cecile
Copy link
Member

Currently the bundle filter will match anything that has all components in a bundle, but imo the filter would be more useful / aligned with user intent if it only matched things that actually added the Bundle.

I really dislike this behavior: it's incredibly magical, special-cases bundles and forces us to store Bundle identity on the entity. Instead, I would prefer to promote the use of "defining" marker components as the first item of each bundle.

This is a natural and useful pattern in end-user code, and you can get the effect you're looking for because no other entities should have that unique marker unless they include the bundle (especially if you don't export the marker's type).

The other benefit of that design is that it dovetails really nicely with the entity inspection patterns discussed in #1467, particularly for a visual editor, where you want to quickly learn the "types" of an entity (in which case you look at the "defining" components of the bundle).

@aevyrie
Copy link
Member Author

aevyrie commented May 26, 2021

I responded in #2255 here, but I will reiterate my perspective here.

In short, my desired behavior for bundle queries is:

Match anything with all components in the bundle (a bundle filter), but return the result from the query as the bundle struct. This retains the current composability of entities, adds the ergonomics of allowing traits/methods over bundles, without the magic of bundles being semantically "special".

I also agree with @alice-i-cecile in that I don't like bundles being "special-cases" or "magical". I mostly don't like that it directly interferes with the composability that we get with only defining archetypes from components.

@mockersf
Copy link
Member

mockersf commented May 26, 2021

let query = Query<&Transform>

for (translation, rotation, scale) in query.iter(){

}

Rust as while let loop that would do this destructuring nicely.

This works today:

let query = Query<&Transform>

while let Some(Transform {translation, rotation, scale}) = query.iter().next() {

}

It gets a little uglier for a mut query though (because we can't destructure Mut):

let mut query = Query<&mut Transform>

while let Some(Transform {
    _translation,
    rotation,
    _scale,
}) = query
    .iter_mut()
    .next()
    .map(|mut_wrapper| Mut::into_inner(mut_wrapper))
{
    *rotation = Quat::from_rotation_z(time.seconds_since_startup().cos() as f32);
}

I think that doing the destructuring ourselves as in @alice-i-cecile example would be very hard (not possible?), as you lose the type relation between the query type and what it returns

@DJMcNab
Copy link
Member

DJMcNab commented May 30, 2021

Obviously, you can't use the Transform name for destructing, as has been brought up every time this has been asked for before.

(Because Transform takes owned fields)

Also, I think this issue's discussion is conflating two or three things:

  • Splitting Transform back into a bundle again (I don't trust the performance characteristics of this)
  • (Per field change tracking, which could be useful but I think is likely to have more overhead than its worth)
  • Bundle convenience queries (potentially useful, although I feel like it encourages bad patterns where you query for more components than you need to, which has some performance cost. As it happens, the unused 'found' component pointers won't be derefed, so it shouldn't be too bad)

@tigregalis
Copy link
Contributor

Instead, I would prefer to promote the use of "defining" marker components as the first item of each bundle.

I like this. I think something as simple as a marker component called, FromSomeBundle for a bundle named SomeBundle captures the intent, is very clear and explicit Query<&FromSomeBundle>, there's no magic involved and is trivial to set up (with almost no development effort). Could there be a noticeable overhead however? Would that overhead be above that of implementing "created by SomeBundle" functionality?

Beyond this, given that bundle structs have owned fields (not references), I'm not sure what the value of a bundle query is, except for conveniently wrapping multiple components in a query, but in that case, why not simply use a type alias?

let query = Query<(&Foo, &mut Bar)>;

// or
type FooBar = (&Foo, &mut Bar);
let query = Query<FooBar>;

If it could be made to work, it could be nice to be able to refer to components by the field names of an "access struct", similar (but smaller in scope) to deriving SystemParam, but I feel this is something that could be separate to Bundles:

#[derive(Component)]
struct Foo(isize);

#[derive(Component)]
struct Bar(isize);

#[derive(BundleQuery)]
struct FooBar<'a> {
  foo: Ref<'a, Foo>, // equivalent to Query<(&Foo, ...)>
  bar: Mut<'a, Bar>, // equivalent to Query<(&mut Bar, ...)>
}

let query = Query<FooBar>;

for foobar in query.iter() {
    foobar.bar.0 += foobar.foo.0;
}

I suppose you could add methods/traits to these "access structs", operating on the data they hold references to.

@lizelive lizelive mentioned this issue Aug 2, 2021
@alice-i-cecile alice-i-cecile added the S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged label Dec 12, 2021
@bors bors bot closed this as completed in ba6b74b Feb 24, 2022
kurtkuehnert pushed a commit to kurtkuehnert/bevy that referenced this issue Mar 6, 2022
# Objective

- Closes bevyengine#786
- Closes bevyengine#2252
- Closes bevyengine#2588

This PR implements a derive macro that allows users to define their queries as structs with named fields.

## Example

```rust
#[derive(WorldQuery)]
#[world_query(derive(Debug))]
struct NumQuery<'w, T: Component, P: Component> {
    entity: Entity,
    u: UNumQuery<'w>,
    generic: GenericQuery<'w, T, P>,
}

#[derive(WorldQuery)]
#[world_query(derive(Debug))]
struct UNumQuery<'w> {
    u_16: &'w u16,
    u_32_opt: Option<&'w u32>,
}

#[derive(WorldQuery)]
#[world_query(derive(Debug))]
struct GenericQuery<'w, T: Component, P: Component> {
    generic: (&'w T, &'w P),
}

#[derive(WorldQuery)]
#[world_query(filter)]
struct NumQueryFilter<T: Component, P: Component> {
    _u_16: With<u16>,
    _u_32: With<u32>,
    _or: Or<(With<i16>, Changed<u16>, Added<u32>)>,
    _generic_tuple: (With<T>, With<P>),
    _without: Without<Option<u16>>,
    _tp: PhantomData<(T, P)>,
}

fn print_nums_readonly(query: Query<NumQuery<u64, i64>, NumQueryFilter<u64, i64>>) {
    for num in query.iter() {
        println!("{:#?}", num);
    }
}

#[derive(WorldQuery)]
#[world_query(mutable, derive(Debug))]
struct MutNumQuery<'w, T: Component, P: Component> {
    i_16: &'w mut i16,
    i_32_opt: Option<&'w mut i32>,
}

fn print_nums(mut query: Query<MutNumQuery, NumQueryFilter<u64, i64>>) {
    for num in query.iter_mut() {
        println!("{:#?}", num);
    }
}
```

## TODOs:
- [x] Add support for `&T` and `&mut T`
  - [x] Test
- [x] Add support for optional types
  - [x] Test
- [x] Add support for `Entity`
  - [x] Test
- [x] Add support for nested `WorldQuery`
  - [x] Test
- [x] Add support for tuples
  - [x] Test
- [x] Add support for generics
  - [x] Test
- [x] Add support for query filters
  - [x] Test
- [x] Add support for `PhantomData`
  - [x] Test
- [x] Refactor `read_world_query_field_type_info`
- [x] Properly document `readonly` attribute for nested queries and the static assertions that guarantee safety
  - [x] Test that we never implement `ReadOnlyFetch` for types that need mutable access
  - [x] Test that we insert static assertions for nested `WorldQuery` that a user marked as readonly
@blueforesticarus
Copy link

blueforesticarus commented Feb 24, 2024

Something I have not seen discussed, is that this would have been useful for refactoring a component into multiple components, since the original component struct could be changed to derive bundle.

If, for example, you wanted to split Velocity{ linear: Vec3, angular: Vec3 } into LinVel and AngVel components.

The concern for foot-gunning here I think is applicable to larger bundles. However, going in the opposite direction (as above) there could be opportunity to use bundles to make one's codebase more modular instead of less, and in cases where it would otherwise be non-ergonomic or a refactor burden to do so. This is especially relevant to rapid-prototyping.

This could be accomplished with a different derive + trait (seperate from bundle), however the existing WorldQuery machinery (as far as I can tell) cannot be made generic over mutability, and is not drop-in the way I'm describing.

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-Feature A new feature, making something new possible C-Performance A change motivated by improving speed, memory usage or compile times C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants