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

Add FullscreenPassNode for post-processing #2027

Closed
wants to merge 15 commits into from

Conversation

mtsr
Copy link
Contributor

@mtsr mtsr commented Apr 27, 2021

This is one of several PRs coming out of #1988. That PR kept growing, because in order to make the base graph accommodate flexible post processing, significant changes were needed.

It depends on #1998 for the additions to WindowTextureNode.
Waiting for #2039 to be able to cleanup plugins boilerplate.
Could really use #2043 to show passing ECS data into FullscreenPassNode shader.

This PR is a simpler addition of just FullscreenPassNode and an example that uses it.

Causes of the larger than expected length of the example:

  • Due to setup of the base graph, we cannot use DefaultPlugins;
  • There's a few steps to force MainPass to use linear color textures;
  • GLTF loader always uses PBR_PIPELINE_HANDLE, so that needs to be overwritten with a linear color pipeline.

Ideally we solve these issues so the example can be simpler.

Edit: Here's the render graph dump.

image

@Moxinilian Moxinilian added C-Enhancement A new feature A-Rendering Drawing game state to the screen labels Apr 27, 2021
@cart
Copy link
Member

cart commented Apr 28, 2021

@mtsr I'm curious to hear your thoughts on a general "strategy" thing. As you know, we're currently investigating major changes to the renderer apis (and we would like this to land before Bevy 0.6 if possible). I'm currently inclined to hold off on reviewing / merging renderer prs until after we make these changes (which could either be a complete api overhaul or iterative changes to what we have).

This both allows us to focus on getting the "renderer rework" changes out asap, cuts down on initial porting efforts, and allows us to review new features like this in the "new context".

@mtsr
Copy link
Contributor Author

mtsr commented Apr 28, 2021

@mtsr I'm curious to hear your thoughts on a general "strategy" thing. As you know, we're currently investigating major changes to the renderer apis (and we would like this to land before Bevy 0.6 if possible). I'm currently inclined to hold off on reviewing / merging renderer prs until after we make these changes (which could either be a complete api overhaul or iterative changes to what we have).

This both allows us to focus on getting the "renderer rework" changes out asap, cuts down on initial porting efforts, and allows us to review new features like this in the "new context".

There's two important reasons I'm still working on this.

  1. I had post processing going on for a while in my prototype and I know there's more than a few people who are actually working on things that can use it, or who have similar way less clean solutions like I did. I just needed to polish my work and fit it into a reusable thing, which I think this is.
  2. I really felt the need to spend some more time getting to know the ins and outs of the current system. Also my lack of graphics background puts me at a significant disadvantage when judging what we need from future API improvements. By building more things now, I'll be able to contribute more to the upcoming work.

Then there's the fact that once I got going this was just really enjoyable and I wanted to finish it.

Also, I did decide to drop integrating it into the base or pbr rendergraphs, as there's complications there that are not worth it with the upcoming changes in my mind.

Still, I think merging this (if technically sound) still makes sense, because this is one of those things that we will want to offer ASAP with renderer improvements anyway, and being forced to update examples is a good way to do it. On the other hand, a lot depends on where you stand, on a day to day basis, regarding the future renderer. It's pretty hard really contributing there, except maybe doing a spike on bevy_rafx.

@mtsr
Copy link
Contributor Author

mtsr commented Apr 28, 2021

Note that I just added a fairly significant addition, GlobalRenderResourcesNode, which is basically like RenderResourcesNode but for ECS Resources, rather than Components. This was done mainly for purposes of the example, where I wanted to access a value from a Resource just to show the possibility of passing uniforms without resorting to copying from complex code like lights_node.

This should probably be split off before merging. I would love to have some feedback on the general idea, though. Note that it's still more or less a draft. I'm aware it doesn't currently handle textures in the Resource and there might be other gotchas I haven't covered.

Edit: I've now removed GlobalRenderResourcesNode and made a separate PR #2043.

@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@mockersf mockersf removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 24, 2021
@alice-i-cecile
Copy link
Member

@mtsr This is super valuable work, but it's going to be a pain to rebase. What would you like done with this PR?

@mockersf
Copy link
Member

is this still needed after all the changed to the renderer? an example post processing node can be seen here: https://github.com/bevyengine/bevy/blob/main/examples/shader/post_processing.rs

@IceSentry
Copy link
Contributor

I think there might still be value in having a more specialized full screen Node but this PR would need to be completely rewritten from scratch and is indeed less relevant now that we do have post processing support.

@IceSentry IceSentry closed this Oct 1, 2023
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-Enhancement A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants