EntityRef/Mut get_components #13375

cart · 2024-05-14T23:27:15Z

Objective

Implements #13127

EntityRef / EntityMut usage is currently unnecessarily verbose and borrow-checker-constrained. It is currently impossible to use EntityMut to get mutable references to more than one component at the same time:

let mut entity = world.entity_mut(some_entity);
let mut a = entity.get_mut::<A>();
// this fails to compile because `entity` is already borrowed mutably
let mut b = entity.get_mut::<B>();

This makes EntityMut significantly less useful than it could be. Querying necessary components on demand is a pattern many prefer to use over traditional top-level queries.

Solution

Implement new get_components and components methods for EntityRef and EntityMut (and mutable variants for EntityMut) to provide multi-component access:

let (a, b) = world.entity(SOME_ENTITY).components::<(&A, &B)>();
let (a, b) = world.entity(SOME_ENTITY).get_components::<(&A, &B)>().unwrap();
let (mut a, b) = world.entity_mut(SOME_ENTITY).components_mut::<(&mut A, &B)>();
let (mut a, b) = world.entity_mut(SOME_ENTITY).get_components_mut::<(&mut A, &B)>().unwrap();

This is implemented using our existing Query implementation to provide multi-component access. It checks access without actually computing QueryState / access using the very convenient Q::matches_component_set.

Next Steps

This doesn't add FilteredEntityRef and FilteredEntityMut variants, as checking that access is actually more challenging (and with the current methods, likely prohibitively expensive because I think we need to compute Access<ComponentId> )
This doesn't do the components to get and get_components to try_get renaming / unification I proposed in Add EntityMut::components and EntityRef and EntityWorldMut equivalent #13127 as I suspect that will be more controversial, and it also touches a lot of code.

Changelog

Added

EntityRef::components, EntityRef::get_components, EntityMut::components, EntityMut::get_components, EntityMut::components_mut, and EntityMut::get_components_mut.

Changed

Renamed FilteredEntityRef::components to FilteredEntityRef::accessed_components and FilteredEntityMut::components to FilteredEntityMut::accessed_components to avoid future name clashes

Migration Guide

Rename FilteredEntityRef::components to FilteredEntityRef::accessed_components and FilteredEntityMut::components to FilteredEntityMut::accessed_components

…_mut

crates/bevy_ecs/src/world/entity_ref.rs

msvbg · 2024-05-14T23:32:15Z

In light of #12660, I would vote against adding more panicking shorthands like components.

crates/bevy_ecs/src/world/entity_ref.rs

cBournhonesque

Would this also allow for

let (mut a, mut b) = world.entity_mut(SOME_ENTITY).components_mut::<(&mut A, &mut B)>();

?

Maybe we can add a unit test for that case as well

…world()

…panic message

Co-authored-by: Periwink <charlesbour@gmail.com>

cart · 2024-05-15T00:02:00Z

In light of #12660, I would vote against adding more panicking shorthands like components.

I think discouraging unwrap/panicking variants entirely (as in, not providing impls) would have pretty nasty ergonomics implications for people, especially with the current Option vs Result compatibility problem for ?. I would be very uncomfortable removing those apis right now.

ramirezmike · 2024-05-15T00:06:45Z

In light of #12660, I would vote against adding more panicking shorthands like components.

I think discouraging unwrap/panicking variants entirely (as in, not providing impls) would have pretty nasty ergonomics implications for people, especially with the current Option vs Result compatibility problem for ?. I would be very uncomfortable removing those apis right now.

I agree somewhat with this. It's frustrating when something like despawn causes a panic due to an unwrap, but if I want to shoot myself in the foot with something like single or components_mut I should be allowed to.

cart · 2024-05-15T00:32:11Z

Left some replies in #12660

hymm · 2024-05-15T00:52:32Z

crates/bevy_ecs/src/world/entity_ref.rs

+        );
+        assert_eq!(None, world.entity(e2).get_components::<(&X, &Y)>());
+        assert_eq!(None, world.entity(e3).get_components::<(&X, &Y)>());
+    }


can you add a test to make sure it returns none or panics if you do world.entity_mut(e).get_components_mut::<&mut X, &mut X>()?

also should have one for (&mut X, &X)

Haha I'm so glad you asked this question. The Access part of the Query infrastructure was what enforced this elsewhere, so this was allowed. We can't merge this as-is, and fixing it will almost certainly incur a significant perf cost.

Wouldn't creating an empty access and calling update_component_access(state, &mut access) on it work?

It would. It would just be dissatisfying from a perf perspective because Access allocates.

FYI I have a PR that removes the allocation from an empty Access: https://github.com/bevyengine/bevy/pull/14026/files which might unblock this?

Or maybe not.. calling update_component_access will itself allocate by updating the bitsets

Could the allocation not just be removed in the meantime by using a smallvec that has 8 items or something?

hymm · 2024-05-15T00:53:53Z

crates/bevy_ecs/src/world/entity_ref.rs

+    }
+
+    /// Returns read-only components for the current entity that match the query `Q`.
+    pub fn get_components<Q: ReadOnlyQueryData>(&self) -> Option<Q::Item<'w>> {


Should add some docs to explain why it might return None. Ditto for the other methods that return an option.

james7132

I find this kind of funny. Thinking about this historically, this feels like we've inverted the relationship between Query::get_component and EntityMut/EntityRef with this change.

I think discouraging unwrap/panicking variants entirely (as in, not providing impls) would have pretty nasty ergonomics implications for people, especially with the current Option vs Result compatibility problem for ?. I would be very uncomfortable removing those apis right now.

I agree that we probably shouldn't remove them until try-blocks or some equivalent are stabilized.

james7132 · 2024-05-15T01:28:13Z

crates/bevy_ecs/src/world/unsafe_world_cell.rs

+                .get(location.archetype_id)
+                .debug_checked_unwrap()
+        };
+        if Q::matches_component_set(&state, &|id| archetype.contains(id)) {


Could use bool::then instead of the if/else here.

ramirezmike · 2024-05-15T01:50:15Z

Since this uses QueryData, this would allow doing stuff like this, right?

world.entity(e).get_components::<(Entity, &X, Option<&Y>, Has<Z>)>();

"components" sorta makes sense for Option/Has... Is Entity a component though? What if QueryData gets implemented with more Component-adjacent stuff in the future?

I'm guessing the next steps item 2 would be renaming these to the get functions in the future?

cart · 2024-05-15T19:03:14Z

I find this kind of funny. Thinking about this historically, this feels like we've inverted the relationship between Query::get_component and EntityMut/EntityRef with this change.

Haha yeah thats an interesting comparison. Although I think this is significantly more defensible / necessary than Query::get_component.

Since this uses QueryData, this would allow doing stuff like this, right?

Yup!

I'm guessing the next steps item 2 would be renaming these to the get functions in the future?

Yup that is something discussed in the Next Steps section above. Haven't thought of a better interim name. And components is approximately correct.

cart · 2024-05-15T19:21:02Z

Welp this PR is sadly stalled until we sort out this problem:
#13375 (comment)

Not sure there will be a solution that doesn't involve doing access checks on each call.

cart · 2024-05-15T19:24:26Z

I think the best we can hope for is "allocation free check every entry against every other entry" (similar to query.many_mut), but accomplishing that will require significant changes (or at least, additions) to how we compute access.

alice-i-cecile

I'm in favor of this existing! I agree with the request for more thorough docs (shock). Nonblocking, but I also think we should be returning Result, not Option here (as with most of our ECS APIs). QueryEntityError looks like the closest fit, but would need to be tweaked to better fit the actual problems encountered (and API used).

Obviously the UB is a blocker here on this getting merged. Given the discovered complexity, I'm removing this from the 0.14 milestone <3 I still want this, and if it makes it it makes it, but it's not correct to try and wait for this to be resolved.

alice-i-cecile · 2024-05-16T00:08:10Z

I think we should be able to reuse the access mechanisms from queries to validate soundness here. Would need to sit down with the code to figure out exactly how. I much prefer that over creating another mechanism though, especially since I'd like to be able to add untyped variants to this in the future.

ItsDoot · 2024-09-08T00:50:38Z

Would it be possible to merge a version of this PR with just the immutable get_components variants for 0.15? Even just the immutable variants would be very useful for me.

alice-i-cecile · 2024-09-08T01:56:01Z

I agree the an immutable form would be useful still. Feel free to spin off a PR that does that please.

alice-i-cecile · 2024-09-09T16:31:41Z

Closing in favor of #15089; further attempts to implement the mutable form will be easier in a separate PR.

# Objective Smaller scoped version of #13375 without the `_mut` variants which currently have unsoundness issues. ## Solution Same as #13375, but without the `_mut` variants. ## Testing - The same test from #13375 is reused. --- ## Migration Guide - Renamed `FilteredEntityRef::components` to `FilteredEntityRef::accessed_components` and `FilteredEntityMut::components` to `FilteredEntityMut::accessed_components`. --------- Co-authored-by: Carter Anderson <mcanders1@gmail.com> Co-authored-by: Periwink <charlesbour@gmail.com>

Entity::components / components_mut / get_components / get_components…

68f09ac

…_mut

cart added the A-ECS Entities, components, systems, and events label May 14, 2024

cart added this to the 0.14 milestone May 14, 2024

cBournhonesque reviewed May 14, 2024

View reviewed changes

crates/bevy_ecs/src/world/entity_ref.rs Outdated Show resolved Hide resolved

cBournhonesque reviewed May 14, 2024

View reviewed changes

crates/bevy_ecs/src/world/entity_ref.rs Outdated Show resolved Hide resolved

cBournhonesque approved these changes May 14, 2024

View reviewed changes

cart and others added 4 commits May 14, 2024 16:54

Update get_components to use world().components() instead of world().…

7b5d7e1

…world()

Add World lifetimes where possible, add EntityWorldMut variants, add …

af6bcc1

…panic message

Update crates/bevy_ecs/src/world/entity_ref.rs

5005365

Co-authored-by: Periwink <charlesbour@gmail.com>

Update crates/bevy_ecs/src/world/entity_ref.rs

8b8a75b

Co-authored-by: Periwink <charlesbour@gmail.com>

hymm reviewed May 15, 2024

View reviewed changes

james7132 reviewed May 15, 2024

View reviewed changes

alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed X-Uncontroversial This work is generally agreed upon S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 15, 2024

alice-i-cecile requested changes May 15, 2024

View reviewed changes

alice-i-cecile removed this from the 0.14 milestone May 15, 2024

iiYese mentioned this pull request May 16, 2024

Use component lifecycle hooks to make Parent + Children hierarchy code simpler and more robust #12235

Open

iiYese mentioned this pull request Jul 8, 2024

Minimal Bubbling Observers #13991

Merged

cBournhonesque mentioned this pull request Jul 16, 2024

Remove extra allocation in AccessFilters #14026

Closed

ItsDoot mentioned this pull request Sep 8, 2024

EntityRef/Mut get_components (immutable variants only) #15089

Merged

alice-i-cecile mentioned this pull request Sep 9, 2024

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

Closed

alice-i-cecile closed this Sep 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EntityRef/Mut get_components #13375

EntityRef/Mut get_components #13375

cart commented May 14, 2024

msvbg commented May 14, 2024

cBournhonesque left a comment

cart commented May 15, 2024

ramirezmike commented May 15, 2024

cart commented May 15, 2024

hymm May 15, 2024

hymm May 15, 2024

cart May 15, 2024

cBournhonesque May 15, 2024

cart May 15, 2024

cBournhonesque Jun 26, 2024 •

edited

Loading

iiYese Jul 8, 2024

hymm May 15, 2024

james7132 left a comment

james7132 May 15, 2024

ramirezmike commented May 15, 2024

cart commented May 15, 2024

cart commented May 15, 2024

cart commented May 15, 2024

alice-i-cecile left a comment

alice-i-cecile commented May 16, 2024

ItsDoot commented Sep 8, 2024

alice-i-cecile commented Sep 8, 2024

alice-i-cecile commented Sep 9, 2024

EntityRef/Mut get_components #13375

EntityRef/Mut get_components #13375

Conversation

cart commented May 14, 2024

Objective

Solution

Next Steps

Changelog

Added

Changed

Migration Guide

msvbg commented May 14, 2024

cBournhonesque left a comment

Choose a reason for hiding this comment

cart commented May 15, 2024

ramirezmike commented May 15, 2024

cart commented May 15, 2024

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

cBournhonesque Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james7132 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramirezmike commented May 15, 2024

cart commented May 15, 2024

cart commented May 15, 2024

cart commented May 15, 2024

alice-i-cecile left a comment

Choose a reason for hiding this comment

alice-i-cecile commented May 16, 2024

ItsDoot commented Sep 8, 2024

alice-i-cecile commented Sep 8, 2024

alice-i-cecile commented Sep 9, 2024

cBournhonesque Jun 26, 2024 •

edited

Loading