Skip to content

Hide the USD component's Util.hh file from public API#850

Merged
scpeters merged 4 commits intosdf12from
adlarkin/hide_sdf_usd_parser_utils
Feb 15, 2022
Merged

Hide the USD component's Util.hh file from public API#850
scpeters merged 4 commits intosdf12from
adlarkin/hide_sdf_usd_parser_utils

Conversation

@adlarkin
Copy link
Copy Markdown
Contributor

@adlarkin adlarkin commented Feb 12, 2022 •
edited
Loading

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

🦟 Bug fix

Summary

When I originally worked on #818, I created a set of helper methods for achieving generic parsing tasks between SDF and USD. These methods were placed in usd/include/sdf/usd/sdf_parser/Utils.hh. After #818 was merged, I realized that the Utils.hh is meant for internal use only, and should not be exposed to downstream users. So, this PR moves Utils.hh to usd/src, and also updates the methods in this file to make use of the UsdError class that was introduced in #836.

It should be noted that the ABI checker may fail since I am removing methods from the public API. However, I talked with others offline, and they said that for a scenario like this, it's okay because the public API I am changing in the USD component has not been released yet. So, we will have to ignore the ABI checker for this PR.

Checklist

  • Signed all commits for DCO
  • Added tests
  • 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

@adlarkin
move usd's utils.hh helpers to src/ directory since it's meant for in…
1f3bf93 
…ternal use

Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
@adlarkin adlarkin requested a review from ahcorde February 12, 2022 03:49
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Feb 12, 2022
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Feb 12, 2022 •
edited
Loading

Codecov Report

Merging #850 (1f3bf93) into sdf12 (eed1aa9) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##            sdf12     #850   +/-   ##
=======================================
  Coverage   90.76%   90.76%           
=======================================
  Files          78       78           
  Lines       12535    12535           
=======================================
  Hits        11378    11378           
  Misses       1157     1157

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 eed1aa9...1f3bf93. Read the comment docs.

scpeters
scpeters reviewed Feb 12, 2022
View reviewed changes
Copy link
Copy Markdown
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.

I wouldn't expect the ABI checker to fail since that CI job does not have the pxr dependency installed or built from source

also, it looks like this isn't simply a move of the header file; it looks like you made some changes to some of the methods in Utils.hh as well. This could be fine, but you should explain them in the pull request description

@adlarkin
Copy link
Copy Markdown
Contributor Author

adlarkin commented Feb 13, 2022

I wouldn't expect the ABI checker to fail since that CI job does not have the pxr dependency installed or built from source

You're right, ABI checker didn't fail 👍

also, it looks like this isn't simply a move of the header file; it looks like you made some changes to some of the methods in Utils.hh as well. This could be fine, but you should explain them in the pull request description

Yes, you are right, there are some slight changes. They're in the PR description if you look at the end of the first paragraph:

also updates the methods in this file to make use of the UsdError class that was introduced in https://github.com/ignitionrobotics/sdformat/pull/836.

Let me know if you think any other descriptions/documentation needs to be added.

@adlarkin
Copy link
Copy Markdown
Contributor Author

adlarkin commented Feb 15, 2022

@scpeters @ahcorde I renamed the file from Utils.hh to UsdUtils.hh and added tests for the methods in this file in 33beba4. This should be ready for review if you wouldn't mind taking a look.

@adlarkin adlarkin requested a review from scpeters February 15, 2022 04:03
@ahcorde
Copy link
Copy Markdown
Collaborator

ahcorde commented Feb 15, 2022

@osrf-jenkins retest this please

scpeters
scpeters reviewed Feb 15, 2022
View reviewed changes
Comment thread usd/src/UsdUtils.hh Outdated
@adlarkin
fix PoseWrtParent docs (get, not set)
e5ac105 
Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
@adlarkin adlarkin requested a review from scpeters February 15, 2022 16:05
@scpeters scpeters merged commit 9272cb6 into sdf12 Feb 15, 2022
@scpeters scpeters deleted the adlarkin/hide_sdf_usd_parser_utils branch February 15, 2022 18:33
@adlarkin adlarkin mentioned this pull request Feb 15, 2022
Merged
7 tasks
@ahcorde ahcorde mentioned this pull request Mar 4, 2022
Closed
8 tasks
@osrf-triage
Copy link
Copy Markdown

osrf-triage commented Apr 13, 2022

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

Reviewers

@ahcorde ahcorde ahcorde approved these changes

@scpeters scpeters scpeters approved these changes

@azeey azeey Awaiting requested review from azeey azeey is a code owner

Assignees

No one assigned

Labels

🏯 fortress Ignition Fortress

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@adlarkin @codecov-commenter @ahcorde @osrf-triage @scpeters @j-rivero