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 @@ -43,7 +43,7 @@ pub mod prelude {
Commands, In, IntoPipeSystem, IntoSystem, Local, NonSend, NonSendMut, ParallelCommands,
ParamSet, Query, RemovedComponents, Res, ResMut, Resource, System, SystemParamFunction,
},
world::{FromWorld, Mut, World},
world::{EntityRef, FromWorld, Mut, World},
};
}

Expand All @@ -61,7 +61,7 @@ mod tests {
Added, ChangeTrackers, 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 @@ -1329,13 +1329,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 @@ -113,6 +113,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
83 changes: 82 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::{Mut, World},
world::{EntityRef, Mut, World},
};
use bevy_ecs_macros::all_tuples;
pub use bevy_ecs_macros::WorldQuery;
Expand Down Expand Up @@ -515,6 +515,87 @@ 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: &'w World,
_state: &Self::State,
_last_change_tick: u32,
_change_tick: u32,
) -> Self::Fetch<'w> {
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> {
world.get_entity(entity).debug_checked_unwrap()
james7132 marked this conversation as resolved.
Show resolved Hide resolved
}

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
34 changes: 34 additions & 0 deletions crates/bevy_ecs/src/system/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,39 @@ 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
///
/// ```
james7132 marked this conversation as resolved.
Show resolved Hide resolved
/// # use bevy_ecs::prelude::*;
/// # #[derive(Component)]
/// # struct ComponentA;
/// # fn system(
/// // This will panic!
/// query: Query<(EntityRef, &mut ComponentA)>
/// # ) {}
/// # bevy_ecs::system::assert_is_system(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 @@ -246,6 +279,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
5 changes: 0 additions & 5 deletions crates/bevy_ecs/src/world/entity_ref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,6 @@ impl<'w> EntityRef<'w> {
&self.world.archetypes[self.location.archetype_id]
}

#[inline]
pub fn world(&self) -> &'w World {
self.world
}

alice-i-cecile marked this conversation as resolved.
Show resolved Hide resolved
#[inline]
pub fn contains<T: Component>(&self) -> bool {
self.contains_type_id(TypeId::of::<T>())
Expand Down