Add FullscreenMaterial #20414
Add FullscreenMaterial #20414
Conversation
alice-i-cecile
added
the
M-Release-Note
alice-i-cecile
added
C-Feature
|
It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note. Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes. |
alice-i-cecile
added
S-Needs-Review
IceSentry
force-pushed
the
fullscreen_material
branch
from
b0f9763 to
70e61bd
Compare
|
As a 2D user, when I first read “ I’d lean into the |
|
I disagree the main goal of this api is to be a general purpose fullscreen render pass. The main feature is that it can be inserted in any order because some people don't want a post process effect, they just want to do something like render their entire game in a fullscreen shader in the main pass. Like I mentioned in the notes. A post processing version of this would be built on top of the same api but with pre configured ordering. |
|
does it make it easy to render the game to a small resolution and then upscale? |
Not really in it's current state since it doesn't let you bind arbitrary textures to it. Eventually I do want to let users set a full bind group so that would let users bind a low res texture and then just write a small shader to sample it and it will be "upscaled" that way. |
HugoPeters1024
left a comment
HugoPeters1024
left a comment
There was a problem hiding this comment.
Amazing work! This is super helpful <3
tychedelia
left a comment
tychedelia
left a comment
There was a problem hiding this comment.
A few comments, but they can also be improved in a 0.18 follow up. I think that we should push to move this justtt a bit closer to other materials, i.e. by allowing AsBindGroup. But this is an awesome footprint to establish. Happy to land this right at the beginning of 0.18 and iterate. Tysm for doing this!
| /// ] | ||
| /// ``` | ||
| /// | ||
| /// This tell the render graph to run your fullscreen effect after the tonemapping pass but |
There was a problem hiding this comment.
I think it's correct? It's just explaining the code snippet just above it.
|
We should consider copying some of the code from RenderTask #21603. We should probably have the same node setup across both APIs. |
|
Alright, I did a bit more work on this PR. The node_label is now optional and we use the type name as the default label. The sub_graph is also optional and we try to guess if it should go on a 3d or 2d camera depending on how it was spawned. I could make the node order optional too and default it to post processing but I'm not sure yet if I want to do that. The next step is to figure out how to use AsBindGroup to let users provide custom bind groups. I would also like to make the default bindings in group 0 include the depth texture if there is one. I could also add the Globals to that group. Maybe it should just include the same bindings as a mesh material. In the future I'm also tempted to make it fully optional if someone simply wants a fullscreen shader that ignores the other passes. |
If users want custom bindings, RenderTask might make more sense for them. Imo this PR should stick to a preset bind group of just the depth/globals/whatever. |
|
I think the main issue is that I fully expect users to want to have access to at least one texture or storage buffer otherwise this is a very limiting api. Also, I'm trying to make this as flexible as possible and to be more general purpose than just post processing. |
| ); | ||
|
|
||
| // We can't use add_render_graph_edges because it doesn't accept a Vec<RenderLabel> | ||
| if let Some(mut render_graph) = render_app.world_mut().get_resource_mut::<RenderGraph>() |
There was a problem hiding this comment.
Did you see what I did in #21603 ? I think we should adopt those changes.
| warn!("Failed to add edges for FullscreenMaterial"); | ||
| }; | ||
| } else { | ||
| // If there was no sub_graph specify we try to determine the graph based on the camera |
There was a problem hiding this comment.
| // If there was no sub_graph specify we try to determine the graph based on the camera | |
| // If there was no sub_graph specified we try to determine the graph based on the camera |
| ShaderRef::Default => { | ||
| // TODO not sure what an actual fallback should be. An empty shader or output a solid | ||
| // color to indicate a missing shader? | ||
| unimplemented!("No default fallback for FullscreenMaterial shader") |
There was a problem hiding this comment.
| unimplemented!("No default fallback for FullscreenMaterial shader") | |
| unimplemented!("FullscreenMaterial::fragment_shader() must not return ShaderRef::Default") |
8 participants
Objective
Solution
Testing
Made a new fullscreen_material example and made sure it works
Follow up
Once this is merged there are various things that should be done to improve it. Add the option to bind the depth texture, offer defaults for post processing, use a full AsBindGroup, add a way to bind the gbuffer.