add articulation affordance - #509
Conversation
Greptile SummaryThe PR adds articulation-backed affordances and a built-in action for planning knob-turn trajectories, together with a microwave tutorial, documentation, and tests. The vertical-axis grasp-frame limitation previously reported remains on the current head.
Confidence Score: 4/5The PR does not appear safe to merge because vertical turn axes can still abort trajectory compilation, including the configuration exercised by the new microwave workflow. The grasp-frame implementation explicitly raises when the transformed turn axis is parallel to world up, so valid articulation configurations can fail before motion planning. Files Needing Attention: embodichain/lab/sim/atomic_actions/affordance.py and scripts/tutorials/atomic_action/turn_knob.py
|
| Filename | Overview |
|---|---|
| embodichain/lab/sim/atomic_actions/affordance.py | Adds articulation-backed antipodal and turn affordances, but TurnAffordance still rejects vertical world-space turn axes and blocks affected plans. |
| embodichain/lab/sim/atomic_actions/primitives/turn_knob.py | Adds the six-segment knob-turn planner and registration contract; its planning path remains dependent on the unresolved vertical-axis grasp-frame construction. |
| scripts/tutorials/atomic_action/turn_knob.py | Adds the microwave demonstration, whose configured articulation axis can still reach the outstanding grasp-frame failure. |
| tests/sim/atomic_actions/test_affordance.py | Adds affordance coverage but codifies rejection of vertical axes rather than covering a valid fallback frame. |
Reviews (4): Last reviewed commit: "update" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return parser.parse_args() | ||
|
|
||
|
|
||
| def create_microwave(sim) -> Articulation: |
There was a problem hiding this comment.
The new create_microwave helper leaves sim untyped, while MicrowaveOven.__init__ declares data_root as str despite accepting None, giving type checkers and API consumers incomplete or inaccurate signatures.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/tutorials/atomic_action/turn_knob.py
Line: 78
Comment:
**Public API annotations are incomplete**
The new `create_microwave` helper leaves `sim` untyped, while `MicrowaveOven.__init__` declares `data_root` as `str` despite accepting `None`, giving type checkers and API consumers incomplete or inaccurate signatures.
**Context Used:** AGENTS.md ([source](https://github.com/dexforce/embodichain/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Pull request overview
This PR extends the atomic-actions subsystem with articulation-backed affordances and a new TurnKnob primitive, enabling deterministic knob-turn planning from an articulation link’s live pose/geometry and providing an end-to-end tutorial + docs/tests.
Changes:
- Added
TurnAffordance(articulation-link knob semantics) andTurnKnobatomic action (approach → reach → close → turn → open → retract). - Extended
AntipodalAffordanceto optionally resolve mesh/pose from an articulation link. - Added tests, docs, and a new tutorial script demonstrating knob turning on a microwave articulation.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/sim/atomic_actions/test_affordance.py | Adds coverage for articulation-backed antipodal affordances and TurnAffordance grasp-pose behavior. |
| tests/sim/atomic_actions/test_actions.py | Adds coverage for TurnKnob planning/segments and validates affordance-type requirements. |
| scripts/tutorials/atomic_action/tutorial_utils.py | Adds optional init_qpos support for tutorial robot setup. |
| scripts/tutorials/atomic_action/turn_knob.py | New tutorial demonstrating TurnKnob on a microwave articulation. |
| embodichain/lab/sim/atomic_actions/primitives/turn_knob.py | New TurnKnob primitive, goal, and options implementation. |
| embodichain/lab/sim/atomic_actions/primitives/init.py | Registers TurnKnob as a built-in primitive and exports symbols. |
| embodichain/lab/sim/atomic_actions/affordance.py | Adds TurnAffordance and extends AntipodalAffordance to support articulation-link geometry/pose resolution. |
| embodichain/lab/sim/atomic_actions/init.py | Re-exports TurnAffordance and TurnKnob* public API. |
| embodichain/data/assets/obj_assets.py | Adds a MicrowaveOven dataset helper entry for the tutorial asset. |
| docs/source/overview/sim/atomic_actions/index.md | Updates built-in action count reference (now 10). |
| docs/source/overview/sim/atomic_actions/builtin_actions.md | Documents TurnKnob contract and adds it to the built-in actions table. |
| docs/source/api_reference/embodichain/embodichain.lab.sim.atomic_actions.primitives.rst | Adds API reference entries for TurnKnob, TurnKnobGoal, TurnKnobOptions. |
Suppressed comments (2)
embodichain/lab/sim/atomic_actions/primitives/turn_knob.py:206
- TurnKnob allocates the output trajectory with self.n_envs rows, but fills it from context.last_qpos and hand_* tensors built with context.batch_size. This will error if those batch sizes ever diverge. Allocate using context.batch_size (or link_pose.shape[0]) so the tensor shapes are consistent within _plan.
full = torch.empty(
(self.n_envs, sum(lengths), self.robot_dof),
dtype=context.robot.qpos.dtype,
device=self.device,
)
full[:] = context.last_qpos.unsqueeze(1)
scripts/tutorials/atomic_action/tutorial_utils.py:759
- create_ur5_gripper_robot_cfg adds an init_qpos parameter but the docstring Args section doesn't document it, which makes the function contract unclear in the tutorial utilities.
init_qpos: Sequence[float] | None = None,
) -> RobotCfg:
"""Build a UR5 arm + DH_PGI_140_80 gripper robot configuration.
The arm is taken from :class:`~embodichain.lab.sim.robots.ur_robot.URRobotCfg`
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| y_axis = torch.tensor( | ||
| [0.0, 0.0, 1.0], dtype=torch.float32, device=device | ||
| ).expand_as(z_axis) | ||
| x_axis = torch.linalg.cross(y_axis, z_axis, dim=1) | ||
| if torch.any(torch.linalg.vector_norm(x_axis, dim=1) <= 1.0e-6): | ||
| raise ValueError( | ||
| "TurnAffordance turn axis must not be parallel to world (0, 0, 1)." | ||
| ) | ||
| x_axis = torch.nn.functional.normalize(x_axis, dim=1) |
| link_pose = affordance.get_link_pose().to( | ||
| device=self.device, dtype=torch.float32 | ||
| ) | ||
| if link_pose.shape != (self.n_envs, 4, 4): | ||
| raise ValueError( | ||
| "Articulation link pose must have shape " | ||
| f"({self.n_envs}, 4, 4), got {tuple(link_pose.shape)}." | ||
| ) |
| def add_ur5_gripper_robot( | ||
| sim: SimulationManager, | ||
| init_pos: Sequence[float] = (0.0, 0.0, 0.0), | ||
| init_qpos: Sequence[float] | None = None, | ||
| ) -> Robot: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (3)
scripts/tutorials/atomic_action/tutorial_utils.py:784
init_qposis accepted as an arbitrary sequence, but there is no validation that it matches the expected number of joints for this tutorial robot (arm + gripper). A length mismatch will fail later during robot initialization/reset with a harder-to-debug error.
qpos = (
[0.0, -1.57, 1.57, -1.57, -1.57, 0.0, 0.0, 0.0]
if init_qpos is None
else list(init_qpos)
)
scripts/tutorials/atomic_action/tutorial_utils.py:230
- The new
init_qposparameter is not mentioned in the function docstring, so it’s easy to miss that callers can override the default tutorial joint pose.
"""Add the standard UR5 plus PGI gripper tutorial robot.
scripts/tutorials/atomic_action/tutorial_utils.py:758
init_qposwas added to the signature, but the docstring description doesn’t mention what it does. Adding a short note here makes the new capability discoverable without scanning the whole function.
This issue also appears on line 780 of the same file.
"""Build a UR5 arm + DH_PGI_140_80 gripper robot configuration.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (2)
embodichain/lab/sim/atomic_actions/primitives/turn_knob.py:115
- TurnKnob mixes batch dimensions: hand joint targets are built with context.batch_size, but the rest of the planner (link_pose shape check, trajectory allocation) assumes self.n_envs from the bound robot. If PlanningContext.batch_size differs (env subset planning), this can trigger shape mismatches or incorrect broadcasting.
hand_open_qpos = end_effector.joint_positions(
OPEN_COMMAND,
n_envs=context.batch_size,
device=self.device,
dtype=context.robot.qpos.dtype,
)
embodichain/data/assets/obj_assets.py:257
- The new MicrowaveOven dataset block has formatting that deviates from the surrounding DataDescriptor pattern (closing paren on the same line, extra whitespace) and is likely to fail black/linters. It also removes the blank line separation between dataset classes.
data_descriptor = o3d.data.DataDescriptor(
os.path.join(EMBODICHAIN_DOWNLOAD_PREFIX, obj_assets, "MicrowaveOven.zip"),
"5c90aa6911b445811fc81d704d461057", )
prefix = type(self).__name__
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (2)
embodichain/lab/sim/atomic_actions/affordance.py:332
- TurnAffordance.get_grasp_pose constructs a rotation matrix with y_axis fixed to world up without re-orthogonalizing it against z_axis. If the provided turn_axis has any world-up component, y_axis will not be perpendicular to z_axis and the resulting pose will not be a valid orthonormal transform (can break downstream FK/IK / relative-rotation math). Project world-up onto the plane orthogonal to z_axis and normalize before computing x_axis.
y_axis = torch.tensor(
[0.0, 0.0, 1.0], dtype=torch.float32, device=device
).expand_as(z_axis)
x_axis = torch.linalg.cross(y_axis, z_axis, dim=1)
if torch.any(torch.linalg.vector_norm(x_axis, dim=1) <= 1.0e-6):
embodichain/data/assets/obj_assets.py:246
- MicrowaveOven dataset docstring points to "MicrowaveOven/microwave_oven.urdf", but this PR’s new TurnKnob tutorial uses "MicrowaveOven/microwave_oven_with_inertials.urdf". This mismatch is confusing for users trying to locate the correct asset path via get_data_path(). Consider updating the docstring to match the tutorial asset (or mention both URDFs if both are shipped).
class MicrowaveOven(EmbodiChainDataset):
"""get_data_path("MicrowaveOven/microwave_oven.urdf")"""
Description
Type of change
Checklist
black .command to format the code base.