implement get_many_unique #18315

Victoronz · 2025-03-14T18:54:26Z

Objective

Continuation to #16547 and #17954.

The get_many family are the last methods on Query/QueryState for which we're still missing a unique version.

Solution

Offer get_many_unique/get_many_unique_mut and get_many_unique_inner!

Their implementation is the same as get_many, the difference lies in their guaranteed-to-be unique inputs, meaning we never do any aliasing checks.

To reduce confusion, we also rename get_many_readonly into get_many_inner and the current get_many_inner into get_many_mut_inner to clarify their purposes.

Testing

Doc examples.

Main Branch Migration Guide

get_many_inner is now called get_many_mut_inner.
get_many_readonly is now called get_many_inner.

Carter0 · 2025-03-14T19:02:09Z

crates/bevy_ecs/src/query/state.rs

+    /// struct A(usize);
+    ///
+    /// let mut world = World::new();
+    /// let entity_vec: UniqueEntityVec<Entity> = world.spawn_batch((0..3).map(A)).collect_set();


Two quick questions if you have the time

Isn't a UniqueEntityVec<Entity> kinda redundant? What else would go between the <>?

Isn't a UniqueEntityVec just a Set? Why not just call it a set?

Do you mean the questions in general? Or just in this specific case?
If its just these lines:

You're right here, this can probably just be inferred.

The variable name is more descriptive as entity_set, yeah!

Nevermind, seems like the Entity parameter in 1. doesn't infer!
While Entity could be the type default for UniqueEntityVec, same with UniqueEntitySlice, this isn't easily the case for UniqueEntityArray.
Because type defaults have to be trailing, this would turn UniqueEntityArray<Entity, N> into UniqueEntityArray<N, Entity>, which is quite an ugly inconsistency with [Entity; N].
Not sure what should be done here.

As for what else can go in this parameter, it can entity newtypes like MainEntity/RenderEntity or EntityRef-like types.

Thanks for responding!

Yeah, this is just a naming nit really. Maybe when the 'unique' functionality is all done it could be polished up in the future.

I think the type UniqueEntityVec<Entity> could maybe be turned into Set<Entity> or even just EntitySet at some point. But I was mostly asking just because I was curious about if there was a reason to call it UniqueEntityVec rather than anything else.

While Entity could be the type default for UniqueEntityVec, same with UniqueEntitySlice, this isn't easily the case for UniqueEntityArray.
Because type defaults have to be trailing, this would turn UniqueEntityArray<Entity, N> into UniqueEntityArray<N, Entity>, which is quite an ugly inconsistency with [Entity; N].
Not sure what should be done here.

Oh, adding defaults there is a good idea! I expect Entity to be the most common type by far. The inconsistency with UniqueEntityArray<N, T> and [T; N] is unfortunate, but if the normal case is UniqueEntityArray<N> vs [Entity; N] then it doesn't seem so bad!

While Entity could be the type default for UniqueEntityVec, same with UniqueEntitySlice, this isn't easily the case for UniqueEntityArray.
Because type defaults have to be trailing, this would turn UniqueEntityArray<Entity, N> into UniqueEntityArray<N, Entity>, which is quite an ugly inconsistency with [Entity; N].
Not sure what should be done here.

Oh, adding defaults there is a good idea! I expect Entity to be the most common type by far. The inconsistency with UniqueEntityArray<N, T> and [T; N] is unfortunate, but if the normal case is UniqueEntityArray<N> vs [Entity; N] then it doesn't seem so bad!

Hmm, one way to potentially remedy the inconsistency is by introducing a different inconsistency, which is also a type default for N, f.e. 0, I haven't thought a lot about that yet, but would you think it makes sense?

Edit: Actually, I think Rust is not intelligent enough to allow for UniqueEntityArray<3> when both generics are defaulted, we'd need to mention both, defeating the whole purpose :P

Carter0 · 2025-03-14T19:09:11Z

crates/bevy_ecs/src/query/state.rs

+    /// let entities: UniqueEntityVec<Entity> = world.spawn_batch((0..3).map(A)).collect_set();
+    /// let entities: UniqueEntityArray<Entity, 3> = entities.try_into().unwrap();
+    ///
+    /// world.spawn(A(73));


You are spawning an entity here with component A(73) because you want to make sure that the query_state.get_many_unique_mut call only returns the entities in the UniqueEntityArray<Entity, 3> right?

I have copied and adjusted the existing doc tests for get_many and get_many_mut for this, so I am not the original writer, but that does seem to be the reason for that spawn.

Cool beans! I think that an example that showcases this functionality would be good then cause you could comment why world.spawn(A(73)); was there.

Totally not blocking tho!

Carter0 · 2025-03-14T19:13:16Z

crates/bevy_ecs/src/query/state.rs

+    /// let invalid_entity = world.spawn_empty().id();
+    ///
+    /// assert_eq!(match query_state.get_many_unique(&mut world, UniqueEntityArray::from([wrong_entity])).unwrap_err() {QueryEntityError::EntityDoesNotExist(error) => error.entity, _ => panic!()}, wrong_entity);
+    /// assert_eq!(match query_state.get_many_unique_mut(&mut world, UniqueEntityArray::from([invalid_entity])).unwrap_err() {QueryEntityError::QueryDoesNotMatch(entity, _) => entity, _ => panic!()}, invalid_entity);


Is the reason this is returning an error because you expect every entity to have component 'A'? If so, why not just return an empty list of components?

These asserts test that the error cases are produced properly.
If an entity does not match the D in QueryData, that should return an error!
Same with an entity that does not exist in the first place.

Carter0

I like it! I would love an example for how to use unique entity lists and why it might be useful!

Victoronz · 2025-03-14T19:33:20Z

I like it! I would love an example for how to use unique entity lists and why it might be useful!

Once I am done with the feature work I'll have a look throughout the engine for where they can improve things :)
For now, one example can be found in propagate_transforms! (Done in #17840)

chescock

Looks good!

I think get_many_inner and get_many_readonly are new in 0.16, so if we can get this in before the release is cut then we won't need the migration guide.

chescock · 2025-03-14T19:20:36Z

crates/bevy_ecs/src/query/state.rs

+    /// assert_eq!(match query_state.get_many_unique(&mut world, UniqueEntityArray::from([wrong_entity])).unwrap_err() {QueryEntityError::EntityDoesNotExist(error) => error.entity, _ => panic!()}, wrong_entity);
+    /// ```
+    #[inline]
+    pub fn get_many_unique<'w, const N: usize>(


I'm hoping to deprecate the methods like this on QueryState, so I'd mildly prefer not to add these in the first place. The functionality would still be available as .query(world).get_many_unique_inner(entities).

Yeah I was a little confused why this function needed to be implemented for both the state module and query module. It seems like it would only really be needed in the query module

Yeah I was a little confused why this function needed to be implemented for both the state module and query module.

For context: Until recently, the actual implementation of all of these methods was on QueryState, and the Query methods just called those. I added the QueryState::query methods and moved the implementations over, with the eventual goal of getting rid of most of the other methods on QueryState.

I am aware that we're trying to deprecate them!
However in the spirit of being incremental, I find it more confusing to not add them here when the other methods do not yet have the #[deprecated] moniker. Once we do deprecate them before 0.16, then feel free to remove these again!

Given that sometimes PRs can miss a cut-off, or contributors don't have time before an RC/release, leaving changes unfinished seems like an uneccessary risk, when both adding and removing is simple implementation-wise.

in the spirit of being incremental, I find it more confusing to not add them here

Well, in that case, you forgot get_many_unique_manual(&self, &World, entities) and get_many_unique_unchecked(&mut self, UnsafeWorldCell, entities) :P

in the spirit of being incremental, I find it more confusing to not add them here

Well, in that case, you forgot get_many_unique_manual(&self, &World, entities) and get_many_unique_unchecked(&mut self, UnsafeWorldCell, entities) :P

These already don't exist for get_many, so I haven't added them here.
These inconsistencies are not nice, but we can at least not make them worse in the interim :P

chescock · 2025-03-14T19:30:41Z

crates/bevy_ecs/src/system/query.rs

+    #[inline]
+    pub fn get_many_unique_inner<const N: usize>(
+        self,
+        entities: UniqueEntityArray<Entity, N>,


Would it make sense to generalize this from Entity to T: TrustedEntityBorrow? (And I suppose the read-only one could be generalized to T: EntityBorrow.)

Yeah, we could generalize them!
However, this also goes for a lot more places than just these methods.
The general decision of "We want to accept EntityBorrow instead of Entity in our APIs" is contentious, so I'd like to leave that for when the EntitySet-family of features is done.
(With iter_many, Borrow<Entity> was already present)

However, this also goes for a lot more places than just these methods.
The general decision of "We want to accept EntityBorrow instead of Entity in our APIs" is contentious, so I'd like to leave that for when the EntitySet-family of features is done.

Oh, I remember waffling about this when writing #18234. Checking now, it looks like I left a few as T: TrustedEntityBorrow. Should I switch those back to Entity, too?

Did you? I don't see them!

The current status quo is:
iter_many: takes IntoIterator<Item: EntityBorrow>
iter_many_unique takes EntitySet

The iter_many signatures are currently the only signatures where we use EntityBorrow, and wherever we use EntitySet we have a hidden TrustedEntityBorrow bound.
Otherwise, the EntityBorrow and TrustedEntityBorrow traits shouldn't appear in non-EntitySet related methods for now I think.

crates/bevy_ecs/src/query/state.rs

implement get_many_unique

e1b1439

Victoronz added A-ECS Entities, components, systems, and events D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 14, 2025

Carter0 reviewed Mar 14, 2025

View reviewed changes

fix missed renames and improve docs

a2f9630

Carter0 approved these changes Mar 14, 2025

View reviewed changes

infer Entity properly

5e6e71c

chescock approved these changes Mar 14, 2025

View reviewed changes

Victoronz mentioned this pull request Mar 14, 2025

Restore separate methods for World::get_many_entities #18234

Open

Victoronz added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 14, 2025

Victoronz added 3 commits March 15, 2025 03:38

Merge branch 'main' into get-many-unique

70ba56b

Account for the new type default

f5c4c9c

doc fixes

b6f4c6a

Victoronz added this to the 0.16 milestone Mar 15, 2025

chescock approved these changes Mar 15, 2025

View reviewed changes

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

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

elide remaining UniqueEntityArray defaults uses

6993690

Victoronz added the C-Feature A new feature, making something new possible label Mar 15, 2025

alice-i-cecile added this pull request to the merge queue Mar 16, 2025

Merged via the queue into bevyengine:main with commit b7d5254 Mar 16, 2025
32 checks passed

Uh oh!

implement get_many_unique #18315

implement get_many_unique #18315

Uh oh!

Conversation

Victoronz commented Mar 14, 2025 • edited by alice-i-cecile Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Testing

Main Branch Migration Guide

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Victoronz Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Carter0 Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Victoronz Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Carter0 left a comment

Choose a reason for hiding this comment

Uh oh!

Victoronz commented Mar 14, 2025

Uh oh!

chescock left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Victoronz Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Victoronz Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Victoronz commented Mar 14, 2025 •

edited by alice-i-cecile

Loading

Victoronz Mar 14, 2025 •

edited

Loading

Carter0 Mar 14, 2025 •

edited

Loading

Victoronz Mar 14, 2025 •

edited

Loading

Victoronz Mar 14, 2025 •

edited

Loading

Victoronz Mar 14, 2025 •

edited

Loading