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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Avoid making Fetchs Clone #5593

Closed
wants to merge 3 commits into from
Closed

[Merged by Bors] - Avoid making Fetchs Clone #5593

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9110910
do it
BoxyUwU Aug 6, 2022
bad65d3
rebase
BoxyUwU Oct 25, 2022
173bcc8
Update crates/bevy_ecs/src/query/fetch.rs
BoxyUwU Oct 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 13 additions & 1 deletion crates/bevy_ecs/macros/src/fetch.rs
View file
Open in desktop
Expand Up @@ -186,7 +186,6 @@ pub fn derive_world_query_impl(ast: DeriveInput) -> TokenStream {
#(#(#ignored_field_attrs)* #ignored_field_visibilities #ignored_field_idents: #ignored_field_types,)*
}

#[derive(Clone)]
#[doc(hidden)]
#visibility struct #fetch_struct_name #user_impl_generics_with_world #user_where_clauses_with_world {
#(#field_idents: <#field_types as #path::query::WorldQueryGats<'__w>>::Fetch,)*
Expand Down Expand Up @@ -239,6 +238,19 @@ pub fn derive_world_query_impl(ast: DeriveInput) -> TokenStream {
}
}

unsafe fn clone_fetch<'__w>(
_fetch: &<Self as #path::query::WorldQueryGats<'__w>>::Fetch
) -> <Self as #path::query::WorldQueryGats<'__w>>::Fetch {
#fetch_struct_name {
#(
#field_idents: <#field_types>::clone_fetch(& _fetch. #field_idents),
)*
#(
#ignored_field_idents: Default::default(),
)*
}
}

const IS_DENSE: bool = true #(&& <#field_types>::IS_DENSE)*;

const IS_ARCHETYPAL: bool = true #(&& <#field_types>::IS_ARCHETYPAL)*;
Expand Down
150 changes: 97 additions & 53 deletions crates/bevy_ecs/src/query/fetch.rs
View file
Open in desktop
Expand Up @@ -327,6 +327,16 @@ pub unsafe trait WorldQuery: for<'w> WorldQueryGats<'w> {
change_tick: u32,
) -> <Self as WorldQueryGats<'w>>::Fetch;

/// This function is safe to call if `Self:ReadOnlyWorldQuery` holds.
BoxyUwU marked this conversation as resolved.
Show resolved Hide resolved
///
/// # Safety
/// While calling this method on its own cannot cause UB it is marked `unsafe` as the caller must ensure
/// that the returned value is not used in any way that would cause two `QueryItem<Self>` for the same
/// `archetype_index` or `table_row` to be alive at the same time.
unsafe fn clone_fetch<'w>(
Copy link
Member

Choose a reason for hiding this comment

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

I feel like we probably want a safe read-only version of this too.

I'm not sure if it's immediately useful though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it really matters, none of the call sites seem to know that Self: ReadOnlyWorldQuery holds, it'd be trivial to write a safe function later if we need it so i'm not bothered about this

fetch: &<Self as WorldQueryGats<'w>>::Fetch,
) -> <Self as WorldQueryGats<'w>>::Fetch;

/// Returns true if (and only if) every table of every archetype matched by this fetch contains
/// all of the matched components. This is used to select a more efficient "table iterator"
/// for "dense" queries. If this returns true, [`WorldQuery::set_table`] and [`WorldQuery::table_fetch`]
Expand Down Expand Up @@ -461,7 +471,6 @@ pub type ROQueryFetch<'w, Q> = QueryFetch<'w, <Q as WorldQuery>::ReadOnly>;
pub type ROQueryItem<'w, Q> = QueryItem<'w, <Q as WorldQuery>::ReadOnly>;

#[doc(hidden)]
#[derive(Clone)]
pub struct EntityFetch<'w> {
entities: Option<ThinSlicePtr<'w, Entity>>,
}
Expand All @@ -488,6 +497,14 @@ unsafe impl WorldQuery for Entity {
EntityFetch { entities: None }
}

unsafe fn clone_fetch<'w>(
fetch: &<Self as WorldQueryGats<'w>>::Fetch,
) -> <Self as WorldQueryGats<'w>>::Fetch {
EntityFetch {
entities: fetch.entities,
}
}

#[inline]
unsafe fn set_archetype<'w>(
fetch: &mut EntityFetch<'w>,
Expand Down Expand Up @@ -595,6 +612,17 @@ unsafe impl<T: Component> WorldQuery for &T {
}
}

unsafe fn clone_fetch<'w>(
fetch: &<Self as WorldQueryGats<'w>>::Fetch,
) -> <Self as WorldQueryGats<'w>>::Fetch {
ReadFetch {
table_components: fetch.table_components,
entity_table_rows: fetch.entity_table_rows,
entities: fetch.entities,
sparse_set: fetch.sparse_set,
}
}

#[inline]
unsafe fn set_archetype<'w>(
fetch: &mut ReadFetch<'w, T>,
Expand Down Expand Up @@ -692,17 +720,6 @@ unsafe impl<T: Component> WorldQuery for &T {
}
}

impl<T> Clone for ReadFetch<'_, T> {
fn clone(&self) -> Self {
Self {
table_components: self.table_components,
entity_table_rows: self.entity_table_rows,
entities: self.entities,
sparse_set: self.sparse_set,
}
}
}

/// SAFETY: access is read only
unsafe impl<T: Component> ReadOnlyWorldQuery for &T {}

Expand Down Expand Up @@ -761,6 +778,20 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T {
}
}

unsafe fn clone_fetch<'w>(
fetch: &<Self as WorldQueryGats<'w>>::Fetch,
) -> <Self as WorldQueryGats<'w>>::Fetch {
WriteFetch {
table_components: fetch.table_components,
table_ticks: fetch.table_ticks,
entities: fetch.entities,
entity_table_rows: fetch.entity_table_rows,
sparse_set: fetch.sparse_set,
last_change_tick: fetch.last_change_tick,
change_tick: fetch.change_tick,
}
}

#[inline]
unsafe fn set_archetype<'w>(
fetch: &mut WriteFetch<'w, T>,
Expand Down Expand Up @@ -887,20 +918,6 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T {
}
}

impl<T> Clone for WriteFetch<'_, T> {
fn clone(&self) -> Self {
Self {
table_components: self.table_components,
table_ticks: self.table_ticks,
entities: self.entities,
entity_table_rows: self.entity_table_rows,
sparse_set: self.sparse_set,
last_change_tick: self.last_change_tick,
change_tick: self.change_tick,
}
}
}

impl<'w, T: Component> WorldQueryGats<'w> for &mut T {
type Fetch = WriteFetch<'w, T>;
type Item = Mut<'w, T>;
Expand All @@ -911,17 +928,6 @@ pub struct OptionFetch<'w, T: WorldQuery> {
fetch: <T as WorldQueryGats<'w>>::Fetch,
matches: bool,
}
impl<'w, T: WorldQuery> Clone for OptionFetch<'w, T>
where
<T as WorldQueryGats<'w>>::Fetch: Clone,
{
fn clone(&self) -> Self {
Self {
fetch: self.fetch.clone(),
matches: self.matches,
}
}
}

// SAFETY: defers to soundness of `T: WorldQuery` impl
unsafe impl<T: WorldQuery> WorldQuery for Option<T> {
Expand All @@ -948,6 +954,15 @@ unsafe impl<T: WorldQuery> WorldQuery for Option<T> {
}
}

unsafe fn clone_fetch<'w>(
fetch: &<Self as WorldQueryGats<'w>>::Fetch,
) -> <Self as WorldQueryGats<'w>>::Fetch {
OptionFetch {
fetch: T::clone_fetch(&fetch.fetch),
matches: fetch.matches,
}
}

#[inline]
unsafe fn set_archetype<'w>(
fetch: &mut OptionFetch<'w, T>,
Expand Down Expand Up @@ -1065,14 +1080,25 @@ impl<'w, T: WorldQuery> WorldQueryGats<'w> for Option<T> {
/// }
/// # bevy_ecs::system::assert_is_system(print_moving_objects_system);
/// ```
#[derive(Clone)]
pub struct ChangeTrackers<T: Component> {
pub(crate) component_ticks: ComponentTicks,
pub(crate) last_change_tick: u32,
pub(crate) change_tick: u32,
marker: PhantomData<T>,
}

impl<T: Component> Clone for ChangeTrackers<T> {
fn clone(&self) -> Self {
Self {
component_ticks: self.component_ticks,
last_change_tick: self.last_change_tick,
change_tick: self.change_tick,
marker: PhantomData,
}
}
}
impl<T: Component> Copy for ChangeTrackers<T> {}

impl<T: Component> std::fmt::Debug for ChangeTrackers<T> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_struct("ChangeTrackers")
Expand Down Expand Up @@ -1111,20 +1137,6 @@ pub struct ChangeTrackersFetch<'w, T> {
change_tick: u32,
}

impl<T> Clone for ChangeTrackersFetch<'_, T> {
fn clone(&self) -> Self {
Self {
table_ticks: self.table_ticks,
entity_table_rows: self.entity_table_rows,
entities: self.entities,
sparse_set: self.sparse_set,
marker: self.marker,
last_change_tick: self.last_change_tick,
change_tick: self.change_tick,
}
}
}

// SAFETY: `ROQueryFetch<Self>` is the same as `QueryFetch<Self>`
unsafe impl<T: Component> WorldQuery for ChangeTrackers<T> {
type ReadOnly = Self;
Expand Down Expand Up @@ -1161,6 +1173,20 @@ unsafe impl<T: Component> WorldQuery for ChangeTrackers<T> {
}
}

unsafe fn clone_fetch<'w>(
fetch: &<Self as WorldQueryGats<'w>>::Fetch,
) -> <Self as WorldQueryGats<'w>>::Fetch {
ChangeTrackersFetch {
table_ticks: fetch.table_ticks,
entity_table_rows: fetch.entity_table_rows,
entities: fetch.entities,
sparse_set: fetch.sparse_set,
marker: fetch.marker,
last_change_tick: fetch.last_change_tick,
change_tick: fetch.change_tick,
}
}

#[inline]
unsafe fn set_archetype<'w>(
fetch: &mut ChangeTrackersFetch<'w, T>,
Expand Down Expand Up @@ -1317,6 +1343,13 @@ macro_rules! impl_tuple_fetch {
($($name::init_fetch(_world, $name, _last_change_tick, _change_tick),)*)
}

unsafe fn clone_fetch<'w>(
fetch: &<Self as WorldQueryGats<'w>>::Fetch,
) -> <Self as WorldQueryGats<'w>>::Fetch {
let ($($name,)*) = &fetch;
($($name::clone_fetch($name),)*)
}

const IS_DENSE: bool = true $(&& $name::IS_DENSE)*;

const IS_ARCHETYPAL: bool = true $(&& $name::IS_ARCHETYPAL)*;
Expand Down Expand Up @@ -1395,7 +1428,6 @@ macro_rules! impl_tuple_fetch {
/// `Query<AnyOf<(&A, &B, &mut C)>>` is equivalent to `Query<(Option<&A>, Option<&B>, Option<&mut C>), Or<(With<A>, With<B>, With<C>)>>`.
/// Each of the components in `T` is returned as an `Option`, as with `Option<A>` queries.
/// Entities are guaranteed to have at least one of the components in `T`.
#[derive(Clone)]
pub struct AnyOf<T>(PhantomData<T>);

macro_rules! impl_anytuple_fetch {
Expand Down Expand Up @@ -1427,6 +1459,13 @@ macro_rules! impl_anytuple_fetch {
($(($name::init_fetch(_world, $name, _last_change_tick, _change_tick), false),)*)
}

unsafe fn clone_fetch<'w>(
fetch: &<Self as WorldQueryGats<'w>>::Fetch,
) -> <Self as WorldQueryGats<'w>>::Fetch {
let ($($name,)*) = &fetch;
($(($name::clone_fetch(& $name.0), $name.1),)*)
}

const IS_DENSE: bool = true $(&& $name::IS_DENSE)*;

const IS_ARCHETYPAL: bool = true $(&& $name::IS_ARCHETYPAL)*;
Expand Down Expand Up @@ -1559,6 +1598,11 @@ unsafe impl<Q: WorldQuery> WorldQuery for NopWorldQuery<Q> {
) {
}

unsafe fn clone_fetch<'w>(
_fetch: &<Self as WorldQueryGats<'w>>::Fetch,
) -> <Self as WorldQueryGats<'w>>::Fetch {
}

#[inline(always)]
unsafe fn set_archetype(
_fetch: &mut (),
Expand Down