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

Implement TaskSpaceInverseDynamics interface #279

Merged
merged 15 commits into from
Apr 19, 2021
Merged

Conversation

GiulioRomualdi
Copy link
Member

This follows from #251 (comment)

Before merging we should change ContactWrench struct since it's not containing the value of the contact wrench.

In your opinion does it make sense to store it in an Eigen::Vector6d?

Copy link
Member

@S-Dafarra S-Dafarra left a comment

Choose a reason for hiding this comment

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

Just a typo

};

/**
* TaskSpaceInverseDynamics implements the interface for the task sapce inverse
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
* TaskSpaceInverseDynamics implements the interface for the task sapce inverse
* TaskSpaceInverseDynamics implements the interface for the task space inverse

@S-Dafarra
Copy link
Member

In your opinion does it make sense to store it in an Eigen::Vector6d?

The only problem would be the possibility to get the linear and angular part. Maybe extending Eigen::Vector6d 🤔?

@prashanthr05
Copy link
Collaborator

prashanthr05 commented Apr 16, 2021

In your opinion does it make sense to store it in an Eigen::Vector6d?

The only problem would be the possibility to get the linear and angular part. Maybe extending Eigen::Vector6d ?

x.head<3>(), x.tail<3>() should be enough right? maybe not descriptive enough.

@traversaro
Copy link
Collaborator

traversaro commented Apr 16, 2021

In your opinion does it make sense to store it in an Eigen::Vector6d?

The only problem would be the possibility to get the linear and angular part. Maybe extending Eigen::Vector6d ?

x.head<3>(), x.tail<3>() should be enough right? maybe not descriptive enough.

Yes, I think those would risk to be quite error prone, even not extending Eigen::Vector6d it could make sense to wrap x.tail<3>() in functions that take Eigen::Vector6d in input.

@GiulioRomualdi
Copy link
Member Author

manif::se3d::tanget? I don't really like it

@prashanthr05
Copy link
Collaborator

manif::se3d::tanget? I don't really like it

technically a co-tangent.

@GiulioRomualdi
Copy link
Member Author

GiulioRomualdi commented Apr 16, 2021

technically a co-tangent.

Yes you're right but manif does not define cotangent vector. 😢

@GiulioRomualdi
Copy link
Member Author

Otherwise iDynTree::Wrench

@S-Dafarra
Copy link
Member

Otherwise iDynTree::Wrench

I think we are too centered around Eigen objects. A wrench could part of a set of optimization variables, so it is better to be consistent across the library.

@S-Dafarra
Copy link
Member

I think that for the time being we can stay simple, and use, as @traversaro suggested, some helper functions to "view" the linear and angular part given a 6D vector.

@S-Dafarra
Copy link
Member

S-Dafarra commented Apr 16, 2021

The take home message is: the assumption "the first 3 elements are the linear part" is error-prone, and we want to remove that assumption. Internally we can still use a 6D vector though.

@GiulioRomualdi
Copy link
Member Author

GiulioRomualdi commented Apr 16, 2021

A possible solution is to define (i don't know where) this class See: http://eigen.tuxfamily.org/dox-3.2/TopicCustomizingEigen.html

class Wrench : public  Eigen::Vector6d
{
public:
    Wrench(void):Eigen::Vector6d() {}
    typedef Eigen::Vector6d Base;
    // This constructor allows you to construct Wrench from Eigen expressions
    template<typename OtherDerived>
    Wrench(const Eigen::MatrixBase<OtherDerived>& other)
        : Eigen::Vector6d(other)
    { }
    // This method allows you to assign Eigen expressions to MyVectorType
    template<typename OtherDerived>
    Wrench & operator= (const Eigen::MatrixBase <OtherDerived>& other)
    {
        this->Base::operator=(other);
        return *this;
    }


Eigen::Block<Wrench, 3, 1>
force()
{
  return this->Base::head<3>();
}
 
Eigen::Block<Wrench, 3, 1> torque()
{
  return this->Base::tail<3>();
}
 
};

@S-Dafarra
Copy link
Member

@GiulioRomualdi
Copy link
Member Author

Hi all, I implement the Wrench class in the Math component. I also defined two operators that can be used to rotate/transform a wrench using manif objects.

I also introduced in ContactWrench in the Contacts component.

@isorrentino, @S-Dafarra @prashanthr05 and/or @traversaro could you please review the PR again?

@GiulioRomualdi
Copy link
Member Author

GiulioRomualdi commented Apr 18, 2021

Before merging I should

  • edit the changelog
  • edit the readme

Furthermore this PR add manif as dependencies of Math. I don’t know if this is a problem.
notice that Wrench is a template class and also the operators that I defined are template. So in theory we need manif only if the user includes Wrench.h and not to compile the math component.

@traversaro and @S-Dafarra

@traversaro
Copy link
Collaborator

Furthermore this PR add manif as dependencies of Math. I don’t know if this is a problem.

I don't think this is a problem, manif is already an almost compulsory dependency.

@GiulioRomualdi
Copy link
Member Author

I don't think this is a problem, manif is already an almost compulsory dependency.

There are actually two possible solutions:

  1. Keep the operator*() methods in the header and add define guards to avoid installing it if manif is not used.
  2. Move the operator*() methods in a separate header and install it only if manif is used. This is similar to what is done in `conversions

Copy link
Collaborator

@prashanthr05 prashanthr05 left a comment

Choose a reason for hiding this comment

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

Minor comment.

src/Math/tests/WrenchTest.cpp Show resolved Hide resolved
@S-Dafarra
Copy link
Member

S-Dafarra commented Apr 19, 2021

I don't think this is a problem, manif is already an almost compulsory dependency.

There are actually two possible solutions:

  1. Keep the operator*() methods in the header and add define guards to avoid installing it if manif is not used.
  2. Move the operator*() methods in a separate header and install it only if manif is used. This is similar to what is done in `conversions

To be honest I don't like too much APIs that may change according to a build configuration. I think that at this point, either we add manif as a required dependency, or we don't define the operator.

@S-Dafarra
Copy link
Member

Yet another possibility could be to create another library in Math that is compiled only if manif is present, and put Wrench in it.

@GiulioRomualdi
Copy link
Member Author

After a discussion with @S-Dafarra, we decided to add manif as dependencies for math. So I updated the cmake 374a5c7, the readme e9b713e and the changelog eba86c7

@GiulioRomualdi GiulioRomualdi merged commit 2a69fed into master Apr 19, 2021
@GiulioRomualdi GiulioRomualdi deleted the TSIDInterface branch April 19, 2021 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants