Skip to content

Commit

Permalink
remove QF generics from all Query/State methods and types (bevyengi…
Browse files Browse the repository at this point in the history
…ne#5170)

# Objective

remove `QF` generics from a bunch of types and methods on query related items. this has a few benefits:
- simplifies type signatures `fn iter(&self) -> QueryIter<'_, 's, Q::ReadOnly, F::ReadOnly>` is (imo) conceptually simpler than `fn iter(&self) -> QueryIter<'_, 's, Q, ROQueryFetch<'_, Q>, F>`
- `Fetch` is mostly an implementation detail but previously we had to expose it on every `iter` `get` etc method
- Allows us to potentially in the future simplify the `WorldQuery` trait hierarchy by removing the `Fetch` trait

## Solution

remove the `QF` generic and add a way to (unsafely) turn `&QueryState<Q1, F1>` into `&QueryState<Q2, F2>`

---

## Changelog/Migration Guide

The `QF` generic was removed from various `Query` iterator types and some methods, you should update your code to use the type of the corresponding worldquery of the fetch type that was being used, or call `as_readonly`/`as_nop` to convert a querystate to the appropriate type. For example:
`.get_single_unchecked_manual::<ROQueryFetch<Q>>(..)` -> `.as_readonly().get_single_unchecked_manual(..)`
`my_field: QueryIter<'w, 's, Q, ROQueryFetch<'w, Q>, F>` -> `my_field: QueryIter<'w, 's, Q::ReadOnly, F::ReadOnly>`
  • Loading branch information
BoxyUwU authored and inodentry committed Aug 8, 2022
1 parent d7b62e1 commit 5555885
Show file tree
Hide file tree
Showing 8 changed files with 273 additions and 206 deletions.
30 changes: 25 additions & 5 deletions crates/bevy_ecs/src/query/fetch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,8 @@ use std::{cell::UnsafeCell, marker::PhantomData};
/// ```
/// # Safety
///
/// component access of `ROQueryFetch<Self>` should be a subset of `QueryFetch<Self>`
/// component access of `ROQueryFetch<Self>` must be a subset of `QueryFetch<Self>`
/// and `ROQueryFetch<Self>` must match exactly the same archetypes/tables as `QueryFetch<Self>`
pub unsafe trait WorldQuery: for<'w> WorldQueryGats<'w, _State = Self::State> {
type ReadOnly: ReadOnlyWorldQuery<State = Self::State>;
type State: FetchState;
Expand Down Expand Up @@ -1608,6 +1609,25 @@ macro_rules! impl_anytuple_fetch {
all_tuples!(impl_tuple_fetch, 0, 15, F, S);
all_tuples!(impl_anytuple_fetch, 0, 15, F, S);

/// [`WorldQuery`] used to nullify queries by turning `Query<Q>` into `Query<NopWorldQuery<Q>>`
///
/// This will rarely be useful to consumers of `bevy_ecs`.
pub struct NopWorldQuery<Q: WorldQuery>(PhantomData<Q>);

/// SAFETY: `Self::ReadOnly` is `Self`
unsafe impl<Q: WorldQuery> WorldQuery for NopWorldQuery<Q> {
type ReadOnly = Self;
type State = Q::State;

fn shrink<'wlong: 'wshort, 'wshort>(_: ()) {}
}
impl<'a, Q: WorldQuery> WorldQueryGats<'a> for NopWorldQuery<Q> {
type Fetch = NopFetch<QueryFetch<'a, Q>>;
type _State = <Q as WorldQueryGats<'a>>::_State;
}
/// SAFETY: `NopFetch` never accesses any data
unsafe impl<Q: WorldQuery> ReadOnlyWorldQuery for NopWorldQuery<Q> {}

/// [`Fetch`] that does not actually fetch anything
///
/// Mostly useful when something is generic over the Fetch and you don't want to fetch as you will discard the result
Expand All @@ -1616,18 +1636,18 @@ pub struct NopFetch<State> {
}

// SAFETY: NopFetch doesnt access anything
unsafe impl<'w, State: FetchState> Fetch<'w> for NopFetch<State> {
unsafe impl<'w, F: Fetch<'w>> Fetch<'w> for NopFetch<F> {
type Item = ();
type State = State;
type State = F::State;

const IS_DENSE: bool = true;
const IS_DENSE: bool = F::IS_DENSE;

const IS_ARCHETYPAL: bool = true;

#[inline(always)]
unsafe fn init(
_world: &'w World,
_state: &State,
_state: &F::State,
_last_change_tick: u32,
_change_tick: u32,
) -> Self {
Expand Down
86 changes: 32 additions & 54 deletions crates/bevy_ecs/src/query/iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,14 @@ use super::{QueryFetch, QueryItem, ReadOnlyWorldQuery};
///
/// This struct is created by the [`Query::iter`](crate::system::Query::iter) and
/// [`Query::iter_mut`](crate::system::Query::iter_mut) methods.
pub struct QueryIter<'w, 's, Q: WorldQuery, QF: Fetch<'w, State = Q::State>, F: WorldQuery> {
pub struct QueryIter<'w, 's, Q: WorldQuery, F: WorldQuery> {
tables: &'w Tables,
archetypes: &'w Archetypes,
query_state: &'s QueryState<Q, F>,
cursor: QueryIterationCursor<'w, 's, Q, QF, F>,
cursor: QueryIterationCursor<'w, 's, Q, F>,
}

impl<'w, 's, Q: WorldQuery, QF, F: WorldQuery> QueryIter<'w, 's, Q, QF, F>
where
QF: Fetch<'w, State = Q::State>,
{
impl<'w, 's, Q: WorldQuery, F: WorldQuery> QueryIter<'w, 's, Q, F> {
/// # Safety
/// This does not check for mutable query correctness. To be safe, make sure mutable queries
/// have unique access to the components they query.
Expand All @@ -44,11 +41,8 @@ where
}
}

impl<'w, 's, Q: WorldQuery, QF, F: WorldQuery> Iterator for QueryIter<'w, 's, Q, QF, F>
where
QF: Fetch<'w, State = Q::State>,
{
type Item = QF::Item;
impl<'w, 's, Q: WorldQuery, F: WorldQuery> Iterator for QueryIter<'w, 's, Q, F> {
type Item = QueryItem<'w, Q>;

#[inline(always)]
fn next(&mut self) -> Option<Self::Item> {
Expand All @@ -69,42 +63,32 @@ where
.map(|id| self.archetypes[*id].len())
.sum();

let archetype_query = F::Fetch::IS_ARCHETYPAL && QF::IS_ARCHETYPAL;
let archetype_query = Q::Fetch::IS_ARCHETYPAL && F::Fetch::IS_ARCHETYPAL;
let min_size = if archetype_query { max_size } else { 0 };
(min_size, Some(max_size))
}
}

// This is correct as [`QueryIter`] always returns `None` once exhausted.
impl<'w, 's, Q: WorldQuery, QF, F: WorldQuery> FusedIterator for QueryIter<'w, 's, Q, QF, F> where
QF: Fetch<'w, State = Q::State>
{
}
impl<'w, 's, Q: WorldQuery, F: WorldQuery> FusedIterator for QueryIter<'w, 's, Q, F> {}

/// An [`Iterator`] over query results of a [`Query`](crate::system::Query).
///
/// This struct is created by the [`Query::iter_many`](crate::system::Query::iter_many) method.
pub struct QueryManyIter<
'w,
's,
Q: WorldQuery,
QF: Fetch<'w, State = Q::State>,
F: WorldQuery,
I: Iterator,
> where
pub struct QueryManyIter<'w, 's, Q: WorldQuery, F: WorldQuery, I: Iterator>
where
I::Item: Borrow<Entity>,
{
entity_iter: I,
entities: &'w Entities,
tables: &'w Tables,
archetypes: &'w Archetypes,
fetch: QF,
fetch: QueryFetch<'w, Q>,
filter: QueryFetch<'w, F>,
query_state: &'s QueryState<Q, F>,
}

impl<'w, 's, Q: WorldQuery, QF: Fetch<'w, State = Q::State>, F: WorldQuery, I: Iterator>
QueryManyIter<'w, 's, Q, QF, F, I>
impl<'w, 's, Q: WorldQuery, F: WorldQuery, I: Iterator> QueryManyIter<'w, 's, Q, F, I>
where
I::Item: Borrow<Entity>,
{
Expand All @@ -119,14 +103,14 @@ where
entity_list: EntityList,
last_change_tick: u32,
change_tick: u32,
) -> QueryManyIter<'w, 's, Q, QF, F, I> {
let fetch = QF::init(
) -> QueryManyIter<'w, 's, Q, F, I> {
let fetch = Q::Fetch::init(
world,
&query_state.fetch_state,
last_change_tick,
change_tick,
);
let filter = QueryFetch::<F>::init(
let filter = F::Fetch::init(
world,
&query_state.filter_state,
last_change_tick,
Expand All @@ -144,12 +128,11 @@ where
}
}

impl<'w, 's, Q: WorldQuery, QF: Fetch<'w, State = Q::State>, F: WorldQuery, I: Iterator> Iterator
for QueryManyIter<'w, 's, Q, QF, F, I>
impl<'w, 's, Q: WorldQuery, F: WorldQuery, I: Iterator> Iterator for QueryManyIter<'w, 's, Q, F, I>
where
I::Item: Borrow<Entity>,
{
type Item = QF::Item;
type Item = QueryItem<'w, Q>;

#[inline(always)]
fn next(&mut self) -> Option<Self::Item> {
Expand Down Expand Up @@ -201,7 +184,7 @@ pub struct QueryCombinationIter<'w, 's, Q: WorldQuery, F: WorldQuery, const K: u
tables: &'w Tables,
archetypes: &'w Archetypes,
query_state: &'s QueryState<Q, F>,
cursors: [QueryIterationCursor<'w, 's, Q, QueryFetch<'w, Q>, F>; K],
cursors: [QueryIterationCursor<'w, 's, Q, F>; K],
}

impl<'w, 's, Q: WorldQuery, F: WorldQuery, const K: usize> QueryCombinationIter<'w, 's, Q, F, K> {
Expand All @@ -219,11 +202,10 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery, const K: usize> QueryCombinationIter<
// Initialize array with cursors.
// There is no FromIterator on arrays, so instead initialize it manually with MaybeUninit

let mut array: MaybeUninit<[QueryIterationCursor<'w, 's, Q, QueryFetch<'w, Q>, F>; K]> =
MaybeUninit::uninit();
let mut array: MaybeUninit<[QueryIterationCursor<'w, 's, Q, F>; K]> = MaybeUninit::uninit();
let ptr = array
.as_mut_ptr()
.cast::<QueryIterationCursor<'w, 's, Q, QueryFetch<'w, Q>, F>>();
.cast::<QueryIterationCursor<'w, 's, Q, F>>();
if K != 0 {
ptr.write(QueryIterationCursor::init(
world,
Expand Down Expand Up @@ -367,10 +349,9 @@ where
}
}

impl<'w, 's, Q: WorldQuery, QF, F> ExactSizeIterator for QueryIter<'w, 's, Q, QF, F>
impl<'w, 's, Q: WorldQuery, F: WorldQuery> ExactSizeIterator for QueryIter<'w, 's, Q, F>
where
QF: Fetch<'w, State = Q::State>,
F: WorldQuery + ArchetypeFilter,
F: ArchetypeFilter,
{
fn len(&self) -> usize {
self.query_state
Expand Down Expand Up @@ -405,21 +386,21 @@ where
{
}

struct QueryIterationCursor<'w, 's, Q: WorldQuery, QF: Fetch<'w, State = Q::State>, F: WorldQuery> {
struct QueryIterationCursor<'w, 's, Q: WorldQuery, F: WorldQuery> {
table_id_iter: std::slice::Iter<'s, TableId>,
archetype_id_iter: std::slice::Iter<'s, ArchetypeId>,
fetch: QF,
fetch: QueryFetch<'w, Q>,
filter: QueryFetch<'w, F>,
// length of the table table or length of the archetype, depending on whether both `Q`'s and `F`'s fetches are dense
current_len: usize,
// either table row or archetype index, depending on whether both `Q`'s and `F`'s fetches are dense
current_index: usize,
phantom: PhantomData<(&'w (), Q)>,
phantom: PhantomData<Q>,
}

impl<'w, 's, Q: WorldQuery, QF, F: WorldQuery> Clone for QueryIterationCursor<'w, 's, Q, QF, F>
impl<'w, 's, Q: WorldQuery, F: WorldQuery> Clone for QueryIterationCursor<'w, 's, Q, F>
where
QF: Fetch<'w, State = Q::State> + Clone,
QueryFetch<'w, Q>: Clone,
QueryFetch<'w, F>: Clone,
{
fn clone(&self) -> Self {
Expand All @@ -435,11 +416,8 @@ where
}
}

impl<'w, 's, Q: WorldQuery, QF, F: WorldQuery> QueryIterationCursor<'w, 's, Q, QF, F>
where
QF: Fetch<'w, State = Q::State>,
{
const IS_DENSE: bool = QF::IS_DENSE && <QueryFetch<'static, F>>::IS_DENSE;
impl<'w, 's, Q: WorldQuery, F: WorldQuery> QueryIterationCursor<'w, 's, Q, F> {
const IS_DENSE: bool = Q::Fetch::IS_DENSE && F::Fetch::IS_DENSE;

unsafe fn init_empty(
world: &'w World,
Expand All @@ -460,13 +438,13 @@ where
last_change_tick: u32,
change_tick: u32,
) -> Self {
let fetch = QF::init(
let fetch = Q::Fetch::init(
world,
&query_state.fetch_state,
last_change_tick,
change_tick,
);
let filter = QueryFetch::<F>::init(
let filter = F::Fetch::init(
world,
&query_state.filter_state,
last_change_tick,
Expand All @@ -485,7 +463,7 @@ where

/// retrieve item returned from most recent `next` call again.
#[inline]
unsafe fn peek_last(&mut self) -> Option<QF::Item> {
unsafe fn peek_last(&mut self) -> Option<QueryItem<'w, Q>> {
if self.current_index > 0 {
if Self::IS_DENSE {
Some(self.fetch.table_fetch(self.current_index - 1))
Expand All @@ -509,7 +487,7 @@ where
tables: &'w Tables,
archetypes: &'w Archetypes,
query_state: &'s QueryState<Q, F>,
) -> Option<QF::Item> {
) -> Option<QueryItem<'w, Q>> {
if Self::IS_DENSE {
loop {
// we are on the beginning of the query, or finished processing a table, so skip to the next
Expand Down
60 changes: 50 additions & 10 deletions crates/bevy_ecs/src/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ pub(crate) unsafe fn debug_checked_unreachable() -> ! {
mod tests {
use super::WorldQuery;
use crate::prelude::{AnyOf, Entity, Or, QueryState, With, Without};
use crate::query::{ArchetypeFilter, QueryCombinationIter, QueryFetch, ReadOnlyWorldQuery};
use crate::system::{IntoSystem, Query, System};
use crate::query::{ArchetypeFilter, QueryCombinationIter, QueryFetch};
use crate::system::{IntoSystem, Query, System, SystemState};
use crate::{self as bevy_ecs, component::Component, world::World};
use std::any::type_name;
use std::collections::HashSet;
Expand Down Expand Up @@ -67,10 +67,11 @@ mod tests {
}
fn assert_combination<Q, F, const K: usize>(world: &mut World, expected_size: usize)
where
Q: ReadOnlyWorldQuery,
F: ReadOnlyWorldQuery + ArchetypeFilter,
for<'w> QueryFetch<'w, Q>: Clone,
for<'w> QueryFetch<'w, F>: Clone,
Q: WorldQuery,
F: WorldQuery,
F::ReadOnly: ArchetypeFilter,
for<'w> QueryFetch<'w, Q::ReadOnly>: Clone,
for<'w> QueryFetch<'w, F::ReadOnly>: Clone,
{
let mut query = world.query_filtered::<Q, F>();
let iter = query.iter_combinations::<K>(world);
Expand All @@ -79,10 +80,11 @@ mod tests {
}
fn assert_all_sizes_equal<Q, F>(world: &mut World, expected_size: usize)
where
Q: ReadOnlyWorldQuery,
F: ReadOnlyWorldQuery + ArchetypeFilter,
for<'w> QueryFetch<'w, Q>: Clone,
for<'w> QueryFetch<'w, F>: Clone,
Q: WorldQuery,
F: WorldQuery,
F::ReadOnly: ArchetypeFilter,
for<'w> QueryFetch<'w, Q::ReadOnly>: Clone,
for<'w> QueryFetch<'w, F::ReadOnly>: Clone,
{
let mut query = world.query_filtered::<Q, F>();
let iter = query.iter(world);
Expand Down Expand Up @@ -653,4 +655,42 @@ count(): {count}"#
system.run((), &mut world);
}
}

#[test]
fn mut_to_immut_query_methods_have_immut_item() {
#[derive(Component)]
struct Foo;

let mut world = World::new();
let e = world.spawn().insert(Foo).id();

// state
let mut q = world.query::<&mut Foo>();
let _: Option<&Foo> = q.iter(&world).next();
let _: Option<[&Foo; 2]> = q.iter_combinations::<2>(&world).next();
let _: Option<&Foo> = q.iter_manual(&world).next();
let _: Option<&Foo> = q.iter_many(&world, [e]).next();
q.for_each(&world, |_: &Foo| ());

let _: Option<&Foo> = q.get(&world, e).ok();
let _: Option<&Foo> = q.get_manual(&world, e).ok();
let _: Option<[&Foo; 1]> = q.get_many(&world, [e]).ok();
let _: Option<&Foo> = q.get_single(&world).ok();
let _: &Foo = q.single(&world);

// system param
let mut q = SystemState::<Query<&mut Foo>>::new(&mut world);
let q = q.get_mut(&mut world);
let _: Option<&Foo> = q.iter().next();
let _: Option<[&Foo; 2]> = q.iter_combinations::<2>().next();
let _: Option<&Foo> = q.iter_many([e]).next();
q.for_each(|_: &Foo| ());

let _: Option<&Foo> = q.get(e).ok();
let _: Option<&Foo> = q.get_component(e).ok();
let _: Option<[&Foo; 1]> = q.get_many([e]).ok();
let _: Option<&Foo> = q.get_single().ok();
let _: [&Foo; 1] = q.many([e]);
let _: &Foo = q.single();
}
}
Loading

0 comments on commit 5555885

Please sign in to comment.