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 to SDF: Initial commit #827

Merged
merged 59 commits into from
Mar 9, 2022
Merged

Conversation

ahcorde
Copy link
Collaborator

@ahcorde ahcorde commented Jan 20, 2022

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

🎉 New feature

Summary

This PR is the initial commit to support the conversion between USD to SDF.

The two classes included here allow to load stage and get the relevant data such us:

  • upaxis
  • meterperunit
  • materials

Test it

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.

adlarkin and others added 21 commits January 7, 2022 12:36
Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.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>
Signed-off-by: ahcorde <ahcorde@gmail.com>
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: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
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: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
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 January 20, 2022 20:28
@ahcorde ahcorde self-assigned this Jan 20, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jan 20, 2022

Codecov Report

Merging #827 (0765a26) into sdf12 (41d370d) will decrease coverage by 0.45%.
The diff coverage is 67.10%.

❗ Current head 0765a26 differs from pull request most recent head bf579ed. Consider uploading reports for the commit bf579ed to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##            sdf12     #827      +/-   ##
==========================================
- Coverage   88.65%   88.19%   -0.46%     
==========================================
  Files          92       95       +3     
  Lines       14221    14525     +304     
==========================================
+ Hits        12607    12811     +204     
- Misses       1614     1714     +100     
Impacted Files Coverage Δ
usd/src/usd_parser/USDMaterial.cc 62.91% <62.91%> (ø)
usd/src/usd_parser/USDData.cc 63.55% <63.55%> (ø)
usd/src/usd_parser/USDStage.cc 97.14% <97.14%> (ø)

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 41d370d...bf579ed. Read the comment docs.

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 noticed that the USD -> SDF converter has a different file structure than the SDF -> USD converter. The USD -> SDF converter has headers in usd/include/sdf/usd_parser instead of usd/include/sdf/usd, and source files are in usd/src/usd_parser instead of usd/src. I think it makes sense to separate the USD -> SDF and SDF -> USD header/source files, but what if we used the following file structure for both converters so that they are more aligned:

  • Have a usd/include/sdf/usd directory, which is the "top level" header directory. SDF -> USD converter headers can be in the usd/include/sdf/usd/sdf_parser directory, and USD -> SDF converter headers can be in the usd/include/sdf/usd/usd_parser.
  • Keep a usd/src directory, which is the "top level" source directory. SDF -> USD converter sources can be in the usd/src/sdf_parser directory, and USD -> SDF converter sources can be in the usd/src/usd_parser directory.

Another option would be to have both converter header files in usd/include/sdf/usd, and both converter source files in usd/src, but this could make it difficult for users to know which files are associated with which parser.

If we decide to re-work the file structure to what I proposed above, then we would need to update the file structure in #817 before merging it.

File structure has been reworked in 2088f1e

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde requested a review from koonpeng March 2, 2022 09:05
ahcorde and others added 3 commits March 7, 2022 17:29
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 haven't reviewed all of the files yet, but I do have a few questions:

  1. test/usd/empty_file.usda is not used. Can this file be removed?
  2. Can tests be added for the API in UsdMaterial.hh?

Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Collaborator Author

ahcorde commented Mar 8, 2022

I haven't reviewed all of the files yet, but I do have a few questions:

  1. test/usd/empty_file.usda is not used. Can this file be removed?
  2. Can tests be added for the API in UsdMaterial.hh?
  1. f77cd78
  2. 0f34f61

@ahcorde
Copy link
Collaborator Author

ahcorde commented Mar 8, 2022

@osrf-jenkins retest this please

@ahcorde
Copy link
Collaborator Author

ahcorde commented Mar 8, 2022

@azeey do you mind to take another look to this pull request?

@adlarkin adlarkin force-pushed the ahcorde/usd_to_sdf_initial_commit branch 2 times, most recently from ebfbc1f to 3ae7fd9 Compare March 8, 2022 22:51
…unit tests

Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
@adlarkin adlarkin force-pushed the ahcorde/usd_to_sdf_initial_commit branch from 3ae7fd9 to 6ada66f Compare March 8, 2022 22:51
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.

Just a few more minor comments. LGTM otherwise.

usd/include/sdf/usd/usd_parser/USDData.hh Outdated Show resolved Hide resolved
usd/include/sdf/usd/usd_parser/USDData.hh Outdated Show resolved Hide resolved
usd/include/sdf/usd/usd_parser/USDData.hh Outdated Show resolved Hide resolved
usd/include/sdf/usd/usd_parser/USDStage.hh Outdated Show resolved Hide resolved
usd/src/usd_parser/USDData_TEST.cc Outdated Show resolved Hide resolved
adlarkin and others added 4 commits March 8, 2022 20:14
Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde merged commit 346f3ae into sdf12 Mar 9, 2022
@ahcorde ahcorde deleted the ahcorde/usd_to_sdf_initial_commit branch March 9, 2022 11:42
@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

This pull request was closed.
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.

7 participants