[usdImaging] Allow prototypes from overs, per https://graphics.pixar.com/usd/docs/api/class_usd_geom_point_instancer.html#UsdGeomPointInstancer_protoProcessing #1436

asluk · 2021-01-27T08:58:14Z

Description of Change(s)

Allows instancer prototypes to come from within overs, as described in https://graphics.pixar.com/usd/docs/api/class_usd_geom_point_instancer.html#UsdGeomPointInstancer_protoProcessing and https://groups.google.com/g/usd-interest/c/RJnYoYWbgo8/m/Rh6UV2rlCwAJ . Tested with the attached City_set.

city_protos_over.zip

spitzak · 2021-01-27T17:34:10Z

pxr/usdImaging/usdImaging/delegate.cpp

-    return _displayUnloadedPrimsWithBounds ?
-            UsdPrimIsActive && UsdPrimIsDefined && !UsdPrimIsAbstract :
-            UsdPrimDefaultPredicate;
+    Usd_PrimFlagsConjunction retval = UsdPrimIsActive && !UsdPrimIsAbstract;


This is the same as

UsdPrimIsActive && !UsdPrimIsAbstract && (forPrototypes || UsdPrimIsDefined) && (_displayUnloadedPrimsWithBounds || UsdPrimIsLoaded);

Might make it easier to figure out what the patch does.

Thanks Bill-- to me, it's more clear as is, but I'm happy to change it per your suggestion if there's a consensus to do so. Thanks for looking!

jilliene · 2021-02-03T18:24:43Z

Filed as internal issue #USD-6552

spiffmon

The additional ask we have is if you could turn your nice test file into a testusdview test, also enhancing it to cover the requested changes (I.e. add some defined geometry under an over inside of one of the prototypes and ensure it doesn't get processed). Super-extra bonus points if you have a nested PI that exercises the new logic, but won't hold you to that!

Thanks again, @asluk !

pxr/usdImaging/usdImaging/delegate.cpp

spiffmon · 2021-02-05T22:56:25Z

pxr/usdImaging/usdImaging/delegate.cpp

+    if (!forPrototypes) {
+        retval = retval && UsdPrimIsDefined;


This is actually incorrect. Rather than just leaving out UsdPrimIsDefined – which will wind up processing even "overs" underneath the top-most "over" – we want to substitute UsdPrimHasDefiningSpecifier for UsdPrimIsDefined , which essentially removes the "all ancestor prims must also be defined" clause from the traversal predicate, while still excluding additional overs from consideration.

nested_ptinst.zip

Thanks Spiff-- here's a zip that has a point instancer with a prototype in an over, and a def underneath, in sphereInstancer.usda -- it doesn't draw as expected unless I remove the UsdPrimHasDefiningSpecifier (or
UsdPrimIsDefined) in the predicate. Am I misunderstanding something?

There's also an ni_pi.usda in the zip that puts the point instancer behind scenegraph instancing. No predicate as yet seems to get that drawing as expected. I'll continue investigating, unless you already know what's going on.

I'll also study testusdview a bit and see about getting these smaller test cases into it.

Thanks again!

Hmm.. the reason I thought this might not be working doesn't hold up. There's alot going on right now, but I'll try to take a deeper look and get back to you. First thing to check is whether, outside of imaging, the prototype root prims (which presumably are children of an undefined "over") pass the predicate. If they do not, then maybe we have a core bug.

Hi Spiff, in the sphereInstancer.usda within the zip archive I posted above, I ran the following in usdview.

I think that is correct from a core standpoint, so I don't think there's a way to get the prototype detection logic to descend into overs without removing Usd.PrimHasDefiningSpecifier . But I think I understand your point that you want the prim range to descend into overs, but do not honor prototypes that themselves are pure overs. I'd have to think more about the right logic for that, if that's what the standard should be.

(I haven't had time to dig more into the nested instancing case yet in the zip archive).

Thanks!

(Dummy is an over, Dummy/sphere is a def)

[x for x in Usd.PrimRange( usdviewApi.stage.GetPrimAtPath('/World/Instancer'), Usd.PrimIsActive & Usd.PrimHasDefiningSpecifier & ~Usd.PrimIsAbstract & Usd.PrimIsLoaded)]
[Usd.Prim(</World/Instancer>)]

[x for x in Usd.PrimRange( usdviewApi.stage.GetPrimAtPath('/World/Instancer'), Usd.PrimIsActive & ~Usd.PrimIsAbstract & Usd.PrimIsLoaded)]
[Usd.Prim(</World/Instancer>), Usd.Prim(</World/Instancer/Dummy>), Usd.Prim(</World/Instancer/Dummy/sphere>)]

What we were thinking, and what I thought the example showed, is that the parent of the prototypes (itself a child of the PI) would be a pure over. In our experience, having the prototypes themselves be overs would not even be practical to achieve, since they are references to defined models.

That's correct-- in my attached scene, Dummy is a pure over beneath the PI, and Sphere is a def for the prototype under Dummy. But with UsdPrimHasDefiningSpecifier, traversals won't reach Dummy from the PI.

But it shouldn't need to traverse from the PI, by my read of the code: https://github.com/PixarAnimationStudios/USD/blob/67af68bac3469b328e7f9dff9e0dc6e198ea7a5f/pxr/usdImaging/usdImaging/pointInstancerAdapter.cpp#L199-L233 . It explicitly starts from each prototype root, gleaned via the prototypes relationship. I'm not super familiar with this code, though.

Good point, Spiff. I reran the snippet above on the prototype root, which is a pure over with Sphere def'd under it.

[x for x in Usd.PrimRange( usdviewApi.stage.GetPrimAtPath('/World/Instancer/Dummy'), Usd.PrimIsLoaded & ~Usd.PrimIsAbstract & Usd.PrimIsActive)]
[Usd.Prim(</World/Instancer/Dummy>), Usd.Prim(</World/Instancer/Dummy/sphere>)]

[x for x in Usd.PrimRange( usdviewApi.stage.GetPrimAtPath('/World/Instancer/Dummy'), Usd.PrimIsLoaded & ~Usd.PrimIsAbstract & Usd.PrimIsActive & Usd.PrimHasDefiningSpecifier)]
[]

I think the predicate gets applied right away, which makes sense, so it won't descend below the pure over of Dummy.

I kept UsdPrimHasDefiningSpecifier out of the predicate, but now have it as a condition alongside checking for the adapter, such that pure overs are skipped, but defs that have ancestral overs are honored.

I'm still debugging the nested instancing case.

Thanks again!

Aaron, I thought we’d agreed above yhat the proto roots themselves are not meant to be pure overs, but their parents can be... a “protos” Xform or scope, if you will. Can you keep to that design? Then we can have nice simple code.

Sorry Spiff--- I've been scatterbrained. Fixed. Will continue investigating nested instances and regression testing.

pxr/usdImaging/usdImaging/primAdapter.cpp

spiffmon · 2021-02-23T23:18:59Z

Hey @asluk ! Both your last force-push and the comment that you're still investigating happened 12 days ago. We were wondering if you're still working on this? Thanks!

asluk · 2021-02-24T14:28:07Z

Hey @asluk ! Both your last force-push and the comment that you're still investigating happened 12 days ago. We were wondering if you're still working on this? Thanks!

Hi Spiff! Yes, I'm still working on it, or rather it's still on my plate but backburnered as I investigate some other more pressing internal issues. I'll update here when I make more progress. Thanks!

asluk · 2021-03-01T17:41:23Z

Not quite ready for re-review yet; working out some kinks with the unit test.

asluk · 2021-03-01T17:50:05Z

Hi @spiffmon , I think this is ready for re-review now. I figured out what was happening with native/nested instances, and added a comment to instanceAdapter.cpp with those findings.

The unit test has a point instancer inside of two native instances, and the point instancer has one prototype under an over, and one prototype that is itself an over, and thus ignored.

Apologies for the delay!

….com/usd/docs/api/class_usd_geom_point_instancer.html#UsdGeomPointInstancer_protoProcessing

asluk · 2021-03-01T18:08:26Z

Added an additional test case to cover native instancing with overs as direct prototypes rather than in the nested point instancer prototype.

spiffmon · 2021-03-04T23:49:38Z

Thanks, @asluk ! The code looks good, but when @tallytalwar ran our (unfortunately) internal tests with it, there were several failures, including geometry going missing in a test involving nested point and native instancing... which blows our minds a little because your change should be strictly more permissive! We'll need to make some time to dig into them - sorry for the delay!

spiffmon · 2021-03-12T23:03:46Z

Update: @tcauchois got to the bottom of it! It's a case where a PI prototype is nested inside a native instance, we never traverse the native instance to find it because the root primof NI's has (early) all of its fields stripped out, including its specifier. We must be doing something special in the implementation of IsDefined that causes the root and all of its children to answer as if the root is defined, but HasDefiningSpecifier has no such special logic for prototype root prims. We have an imaging-side workaround for now, and we'll followup to hopefully address this at the Usd-level instead.
I guess we had good reason to think this might not be straightforward ;-) Thanks for persevering, @asluk !

asluk · 2021-03-13T14:52:18Z

Awesome-- much appreciated, @tcauchois and @spiffmon !

change 2160506. This test case was based on TF_DEBUG output which has been shown to be fragile in past test cases. The test case has been migrated to an internal test suite based on image comparisons that we hope to publish in the future. See #1436 (Internal change: 2160769)

spitzak reviewed Jan 27, 2021

View reviewed changes

spiffmon requested changes Feb 5, 2021

View reviewed changes

asluk force-pushed the allow-protos-in-overs branch from f7fcfbe to 67af68b Compare February 8, 2021 13:40

c64kernal added the blocked Issue fix or pull request blocked until questions are answered or pending notes are addressed label Feb 8, 2021

asluk force-pushed the allow-protos-in-overs branch 2 times, most recently from df8ea5b to 0621f65 Compare February 11, 2021 15:48

asluk force-pushed the allow-protos-in-overs branch from 0621f65 to 5f02a57 Compare March 1, 2021 17:32

asluk force-pushed the allow-protos-in-overs branch from 5f02a57 to 65a2620 Compare March 1, 2021 17:46

[usdImaging] Allow prototypes below overs, per https://graphics.pixar…

4a61b0f

….com/usd/docs/api/class_usd_geom_point_instancer.html#UsdGeomPointInstancer_protoProcessing

asluk force-pushed the allow-protos-in-overs branch from 65a2620 to 4a61b0f Compare March 1, 2021 18:07

sunyab added pending push and removed blocked Issue fix or pull request blocked until questions are answered or pending notes are addressed labels Apr 20, 2021

pixar-oss merged commit 883a8dc into PixarAnimationStudios:dev Apr 26, 2021

sunyab removed the pending push label Jun 15, 2021

asluk deleted the allow-protos-in-overs branch October 8, 2021 09:17

cbillingham mentioned this pull request Nov 4, 2024

UsdGeomPointInstancer ComputeExtents not accurate when using prototypes under an over #3395

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[usdImaging] Allow prototypes from overs, per https://graphics.pixar.com/usd/docs/api/class_usd_geom_point_instancer.html#UsdGeomPointInstancer_protoProcessing #1436

[usdImaging] Allow prototypes from overs, per https://graphics.pixar.com/usd/docs/api/class_usd_geom_point_instancer.html#UsdGeomPointInstancer_protoProcessing #1436

asluk commented Jan 27, 2021

spitzak Jan 27, 2021

asluk Jan 27, 2021

jilliene commented Feb 3, 2021

spiffmon left a comment

spiffmon Feb 5, 2021

asluk Feb 8, 2021 •

edited

Loading

spiffmon Feb 9, 2021

asluk Feb 10, 2021

spiffmon Feb 10, 2021

asluk Feb 10, 2021

spiffmon Feb 10, 2021

asluk Feb 11, 2021

spiffmon Feb 11, 2021

asluk Feb 11, 2021

spiffmon commented Feb 23, 2021

asluk commented Feb 24, 2021

asluk commented Mar 1, 2021

asluk commented Mar 1, 2021

asluk commented Mar 1, 2021

spiffmon commented Mar 4, 2021

spiffmon commented Mar 12, 2021

asluk commented Mar 13, 2021

[usdImaging] Allow prototypes from overs, per https://graphics.pixar.com/usd/docs/api/class_usd_geom_point_instancer.html#UsdGeomPointInstancer_protoProcessing #1436

[usdImaging] Allow prototypes from overs, per https://graphics.pixar.com/usd/docs/api/class_usd_geom_point_instancer.html#UsdGeomPointInstancer_protoProcessing #1436

Conversation

asluk commented Jan 27, 2021

Description of Change(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jilliene commented Feb 3, 2021

spiffmon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asluk Feb 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spiffmon commented Feb 23, 2021

asluk commented Feb 24, 2021

asluk commented Mar 1, 2021

asluk commented Mar 1, 2021

asluk commented Mar 1, 2021

spiffmon commented Mar 4, 2021

spiffmon commented Mar 12, 2021

asluk commented Mar 13, 2021

asluk Feb 8, 2021 •

edited

Loading