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: osrf/subt#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);

contactMotion.mp4

[diegoferigo]

Great work @peci1! I postponed the same task for too long (see #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 #14 (comment)? Otherwise, I can take care of it if this PR gets accepted.

xref: gazebosim/gz-physics#82, gazebosim/sdformat#31, gazebosim/sdformat#508, gazebosim/gz-physics#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 gazebosim/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 gazebosim/sdformat#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).

ign-tracked1.mp4.mp4

ign-tracked2.mp4.mp4

ign-tracked3.mp4.mp4

ign-tracked4.mp4.mp4

[peci1]

This PR allows gazebosim/gz-physics#267, gazebosim/gz-sim#869 and osrf/subt#958 .

[chapulina]

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

That would trigger a cascade of incompatibility issues across all stable Ignition versions. We already have a lot to handle with the various debs coming from DART's PPA and our fork, which vary according to Ubuntu version. I strongly recommend looking into compatibility-retaining solutions.

[peci1]

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

[chapulina]

Thanks for addressing the ABI issue! 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.

Removing my request for changes.

ABI addressed

[azeey]

Thanks for the great contribution. This looks pretty good. I have a couple of questions regarding the design of ContactSurfaceHandler and the ContactSurfaceParams struct.

[azeey]

If we're going through all this trouble to keep ABI compatibility, it would be nice to use the PIMPL idiom to make this extensible. Is that feasible?

[peci1]

Do you mean hiding the whole struct's contents in a PIMPL and adding public non-virtual getters/setters to the struct? I'm not sure what could be the performance impact of creating and destroying the unique pointers, as this structure is pretty short-lived and there can theoretically be thousands of them created every simulation step.

[azeey]

Yes, that is what I meant. I share your concern on the performance impact. I know there is interest in adding more parameters (#22 (comment)), but doing so would break ABI unless (a) we add those parameters now, (b) we provide a means for adding them later on without braking ABI

What about a hybrid approach where we add a pointer to extra parameters that is only allocated on demand. For now, the current list of parameters will be directly available in the struct, but new parameters will be added in a new struct hidden to end users and only made accessible via getters/setters.

[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.

[azeey]

Yes, that is what I meant. I share your concern on the performance impact. I know there is interest in adding more parameters (#22 (comment)), but doing so would break ABI unless (a) we add those parameters now, (b) we provide a means for adding them later on without braking ABI

What about a hybrid approach where we add a pointer to extra parameters that is only allocated on demand. For now, the current list of parameters will be directly available in the struct, but new parameters will be added in a new struct hidden to end users and only made accessible via getters/setters.

[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.

[azeey]

Thanks for the great contribution and for iterating through the review process.

[azeey]

nit: The search for the handler, i.e. gContactSurfaceHandlers[this] could be moved outside of the loop.

[azeey]

Will address this in a followup PR.