Skip to content

OGSMOD-8325 - Investigation of SkyDome utest failure #122

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.

Sign up for GitHub

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

Merged
hodoulp merged 10 commits into from
Dec 15, 2025
Merged

OGSMOD-8325 - Investigation of SkyDome utest failure #122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c802b09
OGSMOD-8325 - Investigation of SkyDome utest failure
hodoulp Dec 9, 2025
ce98a1c
Code cleanups
hodoulp Dec 9, 2025
f8e105c
Update Windows basline
hodoulp Dec 9, 2025
8491ff5
Update macOS baseline image
hodoulp Dec 9, 2025
73439db
Merge branch 'main' into hodoulp/skydome_1
hodoulp Dec 11, 2025
93c309a
Merge branch 'main' into hodoulp/skydome_1
hodoulp Dec 15, 2025
7fb7442
Improve SkyDome utest
hodoulp Dec 15, 2025
9e2ba52
Improve SkyDome utest
hodoulp Dec 15, 2025
3536391
Merge branch 'hodoulp/skydome_1' of https://github.com/Autodesk/hydra…
hodoulp Dec 15, 2025
4349467
The version was not changed
hodoulp Dec 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ endif()

# Declare the project.
project(HydraViewportToolbox
VERSION 0.25.08.0
VERSION 0.25.11.0
DESCRIPTION "Utilities to support graphics viewports using OpenUSD Hydra"
LANGUAGES CXX
HOMEPAGE_URL https://github.com/Autodesk/hydra-viewport-toolbox
Expand Down
2 changes: 1 addition & 1 deletion include/hvt/engine/taskUtils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ HVT_API PXR_NS::TfToken GetRenderingBackendName(PXR_NS::HdRenderIndex const* ren

/// Sets the blend state for the given material tag in the render task parameters.
HVT_API extern void SetBlendStateForMaterialTag(
PXR_NS::TfToken const& materialTag, PXR_NS::HdxRenderTaskParams* renderParams);
PXR_NS::TfToken const& materialTag, PXR_NS::HdxRenderTaskParams& renderParams);
Copy link
Contributor

@sebastienberube-adsk sebastienberube-adsk Dec 15, 2025

Choose a reason for hiding this comment

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

Although this change does not appear to be related to the failure, I am fine with the change from HdxRenderTaskParams* to HdxRenderTaskParams&, it is more consistent.

For the record, the only reason a pointer was used before is because this function was originally from TaskController.cpp.

See:

HdxTaskController::_SetBlendStateForMaterialTag(
    TfToken const& materialTag,
    HdxRenderTaskParams *renderParams) const
{

Open USD repo link:
https://github.com/PixarAnimationStudios/OpenUSD/blob/48cb715a00097e93fa857810d2e948a2c1ad826b/pxr/imaging/hdx/taskController.cpp#L354C1-L392C3


/// Gets the render task path for the given controller and material tag.
HVT_API extern PXR_NS::SdfPath GetRenderTaskPath(
Expand Down
6 changes: 4 additions & 2 deletions source/engine/taskCreationHelpers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -743,6 +743,8 @@ SdfPath CreateRenderTask(TaskManagerPtr& pTaskManager,
TfToken const& taskName, PXR_NS::SdfPath const& atPos = SdfPath::EmptyPath(),
TaskManager::InsertionOrder order = TaskManager::InsertionOrder::insertAtEnd)
{
// Intermediate variable to avoid the capture of the following lambda to take
// the ownership of the variable.
TaskManager& taskManager = *pTaskManager;

auto fnCommit =
Expand All @@ -755,7 +757,7 @@ SdfPath CreateRenderTask(TaskManagerPtr& pTaskManager,
HdxRenderTaskParams params = getLayerSettings()->renderParams;

// Set blend state and depth mask according to material tag (additive, masked, etc).
SetBlendStateForMaterialTag(materialTag, &params);
SetBlendStateForMaterialTag(materialTag, params);

// NOTE: According to pxr::HdxRenderTaskParams, viewport is only used if framing is
// invalid.
Expand Down Expand Up @@ -837,7 +839,7 @@ SdfPath CreateRenderTask(TaskManagerPtr& pTaskManager,
HdxRenderTaskParams renderParams = getLayerSettings()->renderParams;

// Set the blend state based on material tag.
SetBlendStateForMaterialTag(materialTag, &renderParams);
SetBlendStateForMaterialTag(materialTag, renderParams);

return taskManager.AddRenderTask<TRenderTask>(taskName, renderParams, fnCommit, atPos, order);
}
Expand Down
50 changes: 22 additions & 28 deletions source/engine/taskUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,18 +64,12 @@ TfToken GetRenderingBackendName(HdRenderIndex const* renderIndex)
return HgiTokens->OpenGL;
}

void SetBlendStateForMaterialTag(TfToken const& materialTag, HdxRenderTaskParams* renderParams)
void SetBlendStateForMaterialTag(TfToken const& materialTag, HdxRenderTaskParams& renderParams)
{
if (!TF_VERIFY(
renderParams, "SetBlendStateForMaterialTag: The renderParams parameter is null."))
{
return;
}

if (materialTag == HdStMaterialTagTokens->additive)
{
// Additive blend -- so no sorting of drawItems is needed
renderParams->blendEnable = true;
renderParams.blendEnable = true;
// For color, we are setting all factors to ONE.
//
// This means we are expecting pre-multiplied alpha coming out
Expand All @@ -84,47 +78,47 @@ void SetBlendStateForMaterialTag(TfToken const& materialTag, HdxRenderTaskParams
// shader side, since it means we would force a pre-multiplied
// alpha step on the color coming out of the shader.
//
renderParams->blendColorOp = HdBlendOpAdd;
renderParams->blendColorSrcFactor = HdBlendFactorOne;
renderParams->blendColorDstFactor = HdBlendFactorOne;
renderParams.blendColorOp = HdBlendOpAdd;
renderParams.blendColorSrcFactor = HdBlendFactorOne;
renderParams.blendColorDstFactor = HdBlendFactorOne;

// For alpha, we set the factors so that the alpha in the
// framebuffer won't change. Recall that the geometry in the
// additive render pass is supposed to be emitting light but
// be fully transparent, that is alpha = 0, so that the order
// in which it is drawn doesn't matter.
renderParams->blendAlphaOp = HdBlendOpAdd;
renderParams->blendAlphaSrcFactor = HdBlendFactorZero;
renderParams->blendAlphaDstFactor = HdBlendFactorOne;
renderParams.blendAlphaOp = HdBlendOpAdd;
renderParams.blendAlphaSrcFactor = HdBlendFactorZero;
renderParams.blendAlphaDstFactor = HdBlendFactorOne;

// Translucent objects should not block each other in depth buffer
renderParams->depthMaskEnable = false;
renderParams.depthMaskEnable = false;

// Since we are using alpha blending, we disable screen door
// transparency for this renderpass.
renderParams->enableAlphaToCoverage = false;
renderParams.enableAlphaToCoverage = false;

#if defined(DRAW_ORDER)
}
else if (materialTag == HdStMaterialTagTokens->draworder)
{

// ResultColor = SrcColor * SrcAlpha + DestColor * (1-SrcAlpha)
renderParams->blendEnable = true;
renderParams->blendColorOp = HdBlendOpAdd;
renderParams->blendColorSrcFactor = HdBlendFactorSrcAlpha;
renderParams->blendColorDstFactor = HdBlendFactorOneMinusSrcAlpha;
renderParams.blendEnable = true;
renderParams.blendColorOp = HdBlendOpAdd;
renderParams.blendColorSrcFactor = HdBlendFactorSrcAlpha;
renderParams.blendColorDstFactor = HdBlendFactorOneMinusSrcAlpha;

renderParams->blendAlphaOp = HdBlendOpAdd;
renderParams->blendAlphaSrcFactor = HdBlendFactorOne;
renderParams->blendAlphaDstFactor = HdBlendFactorZero;
renderParams.blendAlphaOp = HdBlendOpAdd;
renderParams.blendAlphaSrcFactor = HdBlendFactorOne;
renderParams.blendAlphaDstFactor = HdBlendFactorZero;

// Disable depth buffer for draworder.
renderParams->depthMaskEnable = false;
renderParams.depthMaskEnable = false;

// Since we are using alpha blending, we disable screen door
// transparency for this renderpass.
renderParams->enableAlphaToCoverage = false;
renderParams.enableAlphaToCoverage = false;
#endif
}
else if (materialTag == HdStMaterialTagTokens->defaultMaterialTag ||
Expand All @@ -134,9 +128,9 @@ void SetBlendStateForMaterialTag(TfToken const& materialTag, HdxRenderTaskParams
// we classify them as separate because in the general case, masked
// materials use fragment shader discards while the defaultMaterialTag
// should not.
renderParams->blendEnable = false;
renderParams->depthMaskEnable = true;
renderParams->enableAlphaToCoverage = true;
renderParams.blendEnable = false;
renderParams.depthMaskEnable = true;
renderParams.enableAlphaToCoverage = true;
}
}

Expand Down
6 changes: 5 additions & 1 deletion test/RenderingFramework/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,14 +74,18 @@ target_compile_definitions(${_TARGET}
)

# Where to find all the data files for the unit tests.
cmake_path(SET hvt_test_data_normalized NORMALIZE "${CMAKE_CURRENT_SOURCE_DIR}/..")
cmake_path(SET hvt_test_data_normalized NORMALIZE "${CMAKE_CURRENT_SOURCE_DIR}/../test")
# Remove the last '/' if any.
get_filename_component(hvt_test_data_normalized "${hvt_test_data_normalized}" DIRECTORY)
target_compile_definitions(${_TARGET}
PRIVATE
HVT_TEST_DATA_PATH=${hvt_test_data_normalized}
)

# Where to output any processing results.
cmake_path(SET test_data_output_normalized NORMALIZE "${CMAKE_CURRENT_BINARY_DIR}/..")
# Remove the last '/' if any.
get_filename_component(test_data_output_normalized "${test_data_output_normalized}" DIRECTORY)
Copy link
Contributor Author

@hodoulp hodoulp Dec 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Avoid having a path with some platform specific issues like:

  • Some platforms succeed with abc//cde
  • inconsistency between path abc/ & abc

target_compile_definitions(${_TARGET}
PRIVATE
TEST_DATA_OUTPUT_PATH=${test_data_output_normalized}
Expand Down
2 changes: 1 addition & 1 deletion test/RenderingFramework/TestHelpers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ const std::filesystem::path inAssetsPath = getenv("HVT_TEST_ASSETS");
const std::string resFullpath = getenv("HVT_RESOURCES");
std::filesystem::path inBaselinePath = getenv("HVT_BASELINES");
#else
const auto outFullpath = std::filesystem::path(TOSTRING(TEST_DATA_OUTPUT_PATH)) / "computed";
const std::filesystem::path outFullpath = TOSTRING(TEST_DATA_OUTPUT_PATH) + "/computed";
Copy link
Contributor Author

@hodoulp hodoulp Dec 15, 2025

Choose a reason for hiding this comment

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

Change to have a consistent path between platforms (i.e. *nix vs. Windows)

const std::filesystem::path inAssetsPath = TOSTRING(HVT_TEST_DATA_PATH) + "/data/assets";
const std::string resFullpath = TOSTRING(HVT_RESOURCE_PATH);
std::filesystem::path inBaselinePath = TOSTRING(HVT_TEST_DATA_PATH) + "/data/baselines";
Expand Down
Binary file modified test/data/baselines/howTo/useSkyDomeTask.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/data/baselines/howTo/useSkyDomeTask_osx.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added test/data/baselines/origin_dev/02511/howTo/useSkyDomeTask.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added test/data/baselines/origin_dev/02511/howTo/useSkyDomeTask_osx.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
8 changes: 6 additions & 2 deletions test/howTos/howTo11_UseSkyDomeTask.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,10 @@ HVT_TEST(howTo, useSkyDomeTask)

// Validates the rendering result.

// OGSMOD-8325 - WebGPU & Linux needs a small threshold to use baseline images.
ASSERT_TRUE(context->validateImages(computedImageName, imageFile, 20));
std::string newImageName = imageFile;
#if PXR_VERSION >= 2511 && !defined(ENABLE_ADSK_OPENUSD_PENDING)
newImageName = "origin_dev/02511/" + imageFile;
#endif
Copy link
Contributor Author

@hodoulp hodoulp Dec 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

There is a light difference from origin/dev between 25.08 & 25.11. That was found with the last USD Sync.

Copy link
Contributor

@sebastienberube-adsk sebastienberube-adsk Dec 15, 2025

Choose a reason for hiding this comment

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

I just had a look at the new image, and it looks like it is "sharper" (less blurry). I am not sure what the change is between 25.08 & 25.11 to have this effect. Perhaps a difference in how the image samples are averaged? (MipLevel or MSAA related change?)

The lighting from the DomeLight (Image-Based lighting using the SkyDome) on the 3D object is also slightly different, but to a lesser degree.

I approve adding a new baseline image, but it would be interesting to know what causes this change, and if the baseline images from OpenUSD repo have also been changed recently.

We can answer that question later, just adding my thoughts here, in case we need to investigate some of this later.

Copy link
Contributor Author

@hodoulp hodoulp Dec 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

There is also some issue/change with the MSAA introduced by the change to 25.11.


ASSERT_TRUE(context->validateImages(computedImageName, newImageName));
}