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

Moving to std::shared_ptr #218

Closed
nim65s opened this issue Apr 14, 2021 · 11 comments
Closed

Moving to std::shared_ptr #218

nim65s opened this issue Apr 14, 2021 · 11 comments
Assignees

Comments

@nim65s
Copy link
Member

nim65s commented Apr 14, 2021

Hi,

The HPP framework is now using C++11, and is therefore going move to std::shared_ptr instead of boostones.

Keeping boost::shared_ptr in hpp-fcl would be a pain, and this might not be totally necessary.

I suggest to move hpp-fcl to C++11 and std::shared_ptr, and to release a new major version for this change.

For pinocchio, and other third-parties that rely on hpp-fcl, this might lead to something like:
https://github.com/stack-of-tasks/pinocchio/blob/master/src/CMakeLists.txt#L51-L54

Obviously, I can handle this work, and the related updates in the HPP, SoT, loco-3d & gepetto projects, but is there any objection for this ?

My main motivation is that the current version of hpp-pinocchio is not compatible with pinocchio v2.6.0, so I can't publish pinocchio v2.6.0 on robotpkg until a new HPP release is done, and the switch to std::shared_ptr is already on-going on this new HPP release.

@jcarpent
Copy link
Member

jcarpent commented Apr 14, 2021

Why do say that hpp-fcl is C++11? This is not the case on my side.
I don't think that we need to go towards std::shared_ptr yet. For hpp-fcl 2.0, we already discussed a bit with @jmirabel on some improvements which may simplify hpp-fcl while also improving the whole performances.
See https://github.com/humanoid-path-planner/hpp-fcl/projects/1

@jcarpent
Copy link
Member

jcarpent commented Apr 14, 2021

hpp-pinocchio is not compatible with pinocchio v2.6.0, so I can't publish pinocchio v2.6.0

What do you mean exactly?
I do not see any reason why hpp-pinocchio should be not compatible with Pinocchio 2.6.0 as there is no API or ABI breaching coming from Pinocchio.

@nim65s
Copy link
Member Author

nim65s commented Apr 14, 2021

hpp-fcl is not yet C++11 only.

In this issue, I am suggesting to change this, because this would be easier to switch all HPP at once, instead of having boost::shared_ptr in "hpp-fcl" and std::shared_ptr in "all HPP but hpp-fcl".

This would require less work to get everything working again, and will be faster.

And anyway, I guess we'll want to switch to std::shared_ptr in hpp-fcl at some point. C++11 is 10 years old now. So from my point of view, the question is "What are we waiting for", given that waiting more would require more work.

For hpp-pinocchio, cf. humanoid-path-planner/hpp-pinocchio#146

@jcarpent
Copy link
Member

For me, this will break compatibility with Pinocchio, because hpp-fcl objects are exposed in the current API of Pinocchio.
The only solution on my side will be to wait for Pinocchio 3.x which will make C++11 the default C++ version with template generation support.

My veto is to wait for Pinocchio 3.x. We can discuss in private to see what is missing to launch Pinocchio 3.x

@nim65s
Copy link
Member Author

nim65s commented Apr 14, 2021

ok, let's synchronize this with Pinocchio 3 then.

nim65s added a commit to humanoid-path-planner/hpp-rbprm that referenced this issue Apr 14, 2021
- use shared pointers from std instead of boost, except for hpp-fcl:
  humanoid-path-planner/hpp-fcl#218
- use shared pointers to class core::Problem only
  humanoid-path-planner/hpp-core@7641d24
- constraints::Implicit::create(func) was removed, we need to
  explicitely use create(func, comp)
  humanoid-path-planner/hpp-constraints@837e9b9
- core::ConfigProjector::add(nm, passiveDofs, priority) was removed, we
  need to use add(nm, priority)
  humanoid-path-planner/hpp-core@a658fca
- include/hpp/rbprm/planner/parabola-path.hh &
  include/hpp/rbprm/planner/steering-method-parabola.hh were encoded in
  a DOS format. They have been automatically re-encoded for UNIX.
@jcarpent
Copy link
Member

Thanks @nim65s for your compliance.

@jmirabel
Copy link
Contributor

jmirabel commented Feb 1, 2022

#256 raised the C++ 11 question again. It has been 9 months since this discussion was started. A few questions:

  • What prevents future 2.x release of pinocchio to be C++ 11 ?
  • Anyone else has an opinion on this ?

@jcarpent
Copy link
Member

jcarpent commented Feb 1, 2022

  • What prevents future 2.x release of pinocchio to be C++ 11 ?

As already discussed, starting from Pinocchio 3.x.

@jmirabel
Copy link
Contributor

jmirabel commented Feb 1, 2022 via email

@jcarpent
Copy link
Member

jcarpent commented Feb 1, 2022

For, me the main reason is downstream users of Pinocchio who have to stick to C++98.
I don't want to abandon them at this stage and prefer to prepare the spirit with Pinocchio 3.x
This could be the same for hpp-fcl, with version 2.x and more, compliant with C++14 and more.

@jcarpent
Copy link
Member

@nim65s You can now move on to this issue. #260 forces the switch to C++ 11 and more.

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

No branches or pull requests

3 participants