usdGeom: add validation for gprim parenting

[beersandrew]

Description of Change(s)

• added a validation rule that reports an error on a boundable prim that has a gprim ancestor

Fixes Issue(s)

• This is part of the work to add support for the existing PrimEncapsulation validation from usdchecker (https://github.com/orgs/PixarAnimationStudios/projects/3/views/1?pane=issue&itemId=65424985)

• I have verified that all unit tests pass with the proposed changes

• I have submitted a signed Contributor License Agreement

[nvmkuruc]

Should this be Boundable <%s> has an ancestor prim that is a Gprim?

[beersandrew]

Agreed, updated

[nvmkuruc]

Spiff's comment here suggests that Xformables should also be discouraged.
https://forum.aousd.org/t/clarity-on-nesting-imageables-under-gprims/1603

[marktucker]

I wonder if it would make more sense (and be simpler and faster to run) if we flip this check around and make it so that gprims look at their children/descendants, and raise an error if there is anything under them besides a UsdGeomSubset? Are there other valid descendants for gprims in standard USD? Or would this implementation be too limiting in terms of custom prim types that someone might want to place under gprims? Having written this all down I'm leaning towards leaving the test as-is, but it seems to me like it's worth at least considering.

[nvmkuruc]

I had the same thought @marktucker 👍

I could see an argument in wanting something like Scope to be used for organizing GeomSubsets. But I don't have a strong opinion as to whether or not that should be allowed and arguably the subset properties could be used to organize subsets for user presentation.

# Example of using intermediate scopes to organize subsets
def Mesh "mesh"
{
   def Scope "MaterialSubsets" {
       def GeomSubset "subset_1" { ... }
   }
   def Scope "PhysicsSubsets" {
       def GeomSubset "subset_1" { ... }
    }
}

[beersandrew]

Spiff's comment here suggests that Xformables should also be discouraged. https://forum.aousd.org/t/clarity-on-nesting-imageables-under-gprims/1603

I've included Xformable in the check for now, however following your discussion with @marktucker I also thought to check the Gprims themselves.

I'm not sure what the technical impact or implications of wrapping the Geomsubsets in Scope prims are, but it seems like a reasonable thing to want to do from an organizational standpoint. Would it make sense in that case that a Scope with a Gprim ancestor is valid if and only if it has a Geomsubset descendant?

[marktucker]

Putting GeomSubsets inside scopes is a nice idea. I hope Hydra accepts it... If that organization is acceptable, now we've gone from one "acceptable" descendant type to two, and I think the rule should be left as is. However if GeomSubsets in scopes doesn't work in Hydra, then that would be an argument for checking descendants instead of ancestors.

So I just tried this, and if the GeomSubsets are under a scope, the materials bound to them no longer show up. So I think this is unsupported, at least by Hydra currently, so maybe checking descendants is a good idea after all...

[nvmkuruc]

@marktucker I think even if the acceptable type list increases, your suggestion of checking descendants will always be more efficient than checking ancestors.

[marktucker]

Efficient, yes. But is it correct? My one remaining fear about it is that someone may add a "MyCustomGprimFeature" prim type which is intended to go under a gprim and a descendants check will flag it as an error because the validator doesn't know it's allowed to be there. But maybe this is an edge case not worth worrying about?

Or maybe we'd hit this kind of "exception" problem with the ancestor check (for example, the Scope prim under the gprim would not be flagged as problematic by the current ancestor check unless we start adding special cases as @beersandrew suggested above).

Okay, I'm now firmly on the side of "check descendants, not ancestors".

[jesschimein]

Filed as internal issue #USD-9952

[jesschimein]

/AzurePipelines run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[beersandrew]

Okay, I'm now firmly on the side of "check descendants, not ancestors".

Updated this PR to go in that direction, let me know what you think

[marktucker]

_SubsetFamilies doesn't "break" when it finds an error - it keeps iterating and returns all errors. Maybe the same approach should be used here, pointing out all invalid descendant prims in one run, rather than just returning the first bad prim?

[beersandrew]

I would like to stay consistent however in the case of having multiple skel properties on a prim that does not have the SkelBindingAPI applied, it was suggested to stop after the first one.

#3166 (comment)

Is there a determining factor for when to get all errors vs the first error? (multiple prim errors -> get all, multiple property errors -> get first?) or is it case by case and trying to stay consistent isn't necessary?

[tallytalwar]

I think the context is different. In case of skel binding, the issue is with the non-application of SkelBindingAPI and presence of even a single property which belongs to SkelBindingAPI.

However, here we must report each descendant prim which is illegal within its scope.

[tallytalwar]

@beersandrew looks like you forgot to take the break out here? Was there a reason for this? If not, lmk and I will update it internally, already synced the PR in. Thanks

[jesschimein]

/AzurePipelines run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mattyjams]

Putting GeomSubsets inside scopes is a nice idea. I hope Hydra accepts it... If that organization is acceptable, now we've gone from one "acceptable" descendant type to two, and I think the rule should be left as is. However if GeomSubsets in scopes doesn't work in Hydra, then that would be an argument for checking descendants instead of ancestors.

So I just tried this, and if the GeomSubsets are under a scope, the materials bound to them no longer show up. So I think this is unsupported, at least by Hydra currently, so maybe checking descendants is a good idea after all...

Hey all! Sorry for chiming in late here, but I was recently rooting around in this code and added a bunch of the GeomSubset-related validators in #3123.

As @marktucker found, I don't expect any hierarchical structuring of GeomSubsets to work other than having them as direct descendants of the Imageable they are a subset of. The SubsetParentIsImageable validator tests for this (from GeomSubset prims):

OpenUSD/pxr/usd/usdGeom/validators.cpp

Line 112 in 30392d9

_SubsetParentIsImageable(const UsdPrim& usdPrim)

The subset fetching functions all currently look only at immediate children when looking for subsets, so anything deeper in namespace would not be found, e.g.:

OpenUSD/pxr/usd/usdGeom/subset.cpp

Line 300 in 30392d9

geom.GetPrim().GetFilteredChildren(_GetGeomSubsetPredicate())) {

Given that current restriction, it feels like the SubsetParentIsImageable validator and the GPrimDescendantValidator validator here are testing for almost the same thing? In the context of subsets, the difference would be that you could have a structure with a hierarchy of subsets (.../Imageable/GeomSubset/GeomSubset), and the former validator would complain that the "leaf" subset does not have an Imageable parent while the latter validator would not report any error. They are admittedly testing in different "directions" though, with SubsetParentIsImageable testing a GeomSubset's immediate ancestor, while GPrimDescendantValidator tests all descendants, so the latter would catch other problematic structures that the former would not. I guess I wonder whether there's some way to combine those two or otherwise make it more clear?

Supporting hierarchies of subsets under Imageables might indeed be useful, but it would at least need changes in those fetching functions, if not elsewhere, before they would work.

[beersandrew]

Given that current restriction, it feels like the SubsetParentIsImageable validator and the GPrimDescendantValidator validator here are testing for almost the same thing?

The GPrimDescendantValidator can't have a validation error on a GeomSubset prim and the SubsetParentIsImageable validator can only have a validation error on the GeomSubset prim. Possibly that distinction isn't valuable and if the user runs all validators on all prims, they would get duplicate errors in the Mesh > Scope > GeomSubset hierarchy, once for the Mesh and once for the GeomSubset.

If it's assumed that by default users would run all of the validation rules, then I think it does make sense to specify which of the Mesh or GeomSubset should be invalid. Is there some historic precedent around this where the default blame goes to the parent or the child?

[jesschimein]

/AzurePipelines run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[tallytalwar]

Hey @beersandrew, there are some updated to the dev branch, which will require rebasing / updated here. Can you please do that, once done, I will sync this PR in. Thanks

[beersandrew]

Hey @beersandrew, there are some updated to the dev branch, which will require rebasing / updated here. Can you please do that, once done, I will sync this PR in. Thanks

@tallytalwar This is updated on top of dev now. let me know if you see anything else that needs to be updated

[jesschimein]

/AzurePipelines run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).