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

make SDF to USD a separate component of sdformat #817

Merged
merged 29 commits into from
Jan 24, 2022

Conversation

ahcorde
Copy link
Collaborator

@ahcorde ahcorde commented Jan 7, 2022

Signed-off-by: Ashton Larkin [email protected]

🎉 New feature

Summary

Based on this PR #816 I made this other one smaller

The idea here it's to try to simplify things and be able to merge small PRs

This PR adds

  • USD as a module
  • CMD called sdf2usd to convert from SDF to USD
  • USD metadata

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@ahcorde ahcorde requested a review from azeey as a code owner January 7, 2022 11:40
@ahcorde ahcorde self-assigned this Jan 7, 2022
@ahcorde ahcorde requested a review from scpeters as a code owner January 7, 2022 11:40
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Jan 7, 2022
@ahcorde
Copy link
Collaborator Author

ahcorde commented Jan 7, 2022

The module is not going to compile, the USD libraries are only available in source form from Pixar. NVIDIA is offering pre-built libraries (Linux and Windows) as well as the accompanying tools (e.g., USDView) for download.

I believe we should create our own binaries, isn't it @scpeters ? @j-rivero?

@codecov-commenter
Copy link

codecov-commenter commented Jan 7, 2022

Codecov Report

Merging #817 (594b915) into sdf12 (0a15990) will not change coverage.
The diff coverage is n/a.

❗ Current head 594b915 differs from pull request most recent head 0851949. Consider uploading reports for the commit 0851949 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##            sdf12     #817   +/-   ##
=======================================
  Coverage   90.70%   90.70%           
=======================================
  Files          78       78           
  Lines       12438    12438           
=======================================
  Hits        11282    11282           
  Misses       1156     1156           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a15990...0851949. Read the comment docs.

@j-rivero
Copy link
Contributor

j-rivero commented Jan 7, 2022

I believe we should create our own binaries, isn't it @scpeters ? @j-rivero?

Interesting. I see different goals with respect to the installation and/or distribution of the USD pixar libraries:

  • We probably want to get them in CI as soon as possible. That could be achieved by using the pre-built binaries in our CI environments as the fastest method, if they are compatible. You can launch a quick test inside github actions probably.
  • To build, use and distribute our own binaries we need check the license. Seems a modified version of Apache-2 (I can not find what exactly was modified) but let's review carefully point 4. We can build Debian/Ubuntu/Brew packages, seems like at least vcpkg is already packaging them. The software comes from a a good bunch of embedded software on it, not sure if it exposed embedded symbols to the public API/ABI but we need to be careful with this.

@adlarkin
Copy link
Contributor

adlarkin commented Jan 7, 2022

The module is not going to compile, the USD libraries are only available in source form from Pixar. NVIDIA is offering pre-built libraries (Linux and Windows) as well as the accompanying tools (e.g., USDView) for download.

For further context, I tried to use NVIDIA's pre-built libraries/tools, and I had problems setting them up. I had to end up building USD from source. @j-rivero if you want to see how this was done in case it's useful for integrating this component with CI, I made a Dockerfile that sets up a USD source build: https://github.com/adlarkin/usd_docker

Copy link
Contributor

@adlarkin adlarkin left a comment

Choose a reason for hiding this comment

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

The overall structure looks good to me. I left some minor comments.

I am still seeing the following error when I run the component's installed sdf2usd executable:

sdf2usd: error while loading shared libraries: libusd_usd.so: cannot open shared object file: No such file or directory

usd/src/World.cc Outdated Show resolved Hide resolved
examples/usdConverter/README.md Outdated Show resolved Hide resolved
usd/src/cmd/CMakeLists.txt Outdated Show resolved Hide resolved
usd/include/sdf/usd/CMakeLists.txt Outdated Show resolved Hide resolved
usd/src/cmd/sdf2usd.cc Outdated Show resolved Hide resolved
@scpeters
Copy link
Member

scpeters commented Jan 7, 2022

I believe we should create our own binaries, isn't it @scpeters ? @j-rivero?

Interesting. I see different goals with respect to the installation and/or distribution of the USD pixar libraries:

  • We probably want to get them in CI as soon as possible.

I think we need to use the Pixar libraries in our CI before we can merge any of these pull requests. I can take a look at making a homebrew formula

@koonpeng
Copy link

For further context, I tried to use NVIDIA's pre-built libraries/tools, and I had problems setting them up. I had to end up building USD from source. @j-rivero if you want to see how this was done in case it's useful for integrating this component with CI, I made a Dockerfile that sets up a USD source build: https://github.com/adlarkin/usd_docker

nvidia's binaries may not work because it's compiled with pre cxx11 abi and a bunch of bundled dependencies (python3.6, boost1.70) which are not suitable for distribution (also, it includes gui programs like usdview which we don't need).

@traversaro
Copy link
Contributor

traversaro commented Jan 10, 2022

I believe we should create our own binaries, isn't it @scpeters ? @j-rivero?

Interesting. I see different goals with respect to the installation and/or distribution of the USD pixar libraries:

* We probably want to get them in CI as soon as possible. That could be achieved by using the pre-built binaries in our CI environments as the fastest method, if they are compatible. You can launch a quick test inside github actions probably.

* To build, use and distribute our own binaries we need check the license. Seems a modified version of Apache-2 (I can not find what exactly was modified) but [let's review carefully point 4](https://github.com/PixarAnimationStudios/USD/blob/release/LICENSE.txt#L89-L128). We can build Debian/Ubuntu/Brew packages, seems like at least [vcpkg](https://vcpkg.info/port/usd) is already packaging them.

FYI I did a quick test to add usd to conda-forge some time ago, even if I did not worked a lot on it: conda-forge/staged-recipes#16666 .

The software comes from a a good bunch of embedded software on it, not sure if it exposed embedded symbols to the public API/ABI but we need to be careful with this.

Just to cross-link, there is an issue on usd issue tracker on that: PixarAnimationStudios/OpenUSD#1490 .

@ahcorde
Copy link
Collaborator Author

ahcorde commented Jan 10, 2022

I was testing in this branch to compile USD from sources and at least check that the code is compiling. If you look for sdf2usd we will see how the module is compiling.

It's only compiling on focal because pxr requires the cmake version 3.12 and I was only able to install 3.10 on Bionic.

I can include this changes here but is increasing a lot the CI time

@adlarkin
Copy link
Contributor

@ahcorde I finished up a "bare bones implementation" of usd::ParseSdfWorld in 18f3ac1, and added a unit test for verifying converted world information in 3bbd6ea. I believe that this creates a good baseline of functionality for this PR so that it can be merged into sdf12.

Regarding the tests, we still need to figure out how to address #796 (review). I'll see if I can figure out how to address this, and create a new PR if I come up with any ideas.

@ahcorde
Copy link
Collaborator Author

ahcorde commented Jan 11, 2022

@scpeters or @azeey Do you mind to review this PR ?

@j-rivero
Copy link
Contributor

For further context, I tried to use NVIDIA's pre-built libraries/tools, and I had problems setting them up. I had to end up building USD from source. @j-rivero if you want to see how this was done in case it's useful for integrating this component with CI, I made a Dockerfile that sets up a USD source build: https://github.com/adlarkin/usd_docker

How much does it take to run that from-source build in a github action?

@ahcorde
Copy link
Collaborator Author

ahcorde commented Jan 11, 2022

For further context, I tried to use NVIDIA's pre-built libraries/tools, and I had problems setting them up. I had to end up building USD from source. @j-rivero if you want to see how this was done in case it's useful for integrating this component with CI, I made a Dockerfile that sets up a USD source build: https://github.com/adlarkin/usd_docker

How much does it take to run that from-source build in a github action?

This is a build example https://github.com/ignitionrobotics/sdformat/runs/4776721844?check_suite_focus=true ~ 1hour, 7min

Copy link
Contributor

@adlarkin adlarkin left a comment

Choose a reason for hiding this comment

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

LGTM, but I do have a few final questions/comments that should be addressed before merging.


########################################
# Find PXR
ign_find_package(pxr QUIET REQUIRED_BY usd PKGCONFIG pxr)
Copy link
Contributor

@adlarkin adlarkin Jan 20, 2022

Choose a reason for hiding this comment

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

Configuration warnings should be gone now thanks to gazebo-tooling/release-tools#625, but I realize that until this PR is merged, gazebo-tooling/release-tools#625 will cause CI warnings on other sdformat PRs, as mentioned in gazebo-tooling/release-tools#625 (comment). I'm not sure if we should merge this PR first to take care of CI warnings on other PRs, or revert gazebo-tooling/release-tools#625 until it's time to merge this PR. Does anyone have thoughts on how to handle this?

usd/src/sdf2usd_TEST.cc Outdated Show resolved Hide resolved
Signed-off-by: ahcorde <[email protected]>
@ahcorde ahcorde requested a review from adlarkin January 21, 2022 22:21
Copy link
Collaborator

@azeey azeey left a comment

Choose a reason for hiding this comment

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

LGTM with green CI. Looks like there's a test failure in Github Actions jobs.

usd/include/sdf/usd/World.hh Outdated Show resolved Hide resolved
usd/src/World.cc Outdated Show resolved Hide resolved
usd/include/sdf/usd/World.hh Outdated Show resolved Hide resolved
Comment on lines 71 to 73
std::string output =
custom_exec_str(sdf2usdCommand() + " " + path + " " +
ignition::common::joinPaths(tmp, "shapes.usd"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Has this been resolved @adlarkin ?

Copy link
Contributor

@adlarkin adlarkin left a comment

Choose a reason for hiding this comment

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

LGTM with green CI. Looks like there's a test failure in Github Actions jobs.

CI is green now - see #817 (comment)


The only other question I have before approving this is if we want to change the current file structure. This PR adds the SDF -> USD converter file structure, but this component will also have a USD -> SDF converter. So, I am wondering if we want sub-header and sub-source directories to differentiate between the SDF -> USD converter files and the USD -> SDF converter files: see #827 (review) for more information about this, @ahcorde and @azeey

.github/ci/before_cmake.sh Outdated Show resolved Hide resolved
.github/ci/before_cmake.sh Outdated Show resolved Hide resolved
usd/include/sdf/usd/World.hh Outdated Show resolved Hide resolved
@adlarkin adlarkin mentioned this pull request Jan 24, 2022
8 tasks
Signed-off-by: Ashton Larkin <[email protected]>
Copy link
Contributor

@adlarkin adlarkin left a comment

Choose a reason for hiding this comment

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

We should be good to merge if CI comes back green. Thanks for iterating, @ahcorde and @azeey!

@adlarkin adlarkin merged commit c80d78d into sdf12 Jan 24, 2022
@adlarkin adlarkin deleted the ahcorde/sdf_to_usd_cmake branch January 24, 2022 21:29
@adlarkin adlarkin mentioned this pull request Jan 26, 2022
8 tasks
@adlarkin adlarkin mentioned this pull request Mar 23, 2022
7 tasks
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-04-13-fortress-edifice/1367/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants