search

mrpt-ros-pkg / mrpt_slam

ROS wrappers for SLAM algorithms in MRPT
http://wiki.ros.org/mrpt_slam
BSD 3-Clause "New" or "Revised" License
114 stars 49 forks source link

Feature/refactor mrpt rbpf slam #55

Closed Logrus closed 5 years ago

Logrus commented 5 years ago

Hi @jlblancoc,

I started some refactoring of the rbpf package with the goal to eventually improve other ros packages as well. I was thinking about making some design decision about which I'd like to consult with you and ask questions along the way.

So this PR is WIP and will evolve a bit.

One question is about #define for versioning. I have removed them in the package because I think the code is much cleaner this way. What do you think? Relevant commit: https://github.com/mrpt-ros-pkg/mrpt_slam/commit/f7b37382e1e6ee1cb7c949d12ab6242ead9169a8 I'd prefer to have a lockstep release process. For example mrpt 1.9 would correspond to let's say ros melodic release. But at the same time mrpt master corresponds to ros packages master and there's no direct backward compatibility.

jlblancoc commented 5 years ago

Hi @Logrus ,

So good to see you around again ! I'm really glad to see your proposal... there's so much on MRPT that could be exported to the ROS flavor of rbpf-slam, and so much code clean-up... any proposal in that line is always welcome.

jlblancoc commented 5 years ago

One question is about #define for versioning. I have removed them in the package because I think the code is much cleaner this way. What do you think?

Of course, I think it's cleaner! But... there's one detail: mrpt2 needs C++17, but no ROS version still "allows" its use... It can be used, but I'm not sure how severe are the "policy-keepers" if we ever try to release a ROS package that relies on mrpt2. See this: https://github.com/MRPT/mrpt/wiki/MRPT-ROS-packages

In any case, that only applies to released packages. As source code packages, we are free to use mrpt2 & C++17, of course.

So, in short: if you remove the #if's, the package will be not suitable for releasing as kinetic / melodic packages...

I just checked (here) and it seems that the next ROS1 release will be: Noetic Ninjemys (Noetic) in ~ May 2020. I hope that C++17 will be officially supported by ROS1 by then.

I can live without releasing any new version of the mrpt packages until them, so, I'm open to make the change of removing the #if's... anyone can just build the newer MRPT packages from sources anyway.

Logrus commented 5 years ago

Thanks for pointing out about incompatibility with C++ versions! I was not aware of that. I will put #ifs back if they are needed for release. :) At least since C++17 support is uncertain.

I didn't really had an experience with that, but the question then would be if we could break this dependency between language versions by compiling and releasing mrpt2 with C++17, and link ros packages, which for example would use C++11? Maybe using opaque pointers or something along the lines? I'd probably need to read how ros release process works to understand where we could draw the line and if it's possible at all. ... and if it's worth it.

Logrus commented 5 years ago

@jlblancoc Btw, you mentioned that you'd like to see more mrpt functionality exposed to ROS can you provide some hints what do you want to see? I definitely feel like I'm a bit out of touch with mrpt developments.

jlblancoc commented 5 years ago

I didn't really had an experience with that, but the question then would be if we could break this dependency between language versions by compiling and releasing mrpt2 with C++17, and link ros packages, which for example would use C++11? Maybe using opaque pointers or something along the lines?

I don't have that 100% clear, but it's related to libraries & compilers ABIs... I know of problems due to mixing libraries in C++98 vs C++11, but as long as the API of ROS remaing being C++N and our programs use >=N, it shouldn't be a big deal... at least, it has worked nice until now!

The ROS limitation is probably more focused in what we can expose as API, e.g. we shouldn't use C++17 features in a .h of a ROS node/library.

jlblancoc commented 5 years ago

Btw, you mentioned that you'd like to see more mrpt functionality exposed to ROS can you provide some hints what do you want to see? I definitely feel like I'm a bit out of touch with mrpt developments.

For example, with mrpt-rbpf one can build several gridmaps from different 2D lidars at different heights, or one can use simultaneously a pointcloud + a gridmap to build the likelihood functions used by the rbpf, or to mix different kinds of sensors/maps (e.g. range only beacons, 2d lidars, ...)

I think (since I haven't used this ros pkg for a while!) that many of these options were not available here...

Logrus commented 5 years ago

I exposed motion model parameters as parameters and also reworked parameter loading a bit. Since this PR gets too big, I guess it'd be a good idea to make a re-review and merge it. Any suggestion what else you'd like to see in this PR are welcome! Otherwise we can think about further steps after merging.

Logrus commented 5 years ago

@jlblancoc Please tell me if that's PR is OK to merge from your perspective. :) I added mention to make sure my last msg didn't get lost.

jlblancoc commented 5 years ago

LGTM! :+1:

PS: Any updates required for the ros wiki?

Logrus commented 5 years ago

Sure, thanks for the reminder! I will update wiki as well.