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 iDynTree base type type_caster for pybind11 bindings #931

Closed
wants to merge 29 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
8a40642
Implement raw buffer constructor for the Position class
GiulioRomualdi Nov 6, 2021
f2f6237
Implement pybind11 type_caster for iDynTree core types
GiulioRomualdi Nov 6, 2021
c46a4bf
Use pybidn11 typecaster in the already existing bindings
GiulioRomualdi Nov 6, 2021
030066e
Make rotation and position property readable and writable in the pybi…
GiulioRomualdi Nov 6, 2021
c955175
Update the pybind11 tests
GiulioRomualdi Nov 8, 2021
a6c3058
Update the CHANGELOG
GiulioRomualdi Nov 8, 2021
899643b
Update CHANGELOG.md
GiulioRomualdi Nov 23, 2021
ecc64ad
Improve the idyntree_type_caster structs
GiulioRomualdi Nov 25, 2021
f60121c
Revert "Use pybidn11 typecaster in the already existing bindings"
GiulioRomualdi Nov 25, 2021
97faee1
Use pybind11 typecaster in the already existing bindings
GiulioRomualdi Nov 25, 2021
4a5509a
Revert "Implement raw buffer constructor for the Position class"
GiulioRomualdi Nov 25, 2021
2f7fa5b
Fix type 'struct type_caster' violates one definition rule [-Wodr] bu…
GiulioRomualdi Nov 25, 2021
7037767
Rename idyntree_type_caster.h into idyntree_vector_casters.h
GiulioRomualdi Nov 25, 2021
97236d6
Bugfix th the idyntree_vector_casters load functions
GiulioRomualdi Nov 25, 2021
b3df484
Use a single namespace conversion in the idyntree_vector_casters.h
GiulioRomualdi Nov 25, 2021
6a0db62
Create idyntree-pybind11 library
GiulioRomualdi Nov 25, 2021
0b3b349
Define the iDynTreeBindings project only if IDYNTREE_BINDINGS_BUILD_S…
GiulioRomualdi Nov 25, 2021
7552d12
Fix style in VectorCasters.h file
GiulioRomualdi Nov 25, 2021
c746633
Fix style in bindings/pybind11/idyntree_core.cpp file
GiulioRomualdi Nov 25, 2021
376690f
Revert "Update the pybind11 tests"
GiulioRomualdi Nov 25, 2021
a7522fe
Update of the python tests
GiulioRomualdi Nov 25, 2021
f04e844
Implement baseVectorDefinition and baseMatrixDefinition in bindings/p…
GiulioRomualdi Nov 25, 2021
c16496f
Bugfix in VectorCasters.h
GiulioRomualdi Nov 25, 2021
609923d
Uniform pybind11 target_link_libraries
GiulioRomualdi Nov 25, 2021
25d5728
Implement the test for pybind11 type_caster
GiulioRomualdi Nov 25, 2021
ec7c8f6
Merge remote-tracking branch 'origin/devel' into pybind11/type_caster
GiulioRomualdi Nov 26, 2021
9258e6b
Fix typo in pybind11/CMakeLists.txt
GiulioRomualdi Nov 26, 2021
58f5fca
Fix style in VectorCasters.h
GiulioRomualdi Nov 28, 2021
10658f3
Fix WORKING_DIRECTORY of pybind11 tests
GiulioRomualdi Nov 28, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Added
- Added the possibility to draw in different portions of the visualizer window and textures at the same time. Allow disabling the drawing on textures (https://github.com/robotology/idyntree/pull/903).
- Implement iDynTree base type `type_caster` for pybind11 bindings. In other word, `VectorDynSize`,`VectorFixSize`, `MatrixDynSize` and `MatrixFixSize` are now automatically converted in `NumPy` objects (https://github.com/robotology/idyntree/pull/931).

### Deprecated
- The `iDynTree::Visualizer::enviroment()` was deprecated. Please use the `iDynTree::Visualizer::environment()` method instead (https://github.com/robotology/idyntree/pull/903).
Expand Down
4 changes: 2 additions & 2 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ if(NOT IDYNTREE_ONLY_DOCS)
list(APPEND _IDYNTREE_EXPORTED_DEPENDENCIES_ONLY_STATIC assimp)
endif()

add_subdirectory(bindings)

include(InstallBasicPackageFiles)
install_basic_package_files(iDynTree VARS_PREFIX ${VARS_PREFIX}
VERSION ${${VARS_PREFIX}_VERSION}
Expand All @@ -78,8 +80,6 @@ if(NOT IDYNTREE_ONLY_DOCS)
PRIVATE_DEPENDENCIES ${_IDYNTREE_EXPORTED_DEPENDENCIES_ONLY_STATIC})

include(AddUninstallTarget)

add_subdirectory(bindings)
endif()

# add a dox target to generate doxygen documentation
Expand Down
2 changes: 1 addition & 1 deletion bindings/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
cmake_minimum_required(VERSION 3.16)
project(iDynTreeBindings)

# Detect if we are doing a standalone build of the bindings, using an external iDynTree
if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
Expand All @@ -9,6 +8,7 @@ else()
endif()

if(IDYNTREE_BINDINGS_BUILD_STANDALONE)
project(iDynTreeBindings)
# Add CMake helpers
list(APPEND CMAKE_MODULE_PATH ${PROJECT_SOURCE_DIR}/../cmake)
find_package(iDynTree REQUIRED)
Expand Down
28 changes: 26 additions & 2 deletions bindings/pybind11/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,3 +1,26 @@
set(libraryname idyntree-pybind11)

add_library(${libraryname} INTERFACE)
target_link_libraries(${libraryname} INTERFACE idyntree-core)

# Specify include directories for both compilation and installation process.
# The $<INSTALL_PREFIX> generator expression is useful to ensure to create
# relocatable configuration files, see https://cmake.org/cmake/help/latest/manual/cmake-packages.7.html
# creating-relocatable-packages
target_include_directories(${libraryname} INTERFACE
"$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>"
"$<INSTALL_INTERFACE:$<INSTALL_PREFIX>/${CMAKE_INSTALL_INCLUDEDIR}>")

# Specify installation targets, typology and destination folders.
install(TARGETS ${libraryname}
EXPORT ${PROJECT_NAME})

install(FILES "${CMAKE_CURRENT_SOURCE_DIR}/include/iDynTree/pybind11/VectorCasters.h"
DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/iDynTree/pybind11")

add_library(iDynTree::${libraryname} ALIAS ${libraryname})
set_property(GLOBAL APPEND PROPERTY ${VARS_PREFIX}_TARGETS ${libraryname})

Comment on lines +1 to +23
Copy link
Collaborator

Choose a reason for hiding this comment

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

tl;dr; Why these changes?

If I understood correctly, you want to create a separate library for the caster? Then

  1. The name is very confusing. It does not explain what the library is for and it is a permutation of the other library name.
  2. INTERFACE library, no sources? I know this is ok, but I think adding the (single) header file and setting it as public header you can skip the install FILES command and make it more clear
  3. Which should probably be 0): Why? Why do you need the header file outside the build directory? Do you have C++ libraries that depend on this?

Copy link
Member

Choose a reason for hiding this comment

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

INTERFACE library, no sources? I know this is ok, but I think adding the (single) header file and setting it as public header you can skip the install FILES command and make it more clear

In this specific case you can't as there destination path is iDynTree/pybind11, while the PUBLIC header stuff only support one folder as destination path.

Copy link
Member Author

@GiulioRomualdi GiulioRomualdi Dec 1, 2021

Choose a reason for hiding this comment

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

The name is very confusing. It does not explain what the library is for and it is a permutation of the other library name.

We can change it, what do you suggest?

INTERFACE library, no sources? I know this is ok, but I think adding the (single) header file and setting it as public header you can skip the install FILES command and make it more clear

I will need it to import it in a different project that uses idyntree types in the interface. Having an exported interface library simplifies importing it. It is exactly the same as #include <pybind11/eigen.h>

Copy link
Collaborator

@francesco-romano francesco-romano Dec 1, 2021

Choose a reason for hiding this comment

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

We can change it, what do you suggest?

What about idyntree-pybind11-vector-casters?

Also, can you remove
set(libraryname idyntree-pybind11) and use explicitly the new library name?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you also:

  • move the install part down together with the install of the module?
  • What is the purpose of the ALIAS?

Copy link
Member

Choose a reason for hiding this comment

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

@GiulioRomualdi what do you think of idyntree-pybind11-vector-casters @GiulioRomualdi ? It seems a nice name to me.

pybind11_add_module(pybind11_idyntree idyntree.cpp
error_utilities.h error_utilities.cpp
idyntree_core.h idyntree_core.cpp
Expand All @@ -9,7 +32,8 @@ pybind11_add_module(pybind11_idyntree idyntree.cpp
target_link_libraries(pybind11_idyntree PUBLIC idyntree-core
idyntree-model
idyntree-sensors
idyntree-modelio)
idyntree-modelio
idyntree-pybind11)

# The generated Python dynamic module must have the same name as the pybind11
# module, i.e. `bindings`.
Expand All @@ -19,7 +43,7 @@ set_target_properties(pybind11_idyntree PROPERTIES

# if compile tests execute also python tests
if(IDYNTREE_COMPILE_TESTS)
add_subdirectory(tests)
add_subdirectory(tests idyntree_pybind11_test)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think there is a way to avoid this and avoid modifying the test Cmake working directory variable?

Copy link
Member

Choose a reason for hiding this comment

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

@GiulioRomualdi any thoughts on this? Note that if it is difficult I prefer to merge the PR in the current form, it is not particularly problematic to have this.

endif()

# Output package is:
Expand Down
163 changes: 44 additions & 119 deletions bindings/pybind11/idyntree_core.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "idyntree_core.h"

#include <iDynTree/pybind11/VectorCasters.h>
#include <iDynTree/Core/Axis.h>
#include <iDynTree/Core/Direction.h>
#include <iDynTree/Core/MatrixDynSize.h>
Expand All @@ -24,61 +25,24 @@ namespace bindings {
namespace {
namespace py = ::pybind11;

void vectorDynSizeClassDefinition(py::class_<VectorDynSize>& vector) {
vector.def(py::init())
.def(py::init<unsigned>())
template <typename VectorType>
void baseVectorDefinition(py::class_<VectorType>& vector) {
vector
.def("__getitem__", py::overload_cast<const std::size_t>(
&VectorDynSize::operator(), py::const_))
&VectorType::operator(), py::const_))
.def("__setitem__",
[](VectorDynSize& the_vector, std::size_t index, double new_value) {
the_vector(index) = new_value;
})
.def(
"__iter__",
[](const VectorDynSize& s) {
return py::make_iterator(s.begin(), s.end());
},
py::keep_alive<
0, 1>() /* Essential: keep object alive while iterator exists */)
.def("__len__", &VectorDynSize::size)
.def("set_zero", &VectorDynSize::zero)
.def("resize", &VectorDynSize::resize)
.def("__repr__", &VectorDynSize::toString)
.def_buffer([](VectorDynSize& v) -> py::buffer_info {
return py::buffer_info(
v.data(), /* Pointer to buffer */
sizeof(double), /* Size of one scalar */
py::format_descriptor<double>::format(), /* Python struct-style
format descriptor */
1, /* Number of dimensions */
{v.size()}, /* Buffer dimensions */
{sizeof(double)} /* Strides (in bytes) for each index */
);
});
}

template <unsigned size>
void createFixSizeVector(pybind11::module& module,
const std::string& class_name) {
py::class_<VectorFixSize<size>>(module, class_name.c_str(),
py::buffer_protocol())
.def(py::init())
.def("__getitem__", py::overload_cast<const std::size_t>(
&VectorFixSize<size>::operator(), py::const_))
.def("__setitem__",
[](VectorFixSize<size>& the_vector, std::size_t index,
[](VectorType& the_vector, std::size_t index,
double new_value) { the_vector(index) = new_value; })
.def("__len__", &VectorFixSize<size>::size)
.def(
"__iter__",
[](const VectorFixSize<size>& s) {
.def("__len__", &VectorType::size)
.def("__iter__",
[](const VectorType& s) {
return py::make_iterator(s.begin(), s.end());
},
py::keep_alive<
0, 1>() /* Essential: keep object alive while iterator exists */)
.def("set_zero", &VectorFixSize<size>::zero)
.def("__repr__", &VectorFixSize<size>::toString)
.def_buffer([](VectorFixSize<size>& v) -> py::buffer_info {
.def("set_zero", &VectorType::zero)
.def("__repr__", &VectorType::toString)
.def_buffer([](VectorType& v) -> py::buffer_info {
return py::buffer_info(
v.data(), /* Pointer to buffer */
sizeof(double), /* Size of one scalar */
Expand All @@ -91,58 +55,24 @@ void createFixSizeVector(pybind11::module& module,
});
}

void matrixDynSizeClassDefinition(py::class_<MatrixDynSize>& matrix) {
matrix.def(py::init())
.def(py::init<unsigned, unsigned>())
template <typename MatrixType>
void baseMatrixDefinition(py::class_<MatrixType>& matrix) {
matrix
.def("__getitem__",
[](MatrixDynSize& matrix,
[](MatrixType& matrix,
std::pair<std::size_t, std::size_t> indices) {
return matrix.getVal(indices.first, indices.second);
})
.def("__setitem__",
[](MatrixDynSize& matrix,
[](MatrixType& matrix,
std::pair<std::size_t, std::size_t> indices, double item) {
return matrix.setVal(indices.first, indices.second, item);
})
.def("rows", &MatrixDynSize::rows)
.def("cols", &MatrixDynSize::cols)
.def("set_zero", &MatrixDynSize::zero)
.def("resize", &MatrixDynSize::resize)
.def("__repr__", &MatrixDynSize::toString)
.def_buffer([](MatrixDynSize& m) -> py::buffer_info {
return py::buffer_info(
m.data(), /* Pointer to buffer */
sizeof(double), /* Size of one scalar */
py::format_descriptor<double>::format(), /* Python struct-style
format descriptor */
2, /* Number of dimensions */
{m.rows(), m.cols()}, /* Buffer dimensions */
{sizeof(double) * m.cols(), /* Strides (in bytes) for each index */
sizeof(double)});
});
}

template <unsigned rows, unsigned cols>
void createFixSizeMatrix(pybind11::module& module,
const std::string& class_name) {
py::class_<MatrixFixSize<rows, cols>>(module, class_name.c_str(),
py::buffer_protocol())
.def(py::init())
.def("__getitem__",
[](MatrixFixSize<rows, cols>& matrix,
std::pair<std::size_t, std::size_t> indices) {
return matrix.getVal(indices.first, indices.second);
})
.def("__setitem__",
[](MatrixFixSize<rows, cols>& matrix,
std::pair<std::size_t, std::size_t> indices, double item) {
return matrix.setVal(indices.first, indices.second, item);
})
.def("rows", &MatrixFixSize<rows, cols>::rows)
.def("cols", &MatrixFixSize<rows, cols>::cols)
.def("set_zero", &MatrixFixSize<rows, cols>::zero)
.def("__repr__", &MatrixFixSize<rows, cols>::toString)
.def_buffer([](MatrixFixSize<rows, cols>& m) -> py::buffer_info {
.def("rows", &MatrixType::rows)
.def("cols", &MatrixType::cols)
traversaro marked this conversation as resolved.
Show resolved Hide resolved
.def("set_zero", &MatrixType::zero)
.def("__repr__", &MatrixType::toString)
.def_buffer([](MatrixType& m) -> py::buffer_info {
return py::buffer_info(
m.data(), /* Pointer to buffer */
sizeof(double), /* Size of one scalar */
Expand All @@ -160,8 +90,8 @@ void transformClassDefinition(py::class_<Transform>& transform) {
.def(py::init<const Rotation&, const Position&>())
.def(py::init<const MatrixFixSize<4, 4>&>())
.def_static("Identity", &Transform::Identity)
.def_property_readonly("rotation", &Transform::getRotation)
.def_property_readonly("position", &Transform::getPosition)
.def_property("rotation", &Transform::getRotation, &Transform::setRotation)
.def_property("position", &Transform::getPosition, &Transform::setPosition)
.def("inverse", &Transform::inverse)
.def(py::self * py::self)
.def(
Expand All @@ -178,44 +108,36 @@ void transformClassDefinition(py::class_<Transform>& transform) {
}
} // namespace

void iDynTreeCoreBindings(pybind11::module& module) {
// Vectors and matrices.
py::class_<VectorDynSize> vector_dyn(module, "VectorDynSize",
py::buffer_protocol());
vectorDynSizeClassDefinition(vector_dyn);
createFixSizeVector<3>(module, "Vector3");
createFixSizeVector<4>(module, "Vector4");
createFixSizeVector<6>(module, "Vector6");

py::class_<MatrixDynSize> matrix_dyn(module, "MatrixDynSize",
py::buffer_protocol());
matrixDynSizeClassDefinition(matrix_dyn);
createFixSizeMatrix<3, 3>(module, "Matrix3x3");
createFixSizeMatrix<4, 4>(module, "Matrix4x4");
createFixSizeMatrix<6, 6>(module, "Matrix6x6");
void iDynTreeCoreBindings(pybind11::module& module) {

// Positions, Rotations and Transforms.
py::class_<PositionRaw, VectorFixSize<3>>(module, "_PositionRaw")
// Do not expose constructor as we do not want users to use this class.
.def("__repr__", &PositionRaw::toString);
py::class_<PositionRaw> positionRaw(module, "_PositionRaw", py::buffer_protocol());
baseVectorDefinition(positionRaw);

GiulioRomualdi marked this conversation as resolved.
Show resolved Hide resolved
py::class_<Position, PositionRaw>(module, "Position")
GiulioRomualdi marked this conversation as resolved.
Show resolved Hide resolved
.def(py::init())
.def(py::init<double, double, double>())
.def(py::init([](const VectorFixSize<3>& position) {
return Position(position(0), position(1), position(2));
}))
.def(py::self + py::self)
.def(py::self - py::self)
.def(-py::self)
.def_static("Zero", &Position::Zero);
py::implicitly_convertible<iDynTree::VectorFixSize<3>, iDynTree::Position>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

To better understand this line (and related in Rotation).
This means that you can create a Position object from a VectorFixSize<3> without being explicit, right?

Is this really needed? Maybe write a test with and without this line and see the difference.

Doesn't the following work without this line?

position = Position([0, 1, 3])

As there is no explicit Vector class I was expecting this to work

Copy link
Member Author

Choose a reason for hiding this comment

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

position = Position([0, 1, 3])

should work but

transform = Transform(np.array([[1,0,0],[0,1,0],[0,0,1]]), [0,0,0])

won't.

Without the explicit caster you need to do something like this:

transform = Transform(Rotation(np.array([[1,0,0],[0,1,0],[0,0,1]])), Position([0,0,0]))

This is in general valid for each function that takes as input a position or a Rotation

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. This is what I was referring to.

I like that you can pass a Python list instead of an iDynTree vector, but I am afraid of doing this silently for all other objects that can be represented by a vector but have some other meanings, e.g. Rotation


py::class_<RotationRaw, MatrixFixSize<3, 3>>(module, "_RotationRaw")
// Do not expose constructor as we do not want users to use this class.
.def("__repr__", &RotationRaw::toString);
py::class_<RotationRaw> rotationRaw(module, "_RotationRaw", py::buffer_protocol());
baseMatrixDefinition(rotationRaw);

py::class_<Rotation, RotationRaw>(module, "Rotation")
.def(py::init())
.def(py::init<double, double, double, //
double, double, double, //
double, double, double>())
.def(py::init([](const MatrixFixSize<3,3>& rotation) {
return Rotation(rotation);
}))
.def("inverse", &Rotation::inverse)
.def(py::self * py::self)
.def(
Expand All @@ -225,23 +147,26 @@ void iDynTreeCoreBindings(pybind11::module& module) {
},
py::is_operator())
.def_static("Identity", &Rotation::Identity);
py::implicitly_convertible<iDynTree::MatrixFixSize<3,3>, iDynTree::Rotation>();

py::class_<Transform> transform(module, "Transform");
transformClassDefinition(transform);

// Other classes.
py::class_<Direction, VectorFixSize<3>>(module, "Direction")
.def(py::init<double, double, double>());
py::class_<Direction> direction(module, "Direction", py::buffer_protocol());
direction.def(py::init<double, double, double>());
baseVectorDefinition(direction);

py::class_<Axis>(module, "Axis")
.def(py::init<const Direction&, const Position&>())
.def_property("direction", &Axis::getDirection, &Axis::setDirection)
.def_property("origin", &Axis::getOrigin, &Axis::setOrigin)
.def("__repr__", &Axis::toString);

py::class_<RotationalInertiaRaw, MatrixFixSize<3, 3>>(module,
"RotationalInertia")
.def(py::init());
py::class_<RotationalInertiaRaw> rotationalInertia(module, "RotationalInertia",
py::buffer_protocol());
rotationalInertia.def(py::init());
baseMatrixDefinition(rotationalInertia);

py::class_<SpatialInertiaRaw>(module, "_SpatialInertiaRaw")
.def("from_rotational_inertia_wrt_center_of_mass",
Expand Down
1 change: 1 addition & 0 deletions bindings/pybind11/idyntree_model.cpp
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include "idyntree_model.h"
#include <iDynTree/pybind11/VectorCasters.h>

#include "error_utilities.h"

Expand Down
2 changes: 2 additions & 0 deletions bindings/pybind11/idyntree_modelio_urdf.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#include <iDynTree/pybind11/VectorCasters.h>

#include "idyntree_modelio_urdf.h"

#include "error_utilities.h"
Expand Down
2 changes: 2 additions & 0 deletions bindings/pybind11/idyntree_sensors.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@

#include <iDynTree/pybind11/VectorCasters.h>
#include "idyntree_sensors.h"
#include "error_utilities.h"

Expand Down
Loading