Add physics settle validation check to PooledObjectSolver

[cvolkcvolk]

Replay of upstream isaac-sim#769 onto the fork to exercise the Arena review bot.

[cvolkcvolk]

/review

[arena-review-bot]

🤖 Isaac Lab-Arena Review Bot

Summary

Adds a post-reset physics-settle check to the pooled object placer: after env.reset() the sim is stepped, layouts whose objects keep moving are re-selected from the pool, and settle failures are cached (in-memory, and optionally persisted on disk) so future runs skip them. The placement-validation result is also generalized from a bool to a PlacementValidationChecklist. The mechanism is sensible and well-tested, but it places sim orchestration in the domain layer, defaults the costly pass on for everyone (and the CLI flag can't actually turn it off), and adds a fairly heavy cross-run persistence layer whose payoff may not justify its fragility.

Design, Boundaries & Scope

• Sim orchestration in the placement (domain) layer. PooledObjectPlacer.verify_in_sim_and_reselect(env) steps physics, reads sim velocities, writes poses, and runs the retry loop. PooledObjectPlacer is a pure object-placement class, and the architecture invariants deliberately keep that layer sim-agnostic — holding a live env / stepping the sim is a runtime/event responsibility. The deferred imports inside the method don't change that; the invariants are explicit that boundaries are about responsibility, not the import graph. The loop reads more naturally up in run_placement_physics_settle_check (already sim-side, in placement_events.py), with the pool exposing only dependency-free bookkeeping (note_applied, a public draw_replacement(env_id), record_failures, plus the settle params). Inline below.
• Default-on / blast radius. The settle pass defaults on at every layer — solve_and_apply_relation_placement(..., enable_physics_settle_check=True), rollout_policy(..., enable_physics_settle_check=True), and the CLI — so every reset now steps physics by default for all users. The invariants ask for new/costly behavior to be opt-in and to not silently change the default path. Combined with the CLI flag that can never be False (inline), there is no escape hatch. Suggest defaulting off end-to-end and making the flag a real toggle.
• Does the on-disk persistence earn its place? PhysicsSettleFailureCache + compute_layout_signature are a sizeable, fragile mechanism. Cross-run identity rests on enqueue-order uids staying aligned, guarded by a best-effort sha256 of object geometry/relations (the bbox read is wrapped in a bare except: -> None; relations are hashed via __dict__). If that signature ever misses a relevant input, stale uids silently skip the wrong layouts — a quiet placement corruption — and the payoff is only "skip re-simulating known-bad layouts when the same scene+seed is re-run." Could this ship as in-memory-only dedup for now (the skip-list already works within a run) and defer the on-disk layer until there is a demonstrated need? That removes the signature machinery and its failure mode entirely. Worth a deliberate decision rather than defaulting to persistence.

Findings

See inline comments. The factoring of write_layout_to_sim out of solve_and_place_objects (de-duplicating the pose-write) and the bool -> checklist generalization are clean.

Test Coverage

Strong. The sim tests in test_physics_settle.py use the inner/outer run_simulation_app_function pattern with deferred imports and include a per-env parallel case; being camera-free they correctly land in Phase 1 (same convention as test_events.py — no marker needed). The cache/signature is covered sim-free across the persistence round-trip, both invalidation paths, and signature determinism/sensitivity. Gaps: no end-to-end test of verify_in_sim_and_reselect's re-selection/fallback loop (the retry budget + soft-fallback path), and nothing exercises the post-settle observation handed to the policy (see the stale-obs inline). A regression test for the re-selection loop would be valuable given it's the core new behavior.

Verdict

Significant concerns — the feature is well-built and tested, but the layer placement, default-on blast radius, the inert CLI flag, and the heavyweight persistence each warrant a decision before merge.

[arena-review-bot]

🟡 Warning — action="store_true" with default=True makes this flag inert: argparse only ever sets it to True (when present), so it is True whether or not the user passes it, and there is no way to reach the False the help text describes. Combined with the True defaults downstream, the settle pass (which steps physics on every reset) becomes mandatory-on for all CLI users, against the conservative-defaults invariant. Suggest a real toggle that defaults off:

Suggested change

	arena_group.add_argument(
	"--enable_physics_settle_check",
	action="store_true",
	default=True,
	help=(
	"Enable physics settle check after reset. When False, the settle check is skipped and the placement pool is"
	" not re-selected."
	),
	arena_group.add_argument(
	"--enable_physics_settle_check",
	action=argparse.BooleanOptionalAction,
	default=False,
	help=(
	"Enable the post-reset physics settle check (steps physics and re-selects layouts that do not"
	" settle). Off by default; pass --enable_physics_settle_check to turn it on."
	),
	)

[arena-review-bot]

🟡 Warning (boundary) — this method puts sim orchestration — stepping physics, reading sim velocities, writing poses, and the retry loop — inside PooledObjectPlacer, which is a pure object-placement (domain) class. The invariants keep that layer sim-agnostic: holding a live env and stepping the sim belong in the runtime/event layer, and the deferred physics_settle/placement_events imports here don't make it ok (boundaries are about responsibility, not the import graph). Could the loop move up into run_placement_physics_settle_check (already in the sim-side placement_events.py), with the pool exposing only dependency-free bookkeeping — note_applied, a public draw_replacement(env_id), record_failures, and the settle params? That keeps the placer importable without Isaac Sim and puts the choreography in the layer that owns it.

[arena-review-bot]

🟡 Warning — this defaults False, but solve_and_apply_relation_placement always constructs ObjectPlacerParams(..., enable_physics_settle_check=enable_physics_settle_check) with its own True default, so on the main path this False is never honored — the two defaults disagree and a reader can't trust this value. Suggest aligning them and defaulting off everywhere (per the opt-in invariant) so the dataclass default reflects reality.

[arena-review-bot]

🟡 Warning — obs was captured by env.reset() on the line above, but the settle pass then steps physics (up to (max_physics_settle_retries+1) * physics_settle_num_steps steps) and may re-write objects to a re-selected layout. The first policy.get_action(env, obs) in the loop then runs on a stale observation that no longer matches the sim — especially after a re-selection relocates objects. Should obs be refreshed after the settle pass, before the loop (recomputed from the env's observation manager so it matches how reset() produces it), so the policy's first action sees the settled/re-selected state?