Panav/feat/update mppi visualization#5975
Panav/feat/update mppi visualization#5975Pana1v wants to merge 16 commits intoros-navigation:mainfrom
Conversation
|
This pull request is in conflict. Could you fix it @Pana1v? |
|
@Pana1v, your PR has failed to build. Please check CI outputs and resolve issues. |
588558a to
0f40963
Compare
…ions in utils.hpp for easier unit testing - Moved createTrajectoryMarkers and createFootprintMarkers from TrajectoryVisualizer class to inline free functions in utils.hpp - Renamed visualize_furthest_point to debug_visualizations in path critic plugins - Vectorized cost normalization using Eigen operations Signed-off-by: Pana <panav@users.noreply.github.com> Signed-off-by: Panav Arpit Raaj <praajarpit@gmail.com>
0f40963 to
f996cd4
Compare
|
Based on the CI results, it looks like this isn't compiling. If it is not ready for review yet, please convert it to a draft and ping us again once you have fully tested it locally :) |
| std::string frame_id_; | ||
| nav2::Publisher<visualization_msgs::msg::MarkerArray>::SharedPtr | ||
| trajectories_publisher_; | ||
| nav2::Publisher<nav_msgs::msg::Path>::SharedPtr transformed_path_pub_; |
There was a problem hiding this comment.
Note that this is removed from mppi, see migration guide here https://docs.nav2.org/migration/Kilted.html#centralize-path-handler-logic-in-controller-server
855dcda to
52a9346
Compare
|
@Pana1v how's this going? I'm going to need this soon again so let me know when it's ready for someone to test! |
Just sat down to work on this by coincidence, I've addressed most of the comments by Steve, was just setting up a local env for testing this out, if you could test it that would be even better! |
|
Yeah will give it a try |
|
By the way you have some failing tests: https://app.circleci.com/pipelines/github/ros-navigation/navigation2/18159/workflows/a1887e73-4858-4f97-8264-c7e04fd6daf6/jobs/54640 |
|
I'm considering replacing
What do you think? Would you add any extra metrics? I have my changes in a branch, would you mind if I commit directly on your branch? Another option is that I reopen #5643 |
Definitely more informative to have influence ratio, I was thinking of publishing weights as well, but it would get complicated since a critic such as for obstacle may have multiple weight fields. I've sent an invite to add you on my fork so that there's minimum friction in incorporating changes on your branch. |
Hello, I just want to add my two cents since I am working with MPPI and will find useful to have new debug information. In previous projects I found useful to publish the normalized cost of each critic. navigation2/nav2_mppi_controller/src/optimizer.cpp Lines 544 to 548 in fd8621c Publishing these vallues would allow to examine the actual values that are used to generate the optimal trajectory in that time step. Another thing I would like to say is to keep costs_sum because it is useful to check for the range of values of each critic, so during weight tuning it is possible to inspect these values and change the weights to avoid having critics that span values that are order of magnitude different. it is possible to add more fields to CriticStats.msg so we can have different kind of informations. |
Signed-off-by: Panav Arpit Raaj <praajarpit@gmail.com>
Signed-off-by: Panav Arpit Raaj <praajarpit@gmail.com>
…ppi-visualization
Signed-off-by: Panav Arpit Raaj <praajarpit@gmail.com>
Signed-off-by: Tony Najjar <tony.najjar@dexory.com>
Signed-off-by: Tony Najjar <tony.najjar@dexory.com>
There was a problem hiding this comment.
Pull request overview
This PR updates Nav2 MPPI controller visualization and critic introspection capabilities, including new per-trajectory LINE_STRIP visualization, per-critic cost breakdown support, and improved critic influence statistics.
Changes:
- Replace candidate trajectory visualization with cost-colored line markers and add optional per-critic cost visualizations.
- Update critics statistics message to publish discriminating power (
costs_std_dev) and normalized influence (influence_ratio) instead of summed costs. - Introduce/standardize a
Visualizationparameter namespace and add critic-leveldebug_visualizationstoggles for additional debug publishers.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| nav2_msgs/msg/CriticsStats.msg | Changes critic stats fields to costs_std_dev and influence_ratio. |
| nav2_mppi_controller/test/utils_test.cpp | Updates CriticData aggregate init to match new struct layout. |
| nav2_mppi_controller/test/trajectory_visualizer_tests.cpp | Adds tests for transformed path publishing and updates visualization API usage. |
| nav2_mppi_controller/test/critics_tests.cpp | Updates CriticData init and adds “debug visualizations enabled” coverage for path critics. |
| nav2_mppi_controller/test/critic_manager_test.cpp | Adds test for per-critic cost collection behavior. |
| nav2_mppi_controller/src/trajectory_visualizer.cpp | Implements new visualization toggles/publishers and cost-colored trajectory markers. |
| nav2_mppi_controller/src/critics/path_follow_critic.cpp | Adds debug_visualizations publisher for target pose visualization. |
| nav2_mppi_controller/src/critics/path_angle_critic.cpp | Adds debug_visualizations publisher for target pose visualization. |
| nav2_mppi_controller/src/critics/path_align_critic.cpp | Adds debug_visualizations publisher for furthest-point pose visualization. |
| nav2_mppi_controller/src/critics/obstacles_critic.cpp | Adds trajectory collision mask population for stats filtering. |
| nav2_mppi_controller/src/critics/cost_critic.cpp | Adds trajectory collision mask population for stats filtering. |
| nav2_mppi_controller/src/critic_manager.cpp | Adds per-critic cost diffs collection and new std-dev/influence stats computation. |
| nav2_mppi_controller/src/controller.cpp | Routes all visualization publishing through TrajectoryVisualizer::visualize(...). |
| nav2_mppi_controller/include/nav2_mppi_controller/tools/utils.hpp | Adds free functions for trajectory/footprint marker creation. |
| nav2_mppi_controller/include/nav2_mppi_controller/tools/trajectory_visualizer.hpp | Expands visualizer API to accept costs and additional publish options. |
| nav2_mppi_controller/include/nav2_mppi_controller/optimizer.hpp | Exposes costs and per-critic costs for visualization. |
| nav2_mppi_controller/include/nav2_mppi_controller/critics/path_follow_critic.hpp | Adds publisher members for debug visualization. |
| nav2_mppi_controller/include/nav2_mppi_controller/critics/path_angle_critic.hpp | Adds publisher members for debug visualization. |
| nav2_mppi_controller/include/nav2_mppi_controller/critics/path_align_critic.hpp | Adds publisher members for debug visualization. |
| nav2_mppi_controller/include/nav2_mppi_controller/critic_manager.hpp | Adds visualize_per_critic_costs_ flag. |
| nav2_mppi_controller/include/nav2_mppi_controller/critic_function.hpp | Stores a node clock pointer for timestamping debug messages. |
| nav2_mppi_controller/include/nav2_mppi_controller/critic_data.hpp | Adds individual_critics_cost and trajectory_collisions optionals. |
| nav2_mppi_controller/include/nav2_mppi_controller/controller.hpp | Removes legacy visualization-related members/methods from controller interface. |
| nav2_mppi_controller/README.md | Updates docs for new critic stats fields. |
| nav2_bringup/params/nav2_params.yaml | Updates MPPI params to new Visualization namespace and flags. |
You can also share your feedback on Copilot code review. Take the survey.
nav2_mppi_controller/include/nav2_mppi_controller/tools/trajectory_visualizer.hpp
Show resolved
Hide resolved
Signed-off-by: Tony Najjar <tony.najjar@dexory.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 4 comments.
You can also share your feedback on Copilot code review. Take the survey.
| // Visualize trajectories with total costs | ||
| if (publish_trajectories_with_total_cost_) { | ||
| add(candidate_trajectories, costs, {}, stamp); | ||
| } | ||
|
|
||
| // Visualize trajectories with individual critic costs | ||
| if (publish_trajectories_with_individual_cost_ && !critic_costs.empty()) { | ||
| add(candidate_trajectories, costs, critic_costs, stamp); | ||
| } |
There was a problem hiding this comment.
In visualize(), when both publish_trajectories_with_total_cost_ and publish_trajectories_with_individual_cost_ are enabled, add() is called twice and the second call will also re-add the "Total Costs" markers (since add() unconditionally adds total costs when publish_trajectories_with_total_cost_ is true). This will duplicate total-cost markers in the published MarkerArray. Consider calling add() only once when individual costs are available (passing critic_costs), or add a flag/branch so total-cost markers are generated exactly once per cycle.
There was a problem hiding this comment.
Addressed, combined both if statements, it is gated in the add function already.
| state.local_path_length = std::abs(state.pose.pose.position.x - goal.position.x); | ||
| *data.furthest_reached_path_point = 9; | ||
| critic.score(data); |
There was a problem hiding this comment.
furthest_reached_path_point is initialized as std::nullopt in this test, but the code dereferences it (*data.furthest_reached_path_point = 9;), which is undefined behavior and will crash. Assign the optional directly (e.g., data.furthest_reached_path_point = 9;) before scoring.
| state.pose.pose.position.x = 0.0; | ||
| *data.furthest_reached_path_point = 21; | ||
| for (int i = 0; i < 22; ++i) { |
There was a problem hiding this comment.
furthest_reached_path_point is initialized as std::nullopt in this test, but the code dereferences it (*data.furthest_reached_path_point = 21;), which is undefined behavior and will crash. Assign the optional directly (e.g., data.furthest_reached_path_point = 21;) before scoring.
nav2_mppi_controller/README.md
Outdated
| @@ -59,7 +59,7 @@ This process is then repeated a number of times and returns a converged solution | |||
| | retry_attempt_limit | int | Default 1. Number of attempts to find feasible trajectory on failure for soft-resets before reporting failure. | | |||
| | regenerate_noises | bool | Default false. Whether to regenerate noises each iteration or use single noise distribution computed on initialization and reset. Practically, this is found to work fine since the trajectories are being sampled stochastically from a normal distribution and reduces compute jittering at run-time due to thread wake-ups to resample normal distribution. | | |||
| | publish_optimal_trajectory | bool | Publishes the full optimal trajectory sequence each control iteration for downstream control systems, collision checkers, etc to have context beyond the next timestep. | | |||
| | publish_critics_stats | bool | Default false. Whether to publish statistics about each critic's performance. When enabled, publishes a `nav2_msgs::msg::CriticsStats` message containing critic names, whether they changed costs, and the sum of costs added by each critic. Useful for debugging and tuning critic behavior. | | |||
| | publish_critics_stats | bool | Default false. Whether to publish statistics about each critic's performance. When enabled, publishes a `nav2_msgs::msg::CriticsStats` message containing critic names, discriminating power (std dev), and influence ratios. Useful for debugging and tuning critic behavior. | | |||
There was a problem hiding this comment.
This parameter table still documents visualize, publish_optimal_trajectory, and the TrajectoryVisualizer: namespace as controller-level configuration, but the implementation and bringup params now use the unified <controller>.Visualization.* namespace (e.g., publish_optimal_trajectory_msg, publish_transformed_path, etc.) and the controller no longer reads visualize / publish_optimal_trajectory. Please update this section to reflect the new parameter names and namespace so users don't configure flags that are silently ignored.
There was a problem hiding this comment.
Updated Readme to reflect the new schema
…d remove unused parameters Signed-off-by: Tony Najjar <tony.najjar@dexory.com>
…rver visualization settings Signed-off-by: Tony Najjar <tony.najjar@dexory.com>
Signed-off-by: Tony Najjar <tony.najjar@dexory.com>
|
@tonynajjar can you review and let me know if you approve now? |
…e addition of candidate trajectories Signed-off-by: Panav Arpit Raaj <praajarpit@gmail.com>
|
@tonynajjar I tested it on my PC, might just be my rig but it was a little slow, there was some latency in the visualizations can you try and confirm for once if it works smoothly on yours ? |
…threshold The test set goal distance to 0.15m but PathFollowCritic::score() early-returns when local_path_length < threshold_to_consider (default 1.4m), so costs were always zero. Signed-off-by: Panav Arpit Raaj <praajarpit@gmail.com>
| EXPECT_NEAR(costs(1), 19.845, 0.01); | ||
| } | ||
|
|
||
| TEST(CriticTests, PathFollowCriticDebugVisualization) |
There was a problem hiding this comment.
Here and below: how do these test visualizations? There's not a subscriber to any of it nor is it testing the new fields recorded -- only the main costs. .
| node->declare_parameter( | ||
| "my_name.Visualization.publish_trajectories_with_total_cost", rclcpp::ParameterValue(true)); |
There was a problem hiding this comment.
Where are these results evaulated?
| temperature: 0.3 | ||
| gamma: 0.015 | ||
| motion_model: "DiffDrive" | ||
| visualize: false |
There was a problem hiding this comment.
I think as I mentioned to @tonynajjar I don't want there to be a ton of these publish_XYZ debug parameters. I think there should be 1. Visualize / debug or not. You then publish all or nothign. Its just too much to break out individually and you can always just check if the publisher has any subscriptions to pass population/publication if no one is listening to that one.
As such, the namespace, each of the publish_ can be removed.
publish_optimal_path/publish_optimal_trajectory_msg (which ever existed before, it looks like its been renamed or something) is not a debug topic, that should be removed back to the global namespace
| return std::min(upper_bound, std::max(input, lower_bound)); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
These additions are visualization-specific and I think can live in the vissualization class
| rclcpp::Logger logger_{rclcpp::get_logger("MPPIController")}; | ||
| std::shared_ptr<nav2_costmap_2d::Costmap2DROS> costmap_ros_; | ||
| std::shared_ptr<tf2_ros::Buffer> tf_buffer_; | ||
| nav2::Publisher<nav2_msgs::msg::Trajectory>::SharedPtr opt_traj_pub_; |
There was a problem hiding this comment.
Add this back, this is not debug.
| tf2::Quaternion quat; | ||
| quat.setRPY(0.0, 0.0, yaw); | ||
| msg->pose.orientation = tf2::toMsg(quat); |
There was a problem hiding this comment.
nav2 utils has a function for this
| * @brief Initialize a debug pose publisher for this critic | ||
| * @param topic Topic name suffix (e.g. "furthest_reached_path_point") | ||
| */ | ||
| void initDebugPosePublisher(const std::string & topic) |
There was a problem hiding this comment.
@tonynajjar what do you think? I don't love the idea of each critic having this internally, rather the responsibility of the manager / visualizer?
| if (critics_data_.individual_critics_cost) { | ||
| return *critics_data_.individual_critics_cost; | ||
| } | ||
| static const std::vector<std::pair<std::string, Eigen::ArrayXf>> empty; |
| data.trajectory_collisions = Eigen::Array<bool, Eigen::Dynamic, 1>(data.costs.rows()); | ||
| data.trajectory_collisions->setConstant(false); | ||
| } | ||
|
|
There was a problem hiding this comment.
@tonynajjar thoughts? I do not love the idea of exposing this into the critics. Is this something you really want? Architecturally it is bad design and will slow down the controller and prime for buggyness
|
Hey @Pana1v, after some reflection we believe my original implementation was a bit too intrusive so I started again from scratch with another approach and it is now merged: #6036 Thanks for your efforts pushing this forward though. If you would still like to contribute in this direction, the visualization of the |
Basic Info
Description of contribution in a few bullet points
visualize_furthest_pointparameter todebug_visualizationsso each critic has a single toggle for all its debug visualscreateTrajectoryMarkersandcreateFootprintMarkersfrom class methods to inline free functions inutils.hppfor easier unit testingminCoeff/maxCoeffand array ops) instead of manual loopsDescription of documentation updates required from your changes
visualize_furthest_point→debug_visualizationsfor PathAlignCritic, PathAngleCritic, PathFollowCriticDescription of how this change was tested
Future work that may be required in bullet points
createTrajectoryMarkersandcreateFootprintMarkersinutils.hpp