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

Adds "Write Depth" option to blend and additive material features #2634

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tebjan
Copy link
Member

@tebjan tebjan commented Feb 10, 2025

PR Details

This PR introduces a new "Write Depth" option to the blend and additive material features.

Additionally, I disabled the read-only depth buffer bind when "Bind Depth As Resource During Transparent Rendering" is enabled. This change is necessary to allow materials in the transparent rendering stage to write to the depth buffer. While this might introduce a marginal performance overhead, I expect that the shader recognizes when depth writing is disabled on the material and optimizes accordingly, keeping the same performance as before. Feedback is welcome if anyone has further insights or corrections regarding this assumption.

Related Issue

User request in vvvv to enable more design possibilities.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My change requires a change to the documentation.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have built and run the editor to try this change out.

Screenshot

The following screenshot demonstrates the "Write Depth" feature in action. A transparent sphere interacts with the teapot's geometry, showing the new depth behavior:

image

Here as it was before, which is also the default:

image

(cherry picked from commit 6bbfb45ca6798354bee4521ecc116d98c021cc4c)
@Ethereal77
Copy link
Contributor

Additionally, I disabled the read-only depth buffer bind when "Bind Depth As Resource During Transparent Rendering" is enabled. This change is necessary to allow materials in the transparent rendering stage to write to the depth buffer.

Marginal overhead aside, wouldn't this make more difficult to have custom materials that depend on the depth value? Like, for example, soft particles, or depth-color water, or stencil-based outlines, or effects of that kind? Or are those things better suited for custom render features?

Forgive me if I'm not seeing the full picture.

@johang88
Copy link
Contributor

I don't think it's allowed to bind the depth as input while also having it bound as the active render target. ResolveDepthStencil should probably be modified to always return a copy of the depth buffer if this is to be implemented.

@tebjan
Copy link
Member Author

tebjan commented Feb 13, 2025

@Ethereal77 @johang88

The functionality of "Bind Depth As Resource During Transparent Rendering" remains the same, with a copy of the depth buffer generated regardless of the boolean's state.

What I have removed is the following: Before my PR, when this boolean was enabled, the depth buffer was copied into a read-only texture and bound like that to the pipeline. It now simply stays bound.

if (!BindDepthAsResourceDuringTransparentRendering)

The copy is done here:

public Texture ResolveDepthStencil(Texture texture)

@johang88 do you think there could be a case where ResolveDepthStencil would falsely not make a copy?

@johang88
Copy link
Contributor

@johang88 do you think there could be a case where ResolveDepthStencil would falsely not make a copy?

Yes, on my GPU it will never return a copy as if (renderContext.GraphicsDevice.Features.HasDepthAsReadOnlyRT) always returns true which results in it returning the original depth buffer as no copy has to be made when it was bound as a read only view.

@Ethereal77
Copy link
Contributor

To clarify (please correct me if I'm wrong, as I've not tried to touch that part of the renderer yet):

  • Before: The opaque render stage binds color+depth buffer as render targets, draws the opaque geometry there. The transparent stage continues to use the color+depth buffers as render targets, but disables depth write. An option exists to copy the depth buffer to a texture and bind it as read-only depth view.

  • After: The opaque render stage binds color+depth buffer as render targets, draws the opaque geometry there, the same as before. The transparent stage continues to use the color+depth buffers as render targets, but now you can decide whether to enable/disable depth write. The option to copy the depth buffer to a texture and bind it as read-only depth view continues to exist in case some effect want to use it.

Is it somethink like that?

WRT to the HasDepthAsReadOnlyRT, I think Johang is right. For graphics profile 11+, it is always true, so the original (still bound) depth buffer is used as read-only view, and no copy is made.
Maybe now a copy should be made also when depth writes are enabled?

My original concern persists, however. Transparent shaders with depth writes can, in any case, read only the opaque depth information, isn't it?

Sorry if I'm talking nonsense. I've yet to study your changes and this part of ForwardRenderer.

@johang88
Copy link
Contributor

It's correct that transparent shaders can only every read from the depth written in the opaque pass. With this change a copy must always be made as the depth buffer is always bound as a read/write render target, if a copy is not made then it the depth buffer will not be able to be bound as texture for reading in the transparent stage shaders.

Note that the actual behaviour of this might be driver dependant as I am unsure if it's undefined behaviour or not. But I did check it myself in renderdoc and at least on my system the texture does not get bound properly without forcing a copy to be made. I also noticed that ReleaseDepthStenctilAsShaderResource has to be modified so that the temporary texture created for the copy is actually returned released at the end of the frame.

No copy made, can't be bound to texture slot:
image

If copy is made, then it works correctly:
image

@tebjan
Copy link
Member Author

tebjan commented Feb 15, 2025

@johang88 thanks for your thoughts and investigation on the texture behavior.

@Ethereal77 I'll review the code with the new information and will try to clarify the before/after behavior when I'm done.

Really helpful, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants