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

[Merged by Bors] - ExtractResourcePlugin #3745

Closed
wants to merge 12 commits into from

Conversation

superdump
Copy link
Contributor

@superdump superdump commented Jan 22, 2022

Objective

  • Add an ExtractResourcePlugin for convenience and consistency

Solution

  • Add an ExtractResourcePlugin similar to ExtractComponentPlugin but for ECS Resources. The system that is executed simply clones the main world resource into a render world resource, if and only if the main world resource was either added or changed since the last execution of the system.
  • Add an ExtractResource trait with a fn extract_resource(res: &Self) -> Self function. This is used by the ExtractResourcePlugin to extract the resource
  • Add a derive macro for ExtractResource on a Resource with the Clone trait, that simply returns res.clone()
  • Use ExtractResourcePlugin wherever both possible and appropriate

@superdump superdump self-assigned this Jan 22, 2022
@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jan 22, 2022
@superdump superdump added A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible and removed S-Needs-Triage This issue needs to be labelled labels Jan 22, 2022
@superdump superdump requested a review from cart January 22, 2022 14:01
@kurtkuehnert
Copy link
Contributor

Should we use a ExtractResource trait to specify the extract method explicitly like we do for components? Is there a use case where we want to do something else than simply clone the component/resource into the render world?
And additionally I have thought about adding a derive ExtractComponent macro, which simply clones the component (without a filter), since this seems to be the common case.

@superdump
Copy link
Contributor Author

Should we use a ExtractResource trait to specify the extract method explicitly like we do for components? Is there a use case where we want to do something else than simply clone the component/resource into the render world?

Yup, that is definitely a use case as there are multiple instances of resource extraction systems that do something beyond just cloning. I'll look into it.

And additionally I have thought about adding a derive ExtractComponent macro, which simply clones the component (without a filter), since this seems to be the common case.

I was thinking that it would be good to go through and make use of ExtractComponentPlugin 'everywhere' now too. Mmm, perhaps that could be a reasonable approach... not requiring Clone but rather requiring it when deriving ExtractComponent. And similar for ExtractResource.

@superdump
Copy link
Contributor Author

I had a go at adding an ExtractResource trait, as well as a derive macro for it. This is my first time implementing any macro at all from scratch in Rust as far as I can remember. I pieced together some bits I found among the ECS derive macros and I think I've done it correctly but please do scrutinise it so I can learn if I did anything wrong. :)

@kurtkuehnert
Copy link
Contributor

I really like those changes. The new plugin and its derive macro should make the extraction process way more ergonomic and should remove some boilerplate from user code. I am not familiar with macros as well, but the rest LGTM.
Now only the ExtractComponent derive macro is left, but this should probably be its own PR, because it is a bit less straight forward and maybe even controversial.

@superdump
Copy link
Contributor Author

Yeah, I think keeping this PR to be only about extracting resources is a good idea, so I didn't look into the derive macro for extracting components. If we like the pattern here and we think it makes sense for components too then I can make another PR for that.

Copy link
Contributor

@MinerSebas MinerSebas left a comment

Choose a reason for hiding this comment

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

The new bevy_render_macros crate also needs to be added to the ./tools/publish.sh script

crates/bevy_render/src/render_ecs_resource.rs Outdated Show resolved Hide resolved
@superdump
Copy link
Contributor Author

The new bevy_render_macros crate also needs to be added to the ./tools/publish.sh script

Done.

@superdump superdump force-pushed the extract-resource-plugin branch 3 times, most recently from 8da17f1 to 009f982 Compare March 28, 2022 07:54
@james7132 james7132 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 Apr 8, 2022
Copy link
Member

@cart cart 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 on board for this generally, but I'd like us to consider an alternative trait design.

crates/bevy_render/src/render_ecs_resource.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/render_ecs_resource.rs Outdated Show resolved Hide resolved
@superdump
Copy link
Contributor Author

@cart any further comments or is this good to go now?

@alice-i-cecile alice-i-cecile requested a review from cart May 16, 2022 17:29
@alice-i-cecile
Copy link
Member

This has been sitting for a couple weeks, and isn't controversial. I've done a final pass and I'm going to merge it in. We can tweak it further once it's in as we build on it.

@alice-i-cecile
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented May 30, 2022

Merge conflict.

@alice-i-cecile
Copy link
Member

@superdump can you rebase?

@superdump
Copy link
Contributor Author

@superdump can you rebase?

Merged in main.

@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request May 30, 2022
# Objective

- Add an `ExtractResourcePlugin` for convenience and consistency

## Solution

- Add an `ExtractResourcePlugin` similar to `ExtractComponentPlugin` but for ECS `Resource`s. The system that is executed simply clones the main world resource into a render world resource, if and only if the main world resource was either added or changed since the last execution of the system.
- Add an `ExtractResource` trait with a `fn extract_resource(res: &Self) -> Self` function. This is used by the `ExtractResourcePlugin` to extract the resource
- Add a derive macro for `ExtractResource` on a `Resource` with the `Clone` trait, that simply returns `res.clone()`
- Use `ExtractResourcePlugin` wherever both possible and appropriate
@bors bors bot changed the title ExtractResourcePlugin [Merged by Bors] - ExtractResourcePlugin May 30, 2022
@bors bors bot closed this May 30, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Jun 7, 2022
# Objective

- Add an `ExtractResourcePlugin` for convenience and consistency

## Solution

- Add an `ExtractResourcePlugin` similar to `ExtractComponentPlugin` but for ECS `Resource`s. The system that is executed simply clones the main world resource into a render world resource, if and only if the main world resource was either added or changed since the last execution of the system.
- Add an `ExtractResource` trait with a `fn extract_resource(res: &Self) -> Self` function. This is used by the `ExtractResourcePlugin` to extract the resource
- Add a derive macro for `ExtractResource` on a `Resource` with the `Clone` trait, that simply returns `res.clone()`
- Use `ExtractResourcePlugin` wherever both possible and appropriate
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- Add an `ExtractResourcePlugin` for convenience and consistency

## Solution

- Add an `ExtractResourcePlugin` similar to `ExtractComponentPlugin` but for ECS `Resource`s. The system that is executed simply clones the main world resource into a render world resource, if and only if the main world resource was either added or changed since the last execution of the system.
- Add an `ExtractResource` trait with a `fn extract_resource(res: &Self) -> Self` function. This is used by the `ExtractResourcePlugin` to extract the resource
- Add a derive macro for `ExtractResource` on a `Resource` with the `Clone` trait, that simply returns `res.clone()`
- Use `ExtractResourcePlugin` wherever both possible and appropriate
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-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants