usdValidation: usdShade: NormalMapTextureValidator #3443
Conversation
beersandrew
changed the title
|
Filed as internal issue #USD-10485 |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Looks great, @beersandrew and very thorough test.
Just a few adjustments required for coding conventions, code cleanup etc.
pxr/usdValidation/usdShadeValidators/testenv/testUsdShadeValidators.cpp
Outdated
Show resolved
Hide resolved
pxr/usdValidation/usdShadeValidators/testenv/testUsdShadeValidators.cpp
Outdated
Show resolved
Hide resolved
pxr/usdValidation/usdShadeValidators/testenv/testUsdShadeValidators.cpp
Outdated
Show resolved
Hide resolved
pxr/CMakeLists.txt
Outdated
| @@ -8,7 +8,7 @@ if (EXISTS "${PROJECT_SOURCE_DIR}/pxr/exec") | |||
| add_subdirectory(exec) | |||
| endif() | |||
|
|
|||
| if ($(PXR_BUILD_USD_VALIDATION)) | |||
| if (${PXR_BUILD_USD_VALIDATION}) | |||
There was a problem hiding this comment.
@tallytalwar I believe this is a typo in dev
There was a problem hiding this comment.
Yup @beersandrew , it was fixed with last dev push: 1f0b71a
Can you update your dev branch?
- added all necessary files to move the NormalMapTexture validator from complianceChecker.py to the new Validation Framework
- fixed many code styling bugs, including missing const's - applied calls to ValidateError in all usdShade tests - adding usdShadeShader schema to NormalMapTextureValidator - fixing a CMake typo to include usdValidation
- address tabs in error messages - address long lines
beersandrew
force-pushed
the
validation-usdShade-NormalMapTexture
branch
from
06f80fd to
0a27df9
Compare
| const TfToken expectedErrorIdentifier( | ||
| "usdShadeValidators:NormalMapTextureValidator.NonCompliantBiasAndScale"); | ||
| const 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()); | ||
| ValidatePrimError(errors[0], | ||
| expectedErrorIdentifier, | ||
| usdUvTextureShader.GetPath(), | ||
| expectedErrorMsg); | ||
| } |
There was a problem hiding this comment.
Looks like this one is failing:
Output from the ValidatePrimError seems to say that error identifier is file error and not NonCompliantBiasAndScale:
Error: usdShadeValidators:NormalMapTextureValidator.InvalidFile
Expected Error: usdShadeValidators:NormalMapTextureValidator.NonCompliantBiasAndScale
Can you make sure the test is passing on your end?
There was a problem hiding this comment.
This test is passing for me locally, unfortunately unable to reproduce what you're seeing.
The check for InvalidFile does happen before the NonCompliantBiasAndScale check so the if check here must be true for that test but I'm not sure why I'm not seeing the same
There was a problem hiding this comment.
Ahh I see, weirdly the normalMap.jpg didn't get synced for me.
|
This validator will end up flagging errors in https://github.com/usd-wg/assets/tree/main/test_assets/NormalsTextureBiasAndScale That example uses scale-bias to adjust from DirectX to OpenGL normal map conventions for example. The validation error is noted there though, in the section for "r_normal_map_reversed_y.png". Would it be worthwhile to also allow the DirectX convention scale-bias to pass validation? |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Thanks @jessey-git for bringing this up. To begin with we can do the same treatment for directx related scale/bias values for normal maps as is being done for some scenarios here: b718721 So instead of erroring, usdchecker will report these as warnings. Feel free to file a PR for this. Thanks |
Description of Change(s)
Added an implementation of NormalMapTexture validation rules
Link to proposal (if applicable)
Fixes Issue(s)
Note: this PR is a replacement for #3359
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)