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

Set collision detector and solver from SDF #684

Merged
merged 10 commits into from
Jun 16, 2021
Merged

Conversation

chapulina
Copy link
Contributor

🎉 New feature

Requires gazebosim/gz-physics#225

Summary

This makes it possible to choose a different collision detector and solver when using DART. This way, users can choose the combination that suits their simulation best.

Test it

Try setting different options in SDF, for example:

      <dart>
        <collision_detector>bullet</collision_detector>
        <solver>
          <solver_type>pgs</solver_type>
        </solver>
      </dart>

Try the example world:

ign gazebo physics_options.sdf

See that it behaves different from rolling_shapes.sdf:

rolling_shapes.sdf physics_options.sdf
default_rolling bullet_pgs

I also made the options visible on the GUI:

image

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

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

@chapulina chapulina added enhancement New feature or request physics Involves Ignition Physics close the gap Features from Gazebo-classic needs upstream release Blocked by a release of an upstream library labels Mar 16, 2021
@chapulina chapulina requested a review from azeey as a code owner March 16, 2021 03:29
@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Mar 16, 2021
@chapulina chapulina added the beta Targeting beta release of upcoming collection label Mar 17, 2021
@chapulina chapulina self-assigned this Mar 19, 2021
@chapulina chapulina requested a review from scpeters March 19, 2021 01:39
@codecov
Copy link

codecov bot commented Mar 24, 2021

Codecov Report

Merging #684 (17ddc3b) into ign-gazebo5 (d3ee64a) will decrease coverage by 0.06%.
The diff coverage is 57.30%.

❗ Current head 17ddc3b differs from pull request most recent head 16b1af4. Consider uploading reports for the commit 16b1af4 to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           ign-gazebo5     #684      +/-   ##
===============================================
- Coverage        65.32%   65.26%   -0.07%     
===============================================
  Files              240      240              
  Lines            17624    17718      +94     
===============================================
+ Hits             11513    11563      +50     
- Misses            6111     6155      +44     
Impacted Files Coverage Δ
.../plugins/component_inspector/ComponentInspector.cc 6.73% <0.00%> (-0.48%) ⬇️
src/rendering/RenderUtil.cc 42.10% <0.00%> (-0.05%) ⬇️
src/systems/physics/Physics.hh 100.00% <ø> (ø)
src/SdfEntityCreator.cc 84.94% <16.66%> (-2.28%) ⬇️
src/systems/physics/Physics.cc 69.04% <74.32%> (-0.40%) ⬇️
src/systems/physics/EntityFeatureMap.hh 93.65% <90.90%> (ø)
include/ignition/gazebo/components/Physics.hh 100.00% <100.00%> (ø)
src/LevelManager.cc 88.97% <100.00%> (+0.35%) ⬆️
src/systems/diff_drive/DiffDrive.cc 86.91% <100.00%> (+0.94%) ⬆️
... and 4 more

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 80379fb...16b1af4. Read the comment docs.

@chapulina chapulina removed the beta Targeting beta release of upcoming collection label Mar 24, 2021
@chapulina
Copy link
Contributor Author

Removed from beta, this can come in a minor release.

@chapulina chapulina changed the base branch from main to ign-gazebo5 April 1, 2021 20:36
@chapulina chapulina removed the needs upstream release Blocked by a release of an upstream library label Apr 12, 2021
Signed-off-by: Louise Poubel <[email protected]>
@chapulina
Copy link
Contributor Author

This is ready for review, @scpeters

src/LevelManager.cc Outdated Show resolved Hide resolved
src/SdfEntityCreator.cc Outdated Show resolved Hide resolved
@@ -39,6 +39,7 @@
#include <ignition/physics/RemoveEntities.hh>
#include <ignition/physics/Shape.hh>
#include <ignition/physics/SphereShape.hh>
#include <ignition/physics/World.hh>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for including this header (and other headers) in the header file, if they're really only used in the source file? Does it have something to do with making builds green for macOS and/or Windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a little comment on line 26, these were originally here to satisfy the initial entityCast implementation. There's a chance that this isn't needed anymore since #586, I haven't checked.

test/integration/physics_system.cc Show resolved Hide resolved
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
@iche033 iche033 requested a review from adlarkin June 7, 2021 18:22
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.

One minor comment, but otherwise, LGTM assuming that CI checks come back green 🤞

test/integration/physics_system.cc Show resolved Hide resolved
Signed-off-by: Louise Poubel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
close the gap Features from Gazebo-classic 🏢 edifice Ignition Edifice enhancement New feature or request physics Involves Ignition Physics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants