NVIDIA: Fix issue of switching material bindings for skeletal mesh #3542
Conversation
|
@nvmkuruc for vis |
roggiezhang-nv
changed the title
roggiezhang-nv
force-pushed
the
material_sync
branch
from
bd55721 to
d9372a3
Compare
|
|
||
| // Bring up Hydra | ||
| Hd_UnitTestNullRenderDelegate renderDelegate; | ||
| std::unique_ptr<HdRenderIndex> |
There was a problem hiding this comment.
Core guidelines recommends creating these via make_unique.
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rr-make_unique
There was a problem hiding this comment.
(I assume you are pointing to the following new, not the New factory function, switching to make_unique here would lose the error check New performs.)
There was a problem hiding this comment.
Yup! I don't think we use make_unique with HdRenderIndex because the constructor is private.
|
Filed as internal issue #USD-10696 (This is an automated message. See here for more information.) |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
roggiezhang-nv
force-pushed
the
material_sync
branch
3 times, most recently
from
cb6161b to
d066dbf
Compare
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| // Switch the material on the head first | ||
| auto headPrim = stage->GetPrimAtPath(SdfPath("/Root/Geometry/head")); | ||
| TF_AXIOM(headPrim); | ||
| auto materialBinding = headPrim.GetRelationship(TfToken("material:binding")); |
There was a problem hiding this comment.
Not a huge deal, but it might be nice to use the schema here if possible.
| TF_AXIOM(materialBinding); | ||
| materialBinding.SetTargets({SdfPath("/Root/Looks/green")}); | ||
|
|
||
| delegate->SetTime(0); |
There was a problem hiding this comment.
Is this second SetTime necessary to produce the issue?
There was a problem hiding this comment.
I think so. SetTime(0) is a way to apply all pending changes to setup dirty bits of all prims inside renderindex. That's a necessary step.
There was a problem hiding this comment.
Maybe I should replace them with ApplyPendingUpdates so it's more straightforward. Actually, SetTime(0) calls ApplyPendingUpdates inside.
| // Switch the material on the neck | ||
| auto neckPrim = stage->GetPrimAtPath(SdfPath("/Root/Geometry/neck")); | ||
| TF_AXIOM(neckPrim); | ||
| materialBinding = neckPrim.GetRelationship(TfToken("material:binding")); |
There was a problem hiding this comment.
I'd recommend avoiding reuse of the variables if possible. You can put the two switches into different scopes so the lifetime is clearer.
| materialBinding = neckPrim.GetRelationship(TfToken("material:binding")); | ||
| TF_AXIOM(materialBinding); | ||
| materialBinding.SetTargets({SdfPath("/Root/Looks/green")}); | ||
| delegate->SetTime(0); |
There was a problem hiding this comment.
Is the SetTime necessary to produce the bug?
roggiezhang-nv
force-pushed
the
material_sync
branch
from
d066dbf to
3dea09f
Compare
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
roggiezhang-nv
force-pushed
the
material_sync
branch
from
3dea09f to
7688215
Compare
roggiezhang-nv
force-pushed
the
material_sync
branch
from
7688215 to
56c60a3
Compare
Switching material bindings for skeletal meshes may cause out of sync issue. Fixes PixarAnimationStudios#1747 Closes PixarAnimationStudios#3542 (Internal change: 2369214)
4 participants
Description of Change(s)
Switching material bindings for skeletal meshes may cause out of sync issue.
See #1747 for more details.
Link to proposal (if applicable)
Fixes Issue(s)
#1747
Checklist
I have created this PR based on the dev branch
I have followed the coding conventions
I have added unit tests that exercise this functionality (Reference:
testing guidelines)
I have verified that all unit tests pass with the proposed changes
I have submitted a signed Contributor License Agreement (Reference:
Contributor License Agreement instructions)