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

feat: adds min_pass_through and occupancy_threshold ros parameters #716

Open
wants to merge 2 commits into
base: humble
Choose a base branch
from

Conversation

Morgantiz
Copy link


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
Copy link
Owner

SteveMacenski commented Jul 1, 2024

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
Copy link
Author

Morgantiz commented Jul 1, 2024

Yes, the values were set at 2 and 0.1, see:

m_pMinPassThrough = new Parameter<kt_int32u>("MinPassThrough", 2);
m_pOccupancyThreshold = new Parameter<kt_double>("OccupancyThreshold", 0.1);
.

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
Copy link
Owner

SteveMacenski commented Jul 1, 2024

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.

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.

I approve, but I would like you to add these params to the yaml files so people can see them there to tweak. Otherwise, happy to merge if you open another PR for this on ros2 as well for all future distributions to have this work

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