Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 a post-processing example #4797
[Merged by Bors] - add a post-processing example #4797
Changes from 10 commits
7a596f1
d4de0ed
c8cdd43
e20297f
edbae48
92d1ba9
32b7a09
3de0f90
6b69e18
c579c55
d4af858
ffa5447
45072c9
af177dd
ca1fb9a
6cd96e9
5f14b10
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a quad is fine but also suboptimal. A fullscreen triangle generated from vertex indices would be better, though drawing something which has no mesh entity is a bit more tricky I would think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought fullscreen triangle meant a triangle mesh covering all screen (so having corners outside of the screen). I guess I could modify the quad mesh into a triangle, compute its correct size and adapt uv values (or shader).
Your comment about “no mesh entity” makes me wonder if you meant something different ? A geometry shader ? Oh a compute shader applied to the render without needing a cpu mesh..? Sounds less trivial indeed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By fullscreen triangle, I mean a triangle with one corner in one of the corners of the screen, one at the same y and twice the width, and one at the same x and twice the height. The uvs at those vertices are similarly 0,0, 2,0, and 0,2. This makes it so that the triangle just covers the screen and has correct uvs. But, these can be generated in the vertex shader from the vertex index alone so no vertex buffer is needed, just a draw command drawing indices 0..3 and then the vertex shader clip position is output as:
Untested at this time of writing but that should do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can / should we add a explicit short hand for this? It seems like a common pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggested on Discord that we add a fullscreen pass node that can have its pipeline’s fragment shader stage and bind groups specialised, and that requires no mesh entity.
I think for this PR just making a triangle mesh with index buffer containing 0,1,2, overriding the vertex stage with the above and calculating the uvs in the fragment stage should do it.
A note on the why - I’ve seen articles about gaining significant % performance from using a fullscreen triangle or compute shader because a quad has two triangles, each with a diagonal cutting through the screen which causes duplicate fragment shader invocations because of fragments being shaded in 2x2 pixel quads which overlap the boundary. Maybe msaa also gets involved and makes the situation worse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. We can get this in, and then watch it improve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I try this solution ? And try to benchmark the differences ? Maybe a different PR to make it clear we're not sure the added boilerplate is worth it (if we don't have shortcut available in engine).
Or would it be considered non blocking, add a documentation line about this concept, create an issue for future improvement ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My personal feeling is that this is nonblocking, and just needs an issue opened.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with that approach. If it were for internal bevy post-processing, I would block on it. But for an example, it's good enough.