-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
QuerySingle
family of system params
#15476
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
Changes from all commits
74d8f3d
45a61ec
0ae06f4
8010a0b
859969a
8d0af32
125002a
81a3dbd
1cd8a20
46226aa
7f6dd88
801d37c
3fc7713
7ff336f
1b84446
e48b003
8df8521
c834921
9d0ab0f
4c0ccb2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -227,12 +227,21 @@ pub unsafe trait SystemParam: Sized { | |
/// The [`world`](UnsafeWorldCell) can only be used to read param's data | ||
/// and world metadata. No data can be written. | ||
/// | ||
/// When using system parameters that require `change_tick` you can use | ||
/// [`UnsafeWorldCell::change_tick()`]. Even if this isn't the exact | ||
/// same tick used for [`SystemParam::get_param`], the world access | ||
/// ensures that the queried data will be the same in both calls. | ||
/// | ||
/// This method has to be called directly before [`SystemParam::get_param`] with no other (relevant) | ||
/// world mutations inbetween. Otherwise, while it won't lead to any undefined behavior, | ||
/// the validity of the param may change. | ||
/// | ||
/// # Safety | ||
/// | ||
/// - The passed [`UnsafeWorldCell`] must have read-only access to world data | ||
/// registered in [`init_state`](SystemParam::init_state). | ||
/// - `world` must be the same [`World`] that was used to initialize [`state`](SystemParam::init_state). | ||
/// - all `world`'s archetypes have been processed by [`new_archetype`](SystemParam::new_archetype). | ||
/// - All `world`'s archetypes have been processed by [`new_archetype`](SystemParam::new_archetype). | ||
unsafe fn validate_param( | ||
_state: &Self::State, | ||
_system_meta: &SystemMeta, | ||
|
@@ -356,6 +365,140 @@ 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>; | ||
|
||
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.expect("The query was expected to contain exactly one matching entity."); | ||
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(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Currently only executors use While I can make There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I'll try to thoroughly document this in |
||
) | ||
}; | ||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if now that we have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not every There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, infinitely nested There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there is a decent way to derive it I'm down. |
||
{ | ||
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 | ||
|
@@ -1172,11 +1315,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, | ||
|
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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 aSystemParam
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.