Add some defensive coding when calling UsdImagingDelegate::Get #1232
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…n arbitrary USD Primitive and Attribute name.
spiffmon
reviewed
Jun 2, 2020
| .Get(&value, _time), | ||
| UsdPrim prim = _GetUsdPrim(cachePath); | ||
| UsdAttribute attr = prim ? prim.GetAttribute(key) : UsdAttribute(); | ||
| TF_VERIFY(attr && attr.Get(&value, _time), |
There was a problem hiding this comment.
Hey @marktucker ! I think the only concern we might have here is performance fallout. One thing I am fairly confident about is that if the prim is valid, there should be no way that prim.GetAttribute("mumble").Get() would ever crash. So I think you can get away with something like:
TF_VERIFY(prim && prim.GetAttribute(key).Get(&value, _time)) which imposes only one extra validity check.
|
Filed as internal issue #USD-6102. |
…testing the UsdPrim. The UsdAttribute::Get call is safe (and will return false) if the prim exists but the requested attribute does not. Eliminating this check should make this recent change to UsdImagingDelegate::Get a little faster.
|
Thanks @spiffmon. I verified that this modified version still works with my original test case that inspired this change, and have updated the PR branch. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add some defensive coding when calling UsdImagingDelegate::Get with an arbitrary USD Primitive and Attribute name. Render delegates can call this function with any primpath or attribute name, and the existing code could crash in this case. Obviously render delegates shouldn't make these invalid calls, but printing out an error message seems like punishment enough.
This change was inspired by a crash discovered in the Houdini GL render delegate, which I believe is being fed bad Sync requests on UsdVolVDBAsset sprims. I will try to narrow that down to a github issue. This change makes no attempt to address any underlying issue there, but at least it stops the crash (and I didn't see any harm in making this code generally more robust to bad input).
Description of Change(s)
Check for valid UsdPrim and UsdAttribute before using them.