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

Decouples ROS from the core LOAM algorithms. #66

Merged
merged 1 commit into from
Aug 21, 2018

Conversation

adishavit
Copy link
Contributor

@adishavit adishavit commented May 29, 2018

  • Maintains original API.
  • Core LOAM algorithm now only depend on PCL and Eigen.
  • Uses std::chrono instead of ROS Time (bit-exact change).
  • Fixes some minor bugs and inefficiencies.

It's far being as elegant as it could - my goal was to get it to build on Windows too so I could evaluate it in a non-ROS environment - which it does. So this is not the most that can/should be done.
Having more eyeballs on it would be a tremendous help. Especially with documenting the LOAM code flow and logic.

- Maintains original API.
- Core LOAM algorithm now only depend on PCL and Eigen.
- Uses std::chrono instead of ROS Time (bit-exact change).
@adishavit adishavit mentioned this pull request May 29, 2018
@YoshuaNava
Copy link

YoshuaNava commented May 29, 2018

Hey man,
This is great! Thank you for sharing

Before merging I would like to bring into the discussion the possibility of separating the implementation of loam from the ROS related stuff a bit more, into different folders. Something like this:

main folder
|   loam/
|   +-- include/
|   +--    +-- loam/
|   +--    +--  +-- Parameters.h
|   +--    +--  +-- ScanRegistrationBase.h
|   +--    +--  +-- LaserOdometryBase.h
|   +--    +--  +-- LaserMappingBase.h
|   +--    +--  +-- TransformMaintenanceBase.h
|   +--    +-- loam_velodyne/
|   +--    +--  +-- ScanRegistrationVelodyne.h
|   +--    +--  +-- LaserOdometryVelodyne.h
|   +--    +--  +-- LaserMappingVelodyne.h
|   +--    +--  +-- TransformMaintenanceVelodyne.h
|   +--    +-- loam_variant/
|   +--    +--  +-- ...
|   +--    +-- loam_utils/
|   +--    +--  +-- Angle.h
|   +--    +--  +-- ...
|   +-- src/
|   +--    +-- loam_velodyne/
|   +--    +--  +-- ...
|   +-- CMakeLists.txt
|   +-- package.xml
|   loam_ros
|   +-- config/
|   +--    +-- loam_velodyne/
|   +-- launch/
|   +-- include/
|   +--    +-- loam_ros/
|   +--    +--  +-- ...
|   +-- src/
|   +--    +-- loam_ros/
|   +--    +--  +-- ScanRegistrationROS.cpp
|   +--    +--  +-- LaserOdometryROS.cpp
|   +--    +--  +-- LaserMappingROS.cpp
|   +--    +--  +-- TransformMaintenanceROS.cpp
|   +-- CMakeLists.txt
|   +-- package.xml
|   README.md

My arguments in favor of a structure like that are:

  • It would be easier to integrate the other variants of LOAM at some point.
  • It would be much easier to understand the underpinnings of LOAM and improve the code if the transport layer is not in the way.
  • LOAM could be built and used with no ROS at all.
  • The parameters would not be hidden anymore. They would be changeable via yaml files.

I'm working towards that on my fork: https://github.com/YoshuaNava/loam/tree/detach_ros
But this week I'm very busy, so I won't be able to contribute much.

@adishavit
Copy link
Contributor Author

This is a good idea - I have no problem with setting that up.
My aim was to try and avoid any external changes to the project so that existing users will not need to change their code as all.
What you suggest only requires moving some files around and fixing a few CMakeLists.txt files with new libs and paths.

I would very much prefer for all of use to work on a single repo and not multiple forks - we are all duplicating work (in fact I had already decoupled ROS from the daobilige-su/loam_velodyne repo before finding this one, and had to redo everything again since this project since better maintained (or at least more active).

If there is consensus about proceeding, then we can make the updates and move forward.
@laboshinl, do you have any input on this?

It seems like this is most active project/repo and it would be a shame to not leverage the work of multiple people and letting them duplicate their effort instead.

@YoshuaNava
Copy link

Hey,
I totally support what you are saying about contributing to this repository instead of having multiple forks.

Maybe we can make a list of changes and start introducing them one by one.

@adishavit
Copy link
Contributor Author

My list has mainly:

  • separate ROS while maintaining backward complatibility
  • Bug fixes

Which is what I've done.
I would like to do more refactoring though.

In any case, I'd want feedback from @laboshinl.

@YoshuaNava: What do you have in mind?

@YoshuaNava
Copy link

Hi @adishavit,
Sorry for the delay. The work you did is great. I think it could be merged, and the refactoring you speak about could be introduced in pull requests after that.

What I have in mind is to put together the spinning lidar and velodyne variants of LOAM, so that they can be used and extended by the community.

@laboshinl, @StefanGlaser what do you think?

@StefanGlaser
Copy link
Contributor

Hi all together,
also sorry from my side for not replying to recent issues and pull requests. I'm just too busy working and finishing my master thesis.
Decoupling LOAM from ROS is certainly a good idea and also one of the top items on my TODO list. One might argue about the best way of doing it, but in the end this project is very much work in progress. So from my point of view, any contribution pushing the project forward is appreciated.

@YoshuaNava seems to be fine with your changes and wants to build upon them, so I don't see any reason to reject your pull request.

@laboshinl laboshinl merged commit a06980a into laboshinl:master Aug 21, 2018
matlabbe added a commit to introlab/rtabmap that referenced this pull request Aug 29, 2018
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

Successfully merging this pull request may close these issues.

4 participants