Skip to content

Conversation

@johnhaddon
Copy link
Member

@johnhaddon johnhaddon commented Dec 8, 2025

No description provided.

/// Reads a ShaderNetwork from a material output, typically obtained from `UsdShadeMaterial::GetOutput()`.
/// Returns `nullptr` if `canReadShaderNetwork() == false`, usually because the output has no connected source.
IECOREUSD_API IECoreScene::ShaderNetworkPtr readShaderNetwork( const pxr::UsdShadeOutput &output );
IECOREUSD_API IECoreScene::ShaderNetworkPtr readShaderNetwork( const pxr::UsdShadeOutput &output, pxr::UsdTimeCode timeCode = pxr::UsdTimeCode::Default() );
Copy link
Member Author

Choose a reason for hiding this comment

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

I took advantage of IECoreUSD technically (but ridiculously) still being in contrib to just change the function signature here. I can add an overload if we think maintaining ABI is important to anyone.

@danieldresser-ie
Copy link
Contributor

Looks good to me. The only question I'm left with after reading through it is why shaderNetworkMightBeTimeVarying needs a cache, but lightMightBeTimeVarying doesn't - there's probably a reasonable explanation though, perhaps just because light networks are generally expected to be so much smaller than other shader networks. Regardless, this is just a performance question, and doesn't affect correctness, so I don't think it should hold up getting this merged.

@murraystevenson murraystevenson merged commit 9d1232d into ImageEngine:RB-10.6 Dec 8, 2025
4 of 5 checks passed
@johnhaddon
Copy link
Member Author

The only question I'm left with after reading through it is why shaderNetworkMightBeTimeVarying needs a cache, but lightMightBeTimeVarying doesn't

For the same reason that we were already caching calls to readShaderNetwork() but not to readLight(). Partly, as you say, because materials are typically much more complex than lights (which in many cases have no additional shaders). But also because there are far more opportunities for cache hits - it's very common for many prims to be bound to the same material, so we commonly need to query/load the same material many many times. The only way caching would make sense for lights would be if the same light was instanced many times - that's not an impossibility, but our assumption so far has been that it's not important for our use cases. It may be worth visiting that again at some point, but I judged it to be out of scope for this PR.

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.

3 participants