USDScene : Support material purpose (both read and write) #1273
Conversation
johnhaddon
added
the
pr-hold
|
Marking on hold, while I figure out the test failure and another problem I'm seeing loading the ALab scene. |
USD allows different materials to be bound for different purposes, with those purposes being `full` or `preview`. See https://graphics.pixar.com/usd/release/wp_usdshade.html. We represent this in Cortex as attributes like `ai:surface:full` or `surface:preview` etc. The intention is not to resolve which of these materials to use during loading in the SceneInterface, but simply to make them available for downstream use. In Gaffer, this might mean using a ShuffleAttributes node to copy `surface:preview` to `surface` so that it gets rendered. Possibly contentious : I removed the explicit management of shader type from `populateMaterial()`, replacing calls like `CreateDisplacementOutput()` with `CreateOutput()`. This is now symmetrical with the reading code, where we happily make attributes from all outputs regardless of their name. It also simplifies `populateMaterial()`, since we already have the exact string to be passed to `CreateOutput()`, rather than splitting it only for `CreateDisplacementOutput()` to join it back together again.
They were relying on some other code to have imported the submodules, but that is not the case when testing with `TEST_USD_SCRIPT=contrib/IECoreUSD/test/IECoreUSD/SceneCacheFileFormatTest.py`. Also remove trailing whitespace.
johnhaddon
force-pushed
the
materialPurpose
branch
from
d53bb7d to
61d5da9
Compare
johnhaddon
removed
the
pr-hold
|
Pushed a rebase that should fix the test failure - should be good to review now. The problem with the ALab was just me testing with a stale build. |
danieldresser-ie
left a comment
danieldresser-ie
left a comment
There was a problem hiding this comment.
Looks reasonable to me. As for the explicit management of shader type, I was just trying to match USD conventions, but I have no problem with the suggestion that USD conventions are bad and we shouldn't worry about them. I don't see any real reason to need named accessors for output types.
| pxr::UsdShadeMaterial material = pxr::UsdShadeMaterialBindingAPI( prim ).ComputeBoundMaterial( | ||
| &m_usdBindingsCaches.at( materialPurpose ), &m_usdCollectionQueryCache, materialPurpose, &bindingRelationship | ||
| ); | ||
| if( material && materialPurpose != pxr::UsdShadeTokens->allPurpose && bindingRelationship.GetBaseName() != materialPurpose ) |
There was a problem hiding this comment.
Some of these clauses seem unnecessary - if the first clause is false, then it doesn't matter whether or not we take this branch, we're ending up with an uninitialized material either way. And I don't get the second clause - if the purpose we're looking for is allPurpose, then it's not a problem if it falls back to allPurpose ... but won't bindingRelationship.GetBaseName() == materialPurpose in that case anyway? Could we just use the final clause?
There was a problem hiding this comment.
They do each have a role to play actually, and the tests fail if any one is removed :
materialneeds to be checked because if no material was found,bindingRelationship.GetBaseName()will throw because it hasn't been initialised.materialPurpose != pxr::UsdShadeTokens->allPurposeneeds to be checked, because for all purpose bindings, there is no explicit purpose in the name of the relationship, andGetBaseName()will return"binding"and not the purpose.
There was a problem hiding this comment.
Hopefully that makes sense - I'm going to take the liberty of merging so that I can get a new dependencies package out today.
There was a problem hiding this comment.
Yep, that makes sense.
2 participants
No description provided.