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

MJCF to SDF: Improved inertia #48

Merged
merged 15 commits into from
May 31, 2022
Merged

MJCF to SDF: Improved inertia #48

merged 15 commits into from
May 31, 2022

Conversation

ahcorde
Copy link
Collaborator

@ahcorde ahcorde commented May 26, 2022

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

🦟 Bug fix

Summary

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • 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 azeey May 26, 2022 18:52
@ahcorde ahcorde self-assigned this May 26, 2022
Signed-off-by: ahcorde <ahcorde@gmail.com>
@codecov
Copy link

codecov bot commented May 26, 2022

Codecov Report

Merging #48 (bb61d7a) into main (5a6ce8d) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #48      +/-   ##
==========================================
+ Coverage   97.77%   97.79%   +0.02%     
==========================================
  Files          16       16              
  Lines         628      636       +8     
==========================================
+ Hits          614      622       +8     
  Misses         14       14              
Impacted Files Coverage Δ
...cf_to_sdformat/mjcf_to_sdformat/converters/link.py 100.00% <100.00%> (ø)
...f_to_sdformat/mjcf_to_sdformat/converters/world.py 96.15% <100.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 5a6ce8d...bb61d7a. Read the comment docs.

Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@scpeters
Copy link
Member

I resolved conflicts but am getting a test failure locally

Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde mentioned this pull request May 31, 2022
9 tasks
@@ -23,7 +23,7 @@
COLLISION_NUMBER = 0


def mjcf_geom_to_sdf(geom):
def mjcf_body_to_sdf(geom):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This converting geoms, so I think it should be mjcf_geom_to_sdf. The one in
link.py should be mjcf_body_to_sdf. Is this an accidental
search/replace?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -7,7 +7,7 @@
<joint type="free"/>
<inertial pos="0 1 0" mass="2" fullinertia="2 2 2 0 0 0"/>
<geom type="box" pos="0.1 0.3 0.2" size=".1 .2 .3" rgba="0 .9 0 1"/>
<geom name="cylinder" type="capsule" pos="1 2 3" mass="1" fromto="0 -0.06 0 0 0.06 0" size="0.05"/>
<geom name="cylinder" type="capsule" mass="1" fromto="0 -0.06 0 0 0.06 0" size="0.05"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this named cylinder intentially even though it's a capsule?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ahcorde ahcorde requested a review from azeey May 31, 2022 16:17
Signed-off-by: ahcorde <ahcorde@gmail.com>
@azeey
Copy link
Collaborator

azeey commented May 31, 2022

tests are failing.

@ahcorde
Copy link
Collaborator Author

ahcorde commented May 31, 2022

tests are failing.

@azeey Fixed

physics.named.model.body_iquat[body.name][0],
physics.named.model.body_iquat[body.name][1],
physics.named.model.body_iquat[body.name][2],
physics.named.model.body_iquat[body.name][3]))
Copy link
Member

Choose a reason for hiding this comment

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

nit: I would prefer to have a helper function in sdformat_mjcf_utils/sdf_utils.py that assists with this to ensure that we get the quaternion wxyz ordering right. Maybe wxyz_list_to_quat?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ahcorde
Copy link
Collaborator Author

ahcorde commented May 31, 2022

nit: this is a position, not a pose, so I would rename inertia_pose to inertia_position or inertia_pos

@scpeters 55a3dc5

@ahcorde ahcorde merged commit d559811 into main May 31, 2022
@ahcorde ahcorde deleted the ahcorde/mjcf2sdf/inertia branch May 31, 2022 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants