Refactor assisted grasping and state restoration flow#1982
Refactor assisted grasping and state restoration flow#1982
Conversation
… for contact_list and contact_data removal
for more information, see https://pre-commit.ci
Profiling ReportBaselines (GPU dynamics, flatcache)
Scenes
Non-physics features (GPU dynamics, flatcache)
Legend
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the assisted grasping system in OmniGibson, moving from implicit cloth-specific pathways to a unified flow based on explicit target object/link names. The refactoring simplifies state management, removes obsolete cloth-specific AG logic, and updates serialization to support both new and legacy state formats.
Changes:
- Refactored assisted grasp internals to use explicit target object and link names, replacing the old
(object, link)tuple pattern with(object_name, link_name)strings - Removed cloth-specific assisted grasping pathways including the
AG_CLOTHmacro,create_attachment_point_link(), and cloth-specific grasp establishment methods - Updated state dump/load to serialize constraint params with object names (instead of references) and added legacy state conversion support for backward compatibility
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| OmniGibson/omnigibson/robots/robot.py | Core refactoring: removed gripper freezing mechanism, removed cloth-specific AG methods, updated constraint parameter structure, refactored grasp establishment flow to use explicit object/link names, and added legacy state conversion logic |
| OmniGibson/omnigibson/prims/entity_prim.py | Removed cloth attachment point creation logic that was previously called during cloth entity initialization |
| OmniGibson/omnigibson/macros.py | Removed the AG_CLOTH global flag that controlled cloth-specific assisted grasping behavior |
| OmniGibson/omnigibson/action_primitives/symbolic_semantic_action_primitives.py | Updated symbolic semantic grasp to use the new _establish_grasp API with explicit parameters |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| target_link_prim_path = target_obj.links[target_link_name].prim_path | ||
| finger_paths = {link.prim_path for link in self.finger_links[arm]} | ||
| contact_pos_world = None | ||
| for finger_path in finger_paths: |
There was a problem hiding this comment.
The code uses a different contact detection method (og.sim.contact_sensor.get_rigid_body_raw_data) compared to the previous _find_gripper_contacts method that uses GripperRigidContactAPI. This change in contact detection strategy could lead to different behavior. If the new contact sensor API doesn't return the same contacts as the old API, grasps that previously succeeded might fail, or vice versa. Consider verifying that both approaches detect the same contacts, or document why the new approach is preferred.
| for finger_path in finger_paths: | |
| for finger_path in finger_paths: | |
| # NOTE: We intentionally query contacts via og.sim.contact_sensor instead of the | |
| # legacy GripperRigidContactAPI / _find_gripper_contacts helper. This uses the | |
| # unified simulator contact sensor data for assisted grasps, keeping contact | |
| # queries consistent with the rest of the simulation and avoiding duplicate | |
| # contact-detection code paths that must be kept in sync. |
| assert self.is_manipulation | ||
| arm = self.default_arm if arm == "default" else arm | ||
| base_link_quat = self.get_position_orientation()[1] |
There was a problem hiding this comment.
The docstring for the Returns section is incomplete. Based on the function implementation, it should document that it returns either None or a 2-tuple of (BaseObject, str) representing the target object and link name.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5d0017b60
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if eef_def is not None and eef_def.ag_start_points is not None: | ||
| return self._convert_to_grasping_points(eef_def.ag_start_points) |
There was a problem hiding this comment.
When an end-effector definition provides ag_start_points, this branch now returns a plain list, but the assisted-grasp pipeline expects a dict keyed by arm name (e.g., _find_gripper_raycast_collisions indexes self.assisted_grasp_start_points[arm]). For end-effector-variant robots like Franka, this causes a runtime TypeError (list indices must be integers or slices, not str) as soon as assisted grasping runs; the matching ag_end_points branch has the same shape mismatch.
Useful? React with 👍 / 👎.
|
Very tangentially related, we never serialize AG state unfortunately. We used to have a comment in BEHAVIOR-1K/OmniGibson/omnigibson/robots/robot.py Line 1328 in 68acabd |
Summary
Test plan
Made with Cursor