Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mimic constraint feature #431

Closed
wants to merge 30 commits into from
Closed

Conversation

adityapande-1995
Copy link
Contributor

@adityapande-1995 adityapande-1995 commented Sep 30, 2022

🎉 New feature

Summary

This PR adds a SetMimicConstraint(std::string joint_name, double multiplier, double offset ) feature to joints, and is currently implemented when using dartsim.

Test it

Added a bunch of test cases to joint_features.cc, between different joint combinations and offsets, multipliers.

Pendulum test :

image

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • 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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

Signed-off-by: Aditya <aditya050995@gmail.com>
Signed-off-by: Aditya <aditya050995@gmail.com>
Signed-off-by: Aditya <aditya050995@gmail.com>
Signed-off-by: Aditya <aditya050995@gmail.com>
Signed-off-by: Aditya <aditya050995@gmail.com>
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Sep 30, 2022
Signed-off-by: Aditya <aditya050995@gmail.com>
include/gz/physics/Joint.hh Outdated Show resolved Hide resolved
test/common_test/joint_features.cc Outdated Show resolved Hide resolved
test/common_test/joint_features.cc Outdated Show resolved Hide resolved
Signed-off-by: Aditya <aditya050995@gmail.com>
@codecov
Copy link

codecov bot commented Oct 4, 2022

Codecov Report

Merging #431 (df5fd98) into gz-physics6 (871bab7) will increase coverage by 0.06%.
The diff coverage is 50.00%.

@@               Coverage Diff               @@
##           gz-physics6     #431      +/-   ##
===============================================
+ Coverage        76.18%   76.24%   +0.06%     
===============================================
  Files              142      142              
  Lines             7251     7279      +28     
===============================================
+ Hits              5524     5550      +26     
- Misses            1727     1729       +2     
Files Changed Coverage Δ
dartsim/src/JointFeatures.cc 58.95% <41.66%> (-1.39%) ⬇️
include/gz/physics/detail/Joint.hh 77.46% <100.00%> (+1.34%) ⬆️

... and 1 file with indirect coverage changes

Signed-off-by: Aditya <aditya050995@gmail.com>
Signed-off-by: Aditya <aditya050995@gmail.com>
Signed-off-by: Aditya <aditya050995@gmail.com>
@adityapande-1995
Copy link
Contributor Author

adityapande-1995 commented Oct 4, 2022

TODO Checklist

Checklist for tests (parent - child joint) :

Constructing using sdf - will be done in gz-sim now:

  • Add to ConstructSdfJoint
  • Integration tests
  • Visual demo world

Signed-off-by: Aditya <aditya050995@gmail.com>
Signed-off-by: Aditya <aditya050995@gmail.com>
Signed-off-by: Aditya <aditya050995@gmail.com>
Signed-off-by: Aditya <aditya050995@gmail.com>
Signed-off-by: Aditya <aditya050995@gmail.com>
@adityapande-1995
Copy link
Contributor Author

Based on an offline discussion with @azeey , applying to mimic constraint using the sdf tags would be done in the Physics system inside gz-sim, and not in gz-physics.

Signed-off-by: Aditya <aditya050995@gmail.com>
@adityapande-1995 adityapande-1995 marked this pull request as ready for review November 16, 2022 22:03
public: using Scalar = typename PolicyT::Scalar;

public: void SetMimicConstraint(
const std::string _mimicJoint,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's add the axis name string here to match the SDFormat API

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 6a0294b

{
/// \brief The Joint API for setting the mimic joint constraint.
/// \param[in] _mimicJoint
/// name of the joint to be mimicked, i.e. the parent joint.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use Mimic constraint instead of joint and follower / leader terminology

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 0b4f490

@scpeters
Copy link
Member

scpeters commented May 10, 2023

pinging @azeey for an opinion: should we add support for the Mimic constraints to the ConstructSdfModelImpl function in dartsim/src/SDFFeatures.cc? I think it could be added after the loop that constructs all the joints

…straint()

Signed-off-by: Aditya Pande <adityapande@google.com>
Signed-off-by: Aditya Pande <adityapande@google.com>
{
public: using Scalar = typename PolicyT::Scalar;

public: void SetMimicConstraint(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should add a std::size_t _dof argument (similar to the GetBasicJointState APIs) to specify the follower joint axis

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// \param[in] _joint
/// name of the joint to be mimicked, i.e. the leader joint.
/// \param[in] _multiplier
/// The multiplier to be applied to poistion of leader joint.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// The multiplier to be applied to poistion of leader joint.
/// The multiplier to be applied to position of leader joint.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


// Testing with different (multiplier, offset, reference) combinations.
testMimicFcn(1, 0, 0);
testMimicFcn(-1, 0, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

calling testMimicFcn sequentially results in multiple calls to SetMimicConstraint for a given joint, which appears to work with the dartsim implementation in this PR, but it causes issues with the bullet-featherstone implementation in #517 that adds a new constraint for each call to SetMimicConstraint. Probably I need to update #517 to clear any existing mimic constraints when SetMimicConstraint is called. We should document in the Joint.hh header what the expected behavior is for multiple subsequent calls

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated #517 to clear existing mimic constraints. I made some adjustments to the test expectations, but I've resolved the concern in this comment

adityapande-1995 and others added 3 commits July 10, 2023 18:05
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
public: void SetMimicConstraint(
const std::size_t _dof,
const std::string _joint,
const std::string _axis,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use const references for _joint and _axis

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
Copy link
Member

I've retargeted #517 directly at gz-physics6 and redacted the dartsim implementation from that branch (which is built on this one). I suggest that we close this PR but keep the branch for reference, until dartsim/dart#1756 is addressed upstream

@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 31, 2023
@azeey azeey removed the beta Targeting beta release of upcoming collection label Aug 23, 2023
@scpeters
Copy link
Member

closing until dartsim/dart#1756 is addressed

@scpeters scpeters closed this Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants