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

Minimal version of Envire (only graph + transformations) #32

Open
pnarvor opened this issue Sep 12, 2018 · 22 comments
Open

Minimal version of Envire (only graph + transformations) #32

pnarvor opened this issue Sep 12, 2018 · 22 comments

Comments

@pnarvor
Copy link
Contributor

pnarvor commented Sep 12, 2018

Hello !

I am part of a European project about space robotics (InFuse : https://www.h2020-infuse.eu/). We used the great functionalities of Envire about transform and graph in core libraries in InFuse project to manage both the internal state of the robot and the pose of the robot relative to various frames in the environment.

However, we felt that this "low level" functionality of Envire had too many heavy dependencies to be easily integrated into such basic libraries, so we made a minimal version only depending only on Eigen3 and Boost.

We thought you might be interested so here is the repository:

https://github.com/pnarvor/infuse_envire

Have a nice day !

@planthaber
Copy link
Member

We are, at least i am ;-)

Btw.: What exactly does the viz folder? Could it be moved to envire_visualization?

Another option would be to make the rock macros optional, if not found, build this way without viz ans test.

@chhtz
Copy link
Member

chhtz commented Sep 12, 2018

Definitely interesting. Whether we actually change "rock-envire" to be less rock-dependent would be a bigger policy-decision, though. I guess it will be hard to use both your and the original envire together.

@planthaber I guess viz could indeed be moved outside (maybe @arneboe knows the reason, why it is here). But I think the bigger issue regarding compatibility is whether the reduced base-types, base-logging and boost_serialization could co-exist with the ones provided by rock.

@skasperski This might be interesting for you.

@arneboe
Copy link
Contributor

arneboe commented Sep 12, 2018

@planthaber I guess the viz folder contains visualizations from before envire_visualization was a thing.
They can be moved.

Why did you chose to integrate base-types etc. directly?
The only rock dependency they have is base-cmake which is just a bunch of cmake scripts.
Adding them as submodules instead would not really add any overhead but you would stay up to date with upstream.

@saarnold
Copy link
Member

Integrating source code from other libraries instead of linking them just feels wrong..

Btw.: What exactly does the viz folder? Could it be moved to envire_visualization?

The viz folder contains vizkit plugins for types that are defined in this library. I think it is a rock policy to define those plugins if possible in the same repository the corresponding type is defined. This dependency is optional.

@Rauldg
Copy link
Contributor

Rauldg commented Sep 13, 2018

Integrating source code from other libraries instead of linking them just feels wrong..

I agree, but on the other hand, if you aim for using Envire in an environments which already has their own types (e.g. Mars, ROS ...) the workspace becomes confusing and large: The developer ends up with the base-types and the ones from the environment.

My favourite option right now is to integrate this in our repo as Rock-independent flavor just like it is, in a branch and updating it when needed for any use case.

@AlexanderFabisch
Copy link
Contributor

AlexanderFabisch commented Sep 19, 2018

Integrating source code from other libraries instead of linking them just feels wrong..

Maybe you have to change the way how you think about that. :)

I could give you a list of examples:

  • eigen includes parts of lapack here
  • sklearn includes parts of ATLAS here, joblib and six here
  • numpy includes lapack_lite here
  • scipy included all kind of optimization algorithms from other sources, for example, L-BFGS-B
  • ...

Including code from another project makes it easier to maintain and avoids complicated build mechanisms like autoproj. Does not apply here, but the most important reason why you sometimes should do this is that your continuous integration (which should be triggered with each commit in your main repository) notices when something is not compatible anymore. Otherwise you might, for example, not notice that someone changed the name of some field in TransformWithCovariance and EnviRe is unusable.

Also, for me as someone who might want to use EnviRe from time to time without autorpoj this seems to like a huge improvement.

@arneboe
Copy link
Contributor

arneboe commented Sep 19, 2018

With continuous integration you could just add the dependencies as git submodules and use cmake to build them in the correct order. That way you get the best of both worlds. The continuous integration catches incompatibilities caused by upstream changes, the cmake script avoids using more complex solutions like autoproj and the submodule ensures that the libraries do not diverge.

I think the main question is whether you want to invest the time to setup a good build system and dependency management with cmake or not :)

@AlexanderFabisch
Copy link
Contributor

This is going to be a little bit philosophical now, but git submodules are not perfect, either. I really tried to work with it, with the the result that I cloned the repositories manually instead of using submodules. It's just not in my standard workflow.

It's actually a question of whether you like the mono-repo or multi-repo approach. But if you like the multi-repo approach, you have to make sure that everyone who wants to use your code uses the same repository management tool like you (e.g. autoproj) or you end up supporting a messy hell of build systems.

@saarnold
Copy link
Member

Maybe you have to change the way how you think about that. :)

There are of course cases when embedding an external library makes sense, but it has to be handled with care. There are a lot of issues that can arise if this version of envire_core and for instance base-types happen to be linked together at some point down the road.

@AlexanderFabisch
Copy link
Contributor

AlexanderFabisch commented Sep 19, 2018

Maybe you have to change the way how you think about that. :)

There are of course cases when embedding an external library makes sense, but it has to be handled with care. There are a lot of issues that can arise if this version of envire_core and for instance base-types happen to be linked together at some point down the road.

It's not like the repository includes Eigen. Just Time, Vector3d, Quaterniond, Matrix6d, and TransformWithCovariance from base-types. Those could be put in a different namespace. We also don't need to include unused types like Vector4d, Vector6d, ... While we are at it - why don't we use Eigen's types directly instead of those typedefs? Is boost-serialization used in any other project? Would it maybe make sense to include it in EnviRe directly? boost-logging does not have to be there at all? Why is it used? Don't you use glog in EnviRe?

edit: btw, is the covariance from TransformWithCovariance used somewhere? Why don't we directly change the Transform type to contain just a timestamp, a position and a quaternion?

@AlexanderFabisch
Copy link
Contributor

Sorry for going completely off-topic now, but...

With continuous integration you could just add the dependencies as git submodules and use cmake to build them in the correct order.

I remember that I actually tried this for InFuse. I tried to build envire_core and some other envire modules with a top-level CMakeLists.txt. It did not work because if you usually build multiple repositories with separate CMakeLists.txts, you have to find packages with find_package or pkg-config. That usually requires that the dependencies are already installed. They cannot be installed, however, before the top-level CMakeLists.txt is processed. This problem cannot be solved (easily?).

@Rauldg
Copy link
Contributor

Rauldg commented Sep 20, 2018

Although the discussion is interesting... Can we take a decision and move on?

As far as I see the only proposals on the table are:

  1. To set this code as a branch rock-independent-flavor or autoproj-independent or base-types-independent, whatever.

  2. Not to do anything.

I choose 1): It doesn't imply much work and we can keep the code for the next Rock independent project easily at reach.

@AlexanderFabisch
Copy link
Contributor

Well, there is option 3, which means we could actually replace EnviRe core with a middleware-independent version, but in the end that will be the decision of the maintainers.

@Rauldg
Copy link
Contributor

Rauldg commented Sep 20, 2018

3 is also possible, but the changes will be non backward compatible, at least in terms of dependencies.

I guess that many of the current plugins assume features of Envire Core that would be lost if 3 is applied, right?

@pnarvor
Copy link
Contributor Author

pnarvor commented Sep 21, 2018

Hello @AlexanderFabisch. About the covariance,

btw, is the covariance from TransformWithCovariance used somewhere? Why don't we directly change the Transform type to contain just a timestamp, a position and a quaternion?

The uncertainty propagation through the graph is one of the two main reason (with the easy graph handling) we chose to use Envire. It is based on this article https://link.springer.com/article/10.1023%2FA%3A1007976002485?LI=true, which is a good reference.

Also, there are checks in the TransformWithCovariance type if the covariance as been properly set and ignore it if it is not the case, so it is transparent if you don't want to use it. I don't know what are your use case for Envire but what I can say is that the uncertainty handling is really a good feature.

btw : Since nearly all the graph related classes are templated maybe it would be easier to have two Transform types, with and without covariance. It shouldn't be too long to adapt the graph classes.

@planthaber
Copy link
Member

is there a requirement to use rock-types for compatibility reasons?

If not, we could use our own types for envire-core, starting with using Eigen types (own typedefs) and Time and Transform from base-types.

The only issue is that the interface is a bit more complicated for rock users.

@arneboe
Copy link
Contributor

arneboe commented Sep 27, 2018

btw : Since nearly all the graph related classes are templated maybe it would be easier to have two Transform types, with and without covariance. It shouldn't be too long to adapt the graph classes.

Since no covariance propagation is done when the cov is not set there is very little performance loss (not really measurable). Therefore I don't see a reason to create another graph just to use Transforms without cov.

@arneboe
Copy link
Contributor

arneboe commented Sep 27, 2018

is there a requirement to use rock-types for compatibility reasons?

Not that I am aware of.

If not, we could use our own types for envire-core, starting with using Eigen types (own typedefs) and Time and Transform from base-types.

I still think that a dependency on base-types is not a problem. But if there is a real use case where we cannot have the dependency I am fine with it. But in that case we should also get rid of the dependency on base-cmake, base-logging etc.

The only issue is that the interface is a bit more complicated for rock users.

We could add automatic copy constructors that are only enabled if we are compiling inside a rock environment. That way it would be transparent for rock users.

@chhtz
Copy link
Member

chhtz commented Sep 27, 2018

If not, we could use our own types for envire-core, starting with using Eigen types (own typedefs) and Time and Transform from base-types.

I still think that a dependency on base-types is not a problem. But if there is a real use case where we cannot have the dependency I am fine with it. But in that case we should also get rid of the dependency on base-cmake, base-logging etc.

One issue I would admit exist with base-types is that it must be made sure that, e.g., the dependency on SISL actually is optional. As it looks at the moment, the corresponding parts are getting compiled regardless of whether SISL exists. If it is possible to build base-types with just base-cmake (and Eigen and boost) and everything else really is optional, I would also prefer keeping the dependency as it is.

Having branches or forks of libraries will likely lead to hard-to-merge versions at some point, which will barely be maintainable.

@arneboe
Copy link
Contributor

arneboe commented Sep 27, 2018

One issue I would admit exist with base-types is that it must be made sure that, e.g., the dependency on SISL actually is optional. As it looks at the moment, the corresponding parts are getting compiled regardless of whether SISL exists.

Right now base-types does not build without SISL. However that is easy to fix. We could propose to merge this into the base-types master:
rock-core/base-types@master...arneboe:master

@saarnold
Copy link
Member

Right now base-types does not build without SISL. However that is easy to fix. We could propose to merge this into the base-types master:

Yes that would be a good way to go!

@arneboe
Copy link
Contributor

arneboe commented Oct 10, 2018

I have opened a PR on base-types to make the SISL dependency optional:
rock-core/base-types#121

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

7 participants