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] - Add ClearColor Resource to Pipelined Renderer #2631

Conversation

zicklag
Copy link
Member

@zicklag zicklag commented Aug 10, 2021

Objective

  • Allow the user to set the clear color when using the pipelined renderer

Solution

  • Add a ClearColor resource that can be added to the world to configure the clear color

Remaining Issues

Currently the ClearColor resource is cloned from the app world to the render world every frame. There are two ways I can think of around this:

  1. Figure out why app_world.is_resource_changed::<ClearColor>() always returns true in the extract step and fix it so that we are only updating the resource when it changes
  2. Require the users to add the ClearColor resource to the render sub-app instead of the parent app. This is currently sub-optimal until we have labled sub-apps, and probably a helper funciton on App such as app.with_sub_app(RenderApp, |app| { ... }). Even if we had that, I think it would be more than we want the user to have to think about. They shouldn't have to know about the render sub-app I don't think.

I think the first option is the best, but I could really use some help figuring out the nuance of why is_resource_changed is always returning true in that context.

@zicklag zicklag force-pushed the clear-color-pipelined branch 2 times, most recently from a4945ff to 08eb731 Compare August 10, 2021 18:27
@@ -178,6 +180,12 @@ impl Plugin for RenderPlugin {
}

fn extract(app_world: &mut World, render_app: &mut App) {
// Update the render world clear color resource if it was added or changed in the app world
if app_world.is_resource_added::<ClearColor>() || app_world.is_resource_changed::<ClearColor>()
Copy link
Member

@cart cart Aug 13, 2021

Choose a reason for hiding this comment

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

I don't think this should be updated here / hard coded in the "extract step execution logic". I think ClearColor should probably live in bevy_core_pipeline as it is behavior specific to "bevy's core render pipeline", not core renderer logic. It should just be another "extract stage system". That should also resolve the change tracking issue.

You should only need to query for Changed. "added" things also flip the "changed" flag.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, cool, that makes sense. I'll update as soon as I get back to this. 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed it. I like that much better!

I'm really liking the extract →prepare →queue rendering model. Very nice.

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible labels Aug 16, 2021
@zicklag zicklag force-pushed the clear-color-pipelined branch 2 times, most recently from bd7e8be to b36ab35 Compare August 19, 2021 19:32
@cart
Copy link
Member

cart commented Aug 19, 2021

bors r+

bors bot pushed a commit that referenced this pull request Aug 19, 2021
# Objective

- Allow the user to set the clear color when using the pipelined renderer

## Solution

- Add a `ClearColor` resource that can be added to the world to configure the clear color

## Remaining Issues

Currently the `ClearColor` resource is cloned from the app world to the render world every frame. There are two ways I can think of around this:

1. Figure out why `app_world.is_resource_changed::<ClearColor>()` always returns `true` in the `extract` step and fix it so that we are only updating the resource when it changes
2. Require the users to add the `ClearColor` resource to the render sub-app instead of the parent app. This is currently sub-optimal until we have labled sub-apps, and probably a helper funciton on `App` such as `app.with_sub_app(RenderApp, |app| { ... })`. Even if we had that, I think it would be more than we want the user to have to think about. They shouldn't have to know about the render sub-app I don't think.

I think the first option is the best, but I could really use some help figuring out the nuance of why `is_resource_changed` is always returning true in that context.
@bors
Copy link
Contributor

bors bot commented Aug 19, 2021

@bors bors bot changed the title Add ClearColor Resource to Pipelined Renderer [Merged by Bors] - Add ClearColor Resource to Pipelined Renderer Aug 19, 2021
@bors bors bot closed this Aug 19, 2021
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants