feat: adds min_pass_through and occupancy_threshold ros parameters

SteveMacenski / slam_toolbox

Slam Toolbox for lifelong mapping and localization in potentially massive maps with ROS

GNU Lesser General Public License v2.1

1.6k stars 513 forks source link

feat: adds min_pass_through and occupancy_threshold ros parameters #716

Open Morgantiz opened 3 months ago

Morgantiz commented 3 months ago

Basic Info

Info	Adds missing ros parameters min_pass_through and occupancy_threshold
Ticket(s) this addresses	#108 #448 #121 #124
Primary OS tested on	(Ubuntu)
Robotic platform tested on	Robotnik RbVogui

Description of contribution in a few bullet points

I added the possibility to set the min_pass_through and occupancy_threshold values through ros parameters. These are two variables of the OccupancyGrid class
I modified the inputs of the static CreateFromScans method to allow the two variables to be passed
To set the parameters from the slam toolbox node I added the two variables to the Mapper class

Description of documentation updates required from your changes

The default ROS config and documentation page must be modified to include also these two parameters

Future work that may be required in bullet points

Create a guide for tuning the two parameters
Tests on use cases

SteveMacenski commented 3 months ago

Are 2/0.1 the current set values?

Also, this needs to be targeted towards rolling ros2 branch not humble so that all new features are reflected into future distributions. Also, this might (?) invalidate existing serialized map files since you're adding new fields, so I don't think its wise to backport to Humble due to issues with deserializing existing maps in the distribution.

To get around that, you could add a version number to the files and check if that version is newer than N in an if condition in the serialization/deserialization file to know if it should attempt to do that part because it was made with a version that had that option.

Overall this looks good to have! Please update the README to contain the new parameters like the others described.

Morgantiz commented 3 months ago

Yes, the values were set at 2 and 0.1, see: https://github.com/SteveMacenski/slam_toolbox/blob/19a66904c648a174220be1a1b98e81a9ef0e6758/lib/karto_sdk/include/karto_sdk/Karto.h#L5920-L5921.

I could split the commit with two commits and make the serialization/deserialization happen only on the ros2 branch, instead the humble branch can use the parameter only for new maps. Is it not enough to remove lines 2411 and 2412 from this file? I don't know how the serialization/deserialization of the existing maps works, and I'm not sure where it takes place.

I am also trying to tune these parameters to get a more stable behavior in the case of people present and walking nearby during mapping.

SteveMacenski commented 3 months ago

Perhaps just comment out those parameters for serialization at all, leave them inline, but commented out with a note that we didn't want to break serialized format for general users and instead these values will be taken from the currently launched configuration files as they are 'live' modifying parameters.

I am also trying to tune these parameters to get a more stable behavior in the case of people present and walking nearby during mapping.

Let me know what params you find work! I'm not opposed to modifying the defaults or at least making a comment in the readme about some values that work for users in dynamic settings. Information is power.