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

QuerySingle family of system params #15476

Merged
merged 20 commits into from
Sep 28, 2024
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -1892,6 +1892,17 @@ description = "Run systems only when one or multiple conditions are met"
category = "ECS (Entity Component System)"
wasm = false

[[example]]
name = "fallible_params"
path = "examples/ecs/fallible_params.rs"
doc-scrape-examples = true

[package.metadata.example.fallible_params]
name = "Fallible System Parameters"
description = "Systems are skipped if their parameters cannot be acquired"
category = "ECS (Entity Component System)"
wasm = false

[[example]]
name = "startup_system"
path = "examples/ecs/startup_system.rs"
Expand Down
21 changes: 9 additions & 12 deletions crates/bevy_ecs/src/change_detection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -543,11 +543,10 @@ impl<'w> From<TicksMut<'w>> for Ticks<'w> {
///
/// If you need a unique mutable borrow, use [`ResMut`] instead.
///
/// # Panics
/// This [`SystemParam`](crate::system::SystemParam) fails validation if resource doesn't exist.
/// This will cause systems that use this parameter to be skipped.
///
/// Panics when used as a [`SystemParameter`](crate::system::SystemParam) if the resource does not exist.
///
/// Use `Option<Res<T>>` instead if the resource might not always exist.
/// Use [`Option<Res<T>>`] instead if the resource might not always exist.
pub struct Res<'w, T: ?Sized + Resource> {
pub(crate) value: &'w T,
pub(crate) ticks: Ticks<'w>,
Expand Down Expand Up @@ -622,11 +621,10 @@ impl_debug!(Res<'w, T>, Resource);
///
/// If you need a shared borrow, use [`Res`] instead.
///
/// # Panics
///
/// Panics when used as a [`SystemParam`](crate::system::SystemParam) if the resource does not exist.
/// This [`SystemParam`](crate::system::SystemParam) fails validation if resource doesn't exist.
/// This will cause systems that use this parameter to be skipped.
///
/// Use `Option<ResMut<T>>` instead if the resource might not always exist.
/// Use [`Option<ResMut<T>>`] instead if the resource might not always exist.
pub struct ResMut<'w, T: ?Sized + Resource> {
pub(crate) value: &'w mut T,
pub(crate) ticks: TicksMut<'w>,
Expand Down Expand Up @@ -684,11 +682,10 @@ impl<'w, T: Resource> From<ResMut<'w, T>> for Mut<'w, T> {
/// the scheduler to instead run the system on the main thread so that it doesn't send the resource
/// over to another thread.
///
/// # Panics
///
/// Panics when used as a `SystemParameter` if the resource does not exist.
/// This [`SystemParam`](crate::system::SystemParam) fails validation if non-send resource doesn't exist.
/// This will cause systems that use this parameter to be skipped.
///
/// Use `Option<NonSendMut<T>>` instead if the resource might not always exist.
/// Use [`Option<NonSendMut<T>>`] instead if the resource might not always exist.
pub struct NonSendMut<'w, T: ?Sized + 'static> {
pub(crate) value: &'w mut T,
pub(crate) ticks: TicksMut<'w>,
Expand Down
5 changes: 3 additions & 2 deletions crates/bevy_ecs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,9 @@ pub mod prelude {
},
system::{
Commands, Deferred, EntityCommand, EntityCommands, In, InMut, InRef, IntoSystem, Local,
NonSend, NonSendMut, ParallelCommands, ParamSet, Query, ReadOnlySystem, Res, ResMut,
Resource, System, SystemIn, SystemInput, SystemParamBuilder, SystemParamFunction,
NonSend, NonSendMut, ParallelCommands, ParamSet, Query, QuerySingle, ReadOnlySystem,
Res, ResMut, Resource, System, SystemIn, SystemInput, SystemParamBuilder,
SystemParamFunction,
},
world::{
Command, EntityMut, EntityRef, EntityWorldMut, FromWorld, OnAdd, OnInsert, OnRemove,
Expand Down
40 changes: 39 additions & 1 deletion crates/bevy_ecs/src/system/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@ use crate::{
},
world::unsafe_world_cell::UnsafeWorldCell,
};
use core::borrow::Borrow;
use core::{
borrow::Borrow,
marker::PhantomData,
ops::{Deref, DerefMut},
};

/// [System parameter] that provides selective access to the [`Component`] data stored in a [`World`].
///
Expand Down Expand Up @@ -1629,3 +1633,37 @@ impl<'w, 'q, Q: QueryData, F: QueryFilter> From<&'q mut Query<'w, '_, Q, F>>
value.transmute_lens_filtered()
}
}

/// [System parameter] that provides access to single entity's components, much like [`Query::single`]/[`Query::single_mut`].
///
/// This [`SystemParam`](crate::system::SystemParam) fails validation if zero or more than one matching entity exists.
/// This will cause systems that use this parameter to be skipped.
///
/// Use [`Option<QuerySingle<D, F>>`] instead if zero or one matching entities can exist.
///
/// See [`Query`] for meaning behind the generic arguments.
pub struct QuerySingle<'w, D: QueryData, F: QueryFilter = ()> {
pub(crate) item: D::Item<'w>,
pub(crate) _filter: PhantomData<F>,
}

impl<'w, D: QueryData, F: QueryFilter> Deref for QuerySingle<'w, D, F> {
type Target = D::Item<'w>;

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

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

impl<'w, D: QueryData, F: QueryFilter> QuerySingle<'w, D, F> {
/// Returns the inner item with ownership.
pub fn into_inner(self) -> D::Item<'w> {
self.item
}
}
144 changes: 138 additions & 6 deletions crates/bevy_ecs/src/system/system_param.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ use crate::{
entity::Entities,
query::{
Access, AccessConflicts, FilteredAccess, FilteredAccessSet, QueryData, QueryFilter,
QueryState, ReadOnlyQueryData,
QuerySingleError, QueryState, ReadOnlyQueryData,
},
storage::{ResourceData, SparseSetIndex},
system::{Query, SystemMeta},
system::{Query, QuerySingle, SystemMeta},
world::{unsafe_world_cell::UnsafeWorldCell, DeferredWorld, FromWorld, World},
};
use bevy_ecs_macros::impl_param_set;
Expand Down Expand Up @@ -356,6 +356,139 @@ fn assert_component_access_compatibility(
panic!("error[B0001]: Query<{query_type}, {filter_type}> in system {system_name} accesses component(s){accesses} in a way that conflicts with a previous system parameter. Consider using `Without<T>` to create disjoint Queries or merging conflicting Queries into a `ParamSet`. See: https://bevyengine.org/learn/errors/b0001");
}

// 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<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam
for QuerySingle<'a, D, F>
{
type State = QueryState<D, F>;
type Item<'w, 's> = QuerySingle<'w, D, F>;
Copy link
Member

Choose a reason for hiding this comment

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

Why isn't this just <D as QueryData>::Item? I think that would improve the ergonomics and complexity quite a bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ergonomics comment got removed, CI told me to remove unnecessary syntax which made it way better.

<D as WorldQuery>::Item is not a SystemParam so that's one issue with this.
Another would be, if SystemParam returns anything else than itself with changed lifetimes, it won't fit as the system argument.

fn my_system(q: QuerySingle<Entity>) { ... }

// imaginary executor code
let q: <Entity as WorldQuery>::Item = QuerySingle<Entity>::get_param(world);
system(q); // Expected `QuerySingle` got `<Entity as WorldQuery>::Item`


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.
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> {
state.validate_world(world.id());
// SAFETY: State ensures that the components it accesses are not accessible somewhere elsewhere.
let result =
unsafe { state.get_single_unchecked_manual(world, system_meta.last_run, change_tick) };
let single = result.unwrap();
MiniaczQ marked this conversation as resolved.
Show resolved Hide resolved
MiniaczQ marked this conversation as resolved.
Show resolved Hide resolved
QuerySingle {
item: single,
_filter: PhantomData,
}
}

#[inline]
unsafe fn validate_param(
state: &Self::State,
system_meta: &SystemMeta,
world: UnsafeWorldCell,
) -> bool {
state.validate_world(world.id());
// SAFETY: State ensures that the components it accesses are not mutably accessible elsewhere
// and the query is read only.
let result = unsafe {
state.as_readonly().get_single_unchecked_manual(
world,
system_meta.last_run,
world.change_tick(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure this is the change tick that will be used for calling get_param, or an equivalent one for what matters to it? If so could you write a comment explaining why that's the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Practically yes, this happens to be the exact tick that is received by get_param, but there are not validation mechanisms/contract for that.

Currently only executors use validate_param and since the validation and running happens one after another, this is the correct change tick.
For run_systems and observers this also shouldn't be a problem.

While I can make validate_param accept a change_tick, the problem is that the tick used by get_param is generated inside run_unsafe, so we'd still need to uphold this "contract" of calling them back-to-back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm open for docs/mechanisms on making this better, I'm not sure how to go about it

Copy link
Contributor

Choose a reason for hiding this comment

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

It won't always be exactly the same tick, because the multi-threaded executor will call validate_param and then spawn a task to run the system and call get_param at some later point. Another system could start in between and increment the current tick.

I think it will be equivalent, though. Any components read by this param can't be written by another system in between the calls, so they can't have an added or changed tick in between the two change_tick values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll try to thoroughly document this in validate_param

)
};
result.is_ok()
}
}

// 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<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam
for Option<QuerySingle<'a, D, F>>
Comment on lines +430 to +431
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if now that we have validate_param we could use it to write a generic impl<S: SystemParam> SystemParam for Option<S>. This could be tackled in a separate PR though.

Copy link
Contributor Author

@MiniaczQ MiniaczQ Sep 28, 2024

Choose a reason for hiding this comment

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

Not every Option<P> makes sense, like Option<Query>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, infinitely nested Options

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, though maybe this could be solved with a marker trait for those system parameters for which Option can make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there is a decent way to derive it I'm down.
There are some cases where Option needs some specialized code, like in QuerySingle

{
type State = QueryState<D, F>;
type Item<'w, 's> = Option<QuerySingle<'w, D, F>>;

fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State {
QuerySingle::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.
unsafe { QuerySingle::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> {
state.validate_world(world.id());
// SAFETY: State ensures that the components it accesses are not accessible elsewhere.
let result =
unsafe { state.get_single_unchecked_manual(world, system_meta.last_run, change_tick) };
match result {
Ok(single) => Some(QuerySingle {
item: single,
_filter: PhantomData,
}),
Err(QuerySingleError::NoEntities(_)) => None,
Err(QuerySingleError::MultipleEntities(e)) => panic!("{}", e),
}
}

#[inline]
unsafe fn validate_param(
state: &Self::State,
system_meta: &SystemMeta,
world: UnsafeWorldCell,
) -> bool {
state.validate_world(world.id());
// SAFETY: State ensures that the components it accesses are not mutably accessible elsewhere
// and the query is read only.
let result = unsafe {
state.as_readonly().get_single_unchecked_manual(
world,
system_meta.last_run,
world.change_tick(),
)
};
!matches!(result, Err(QuerySingleError::MultipleEntities(_)))
}
}

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

// SAFETY: QueryState is constrained to read-only fetches, so it only reads World.
unsafe impl<'a, D: ReadOnlyQueryData + 'static, F: QueryFilter + 'static> ReadOnlySystemParam
for Option<QuerySingle<'a, 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 Expand Up @@ -1172,11 +1305,10 @@ unsafe impl<T: SystemBuffer> SystemParam for Deferred<'_, T> {
/// the scheduler to instead run the system on the main thread so that it doesn't send the resource
/// over to another thread.
///
/// # Panics
///
/// Panics when used as a `SystemParameter` if the resource does not exist.
/// This [`SystemParam`] fails validation if non-send resource doesn't exist.
/// This will cause systems that use this parameter to be skipped.
///
/// Use `Option<NonSend<T>>` instead if the resource might not always exist.
/// Use [`Option<NonSend<T>>`] instead if the resource might not always exist.
pub struct NonSend<'w, T: 'static> {
pub(crate) value: &'w T,
ticks: ComponentTicks,
Expand Down
1 change: 1 addition & 0 deletions examples/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,7 @@ Example | Description
[Dynamic ECS](../examples/ecs/dynamic.rs) | Dynamically create components, spawn entities with those components and query those components
[ECS Guide](../examples/ecs/ecs_guide.rs) | Full guide to Bevy's ECS
[Event](../examples/ecs/event.rs) | Illustrates event creation, activation, and reception
[Fallible System Parameters](../examples/ecs/fallible_params.rs) | Systems are skipped if their parameters cannot be acquired
[Fixed Timestep](../examples/ecs/fixed_timestep.rs) | Shows how to create systems that run every fixed timestep, rather than every tick
[Generic System](../examples/ecs/generic_system.rs) | Shows how to create systems that can be reused with different types
[Hierarchy](../examples/ecs/hierarchy.rs) | Creates a hierarchy of parents and children entities
Expand Down
Loading
Loading