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

USD -> SDF: Parse Physics and generated SDF file #863

Merged
merged 22 commits into from
Mar 15, 2022

Conversation

ahcorde
Copy link
Collaborator

@ahcorde ahcorde commented Feb 25, 2022

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

🎉 New feature

Summary

Parse Physics and generated SDF file

Test it

If you have the isaac-sim-assets-2021.1.1 assets you can try for example:

usd2sdf isaac-sim-assets-2021.1.1//Robots/UR10/ur10.usd ur10.sdf

The expected output must look like:

<sdf version="1.7">
    <world name="ur10_world">
          <gravity>0 0 -9.8</gravity>
    </world>
</sdf>

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 added the usd label Feb 25, 2022
@ahcorde ahcorde self-assigned this Feb 25, 2022
@codecov-commenter
Copy link

codecov-commenter commented Feb 25, 2022

Codecov Report

Merging #863 (5d05f55) into sdf12 (b7447df) will decrease coverage by 0.02%.
The diff coverage is 69.04%.

❗ Current head 5d05f55 differs from pull request most recent head 966dc01. Consider uploading reports for the commit 966dc01 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##            sdf12     #863      +/-   ##
==========================================
- Coverage   88.17%   88.15%   -0.03%     
==========================================
  Files          96      100       +4     
  Lines       14547    14629      +82     
==========================================
+ Hits        12827    12896      +69     
- Misses       1720     1733      +13     
Impacted Files Coverage Δ
usd/src/cmd/usd2sdf.cc 85.18% <42.85%> (+12.45%) ⬆️
usd/src/usd_parser/USD2SDF.cc 50.00% <50.00%> (ø)
usd/src/usd_parser/USDWorld.cc 73.07% <73.07%> (ø)
usd/src/usd_parser/Parser.cc 73.33% <73.33%> (ø)
usd/src/usd_parser/USDPhysics.cc 85.00% <85.00%> (ø)
usd/src/usd_parser/USDMaterial.cc 64.90% <0.00%> (+1.98%) ⬆️
usd/src/usd_parser/USDData.cc 66.94% <0.00%> (+3.38%) ⬆️

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 b7447df...966dc01. Read the comment docs.

@scpeters
Copy link
Member

usd2sdf isaac-sim-assets-2021.1.1//Robots/UR10/ur10.usd ur10.sdf

The expected output must look like:

<sdf version="1.7">
    <world name="ur10_world">
        <physics>
            <gravity>0 0 -9.8</gravity>
        </physics>
    </world>
</sdf>

this example doesn't look right to me. <gravity> should be directly under the <world>; it is not under <physics> in version 1.6+. In 1.5 and earlier, gravity was in the physics block, but not in newer versions.

@ahcorde
Copy link
Collaborator Author

ahcorde commented Feb 25, 2022

usd2sdf isaac-sim-assets-2021.1.1//Robots/UR10/ur10.usd ur10.sdf

The expected output must look like:

<sdf version="1.7">
    <world name="ur10_world">
        <physics>
            <gravity>0 0 -9.8</gravity>
        </physics>
    </world>
</sdf>

this example doesn't look right to me. <gravity> should be directly under the <world>; it is not under <physics> in version 1.6+. In 1.5 and earlier, gravity was in the physics block, but not in newer versions.

Thank you @scpeters , I updated the PR

Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde force-pushed the ahcorde/usd_to_sdf_physics branch from 8955a74 to 3a9b349 Compare March 9, 2022 12:49
ahcorde and others added 4 commits March 9, 2022 19:50
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
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.

I still have a few more files to review, but here are a few questions that I have so far.

usd/src/usd_parser/USDWorld.cc Show resolved Hide resolved
usd/src/usd_parser/USDWorld.cc Outdated Show resolved Hide resolved
usd/src/usd_parser/USDWorld.cc Outdated Show resolved Hide resolved
usd/src/usd_parser/usd_model/WorldInterface.hh Outdated Show resolved Hide resolved
Base automatically changed from ahcorde/usd_to_sdf_cmd to sdf12 March 10, 2022 08:59
@ahcorde ahcorde requested a review from adlarkin March 10, 2022 09:14
ahcorde and others added 2 commits March 10, 2022 13:39
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
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.

I made final iterations in 318eff9. Overall, LGTM, but I don't know much about tinyxml2, so @azeey or @scpeters, would you mind taking one final look before we merge this?

Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
usd/src/cmd/usd2sdf.cc Outdated Show resolved Hide resolved
usd/src/usd_parser/USD2SDF.hh Outdated Show resolved Hide resolved
usd/src/usd_parser/USD2SDF.hh Outdated Show resolved Hide resolved
usd/src/usd_parser/Parser.cc Outdated Show resolved Hide resolved
usd/src/usd_parser/USD2SDF.cc Outdated Show resolved Hide resolved
usd/src/usd_parser/USD2SDF.cc Outdated Show resolved Hide resolved
usd/src/usd_parser/USDPhysics.cc Outdated Show resolved Hide resolved
usd/src/usd_parser/USDPhysics.hh Outdated Show resolved Hide resolved
usd/src/usd_parser/usd_model/WorldInterface.hh Outdated Show resolved Hide resolved
Signed-off-by: ahcorde <ahcorde@gmail.com>
@koonpeng koonpeng self-requested a review March 14, 2022 09:03
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde requested a review from azeey March 14, 2022 11:52
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde requested a review from adlarkin March 14, 2022 16:34
Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
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.

Thank you for the feedback and iterations, @ahcorde and @azeey! The new changes look good, and I think it's much easier to follow with sdf::Root. I just have one small comment, and will let @azeey take one last look before we merge.

usd/src/usd_parser/USDWorld.cc Outdated Show resolved Hide resolved
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde requested a review from adlarkin March 15, 2022 12:00
usd/src/usd_parser/USDPhysics.hh Outdated Show resolved Hide resolved
usd/src/usd_parser/USDPhysics.hh Show resolved Hide resolved
usd/src/usd_parser/USDWorld.cc Outdated Show resolved Hide resolved
usd/src/usd_parser/USD2SDF.cc Outdated Show resolved Hide resolved
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde requested a review from azeey March 15, 2022 15:42
@ahcorde ahcorde merged commit 3c140a1 into sdf12 Mar 15, 2022
@ahcorde ahcorde deleted the ahcorde/usd_to_sdf_physics branch March 15, 2022 18:04
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants