Release 3 #1
Release 3 #1
Conversation
There was a problem hiding this comment.
Pull request overview
This PR represents "Release 3" of the cartesian planner, refactoring the package from a SLERP-based trajectory planner to a Spline-based implementation. The changes introduce new action/service interfaces for spline-based motion planning and add a raster scanning capability for surface inspection tasks.
Key changes:
- Replaced SLERP interpolation with cubic spline-based trajectory generation
- Added action server for executing spline-based motion plans
- Added service for raster scan pattern generation and execution
- Removed KDL-related dependencies in favor of direct tf_transformations usage
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
srv/PlanScanPath.srv |
New service definition for raster scan path planning with configurable scan parameters |
package.xml |
Updated package description and dependencies; removed KDL dependencies, added rosidl_default_runtime |
images/RasterPath.png |
Added visualization image for raster scan documentation |
cartesian_planner/spline_planner.py |
New spline-based planner implementation with action server and raster scan service |
cartesian_planner/slerp_planner.py |
Removed old SLERP-based implementation |
action/PlanSpline.action |
New action definition for spline-based trajectory execution with progress feedback |
README.md |
Updated documentation to reflect spline-based approach and raster scan feature |
CMakeLists.txt |
Updated build configuration for new interfaces and Python package installation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| height = request.height if request.height > 0 else 0.80 | ||
|
|
||
| start_pose = self._get_current_pose() | ||
| if start_pose is None or isinstance(start_pose, Exception): |
There was a problem hiding this comment.
The condition isinstance(start_pose, Exception) is incorrect. The _get_current_pose method returns either a Pose object or None, never an Exception. This check will always be False and should be removed.
| if start_pose is None or isinstance(start_pose, Exception): | |
| if start_pose is None: |
| return [] | ||
| spline = CubicSpline(t_knots, points, axis=0, bc_type="clamped") | ||
|
|
||
| spacing_along = max(spacing_along, 0.05) |
There was a problem hiding this comment.
The reassignment of spacing_along here is redundant since it was already constrained to a minimum of 0.05 at line 206. This duplicate constraint should be removed or the comment should clarify why it's needed here.
| spacing_along = max(spacing_along, 0.05) |
| response.message = "Raster execution failed" | ||
| return response | ||
|
|
||
| def _generate_raster(self, center: Pose, width: float, height: float, spacing_along: float, spacing_lines: float, orientation) -> List[Pose]: |
There was a problem hiding this comment.
The parameter name orientation is ambiguous without type hints. Consider renaming it to fixed_orientation to match its usage in the calling code and improve clarity.
| def _generate_raster(self, center: Pose, width: float, height: float, spacing_along: float, spacing_lines: float, orientation) -> List[Pose]: | |
| def _generate_raster(self, center: Pose, width: float, height: float, spacing_along: float, spacing_lines: float, fixed_orientation) -> List[Pose]: |
| goal_pos = np.array([goal.position.x, goal.position.y, goal.position.z], dtype=float) | ||
| translation_distance = float(np.linalg.norm(goal_pos - start_pos)) | ||
| if translation_distance < 1e-6: | ||
| return [], [] |
There was a problem hiding this comment.
The function returns a tuple of two empty lists when translation_distance is very small, but the caller expects only a single list of poses. This will cause the unpacking to fail. The return statement should be return [] instead of return [], [].
| return [], [] | |
| return [] |
| last = None | ||
| for t in t_samples: | ||
| pos = spline(t) | ||
| if last is not None and np.linalg.norm(pos - last) < spacing_along - 1e-4: |
There was a problem hiding this comment.
The spacing filtering condition may cause unexpected behavior. The check np.linalg.norm(pos - last) < spacing_along - 1e-4 uses spacing_along - 1e-4 as a threshold, but this could skip valid waypoints if the distance is exactly at or near spacing_along. The condition should likely be < spacing_along * 0.5 or similar to filter only truly redundant points, or the logic should be reconsidered.
| if last is not None and np.linalg.norm(pos - last) < spacing_along - 1e-4: | |
| if last is not None and np.linalg.norm(pos - last) < 0.5 * spacing_along: |
| try: | ||
| executor.spin() | ||
| except (KeyboardInterrupt, ExternalShutdownException): | ||
| pass |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| pass | |
| node.get_logger().info("Shutting down spline_planner executor due to external signal.") |
|
@ashleshp you haven't added any references for your math, which I asked you last time. And, why didn't you use existing implementations of raster scan? why did you made functions you souldn't control the robot directly. you should send the trajectory and send updates to existing ones. but the control should happen by the control node. raster_scan implementation seems semantically wrong in terms of "raster_scan" definition. you did |
|
2 participants
No description provided.