PixarAnimationStudios / OpenUSD

Universal Scene Description
http://www.openusd.org
Other
5.48k stars 1.14k forks source link

[al] Invalid Prims Validation in UsdImagingMaterialAdapter::GetMaterialResource() #2930

Closed NickWu closed 1 month ago

NickWu commented 3 months ago

Description of Change(s)

We came crossed this issue in our pipeline where there is a possibility that the UsdPrim passed in to UsdImagingMaterialAdapter::GetMaterialResource() could be invalid so we'd like to propose a prim validation check here.

Fixes Issue(s)

jesschimein commented 3 months ago

Filed as internal issue #USD-9240

FlorianZ commented 3 months ago

Hi @NickWu. Good find! Our theory is that if prim is invalid, material should be invalid, too, but then we "dereference" the prim in generating the TF_RUNTIME_ERROR. We might not accept the PR as is, but will take a look internally! Thanks again!

NickWu commented 3 months ago

Hi @NickWu. Good find! Our theory is that if prim is invalid, material should be invalid, too, but then we "dereference" the prim in generating the TF_RUNTIME_ERROR. We might not accept the PR as is, but will take a look internally! Thanks again!

No worries!

tcauchois commented 2 months ago

Hey Nick,

In our current usage of this function, I'd consider it an error for this function to be passed a null prim. If I replace the early out with an error and an early out, you'd get an error message but no crash. Would that solve your issue?

Thanks! Tom

NickWu commented 2 months ago

Hey Nick,

In our current usage of this function, I'd consider it an error for this function to be passed a null prim. If I replace the early out with an error and an early out, you'd get an error message but no crash. Would that solve your issue?

Thanks! Tom

Hey @tcauchois , I'll give it a try and get back to you. Thank you for the suggestion.

NickWu commented 2 months ago

Hi @tcauchois , I tested it and it worked fine to me. I have updated the code based on your suggestion.

NickWu commented 2 months ago

and get back to you. Thank you for the suggestion.

Hey @tcauchois , I think I might have misunderstood what you meant. I think you meant we only issue an error and early out in the case of a null prim and keep the _GetSceneMaterialsEnabled() check a simple early out?

if (!prim) {
    TF_RUNTIME_ERROR("Received prim is null.");
    return VtValue();
}

if (!_GetSceneMaterialsEnabled()) {
    return VtValue();
}
jesschimein commented 2 months ago

/AzurePipelines run

azure-pipelines[bot] commented 2 months ago
Azure Pipelines successfully started running 1 pipeline(s).
tcauchois commented 2 months ago

Yeah exactly, the code in your comment was what I had in mind. But if the current PR works for you, the code in your comment should as well. Thanks!

tcauchois commented 2 months ago

Hi Nick, could you update your PR with the code proposed above? Thanks!

NickWu commented 2 months ago

Hi Nick, could you update your PR with the code proposed above? Thanks!

Sure thing. I'll update it. Thanks!

NickWu commented 2 months ago

Hi Nick, could you update your PR with the code proposed above? Thanks!

Sure thing. I'll update it. Thanks!

Hi @tcauchois , I have updated the code.

jesschimein commented 1 month ago

/AzurePipelines run

azure-pipelines[bot] commented 1 month ago
Azure Pipelines successfully started running 1 pipeline(s).