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

Rename WorldQueryData & WorldQueryFilter to QueryData & QueryFilter #10779

Merged

Conversation

taizu-jin
Copy link
Contributor

@taizu-jin taizu-jin commented Nov 28, 2023

Rename WorldQueryData & WorldQueryFilter to QueryData & QueryFilter

Fixes #10776

Solution

Traits WorldQueryData & WorldQueryFilter were renamed to QueryData and QueryFilter, respectively. Related Trait types were also renamed.


Changelog

  • Trait WorldQueryData has been renamed to QueryData. Derive macro's QueryData attribute world_query_data has been renamed to query_data.
  • Trait WorldQueryFilter has been renamed to QueryFilter. Derive macro's QueryFilter attribute world_query_filter has been renamed to query_filter.
  • Trait's ExtractComponent type Query has been renamed to Data.
  • Trait's GetBatchData types Query & QueryFilter has been renamed to Data & Filter, respectively.
  • Trait's ExtractInstance type Query has been renamed to Data.
  • Trait's ViewNode type ViewQuery has been renamed to ViewData.
  • Trait's RenderCommand types ViewWorldQuery & ItemWorldQuery has been renamed to ViewData & ItemData, respectively.

Migration Guide

Note: if merged before 0.13 is released, this should instead modify the migration guide of #10776 with the updated names.

  • Rename WorldQueryData & WorldQueryFilter trait usages to QueryData & QueryFilter and their respective derive macro attributes world_query_data & world_query_filter to query_data & query_filter.
  • Rename the following trait type usages:
    • Trait's ExtractComponent type Query to Data.
    • Trait's GetBatchData type Query to Data.
    • Trait's ExtractInstance type Query to Data.
    • Trait's ViewNode type ViewQuery to ViewData'
    • Trait's RenderCommand types ViewWorldQuery & ItemWorldQuery to ViewData & ItemData, respectively.
// Before
#[derive(WorldQueryData)]
#[world_query_data(derive(Debug))]
struct EmptyQuery {
    empty: (),
}

// After
#[derive(QueryData)]
#[query_data(derive(Debug))]
struct EmptyQuery {
    empty: (),
}

// Before
#[derive(WorldQueryFilter)]
struct CustomQueryFilter<T: Component, P: Component> {
    _c: With<ComponentC>,
    _d: With<ComponentD>,
    _or: Or<(Added<ComponentC>, Changed<ComponentD>, Without<ComponentZ>)>,
    _generic_tuple: (With<T>, With<P>),
}

// After
#[derive(QueryFilter)]
struct CustomQueryFilter<T: Component, P: Component> {
    _c: With<ComponentC>,
    _d: With<ComponentD>,
    _or: Or<(Added<ComponentC>, Changed<ComponentD>, Without<ComponentZ>)>,
    _generic_tuple: (With<T>, With<P>),
}

// Before
impl ExtractComponent for ContrastAdaptiveSharpeningSettings {
    type Query = &'static Self;
    type Filter = With<Camera>;
    type Out = (DenoiseCAS, CASUniform);

    fn extract_component(item: QueryItem<Self::Query>) -> Option<Self::Out> {
        //...
    }
}

// After
impl ExtractComponent for ContrastAdaptiveSharpeningSettings {
    type Data = &'static Self;
    type Filter = With<Camera>;
    type Out = (DenoiseCAS, CASUniform);

    fn extract_component(item: QueryItem<Self::Data>) -> Option<Self::Out> {
        //...
    }
}

// Before
impl GetBatchData for MeshPipeline {
    type Param = SRes<RenderMeshInstances>;
    type Query = Entity;
    type QueryFilter = With<Mesh3d>;
    type CompareData = (MaterialBindGroupId, AssetId<Mesh>);
    type BufferData = MeshUniform;

    fn get_batch_data(
        mesh_instances: &SystemParamItem<Self::Param>,
        entity: &QueryItem<Self::Query>,
    ) -> (Self::BufferData, Option<Self::CompareData>) {
        // ....
    }
}

// After
impl GetBatchData for MeshPipeline {
    type Param = SRes<RenderMeshInstances>;
    type Data = Entity;
    type Filter = With<Mesh3d>;
    type CompareData = (MaterialBindGroupId, AssetId<Mesh>);
    type BufferData = MeshUniform;

    fn get_batch_data(
        mesh_instances: &SystemParamItem<Self::Param>,
        entity: &QueryItem<Self::Data>,
    ) -> (Self::BufferData, Option<Self::CompareData>) {
        // ....
    }
}

// Before
impl<A> ExtractInstance for AssetId<A>
where
    A: Asset,
{
    type Query = Read<Handle<A>>;
    type Filter = ();

    fn extract(item: QueryItem<'_, Self::Query>) -> Option<Self> {
        Some(item.id())
    }
}

// After
impl<A> ExtractInstance for AssetId<A>
where
    A: Asset,
{
    type Data = Read<Handle<A>>;
    type Filter = ();

    fn extract(item: QueryItem<'_, Self::Data>) -> Option<Self> {
        Some(item.id())
    }
}

// Before
impl ViewNode for PostProcessNode {
    type ViewQuery = (
        &'static ViewTarget,
        &'static PostProcessSettings,
    );

    fn run(
        &self,
        _graph: &mut RenderGraphContext,
        render_context: &mut RenderContext,
        (view_target, _post_process_settings): QueryItem<Self::ViewQuery>,
        world: &World,
    ) -> Result<(), NodeRunError> {
        // ...
    }
}

// After
impl ViewNode for PostProcessNode {
    type ViewData = (
        &'static ViewTarget,
        &'static PostProcessSettings,
    );

    fn run(
        &self,
        _graph: &mut RenderGraphContext,
        render_context: &mut RenderContext,
        (view_target, _post_process_settings): QueryItem<Self::ViewData>,
        world: &World,
    ) -> Result<(), NodeRunError> {
        // ...
    }
}

// Before
impl<P: CachedRenderPipelinePhaseItem> RenderCommand<P> for SetItemPipeline {
    type Param = SRes<PipelineCache>;
    type ViewWorldQuery = ();
    type ItemWorldQuery = ();
    #[inline]
    fn render<'w>(
        item: &P,
        _view: (),
        _entity: (),
        pipeline_cache: SystemParamItem<'w, '_, Self::Param>,
        pass: &mut TrackedRenderPass<'w>,
    ) -> RenderCommandResult {
        // ...
    }
}

// After
impl<P: CachedRenderPipelinePhaseItem> RenderCommand<P> for SetItemPipeline {
    type Param = SRes<PipelineCache>;
    type ViewData = ();
    type ItemData = ();
    #[inline]
    fn render<'w>(
        item: &P,
        _view: (),
        _entity: (),
        pipeline_cache: SystemParamItem<'w, '_, Self::Param>,
        pass: &mut TrackedRenderPass<'w>,
    ) -> RenderCommandResult {
        // ...
    }
}

Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@alice-i-cecile alice-i-cecile added this to the 0.13 milestone Nov 28, 2023
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Nov 28, 2023
@alice-i-cecile
Copy link
Member

@ItsDoot @james-j-obrien review please :D

@alice-i-cecile
Copy link
Member

@taizu-jin great PR description <3 Thanks for tackling this.

@taizu-jin
Copy link
Contributor Author

So the "fix" 25b9d7d for missing_deref test was actually an opposite. Is there a reason why my test run had a different output there and I could fix that instead or I should've just ignored it because I didn't touch that part of the code?

Copy link
Contributor

@james-j-obrien james-j-obrien left a comment

Choose a reason for hiding this comment

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

I'm a fan. The World prefix always looked odd to me (it's not used anywhere else) and seemed like a way to avoid conflicting with the Query type. Now that it's not a problem I see no reason not to do this.

@@ -29,7 +29,7 @@ mod field_attr_keywords {
syn::custom_keyword!(ignore);
}

pub static WORLD_QUERY_DATA_ATTRIBUTE_NAME: &str = "world_query_data";
pub static WORLD_QUERY_DATA_ATTRIBUTE_NAME: &str = "query_data";
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps remove WORLD here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was contemplating this as well. How about derive_world_query_filter, derive_world_query_filter_impl fns and their helper structs? Does it make sense to also rename the source file as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Imo it doesn't make sense to leave traces of the old naming that might mislead future contributors, but in the end these internal names/filenames aren't going to affect users so it's not a huge deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, I'm leaning on cleaning it up completely. Do you mind if I re-request for a review after the changes?

Copy link
Contributor

@ItsDoot ItsDoot left a comment

Choose a reason for hiding this comment

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

Looks good as-is, as long as James' concerns are satisfied. All the suggestions I list here are just suggestions and not required for my approval.

crates/bevy_render/src/extract_instances.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/extract_component.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/batching/mod.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/render_graph/node.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/render_phase/draw.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/render_phase/draw.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Nilirad Nilirad left a comment

Choose a reason for hiding this comment

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

I think WorldQueryDataFilter and WorldQueryDataAttributes should be changed. They are private, but still worth changing for consistency.

Also, module names and their respective files should be adapted too (e.g. world_query_data.rsquery_data.rs)

Copy link
Contributor

@Nilirad Nilirad left a comment

Choose a reason for hiding this comment

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

Review was tedious but straightforward. I think we can proceed.

Nilirad

This comment was marked as duplicate.

@Nilirad Nilirad added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Dec 10, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 12, 2023
Merged via the queue into bevyengine:main with commit 5af2f02 Dec 12, 2023
23 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Dec 13, 2023
…10782)

# Objective

Since #10776 split `WorldQuery` to `WorldQueryData` and
`WorldQueryFilter`, it should be clear that the query is actually
composed of two parts. It is not factually correct to call "query" only
the data part. Therefore I suggest to rename the `Q` parameter to `D` in
`Query` and related items.

As far as I know, there shouldn't be breaking changes from renaming
generic type parameters.

## Solution

I used a combination of rust-analyzer go to reference and `Ctrl-F`ing
various patterns to catch as many cases as possible. Hopefully I got
them all. Feel free to check if you're concerned of me having missed
some.

## Notes

This and #10779 have many lines in common, so merging one will cause a
lot of merge conflicts to the other.

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@taizu-jin taizu-jin deleted the rename-world-query-data-and-filter branch December 16, 2023 09:31
@eerii eerii mentioned this pull request Dec 21, 2023
10 tasks
@eerii eerii mentioned this pull request Dec 21, 2023
github-merge-queue bot pushed a commit that referenced this pull request Jan 22, 2024
# Objective

> Can anyone explain to me the reasoning of renaming all the types named
Query to Data. I'm talking about this PR
#10779 It doesn't make sense to
me that a bunch of types that are used to run queries aren't named Query
anymore. Like ViewQuery on the ViewNode is the type of the Query. I
don't really understand the point of the rename, it just seems like it
hides the fact that a query will run based on those types.


[@IceSentry](https://discord.com/channels/691052431525675048/692572690833473578/1184946251431694387)

## Solution

Revert several renames in #10779.

## Changelog

- `ViewNode::ViewData` is now `ViewNode::ViewQuery` again.

## Migration Guide

- This PR amends the migration guide in
#10779

---------

Co-authored-by: atlas dostal <rodol@rivalrebels.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename WorldQueryData and WorldQueryFilter to QueryData and QueryFilter, respectively
5 participants