Skip to content

Windows fixes from robostack #5333

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

Open
wants to merge 16 commits into
base: jazzy
Choose a base branch
from
Open

Conversation

gftabor
Copy link

@gftabor gftabor commented Jul 6, 2025


Basic Info

Info Please fill out this column
Ticket(s) this addresses ()
Primary OS tested on ( Windows)
Robotic platform tested on (gazebo simulation of turtlebot)
Does this PR contain AI generated software? (No)

Description of contribution in a few bullet points

Fixes to numerous windows build errors. Robostack has a patch for every single nav2 package fixing it to build on windows. Those are documented here https://github.com/RoboStack/ros-jazzy/tree/main/patch . Predominately made by @traversaro . Actually, building Nav2 from source on Windows requires 15 separate patches being applied.
Other change was running replace all on uint -> unsigned int.

Description of documentation updates required from your changes

Description of how this change was tested

Builds on windows, Nav2 turtlebot demos run.


Future work that may be required in bullet points

For Maintainers:

  • Check that any new parameters added are updated in docs.nav2.org
  • Check that any significant change is added to the migration guide
  • Check that any new features OR changes to existing behaviors are reflected in the tuning guide
  • Check that any new functions have Doxygen added
  • Check that any new features have test coverage
  • Check that any new plugins is added to the plugins page
  • If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists
  • Should this be backported to current distributions? If so, tag with backport-*.

Copy link
Contributor

mergify bot commented Jul 6, 2025

@gftabor, all pull requests must be targeted towards the main development branch.
Once merged into main, it is possible to backport to @jazzy, but it must be in main
to have these changes reflected into new distributions.

Copy link
Member

@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.

Generally looks good, but there are a few places that need adjustments to be accepted

Copy link
Member

@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 think remaining are the 2x TODOs, the ODE, Robinhood, and updating the realtime prioritization exception text

@gftabor
Copy link
Author

gftabor commented Jul 11, 2025

Seems like the TODOs were not the correct solution to the core problem. There were 2 declared but never defined slots in rviz_plugins. Robostack got around it by just defining 2 blank functions. #4974 deleted the declaration which seems like a better solution. It was just never backported to jazzy.

Now it should be linting fixes.

Copy link

codecov bot commented Jul 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines Coverage Δ
...her/include/nav2_constrained_smoother/smoother.hpp 95.93% <100.00%> (ø)
...v2_constrained_smoother/smoother_cost_function.hpp 100.00% <ø> (ø)
nav2_map_server/src/map_io.cpp 92.51% <ø> (ø)
..._smac_planner/include/nav2_smac_planner/a_star.hpp 50.00% <ø> (ø)
...smac_planner/include/nav2_smac_planner/node_2d.hpp 100.00% <ø> (ø)
nav2_util/include/nav2_util/costmap.hpp 100.00% <ø> (ø)
nav2_util/src/node_utils.cpp 82.14% <ø> (ø)
...v2_waypoint_follower/plugins/photo_at_waypoint.cpp 91.52% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@@ -37,6 +36,13 @@
#include "nav2_smac_planner/types.hpp"
#include "nav2_smac_planner/constants.hpp"

#ifdef _MSC_VER
#define SELECTED_UNORDERED_MAP std::unordered_map
Copy link
Member

@SteveMacenski SteveMacenski Jul 11, 2025

Choose a reason for hiding this comment

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

This is significantly slower than the robin hood hashing that we use in the thirdparty directory. Would a major reduction in performance for windows users really be preferable? I think the print out you showed is mostly warnings which can probably be silenced. We have it in the Nav2 codebase so that can be modified and resolved, no?

@SteveMacenski
Copy link
Member

Also make sure to sign off with DCO

gftabor added 16 commits July 11, 2025 15:53
amcl, behavior_tree, nav2_common, nav2_constrained_smoother, nav2_map_server, nav2_msgs,
Signed-off-by: Griffin Tabor <[email protected]>
replace all uint as unsigned int
Signed-off-by: Griffin Tabor <[email protected]>
nav2_behavior_tree, opennav_docking, opennav_docking_bt, nav2_map_server, nav2_mppi_controller, nav2_smac_planner, nav2_waypoint_follder
Signed-off-by: Griffin Tabor <[email protected]>
Signed-off-by: Griffin Tabor <[email protected]>
Signed-off-by: Griffin Tabor <[email protected]>
Signed-off-by: Griffin Tabor <[email protected]>
Signed-off-by: Griffin Tabor <[email protected]>
remove unimplemented slots from rviz plugins
Signed-off-by: Griffin Tabor <[email protected]>
Signed-off-by: Griffin Tabor <[email protected]>
Signed-off-by: Griffin Tabor <[email protected]>
Signed-off-by: Griffin Tabor <[email protected]>
Signed-off-by: Griffin Tabor <[email protected]>
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.

3 participants