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

Let query items borrow from query state to avoid needing to clone #15396

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
e99a0c1
Don't obtain two NameOrEntity items at once from QueryState::get().
chescock Sep 18, 2024
f295624
Introduce a ReleaseStateQueryData trait that will release borrows on …
chescock Sep 22, 2024
648eb31
Add a 'state lifetime to query items and fetches.
chescock Sep 22, 2024
b870249
Merge remote-tracking branch 'remotes/origin/main' into state-in-quer…
chescock Sep 23, 2024
f6f4259
Fix merge conflicts with traversal changes.
chescock Sep 23, 2024
be7a1de
Merge remote-tracking branch 'remotes/origin/main' into state-in-quer…
chescock Sep 28, 2024
0872293
Link to update_archetypes() and corresponding _manual() methods where…
chescock Sep 29, 2024
70ced02
Merge remote-tracking branch 'remotes/origin/main' into state-in-quer…
chescock Sep 29, 2024
d1fbada
Fix merge conflicts with QuerySingle.
chescock Sep 29, 2024
22eee9e
Merge remote-tracking branch 'remotes/origin/main' into state-in-quer…
chescock Nov 10, 2024
174af45
Fix new uses of Item<'w>.
chescock Nov 10, 2024
18520ef
Merge commit '6be11a8a42c2c73646cc994392bbd628b6de523b' into state-in…
chescock Feb 9, 2025
a080cb0
Fix build.
chescock Feb 9, 2025
ac313fd
Update AssetChanged.
chescock Feb 9, 2025
9a74906
impl ReleaseStateQueryData for Traversal types.
chescock Feb 9, 2025
6188a86
Add `expect` and `reason` for `allow(clippy::unused_unit)` in tuple i…
chescock Feb 9, 2025
9871970
Merge commit '03af547c2898cb905b3917725b13081e48654b15' into state-in…
chescock Feb 9, 2025
371ca14
Merge remote-tracking branch 'remotes/origin/main' into state-in-quer…
chescock Feb 10, 2025
5dce18f
Restore changes lost by bad merging.
chescock Feb 10, 2025
d63f936
impl ReleaseStateQueryData for Option, MainEntity, and RenderEntity.
chescock Feb 10, 2025
7ad98aa
Implement ReleaseStateQueryData for derived QueryData.
chescock Feb 10, 2025
e89a909
Use `collect()` in `QueryIter::sort()`.
chescock Feb 9, 2025
df2fcce
Merge remote-tracking branch 'remotes/origin/main' into state-in-quer…
chescock Feb 12, 2025
f7c3b27
Update new uses of `QueryItem` to include `'s` lifetime.
chescock Feb 12, 2025
4e60256
Document that most QueryData types impl ReleaseStateQueryData.
chescock Feb 12, 2025
42d5aa3
Merge remote-tracking branch 'remotes/origin/main' into state-in-quer…
chescock Mar 7, 2025
a748abe
Add `'s` lifetime to new uses of QueryItem.
chescock Mar 7, 2025
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
4 changes: 2 additions & 2 deletions crates/bevy_core/src/name.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ pub struct NameOrEntity {
pub entity: Entity,
}

impl<'a> std::fmt::Display for NameOrEntityItem<'a> {
impl<'w, 's> std::fmt::Display for NameOrEntityItem<'w, 's> {
#[inline(always)]
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
match self.name {
Expand Down Expand Up @@ -229,9 +229,9 @@ mod tests {
let e2 = world.spawn(name.clone()).id();
let mut query = world.query::<NameOrEntity>();
let d1 = query.get(&world, e1).unwrap();
let d2 = query.get(&world, e2).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

This needing to be moved is suspicious. Can you not call get multiple times on an immutable borrow of the world anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not in this case, but I promise it's not as bad as it looks!

QueryState::get() takes &mut self so that it can call update_archetypes(). And NameOrEntity uses #[derive(QueryData)], which now generates an item struct that includes the 's lifetime. So the item is treated as having a mutable borrow of the QueryState.

This doesn't affect Query::get(), which always takes &self.

And this doesn't affect concrete types where QueryData::Item doesn't include 's. That is, it only affects generic parameters, #[derive(QueryData)] types, and (in the future) FilteredEntityRef|Mut. If we use let mut query = world.query::<(Option<&Name>, Entity)>(); here, it will compile.

And there is a workaround in this case, which is to call query.update_archetypes() manually and then use query.get_manual(), which takes &self and so will allow overlapping borrows.

But, yeah, it is a breaking change here.

Copy link
Contributor

@hymm hymm Sep 28, 2024

Choose a reason for hiding this comment

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

After digesting this for a bit, maybe it isn't such a bad change. get takes an &mut self so hopefully it won't be that surprising for new users if you can't get multiple items out of it. And the workaround is more performant, so leading users to that is probably better.

But we should document the workaround on QueryState::get and in the migration guide.

I haven't done a full review yet, but hopefully will get to that soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this not also affect query iteration? As in, for the affected types, any item would have to be dropped before another can be retrieved?
(Like what happens in QueryManyIter::fetch_next(), where item lifetimes are shrunk like this intentionally)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this not also affect query iteration?

No, it doesn't. Query has a &'s QueryState, so we can copy that reference and hand it out with the full 's lifetime. This change only affects methods that take &mut QueryState.

(The issue with things like QueryManyIter::fetch_next() and Query::iter_mut() are that the UnsafeWorldCell<'w> is acting like a &'w mut, so we can't copy it and instead have to reborrow it with a shorter 'w lifetime.)

Copy link
Contributor

@Victoronz Victoronz Sep 29, 2024

Choose a reason for hiding this comment

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

Okay, so this does not present an issue within individual iterators.
However, this will be a breaking change to more methods of QueryState than what you've currently listed:

  • get
  • get_single (no checked manual version)
  • get_many (no manual version)
  • single (no manual version)
  • iter
  • iter_many
  • iter_combinations (no manual version)
  • par_iter (no manual version)
  • and naturally, any method that takes at least &'a QueryState while the query items returned by one of the above is live.

The lack of manual versions can be considered API holes for some of the above, however not all of them (f.e. single)

Do I understand correctly?

If so, I think the ability to mix these methods of QueryState contains important use cases, and I would see those as more valuable than this change in its current form :/

// NameOrEntity Display for entities without a Name should be {index}v{generation}
assert_eq!(d1.to_string(), "0v1");
let d2 = query.get(&world, e2).unwrap();
// NameOrEntity Display for entities with a Name should be the Name
assert_eq!(d2.to_string(), "MyName");
}
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_core_pipeline/src/bloom/settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ impl ExtractComponent for Bloom {
type QueryFilter = ();
type Out = (Self, BloomUniforms);

fn extract_component((bloom, camera): QueryItem<'_, Self::QueryData>) -> Option<Self::Out> {
fn extract_component((bloom, camera): QueryItem<'_, '_, Self::QueryData>) -> Option<Self::Out> {
match (
camera.physical_viewport_rect(),
camera.physical_viewport_size(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ impl ViewNode for MainOpaquePass2dNode {
&self,
graph: &mut RenderGraphContext,
render_context: &mut RenderContext<'w>,
(camera, target, depth): QueryItem<'w, Self::ViewQuery>,
(camera, target, depth): QueryItem<'w, '_, Self::ViewQuery>,
world: &'w World,
) -> Result<(), NodeRunError> {
let (Some(opaque_phases), Some(alpha_mask_phases)) = (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ impl ViewNode for MainTransparentPass2dNode {
&self,
graph: &mut RenderGraphContext,
render_context: &mut RenderContext<'w>,
(camera, target, depth): bevy_ecs::query::QueryItem<'w, Self::ViewQuery>,
(camera, target, depth): bevy_ecs::query::QueryItem<'w, '_, Self::ViewQuery>,
world: &'w World,
) -> Result<(), NodeRunError> {
let Some(transparent_phases) =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ impl ViewNode for MainOpaquePass3dNode {
skybox_pipeline,
skybox_bind_group,
view_uniform_offset,
): QueryItem<'w, Self::ViewQuery>,
): QueryItem<'w, '_, Self::ViewQuery>,
world: &'w World,
) -> Result<(), NodeRunError> {
let (Some(opaque_phases), Some(alpha_mask_phases)) = (
Expand Down
6 changes: 5 additions & 1 deletion crates/bevy_core_pipeline/src/deferred/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,11 @@ impl ViewNode for DeferredGBufferPrepassNode {
&self,
graph: &mut RenderGraphContext,
render_context: &mut RenderContext<'w>,
(view, camera, view_depth_texture, view_prepass_textures): QueryItem<'w, Self::ViewQuery>,
(view, camera, view_depth_texture, view_prepass_textures): QueryItem<
'w,
'_,
Self::ViewQuery,
>,
world: &'w World,
) -> Result<(), NodeRunError> {
let (Some(opaque_deferred_phases), Some(alpha_mask_deferred_phases)) = (
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_core_pipeline/src/dof/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ impl ViewNode for DepthOfFieldNode {
view_bind_group_layouts,
depth_of_field_uniform_index,
auxiliary_dof_texture,
): QueryItem<'w, Self::ViewQuery>,
): QueryItem<'w, '_, Self::ViewQuery>,
world: &'w World,
) -> Result<(), NodeRunError> {
let pipeline_cache = world.resource::<PipelineCache>();
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_core_pipeline/src/msaa_writeback.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ impl ViewNode for MsaaWritebackNode {
&self,
_graph: &mut RenderGraphContext,
render_context: &mut RenderContext<'w>,
(target, blit_pipeline_id, msaa): QueryItem<'w, Self::ViewQuery>,
(target, blit_pipeline_id, msaa): QueryItem<'w, '_, Self::ViewQuery>,
world: &'w World,
) -> Result<(), NodeRunError> {
if *msaa == Msaa::Off {
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_core_pipeline/src/post_process/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ impl ViewNode for PostProcessingNode {
&self,
_: &mut RenderGraphContext,
render_context: &mut RenderContext<'w>,
(view_target, pipeline_id, chromatic_aberration, post_processing_uniform_buffer_offsets): QueryItem<'w, Self::ViewQuery>,
(view_target, pipeline_id, chromatic_aberration, post_processing_uniform_buffer_offsets): QueryItem<'w, '_, Self::ViewQuery>,
world: &'w World,
) -> Result<(), NodeRunError> {
let pipeline_cache = world.resource::<PipelineCache>();
Expand Down Expand Up @@ -493,7 +493,7 @@ impl ExtractComponent for ChromaticAberration {
type Out = ChromaticAberration;

fn extract_component(
chromatic_aberration: QueryItem<'_, Self::QueryData>,
chromatic_aberration: QueryItem<'_, '_, Self::QueryData>,
) -> Option<Self::Out> {
// Skip the postprocessing phase entirely if the intensity is zero.
if chromatic_aberration.intensity > 0.0 {
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_core_pipeline/src/prepass/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ impl ViewNode for PrepassNode {
skybox_prepass_pipeline,
skybox_prepass_bind_group,
view_prev_uniform_offset,
): QueryItem<'w, Self::ViewQuery>,
): QueryItem<'w, '_, Self::ViewQuery>,
world: &'w World,
) -> Result<(), NodeRunError> {
let (Some(opaque_prepass_phases), Some(alpha_mask_prepass_phases)) = (
Expand Down
4 changes: 3 additions & 1 deletion crates/bevy_core_pipeline/src/skybox/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,9 @@ impl ExtractComponent for Skybox {
type QueryFilter = ();
type Out = (Self, SkyboxUniforms);

fn extract_component((skybox, exposure): QueryItem<'_, Self::QueryData>) -> Option<Self::Out> {
fn extract_component(
(skybox, exposure): QueryItem<'_, '_, Self::QueryData>,
) -> Option<Self::Out> {
let exposure = exposure
.map(Exposure::exposure)
.unwrap_or_else(|| Exposure::default().exposure());
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_core_pipeline/src/smaa/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -845,7 +845,7 @@ impl ViewNode for SmaaNode {
view_smaa_uniform_offset,
smaa_textures,
view_smaa_bind_groups,
): QueryItem<'w, Self::ViewQuery>,
): QueryItem<'w, '_, Self::ViewQuery>,
world: &'w World,
) -> Result<(), NodeRunError> {
let pipeline_cache = world.resource::<PipelineCache>();
Expand Down
1 change: 1 addition & 0 deletions crates/bevy_ecs/macros/src/query_data.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ pub fn derive_query_data_impl(input: TokenStream) -> TokenStream {
let user_generics_with_world = {
let mut generics = ast.generics;
generics.params.insert(0, parse_quote!('__w));
generics.params.insert(0, parse_quote!('__s));
generics
};
let (user_impl_generics_with_world, user_ty_generics_with_world, user_where_clauses_with_world) =
Expand Down
5 changes: 3 additions & 2 deletions crates/bevy_ecs/macros/src/query_filter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ pub fn derive_query_filter_impl(input: TokenStream) -> TokenStream {
let user_generics_with_world = {
let mut generics = ast.generics;
generics.params.insert(0, parse_quote!('__w));
generics.params.insert(0, parse_quote!('__s));
generics
};
let (user_impl_generics_with_world, user_ty_generics_with_world, user_where_clauses_with_world) =
Expand Down Expand Up @@ -127,8 +128,8 @@ pub fn derive_query_filter_impl(input: TokenStream) -> TokenStream {

#[allow(unused_variables)]
#[inline(always)]
unsafe fn filter_fetch<'__w>(
_fetch: &mut <Self as #path::query::WorldQuery>::Fetch<'__w>,
unsafe fn filter_fetch<'__w, '__s>(
_fetch: &mut <Self as #path::query::WorldQuery>::Fetch<'__w, '__s>,
_entity: #path::entity::Entity,
_table_row: #path::storage::TableRow,
) -> bool {
Expand Down
54 changes: 27 additions & 27 deletions crates/bevy_ecs/macros/src/world_query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,14 @@ pub(crate) fn item_struct(
#derive_macro_call
#item_attrs
#visibility struct #item_struct_name #user_impl_generics_with_world #user_where_clauses_with_world {
#(#(#field_attrs)* #field_visibilities #field_idents: <#field_types as #path::query::WorldQuery>::Item<'__w>,)*
#(#(#field_attrs)* #field_visibilities #field_idents: <#field_types as #path::query::WorldQuery>::Item<'__w, '__s>,)*
}
},
Fields::Unnamed(_) => quote! {
#derive_macro_call
#item_attrs
#visibility struct #item_struct_name #user_impl_generics_with_world #user_where_clauses_with_world(
#( #field_visibilities <#field_types as #path::query::WorldQuery>::Item<'__w>, )*
#( #field_visibilities <#field_types as #path::query::WorldQuery>::Item<'__w, '__s>, )*
);
},
Fields::Unit => quote! {
Expand Down Expand Up @@ -88,16 +88,16 @@ pub(crate) fn world_query_impl(
)]
#[automatically_derived]
#visibility struct #fetch_struct_name #user_impl_generics_with_world #user_where_clauses_with_world {
#(#named_field_idents: <#field_types as #path::query::WorldQuery>::Fetch<'__w>,)*
#marker_name: &'__w (),
#(#named_field_idents: <#field_types as #path::query::WorldQuery>::Fetch<'__w, '__s>,)*
#marker_name: (&'__w(), &'__s()),
}

impl #user_impl_generics_with_world Clone for #fetch_struct_name #user_ty_generics_with_world
#user_where_clauses_with_world {
fn clone(&self) -> Self {
Self {
#(#named_field_idents: self.#named_field_idents.clone(),)*
#marker_name: &(),
#marker_name: (&(), &()),
}
}
}
Expand All @@ -106,37 +106,37 @@ pub(crate) fn world_query_impl(
unsafe impl #user_impl_generics #path::query::WorldQuery
for #struct_name #user_ty_generics #user_where_clauses {

type Item<'__w> = #item_struct_name #user_ty_generics_with_world;
type Fetch<'__w> = #fetch_struct_name #user_ty_generics_with_world;
type Item<'__w, '__s> = #item_struct_name #user_ty_generics_with_world;
type Fetch<'__w, '__s> = #fetch_struct_name #user_ty_generics_with_world;
type State = #state_struct_name #user_ty_generics;

fn shrink<'__wlong: '__wshort, '__wshort>(
item: <#struct_name #user_ty_generics as #path::query::WorldQuery>::Item<'__wlong>
) -> <#struct_name #user_ty_generics as #path::query::WorldQuery>::Item<'__wshort> {
fn shrink<'__wlong: '__wshort, '__wshort, '__s>(
item: <#struct_name #user_ty_generics as #path::query::WorldQuery>::Item<'__wlong, '__s>
) -> <#struct_name #user_ty_generics as #path::query::WorldQuery>::Item<'__wshort, '__s> {
#item_struct_name {
#(
#field_idents: <#field_types>::shrink(item.#field_idents),
)*
}
}

fn shrink_fetch<'__wlong: '__wshort, '__wshort>(
fetch: <#struct_name #user_ty_generics as #path::query::WorldQuery>::Fetch<'__wlong>
) -> <#struct_name #user_ty_generics as #path::query::WorldQuery>::Fetch<'__wshort> {
fn shrink_fetch<'__wlong: '__wshort, '__wshort, '__s>(
fetch: <#struct_name #user_ty_generics as #path::query::WorldQuery>::Fetch<'__wlong, '__s>
) -> <#struct_name #user_ty_generics as #path::query::WorldQuery>::Fetch<'__wshort, '__s> {
#fetch_struct_name {
#(
#named_field_idents: <#field_types>::shrink_fetch(fetch.#named_field_idents),
)*
#marker_name: &(),
#marker_name: (&(), &()),
}
}

unsafe fn init_fetch<'__w>(
unsafe fn init_fetch<'__w, '__s>(
_world: #path::world::unsafe_world_cell::UnsafeWorldCell<'__w>,
state: &Self::State,
state: &'__s Self::State,
_last_run: #path::component::Tick,
_this_run: #path::component::Tick,
) -> <Self as #path::query::WorldQuery>::Fetch<'__w> {
) -> <Self as #path::query::WorldQuery>::Fetch<'__w, '__s> {
#fetch_struct_name {
#(#named_field_idents:
<#field_types>::init_fetch(
Expand All @@ -146,17 +146,17 @@ pub(crate) fn world_query_impl(
_this_run,
),
)*
#marker_name: &(),
#marker_name: (&(), &()),
}
}

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

/// SAFETY: we call `set_archetype` for each member that implements `Fetch`
#[inline]
unsafe fn set_archetype<'__w>(
_fetch: &mut <Self as #path::query::WorldQuery>::Fetch<'__w>,
_state: &Self::State,
unsafe fn set_archetype<'__w, '__s>(
_fetch: &mut <Self as #path::query::WorldQuery>::Fetch<'__w, '__s>,
_state: &'__s Self::State,
_archetype: &'__w #path::archetype::Archetype,
_table: &'__w #path::storage::Table
) {
Expand All @@ -165,21 +165,21 @@ pub(crate) fn world_query_impl(

/// SAFETY: we call `set_table` for each member that implements `Fetch`
#[inline]
unsafe fn set_table<'__w>(
_fetch: &mut <Self as #path::query::WorldQuery>::Fetch<'__w>,
_state: &Self::State,
unsafe fn set_table<'__w, '__s>(
_fetch: &mut <Self as #path::query::WorldQuery>::Fetch<'__w, '__s>,
_state: &'__s Self::State,
_table: &'__w #path::storage::Table
) {
#(<#field_types>::set_table(&mut _fetch.#named_field_idents, &_state.#named_field_idents, _table);)*
}

/// SAFETY: we call `fetch` for each member that implements `Fetch`.
#[inline(always)]
unsafe fn fetch<'__w>(
_fetch: &mut <Self as #path::query::WorldQuery>::Fetch<'__w>,
unsafe fn fetch<'__w, '__s>(
_fetch: &mut <Self as #path::query::WorldQuery>::Fetch<'__w, '__s>,
_entity: #path::entity::Entity,
_table_row: #path::storage::TableRow,
) -> <Self as #path::query::WorldQuery>::Item<'__w> {
) -> <Self as #path::query::WorldQuery>::Item<'__w, '__s> {
Self::Item {
#(#field_idents: <#field_types>::fetch(&mut _fetch.#named_field_idents, _entity, _table_row),)*
}
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/observer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,7 @@ mod tests {
struct Parent(Entity);

impl Traversal for &'_ Parent {
fn traverse(item: Self::Item<'_>) -> Option<Entity> {
fn traverse(item: Self::Item<'_, '_>) -> Option<Entity> {
Some(item.0)
}
}
Expand Down
Loading