Skip to content
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

removes redundant "vals" namespace from the primvars namespace when r… #1635

Merged
merged 3 commits into from
Feb 22, 2022

Conversation

BSalem
Copy link

@BSalem BSalem commented Sep 30, 2021

removes redundant "vals" namespace from the primvars namespace when reading array primvars from alembic

@spiffmon
Copy link
Member

Thanks, @BSalem ! If there is no existing test case that changes as a result, would it be possible for you to add one?

@BSalem
Copy link
Author

BSalem commented Oct 1, 2021

would it be enough to add to the PR an alembic archive with two primvars (two color sets, one called vals - just in case! And the other called colorSet), that then read in usdview showing properly named primvars and properly returned values? (actually that's only how I tested it at my end).
Otherwise, guide me to what should the test look like in this case please.

@spiffmon
Copy link
Member

spiffmon commented Oct 1, 2021

Sure, @BSalem ! Have a look at the existing usdAbc tests, the majority of which are baseline-comparison tests. So you just take your simple abc file, run it through usdcat to get a usda equivalent (good to verify that it produces the bad output without your fix), and add a py file that effectively reruns usdcat. I'd maybe look at testUsdAbcIndexedProperties, including what gets added to the CMakeLists.txt to perform the baseline diff.

Thanks!

…eading array primvars from alembic

implemented test
@BSalem
Copy link
Author

BSalem commented Oct 1, 2021

Hi @spiffmon, I implemented a similar test, please let me know if this is OK

pxr_register_test(testUsdAbcIndexedGeomArb
PYTHON
COMMAND "${CMAKE_INSTALL_PREFIX}/tests/testUsdAbcIndexedGeomArb"
DIFF_COMPARE bad_primvars_namespace_before_fix.usda
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we be comparing to the "good" , after-fix baseline, here? And I don't think we need the "bad" version included in the PR - I only mentioned it so that you could have an extra validation that your fix works by running the test against the good baseline, without your fix, to see that the test fails, a la test-driven development.

Thanks, @BSalem !

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be honest, I didn't ever work with tests modules, so I'm not familiar with their dependencies and how it work... I blindly followed the example you shared.
I just pushed an updated one though.

@jilliene
Copy link

jilliene commented Oct 4, 2021

Filed as internal issue #USD-6940

@pixar-oss pixar-oss merged commit b2dd9ef into PixarAnimationStudios:dev Feb 22, 2022
pixar-oss pushed a commit that referenced this pull request Feb 25, 2022
Invoke usdcat directly instead of going through the
test script which simply does the same thing. This
is similar to how the usdcat tests are set up and
avoids issues where usdcat may not be in the PATH
when the test is run.

See #1635

(Internal change: 2217289)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants