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

Python bidings for the measured force contact #251

Open
wants to merge 8 commits into
base: devel
Choose a base branch
from

Conversation

aleolive38
Copy link

Hi,

I noticed there was no python bindings for the measured force contacts, so I implemented them following the recommendation made for a similar issue.
I added the measure6Dwrench contact type to the bindings, the addMeasuredForce binding in the formulation and a test script that checks the creation of this contact, the setting of its value and its addition to the inverse dynamics formulation.
Let me know if it is ok or if you have any suggestions,

Have a nice day

Copy link
Contributor

@nim65s nim65s left a comment

Choose a reason for hiding this comment

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

Thanks for this !

Could you add an entry in tests/python/CMakeLists.txt for your test ?

@@ -19,6 +19,7 @@ set(${PYWRAP}_SOURCES
contacts/contact-6d.cpp
contacts/contact-point.cpp
contacts/contact-two-frame-positions.cpp
contacts/measured-6Dwrench
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing file extension
I would rename the file following the naming convention of the project.

@@ -0,0 +1,114 @@
//
// Copyright (c) 2022 CNRS INRIA LORIA
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure about the year?

@@ -273,6 +276,10 @@ struct InvDynPythonVisitor
const solvers::HQPOutput& sol) {
return self.getContactForces(name, sol);
}
static bool addMeasuredForce(T& self,
contacts::Measured6Dwrench measured_force) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing reference.


inline void exposeContact() {
exposeContact6d();
exposeContactPoint();
exposeContactTwoFramePositions();
exposeMeasured6Dwrench();
Copy link
Collaborator

Choose a reason for hiding this comment

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

For consistency with the already existing "Contact6d" I would prefer using "Measured6dWrench", without capitalizing the "d" after 6.

@@ -21,19 +21,22 @@
#include "tsid/bindings/python/contacts/contact-6d.hpp"
#include "tsid/bindings/python/contacts/contact-point.hpp"
#include "tsid/bindings/python/contacts/contact-two-frame-positions.hpp"
#include "tsid/bindings/python/contacts/measured-6Dwrench.hpp"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Even here, for consistency, I would rename this file "measured-6d-wrench.hpp"

@aleolive38
Copy link
Author

Hi,
Thanks for the reviews!
For the naming convention, I don't mind changing them though I just used the existing name from the existing cpp files.
Do you want them to be changed only in the bindings you pointed out or in the rest of the code too? Then I guess also the Measured3Dforce should be changed to Measured3dForce and the corresponding files to measured-3d-force (I did not implement bindings for this one in this pull request though)

@andreadelprete
Copy link
Collaborator

You are right that we already have inconsistencies. However, we should at least avoid adding more inconsistencies to the code. If you also wanna fix the naming inconsistencies of the 3d force that'd be an added value for this PR.

@aleolive38
Copy link
Author

For now I fixed the inconsistent naming only on the bindings side

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.

4 participants