Generalize the way properties of a contact constraints are computed, allow for custom implementations

[peci1]

This PR adds the possibility to adjust properties of contact constraints (joints) before they are added to the simulation loop.

It resolves a TODO from ContactConstraint.cpp that was reading:

TODO(JS): Consider providing various ways of the combined friction or allowing to override this method by a custom method

The contact surface structure was inspired by the existing contact constraint class members and by http://ode.org/wiki/index.php?title=Manual#Contact .

This PR also adds support for specifying friction-unrelated contact velocity (dContactMotion1 etc. in ODE words). This is needed for simulation of tracks or conveyor belts. It basically specifies that the velocity LCP tries to achieve should not be zero-up-to-friction-coefficients, but the specified velocity (very similar to how bouncing works). This velocity target can be set using the custom contact surface handler this PR adds.

Adding the custom contact surface handler also allowed to separate the odd-placed force-dependent slip settings from ConstraintSolver to the handler class, which looks to me like a desired separation of concerns.

This is the first step towards adding proper tracked vehicle support in SubT simulator, as requested here: https://github.com/osrf/subt/issues/365 . I verified viability of the proposed API by hacking it into ign-physics and using a custom handler from there. In the upcoming days, I'll try to come with a proper integration into ign-physics.

I did not take much care about keeping ABI compatibility. API compatibility should remain untouched regarding public members of classes. There are some new classes and I moved a few static protected members of ContactConstraint to DefaultContactSurfaceHandler. I also moved a few .cpp-only constants from ContactConstraint.cpp to ContactSurface.hpp.

The example API usage in my quick hack into ign-physics is like this:

class tmp : public dart::constraint::ContactSurfaceHandler
{
public:

  dart::constraint::ContactSurfaceParams createParams(const dart::collision::Contact& contact, const size_t numContactsOnCollisionObject) const override
  {
    auto params = this->mParent->createParams(contact);
    params.mFirstFrictionalDirection = Eigen::Vector3d::UnitY();
    params.mContactSurfaceMotionVelocity = Eigen::Vector3d::UnitY();
    return params;
  }
};

auto tmpPtr = std::make_shared<tmp>();

...

world->getConstraintSolver()->setContactSurfaceHandler(tmpPtr);

https://user-images.githubusercontent.com/182533/113764677-92ef4e80-971b-11eb-98e1-2c7fe3f3a7c6.mp4

[diegoferigo]

Great work @peci1! I postponed the same task for too long (see https://github.com/ignition-forks/dart/issues/14), thanks for providing a way to expose these settings. If it's not too demanding, would it be possible exposing also the CFM parameter as discussed in https://github.com/ignition-forks/dart/issues/14#issuecomment-720633043? Otherwise, I can take care of it if this PR gets accepted.

xref: https://github.com/ignitionrobotics/ign-physics/issues/82, https://github.com/osrf/sdformat/issues/31, https://github.com/osrf/sdformat/issues/508, https://github.com/ignitionrobotics/ign-physics/pull/139.

[peci1]

Hi @diegoferigo. Quickly skimming through the code, it seemed to me that support for CFM setting would need more ABI breaking changes. I would like to keep this PR as "slim" as possible regarding ABI breaks (although there already are some). So I suggest you create another PR based on this one and you'll see whether the implementation provided by this PR is sufficient for the CFM case too. If not, we can make some adjustments so that it is still useful for you.

Just thinking about where to get the CFM values. One place is definitely the ODE SDF tag, which is kind of a bad practice but used for many other physics properties, so until osrf/sdformat#31 is resolved, it has to be done like that. Another way how CFM could be injected is via custom callbacks as is intended with the contact surface motion. You can choose whether you'd like to support this way of setting it, too, or not.

[diegoferigo]

Hi @diegoferigo. Quickly skimming through the code, it seemed to me that support for CFM setting would need more ABI breaking changes. I would like to keep this PR as "slim" as possible regarding ABI breaks (although there already are some). So I suggest you create another PR based on this one and you'll see whether the implementation provided by this PR is sufficient for the CFM case too. If not, we can make some adjustments so that it is still useful for you.

Totally fine with me. Exposing the CFM is one of my desiderata on the long term, and this can be done in a new PR. Since you started this task, I'm definitely willing to integrate with it. Once ABI are broken, let's try to merge as many changes as possible.

As an independent remark, I noticed a new 6.10 upstream release. I'm not sure how it plays with the custom features implemented here and optionally enabled in ign-physics. Increasing the versioning of this fork might be required, and it could address ABI problems. The entire picture is not crystal clear to my eyes, maybe the developers can comment about it.

Just thinking about where to get the CFM values. One place is definitely the ODE SDF tag, which is kind of a bad practice but used for many other physics properties, so until osrf/sdformat#31 is resolved, it has to be done like that. Another way how CFM could be injected is via custom callbacks as is intended with the contact surface motion. You can choose whether you'd like to support this way of setting it, too, or not.

Using ODE tags was my original idea, waiting https://github.com/osrf/sdformat/issues/31. Good to know that it could be also implemented with custom callback, I'll keep this in mind.

[peci1]

@mjcarroll @scpeters if proper simulation of tracked vehicles should become a part of SubT simulator on time to be used in the challenge, I think we need to start discussing this PR as early as possible (it makes some ABI/API breaks and I'm not sure if that's okay, or if I should seek for some compatibility-retaining solutions). I'm sorry for putting pressure on you, but time is running.

[peci1]

Friendly ping to maintainers.

[peci1]

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

https://user-images.githubusercontent.com/182533/122688768-75ac2580-d21e-11eb-9947-67844b548381.mp4

https://user-images.githubusercontent.com/182533/122688763-71800800-d21e-11eb-80b0-0fc894cdd7de.mp4

https://user-images.githubusercontent.com/182533/122688779-8bb9e600-d21e-11eb-8e36-b23a035fb8b5.mp4

https://user-images.githubusercontent.com/182533/122688788-9c6a5c00-d21e-11eb-9e18-60d625bc8761.mp4

[peci1]

This PR allows https://github.com/ignitionrobotics/ign-physics/pull/267, https://github.com/ignitionrobotics/ign-gazebo/pull/869 and https://github.com/osrf/subt/pull/958 .

[peci1]

I resolved (hopefully) all ABI breaking parts of this PR. Could you please re-review it?

[peci1]

It would be nice to add a comment to the hack so people know why it's there and can come back to fix it in a future (major?) version.

Comments added.

[peci1]

@azeey I addressed all your comments. Now I'm leaving for 5-6 days, so if you want some minor changes, feel free to do them yourself in order to speed up acceptance of this PR.