rst-tu-dortmund / costmap_converter

A ros package that includes plugins and nodes to convert occupied costmap2d cells to primitive types
140 stars 107 forks source link

Breaks repo into several packages #4

Closed artivis closed 2 years ago

artivis commented 6 years ago

This PR breaks the repo into several packages and one metapackage costmap_converter.

costmap_converter

Is now a metapackage.

costmap_converter_msgs

Contains both ObstacleMsg and ObstacleArrayMsg.
Messages are thus defined in the namespace costmap_converter_msgs.

costmap_converter_core

Contains the base class BaseCostmapToPolygons and the node CostmapStandaloneConversion. It depends on costmap_converter_msgs.

costmap_converter_dynamic_obstacles

Contains the plugin CostmapToDynamicObstacles.

costmap_converter_DBScan

Contains the plugins CostmapToPolygonsDBSMCCH, CostmapToLinesDBSMCCH, CostmapToLinesDBSRANSAC and CostmapToPolygonsDBSConcaveHull.

Modification to the actual code where kept to a minimum :

fix #3

artivis commented 6 years ago

I could not test anything so far, I'll come back to you once done.

artivis commented 6 years ago

Fixed compilation.

artivis commented 6 years ago

This PR now compiles and runs :+1: (Ubuntu 14.04 + ROS Indigo).

croesmann commented 6 years ago

Hey @artivis, thank you for this pull request. I agree that a metapackage makes sense in order to separate the plugin interface from particular implementations and furthermore to keep messages separated from code at all. I think we cannot merge this into indigo, kinetic and lunar since these changes drastically change ABI and API compatibility. However, we could try to get this into melodic.

I am wondering, whether it would be sufficient to merge DBScan and dynamics obstacle into a single package, like _costmap_converterplugins, in order to reduce the number of packages to be published and maintained. In this way, we would separate the interface, messages, and the default plugin implementations from each other. What do you think?

artivis commented 6 years ago

Hi @croesmann, merging these changes in indigo/kinetic/lunar while letting the users know is definitely doable, bumping the package major version. Now whether to do it or not is up to you :+1:.

Regarding having DBScan & dynamic obstacle in two different packages, the main motivation here was that they are released under two (quite) different licenses. It is thus simpler to understand which license applies to which chunk of code... Again, this is up to you.

croesmann commented 6 years ago

I thought about this merge request during the last week. I would like to keep the current structure for indigo/kinetic/lunar since otherwise this would require updating all packages that depend on the costmap converter, e.g. the teb_local_planner and all its forks from different users (and also in all distros). We could merge this into melodic to keep things clear.

I suggest the following package structure of the new metapackage costmap_converter:

I understand your motivation regarding the licenses. But I would also like to avoid to have lots of packages with minor content. Instead, I could try to get rid of the dependency for the dynamic obstacles plugin.

If you agree, please rebase and update the merge request. Otherwise, please let me know your concerns.

artivis commented 6 years ago

Hi @croesmann, your suggestions sounds good to me. Even though removing the dependency may be a lot of work. Unfortunately I will not have time now to update this PR, but I suppose it can wait a bit as it is targeted for Melodic.