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

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

Open
alice-i-cecile opened this issue Apr 28, 2024 · 19 comments
Open
Labels
A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use

Comments

@alice-i-cecile
Copy link
Member

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.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use labels Apr 28, 2024
@cart
Copy link
Member

cart commented Apr 28, 2024

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
Copy link
Member

cart commented Apr 28, 2024

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
Copy link
Member

cart commented Apr 28, 2024

Even more so for get_mut:

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

vs

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

@cart
Copy link
Member

cart commented Apr 28, 2024

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

@cart
Copy link
Member

cart commented Apr 28, 2024

// 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
Copy link
Member

cart commented Apr 28, 2024

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

@iiYese
Copy link
Contributor

iiYese commented Apr 28, 2024

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
Copy link
Member

cart commented Apr 28, 2024

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
Copy link
Contributor

iiYese commented Apr 28, 2024

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
Copy link
Member Author

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
Copy link
Member

cart commented Apr 28, 2024

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
Copy link
Contributor

iiYese commented Apr 28, 2024

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

@cart
Copy link
Member

cart commented Apr 28, 2024

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
Copy link
Member

cart commented Apr 28, 2024

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
Copy link
Member

cart commented Apr 28, 2024

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
Copy link
Member

cart commented Apr 28, 2024

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
Copy link
Contributor

iiYese commented Apr 28, 2024

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

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

  • It would be the only inconsistent exception among every other access API like Query::get_(mut) & World::(get)_resource_(mut).
  • It's also inconsistent with all of rust because rust is immutable by default making it not only an exception among bevy ecs APIs but also an exception among many rust library APIs including libraries in bevy.
  • Read access is the thing you can get multiple times while mut access you can only get once. Meaning the thing you can do multiple times is the more verbose thing.

Edit: Missed the query rename but point stands.

@cart
Copy link
Member

cart commented Apr 28, 2024

Agreed those are good arguments :)

@mockersf mockersf changed the title Add EntityMut::components andEntityRef and EntityWorldMut` equivalent Add EntityMut::components and EntityRef and EntityWorldMut equivalent Apr 28, 2024
@iiYese
Copy link
Contributor

iiYese commented May 2, 2024

These extensions should also be added to FilteredEntityRef & FilteredEntityMut.

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-Usability A simple quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

No branches or pull requests

3 participants