HdStMesh is not converting the geom subset material id cache path to an index path

[williamkrick]

Description of Issue

HdTopology holds the sub geoms for mesh rprims. The material ids stored with each sub geom are cache paths, and I don't have a good way to convert them into index paths.

Steps to Reproduce

1. Set a break point in HdStMesh::_UpdateDrawItemsForGeomSubsets. This code reads the geom subset material id's from the geom subsets, and finds the sprim with that id. There is a helper function HdStGetMaterialNetworkShader.
2. In UsdView open a scene that has sub geoms with material assignments, such as fendor_stratocaster.usdz from Apple's ARKit.

It works, and all the materials show. But it only works by accident. The geom subset material id is a cache path which is passed directly into HdRenderIndex::GetSprim, without conversion into an index path! It works because UsdImagingDelegate::ConvertCachePathToIndexPath is a no-op when doing pure usdImaging. The delegate id is SdfPath::AbsoluteRootPath(), and so the cache path is returned as the index path unmodified.

This is a problem for me in MayaUsd because my UsdImagingDelegate does have an id, and so my cache paths and index paths are not equal. With USD 21.11 the scene delegate passed into HdVP2Mesh::Sync is no longer a UsdImagingDelegate, so I can't use dynamic_cast<> and call ConvertCachePathToIndexPath. I can work around this in an unpleasant way by knowing exactly how the cache path to index path conversion is done, duplicating that code, and then using it in HdVP2Mesh to convert my sub geom material ids to index paths. Then I can properly find the sprims in the HdRenderIndex.

For a solution maybe the right answer is that when building the HdTopology convert the material id cache path to an index path. Or maybe the right answer is to expose ConvertCachePathToIndexPath on HdSceneDelegate. Or maybe the right answer is something else, I'll leave it up to y'all to decide what is best.

I would also like to suggest that you change the behavior of ConvertCachePathToIndexPath when doing pure usdImaging so that the cache path and index path are no longer equal. This could be hiding other bugs where a cache path is misused as an index path, or vice versa, which will accidentally work for UsdView but which won't work for anyone who sets the delegate id to something that is not SdfPath::AbsoluteRootPath().

For more information you can see my rambling train of thought while I worked this out in Autodesk/maya-usd#1838. In that PR I'm doing unrelated work to fix selection highlighting in vp2RenderDelegate with USD 21.11, and fixing that allowed enough tests to run that we hit a crash and discovered this issue.

System Information (OS, Hardware)

Windows 10

Package Versions

Usd 21.11, MayaUsd PR 1838, Maya PR 130 but I think earlier Maya versions will also reproduce the issue.

Build Flags

[spiffmon]

Speaking only to your last suggestion, William, I have always pushed back on that because re-prefixing SdfPaths is not cheap and we want usdImaging to be as fast as it can be. Yep, that puts a higher tax on correctness, and we’ve done a couple of passes in the past of making use of the two types of paths clearer and more obvious, but the geomSubset code is recent and is still not yet in high use within the studio, so thanks for pointing this out.

On Thu, Nov 18, 2021 at 12:05 PM William Krick ***@***.***> wrote: Description of Issue HdTopology holds the sub geoms for mesh rprims. The material ids stored with each sub geom are cache paths, and I don't have a good way to convert them into index paths. Steps to Reproduce 1. Set a break point in HdStMesh::_UpdateDrawItemsForGeomSubsets. This code reads the geom subset material id's from the geom subsets, and finds the sprim with that id. There is a helper function HdStGetMaterialNetworkShader. 2. In UsdView open a scene that has sub geoms with material assignments, such as fendor_stratocaster.usdz from Apple's ARKit. It works, and all the materials show. But it only works by accident. The geom subset material id is a cache path which is passed directly into HdRenderIndex::GetSprim, without conversion into an index path! It works because UsdImagingDelegate::ConvertCachePathToIndexPath is a no-op when doing pure usdImaging. The delegate id is SdfPath::AbsoluteRootPath(), and so the cache path is returned as the index path unmodified. This is a problem for me in MayaUsd because my UsdImagingDelegate does have an id, and so my cache paths and index paths are not equal. With USD 21.11 the scene delegate passed into HdVP2Mesh::Sync is no longer a UsdImagingDelegate, so I can't use dynamic_cast<> and call ConvertCachePathToIndexPath. I can work around this in an unpleasant way by knowing exactly how the cache path to index path conversion is done, duplicating that code, and then using it in HdVP2Mesh to convert my sub geom material ids to index paths. Then I can properly find the sprims in the HdRenderIndex. For a solution maybe the right answer is that when building the HdTopology convert the material id cache path to an index path. Or maybe the right answer is to expose ConvertCachePathToIndexPath on HdSceneDelegate. Or maybe the right answer is something else, I'll leave it up to y'all to decide what is best. I would also like to suggest that you change the behavior of ConvertCachePathToIndexPath when doing pure usdImaging so that the cache path and index path are no longer equal. This could be hiding other bugs where a cache path is misused as an index path, or vice versa, which will accidentally work for UsdView but which won't work for anyone who sets the delegate id to something that is not SdfPath::AbsoluteRootPath(). For more information you can see my rambling train of thought while I worked this out in Autodesk/maya-usd#1838 <Autodesk/maya-usd#1838>. In that PR I'm doing unrelated work to fix selection highlighting in vp2RenderDelegate with USD 21.11, and fixing that allowed enough tests to run that we hit a crash and discovered this issue. System Information (OS, Hardware) Windows 10 Package Versions Usd 21.11, MayaUsd PR 1838 <Autodesk/maya-usd#1838>, Maya PR 130 but I think earlier Maya versions will also reproduce the issue. Build Flags — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1687>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABPOU2AEOGJ5HAMGQ37KTV3UMVMB7ANCNFSM5IKPPNYA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

-- --spiffiPhone

[jilliene]

Filed as internal issue #USD-7030

[williamkrick]

@spiffmon performance is a concern for us too, maybe we can brainstorm another approach that gives us performance and correctness? I've hit bugs with paths being accidentally misused before (#1333), and the fact that you have invested time making sure the two types of paths are used correctly indicates to me that we both recognize that the current pattern is not robust.

What about making cache path and index path explicit types (SdfCachePath & SdfIndexPath)? They would just be a thin wrapper around SdfPath, but the compiler wouldn't allow one to be assigned to the other. Function prototypes using paths would be self-documenting and the compiler would tell the user when the wrong type of path is used. Inside ConvertCachePathToIndexPath we could have extra access into the types which allows fast conversion through move operations.

[spiffmon]

That could indeed be a way forward, @williamkrick . I'll suggest that we wait for the Scene Indices revamp of usdImaging (IP now) to land and settle, and then take this concern back up then?

[williamkrick]

That sounds great to me, thanks!