PlanningRequestAdapter helper method getParam

[DLu]

Description

The same logic for resolving parameter names, checking for the existence of the parameter and printing the results was invoked several times in the different PlanningRequestAdapters. This PR modularizes that logic and adds it for TOTG.

I admit that the getParam method parameters are not the cleanest. There's an argument that instead, node and parameter_namespace are made class members and there's a base implementation of initialize that sets them. However, you'd still have to pass the LOGGER around (or create another instance).

Checklist

• Required by CI: Code is auto formatted using clang-format
• Extend the tutorials / documentation reference
• Document API changes relevant to the user in the MIGRATION.md notes
• Create tests, which fail without this PR reference
• Include a screenshot if changing a GUI
• While waiting for someone to review your request, please help review another open pull request to support the maintainers

[henningkayser]

This is a good cleanup. We will at some point apply a repo-wide refactoring for declaring and loading all parameters. Wrapping the getters and checks in clean functions like this will help a lot with that.

[henningkayser]

By increasing sample_count to num_points the trajectory is filled with duplicate trajectory points with the same time stamps at the end which breaks CI (ros2_control isn't happy about that). The reason is that the computed timestamps in the loop below exceed the trajectory duration (see https://github.com/ros-planning/moveit2/pull/468/files#diff-227cef8a99e7eaf48f74355541c0f673279e65839065e5f29646319063377cdeR1008). Is it necessary for you to have at least num_points waypoints? If so, you would have to recompute resample_dt_ as parameterized.getDuration() / num_points to match the trajectory duration. But I think the better option would be to just specify a resampling step that works in general and remove this change.

[DLu]

But I think the better option would be to just specify a resampling step that works in general and remove this change.

Do you mean an entirely separate PlanningRequestAdapter that does the resampling? Because I agree, but don't know how to move forward with that in a way that doesn't break everyone's existing configurations.

[henningkayser]

No, I meant setting the resampling_dt parameter small enough so that you don't have to worry about trajectory waypoint count at all.

[codecov]

Codecov Report

Merging #468 (f4a1717) into main (8423af3) will decrease coverage by 0.02%.
The diff coverage is 85.72%.

@@            Coverage Diff             @@
##             main     #468      +/-   ##
==========================================
- Coverage   48.86%   48.84%   -0.01%     
==========================================
  Files         218      218              
  Lines       22987    22969      -18     
==========================================
- Hits        11230    11217      -13     
+ Misses      11757    11752       -5     

Impacted Files	Coverage Δ
...lanning_request_adapter/planning_request_adapter.h	0.00% <ø> (ø)
..._adapter_plugins/src/fix_start_state_collision.cpp	32.82% <66.67%> (-2.80%)	⬇️
..._plugins/src/add_time_optimal_parameterization.cpp	93.75% <100.00%> (+1.45%)	⬆️
...est_adapter_plugins/src/fix_start_state_bounds.cpp	40.00% <100.00%> (-3.03%)	⬇️
...quest_adapter_plugins/src/fix_workspace_bounds.cpp	95.00% <100.00%> (+3.70%)	⬆️
...e/collision_detection_fcl/src/collision_common.cpp	74.69% <0.00%> (-0.49%)	⬇️
...nning_scene_monitor/src/planning_scene_monitor.cpp	51.53% <0.00%> (-0.12%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 987bbd3...f4a1717. Read the comment docs.

[henningkayser]

@DLu Awesome, you broke the curse!