Add Build Map objective for hangar_sim - #931
Draft
bkanator wants to merge 2 commits into
Draft
Conversation
Epic #16487 promised operators could build maps through the MoveIt Pro Desktop App. What shipped instead was a single checked-in map a developer generated once through a CLI, and localization_launch.py pointed readers at slam:=True with no Objective to drive it from. Teleoperate to explore while slam_toolbox builds the grid, then save it on the operator's confirmation. Teleop mode 1 (JOINT_JOG) is what drives the base here, and the response timeout accounts for slam_toolbox shelling out to map_saver_cli. Refs #18066 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
CallSaveMapService ships in moveit_pro_slam_behavior rather than in the shared Behavior library, so a configuration has to opt in. Only configurations that build maps then depend on slam_toolbox. Refs #18066 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[written by AI]
needs: moveit_pro/#22585
Motivation
Closes #18066 (epic #16487).
Epic #16487 promised operators could build maps through the MoveIt Pro Desktop App — teleoperate to explore, click save when done. That never shipped. What ships instead is a single checked-in
hangar_map.pgmthat a developer generated once through a CLI, andlocalization_launch.pytells the reader to "Run SLAM first (slam:=True)" with no Objective to do it from.This is the operator-facing half: the Objective. It depends on the
CallSaveMapServiceBehavior added in moveit_pro/#22585, which must merge and reach the base image first.Brief description
Adds
src/hangar_sim/objectives/build_map.xml, a runnable "Build Map" Objective: teleoperate to explore the hangar while slam_toolbox builds the occupancy grid, then save it on the operator's confirmation. Objectives are auto-discovered from that folder.It also opts this configuration into the new SLAM Behaviors package:
moveit_pro::behaviors::SlamBehaviorsLoaderinconfig/config.yaml, and<exec_depend>moveit_pro_slam_behavior</exec_depend>inpackage.xml. Those two lines are what the split in moveit_pro/#22585 buys —CallSaveMapServicelives outside the shared Behavior library, so only configurations that actually build maps load it or depend on slam_toolbox. A manipulation-only configuration adds neither line and is unaffected.It assumes slam_toolbox is already running in mapping mode, which
robot_drivers_to_persist_sim.launch.pystarts withslam:=True(it includes nav2_bringup'sslam_launch.py). Theslam_toolboxblock already inparams/nav2_params.yamlsuppliesmode: mapping, andhangar_simalready declares<exec_depend>slam_toolbox</exec_depend>.On the tree shape: Sequence, not Parallel
The issue suggests "likely uses a
Parallelnode withsuccess_count=1to handle teleop + user interaction concurrency". I used a plainSequenceinstead, because the concurrency the Parallel would provide already exists inside the teleop subtree:DoTeleoperateActioncarriesenable_user_interactionanduser_interaction_prompton its own action goal, and holds that prompt open for as long as teleoperation is active. So the operator drives and confirms against one node:Continueends teleop with SUCCESS and the Sequence proceeds to save;Abortends it with FAILURE and no map is written.Building the same thing with
Parallelwould need a separate "wait for a button" Behavior, and no such Behavior exists — the user-interaction Behaviors areGetPoseFromUser,GetPointsFromUser,GetRegionFromUser, andGetTextFromUser, all of which demand a value the workflow does not need. The Sequence reaches the same operator experience with nothing new added. The tradeoff: the button readsContinuerather thanSave Map, so the prompt text carries the meaning.Teleop mode, and the controller question
initial_teleop_mode="1"(JOINT_JOG), not3. Mode 3 isMOVE_TO_WAYPOINT(TeleoperationMode.msg), which replays a pre-recorded joint state — useless for exploring space no waypoint covers, and on this config its branch passes the three base joints to a planned, collision-checked base motion whose planning scene has no hangar in it yet, because the map is what we are building.JOINT_JOGis what drives the base here: themanipulatorgroup carrieslinear_x_joint,linear_y_jointandrotational_yaw_joint(picknik_ur.srdf),joint_jog.yamlmaps that group tojoint_velocity_controller, and that controller'scommand_jointsareplatform_velocity_controller/<joint>reference interfaces.No
SwitchControllerprologue. I drafted one, on the reasoning that the navigate Objectives hand the wheels toplatform_velocity_controller_nav2and never hand them back, so a later jog would command a controller whose downstream was inactive. It turned out to be unnecessary:SwitchControllerdefaults bothautomatic_activationandautomatic_deactivationtotrue(switch_controller.cpp:228,234), and the former exists specifically to activate downstream chained controllers, so the subtree's own jog switch both activatesplatform_velocity_controllerand drops whatever held the wheel interfaces. Recording it because "why is there no controller setup here, when every neighbouring Objective has one" is a fair reviewer question.Map quality: two limitations the operator should know about
Neither is introduced by this PR and neither is fixed here, but both shape what this Objective produces, and the second changed the operator prompt:
use_scan_matching: falseanddo_loop_closing: falsein theslam_toolboxblock ofparams/nav2_params.yaml. With both off the map is the scan integrated along raw odometry, so a hangar-scale circuit will smear where the path re-crosses itself — andCallSaveMapServicereturns SUCCESS for a smeared map exactly as for a good one. These were disabled deliberately (commitc72308b0), so flipping them is a tuning decision, not a drive-by fix; #19530 is where realistic SLAM params belong.slam_toolbox.scan_topicis/scan_front_filteredwhile AMCL consumes/scan_merged. That is not a tuning choice:dual_laser_mergerlives inlocalization_launch.py, whichrobot_drivers_to_persist_sim.launch.py:243includes only undercondition=IfCondition(PythonExpression(["not ", slam])). Withslam:=Truethe merger never starts and/scan_mergeddoes not exist. Since each lidar covers ~230°, the rear arc is never ray-traced, so the saved map keeps unknown regions that are physically free — and AMCL then matches a 360° scan against it. The principled fix is to move the merger out of thenot slambranch and pointscan_topicat/scan_merged; that touches the localization launch path, so it is deliberately not in this PR. Until it lands, the operator prompt tells the operator to turn in place so every surface passes the front lidar.Map name
map_name="hangar_map"is a literal on the Behavior, matching how the neighbouring navigation Objectives hardcode their configuration. slam_toolbox resolves a relative name against its own working directory, so running this does not overwritemaps/hangar_map.pgmin the source tree; promoting a new map into the config package stays a deliberate step. The Objective comment says so, because that is exactly the surprise worth pre-empting.How it was tested
pre-commit run --files src/hangar_sim/objectives/build_map.xmlclean, including prettier and the Objective-favorites validator. XML parses.Manual verification still required — this Objective has not been run. It needs the paired Behavior in the base image, plus exclusive simulator access that was not available on this host. Specifically unverified:
JOINT_JOGlands the operator on a panel that actually drives the base on this config, and thatDoTeleoperateActionreportsjoint_velocity_controllerin itscontrollersoutput so the chain comes up;map_saver_cliand sleeps 1 s, so the budget is dominated by process start and DDS discovery rather than the raster);Worth pairing with a rebuild of
hangar_mapon the 811-beam lidar, which is what #19530 wants a home for — but that is follow-up, not this PR.Documentation follow-up
documentation-botfound no stale page — no public doc describes map building at all today — but flagged that an operator who seesBuild Mapin the Objectives panel has nothing to read. The natural home is a### Build Mapsection onsrc/docs/docs/how_to/mobile_navigation/nav2_mobile_navigation/nav2_mobile_navigation.mdxin themoveit_prorepo, covering theslam:=Trueprerequisite and the step of copying the saved map into the configuration package.I deliberately have not written it yet: the Objective has not been run once, and two of its parameters are explicitly unverified above. Documenting the workflow before it has been exercised would publish guidance nobody has followed. Once this is verified, that section should land in the same pass.
Release notes
Build MapObjective to the hangar simulation configuration. Teleoperate the robot to explore the environment while mapping runs, then confirm to save the occupancy grid map, without leaving the MoveIt Pro Desktop App. Requires the runtime to be launched withslam:=True.Claude agent checks
code-reviewerdocumentation-botBuild Mapnew-doc suggestion is recorded under "Documentation follow-up" abovelicensing-privacy-botplatform-architect-botroboticist-botinitial_teleop_mode="3"wasMOVE_TO_WAYPOINT; corrected to1(JOINT_JOG). Its controller-prologue finding was investigated and not applied - see abovefrontend-noah-botsecurity-auditormap_namehere is relative and was not a findingcompatibility-botsonar-bottest-runner