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

Dynamic queries and builder API #9774

Merged
merged 57 commits into from
Jan 16, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
b56e1c7
Fully dynamic term based query builder
james-j-obrien Sep 11, 2023
5917065
System installation of dynamically built queries
james-j-obrien Sep 11, 2023
b7f04df
Fix change detection, add Changed and Added
james-j-obrien Sep 11, 2023
07dc116
Add additional tests, fix bugs and clean up
james-j-obrien Sep 11, 2023
a12b5dd
Add trivial examples
james-j-obrien Sep 11, 2023
f6382c1
Performance tweaks
james-j-obrien Sep 12, 2023
25623dc
Add dynamic reference methods
james-j-obrien Sep 12, 2023
c16e0ec
Fix rebase issue
james-j-obrien Sep 12, 2023
799b318
Remove examples so github is less angry, rename builder methods to be…
james-j-obrien Sep 12, 2023
711653b
Better builder method names
james-j-obrien Sep 12, 2023
ff1e33a
Major perf improvements, add additional dynamic builder methods
james-j-obrien Sep 12, 2023
40bd8c5
Remove closures from hot loop, remove more runtime checks
james-j-obrien Sep 12, 2023
d0736f5
Remove allocations from hot loop
james-j-obrien Sep 12, 2023
c3d37da
Minor cleanup
james-j-obrien Sep 12, 2023
353c248
Remove #[inline(always)] where it doesn't improve or degrades perform…
james-j-obrien Sep 13, 2023
40c1c62
Flatten terms
james-j-obrien Sep 13, 2023
7667c0a
Fix AnyOf issue
james-j-obrien Sep 13, 2023
031779c
Reorder impls to be more intuitive
james-j-obrien Sep 13, 2023
5494b28
Add dynamic_query example
james-j-obrien Sep 14, 2023
688459d
First docs pass, bug fixes and minor improvements
james-j-obrien Sep 15, 2023
666edb3
Improve documentation, add additional utility functions, add benches
james-j-obrien Sep 18, 2023
5ac0e50
Remove temporary test
james-j-obrien Sep 18, 2023
9d6382f
Minor cleanup
james-j-obrien Sep 18, 2023
f7abf12
First CI pass
james-j-obrien Sep 18, 2023
5c880e9
Second CI pass
james-j-obrien Sep 18, 2023
7680abd
Third times the charm
james-j-obrien Sep 18, 2023
7bd46ab
Add comments
james-j-obrien Sep 18, 2023
db48ac8
Flatten terms, remove branching where possible
james-j-obrien Sep 29, 2023
02091d9
Fix example
james-j-obrien Sep 29, 2023
9a3f5b3
Static iteration, remove FetchedTerm and FetchBuffer
james-j-obrien Sep 30, 2023
88ba5e3
Cleanup component ptr access
james-j-obrien Sep 30, 2023
2cf8f1e
Tidy docs slightly
james-j-obrien Sep 30, 2023
9c65312
Swap to constants to speed up dense iteration, minor API cleanup
james-j-obrien Sep 30, 2023
e9c7861
Consistency pass
james-j-obrien Sep 30, 2023
c674c4a
Move to pointers from iterators, split fetch and filter terms
james-j-obrien Oct 4, 2023
5952756
Add From<Mut<T>> For MutUntyped
james-j-obrien Oct 7, 2023
a76370c
FilteredAccess based implementation
james-j-obrien Nov 14, 2023
323cc74
Merge branch 'dynamic-term-builder' into query-builder
james-j-obrien Nov 14, 2023
ce0ea41
Merge branch 'bevyengine:main' into dynamic-term-builder
james-j-obrien Nov 14, 2023
89f1eac
Doc fixes
james-j-obrien Nov 14, 2023
bf9cee7
Merge branch 'dynamic-term-builder' of https://github.com/james-j-obr…
james-j-obrien Nov 14, 2023
32a6796
Add compile fail tests, add safety comments to example
james-j-obrien Nov 20, 2023
289ca79
Merge main
james-j-obrien Nov 22, 2023
52e0391
Learn how to spell required correctly
james-j-obrien Nov 22, 2023
fecd6bc
Merge main, incorporate feedback
james-j-obrien Nov 30, 2023
04d88cc
Fix doc test
james-j-obrien Nov 30, 2023
3bfcc3f
Fix docs and compile fail tests
james-j-obrien Nov 30, 2023
60c006f
Fix trailing newline
james-j-obrien Nov 30, 2023
b59bb38
Fix trailing whitespace...
james-j-obrien Nov 30, 2023
cf951bb
Merge branch 'main' into dynamic-term-builder
james-j-obrien Dec 2, 2023
2e8a197
Remove unused import
james-j-obrien Dec 2, 2023
5c98ef4
Improve safety comments
james-j-obrien Dec 15, 2023
6f6a0cc
Merge main
james-j-obrien Dec 15, 2023
63f97a3
Address remaining feedback
james-j-obrien Jan 16, 2024
056ff2a
Merge branch 'main' into dynamic-term-builder
james-j-obrien Jan 16, 2024
a3ab076
Fix panic strings
james-j-obrien Jan 16, 2024
8c1bbb0
Fix doc test
james-j-obrien Jan 16, 2024
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
Prev Previous commit
Next Next commit
First CI pass
  • Loading branch information
james-j-obrien committed Sep 18, 2023
commit f7abf1232eb45d9bb32e0b732fd0d85e575bdd24
4 changes: 3 additions & 1 deletion crates/bevy_ecs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,9 @@ pub mod prelude {
ParamSet, Query, ReadOnlySystem, Res, ResMut, Resource, System, SystemParamFunction,
TermQuery,
},
term_query::TermQueryState,
term_query::{
FetchedTerm, QueryBuilder, QueryTerm, QueryTermGroup, Term, TermAccess, TermQueryState,
},
world::{EntityMut, EntityRef, EntityWorldMut, FromWorld, World},
};
}
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_ecs/src/system/function_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -359,10 +359,10 @@ impl<Marker, F> FunctionSystem<Marker, F>
where
F: SystemParamFunction<Marker>,
{
/// Retrieves this systems [SystemParam::State] mutably
/// Retrieves this systems [`SystemParam::State`] mutably
///
/// # Safety
/// Mutable reference must not be used to put the [SystemParam::State]
/// Mutable reference must not be used to put the [`SystemParam::State`]
/// in an invalid state for this [`FunctionSystem`]
Copy link
Member

Choose a reason for hiding this comment

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

I would like more clarity here if possible. Ideally we can point to exactly where the invariants are documented, and which ones are memory safety concerns.

Copy link
Contributor Author

@james-j-obrien james-j-obrien Sep 18, 2023

Choose a reason for hiding this comment

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

The tricky part here is this function allows you to modify any system param state, including user defined system params. That means that we can't really know all the invariants that need to be maintained here. We could try and list invariants for existing system params though e.g. if you change the ComponentId that a Res stores it will point to the wrong component and cast it in an unsafe way. These invariants aren't currently listed anywhere as there was no safe or unsafe way to break them.

Exposing the state directly like this is a little hacky. My preferred solution would be a macro that generates syntax like this:

system.set_state::<0>(query_state)

I didn't want to add that in this PR as it requires a new proc macro and this PR is already big enough, it helps in that the invariant just becomes "make sure the new state is valid" rather than "make sure you don't make invalid edits to the state". I'm open to a safer approach but I'm not sure what that looks like.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this API is too hard for the user to use safely and we should maybe punt this to a separate pr and see if we can provide a safe panicking version similar to Query::transmute

pub unsafe fn state_mut(&mut self) -> &mut <F::Param as SystemParam>::State {
// Archetype generation reset so that new params will be updated with all archetypes
Expand Down
2 changes: 2 additions & 0 deletions crates/bevy_ecs/src/system/system_param.rs
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,8 @@ 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`.");
}

// SAFETY: Relevant query ComponentId and ArchetypeComponentId access is applied to SystemMeta. If
// this TermQuery conflicts with any prior access, a panic will occur.
unsafe impl<Q: QueryTermGroup + 'static, F: QueryTermGroup + 'static> SystemParam
for TermQuery<'_, '_, Q, F>
{
Expand Down
43 changes: 22 additions & 21 deletions crates/bevy_ecs/src/system/term_query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ use crate::{
/// For more information on dynamically building a [`TermQuery`] see [`QueryBuilder`]
///
/// [System parameter]: crate::system::SystemParam
/// [`QueryBuilder`]: crate::term_query::QueryBuilder
/// [`Query`]: crate::system::Query
/// [`Component`]: crate::component::Component
/// [`World`]: crate::World
/// [`QueryBuilder`]: crate::prelude::QueryBuilder
/// [`Query`]: crate::prelude::Query
/// [`Component`]: crate::prelude::Component
/// [`World`]: crate::prelude::World
pub struct TermQuery<'w, 's, Q: QueryTermGroup, F: QueryTermGroup = ()> {
// SAFETY: Must have access to the components registered in `state`.
world: UnsafeWorldCell<'w>,
Expand Down Expand Up @@ -63,8 +63,7 @@ impl<'w, 's, Q: QueryTermGroup, F: QueryTermGroup> TermQuery<'w, 's, Q, F> {
/// # See also
///
/// - [`iter_mut`](Self::iter_mut) for mutable query items.
/// - [`for_each`](Self::for_each) for the closure based alternative.
/// - [`Query::iter`](crate::query::Query::iter) for more examples
/// - [`Query::iter`](crate::prelude::Query::iter) for more examples
#[inline]
pub fn iter(&self) -> TermQueryIter<'_, 's, Q::ReadOnly> {
// SAFETY:
Expand All @@ -82,8 +81,7 @@ impl<'w, 's, Q: QueryTermGroup, F: QueryTermGroup> TermQuery<'w, 's, Q, F> {
/// # See also
///
/// - [`iter`](Self::iter) for read-only query items.
/// - [`for_each_mut`](Self::for_each_mut) for the closure based alternative.
/// - [`Query::iter_mut`](crate::query::Query::iter_mut) for more examples
/// - [`Query::iter_mut`](crate::prelude::Query::iter_mut) for more examples
#[inline]
pub fn iter_mut(&mut self) -> TermQueryIter<'_, 's, Q> {
// SAFETY: `self.world` has permission to access the required components.
Expand All @@ -94,8 +92,8 @@ impl<'w, 's, Q: QueryTermGroup, F: QueryTermGroup> TermQuery<'w, 's, Q, F> {
}

/// Returns an untyped [`Iterator`] over the query items.
/// Provides access to a list of the [`Term`]s used to resolve this query as well as
/// a list of the resolved [`FetchedTerm`]
/// Provides access to a list of the [`Term`](crate::prelude::Term)s used to resolve this query as well as
/// a list of the resolved [`FetchedTerm`](crate::prelude::FetchedTerm)
///
/// # Example
///
Expand All @@ -115,12 +113,12 @@ impl<'w, 's, Q: QueryTermGroup, F: QueryTermGroup> TermQuery<'w, 's, Q, F> {
/// # struct C(usize);
///
/// fn update_system(mut query: TermQuery<(&mut A, &B, &mut C)>) {
/// query.iter_raw().for_each(|terms| {
/// terms.for_each(|(term, fetch)| {
/// query.iter_raw().for_each(|(terms, fetches)| {
/// terms.iter().zip(fetches.iter()).for_each(|(term, fetch)| {
/// if term.access == TermAccess::Write {
/// // Since all the components have the same layout we can cast them all to the same value
/// let component = <mut &A>::from_fetch(fetch);
/// component += 1;
/// // SAFETY: Since all the components have the same layout we can cast them all to the same value
/// let mut component = unsafe { <&mut A>::from_fetch(fetch) };
/// component.0 += 1;
/// }
/// })
/// });
Expand All @@ -129,6 +127,7 @@ impl<'w, 's, Q: QueryTermGroup, F: QueryTermGroup> TermQuery<'w, 's, Q, F> {
/// ```
#[inline]
pub fn iter_raw(&mut self) -> TermQueryIterUntyped<'_, 's> {
// SAFETY: `self.world` has permission to access the required components.
unsafe {
self.state
.iter_raw_manual(self.world, self.last_run, self.this_run)
Expand All @@ -142,7 +141,7 @@ impl<'w, 's, Q: QueryTermGroup, F: QueryTermGroup> TermQuery<'w, 's, Q, F> {
/// # See also
///
/// - [`get_mut`](Self::get_mut) to get a mutable query item.
/// - [`Query::get_mut`](crate::query::Query::get_mut) for more examples
/// - [`Query::get_mut`](crate::prelude::Query::get_mut) for more examples
#[inline]
pub fn get(&self, entity: Entity) -> Result<ROTermItem<'_, Q>, QueryEntityError> {
// SAFETY: system runs without conflicts with other systems.
Expand All @@ -164,7 +163,7 @@ impl<'w, 's, Q: QueryTermGroup, F: QueryTermGroup> TermQuery<'w, 's, Q, F> {
/// # See also
///
/// - [`get`](Self::get) to get a read-only query item.
/// - [`Query::get`](crate::query::Query::get) for more examples
/// - [`Query::get`](crate::prelude::Query::get) for more examples
#[inline]
pub fn get_mut(&mut self, entity: Entity) -> Result<Q::Item<'_>, QueryEntityError> {
// SAFETY: system runs without conflicts with other systems.
Expand All @@ -181,7 +180,7 @@ impl<'w, 's, Q: QueryTermGroup, F: QueryTermGroup> TermQuery<'w, 's, Q, F> {
///
/// - [`get_single`](Self::get_single) for the non-panicking version.
/// - [`single_mut`](Self::single_mut) to get the mutable query item.
/// - [`Query::single`](crate::query::Query::single) for more examples
/// - [`Query::single`](crate::prelude::Query::single) for more examples
pub fn single(&self) -> ROTermItem<'_, Q> {
self.get_single().unwrap()
}
Expand All @@ -194,7 +193,7 @@ impl<'w, 's, Q: QueryTermGroup, F: QueryTermGroup> TermQuery<'w, 's, Q, F> {
///
/// - [`get_single_mut`](Self::get_single_mut) to get the mutable query item.
/// - [`single`](Self::single) for the panicking version.
/// - [`Query::get_single`](crate::query::Query::get_single) for more examples
/// - [`Query::get_single`](crate::prelude::Query::get_single) for more examples
#[inline]
pub fn get_single(&self) -> Result<ROTermItem<'_, Q>, QuerySingleError> {
// SAFETY:
Expand All @@ -215,7 +214,7 @@ impl<'w, 's, Q: QueryTermGroup, F: QueryTermGroup> TermQuery<'w, 's, Q, F> {
///
/// - [`get_single_mut`](Self::get_single_mut) for the non-panicking version.
/// - [`single`](Self::single) to get the read-only query item.
/// - [`Query::single_mut`](crate::query::Query::single_mut) for more examples
/// - [`Query::single_mut`](crate::prelude::Query::single_mut) for more examples
pub fn single_mut(&mut self) -> Q::Item<'_> {
self.get_single_mut().unwrap()
}
Expand All @@ -228,7 +227,7 @@ impl<'w, 's, Q: QueryTermGroup, F: QueryTermGroup> TermQuery<'w, 's, Q, F> {
///
/// - [`get_single`](Self::get_single) to get the read-only query item.
/// - [`single_mut`](Self::single_mut) for the panicking version.
/// - [`Query::get_single_mut`](crate::query::Query::get_single_mut) for more examples
/// - [`Query::get_single_mut`](crate::prelude::Query::get_single_mut) for more examples
#[inline]
pub fn get_single_mut(&mut self) -> Result<Q::Item<'_>, QuerySingleError> {
// SAFETY:
Expand All @@ -246,6 +245,7 @@ impl<'w, 's, Q: QueryTermGroup, F: QueryTermGroup> IntoIterator for &'w TermQuer
type IntoIter = TermQueryIter<'w, 's, Q::ReadOnly>;

fn into_iter(self) -> Self::IntoIter {
// SAFETY: invariants guaranteed in TermQuery::new
unsafe {
TermQueryIter::new(
self.world,
Expand All @@ -264,6 +264,7 @@ impl<'w, 's, Q: QueryTermGroup, F: QueryTermGroup> IntoIterator
type IntoIter = TermQueryIter<'w, 's, Q>;

fn into_iter(self) -> Self::IntoIter {
// SAFETY: invariants guaranteed in TermQuery::new
unsafe {
TermQueryIter::new(
self.world,
Expand Down
29 changes: 22 additions & 7 deletions crates/bevy_ecs/src/term_query/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,17 @@ use super::{QueryTermGroup, Term, TermOperator, TermQueryState};
/// Builder for [`TermQuery`](crate::system::TermQuery)
///
/// ```
/// use bevy_ecs::prelude::*;
///
/// #[derive(Component)]
/// struct A(usize);
///
/// #[derive(Component)]
/// struct B(usize);
///
/// #[derive(Component)]
/// struct C(usize);
///
/// let mut world = World::new();
/// let entity_a = world.spawn((A(0), B(0))).id();
/// let entity_b = world.spawn((A(0), C(0))).id();
Expand Down Expand Up @@ -49,9 +60,10 @@ impl<'w, Q: QueryTermGroup> QueryBuilder<'w, Q> {

/// Sets `current_term` to `index`
///
/// This is primarily intended for use with [`Self::set_dynamic`] and [`Self::set_dynamic_by_id].
/// This is primarily intended for use with [`Self::set_dynamic`] and [`Self::set_dynamic_by_id`].
///
/// SAFETY: terms must not be modified such that they become incompatible with Q
/// # Safety
/// Caller must not modify terms such that they become incompatible with Q
pub unsafe fn term_at(&mut self, index: usize) -> &mut Self {
james-j-obrien marked this conversation as resolved.
Show resolved Hide resolved
self.current_term = index;
self
Expand Down Expand Up @@ -105,7 +117,7 @@ impl<'w, Q: QueryTermGroup> QueryBuilder<'w, Q> {

/// Takes a function over mutable access to a [`QueryBuilder`], calls that function
/// on an empty builder and then adds all terms from that builder to self marked as
/// [`TermAccess::Optional`]
/// [`TermOperator::Optional`]
pub fn optional(&mut self, f: impl Fn(&mut QueryBuilder)) -> &mut Self {
let mut builder = QueryBuilder::new(self.world);
f(&mut builder);
Expand Down Expand Up @@ -147,7 +159,7 @@ impl<'w, Q: QueryTermGroup> QueryBuilder<'w, Q> {

/// Set the [`ComponentId`] of the term indexed by `current_term` to the one associated with `T`
///
/// Intended to be used primarily with queries with [`Ptr`] terms
/// Intended to be used primarily with queries with [`Ptr`](bevy_ptr::Ptr) terms
pub fn set_dynamic<T: Component>(&mut self) -> &mut Self {
let id = self.world.init_component::<T>();
self.set_dynamic_by_id(id);
Expand All @@ -156,7 +168,7 @@ impl<'w, Q: QueryTermGroup> QueryBuilder<'w, Q> {

/// Set the [`ComponentId`] of the term indexed by `current_term`
///
/// Intended to be used primarily with queries with [`Ptr`] terms
/// Intended to be used primarily with queries with [`Ptr`](bevy_ptr::Ptr) terms
pub fn set_dynamic_by_id(&mut self, id: ComponentId) -> &mut Self {
self.terms[self.current_term].component = Some(id);
self
Expand All @@ -178,8 +190,9 @@ impl<'w, Q: QueryTermGroup> QueryBuilder<'w, Q> {
}

/// Attempts to re-interpret this builder as [`QueryBuilder<NewQ>`]
pub fn try_transmute<NewQ: QueryTermGroup>(mut self) -> Option<QueryBuilder<'w, NewQ>> {
pub fn try_transmute<NewQ: QueryTermGroup>(&mut self) -> Option<&mut QueryBuilder<'w, NewQ>> {
if self.interpretable_as::<NewQ>() {
// SAFETY: Just checked that NewQ is compatible with Q
Some(unsafe { std::mem::transmute(self) })
} else {
None
Expand All @@ -188,7 +201,9 @@ impl<'w, Q: QueryTermGroup> QueryBuilder<'w, Q> {

/// Re-interprets this builder as [`QueryBuilder<NewQ>`]
///
/// SAFETY: Caller must ensure that [`Self::interpretable_as::<NewQ>()`] is true
/// # Safety
///
/// Caller must ensure that [`Self::interpretable_as::<NewQ>()`] is true
pub unsafe fn transmute<NewQ: QueryTermGroup>(self) -> QueryBuilder<'w, NewQ> {
std::mem::transmute(self)
}
Expand Down
14 changes: 6 additions & 8 deletions crates/bevy_ecs/src/term_query/iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ impl<'w, 's> TermQueryCursor<'w, 's> {

let entity = archetype_entity.entity();
let row = archetype_entity.table_row();
if query_state.filter_fetch(&mut self.term_state, entity, row) {
if query_state.filter_fetch(&self.term_state, entity, row) {
return Some(query_state.fetch(
&self.term_state,
entity,
Expand Down Expand Up @@ -162,6 +162,9 @@ impl<'w, 's> Iterator for TermQueryIterUntyped<'w, 's> {

#[inline]
fn next(&mut self) -> Option<Self::Item> {
// SAFETY:
// `tables` and `archetypes` belong to the same world that the cursor was initialized for.
// `query_state` is the state that was passed to `TermQueryCursor::new`.
unsafe {
self.cursor
.next(self.tables, self.archetypes, self.query_state)
Expand Down Expand Up @@ -210,14 +213,9 @@ impl<'w, 's, Q: QueryTermGroup> Iterator for TermQueryIter<'w, 's, Q> {
// `tables` and `archetypes` belong to the same world that the cursor was initialized for.
// `query_state` is the state that was passed to `QueryIterationCursor::init`.
unsafe {
if let Some(fetches) = self
.cursor
self.cursor
.next(self.tables, self.archetypes, self.query_state)
{
Some(Q::from_fetches(&mut fetches.iter()))
} else {
None
}
.map(|fetches| Q::from_fetches(&mut fetches.iter()))
}
}
}
Loading