EMSUSD-1113 suport legacy metarial scope mode #3810
Conversation
pierrebai-adsk
commented
Jun 7, 2024
pierrebai-adsk
commented
- Add a legacyMaterialScope flag to the export command.
- The flag default to false (off).
- When turned on, we use the previous method of determining where a material is generated, which is under the common root prim of all meshes using the material.
- The merge-to-USD workflow use that flag to ensure it receives the material under the mesh.
- The export-to-USD UI forces that flag to be false (off), to ensure we always use the new material scope behavior.
- Added some unit tests for the legacy mode.
- Add a legacyMaterialScope flag to the export command. - The flag default to false (off). - When turned on, we use the previous method of determining where a material is generated, which is under the common root prim of all meshes using the material. - The merge-to-USD workflow use that flag to ensure it receives the material under the mesh. - The export-to-USD UI forces that flag to be false (off), to ensure we always use the new material scope behavior. - Added some unit tests for the legacy mode.
pierrebai-adsk
force-pushed
the
bailp/EMSUSD-1113/legacy-material-scope
branch
from
8e625f4 to
9c306c7
Compare
|
@dj-mcg here is the change to support legacy material scope. |
| const std::string& defaultPrim, | ||
| const TfToken& materialsScopeName) | ||
| { | ||
| SdfPath shaderExportLocation = SdfPath::AbsoluteRootPath(); | ||
|
|
There was a problem hiding this comment.
The new approach no longer considers CommonAncestors as all - is that intentional? Ultimately this new approach makes more sense to me, since I was confused about the intended behavior if both defaultPrim and exportMaterialUnderPrim were specified.
I still find it a little unusual that defaultPrim needs to be explicitly specified in export arguments here, given that the export process provides a fallback approach that we rely on to actually author this value in the root metadata. In fact, if we allowed for that same fallback behavior here, I think we would fully prefer the newer structure you've introduced here. Any thoughts about that?
There was a problem hiding this comment.
For the first question, yes it is intentional. The code that I'm replacing now alredy did that, but was hidden in the caller, which passed an empty set of assignments in the new mode.
For the second question, we've had the same discussion internally this morning. For now, we keep it that way. The main issues were:
- Which the root prim to use as the default prim was selected "at random" from the pool of root prims.
- This decision is done at the very end of the export, so after material are already exported.
- We would need to choose the root prim earlier instead, probably as soon as we export a root prim,
- This required more code changes and it could change the behavior since which prim is first when we ask the stage for the set of root prims might not be in the order they are exported but in some other order, for example alphabetical.
In addition, we consider that not specifying a default prim but relying in this implicit selection was a legacy behaviour. In our export UI, the user selects the default prim, so the default prim is always known. The thought was that if you use the new material scope logic, you should specify a default prim.
That being said, if it causes headaches, we can revisit the decision.
There was a problem hiding this comment.
Sounds good, @pierrebai-adsk thanks for prompt and thorough attention to this issue!
pierrebai-adsk
added
the
ready-for-merge
