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

Populated (query) system param #15488

Merged
merged 13 commits into from
Sep 30, 2024
4 changes: 2 additions & 2 deletions crates/bevy_ecs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ pub mod prelude {
},
system::{
Commands, Deferred, EntityCommand, EntityCommands, In, InMut, InRef, IntoSystem, Local,
NonSend, NonSendMut, ParallelCommands, ParamSet, Query, ReadOnlySystem, Res, ResMut,
Resource, Single, System, SystemIn, SystemInput, SystemParamBuilder,
NonSend, NonSendMut, ParallelCommands, ParamSet, Populated, Query, ReadOnlySystem, Res,
ResMut, Resource, Single, System, SystemIn, SystemInput, SystemParamBuilder,
SystemParamFunction,
},
world::{
Expand Down
42 changes: 42 additions & 0 deletions crates/bevy_ecs/src/system/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,15 @@ use core::{
///
/// [`World`]: crate::world::World
///
/// # Similar parameters
///
/// [`Query`] has few sibling [`SystemParam`](crate::system::system_param::SystemParam)s, which perform additional validation:
/// - [`Single`] - Exactly one matching query item.
/// - [`Option<Single>`] - Zero or one matching query item.
/// - [`Populated`] - At least one matching query item.
///
/// Those parameters will prevent systems from running if their requirements aren't met.
///
/// # System parameter declaration
///
/// A query should always be declared as a system parameter.
Expand Down Expand Up @@ -1667,3 +1676,36 @@ impl<'w, D: QueryData, F: QueryFilter> Single<'w, D, F> {
self.item
}
}

/// [System parameter] that works very much like [`Query`] except it always contains at least one matching entity.
alice-i-cecile marked this conversation as resolved.
Show resolved Hide resolved
///
/// This [`SystemParam`](crate::system::SystemParam) fails validation if no matching entities exist.
/// This will cause systems that use this parameter to be skipped.
///
/// Much like [`Query::is_empty`] the worst case runtime will be `O(n)` where `n` is the number of *potential* matches.
/// This can be notably expensive for queries that rely on non-archetypal filters such as [`Added`](crate::query::Added) or [`Changed`](crate::query::Changed)
/// which must individually check each query result for a match.
///
/// See [`Query`] for more details.
MiniaczQ marked this conversation as resolved.
Show resolved Hide resolved
pub struct Populated<'w, 's, D: QueryData, F: QueryFilter = ()>(pub(crate) Query<'w, 's, D, F>);

impl<'w, 's, D: QueryData, F: QueryFilter> Deref for Populated<'w, 's, D, F> {
type Target = Query<'w, 's, D, F>;

fn deref(&self) -> &Self::Target {
&self.0
}
}

impl<D: QueryData, F: QueryFilter> DerefMut for Populated<'_, '_, D, F> {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.0
}
}

impl<'w, 's, D: QueryData, F: QueryFilter> Populated<'w, 's, D, F> {
/// Returns the inner item with ownership.
pub fn into_inner(self) -> Query<'w, 's, D, F> {
self.0
}
}
57 changes: 57 additions & 0 deletions crates/bevy_ecs/src/system/system_param.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ use core::{
ops::{Deref, DerefMut},
};

use super::Populated;

/// A parameter that can be used in a [`System`](super::System).
///
/// # Derive
Expand Down Expand Up @@ -497,6 +499,61 @@ unsafe impl<'a, D: ReadOnlyQueryData + 'static, F: QueryFilter + 'static> ReadOn
{
}

// SAFETY: Relevant query ComponentId and ArchetypeComponentId access is applied to SystemMeta. If
// this Query conflicts with any prior access, a panic will occur.
unsafe impl<D: QueryData + 'static, F: QueryFilter + 'static> SystemParam
for Populated<'_, '_, D, F>
{
type State = QueryState<D, F>;
type Item<'w, 's> = Populated<'w, 's, D, F>;

fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State {
Query::init_state(world, system_meta)
}

unsafe fn new_archetype(
state: &mut Self::State,
archetype: &Archetype,
system_meta: &mut SystemMeta,
) {
// SAFETY: Delegate to existing `SystemParam` implementations.
Copy link
Contributor

Choose a reason for hiding this comment

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

SAFETY: Delegate to existing ``Query`` implementation.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then the doc comment says exactly what's written
The point is, we use a different SystemParam implementation which satisfies the same safety restrictions

unsafe { Query::new_archetype(state, archetype, system_meta) };
}

#[inline]
unsafe fn get_param<'w, 's>(
state: &'s mut Self::State,
system_meta: &SystemMeta,
world: UnsafeWorldCell<'w>,
change_tick: Tick,
) -> Self::Item<'w, 's> {
// SAFETY: Delegate to existing `SystemParam` implementations.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

let query = unsafe { Query::get_param(state, system_meta, world, change_tick) };
Populated(query)
}

#[inline]
unsafe fn validate_param(
state: &Self::State,
system_meta: &SystemMeta,
world: UnsafeWorldCell,
) -> bool {
state.validate_world(world.id());
// SAFETY:
// - We have read-only access to the components accessed by query.
// - The world has been validated.
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be clearer to say something like, The world matches the one used to create ``state``. (Just means the reader doesn't need to look at validate_world to know what it does)

Copy link
Contributor Author

@MiniaczQ MiniaczQ Sep 29, 2024

Choose a reason for hiding this comment

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

it's a common term in ECS safety comments,
I'm pretty sure all of the safety comments here are copy-pasted from other implementations

!unsafe {
state.is_empty_unsafe_world_cell(world, system_meta.last_run, world.change_tick())
}
}
}

// SAFETY: QueryState is constrained to read-only fetches, so it only reads World.
unsafe impl<'w, 's, D: ReadOnlyQueryData + 'static, F: QueryFilter + 'static> ReadOnlySystemParam
for Populated<'w, 's, D, F>
{
}

/// A collection of potentially conflicting [`SystemParam`]s allowed by disjoint access.
///
/// Allows systems to safely access and interact with up to 8 mutually exclusive [`SystemParam`]s, such as
Expand Down
13 changes: 7 additions & 6 deletions examples/ecs/fallible_params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@
//! from running if their acquiry conditions aren't met.
//!
//! Fallible parameters include:
//! - [`Res<R>`], [`ResMut<R>`] - If resource doesn't exist.
//! - [`Single<D, F>`] - If there is no or more than one entities matching.
//! - [`Option<Single<D, F>>`] - If there are more than one entities matching.
//! - [`Res<R>`], [`ResMut<R>`] - Resource has to exist.
//! - [`Single<D, F>`] - There must be exactly one matching entity.
//! - [`Option<Single<D, F>>`] - There must be zero or one matching entity.
//! - [`Populated<D, F>`] - There must be at least one matching entity.

use bevy::prelude::*;
use rand::Rng;
Expand Down Expand Up @@ -105,9 +106,9 @@ fn user_input(
}

// System that moves the enemies in a circle.
// TODO: Use [`NonEmptyQuery`] when it exists.
fn move_targets(mut enemies: Query<(&mut Transform, &mut Enemy)>, time: Res<Time>) {
for (mut transform, mut target) in &mut enemies {
// Only runs if there are enemies.
fn move_targets(mut enemies: Populated<(&mut Transform, &mut Enemy)>, time: Res<Time>) {
for (mut transform, mut target) in &mut *enemies {
target.rotation += target.rotation_speed * time.delta_seconds();
transform.rotation = Quat::from_rotation_z(target.rotation);
let offset = transform.right() * target.radius;
Expand Down