Upstream changes from iit-danieli-joint-lab/abb_libegm fork

gavanderhoorn commented 4 years ago

@traversaro et al.: your fork appears to have some interesting changes that may be good to upstream.

Things that caught my eye:

windows build fixes (I don't believe we currently target Windows too much, but it'd be good to try and be nice to other users)
s/Boost/std/g
Protobuf related changes/fixes (although we need to maintain bw-compatibility with CMake 3.6 for now)
changes to better support cross-compilation

and a few others.

Would you be interested in submitting some of that as PRs here?

traversaro commented 4 years ago

Hi @gavanderhoorn , we are definitely interested in providing the fix upstream, especially because that would reduce the maintenance effort for us in the long term.

windows build fixes (I don't believe we currently target Windows too much, but it'd be good to try and be nice to other users)

Most of those are just earlier versions of https://github.com/ros-industrial/abb_libegm/pull/63 , so I think there is not a lot to port.

s/Boost/std/g

This is probably the most important and under some aspects non-trivial change. As soon as it is clear that C++98 compatibility can be dropped, I would be happy to provide the PRs to migrate away from boost. Note that if you want to avoid Boost completely we need at least do depend on standalone Asio and this may be desirable or not depending on the use case. In theory to avoid depending on boost::math we used Eigen, but that can probably be avoided as the only operation used is actually the equation conjugate, that is trivial to implement.

Relevant commits (pay attention as we have also subsequent bug fixes not squashed):

Protobuf related changes/fixes (although we need to maintain bw-compatibility with CMake 3.6 for now)

Yes, there are some strange issues on the location of protobuf generated files with recent protobuf and CMAke versions, probably setting up a CI job that consumes dependencies from vcpkg is the best way to highlight those issues (see https://github.com/iit-danieli-joint-lab/abb_libegm/commit/f80b8b1168f793097b8231931f648ab62ca3991b).

changes to better support cross-compilation

Those are a bit tricky, because you need to define how to handle the code generation with an external protoc, and unfortunately probably this requires changes in the FindProtobuf.cmake shipped with CMake, or with the ProtobufConfig.cmake shipped with Protobuf itself, and especially protobuf upstream is simply ignoring issues regarding non-standard platforms (see for example https://github.com/protocolbuffers/protobuf/issues/4198 ). The alternative that we used is to incude the generated files directly in the source, but unfortunatly this only works for a given version of protobuf. Related ign-msgs issue: https://bitbucket.org/ignitionrobotics/ign-msgs/issues/34/add-support-for-cross-compilation .

gavanderhoorn commented 4 years ago

s/Boost/std/g

This is probably the most important and under some aspects non-trivial change. As soon as it is clear that C++98 compatibility can be dropped [..]

We're currently targetting Kinetic and Melodic, mostly, here.

Kinetic allows to set the minimum level of C++ to c++11.

We have no (and don't state any) requirement to be c++98 compatible afaik.

@jontje ?

jontje commented 4 years ago

We have no (and don't state any) requirement to be c++98 compatible afaik.

@jontje ?

There is no such requirement.

s/Boost/std/g

This is probably the most important and under some aspects non-trivial change. As soon as it is clear that C++98 compatibility can be dropped, I would be happy to provide the PRs to migrate away from boost.

I would be glad to see those changes; I have had that on my agenda for years, but no time for it 😄 @traversaro can you provide link(s) to the changes? I just want to skim through them.

traversaro commented 4 years ago

@traversaro can you provide link(s) to the changes? I just want to skim through them.

There are links to the relevant commit in the issue https://github.com/ros-industrial/abb_libegm/issues/70#issuecomment-581354215 , if you tell us what are the changes you are first interested in we can prepare clean PRs/patches .

gavanderhoorn commented 4 years ago

@traversaro: the Windows build related changes may be easiest / most stand-alone?

traversaro commented 4 years ago

@traversaro: the Windows build related changes may be easiest / most stand-alone?

As far as I remember the Windows-related changes are due to the use of a newer protobuf (3.11) installed by vcpkg, see https://github.com/iit-danieli-joint-lab/abb_libegm/issues/2 . Interestingly, currently Debian sid and Ubuntu 20.04 still ship protobuf 3.6, so even on 20.04 you will not experience the issue. I would be happy to start tackling those one, but without having a CI that covers the Windows compilation while installing dependencies with vcpkg it would be quite easy to have regression. I would be happy to add a new GitHub Actions based CI for covering the vcpkg case, but that is yes another thing to maintain so let me know if you are interested in it or not.

jontje commented 4 years ago

There are links to the relevant commit in the issue #70 (comment) , if you tell us what are the changes you are first interested in we can prepare clean PRs/patches .

Regarding the code changes, then I think it would be reasonable to do it in this order:

Remove the boost::math dependency by implementing the quaternion conjugate operation, which should be straight-forward.
Replace as much as possible of the boost components with std components (i.e. the non-asio components).
Finally consider replacing boost::Asio with standalone Asio.

gavanderhoorn commented 4 years ago

@traversaro: would what @jontje suggests be acceptable?

traversaro commented 4 years ago

@traversaro: would what @jontje suggests be acceptable?

Totally! Recent events here in Italy kind of changed our workplan, but I think we can start with a PR removing the boost::math depending relatively soon.

gavanderhoorn commented 4 years ago

Of course, there is no hurry.

Some things are more important.

traversaro commented 4 years ago

Third PR: https://github.com/ros-industrial/abb_libegm/pull/103 .

ros-industrial / abb_libegm

Upstream changes from iit-danieli-joint-lab/abb_libegm fork #70