search

SteveMacenski / slam_toolbox

Slam Toolbox for lifelong mapping and localization in potentially massive maps with ROS
GNU Lesser General Public License v2.1
1.67k stars 525 forks source link

Multi-Robot SLAM in ROS 2 #727

Open Daniel-Meza opened 2 months ago

Daniel-Meza commented 2 months ago

Basic Info

Info Please fill out this column
Ticket(s) this addresses #76
Primary OS tested on Ubuntu 22.04
Robotic platform tested on Turtlebot3 Burger (hardware and simulation)

Description of contribution in a few bullet points

This is a ROS 2 Humble port of the ROS 1 implementation from #541, extending SLAM Toolbox to support multi-robot mapping.

Description of documentation updates required from your changes

Source code, launch and configuration files are all contained within dedicated multirobot directories.

Future work that may be required in bullet points

Any assistance/suggestions on these is appreciated!

SteveMacenski commented 2 months ago

Localization and lifelong mapping are pending. Currently, not a priority for me but would be good to have.

I wouldn't implement them. Localization is better suited on a per-robot basis anyway and the lifelong is experimental that should probably just be removed. I'd say this is done

Can you include a link back to the multirobot configuration file in your README section to point to an example?

I see both images/multirobot/pose_graph_colors.png and images/multirobot/pose_graph_colors_2.png, can the first be removed if its unused?

Motivational question before the review: Is there a reason these are duplicated files instead of having the couple hundred line changes inline in the main code? The previous PRs adding multirobot support didn't have that many changes to the core files. It looks like I wasn't able to merge them in because the authors either didn't respond or fix the items I needed fixed (or were on ROS 1 which I couldn't test)

I am honestly of 2 minds about it:

  1. I can more easily just merge this in with some quick testing since it is independent of existing capabilities so it won't break anyone, but also can't be used without knowing about the new nodes
  2. I can less easily merge this in because I can't see the few changes you made w.r.t. the original files, so I'll have to track down every line of the 2,400 line PR to validate it since I don't know what the few changes are.

I think I prefer it being a couple hundred line change in-line over the completely separated nodes, but I could be convinced otherwise...

Daniel-Meza commented 2 months ago

Can you include a link back to the multirobot configuration file in your README section to point to an example?

Done!

I see both images/multirobot/pose_graph_colors.png and images/multirobot/pose_graph_colors_2.png, can the first be removed if its unused?

Done!

As for your motivational question...

The main reason was robustness ansd safety. Since this work was a "learn as I go" task, I wanted to avoid breaking anything in the application while modifying the original files.

Additionally, keeping the multi-robot functionality separate, at least in my mind, maintains it as an extended feature rather than a core capability. Like lifelong mapping, this approach makes it trivial to update the original files when new commits are made.

Lastly, do you have any suggestions on how (and where) to handle this issue?

There is a poblem where robots identify each other as osbtacles. Especially when they're not moving upon first starting the mapping procedure.

Resolving this would be necessary for a proper multi-robot SLAM solution.

SteveMacenski commented 2 months ago

The main reason was robustness ansd safety. Since this work was a "learn as I go" task, I wanted to avoid breaking anything in the application while modifying the original files.

Would it be too much effort to have another branch where you make the few changes you made to these files in-line with the original software? It might help me make that judgement call if I think there's anything you've done that makes me think 'yeah, maybe its better to keep these separated'.

Additionally, keeping the multi-robot functionality separate, at least in my mind, maintains it as an extended feature rather than a core capability.

If its mostly allowing an array of information to stream in and fixing a few frames, I think its not much to have separated (and adds double the maintainer time to fix the multi-robot version when new features / bug fixes are added to the main software, since its duplicated).

There is a poblem where robots identify each other as osbtacles. Especially when they're not moving upon first starting the mapping procedure.

This is virtually a non-issue since other robots are no different than people, other vehicles, etc in a dynamic scene :-)

Daniel-Meza commented 2 months ago

Would it be too much effort to have another branch where you make the few changes you made to these files in-line with the original software? It might help me make that judgement call if I think there's anything you've done that makes me think 'yeah, maybe its better to keep these separated'.

I’ve created the new branch with the changes integrated into the original files. For now, I’ve commented out the compilation of the "lifelong mapping" and "localization" components, as they require additional adjustments to align with the updated source code. However, this should give you a clear overview of the modifications made for the multirobot functionality.

Should I open a separate pull request? Or is there a cleaner way to integrate the new branch into this thread?

SteveMacenski commented 2 months ago

Should I open a separate pull request? Or is there a cleaner way to integrate the new branch into this thread?

Lets start with just a link. I can look at the diff and see what seems like the best path forward

Daniel-Meza commented 2 months ago

There you go: https://github.com/UTNuclearRoboticsPublic/slam_toolbox/tree/humble_multirobot_v2

Amronos commented 1 month ago

Are there any updates on the status of this PR?

SteveMacenski commented 1 month ago

@Daniel-Meza sorry for the delay. ROSCon/ROS-I/Actuate conferences and talks have really delayed some of my reviews.

I think your diff looks good against the main servers. I have some review comments for changes, but largely that should sail through with a couple of iterations! I'll just test myself that the single robot case still works and if you can show the 2+ robot situation works, I'm happy with that

Daniel-Meza commented 1 month ago

No problem at all. Feel free to send over your comments and I'll incorporate them as best as I can.

Also, I don't feel qualified to speak on other multi-robot slam implementations so I'll limit my contributions to this work. However, I'm happy to make necessary adjustments to ensure those who have made further progress can contribute their work to this project.

SteveMacenski commented 2 weeks ago

Sorry for the delay, ROSCon, ROS-I and so forth has kept me busy as of late and returning back to normal.

@Daniel-Meza can you follow up on the comment thread in #592 I started on a meeting between yourself, me, and @acachathuranga to find a shared path forward here?

I like your work here and appreciate your time! This is all great work :-)

SteveMacenski commented 1 day ago

Notes from our conversation: