OGSMOD-8260 - Fix the 'depth' display of two frame passes #112

hodoulp · 2025-11-26T22:51:19Z

Description

The pull request improves the share of AOV buffers by handling the case where the first & second frame passes must display the depth OV. In case of any number of frame passes, the last frame pass dictates the visualized OAV.

Changes Made

Improve the RenderBufferManager::SetRenderOutputs() to handle to depth to depth copy from different render delegate using CopyDepthShader;
Add a new shader CopyDepthShader to perform depth copy only because HdxFullscreenShader only supports case where there is a color to output (and depth is optional);
Cleanup on the helpers from composeTaskHelpers.h
Add helper methods to get the list of AOVs and supported AOVs plus a unit test

Tests Performed

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

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

Signed-off-by: Patrick Hodoul <[email protected]>

hodoulp · 2025-11-27T05:14:05Z

source/engine/renderBufferManager.cpp

+    else if (outputs[0] == pxr::HdAovTokens->depth && depthInput.texture)
+    {
+        PrepareDepthOnlyFromInput(depthInput, depthDesc, controllerId);
+    }


That was mainly the missing piece when the visualized OAV is the depth. The existing previous lines were correctly handling the color case (i.e., color or color + depth) but the depth only case was missing.

Does this mean only sharing of color and depth are supported here ? I just test with the visualizeAOV as "Neye", the sharing of "Neye" buffer seems to be not working even both two passes are using Storm and Storm support "Neye" AOV, is this as expected?

Yes, only sharing color and depth AOVs are currently supported.

Signed-off-by: Patrick Hodoul <[email protected]>

lilike-adsk

Thanks for quick fix!
After chat with Patrick, after applying some modification in MayaHydra, this depth sharing across different renderers is working when we're visualizing depth AOV.

Now, in MayaHydra, when a user is specifing an AOV to visualize, we only apply that AOV as visualizeAOV on last pass, and always apply color as visualizeAOV on other passes.

sebastienberube-adsk

Changes approved, but I would really like the comment about outputs indices to be addressed.

Other comments, such as the repeated block at lines 467 to 505 of RenderBufferManager.cpp can be improved later.

sebastienberube-adsk · 2025-12-01T19:55:19Z

source/engine/renderBufferManager.cpp

+    const SdfPath aovPath = GetAovPath(controllerId, inputDepthAov.aovName);
+    // Get the buffer that the renderer will draw into from the render index.
+    HdRenderBuffer* buffer = static_cast<HdRenderBuffer*>(
+        _pRenderIndex->GetBprim(HdPrimTypeTokens->renderBuffer, aovPath));
+
+    // If there is no buffer in this render index it was determined that the buffer
+    // to write into should come from the input buffer from the previous pass.
+    if (!buffer)
+    {
+        // Use the input buffer
+        buffer = inputDepthAov.buffer;
+    }
+    else
+    {
+        if (!buffer->IsMapped())
+        {
+            // This might be a newly created BPrim.  Allocate the GPU texture if needed.
+            buffer->Allocate(desc.dimensions, desc.format, desc.multiSampled);
+        }
+    }
+
+    HgiTextureHandle output;
+    VtValue outputValue = buffer->GetResource(desc.multiSampled);
+    if (outputValue.IsHolding<HgiTextureHandle>())
+    {
+        output = outputValue.Get<HgiTextureHandle>();
+        if (!output)
+        {
+            TF_CODING_ERROR("The output render buffer does not have a valid texture %s.",
+                inputDepthAov.aovName.GetText());
+            return;
+        }
+    }
+    else
+    {
+        // The output render buffer is not holding a writeable buffer.
+        // You will need to composite to blend passes results.
+        return;
+    }


[Minor Observation]

This is not required for this PR, but just an observation:
The logic of lines 467 to 505 is repeated 3 times in the file (twice for color and depth in PrepareBuffersFromInputs, once for PrepareDepthOnlyFromInput).

Not a big deal, but utility function could be created to reduce the volume of redundant code and make it more concise and readable.

sebastienberube-adsk · 2025-12-01T20:59:35Z

source/engine/renderBufferManager.cpp

        PrepareBuffersFromInputs(colorInput, depthInput, colorDesc, controllerId);
    }
+    // But depth AOV only means depth AOV only.
+    else if (outputs[0] == pxr::HdAovTokens->depth && depthInput.texture)


We should not make assumptions about the order of outputs.
Nothing in the interface tells that {depth, color} is different from {color, depth}.

Also, I believe localOutputs should be used instead of outputs (the difference is that localOutputs is validated and will only contain supported elements).

So, instead of using the index, I suggest to create some local "_Contains" function to search the output tokens vector as follow:

(Note: searching a small vector of token here would be quick, I don't think this is a performance issue)

bool hasColorOutput = _Contains(localOutputs, pxr::HdAovTokens->color); bool hasDepthOutput = _Contains(localOutputs, pxr::HdAovTokens->depth); if (hasColorOutput && colorInput.texture) { PrepareBuffersFromInputs(colorInput, depthInput, colorDesc, controllerId); } // But depth AOV only means depth AOV only. else if (hasDepthOutput && depthInput.texture) { PrepareDepthOnlyFromInput(depthInput, depthDesc, controllerId); }

Where _Contains would be something like:

template <class TContainer, class TValue> bool _Contains(TContainer const& container, TValue const& val) { return (std::find(container.begin(), container.end(), val) != container.end()); }

We should not make assumptions about the order of outputs.
Nothing in the interface tells that {depth, color} is different from {color, depth}.

In theory yes, but that's currently hard-coded in FramePass::GetRenderTasks() which then sets localOutputs in RenderBufferManager::Impl::SetRenderOutputs(). However, the optional inputs received by RenderBufferManager::Impl::SetRenderOutputs() can be arbitrary but they don't dictate the order.
In conclusion the order is currently hard-coded.

On long-term we want to have the outputs & inputs being defined by the caller (but still providing default outputs & inputs like right now) so the assumption could then fall. But the first output is the one to display and that's currently hard-coded in the code. Even with a fully customizable design the first output will remain the one to display. My 2 cents.

In case you noticed something else (short-term or long-term) the proposed code doesn't help. Let's chat.

We should aim to remove all the hard-coded parts.
In my view, the output order being hard-coded in the FramePass should not justify hard-coding it in the RenderBufferManager.

Agree. We can definitively remove any hard-coded assumptions except which one to display. Today, that's the first one. Let me check if I can find something to improve that part.

Regarding hardcoded AOVs, another case is the hardcoded list here:
https://github.com/Autodesk/hydra-viewport-toolbox/blob/main/source/engine/framePass.cpp#L305
I think a supported AOV list should be queried from a renderer, but currently, there's no such interface in Hydra.

In MayaHydra, we have to use a pre-defined AOV list and test if a renderer support an AOV by using renderDelegate->GetDefaultAovDescriptor(aovName).format != HdFormatInvalid

Regarding hardcoded AOVs, another case is the hardcoded list here:
https://github.com/Autodesk/hydra-viewport-toolbox/blob/main/source/engine/framePass.cpp#L305
I think a supported AOV list should be queried from a renderer, but currently, there's no such interface in Hydra.

Yep that's one of the hard-coded code I mentioned above.

In MayaHydra, we have to use a pre-defined AOV list and test if a renderer support an AOV by using renderDelegate->GetDefaultAovDescriptor(aovName).format != HdFormatInvalid

Out of curiosity, can you point me to the MayaHydra code?
In my mind, color and depth are 'mandatory' ones for any render delegate. Which other AOV types does MayaHydra need to validate?

MayaHydra doesn't know any AOV stuff beyond color and depth, it just pass the visualized AOV requested by user (could be any AOV like "primId") to Hydra (renderers) if the renderer supports(by the API I mentioned above, which was also used by USDView https://github.com/PixarAnimationStudios/OpenUSD/blob/release/pxr/usdImaging/usdImagingGL/engine.cpp#L1704).
The typical workflow/feature is we provide a AOVs dropdown list and allow user to pick one AOV to visualize, same as USDView.

The current situation the list is pre-defined/hardcoded, same as UsdImagingGLEngine::GetRendererAovs() used by USDView, there's no way to ask a renderer to return all supported AOVs

Very interesting. We can certainly reuse that technic to dynamically build the default list of AOVs.

source/engine/copyDepthShader.cpp

Co-authored-by: Sébastien Bérubé <[email protected]>

Signed-off-by: Patrick Hodoul <[email protected]>

hodoulp · 2025-12-02T19:01:57Z

@sebastienberube-adsk Look at the last commit to see how I fix the 'which AOV to visualize' challenge you highlighted.

Signed-off-by: Patrick Hodoul <[email protected]>

hodoulp · 2025-12-03T21:49:06Z

@lilike-adsk There are now two methods to find the AOVs:

static TfTokenVector RenderBufferManager::RenderBufferManager::GetAllRendererAovs() to have the default list
TfTokenVector RenderBufferManager::GetSupportedRendererAovs() const to have the list of supported AOVs

Signed-off-by: Patrick Hodoul <[email protected]>

Remove misleading comments.

Signed-off-by: Patrick Hodoul <[email protected]>

lilike-adsk · 2025-12-04T14:06:29Z

@lilike-adsk There are now two methods to find the AOVs:

static TfTokenVector RenderBufferManager::RenderBufferManager::GetAllRendererAovs() to have the default list

TfTokenVector RenderBufferManager::GetSupportedRendererAovs() const to have the list of supported AOVs

Thanks, will try it, hopefully, we could have this change and the AOV crash fix in main branch soon.

github-actions · 2025-12-10T00:08:58Z

Package	Line Rate	Health
include.hvt.dataSource	0%	❌
include.hvt.engine	97%	✔
include.hvt.geometry	0%	❌
include.hvt.sceneIndex	100%	✔
include.hvt.tasks	72%	➖
source.dataSource	0%	❌
source.engine	58%	➖
source.geometry	7%	❌
source.sceneIndex	64%	➖
source.shadow	0%	❌
source.tasks	82%	✔
source.utils	0%	❌
Summary	57% (2147 / 3739)	➖

hodoulp added 4 commits November 21, 2025 11:32

OGSMOD-8260 - Some tries

140be44

Signed-off-by: Patrick Hodoul <[email protected]>

Fix depth-depth case

fe43e7d

Signed-off-by: Patrick Hodoul <[email protected]>

Improve depth-depth case

20e7b70

Signed-off-by: Patrick Hodoul <[email protected]>

Update baseline images

1e7f986

Signed-off-by: Patrick Hodoul <[email protected]>

hodoulp commented Nov 27, 2025

View reviewed changes

hodoulp and others added 5 commits November 27, 2025 11:43

Few changes

0e992db

Signed-off-by: Patrick Hodoul <[email protected]>

Improve utests

bbc6c7b

Signed-off-by: Patrick Hodoul <[email protected]>

Fix compose issue

9c011b1

Signed-off-by: Patrick Hodoul <[email protected]>

Fix ComposeTask

dfce6dd

Signed-off-by: Patrick Hodoul <[email protected]>

Simplify the CopyDepthShader code

1447375

Signed-off-by: Patrick Hodoul <[email protected]>

hodoulp marked this pull request as ready for review November 28, 2025 21:40

hodoulp requested review from AdamFelt and sebastienberube-adsk November 28, 2025 21:40

Cleanup

2dee5df

Signed-off-by: Patrick Hodoul <[email protected]>

hodoulp requested a review from lilike-adsk December 1, 2025 04:30

Fix build break

98a7909

Signed-off-by: Patrick Hodoul <[email protected]>

lilike-adsk approved these changes Dec 1, 2025

View reviewed changes

sebastienberube-adsk approved these changes Dec 1, 2025

View reviewed changes

hodoulp and others added 2 commits December 2, 2025 10:48

Update source/engine/copyDepthShader.cpp

389eb4d

Co-authored-by: Sébastien Bérubé <[email protected]>

More obvious which AOV to visualize

4fa2f55

Signed-off-by: Patrick Hodoul <[email protected]>

hodoulp and others added 3 commits December 2, 2025 14:10

Improve parameter names

057a93f

Signed-off-by: Patrick Hodoul <[email protected]>

Better define the outputs

a7365b2

Signed-off-by: Patrick Hodoul <[email protected]>

Merge branch 'main' into hodoulp/depth_issues

a604926

hodoulp and others added 4 commits December 3, 2025 17:33

Fix build break

96dbdf1

Signed-off-by: Patrick Hodoul <[email protected]>

Update OpenGLTestContext.cpp

a824300

Remove misleading comments.

Add a utest

759cb70

Signed-off-by: Patrick Hodoul <[email protected]>

Fix bug

d43434b

Signed-off-by: Patrick Hodoul <[email protected]>

hodoulp changed the title ~~OGSMOD-8260 - Fix the frame1 (depth) and frame2 (depth) display~~ OGSMOD-8260 - Fix the 'depth' display of two frame passes Dec 5, 2025

hodoulp added 3 commits December 5, 2025 18:10

Update OpenGLTestContext.cpp

cb176cc

Merge branch 'main' into hodoulp/depth_issues

fcd3b1d

Merge branch 'main' into hodoulp/depth_issues

dedae17

OGSMOD-8260 - Fix the 'depth' display of two frame passes #112

Are you sure you want to change the base?

OGSMOD-8260 - Fix the 'depth' display of two frame passes #112

Uh oh!

Conversation

hodoulp commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes Made

Tests Performed

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lilike-adsk left a comment

Choose a reason for hiding this comment

Uh oh!

sebastienberube-adsk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hodoulp Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hodoulp commented Dec 2, 2025

Uh oh!

hodoulp commented Dec 3, 2025

Uh oh!

lilike-adsk commented Dec 4, 2025

Uh oh!

github-actions bot commented Dec 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hodoulp commented Nov 26, 2025 •

edited

Loading

hodoulp Dec 2, 2025 •

edited

Loading