Autodesk: Fix issue where ResolveParameter doesn't support type conversion

[erikaharrison-adsk]

Description of Change(s)

When resolving the parameter for wrapSampler, minSampler, and magSampler, if the value got is not a token, we try to get the value as a string, and construct the token from the string.

Related: Prior approach/discussion in #2473

Fixes Issue(s)

• N/A

• I have verified that all unit tests pass with the proposed changes

• I have submitted a signed Contributor License Agreement

[jesschimein]

Filed as internal issue #USD-8475

[tcauchois]

Hi Erika,

Given the discussion on #2473, it seemed like we decided to drop this PR? Note that all of the sampler parameters are in the schema and Sdr as tokens, so accepting strings here would be deviating from the USD spec. Is there a reason you can't write these as tokens in the assets? If these are strings in the assets, I'd expect them to fail usd validation; though I don't know if we have coverage for this yet in fixbrokenschemas.

Let me know if I'm missing something! Thanks,
Tom

[PierreWang]

Hi Erika,

Given the discussion on #2473, it seemed like we decided to drop this PR? Note that all of the sampler parameters are in the schema and Sdr as tokens, so accepting strings here would be deviating from the USD spec. Is there a reason you can't write these as tokens in the assets? If these are strings in the assets, I'd expect them to fail usd validation; though I don't know if we have coverage for this yet in fixbrokenschemas.

Let me know if I'm missing something! Thanks, Tom

The problem is that with the current implementation, _ResolveParameter in material.cpp will not correctly resolve the token parameter. For example, the below code will always return empty sampler.
" TfToken wrapSampler = _ResolveParameter(
node, sdrNode, name, TfToken());"

I have tested the simpleShading sample in extras\usd\tutorials. The wrapS, wrapT, and wrapR sampler will all be empty after _ResolveParameter. Maybe I don't get the root cause of the problem, but resolve the parameter as a string will resolve the issue.

[jesschimein]

/AzurePipelines run

[azure-pipelines]

Pull request contains merge conflicts.

[tcauchois]

Pierre: the script you linked never sets the sampler attributes. Have you modified the script at all? And what are you getting vs expecting from _ResolveParameter, exactly?

Thanks!

[PierreWang]

Pierre: the script you linked never sets the sampler attributes. Have you modified the script at all? And what are you getting vs expecting from _ResolveParameter, exactly?

Thanks!

Sorry. It seems that the problem is "_ResolveParameter" will not return correct default value for a token property. For example, for the UsdUVTexture shader, there are default values for "wrapS" and "wrapT", which are saved in shaderDefs.usda. But current implementation can not read it:
// Then fallback to SdrNode. if (sdrNode) { if (const SdrShaderPropertyConstPtr input = sdrNode->GetShaderInput(name)) { const VtValue& value = input->GetDefaultValue(); if (value.IsHolding<T>()) { return value.UncheckedGet<T>(); } } }
Here the value will not hold a token. So it will not go to the line "return value.UncheckedGet();".

You can have a try with the simpleShading sample under extras\usd\tutorials. If you set a breakpoint at _ResolveParameter, you will find that it will not correctly get the default value. But if you call _ResolveParameter with the “string" parameter type, the default value can be correctly fetched.

[jesschimein]

/AzurePipelines run

[azure-pipelines]

Pull request contains merge conflicts.

[spiffmon]

Ah! THank you for bearing with us, @PierreWang ! I think that means the correct fix is to change the call to GetDefault() to GetDefaultValueAsSdfType()

[tcauchois]

Ok @spiffmon so your recommendation is here:
https://github.com/PixarAnimationStudios/OpenUSD/pull/2519/files#diff-ba83f6b9afea735ef4351df227d7424447be68b0ddc0201a591f1a7c83ef265fR560
... we change "GetDefaultValue" to "GetDefaultValueAsSdfType"?

@PierreWang, can you try that and let us know if it fixes your issue? It should make the rest of this PR unnecessary. If that works, please update the PR and we'll pull it in. Thanks!

[PierreWang]

Ok @spiffmon so your recommendation is here: https://github.com/PixarAnimationStudios/OpenUSD/pull/2519/files#diff-ba83f6b9afea735ef4351df227d7424447be68b0ddc0201a591f1a7c83ef265fR560 ... we change "GetDefaultValue" to "GetDefaultValueAsSdfType"?

@PierreWang, can you try that and let us know if it fixes your issue? It should make the rest of this PR unnecessary. If that works, please update the PR and we'll pull it in. Thanks!

Great, this will resolve the problem. I will update this branch and use "GetDefaultValueAsSdfType" fix instead what I do now.

[jesschimein]

/AzurePipelines run

[erikaharrison-adsk]

PR updated with suggested change.

[jesschimein]

/AzurePipelines run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).