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

Added Mesh Moment of Inertia Calculator #2061

Merged

Conversation

jasmeet0915
Copy link
Contributor

@jasmeet0915 jasmeet0915 commented Jul 28, 2023

🎉 New feature

Based on this proposal.

Summary

  • The PR implements a Moment of Inertia Calculator based on the method described in this document.
  • The calculator works using the callback-based API from the sdformat made in this PR.

Demo & Usage Example

Demo 1: This demo shows the automatic inertia calculation feature on a rubber ducky model which is a non-convex mesh. On the left, we have the rubber ducky mesh with automatic calculations enabled and on the right, the mesh uses the default values.

Note: The inertial values are due to the scale of the mesh. You can see the banana for scale in between the 2 ducks. A density value for the duck was used which was calculated by using the mass and volume data of the duck found online.

Note: The voxel_size inertia param given in the snippet below is just for showing how the <auto_inertia_params> element could be used. This is not actually used by the calculator since we are not using a voxelization-based calculator.

SDF snippet for the duck mesh with auto inertial
<?xml version="1.0" ?>
<sdf version="1.6">
<model name="duck">
  <link name="duck_link">
    <pose>0 0 0 0 0 0</pose>
    <inertial auto="true" />
    <collision name="duck_collision">
    	<pose>0 0 0 0 0 0</pose>
      <density>111.8</density>
      <auto_inertia_params>
        <gz:voxel_size>0.01</gz:voxel_size>
      </auto_inertia_params>
      <geometry>
        <mesh>
          <uri>meshes/duck_collider.dae</uri>
        </mesh>
      </geometry>
    </collision>
    <visual name="duck_visual">
      <pose>0 0 0 0 0 0</pose>
      <geometry>
        <mesh>
          <uri>meshes/duck.dae</uri>
        </mesh>
      </geometry>
    </visual>
  </link>
  <static>true</static>
</model>
</sdf>

duck_mesh_inertia

Demo 2: This demo shows 2 cylinders: One using a Collada cylinder mesh (right) and the other made using the <cylinder> geometry from SDF (left).

Both use <inertial auto="true" /> and we can see that the inertia values for both come up to be almost the same (within 0.005 tolerance). The mesh cylinder uses the mesh inertia calculator added to gz-sim and is used with libsdformat using the callback-based API.

SDF snippet for the mesh cylinder
  <model name="cylinder_dae">
    <pose>4 4 1 0 0 0</pose>
    <link name="cylinder_dae">
      <pose>0 0 0 0 0 0</pose>
      <inertial auto="false" />
      <collision name="cylinder_collision">
        <density>1240.0</density>
        <auto_inertia_params>
          <gz:voxel_size>0.01</gz:voxel_size>
        </auto_inertia_params>
        <geometry>
          <mesh>
            <uri>cylinder_dae/meshes/cylinder.dae</uri>
          </mesh>
        </geometry>
      </collision>
      <visual name="cylinder_visual">
        <pose>0 0 0 0 0 0</pose>
        <geometry>
          <mesh>
            <uri>cylinder_dae/meshes/cylinder.dae</uri>
          </mesh>
        </geometry>
      </visual>
    </link>
    <static>true</static>
  </model>

mesh_and_sdf_cylinder

TODO:

  • Update return type for Inertia Calculator to optional inertial object
  • Add Tests

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • 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.

@github-actions github-actions bot added the 🎵 harmonic Gazebo Harmonic label Jul 28, 2023
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

some lines are more than 100 characters, review style too

Is there any reason to include the implementation in the hpp ? I think you should move the implementation to a cpp file.

Add some tests too

and add some tests

include/gz/sim/MeshInertiaCalculator.hh Outdated Show resolved Hide resolved
include/gz/sim/MeshInertiaCalculator.hh Outdated Show resolved Hide resolved
include/gz/sim/MeshInertiaCalculator.hh Outdated Show resolved Hide resolved
include/gz/sim/MeshInertiaCalculator.hh Outdated Show resolved Hide resolved
include/gz/sim/MeshInertiaCalculator.hh Outdated Show resolved Hide resolved
@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 31, 2023
@azeey azeey self-requested a review July 31, 2023 18:27
@azeey
Copy link
Contributor

azeey commented Aug 16, 2023

Can you merge from main. Some of the changes in this PR are already in main.

@jasmeet0915 jasmeet0915 force-pushed the jasmeet/custom_mesh_inerita_calculator branch from 96278c4 to d440184 Compare August 17, 2023 16:41
Signed-off-by: Jasmeet Singh <[email protected]>
@jasmeet0915 jasmeet0915 force-pushed the jasmeet/custom_mesh_inerita_calculator branch from d440184 to 3fc1587 Compare August 21, 2023 22:42
@jasmeet0915 jasmeet0915 marked this pull request as ready for review August 21, 2023 23:10
@jasmeet0915 jasmeet0915 requested a review from mjcarroll as a code owner August 21, 2023 23:10
@jasmeet0915
Copy link
Contributor Author

@ahcorde

some lines are more than 100 characters, review style too

Corrected the style and completed codecheck in 3fc1587

Is there any reason to include the implementation in the hpp ? I think you should move the implementation to a cpp file.

I have removed implementation from the header file and added it to a separate source file.

@jasmeet0915
Copy link
Contributor Author

@azeey I have merged main in 40c5100

Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

Maybe add a test for the MeshInertiaCalculator class?

CMakeLists.txt Outdated Show resolved Hide resolved
include/gz/sim/MeshInertiaCalculator.hh Outdated Show resolved Hide resolved
src/MeshInertiaCalculator.cc Outdated Show resolved Hide resolved
src/MeshInertiaCalculator.cc Outdated Show resolved Hide resolved
@jasmeet0915
Copy link
Contributor Author

@iche033 I have added an integration test for the class in a53f441

include/gz/sim/MeshInertiaCalculator.hh Outdated Show resolved Hide resolved
include/gz/sim/MeshInertiaCalculator.hh Outdated Show resolved Hide resolved
src/MeshInertiaCalculator.cc Show resolved Hide resolved
src/MeshInertiaCalculator.cc Show resolved Hide resolved
src/Server.cc Outdated Show resolved Hide resolved
test/integration/mesh_inertia_calculation.cc Show resolved Hide resolved
test/integration/mesh_inertia_calculation.cc Show resolved Hide resolved
test/integration/mesh_inertia_calculation.cc Outdated Show resolved Hide resolved
test/integration/mesh_inertia_calculation.cc Show resolved Hide resolved
@iche033
Copy link
Contributor

iche033 commented Aug 31, 2023

hmm there seems to be more test failures than usual

@iche033
Copy link
Contributor

iche033 commented Aug 31, 2023

merging #2105 first

@jasmeet0915
Copy link
Contributor Author

hmm there seems to be more test failures than usual

@iche033 the test failures were coming due to not using the ParserConfig::GlobalConfig() before registering the mesh inertia calculator and calling Root::Load() in Sever.cc.This was causing the tests using fuel resources to not be able to resolve the URIs. This should be fixed now in 7864454

@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

Merging #2061 (0b95e6f) into gz-sim8 (8e7f612) will increase coverage by 0.08%.
The diff coverage is 91.89%.

❗ Current head 0b95e6f differs from pull request most recent head 7b1dbe5. Consider uploading reports for the commit 7b1dbe5 to get more accurate results

@@             Coverage Diff             @@
##           gz-sim8    #2061      +/-   ##
===========================================
+ Coverage    65.34%   65.43%   +0.08%     
===========================================
  Files          322      323       +1     
  Lines        30554    30699     +145     
===========================================
+ Hits         19967    20088     +121     
- Misses       10587    10611      +24     
Files Changed Coverage Δ
src/Server.cc 87.07% <80.00%> (-0.06%) ⬇️
src/MeshInertiaCalculator.cc 92.75% <92.75%> (ø)

... and 2 files with indirect coverage changes

- Added check to compare inertia origin and centre of mass

Signed-off-by: Jasmeet Singh <[email protected]>
Signed-off-by: Jasmeet Singh <[email protected]>
Signed-off-by: Jasmeet Singh <[email protected]>
@azeey azeey changed the base branch from main to gz-sim8 August 31, 2023 13:17
@azeey azeey dismissed ahcorde’s stale review August 31, 2023 14:54

Feedback addressed

@azeey azeey merged commit 52167a6 into gazebosim:gz-sim8 Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🎵 harmonic Gazebo Harmonic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants