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 contact models library #7

Merged

Conversation

GiulioRomualdi
Copy link
Member

@GiulioRomualdi GiulioRomualdi commented Jan 9, 2020

This PR introduces the contact models library.

In details:

  1. An abstract interface named ContactModel has been implemented. This class will be useful to implement different contact models
  2. The ContinuousContactModel implements the a continuous contact model. contactModel
  3. C++17 is now required. (i.e. the library widly uses std::any)

@GiulioRomualdi GiulioRomualdi self-assigned this Jan 9, 2020
@GiulioRomualdi GiulioRomualdi marked this pull request as ready for review January 12, 2020 22:00
Copy link
Collaborator

@traversaro traversaro left a comment

Choose a reason for hiding this comment

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

Minor comments.

CMakeLists.txt Outdated Show resolved Hide resolved
*/
class ContinuousContactModel final : public ContactModel
{
iDynTree::Transform m_frameTransform; /**< Homogeneous transformation of the frame placed in the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Of the frame "place in the center of the contact surface", w.r.t. to which other frame?

src/ContactModels/tests/ContinousContactModelTest.cpp Outdated Show resolved Hide resolved
@traversaro
Copy link
Collaborator

Ah, a minor comment: it may be possible the unordered_set<string, any> structure for methods that are supposed to be called at each control loop may may be not efficient in term of computation time, so if you ever want to optimize the code this could be one of the things to check.

@S-Dafarra
Copy link
Member

S-Dafarra commented Jan 13, 2020

it may be possible the unordered_set<string, any> structure for methods that are supposed to be called at each control loop may may be not efficient in term of computation time

Is it? Search, insertion, and removal have average constant-time complexity (https://en.cppreference.com/w/cpp/container/unordered_map).

As far as I understood unordered_map is preferable to map since the latter orders the elements and performs a binary search, while the former stores the element in a hash table.

@traversaro
Copy link
Collaborator

it may be possible the unordered_set<string, any> structure for methods that are supposed to be called at each control loop may may be not efficient in term of computation time

Is it? Search, insertion, and removal have average constant-time complexity (https://en.cppreference.com/w/cpp/container/unordered_set)

Constant is not negligible, especially if any of those operation uses dynamic memory allocation. To use an hyperbole, If the constant time complexity is 100 ms, it would be indeed constant, but also unusable in practice. However I think is perfectly fine to ignore this for now and come back to it to see if it is actually relevant if we need to speed up the code.

@S-Dafarra
Copy link
Member

S-Dafarra commented Jan 13, 2020

Constant is not negligible, especially if any of those operation uses dynamic memory allocation.

As long as you don't add or remove elements, I don't think it does. There could be a difference when you need to scan all the elements given that they may be stored in non-contiguous portions of memory. In that case, vectors are always the best pick.

Once I had to find all the equal strings in a very large set of strings. A simple iterative algorithm was taking minutes. I then decided to use the strings as keys in an unordered map<string, vector<size_t>> and saved the corresponding index in the vector. At the end, it was enough to count the elements in each vector to know the number of equal strings. This was taking milliseconds instead.

* @return true/false in case of success/failure
*/
template <class T>
bool getVariable(const std::unordered_map<std::string, std::any>& variables,
Copy link
Member

Choose a reason for hiding this comment

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

I would detach VariablesHandler from the OptimalControlUtilities and use it also here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would do it later with another PR

// the parameters has been update the previous quantities has to be evaluated again
m_isContactWrenchComputed = false;
m_isControlMatrixComputed = false;
m_isAutonomusDynamicsComputed = false;
Copy link
Member

Choose a reason for hiding this comment

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

To be honest I don't like too much that child classes have to set a parameter that is used in the implementation of the base class. IMHO, the setting of these parameters to false should be handled by the base class or through a method in the base class, like invalidateBuffers() or similar.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree!

Copy link
Member Author

Choose a reason for hiding this comment

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

I implemented the solution you suggested in #33 (comment)

Code: f4092bf

// compute the torque
auto skewRe1 = iDynTree::skew(rotation.col(0));
auto skewRe2 = iDynTree::skew(rotation.col(1));
torque = std::abs(rotation(2, 2)) * area / 12 * (m_length * m_length
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would be better to have a link to reference how this formula is obtained.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can update the doxygen documentation adding the explanation of the model

src/ContactModels/tests/CMakeLists.txt Outdated Show resolved Hide resolved
* as a continuum of springs and dampers.
* The contact surface is supposed to be <b>rectangular</b> and the frame placed on the contact
* surfaced is centered in the middle of the surface, with the \a z-axis pointing upwards and the \a
* x-axis pointing forward (parallel to one edge of the rectangle).
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you can define here what do you mean by length and width. In addition, I would mention that this model assumes infinite friction.

@GiulioRomualdi
Copy link
Member Author

I updated the code accordingly to #28

I notice that we are implementing the initialize() method for each class and in general the class cannot be used till the initialize() function is called.
Probably we can generalize the concept by designing a class block and all the other classes will inherit from that class.

This is an example of the class

class Block
{
protected:
    enum class State
    {
         NotInitialized, Initialized, ...
    };
   State m_state;
public:
   virtual bool initialize(std::weak_ptr<IParameterHandler> handler) = 0;
};

We can discuss this on another issue

@S-Dafarra you can review again the code

@DanielePucci
Copy link
Member

CC @gabrielenava

@GiulioRomualdi GiulioRomualdi force-pushed the feature/implement_contact_models branch from 50227ea to 7043c20 Compare May 28, 2020 14:23
@GiulioRomualdi
Copy link
Member Author

I updated the code to be compliant with the latest version of master. And I think I also handled all @S-Dafarra comments.

Let me know if we can merge this PR since it is blocking #46 😄

@S-Dafarra
Copy link
Member

Go for me!

@GiulioRomualdi GiulioRomualdi merged commit ec1c7b1 into ami-iit:master May 28, 2020
@GiulioRomualdi GiulioRomualdi deleted the feature/implement_contact_models branch May 28, 2020 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants