Add force dependent slip #1505
Add force dependent slip #1505
Conversation
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
The failing test expectation was that the bodies were at rest. Instead, the velocities of the bodies fluctuated unpredictably with a large enough velocity that made the test flaky. I believe the root cause is the use of a plane shape with the FCL collision detector. Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
|
The fix was to replace the plane shape for the ground skeleton with a box shape. The velocities seem much more reasonable |
|
we are hoping that these changes could be included in 6.10.0, since we have been using them in our fork of dart for inn-physics and osrf/subt |
…tructor overload Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Codecov Report
@@ Coverage Diff @@
## master #1505 +/- ##
==========================================
+ Coverage 58.17% 58.28% +0.11%
==========================================
Files 414 414
Lines 30008 30077 +69
==========================================
+ Hits 17456 17530 +74
+ Misses 12552 12547 -5
|
| } | ||
| }; | ||
|
|
||
| std::map<ContactPair, size_t, ContactPairCompare> contactPairMap; |
There was a problem hiding this comment.
Consider using std::unordered_map as the order of the pair doesn't matter in the use cases
There was a problem hiding this comment.
I opted for using a std::map because I thought a writing a compare function was easier than writing a hash function. If you think performance would be a problem, I can look into a hash function for ContactPair and using std::unordered_map.
There was a problem hiding this comment.
Using std::unordered_map would increase the performance since the find query will be called per contact constraints for every simulation step. But it's not critical for this PR. Let's save it for a future change.
| } | ||
|
|
||
| double slipCompliance = dynamicAspect->getSlipCompliance(); | ||
| if (slipCompliance < 0) |
There was a problem hiding this comment.
Correct me if I'm wrong. It seems a negative slip also makes sense, unlike friction. For example, a rotating tire could make slip either left or right.
There was a problem hiding this comment.
I understand slip compliance to be similar to a linear damping coefficient (actually the inverse, see gazebo's WheelSlipPlugin documentation), with a positive value corresponding to a dissipative effect. A negative value, on the other hand, would be non-dissipative and non-physical, similar to having a negative coefficient of friction.
There was a problem hiding this comment.
Yeah, I agree with @scpeters. In the rotating tire example, the tire can slip left or right, but that will be determined by the force applied on the tire. The slip velocity will always be in the same direction as the net external force. A negative slip parameter would mean a velocity opposite to the net external force, which seems non-physical.
Quoting from the ODE manual:
FDS is an effect that causes the contacting surfaces to side past each other with a velocity that is proportional to the force that is being applied tangentially to that surface.
Consider a contact point where the coefficient of friction mu is infinite. Normally, if a force f is applied to the two contacting surfaces, to try and get them to slide past each other, they will not move. However, if the FDS coefficient is set to a positive value k then the surfaces will slide past each other, building up to a steady velocity of kf relative to each other.
There was a problem hiding this comment.
Makes sense to me. Thanks for the explanation
| contactConstraint->setPrimarySlipCompliance( | ||
| contactConstraint->getPrimarySlipCompliance() * numContacts); | ||
| contactConstraint->setSecondarySlipCompliance( | ||
| contactConstraint->getSecondarySlipCompliance() * numContacts); |
There was a problem hiding this comment.
Could you let me know why we need to multiply the slip compliance by the number of contacts?
If I understand correctly, the slip effect will be applied per contact anyway when there were multiple contacts for the same object pair. Suppose we have two contacts for the same object pair and multiply the slip compliance by two, then the doubled slip effect will happen at the two contact points.
There was a problem hiding this comment.
A similar change was made in gazebosim/gazebo-classic@402b9d3 (see also bitbucket PR 2965). The analogy between slip and linear damping is useful here as well. Each contact point with slip acts like an extra damper acting in parallel, which increases the total slip acting on the body. This multiplication aims to make the total slip invariant of the number of contact points.
There was a problem hiding this comment.
This description seems very useful. Could we add a note about that something like these lines?
// The slip compliance acts like a damper at each contact point so the total
// damping for each collision is multiplied by the number of contact points
// (numContacts). To eliminate this dependence on numContacts, the inverse
// damping is multiplied by numContacts.
I tried to edit it myself, but it seems the fork doesn't exist anymore. If that's the case then it's okay to merge this as it is and then add the comment in a new PR.
Never mind, it's edited.
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
|
@jslee02 do you think there's a chance to include this in dart? Let me know if it needs more explanation or motivation. |
| } | ||
|
|
||
| double slipCompliance = dynamicAspect->getSlipCompliance(); | ||
| if (slipCompliance < 0) |
There was a problem hiding this comment.
Makes sense to me. Thanks for the explanation
| contactConstraint->setPrimarySlipCompliance( | ||
| contactConstraint->getPrimarySlipCompliance() * numContacts); | ||
| contactConstraint->setSecondarySlipCompliance( | ||
| contactConstraint->getSecondarySlipCompliance() * numContacts); |
There was a problem hiding this comment.
This description seems very useful. Could we add a note about that something like these lines?
// The slip compliance acts like a damper at each contact point so the total
// damping for each collision is multiplied by the number of contact points
// (numContacts). To eliminate this dependence on numContacts, the inverse
// damping is multiplied by numContacts.
I tried to edit it myself, but it seems the fork doesn't exist anymore. If that's the case then it's okay to merge this as it is and then add the comment in a new PR.
Never mind, it's edited.
| } | ||
| }; | ||
|
|
||
| std::map<ContactPair, size_t, ContactPairCompare> contactPairMap; |
There was a problem hiding this comment.
Using std::unordered_map would increase the performance since the find query will be called per contact constraints for every simulation step. But it's not critical for this PR. Let's save it for a future change.
This adds force dependent slip similar to the implementation in ODE. This is a follow up to #1424 and adds slip compliance in the primary and secondary friction directions. I had to add code to count up the number of contacts between two objects since this information is not exposed by
dart::collision::CollisionResult. It might be possible to have the collision detector add this todart::collision::CollisionResult, but I opted for a solution that doesn't need to break ABI.Relevant forum post: https://dartsim.discourse.group/t/would-dart-be-able-to-support-slip/85
/cc @scpeters
Before creating a pull request
clang-formatBefore merging a pull request
CHANGELOG.md