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

chore: Renamed RenderInstance trait to ExtractInstance #10065

Merged

Conversation

jancespivo
Copy link
Contributor

@jancespivo jancespivo commented Oct 9, 2023

Objective

Fixes [#10061]

Solution

Renamed RenderInstance to ExtractInstance, RenderInstances to ExtractedInstances and RenderInstancePlugin to ExtractInstancesPlugin

@github-actions
Copy link
Contributor

github-actions bot commented Oct 9, 2023

Welcome, new contributor!

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

@jancespivo jancespivo changed the title chore: Rename RenderInstance trait to Extractable chore: Renamed RenderInstance trait to Extractable Oct 9, 2023
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change labels Oct 9, 2023
@alice-i-cecile alice-i-cecile added this to the 0.12 milestone Oct 9, 2023
@alice-i-cecile
Copy link
Member

Is this a breaking change relative to Bevy 0.11?

@superdump
Copy link
Contributor

@alice-i-cecile no. It's new API that was added to main recently. No 0.11 API was harmed in the making of this API. :)

@jancespivo - The Extractable naming doesn't feel right to me. The RenderInstance trait is implemented on the target type and can pull data from multiple main world components into that one type. I suggest we have a discussion to choose a name before making changes, just to save your time and effort. :)

@jancespivo
Copy link
Contributor Author

Is this a breaking change relative to Bevy 0.11?

It shouldn't be. This is "only" renaming of types introduced in #10002

@jancespivo
Copy link
Contributor Author

@jancespivo - The Extractable naming doesn't feel right to me. The RenderInstance trait is implemented on the target type and can pull data from multiple main world components into that one type. I suggest we have a discussion to choose a name before making changes, just to save your time and effort. :)

@superdump ok, sorry for rushing from my side. If it is more comfortable to have a discussion in the issue, let's move there. This is just quick and dirty renaming done in 2 minutes, so no problem for me. Feel free to close this PR :)

@pcwalton
Copy link
Contributor

pcwalton commented Oct 9, 2023

I don't have any opinion on the naming. Feel free to rename.

@jancespivo jancespivo force-pushed the chore/rename_render_instance_trait branch 2 times, most recently from 85d0fcd to 4afd3e4 Compare October 10, 2023 06:51
@jancespivo jancespivo changed the title chore: Renamed RenderInstance trait to Extractable chore: Renamed RenderInstance trait to ExtractInstance Oct 10, 2023
@jancespivo jancespivo marked this pull request as draft October 10, 2023 07:02
@alice-i-cecile
Copy link
Member

struct Body;

impl ExtractInstance for Body {
    type Query = Read<ArHeadset>;
    type Filter = ();
    fn extract(item: QueryStuff) -> Self {}
}

pub struct ExtractedInstances<T>;

pub struct ExtractInstancePlugin;

FYI, this was the consensus naming on Discord.

@jancespivo jancespivo force-pushed the chore/rename_render_instance_trait branch from 1b0f89c to 211ed32 Compare October 10, 2023 10:48
@jancespivo jancespivo marked this pull request as ready for review October 10, 2023 20:07
Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

Approving but please don't merge until #9258 is done to avoid creating conflicts in a big PR.

@alice-i-cecile alice-i-cecile added S-Blocked This cannot move forward until something else changes S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Oct 10, 2023
@jancespivo jancespivo force-pushed the chore/rename_render_instance_trait branch from 211ed32 to 260db53 Compare October 13, 2023 10:30
@superdump superdump added this pull request to the merge queue Oct 13, 2023
Merged via the queue into bevyengine:main with commit 4a61f89 Oct 13, 2023
ameknite pushed a commit to ameknite/bevy that referenced this pull request Nov 6, 2023
)

# Objective

Fixes [bevyengine#10061]

## Solution

Renamed `RenderInstance` to `ExtractInstance`, `RenderInstances` to
`ExtractedInstances` and `RenderInstancePlugin` to
`ExtractInstancesPlugin`
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
)

# Objective

Fixes [bevyengine#10061]

## Solution

Renamed `RenderInstance` to `ExtractInstance`, `RenderInstances` to
`ExtractedInstances` and `RenderInstancePlugin` to
`ExtractInstancesPlugin`
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-Code-Quality A section of code that is hard to understand or change S-Blocked This cannot move forward until something else changes 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