-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added tests for usdImagingGL and testusdview #1743
Added tests for usdImagingGL and testusdview #1743
Conversation
# The following tests are disabled since the stages are unable to load any .tx files | ||
# In theory these image files should be able to load through the hioOiio plugin but I have not | ||
# been able to get that too work. I did try to convert these images through OIIO's iconvert tool | ||
# but it also fails trying to open the .tx files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment pretty much describes the issue I ran into with .tx files. I just want to point this out so we can discuss how we should handle these tests
TESTENV testUsdImagingGLUsdLux | ||
) | ||
|
||
# TODO: Requires create_symlinks.py to run properly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The testSpecs.xml reference this create_symlinks.py
script. I'm not sure how this exactly sets up the symlinks so the following test is disabled for now. It might be worth asking about the script so we can enable this test
pxr/usdImaging/usdImagingGL/testenv/testUsdImagingGLAnimatedLights/lightMoving.usda
Show resolved
Hide resolved
This is exciting, @nvidia-jomiller ! Before we can proceed, though, can you please ask Rev to update the NVidia CLA to add you as one of the sanctioned contributors? Thanks so much! |
@spiffmon absolutely! |
pxr/usdImaging/usdImagingGL/testenv/testUsdImagingGLMaterialX/basicMxYup.usda
Show resolved
Hide resolved
Filed as internal issue #USD-7144 |
0981420
to
b204b9f
Compare
@spiffmon from my understanding Rev sent an email to update the CLA. So once we're good there we can work on getting this reviewed and merged into |
You have been added, and we are IP reviewing!
On Wed, Feb 9, 2022 at 9:52 AM nvidia-jomiller ***@***.***> wrote:
@spiffmon <https://github.com/spiffmon> from my understanding Rev sent an
email to update the CLA. So once we're good there we can work on getting
this reviewed and merged into dev. Thanks!
—
Reply to this email directly, view it on GitHub
<#1743 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABPOU2CEDPBCFSBKRWOGPHLU2KSWZANCNFSM5L7VGSNA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
--spiffiPhone
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a first pass at reviewing and testing this change and left some notes.
Just FYI, I'm going to be pulling in parts of this PR separately -- I've already merged the .tex -> .hdr change, and will be merging the Z-up asset updates soon.
b0576bc
to
115001d
Compare
One other thing to mention: when the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did another testing pass and ran into a few remaining issues that I've noted below.
One other request: there were several tests that were already using idiff in the testSpecs.xml we sent along to you, and I think we just took the thresholds as-is from those files. Those tests fail on my separate test machine, so if possible I'd love to just change them to use perceptual diffs. These tests are:
- testUsdImagingGLBasicDrawingNonBindless_batch
- testUsdImagingGLDomeLight_ZupFront
- testUsdImagingGLDomeLight_ZupBack
- testUsdImagingGLDomeLight_YupFront
- testUsdImagingGLDomeLight_YupBack
- testUsdImagingGLDomeLight_multi
- testUsdImagingGLDomeLight_rough
- testUsdImagingGLDomeLight_grid
- testUsdImagingGLDomeLight_grid_specwf
- testUsdImagingGLSurfaceShader_1_2_5
Otherwise I think this change looks ready to go. Thanks so much for all of your work on this!
for dome light textures instead of a .tex file. .tex files are a RenderMan-specific image format that are not usable in a standard USD build. .hdr files are supported by the built-in stb_image-based image reader. Thanks to @nvidia-jomiller for identifying this issue as part of PR #1743. (Internal change: 2215594)
The baseline images for these tests were generated under Pixar's studio-specific configuration which specifies Z-up by default. This change authors that explicitly into the test assets to isolate them from site-specific settings and ensure consistent behavior. This is part of PR #1743 from @nvidia-jomiller. (Internal change: 2216745)
The pxr_register_test function now has an IMAGE_DIFF_COMPARE argument. This argument allows for tests that rely on comparing output images with a baseline. Threshold arguments (FAIL, FAIL_PERCENT, etc.) have also been added to control the acceptable tolerance for the image diff. Under the hood, this is currently using idiff from OpenImageIO. Using idiff does require OpenImageIO to be built if tests are also being built. There is also a subtle change in build_usd.py which enables OpenImageIO when tests are being built
115001d
to
a6d32bd
Compare
This was part of PR #1743 by @nvidia-jomiller (Internal change: 2217350)
See previous change 2216745 for more details. This is part of PR #1743 from @nvidia-jomiller. (Internal change: 2217658)
Hey @nvidia-jomiller We will be pushing the PR along with some followup fixes from @sunyab in the next dev branch update, in a bit. But just wanted to note that we have been seeing test failures because of perceptual differences in our static build configuration on Linux (https://github.com/PixarAnimationStudios/USD/blob/release/BUILDING.md#static-libraries) An example snippet:
We have not investigated this further on our end, but just wanted to highlight just in case you have noticed these on your end. Thanks |
Hey @tallytalwar, I haven't ran into that specific test failing but when I looked at the points.usd I noticed that it doesn't have the Regardless, I think |
Description of Change(s)
The pxr_register_test function now has an IMAGE_DIFF_COMPARE argument. This argument allows for tests that rely on comparing output images with a baseline. Threshold arguments (FAIL, FAIL_PERCENT, etc.) have also been added to control the acceptable tolerance for the image diff.
Under the hood, this is currently using idiff from OpenImageIO. Using idiff does require OpenImageIO to be built if tests are also being built. There is also a subtle change in build_usd.py which enables OpenImageIO when tests are being built
Fixes Issue(s)