Skip to content

Commit

Permalink
[usdImaging] Fix crash when processing multiple native instancer edit…
Browse files Browse the repository at this point in the history
…s in the same frame.

This is actually a rehash of github issue PixarAnimationStudios#1417, which was the same bug in PointInstancers rather than native USD instancing.  If we get multiple property edits to native instancers in the same frame, it's possible that the first will resync the instancer and erase instancer map data, and then the second property edit will be unable to find the instancer to update.  This is totally expected and fine, since the resync will repopulate everything correctly, but our TF_VERIFY statements were a bit too aggressive.

This change relaxes the TF_VERIFY calls in the edit codepath.  I also fixed up a few stray callers of the affected utility functions to do return value checking/TF_VERIFY where appropriate.

Fixes PixarAnimationStudios#1551

(Internal change: 2178348)
  • Loading branch information
tcauchois authored and marktucker committed Sep 16, 2021
1 parent 7707124 commit 372a1c2
Showing 1 changed file with 21 additions and 4 deletions.
25 changes: 21 additions & 4 deletions pxr/usdImaging/usdImaging/instanceAdapter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1261,8 +1261,11 @@ UsdImagingInstanceAdapter::ProcessPropertyChange(UsdPrim const& prim,
_ProtoPrim const& proto = _GetProtoPrim(prim.GetPath(),
cachePath,
&instancerContext);
if (!TF_VERIFY(proto.adapter, "%s", cachePath.GetText())) {
return HdChangeTracker::AllDirty;
if (!proto.adapter) {
// Note: if we can't find the correct prototype, it may have been
// removed by a previous property update, so we can't treat it
// as an error. Instead, we just return Clean.
return HdChangeTracker::Clean;
}

UsdPrim protoPrim = _GetPrim(proto.path);
Expand Down Expand Up @@ -2170,8 +2173,13 @@ UsdImagingInstanceAdapter::_GetProtoPrim(SdfPath const& instancerPath,
}
}
}
if (!TF_VERIFY(r, "instancer = %s, cachePath = %s",
instancerPath.GetText(), cachePath.GetText())) {

if (!r) {
// Note: for some callers, like ProcessPropertyChange, it's possible
// for this call to fail when trying to process property changes on
// things that have already been removed from the instancer map by
// a previous change. Callers that expect this call to succeed should
// TF_VERIFY(r->adapter).
return EMPTY;
}

Expand Down Expand Up @@ -2457,6 +2465,10 @@ UsdImagingInstanceAdapter::GetScenePrimPath(
cachePath.GetAbsoluteRootOrPrimPath(),
cachePath, &instancerContext);

if (!proto.adapter) {
return SdfPath();
}

_InstancerData const* instrData =
TfMapLookupPtr(_instancerData, instancerContext.instancerCachePath);
if (!instrData) {
Expand Down Expand Up @@ -2796,6 +2808,11 @@ UsdImagingInstanceAdapter::GetVolumeFieldDescriptors(
UsdImagingInstancerContext instancerContext;
_ProtoPrim const& proto = _GetProtoPrim(usdPrim.GetPath(),
id, &instancerContext);

if (!TF_VERIFY(proto.adapter, "%s", usdPrim.GetPath().GetText())) {
return HdVolumeFieldDescriptorVector();
}

return proto.adapter->GetVolumeFieldDescriptors(
_GetPrim(proto.path), id, time);
}
Expand Down

0 comments on commit 372a1c2

Please sign in to comment.