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

Updates to support core USD release 20.08 #617

Merged

Conversation

mattyjams
Copy link
Contributor

The _engine property was made private starting with USDIMAGINGGL_API_VERSION 6 in core USD commit PixarAnimationStudios/OpenUSD@c03508b. It can still be accessed using the protected _GetHdEngine() accessor.

These changes should continue to work before and after that version.

@kxl-adsk kxl-adsk added the al Related to AnimalLogic plugin label Jul 1, 2020
@mattyjams mattyjams force-pushed the pr/use_HdEngine_accessor_in_AL_Engine branch from 21bd3ed to c23b87c Compare July 2, 2020 17:33
@kxl-adsk
Copy link

kxl-adsk commented Jul 2, 2020

@mattyjams I was testing the latest USD changes and noticed that the following test is failing with it

14 - testUsdExportAsClip (Failed)

Verbose results

14: ======================================================================
14: FAIL: testExportAsClip (testUsdExportAsClip.testUsdExportAsClip)
14: ----------------------------------------------------------------------
14: Traceback (most recent call last):
14:   File "testUsdExportAsClip.py", line 123, in testExportAsClip
14:   File "testUsdExportAsClip.py", line 65, in _ValidateSamples
14: AssertionError: different values found on frame: 15
14: non clip: inherited
14: clips:    None
14:
14: ----------------------------------------------------------------------
14: Ran 1 test in 1.083s

Can you reproduce this on your end?

@kxl-adsk kxl-adsk self-requested a review July 2, 2020 19:32
@mattyjams
Copy link
Contributor Author

@kxl-adsk Ahh, sorry, I must have missed that earlier, but I actually am seeing that test failure on my end as well.

I did some digging, and it looks like this particular test was actually disabled for us internally a while back since we were seeing it fail intermittently in a similar way. Apparently that change to turn the test off was never translated from our SCons build to the CMake build, so it has just continued to run in the maya-usd repo. Now it seems that there may be a regression between 20.05 and later versions of USD that we weren't able to detect with the test disabled.

I'm able to reproduce the issue just using the USD API in Python and cutting Maya out of the loop, so I don't think there's any issue on the maya-usd side. I've provided some additional notes to the core USD team to see if that helps identify the issue with stitched clips and value resolution.

If it sounds ok to you, I can simply disable that test in CMake for now and then re-enable it once we're able to address the core USD issue and turn the test back on?

@kxl-adsk
Copy link

kxl-adsk commented Jul 2, 2020

this particular test was actually disabled for us internally a while back since we were seeing it fail intermittently in a similar way

This is very interesting. So this particular test is very stable (with previous versions of USD) on all platforms when running all tests serially. It's as well stable on windows BUT fails randomly on MacOS.

It is fine to disable the test for the unreleased USD version, i.e. 20.08. We should address the stability during parallel test execution before re-enabling as well. Do you mind logging the GitHub issue to re-enable the test and assign it to yourself?

Copy link

@kxl-adsk kxl-adsk left a comment

Choose a reason for hiding this comment

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

Making the status of the review to reflect our conversation, i.e. currently one test is falling if we adopt the latest changes from USD dev branch. Possible next steps were covered in #617 (comment)

@mattyjams mattyjams force-pushed the pr/use_HdEngine_accessor_in_AL_Engine branch from c23b87c to 7388d85 Compare July 9, 2020 21:34
@mattyjams mattyjams changed the title Use the UsdImagingGLEngine accessor to retrieve its HdEngine Updates to support core USD 20.08 release candidate 1 Jul 9, 2020
@mattyjams
Copy link
Contributor Author

@kxl-adsk: I worked with the core USD team on this and it turns out that this just needed a test-only fix. The behavior of value clips and stitching was overhauled between 20.05 and 20.08, and we now need to pass an additional option when stitching to preserve the earlier results. See 9f1a0c4e9e6fd49aac860c54c8694a2bc0d84eb9 for that change. I'm not sure whether this will address the intermittent failure issues that you were seeing, but we at least shouldn't need to disable the test.

The documentation on value clips has been updated to describe the new behavior:
https://github.com/PixarAnimationStudios/USD/blob/dev/pxr/usd/usd/doxygen/valueClips.dox

I had to wait until the necessary changes to core USD were pushed publicly, so I also updated the build.md to reflect the new core USD commit SHA, which is also the one identified by the v20.08-rc1 tag.

Re-rebranding this PR as "update to USD v20.08-rc1"...

@kxl-adsk
Copy link

We have adopted an earlier version commit of USD 20.08 internally and if we merge this change now, we are getting

06:22:55     ======================================================================
06:22:55     ERROR: testExportAsClip (testUsdExportAsClip.testUsdExportAsClip)
06:22:55     ----------------------------------------------------------------------
06:22:55     Traceback (most recent call last):
06:22:55       File "testUsdExportAsClip.py", line 120, in testExportAsClip
06:22:55     ArgumentError: Python argument types in
06:22:55         pxr.UsdUtils._usdUtils.StitchClips(Layer, list, str)
06:22:55     did not match C++ signature:
06:22:55         StitchClips(pxrInternal_v0_20__pxrReserved__::TfWeakPtr<pxrInternal_v0_20__pxrReserved__::SdfLayer> resultLayer, std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > clipLayerFiles, pxrInternal_v0_20__pxrReserved__::SdfPath clipPath, boost::python::api::object startFrame=None, boost::python::api::object endFrame=None, boost::python::api::object clipSet=None)
06:22:55     
06:22:55     ----------------------------------------------------------------------
06:22:55     Ran 1 test in 2.510s

We will have to adopt v20.08 final release first before merging. @mattyjams please let me know if this causes you trouble.

@mattyjams
Copy link
Contributor Author

@kxl-adsk: Yeah, unfortunately the dependency on the newly added interpolateMissingClipValues parameter means that this test won't work for core USD commits between 20.05 and PixarAnimationStudios/OpenUSD@92ca995.

No problem for us waiting until maya-usd adopts the official 20.08 USD release.

@kxl-adsk kxl-adsk added the do-not-merge-yet Development is not finished, PR not ready for merge label Jul 12, 2020
@mattyjams mattyjams force-pushed the pr/use_HdEngine_accessor_in_AL_Engine branch from 7388d85 to ccfbc7f Compare July 17, 2020 18:24
@mattyjams mattyjams changed the title Updates to support core USD 20.08 release candidate 1 Updates to support core USD 20.08 release candidate 3 Jul 17, 2020
@mattyjams
Copy link
Contributor Author

Just rebased this and updated build.md to tag it against core USD rc3 so people running into issues like #651 and #654 can build from this branch directly if needed.

@mattyjams mattyjams force-pushed the pr/use_HdEngine_accessor_in_AL_Engine branch from ccfbc7f to 093a923 Compare July 21, 2020 20:17
@mattyjams mattyjams changed the title Updates to support core USD 20.08 release candidate 3 Updates to support core USD 20.08 release candidate 4 Jul 21, 2020
@mattyjams
Copy link
Contributor Author

Still just keeping this up to date. Rebased and tagged against core USD rc4.

@kxl-adsk
Copy link

Thank you, Matt. We are waiting for the final 20.08 release before adopting it internally.

@mattyjams mattyjams force-pushed the pr/use_HdEngine_accessor_in_AL_Engine branch from 093a923 to 0979012 Compare July 21, 2020 22:31
@mattyjams mattyjams changed the title Updates to support core USD 20.08 release candidate 4 Updates to support core USD release 20.08 Jul 21, 2020
@mattyjams
Copy link
Contributor Author

USD 20.08 is now officially released, so one last update to doc/build.md. Note that the release branch for core USD is now called release instead of master, so that is also reflected in the docs now.

… and up

This change corresponds to core USD commit
PixarAnimationStudios/OpenUSD@c03508b

(Internal change: 2077286)
…rsions after 20.05

Clip stitching behavior changed significantly between core USD 20.05 and 20.08.
Beginning with 20.08, we need to pass an additional option to ensure that
authored time samples are held across gaps in value clips.

This change corresponds to core USD commit
PixarAnimationStudios/OpenUSD@92ca995

(Internal changes: 2082024, 2082081, 2082660)
This identifies core USD release 20.08 as a supported release of USD, as well
as its corresponding core USD dev branch commit:
PixarAnimationStudios/OpenUSD@238d0f4

This change also reflects the change in the core USD primary release branch
name to "release" instead of "master".
@mattyjams mattyjams force-pushed the pr/use_HdEngine_accessor_in_AL_Engine branch from 0979012 to 3feb8b6 Compare July 22, 2020 17:01
@mattyjams
Copy link
Contributor Author

Rebased to pickup the fix from #676 and help keep this branch directly buildable.

@kxl-adsk kxl-adsk removed the do-not-merge-yet Development is not finished, PR not ready for merge label Jul 28, 2020
@kxl-adsk kxl-adsk merged commit 112773e into Autodesk:dev Jul 28, 2020
@mattyjams mattyjams deleted the pr/use_HdEngine_accessor_in_AL_Engine branch July 28, 2020 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
al Related to AnimalLogic plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants