[Merged by Bors] - Query::get_unique #1263

TheRawMeatball · 2021-01-19T18:00:35Z

Adds get_unique and get_unique_mut to extend the query api and cover a common use case. Also establishes a second impl block where non-core APIs that don't access the internal fields of queries can live.

cart

Enough people have asked for this that its definitely worth considering. I think I'm on board, but I'll need a sec to consider alternatives.

crates/bevy_ecs/src/system/query/extensions.rs

DJMcNab

Implementation looks good, just a few thoughts about the usage patterns

DJMcNab · 2021-01-22T17:55:13Z

examples/game/breakout.rs

-    for mut text in query.iter_mut() {
-        text.value = format!("Score: {}", scoreboard.score);
-    }
+    query.get_unique_mut().unwrap().value = format!("Score: {}", scoreboard.score)


I would say this is a good use of this api

DJMcNab · 2021-01-22T17:57:09Z

examples/game/breakout.rs

@@ -201,7 +199,7 @@ fn ball_collision_system(
    mut ball_query: Query<(&mut Ball, &Transform, &Sprite)>,
    collider_query: Query<(Entity, &Collider, &Transform, &Sprite)>,
 ) {
-    for (mut ball, ball_transform, sprite) in ball_query.iter_mut() {
+    if let Ok((mut ball, ball_transform, sprite)) = ball_query.get_unique_mut() {


In general however, I think constraining uses like this not ideal. For example, the current version would trivially extend to having multiple fields with the same controls (to allow something like those things where people play multiple pokemon games with one set of inputs)

Although on the other hand, for this simple example this is probably fine.

Yeah this is sort of the crux of my hesitance to adopt get_unique(). It encourages people to design around "single entity" patterns. Maybe thats a good thing, but its a paradigm shift with implications we need to consider.

I expect that the cases where we don't mind this pattern would be for rendering Resources, i.e. it's just for getting data from resources onto the screen, which I've called out as a sensible use above. That is, it's a logic error to have multiple scoreboard texts but a single Score resource

But this case is just using that API for the sake of using it, where there's not actually an especially compelling reason to.

Yeah I agree. In this case, there might be a future "breakout++" where multiple balls come into play. And I think that for most things even if there is only one item we should encourage query.iter(). Imo get_unique() should be reserved for cases that are truly game breaking.

alice-i-cecile · 2021-02-08T00:33:47Z

I ran into another really useful application for this today: unit testing and debugging with marker components.

When unit testing (or interactively debugging), you'll often set up toy examples where you create a few entities, give them each a different marker component, and then query them to inspect their behavior. This pattern is made much more ergonomic with get_unique and get_unique_mut.

The other approaches don't work well here:

Refactoring to use a resource instead of a singleton component. This doesn't work at all, since the point is to test out your existing systems without modifications.
Storing the Entity ID within a Resource, then fetching that resource, and using query.get(my_entity). This is an added layer of indirection for now real benefit, and makes your tests much less clear. In general, this pattern also has serious issues with ArchetypeComponent access clashes compared to just using a marker component.
Manually reimplementing this functionality. Very tedious boilerplate when writing many tests; especially since you want nice error messages if something breaks.

alice-i-cecile · 2021-02-11T20:52:08Z

@cart I think this is good to merge, if you've been convinced that it's a useful API :)

alice-i-cecile · 2021-02-23T21:46:34Z

Another really central pattern that's coming up for my turn-based RPG:

Only one unit can act at once, since it's turn-based.
I very much want to be able to have each unit as an entity, and their data as components, rather than sticking them into resources, for what should be obvious reasons.
In order to determine which unit is active, I give them a temporary Active (or Attacker etc.) marker component.
Then, I need to extract the other components for that entity, querying only for the Active entity. These queries will always return exactly one entity, is a logic error if it doesn't, and the entity in question changes over time.

This is a clear, idiomatic use of get_unique in real game code, and isn't appropriately handled by either converting it to a resource or storing the Entity in a resource and then looking things up within the system using query.get.

Bromeon · 2021-02-24T15:01:25Z

Regarding naming, I think "unique" is unfortunate. In many contexts, "unique" is understood as de-duplication, i.e. removing duplicate elements. Instead, I would suggest "single" for this functionality, as it clarifies you're dealing with a single element and is more common:

Rust single crate
Kotlin single() and singleOrNull() functions

Could thus be get_single() and get_single_mut() or also just simply single() and single_mut().

Regarding semantics, I think cart's point is important here:

Yeah this is sort of the crux of my hesitance to adopt get_unique(). It encourages people to design around "single entity" patterns. Maybe thats a good thing, but its a paradigm shift with implications we need to consider.

I've only started to use Bevy recently, but it seems to me that its components are specifically designed and optimized for being used across a large number of entities, and there are already different approaches for the problems stated here:

single-entity components which model a singleton (one global instance) can be represented as resources
in alice-i-cecile's use case, it might be more straightforward to store a entity_in_turn: Entity variable and use World::get() to access it, rather than shifting components around

alice-i-cecile · 2021-02-24T18:06:50Z

Could thus be get_single() and get_single_mut() or also just simply single() and single_mut().

I'm alright with that. The idea of "there should only be one" is conveyed a bit more clearly with "unique" over "single" however.

single-entity components which model a singleton (one global instance) can be represented as resources

This doesn't work for complex singletons whose fields you want to be able to mutate in parallel. It also fails badly when you have common behavior that you would like to share with other entities. This is a common pattern for really simple games like roguelikes and platformers, and I'd hate to either push people into making their singleton players resources, iterating over a query that they know is always length one or rolling their own version of this constantly.

Re the paradigm shift discussed by @cart: pushing people towards using complex resources in Bevy is commonly a trap: it strips away a huge amount of the modular, expressive power of the ECS and tends to block parallelism. Singleton entities are superior to resources for everything that isn't a simple natural monolith.

in alice-i-cecile's use case, it might be more straightforward to store a entity_in_turn: Entity variable and use World::get() to access it, rather than shifting components around

FWIW, you'll almost never want to use World::get directly, since it requires a completely-blocking exclusive system :) Using query::get instead has some serious issues though still:

It's much less clear what's happening from the system's type signature or function body.
There's dramatically more boilerplate within the function body that can be messed up.
You have to store the data nonlocally, which makes it harder to reason about.
It's very painful to extend this pattern to more-than-one entity of this sort. Marker components scale naturally; you just replace the internal system logic with a loop rather than needing to change the entire piping around them.
You get mutable access to every entity in the entire query, despite only needing to modify a tiny bit of its data.
This can create spurious system ambiguity issues that make it harder to spot real issues, since the ambiguity checker can't see past the type signature.

With the addition of sparse set components, marker components will become even faster, and I would expect to benchmark much better than the Resource approach, in addition to having better conceptual clarity.

inodentry · 2021-02-24T19:20:38Z

Yes, i am strongly in favor of encouraging "singleton entities" over resources, because they get many advantages due to being made of components. We should not encourage the liberal use of resources.

For example, a game might only have one player. But some components of that player (like health) might be common with other entities. It's nice to have systems that operate on all those entities (say, health system), while also having systems that operate specifically on the player.

This is the idiomatic and preferable way to do things as it is, regardless of whether this new API is merged or not.

The big selling point of this new API is that it makes these use cases safer. It gives you a way to actually be explicit about your intentions/invariants and enforce them. Because there are many use cases in typical games where you'd only have one of a particular entity.

It doesn't make sense to think of "but i want to leave the possibility of there being more than one player at some point", if you know very well that that will never be the case in your game. You are probably designing many things around that assumption.

Overall, i am strongly in favor of this new API. It helps make things clear and explicit, and gives extra safety for a common pattern (that is currently only able to be expressed implicitly).

EDIT: as for the concern about the kinds of patterns/mindsets that new APIs would encourage for new users learning bevy, this is something to be addressed with proper teaching and documentation. We shouldn't leave out useful APIs because they might be confusing to newcomers.

Bromeon · 2021-02-25T09:54:56Z

@alice-i-cecile @jamadazi Thanks a lot for the clarification! That sounds like single() might be a replacement for resources, or at least for many of the cases where resources are used today.

From an API design perspective and Bevy's aspiration to be simple, I believe it would make sense if there are some clear guidelines on when to use which approach (Query::single() vs. Query::get() vs. resources), ideally with straightforward examples. It would also allow to clearly see what the benefits of multiple coexisting APIs are.

inodentry · 2021-02-25T22:29:49Z

Yes, as I was saying, it is a matter of teaching. We can teach users to have the proper understanding.

The API is useful and idiomatic IMO, so it should be accepted. Even with the concern that it might make things slightly more confusing to beginners.

We already have people (notably alice and myself) who are actively teaching newcomers good practices and mentality around resources vs entities, and other ECS patterns. We'll just adapt to this.

cart · 2021-03-05T21:07:12Z

I think if this gets adapted to the latest changes on main, I think this is good to merge.

crates/bevy_ecs/src/query/extensions.rs

examples/game/alien_cake_addict.rs

Bromeon · 2021-03-06T11:21:26Z

Cool to see this moving ahead! 🙂

Maybe concerning the name of the methods, just so this isn't overlooked:

Regarding naming, I think "unique" is unfortunate. In many contexts, "unique" is understood as de-duplication, i.e. removing duplicate elements. Instead, I would suggest "single" for this functionality, as it clarifies you're dealing with a single element and is more common:

Rust single crate

Kotlin single() and singleOrNull() functions

Could thus be get_single() and get_single_mut() or also just simply single() and single_mut().

If the consensus is that "unique" is a better term than "single", that's OK!

Regarding semantics, is there a guideline/rule of thumb when to use resources vs. this pattern?
I have quite some code based on resources, when should I consider moving to this instead?

bjorn3 · 2021-03-06T11:40:27Z

crates/bevy_ecs/src/system/query.rs

@@ -212,6 +212,42 @@ where
            Err(QueryComponentError::MissingWriteAccess)
        }
    }
+
+    pub fn get_unique(&self) -> Result<<Q::Fetch as Fetch<'_>>::Item, UniqueQueryError<'_, Q>>


You may want to box the error to shrink the return type.

How would that help?

This could improve performance if the smaller return type fits into two registers by avoiding stack accesses.

Hmm, I see... Personally I don't think this would change the performance much, but I don't have a way to benchmark that. This seems quite micro-optimization-y, so I'd rather wait on this until we have realistic uses of this function we can benchmark.

Maybe we don't need to return Item at all in the Error? The query isn't consumed by this operation, so users that want the other values in the event of an error can always iterate over them. Generally when someone wants a "single" item and there is more than one, there is nothing particularly special about the first item.

I think we should resolve this conversation before merging. My preference is to remove "result: Item" from Error. What do you think @TheRawMeatball ?

Hmm, while I prefer it as-is I suppose the use case is very slim and I'm willing to compromise to get the ball rolling. I'll update the PR to remove this now.

cart · 2021-03-06T19:44:35Z

Maybe concerning the name of the methods, just so this isn't overlooked:

Ah yeah sorry for not commenting here (overlooked is the right word). I don't have strong opinions here. std has managed to avoid usage of both single and unique terms, which is amazing to me. So drawing from the consensus across other languages / ecosystems is a good call. I'll add C# to the stack of things that uses "single" in this context (returns the next item in an enumerator if it is the only value in the enumerator, otherwise throws an exception).

My vote is for single unless counterexamples are provided from other prominent projects. I do also like the shorter single() over get_single().

Regarding semantics, is there a guideline/rule of thumb when to use resources vs. this pattern?
I have quite some code based on resources, when should I consider moving to this instead?

This is still TBD, but i think its fair to say that Resources are for "unique" types that do not have an "identity" other than their type and should not be "correlated" to other values/types. They are "singletons" for things like Asset collections, Time, etc.

query.single() / query.unique() is for things that need an identity other than their type and might correlate to other things (ex: a "player" might have a Name, a Velocity, a Position, etc).

In my mind nothing has really changed here in the Entity vs Resource relationship other than the fact that you can now add a "single" assertion to entity access.

alice-i-cecile · 2021-03-06T20:18:29Z

My vote is for single unless counterexamples are provided from other prominent projects. I do also like the shorter single() over get_single().

FWIW, I like get_* and get_*_mut over just single because what you get out of the function perfectly parallels the existing Query::get and Query::get_mut APIs. Rather than passing in a specific entity to look for, you're just grabbing the only entity possible.

cart · 2021-03-06T20:55:58Z

My justification for the "inconsistency" is that get(thing) terminology is generally used when retrieving key->value pairs. Whereas terms like next(), unwrap(), and first() are for "operations that return a thing". single() seems to fit cleanly into the latter group.

DJMcNab · 2021-03-06T21:07:56Z

Could .only() work?

alice-i-cecile · 2021-03-06T21:09:15Z

My justification for the "inconsistency" is that get(thing) terminology is generally used when retrieving key->value pairs. Whereas terms like next(), unwrap(), and first() are for "operations that return a thing". single() seems to fit cleanly into the latter group.

This makes sense. .only() seems like the best option I've seen.

Bromeon · 2021-03-06T22:01:25Z

Thanks for all the feedback! 🙂

I tend to agree with cart that it's a special method very similar to first() and last() and a get_ prefix is rather unusual.

only() would again have the problem that it's not common (in existing crates or other languages).
Additionally, "only" also means "just/merely" in English, while "single" is a property implying 1 element.

inodentry · 2021-03-07T14:06:44Z

I really don't like only. It throws me off, looks unusual.

single is my favorite, i strongly prefer it over unique

i like the get_ prefix, but it's not a strong opinion, it's fine without the prefix, too

TheRawMeatball · 2021-03-07T14:20:28Z

@cart, there is a lot of opinions here. If you can make a final call, I'd like to finalize this PR.

cart · 2021-03-07T18:43:57Z

I still prefer single() so lets roll with that. I think it reads better and has the benefit of consistency in the wider language ecosystem.

crates/bevy_ecs/src/system/query.rs

Bromeon · 2021-03-08T09:21:01Z

crates/bevy_ecs/src/system/query.rs

+#[derive(Error)]
+pub enum QuerySingleError<'a, Q: WorldQuery> {
+    #[error("No entities fit the query {0}")]
+    NoEntities(&'static str),


Would it make sense to have the same layout for NoEntities as for MultipleEntities,
i.e NoEntities { query_name: &'static str }?

This removes the slightly strange asymmetry and would allow to extend the error information in the future.

cart · 2021-03-08T21:21:32Z

bors r+

Adds `get_unique` and `get_unique_mut` to extend the query api and cover a common use case. Also establishes a second impl block where non-core APIs that don't access the internal fields of queries can live.

bors · 2021-03-08T21:39:17Z

Pull request successfully merged into main.

Build succeeded:

alice-i-cecile · 2021-03-12T06:09:12Z

@cart I found the ecosystem equivalent of this method: exactly_one. Might be worth renaming and unifying behavior?

inodentry · 2021-03-12T08:54:36Z

ecosystem equivalent

Is this used outside of itertools? If it is not widely prevalent, IDK if we could call it that, and might not be worthwhile to copy. I personally haven't seen it anywhere.

single still feels more intuitive to me.

TheRawMeatball added 4 commits January 19, 2021 19:44

get_unique and get_unique_mut

85ced1e

Documentation + update breakout example

573f293

Doc fix

326dea9

Doc fix

93f07e3

cart reviewed Jan 19, 2021

View reviewed changes

crates/bevy_ecs/src/system/query/extensions.rs Outdated Show resolved Hide resolved

crates/bevy_ecs/src/system/query/extensions.rs Outdated Show resolved Hide resolved

crates/bevy_ecs/src/system/query/extensions.rs Outdated Show resolved Hide resolved

Implement suggestions

964bfcf

DJMcNab approved these changes Jan 22, 2021

View reviewed changes

TheRawMeatball added 2 commits January 22, 2021 21:43

Merge remote-tracking branch 'upstream/master' into query-helpers

4f577ab

Update new example

370c044

cart added this to the Bevy 0.5 milestone Feb 6, 2021

alice-i-cecile approved these changes Feb 11, 2021

View reviewed changes

alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Feb 17, 2021

Base automatically changed from master to main February 19, 2021 20:44

Merge 'upstream/main' into query-helpers

a83531e

alice-i-cecile mentioned this pull request Feb 26, 2021

[Merged by Bors] - Bevy ECS V2 #1525

Closed

TheRawMeatball added 2 commits March 6, 2021 00:45

Merge branch 'main' into query-helpers

aa42a63

Update to latest

5af71fb

cart changed the title ~~Query helpers~~ Query::get_unique Mar 5, 2021

cart reviewed Mar 5, 2021

View reviewed changes

crates/bevy_ecs/src/query/extensions.rs Outdated Show resolved Hide resolved

cart reviewed Mar 5, 2021

View reviewed changes

examples/game/alien_cake_addict.rs Outdated Show resolved Hide resolved

TheRawMeatball added 3 commits March 6, 2021 13:30

Join impl blocks

cc2f5c9

Change examples

4bc8f0c

fix formatting

435a7f2

bjorn3 reviewed Mar 6, 2021

View reviewed changes

Refactor code tu use single

0be6c8c

cart reviewed Mar 7, 2021

View reviewed changes

crates/bevy_ecs/src/system/query.rs Outdated Show resolved Hide resolved

TheRawMeatball added 2 commits March 7, 2021 23:35

Refactor error

11c0606

cargo fmt

e0c7e36

Bromeon reviewed Mar 8, 2021

View reviewed changes

Remove item from error

06cd9cd

bors bot changed the title ~~Query::get_unique~~ [Merged by Bors] - Query::get_unique Mar 8, 2021

bors bot closed this Mar 8, 2021

alice-i-cecile mentioned this pull request Mar 12, 2021

Clone and IntoIter support for queries #1631

Closed

mockersf mentioned this pull request Mar 26, 2021

[Merged by Bors] - Add a method iter_combinations on query to iterate over combinations of query results #1763

Closed

[Merged by Bors] - Query::get_unique #1263

[Merged by Bors] - Query::get_unique #1263

Conversation

TheRawMeatball commented Jan 19, 2021

cart left a comment

Choose a reason for hiding this comment

DJMcNab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DJMcNab Jan 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alice-i-cecile commented Feb 8, 2021

alice-i-cecile commented Feb 11, 2021

alice-i-cecile commented Feb 23, 2021 • edited Loading

Bromeon commented Feb 24, 2021

alice-i-cecile commented Feb 24, 2021

inodentry commented Feb 24, 2021 • edited Loading

Bromeon commented Feb 25, 2021

inodentry commented Feb 25, 2021

cart commented Mar 5, 2021

Bromeon commented Mar 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cart commented Mar 6, 2021

alice-i-cecile commented Mar 6, 2021

cart commented Mar 6, 2021 • edited Loading

DJMcNab commented Mar 6, 2021

alice-i-cecile commented Mar 6, 2021

Bromeon commented Mar 6, 2021

inodentry commented Mar 7, 2021

TheRawMeatball commented Mar 7, 2021

cart commented Mar 7, 2021

Bromeon Mar 8, 2021 • edited Loading

Choose a reason for hiding this comment

cart commented Mar 8, 2021

bors bot commented Mar 8, 2021

alice-i-cecile commented Mar 12, 2021

inodentry commented Mar 12, 2021

DJMcNab Jan 24, 2021 •

edited

Loading

alice-i-cecile commented Feb 23, 2021 •

edited

Loading

inodentry commented Feb 24, 2021 •

edited

Loading

cart commented Mar 6, 2021 •

edited

Loading

Bromeon Mar 8, 2021 •

edited

Loading