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

hdUsdWriter plugin - Render to text delegate #3003

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

tylerm-nv
Copy link
Contributor

@tylerm-nv tylerm-nv commented Mar 15, 2024

Description of Change(s)

This PR adds a new plugin, hdUsdWriter and tools for creating tests or debugging render output. The plugin provides a render delegate that stores information it receives from Hydra and serializes it (the included tooling is for text diffing and outputs usda).

The library includes a python module that utilizes the driver to perform fuzzy text diffing between the output and an expected result. A new tool called hdCapture provides a simple way to exercise this functionality.

The delegate supports most prim types, and is easily extendable by downstream projects.

Fixes Issue(s)

  • I have verified that all unit tests pass with the proposed changes
  • I have submitted a signed Contributor License Agreement

@jesschimein
Copy link

Filed as internal issue #USD-9460

@jesschimein
Copy link

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jesschimein
Copy link

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@tcauchois tcauchois left a comment

Choose a reason for hiding this comment

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

Left some suggestions on Usd API usage, a few quick questions, and a request for a C++ driver. Let me know if you have any issues with any of the feedback.

Comment on lines +150 to +151
const auto& attr = prim.CreateAttribute(UsdGeomTokens->clippingPlanes, SdfValueTypeNames->Float4Array,
false /*custom*/, SdfVariabilityVarying);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const auto& attr = prim.CreateAttribute(UsdGeomTokens->clippingPlanes, SdfValueTypeNames->Float4Array,
false /*custom*/, SdfVariabilityVarying);
const auto& attr = camera.CreateClippingPlanesAttr();

Comment on lines +241 to +242
const auto& propertySpec = schemaDefinition->GetSchemaPropertySpec(param);
auto attr = prim.CreateAttribute(param, propertySpec->GetTypeName(), false, propertySpec->GetVariability());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const auto& propertySpec = schemaDefinition->GetSchemaPropertySpec(param);
auto attr = prim.CreateAttribute(param, propertySpec->GetTypeName(), false, propertySpec->GetVariability());
const auto& attributeDef = schemaDefinition->GetAttributeDefinition(param);
auto attr = prim.CreateAttribute(param, attributeDef->GetTypeName(), false, attributeDef->GetVariability());

PrimvarMap::accessor pvAccessor;
if (_primvars.find(pvAccessor, name) && pvAccessor->second.value.IsHolding<T>())
{
f(pvAccessor->second.value.template UncheckedGet<T>());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
f(pvAccessor->second.value.template UncheckedGet<T>());
f(pvAccessor->second.value.UncheckedGet<T>());

// incorrect, std::string instead of TfToken.
// TfToken parameters might come through as std::string, so we need to do some extra
// conversions.
if (inputType == SdfValueTypeNames->Token && !parameter.second.template IsHolding<TfToken>())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (inputType == SdfValueTypeNames->Token && !parameter.second.template IsHolding<TfToken>())
if (inputType == SdfValueTypeNames->Token && !parameter.second.IsHolding<TfToken>())

// conversions.
if (inputType == SdfValueTypeNames->Token && !parameter.second.template IsHolding<TfToken>())
{
if (parameter.second.template IsHolding<std::string>())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (parameter.second.template IsHolding<std::string>())
if (parameter.second.IsHolding<std::string>())

const auto& token = parameter.second.template UncheckedGet<TfToken>();
input.Set(SdfAssetPath{ token.GetString(), token.GetString() });
}
else if (parameter.second.template IsHolding<std::string>())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
else if (parameter.second.template IsHolding<std::string>())
else if (parameter.second.IsHolding<std::string>())

}
else if (parameter.second.template IsHolding<std::string>())
{
const auto& string = parameter.second.template UncheckedGet<std::string>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const auto& string = parameter.second.template UncheckedGet<std::string>();
const auto& string = parameter.second.UncheckedGet<std::string>();

/*!
\page hdUsdWriter HdUsdWriterRenderDelegate : Renderer plugin.

\section hdUsdWriter_overview Usd Writer Render Delegate
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker, but it would be wonderful to have more documentation here, e.g. of example invocation of hdcapture and example output or something. Also, possibly the HdTiny comment is extraneous?

light
material
mesh
openvdbasset
Copy link
Contributor

Choose a reason for hiding this comment

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

We had some questions internally about whether we need to guard openvdbasset if we don't compile against openvdb; not sure if you know offhand. My memory is the only place we actually invoke OpenVDB is in the render delegate, which makes me think this is ok.


PYMODULE_FILES
fuzzytextdiff.py
hdusdwriterdriver.py
Copy link
Contributor

Choose a reason for hiding this comment

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

We've had requests even before this has landed to (1) move this to pxr/usdImaging/usdAppUtils; and (2) rewrite it in C++ :). Are you all up for doing that? It's not a ton of code. It would probably mean you'd need to get rid of the fuzzytextdiff reference from the driver, and move fuzzytextdiff and the compare() function to pxr/usdImaging/bin/hdcapture directly.

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.

None yet

3 participants