Skip to content

Add move_object atomic action#313

Open
skywhite1024 wants to merge 9 commits into
mainfrom
ljd/move_object_atomic_actions
Open

Add move_object atomic action#313
skywhite1024 wants to merge 9 commits into
mainfrom
ljd/move_object_atomic_actions

Conversation

@skywhite1024

@skywhite1024 skywhite1024 commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • add HeldObjectState and MoveObjectTarget for object-centric held-object motion
  • add MoveObjectAction / MoveObjectActionCfg and register the move_object action in the atomic action engine
  • keep MoveObjectAction hold-only: it moves/rotates the held object in air and keeps the gripper closed
  • keep PlaceAction focused on regular place/release behavior and avoid object-centric place overloading
  • add a UR5 + DH_PGI_140_80 demo for move -> pick_up -> move_object

Validation

  • python -m py_compile embodichain/lab/sim/atomic_actions/core.py embodichain/lab/sim/atomic_actions/actions.py embodichain/lab/sim/atomic_actions/engine.py embodichain/lab/sim/atomic_actions/init.py scripts/tutorials/atomic_action/move_object_atomic_actions.py
  • git diff --check
  • python scripts/tutorials/atomic_action/move_object_atomic_actions.py --help

Demo

python3 scripts/tutorials/atomic_action/move_object_atomic_actions.py --auto_play
python3 scripts/tutorials/atomic_action/move_object_atomic_actions.py --auto_play --debug_state

Notes

  • This PR intentionally does not add release modes to move_object. Follow-up motion or release should be composed with later atomic actions such as move or place.
  • Follow-up work can add contact-aware placement, release policies, grasp lookahead, and collision-aware planning.

@yuecideng yuecideng self-requested a review June 18, 2026 14:17
- {doc}`planners/motion_generator` — the trajectory planner used by every action
- {doc}`sim_robot` — how control parts and IK solvers are configured
- Tutorial: `scripts/tutorials/sim/atomic_actions.py`
- Tutorial: `scripts/tutorials/atomic_action/atomic_actions.py`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change the tutorial directory?

@@ -238,4 +238,4 @@ is_success, traj = engine.execute_static(target_list=[target_pose])

- {doc}`planners/motion_generator` — the trajectory planner used by every action
- {doc}`sim_robot` — how control parts and IK solvers are configured

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add docs and unitest for the new atomic action



@dataclass
class HeldObjectState:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two class could be a part of ObjectSemantics?

@yuecideng

Copy link
Copy Markdown
Contributor

UprightAction is the bigger concern. It's not really "atomic" — it's a
10-phase composite (approach → close → lift → rotate upright → place → press →
open → retreat) stuffed into a single 315-line execute. That's why it
accumulates the problems I flagged: it has to override _resolve_grasp_pose
with a different return arity (the base contract can't express "I also need
open_length"), it regresses to a per-env IK loop (the parent's batched pattern
doesn't fit the extra upright/place/press targets), and it silently breaks
the engine's held_object_state propagation (because the contract for "does
this action produce/consume held state?" is implicit via getattr duck-typing
rather than declared on the ABC). The magic 0.25/0.60 segment-budget ratios
mirrored in the tutorial script are a symptom — the planning math should be a
public method, not copy-pasted.

MoveObjectAction is cleaner in concept (it's a genuine primitive: move a held
object to a target pose) but its class hierarchy is wrong — it inherits
MoveAction and redeclares hand_close_qpos/hand_control_part instead of
inheriting GraspActionCfg/PickUpAction, duplicating ~25 lines of init
boilerplate. That's a 5-minute fix, not a design problem.

My recommendation: keep MoveObjectAction as-is (minus the inheritance fix),
but reconsider UprightAction. Either (a) decompose it into an
engine-orchestrated sequence [pick_up, rotate_upright, place, release] so each
phase is a real atomic action and held-state flows naturally, or (b) if
composites are a first-class concept, make that explicit — a
CompositeAtomicAction base that the engine treats specially, with a declared
contract for held-state production/consumption. The current middle ground (a
composite disguised as an atomic, with ad-hoc overrides) is what's causing the
Liskov violation, the held-state leak, and the IK regression. The main
tradeoff: option (a) is cleaner but requires the engine to support mid-action
phase sequencing; option (b) is less invasive but adds a new concept to the
API.

@yuecideng

Copy link
Copy Markdown
Contributor

Code Review — ljd/move_object_atomic_actions vs main

The branch adds two new atomic actions (UprightAction, MoveObjectAction) plus
engine/core refactors. Review surface: ~2200 lines across 9 files. Findings
ranked most-severe first.

Critical — correctness bugs

  1. _interpolate_hand_qpos shape change breaks 2 existing unit tests
    embodichain/lab/sim/atomic_actions/actions.py:234-260
    The rewrite always returns (1, n_waypoints, hand_dof) for 1-D input, where the
    old code returned (n_waypoints, hand_dof).
    tests/sim/atomic_actions/test_actions.py::test_interpolate_hand_qpos_shape
    (asserts old shape) and ::test_interpolate_hand_qpos_linear (result[1] now
    IndexError) both fail. Production callers survive only because they broadcast
    into a batched slice; the documented API contract is broken.

  2. UprightAction.execute silently clears held_object_state for downstream
    actions
    embodichain/lab/sim/atomic_actions/actions.py:993-1307
    The ObjectSemantics branch never assigns self._held_object_state (stays None
    from PickUpAction.init). The engine pops
    _action_context["held_object_state"] when the inherited getter returns None
    (engine.py:258). Sequence [pick_up, upright, move_object]: pick_up sets held
    state → upright silently pops it →
    MoveObjectAction._resolve_move_object_target raises
    ValueError("MoveObjectTarget requires a HeldObjectState from a prior
    PickUpAction.") despite a successful prior pick_up.

  3. UprightAction is exported but not registered in builtin_action_map
    embodichain/lab/sim/atomic_actions/engine.py:192-197
    UprightAction/UprightActionCfg are in init.py.all, but
    builtin_action_map only has move/pick_up/move_object/place. Passing
    UprightActionCfg(...) to AtomicActionEngine makes _init_actions raise
    RuntimeError: Unknown action name in config: upright (the diff added
    move_object and place but missed upright).

  4. PickUpAction.execute collapses per-env success with torch.all
    embodichain/lab/sim/atomic_actions/actions.py:408-412 (new in this PR)
    is_success = torch.all(is_success).item() collapses the (n_envs,) per-env bool
    into one scalar. In multi-env runs, one env failing grasp resolution aborts
    the entire batch — trajectories for successfully-resolved envs are silently
    dropped. No per-env masking exists. Multi-env is clearly supported
    (self.n_envs, batched trajectories throughout).

  5. UprightAction.execute silently falls back to PickUpAction for tensor
    targets
    embodichain/lab/sim/atomic_actions/actions.py:993-1001
    Signature advertises target: Union[ObjectSemantics, torch.Tensor] and
    docstring promises "rotate upright, place down, release", but if not
    isinstance(target, ObjectSemantics): return super().execute(...) silently runs
    a plain pick-up (no upright/place/release) with no warning or
    NotImplementedError.

Moderate — design / API

  1. UprightAction._resolve_grasp_pose regressed to per-env IK loop
    embodichain/lab/sim/atomic_actions/actions.py:716-862
    A for env_idx in range(n_envs) loop calls compute_batch_ik 6 times per env
    (384 launches for n_envs=64). The parent PickUpAction._resolve_grasp_pose pads
    to (n_envs, n_max_pose, 4, 4) and issues a single batched IK call.
    compute_batch_ik supports arbitrary n_batch, so cross-env batching is feasible
    (only the within-env 3-target seed-chain needs serializing).

  2. MoveObjectActionCfg should inherit GraspActionCfg; init duplicates
    PickUpAction boilerplate
    embodichain/lab/sim/atomic_actions/actions.py:1311-1347
    MoveObjectActionCfg(MoveActionCfg) redeclares hand_close_qpos and
    hand_control_part that GraspActionCfg already provides.
    MoveObjectAction.init is line-for-line the same setup boilerplate as
    PickUpAction.init (hand_close_qpos, hand_joint_ids, joint_ids, arm_dof,
    dof). Inheriting from PickUpAction (or a shared GraspAction base) collapses
    both.

Lower severity — cleanup / altitude

  1. _invert_pose duplicated identically on MoveAction and UprightAction,
    shadowing existing pose_inv
    embodichain/lab/sim/atomic_actions/actions.py:262-269 and :867-874
    Both bodies are byte-identical. embodichain/utils/math.py:1903 already exports
    pose_inv (used in configurable_action.py, events.py). UprightAction inherits
    through PickUpAction → MoveAction, so the second copy is dead code shadowing
    the inherited one.

  2. UprightAction._resolve_grasp_pose Liskov arity violation (3-tuple vs
    parent's 2-tuple)
    embodichain/lab/sim/atomic_actions/actions.py:675 (returns 3) vs :520/:573
    (parent returns 2, parent's execute unpacks 2)
    No current caller triggers the crash (UprightAction overrides execute), but
    any future subclass calling super().execute() on the ObjectSemantics path
    lands in PickUpAction.execute and crashes with too many values to unpack.

Notes (not counted)

  • _action_context stale-leak (engine.py:181,210-274): the dict is never
    cleared between execute_static calls, but the engine's fixed _actions dict and
    the len(target_list) == len(actions) invariant make the candidate's specific
    stale-read scenario unreachable through normal public-API usage. Flagged
    PLAUSIBLE; lower priority than the held-state-clearing bug in Try to fix docs build #2.
  • Per-phase copy-paste in UprightAction.execute (~315 lines, 10 near-identical
    phase blocks) and ~250 lines of duplicated scaffolding across the two
    tutorial scripts — significant simplification/reuse opportunities but no
    correctness impact.

The most actionable fixes are #1 (restore the 2D return shape or update the
tests deliberately), #2 (set self._held_object_state in UprightAction's
success path), #3 (add "upright": UprightAction to builtin_action_map), and #5
(warn or NotImplementedError on tensor targets).

✻ Cogitated for 9m 32s

❯ Do you think the two new atomic action definition is reasonable?

● Short answer: the intent of both actions is reasonable, but the abstraction is
leaking in ways that will bite future maintainers.

UprightAction is the bigger concern. It's not really "atomic" — it's a
10-phase composite (approach → close → lift → rotate upright → place → press →
open → retreat) stuffed into a single 315-line execute. That's why it
accumulates the problems I flagged: it has to override _resolve_grasp_pose
with a different return arity (the base contract can't express "I also need
open_length"), it regresses to a per-env IK loop (the parent's batched pattern
doesn't fit the extra upright/place/press targets), and it silently breaks
the engine's held_object_state propagation (because the contract for "does
this action produce/consume held state?" is implicit via getattr duck-typing
rather than declared on the ABC). The magic 0.25/0.60 segment-budget ratios
mirrored in the tutorial script are a symptom — the planning math should be a
public method, not copy-pasted.

MoveObjectAction is cleaner in concept (it's a genuine primitive: move a held
object to a target pose) but its class hierarchy is wrong — it inherits
MoveAction and redeclares hand_close_qpos/hand_control_part instead of
inheriting GraspActionCfg/PickUpAction, duplicating ~25 lines of init
boilerplate. That's a 5-minute fix, not a design problem.

My recommendation: keep MoveObjectAction as-is (minus the inheritance fix),
but reconsider UprightAction. Either (a) decompose it into an
engine-orchestrated sequence [pick_up, rotate_upright, place, release] so each
phase is a real atomic action and held-state flows naturally, or (b) if
composites are a first-class concept, make that explicit — a
CompositeAtomicAction base that the engine treats specially, with a declared
contract for held-state production/consumption. The current middle ground (a
composite disguised as an atomic, with ad-hoc overrides) is what's causing the
Liskov violation, the held-state leak, and the IK regression. The main
tradeoff: option (a) is cleaner but requires the engine to support mid-action
phase sequencing; option (b) is less invasive but adds a new concept to the
API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants