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 -> USD code a separate component of SDF #816

Closed
wants to merge 39 commits into from

Conversation

adlarkin
Copy link
Contributor

@adlarkin adlarkin commented Jan 7, 2022

🎉 New feature

Summary

This updates the work being done in #762 to make the SDF -> USD conversion code into a separate cmake component. This is now possible thanks to #780.

The commit history in this PR appears messy because at the moment, the adlarkin/sdf_to_usd branch is not up to date withsdf12 - it's not up to date because merging in sdf12 would change the cmake code in #762, but the current code structure in #762 isn't compatible with the recent cmake updates from #780. So, this PR merged in the recent changes from sdf12 and then updated the structure of #762 to be compatible with sdf12 via a cmake component. To make it easier to view the actual changes being made in this PR, view the history/files changed starting at commit 26c1a47.

Test it

The old usdConverter executable that converts code from SDF to USD has been moved to a standalone example. To test if this component works, you can try to build this example and then test the resulting executable: https://github.com/ignitionrobotics/sdformat/tree/adlarkin/sdf_to_usd_cmake/examples/usdConverter

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.

iche033 and others added 30 commits November 8, 2021 12:00
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
* Fix cmake warning about newlines
* Find python3 in cmake, fix warning (#328)

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
* Added ToElement conversion for physics and atmosphere

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* spelling

Signed-off-by: Nate Koenig <nate@openrobotics.org>

Co-authored-by: Nate Koenig <nate@openrobotics.org>
* Aded ToElement conversion for shapes

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* doxygen

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

Co-authored-by: Nate Koenig <nate@openrobotics.org>
Co-authored-by: Ian Chen <ichen@osrfoundation.org>
* Working on material DOM ToElement

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* Update material ToElement

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* Use floats

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* Fix build

Signed-off-by: Nate Koenig <nate@openrobotics.org>

Co-authored-by: Nate Koenig <nate@openrobotics.org>
Signed-off-by: Nate Koenig <nate@openrobotics.org>

Co-authored-by: Nate Koenig <nate@openrobotics.org>
Co-authored-by: Louise Poubel <louise@openrobotics.org>
* Added ToElement conversion for Collision, Surface, and Visual

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* Update src/Visual_TEST.cc

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

Co-authored-by: Nate Koenig <nate@openrobotics.org>
Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
Co-authored-by: Marco A. Gutiérrez <marco@openrobotics.org>
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
* Added ToElement for ParticleEmitter and Link

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* expect double eq

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

Co-authored-by: Nate Koenig <nate@openrobotics.org>
Co-authored-by: Ian Chen <ichen@osrfoundation.org>
* Adding toelement for model and actor

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* Updated tests

Signed-off-by: Nate Koenig <nate@openrobotics.org>

Co-authored-by: Nate Koenig <nate@openrobotics.org>
Adds a sentence to the behavior of //model/static to avoid confusion on what might happen if it is used within a model that is itself included in another model. Also adds some clarification to //model/@canonical_link.

Signed-off-by: FirefoxMetzger <sebastian@wallkoetter.net>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
* Added more ToElement functions

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* Revert Link toelement changes

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* Added toelement conversion for world, scene, sky, gui

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* Fix element insertion, and updated tests

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* Update docs

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* Updates

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* minor doxygen update

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

Co-authored-by: Nate Koenig <nate@openrobotics.org>
Co-authored-by: Ian Chen <ichen@osrfoundation.org>
Signed-off-by: Nate Koenig <nate@openrobotics.org>

Co-authored-by: Nate Koenig <nate@openrobotics.org>
* Support URI in the Model DOM

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* One minor test

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* placement frame and static

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* relative_to

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* Added nested include test

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* Updates

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* useincludetag

Signed-off-by: Nate Koenig <nate@openrobotics.org>

Co-authored-by: Nate Koenig <nate@openrobotics.org>
* Added plugin to SDF DOM

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* tweaks

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* Fixed doxygen

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* Updates

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* Update plugin copy and tests

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* Remove string and add move functions

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* Fix build and tests

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* Fix windows warnings

Signed-off-by: Nate Koenig <nate@openrobotics.org>

Co-authored-by: Nate Koenig <nate@openrobotics.org>
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
The buildFrameAttachedToGraph and buildPoseRelativeToGraph have overloads for the type of parent element, but the code in each overload is very similar to each other. This refactors these functions so there's less code duplication.

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Co-authored-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
…used angles (#689)

* Ruby option to print in_degrees or snap_to_degrees

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Basic PrintConfig added

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* PrintConfig gets passed into printing implementations of Element and Param

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Adding basic test for print options

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Reverting to PrintConfig with basic API

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Moved creation of PrintConfig into ign functions

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Param value GetPoseAsString and tests

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Moved attribute painting to its own function, fixed test strings

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Added basic tests for pose rotation input as quaternions

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Using different flags for ign sdf -p, allow snapping to different values

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Disabling test on windows, fixing comment

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Remove stale function, fixed linting

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Adding tolerance as a argument, added tests

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Use 3 spaces when changing rotation formats or snapping to degrees

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Added check for tolerance larger than snapping interval

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Moving PrintAttributes to ElementPrivate to remain ABI stability

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Using true/false instead of 1/0

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Remove use of SDF_ASSERT in GetAsString

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Added tests for //include/pose

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Adding parsing passing test for empty quat_xyzw pose

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Added check for default string values to be modified when rotation_format is defined

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Added tests

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Reparsing translates default value into string to be used if values have not been assigned to param

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Using StringFromValueImpl for getting strings from all ParamVariants

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Refactor pose string from value into its own function

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Fixing casting erroerror, added documentation and tests for tolerance < interval

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Correcting stale comments

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Fixing snapToInterval math, added more tests

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Removed unneeded visibility macro

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Adding return documentation and using const reference to variant instead of pointer

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Returning string directly, removing stale _config, reverting strValue to nullopt

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Remove use of assertions

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Suggested changes to #729 (#748)

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

* Using three space delimiter between position and rotation if attributes are set

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Added comment regarding use of default PrintConfig in Reparse

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Adding equality comparison for PrintConfig

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Removed stale include

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Uniied string and value parsing behavior, and modified necessary tests

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Overloaded function to maintain ABI stability

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Fixing missing space in test for exec command

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Adding comment regarding attributeExceptions

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Indenting help message, adding test for shuffling command flags

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Modifying cmd flag shuffling test to handling flags with arguments too

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Removed Get from PrintConfig getter functions

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Using std optional's converting constructor

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Modified snapToInterval implementation, added test

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Added bool type specific value parser, values are parsed using ParamStreamer by default

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Reverting all unnecessary changes made in sdf12 to old tests

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Added comparison for PreserveIncludes

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Check for 'type' attribute in unknown elements as well, in order to parse booleans into true/false, instead of 1/0

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Only checking for pose related PrintConfig options for returning string instead of default PrintConfig

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Added comment regarding sanitizing -0 in test outputs

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
This replaces most of the custom cmake code in
libsdformat with the functionality provided by
ignition-cmake2. The root CMakeLists.txt is much shorter
now and most of the cmake folder has been deleted.
This is made possible by the NO_IGNITION_PREFIX
and REPLACE_IGNITION_INCLUDE_PATH parameters
added to ign_configure_project in ign-cmake#190 and
ign-cmake#191. Closes #181. Other details:

* Use FindIgnURDFDOM from ign-cmake#193
* Use HIDE_SYMBOLS_BY_DEFAULT from ign-cmake#196
* Set LEGACY_PROJECT_PREFIX from ign-cmake#199

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
The USE_INTERNAL_URDF logic for include and
windows compiler definitions needs to be repeated
for tests that add parser_urdf.cc via target_sources.
An interface library is used to deduplicate the cmake
logic.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Account for new behavior of nested models and :: syntax.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
scpeters and others added 9 commits December 29, 2021 13:28
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Add line with unrecognized type

Signed-off-by: Steve Peters <scpeters@openrobotics.org>

* Fix for custom element with attribute 'type' (#811)

* Reverted changes to parser for custom element with attribute 'type',
  added fix in ParamPrivate::ValueFromStringImpl instead

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

Co-authored-by: Steve Peters <scpeters@openrobotics.org>
Co-authored-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (adlarkin/sdf_to_usd@b70ce48). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@                  Coverage Diff                   @@
##             adlarkin/sdf_to_usd     #816   +/-   ##
======================================================
  Coverage                       ?   90.70%           
======================================================
  Files                          ?       78           
  Lines                          ?    12432           
  Branches                       ?        0           
======================================================
  Hits                           ?    11276           
  Misses                         ?     1156           
  Partials                       ?        0           

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 b70ce48...26c1a47. Read the comment docs.

@adlarkin adlarkin changed the title Adlarkin/sdf to usd cmake Make SDF -> USD code a separate component of SDF Jan 7, 2022
@adlarkin adlarkin requested review from scpeters and ahcorde January 7, 2022 00:39
Copy link
Contributor Author

@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.

@scpeters and @ahcorde: I have a few questions about what I've done so far, would you mind responding to my comments/questions below? Also, for your initial review, I'd recommend only looking at the changes from commit 26c1a47, since all of the previous commits were a merge from sdf12 so that I could build on top of the cmake changes made in #780.


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

Choose a reason for hiding this comment

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

I'm not sure if the PKGCONFIG pxr portion should be here. If I didn't include this, I would see the following warning: https://github.com/ignitionrobotics/ign-cmake/blob/a1eb36a9a6283bb1fe5b4c755a2b3669572e0c08/cmake/IgnUtils.cmake#L378-L391

Can someone confirm if adding PKGCONFIG pxr is correct or not?

Comment on lines 112 to 115
########################################
# Find ignition common
ign_find_package(ignition-common4 COMPONENTS graphics REQUIRED_BY usd)
set(IGN_COMMON_VER ${ignition-common4_VERSION_MAJOR})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to have a dependency on ign-common in order to parse meshes to/from SDF and USD. I realize that this is a new dependency that we probably don't want to have, but since it's only needed by the usd component, is it okay to have this new dependency?

Copy link
Member

Choose a reason for hiding this comment

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

yes, it's ok

You should now have an executable named `usdConverter`, which can be used to convert a SDF world file to a USD file.
The following command converts the example `shapes.sdf` file to its USD representation, stored in a file called `shapes.usda` (run this command from the `build` directory):
```bash
./usdConverter ../shapes.sdf shapes.usda
Copy link
Contributor Author

Choose a reason for hiding this comment

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

right now, this is not working - whenever I try to run the executable generated by the example, I get the following error:

./usdConverter: error while loading shared libraries: libusd_usdPhysics.so: cannot open shared object file: No such file or directory

This was not happening with this executable before turning all of this code into a separate component, so perhaps the component is not being created correctly at the moment. Does anyone know what's happening here? cc @scpeters

#include "sdf_usd_parser/world.hh"
#include "sdf/world.hh"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried following the file structure for components in ign-common:

<component name>
    |__include
    |    |__<project name>
    |         |__component header files
    |__src
         |__component source files

This means that for the USD component, header files are in the path usd/include/sdformat, and when it's time to #include them in the component's source files, they have the same prefix as sdformat's header files (sdf/). This doesn't result in naming collisions right now because the component header files are not upper case, but a lot of the USD component header files are essentially just a lower case name of the sdformat header files, which can make this code confusing to read (example: world.hh vs World.hh). Should I change the component header file structure somehow so that the #include path is unique (for example, something like #include "sdf/usd/...")?

Comment on lines +19 to +20
# Build the unit tests
ign_build_tests(TYPE UNIT SOURCES ${gtest_sources} LIB_DEPS ${usd_target})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are no tests in this PR right now, but the tests being written in #796 will eventually make use of this cmake code (#796 will need to be updated to reflect the cmake component structure being done in this PR).

Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

all of the usd header files should be in a usd subfolder, like usd/include/sdf/usd/*

@ahcorde
Copy link
Collaborator

ahcorde commented Jan 7, 2022

I created this small PR #817, we will have a lot of conflics and the review will be a mess with the current prototype.

With this small PR I hope we can start to merge things into sdf12

@adlarkin
Copy link
Contributor Author

adlarkin commented Jan 7, 2022

I created this small PR #817, we will have a lot of conflics and the review will be a mess with the current prototype.

With this small PR I hope we can start to merge things into sdf12

Thanks for breaking this down into something simpler, @ahcorde! Since we now have #817, should I close this PR?

@adlarkin
Copy link
Contributor Author

closing in favor of #817

@adlarkin adlarkin closed this Jan 19, 2022
@adlarkin adlarkin deleted the adlarkin/sdf_to_usd_cmake branch March 24, 2022 15:05
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.