Skip to content

Conversation

@asoriano1
Copy link

@asoriano1 asoriano1 commented Jun 21, 2025


Basic Info

Info Please fill out this column
Ticket(s) this addresses N/A
Primary OS tested on Ubuntu 22.04
Robotic platform tested on gazebo simulation of Robotnik RB-VOGUI

Description of contribution in a few bullet points

  • Adds intensity data processing alongside occupancy in the SLAM pipeline.
  • Extends internal data structures to store per-cell intensity values alongside occupancy data.
  • Ensures full backward compatibility when intensity data is not available.
  • ROS2 service 'save_map' now saves the occupancy and the intensity map with same name but with "_intensity" sufix.

Description of documentation updates required from your changes

  • New parameter: min_intensity_counter (default: 2).
  • Parameter to select intensity range: intensity_min, intensity_max (default: 0, 255).
  • New param intensity_strategy for different strategies to store the intensity data on a cell ("mean", "max", "latest"). (default: "mean")
  • Optionally update launch/param examples and the README to reflect the new functionality.

Future work that may be required in bullet points

  • Add automated tests (unit/integration) to verify the correct behavior.
  • Validate behavior on different robot platforms and real-world datasets with variable intensity profiles because now the asumed range is from 0 to 255.
  • Define default values for well-known laserscans. As it is defined for minRange and maxRange.

@SteveMacenski
Copy link
Owner

@asoriano1 can you give us some metrics or examples of this in use for how much it improves localization and/or better addresses some situations that it previously didn't do well at?

Copy link
Owner

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some preliminary notes

#include <cstdlib>
#include "rclcpp/rclcpp.hpp"

namespace intensity_map_saver {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any difference here between this and the map saver? If not, this can be removed and we can just have 2x instances of the map_saver. One for the occ grid and another for the intensity map.

I think actually we should have them both run in the map_saver in 1 instance that way the maps are synchronized

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no difference between intensity_map_saver and map_saver but map_saver have not parametrize the name of the map, only the name of the topic. This means that if we use two instances of map_saver for different topics, we need to ensure that the output file names are different to avoid overwriting. When the service is called without specifying a map name, this is not an issue: two files are created (e.g., map.pgm and map_2.pgm).
However, if a file name is provided via the service, both savers would try to use the same name, which could lead to one overwriting the other. We could solve this by automatically adding a suffix based on the topic name (the only param we have), but then the final file name would no longer exactly match what the user specified in the service call.
Other option is to add a new parameter (e.g. bool) to MapSaver to specify if is_intensity or not. I'm not sure which approach is the least problematic.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think adding an argument for which to save could make sense, but I think the best is to save both in 1 instance of the map saver node. That way both are saved close to simultaneously. Then, we can just append _intensity (or whatever) to the service call for the intensity one.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done with 1 instance of the map saver node at 44a9829.
The service still receives just the "map_name" and it will save occupancy map as "map_name" and intensity map as "map_name_intensity".

@asoriano1 asoriano1 force-pushed the feature-intensities branch from b68f08f to 8d22542 Compare June 28, 2025 21:48
@SteveMacenski
Copy link
Owner

I jsut also noticed this is open against Humble. That's OK, but we will need one on ros2 too so that this can be enabled on all future distributions and backported to Jazzy as well.

* @param gridIndex index
* @param intensity intensity value (p.e. 0 a 255)
*/
inline void SetIntensity(kt_int32s gridIndex, kt_int8u intensity)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ... don't actually sett this or the GetIntensity function used anywhere. Should it be?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed. The method and the file. Now all is managed by karto lib

/*****************************************************************************/
{
// Create the occupancy grid from scans
karto::OccupancyGrid * occ_grid = karto::OccupancyGrid::CreateFromScans(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very, very inefficient to call twice (once in just getOccupancyGrid above). Remove this function and get the intensity grid in the same getOccupancyGrid function.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

solved at 1c8bb3a.
Now everything is processed at Karto lib.

// For each scan update the intensity grid
const auto & scans = mapper_->GetAllProcessedScans();
for (auto scan : scans) {
slam_toolbox::updateIntensityGridFromScan(scan, occ_grid, *intensity_grid, min_intensity_threshold_,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the occ grid creation process not be combined with the intensity grid creation process? Doing this separately after iterating through all the data is very inefficient

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

solved at 1c8bb3a.
Now everything is processed at Karto lib.

@SteveMacenski
Copy link
Owner

@asoriano1 let me know if / when you're ready for another review!

@asoriano1 asoriano1 marked this pull request as draft July 15, 2025 12:46
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.

2 participants