USD to SDF converter by ahcorde · Pull Request #736 · gazebosim/sdformat

ahcorde · 2021-10-27T16:21:43Z

🎉 New feature

Summary

FYI @adlarkin @koonpeng . Feel free to push code, cleanups or suggestions

This draft PR creates a prototype to convert USD files into SDF files.

For now I already tested with simple_articulated.usd and ur10.usd.

Regarding joints there are two fields physics:body0 and physics:body1 that indicate the parent and child of the joint, but in usd format there is no distinction between them. We need to rework this part a little bit more. I have to modify ur10.usda to set parent as body1 and child as body0. No changes are required in the simple_articulated.usd file.

The current implementation supports:

Links
- Geometris: cylinders, spheres and boxes
- Collisions
- Color
Joints: fixed, revolute and prismatic
Physics (only fill data structure)

Code need a huge refactor. We can probably use some ignition library such as ignition-common or ignition-math in the usd_parser folders because I copy-paste the code from urdf. There are object like Sphere, poses, Vectors quaternions, links and many other that I'm pretty sure that we can use something from these libraries.

Test it

Install PXR lbrary.
- If you only want the cpp API you should compile the library with -DPXR_ENABLE_PYTHON_SUPPORT=0
- if you compile the library with python support you will be able to use some of the usd tools such us:
  - usdcat: It allows to convert usd (binary format) into usda (human redeable format). `usdcat binary.usa > output.usda
  - usdview. It allows to visualize usd files
There is a cmd util called sdfconverter which allows to convert a urdf or usd files in to sdf. it's simple to use: sdfconverter -i <input> -o <ouput>

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

Signed-off-by: Alejandro Hernández <ahcorde@gmail.com>

Signed-off-by: ahcorde <ahcorde@gmail.com>

scpeters · 2021-10-28T01:20:52Z

Of all the parts of libsdformat, the parser_urdf code is probably the most in need of improvement. So building USD support off of this code is not the best foundation, though I understand why you would do it, since it is there and basically functional. If we could think of a better implementation for this type of conversion, I think it would be valuable, since this parser_urdf code is so hard to maintain.

cc @azeey

azeey · 2021-10-28T17:01:57Z

cmake/SearchForStuff.cmake

+########################################
+# Find ignition common
+# Set a variable for generating ProjectConfig.cmake
+find_package(ignition-common4 4.0 QUIET)


Is ign-common needed for anything other than common::split? I'd avoid adding a dependency if we don't need it. We already have sdf::split that provides the same functionality.

azeey · 2021-10-28T17:15:48Z

I agree with @scpeters about parser_urdf.cc. I would also vote putting this converter in a separate repository and keep libsdformat be more focused on parsing SDFormat. I'm concerned that adding a dependency like pxr would make it difficult for downstream applications that only use libsdformat for parsing SDFormat files (eg. Drake).

chapulina · 2021-10-28T17:29:08Z

I would also vote putting this converter in a separate repository and keep libsdformat be more focused on parsing SDFormat.

Another possibility is keeping USD support in a separate component within this codebase. The advantage would be giving it more visibility, tighter maintenance, and maybe setting the standard for adding other converters in the future. My concern with a separate repository is that unless we make it a first-class Ignition library, it may just go unmaintained very quickly (similar to ign-rndf).

scpeters · 2021-10-28T17:36:18Z

I would also vote putting this converter in a separate repository and keep libsdformat be more focused on parsing SDFormat.

Another possibility is keeping USD support in a separate component within this codebase. The advantage would be giving it more visibility, tighter maintenance, and maybe setting the standard for adding other converters in the future. My concern with a separate repository is that unless we make it a first-class Ignition library, it may just go unmaintained very quickly (similar to ign-rndf).

sdformat doesn't use ign-cmake2, so it would be some work to add a component to this package

chapulina · 2021-10-28T18:00:16Z

sdformat doesn't use ign-cmake2, so it would be some work to add a component to this package

If this is something that we think would be valuable, it would be a good excuse to update SDFormat to use ign-cmake (#181)

koonpeng · 2021-11-02T10:11:34Z

Another possibility is keeping USD support in a separate component within this codebase.

I'm very new to the ignition build system so I might be totally off, does a separate component means declaring it with ign_add_component? From what I understand, the function adds a "library or component" to a "core library", but I think this PR adds a new executable.

scpeters · 2021-11-02T15:27:33Z

sdformat doesn't use ign-cmake2, so it would be some work to add a component to this package

If this is something that we think would be valuable, it would be a good excuse to update SDFormat to use ign-cmake (#181)

I've taken an initial step towards this in gazebosim/gz-cmake#190, which is awaiting review

scpeters · 2021-11-02T15:31:45Z

Another possibility is keeping USD support in a separate component within this codebase.

I'm very new to the ignition build system so I might be totally off, does a separate component means declaring it with ign_add_component? From what I understand, the function adds a "library or component" to a "core library", but I think this PR adds a new executable.

There is an aspect that is more significant than whether the code produces a library or executable, which is to define a cmake component for packaging purposes. See for instance the discussion of the COMPONENTS keyword in the find_package documentation. A different component can be found installed and found independently, and is especially useful for isolating dependencies, so you can build or install the core components without needing all the dependencies used by other components.

Signed-off-by: ahcorde <ahcorde@gmail.com>

adlarkin · 2021-11-04T16:25:29Z

sdformat doesn't use ign-cmake2, so it would be some work to add a component to this package

It looks like ign-cmake2 is listed in sdformat's packages.apt file: https://github.com/ignitionrobotics/sdformat/blob/5385ce9500bd1662d52ba3d0c2c919d13f86c949/.github/ci/packages.apt#L1

Is this a mistake, or does sdformat actually depend on ign-cmake2 somehow?

adlarkin · 2021-11-04T16:33:15Z

Of all the parts of libsdformat, the parser_urdf code is probably the most in need of improvement. So building USD support off of this code is not the best foundation, though I understand why you would do it, since it is there and basically functional. If we could think of a better implementation for this type of conversion, I think it would be valuable, since this parser_urdf code is so hard to maintain.

On a somewhat related note, it looks like parser_urdf is deprecated and should have been removed by now (I think?): https://github.com/ignitionrobotics/sdformat/blob/913a7754c319190defa549bb73dc584e59f9734f/src/parser_urdf.hh#L35-L40

Do we need to wait until the next sdformat version to remove this class, or can we do it now since it's not installed?

scpeters · 2021-11-04T17:46:53Z

sdformat doesn't use ign-cmake2, so it would be some work to add a component to this package

It looks like ign-cmake2 is listed in sdformat's packages.apt file:

https://github.com/ignitionrobotics/sdformat/blob/5385ce9500bd1662d52ba3d0c2c919d13f86c949/.github/ci/packages.apt#L1

Is this a mistake, or does sdformat actually depend on ign-cmake2 somehow?

libsdformat depends on ignition-cmake2 but isn't an ignition-cmake project that uses the cmake macros like ign_configure_project and ign_configure_build. Until recently, it only depended on ignition-cmake2 indirectly via ignition-math6, although it now uses IgnCodeCheck.cmake as of #682. It will take a bit more work (see #181) including changes in ign-cmake to allow libsdformat to use the ign-cmake cmake helpers to add a component to this package

scpeters · 2021-11-04T17:48:26Z

Of all the parts of libsdformat, the parser_urdf code is probably the most in need of improvement. So building USD support off of this code is not the best foundation, though I understand why you would do it, since it is there and basically functional. If we could think of a better implementation for this type of conversion, I think it would be valuable, since this parser_urdf code is so hard to maintain.

On a somewhat related note, it looks like parser_urdf is deprecated and should have been removed by now (I think?):

https://github.com/ignitionrobotics/sdformat/blob/913a7754c319190defa549bb73dc584e59f9734f/src/parser_urdf.hh#L35-L40

Do we need to wait until the next sdformat version to remove this class, or can we do it now since it's not installed?

that comment is wrong; this code is still needed. parser_urdf.hh is a private header similar to FrameSemantics.hh

adlarkin · 2021-11-04T18:41:26Z

that comment is wrong; this code is still needed. parser_urdf.hh is a private header similar to FrameSemantics.hh

I made a PR that removes this comment in #740

It will take a bit more work (see #181) including changes in ign-cmake to allow libsdformat to use the ign-cmake cmake helpers to add a component to this package

Before we go ahead and make changes to ign-cmake, I think we should determine if we actually want to place the functionality of this PR into a separate component. If we decide on taking a different approach (for example, placing the USD converter in a separate repository), then it wouldn't make sense to put forth the effort of updating ign-cmake right now. Does everyone think we should place this PR in a separate component of sdformat, or should we consider other alternatives?

scpeters · 2021-11-04T20:37:27Z

It will take a bit more work (see #181) including changes in ign-cmake to allow libsdformat to use the ign-cmake cmake helpers to add a component to this package

Before we go ahead and make changes to ign-cmake, I think we should determine if we actually want to place the functionality of this PR into a separate component. If we decide on taking a different approach (for example, placing the USD converter in a separate repository), then it wouldn't make sense to put forth the effort of updating ign-cmake right now. Does everyone think we should place this PR in a separate component of sdformat, or should we consider other alternatives?

If we want to perpetually support conversion between SDFormat and USD, I think there are advantages to keeping this code in the same repository. Since it requires additional dependencies, I think adding it in a separate component would be appropriate.

If we wanted to put this code in a separate repository, I think that would be a valid choice as well; it would just take a bit more effort to keep the code synchronized and manage version differences. It would also make it easier to limit our commitment to support of the USD format, for better or worse.

Signed-off-by: ahcorde <ahcorde@gmail.com>

koonpeng · 2022-01-05T08:06:34Z

src/usd/usd_parser/polygon_helper.cc

+            fit != triangulation.finite_facets_end(); ++fit)
+        {
+          auto &face = fit;
+          if (face->first->vertex(0)->info() > maxIndex ||


maxIndex is not defined.

koonpeng · 2022-01-05T08:08:26Z

src/Conversions_TEST.cc

+
+/////////////////////////////////////////////////
+/// Main
+int main(int argc, char **argv)


This shouldn't be needed if you link to gtest_main.

koonpeng · 2022-01-05T08:12:20Z

src/cmd/sdfconverter.cpp

+{
+  auto opt = std::make_shared<Options>();
+
+  _app.add_option("-i,--input",


nit: It will be nice to have the same cli for usd -> sdf and sdf -> usd converter, iirc, the sdf -> usd converter takes the input and output args as positionals.

koonpeng · 2022-01-05T08:18:33Z

src/parser.cc

    return false;
  }

+  if (USD2SDF::IsUSD(filename))


Is embedding the usd converter into the sdf parser something we should be doing?

koonpeng · 2022-01-05T08:24:02Z

src/parser.cc

+    auto doc = makeSdfDoc();
+    usd2g.read(filename, &doc);
+    sdferr << "here! it's a USD!\n";
+    if (sdf::readDoc(&doc, _sdf, "usd file", _convert, _config, _errors))


I don't quite understand this, shouldn't the 3rd param be the filename?

koonpeng · 2022-01-06T07:31:41Z

src/usd/usd_parser/links.cc

+    {
+      std::cerr << "UsdPhysicsMassAPI " <<  pxr::TfStringify(_prim.GetPath()) << '\n';
+      if (pxr::UsdAttribute massAttribute =
+        _prim.GetAttribute(pxr::TfToken("physics:mass")))


Could we use pxr::UsdPhysicsMassApi to avoid hard coded tokens?

koonpeng · 2022-01-06T07:46:05Z

src/usd/usd_parser/links.cc

+        if (mass < 0.0001)
+        {
+          mass = 0.1;
+        }


whats the reason for having a minimal mass?

koonpeng · 2022-01-06T08:02:36Z

src/usd/usd_parser/physics.cc

+    _world->gravity[0] = gravity[0];
+    _world->gravity[1] = gravity[1];
+    _world->gravity[2] = gravity[2];


Do we need to account for up axis?

koonpeng · 2022-01-06T08:03:56Z

src/usd/usd_parser/sensors.cc

+      sdf::Camera camera;
+
+      auto variantCamera = pxr::UsdGeomCamera(_prim);
+      float horizontalAperture = 20.955;


What is this value based off from?

koonpeng · 2022-01-06T08:06:08Z

src/usd/usd_parser/utils.cc

+            {
+              pxr::SdfAssetPath materialPath;
+              pxr::UsdShadeInput diffuseTextureShaderInput =
+                variantshader.GetInput(pxr::TfToken("diffuse_texture"));


Is there a doc explaining the available tokens?

ahcorde · 2022-01-06T15:20:33Z

TODO

Review "Y axis" usd files
Generate triangles from a polygon (with more than 3 vertices per polygon)
- Remove CGAL dependency, use instead gts
Cleanup
Document

Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>

Signed-off-by: ahcorde <ahcorde@gmail.com>

adlarkin · 2022-01-13T17:55:45Z

I've added the needs upstream release label to this PR because the build fails without gazebosim/gz-math#366. Based on the discussion had in gazebosim/gz-math#366 (comment), I'm wondering if this PR is actually needed - it seems like an unrelated change/addition in this PR is what's introducing the build error. I will follow-up on this if I find anything in this PR that could be related to the build error.

adlarkin

This PR is really large and doesn't have documentation, so it's hard to review all at once. Once #817 is merged, would you mind refactoring this PR to match the component structure? Then, we can break it up into smaller PRs and review the smaller PRs, like what we're doing for SDF -> USD conversion.

adlarkin · 2022-01-14T22:54:13Z

src/usd/usd_parser/parser_usd.cc

+    if (tokens.size() > 1)
+    {
+      bool shortName = false;
+      if (tokens.size() == 2)
+      {
+        if (prim.IsA<pxr::UsdGeomGprim>() ||
+            (std::string(prim.GetPrimTypeInfo().GetTypeName().GetText()) == "Plane"))
+        {
+          if (prim.GetPath().GetName() != "imu")
+          {
+            baseLink = "/" + tokens[0];
+            shortName = true;
+            // exit(-1);
+          }
+        }
+      }
+      if(!shortName)
+      {
+        baseLink = "/" + tokens[0] + "/" + tokens[1];
+      }
+    }


it looks like this code block can be deleted, because tokens.size() > 1 is already being checked in the above if statement: https://github.com/ignitionrobotics/sdformat/blob/58ac549f454601bcf1f58917965c64d93617f527/src/usd/usd_parser/parser_usd.cc#L183

adlarkin · 2022-01-14T22:59:03Z

src/usd/usd_parser/utils.cc

+      if (pxr::TfStringify(parent.GetPath()) == _name)
+      {
+        return;
+      }


This check can be removed, because it's comparing the prim's path (which has a /) with the prim's name (which does not have a /).

src/usd/usd_parser/utils.cc

* polygon triangulation using the fan-triangulation algorithm Signed-off-by: Teo Koon Peng <koonpeng@openrobotics.org> * remove cgal dep Signed-off-by: Teo Koon Peng <koonpeng@openrobotics.org> * add todo Signed-off-by: Teo Koon Peng <koonpeng@openrobotics.org> * fix xformOp:transform parsing (#822) Signed-off-by: Teo Koon Peng <koonpeng@openrobotics.org> Co-authored-by: ahcorde <ahcorde@gmail.com>

Signed-off-by: ahcorde <ahcorde@gmail.com>

* fix y up Signed-off-by: Teo Koon Peng <koonpeng@openrobotics.org> * cleanup Signed-off-by: Teo Koon Peng <koonpeng@openrobotics.org>

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde · 2022-04-19T13:28:27Z

Closing this PR in favour of this metaticket #866

ahcorde added 3 commits October 27, 2021 14:10

prototype

04ac451

Signed-off-by: Alejandro Hernández <ahcorde@gmail.com>

prototype

fec7541

Signed-off-by: ahcorde <ahcorde@gmail.com>

prototype

9ce073f

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde requested a review from adlarkin October 27, 2021 16:21

ahcorde assigned ahcorde and adlarkin Oct 27, 2021

azeey reviewed Oct 28, 2021

View reviewed changes

ahcorde added 3 commits November 2, 2021 17:31

User ignition Math, common and sdformat typs

787e8f5

Signed-off-by: ahcorde <ahcorde@gmail.com>

using sdf::joint

5115f44

Signed-off-by: ahcorde <ahcorde@gmail.com>

Vehicle joint and some other fixes

d4cb4cc

Signed-off-by: ahcorde <ahcorde@gmail.com>

adlarkin mentioned this pull request Nov 4, 2021

Remove outdated deprecation note from parser_urdf.hh #740

Merged

7 tasks

ahcorde added 3 commits November 5, 2021 15:12

materials and geometry subsets

9b73015

Signed-off-by: ahcorde <ahcorde@gmail.com>

Improvements in materials, meshes and joints

a74ee34

Signed-off-by: ahcorde <ahcorde@gmail.com>

Improve materials, meshes and others

fe2ddb0

Signed-off-by: ahcorde <ahcorde@gmail.com>

chapulina mentioned this pull request Nov 12, 2021

Bump in Garden: ign-msgs and dependents gazebo-tooling/release-tools#554

Closed

21 tasks

more updates

ce46639

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde added 4 commits November 23, 2021 21:17

Fix light API and other things

163637a

Signed-off-by: ahcorde <ahcorde@gmail.com>

Fixed material and polygon to triangles with CGAL

09eea1f

Signed-off-by: ahcorde <ahcorde@gmail.com>

Fixed textures and subgeom pose

c344a55

Signed-off-by: ahcorde <ahcorde@gmail.com>

More changes

5bd5c7b

Signed-off-by: ahcorde <ahcorde@gmail.com>

This was referenced Nov 30, 2021

SDF to USD (main) #763

Closed

convert SDF to USD #762

Closed

ahcorde added 2 commits December 2, 2021 00:01

Added lidar, collisions and others

0c629c2

Signed-off-by: ahcorde <ahcorde@gmail.com>

Add more code

58ac549

Signed-off-by: ahcorde <ahcorde@gmail.com>

koonpeng reviewed Jan 6, 2022

View reviewed changes

adlarkin and others added 4 commits January 13, 2022 09:45

fix USD -> SDF converter build

1e9890b

Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>

Fixed directory name

304d27c

Signed-off-by: ahcorde <ahcorde@gmail.com>

Added tutorials

e11a18e

Signed-off-by: ahcorde <ahcorde@gmail.com>

Added missing dependency

83f86ab

Signed-off-by: ahcorde <ahcorde@gmail.com>

adlarkin mentioned this pull request Jan 13, 2022

Use const instead of constexpr in Ellipsoid constructor gazebosim/gz-math#366

Merged

7 tasks

adlarkin added the needs upstream release Blocked by a release of an upstream library label Jan 13, 2022

adlarkin suggested changes Jan 15, 2022

View reviewed changes

Teo Koon Peng and others added 6 commits January 18, 2022 13:58

Some more fixes

f2e88dd

Signed-off-by: ahcorde <ahcorde@gmail.com>

cleanup

4deb2bc

Signed-off-by: ahcorde <ahcorde@gmail.com>

Fix

1c4ba22

Signed-off-by: ahcorde <ahcorde@gmail.com>

USD -> SDF: fix y up (#825)

d2fd2a8

* fix y up Signed-off-by: Teo Koon Peng <koonpeng@openrobotics.org> * cleanup Signed-off-by: Teo Koon Peng <koonpeng@openrobotics.org>

cleanup

d82cf39

Signed-off-by: ahcorde <ahcorde@gmail.com>

scpeters added the usd label Feb 21, 2022

some fixes

bfd8d08

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde mentioned this pull request Mar 4, 2022

USD -> SDF conversion support #866

Closed

ahcorde closed this Apr 19, 2022

rbonghi mentioned this pull request May 12, 2022

Nanosaur on IsaacSIM rnanosaur/nanosaur_simulation#4

Closed

Conversation

ahcorde commented Oct 27, 2021

🎉 New feature

Summary

Test it

Checklist

Uh oh!

scpeters commented Oct 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azeey commented Oct 28, 2021

Uh oh!

chapulina commented Oct 28, 2021

Uh oh!

scpeters commented Oct 28, 2021

Uh oh!

chapulina commented Oct 28, 2021

Uh oh!

koonpeng commented Nov 2, 2021

Uh oh!

scpeters commented Nov 2, 2021

Uh oh!

scpeters commented Nov 2, 2021

Uh oh!

adlarkin commented Nov 4, 2021

Uh oh!

adlarkin commented Nov 4, 2021

Uh oh!

scpeters commented Nov 4, 2021

Uh oh!

scpeters commented Nov 4, 2021

Uh oh!

adlarkin commented Nov 4, 2021

Uh oh!

scpeters commented Nov 4, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahcorde commented Jan 6, 2022

Uh oh!

adlarkin commented Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adlarkin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahcorde commented Apr 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

adlarkin commented Jan 13, 2022 •

edited

Loading