bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Add depth and normal prepass

Open IceSentry opened this issue 3 years ago • 8 comments

Objective

  • Add a configurable prepass
  • A depth prepass is useful for various shader effects and to reduce overdraw. It can be expansive depending on the scene so it's important to be able to disable it if you don't need any effects that uses it or don't suffer from excessive overdraw.
  • The goal is to eventually use it for things like TAA, Ambient Occlusion, SSR and various other techniques that can benefit from having a prepass.

Solution

The prepass node is inserted before the main pass. It runs for each Camera with a PrepassSettings that you can use to configure if it runs a depth prepass, a normal prepass, or both. When the depth prepass is enabled, it will be used by the main pass.

The prepass runs for each Material created with the MaterialPlugin::prepass_enabled option set to true. You can overload the shader used by the prepass by using Material::prepass_vertex_shader() and/or Material::prepass_fragment_shader(). It will also use the Material::specialize() for more advanced use cases.

The prepass works on opaque materials and materials using an alpha mask. Transparent materials are ignored.

The StandardMaterial overloads the prepass fragment shader to support alpha mask and normal maps.


Changelog

  • Add a new PrepassNode that runs before the main pass
  • Add a PrepassPlugin to extract/prepare/queue the necessary data
  • Add a PrepassSettings to control which textures will be created by the prepass and available in later passes.
  • Add a new prepass_enabled flag to the MaterialPlugin that will control if a material uses the prepass or not.
  • Add a new prepass_enabled flag to the PbrPlugin to control if the StandardMaterial uses the prepass. Currently defaults to false.
  • Add Material::prepass_vertex_shader() and Material::prepass_fragment_shader() to control the prepass from the Material

Notes

In bevy's sample 3d scene, the performance is actually worse when enabling the prepass, but on more standard scenes with complex models the performance is generally better. I would like more testing on this, but @DGriffin91 has reported a very noticeable improvements in some scenes.

The prepass is also used by @JMS55 for TAA.

discord thread: https://discord.com/channels/691052431525675048/1011624228627419187

This PR was built on top of the work of multiple people

Co-Authored-By: @superdump Co-Authored-By: @robtfm Co-Authored-By: @JMS55

IceSentry avatar Oct 17 '22 22:10 IceSentry

A small request: Vary the pipeline labels based on opaque/transparent, similar to how MeshPipeline does it. Currently in Renderdoc/NSight, the prepass shows up as "prepass" twice.

JMS55 avatar Oct 22 '22 03:10 JMS55

Something I've seen people (myself included) wanting is a way to sample the depth prepass, converting it back to a linear value (if I understand the conversions correctly). Would be nice to have with this PR.

JMS55 avatar Nov 11 '22 00:11 JMS55

So, I just learned that unity only applies the depth prepass to meshes that are also shadow caster. https://docs.unity3d.com/Manual/SL-CameraDepthTexture.html should we also do this for the prepass? Or at least make it an option? I'm not entirely sure of the reason behind that decision. It seems to me like doing it for all opaque mesh should be fine.

IceSentry avatar Nov 11 '22 18:11 IceSentry

How do you feel about changing the PbrPlugin prepass_enabled to true by default, and leaving PrepassSettings off of Camera3dBundle? Or maybe adding it, but making the default all outputs false.

JMS55 avatar Nov 11 '22 18:11 JMS55

So, essentially that would result in the prepass being enabled for the StandardMaterial but disabled by default on any other material? Is it for the goal of having it enabled to build stuff on top of it like taa/ao?

I'd still like to benchmark it a bit more but I don't mind enabling it by default. I just don't want people to launch the 3d_example and wonder why their fps is lower than it was when the prepass wasn't a thing, but that's probably not that big of a deal.

IceSentry avatar Nov 11 '22 19:11 IceSentry

Ah I forgot it was a per-material toggle, and the PbrPlugin just controls the StandardMaterial iirc? Maybe we should rename the field to standard_material_prepass?

But yeah, when testing AO atm, it's annoying to have to remember both to add the PrepassSettings (the default has normals off, I need both depth and normals), and then set it enabled on the PbrPlugin.

I'm not sure how I feel, but maybe it would be better to remove PrepassSettings from the Camera3dBundle, and turn the PbrPlugin toggle to true.

JMS55 avatar Nov 11 '22 19:11 JMS55

The way I see it, the PbrPlugin is essentially the StandardMaterialPlugin, so naming it standard_material_prepass feels a bit redundant to me.

As for removing the PrepassSettings, I'm not sure how that would work because we still need to make this configurable and with camera driven rendering this needs to be controlled per camera.

IceSentry avatar Nov 11 '22 19:11 IceSentry

The way I see it, the PbrPlugin is essentially the StandardMaterialPlugin, so naming it standard_material_prepass feels a bit redundant to me.

Ah, but when we add AO, it becomes more than that. So it would be confusing if you had a custom material, and toggled the PbrPlugin prepass to on (thinking it would let you use AO for your custom material), only to realize that toggle only applies to the StandardMaterial. Again, not sure how I feel.

As for removing the PrepassSettings, I'm not sure how that would work because we still need to make this configurable and with camera driven rendering this needs to be controlled per camera.

I didn't mean remove it entirely (I actually really like this way of controlling the effects). Just remove it from the camera 3d bundle, so that it's always disabled unless you add it yourself.

JMS55 avatar Nov 11 '22 19:11 JMS55

So, I just learned that unity only applies the depth prepass to meshes that are also shadow caster. https://docs.unity3d.com/Manual/SL-CameraDepthTexture.html should we also do this for the prepass? Or at least make it an option? I'm not entirely sure of the reason behind that decision. It seems to me like doing it for all opaque mesh should be fine.

I suppose if the depth buffer is used for ambient occlusion, perhaps objects that are not shadow casters should also not occlude ambient light as that is practically a kind of shadow casting. It's a good question though!

superdump avatar Dec 04 '22 12:12 superdump

So, essentially that would result in the prepass being enabled for the StandardMaterial but disabled by default on any other material? Is it for the goal of having it enabled to build stuff on top of it like taa/ao?

I'd still like to benchmark it a bit more but I don't mind enabling it by default. I just don't want people to launch the 3d_example and wonder why their fps is lower than it was when the prepass wasn't a thing, but that's probably not that big of a deal.

This is a tough call. It will likely kill performance on mobile in more situations than it would on desktop. And, until we have good batching/instancing, the two full passes over meshes means double the draw commands. There are some definite tradeoffs and my gut says that maybe having it disabled by default until we have good batching and instancing is probably a better call, but that is not based on real-world testing.

superdump avatar Dec 04 '22 12:12 superdump

But yeah, when testing AO atm, it's annoying to have to remember both to add the PrepassSettings (the default has normals off, I need both depth and normals), and then set it enabled on the PbrPlugin.

I'm not sure how I feel, but maybe it would be better to remove PrepassSettings from the Camera3dBundle, and turn the PbrPlugin toggle to true.

If we can do things per-view rather than globally, I think we should. In general, I expect more things to move from being global to per-view than the other way around. We can add convenience on top of that to set a global view default and then you can either control all at once, or just deviate with the exceptions.

superdump avatar Dec 04 '22 12:12 superdump

The way I see it, the PbrPlugin is essentially the StandardMaterialPlugin, so naming it standard_material_prepass feels a bit redundant to me.

Ah, but when we add AO, it becomes more than that. So it would be confusing if you had a custom material, and toggled the PbrPlugin prepass to on (thinking it would let you use AO for your custom material), only to realize that toggle only applies to the StandardMaterial. Again, not sure how I feel.

Hmm... perhaps if the PbrPlugin has the prepass enabled, but a custom material doesn't support the prepass, there could be a way of warning about it. So custom materials should default to opting into wanting prepass support. So if the vertex shader was overridden and the prepass is enabled (it would have to be part of the material plugin? I haven't yet reviewed the PR - is it? I'll check soon) then the prepass vertex shader must also be overridden else a warning message would be issued perhaps when extracting/preparing the material asset or something?

superdump avatar Dec 04 '22 12:12 superdump

it would have to be part of the material plugin? is it?

Yes, it's part of the material plugin. To be more precise, it's a separate plugin, but it's inserted per-material when using the material plugin.

the prepass vertex shader must also be overridden else a warning message would be issued

Right now there isn't any warnings when not overriding a stage, but I'll see how easy it would be to check if the default vertex stage is overridden and to warn that the vertex prepass should probably be overridden too.

IceSentry avatar Dec 06 '22 19:12 IceSentry

My approval should only be weakly weighted: this is for docs and code quality only.

alice-i-cecile avatar Dec 06 '22 21:12 alice-i-cecile

Further thoughts on how to configure the prepass, after using it for a few months:

There's two different toggles for the prepass, and both must be on to work:

  1. Per material
  2. Per camera

When it comes to per material, I strongly think the prepass should be enabled by default for every material, including the standard pbr material, and custom materials (although it is currently tricky to automatically insert the necessary code into custom materials).

It's far too easy to forget to enable the prepass for a material, and then wonder why certain effects aren't working. I'd like to have as low friction as possible to use prepass-dependent effects. Opting out of the prepass per-material should be a considered choice - not the default.

When it comes to per camera, I think it should be off by default (as it is now), but be a separate marker component and not a field on Camera as it currently is. Furthermore, it should be separate components per prepass-subpass (depth, normal, velocity, etc).

The reason is, this leads to the cleanest and easiest UX for using prepass-dependent effects. All that would be needed would be to add a component bundle to enable the effect itself (e.g., SSAOBundle), which would then bundle the required prepass component. Furthermore, using multiple prepass-dependent effects becomes conflict free. If TAA requires depth + velocity, and SSAO requires depth + normals, then each bundle could have the two components it needs, and adding both bundles would lead to all parts of the prepass being enabled without conflict.

JMS55 avatar Dec 24 '22 03:12 JMS55

Alright, based on that and what @robtfm said, I'll switch to using separate components for each prepass element and consider the presence of that component as enabling it. I don't really have any strong opinions for the current approach and it clearly has some issues.

IceSentry avatar Dec 24 '22 03:12 IceSentry

We should also derive Default for the prepass components, and probably Reflect and register them.

JMS55 avatar Dec 25 '22 05:12 JMS55

@JMS55 the prepass is now enabled by default on all Materials and the PbrPlugin, so as long as the appropriate components are present everything should work. I also added the Default and Reflect derive.

IceSentry avatar Dec 25 '22 19:12 IceSentry

I like the composability for effects to be able to express what prepass data they need based on inserting components on the camera entity.

superdump avatar Dec 27 '22 14:12 superdump

I like the composability for effects to be able to express what prepass data they need based on inserting components on the camera entity.

Note that we discovered this doesn't quite work, unfortunately. Trying to insert two bundles in the same spawn() that use the same components won't work, even if they're marker components as the prepass components are :(. You can chain inserts() though, and it's still nice overall. Maybe in the future we can allow spawning bundles with the same components if they're marker components.

JMS55 avatar Dec 27 '22 16:12 JMS55

One thing I thought of after having read through everything is that the standard material opaque/alpha mask main pass could also make use of the normals from the prepass if they're available, rather than recalculating them. That could also be a follow-up PR though. I don't want to hold this up on things that could be done separately.

Yeah, I thought about that yesterday. It would indeed make sense to reuse normals, but at this point in the review process I'd prefer doing it in a separate PR.

IceSentry avatar Dec 28 '22 17:12 IceSentry

8e3c28b has broken viewports. i tried adding a viewport to the shader_prepass example and got a variety of weird effects. reverted this commit and it all works fine.

robtfm avatar Jan 16 '23 11:01 robtfm

Huh, really weird. I thought it wouldn't make a difference. Leave it in I guess then.

JMS55 avatar Jan 16 '23 15:01 JMS55

Build failed:

bors[bot] avatar Jan 18 '23 18:01 bors[bot]

Synchronization error!

bors[bot] avatar Jan 18 '23 19:01 bors[bot]

bors r+

superdump avatar Jan 18 '23 19:01 superdump

Build failed:

  • run-examples-on-wasm

bors[bot] avatar Jan 18 '23 20:01 bors[bot]

bors try

IceSentry avatar Jan 19 '23 00:01 IceSentry

bors try

IceSentry avatar Jan 19 '23 22:01 IceSentry