Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 66 additions & 0 deletions nav2_map_server/launch/map_server.launch.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
#!/usr/bin/env python3

# Copyright (c) 2020 Samsung Research Russia
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

import os

from ament_index_python.packages import get_package_share_directory
from launch import LaunchDescription
import launch_ros.actions


def generate_launch_description() -> LaunchDescription:
# Parameters
bringup_dir = get_package_share_directory('nav2_bringup')
map_yaml_file = os.path.join(bringup_dir, 'maps', 'depot.yaml')

lifecycle_nodes = ['map_server']
use_sim_time = True
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggest passing use_sim_time as a launch argument

autostart = True
topic_name = 'map'
frame_id = 'map'
service_introspection_mode = 'disabled'

start_map_server_cmd = launch_ros.actions.Node(
package='nav2_map_server',
executable='map_server',
name='map_server',
output='screen',
parameters=[
{'yaml_filename': map_yaml_file},
{'topic_name': topic_name},
{'frame_id': frame_id},
{'service_introspection_mode': service_introspection_mode},
],
)

start_lifecycle_manager_cmd = launch_ros.actions.Node(
package='nav2_lifecycle_manager',
executable='lifecycle_manager',
name='lifecycle_manager',
output='screen',
parameters=[
{'use_sim_time': use_sim_time},
{'autostart': autostart},
{'node_names': lifecycle_nodes},
],
)

ld = LaunchDescription()

ld.add_action(start_map_server_cmd)
ld.add_action(start_lifecycle_manager_cmd)

return ld
17 changes: 15 additions & 2 deletions nav2_map_server/src/map_io.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -261,12 +261,25 @@ void loadMapFromFile(
normalized = (1.0f - normalized.array()).matrix();
}

double occupied_thresh = load_parameters.occupied_thresh;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Remove and use load_parameters.occupied_thresh directly please - we do so lots of places in the code below so changing just the one instance below this isn't consistent

double free_thresh = load_parameters.free_thresh;

// For Trinary mode, the thresholds depend on the constants used when saving the map,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But isn't that supposed to be represented in the thresholds in the yaml file? That is kind of the point of the yaml file.

I don't know about occupied is always 1.0 and free is always 0.039. Where is that coming from?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am assuming the images used were saved by the map_saver, not by hand. If so, the image created can only have 3 pixel values (0 205 254), from here
When loading, the values are normalized and complemented to 1 (I think by Magick). So you can only have the 3 values in the comment.

Regarding thresholds: the yaml file now records the thresholds used when saving the file. Those still make sense if the map being saved has graduation and are used to threshold it. Only after the shareholding to Trinary (so when loading), they become meaningless.
Another option could be to overwrite the th thresholds when saving a trinary map. I like this idea less because if someone modifies the yaml file by hand it will break again. Also, specifying thresholds values when saving a map and the discovering different values in the yaml file would look like a bug.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think the assumption that it is always the map saver is sound, unfortunately. Others have their own map saving utilities so we should look at the yaml values stored (which should correspond properly to those 3 values, right?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right now the map_saver will save to yaml the thresholds used to threshold the probability map being saved.
These are not correct when loading Trinary map.
For example, if you use the default map_saver thresholds of free=0.25, occupied=0.65, it will save a nice map properly. But when loading later on, it won't work since the UNKNOWN value is 0.19 and doesn't lie between the 2 thresholds.

If that assumption is wrong, so maybe the other option is better? When saving with the map_saver, save the thresholds for loading instead of the thresholds for saving. Like I mentioned, it will break if someone change the file by hand and may also be a bit confusing, but imo still better than nothing.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is it not correct? Aren't these all user defined settings they can set to their needs in Nav2?

I'm not trying to be obtuse here, but I'm also very cautious about any changes to map server or AMCL since these are mega-legacy packages that have alot of annoying subtle implications so I prefer to not touch them or their behavior unless we have extremely good reason. I don't really understand the issue.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What I mean by not correct is:
Say you have a probability map. You want to threshold it to a Trinary map, so you choose 2 thresholds. These 2 thresholds really depend on the probability map (e.g. how it was created and how noisy it is). So it makes sense to let the user choose these thresholds (which he can now - all good).

Once the map is saved and you load it back, it won't load correctly - all the UNKNOWN values will become FREE. This is what I mean by "not correct".

// not on the thresholds from the yaml file
// occupied is always 1.0, free is always 0.00392157 ( = 1.0 - 254 / 255.0),
// and unknown is always 0.196078431 ( = 1.0 - 205 / 255.0)
// For Scale mode, the thresholds are set by the user in the yaml file
if (load_parameters.mode == MapMode::Trinary) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The behavior in Jazzy has the same issue. I was planning to fix it for Jazzy, even before the new Eigen PR

Copy link
Copy Markdown
Member

@SteveMacenski SteveMacenski May 12, 2025

Choose a reason for hiding this comment

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

Can you point to where this is in ROS 1 then? I'm looking for validation that this change is needed. I know we made some changes here from ROS 1 to ROS 2, so if its in ROS 1 and I can map that change as clearly missing in ROS 2, then that makes my approval path here easier :-)

Copy link
Copy Markdown
Contributor Author

@adivardi adivardi May 13, 2025

Choose a reason for hiding this comment

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

Loading: https://github.com/ros-planning/navigation/blob/noetic-devel/map_server/src/image_loader.cpp
Saving pixels: https://github.com/ros-planning/navigation/blob/noetic-devel/map_server/src/map_saver.cpp#L77-L82
Saving yaml file: https://github.com/ros-planning/navigation/blob/noetic-devel/map_server/src/map_saver.cpp#L96-L116

In ROS1 they use the 2nd option I mentioned, i.e. always saving occupied_thresh: 0.65, free_thresh: 0.196 regardless of the thresholds used to threshold the map (given by the user with --occ and --free

Copy link
Copy Markdown
Member

@SteveMacenski SteveMacenski May 14, 2025

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, but those are used to threshold the probability map. They are also used in ROS 1 here
But ROS1 doesn't save these to file, instead always saving 0.196 & 0.65.

occupied_thresh = 254 / 255.0;
free_thresh = 2 / 255.0;
}

// Compute binary occupancy masks
Eigen::Matrix<uint8_t, Eigen::Dynamic, Eigen::Dynamic, Eigen::RowMajor> occupied =
(normalized.array() >= load_parameters.occupied_thresh).cast<uint8_t>();
(normalized.array() >= occupied_thresh).cast<uint8_t>();

Eigen::Matrix<uint8_t, Eigen::Dynamic, Eigen::Dynamic, Eigen::RowMajor> free =
(normalized.array() <= load_parameters.free_thresh).cast<uint8_t>();
(normalized.array() <= free_thresh).cast<uint8_t>();

// Initialize occupancy grid with UNKNOWN values (-1)
result.setConstant(nav2_util::OCC_GRID_UNKNOWN);
Expand Down
Loading