Fix hpp-fcl dependency

[abussy-aldebaran]

If template instantiation is not enabled, configuration fails because the target pinocchio_default becomes an interface library. Instead of basing the fix on the parameter ENABLE_TEMPLATE_INSTANTIATION, we detect directly if pinocchio_default is an interface library.

I assumed that PRIVATE dependency was necessary if pinocchio_default is not an interface library. If not, we can simply replace PUBLIC with INTERFACE in all cases.

[github-actions]

👋 Hi,
This is a reminder message to assign an extra build label to this Pull Request if needed.
By default, this PR will be build with minimal build options (URDF support and Python bindings)
The possible extra labels are:

• build_collision (build Pinocchio with coal support)
• build_casadi (build Pinocchio with CasADi support)
• build_autodiff (build Pinocchio with CppAD support)
• build_codegen (build Pinocchio with CppADCodeGen support)
• build_extra (build Pinocchio with extra algorithms)
• build_mpfr (build Pinocchio with Boost.Multiprecision support)
• build_sdf (build Pinocchio with SDF parser)
• build_accelerate (build Pinocchio with APPLE Accelerate framework support)
• build_all (build Pinocchio with ALL the options stated above)

Thanks.
The Pinocchio development team.

[jorisv]

Hello @abussy-aldebaran,

Thank's for the contribution !

I plan to change the way I manage ENABLE_TEMPLATE_INSTANTIATION.
Right now, I have hacked something really ugly by playing with the library target type.

• We add the template instantiation sources to the target if ENABLE_TEMPLATE_INSTANTIATION is activated.
• If the target is header only, we change his library target type
• The library type must then be forwarded in other part of the CMakeList.txt, creating some confusion

It's more easy to create a SHARED library with the template instantiation sources and to link it to the original target when ENABLE_TEMPLATE_INSTANTIATION is activated.

What do you think of this approach ?

[abussy-aldebaran]

To begin with, I am a bit confused by the current behaviour. I checked the installed pinocchioTargets.cmake without template instantiation :

# Create imported target pinocchio::pinocchio_headers
add_library(pinocchio::pinocchio_headers INTERFACE IMPORTED)

set_target_properties(pinocchio::pinocchio_headers PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS "BOOST_MPL_LIMIT_LIST_SIZE=30;BOOST_MPL_LIMIT_VECTOR_SIZE=30"
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include"
  INTERFACE_LINK_LIBRARIES "Eigen3::Eigen;Boost::boost;Boost::serialization"
)

# Create imported target pinocchio::pinocchio_default
add_library(pinocchio::pinocchio_default INTERFACE IMPORTED)

set_target_properties(pinocchio::pinocchio_default PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS "PINOCCHIO_WITH_HPP_FCL"
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include;\$<TARGET_PROPERTY:hpp-fcl::hpp-fcl,INTERFACE_INCLUDE_DIRECTORIES>"
  INTERFACE_LINK_LIBRARIES "pinocchio::pinocchio_headers"
)

Why the difference ?

I totally agree with you, it would simplify the logic greatly. Maybe you could create a third target ?

pinocchio_headers (interface)
pinocchio_default (alias)
pinocchio_binary (shared)

pinocchio_default would be an alias of one or the other depending on whether pinocchio was built with or without template instantiation.

As a bonus, an error would be raised if the user specifically depended on pinocchio_binary but pinocchio was built without template instantiation.

Feel free to find another name for pinocchio_binary. Maybe pinocchio_shared ?

[jorisv]

Hello,

Ok, some explanations (ETI = Explicit template instantiation):

target	description	type	type without ETI
pinocchio_headers	Pinocchio core (core, spatial, multibody, math, algorithms, serialization)	INTERFACE	INTERFACE
pinocchio_default	Pinocchio core + algorithms explicit template instantiation	SHARED	INTERFACE
pinocchio_extra	Extra algorithms	SHARED	SHARED
pinocchio_parallel	Parallel algorithms	INTERFACE	INTERFACE
pinocchio_collision	Collision algorithms	SHARED	INTERFACE
pinocchio_collision_parallel	Collision algorithms	INTERFACE	INTERFACE
pinocchio_visualizers	Visualizers interface	SHARED	SHARED
pinocchio_parsers	Parsers	SHARED	SHARED
pinocchio_cppadcg	Parsers	SHARED	INTERFACE
pinocchio_cppad	Parsers	SHARED	INTERFACE
pinocchio_casadi	Parsers	SHARED	INTERFACE
pinocchio_python_parser	Parsers	SHARED	SHARED

As you can see, the library type change for pinocchio_default, pinocchio_collision, pinocchio_cppadcg, pinocchio_cppad and pinocchio_casadi when ENABLE_TEMPLATE_INSTANTIATION=OFF.

To answer your first question, without ENABLE_TEMPLATE_INSTANTIATION, pinocchio_default look a lot alike pinocchio_headers. But this option is more for internal pinocchio development than for consumers. It allow to rebuild 1 unit test/benchmark without rebuilding the whole library.

In src/CMakeLists.txt managing to switch a library type add some convolution to the code. Instead of fixing the remaining issues, I suggest a more CMake idiomatic way: Building all ETI code in a separated target.

As an example, we will have:

• pinocchio_default: INTERFACE
• pinocchio_default_eti: SHARED

If ENABLE_TEMPLATE_INSTANTIATION=ON, then pinocchio_default_eti will be built and linked to pinocchio_default.
This will simplify the CMakeLists.txt a lot an fix the remaining bugs.

I'm not happy with eti suffix. Maybe binary is better as you suggested. impl is weird…

[abussy-aldebaran]

If ENABLE_TEMPLATE_INSTANTIATION=ON, then pinocchio_default_eti will be built and linked to pinocchio_default.

Do you mean

target_link_libraries(pinocchio_default INTERFACE pinocchio_default_eti)

or

target_link_libraries(pinocchio_default_eti PUBLIC pinocchio_default)

?

[jorisv]

I mean target_link_libraries(pinocchio_default INTERFACE pinocchio_default_eti) so end user don't care and only have to link against pinocchio_default.