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

Remove Dependency to TransformWithCovariance #43

Open
arneboe opened this issue Mar 25, 2019 · 12 comments
Open

Remove Dependency to TransformWithCovariance #43

arneboe opened this issue Mar 25, 2019 · 12 comments

Comments

@arneboe
Copy link
Contributor

arneboe commented Mar 25, 2019

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
Copy link
Contributor Author

arneboe commented Mar 25, 2019

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
Copy link
Member

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
Copy link
Member

chhtz commented Mar 25, 2019

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
Copy link
Member

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
Copy link
Contributor Author

arneboe commented Mar 26, 2019

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
Copy link

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
Copy link
Member

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
Copy link
Member

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

@HWiese1980
Copy link

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

@arneboe
Copy link
Contributor Author

arneboe commented Sep 5, 2019

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 and Eigen::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
Copy link
Contributor Author

arneboe commented Sep 5, 2019

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
Copy link
Contributor Author

arneboe commented Sep 5, 2019

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

The current external dependencies are:

  • base-cmake.git Everything depends on it
  • base-logging plugin manager depends on it
  • base-types We need TransformWithCovariance
  • base_boost_serialization We need this to serialize the Transforms
  • base-numeric **Is needed by boost_serialization **

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants