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

[al]Updating AL USDMaya's NurbsCurve static import export to use USD's curve spec #2957

Merged

Conversation

NickWu
Copy link

@NickWu NickWu commented Mar 21, 2023

Previously, AL USDMaya was exporting maya NurbsCurves as-is but USD's NurbsCurve spec is different from maya's. From USD's doc, we can see it uses a different knot spec than Maya does:
https://graphics.pixar.com/usd/release/api/class_usd_geom_nurbs_curves.html#details

Also, in Maya's MFnNurbsCurve API doc, it says:

Managing different knot representations in external applications

Note that some third party applications use a different format for knots, where the number of knots required for a curve is M+2N+1 rather than M+2N-1 as used in Maya. Both knot representations are equivalent mathematically. To convert from one of these external representations into the Maya representation, simply omit the first and last knots from the external representation when creating the Maya representation. To convert from the Maya representation into the external representation, add two new knots at the beginning and end of the Maya knot sequence. The value of these new knots depends on the existing knot sequence. For a knot sequence with multiple end knots, simply duplicate the existing first and last knots once more, for example:

Maya representation: {0,0,0,...,N,N,N}
External representation: {0,0,0,0,...,N,N,N,N}

For a knot sequence with uniform end knots, create the new knots offset at an interval equal to the existing first and last knot intervals, for example:

Maya representation: {0,1,2,...,N,N+1,N+2}
External representation: {-1,0,1,2,...,N,N+1,N+2,N+3}

@csyshing csyshing added the al Related to AnimalLogic plugin label Mar 21, 2023
@seando-adsk
Copy link
Collaborator

@NickWu It says there are conflicts that must be resolved. Can you fix that before we start the review/preflight process.

@csyshing csyshing force-pushed the NickWu/fixNurbsCurveImportExport branch from 9500a57 to cba668e Compare March 28, 2023 00:59
@NickWu
Copy link
Author

NickWu commented Mar 28, 2023

@NickWu It says there are conflicts that must be resolved. Can you fix that before we start the review/preflight process.

Hey @seando-adsk , I have fixed the conflicts. Thank you.

@seando-adsk
Copy link
Collaborator

@pierrebai-adsk Can you have a quick look at these changes in the AL plugin? I assume their team already reviewed it so just a quick check by us.

pierrebai-adsk
pierrebai-adsk previously approved these changes Mar 30, 2023
Copy link
Collaborator

@pierrebai-adsk pierrebai-adsk left a comment

Choose a reason for hiding this comment

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

Only inconsequential typos.

@NickWu
Copy link
Author

NickWu commented Mar 31, 2023

Only inconsequential typos.

Thank you for taking a look. I'll update it shortly.

pierrebai-adsk
pierrebai-adsk previously approved these changes Mar 31, 2023
@seando-adsk
Copy link
Collaborator

@NickWu I ran the preflight and there is some failures. Can you please have a look.

@NickWu
Copy link
Author

NickWu commented Apr 13, 2023

Hi @seando-adsk , I don't see any error message from the build_preflight saying why it's failing. Do you have this information?

@seando-adsk
Copy link
Collaborator

  • Click on the "Checks" tab
  • Then on left pane at the bottom click on "Pre-flight build on pull request"
  • At the bottom click on "build logs"
  • This will download a .zip file with all the logs
  • The "xxx_result.txt" file shows all the builds that failed
  • Then you can open each individual log to see the error(s)
  • I checked on build and saw that test "TestAdditionalTranslators" failed

@NickWu
Copy link
Author

NickWu commented Apr 24, 2023

  • Click on the "Checks" tab
  • Then on left pane at the bottom click on "Pre-flight build on pull request"
  • At the bottom click on "build logs"
  • This will download a .zip file with all the logs
  • The "xxx_result.txt" file shows all the builds that failed
  • Then you can open each individual log to see the error(s)
  • I checked on build and saw that test "TestAdditionalTranslators" failed

Hey @seando-adsk , I have fixed the failed tests. Thanks.

pierrebai-adsk
pierrebai-adsk previously approved these changes Apr 24, 2023
@NickWu
Copy link
Author

NickWu commented May 11, 2023

Hey @seando-adsk , is it all good to merge this one?

@seando-adsk
Copy link
Collaborator

@NickWu We've had some issues with our preflight for the last 2 weeks because of a problem with Github. We implemented a workaround and our preflight system is now functional again.

I've launched the preflight on this PR. Assuming it passes, it will be all good to merge.

@NickWu
Copy link
Author

NickWu commented May 12, 2023

Hi @seando-adsk , I found pre-flight tests for Windows failed with the following error:

[2023-05-11T22:36:55.367Z]     Runtime Error: in WriteToFile at line 358 of S:\jenkins\workspace\ECP\ecg-usd-build\ecg-usd-full-windows\ecg-usd-build\usd\pxr\usd\sdf\textFileFormat.cpp -- Failed to rename temporary file 'w:\build\relwithdebinfo\test\temporary\testadditionaltranslators/test_USDNurbsCurveExportImport_nxpvfb.3oxy6k' to 'w:\build\relwithdebinfo\test\temporary\testadditionaltranslators\test_USDNurbsCurveExportImport_nxpvfb.usda': Access is denied.

Looks to be a permission error?

@seando-adsk
Copy link
Collaborator

@NickWu There were a many builds which had test failure TestAdditionalTranslators. Are you sure that isn't from your changes?

@NickWu
Copy link
Author

NickWu commented Jun 5, 2023

@NickWu There were a many builds which had test failure TestAdditionalTranslators. Are you sure that isn't from your changes?

Sorry for the late reply. I got a chance to look at this today and I found there is a piece of code in the test that runs fine on Linux but problematic on Windows. I'll update the test shortly.

@NickWu
Copy link
Author

NickWu commented Jun 12, 2023

Hi @seando-adsk , I can see there is a test failing due to VP2 Error : Failed to initialize graphics device on the following test:
MayaUsd - Maya 2022 [USD min, Python 2, Interactive] - Branch Preflight (Windows): failed -- /job/ecg-mayausd-branch-preflight-2022-python2-pxrusd-min-interactive-windows/1043

This doesn't look like it's related to my PR here. Could you take a look?

@seando-adsk
Copy link
Collaborator

@NickWu Yes don't worry that failure is not related to your changes. One of our internal build machines is having issues and is being looked at. Once fixed I'll re-run the preflight build.

@seando-adsk
Copy link
Collaborator

Re-ran preflight - random test timeout on Maya 2022 (py3) Windows with testUsdExportUVTransforms. Ready for merge.

@seando-adsk seando-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Jul 24, 2023
@seando-adsk seando-adsk merged commit 63a60b2 into Autodesk:dev Jul 25, 2023
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
al Related to AnimalLogic plugin ready-for-merge Development process is finished, PR is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants