Allow customization of contact surface properties #267

peci1 · 2021-06-20T21:07:03Z

🎉 New feature

Closes #15, #82

Is needed for gazebosim/gz-sim#869 and osrf/subt#958.

Summary

This PR adds support for registering callbacks that modify properties of all contact joints generated by the dynamics engine. Example implementation for DART is provided.

This feature might be highly useful for DARPA SubT virtual challenge to allow implementation of a proper track drivetrain.

Test it

Testing is best done by running some of the ign-gazebo examples added in PR #TODO .

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

peci1 · 2021-06-20T21:25:57Z

Videos of ign-gazebo implementation (they only work in Chrome, I don't know why).

ign-tracked1.mp4.mp4

ign-tracked2.mp4.mp4

ign-tracked3.mp4.mp4

ign-tracked4.mp4.mp4

include/ignition/physics/detail/ContactJointProperties.hh

azeey · 2021-07-07T15:21:25Z

include/ignition/physics/ContactJointProperties.hh

+  public: template <typename PolicyT, typename FeaturesT>
+  class World : public virtual Feature::World<PolicyT, FeaturesT>
+  {
+    public: using JointPtrType = JointPtr<PolicyT, FeaturesT>;


nit: type alias not used anywhere.

I think JointPtrType hasn't been removed but is unused

include/ignition/physics/ContactJointProperties.hh

dartsim/src/SimulationFeatures.hh

dartsim/src/SimulationFeatures.cc

include/ignition/physics/ContactJointProperties.hh

dartsim/src/SimulationFeatures.cc

peci1 · 2021-08-17T23:13:02Z

I updated this PR to work with the latest changes in DART, and I addressed the two issues found in review.

azeey · 2021-10-06T19:11:32Z

I think we can actually target ign-physics2. I'll go ahead and do that.

Signed-off-by: Martin Pecka <[email protected]>

codecov · 2021-10-06T19:34:36Z

Codecov Report

Merging #267 (6a1c257) into ign-physics2 (f1aee5a) will increase coverage by 0.07%.
The diff coverage is 88.18%.

@@               Coverage Diff                @@
##           ign-physics2     #267      +/-   ##
================================================
+ Coverage         83.16%   83.23%   +0.07%     
================================================
  Files               106      108       +2     
  Lines              4051     4158     +107     
================================================
+ Hits               3369     3461      +92     
- Misses              682      697      +15

Impacted Files	Coverage Δ
dartsim/src/SimulationFeatures.cc	`82.90% <85.00%> (-3.59%)`	⬇️
dartsim/src/SimulationFeatures.hh	`100.00% <100.00%> (ø)`
...clude/ignition/physics/detail/ContactProperties.hh	`100.00% <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 f1aee5a...6a1c257. Read the comment docs.

This allows CI to pass on Focal. Signed-off-by: Addisu Z. Taddese <[email protected]>

…zation. Signed-off-by: Martin Pecka <[email protected]>

peci1 · 2021-10-24T10:38:12Z

I started working on adding the tests, but got stuck for some time because CMake told me DART does not support contact surface customization (even after resetting cache). Please check be31da3 which fixed it for me.

dartsim/src/SimulationFeatures.hh

Signed-off-by: Martin Pecka <[email protected]>

peci1 · 2021-10-25T01:18:55Z

I extended the RetrieveContacts test to also check the functionality of the callback. It does not check it extensively, but the basic functionality is covered. What remains untested is e.g. the case when one collision shape has more contact points on it.

I ran the changed test with both DART_HAS_CONTACT_SURFACE_HEADER set to true and false and both compiled and succeeded.

A few things surprised me, but are not necessarily wrong:

When DART creates the contact joint, but before the forward step is run, the contact joint force is reported to be zero. I'll add a comment about this so that it doesn't surprise more users.
The first frictional direction computed in the contact.sdf world for the spheres is [0, 0, 1] which is weird because it is parallel to the contact normal. I'd expect something in the XY plane. The spheres do not change the direction settings, and SDF defaults to [0, 0, 0] which means (IMO) to compute the direction from the relative motion of the colliding bodies (but I always thought the result has to be perpendicular to the contact normal).
Although the spheres intersect the ground plane (do not just touch it with their apex), each reports only a single contact point in the axis of the sphere. That's weird, I'd expect a circle of points. But it's probably how the particular collision engine works.

Signed-off-by: Martin Pecka <[email protected]>

azeey · 2021-10-25T16:38:43Z

I started working on adding the tests, but got stuck for some time because CMake told me DART does not support contact surface customization (even after resetting cache). Please check be31da3 which fixed it for me.

Yes, I was working on that last week and came to the same fix, I just hadn't pushed. I thought it was working properly before this fix, but I realized I was seeing the cached results.

Signed-off-by: Martin Pecka <[email protected]>

Signed-off-by: Addisu Z. Taddese <[email protected]>

azeey · 2021-10-25T20:58:50Z

The logs from the macOS build showed ContactSurface.hh was not found. 7ca8007 fixes the problem.

include/ignition/physics/ContactJointProperties.hh

scpeters · 2021-10-26T01:24:48Z

dartsim/src/SimulationFeatures.cc

+    if (!warnedSecondaryRollingFrictionCoeff &&
+      pIgn.secondaryRollingFrictionCoeff)
+    {
+      ignwarn << "DART doesn't support secondary rolling friction setting"


one of my goals for the Feature system is that physics engine plugins can express what they do and do not implement, so you don't have to rely on console messages like this one. I discussed this with @azeey, and I think the API based on contact callback functions and single struct data structure make sense for performance reasons, so I wouldn't try to change that. I do, however, think that we could add Features to signal which parameters a physics engine claims to support. I will create an issue for this.

Signed-off-by: Martin Pecka <[email protected]>

azeey

This is great! Thanks for the contribution!

scpeters

my apologies for all these renaming requests, but I'd like to see Joint removed from the Feature and header file name, since while some physics engines treat contacts as joints, not all do

so please rename to include/ignition/physics/ContactProperties.hh and SetContactPropertiesCallbackFeature

scpeters · 2021-10-27T16:36:00Z

include/ignition/physics/ContactJointProperties.hh

+  public: template <typename PolicyT, typename FeaturesT>
+  class World : public virtual Feature::World<PolicyT, FeaturesT>
+  {
+    public: using JointPtrType = JointPtr<PolicyT, FeaturesT>;


I think JointPtrType hasn't been removed but is unused

Signed-off-by: Martin Pecka <[email protected]>

peci1 · 2021-10-28T15:23:34Z

That's okay, it makes sense to not mention the joint if it's not called like that everywhere.

peci1 · 2021-10-28T16:57:55Z

Thanks for getting this merged! Should the followup Gazebo part also target Citadel, or will we keep it targeted at Dome?

azeey · 2021-10-28T17:18:20Z

Yes, targetting Citadel would be great if possible.

peci1 requested a review from mxgrey as a code owner June 20, 2021 21:07

github-actions bot added the 🔮 dome Ignition Dome label Jun 20, 2021

This was referenced Jun 20, 2021

Added support for tracked vehicles gazebosim/gz-sim#869

Merged

Implement proper tracked motion model osrf/subt#958

Merged

peci1 mentioned this pull request Jun 20, 2021

Generalize the way properties of a contact constraints are computed, allow for custom implementations gazebo-forks/dart#22

Merged

nkoenig requested a review from azeey June 24, 2021 20:53

chapulina added needs upstream release Blocked by a release of an upstream library DART DART engine labels Jul 6, 2021

azeey reviewed Jul 7, 2021

View reviewed changes

diegoferigo mentioned this pull request Jul 7, 2021

Analyze performance drop in Physics system robotology-legacy/gym-ignition#319

Open

azeey reviewed Jul 23, 2021

View reviewed changes

dartsim/src/SimulationFeatures.cc Outdated Show resolved Hide resolved

dartsim/src/SimulationFeatures.cc Outdated Show resolved Hide resolved

azeey requested a review from scpeters as a code owner October 6, 2021 19:08

peci1 added 7 commits October 6, 2021 14:20

Added implementation of contact surface velocity.

aebcac1

Signed-off-by: Martin Pecka <[email protected]>

Fixed codecheck issues.

dd7ac99

Signed-off-by: Martin Pecka <[email protected]>

Fixed issues from review.

d43eed2

Signed-off-by: Martin Pecka <[email protected]>

Exchanged CompositeData for a struct to improve performance.

54afd19

Signed-off-by: Martin Pecka <[email protected]>

Codecheck

0bbb408

Signed-off-by: Martin Pecka <[email protected]>

Updated with latest DART changes

031fad8

Signed-off-by: Martin Pecka <[email protected]>

Fixed bugs from review

2789011

Signed-off-by: Martin Pecka <[email protected]>

azeey force-pushed the contact-velocity branch from c36c165 to 2789011 Compare October 6, 2021 19:31

azeey changed the base branch from ign-physics3 to ign-physics2 October 6, 2021 19:32

azeey added 🏰 citadel Ignition Citadel and removed 🔮 dome Ignition Dome labels Oct 6, 2021

azeey mentioned this pull request Oct 6, 2021

[email protected] bump commit osrf/homebrew-simulation#1616

Merged

azeey and others added 2 commits October 11, 2021 14:18

Detect if contact constraint customization is available

65397fb

This allows CI to pass on Focal. Signed-off-by: Addisu Z. Taddese <[email protected]>

Fixed CMake for finding whether DART supports contact surface customi…

be31da3

…zation. Signed-off-by: Martin Pecka <[email protected]>

peci1 commented Oct 24, 2021

View reviewed changes

dartsim/src/SimulationFeatures.hh Outdated Show resolved Hide resolved

Added a test for SetContactJointPropertiesCallback feature.

9025454

Signed-off-by: Martin Pecka <[email protected]>

Documented the ContactSurfaceParams struct.

967e5d3

Signed-off-by: Martin Pecka <[email protected]>

peci1 and others added 2 commits October 25, 2021 19:12

Fix include guard.

5080c40

Signed-off-by: Martin Pecka <[email protected]>

Fix detection of ContactSurface.hh in macOS

7ca8007

Signed-off-by: Addisu Z. Taddese <[email protected]>

scpeters reviewed Oct 25, 2021

View reviewed changes

include/ignition/physics/ContactJointProperties.hh Outdated Show resolved Hide resolved

scpeters reviewed Oct 26, 2021

View reviewed changes

Renamed normalRolling friction to torsional friction.

553edad

Signed-off-by: Martin Pecka <[email protected]>

azeey approved these changes Oct 26, 2021

View reviewed changes

scpeters reviewed Oct 27, 2021

View reviewed changes

peci1 added 2 commits October 28, 2021 17:12

Rename ContactJointProperties to ContactProperties.

854af93

Signed-off-by: Martin Pecka <[email protected]>

Removed JointPtrType.

6a1c257

Signed-off-by: Martin Pecka <[email protected]>

scpeters approved these changes Oct 28, 2021

View reviewed changes

scpeters mentioned this pull request Oct 28, 2021

Expose contact parameters #82

Open

azeey merged commit 1fd83e0 into gazebosim:ign-physics2 Oct 28, 2021

chapulina removed the needs upstream release Blocked by a release of an upstream library label Nov 1, 2021

scpeters mentioned this pull request Nov 9, 2023

bullet-featherstone: implement support for contactSurfaceMotionVelocity #569

Open

azeey mentioned this pull request Mar 11, 2024

Port wheel plowing model from gz-sim gazebosim/gz-sim#2328

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow customization of contact surface properties #267

Allow customization of contact surface properties #267

peci1 commented Jun 20, 2021 •

edited

Loading

peci1 commented Jun 20, 2021

azeey Jul 7, 2021

peci1 Jul 13, 2021

scpeters Oct 27, 2021

peci1 commented Aug 17, 2021

azeey commented Oct 6, 2021

codecov bot commented Oct 6, 2021 •

edited

Loading

peci1 commented Oct 24, 2021

peci1 commented Oct 25, 2021

azeey commented Oct 25, 2021

azeey commented Oct 25, 2021

scpeters Oct 26, 2021

scpeters Oct 26, 2021

azeey left a comment

scpeters left a comment

scpeters Oct 27, 2021

peci1 commented Oct 28, 2021

peci1 commented Oct 28, 2021

azeey commented Oct 28, 2021

Allow customization of contact surface properties #267

Allow customization of contact surface properties #267

Conversation

peci1 commented Jun 20, 2021 • edited Loading

🎉 New feature

Summary

Test it

Checklist

peci1 commented Jun 20, 2021

azeey Jul 7, 2021

Choose a reason for hiding this comment

peci1 Jul 13, 2021

Choose a reason for hiding this comment

scpeters Oct 27, 2021

Choose a reason for hiding this comment

peci1 commented Aug 17, 2021

azeey commented Oct 6, 2021

codecov bot commented Oct 6, 2021 • edited Loading

Codecov Report

peci1 commented Oct 24, 2021

peci1 commented Oct 25, 2021

azeey commented Oct 25, 2021

azeey commented Oct 25, 2021

scpeters Oct 26, 2021

Choose a reason for hiding this comment

scpeters Oct 26, 2021

Choose a reason for hiding this comment

azeey left a comment

Choose a reason for hiding this comment

scpeters left a comment

Choose a reason for hiding this comment

scpeters Oct 27, 2021

Choose a reason for hiding this comment

peci1 commented Oct 28, 2021

peci1 commented Oct 28, 2021

azeey commented Oct 28, 2021

peci1 commented Jun 20, 2021 •

edited

Loading

codecov bot commented Oct 6, 2021 •

edited

Loading