add a way for pxrUsdProxyShape to disable subpath selection with the VP2.0 render delegate

[mattyjams]

I wanted to float this change as a possible solution to #262. As we're testing the VP2.0 render delegate internally at Pixar, we're trying to ensure that the selection behavior of our proxy shapes is consistent when switching back and forth with our current batch renderer viewport integration. Our pxrUsdProxyShape node (and pxrUsdReferenceAssembly nodes which are the primary way our proxies are introduced) aren't really designed to support subpatch selection, so workflows aiming to expose more of the guts of a USD stage should favor one of the other proxy shape types.

[kxl-adsk]

I think it makes sense...but I'm getting concerned where it will bring us if we continue to add virtual methods for things like this on the base class. Maybe it's a good time to introduce flags?

[mattyjams]

Famous last words, I know, but I'm hoping that this is a fairly temporary thing, and that we're not going to have a huge proliferation of new virtuals we'll want to add. Or at least not for the purposes of the Pixar proxy shape anyway.

Our current aim is to get feature (and performance) parity when using pxrUsdProxyShape between the Pixar batch renderer and the VP2.0 render delegate. Once we get there, the next goal would be to start phasing out pxrUsdProxyShape and pxrUsdReferenceAssembly. The Pixar proxy shape won't really offer new functionality at that point, and assemblies are dead tech, so we'll be working towards adopting mayaUsdProxyShape and UFE instead, and we'll want to steer others that way as well.

I'm not sure what you have in mind for flags, but for this particular issue it can't really be a build-time or env var thing. We'll want to be using pxrUsdProxyShape and testing mayaUsdProxyShape both while imaging with the VP2.0 render delegate. The render delegate will need to know how to deal with selection depending on which node type it's working with.

[huidong-chen]

@mattyjams I am not sure whether I understand what you mean by "temporary thing". Does it make sense to add a TODO to remove this code at some point?

The other suggestion is, can you rename the new method to enableUfeSelection(), which is the term we usually use.

[huidong-chen]

I think what @kxl-adsk meant by flags is to introduce a flag in the ProxyRenderDelegate that can be checked in getInstancedSelectionPath(). And it makes sense to me.

[kxl-adsk]

Actually, I was trying to find this pattern we use internally but didn't yet have time. What I had in mind is a simple bitfield which you can set during the construction of an object (either from a base class or derived). In this bitfield, you can encode information about different characteristics of your object, like disable_ufe_selection. This will remove the need for a virtual method.

After writing this, I convinced myself that it's too early to do this. So ignore me.

[huidong-chen]

But the performance concern still makes sense. There seems to be no need to call into this virtual method for every selection hit. Thus I would suggest to query the flag here and use it in selection interpretation.

[kxl-adsk]

Right, good point @HdC-adsk !

[mattyjams]

Thanks @kxl-adsk and @HdC-adsk for the great feedback here.

I just pushed a new version of this in commit 5c8528e1c38ec3771de42f1054990919a06fa6f6. I kept the virtual function, but I renamed it to enableUfeSelection() as @HdC-adsk suggested. It also now gets called and its return value is stored when the ProxyRenderDelegate is constructed. We can then just check the member variable during getInstancedSelectionPath() and avoid the virtual dispatch. It's not expected to vary over the course of a Maya session (or frankly at all), so we don't need to re-query it on every execute.

And in terms of the temporary-ness of this, I think it is temporary insofar as the pxrUsdProxyShape node is temporary. We don't really intend to support UFE workflows with pxrUsdProxyShape and instead are happy to see its usage gradually replaced with mayaUsdProxyShape instead. That led me to think it didn't really warrant a TODO comment, but let me know if you think otherwise.

[huidong-chen]

@mattyjams Makes sense. Thanks.

[kxl-adsk]

I can't convince myself that we want to go this way. There is nothing in the interface that prevents people from attempting implementation of runtime switch between UFE selection and proxy selection via this new virtual method. And then they will get surprised by the cache in VP2RenderDelegate.

The solution I would suggest: have derived class to configure this at the construction of base proxy shape (extra argument in the constructor?).

[mattyjams]

@kxl-adsk Ok, I just posted a new version of this in commit a879ac268c63d1e84b761db4e29c0583e56f537c that removes the virtual entirely and adds a parameter to the MayaUsdProxyShapeBase constructor instead.

You can compare this to the previous iteration in commit 5c8528e1c38ec3771de42f1054990919a06fa6f6.

[kxl-adsk]

@mattyjams What's the remaining value of this member given that we have inline access to it on proxy shape?

[mattyjams]

Good point. None really, I suppose. We can just rely on the compiler to inline away the function call.

Rebased and updated to remove this member variable in commit dab4b46.