feat: add validator SkelBindingAPIAppliedChecker #3166

beersandrew · 2024-07-15T19:20:13Z

Description of Change(s)

add validator SkelBindingAPIAppliedChecker

added a validation check for when a skel binding property is present but the skel binding api is not applied
added a validation check for when a skel binding api is applied to a prim that is not parented by a skel root
added a unit test

Fixes Issue(s)

https://github.com/orgs/PixarAnimationStudios/projects/3/views/1?pane=issue&itemId=65425045

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

I have submitted a signed Contributor License Agreement

- added a validation check for when a skel binding property is present but the skel binding api is not applied - added a validation check for when a skel binding api is applied to a prim that is not parented by a skel root - added a unit test

jesschimein · 2024-07-16T16:50:36Z

Filed as internal issue #USD-9849

jesschimein · 2024-07-16T16:50:37Z

/AzurePipelines run

azure-pipelines · 2024-07-16T16:50:48Z

Azure Pipelines successfully started running 1 pipeline(s).

dgovil · 2024-07-16T20:49:08Z

pxr/usd/usdSkel/testenv/testUsdSkelValidators.cpp

+    TF_AXIOM(validator);
+
+    // Create Stage and mesh with a skel binding property
+    UsdStageRefPtr usdStage = UsdStage::CreateNew("/Users/andrewbeers/test.usda");


Nit: I'm sure you'll remove it but good to just write it to a temp test location instead before it's merged in, and/or just keep it in memory if you don't need to write it out.

dgovil · 2024-07-16T20:51:30Z

pxr/usd/usdSkel/testenv/testUsdSkelValidators.cpp

+
+    UsdValidationErrorVector errors = validator->Validate(mesh.GetPrim());
+
+    // test and verify error for not having a skel binding api applied


Smallest of nitpicks: Comment style should be sentence case just to match the rest of the comments.

dgovil · 2024-07-16T20:54:24Z

pxr/usd/usdSkel/testenv/testUsdSkelValidators.cpp

+    TF_AXIOM(errors[0].GetSites()[0].IsPrim());
+    TF_AXIOM(errors[0].GetSites()[0].GetPrim().GetPath() ==
+             SdfPath("/SkelRoot/Mesh"));
+    expectedErrorMsg = "UsdSkelBindingAPI applied on prim: </SkelRoot/Mesh>, " \


Style nit: You can put the string inside parenthesis and avoid escaping the line break. It's a little cleaner and less fragile if someone forgets in the future.

didn't know about this! thanks!

dgovil · 2024-07-16T20:59:02Z

pxr/usd/usdSkel/validators.cpp

+
+        for (const TfToken &primToken : primPropertyNames){
+            for (const TfToken &skelToken : skelPropertyNames){
+                if (skelToken == primToken){


Nit: If you invert this comparison + use a continue, you can unnest the contents of the check. Ever so slightly improves readability.

dgovil · 2024-07-16T21:00:25Z

pxr/usd/usdSkel/validators.cpp

+    else {
+        if (usdPrim.GetTypeName() != UsdSkelTokens->SkelRoot) {
+            UsdPrim parentPrim = usdPrim.GetParent();
+            while (!parentPrim.IsPseudoRoot()) {


I think you should also check if the parentPrim isn't null, just in case you get a bad prim here.
while (parentPrim && !parentPrim.IsPseudoRoot())

dgovil · 2024-07-16T21:02:24Z

LGTM! other than a couple small notes, most of which are just minor style ones.

- update comments to be in sentence form - remove local path, update to an in memory stage - remove \ from multi-line strings and wrap them in parenthesis instead - reduce nesting in validation code - check parentPrim for null before checking it's type

beersandrew · 2024-07-16T21:21:54Z

LGTM! other than a couple small notes, most of which are just minor style ones.

Made the suggested changes. Thanks so much for the review & ideas.

tallytalwar · 2024-07-17T06:08:41Z

Just curious why is this a draft PR?

tallytalwar

Hey @beersandrew

Thanks for doing this. Left a few suggestions for you to consider. Let me know if you want to talk through any of these or more here or on slack.

pxr/usd/usdSkel/CMakeLists.txt

pxr/usd/usdSkel/testenv/testUsdSkelValidators.cpp

pxr/usd/usdSkel/validators.cpp

pxr/usd/usdSkel/testenv/testUsdSkelValidators.cpp

pxr/usd/usdSkel/validators.cpp

pxr/usd/usdSkel/plugInfo.json

pxr/usd/usdSkel/testenv/testUsdSkelValidators.cpp

- reenabled testUsdSkelRoot, accidentally removed in a previous commit - removed sdr dependency from skel test - removed unused headers - added TestUsdSkelValidators test to verify validator existence - removed SkelBindingApiChecker name from error messages - refactored how skelPropertyNames are collected, stored, and checked against

- no need to catch multiple errors here

tallytalwar · 2024-07-18T04:05:16Z

pxr/usd/usdSkel/validators.cpp

+    UsdValidationErrorVector errors;
+
+    if (!usdPrim.HasAPI<UsdSkelBindingAPI>()){
+        std::vector<TfToken> primPropertyNames = usdPrim.GetPropertyNames();


nitpick: Its a good practice to declare variables just before use. So I would move primPropertyNames below skelPropertyNames.

Also you can make this a const vector.

tallytalwar · 2024-07-18T04:12:51Z

pxr/usd/usdSkel/validators.cpp

+        std::vector<TfToken> primPropertyNames = usdPrim.GetPropertyNames();
+        static const std::unordered_set<TfToken, TfHash> skelPropertyNames = []() {
+            UsdSchemaRegistry& usdSchemaRegistry = UsdSchemaRegistry::GetInstance();
+            std::unique_ptr<UsdPrimDefinition> primDef = usdSchemaRegistry.BuildComposedPrimDefinition(TfToken(), {TfToken("SkelBindingAPI")});


Ahh apologies, I should have noticed this before, you can use UsdSkelTokens->SkelBindingAPI, instead of TfToken("SkelBindingAPI"). Such tokens are often generated along with the schema.

https://github.com/PixarAnimationStudios/OpenUSD/blob/release/pxr/usd/usdSkel/tokens.cpp#L55

tallytalwar · 2024-07-18T04:13:48Z

pxr/usd/usdSkel/validators.cpp

+        static const std::unordered_set<TfToken, TfHash> skelPropertyNames = []() {
+            UsdSchemaRegistry& usdSchemaRegistry = UsdSchemaRegistry::GetInstance();
+            std::unique_ptr<UsdPrimDefinition> primDef = usdSchemaRegistry.BuildComposedPrimDefinition(TfToken(), {TfToken("SkelBindingAPI")});
+            std::vector<TfToken> vectorOfNames = primDef->GetPropertyNames();


const vector

- switched from using TfToken("SkelBindingAPI") to UsdSkelTokens->SkelBindingAPI - use const vectors where possible - refactoring / reordering of code for clarity

tallytalwar

Thanks a bunch @beersandrew. Will sync this in.

jesschimein · 2024-07-18T19:01:22Z

/AzurePipelines run

azure-pipelines · 2024-07-18T19:01:31Z

Azure Pipelines successfully started running 1 pipeline(s).

dgovil reviewed Jul 16, 2024

View reviewed changes

beersandrew requested a review from dgovil July 16, 2024 21:22

tallytalwar reviewed Jul 17, 2024

View reviewed changes

beersandrew added 2 commits July 17, 2024 14:02

refactor: break out once an error is found

2693885

- no need to catch multiple errors here

beersandrew marked this pull request as ready for review July 17, 2024 18:14

beersandrew requested a review from tallytalwar July 17, 2024 18:14

fix: rename checker to validator

19fa668

tallytalwar reviewed Jul 18, 2024

View reviewed changes

fix: addressed PR comments

3ccf73f

- switched from using TfToken("SkelBindingAPI") to UsdSkelTokens->SkelBindingAPI - use const vectors where possible - refactoring / reordering of code for clarity

tallytalwar approved these changes Jul 18, 2024

View reviewed changes

pixar-oss merged commit 7eb18f5 into PixarAnimationStudios:dev Aug 6, 2024
3 of 5 checks passed

beersandrew mentioned this pull request Aug 9, 2024

usdGeom: add validation for gprim parenting #3212

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add validator SkelBindingAPIAppliedChecker #3166

feat: add validator SkelBindingAPIAppliedChecker #3166

beersandrew commented Jul 15, 2024

jesschimein commented Jul 16, 2024

jesschimein commented Jul 16, 2024

azure-pipelines bot commented Jul 16, 2024

dgovil Jul 16, 2024

dgovil Jul 16, 2024

dgovil Jul 16, 2024

beersandrew Jul 16, 2024

dgovil Jul 16, 2024

dgovil Jul 16, 2024

dgovil commented Jul 16, 2024 •

edited

Loading

beersandrew commented Jul 16, 2024

tallytalwar commented Jul 17, 2024

tallytalwar left a comment

tallytalwar Jul 18, 2024 •

edited

Loading

tallytalwar Jul 18, 2024

tallytalwar Jul 18, 2024

tallytalwar left a comment

jesschimein commented Jul 18, 2024

azure-pipelines bot commented Jul 18, 2024


		UsdValidationErrorVector errors = validator->Validate(mesh.GetPrim());

		// test and verify error for not having a skel binding api applied

feat: add validator SkelBindingAPIAppliedChecker #3166

feat: add validator SkelBindingAPIAppliedChecker #3166

Conversation

beersandrew commented Jul 15, 2024

Description of Change(s)

Fixes Issue(s)

jesschimein commented Jul 16, 2024

jesschimein commented Jul 16, 2024

azure-pipelines bot commented Jul 16, 2024

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

dgovil commented Jul 16, 2024 • edited Loading

beersandrew commented Jul 16, 2024

tallytalwar commented Jul 17, 2024

tallytalwar left a comment

Choose a reason for hiding this comment

tallytalwar Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tallytalwar left a comment

Choose a reason for hiding this comment

jesschimein commented Jul 18, 2024

azure-pipelines bot commented Jul 18, 2024

dgovil commented Jul 16, 2024 •

edited

Loading

tallytalwar Jul 18, 2024 •

edited

Loading