I will try to implement the changes described above to complete the task. Since the current changes affect the entire project, I think it would be easier for the review to separate them as new PR referencing this one to keep everything a little cleaner. What do you think?

Please rebase / pull in main to fix CI. We've updated some cache keys and rebalanced our pipeline recently

On your open items: maybe we should invert these to be all snake_case then if we're having behavior tied this way. I don't like the idea of having topic names changed, especially to use CamelCase since that's an antipattern for topic naming.

We could force snake_case the plugin names for topic naming, but that seems like trying to make it harder for ourselves.

Codecov Report

✅ All modified and coverable lines are covered by tests.

... and 7 files with indirect coverage changes

I don't like the idea of having topic names changed, especially to use CamelCase

I agree with you and tried to avoid this during the change process. Currently, I see several ways how this can be achieved further:

1. Implement additional methods in the plugins themselves that will return snake_case names when creating topics.

As an example, I have seen how this has already been implemented in behavior_tree_navigator.hpp

There is used a virtual method:

And each plugin override it according to its name:

This way we can decouple the names from the configuration files and the names used for topics, action or services.
If you agree, I can begin implementation and I'll come back here if there are other pitfalls.

2. As you mentioned, we can actually bring all the names related to plugins to snake_case. And this will require reverting all current changes and changing other part of the plugin names that already had CamelCase style.
   On the positive side, this will help avoid potential side effects associated with topic naming now or in the future. And I think this is also a good option, since during testing I already encountered mixed service names:

/route_server/DynamicEdgesScorer/adjust_edges  
/route_server/ReroutingService/reroute
...
/fromLL

And I could fix them at the same time.

As you mentioned, we can actually bring all the names related to plugins to snake_case. And this will require reverting all current changes and changing other part of the plugin names that already had CamelCase style.  On the positive side, this will help avoid potential side effects associated with topic naming now or in the future. And I think this is also a good option, since during testing I already encountered mixed service names:

I think this is the right direction to go! :-)

I have prepared the new changes as discussed above, which include:

• Reverted changes from the previous commit. I have kept only a few comments and log messages.
• Changed style for the rest of plugin names from CamelCase to snake_case.
• Changed names of the services I mentioned above. The only service name that was not changed is fromLL, as the server is declared in the robot_localisation package, and we subscribe to it.

As before, the new changes were tested locally using colcon test and pre-commit (except ament_mypy as mentioned in one of the recent issues).

The only thing I want to note is that the names for critics (for MPPI and DWB controllers) also belong to plugins, but at the moment they cannot be changed since they are directly used as lookup_name for pluginlib::ClassLoader and must match the class names.

I also have a couple points to discuss. I know this doesn't apply to plugins, but the following names stand out too:

• polygons names for collision_monitor

    polygons: ["FootprintApproach"]
    FootprintApproach:

• TrajectoryVisualizer and AckermannConstraints for controller_server

      TrajectoryVisualizer:
        trajectory_step: 5
        time_step: 3
      trajectory_validator:
        plugin: "mppi::DefaultOptimalTrajectoryValidator"
        collision_lookahead_time: 2.0  
        consider_footprint: false
      AckermannConstraints:
        min_turning_r: 0.2

Would you also consider replacing them?

This pull request is in conflict. Could you fix it @AJedancov?

The only thing I want to note is that the names for critics (for MPPI and DWB controllers) also belong to plugins, but at the moment they cannot be changed since they are directly used as lookup_name for pluginlib::ClassLoader and must match the class names.

OK!

polygons names for collision_monitor

That's just a parameterized dynamic namespace, not a plugin. This can be changed though and not a problem.

TrajectoryVisualizer and AckermannConstraints for controller_server

These are simply nested namespaces for the parameters (no plugins, no dynamic sizing, etc), so I think these should just stay as-is.

Also, we need to update to snake_case in the docs / tutorials / configuration guides as well

Sure! I was planning to do that after this PR.

There's a few merge conflicts now that should be addressed

I've resolved them locally for now.

Overall, very good! 😄

Thanks for the review!

The last few commits include:

• Reverted changes from the comments above.
• Changed names for polygons.
• Changed observation_sources names in some tests to match the names in existing config files.

I think just the documentation updates I mentioned are left! Those are pretty important here since this is a big change that everyone's going to need to make.

I am currently working on this. And while changing the documentation, I noticed that there is a shapes parameter for vector_object_server. I suppose this can also be changed in a similar way to polygons?

Sure :-)

Anymore? 😆

Anymore? 😆

Who knows 😄 Maybe it's really better to leave it unmerged for now while I update the docs, in case anything else comes up.

I have opened PR ros-navigation/docs.nav2.org#854 for the documentation as you mentioned above, including information for migration guide. So if any improvements are needed, we can continue there.

Additionally, the latest commit here updates some names in the behavior tree configurations for tests so they match their BT node names.

@mini-1235 please sanity check that you don't see any more gaps :-)