usdValidation: usdUtils: MissingReferenceValidator #3450

beersandrew · 2024-12-03T03:54:04Z

Description of Change(s)

created implementation, test, as well as associated entries in plugInfo.json & validatorTokens.h

Link to proposal (if applicable)

Fixes Issue(s)

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

Note: This PR is a replacement for #3403

Checklist

[X] I have created this PR based on the dev branch

[X] I have followed the coding conventions

[X] I have added unit tests that exercise this functionality (Reference:
testing guidelines)

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

[X] I have submitted a signed Contributor License Agreement (Reference:
Contributor License Agreement instructions)

beersandrew · 2024-12-03T03:55:48Z

pxr/usdValidation/usdUtilsValidators/validators.cpp

+            ("Found unresolvable external dependency "
+                "'%s'."), unresolvedPath.c_str())
+        );
+    }


@tallytalwar One note here, it looks like complianceChecker.py is doing another check here which gets some diagnostic information, but it seems like this process of collecting diagnostic information should be started before opening the stage (here). I tried doing this inside the validator with stage.Reload() but was unable to populate the diagnostics this way.

In my specific test, the errors were similar so I wanted your opinion on if it's possible to do something similar here, or if it's necessary to move the diagnostic part over in this case.

I think we might not need an explicit validator for this now! https://github.com/PixarAnimationStudios/OpenUSD/blob/dev/pxr/usdValidation/usdValidation/coreValidators.cpp#L16-L29 should provide coverage for this? Do you want to try running it through the CoreValidator and see if it gives you the appropriate composition error wrapped by a CoreValidation error?

Yep, it does! awesome. I'll close this one out then. Thanks

jesschimein · 2024-12-03T17:46:00Z

Filed as internal issue #USD-10493

jesschimein · 2024-12-03T17:46:02Z

/AzurePipelines run

azure-pipelines · 2024-12-03T17:46:13Z

Azure Pipelines successfully started running 1 pipeline(s).

beersandrew · 2024-12-10T15:17:21Z

Closing since this work is already complete, see Varun's comment above

tallytalwar

A few notes, but otherwise lgtm.

tallytalwar · 2024-12-10T00:45:08Z

pxr/usdValidation/usdUtilsValidators/plugInfo.json

@@ -3,6 +3,9 @@
        {
            "Info": {
                "Validators": {
+                    "MissingReferenceValidator": {
+                        "doc": "The composed USD stage should not contain any unresolvable asset dependencies (in every possible variation of the asset), when using the default asset resolver."


Lets remove the "every possible variation of the asset" thats part of the usdchecker to iterate through various variants. Validator should just be checking a stage/variant.

Lets leave it as is, since UsdUtilsComputeAllDependencies operates at Sdf level and will give unresolved references across variants.

tallytalwar · 2024-12-10T00:46:54Z

pxr/usdValidation/usdUtilsValidators/validatorTokens.h

-#define USD_UTILS_VALIDATOR_KEYWORD_TOKENS                                     \
-    (UsdUtilsValidators)                                                       \
+#define USD_UTILS_VALIDATOR_KEYWORD_TOKENS                                    \
+    (UsdUtilsValidators)                                                      \


Not sure if UsdUtilsValidators should be the name of it! (I also want to move this pxr/usdValidation/usdUtilsValidators to be called something else). :/

From my perspective, the naming started from "which folder in the usd code base does this validator depend on?". Now that the validation code is moved out of those folders, this feels like a broader problem of defining validation domains, which may be fine for the others already (usdGeom, usdShade... etc).

Maybe this validator could go into pxr/usd/usdValidation? I'm not sure if it's important for that project to not have external dependencies on usdUtils or not.

Slightly correction the suffix here is the name of the plugin (the directory name happens to be matching the plugin name :D).

I do plan to move the usdUtilsValidators plugin sometime later so lets leave it as-is for now.

tallytalwar · 2024-12-10T00:58:12Z

pxr/usdValidation/usdUtilsValidators/testenv/testUsdUtilsValidators.cpp

+    const UsdStageRefPtr& stage = UsdStage::CreateInMemory();
+
+    const UsdGeomXform xform = UsdGeomXform::Define(stage, SdfPath("/Xform"));
+    const SdfReference badReference("doesNotExist.usd");


Can you also try adding a non-layer asset which is unresolved / missing and see if the validator reports that?

Might also make sense for the test to include an unresolved layer in a variant which isn't selected in the stage (this is when the composition validator will not report this reference layer in a not-selected variant).

I think i've added the non-layer asset test, but take a look and make sure it's what you wanted. I'll have the variant test added tomorrow

jesschimein · 2024-12-12T17:57:15Z

/AzurePipelines run

azure-pipelines · 2024-12-12T17:57:24Z

Azure Pipelines successfully started running 1 pipeline(s).

jesschimein · 2024-12-13T17:51:40Z

/AzurePipelines run

azure-pipelines · 2024-12-13T17:51:51Z

Azure Pipelines successfully started running 1 pipeline(s).

tallytalwar · 2024-12-18T05:58:36Z

pxr/usdValidation/usdUtilsValidators/testenv/testUsdUtilsValidators.cpp

+
+    const UsdStageRefPtr& stage = UsdStage::CreateInMemory();
+
+    const UsdGeomXform xform = UsdGeomXform::Define(stage, SdfPath("/Xform"));


Though this is fine, but you can also create a prim like:

prim = stage.DefinePrim("/Prim", "Xform")

which won't require you to include usdGeom/xform.h

tallytalwar · 2024-12-18T06:08:40Z

pxr/usdValidation/usdUtilsValidators/testenv/testUsdUtilsValidators.cpp

+    xform.GetPrim().GetReferences().RemoveReference(badLayerReference);
+    const SdfReference badAssetReference("doesNotExist.jpg");
+    xform.GetPrim().GetReferences().AddReference(badAssetReference);


Ahh don't think its legal to have a non-usd as a reference like this.

An asset would be something like this:

OpenUSD/extras/usd/tutorials/simpleShading/simpleShading.usda

Line 48 in 9b0c13b

asset inputs:file = @USDLogoLrg.png@

So what you can do is:

>>> from pixar import Sdf, Usd, UsdShade >>> stage = Usd.Stage.CreateInMemory() >>> shader = UsdShade.Shader.Define(stage, "/Shader") >>> assetInput = shader.CreateInput("notFoundAsset", Sdf.ValueTypeNames.Asset) >>> assetInput.Set("NotFoundTexture.png") True >>> stage.ExportToString() '#usda 1.0\n(\n doc = """Generated from Composed Stage of root layer \n"""\n)\n\ndef Shader "Shader"\n{\n asset inputs:notFoundAsset = @NotFoundTexture.png@\n}\n\n' >>>

Thanks! This has been updated

tallytalwar · 2024-12-18T06:50:24Z

pxr/usdValidation/usdUtilsValidators/testenv/testUsdUtilsValidators.cpp

+    UsdVariantSets variantSets = xform.GetPrim().GetVariantSets();
+    UsdVariantSet colorsVariantSet = variantSets.AddVariantSet("myVariant");
+
+    UsdGeomXform selectedXform = UsdGeomXform::Define(stage, xform.GetPath().AppendChild(TfToken("selected")));
+    UsdGeomXform notSelectedXform = UsdGeomXform::Define(stage, xform.GetPath().AppendChild(TfToken("notSelected")));


Don't think this is correct.

You can refer the authoring variant tutorials here: https://openusd.org/docs/Authoring-Variants.html

to see how you can add a variantSet with 2 variants on a prim, with one having a good reference and other a bad reference. Have the one which has a good reference be the default set on the prim.

Does that make sense?

Yep, thanks for the help here. Should be good now

created implementation, test, as well as associated entries in plugInfo.json & validatorTokens.h

- remove unnecessary usage of usdGeom - corrected how the missing asset test was written - corrected how the variant test was written

beersandrew commented Dec 3, 2024

View reviewed changes

beersandrew mentioned this pull request Dec 3, 2024

usdUtils: Added MissingReferenceValidator #3403

Closed

beersandrew closed this Dec 10, 2024

beersandrew reopened this Dec 10, 2024

tallytalwar reviewed Dec 10, 2024

View reviewed changes

beersandrew force-pushed the validation-usdUtils-MissingReferenceValidator branch from 595a8d6 to b6e248d Compare December 12, 2024 05:52

beersandrew requested a review from tallytalwar December 13, 2024 16:42

tallytalwar reviewed Dec 18, 2024

View reviewed changes

beersandrew added 4 commits December 18, 2024 12:39

usdValidation: usdUtils: MissingReferenceValidator

dedd413

created implementation, test, as well as associated entries in plugInfo.json & validatorTokens.h

feat: adding an asset reference test

a079178

feat: added unused variant test

0fa86de

fix: address PR comments

d71c0a1

- remove unnecessary usage of usdGeom - corrected how the missing asset test was written - corrected how the variant test was written

beersandrew force-pushed the validation-usdUtils-MissingReferenceValidator branch from 73b6890 to d71c0a1 Compare December 18, 2024 18:35

beersandrew requested a review from tallytalwar December 18, 2024 18:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

usdValidation: usdUtils: MissingReferenceValidator #3450

usdValidation: usdUtils: MissingReferenceValidator #3450

beersandrew commented Dec 3, 2024

beersandrew Dec 3, 2024

tallytalwar Dec 7, 2024

beersandrew Dec 10, 2024

jesschimein commented Dec 3, 2024

jesschimein commented Dec 3, 2024

azure-pipelines bot commented Dec 3, 2024

beersandrew commented Dec 10, 2024

tallytalwar left a comment

tallytalwar Dec 10, 2024

tallytalwar Dec 10, 2024

tallytalwar Dec 10, 2024

beersandrew Dec 12, 2024

tallytalwar Dec 12, 2024

tallytalwar Dec 10, 2024

tallytalwar Dec 10, 2024

beersandrew Dec 12, 2024

jesschimein commented Dec 12, 2024

azure-pipelines bot commented Dec 12, 2024

jesschimein commented Dec 13, 2024

azure-pipelines bot commented Dec 13, 2024

tallytalwar Dec 18, 2024

tallytalwar Dec 18, 2024

beersandrew Dec 18, 2024

tallytalwar Dec 18, 2024

beersandrew Dec 18, 2024


		const UsdStageRefPtr& stage = UsdStage::CreateInMemory();

		const UsdGeomXform xform = UsdGeomXform::Define(stage, SdfPath("/Xform"));

usdValidation: usdUtils: MissingReferenceValidator #3450

Are you sure you want to change the base?

usdValidation: usdUtils: MissingReferenceValidator #3450

Conversation

beersandrew commented Dec 3, 2024

Description of Change(s)

Link to proposal (if applicable)

Fixes Issue(s)

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesschimein commented Dec 3, 2024

jesschimein commented Dec 3, 2024

azure-pipelines bot commented Dec 3, 2024

beersandrew commented Dec 10, 2024

tallytalwar left a 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

Choose a reason for hiding this comment

jesschimein commented Dec 12, 2024

azure-pipelines bot commented Dec 12, 2024

jesschimein commented Dec 13, 2024

azure-pipelines bot commented Dec 13, 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