feat(sim): add PushShapes Sim V2 with rigid parallel-gripper physics

[ElmoPA]

feat(sim): add PushShapes simulator embodiments

fix(sim): make parallel-gripper grasps rigid and collision-safe

[ElmoPA]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• feat(rollout): node-based rollout pipeline #588
• feat(dfot): diffusion-forcing algo, evaluators, configs and tests #587
• feat(sim): add PushShapes Sim V2 with rigid parallel-gripper physics #586 👈 (View in Graphite)
• feat(pipeline): port arc-length action tokens #585
• feat(pipeline): consolidate Fold, PushT, overlays, and MoE #579 : 2 other dependent PRs (#580 , #583 )
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[github-actions]

Claude Code Review

Review of PR #586

Summary

This PR introduces a versioned Tsimulation/ package with sim_v1 (frozen) and (presumably) sim_v2 (new rigid-gripper physics), selected via a TSIM_VERSION env var. However, the diff shows only sim_v1 content — the actual sim_v2 implementation (the stated purpose of the PR) is not visible.

Key concerns

1. PR title/description mismatch with diff (blocking)

The PR claims to add "PushShapes Sim V2 with rigid parallel-gripper physics" but the diff only contains sim_v1/ files. Either sim_v2/ is missing from what I can see, or this PR is misnamed. Cannot approve a "v2" PR that shows no v2 code. Please confirm sim_v2/ is included — if the diff was truncated at 80k, split this into smaller PRs.

2. Top-level Tsimulation/ package pollutes the repo root

This is a research codebase organized under egomimic/. Adding a sibling top-level package (Tsimulation/) breaks that convention. It should live under egomimic/sim/pushshapes/ or similar. The README even admits the layout was consolidated away from a symlink — that history suggests this doesn't have a settled home yet.

3. Runtime-swappable global simulator via env var is dangerous

Tsimulation/__init__.py aliases submodules of ACTIVE into sys.modules[f"{__name__}.{sub}"]. This means:

• from Tsimulation.pushshapes import env resolves to different code depending on TSIM_VERSION
• Two processes on the same node with different env vars will import different code under the same module name — very hard to debug
• Import order matters: if anything imports Tsimulation.sim_v1.pushshapes before Tsimulation.pushshapes, aliasing is a no-op
• The docstring says "jobs should still assert env.SIM_VERSION == 2" — this is admitting the design is fragile

Prefer explicit imports: from egomimic.sim.pushshapes.v2 import env. No global switch.

4. New embodiment PUSHSHAPES_SIM = 15 — enum stability

Adding to the EMBODIMENT enum requires care. Is 15 the next free slot? Any risk of colliding with an in-flight branch (Mecka, Scale)? Please verify against main HEAD at merge time. Also — the SCHEMA_NOTES says "transform list is left unset" — this will break any code path that iterates all embodiments assuming transforms exist.

5. Data schema deviations

• observations.state is [agent_x, agent_y, obj_x, obj_y, obj_theta] — sim-only, fine, but does not match any real embodiment's state layout. Confirm downstream training code won't try to feed this through the standard head-frame transforms.
• No coordinate-frame concern (sim is 2D top-down) — OK.
• Uses ZarrWriter.create_and_write — ✅ correct per conventions.
• Episode naming episode_{obj}_{pusher}_obs{N}_{idx:06d}.zarr deviates from the standard episode_{idx}.zarr and from the timestamp-hash convention (YYYY-MM-DD-HH-MM-SS-ffffff). The reader accepts both (per dataset_stats.py regex) but this creates two naming schemes to maintain. Prefer the standard timestamp hash.

6. Operator field / SQL insertion

No SQL insertion is done here (sim data is local). That's fine, but if these episodes ever get registered in the metadata DB, the operator field for scripted collection must still be SHA-256 hashed (e.g., sha256("scripted")) — not left as raw string.

7. Upload path

No upload code in this diff. If sim demos ever get uploaded, confirm they go to Cloudflare R2, not S3.

8. Test coverage

• tests/test_smoke.py and test_features.py are referenced but not shown in the diff.
• No tests for the version-switching import machinery in Tsimulation/__init__.py — this is the most fragile piece.
• No test that set_state round-trips exactly (the replay_zarr.py example measures drift but isn't a pytest).
• No test that PUSHSHAPES_SIM embodiment resolves through get_embodiment_id.

Suggestions

1. Split this PR: (a) sim_v1 move + embodiment registration, (b) sim_v2 physics, (c) version-selection mechanism.
2. Move Tsimulation/ under egomimic/sim/pushshapes/. Kill the top-level package.
3. Replace the TSIM_VERSION env var indirection with explicit versioned imports.
4. Add a pytest that: (i) creates a demo episode via ZarrDemoWriter, (ii) loads it through ZarrDataset, (iii) asserts state/action shapes and JPEG decoding.
5. Add an explicit test that get_embodiment_id("pushshapes_sim") == 15.
6. Use the standard episode-hash naming (YYYY-MM-DD-HH-MM-SS-ffffff) for output stores.
7. Remove DEPRECATED.md reference to PUSHSHAPES_SIM=Tsimulation_legacy — that env-var-flag pattern isn't documented elsewhere and confuses the story.
8. Confirm EMBODIMENT.PUSHSHAPES_SIM = 15 is next free vs. main.
9. In env.py, set_state correctly notes the pymunk angle-before-position CoG gotcha — good catch. Add a regression test for this.

Verdict: Request Changes

The physics/schema work looks careful and the CoG-ordering

---

Reviewed by Claude · Review workflow