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

Revert rendering-related associated type name changes #11027

Merged
merged 6 commits into from
Jan 22, 2024

Conversation

alice-i-cecile
Copy link
Member

@alice-i-cecile alice-i-cecile commented Dec 19, 2023

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

Solution

Revert several renames in #10779.

Changelog

  • ViewNode::ViewData is now ViewNode::ViewQuery again.

Migration Guide

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen 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 Dec 19, 2023
@alice-i-cecile
Copy link
Member Author

Of the remaining changes, I could go either way on the renames. @IceSentry, I'll defer to your preferences here: just tell me which ones you'd like reverted.

@alice-i-cecile alice-i-cecile marked this pull request as ready for review December 19, 2023 16:32
@alice-i-cecile alice-i-cecile added this to the 0.13 milestone Dec 19, 2023
@JMS55
Copy link
Contributor

JMS55 commented Dec 19, 2023

I prefer ViewNode::ViewQuery

@IceSentry
Copy link
Contributor

IceSentry commented Dec 20, 2023

Here's my opinion on all of these:

  • Trait's ExtractComponent type Query has been renamed to Data.
    • Revert it to Query or QueryData
  • Trait's GetBatchData types Query & QueryFilter has been renamed to Data & Filter, respectively.
    • QueryData and QueryFilter
  • Trait's ExtractInstance type Query has been renamed to Data.
    • Revert it to Query or QueryData
  • Trait's ViewNode type ViewQuery has been renamed to ViewData.
  • Trait's RenderCommand types ViewWorldQuery & ItemWorldQuery has been renamed to ViewData & ItemData, respectively.
    • I'd prefer ViewQuery and ItemQuery

Essentially, in the spirit of the original PR, drop the World where it isn't needed, but keep the Query terminology otherwise. I mostly don't like when types are just called Data because it doesn't really give any hints as to what it actually is. I guess it makes sense to use QueryData where applicable to keep consistency with the rest of the engine. I just don't want to lose the Query part.

Copy link
Member

@JoJoJet JoJoJet left a comment

Choose a reason for hiding this comment

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

I agree with this. I think we should also implement the other renames IceSentry suggested.

Copy link
Contributor

@atlv24 atlv24 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, also here's icesentry's rename suggestions implemented and tested
alice-i-cecile#163

@alice-i-cecile alice-i-cecile 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 Jan 22, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 22, 2024
Merged via the queue into bevyengine:main with commit eb07d16 Jan 22, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen 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.

5 participants