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

C++ refactoring of Contact #204

Merged
merged 6 commits into from
Feb 23, 2021
Merged

C++ refactoring of Contact #204

merged 6 commits into from
Feb 23, 2021

Conversation

GiulioRomualdi
Copy link
Member

@GiulioRomualdi GiulioRomualdi commented Feb 23, 2021

This is the C++ refactoring of Contact class. I'm not really sure about this solution for the following reasons:

  1. Right now we are exposing Contacts and Contact. The former is in https://github.com/dic-iit/bipedal-locomotion-framework/tree/master/src/Contacts while the latter is in https://github.com/dic-iit/bipedal-locomotion-framework/tree/master/src/Planners. This is confusing 😭
    image

  2. Shall we move also the ContactLists etc in Contacts?

  3. I do not know how to handle the transition in the bindings for the following reasons

    1. the old Contact is just a typedef of PlannedContact,
    2. The new Contact class is a base class that we should wrap in python so this will destroy the retro compatibility
    3. I do not know how to handle the deprecation with pybind

cc @prashanthr05 @traversaro @S-Dafarra and @diegoferigo

This will close #162

@GiulioRomualdi GiulioRomualdi self-assigned this Feb 23, 2021
@traversaro
Copy link
Collaborator

The new Contact class is a base class that we should wrap in python so this will destroy the retro compatibility

I think it is safe to do so at this point? We have a big warning in the README exactly for that.

@GiulioRomualdi
Copy link
Member Author

GiulioRomualdi commented Feb 23, 2021

If @paolo-viceconte and @diegoferigo agree we can proceed in this way

  1. in all your python code you should just substitute Contact with PlannedContact
  2. If you want to run code that that requires the old implementation you can use the release 0.1.0 https://github.com/dic-iit/bipedal-locomotion-framework/releases/tag/v0.1.0

This will unblock @prashanthr05 from porting the estimator in python

@diegoferigo
Copy link
Member

Yes you can proceed in this way. No problem if some code here and there breaks, it seems a trivial change.

@paolo-viceconte
Copy link
Contributor

I agree as well.

@prashanthr05
Copy link
Collaborator

Related issue: #162

@prashanthr05
Copy link
Collaborator

2.Shall we move also the ContactLists etc in Contacts?

I suppose we could move the data structures

  • ContactList,
  • ContactPhase, and
  • ContactPhaseList

to Contacts. If we break, let's break it altogether in an uniform way. But, should we also rename them?

@GiulioRomualdi
Copy link
Member Author

Yes you can proceed in this way. No problem if some code here and there breaks, it seems a trivial change.

This is the only modification required in python
image

@GiulioRomualdi
Copy link
Member Author

I also created the bindings for the EstimatedContacts so @prashanthr05 can start using it

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.

Looks good! Thanks @GiulioRomualdi !!

@GiulioRomualdi GiulioRomualdi merged commit 71455af into master Feb 23, 2021
@GiulioRomualdi GiulioRomualdi deleted the refactor/contacts branch February 23, 2021 20:15
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.

Migrate Contact library outside Planners folder
5 participants