Cleanup planning request adapter interface #2266
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| add_executable(moveit_list_planning_adapter_plugins src/list_planning_adapter_plugins.cpp) | ||
| ament_target_dependencies(moveit_list_planning_adapter_plugins | ||
| Boost | ||
sjahr marked this conversation as resolved.
Show resolved
Hide resolved
sjahr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| moveit_core | ||
| rclcpp | ||
| pluginlib | ||
| ) | ||
|
|
||
| install(TARGETS moveit_list_planning_adapter_plugins | ||
| DESTINATION lib/${PROJECT_NAME} | ||
| ) | ||
This file contains 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
This file contains 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
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| default_plan_request_adapter_parameters: | ||
sjahr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| path_tolerance: { | ||
| type: double, | ||
| description: "AddTimeOptimalParameterization: Tolerance per joint in which the time parameterization is allowed to deviate from the original path.", | ||
| default_value: 0.1, | ||
| } | ||
| resample_dt: { | ||
| type: double, | ||
| description: "AddTimeOptimalParameterization: Time steps between two waypoints of the parameterized trajectory. The trajectory is re-sampled from the original path.", | ||
sjahr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| default_value: 0.1, | ||
| } | ||
| min_angle_change: { | ||
sjahr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| type: double, | ||
| description: "AddTimeOptimalParameterization: ?", | ||
sjahr marked this conversation as resolved.
Show resolved
Hide resolved
sjahr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| default_value: 0.001, | ||
| } | ||
| start_state_max_dt: { | ||
| type: double, | ||
| description: "FixStartStateCollision/FixStartStateBounds: Maximum temporal distance of the fixed start state from the original state.", | ||
|
There was a problem hiding this comment. How is this temporal distance computed? As a user, it's now clear to me how to set this parameter because there is no context about how the trajectory to the fixed start state from the original state is derived. There was a problem hiding this comment. It is not computed but heuristically chosen: // heuristically decide a duration offset for the trajectory (induced by the additional point added as a
// prefix to the computed trajectory)
res.trajectory->setWayPointDurationFromPrevious(0, std::min(params.start_state_max_dt,
res.trajectory->getAverageSegmentDuration()));Looking at this it might be worth creating a follow-up PR to change this to something that respects the joint limits & take spatial distance into account 🤔 There was a problem hiding this comment. Ok, maybe there is more than the parameter description lets on but it probably belongs in a follow-up. |
||
| default_value: 0.5, | ||
| } | ||
| jiggle_fraction: { | ||
| type: double, | ||
| description: "FixStartStateCollision: ?", | ||
sjahr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| default_value: 0.02, | ||
| } | ||
| max_sampling_attempts: { | ||
| type: int, | ||
| description: "FixStartStateCollision: Maximum number of attempts to re-sample a valid start state.", | ||
| default_value: 100, | ||
| validation: { | ||
| bounds<>: [0, 255] | ||
|
There was a problem hiding this comment. Why the upper bound here? Maybe I want to sample all day... 😄 There was a problem hiding this comment. Good point, I've updated that. There was a problem hiding this comment. in that case, does the adapter take care of timeouts? If not, unlimited sampling wouldn't be too useful |
||
| } | ||
| } | ||
| start_state_max_bounds_error: { | ||
| type: double, | ||
| description: "FixStartStateBounds: ?", | ||
sjahr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| default_value: 0.05, | ||
| } | ||
| default_workspace_bounds: { | ||
| type: double, | ||
| description: "FixWorkspaceBounds: Default workspace bounds representing a cube around the robot's origin whose edge length this parameter defines.", | ||
| default_value: 10.0, | ||
| } | ||
sjahr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
This file contains 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
This file contains 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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern of passing the default
std::vector<std::size_t>& added_path_index = EMPTY_PATH_INDEX_VECTORisn't making sense to me. It is an out-parameter? If the user doesn't pass their own, does it not mutate the globalEMPTY_PATH_INDEX_VECTOR, which surely is not the intent?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also note that clang-tidy CI is complaining about the naming of this and failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My intend with this was to remove these functions:
As far as I understand there are no optional references, so like before we just write the path indices to a memory location we never read and dump them that way. Maybe another possible implementation would be to change this interface to use a pointer but that would be a bigger change. Do you have a smarter idea of how to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't you just use
std::vector<std::size_t>& added_path_index = {}?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@henningkayser C++ won't let you assign a default reference like that.
@sjahr I guess this pattern will technically work as long as
adaptAndPlanmakes sure to clear the "empty" vector before it starts every time. Otherwise it's a memory leak!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh of course not... content-wise, this parameter would fit into the MotionPlanResponse as well, or at least the detailed response. I wouldn't sweat it too much since we should take another close look at this interface anyway.