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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Add a separate ClearPass #3209

Closed
wants to merge 4 commits into from
Closed

[Merged by Bors] - Add a separate ClearPass #3209

wants to merge 4 commits into from

Conversation

KirmesBude
Copy link
Contributor

@KirmesBude KirmesBude commented Nov 27, 2021 •
edited
Loading

Objective

Solution

  • I added a "Clear" SubGraph, a "ClearPassNode" Node, that clears the color and depth attachments of all views and a "ClearNodeDriver" Node, that schedules the "ClearPassNode" before MainPass.
  • Make sure that the 2d and 3d draw passes do not clear their attachments anymore.

Notes

  • It works in the way, that with the current pipeline examples nothing should have changed in their behaviour
  • I would like to add an example that adds a pass inbetween ClearPass and MainPass, but I do not understand enough about the new render architecture to do that yet
  • Clears all attachment for all views: I do not know enough about rendering in general to say, whether there is a use case for not clearing
  • Does not solve Pipelined Rendering: App crashes when there is no camera #3043 as we still need Cameras/ViewTargets to clear.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Nov 27, 2021
@alice-i-cecile alice-i-cecile 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 Nov 27, 2021
@alice-i-cecile alice-i-cecile added this to the Bevy 0.6 milestone Nov 27, 2021
@jakobhellermann
Copy link
Contributor

The way this is implemented right now won't quite fix #3190 yet because ViewTargets are only extracted for cameras in the scene. Are you in the bevy discord? I opened a thread today discussing possible solutions: https://discord.com/channels/691052431525675048/743663924229963868/914117453150322718

@KirmesBude
Copy link
Contributor Author

Yes, I do occasionally lurk on the discord :D. Came across the thread after I had already done my initial changes.
If I understood the thread correctly its specifically about the case where no cameras are used? (the MSAA discussion did not seem very relevant)

I feel like this would theoretically fix #3190, but I if there is more to it as was discussed in the thread, maybe it makes sense to have a complete solution from the beginning.
I will try to understand the proposed solutions and see if I am able to implement any of them.

@jakobhellermann
Copy link
Contributor

Oh, I mixed up #3190 and #3043. I guess I had the other issue on my mind and didn't read the PR description properly 😃

@Davier Davier mentioned this pull request Nov 28, 2021
Closed
9 tasks
StarArawn
StarArawn approved these changes Dec 9, 2021
View reviewed changes
Copy link
Contributor

@StarArawn StarArawn left a comment

Choose a reason for hiding this comment

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

This looks good to me! It allows rendering of both 2d and 3d at the same time. I think in the future we might want to make the views configurable so you can specify which ones get cleared, but I don't think that's required yet. Can we get main merged in or this branch rebased to get rid of the merge conflicts?

@KirmesBude KirmesBude changed the title Feature/issue 3190 clear pass Add a separate ClearPass Dec 9, 2021
@cart
Copy link
Member

cart commented Dec 9, 2021

Yeah I agree that ultimately we'll want "per view" clear behaviors / graphs, but those changes seem like they will go hand in hand with the addition of "multiple render targets". This implementation seems like a perfectly reasonable interim solution.

@cart
Copy link
Member

cart commented Dec 9, 2021

I believe CI should be working now. Lets give it a go!

@cart
Copy link
Member

cart commented Dec 9, 2021

bors r+

bors bot pushed a commit that referenced this pull request Dec 9, 2021
@KirmesBude
Add a separate ClearPass (#3209)
82c04f9 
# Objective

- Rendering before MainPass should be possible, so clearing needs to happen in an earlier pass.
- Fixes #3190.

## Solution

- I added a "Clear" SubGraph, a "ClearPassNode" Node, that clears the color and depth attachments of all views and a "ClearNodeDriver" Node, that schedules the "ClearPassNode" before MainPass.
- Make sure that the 2d and 3d draw passes do not clear their attachments anymore.

### Notes

- It works in the way, that with the current pipeline examples nothing should have changed in their behaviour
- I would like to add an example that adds a pass inbetween ClearPass and MainPass, but I do not understand enough about the new render architecture to do that yet
- Clears all attachment for all views: I do not know enough about rendering in general to say, whether there is a use case for not clearing
- Does not solve #3043 as we still need Cameras/ViewTargets to clear.
@bors
Copy link
Contributor

bors bot commented Dec 9, 2021

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Add a separate ClearPass [Merged by Bors] - Add a separate ClearPass Dec 9, 2021
@bors bors bot closed this Dec 9, 2021
@mockersf mockersf mentioned this pull request Dec 10, 2021
Closed
bors bot pushed a commit that referenced this pull request Dec 10, 2021
@mockersf
clearpass: also clear views without depth (2d) (#3286)
cf48132 
# Objective

- after #3209, 2d examples background were not cleared

<img width="912" alt="Screenshot 2021-12-10 at 00 48 04" src="https://user-images.githubusercontent.com/8672791/145494415-d4b7a149-6f9a-4036-9ac5-3d1227b4de69.png">

## Solution

- Change the query to also work when there isn't a `ViewDepthTexture`
@folke folke mentioned this pull request Dec 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StarArawn StarArawn StarArawn approved these changes

Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible
Projects
None yet
Milestone
Bevy 0.6
Development

Successfully merging this pull request may close these issues.

Support for RenderPass that renders to WindowSwapChain before MainPass
5 participants
@KirmesBude @jakobhellermann @cart @StarArawn @alice-i-cecile