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

Implement WorldQuery for EntityRef #6960

Merged
merged 13 commits into from
Jun 22, 2023

Conversation

james7132
Copy link
Member

@james7132 james7132 commented Dec 15, 2022

Objective

Partially address #5504. Fix #4278. Provide "whole entity" access in queries. This can be useful when you don't know at compile time what you're accessing (i.e. reflection via ReflectComponent).

Solution

Implement WorldQuery for EntityRef.

  • This provides read-only access to the entire entity, and supports anything that EntityRef can normally do.
  • It matches all archetypes and tables and will densely iterate when possible.
  • It marks all of the ArchetypeComponentIds of a matched archetype as read.
  • Adding it to a query will cause it to panic if used in conjunction with any other mutable access.
  • Expanded the docs on Query to advertise this feature.
  • Added tests to ensure the panics were working as intended.
  • Added EntityRef to the ECS prelude.

To make this safe, EntityRef::world was removed as it gave potential UnsafeCell-like access to other parts of the World including aliased mutable access to the components it would otherwise read safely.

Performance

Not great beyond the additional parallelization opportunity over exclusive systems. The EntityRef is fetched from Entities like any other call to World::entity, which can be very random access heavy. This could be simplified if ArchetypeRow is available in WorldQuery::fetch's arguments, but that's likely not something we should optimize for.

Future work

An equivalent API where it gives mutable access to all components on a entity can be done with a scoped version of EntityMut where it does not provide &mut World access nor allow for structural changes to the entity is feasible as well. This could be done as a safe alternative to exclusive system when structural mutation isn't required or the target set of entities is scoped.


Changelog

Added: Access::has_any_write
Added: EntityRef now implements WorldQuery. Allows read-only access to the entire entity, incompatible with any other mutable access, can be mixed with With/Without filters for more targeted use.
Added: EntityRef to bevy::ecs::prelude.
Removed: EntityRef::world

Migration Guide

EntityRef::world has been removed to make EntityRef sound to use as a query result. If you were retrieving EntityRef via World::entity or World::get_entity. Save a copy of the reference to the World before calling World::entity.

// In 0.10
let entity_ref = world.entity(id);
let world_2 = entity_ref.world();

// In 0.11
let world_2 = &world;
let entity_ref = world.entity(id);

@james7132 james7132 added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Dec 15, 2022
Co-authored-by: Carter Weinberg <[email protected]>
Copy link
Member

@JoJoJet JoJoJet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The WorldQuery impl looks right to me, and I don't see any real issues. Just a few things that could be added.

I imagine this will be very useful in niche situations.

crates/bevy_ecs/src/system/query.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/query.rs Show resolved Hide resolved
crates/bevy_ecs/src/query/fetch.rs Outdated Show resolved Hide resolved
!access.access().has_any_write(),
"EntityRef conflicts with a previous access in this query. Shared access cannot coincide with exclusive access.",
);
access.read_all();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does a Query<EntityRef> conflict with Res<Foo> with a read_all?

Copy link
Member Author

@james7132 james7132 Jan 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, but a ResMut<T> would conflict under this access model. We could split this between "read all components" and "reads all resources" but I'm not sure how that would interact with the bitsets. Happy to try to do this in this PR, or defer it until later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to split that out (and I do think that should be done): that sort of work needs careful review.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I address this as a part of this PR or should I leave this for later?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave it until later please.

access: &mut Access<ArchetypeComponentId>,
) {
for component_id in archetype.components() {
access.add_read(archetype.get_archetype_component_id(component_id).unwrap());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means that you can have a system with Query<(EntityRef, &A)> that is fine, until you do commands.entity(entity).insert(A), right? Can we have a test for that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's probably still fine since it only mutates the command queue and not the entity directly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I didn't expect it to be unsound, just something to keep in mind where a system may panic depending on the entities and components in the world.

Like how (a: Query<(&mut A, &C)>, b: Query<(&mut A, &B)>) used to only conflict as soon as a (A, B, C) entity is spawned. https://bevyengine.org/news/bevy-0-5/#query-conflicts-use-componentid-instead-of-archetypecomponentid

crates/bevy_ecs/src/world/entity_ref.rs Outdated Show resolved Hide resolved
@james7132 james7132 added this to the 0.11 milestone Feb 27, 2023
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 19, 2023
@alice-i-cecile
Copy link
Member

@james7132 I'm happy with this work and glad to see more steps towards runtime component access coming together! Once you've resolved conflicts feel free to merge <3

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me!
I've resolved conflicts (and adjusted to the UnsafeWorldCell changes)

@cart cart enabled auto-merge June 22, 2023 21:08
@cart cart added this pull request to the merge queue Jun 22, 2023
Merged via the queue into bevyengine:main with commit 70f91b2 Jun 22, 2023
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 targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow users to read / write from all components of a given entity using an All WorldQuery types
6 participants