Skip to content

[NOM-5453] Fix depth handling when chaining passes with different sample counts - #195

Open
LukPopp0 wants to merge 4 commits into
mainfrom
poppl/skip-depth-copy-on-mismatch
Open

[NOM-5453] Fix depth handling when chaining passes with different sample counts#195
LukPopp0 wants to merge 4 commits into
mainfrom
poppl/skip-depth-copy-on-mismatch

Conversation

@LukPopp0

@LukPopp0 LukPopp0 commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Description

A buffer that was freshly allocated and then got no copy has no initialization at all, so the depth AOV takes its default clear value in that one case. The clear is additionally gated on this pass owning an output buffer for that AOV, because the mismatch and the allocation are decided by different conditions: the allocation is skipped when the input bindings and output tokens are unchanged, so without the ownership test the clear would land on the buffer still shared with the previous pass.

In RenderBufferManager::Impl::SetRenderOutputs, the same-renderer test is hoisted into its own local so the depth decision can be stated directly:

const bool sameRenderer = (rendererName == input.rendererName);
const bool multisampleMismatch =
    input.buffer && (input.buffer->IsMultiSampled() != desc.multiSampled);
inputFound = sameRenderer && !multisampleMismatch;
...
if (sameRenderer)
{
    depthInput.texture = HgiTextureHandle();
    clearFreshDepth    = !inputFound;
}

A buffer that was freshly allocated and then got no copy has no initialization at all, so the depth AOV takes its default clear value in that one case:

 HdRenderBuffer* outputBuffer = static_cast<HdRenderBuffer*>(
     _pRenderIndex->GetBprim(HdPrimTypeTokens->renderBuffer, aovId));

 const bool clearThisAov = !foundInput.buffer ||
     (localOutputs[i] == HdAovTokens->depth && clearFreshDepth && outputBuffer);

Changes Made

  • When the renderer is the same, never copy the chained depth into the pass's own buffer. A reused buffer already holds valid depth, and on a multisample mismatch a fresh buffer is allocated, so the copy would only flatten sub-pixel depth.
  • Clear that fresh depth buffer. It is the one case where a buffer receives neither a copy nor pre-existing depth, so without the clear the pass depth-tests against undefined contents on the first frame and after every resize.
  • Add tests for the various cases or matching or mismatching multisampling values between passes (state comparison, no image comparison).
  • Only clear a buffer the pass owns. The mismatch that sets the clear and the allocation of the fresh buffer are gated on different conditions, so a pass that toggles its own MSAA setting while its bindings stay identical would otherwise clear the depth buffer it is still sharing with the previous pass.
  • Document the buffer sharing rules in docs/renderbuffermgr.md, which described reuse unconditionally and mentioned neither the same-renderer requirement introduced by [NOM-5453] Allocate fresh depth buffer when pass's sample count differs from the chained input #184 nor the matching-sample-count requirement.
  • Add a FIXME next to the sameRenderer test recording that chaining across render delegates still copies the depth.

Behavior change

In the same-renderer, mismatched-sample-count case the previous pass's depth is now discarded rather than copied forward, so geometry in the second pass is no longer occluded by geometry from the first. This is intended for overlay passes such as the ViewCube, and is what docs/renderbuffermgr.md now describes.

Known limitations

  • Chaining across render delegates still copies the depth, because that copy is the only mechanism carrying depth between them. It therefore still binds depth as a sampled texture and can still fail on WebGPU. Marked with a FIXME in the code.
  • A pass that toggles its own MSAA setting while its input bindings and output tokens stay identical keeps sharing a buffer whose sample count no longer matches its attachments. This PR stops the destructive clear on that path but does not fix the sharing; that needs the multisample state folded into the somethingChanged comparison.

Testing

Test Configuration

  • Platform(s) tested: macOS 15
  • Build configuration(s): Debug/Release
  • Render delegate(s): Storm (Metal)
  • OpenUSD version: 26.05

Tests Performed

  • Existing unit tests pass
  • Added/Updated unit test(s) for the changes
  • Tested on multiple platforms
  • Tested with different render delegates
  • Performance testing (if applicable)

Documentation

  • Code is self-documenting / well-commented
  • Public API changes are documented with Doxygen comments

Checklist

  • I have signed the Contributor License Agreement (CLA) (Corporate or Individual)
  • My code follows the project's coding standards
  • My changes generate no new warnings or errors

Lukas Popp added 2 commits August 17, 2026 12:38
The generalized sample-count fix (#184) allocates a fresh buffer on a mismatch but then lets _PrepareBuffersFromInputs copy the resolved depth into it. That copy binds the depth texture as a float-sampled texture in the fullscreen copy shader, which WebGPU rejects (depth is Depth/UnfilterableFloat) - producing an invalid bind group and a blank frame. Metal tolerates it, so it only broke on web.

Skip the depth copy on a mismatch, as the original depth-specific change did: the fresh MSAA depth buffer is left uninitialised and the overlay clears it before drawing. Color priming is unaffected.
Adds TestMsaaChainedPassResolvedToMsaa, TestMsaaChainedPassMsaaToResolved and TestMsaaChainedPassMatchingSampleCount to cover the render buffers a frame pass ends up drawing into when it is chained after a pass with a different multisample state.

The tests assert the buffer allocation rather than comparing images, so they need no baselines: attaching buffers of differing sample counts is invalid, but backends disagree on whether they reject it, tolerate it, or render something plausible. Each mismatch case checks that the second pass allocated its own buffers at its own sample count, and the matching case guards against the mismatch detection allocating buffers the passes could have shared.
Skipping the copy left the freshly allocated depth buffer with no
initialisation at all, so the chained pass depth-tested against
undefined contents on the first frame and after every resize. Give the
depth AOV its default clear value in exactly that case: a fresh buffer
that received neither a copy nor pre-existing depth.

Also narrow the skip back to the same-renderer case. With the clear in
place, the multisample-mismatch term only affected chaining across
different render delegates, where the resolved copy remains the only
mechanism carrying depth between them.

Minor test tidy-ups: drop a dead initialiser and a tautological
assertion in testMultiSampling.cpp.
@LukPopp0
LukPopp0 requested a review from hodoulp August 20, 2026 14:03
@hodoulp

hodoulp commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

As human reviewer, the code is fine & the logic is improved and unit tests validate the fix. That's fine.
Using coding models, one mentioned that everything is fine. Another mentioned a potential bug, I'm currently digging in the explanation i.e., "the new depth clear can be applied to a buffer this pass doesn't own" & "Undocumented new behavoir". Stay tuned.

The clear added for the skipped depth copy was driven by the multisample
mismatch alone, but the fresh buffer it was meant for is only allocated
when the input bindings or output tokens also changed. A pass that
toggles its own MSAA setting while its bindings stay identical hits the
mismatch without allocating anything, and the clear then landed on the
buffer it is still sharing with the previous pass. Gate the clear on the
pass actually owning an output buffer for that AOV.

Also record the two limitations the code left implicit: chaining across
render delegates still copies the depth, so it still binds it as a
sampled texture and can still fail on WebGPU, and the buffer sharing
rules in docs/renderbuffermgr.md never mentioned the same-renderer or
matching-sample-count requirements, nor that a same-renderer mismatch
now discards the previous pass's depth instead of copying it.
@LukPopp0 LukPopp0 changed the title [NOM-5453] Skip depth copy into a fresh buffer on sample-count mismatch [NOM-5453] Fix depth handling when chaining passes with different sample counts Aug 21, 2026
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.

2 participants