Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
16 changes: 16 additions & 0 deletions crates/bevy_ecs/macros/src/query_data.rs
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,14 @@ pub fn derive_query_data_impl(input: TokenStream) -> TokenStream {
}
}

fn provide_extra_access(
state: &mut Self::State,
access: &mut #path::query::Access<#path::component::ComponentId>,
available_access: &#path::query::Access<#path::component::ComponentId>,
) {
#(<#field_types>::provide_extra_access(&mut state.#named_field_idents, access, available_access);)*
}

/// SAFETY: we call `fetch` for each member that implements `Fetch`.
#[inline(always)]
unsafe fn fetch<'__w>(
Expand Down Expand Up @@ -305,6 +313,14 @@ pub fn derive_query_data_impl(input: TokenStream) -> TokenStream {
}
}

fn provide_extra_access(
state: &mut Self::State,
access: &mut #path::query::Access<#path::component::ComponentId>,
available_access: &#path::query::Access<#path::component::ComponentId>,
) {
#(<#field_types>::provide_extra_access(&mut state.#named_field_idents, access, available_access);)*
}

/// SAFETY: we call `fetch` for each member that implements `Fetch`.
#[inline(always)]
unsafe fn fetch<'__w>(
Expand Down
136 changes: 75 additions & 61 deletions crates/bevy_ecs/src/query/access.rs
Original file line number Diff line number Diff line change
Expand Up @@ -430,77 +430,57 @@ impl<T: SparseSetIndex> Access<T> {

/// Adds all access from `other`.
pub fn extend(&mut self, other: &Access<T>) {
let component_read_and_writes_inverted =
self.component_read_and_writes_inverted || other.component_read_and_writes_inverted;
let component_writes_inverted =
self.component_writes_inverted || other.component_writes_inverted;

match (
self.component_read_and_writes_inverted,
invertible_union_with(
&mut self.component_read_and_writes,
&mut self.component_read_and_writes_inverted,
&other.component_read_and_writes,
other.component_read_and_writes_inverted,
) {
(true, true) => {
self.component_read_and_writes
.intersect_with(&other.component_read_and_writes);
}
(true, false) => {
self.component_read_and_writes
.difference_with(&other.component_read_and_writes);
}
(false, true) => {
// We have to grow here because the new bits are going to get flipped to 1.
self.component_read_and_writes.grow(
self.component_read_and_writes
.len()
.max(other.component_read_and_writes.len()),
);
self.component_read_and_writes.toggle_range(..);
self.component_read_and_writes
.intersect_with(&other.component_read_and_writes);
}
(false, false) => {
self.component_read_and_writes
.union_with(&other.component_read_and_writes);
}
}

match (
self.component_writes_inverted,
);
invertible_union_with(
&mut self.component_writes,
&mut self.component_writes_inverted,
&other.component_writes,
other.component_writes_inverted,
) {
(true, true) => {
self.component_writes
.intersect_with(&other.component_writes);
}
(true, false) => {
self.component_writes
.difference_with(&other.component_writes);
}
(false, true) => {
// We have to grow here because the new bits are going to get flipped to 1.
self.component_writes.grow(
self.component_writes
.len()
.max(other.component_writes.len()),
);
self.component_writes.toggle_range(..);
self.component_writes
.intersect_with(&other.component_writes);
}
(false, false) => {
self.component_writes.union_with(&other.component_writes);
}
}
);

self.reads_all_resources = self.reads_all_resources || other.reads_all_resources;
self.writes_all_resources = self.writes_all_resources || other.writes_all_resources;
self.component_read_and_writes_inverted = component_read_and_writes_inverted;
self.component_writes_inverted = component_writes_inverted;
self.resource_read_and_writes
.union_with(&other.resource_read_and_writes);
self.resource_writes.union_with(&other.resource_writes);
}

/// Removes any access from `self` that would conflict with `other`.
/// This removes any reads and writes for any component written by `other`,
/// and removes any writes for any component read by `other`.
pub fn remove_conflicting_access(&mut self, other: &Access<T>) {
invertible_difference_with(
&mut self.component_read_and_writes,
&mut self.component_read_and_writes_inverted,
&other.component_writes,
other.component_writes_inverted,
);
invertible_difference_with(
&mut self.component_writes,
&mut self.component_writes_inverted,
&other.component_read_and_writes,
other.component_read_and_writes_inverted,
);

if other.reads_all_resources {
self.writes_all_resources = false;
self.resource_writes.clear();
}
if other.writes_all_resources {
self.reads_all_resources = false;
self.resource_read_and_writes.clear();
}
self.resource_read_and_writes
.difference_with(&other.resource_writes);
self.resource_writes
.difference_with(&other.resource_read_and_writes);
}

/// Returns `true` if the access and `other` can be active at the same time,
/// only looking at their component access.
///
Expand Down Expand Up @@ -838,6 +818,40 @@ impl<T: SparseSetIndex> Access<T> {
}
}

/// Performs an in-place union of `other` into `self`, where either set may be inverted.
fn invertible_union_with(
self_set: &mut FixedBitSet,
self_inverted: &mut bool,
other_set: &FixedBitSet,
other_inverted: bool,
) {
match (*self_inverted, other_inverted) {
(true, true) => self_set.intersect_with(other_set),
(true, false) => self_set.difference_with(other_set),
(false, true) => {
*self_inverted = true;
// We have to grow here because the new bits are going to get flipped to 1.
self_set.grow(other_set.len());
self_set.toggle_range(..);
self_set.intersect_with(other_set);
}
(false, false) => self_set.union_with(other_set),
}
}

/// Performs an in-place set difference of `other` from `self`, where either set may be inverted.
fn invertible_difference_with(
self_set: &mut FixedBitSet,
self_inverted: &mut bool,
other_set: &FixedBitSet,
other_inverted: bool,
) {
// A - B = A & !B = !(!A | B)
*self_inverted = !*self_inverted;
invertible_union_with(self_set, self_inverted, other_set, other_inverted);
*self_inverted = !*self_inverted;
}

/// Error returned when attempting to iterate over items included in an [`Access`]
/// if the access excludes items rather than including them.
#[derive(Clone, Copy, PartialEq, Eq, Debug, Error)]
Expand Down
92 changes: 88 additions & 4 deletions crates/bevy_ecs/src/query/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -248,11 +248,9 @@ impl<'w, D: QueryData, F: QueryFilter> QueryBuilder<'w, D, F> {
pub fn transmute_filtered<NewD: QueryData, NewF: QueryFilter>(
&mut self,
) -> &mut QueryBuilder<'w, NewD, NewF> {
let mut fetch_state = NewD::init_state(self.world);
let fetch_state = NewD::init_state(self.world);
let filter_state = NewF::init_state(self.world);

NewD::set_access(&mut fetch_state, &self.access);

let mut access = FilteredAccess::default();
NewD::update_component_access(&fetch_state, &mut access);
NewF::update_component_access(&filter_state, &mut access);
Expand All @@ -275,7 +273,10 @@ impl<'w, D: QueryData, F: QueryFilter> QueryBuilder<'w, D, F> {

#[cfg(test)]
mod tests {
use crate::{prelude::*, world::FilteredEntityRef};
use crate::{
prelude::*,
world::{EntityMutExcept, EntityRefExcept, FilteredEntityMut, FilteredEntityRef},
};
use std::dbg;

#[derive(Component, PartialEq, Debug)]
Expand Down Expand Up @@ -422,6 +423,89 @@ mod tests {
}
}

#[test]
fn builder_provide_access() {
Copy link
Contributor

@cBournhonesque cBournhonesque May 4, 2025

Choose a reason for hiding this comment

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

great tests!

let mut world = World::new();
world.spawn((A(0), B(1)));

let mut query =
QueryBuilder::<(Entity, FilteredEntityRef, FilteredEntityMut)>::new(&mut world)
.data::<&mut A>()
.data::<&B>()
.build();

// The `FilteredEntityRef` only has read access, so the `FilteredEntityMut` can have read access without conflicts
Copy link
Contributor

Choose a reason for hiding this comment

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

The access is provided from left to right, so if the tuple had been (FilteredEntityMut, FilteredEntityRef), the Ref wouldn't have read access to the mutable component?

It is not super obvious that the order matters, maybe this should be documented slightly more in the FilteredEntityRef/Mut docstrings?

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 access is provided from left to right, so if the tuple had been (FilteredEntityMut, FilteredEntityRef), the Ref wouldn't have read access to the mutable component?

Yes, that's right!

It is not super obvious that the order matters, maybe this should be documented slightly more in the FilteredEntityRef/Mut docstrings?

I wanted to ensure that it was sound to use more than one FilteredEntity thing in a query, but it's not really useful, and I don't expect anyone to actually want to do it. So I'd be inclined to leave the exact semantics out of the documentation so as not to confuse normal users with the details of unimportant edge cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's reasonable

let (_entity, entity_ref_1, mut entity_ref_2) = query.single_mut(&mut world).unwrap();
assert!(entity_ref_1.get::<A>().is_some());
assert!(entity_ref_1.get::<B>().is_some());
assert!(entity_ref_2.get::<A>().is_some());
assert!(entity_ref_2.get_mut::<A>().is_none());
assert!(entity_ref_2.get::<B>().is_some());
assert!(entity_ref_2.get_mut::<B>().is_none());

let mut query =
QueryBuilder::<(Entity, FilteredEntityMut, FilteredEntityMut)>::new(&mut world)
.data::<&mut A>()
.data::<&B>()
.build();

// The first `FilteredEntityMut` has write access to A, so the second one cannot have write access
let (_entity, mut entity_ref_1, mut entity_ref_2) = query.single_mut(&mut world).unwrap();
assert!(entity_ref_1.get::<A>().is_some());
assert!(entity_ref_1.get_mut::<A>().is_some());
assert!(entity_ref_1.get::<B>().is_some());
assert!(entity_ref_1.get_mut::<B>().is_none());
assert!(entity_ref_2.get::<A>().is_none());
assert!(entity_ref_2.get_mut::<A>().is_none());
assert!(entity_ref_2.get::<B>().is_some());
assert!(entity_ref_2.get_mut::<B>().is_none());

let mut query = QueryBuilder::<(FilteredEntityMut, &mut A, &B)>::new(&mut world)
.data::<&mut A>()
.data::<&mut B>()
.build();

// Any `A` access would conflict with `&mut A`, and write access to `B` would conflict with `&B`.
let (mut entity_ref, _a, _b) = query.single_mut(&mut world).unwrap();
assert!(entity_ref.get::<A>().is_none());
assert!(entity_ref.get_mut::<A>().is_none());
assert!(entity_ref.get::<B>().is_some());
assert!(entity_ref.get_mut::<B>().is_none());

let mut query = QueryBuilder::<(FilteredEntityMut, &mut A, &B)>::new(&mut world)
.data::<EntityMut>()
.build();

// Same as above, but starting from "all" access
let (mut entity_ref, _a, _b) = query.single_mut(&mut world).unwrap();
assert!(entity_ref.get::<A>().is_none());
assert!(entity_ref.get_mut::<A>().is_none());
assert!(entity_ref.get::<B>().is_some());
assert!(entity_ref.get_mut::<B>().is_none());

let mut query = QueryBuilder::<(FilteredEntityMut, EntityMutExcept<A>)>::new(&mut world)
.data::<EntityMut>()
.build();

// Removing `EntityMutExcept<A>` just leaves A
let (mut entity_ref_1, _entity_ref_2) = query.single_mut(&mut world).unwrap();
assert!(entity_ref_1.get::<A>().is_some());
assert!(entity_ref_1.get_mut::<A>().is_some());
assert!(entity_ref_1.get::<B>().is_none());
assert!(entity_ref_1.get_mut::<B>().is_none());

let mut query = QueryBuilder::<(FilteredEntityMut, EntityRefExcept<A>)>::new(&mut world)
.data::<EntityMut>()
.build();

// Removing `EntityRefExcept<A>` just leaves A, plus read access
let (mut entity_ref_1, _entity_ref_2) = query.single_mut(&mut world).unwrap();
assert!(entity_ref_1.get::<A>().is_some());
assert!(entity_ref_1.get_mut::<A>().is_some());
assert!(entity_ref_1.get::<B>().is_some());
assert!(entity_ref_1.get_mut::<B>().is_none());
}

/// Regression test for issue #14348
#[test]
fn builder_static_dense_dynamic_sparse() {
Expand Down
Loading