Add standalone physics settle check over pooled layouts#769
Conversation
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: Add physics settle validation check to PooledObjectSolver
Good feature direction — gating layouts on physical settling is the right post-validation strategy for agentic env gen. The PlacementValidationChecklist refactor cleanly generalizes validation, and the failure cache with layout-signature invalidation is well-thought-out. A few issues need attention before merging:
🔴 Critical: CLI argument --enable_physics_settle_check is always True
isaaclab_arena/cli/isaaclab_arena_cli.py
arena_group.add_argument(
"--enable_physics_settle_check",
action="store_true",
default=True,
...
)action="store_true" combined with default=True means the flag is always True — there's no way for the user to disable it. store_true only sets the value to True when the flag is present; since the default is already True, the flag is a no-op.
Fix: Use BooleanOptionalAction (Python 3.9+) to get both --enable_physics_settle_check and --no-enable_physics_settle_check:
arena_group.add_argument(
"--enable_physics_settle_check",
action=argparse.BooleanOptionalAction,
default=True,
help=...,
)🔴 Critical: Wrong TYPE_CHECKING import in placement_events.py
isaaclab_arena/relations/placement_events.py:19
if TYPE_CHECKING:
from isaaclab_arena.assets.object_base import ObjectBase
from isaaclab_arena.relations.placement_validation import PlacementResultPlacementResult lives in isaaclab_arena.relations.placement_result, not placement_validation (which only defines PlacementValidationChecklist). This won't crash at runtime (guarded by TYPE_CHECKING), but type checkers and IDE navigation will break on the result: PlacementResult annotation in write_layout_to_sim.
Fix:
from isaaclab_arena.relations.placement_result import PlacementResult🟡 Warning: PhysicsSettleFailureCache._load can crash on corrupted JSON
isaaclab_arena/relations/physics_settle_failure_cache.py:99-108
def _load(self) -> set[int]:
path = self._path()
if path is None or not os.path.exists(path):
return set()
with open(path) as cache_file:
data = json.load(cache_file)
...A corrupted or truncated cache file (e.g. from a crash mid-write, disk full, or manual editing) will raise json.JSONDecodeError and crash pool construction with a confusing traceback. Since this is an optimization cache (not critical state), the safe behavior is to log a warning and return an empty set.
Suggested fix:
def _load(self) -> set[int]:
path = self._path()
if path is None or not os.path.exists(path):
return set()
try:
with open(path) as cache_file:
data = json.load(cache_file)
except (json.JSONDecodeError, OSError):
return set()
...🟡 Warning: verify_in_sim_and_reselect may append duplicate entries to newly_failed
isaaclab_arena/relations/pooled_object_placer.py:600-620
In the retry loop, when an env fails to settle on attempt 0, the layout is appended to newly_failed. On attempt 1, a replacement is drawn and written, but if the same original layout object (already marked with physics_settled: False via add_checklist_item) is somehow still referenced, the "physics_settled" in checklist.checklist_items guard will use the cached False verdict and append it again. More practically: the current[env_id] is updated to the replacement after the first failure, but the original layout is already in newly_failed — so the replacement (if it also fails) gets appended too, which is correct. However, the if "physics_settled" in checklist.checklist_items branch re-reads the old verdict without re-stepping, which means a replacement layout that was previously cached as failed will short-circuit correctly. This logic is sound but fragile — a brief comment clarifying that current[env_id] is always the replacement at this point would help future readers.
🟡 Warning: _last_applied is never cleared between resets
isaaclab_arena/relations/pooled_object_placer.py:563-565
note_applied accumulates entries in _last_applied keyed by env_id. On subsequent resets, solve_and_place_objects calls note_applied again which overwrites the entry — so it's fine for the settle pass. But if some envs are not reset (partial reset), _last_applied still holds stale entries from prior resets. When verify_in_sim_and_reselect is called, it processes all entries in _last_applied, potentially re-stepping and re-checking envs that weren't just reset.
Suggestion: Clear _last_applied at the start of verify_in_sim_and_reselect (after copying current = dict(self._last_applied)) or only record applied layouts for the current reset batch.
🔵 Improvement: EnvLayoutPool.available is O(n) on every call
isaaclab_arena/relations/pooled_object_placer.py:37-40
@property
def available(self) -> int:
return sum(
1 for layout in self.layouts[self.cursor :] if not layout.validation_checklist.physics_settled_failed
)This scans all remaining layouts on every access. It's called from _available_per_env() which is called frequently during solve loops. For typical pool sizes (~100 layouts) this is fine, but if pools grow large or the check is called in tight loops, consider maintaining a counter decremented when a layout is marked as a settle failure.
🧪 Test Coverage
The PR includes solid test coverage:
test_physics_settle.py: End-to-end sim tests for the settle primitives (at-rest, colliding, parallel per-env) — good.test_physics_settle_failure_cache.py: Persistence round-trip, invalidation (seed mismatch, signature mismatch), and signature determinism/sensitivity — thorough.- Existing tests updated to use the new
PlacementValidationChecklistAPI — no regressions.
Gap: No unit test for verify_in_sim_and_reselect with the retry loop (e.g. mock that demonstrates re-selection when first layout fails and next one settles). The sim tests exercise the primitives, but the orchestration logic is only tested end-to-end via the sim. Consider a focused mock test for the retry/re-selection flow.
CI Status
Pre-commit is currently failing — likely a lint/formatting issue that needs a fix-up commit before merge.
Verdict: Minor fixes needed. The critical CLI bug and the wrong import should be fixed before merge. The cache robustness and _last_applied clearing are recommended improvements.
Update (e6d8178): Significant refactor addressing several issues from the initial review and restructuring the physics settle check architecture:
✅ Issues Addressed
-
CLI default fixed —
--enable_physics_settle_checknow defaults toFalsewithaction="store_true", which is the correct pattern (flag absent → disabled, flag present → enabled). This resolves the "always True" bug from the initial review. -
PhysicsSettleFailureCacheremoved entirely — The on-disk persistence cache (physics_settle_failure_cache.py) and its test file have been deleted. The approach was replaced by a simpler design: failed layouts are marked in-memory only via thePlacementValidationChecklist. This eliminates the corrupted-JSON crash risk and the layout-signature complexity flagged in the initial review. -
_last_appliedis now a public property —note_applied()was removed; callers write directly topool.last_applied[env_id] = result. The stale-partial-reset concern remains (the dict is never cleared), but the logic is now inplacement_events.pywhere the caller has full control. -
verify_in_sim_and_reselectmoved out ofPooledObjectPlacer— It's now a free function inplacement_events.py, receiving the pool, env, and a newPhysicsSettleParamsdataclass. This is a cleaner separation of concerns: the pool owns layout storage/sampling; the event module owns sim-stepping logic. -
newly_failedtracking removed — Since the cache is gone, there's no need to accumulate and persist failures. The logic now simply marks the checklist and moves on, removing the duplicate-append fragility.
🆕 New Architecture
PhysicsSettleParams(new dataclass): Holdsnum_steps,lin_vel_thresh,ang_vel_thresh,max_retries. Decoupled fromObjectPlacerParamswhich previously held these fields.PlacementCheckenum (newStrEnum): Replaces raw string keys ("no_overlap","on_relation","physics_settled") with typed constants. Good for IDE support and typo prevention.run_placement_physics_settle_checkreturnsVecEnvObs | None— After the settle pass steps physics and potentially swaps layouts, the reset-time observation is stale. The function now recomputes and returns a fresh observation (including RTX sensor re-renders). The caller inpolicy_runner.pyadopts it. This is a correctness fix for camera-based policies.draw_replacementis now public — renamed from_draw_replacementsince it's called from outside the class.
🟡 Remaining Concerns
-
_last_appliedstale-entry issue persists — The dict is still never cleared between resets. In partial-reset scenarios,verify_in_sim_and_reselectprocesses all entries including envs not just reset. The caller (policy_runner.py) invokes it once afterenv.reset(), so for full resets this is fine, but partial resets could re-verify stale layouts unnecessarily. -
PlacementResultimport — TheTYPE_CHECKINGimport inplacement_events.pystill importsPlacementResultfromplacement_validation(line in the diff showsfrom isaaclab_arena.relations.placement_validation import PlacementResult). This was flagged in the initial review and appears unchanged. (Needs verification — the diff shows the import block was restructured but the target module may still be wrong.) -
objects_settledlogic flip — The condition was changed fromnot (lin > thresh or ang > thresh)tolin <= thresh and ang <= thresh. These are logically equivalent (De Morgan's law), so this is a readability improvement, not a bug fix. 👍 -
New test
test_settle_check_reselects_unstable_layout— Excellent addition! This directly addresses the test coverage gap identified in the initial review. It builds a real sim env, seeds an unstable layout (cracker box dropped from height), and verifies the settle pass re-selects a stable resting layout. This is the orchestration-level test that was missing.
🧪 Test Coverage Update
- ✅
test_physics_settle_failure_cache.pyremoved (no longer needed since cache is deleted) - ✅
test_settle_check_reselects_unstable_layoutadded — covers the full retry/re-selection flow with live physics - ✅
test_placement_events.pyupdated — mock pools now exposelast_applied: dict = {}instead ofnote_applied()method
Updated Verdict: Good progress. The refactor simplifies the architecture significantly (removing the cache, separating concerns, fixing the CLI default). The observation-refresh after settle is a meaningful correctness improvement. The TYPE_CHECKING import issue from the initial review should still be verified/fixed. Overall, closer to merge-ready.
Update (9078efe): Minor cleanup commit — no new issues.
Changes
-
policy_runner.py— Addedfrom __future__ import annotationsand removed string-quoted type annotations ("PolicyBase"→PolicyBase). This is a standard Python 3.10+ forward-reference pattern backported via the future import. Clean. -
test_relation_solver_interface.py— Introduced_fallback_layout()test helper that constructsPlacementResultwith the newPlacementValidationChecklistAPI (usingPlacementCheck.NO_OVERLAP: Falseto indicate failure). Replaced inlinePlacementResult(success=False, ...)calls throughout the test file. This aligns the tests with the API changes from the previous commit — good housekeeping.
Assessment
Both changes are mechanical/cleanup in nature. No new logic, no new issues. Tests properly updated to match the new validation checklist interface. ✅
|
Thanks for putting this together! I read through it and wanted to share some thoughts, mostly around keeping the placement/solver layer sim-agnostic. Main design point to keep the solver/placement code sim-free We deliberately kept the solver and placement code (ObjectPlacer, PooledObjectPlacer, ...) free of any SimulationApp dependency. Both to keep its dependency surface lean and because that path is fast, whereas anything touching sim is comparatively expensive. I'd love to preserve that boundary here. Conceptually Idea: move the settle/reselection loop into On the cross-run failure cache
|
| placement_seed: int | None = None, | ||
| resolve_on_reset: bool | None = None, | ||
| random_yaw_init: bool = False, | ||
| physics_settle_cache_key: str | None = None, |
There was a problem hiding this comment.
We may want to add more validation checks at the end, which would likely introduce more arguments to the function. I’m wondering whether we can design a clean and extensible interface so that users can easily understand the options, or at least define a well-structured argument list.
There was a problem hiding this comment.
let's scope out a design tgt and imple it in future MRs
There was a problem hiding this comment.
Sounds good — scoping a design target first makes sense here. Happy to review the follow-up when it lands. 👍
xyao-nv
requested review from
alexmillane,
peterd-NV,
qianl-nv and
viiik-inside
as code owners
xyao-nv
changed the title
|
@isaaclab-review-bot |
Greptile SummaryThis PR adds a standalone physics-settle validation sweep over pooled placement layouts. It refactors the existing boolean
Confidence Score: 5/5Safe to merge; the new physics-settle pipeline is well-structured, self-contained, and covered by end-to-end sim tests. The changes are additive and well-tested: the existing placement path is refactored cleanly (boolean flags replaced by structured checklists), and the new validate_pool_layouts sweep is guarded by comprehensive subprocess-sim tests. The two checklist issues (KeyError on bare dict subscript and the implicit-to-explicit required-item transition) are latent API risks rather than bugs reachable from current call sites. No regressions in existing tests, no broken import paths, and the previously-flagged PlacementResult import is now correctly pointing at placement_result. placement_validation.py — the pass_validation_checklist subscript and the _required() implicit-to-explicit transition are the two spots worth a second look before the checklist API is exposed more broadly. Important Files Changed
Sequence DiagramsequenceDiagram
participant Script as run_placement_pool_validation
participant VPL as validate_pool_layouts
participant WL as _write_layouts_per_episode
participant PS as step_physics
participant GB as _grade_settled_batch
participant OSE as objects_settled_per_episode
participant Pool as PooledObjectPlacer
participant Sim as Isaac Lab Sim
Script->>VPL: validate_pool_layouts(env, settle_params)
VPL->>Pool: layouts_per_env()
Pool-->>VPL: list[list[PlacementResult]]
loop for each episode_index
VPL->>WL: write layouts for this episode
WL->>Sim: write_root_pose_to_sim + write_root_velocity_to_sim (per env)
WL-->>VPL: batch [(env_id, layout)]
VPL->>PS: "step_physics(env, num_steps * decimation)"
PS->>Sim: sim.step() x num_steps + scene.update()
VPL->>GB: _grade_settled_batch(env, batch, ...)
GB->>OSE: objects_settled_per_episode(env, env_ids, ...)
OSE->>Sim: read root_lin_vel_w / root_ang_vel_w via wp.to_torch
OSE-->>GB: list[bool]
GB->>Pool: layout.validation_checklist.add_checklist_item(PHYSICS_SETTLED, settled)
GB-->>VPL: [(env_id, checklist)]
end
VPL-->>Script: [(env_id, episode_index, checklist)]
Script->>Script: log_validation_results(results)
Reviews (7): Last reviewed commit: "self revoew" | Re-trigger Greptile |
|
👋 @xyao-nv — I see 5 new commits since my last review ( Summary of ChangesMajor refactor that lifts the physics-settle logic out of What Looks Good ✅
Minor Observations / Questions ❓
Overall this is a clean refactor. The interface is simpler, responsibilities are better isolated, and the new test ( |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def pass_validation_checklist(self, checklist_items: list[str] | None = None) -> bool: | ||
| """Check whether the gating checks pass. Defaults to the required checks only. | ||
| Args: | ||
| checklist_items: checks that must pass. If None, only required checks gate (optional checks are ignored). | ||
| Returns: | ||
| True if all the considered checks pass, False otherwise | ||
| """ | ||
| if checklist_items is None: | ||
| checklist_items = self._required() | ||
| return all(self.checklist_items[item] for item in checklist_items) |
There was a problem hiding this comment.
KeyError
The parameter checklist_items in pass_validation_checklist has the same name as the instance attribute self.checklist_items. Inside the function, the local checklist_items is used as the set of keys to evaluate, while self.checklist_items is the dict of results. The lookup self.checklist_items[item] is correct today, but any future edit that accidentally drops the self. prefix would silently attempt to subscript the list[str] parameter and either produce a wrong result or raise a TypeError. Renaming the parameter (e.g., items_to_check) removes the ambiguity entirely.
| def pass_validation_checklist(self, checklist_items: list[str] | None = None) -> bool: | |
| """Check whether the gating checks pass. Defaults to the required checks only. | |
| Args: | |
| checklist_items: checks that must pass. If None, only required checks gate (optional checks are ignored). | |
| Returns: | |
| True if all the considered checks pass, False otherwise | |
| """ | |
| if checklist_items is None: | |
| checklist_items = self._required() | |
| return all(self.checklist_items[item] for item in checklist_items) | |
| def pass_validation_checklist(self, items_to_check: list[str] | None = None) -> bool: | |
| """Check whether the gating checks pass. Defaults to the required checks only. | |
| Args: | |
| items_to_check: checks that must pass. If None, only required checks gate (optional checks are ignored). | |
| Returns: | |
| True if all the considered checks pass, False otherwise | |
| """ | |
| if items_to_check is None: | |
| items_to_check = self._required() | |
| return all(self.checklist_items[item] for item in items_to_check) |
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!
xyao-nv
changed the title
There was a problem hiding this comment.
Review: Add standalone physics settle check over pooled layouts
Nice work structuring the physics settle validation as a separate, composable layer on top of the existing analytical checks. The PlacementValidationChecklist abstraction is clean, and separating the sim-side primitives (physics_settle.py) from the orchestration logic (placement_pool_validation.py) is a good design decision. The test coverage for the new settle primitives is thorough.
A few issues I'd like to flag:
🐛 Bug: Incorrect TYPE_CHECKING import in placement_events.py
if TYPE_CHECKING:
from isaaclab_arena.assets.object_base import ObjectBase
from isaaclab_arena.relations.placement_validation import PlacementResult # ← wrong modulePlacementResult lives in isaaclab_arena.relations.placement_result, not placement_validation. This will cause type-checker failures (mypy/pyright) when analyzing this file. Should be:
from isaaclab_arena.relations.placement_result import PlacementResult⚠️ enable_physics_settle_check is accepted but unused
The rollout_policy() function in policy_runner.py accepts enable_physics_settle_check: bool = False (line 68), and main() passes the CLI arg through (line 255), but the parameter is never referenced in the function body. The CLI arg and plumbing exist, but no settle check is actually invoked during rollout.
Is this intentional scaffolding for a follow-up PR? If so, consider adding a # TODO or raising NotImplementedError when the flag is True, so users don't assume it's functional.
📝 Misleading docstring on draw_replacement
The docstring states:
"Draw the next pooled layout to retry in one env, skipping layouts known to fail to settle."
But the implementation simply draws the next layout from the queue (sample_for_envs or _sample_reusable_without_replacement) without any filtering of previously-failed layouts. The description implies a skip-list mechanism that isn't present. Consider updating the docstring to match what it actually does (draws the next available layout), or noting that the skip behavior is planned for the follow-up cache PR.
🛡️ Potential KeyError in pass_validation_checklist
In PlacementValidationChecklist.pass_validation_checklist():
def pass_validation_checklist(self, checklist_items: list[str] | None = None) -> bool:
if checklist_items is None:
checklist_items = self._required()
return all(self.checklist_items[item] for item in checklist_items)If required_items contains a check name that hasn't yet been added to checklist_items (e.g., a check that's registered as required but the validation hasn't run yet), this raises a KeyError. Consider using .get(item, False) with a clear semantic ("missing checks are treated as failing"), or validating upfront that all required items are present and raising a more informative error.
💡 Minor: _grade_settled_batch passes gym-wrapped env to objects_settled_per_episode
In placement_pool_validation.py, _grade_settled_batch passes the gym-wrapped env to physics_settle.objects_settled_per_episode, which then calls env.unwrapped.scene internally. This works fine, but it's inconsistent with _write_layouts_per_episode which passes env.unwrapped directly to write_layout_to_sim. Consider making the unwrap point consistent across helpers.
Overall this is solid foundational work. The split between the offline validator script (run_placement_pool_validation.py) and the rollout-time check is well-motivated. Looking forward to seeing the runtime settle-and-reselect loop land in a follow-up!
Update (2cfa1b8): New commits address most previous concerns. Summary:
✅ enable_physics_settle_check unused parameter — Resolved. The CLI arg and rollout_policy parameter have been removed entirely. Clean approach.
✅ Misleading draw_replacement docstring — Resolved. The method (and last_applied property) have been removed entirely, simplifying the PooledObjectPlacer API.
✅ Typo: "recoder" → "recorder" — Fixed in physics_settle.py.
✅ Removed unused last_applied tracking — The last_applied dict and draw_replacement method have been completely removed from PooledObjectPlacer, along with the tracking in placement_events.py. This is a cleaner approach than trying to maintain state for a runtime settle-and-reselect feature that isn't implemented yet.
✅ Tests updated — Mock pools in test_placement_events.py no longer expose last_applied, and test_physics_settle.py now reads from layouts_per_env() instead of last_applied.
⬜ Wrong TYPE_CHECKING import (PlacementResult from placement_validation instead of placement_result) — Still present at line 20 of placement_events.py. This bug was flagged in the initial review and remains unaddressed.
⬜ pass_validation_checklist potential KeyError — Not addressed in this diff (not blocking).
Verdict: Good cleanup. The new commit removes scaffolding for the runtime settle-and-reselect feature that wasn't being used, keeping only the offline pool validator path. One TYPE_CHECKING import bug remains from the initial review (wrong module for PlacementResult). Fix that and this is merge-ready.
| or ``None`` when ``placement_pool`` is omitted and the env has no pooled layouts. | ||
| """ | ||
| # No-ops when no layouts are stored in the pool | ||
| if placement_pool is None: |
There was a problem hiding this comment.
Can we simplify the if condition here? We have two if placement_pool is None
…_cache) Strip the persistent disk cache from this branch: PhysicsSettleFailureCache, compute_layout_signature, the physics_settle_cache_key plumbing, and the _enqueue wrappers that pre-marked cached failures. The in-sim settle check and live re-selection (verify_in_sim_and_reselect) remain. Full cache implementation is preserved on branch xyao/dev/physcis_failure_cache. Signed-off-by: Xinjie Yao <xyao@nvidia.com>
xyao-nv
force-pushed
the
xyao/dev/add_physics_validation
branch
from
4ce8ad8 to
48c1d9d
Compare
| @property | ||
| def is_valid(self) -> bool: | ||
| """True when every validation check passes.""" | ||
| return self.validation_checklist.pass_validation_checklist() |
There was a problem hiding this comment.
Does this method check that all checks pass? Suggestion to: validation_checklist.do_all_checks_pass()
|
|
||
| Returns: | ||
| True if no overlaps exist and On relations hold, False otherwise. | ||
| PlacementValidationChecklist containing the validation results. |
There was a problem hiding this comment.
To me a "checklist" is something to be done. whereas the return here is something that has been done.
I would call this PlacementValidationResults
Does that make sense?
| sorted( | ||
| env_candidates, | ||
| key=lambda candidate: ( | ||
| *candidate.validation_checklist.rank_required_and_optional_failures, |
There was a problem hiding this comment.
suggestion to change the name of: rank_required_and_optional_failures
It doesn't really do any ranking. It produces a score, which is then used to do the ranking (via the sorted here).
|
|
||
| anchor_objects_set = set(get_anchor_objects(objects)) | ||
| base_rotations = {obj: get_rotation_xyzw(obj) for obj in objects if obj not in anchor_objects_set} | ||
| base_rotations = get_base_rotations(objects, anchor_objects_set) |
There was a problem hiding this comment.
From looking at how base_rotations are used, the anchor objects are excluded anyway. Suggestion to simplify the function to get_rotation_per_object which gets all rotations for all objects.
| objects: list[ObjectBase], | ||
| anchor_objects_set: set[ObjectBase], | ||
| ) -> list[str]: | ||
| """Return the names of non-anchor objects, used for the settle velocity check.""" |
There was a problem hiding this comment.
Suggestion to remove the "used for the settle velocity check", a docstring should specify what a function does, not how the caller uses it.
| """Required check names, defaulting to every check when none are declared.""" | ||
| return self.required_items or set(self.checklist_items.keys()) | ||
|
|
||
| def pass_validation_checklist(self, required_checks: list[PlacementCheck] | None = None) -> bool: |
There was a problem hiding this comment.
In, general verb method names do something, and question method names return an answer. For example, here a more natural name would be does_validation_checklist_pass. The current name tells the user, "I'm going to make this checklist pass", whereas I believe that this method answers the question "do all the items in the checklist pass".
| def _grade_settled_batch( | ||
| env: ManagerBasedEnv, | ||
| layouts: list[tuple[int, PlacementResult]], | ||
| movable_object_names: list[str], | ||
| settle_params: PhysicsSettleParams, | ||
| ) -> list[tuple[int, PlacementValidationChecklist]]: | ||
| """Read back per-object velocities for a list of layouts and stamp ``PHYSICS_SETTLED`` per layout. |
There was a problem hiding this comment.
compute_physics_settled_and_add_to_validation_checklist
| for env_id, checklist in graded_layouts: | ||
| results.append((env_id, episode_index, checklist)) | ||
|
|
||
| results.sort(key=lambda item: (item[0], item[1])) |
There was a problem hiding this comment.
The docstring on this function doesn't mention that the results are sorted. Is this important or can it be removed?
| results = validate_pool_layouts(env, settle_params=settle_params, render=args_cli.render) | ||
| assert results is not None, ( | ||
| "The selected environment has no pooled placement, so there are no candidates to validate. " | ||
| "Pooled placement is created when objects declare placement relations (e.g. On)." | ||
| ) | ||
| log_validation_results(results) |
| return results | ||
|
|
||
|
|
||
| def log_validation_results(results: list[tuple[int, int, PlacementValidationChecklist]]) -> None: |
4 participants
Summary
When a solver returns a pooled layouts, there are 2 types of validations. One is analytical, fast, does not require SimApp, e.g. check no overlapping & on relation (existing). The other involves stepping physics, slow but more closely reflects how end user consumes, e.g. physics stability.
In an ideal world, placement_enev shall only use valid layout everytime thru
env.reset()Detailed description
Why
Agentic Env Gen requires a post-validation step to rule out envs with unstable physics, and re-trigger generation if nothing valid. It has to be automatic.
How
python isaaclab_arena/scripts/run_placement_pool_validation.py (--viz kit --render) --num_envs 10 pick_and_place_maple_tableThe standalone script checks if physics settled by advancing physics only, over the pool of layouts returned by the solver, and report results.
What
PlacementValidationChecklistdata class to contain each individual validation check results;physics_settle.pyto execute physics stepping without rendering, and check linear_vel, and ang_vel if below thresholds.placement_pool_validation.pyorchestras how many episodes over how many envs needed for the fixed pool of layoutsrun_placement_pool_validation.pyto execute the checkNext
Add integration with Pool filling and lookup table.
Ideal pipeline:
Each time pool is filled (after solver, before placement event) -> [Optional] run physics check for all env's pool unvalidated layouts & log it down -> Select (valid) one to load in placement events