Add secondary friction coefficient parameter #1424
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #1424 +/- ##
==========================================
- Coverage 57.98% 57.98% -0.01%
==========================================
Files 412 412
Lines 29794 29832 +38
==========================================
+ Hits 17277 17299 +22
- Misses 12517 12533 +16
|
scpeters
changed the title
|
I've removed the WIP from the title since I think this is now ready for review. |
5 tasks
|
I have a proposed fix for the osx CI in #1426 |
mxgrey
requested changes
Nov 17, 2019
| const double frictionCoeff = 1.0, const double restitutionCoeff = 0.0); | ||
| const double primaryFrictionCoeff = 1.0, | ||
| const double restitutionCoeff = 0.0, | ||
| const double secondaryFrictionCoeff = 1.0); |
There was a problem hiding this comment.
Instead of adding another function argument with a default of 1.0, I recommend overloading like this:
/// The frictionCoeff argument will be used for both primary and secondary friction
DynamicsAspectProperties(
const double frictionCoeff = 1.0,
const double restitutionCoeff = 0.0);
DynamicsAspectProperties(
const double primaryFrictionCoeff,
const double secondaryFrictionCoeff,
const double restitutionCoeff);
I think this should be more intuitive and also should also maintain backwards compatibility.
| @@ -114,14 +114,21 @@ ContactConstraint::ContactConstraint( | |||
| //---------------------------------------------- | |||
| // TODO(JS): Assume the frictional coefficient can be changed during | |||
| // simulation steps. | |||
| // Update mFrictionalCoff | |||
| // Update mFrictionCoeff | |||
| const double frictionCoeffA = computeFrictionCoefficient(shapeNodeA); | |||
There was a problem hiding this comment.
Suggested change
| const double frictionCoeffA = computeFrictionCoefficient(shapeNodeA); | |
| const double primaryFrictionCoeffA = computeFrictionCoefficient(shapeNodeA); |
Nitpick: I know it'll make for a bigger diff, but I think I'd recommend adding primary to these variable names for better symmetry.
mxgrey
previously approved these changes
Nov 18, 2019
Rename mFrictionCoeff to mPrimaryFrictionCoeff and replace auto-generated (set|get)FrictionCoeff with custom functions that set both coefficients and return the average of the coefficients.
scpeters
force-pushed
the
secondary_friction
branch
from
f001b59 to
2a0934f
Compare
jslee02
requested changes
Nov 18, 2019
| /// Coefficient of friction | ||
| double mFrictionCoeff; | ||
| /// Primary coefficient of friction | ||
| double mPrimaryFrictionCoeff; |
There was a problem hiding this comment.
This breaks backward compatibility since we're changing the name of the public member variable. Could we keep the variable name and add custom functions (i.e., set/getPrimaryFrictionCoeff) to DynamicAspect instead of DART_COMMON_SET_GET_ASPECT_PROPERTY(double, PrimaryFrictionCoeff)?
Co-Authored-By: Jeongseok Lee <[email protected]>
jslee02
previously approved these changes
Nov 20, 2019
jslee02
approved these changes
Nov 20, 2019
mxgrey
approved these changes
Nov 21, 2019
5 tasks
mdecourse
added a commit
to mdecourse/dart
that referenced
this pull request
Nov 22, 2019
Add secondary friction coefficient parameter (dartsim#1424)
5 tasks
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently the ContactConstraint uses a friction pyramid with 2 directions, and it uses the same friction coefficient in each direction. We have found it useful in gazebo to be able to specify different values in each direction (anisotropic wheel friction for example). As such, I've added
mSecondaryFrictionCoeffto the ShapeNode DynamicsAspect and updated the ContactConstraint to use this secondary coefficient. @azeey and I have additional code in his fork that adds parameters for specifying the friction directions and slip parameters, which will be addressed in subsequent pull requests.[edited] To avoid a behavior change, the
mFrictionCoeffmember variable in the ShapeFrame DynamicsAspect is renamed tomPrimaryFrictionCoeffalong with its auto-generated get/set functions. The existing auto-generatedgetFrictionCoeffandsetFrictionCoeffare explicitly implemented such thatsetFrictionCoeffsets both friction parameters andgetFrictionCoeffreturns the average (and also returnsdoubleinstead ofconst double &). I've added test coverage for these new functions. We have test cases in gazebo demonstrating anisotropic friction in case you'd like to add one here in the future.Before creating a pull request
clang-formatBefore merging a pull request
CHANGELOG.md