envire / envire-envire_core

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

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

Open pnarvor opened 6 years ago

pnarvor commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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:

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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: https://github.com/rock-core/base-types/compare/master...arneboe:master

saarnold commented 6 years ago

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 commented 5 years ago

I have opened a PR on base-types to make the SISL dependency optional: https://github.com/rock-core/base-types/pull/121