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

SDF to USD: set UsdPhysicsArticulationRootAPI #914

Merged
merged 7 commits into from
Mar 30, 2022

Conversation

ahcorde
Copy link
Collaborator

@ahcorde ahcorde commented Mar 28, 2022

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

🦟 Bug fix

We should set the UsdPhysicsArticulationRootAPI to allow articulation arms to move in IsaacSim

Summary

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

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.

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde requested a review from adlarkin March 28, 2022 13:17
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Mar 28, 2022
@codecov-commenter
Copy link

codecov-commenter commented Mar 28, 2022

Codecov Report

Merging #914 (83cfcac) into sdf12 (e8de24e) will decrease coverage by 0.05%.
The diff coverage is 45.00%.

@@            Coverage Diff             @@
##            sdf12     #914      +/-   ##
==========================================
- Coverage   88.05%   88.00%   -0.06%     
==========================================
  Files         100      100              
  Lines       14696    14714      +18     
==========================================
+ Hits        12941    12949       +8     
- Misses       1755     1765      +10     
Impacted Files Coverage Δ
usd/src/sdf_parser/Model.cc 52.05% <44.44%> (-2.50%) ⬇️
usd/src/sdf_parser/Link.cc 61.84% <50.00%> (ø)

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 e8de24e...83cfcac. Read the comment docs.

@adlarkin
Copy link
Contributor

@ahcorde I proposed an alternative approach in #916, would you mind taking a look?

@chapulina chapulina added the usd label Mar 28, 2022
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>
@ahcorde ahcorde requested a review from adlarkin March 28, 2022 20:53
@ahcorde
Copy link
Collaborator Author

ahcorde commented Mar 28, 2022

@adlarkin I made more changes, it was not working in same cases.

Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
@adlarkin adlarkin force-pushed the ahcorde/sdf_to_usd/fix_articulation_arms branch from 19e40f1 to b9dc8ae Compare March 28, 2022 21:40
Comment on lines +178 to +180
if (!markedArticulationRoot && !_model.Static())
{
if (!pxr::UsdPhysicsRigidBodyAPI::Apply(modelPrim))
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a few questions about this:

  1. If we are marking the model as a rigid body API, then why do we need to also mark the link as a rigid body API? I believe that marking only the model as a rigid body API should be sufficient, since the link is a child prim of the model prim.
  2. In the first if block, why do we check if the model is not static and if the model has not been marked as an articulation root? As I understand it, pxr::UsdPhysicsRigidBodyAPI only implies static/non-static, which means that articulation root API should not have any influence on whether a prim is marked as a rigid body API or not. The only thing that should influence whether a prim is a rigid body API is whether it is static or not, so I believe the first if statement here should only check if the model is not static instead of checking of the model isn't static and also isn't marked as an articulation root API.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When I was converting the panda robot to USD I was having some issues. Here it's what I found testing the model in Isaac Sim

  • A UsdPhysicsRigidBodyAPI can't be marked as rigidbody otherwise I was having issue with the joints
  • If the parent is marked as UsdPhysicsRigidBodyAPI then I need to set all children to rigidbody.

I don't have a clear explanation about why is this happening but I fix this based on the behaviour in Issac Sim

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, thanks for the feedback 👍 so if a model prim is marked as an articulation root API, but not as a rigid body API, does it still move without issues? From the initial reading I did on USD, if a prim isn't marked as a rigid body, then I thought this prim would be interpreted as static.

@ahcorde ahcorde self-assigned this Mar 29, 2022
@ahcorde ahcorde merged commit 4d9f8a4 into sdf12 Mar 30, 2022
@ahcorde ahcorde deleted the ahcorde/sdf_to_usd/fix_articulation_arms branch March 30, 2022 12:22
adlarkin added a commit that referenced this pull request Apr 1, 2022
Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress usd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants