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
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>(
james7132 marked this conversation as resolved.
Show resolved Hide resolved
_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();
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.

}

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());
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

}
}

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.
alice-i-cecile marked this conversation as resolved.
Show resolved Hide resolved
///
/// ```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