From 3797696371d50c475ec7ce56b3bb2c7ab332c175 Mon Sep 17 00:00:00 2001 From: andybeers Date: Fri, 20 Sep 2024 15:51:55 -0400 Subject: [PATCH 1/6] feat: initial implementation --- pxr/usd/usdShade/plugInfo.json | 5 +- .../testenv/testUsdShadeValidators.cpp | 109 +++--------- pxr/usd/usdShade/validatorTokens.h | 3 - pxr/usd/usdShade/validators.cpp | 160 +++++++++++++++++- 4 files changed, 190 insertions(+), 87 deletions(-) diff --git a/pxr/usd/usdShade/plugInfo.json b/pxr/usd/usdShade/plugInfo.json index c7dcb46998..8b5ef61ae3 100644 --- a/pxr/usd/usdShade/plugInfo.json +++ b/pxr/usd/usdShade/plugInfo.json @@ -149,7 +149,10 @@ }, "MaterialBindingRelationships": { "doc": "All properties named 'material:binding' or in that namespace should be relationships." - }, + }, + "NormalMapTextureValidator" : { + "doc": "UsdUVTexture nodes that feed the _inputs:normals_ of a UsdPreviewSurface must ensure that the data is encoded and scaled properly. Specifically, since normals are expected to be in the range [(-1,-1,-1), (1,1,1)], the Texture node must transform 8-bit textures from their [0..1] range by setting its _inputs:scale_ to (2, 2, 2, 1) and _inputs:bias_ to (-1, -1, -1, 0). Normal map data is commonly expected to be linearly encoded. However, many image-writing tools automatically set the profile of three-channel, 8-bit images to SRGB. To prevent an unwanted transformation, the UsdUVTexture's _inputs:sourceColorSpace_ must be set to raw." + }, "ShaderSdrCompliance": { "doc": "Shader prim's input types must be conforming to their appropriate sdf types in the respective sdr shader.", "schemaTypes": [ diff --git a/pxr/usd/usdShade/testenv/testUsdShadeValidators.cpp b/pxr/usd/usdShade/testenv/testUsdShadeValidators.cpp index fb54cd9f1a..6e0cc6a583 100644 --- a/pxr/usd/usdShade/testenv/testUsdShadeValidators.cpp +++ b/pxr/usd/usdShade/testenv/testUsdShadeValidators.cpp @@ -31,21 +31,23 @@ PXR_NAMESPACE_USING_DIRECTIVE -TF_DEFINE_PRIVATE_TOKENS(_tokens, - ((usdShadePlugin, "usdShade")) -); - void TestUsdShadeValidators() { // This should be updated with every new validator added with the // UsdShadeValidators keyword. const std::set expectedUsdShadeValidatorNames = { - UsdShadeValidatorNameTokens->encapsulationValidator, UsdShadeValidatorNameTokens->materialBindingApiAppliedValidator, UsdShadeValidatorNameTokens->materialBindingRelationships, - UsdShadeValidatorNameTokens->materialBindingCollectionValidator, UsdShadeValidatorNameTokens->shaderSdrCompliance, + UsdShadeValidatorNameTokens->subsetMaterialBindFamilyName, + UsdShadeValidatorNameTokens->subsetsMaterialBindFamily, + UsdShadeValidatorNameTokens->encapsulationValidator + }; + + // This should be updated with every new validator added with the + // UsdGeomSubset keyword. + const std::set expectedUsdGeomSubsetNames = { UsdShadeValidatorNameTokens->subsetMaterialBindFamilyName, UsdShadeValidatorNameTokens->subsetsMaterialBindFamily }; @@ -59,84 +61,30 @@ TestUsdShadeValidators() std::set validatorMetadataNameSet; UsdValidatorMetadataVector metadata = - registry.GetValidatorMetadataForPlugin(_tokens->usdShadePlugin); - TF_AXIOM(metadata.size() == 7); + registry.GetValidatorMetadataForKeyword( + UsdShadeValidatorKeywordTokens->UsdShadeValidators); for (const UsdValidatorMetadata& metadata : metadata) { validatorMetadataNameSet.insert(metadata.name); } - TF_AXIOM(validatorMetadataNameSet == expectedUsdShadeValidatorNames); -} - -void -TestUsdShadeMaterialBindingCollections() -{ - UsdValidationRegistry& registry = UsdValidationRegistry::GetInstance(); - const UsdValidator* validator = registry.GetOrLoadValidatorByName( - UsdShadeValidatorNameTokens->materialBindingCollectionValidator); - TF_AXIOM(validator); - - UsdStageRefPtr usdStage = UsdStage::Open("./badMaterialCollections.usda"); + TF_AXIOM(std::includes(validatorMetadataNameSet.begin(), + validatorMetadataNameSet.end(), + expectedUsdShadeValidatorNames.begin(), + expectedUsdShadeValidatorNames.end())); - // Test prim with relationship to material binding collection - // to a single target fails validation. - { - const SdfPath primPath("/SingleTargetMaterialCollection"); - const UsdPrim usdPrim = usdStage->GetPrimAtPath(primPath); - const UsdValidationErrorVector errors = validator->Validate(usdPrim); + // Repeat the test using a different keyword. + validatorMetadataNameSet.clear(); - TF_AXIOM(errors.size() == 1u); - const TfToken expectedErrorIdentifier( - "usdShade:MaterialBindingCollectionValidator." - "InvalidMaterialCollection"); - - const UsdValidationError& error = errors[0]; - - const SdfPath expectedAttrPath = - primPath.AppendProperty(UsdShadeTokens->materialBindingCollection); - - TF_AXIOM(error.GetIdentifier() == expectedErrorIdentifier); - TF_AXIOM(error.GetType() == UsdValidationErrorType::Error); - TF_AXIOM(error.GetSites().size() == 1u); - const UsdValidationErrorSite& errorSite = error.GetSites()[0]; - TF_AXIOM(errorSite.IsValid()); - TF_AXIOM(errorSite.IsProperty()); - TF_AXIOM(errorSite.GetProperty().GetPath() == expectedAttrPath); - const std::string expectedErrorMsg = - "Collection-based material binding on " - " has 1 target , " - "needs 2: a collection path and a UsdShadeMaterial path."; - TF_AXIOM(error.GetMessage() == expectedErrorMsg); + metadata = registry.GetValidatorMetadataForKeyword( + UsdGeomValidatorKeywordTokens->UsdGeomSubset); + for (const UsdValidatorMetadata& metadata : metadata) { + validatorMetadataNameSet.insert(metadata.name); } - // Test prim with relationship to a material binding collection - // referencing nonexistent resources fails validation. - { - const SdfPath primPath("/IncompleteMaterialCollection/Bind1"); - const UsdPrim usdPrim = usdStage->GetPrimAtPath(primPath); - const UsdValidationErrorVector errors = validator->Validate(usdPrim); - - TF_AXIOM(errors.size() == 1u); - const TfToken expectedErrorIdentifier( - "usdShade:MaterialBindingCollectionValidator.InvalidResourcePath"); - - const UsdValidationError& error = errors[0]; - const SdfPath expectedAttrPath = - primPath.AppendProperty(UsdShadeTokens->materialBindingCollection); - - TF_AXIOM(error.GetIdentifier() == expectedErrorIdentifier); - TF_AXIOM(error.GetType() == UsdValidationErrorType::Error); - TF_AXIOM(error.GetSites().size() == 1u); - const UsdValidationErrorSite& errorSite = error.GetSites()[0]; - TF_AXIOM(errorSite.IsValid()); - TF_AXIOM(errorSite.IsProperty()); - TF_AXIOM(errorSite.GetProperty().GetPath() == expectedAttrPath); - const std::string expectedErrorMsg = - "Collection-based material binding targets an invalid collection" - " ."; - TF_AXIOM(error.GetMessage() == expectedErrorMsg); - } + TF_AXIOM(std::includes(validatorMetadataNameSet.begin(), + validatorMetadataNameSet.end(), + expectedUsdGeomSubsetNames.begin(), + expectedUsdGeomSubsetNames.end())); } void @@ -171,7 +119,7 @@ TestUsdShadeMaterialBindingRelationships() { const UsdValidationError& error = errors[0u]; - const SdfPath expectedAttrPath = + const SdfPath attrPath = primPath.AppendProperty(UsdShadeTokens->materialBinding); TF_AXIOM(error.GetIdentifier() == expectedErrorIdentifier); TF_AXIOM(error.GetType() == UsdValidationErrorType::Error); @@ -179,7 +127,7 @@ TestUsdShadeMaterialBindingRelationships() const UsdValidationErrorSite& errorSite = error.GetSites()[0u]; TF_AXIOM(errorSite.IsValid()); TF_AXIOM(errorSite.IsProperty()); - TF_AXIOM(errorSite.GetProperty().GetPath() == expectedAttrPath); + TF_AXIOM(errorSite.GetProperty().GetPath() == attrPath); const std::string expectedErrorMsg = "Prim has material binding property " "'material:binding' that is not a relationship."; @@ -189,7 +137,7 @@ TestUsdShadeMaterialBindingRelationships() { const UsdValidationError& error = errors[1u]; - const SdfPath expectedAttrPath = + const SdfPath attrPath = primPath.AppendProperty(TfToken( SdfPath::JoinIdentifier( UsdShadeTokens->materialBinding, "someAttribute"))); @@ -200,7 +148,7 @@ TestUsdShadeMaterialBindingRelationships() const UsdValidationErrorSite& errorSite = error.GetSites()[0u]; TF_AXIOM(errorSite.IsValid()); TF_AXIOM(errorSite.IsProperty()); - TF_AXIOM(errorSite.GetProperty().GetPath() == expectedAttrPath); + TF_AXIOM(errorSite.GetProperty().GetPath() == attrPath); const std::string expectedErrorMsg = "Prim has material binding property " "'material:binding:someAttribute' that is not a relationship."; @@ -563,7 +511,6 @@ main() TestUsdShadeValidators(); TestUsdShadeMaterialBindingAPIAppliedValidator(); TestUsdShadeMaterialBindingRelationships(); - TestUsdShadeMaterialBindingCollections(); TestUsdShadeShaderPropertyCompliance(); TestUsdShadeSubsetMaterialBindFamilyName(); TestUsdShadeSubsetsMaterialBindFamily(); diff --git a/pxr/usd/usdShade/validatorTokens.h b/pxr/usd/usdShade/validatorTokens.h index afc2a909b6..3b99f60b4a 100644 --- a/pxr/usd/usdShade/validatorTokens.h +++ b/pxr/usd/usdShade/validatorTokens.h @@ -20,7 +20,6 @@ PXR_NAMESPACE_OPEN_SCOPE ((encapsulationValidator, "usdShade:EncapsulationRulesValidator")) \ ((materialBindingApiAppliedValidator, "usdShade:MaterialBindingApiAppliedValidator")) \ ((materialBindingRelationships, "usdShade:MaterialBindingRelationships")) \ - ((materialBindingCollectionValidator, "usdShade:MaterialBindingCollectionValidator")) \ ((shaderSdrCompliance, "usdShade:ShaderSdrCompliance")) \ ((subsetMaterialBindFamilyName, "usdShade:SubsetMaterialBindFamilyName")) \ ((subsetsMaterialBindFamily, "usdShade:SubsetsMaterialBindFamily")) @@ -33,8 +32,6 @@ PXR_NAMESPACE_OPEN_SCOPE ((invalidConnectableHierarchy, "InvalidConnectableHierarchy")) \ ((missingMaterialBindingAPI, "MissingMaterialBindingAPI")) \ ((materialBindingPropNotARel, "MaterialBindingPropNotARel")) \ - ((invalidMaterialCollection, "InvalidMaterialCollection")) \ - ((invalidResourcePath, "InvalidResourcePath")) \ ((invalidImplSource, "InvalidImplementationSrc")) \ ((missingSourceType, "MissingSourceType")) \ ((missingShaderIdInRegistry, "MissingShaderIdInRegistry")) \ diff --git a/pxr/usd/usdShade/validators.cpp b/pxr/usd/usdShade/validators.cpp index 49656e6d4e..37fe5feb73 100644 --- a/pxr/usd/usdShade/validators.cpp +++ b/pxr/usd/usdShade/validators.cpp @@ -27,9 +27,10 @@ #include #include -PXR_NAMESPACE_OPEN_SCOPE +#include "../ar/resolver.h" -static +PXR_NAMESPACE_OPEN_SCOPE + static UsdValidationErrorVector _EncapsulationValidator(const UsdPrim& usdPrim) { @@ -590,6 +591,157 @@ _SubsetsMaterialBindFamily(const UsdPrim& usdPrim) return errors; } +static +UsdValidationErrorVector +_NormalMapTextureValidator(const UsdPrim& usdPrim) { + + if (!(usdPrim && usdPrim.IsInFamily( + UsdSchemaRegistry::VersionPolicy::All))) { + return {}; + } + + const UsdShadeShader shader(usdPrim); + if (!shader) { + return {}; + } + + TfToken shaderId; + TfToken UsdPreviewSurface("UsdPreviewSurface"); + if (!shader.GetShaderId(&shaderId) || shaderId != UsdPreviewSurface) { + return {}; + } + + const UsdShadeInput normalInput = shader.GetInput(TfToken("normal")); + if (!normalInput) { + return {}; + } + + const UsdShadeAttributeVector valueProducingAttributes = UsdShadeUtils::GetValueProducingAttributes(normalInput); + if (valueProducingAttributes.empty() || valueProducingAttributes[0].GetPrim() == usdPrim) { + return {}; + } + + UsdValidationErrorVector errors; + + const UsdPrim sourcePrim = valueProducingAttributes[0].GetPrim(); + UsdShadeShader sourceShader(sourcePrim); + if (!sourceShader) { + // In theory, could be connected to an interface attribute of a + // parent connectable... not useful, but not an error + const UsdShadeConnectableAPI& connectable = + UsdShadeConnectableAPI(sourcePrim); + + if (!connectable){ + return {}; + } + + errors.emplace_back( + UsdShadeValidationErrorNameTokens->nonShaderConnection, + UsdValidationErrorType::Error, + UsdValidationErrorSites{ + UsdValidationErrorSite(usdPrim.GetStage(), + usdPrim.GetPath()) + }, + TfStringPrintf("UsdPreviewSurface.normal on prim <%s> is connected to a" + " non-Shader prim.", + usdPrim.GetPath().GetText())); + } + + TfToken sourceShaderId; + TfToken UsdUVTexture("UsdUVTexture"); + // We may have failed to fetch an identifier for asset/source-based + // nodes. OR, we could potentially be driven by a UsdPrimvarReader, + // in which case we'd have nothing to validate + if (!shader.GetShaderId(&shaderId) || shaderId != UsdUVTexture) { + return {}; + } + + const UsdShadeInput input = shader.GetInput(TfToken("file")); + + if (!input) { + errors.emplace_back( + UsdShadeValidationErrorNameTokens->nonShaderConnection, + UsdValidationErrorType::Error, + UsdValidationErrorSites{ + UsdValidationErrorSite(usdPrim.GetStage(), + usdPrim.GetPath()) + }, + TfStringPrintf("UsdPreviewSurface.normal on prim <%s> is connected to a" + " non-Shader prim.", + usdPrim.GetPath().GetText())); + } + + const auto getInputValue = [](UsdShadeShader shader, TfToken token, auto& outputValue) -> bool { + + const UsdShadeInput input = shader.GetInput(token); + if (!input) { + return false; + } + const UsdShadeAttributeVector attributes = + UsdShadeUtils::GetValueProducingAttributes(input); + + if (attributes.empty() || attributes.size() != 1 || + !UsdShadeInput::IsInput(attributes[0])) { + return false; + } + + return attributes[0].Get(&outputValue, + UsdTimeCode::EarliestTime()); + }; + + SdfAssetPath textureAssetPath; + bool gotValue = getInputValue(shader, TfToken("file"), textureAssetPath); + + if (!gotValue || textureAssetPath.GetResolvedPath().empty()) { + std::string assetPath = textureAssetPath.GetAssetPath().empty() ? "" : textureAssetPath.GetAssetPath(); + errors.emplace_back( + UsdShadeValidationErrorNameTokens->invalidNormalMapTextureFile, + UsdValidationErrorType::Error, + UsdValidationErrorSites{ + UsdValidationErrorSite(usdPrim.GetStage(), + sourcePrim.GetPath()) + }, + TfStringPrintf("UsdUVTexture prim <%s> has invalid or unresolvable " + "inputs:file of @%s@", + sourcePrim.GetPath().GetText(), assetPath.c_str())); + } + + auto textureIs8Bit = [](std::string resolvedPath) { + + std::string extension = ArGetResolver().GetExtension(resolvedPath); + + extension = TfStringToLower(extension); + + static const std::unordered_set eightBitExtensions = + {"bmp", "tga", "png", "jpg", "jpeg", "tif"}; + + return eightBitExtensions.find(extension) != eightBitExtensions.end(); + }; + + if (!textureIs8Bit(textureAssetPath.GetResolvedPath())) { + return errors; + } + + std::string colorSpace; + gotValue = getInputValue(shader, TfToken("sourceColorSpace"), colorSpace); + if (!gotValue || colorSpace != "raw") { + errors.emplace_back( + UsdShadeValidationErrorNameTokens->invalidNormalMapTextureColorSpace, + UsdValidationErrorType::Error, + UsdValidationErrorSites{ + UsdValidationErrorSite(usdPrim.GetStage(), + sourcePrim.GetPath()) + }, + TfStringPrintf("UsdUVTexture prim <%s> that reads" + " Normal Map @%s@ should set " + "inputs:sourceColorSpace to 'raw'.", + sourcePrim.GetPath().GetText(), + textureAssetPath.GetAssetPath().c_str())); + } + + return errors; +} + TF_REGISTRY_FUNCTION(UsdValidationRegistry) { UsdValidationRegistry ®istry = UsdValidationRegistry::GetInstance(); @@ -602,6 +754,10 @@ TF_REGISTRY_FUNCTION(UsdValidationRegistry) UsdShadeValidatorNameTokens->materialBindingRelationships, _MaterialBindingRelationships); + + registry.RegisterPluginValidator( + UsdShadeValidatorNameTokens->normalMapTextureValidator, + _NormalMapTextureValidator); registry.RegisterPluginValidator( UsdShadeValidatorNameTokens->materialBindingCollectionValidator, _MaterialBindingCollectionValidator); From 43523288201891a8fad6809268cb0e585aa02b7c Mon Sep 17 00:00:00 2001 From: Andy Date: Thu, 10 Oct 2024 10:33:45 -0400 Subject: [PATCH 2/6] feat: finish normalMapTexture implementation and tests - added new validatorTokens - added implementation to match previous complianceChecker implementation - added tests to test each failure condition --- .../testenv/testUsdShadeValidators.cpp | 209 ++++++++++++++++++ .../testUsdShadeValidators/normalMap.jpg | Bin 0 -> 1229 bytes pxr/usd/usdShade/validatorTokens.h | 33 +-- pxr/usd/usdShade/validators.cpp | 181 +++++++++++---- 4 files changed, 363 insertions(+), 60 deletions(-) create mode 100644 pxr/usd/usdShade/testenv/testUsdShadeValidators/normalMap.jpg diff --git a/pxr/usd/usdShade/testenv/testUsdShadeValidators.cpp b/pxr/usd/usdShade/testenv/testUsdShadeValidators.cpp index 6e0cc6a583..1a3910300a 100644 --- a/pxr/usd/usdShade/testenv/testUsdShadeValidators.cpp +++ b/pxr/usd/usdShade/testenv/testUsdShadeValidators.cpp @@ -17,6 +17,7 @@ #include "pxr/usd/usd/validator.h" #include "pxr/usd/usdGeom/validatorTokens.h" #include "pxr/usd/usdGeom/scope.h" +#include "pxr/usd/usdGeom/xform.h" #include "pxr/usd/usdShade/shader.h" #include "pxr/usd/usdShade/shaderDefUtils.h" #include "pxr/usd/usdShade/tokens.h" @@ -39,6 +40,7 @@ TestUsdShadeValidators() const std::set expectedUsdShadeValidatorNames = { UsdShadeValidatorNameTokens->materialBindingApiAppliedValidator, UsdShadeValidatorNameTokens->materialBindingRelationships, + UsdShadeValidatorNameTokens->normalMapTextureValidator, UsdShadeValidatorNameTokens->shaderSdrCompliance, UsdShadeValidatorNameTokens->subsetMaterialBindFamilyName, UsdShadeValidatorNameTokens->subsetsMaterialBindFamily, @@ -505,6 +507,213 @@ TestUsdShadeEncapsulationRulesValidator() } } +void ValidateError(const UsdValidationErrorVector& errors, + const TfToken& expectedErrorIdentifier, + const SdfPath& expectedPrimPath, + const std::string& expectedErrorMsg, + UsdValidationErrorType expectedErrorType = UsdValidationErrorType::Error) +{ + TF_AXIOM(errors.size() == 1); + TF_AXIOM(errors[0].GetIdentifier() == expectedErrorIdentifier); + TF_AXIOM(errors[0].GetType() == expectedErrorType); + TF_AXIOM(errors[0].GetSites().size() == 1); + TF_AXIOM(errors[0].GetSites()[0].IsValid()); + TF_AXIOM(errors[0].GetSites()[0].IsPrim()); + TF_AXIOM(errors[0].GetSites()[0].GetPrim().GetPath() == + expectedPrimPath); + TF_AXIOM(errors[0].GetMessage() == expectedErrorMsg); +} + +void +TestUsdShadeNormalMapTextureValidator() +{ + UsdValidationRegistry ®istry = UsdValidationRegistry::GetInstance(); + const UsdValidator *validator = registry.GetOrLoadValidatorByName( + UsdShadeValidatorNameTokens->normalMapTextureValidator); + TF_AXIOM(validator); + + // Create a Stage, Material, and Two Shaders (UsdPreviewSurface, + // UsdUVTexture). + UsdStageRefPtr usdStage = UsdStage::CreateInMemory(); + UsdShadeMaterial material = UsdShadeMaterial::Define(usdStage, + SdfPath("/RootMaterial")); + + const std::string usdPreviewSurfaceShaderPath = + "/RootMaterial/UsdPreviewSurface"; + UsdShadeShader usdPreviewSurfaceShader = UsdShadeShader::Define( + usdStage, SdfPath(usdPreviewSurfaceShaderPath)); + usdPreviewSurfaceShader.CreateIdAttr( + VtValue(TfToken("UsdPreviewSurface"))); + UsdPrim usdPreviewSurfaceShaderPrim = usdPreviewSurfaceShader.GetPrim(); + + UsdShadeShader usdUvTextureShader = UsdShadeShader::Define( + usdStage, SdfPath("/RootMaterial/NormalTexture")); + usdUvTextureShader.CreateIdAttr(VtValue(TfToken("UsdUVTexture"))); + + // Add initial valid file and sourceColorSpace input values. + std::string textureAssetPath = "./normalMap.jpg"; + UsdShadeInput fileInput = usdUvTextureShader.CreateInput( + TfToken("file"), SdfValueTypeNames->Asset); + fileInput.Set(SdfAssetPath(textureAssetPath)); + UsdShadeInput sourceColorSpaceInput = usdUvTextureShader.CreateInput( + TfToken("sourceColorSpace"), SdfValueTypeNames->Token); + const TfToken rawToken("raw"); + sourceColorSpaceInput.Set(rawToken); + + // Connect the output of the UsdUVTexture Shader to the normal of the + // UsdPreviewSurface Shader. + usdUvTextureShader.CreateOutput(TfToken("rgb"), SdfValueTypeNames->Float3); + UsdShadeInput normalInput = usdPreviewSurfaceShader.CreateInput( + TfToken("normal"), SdfValueTypeNames->Normal3f); + normalInput.ConnectToSource( + SdfPath("/RootMaterial/NormalTexture.outputs:rgb")); + + // Verify invalid bias & scale error, they should exists and do + // not exist at this point. + UsdValidationErrorVector errors = validator->Validate( + usdPreviewSurfaceShaderPrim); + TfToken expectedErrorIdentifier( + "usdShade:NormalMapTextureValidator.NonCompliantBiasAndScale"); + std::string expectedErrorMsg = + TfStringPrintf("UsdUVTexture prim <%s> reads 8 bit Normal Map " + "@./normalMap.jpg@, which requires that " + "inputs:scale be set to (2, 2, 2, 1) and " + "inputs:bias be set to (-1, -1, -1, 0) for proper " + "interpretation as per the UsdPreviewSurface and " + "UsdUVTexture docs.", + usdUvTextureShader.GetPath().GetText()); + ValidateError(errors, + expectedErrorIdentifier, + usdUvTextureShader.GetPath(), + expectedErrorMsg); + + // Add bias and scale, but add a non-compliant bias value. + UsdShadeInput biasInput = usdUvTextureShader.CreateInput( + TfToken("bias"), SdfValueTypeNames->Float4); + const GfVec4f compliantBias = GfVec4f(-1, -1, -1, 0); + const GfVec4f nonCompliantVector = GfVec4f(-9, -9, -9, -9); + biasInput.Set(nonCompliantVector); + UsdShadeInput scaleInput = usdUvTextureShader.CreateInput( + TfToken("scale"), SdfValueTypeNames->Float4); + const GfVec4f compliantScale = GfVec4f(2, 2, 2, 1); + scaleInput.Set(compliantScale); + + // Verify the non-compliant bias value error occurs. + errors = validator->Validate(usdPreviewSurfaceShaderPrim); + expectedErrorIdentifier = TfToken( + "usdShade:NormalMapTextureValidator.NonCompliantBiasValues"); + expectedErrorMsg = + TfStringPrintf("UsdUVTexture prim <%s> reads an 8 bit Normal " + "Map, but has non-standard inputs:bias value of " + "(%.6g, %.6g, %.6g, %.6g). inputs:bias must be set to " + "[-1,-1,-1,0] so as to fulfill the requirements " + "of the normals to be in tangent space of " + "[(-1,-1,-1), (1,1,1)] as documented in the " + "UsdPreviewSurface and UsdUVTexture docs.", + usdUvTextureShader.GetPath().GetText(), + nonCompliantVector[0], nonCompliantVector[1], + nonCompliantVector[2], nonCompliantVector[3]); + ValidateError(errors, + expectedErrorIdentifier, + usdUvTextureShader.GetPath(), + expectedErrorMsg); + + // Update to a compliant bias and a non-compliant scale value. + biasInput.Set(compliantBias); + scaleInput.Set(nonCompliantVector); + + // Verify the non-compliant scale value error occurs. + errors = validator->Validate(usdPreviewSurfaceShaderPrim); + expectedErrorIdentifier = TfToken( + "usdShade:NormalMapTextureValidator.NonCompliantScaleValues"); + expectedErrorMsg = + TfStringPrintf("UsdUVTexture prim <%s> reads an 8 bit Normal " + "Map, but has non-standard inputs:scale value " + "of (%.6g, %.6g, %.6g, %.6g). inputs:scale must " + "be set to (2, 2, 2, 1) so as fulfill the " + "requirements of the normals to be in tangent " + "space of [(-1,-1,-1), (1,1,1)] as documented in " + "the UsdPreviewSurface and UsdUVTexture docs.", + usdUvTextureShader.GetPath().GetText(), + nonCompliantVector[0], nonCompliantVector[1], + nonCompliantVector[2], nonCompliantVector[3]); + ValidateError(errors, + expectedErrorIdentifier, + usdUvTextureShader.GetPath(), + expectedErrorMsg, + UsdValidationErrorType::Warn); + + // Set a compliant scale value, and an invalid sourceColorSpace. + scaleInput.Set(compliantScale); + sourceColorSpaceInput.Set(TfToken("error")); + + // Verify the invalid sourceColorSpace error occurs. + errors = validator->Validate(usdPreviewSurfaceShaderPrim); + expectedErrorIdentifier = TfToken( + "usdShade:NormalMapTextureValidator.InvalidSourceColorSpace"); + expectedErrorMsg = + TfStringPrintf("UsdUVTexture prim <%s> that reads" + " Normal Map @%s@ should set " + "inputs:sourceColorSpace to 'raw'.", + usdUvTextureShader.GetPath().GetText(), + textureAssetPath.c_str()); + ValidateError(errors, + expectedErrorIdentifier, + usdUvTextureShader.GetPath(), + expectedErrorMsg); + + // Correct the sourceColorSpace, hook up the normal input of + // UsdPreviewSurface to a non-shader output. + sourceColorSpaceInput.Set(rawToken); + UsdGeomXform nonShaderPrim = UsdGeomXform::Define( + usdStage, SdfPath("/RootMaterial/Xform")); + UsdShadeConnectableAPI connectableNonShaderAPI(nonShaderPrim.GetPrim()); + UsdShadeOutput nonShaderOutput = connectableNonShaderAPI.CreateOutput( + TfToken("myOutput"), SdfValueTypeNames->Float3); + nonShaderOutput.Set(GfVec3f(1.0f, 2.0f, 3.0f)); + normalInput.ConnectToSource(nonShaderOutput); + + // Verify a non-shader connection error occurs. + errors = validator->Validate(usdPreviewSurfaceShaderPrim); + expectedErrorIdentifier = TfToken( + "usdShade:NormalMapTextureValidator.NonShaderConnection"); + expectedErrorMsg = + TfStringPrintf("UsdPreviewSurface.normal on prim <%s> is connected " + "to a non-Shader prim.", + usdPreviewSurfaceShaderPath.c_str()); + ValidateError(errors, + expectedErrorIdentifier, + usdPreviewSurfaceShader.GetPath(), + expectedErrorMsg); + + // Set the normal input back to a valid shader and update the file input + // to an invalid file path. + normalInput.ConnectToSource( + SdfPath("/RootMaterial/NormalTexture.outputs:rgb")); + fileInput.Set(SdfAssetPath("./doesNotExist.jpg")); + + // Verify the invalid input file error occurs. + errors = validator->Validate(usdPreviewSurfaceShaderPrim); + expectedErrorIdentifier = + TfToken("usdShade:NormalMapTextureValidator.InvalidFile"); + expectedErrorMsg = + TfStringPrintf("UsdUVTexture prim <%s> has invalid or unresolvable " + "inputs:file of @%s@", + usdUvTextureShader.GetPath().GetText(), + "./doesNotExist.jpg"); + ValidateError(errors, + expectedErrorIdentifier, + usdUvTextureShader.GetPath(), + expectedErrorMsg); + + // Reset the file to a valid path. + fileInput.Set(SdfAssetPath("./normalMap.jpg")); + + // Verify no errors exist. + errors = validator->Validate(usdPreviewSurfaceShaderPrim); + TF_AXIOM(errors.empty()); +} + int main() { diff --git a/pxr/usd/usdShade/testenv/testUsdShadeValidators/normalMap.jpg b/pxr/usd/usdShade/testenv/testUsdShadeValidators/normalMap.jpg new file mode 100644 index 0000000000000000000000000000000000000000..562ac9304ca25b07dab155cdbc4cf30d691c5a0b GIT binary patch literal 1229 zcmb7DO=uHA6#iy6+oZ`R-EB5Ch~O5Y2T4Ix#9K7cgh+}ZR%olB$>tZ5G;C6O5qc54 z7CiWKY1M-l51#ZY(yJf{+Ny|n^599)i`4PWrZyFl6yMJ7e7yI4Z{EC_gZsK<|cXC$Wc5# zK)>J7XGv8cRSgy?&SC)#~zc_c6Pg}0LZOg4wl6$kgvX7LA(AkeG%jUSQAQ2F&u{#< z@fXlNbj_K}bVxdrb;j3WbT7VW#I|{aSNvH28h-~>W{@;lqq}?LpkQhJSRT?cMOqdF uy+1)_EXIZrq#eS*B}QW1B?b~$Y;yT;(auS0t9z8B?Nw5jFl0p{Z2kcd-oi8h literal 0 HcmV?d00001 diff --git a/pxr/usd/usdShade/validatorTokens.h b/pxr/usd/usdShade/validatorTokens.h index 3b99f60b4a..8781279859 100644 --- a/pxr/usd/usdShade/validatorTokens.h +++ b/pxr/usd/usdShade/validatorTokens.h @@ -27,19 +27,26 @@ PXR_NAMESPACE_OPEN_SCOPE #define USD_SHADE_VALIDATOR_KEYWORD_TOKENS \ (UsdShadeValidators) -#define USD_SHADE_VALIDATION_ERROR_NAME_TOKENS \ - ((connectableInNonContainer, "ConnectableInNonContainer")) \ - ((invalidConnectableHierarchy, "InvalidConnectableHierarchy")) \ - ((missingMaterialBindingAPI, "MissingMaterialBindingAPI")) \ - ((materialBindingPropNotARel, "MaterialBindingPropNotARel")) \ - ((invalidImplSource, "InvalidImplementationSrc")) \ - ((missingSourceType, "MissingSourceType")) \ - ((missingShaderIdInRegistry, "MissingShaderIdInRegistry")) \ - ((missingSourceTypeInRegistry, "MissingSourceTypeInRegistry")) \ - ((incompatShaderPropertyWarning, "IncompatShaderPropertyWarning")) \ - ((mismatchPropertyType, "MismatchedPropertyType")) \ - ((missingFamilyNameOnGeomSubset, "MissingFamilyNameOnGeomSubset")) \ - ((invalidFamilyType, "InvalidFamilyType")) \ +#define USD_SHADE_VALIDATION_ERROR_NAME_TOKENS \ + ((connectableInNonContainer, "ConnectableInNonContainer")) \ + ((invalidConnectableHierarchy, "InvalidConnectableHierarchy")) \ + ((missingMaterialBindingAPI, "MissingMaterialBindingAPI")) \ + ((materialBindingPropNotARel, "MaterialBindingPropNotARel")) \ + ((invalidImplSource, "InvalidImplementationSrc")) \ + ((missingSourceType, "MissingSourceType")) \ + ((missingShaderIdInRegistry, "MissingShaderIdInRegistry")) \ + ((missingSourceTypeInRegistry, "MissingSourceTypeInRegistry")) \ + ((incompatShaderPropertyWarning, "IncompatShaderPropertyWarning")) \ + ((mismatchPropertyType, "MismatchedPropertyType")) \ + ((missingFamilyNameOnGeomSubset, "MissingFamilyNameOnGeomSubset")) \ + ((invalidFamilyType, "InvalidFamilyType")) \ + ((nonShaderConnection, "NonShaderConnection")) \ + ((invalidNormalMapTextureFile, "InvalidNormalMapTextureFile")) \ + ((invalidNormalMapTextureColorSpace, "InvalidNormalMapTextureColorSpace")) \ + ((nonCompliantBiasAndScale, "NonCompliantBiasAndScale")) \ + ((invalidShaderPrim, "InvalidShaderPrim")) \ + ((nonCompliantScale, "NonCompliantScaleValues")) \ + ((nonCompliantBias, "NonCompliantBiasValues")) \ /// \def USD_SHADE_VALIDATOR_NAME_TOKENS /// Tokens representing validator names. Note that for plugin provided diff --git a/pxr/usd/usdShade/validators.cpp b/pxr/usd/usdShade/validators.cpp index 37fe5feb73..09892007aa 100644 --- a/pxr/usd/usdShade/validators.cpp +++ b/pxr/usd/usdShade/validators.cpp @@ -595,18 +595,32 @@ static UsdValidationErrorVector _NormalMapTextureValidator(const UsdPrim& usdPrim) { - if (!(usdPrim && usdPrim.IsInFamily( - UsdSchemaRegistry::VersionPolicy::All))) { + if (!usdPrim.IsA()) { return {}; } const UsdShadeShader shader(usdPrim); if (!shader) { - return {}; + return { + UsdValidationError{ + UsdShadeValidationErrorNameTokens->invalidShaderPrim, + UsdValidationErrorType::Error, + UsdValidationErrorSites{ + UsdValidationErrorSite(usdPrim.GetStage(), + usdPrim.GetPath()) + }, + TfStringPrintf("Invalid shader prim <%s>.", + usdPrim.GetPath().GetText()) + } + }; } TfToken shaderId; TfToken UsdPreviewSurface("UsdPreviewSurface"); + + // We may have failed to fetch an identifier for asset/source-based + // nodes. OR, we could potentially be driven by a UsdPrimvarReader, + // in which case we'd have nothing to validate if (!shader.GetShaderId(&shaderId) || shaderId != UsdPreviewSurface) { return {}; } @@ -621,8 +635,6 @@ _NormalMapTextureValidator(const UsdPrim& usdPrim) { return {}; } - UsdValidationErrorVector errors; - const UsdPrim sourcePrim = valueProducingAttributes[0].GetPrim(); UsdShadeShader sourceShader(sourcePrim); if (!sourceShader) { @@ -631,71 +643,67 @@ _NormalMapTextureValidator(const UsdPrim& usdPrim) { const UsdShadeConnectableAPI& connectable = UsdShadeConnectableAPI(sourcePrim); - if (!connectable){ + if (connectable){ return {}; } - errors.emplace_back( - UsdShadeValidationErrorNameTokens->nonShaderConnection, - UsdValidationErrorType::Error, - UsdValidationErrorSites{ + return { + UsdValidationError{ + UsdShadeValidationErrorNameTokens->nonShaderConnection, + UsdValidationErrorType::Error, + UsdValidationErrorSites{ UsdValidationErrorSite(usdPrim.GetStage(), usdPrim.GetPath()) - }, - TfStringPrintf("UsdPreviewSurface.normal on prim <%s> is connected to a" + }, + TfStringPrintf("UsdPreviewSurface.normal on prim <%s> is connected to a" " non-Shader prim.", - usdPrim.GetPath().GetText())); + usdPrim.GetPath().GetText()) + } + }; } TfToken sourceShaderId; TfToken UsdUVTexture("UsdUVTexture"); + + bool gotShaderSourceId = sourceShader.GetShaderId(&sourceShaderId); + // We may have failed to fetch an identifier for asset/source-based // nodes. OR, we could potentially be driven by a UsdPrimvarReader, // in which case we'd have nothing to validate - if (!shader.GetShaderId(&shaderId) || shaderId != UsdUVTexture) { + if (!gotShaderSourceId || sourceShaderId != UsdUVTexture) { return {}; } - const UsdShadeInput input = shader.GetInput(TfToken("file")); - - if (!input) { - errors.emplace_back( - UsdShadeValidationErrorNameTokens->nonShaderConnection, - UsdValidationErrorType::Error, - UsdValidationErrorSites{ - UsdValidationErrorSite(usdPrim.GetStage(), - usdPrim.GetPath()) - }, - TfStringPrintf("UsdPreviewSurface.normal on prim <%s> is connected to a" - " non-Shader prim.", - usdPrim.GetPath().GetText())); - } - - const auto getInputValue = [](UsdShadeShader shader, TfToken token, auto& outputValue) -> bool { - - const UsdShadeInput input = shader.GetInput(token); + const auto getInputValue = [](const UsdShadeShader &inputShader, const TfToken &token, auto& outputValue) -> bool { + const UsdShadeInput input = inputShader.GetInput(token); if (!input) { return false; } - const UsdShadeAttributeVector attributes = + const UsdShadeAttributeVector valueProducingAttributes = UsdShadeUtils::GetValueProducingAttributes(input); - if (attributes.empty() || attributes.size() != 1 || - !UsdShadeInput::IsInput(attributes[0])) { + if (valueProducingAttributes.empty() || + valueProducingAttributes.size() != 1 || + !UsdShadeInput::IsInput(valueProducingAttributes[0])) { return false; } - return attributes[0].Get(&outputValue, + return valueProducingAttributes[0].Get(&outputValue, UsdTimeCode::EarliestTime()); }; SdfAssetPath textureAssetPath; - bool gotValue = getInputValue(shader, TfToken("file"), textureAssetPath); + bool valueForFileExists = getInputValue(sourceShader, TfToken("file"), + textureAssetPath); - if (!gotValue || textureAssetPath.GetResolvedPath().empty()) { - std::string assetPath = textureAssetPath.GetAssetPath().empty() ? "" : textureAssetPath.GetAssetPath(); + UsdValidationErrorVector errors; + + if (!valueForFileExists || textureAssetPath.GetResolvedPath().empty()) { + std::string assetPath = !textureAssetPath.GetAssetPath().empty() + ? textureAssetPath.GetAssetPath() + : ""; errors.emplace_back( - UsdShadeValidationErrorNameTokens->invalidNormalMapTextureFile, + UsdShadeValidationErrorNameTokens->invalidFile, UsdValidationErrorType::Error, UsdValidationErrorSites{ UsdValidationErrorSite(usdPrim.GetStage(), @@ -709,9 +717,7 @@ _NormalMapTextureValidator(const UsdPrim& usdPrim) { auto textureIs8Bit = [](std::string resolvedPath) { std::string extension = ArGetResolver().GetExtension(resolvedPath); - extension = TfStringToLower(extension); - static const std::unordered_set eightBitExtensions = {"bmp", "tga", "png", "jpg", "jpeg", "tif"}; @@ -719,14 +725,17 @@ _NormalMapTextureValidator(const UsdPrim& usdPrim) { }; if (!textureIs8Bit(textureAssetPath.GetResolvedPath())) { + // Nothing more is required for image depths > 8 bits, which + // we assume FOR NOW, are floating point return errors; } - std::string colorSpace; - gotValue = getInputValue(shader, TfToken("sourceColorSpace"), colorSpace); - if (!gotValue || colorSpace != "raw") { + TfToken colorSpace; + TfToken rawColorSpace("raw"); + bool valueForColorSpaceExists = getInputValue(sourceShader, TfToken("sourceColorSpace"), colorSpace); + if (!valueForColorSpaceExists || colorSpace != rawColorSpace) { errors.emplace_back( - UsdShadeValidationErrorNameTokens->invalidNormalMapTextureColorSpace, + UsdShadeValidationErrorNameTokens->invalidSourceColorSpace, UsdValidationErrorType::Error, UsdValidationErrorSites{ UsdValidationErrorSite(usdPrim.GetStage(), @@ -739,6 +748,84 @@ _NormalMapTextureValidator(const UsdPrim& usdPrim) { textureAssetPath.GetAssetPath().c_str())); } + GfVec4f bias; + bool valueForBiasExists = getInputValue(sourceShader, TfToken("bias"), bias); + + GfVec4f scale; + bool valueForScaleExists = getInputValue(sourceShader, TfToken("scale"), scale); + + if (!(valueForBiasExists && valueForScaleExists)) + { + errors.emplace_back( + UsdShadeValidationErrorNameTokens->nonCompliantBiasAndScale, + UsdValidationErrorType::Error, + UsdValidationErrorSites{ + UsdValidationErrorSite(usdPrim.GetStage(), sourcePrim.GetPath()) + }, + TfStringPrintf("UsdUVTexture prim <%s> reads 8 bit Normal Map " + "@%s@, which requires that inputs:scale be set to " + "(2, 2, 2, 1) and inputs:bias be set to " + "(-1, -1, -1, 0) for proper interpretation as per " + "the UsdPreviewSurface and UsdUVTexture docs.", + sourcePrim.GetPath().GetText(), textureAssetPath.GetAssetPath().c_str()) + ); + return errors; + } + + // We still warn for inputs:scale not conforming to UsdPreviewSurface + // guidelines, as some authoring tools may rely on this to scale an + // effect of normal perturbations. + // don't really care about fourth components... + bool nonCompliantScaleValues = scale[0] != 2 || + scale[1] != 2 || scale[2] != 2; + + if (nonCompliantScaleValues) + { + errors.emplace_back( + UsdShadeValidationErrorNameTokens->nonCompliantScale, + UsdValidationErrorType::Warn, + UsdValidationErrorSites{ + UsdValidationErrorSite(usdPrim.GetStage(), sourcePrim.GetPath()) + }, + TfStringPrintf("UsdUVTexture prim <%s> reads an 8 bit Normal " + "Map, but has non-standard inputs:scale value " + "of (%.6g, %.6g, %.6g, %.6g). inputs:scale must be set to " + "(2, 2, 2, 1) so as fulfill the requirements " + "of the normals to be in tangent space of " + "[(-1,-1,-1), (1,1,1)] as documented in the " + "UsdPreviewSurface and UsdUVTexture docs.", + sourcePrim.GetPath().GetText(), + scale[0], scale[1], scale[2], scale[3]) + ); + } + + // Note that for a 8bit normal map, inputs:bias must be appropriately + // set to [-1, -1, -1, 0] to fulfill the requirements of the + // normals to be in tangent space of [(-1,-1,-1), (1,1,1)] as documented + // in the UsdPreviewSurface docs. Note this is true only when scale + // values are respecting the requirements laid in the + // UsdPreviewSurface / UsdUVTexture docs. We continue to warn! + if (!nonCompliantScaleValues && (bias[0] != -1 || bias[1] != -1 || + bias[2] != -1)) + { + errors.emplace_back( + UsdShadeValidationErrorNameTokens->nonCompliantBias, + UsdValidationErrorType::Error, + UsdValidationErrorSites{ + UsdValidationErrorSite(usdPrim.GetStage(), sourcePrim.GetPath()) + }, + TfStringPrintf("UsdUVTexture prim <%s> reads an 8 bit Normal " + "Map, but has non-standard inputs:bias value of " + "(%.6g, %.6g, %.6g, %.6g). inputs:bias must be set to " + "[-1,-1,-1,0] so as to fulfill the requirements " + "of the normals to be in tangent space of " + "[(-1,-1,-1), (1,1,1)] as documented in the " + "UsdPreviewSurface and UsdUVTexture docs.", + sourcePrim.GetPath().GetText(), + bias[0], bias[1], bias[2], bias[3]) + ); + } + return errors; } @@ -754,10 +841,10 @@ TF_REGISTRY_FUNCTION(UsdValidationRegistry) UsdShadeValidatorNameTokens->materialBindingRelationships, _MaterialBindingRelationships); - registry.RegisterPluginValidator( UsdShadeValidatorNameTokens->normalMapTextureValidator, _NormalMapTextureValidator); + registry.RegisterPluginValidator( UsdShadeValidatorNameTokens->materialBindingCollectionValidator, _MaterialBindingCollectionValidator); From 1f54c9b39d3038a4348b456bb4039c50d0ef72ea Mon Sep 17 00:00:00 2001 From: Andy Date: Thu, 10 Oct 2024 10:38:55 -0400 Subject: [PATCH 3/6] fix: adjust include path --- pxr/usd/usdShade/validators.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/pxr/usd/usdShade/validators.cpp b/pxr/usd/usdShade/validators.cpp index 09892007aa..5fc1ed5daf 100644 --- a/pxr/usd/usdShade/validators.cpp +++ b/pxr/usd/usdShade/validators.cpp @@ -21,14 +21,12 @@ #include "pxr/usd/usdShade/shader.h" #include "pxr/usd/usdShade/tokens.h" #include "pxr/usd/usdShade/validatorTokens.h" -#include "pxr/usd/usdShade/materialBindingAPI.h" +#include "pxr/usd/ar/resolver.h" #include #include #include -#include "../ar/resolver.h" - PXR_NAMESPACE_OPEN_SCOPE static UsdValidationErrorVector From 0e796e53e696a5251dbbbdcb90975819817e5975 Mon Sep 17 00:00:00 2001 From: Andy Date: Thu, 10 Oct 2024 10:46:45 -0400 Subject: [PATCH 4/6] fix: update validatorToken names --- pxr/usd/usdShade/validatorTokens.h | 40 +++++++++++++++--------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/pxr/usd/usdShade/validatorTokens.h b/pxr/usd/usdShade/validatorTokens.h index 8781279859..8a3cf0e53c 100644 --- a/pxr/usd/usdShade/validatorTokens.h +++ b/pxr/usd/usdShade/validatorTokens.h @@ -27,26 +27,26 @@ PXR_NAMESPACE_OPEN_SCOPE #define USD_SHADE_VALIDATOR_KEYWORD_TOKENS \ (UsdShadeValidators) -#define USD_SHADE_VALIDATION_ERROR_NAME_TOKENS \ - ((connectableInNonContainer, "ConnectableInNonContainer")) \ - ((invalidConnectableHierarchy, "InvalidConnectableHierarchy")) \ - ((missingMaterialBindingAPI, "MissingMaterialBindingAPI")) \ - ((materialBindingPropNotARel, "MaterialBindingPropNotARel")) \ - ((invalidImplSource, "InvalidImplementationSrc")) \ - ((missingSourceType, "MissingSourceType")) \ - ((missingShaderIdInRegistry, "MissingShaderIdInRegistry")) \ - ((missingSourceTypeInRegistry, "MissingSourceTypeInRegistry")) \ - ((incompatShaderPropertyWarning, "IncompatShaderPropertyWarning")) \ - ((mismatchPropertyType, "MismatchedPropertyType")) \ - ((missingFamilyNameOnGeomSubset, "MissingFamilyNameOnGeomSubset")) \ - ((invalidFamilyType, "InvalidFamilyType")) \ - ((nonShaderConnection, "NonShaderConnection")) \ - ((invalidNormalMapTextureFile, "InvalidNormalMapTextureFile")) \ - ((invalidNormalMapTextureColorSpace, "InvalidNormalMapTextureColorSpace")) \ - ((nonCompliantBiasAndScale, "NonCompliantBiasAndScale")) \ - ((invalidShaderPrim, "InvalidShaderPrim")) \ - ((nonCompliantScale, "NonCompliantScaleValues")) \ - ((nonCompliantBias, "NonCompliantBiasValues")) \ +#define USD_SHADE_VALIDATION_ERROR_NAME_TOKENS \ + ((connectableInNonContainer, "ConnectableInNonContainer")) \ + ((invalidConnectableHierarchy, "InvalidConnectableHierarchy")) \ + ((missingMaterialBindingAPI, "MissingMaterialBindingAPI")) \ + ((materialBindingPropNotARel, "MaterialBindingPropNotARel")) \ + ((invalidImplSource, "InvalidImplementationSrc")) \ + ((missingSourceType, "MissingSourceType")) \ + ((missingShaderIdInRegistry, "MissingShaderIdInRegistry")) \ + ((missingSourceTypeInRegistry, "MissingSourceTypeInRegistry")) \ + ((incompatShaderPropertyWarning, "IncompatShaderPropertyWarning")) \ + ((mismatchPropertyType, "MismatchedPropertyType")) \ + ((missingFamilyNameOnGeomSubset, "MissingFamilyNameOnGeomSubset")) \ + ((invalidFamilyType, "InvalidFamilyType")) \ + ((nonShaderConnection, "NonShaderConnection")) \ + ((invalidFile, "InvalidFile")) \ + ((invalidShaderPrim, "InvalidShaderPrim")) \ + ((invalidSourceColorSpace, "InvalidSourceColorSpace")) \ + ((nonCompliantBiasAndScale, "NonCompliantBiasAndScale")) \ + ((nonCompliantScale, "NonCompliantScaleValues")) \ + ((nonCompliantBias, "NonCompliantBiasValues")) \ /// \def USD_SHADE_VALIDATOR_NAME_TOKENS /// Tokens representing validator names. Note that for plugin provided From 454ae8c61ca91e9ceb9eb1b72161b2f518c9f977 Mon Sep 17 00:00:00 2001 From: Andy Date: Wed, 23 Oct 2024 10:45:19 -0400 Subject: [PATCH 5/6] fix: fixing various rebasing issues --- .../testenv/testUsdShadeValidators.cpp | 110 +++++++++++++----- pxr/usd/usdShade/validatorTokens.h | 14 ++- 2 files changed, 91 insertions(+), 33 deletions(-) diff --git a/pxr/usd/usdShade/testenv/testUsdShadeValidators.cpp b/pxr/usd/usdShade/testenv/testUsdShadeValidators.cpp index 1a3910300a..d4b41176c2 100644 --- a/pxr/usd/usdShade/testenv/testUsdShadeValidators.cpp +++ b/pxr/usd/usdShade/testenv/testUsdShadeValidators.cpp @@ -32,6 +32,10 @@ PXR_NAMESPACE_USING_DIRECTIVE +TF_DEFINE_PRIVATE_TOKENS(_tokens, + ((usdShadePlugin, "usdShade")) +); + void TestUsdShadeValidators() { @@ -40,6 +44,7 @@ TestUsdShadeValidators() const std::set expectedUsdShadeValidatorNames = { UsdShadeValidatorNameTokens->materialBindingApiAppliedValidator, UsdShadeValidatorNameTokens->materialBindingRelationships, + UsdShadeValidatorNameTokens->materialBindingCollectionValidator, UsdShadeValidatorNameTokens->normalMapTextureValidator, UsdShadeValidatorNameTokens->shaderSdrCompliance, UsdShadeValidatorNameTokens->subsetMaterialBindFamilyName, @@ -47,15 +52,8 @@ TestUsdShadeValidators() UsdShadeValidatorNameTokens->encapsulationValidator }; - // This should be updated with every new validator added with the - // UsdGeomSubset keyword. - const std::set expectedUsdGeomSubsetNames = { - UsdShadeValidatorNameTokens->subsetMaterialBindFamilyName, - UsdShadeValidatorNameTokens->subsetsMaterialBindFamily - }; - const UsdValidationRegistry& registry = - UsdValidationRegistry::GetInstance(); + UsdValidationRegistry::GetInstance(); // Since other validators can be registered with the same keywords, // our validators registered in usdShade are/may be a subset of the @@ -63,30 +61,84 @@ TestUsdShadeValidators() std::set validatorMetadataNameSet; UsdValidatorMetadataVector metadata = - registry.GetValidatorMetadataForKeyword( - UsdShadeValidatorKeywordTokens->UsdShadeValidators); + registry.GetValidatorMetadataForPlugin(_tokens->usdShadePlugin); + TF_AXIOM(metadata.size() == 8); for (const UsdValidatorMetadata& metadata : metadata) { validatorMetadataNameSet.insert(metadata.name); } - TF_AXIOM(std::includes(validatorMetadataNameSet.begin(), - validatorMetadataNameSet.end(), - expectedUsdShadeValidatorNames.begin(), - expectedUsdShadeValidatorNames.end())); + TF_AXIOM(validatorMetadataNameSet == expectedUsdShadeValidatorNames); +} + +void +TestUsdShadeMaterialBindingCollections() +{ + UsdValidationRegistry& registry = UsdValidationRegistry::GetInstance(); + const UsdValidator* validator = registry.GetOrLoadValidatorByName( + UsdShadeValidatorNameTokens->materialBindingCollectionValidator); + TF_AXIOM(validator); + + UsdStageRefPtr usdStage = UsdStage::Open("./badMaterialCollections.usda"); - // Repeat the test using a different keyword. - validatorMetadataNameSet.clear(); + // Test prim with relationship to material binding collection + // to a single target fails validation. + { + const SdfPath primPath("/SingleTargetMaterialCollection"); + const UsdPrim usdPrim = usdStage->GetPrimAtPath(primPath); + const UsdValidationErrorVector errors = validator->Validate(usdPrim); - metadata = registry.GetValidatorMetadataForKeyword( - UsdGeomValidatorKeywordTokens->UsdGeomSubset); - for (const UsdValidatorMetadata& metadata : metadata) { - validatorMetadataNameSet.insert(metadata.name); + TF_AXIOM(errors.size() == 1u); + const TfToken expectedErrorIdentifier( + "usdShade:MaterialBindingCollectionValidator." + "InvalidMaterialCollection"); + + const UsdValidationError& error = errors[0]; + + const SdfPath expectedAttrPath = + primPath.AppendProperty(UsdShadeTokens->materialBindingCollection); + + TF_AXIOM(error.GetIdentifier() == expectedErrorIdentifier); + TF_AXIOM(error.GetType() == UsdValidationErrorType::Error); + TF_AXIOM(error.GetSites().size() == 1u); + const UsdValidationErrorSite& errorSite = error.GetSites()[0]; + TF_AXIOM(errorSite.IsValid()); + TF_AXIOM(errorSite.IsProperty()); + TF_AXIOM(errorSite.GetProperty().GetPath() == expectedAttrPath); + const std::string expectedErrorMsg = + "Collection-based material binding on " + " has 1 target , " + "needs 2: a collection path and a UsdShadeMaterial path."; + TF_AXIOM(error.GetMessage() == expectedErrorMsg); } - TF_AXIOM(std::includes(validatorMetadataNameSet.begin(), - validatorMetadataNameSet.end(), - expectedUsdGeomSubsetNames.begin(), - expectedUsdGeomSubsetNames.end())); + // Test prim with relationship to a material binding collection + // referencing nonexistent resources fails validation. + { + const SdfPath primPath("/IncompleteMaterialCollection/Bind1"); + const UsdPrim usdPrim = usdStage->GetPrimAtPath(primPath); + const UsdValidationErrorVector errors = validator->Validate(usdPrim); + + TF_AXIOM(errors.size() == 1u); + const TfToken expectedErrorIdentifier( + "usdShade:MaterialBindingCollectionValidator.InvalidResourcePath"); + + const UsdValidationError& error = errors[0]; + const SdfPath expectedAttrPath = + primPath.AppendProperty(UsdShadeTokens->materialBindingCollection); + + TF_AXIOM(error.GetIdentifier() == expectedErrorIdentifier); + TF_AXIOM(error.GetType() == UsdValidationErrorType::Error); + TF_AXIOM(error.GetSites().size() == 1u); + const UsdValidationErrorSite& errorSite = error.GetSites()[0]; + TF_AXIOM(errorSite.IsValid()); + TF_AXIOM(errorSite.IsProperty()); + TF_AXIOM(errorSite.GetProperty().GetPath() == expectedAttrPath); + const std::string expectedErrorMsg = + "Collection-based material binding targets an invalid collection" + " ."; + TF_AXIOM(error.GetMessage() == expectedErrorMsg); + } } void @@ -121,7 +173,7 @@ TestUsdShadeMaterialBindingRelationships() { const UsdValidationError& error = errors[0u]; - const SdfPath attrPath = + const SdfPath expectedAttrPath = primPath.AppendProperty(UsdShadeTokens->materialBinding); TF_AXIOM(error.GetIdentifier() == expectedErrorIdentifier); TF_AXIOM(error.GetType() == UsdValidationErrorType::Error); @@ -129,7 +181,7 @@ TestUsdShadeMaterialBindingRelationships() const UsdValidationErrorSite& errorSite = error.GetSites()[0u]; TF_AXIOM(errorSite.IsValid()); TF_AXIOM(errorSite.IsProperty()); - TF_AXIOM(errorSite.GetProperty().GetPath() == attrPath); + TF_AXIOM(errorSite.GetProperty().GetPath() == expectedAttrPath); const std::string expectedErrorMsg = "Prim has material binding property " "'material:binding' that is not a relationship."; @@ -139,7 +191,7 @@ TestUsdShadeMaterialBindingRelationships() { const UsdValidationError& error = errors[1u]; - const SdfPath attrPath = + const SdfPath expectedAttrPath = primPath.AppendProperty(TfToken( SdfPath::JoinIdentifier( UsdShadeTokens->materialBinding, "someAttribute"))); @@ -150,7 +202,7 @@ TestUsdShadeMaterialBindingRelationships() const UsdValidationErrorSite& errorSite = error.GetSites()[0u]; TF_AXIOM(errorSite.IsValid()); TF_AXIOM(errorSite.IsProperty()); - TF_AXIOM(errorSite.GetProperty().GetPath() == attrPath); + TF_AXIOM(errorSite.GetProperty().GetPath() == expectedAttrPath); const std::string expectedErrorMsg = "Prim has material binding property " "'material:binding:someAttribute' that is not a relationship."; @@ -720,10 +772,12 @@ main() TestUsdShadeValidators(); TestUsdShadeMaterialBindingAPIAppliedValidator(); TestUsdShadeMaterialBindingRelationships(); + TestUsdShadeMaterialBindingCollections(); TestUsdShadeShaderPropertyCompliance(); TestUsdShadeSubsetMaterialBindFamilyName(); TestUsdShadeSubsetsMaterialBindFamily(); TestUsdShadeEncapsulationRulesValidator(); + TestUsdShadeNormalMapTextureValidator(); return EXIT_SUCCESS; }; diff --git a/pxr/usd/usdShade/validatorTokens.h b/pxr/usd/usdShade/validatorTokens.h index 8a3cf0e53c..2ec0507a33 100644 --- a/pxr/usd/usdShade/validatorTokens.h +++ b/pxr/usd/usdShade/validatorTokens.h @@ -17,11 +17,13 @@ PXR_NAMESPACE_OPEN_SCOPE #define USD_SHADE_VALIDATOR_NAME_TOKENS \ - ((encapsulationValidator, "usdShade:EncapsulationRulesValidator")) \ - ((materialBindingApiAppliedValidator, "usdShade:MaterialBindingApiAppliedValidator")) \ - ((materialBindingRelationships, "usdShade:MaterialBindingRelationships")) \ - ((shaderSdrCompliance, "usdShade:ShaderSdrCompliance")) \ - ((subsetMaterialBindFamilyName, "usdShade:SubsetMaterialBindFamilyName")) \ + ((encapsulationValidator, "usdShade:EncapsulationRulesValidator")) \ + ((materialBindingApiAppliedValidator, "usdShade:MaterialBindingApiAppliedValidator")) \ + ((materialBindingRelationships, "usdShade:MaterialBindingRelationships")) \ + ((materialBindingCollectionValidator, "usdShade:MaterialBindingCollectionValidator")) \ + ((normalMapTextureValidator, "usdShade:NormalMapTextureValidator")) \ + ((shaderSdrCompliance, "usdShade:ShaderSdrCompliance")) \ + ((subsetMaterialBindFamilyName, "usdShade:SubsetMaterialBindFamilyName")) \ ((subsetsMaterialBindFamily, "usdShade:SubsetsMaterialBindFamily")) #define USD_SHADE_VALIDATOR_KEYWORD_TOKENS \ @@ -32,6 +34,8 @@ PXR_NAMESPACE_OPEN_SCOPE ((invalidConnectableHierarchy, "InvalidConnectableHierarchy")) \ ((missingMaterialBindingAPI, "MissingMaterialBindingAPI")) \ ((materialBindingPropNotARel, "MaterialBindingPropNotARel")) \ + ((invalidMaterialCollection, "InvalidMaterialCollection")) \ + ((invalidResourcePath, "InvalidResourcePath")) \ ((invalidImplSource, "InvalidImplementationSrc")) \ ((missingSourceType, "MissingSourceType")) \ ((missingShaderIdInRegistry, "MissingShaderIdInRegistry")) \ From 6dd5ca0b2db07236f13f58b7d239ea973a15ae44 Mon Sep 17 00:00:00 2001 From: Andy Date: Wed, 23 Oct 2024 10:46:07 -0400 Subject: [PATCH 6/6] fix: addressing comments - remove unnecessary check - add comment from complianceChecker.py that was missed --- pxr/usd/usdShade/validators.cpp | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/pxr/usd/usdShade/validators.cpp b/pxr/usd/usdShade/validators.cpp index 5fc1ed5daf..ae0a0392bb 100644 --- a/pxr/usd/usdShade/validators.cpp +++ b/pxr/usd/usdShade/validators.cpp @@ -21,12 +21,15 @@ #include "pxr/usd/usdShade/shader.h" #include "pxr/usd/usdShade/tokens.h" #include "pxr/usd/usdShade/validatorTokens.h" +#include "pxr/usd/usdShade/connectableAPI.h" +#include "pxr/usd/usdShade/materialBindingAPI.h" #include "pxr/usd/ar/resolver.h" #include #include #include + PXR_NAMESPACE_OPEN_SCOPE static UsdValidationErrorVector @@ -598,20 +601,6 @@ _NormalMapTextureValidator(const UsdPrim& usdPrim) { } const UsdShadeShader shader(usdPrim); - if (!shader) { - return { - UsdValidationError{ - UsdShadeValidationErrorNameTokens->invalidShaderPrim, - UsdValidationErrorType::Error, - UsdValidationErrorSites{ - UsdValidationErrorSite(usdPrim.GetStage(), - usdPrim.GetPath()) - }, - TfStringPrintf("Invalid shader prim <%s>.", - usdPrim.GetPath().GetText()) - } - }; - } TfToken shaderId; TfToken UsdPreviewSurface("UsdPreviewSurface"); @@ -677,9 +666,13 @@ _NormalMapTextureValidator(const UsdPrim& usdPrim) { if (!input) { return false; } + const UsdShadeAttributeVector valueProducingAttributes = UsdShadeUtils::GetValueProducingAttributes(input); + // Query value producing attributes for input values. + // This has to be a length of 1, otherwise no attribute is producing a value. + // We require an input parameter producing the value. if (valueProducingAttributes.empty() || valueProducingAttributes.size() != 1 || !UsdShadeInput::IsInput(valueProducingAttributes[0])) {