Compare HdPrimvarDesrciptor equality by name and role #2096
Conversation
|
Filed as internal issue #USD-7781 |
|
Hi! Thanks for the submission but I don't think changing the equality operator here is safe. Is there a way to fix the bug by changing UsdImagingPrimAdapter::_MergePrimvar instead? Thanks, |
|
It could be fixed with a custom |
|
Yeah, that sounds good. Although maybe we only want to check name? Unique name = unique primvar, and "interpolation" and "role" are just modifiers on how we access that information. Thanks! |
|
New version looks a lot better! On github it looks like the PR still has the sceneDelegate.h changes but I'm not sure if I'm just looking at the wrong thing. Anyway, we'll pull this down and run it through the test suite and then we can merge it! |
|
Oops, I forgot to revert the original changes. Pushed a commit for that. |
| else | ||
| *it = primvar; | ||
|
|
||
| for (HdPrimvarDescriptorVector::iterator it = vec->begin(); |
There was a problem hiding this comment.
If you're open to the suggestion, I think you can preserve the existing structure and the usage of the STL with std::find_if.
Either way, it's great to see this fix!
Description of Change(s)
Removes interpolation from the
HdPrimvarDescriptor==overload.Fixes Issue(s)
Fixes an issue where
UsdImagingPrimAdapter::_MergePrimvarwill insert a duplicate descriptor into the vector if the interpolation is changed.We observe this issue when calling
SetWidthsInterpolationon basis curves after the widths attribute has been created. The equality check always returns false because the interpolation was changed, so it inserts a duplicate. Our delegate queries each interpolation type, and the last one found wins. This often results in the wrong type, and we can't proceed with some logic.I tried to write tests for this, but could not reproduce with testusdview. The shader used for curves requires the primvar to be defined, it emits a shader compilation error if only the WidthsAttr is present. And it will render in some invalid states, but error in others, like this:
Initial render state:
Storm renders the expected image
Second render state:
Storm renders the expected image despite not changing interpolation.
If you reverse that order, so that it starts with a valid constant state and change to varying, it will fail with a shader error. Even with the operations in a changeblock.