Uh oh!
There was an error while loading. Please reload this page.
add articulation affordance - #509
Merged
Merged
Conversation
Greptile SummaryThe PR adds open-loop press, slide, and twist affordances and atomic-action primitives, together with Cartesian trajectory support, tutorials, documentation, assets, and focused tests.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| embodichain/lab/sim/atomic_actions/affordance.py | Adds validated press, slide, and twist affordances with adaptive right-handed frame construction. |
| embodichain/lab/sim/atomic_actions/primitives/press.py | Expands press planning into approach, contact, penetration, and retract phases. |
| embodichain/lab/sim/atomic_actions/primitives/slide.py | Adds an open-loop linear articulation interaction primitive. |
| embodichain/lab/sim/atomic_actions/primitives/twist.py | Adds an open-loop rotational interaction primitive around a configured three-dimensional axis. |
| embodichain/lab/sim/planners/motion_generator.py | Adds support for preserving dense Cartesian samples through motion generation. |
| tests/sim/atomic_actions/test_affordance.py | Covers affordance validation and finite, orthonormal frames for vertical and oblique interaction axes. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
G[Press, Slide, or Twist goal] --> P[Resolve target pose and affordance]
P --> F[Construct interaction frame]
F --> K[Generate Cartesian keyframes]
K --> IK[Solve sampled poses with IK]
IK --> T[Assemble timed trajectory]
T --> E[Execute open-loop motion]
Reviews (17): Last reviewed commit: "add docs" | Re-trigger Greptile
Uh oh!
There was an error while loading. Please reload this page.
| 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!
Contributor
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.
Comment on lines
+328
to
+336
| 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) |
Comment on lines
+123
to
+130
| 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)}." | ||
| ) |
Comment on lines
224
to
228
| def add_ur5_gripper_robot( | ||
| sim: SimulationManager, | ||
| init_pos: Sequence[float] = (0.0, 0.0, 0.0), | ||
| init_qpos: Sequence[float] | None = None, | ||
| ) -> Robot: |
Contributor
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.
Contributor
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__
Contributor
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")"""
Uh oh!
There was an error while loading. Please reload this page.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
docs/source/api_reference/embodichain/embodichain.lab.sim.atomic_actions.rst:75
- The autosummary list was updated for
PressButton*, but it still omits the newTurnKnob*symbols andTurnAffordance, even though they are exported fromembodichain.lab.sim.atomic_actions. This makes the generated API reference incomplete.
PressGoal
PressButtonGoal
PressButtonOptions
PressButtonAffordance
CoordinatedPickGoal
embodichain/data/assets/obj_assets.py:246
- The
MicrowaveOvendataset docstring example path is inconsistent with the new tutorials (which referencemicrowave_oven_with_inertials.urdf). Updating this example avoids confusion about which asset path is intended.
class MicrowaveOven(EmbodiChainDataset):
"""get_data_path("MicrowaveOven/microwave_oven.urdf")"""
| angle_b = get_relative_rotation( | ||
| reference_xpos[:, :3, :3], symmetric_xpos[:, :3, :3] | ||
| ) | ||
| target_xpos = torch.where(angle_a < angle_b, target_xpos, symmetric_xpos) |
| angle_b = get_relative_rotation( | ||
| reference_xpos[:, :3, :3], symmetric_xpos[:, :3, :3] | ||
| ) | ||
| target_xpos = torch.where(angle_a < angle_b, target_xpos, symmetric_xpos) |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (4)
embodichain/lab/sim/atomic_actions/affordance.py:336
- TurnAffordance.get_grasp_pose currently sets y_axis to world up and sets z_axis from the turn axis, but it never re-orthogonalizes y_axis against z_axis. Unless z_axis is exactly perpendicular to world up, the resulting rotation is not orthonormal (y·z != 0), which can break downstream IK / motion generation that assumes valid rotation matrices.
Compute y_axis as cross(z_axis, x_axis) after constructing x_axis from a world-up reference vector.
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)
embodichain/lab/sim/atomic_actions/affordance.py:310
- The TurnAffordance.get_grasp_pose docstring states that the pose y-axis is fixed to world (0, 0, 1), but the implementation needs to (and should) orthonormalize the frame for arbitrary turn axes. The docstring should describe that world-up is used as a reference to build an orthonormal basis rather than being kept as an exact axis.
The pose z-axis follows :attr:`turn_axis` transformed into the world
frame, while its y-axis is fixed to world ``(0, 0, 1)``.
docs/source/api_reference/embodichain/embodichain.lab.sim.atomic_actions.rst:75
- The atomic actions API reference autosummary was updated for PressButton, but it doesn’t list the newly added TurnKnob / TurnAffordance symbols. This makes the public API docs incomplete/inconsistent with the new built-ins exported from embodichain.lab.sim.atomic_actions.
PressGoal
PressButtonGoal
PressButtonOptions
PressButtonAffordance
CoordinatedPickGoal
embodichain/lab/sim/atomic_actions/affordance.py:450
- PressButtonAffordance.get_press_pose sets y_axis to world up but doesn’t enforce orthogonality between y_axis and the computed z_axis. For press axes that aren’t perpendicular to world up, this produces a non-orthonormal rotation matrix (invalid transform).
Derive y_axis from cross(z_axis, x_axis) after constructing x_axis from the world-up reference vector.
z_axis = torch.matmul(link_pose[:, :3, :3], press_axis)
z_axis = torch.nn.functional.normalize(z_axis, dim=1)
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):
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 36 changed files in this pull request and generated no new comments.
Suppressed comments (3)
scripts/tutorials/atomic_action/tutorial_utils.py:69
GRIPPER_TCP_Zis no longer defined in this module, but it is still listed in__all__. That makesfrom tutorial_utils import *(and any docs/autodoc relying on__all__) fail to export the name, and breaks backwards compatibility for callers that referencedtutorial_utils.GRIPPER_TCP_Z. Consider re-introducing it as a constant alias for the default TCP offset (or remove it from__all__if intentional).
GRIPPER_URDF_PATH = "DH_PGI_140_80/DH_PGI_140_80.urdf"
GRIPPER_HAND_JOINT_PATTERN = "gripper_finger1_joint_1"
GRIPPER_MAX_OPEN_WIDTH = 0.100
GRIPPER_MIN_OPEN_WIDTH = 0.003
GRIPPER_FINGER_LENGTH = 0.10
docs/source/tutorial/atomic_actions.rst:106
- The PR description says the new tutorials are
turn_knob.py,press_button.py, andpull_push_articulated_part.py, but the changes/documentation addtwist.pyandslide.py(and updatepress.py). Please update the PR description (or provide wrapper scripts with the described filenames) so the documented CLI entry points match what’s actually added.
* ``press.py``
* ``slide.py``
* ``twist.py``
scripts/benchmark/atomic_action/run_benchmark.py:59
- The aggregate benchmark CLI no longer supports
--action press(and the default action changed frompresstomove_end_effector). If users/scripts depended on the old benchmark action list, this is a breaking CLI change; consider either keeping a deprecatedpressentry (even if it just prints a clear removal message), or documenting this change explicitly in the PR description/release notes.
ACTION_MODULES = {
"move_end_effector": "scripts.benchmark.atomic_action.move_end_effector_benchmark",
"move_joints": "scripts.benchmark.atomic_action.move_joints_benchmark",
"pick_up": "scripts.benchmark.atomic_action.pickup_benchmark",
"move_held_object": "scripts.benchmark.atomic_action.move_held_object_benchmark",
"place": "scripts.benchmark.atomic_action.place_benchmark",
}
DEFAULT_ACTIONS = tuple(ACTION_MODULES.keys())
MESH_OBJECT_ACTIONS = {"pick_up", "move_held_object", "place"}
MESH_OBJECT_TYPES = {*MESH_OBJECT_PRESETS.keys(), "all"}
def add_benchmark_args(parser: argparse.ArgumentParser) -> None:
"""Add atomic-action aggregate benchmark CLI arguments."""
parser.add_argument(
"--action",
nargs="+",
choices=(*ACTION_MODULES.keys(), "all"),
default=["move_end_effector"],
help="Atomic action benchmark(s) to run. Use 'all' for every action.",
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 35 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
scripts/tutorials/atomic_action/tutorial_utils.py:84
GRIPPER_TCP_Zis referenced in_GRIPPER_TCP(and exported via__all__) but the constant definition was removed, so importing this module will raiseNameErrorand break all tutorials/benchmarks that importtutorial_utils(including the new import regression test). Reintroduce the constant or refactor_GRIPPER_TCPto avoid the undefined name.
_GRIPPER_TCP = (
(1.0, 0.0, 0.0, 0.0),
(0.0, 1.0, 0.0, 0.0),
(0.0, 0.0, 1.0, GRIPPER_TCP_Z),
(0.0, 0.0, 0.0, 1.0),
embodichain/lab/sim/atomic_actions/affordance.py:563
PressAffordance.get_press_pose()fixesy_axisto world up and setsz_axisfrom the press axis, but does not re-orthogonalizey_axisagainstz_axis. This can produce a non-orthonormal rotation matrix (invalid pose) whenever the press axis is not perpendicular to world up. Buildy_axisfromz_axisandx_axisto guarantee an orthonormal basis.
z_axis = torch.matmul(link_pose[:, :3, :3], press_axis)
z_axis = torch.nn.functional.normalize(z_axis, dim=1)
y_axis = torch.tensor(
[0.0, 0.0, 1.0], dtype=torch.float32, device=device
).expand_as(z_axis)
scripts/benchmark/atomic_action/run_benchmark.py:46
run_benchmark.pydrops thepressbenchmark entry (and the PR deletespress_benchmark.py). This is a breaking CLI change for users relying onembodichain benchmark atomic-action --action press(the prior docstring even showed that invocation). Ifpressis intentionally deprecated, consider keeping thepressoption as a compatibility shim that exits with a clear deprecation message, or restore a minimalpressbenchmark module.
ACTION_MODULES = {
"move_end_effector": "scripts.benchmark.atomic_action.move_end_effector_benchmark",
"move_joints": "scripts.benchmark.atomic_action.move_joints_benchmark",
"pick_up": "scripts.benchmark.atomic_action.pickup_benchmark",
"move_held_object": "scripts.benchmark.atomic_action.move_held_object_benchmark",
"place": "scripts.benchmark.atomic_action.place_benchmark",
}
Comment on lines
+375
to
+385
| z_axis = torch.matmul(link_pose[:, :3, :3], twist_axis) | ||
| z_axis = torch.nn.functional.normalize(z_axis, dim=1) | ||
| 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( | ||
| "TwistAffordance twist axis must not be parallel to world (0, 0, 1)." | ||
| ) | ||
| x_axis = torch.nn.functional.normalize(x_axis, dim=1) |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 42 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
embodichain/lab/sim/planners/motion_generator.py:443
preserve_cartesian_samplesis documented as a Cartesian-only constraint, but here it unconditionally forces the IK-interpolation path regardless ofmove_type. If a caller accidentally enables it forJOINT_MOVE, planning will silently switch away from the configured backend planner, and the flag will be ignored in the JOINT_MOVE branch of_generate_ik_interpolation, which is surprising and hard to debug. Consider validating thatpreserve_cartesian_samplesis only used withEEF_MOVEtargets (or otherwise define behavior for joint targets).
use_interpolation = (
options.preserve_cartesian_samples
or options.strategy == "ik_interp"
or (
move_type is MoveType.JOINT_MOVE
and not self.planner.supports_move_type(MoveType.JOINT_MOVE)
)
)
tests/sim/atomic_actions/test_module_imports.py:65
- This subprocess-based import check can hang indefinitely if any tutorial/primitive accidentally blocks during import (e.g., starts a sim loop, waits for input, downloads assets, etc.). Adding a timeout makes the test suite fail fast and avoids stuck CI jobs.
Comment on lines
67
to
+78
| @dataclass(frozen=True, slots=True, eq=False) | ||
| class PressGoal: | ||
| """Single end-effector contact pose used by :class:`Press`.""" | ||
| class PressGoal(ObjectActionGoal): | ||
| """Target object described by a press affordance.""" | ||
| goal_kind: ClassVar[str] = "press" | ||
| xpos: PoseGoalValue | ||
| """Contact pose, shape ``(4, 4)`` or ``(num_envs, 4, 4)``.""" | ||
| target_pose: PoseGoalValue | ||
| """Target pose snapshot or late-bound stable scene-entity reference.""" | ||
| def __post_init__(self) -> None: | ||
| validate_pose_goal(self.xpos, "xpos", allow_waypoints=False) | ||
| ObjectActionGoal.__post_init__(self) | ||
| validate_pose_goal(self.target_pose, "target_pose", allow_waypoints=False) |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 43 changed files in this pull request and generated no new comments.
Suppressed comments (1)
embodichain/data/assets/obj_assets.py:246
- MicrowaveOven dataset docstring advertises
get_data_path("MicrowaveOven/microwave_oven.urdf"), but the new tutorials useMicrowaveOven/microwave_oven_with_inertials.urdf. This is confusing and can mislead users about the correct asset path to request/download.
class MicrowaveOven(EmbodiChainDataset):
"""get_data_path("MicrowaveOven/microwave_oven.urdf")"""
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 43 changed files in this pull request and generated no new comments.
Suppressed comments (1)
scripts/tutorials/atomic_action/twist.py:229
- The fallback ValueError message references the Press demo, but this is the Twist tutorial. This will confuse users if an unexpected target type is ever passed here.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 43 changed files in this pull request and generated no new comments.
Suppressed comments (2)
embodichain/lab/sim/planners/motion_generator.py:443
preserve_cartesian_samplesis treated as a generic trigger for IK interpolation, but_generate_ik_interpolation()ignores it forMoveType.JOINT_MOVEand will still resample viainterpolate_with_distance(). This means settingpreserve_cartesian_samples=Truefor joint targets silently changes the planning path (forces IK interpolation route) without actually preserving samples. Consider rejecting this option unlessmove_typeisEEF_MOVE.
use_interpolation = (
options.preserve_cartesian_samples
or options.strategy == "ik_interp"
or (
move_type is MoveType.JOINT_MOVE
and not self.planner.supports_move_type(MoveType.JOINT_MOVE)
)
)
scripts/tutorials/atomic_action/twist.py:229
- The error message in the final
elsebranch refers to the Press demo, but this is the Twist tutorial. This is likely a copy/paste mistake and will confuse users when debugging unsupported target types.
yuecideng
approved these changes
Aug 19, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 46 changed files in this pull request and generated no new comments.
Suppressed comments (5)
scripts/tutorials/atomic_action/twist.py:229
- The raised error message refers to the Press demo, but this is the Twist tutorial. This can confuse users when an unexpected target type is encountered.
scripts/tutorials/atomic_action/twist.py:230 - This line is long enough to bypass Black's default formatting and may fail style checks; splitting it improves readability and keeps formatting consistent.
scripts/tutorials/atomic_action/press.py:254 - This line is long enough to bypass Black's default formatting and may fail style checks; splitting it improves readability and keeps formatting consistent.
scripts/tutorials/atomic_action/slide.py:288 - This line is long enough to bypass Black's default formatting and may fail style checks; splitting it improves readability and keeps formatting consistent.
embodichain/data/assets/obj_assets.py:246 - The docstring example path for the MicrowaveOven asset does not match the URDF path used by the new atomic-action tutorials (which reference
microwave_oven_with_inertials.urdf). Updating this avoids sending users to a non-existent or unintended file.
class MicrowaveOven(EmbodiChainDataset):
"""get_data_path("MicrowaveOven/microwave_oven.urdf")"""
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
twistaction:python scripts/tutorials/atomic_action/twist.py (--rigid_object)pressaction:python scripts/tutorials/atomic_action/press.py (--rigid_object)slide(for drawer, translation only):python scripts/tutorials/atomic_action/slide.pyType of change
Checklist
black .command to format the code base.