Point instancers can't be hidden in USD 25.05 & Hydra 2

[robp-sidefx]

In #3011 we had an issue where geometry instanced by a PointInstancer wouldn't be visible. The fix for this (1cea084#diff-d1aaf0c1369d2e06760f26bb7e80b5783b21a10a3a5f7edcf725488aab714b4fR76-R78) was, I think, a bit too heavy-handed, whereby visibility is now always forced to true.

The problem is that if the PointInstancer is itself invisible (directly or through inheritance), the items it's instancing are now always visible.

This is proving a problem in workflows where we want to create native instances of point instancers, hiding the original one.

I think/suspect/hope the solution is to rather than hardcode true for the visibility of the point instances, use the visibility of the point instancer.

Simple example:

#usda 1.0
(
    endTimeCode = 1
    framesPerSecond = 24
    metersPerUnit = 1
    startTimeCode = 1
    timeCodesPerSecond = 24
    upAxis = "Y"
)

def PointInstancer "pointinstancer1" (
    kind = "group"
)
{
    float3[] extent.timeSamples = {
        1: [(-1, -1, -1), (1, 1, 1)],
    }
    int64[] invisibleIds = []
    quath[] orientations = [(1, 0, 0, 0)]
    point3f[] positions = [(0, 0, 0)]
    int[] protoIndices = [0]
    rel prototypes = </pointinstancer1/Prototypes/sphere1>
    token visibility = "invisible"

    def Scope "Prototypes" (
        kind = "group"
    )
    {
        def Sphere "sphere1"
        {
            float3[] extent = [(-1, -1, -1), (1, 1, 1)]
            double radius = 1
            matrix4d xformOp:transform = ( (1, 0, 0, 0), (0, 1, 0, 0), (0, 0, 1, 0), (0, 0, 0, 1) )
            uniform token[] xformOpOrder = ["xformOp:transform"]
        }
    }
}

NI of PI example:

#usda 1.0
(
    endTimeCode = 1
    framesPerSecond = 24
    metersPerUnit = 1
    startTimeCode = 1
    timeCodesPerSecond = 24
    upAxis = "Y"
)

def Xform "instancer1" (
    kind = "group"
)
{
    def Scope "Prototypes" (
        kind = "group"
    )
    {
        token visibility = "invisible"

        def PointInstancer "pointinstancer1" (
            kind = "group"
        )
        {
            float3[] extent.timeSamples = {
                1: [(-1, -1, -1), (1, 1, 1)],
            }
            int64[] invisibleIds = []
            quath[] orientations = [(1, 0, 0, 0)]
            point3f[] positions = [(0, 0, 0)]
            int[] protoIndices = [0]
            rel prototypes = </instancer1/Prototypes/pointinstancer1/Prototypes/sphere1>

            def Scope "Prototypes" (
                kind = "group"
            )
            {
                def Sphere "sphere1"
                {
                    float3[] extent = [(-1, -1, -1), (1, 1, 1)]
                    double radius = 1
                    matrix4d xformOp:transform = ( (1, 0, 0, 0), (0, 1, 0, 0), (0, 0, 1, 0), (0, 0, 0, 1) )
                    uniform token[] xformOpOrder = ["xformOp:transform"]
                }
            }
        }
    }

    def "Instance0" (
        instanceable = true
        prepend references = </instancer1/Prototypes/pointinstancer1>
    )
    {
        matrix4d xformOp:transform:instancer1 = ( (1, 0, 0, 0), (0, 1, 0, 0), (0, 0, 1, 0), (2, 0, 0, 1) )
        uniform token[] xformOpOrder = ["xformOp:transform:instancer1"]
    }
}

[robp-sidefx]

It's perhaps only subtly implied in the image, but testing with hdEmbree will reveal the issue; testing with hdPrman will not.

[tpalmergit15]

Filed as internal issue #USD-11149

(This is an automated message. See here for more information.)

[unhyperbolic]

If an instancer is invisible, the render delegate is supposed to not show anything that the instancer is instancing. I updated Storm to have this behavior:
b58b09f
I propose to update hdEmbree accordingly.

Some more background: Adding this feature to a render delegate seems semantically natural. It is also easier than resolving prototype visibility in the usdImaging scene indices. In particular, given that a render delegate has to support the harder per-instance visibility (mask in the instancer topology schema). In the case of Storm, it is a single "if" in HdStInstancer::_GetInstanceIndices.

Note that this is a departure on how it worked with the USD imaging delegate - which turned out to sometimes produce garbage for the instancer visibility.

Thoughts?

[robp-sidefx]

I see there's a relevant comment in UsdImagingDataSourcePointInstancerMask::GetTypedValue() about PI visibility.

Building on that comment and looking at what you said: "a render delegate has to support the harder per-instance visibility (mask in the instancer topology schema)", I'm going to push back and suggest this should be upstream of the render delegate. The render delegate doesn't apply the masking, that's done in the schema you mentioned, as triggered by the scene delegate. So why couldn't/shouldn't the instancer visibility be similarly handled that way?

[robp-sidefx]

This would certainly be a much simpler solution that what I'd proposed in the PR ( #3769 ), adding 3 lines to HdSceneIndexAdapterSceneDelegate::GetInstanceIndices():

Index: pxr/imaging/hd/sceneIndexAdapterSceneDelegate.cpp
===================================================================
diff --git a/pxr/imaging/hd/sceneIndexAdapterSceneDelegate.cpp b/pxr/imaging/hd/sceneIndexAdapterSceneDelegate.cpp
--- a/pxr/imaging/hd/sceneIndexAdapterSceneDelegate.cpp	(revision bc81617c482cfa6776038dd63dc0a54ebdfd5081)
+++ b/pxr/imaging/hd/sceneIndexAdapterSceneDelegate.cpp	(date 1757073135588)
@@ -2568,6 +2568,10 @@
     HF_MALLOC_TAG_FUNCTION();
     VtIntArray indices;
 
+    if (!GetVisible(instancerId)) {
+        return indices;
+    }
+
     HdSceneIndexPrim prim = _GetInputPrim(instancerId);
 
     if (HdInstancerTopologySchema instancerTopology =

[unhyperbolic]

Good points.

I am actually fine with the three line change to the HdSceneIndexAdapterSceneDelegate. Do you feel the same way? Does invalidation work properly when vis'ing and invis'ing the instancer?

TL;DR Achieving instancer visibility by setting the visibility on all the prototype prims seems a much heavier mechanism to me than emptying the instance indices. I am not sure though where the best place to do this is. I actually disagree with the comment in UsdImagingDataSourcePointInstancerMask::GetTypedValue() in that we try to keep these data sources to simply consume the USD attribute values and implement such behaviors in filtering scene indices. That is probably what we want to do long-term when render delegates are simply scene index observers and there is no more HdSceneIndexAdapterSceneDelegate. For now, I am fine in just parking it in the HdSceneIndexAdapterSceneDelegate since it is just so easy to do there.

[robp-sidefx]

I am actually fine with the three line change to the HdSceneIndexAdapterSceneDelegate. Do you feel the same way?

Yes, I'm okay with that. I'm still of somewhat mixed minds as to where "post-processing" of the scene should continue to be done by the scene/render delegate as opposed to the Scene Index pipeline, but in this case I'm happy with what we've discussed.

Do I need to setup a new PR, or is the discussion and patch above sufficient?

Does invalidation work properly when vis'ing and invis'ing the instancer?

It seems to work just fine in Solaris, I've checked and double-checked :)

[unhyperbolic]

It is fastest if I just do the change. No need for you setup another PR. Thank you.