Skip to content

Always suppose hpp-fcl Python binding are present when hpp-fcl C++ library is found #2288

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 18, 2024
Merged

Always suppose hpp-fcl Python binding are present when hpp-fcl C++ library is found #2288

merged 2 commits into from
Jun 18, 2024

Conversation

jorisv
Copy link
Contributor

@jorisv jorisv commented Jun 18, 2024

Pinocchio Python binding needs hpp-fcl Python binding for collision/contact related algorithms.

We allow with the BUILD_WITH_HPP_FCL_PYTHON_BINDINGS CMake variable to build Pinocchio C++ library with hpp-fcl but to ignore hpp-fcl in the Python bindings if the hpp-fcl Python bindings are not installed.

This create the following issues:

  • To compute BUILD_WITH_HPP_FCL_PYTHON_BINDINGS we must call the Python interpreter. This create some issues when cross compiling.
  • We never test this case in our CI, so we don't know if this option is working.

This PR remove this option to simplify the build.
In the case an user build Pinocchio Python binding with hpp-fcl C++ library but without hpp-fcl binding, the Python interpreter will raise an ImportError.

@jorisv jorisv self-assigned this Jun 18, 2024
jorisv added 2 commits June 18, 2024 11:45
@jorisv
cmake: Always suppose hpp-fcl Python binding are present when hpp-fcl…
9467f1a 
… C++ library is found

Pinocchio Python binding needs hpp-fcl Python binding for
collision/contact related algorithms.

We allow with the `BUILD_WITH_HPP_FCL_PYTHON_BINDINGS` CMake variable to build
Pinocchio C++ library with hpp-fcl but to ignore hpp-fcl in the Python
bindings if the hpp-fcl Python bindings are not installed.

This create the following issues:
- To compute `BUILD_WITH_HPP_FCL_PYTHON_BINDINGS` we must call the
  Python interpreter. This create some issue when cross compiling.
- We never test this case in our CI, so we don't know if this option is
  working.

This PR remove this option to simplify the build.
In the case an user build Pinocchio Python binding with hpp-fcl
C++ library but without hpp-fcl binding, the Python interpreter will
raise an ImportError.
@jorisv
changelog: Add changelog entry
814c506
@jorisv jorisv force-pushed the topic/simplify_hpp_fcl_bindings branch from febb5d8 to 814c506 Compare June 18, 2024 09:45
@ManifoldFR
Copy link
Member

ManifoldFR commented Jun 18, 2024 •
edited
Loading

This seems sensible to me, noting the issues we've had over at the conda-forge feedstock with cross-building: https://github.com/conda-forge/pinocchio-feedstock/pull/117/checks?check_run_id=26326030013.

We had a similar problem with getting proxsuite-nlp over on aarch64 and ppc due to the same check happening during crossbuilding.

It would be perhaps better to check if the hpp-fcl Python bindings' CMake target exists, if we want to reintroduce a check like this in the future.

nim65s
nim65s approved these changes Jun 18, 2024
View reviewed changes
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.

This has been on my todolist for years, thanks a lot !

@jorisv
Copy link
Contributor Author

jorisv commented Jun 18, 2024

It would be perhaps better to check if the hpp-fcl Python bindings' CMake target exists, if we want to reintroduce a check like this in the future.

I don't think we want to reintroduce this in the future. The build issues in conda-forge show this option was broken. We already have too much options in Pinocchio, and it's really hard to test all the combination.

I also think this kind of issue should be managed by a package manager instead of CMake.

@ManifoldFR
Copy link
Member

It would be perhaps better to check if the hpp-fcl Python bindings' CMake target exists, if we want to reintroduce a check like this in the future.

I don't think we want to reintroduce this in the future. The build issues in conda-forge show this option was broken. We already have too much options in Pinocchio, and it's really hard to test all the combination.

I also think this kind of issue should be managed by a package manager instead of CMake.

Totally makes sense! I agree the combinatorics of all the options was too much.

@jorisv jorisv merged commit df3aa95 into stack-of-tasks:devel Jun 18, 2024
18 checks passed
@jorisv jorisv deleted the topic/simplify_hpp_fcl_bindings branch June 18, 2024 12:49
nim65s added a commit to nim65s/robotpkg that referenced this pull request Oct 9, 2024
@nim65s
[math/{py-,}pinocchio] Update to v3.2.0
197d1f3 
    ## [3.2.0] - 2024-08-27

    ### Fixed
    - Append pinocchio optional libraries into pkg-config file (stack-of-tasks/pinocchio#2322)
    - Fixed support of DAE meshes with MeshCat (stack-of-tasks/pinocchio#2331)
    - Fixed pointer casts in urdf parser (stack-of-tasks/pinocchio#2339)
    - Remove CMake CMP0167 warnings (stack-of-tasks/pinocchio#2347)
    - Fixed urdfdom in ROS packaging (stack-of-tasks/pinocchio#2341)
    - Fixed overview-urdf cpp example (stack-of-tasks/pinocchio#2384)
    - Fixed mjcf model without a base link parsing (stack-of-tasks/pinocchio#2386)
    - Fixed talos-simulation.py, simulation-contact-dynamics.py and simulation-closed-kinematic-chains.py examples (stack-of-tasks/pinocchio#2392)

    ### Added
    - Add getMotionAxis method to helical, prismatic, revolute and ubounded revolute joint (stack-of-tasks/pinocchio#2315)
    - Add initial compatiblity with coal (coal needs `-DCOAL_BACKWARD_COMPATIBILITY_WITH_HPP_FCL=ON`) (stack-of-tasks/pinocchio#2323)
    - Add compatibility with jrl-cmakemodules workspace (stack-of-tasks/pinocchio#2333)
    - Add ``collision_color`` parameter to `MeshcatVisualizer.loadViewerModel` (stack-of-tasks/pinocchio#2350)
    - Add ``BuildFromMJCF`` function to RobotWrapper (stack-of-tasks/pinocchio#2363)
    - Add more CasADi examples (stack-of-tasks/pinocchio#2388)

    ### Removed
    - Remove deprecated headers related to joint constraints (stack-of-tasks/pinocchio#2382)

    ### Changed
    - Use eigenpy to expose `GeometryObject::meshMaterial` variant (stack-of-tasks/pinocchio#2315)
    - GepettoViewer is no more the default viewer for RobotWrapper (stack-of-tasks/pinocchio#2331)
    - Modernize python code base with ruff (stack-of-tasks/pinocchio#2367)
    - Restructure CppAD and CasADi examples (stack-of-tasks/pinocchio#2388)
    - Enhance and fix CppAD benchmarks outputs (stack-of-tasks/pinocchio#2393)

    ## [3.1.0] - 2024-07-04

    ### Fixed

    - Fix `appendModel` when joints after the base are in parallel (stack-of-tasks/pinocchio#2295)
    - Fix `appendModel` build when called with template arguments different than the ones from `context` (stack-of-tasks/pinocchio#2284)
    - Fix `TransformRevoleTpl::rotation` and `TransformHelicalTpl::rotation` build (stack-of-tasks/pinocchio#2284)
    - Fix compilation issue for Boost 1.85 (stack-of-tasks/pinocchio#2255)
    - Fix python bindings of `contactInverseDynamics` (stack-of-tasks/pinocchio#2263)
    - Deactivate `BUILD_WITH_LIBPYTHON` when building with PyPy (stack-of-tasks/pinocchio#2274)
    - Fix Python bindings cross building with `hpp-fcl` (stack-of-tasks/pinocchio#2288)
    - Fix build issue on Windows when a deprecated header is included (stack-of-tasks/pinocchio#2292)
    - Fix build issue on Windows when building in Debug mode (stack-of-tasks/pinocchio#2292)
    - Fix visualization of meshes in meshcat (stack-of-tasks/pinocchio#2294)
    - Fix Anymal simulation test (stack-of-tasks/pinocchio#2299)
    - Fix contact derivatives and impulse dynamics tests (stack-of-tasks/pinocchio#2300)
    - Fix CMake compatibility with old console_bridge version (stack-of-tasks/pinocchio#2312)

    ### Added

    - Python unittest for `contactInverseDynamics` function (stack-of-tasks/pinocchio#2263)
    - Added helper functions to return operation count of CasADi functions. (stack-of-tasks/pinocchio#2275)
    - C++ and Python unittest for `dIntegrateTransport` to check vector transport and its inverse (stack-of-tasks/pinocchio#2273)
    - Add kinetic and potential energy regressors (stack-of-tasks/pinocchio#2282)

    ### Removed

    - Remove header `list.hpp` include for bindings of model and rnea (stack-of-tasks/pinocchio#2263)

Packaging Changes:
- Removed patches af, an, ap: fixed upstream
- Updated patches ag, ak
nim65s added a commit to nim65s/robotpkg that referenced this pull request Oct 11, 2024
@nim65s
[math/{py-,}pinocchio] Update to v3.2.0
1ccd712 
    ## [3.2.0] - 2024-08-27

    ### Fixed
    - Append pinocchio optional libraries into pkg-config file (stack-of-tasks/pinocchio#2322)
    - Fixed support of DAE meshes with MeshCat (stack-of-tasks/pinocchio#2331)
    - Fixed pointer casts in urdf parser (stack-of-tasks/pinocchio#2339)
    - Remove CMake CMP0167 warnings (stack-of-tasks/pinocchio#2347)
    - Fixed urdfdom in ROS packaging (stack-of-tasks/pinocchio#2341)
    - Fixed overview-urdf cpp example (stack-of-tasks/pinocchio#2384)
    - Fixed mjcf model without a base link parsing (stack-of-tasks/pinocchio#2386)
    - Fixed talos-simulation.py, simulation-contact-dynamics.py and simulation-closed-kinematic-chains.py examples (stack-of-tasks/pinocchio#2392)

    ### Added
    - Add getMotionAxis method to helical, prismatic, revolute and ubounded revolute joint (stack-of-tasks/pinocchio#2315)
    - Add initial compatiblity with coal (coal needs `-DCOAL_BACKWARD_COMPATIBILITY_WITH_HPP_FCL=ON`) (stack-of-tasks/pinocchio#2323)
    - Add compatibility with jrl-cmakemodules workspace (stack-of-tasks/pinocchio#2333)
    - Add ``collision_color`` parameter to `MeshcatVisualizer.loadViewerModel` (stack-of-tasks/pinocchio#2350)
    - Add ``BuildFromMJCF`` function to RobotWrapper (stack-of-tasks/pinocchio#2363)
    - Add more CasADi examples (stack-of-tasks/pinocchio#2388)

    ### Removed
    - Remove deprecated headers related to joint constraints (stack-of-tasks/pinocchio#2382)

    ### Changed
    - Use eigenpy to expose `GeometryObject::meshMaterial` variant (stack-of-tasks/pinocchio#2315)
    - GepettoViewer is no more the default viewer for RobotWrapper (stack-of-tasks/pinocchio#2331)
    - Modernize python code base with ruff (stack-of-tasks/pinocchio#2367)
    - Restructure CppAD and CasADi examples (stack-of-tasks/pinocchio#2388)
    - Enhance and fix CppAD benchmarks outputs (stack-of-tasks/pinocchio#2393)

    ## [3.1.0] - 2024-07-04

    ### Fixed

    - Fix `appendModel` when joints after the base are in parallel (stack-of-tasks/pinocchio#2295)
    - Fix `appendModel` build when called with template arguments different than the ones from `context` (stack-of-tasks/pinocchio#2284)
    - Fix `TransformRevoleTpl::rotation` and `TransformHelicalTpl::rotation` build (stack-of-tasks/pinocchio#2284)
    - Fix compilation issue for Boost 1.85 (stack-of-tasks/pinocchio#2255)
    - Fix python bindings of `contactInverseDynamics` (stack-of-tasks/pinocchio#2263)
    - Deactivate `BUILD_WITH_LIBPYTHON` when building with PyPy (stack-of-tasks/pinocchio#2274)
    - Fix Python bindings cross building with `hpp-fcl` (stack-of-tasks/pinocchio#2288)
    - Fix build issue on Windows when a deprecated header is included (stack-of-tasks/pinocchio#2292)
    - Fix build issue on Windows when building in Debug mode (stack-of-tasks/pinocchio#2292)
    - Fix visualization of meshes in meshcat (stack-of-tasks/pinocchio#2294)
    - Fix Anymal simulation test (stack-of-tasks/pinocchio#2299)
    - Fix contact derivatives and impulse dynamics tests (stack-of-tasks/pinocchio#2300)
    - Fix CMake compatibility with old console_bridge version (stack-of-tasks/pinocchio#2312)

    ### Added

    - Python unittest for `contactInverseDynamics` function (stack-of-tasks/pinocchio#2263)
    - Added helper functions to return operation count of CasADi functions. (stack-of-tasks/pinocchio#2275)
    - C++ and Python unittest for `dIntegrateTransport` to check vector transport and its inverse (stack-of-tasks/pinocchio#2273)
    - Add kinetic and potential energy regressors (stack-of-tasks/pinocchio#2282)

    ### Removed

    - Remove header `list.hpp` include for bindings of model and rnea (stack-of-tasks/pinocchio#2263)

Packaging Changes:
- Removed patches af, an, ap: fixed upstream
- Updated patches ag, ak
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nim65s nim65s nim65s approved these changes

@jcarpent jcarpent jcarpent approved these changes

Assignees

@jorisv jorisv

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jorisv @ManifoldFR @nim65s @jcarpent