Repository navigation
Conversation
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.
|
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 configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe 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. ChangesPadded Self-Collision Checks
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Merge Risk: ⚪ Minimal · up to 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 ReviewSecurity architecture risk: 🔵 Low · up to 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 Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation 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.)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Description
#3088 added
CollisionRequest::pad_self_collisionsso 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_collisionsparameter (defaultfalse, so current behavior is unchanged):OMPL – new per-group parameter in
ompl_planning.yaml, read byStateValidityChecker. Every collision request it builds (simple, distance, cost, verbose) uses padded self-collision checking when the parameter is set:The key is removed in
ModelBasedPlanningContext::useConfig()so it is not forwarded to the OMPL planner parameters.CheckStartStateCollisionrequest adapter – newpad_self_collisionsparameter indefault_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 calledgetCollidingPairs(), which checks the scene's current state without padding.ValidateSolutionresponse adapter – newpad_self_collisionsparameter indefault_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:Checklist
Tests: added
testPaddedSelfCollisionfor Panda and Fanuc intest_state_validity_checker.cpp. It adds padding to a self-collision-free state and checks that the state is only rejected whenpad_self_collisionsis enabled. The test fails without this change.Summary by CodeRabbit