envire / envire-envire_core

Core part for the Environment Representation library
BSD 2-Clause "Simplified" License
7 stars 13 forks source link

Remove Dependency to TransformWithCovariance #43

Open arneboe opened 5 years ago

arneboe commented 5 years ago

If we can get rid of the dependency, we can get rid of nearly all dependencies.

We could remove the dependency by templating items/Transform and the TransformGraph. But this would break backward compatibility with all existing users.

arneboe commented 5 years ago

A quick and dirty attempt to replace the dependency to TransformWithCovariance with a template parameter. https://github.com/envire/envire-envire_core/tree/lessDependencies

The next step would be to replace it with Eigen::Transform and see what happens. I guess Transform and TransformWithCov are more or less api compatible?

This is a breaking change that requires all users to change their code...

planthaber commented 5 years ago

Can't we check if base-types is available in the CMakeLists.txt and use either that transform or the internal one?

Transformwithcov has two fileds:

base::Position translation; base::Quaterniond orientation;

where base::Position is a Eigen::Matrix<double, 3, 1, Eigen::DontAlign> and base::Quaterniond a Eigen::Quaternion<double, Eigen::DontAlign>

https://github.com/rock-core/base-types/blob/master/src/Pose.hpp#L10 https://github.com/rock-core/base-types/blob/master/src/Eigen.hpp

chhtz commented 5 years ago

Is the Covariance part used at any point? If not, it would make sense to get rid of that anyway (and save 36 doubles per Frame -- but more importantly the effort to propagate covariances). I would not suggest to make the API depend on compile-time switches.

Whether to store Transformations as Vec3+Quaternion (as in base::Pose) or 3x4 matrix (as in Eigen::Affine3d) probably does not make too much difference.

saarnold commented 5 years ago

Is the Covariance part used at any point? If not, it would make sense to get rid of that anyway (and save 36 doubles per Frame -- but more importantly the effort to propagate covariances).

This is the reason why the TransformWithCovariance type was used. Whether they are used or not was supposed to be the decision of the user.

arneboe commented 5 years ago

IMO replacing the dependency with a template parameter is still the best option. This way users inside of rock can use TransformWithCovariance and users outside can use Eigen. The interface of TransformWithCovariance that we are actually using is not that large and should be more or less compatible with Eigen::Transform? I'll take a look and try to write a simple concept that we can check for to communicate the needed interface to the user.

HWiese1980 commented 5 years ago

How far has this evolved? I'm currently running into the dependency issue where I'd like to compile a small test program and apparently need base types...

planthaber commented 5 years ago

Not much further, but you can use the install_dependencies.sh script from the top folder in the repo to install all source dependencies at once.

It takes on parameter (the folder to install to)

planthaber commented 5 years ago

It also creates an env.sh script to be sourced befor you can compile envire

HWiese1980 commented 5 years ago

I've managed to compile my test program. The more important issue I'm currently dealing with is #53

arneboe commented 4 years ago

I looked into this some more. I replaced the TransformWithCovariance type with a template parameter. Thus in theory the user could choose a different transformation type. However it is not possible to replace it with Eigen::Transform because TransformWithCovariance andEigen::Transform are not api compatible at all. Several breaking changes to TransformWithCovariance are required to make them compatible. I guess that is out of the question.

Creating a wrapper around Eigen::Transform that is compatible with TransformWithCovariance is seems impossible as well because TransformWithCovariance exposes the translation and orientation attributes publicly and they are used everywhere.

I guess the easiest solution would be to copy TransformWithCovariance back into envire_core and just use it.

arneboe commented 4 years ago

Or the install script could download the TransformWithCovariance headers and sources and add them to the include path instead of installing the complete base-types package.

arneboe commented 4 years ago

Ok after some further investigation I think that it is not worth the effort.

The current external dependencies are:

All of those packages install without any problem and none of them does any harm. It's just too much effort to reduce the dependencies any further.