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

Implement opt-in sharp screen-space reflections for the deferred renderer, with improved raymarching code. #13418

Merged
merged 26 commits into from
May 27, 2024

Conversation

pcwalton
Copy link
Contributor

@pcwalton pcwalton commented May 18, 2024

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 of sampler2D; 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 the ScreenSpaceReflectionsSettings component. In addition to ScreenSpaceReflectionsSettings, DepthPrepass and DeferredPrepass must also be present for the reflections to show up. The ScreenSpaceReflectionsSettings 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

  • Screen-space reflections can be enabled for very smooth surfaces by adding the ScreenSpaceReflections component to a camera. Deferred rendering must be enabled for the reflections to appear.

Screenshot 2024-05-18 143555

Screenshot 2024-05-18 143606

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
This fixes numerous artifacts.
@alice-i-cecile alice-i-cecile added C-Enhancement A new feature A-Rendering Drawing game state to the screen C-Needs-Release-Note Work that should be called out in the blog due to impact labels May 18, 2024
@pcwalton
Copy link
Contributor Author

I made an attempt to get things working on WebGL 2 but hit a wall due to our old friend, the Naga bug whereby sampler2D is generated instead of sampler2DShadow. So I'm disabling on WebGL 2 and marking as ready for review.

@pcwalton pcwalton marked this pull request as ready for review May 18, 2024 01:49
@pcwalton pcwalton requested review from IceSentry and JMS55 May 18, 2024 01:49
Copy link
Contributor

@atlv24 atlv24 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 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

examples/3d/ssr.rs Outdated Show resolved Hide resolved

/// 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.
Copy link
Contributor

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

Copy link
Contributor Author

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
Copy link
Contributor

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?

Copy link
Contributor Author

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 #defines with values, do they? So I don't think I have the option to do that.

Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor

@IceSentry IceSentry May 20, 2024

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}.

Copy link
Contributor

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 :)

Copy link
Contributor Author

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.

crates/bevy_pbr/src/render/pbr_functions.wgsl Show resolved Hide resolved
crates/bevy_core_pipeline/src/core_3d/mod.rs Outdated Show resolved Hide resolved
@pcwalton pcwalton added this to the 0.15 milestone May 20, 2024
@pcwalton pcwalton added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label May 20, 2024
Copy link
Contributor

@ricky26 ricky26 left a 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. 🙂

@pcwalton pcwalton added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 25, 2024
Copy link
Contributor

@IceSentry IceSentry left a 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.

@pcwalton
Copy link
Contributor Author

Should be fixed now.

Copy link
Contributor

@IceSentry IceSentry left a 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.

crates/bevy_pbr/src/meshlet/material_draw_prepare.rs Outdated Show resolved Hide resolved
crates/bevy_pbr/src/ssr/mod.rs Outdated Show resolved Hide resolved
crates/bevy_pbr/src/ssr/mod.rs Show resolved Hide resolved
crates/bevy_pbr/src/ssr/raymarch.wgsl Outdated Show resolved Hide resolved
(*root_finder).min_t,
(*root_finder).max_t,
pow(
(*root_finder).jitter / f32((*root_finder).linear_steps),
Copy link
Contributor

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.

Copy link
Contributor Author

@pcwalton pcwalton May 26, 2024

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, makes sense

@pcwalton
Copy link
Contributor Author

Addressed review comments.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 27, 2024
Merged via the queue into bevyengine:main with commit f398674 May 27, 2024
31 checks passed
github-merge-queue bot pushed a commit that referenced this pull request May 27, 2024
# Objective

- #13418 introduced unwanted changes to the meshlet example

## Solution

- Remove them
github-merge-queue bot pushed a commit that referenced this pull request May 27, 2024
# 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
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 C-Needs-Release-Note Work that should be called out in the blog due to impact S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants