rm pick plane - #974
Conversation
| max_range: 1.5 | ||
| point_subsample: 1 | ||
| padding_offset: 0.1 | ||
| padding_offset: 0.02 |
There was a problem hiding this comment.
Could you add a short comment justifying this value? Going from 0.1 to 0.02 is a 5× reduction and without context someone is going to "fix" it back later.
| rclcpp::Client<frida_interfaces::srv::RemovePlane>::SharedPtr | ||
| remove_plane_client; |
There was a problem hiding this comment.
The RemovePlane cleanup is half-done here. The calls were removed from the pick flow, but:
- The remove_plane_client member declaration and create_client / wait_for_service(30s) are still in place.
There was a problem hiding this comment.
The remove_plane_client is still actively used in the place workflow via place_service_callback. The cleanup only removed it from the pick workflow. In the place flow, the plane is not added to MoveIt's planning scene as a collision object, the extracted plane cloud is passed directly to the heatmap service to compute the optimal placement point on the table surface, so the self-filtering concern doesn't apply here.
| this->remove_plane_client = | ||
| this->create_client<frida_interfaces::srv::RemovePlane>( | ||
| REMOVE_PLANE_SERVICE); |
| this->add_pick_primitives_client->wait_for_service( | ||
| std::chrono::seconds(30)); | ||
| RCLCPP_INFO(this->get_logger(), "Service add_pick_primitives ready"); | ||
| this->remove_plane_client->wait_for_service(std::chrono::seconds(30)); |
| sensor_msgs::msg::PointCloud2::SharedPtr cloud) { | ||
|
|
||
| auto req_extract_table = | ||
| std::make_shared<frida_interfaces::srv::RemovePlane::Request>(); |
There was a problem hiding this comment.
There is still a live RemovePlane call inside place_service
There was a problem hiding this comment.
If the goal of this PR is to deprecate the RemovePlane service, please finish removing it here too. If not, that is fine but note that the constructor will block the node for 30 s on startup whenever the RemovePlane service isn't running.
There was a problem hiding this comment.
Heads-up: check_feasibility was acting as a cheap belt-and-suspenders against grasps falling below the table. With it gone, the entire safety net is MoveIt + the octomap. If for any reason the octomap doesn't contain the table at the moment of planning (occlusion, late TF, dropped frames, sensor restart), we no longer have a fallback that rejects the grasp.
There was a problem hiding this comment.
Would it be worth keeping a minimal floor-guard based on base_link.z (or a fixed world-z threshold), independent of the plane primitive? It would cost almost nothing and cover the failure mode where the octomap is momentarily empty.
There was a problem hiding this comment.
This verification is not necessary because the octomap already detects collision objects, and both MoveIt and GPD reject collision grasps.
| self.get_logger().info(f"Plane object found: {obj.id}") | ||
| self.plane = obj | ||
|
|
||
| def attach_pick_object(self): |
There was a problem hiding this comment.
Previously this method dropped any collision object that sat below the detected plane (if self.plane is not None and self.object_in_plane(obj, self.plane)). With the plane gone, ghost objects from perception that show up below the table will now stay in the planning scene and could end up attached to the EEF.
Can you confirm that the perception side no longer produces those false positives? If it still does, we'll need an equivalent filter (e.g., absolute z threshold) before this is safe to merge.
There was a problem hiding this comment.
These false positives are no longer a problem
* rm plane * add comment * add padding scale
* rm plane * add comment * add padding scale
* rm plane * add comment * add padding scale
2 participants
Removed plane collision; the octomap is enough. Also reduced padding_offset to maintain octomap voxels around the objects.
WhatsApp.Video.2026-05-15.at.12.41.49.PM.mp4