Skip to content

Add pad_self_collisions options to OMPL and planning adapters - #3911

Open
rdantasb wants to merge 2 commits into
moveit:mainfrom
rdantasb:feature/pad-self-collisions-in-planning
Open

rdantasb wants to merge 2 commits into
moveit:mainfrom
rdantasb:feature/pad-self-collisions-in-planning

Conversation

@rdantasb

@rdantasb rdantasb commented Oct 1, 2026 •

Copy link
Copy Markdown

Description

#3088 added CollisionRequest::pad_self_collisions so that self-collisions can be checked against the padded robot, but nothing in the planning pipeline sets it. As a result, robot padding (e.g. robot_description_planning.default_robot_padding) is only applied to robot–world checks while planning, and planners can still produce paths where links come arbitrarily close to each other.

I need this for my application: I have multiple robots in one scene (combined into a single robot model), so robot-to-robot collisions are self-collisions, and I need a safety distance between the robots while planning.

This PR adds an opt-in pad_self_collisions parameter (default false, so current behavior is unchanged):

OMPL – new per-group parameter in ompl_planning.yaml, read by StateValidityChecker. Every collision request it builds (simple, distance, cost, verbose) uses padded self-collision checking when the parameter is set:

panda_arm:
  pad_self_collisions: true

The key is removed in ModelBasedPlanningContext::useConfig() so it is not forwarded to the OMPL planner parameters.

CheckStartStateCollision request adapter – new pad_self_collisions parameter in default_request_adapter_parameters. Without it, a start state that is only in collision because of the padding would pass the adapter and then fail inside OMPL with a less helpful error. The contact report now reuses the same request on the actual start state. Previously it called getCollidingPairs(), which checks the scene's current state without padding.

ValidateSolution response adapter – new pad_self_collisions parameter in default_response_adapter_parameters. PlanningScene::isPathValid() always checks self-collisions without padding, so with this parameter enabled the adapter adds a padded self-collision pass over all waypoints and publishes the contacts it finds. This also covers planners that rely on this adapter for collision validation (e.g. Pilz).

Example for a pipeline namespace ompl:

ompl:
  pad_self_collisions: true      # adapters
  panda_arm:
    pad_self_collisions: true    # OMPL state validity checker

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

Tests: added testPaddedSelfCollision for Panda and Fanuc in test_state_validity_checker.cpp. It adds padding to a self-collision-free state and checks that the state is only rejected when pad_self_collisions is enabled. The test fails without this change.

Summary by CodeRabbit

  • New Features
    • Added an optional setting to include robot padding in self-collision checks during motion planning. When enabled, padded self-collisions can invalidate a start state or planned path; the setting is off by default.

Adds a per-group 'pad_self_collisions' parameter to ompl_planning.yaml. When enabled, the StateValidityChecker sets CollisionRequest::pad_self_collisions (introduced in moveit#3088) so self-collisions are checked against the padded robot during planning. Defaults to false to keep current behavior.
…teSolution adapters

Lets the start state and solution path checks use padded self-collision checking, consistent with the planner. CheckStartStateCollision now reports contacts for the requested start state using the same collision request.
@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 4957c0c7-7264-464d-8238-989aa678a766

📥 Commits

Reviewing files that changed from the base of the PR and between a9004a4 and 6539668.

📒 Files selected for processing (8)
  • moveit_planners/ompl/ompl_interface/src/detail/state_validity_checker.cpp
  • moveit_planners/ompl/ompl_interface/src/model_based_planning_context.cpp
  • moveit_planners/ompl/ompl_interface/src/ompl_interface.cpp
  • moveit_planners/ompl/ompl_interface/test/test_state_validity_checker.cpp
  • moveit_ros/planning/planning_request_adapter_plugins/res/default_request_adapter_params.yaml
  • moveit_ros/planning/planning_request_adapter_plugins/src/check_start_state_collision.cpp
  • moveit_ros/planning/planning_response_adapter_plugins/res/default_response_adapter_params.yaml
  • moveit_ros/planning/planning_response_adapter_plugins/src/validate_path.cpp

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

The change adds configurable padded self-collision checks to OMPL state validation, request-adapter start-state checks, and response-adapter solution-path validation. The setting defaults to disabled in the request and response adapters.

Changes

Padded Self-Collision Checks

Layer / File(s) Summary
OMPL configuration and state checks
moveit_planners/ompl/ompl_interface/src/ompl_interface.cpp, moveit_planners/ompl/ompl_interface/src/model_based_planning_context.cpp, moveit_planners/ompl/ompl_interface/src/detail/state_validity_checker.cpp, moveit_planners/ompl/ompl_interface/test/test_state_validity_checker.cpp
OMPL recognizes pad_self_collisions as a group setting and removes it before forwarding planner parameters. The state validity checker applies padding to its collision requests when the value is "1" or "true". Tests cover padded checks with Panda and Fanuc states.
Start-state collision checks
moveit_ros/planning/planning_request_adapter_plugins/res/default_request_adapter_params.yaml, moveit_ros/planning/planning_request_adapter_plugins/src/check_start_state_collision.cpp
The request adapter adds the setting with a default of false and applies it to the initial collision check. If a collision is found, it repeats the check with contact collection enabled and uses the returned contacts for the collision message.
Solution-path validation
moveit_ros/planning/planning_response_adapter_plugins/res/default_response_adapter_params.yaml, moveit_ros/planning/planning_response_adapter_plugins/src/validate_path.cpp
The response adapter adds the setting with a default of false. When enabled and the existing path check succeeds, it checks each waypoint for padded self-collisions and marks colliding waypoint indices invalid. Contact checks for invalid states use the configured setting.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Feature

Merge Risk: ⚪ Minimal · up to 65396

The change adds opt-in padded self-collision checks while preserving default behavior. No concrete merge-blocking risk is established; merge after normal checks pass.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 65396

The additional checks preserve existing validation and rejection controls. No introduced security vulnerability was established. Remaining uncertainty concerns consistent configuration and whether downstream consumers correctly honor failed-plan status.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The material effect is on motion-plan acceptance for configured robot groups. Collision requests retain their existing group scope and allowed-collision matrix, while changing which robot geometry is used for self-collision checks.

Trust Boundaries and Controls

  • observed — Detected padded collisions produce failed planning status. PlanningPipeline aborts the adapter chain on failure, and the normal plan-and-execute loop invokes execution only for SUCCESS, including across replanning and preemption handling.
  • observed — Failure status does not clear the trajectory payload: response serialization includes a nonempty trajectory independently of error_code, and pipeline progress is published before the failure check. This pre-existing contract requires consumers to honor status; it is not established as an introduced or worsened attack path.

Resilience and Maintainability Implications

  • observed — Start-state rejection and its contact explanation now use the same reconstructed request state and padding setting. Contact collection is bounded, and a diagnostic recheck does not turn the original collision result into success.

Hardening Proposals

  • proposed — For deployments relying on padded separation, define one explicit configuration policy covering planner, start-state, and solution validation, and require every trajectory consumer to reject failed-plan status. This is deployment hardening, not an observed PR vulnerability.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 6 files. (2 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding pad_self_collisions options to OMPL and planning adapters.
Full details: Docstring Coverage

Explanation

Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 6 files. (2 skipped: 2 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

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.

1 participant