Add move_object atomic action

[skywhite1024]

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]

Add docs and unitest for the new atomic action

[skywhite1024]

Added both.
Docs now cover:

• MoveObjectAction
• MoveObjectActionCfg
• MoveObjectTarget
• HeldObjectState
• target resolution for MoveObjectTarget / {"object_target_pose": ...}
  Tests now cover:
• MoveObjectAction target resolution
• missing held-state failure
• closed-hand trajectory padding
• held-state preservation
• engine held-state propagation/clearing
• batched pickup success handling

[yuecideng]

These two class could be a part of ObjectSemantics?

[skywhite1024]

I kept them separate intentionally and documented the distinction.
ObjectSemantics is a reusable object description: geometry, affordance, label, entity.
HeldObjectState is runtime execution state produced by a successful semantic pickup. It stores the selected grasp and object-to-EEF transform.
MoveObjectTarget is an action-specific target describing where the already-held object should move.
Putting them into ObjectSemantics would mix static object semantics with per-execution robot state, which seems harder to reason about across repeated executions.

[yuecideng]

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]

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

6. 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).

7. 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

8. _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.

9. 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.

[skywhite1024]

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.

Thanks, I agree with this direction.
I removed UprightAction from this branch instead of trying to keep a composite action disguised as an atomic primitive. The current PR now focuses on MoveObjectAction, which is a genuine primitive: move the currently held object to an object-centric target pose while keeping the gripper closed.
For MoveObjectAction, I also fixed the hierarchy issue you pointed out. It now reuses shared closed-hand setup through HandCloseActionCfg / _HandCloseAction, instead of redeclaring hand_close_qpos / hand_control_part and duplicating the pickup-style initialization boilerplate.
If we revisit upright behavior later, I agree it should either be decomposed into engine-orchestrated phases, or introduced explicitly as a composite action concept with its own held-state contract.

[skywhite1024]

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

6. 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).
7. 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

8. _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.
9. 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.

Thanks for the detailed review. I went through the findings and addressed the correctness issues.
What changed:

• Removed UprightAction from this branch. That resolves the composite-action concern as well as the export/register mismatch, tensor-target fallback, per-env IK regression, held-state clearing, and return-arity violation.
• Restored _interpolate_hand_qpos behavior for 1-D inputs. It now returns (n_waypoints, hand_dof) for unbatched inputs, while still supporting batched (n_envs, n_waypoints, hand_dof).
• Fixed batched success handling in PickUpAction and _plan_arm_trajectory. The public API still returns one overall bool, and per-env success tensors are now explicitly reduced with all-env success semantics.
• Refactored MoveObjectAction to reuse closed-hand setup through HandCloseActionCfg / _HandCloseAction.
• Added explicit held-state contract via updates_held_object_state / get_held_object_state.
• Removed duplicate pose inversion logic and use the existing pose_inv.
• Cleared _action_context at the start of execute_static.
• Added docs and tests for MoveObjectAction, MoveObjectTarget, held-state propagation, and batched pickup success handling.
  Validation:
• python -m pytest tests/sim/atomic_actions/test_actions.py tests/sim/atomic_actions/test_engine.py -q
  • 49 passed
• 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 tests/sim/atomic_actions/test_actions.py tests/sim/atomic_actions/test_engine.py
• git diff --check
  The remaining tutorial/demo scaffolding duplication is intentionally left as follow-up cleanup because it does not affect the MoveObjectAction API or correctness.