Skip to content

Commit

Permalink
Implement WorldQuery for EntityRef (#6960)
Browse files Browse the repository at this point in the history
# 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
TODO

---------

Co-authored-by: Carter Weinberg <[email protected]>
Co-authored-by: Jakob Hellermann <[email protected]>
Co-authored-by: Carter Anderson <[email protected]>
  • Loading branch information
4 people authored Jun 22, 2023
1 parent 724e69b commit 70f91b2
Show file tree
Hide file tree
Showing 5 changed files with 236 additions and 68 deletions.
18 changes: 16 additions & 2 deletions crates/bevy_ecs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ pub mod prelude {
Commands, Deferred, In, IntoSystem, Local, NonSend, NonSendMut, ParallelCommands,
ParamSet, Query, ReadOnlySystem, Res, ResMut, Resource, System, SystemParamFunction,
},
world::{FromWorld, World},
world::{EntityRef, FromWorld, World},
};
}

Expand All @@ -70,7 +70,7 @@ mod tests {
entity::Entity,
query::{Added, Changed, FilteredAccess, ReadOnlyWorldQuery, With, Without},
system::Resource,
world::{Mut, World},
world::{EntityRef, Mut, World},
};
use bevy_tasks::{ComputeTaskPool, TaskPool};
use std::{
Expand Down Expand Up @@ -1323,13 +1323,27 @@ mod tests {
world.query::<(&A, &mut A)>();
}

#[test]
#[should_panic]
fn entity_ref_and_mut_query_panic() {
let mut world = World::new();
world.query::<(EntityRef, &mut A)>();
}

#[test]
#[should_panic]
fn mut_and_ref_query_panic() {
let mut world = World::new();
world.query::<(&mut A, &A)>();
}

#[test]
#[should_panic]
fn mut_and_entity_ref_query_panic() {
let mut world = World::new();
world.query::<(&mut A, EntityRef)>();
}

#[test]
#[should_panic]
fn mut_and_mut_query_panic() {
Expand Down
5 changes: 5 additions & 0 deletions crates/bevy_ecs/src/query/access.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,11 @@ impl<T: SparseSetIndex> Access<T> {
self.writes.contains(index.sparse_set_index())
}

/// Returns `true` if this accesses anything mutably.
pub fn has_any_write(&self) -> bool {
!self.writes.is_clear()
}

/// Sets this as having access to all indexed elements (i.e. `&World`).
pub fn read_all(&mut self) {
self.reads_all = true;
Expand Down
85 changes: 84 additions & 1 deletion crates/bevy_ecs/src/query/fetch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use crate::{
entity::Entity,
query::{Access, DebugCheckedUnwrap, FilteredAccess},
storage::{ComponentSparseSet, Table, TableRow},
world::{unsafe_world_cell::UnsafeWorldCell, Mut, Ref, World},
world::{unsafe_world_cell::UnsafeWorldCell, EntityRef, Mut, Ref, World},
};
pub use bevy_ecs_macros::WorldQuery;
use bevy_ptr::{ThinSlicePtr, UnsafeCellDeref};
Expand Down Expand Up @@ -536,6 +536,89 @@ unsafe impl WorldQuery for Entity {
/// SAFETY: access is read only
unsafe impl ReadOnlyWorldQuery for Entity {}

/// SAFETY: `Self` is the same as `Self::ReadOnly`
unsafe impl<'a> WorldQuery for EntityRef<'a> {
type Fetch<'w> = &'w World;
type Item<'w> = EntityRef<'w>;
type ReadOnly = Self;
type State = ();

fn shrink<'wlong: 'wshort, 'wshort>(item: Self::Item<'wlong>) -> Self::Item<'wshort> {
item
}

const IS_DENSE: bool = true;

const IS_ARCHETYPAL: bool = true;

unsafe fn init_fetch<'w>(
world: UnsafeWorldCell<'w>,
_state: &Self::State,
_last_run: Tick,
_this_run: Tick,
) -> Self::Fetch<'w> {
// SAFE: EntityRef has permission to access the whole world immutably thanks to update_component_access and update_archetype_component_access
world.world()
}

unsafe fn clone_fetch<'w>(world: &Self::Fetch<'w>) -> Self::Fetch<'w> {
world
}

#[inline]
unsafe fn set_archetype<'w>(
_fetch: &mut Self::Fetch<'w>,
_state: &Self::State,
_archetype: &'w Archetype,
_table: &Table,
) {
}

#[inline]
unsafe fn set_table<'w>(_fetch: &mut Self::Fetch<'w>, _state: &Self::State, _table: &'w Table) {
}

#[inline(always)]
unsafe fn fetch<'w>(
world: &mut Self::Fetch<'w>,
entity: Entity,
_table_row: TableRow,
) -> Self::Item<'w> {
// SAFETY: `fetch` must be called with an entity that exists in the world
unsafe { world.get_entity(entity).debug_checked_unwrap() }
}

fn update_component_access(_state: &Self::State, access: &mut FilteredAccess<ComponentId>) {
assert!(
!access.access().has_any_write(),
"EntityRef conflicts with a previous access in this query. Shared access cannot coincide with exclusive access.",
);
access.read_all();
}

fn update_archetype_component_access(
_state: &Self::State,
archetype: &Archetype,
access: &mut Access<ArchetypeComponentId>,
) {
for component_id in archetype.components() {
access.add_read(archetype.get_archetype_component_id(component_id).unwrap());
}
}

fn init_state(_world: &mut World) {}

fn matches_component_set(
_state: &Self::State,
_set_contains_id: &impl Fn(ComponentId) -> bool,
) -> bool {
true
}
}

/// SAFETY: access is read only
unsafe impl<'a> ReadOnlyWorldQuery for EntityRef<'a> {}

#[doc(hidden)]
pub struct ReadFetch<'w, T> {
// T::Storage = TableStorage
Expand Down
149 changes: 84 additions & 65 deletions crates/bevy_ecs/src/system/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,71 +125,6 @@ pub use system_param::*;

use crate::world::World;

/// Ensure that a given function is a [system](System).
///
/// This should be used when writing doc examples,
/// to confirm that systems used in an example are
/// valid systems.
///
/// # Examples
///
/// The following example will panic when run since the
/// system's parameters mutably access the same component
/// multiple times.
///
/// ```should_panic
/// # use bevy_ecs::{prelude::*, system::assert_is_system};
/// #
/// # #[derive(Component)]
/// # struct Transform;
/// #
/// fn my_system(query1: Query<&mut Transform>, query2: Query<&mut Transform>) {
/// // ...
/// }
///
/// assert_is_system(my_system);
/// ```
pub fn assert_is_system<In: 'static, Out: 'static, Marker>(
system: impl IntoSystem<In, Out, Marker>,
) {
let mut system = IntoSystem::into_system(system);

// Initialize the system, which will panic if the system has access conflicts.
let mut world = World::new();
system.initialize(&mut world);
}

/// Ensure that a given function is a [read-only system](ReadOnlySystem).
///
/// This should be used when writing doc examples,
/// to confirm that systems used in an example are
/// valid systems.
///
/// # Examples
///
/// The following example will fail to compile
/// since the system accesses a component mutably.
///
/// ```compile_fail
/// # use bevy_ecs::{prelude::*, system::assert_is_read_only_system};
/// #
/// # #[derive(Component)]
/// # struct Transform;
/// #
/// fn my_system(query: Query<&mut Transform>) {
/// // ...
/// }
///
/// assert_is_read_only_system(my_system);
/// ```
pub fn assert_is_read_only_system<In: 'static, Out: 'static, Marker, S>(system: S)
where
S: IntoSystem<In, Out, Marker>,
S::System: ReadOnlySystem,
{
assert_is_system(system);
}

/// Conversion trait to turn something into a [`System`].
///
/// Use this to get a system from a function. Also note that every system implements this trait as
Expand Down Expand Up @@ -477,6 +412,83 @@ pub mod adapter {
pub fn ignore<T>(In(_): In<T>) {}
}

/// Ensure that a given function is a [system](System).
///
/// This should be used when writing doc examples,
/// to confirm that systems used in an example are
/// valid systems.
///
/// # Examples
///
/// The following example will panic when run since the
/// system's parameters mutably access the same component
/// multiple times.
///
/// ```should_panic
/// # use bevy_ecs::{prelude::*, system::assert_is_system};
/// #
/// # #[derive(Component)]
/// # struct Transform;
/// #
/// fn my_system(query1: Query<&mut Transform>, query2: Query<&mut Transform>) {
/// // ...
/// }
///
/// assert_is_system(my_system);
/// ```
pub fn assert_is_system<In: 'static, Out: 'static, Marker>(
system: impl IntoSystem<In, Out, Marker>,
) {
let mut system = IntoSystem::into_system(system);

// Initialize the system, which will panic if the system has access conflicts.
let mut world = World::new();
system.initialize(&mut world);
}

/// Ensure that a given function is a [read-only system](ReadOnlySystem).
///
/// This should be used when writing doc examples,
/// to confirm that systems used in an example are
/// valid systems.
///
/// # Examples
///
/// The following example will fail to compile
/// since the system accesses a component mutably.
///
/// ```compile_fail
/// # use bevy_ecs::{prelude::*, system::assert_is_read_only_system};
/// #
/// # #[derive(Component)]
/// # struct Transform;
/// #
/// fn my_system(query: Query<&mut Transform>) {
/// // ...
/// }
///
/// assert_is_read_only_system(my_system);
/// ```
pub fn assert_is_read_only_system<In: 'static, Out: 'static, Marker, S>(system: S)
where
S: IntoSystem<In, Out, Marker>,
S::System: ReadOnlySystem,
{
assert_is_system(system);
}

/// Ensures that the provided system doesn't with itself.
///
/// This function will panic if the provided system conflict with itself.
///
/// Note: this will run the system on an empty world.
pub fn assert_system_does_not_conflict<Out, Params, S: IntoSystem<(), Out, Params>>(sys: S) {
let mut world = World::new();
let mut system = IntoSystem::into_system(sys);
system.initialize(&mut world);
system.run((), &mut world);
}

#[cfg(test)]
mod tests {
use std::any::TypeId;
Expand Down Expand Up @@ -1739,6 +1751,13 @@ mod tests {
query.iter();
}

#[test]
#[should_panic]
fn assert_system_does_not_conflict() {
fn system(_query: Query<(&mut W<u32>, &mut W<u32>)>) {}
super::assert_system_does_not_conflict(system);
}

#[test]
#[should_panic]
fn panic_inside_system() {
Expand Down
47 changes: 47 additions & 0 deletions crates/bevy_ecs/src/system/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,52 @@ use std::{any::TypeId, borrow::Borrow, fmt::Debug};
///
/// An alternative to this idiom is to wrap the conflicting queries into a [`ParamSet`](super::ParamSet).
///
/// ## Whole Entity Access
///
/// [`EntityRef`]s can be fetched from a query. This will give read-only access to any component on the entity,
/// and can be use to dynamically fetch any component without baking it into the query type. Due to this global
/// access to the entity, this will block any other system from parallelizing with it. As such these queries
/// should be sparingly used.
///
/// ```
/// # use bevy_ecs::prelude::*;
/// # #[derive(Component)]
/// # struct ComponentA;
/// # fn system(
/// query: Query<(EntityRef, &ComponentA)>
/// # ) {}
/// # bevy_ecs::system::assert_is_system(system);
/// ```
///
/// As `EntityRef` can read any component on an entity, a query using it will conflict with *any* mutable
/// access. It is strongly advised to couple `EntityRef` queries with the use of either `With`/`Without`
/// filters or `ParamSets`. This also limits the scope of the query, which will improve iteration performance
/// and also allows it to parallelize with other non-conflicting systems.
///
/// ```should_panic
/// # use bevy_ecs::prelude::*;
/// # #[derive(Component)]
/// # struct ComponentA;
/// # fn system(
/// // This will panic!
/// query: Query<(EntityRef, &mut ComponentA)>
/// # ) {}
/// # bevy_ecs::system::assert_system_does_not_conflict(system);
/// ```
/// ```
/// # use bevy_ecs::prelude::*;
/// # #[derive(Component)]
/// # struct ComponentA;
/// # #[derive(Component)]
/// # struct ComponentB;
/// # fn system(
/// // This will not panic.
/// query_a: Query<EntityRef, With<ComponentA>>,
/// query_b: Query<&mut ComponentB, Without<ComponentA>>,
/// # ) {}
/// # bevy_ecs::system::assert_system_does_not_conflict(system);
/// ```
///
/// # Accessing query items
///
/// The following table summarizes the behavior of the safe methods that can be used to get query items.
Expand Down Expand Up @@ -248,6 +294,7 @@ use std::{any::TypeId, borrow::Borrow, fmt::Debug};
/// [`Changed`]: crate::query::Changed
/// [components]: crate::component::Component
/// [entity identifiers]: crate::entity::Entity
/// [`EntityRef`]: crate::world::EntityRef
/// [`for_each`]: Self::for_each
/// [`for_each_mut`]: Self::for_each_mut
/// [`get`]: Self::get
Expand Down

0 comments on commit 70f91b2

Please sign in to comment.