bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
34.23k stars 3.35k forks source link

Add `EntityMut::components` and `EntityRef` and `EntityWorldMut` equivalent #13127

Open alice-i-cecile opened 2 months ago

alice-i-cecile commented 2 months ago

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

Some users (and applications) prefer a more game-object style of writing Bevy: fetching fat components directly from the ECS and quickly changing the data needed as their systems evolve.

This style can be comfortable and productive to write, but this path isn't adequately taught or supported. In particular, accessing multiple components from a single entity in an ad hoc way is very useful for avatar-centric games like platformers or ARPGs, but is frustrating to do.

What solution would you like?

fn get_components<D: QueryData>(&mut self) ->Option<D::Item>

This proposed method takes any type that implements QueryData (like (&mut Transform, &Life) and returns the type that would be fetched by an equivalent query. Note that the type signature is actually messier, using as associated types and an as WorldQuery.

Both EntityMut and EntityWorldMut should have this method added as is. EntityRef requires an additional bound on D: it must instead be a ReadOnlyQueryData.

What alternative(s) have you considered?

There's various get methods which return a single component at a time. These are less flexible, as they return only a single component.

We could return a Result with a QueryItemError, like Query::get, but most of its variants will never be hit.

We could add a QueryFilter generic to these methods, but there's no point: we're already working on a single entity!

We could add a get_components_mut method, and have get_components as a way to fetch the read-only form. Given that this is a convenience API designed for handcrafted code, I don't think this is a useful transformation to expose here at the cost of ergonomics.

We could make this panicking by default or add a panicking equivalent, but as per #12660, this is likely to increase user suffering overall.

Additional context

I also think we should prioritize implementing let (mut transform, player) = entity.components::<(&mut Transform, &Player)>(), as I think it shores up a pretty big pain point.

cart commented 2 months ago

I was worried our Query infrastructure was missing the necessary pieces to do this efficiently (namely determining query matches without allocating and setting FilteredAccess). But thanks to matches_component_set we have all we need. I have an impl incoming.

cart commented 2 months ago

We could add a get_components_mut method, and have get_components as a way to fetch the read-only form. Given that this is a convenience API designed for handcrafted code, I don't think this is a useful transformation to expose here at the cost of ergonomics.

I think we probably want a read-only variant. Disallowing things like this seems overly borrow-checker-restrictive, especially given that this exists to lift borrow checker constraints:

let (a, b) = entity.components::<(&A, &B)>();
if SOME_CONDITION {
  let (c, d) = entity.components::<(&C, &D)>();
}

I think the only question is naming:

  1. components (read only), components_mut (unrestricted)
  2. components_read_only (read only), components (unrestricted)
  3. read_components (read only), components (unrestricted)

We could also consider replacing the current get/get_mut with this api (especially if the perf is the same ... get/get_mut currently use a smaller / simpler code path, so they might perform differently).

let a = entity.get::<&A>();
let mut a = entity.get_mut::<&A>();
let (a, b) = entity.get::<(&A, &B)>();
let (mut a, b) = entity.get_mut::<(&mut A, &B)>();

Of course, this would be a breaking change. And it does notably make single component accesses less ergonomic:

let a = entity.get::<&A>();

vs

let a = entity.get::<A>();
cart commented 2 months ago

Even more so for get_mut:

let a = entity.get_mut::<&mut A>();

vs

let a = entity.get_mut::<A>();
cart commented 2 months ago

But a single unified API (that exactly matches the Query API) does feel like a solid unification.

cart commented 2 months ago
// EntityMut
let (mut a, b) = entity.get_mut::<(&mut A, &B)>;

// Direct World Queries
let mut query = world.query::<(&mut A, &B)>;
let (mut a, b) = query.get_mut(world, SOME_ENTITY);

// Systems
fn system(query: Query<(&mut A, &B)>) {
    let (mut a, b) = query.get_mut(SOME_ENTITY);
} 

Hard to deny how nice the overlap is.

cart commented 2 months ago

(I've wrapped up an implementation so the only remaining question is how we expose it)

iiYese commented 2 months ago

I would like replacing get_(mut) anyway because it gives it parity with QueryData & makes it naturally consistent with the other bevy APIs like add_systems, insert, add_plugins etc. The breaking change is very minor.

cart commented 2 months ago

Just realized that I forgot to do the unwraps on all of those get/get_mut calls :)

I am personally very interested in implicit-unwrap variants (both for Queries and Entities) as I think a high percentage of use cases benefit from that ergonomically. I end up throwing a lot of get().unwrap() into my game code and I would prefer a single friendly method to call for those cases (ideally using the same verb as the fallible variant).

get_X is our convention for returning a result/option for X and x() is our convention for a panicking version of that. For our current methods get() / get_mut(), that leaves us with no X. Not quite sure how to solve that naming problem in a satisfying way.

If we could implement Index for Query, this would be pretty satisfying, although even if query[entity] were possible, the mut vs read-only question comes into play:

let (mut a, b) = entity.query::<(&mut A, &B)>();
let (mut a, b) = entity.get_query::<(&mut A, &B>().unwrap();
let (mut a, b) = query[entity];
let (mut a, b) = query.get_mut(entity).unwrap();

I think our best bet might be get() and try_get()

let (mut a, b) = entity.get::<(&mut A, &B)>();
let (mut a, b) = entity.try_get::<(&mut A, &B>().unwrap();
let (mut a, b) = query.get(entity);
let (mut a, b) = query.try_get(entity).unwrap();

(Notably, GetComponent and TryGetComponent is used for Unity component access)

iiYese commented 2 months ago

How about:

entity.data::<(&mut A, &B)>();
enttiy.get_data::<(&mut A, &B)>();

entity.data_mut::<(&mut A, &B)>();
enttiy.get_data_mut::<(&mut A, &B)>();

like QueryData.

alice-i-cecile commented 2 months ago

I'm on board with a mutable / immutable split, and unification with get. Panicking variants are fine too: I can just not use them ;)

cart commented 2 months ago

like QueryData.

I think Data is irrelevant / implicit from a user perspective. Everything is "data". And its not an ECS term that users think about.

iiYese commented 2 months ago

I'm more a fan of that redundancy than try_get which is 2 rust terms for fallible.

cart commented 2 months ago

I'm on board with a mutable / immutable split, and unification with get. Panicking variants are fine too: I can just not use them ;)

I think the biggest ergonomic causality of the unified try (non panicking) get (retrieve components) and mut (allow mutations) approach is:

let mut a = entity.get_mut::<A>().unwrap();

Which becomes:

let mut a = entity.try_get_mut::<&mut A>().unwrap();

However it also makes this possible (and often preferable), so I'll call it a win:

let mut a = entity.get_mut::<&mut A>();

We could also invert things / make mutable access the default. That does sort of make sense in the context of queries, where "read only"-ness is an additional constraint added:

let mut a = entity.get::<&mut A>();
// still a mutable `entity` access
let b = entity.get::<&B>();
let c = entity.get_ref::<&C>();

That approach would also remove a bunch of mut stutters in cases like this:

fn system(query: Query<&mut A>) {
  let mut a = query.get(ENTITY);
}

At the cost of making ref necessary in cases where multiple reads take place:

fn system(query: Query<&A>) {
  // this is fine, but it borrows query mutably
  let a = query.get(ENTITY);
}
fn system(query: Query<&A>) {
  let a1 = query.get(ENTITY_1);
   // this would fail
  let a2 = query.get(ENTITY_2);
}
fn system(query: Query<&A>) {
  let a1 = query.get_ref(ENTITY_1);
  // this succeeds
  let a2 = query.get_ref(ENTITY_2);
}
cart commented 2 months ago

I'm more a fan of that redundancy than try_get which is 2 rust terms for fallible.

I do see your point, but get does also fall into the "non fallible" category in std:

I see it as more of a "generic retrieve some thing based on context" verb.

cart commented 2 months ago

I will concede that those are all cases where there is no fallible option. In std, in cases where there is a fallible variant, get is that variant.

That being said, std tends to use the Index operator for the panicking variants in these cases, which isn't an option for us.

cart commented 2 months ago

I think try_get and get is a reasonable / std compatible interpretation. I think this particular ambiguity has never been resolved in std and both interpretations are valid.

iiYese commented 2 months ago

We could also invert things / make mutable access the default.

I don't think that's a good idea because:

Edit: Missed the query rename but point stands.

cart commented 2 months ago

Agreed those are good arguments :)

iiYese commented 2 months ago

These extensions should also be added to FilteredEntityRef & FilteredEntityMut.

alice-i-cecile commented 1 week ago

Related to #14231.