-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Implement opt-in sharp screen-space reflections for the deferred renderer, with improved raymarching code. #13418
Conversation
renderer. This commit implements *screen-space reflections* (SSR), which approximate real-time reflections based on raymarching through the depth buffer and copying samples from the final rendered frame. Numerous variations and refinements to screen-space reflections exist in the literature. This patch foregoes all of them in favor of implementing the bare minimum, so as to provide a flexible base on which to customize and build in the future. For a general basic overview of screen-space reflections, see [1]. The raymarching shader uses the basic algorithm of tracing forward in large steps (what I call a *major* trace), and then refining that trace in smaller increments via binary search (what I call a *minor* trace). No filtering, whether temporal or spatial, is performed at all; for this reason, SSR currently only operates on very shiny surfaces. No acceleration via the hierarchical Z-buffer is implemented (though note that bevyengine#12899 will add the infrastructure for this). Reflections are traced at full resolution, which is often considered slow. All of these improvements and more can be follow-ups. SSR is built on top of the deferred renderer and is currently only supported in that mode. Forward screen-space reflections are possible albeit uncommon (though e.g. *Doom Eternal* uses them); however, they require tracing from the previous frame, which would add complexity. This patch leaves the door open to implementing SSR in the forward rendering path but doesn't itself have such an implementation. Screen-space reflections *are* supported in WebGL 2. To add screen-space reflections to a camera, use the `ScreenSpaceReflections` component. `DepthPrepass` and `DeferredPrepass` must also be present for the reflections to show up. The `ScreenSpaceReflections` component contains several settings that artists can tweak, and also comes with sensible defaults. A new example, `ssr`, has been added. It's loosely based on the [three.js ocean sample], but all the assets are original. Note that the three.js demo has no screen-space reflections and instead renders a mirror world. Additionally, this patch fixes a random bug I ran across: that the `"TONEMAP_METHOD_ACES_FITTED"` `#define` is incorrectly supplied to the shader as `"TONEMAP_METHOD_ACES_FITTED "` (with an extra space) in some paths. [1]: https://lettier.github.io/3d-game-shaders-for-beginners/screen-space-reflection.html [three.js ocean sample]: https://threejs.org/examples/webgl_shaders_ocean.html
`SCREEN_SPACE_REFLECTIONS`
This fixes numerous artifacts.
I made an attempt to get things working on WebGL 2 but hit a wall due to our old friend, the Naga bug whereby |
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.
This looks great! Some minor nits, plus I'd mention water in the example description, as I dont think we have other examples with it and its a valuable thing to show
|
||
/// Jitter to apply to the first step of the linear search; 0..=1 range, mapping | ||
/// to the extent of a single linear step in the first phase of the search. | ||
/// Use 1.0 if you don't want jitter. |
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.
this is a bit unintuitive, why isnt 0.0 no jitter? i understand this is a straight port, just curious
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.
jitter
is really "length of first step". So no jitter would be 1.
/// compress the earlier steps, and expand the later ones. This might be | ||
/// desirable in order to get more detail close to objects. | ||
/// | ||
/// For optimal performance, this should be a small unsigned integer, such |
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.
this is not compile time expanded like the other comment suggests, is this optimization future work?
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 don't think our shader_defs
allow for #define
s with values, do they? So I don't think I have the option to do that.
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.
if only 1 or 2 is ever used, it could be a simple gate, although it does feel a bit ugly. im fine with merge as-is regardless, this should be noted somewhere though
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 not sure the decrease in maintainability is worth it. Do you want me to do performance tests?
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.
shader_defs can have values from the cpu side. We also support this syntax inside a shader file #define PI 3.1416
and you need to use it with #{PI}
.
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.
not particularly, there's better things to do with your time :)
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.
Actually, in the original raymarch.hlsl
this value isn't a compile time constant: https://gist.github.com/h3r2tic/9c8356bdaefbe80b1a22ae0aaee192db#file-raymarch-hlsl-L554
So I don't think it matters. I'll leave it as is unless there are objections.
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.
Awesome work. Other than the ray-marching implementation it all looks straight-forward. I feel slightly uneasy because I couldn't find anything to comment on in the code. 😅
The only technically breaking change I can see here is the slight rejigging of the bindings.
I had a look at the original raymarch.hlsl
.
Thanks for working on this. 🙂
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.
The transmission example is broken because of the changes in pbr_functions.
Should be fixed now. |
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 added a few tiny nits, but I haven't found any other bugs and everything seems to work as expected now. So LGTM.
(*root_finder).min_t, | ||
(*root_finder).max_t, | ||
pow( | ||
(*root_finder).jitter / f32((*root_finder).linear_steps), |
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.
The original does 0 + jitter
here. I'm not really sure what that's about or if it's necessary.
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 guessed it was probably just leftovers from some tweaking that the original author did. Like, probably 0.0 was 0.1 or something at some point, and it eventually got changed to 0.0 instead of being removed. I just deleted the term entirely.
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.
Yeah, makes sense
Addressed review comments. |
# Objective - #13418 introduced unwanted changes to the meshlet example ## Solution - Remove them
# Objective - #13418 broke volumetric fog ``` wgpu error: Validation Error Caused by: In a RenderPass note: encoder = `<CommandBuffer-(2, 4, Metal)>` In a set_bind_group command note: bind group = `mesh_view_bind_group` Bind group 0 expects 5 dynamic offsets. However 4 dynamic offsets were provided. ``` ## Solution - add ssr offset to volumetric fog bind group
This commit, a revamp of #12959, implements screen-space reflections (SSR), which approximate real-time reflections based on raymarching through the depth buffer and copying samples from the final rendered frame. This patch is a relatively minimal implementation of SSR, so as to provide a flexible base on which to customize and build in the future. However, it's based on the production-quality raymarching code by Tomasz Stachowiak.
For a general basic overview of screen-space reflections, see 1. The raymarching shader uses the basic algorithm of tracing forward in large steps, refining that trace in smaller increments via binary search, and then using the secant method. No temporal filtering or roughness blurring, is performed at all; for this reason, SSR currently only operates on very shiny surfaces. No acceleration via the hierarchical Z-buffer is implemented (though note that #12899 will add the infrastructure for this). Reflections are traced at full resolution, which is often considered slow. All of these improvements and more can be follow-ups.
SSR is built on top of the deferred renderer and is currently only supported in that mode. Forward screen-space reflections are possible albeit uncommon (though e.g. Doom Eternal uses them); however, they require tracing from the previous frame, which would add complexity. This patch leaves the door open to implementing SSR in the forward rendering path but doesn't itself have such an implementation. Screen-space reflections aren't supported in WebGL 2, because they require sampling from the depth buffer, which Naga can't do because of a bug (
sampler2DShadow
is incorrectly generated instead ofsampler2D
; this is the same reason why depth of field is disabled on that platform).To add screen-space reflections to a camera, use the
ScreenSpaceReflectionsBundle
bundle or theScreenSpaceReflectionsSettings
component. In addition toScreenSpaceReflectionsSettings
,DepthPrepass
andDeferredPrepass
must also be present for the reflections to show up. TheScreenSpaceReflectionsSettings
component contains several settings that artists can tweak, and also comes with sensible defaults.A new example,
ssr
, has been added. It's loosely based on the three.js ocean sample, but all the assets are original. Note that the three.js demo has no screen-space reflections and instead renders a mirror world. In contrast to #12959, this demo tests not only a cube but also a more complex model (the flight helmet).Changelog
Added
ScreenSpaceReflections
component to a camera. Deferred rendering must be enabled for the reflections to appear.