[Diagnosis Only ]Object Placement Diagnosis#660
Conversation
zhx06
requested review from
alexmillane,
cvolkcvolk,
peterd-NV,
qianl-nv,
viiik-inside and
xyao-nv
as code owners
Greptile SummaryThis diagnosis-only PR fixes multi-object tabletop placement instability by switching the no-collision solver to XY-only overlap loss (
Confidence Score: 4/5The solver default changes and the stage-wide convex-hull patch affect production code paths beyond the diagnostic scripts; those two concerns warrant attention before merging to main. The two substantive changes — defaulting RelationSolverParams to XY-only/anchor-excluded and walking the entire USD stage to patch collision meshes — both affect production environments. The default change silently alters solver behavior for any caller that creates RelationSolverParams() without explicit overrides, and the stage traversal in _force_convex_hull reaches the robot embodiment's collision geometry when --force_convex_hull is used.
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
CLI["CLI args\n(--xy_only, --force_convex_hull\n--origin_main_solver)"] --> Builder["ArenaEnvBuilder\n.make_registered_and_return_cfg()"]
Builder --> Params["RelationSolverParams\n no_collision_xy_only=True\n no_collision_include_anchors=False"]
Params --> Solver["RelationSolver\n._no_collision_strategy"]
Solver --> Strategy["NoCollisionLossStrategy\n xy_only=True: XY area loss\n xy_only=False: volume loss"]
Builder --> FCH{"force_convex_hull?"}
FCH -- Yes --> Patch["_force_convex_hull(env)\nTraverse entire USD stage\nconvexDecomposition to convexHull"]
FCH -- No --> Skip["skip"]
Solver --> Loop["_compute_no_overlap_loss\nnon-anchor pairs only"]
Loop --> Loss["gradient to object positions"]
Reviews (3): Last reviewed commit: "continue analysis" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| no_collision_xy_only: bool = True | ||
| """If True, built-in no-collision only penalizes XY overlap. | ||
|
|
||
| Objects constrained by ``On(table)`` are expected to overlap the support in Z; | ||
| allowing the no-collision loss to resolve collisions by moving objects upward | ||
| creates vertical stacking and larger physics drops. | ||
| """ | ||
|
|
||
| no_collision_include_anchors: bool = False | ||
| """If True, built-in no-collision also compares non-anchor objects with anchors. | ||
|
|
||
| This is disabled by default because anchors are often support surfaces used by | ||
| ``On`` relations. Including them makes ``On`` pull objects down while | ||
| no-collision pushes them up/outward. | ||
| """ |
There was a problem hiding this comment.
no_collision_xy_only defaults to True and no_collision_include_anchors defaults to False. Any code that creates RelationSolverParams() without explicitly setting these fields now gets XY-only overlap and no anchor comparisons, whereas the previous behavior was 3D volume overlap and anchors included. Environments not built through ArenaEnvBuilder (or built through it without the new CLI args) will silently switch to the new solver semantics. This is fine as the intended fix, but carries real risk of silent regressions in non-tabletop scenes or other solver callers where the old 3D behaviour was load-bearing.
| def _force_convex_hull(env: ManagerBasedEnv) -> None: | ||
| """Replace ``convexDecomposition`` with ``convexHull`` on all MeshCollision prims. | ||
|
|
||
| ``convexDecomposition`` on raw scanned meshes (e.g. robolab assets) creates | ||
| irregular contact surfaces that are unstable in multi-object scenarios. | ||
| ``convexHull`` produces a single convex shape that behaves predictably. | ||
| """ | ||
| from pxr import UsdPhysics | ||
|
|
||
| stage = env.unwrapped.sim.stage | ||
| for prim in stage.Traverse(): | ||
| if not prim.HasAPI(UsdPhysics.MeshCollisionAPI): | ||
| continue | ||
| mesh_col = UsdPhysics.MeshCollisionAPI(prim) | ||
| approx_attr = mesh_col.GetApproximationAttr() | ||
| if not approx_attr or not approx_attr.HasValue(): | ||
| continue | ||
| if approx_attr.Get() == "convexDecomposition": |
There was a problem hiding this comment.
_force_convex_hull modifies all MeshCollision prims including the robot
stage.Traverse() walks the entire USD stage. Every prim carrying UsdPhysics.MeshCollisionAPI with convexDecomposition is patched — including the robot embodiment's collision meshes, the table, and any other scenery that was explicitly authored with convexDecomposition for good reason. If the GR1 joint (or any other articulated body) uses convexDecomposition to correctly approximate its link geometry, silently switching it to convexHull may produce a less accurate (over-approximate) collision shape and change the robot's grasp physics. Scope-filtering by prim path prefix (e.g. skip paths under the embodiment's prim path) would limit the patch to only the placed objects.
| else: | ||
| # Pick the worst non-collision status as the headline label. | ||
| overall = next(s for s in statuses if s != STABILITY_STATUS_STABLE) | ||
| exit_code = 4 |
There was a problem hiding this comment.
object_stability_repro._overall_status.
| else: | |
| # Pick the worst non-collision status as the headline label. | |
| overall = next(s for s in statuses if s != STABILITY_STATUS_STABLE) | |
| exit_code = 4 | |
| else: | |
| # Pick the worst non-collision status as the headline label (priority-ordered). | |
| _priority = [STABILITY_STATUS_FELL_OFF, STABILITY_STATUS_TIPPED, STABILITY_STATUS_SLID, STABILITY_STATUS_UNSETTLED] | |
| overall = next((s for s in _priority if s in statuses), next(s for s in statuses if s != STABILITY_STATUS_STABLE)) | |
| exit_code = 4 |
| test_args = parser.parse_args([ | ||
| "--headless", | ||
| "--num_envs", |
There was a problem hiding this comment.
args_cli parameter and duplicate ROBOLAB_OBJECTS list
run_one_object(args_cli, ...) takes args_cli as its first argument but never reads it — a fresh parser is created internally using only a hardcoded argument list. The parameter is misleading and should either be removed or used to forward relevant flags (e.g. --seed). Additionally, ROBOLAB_OBJECTS is defined identically in both run_stability_scan.py and scan_robolab_stability.py. Two independent copies will diverge over time and produce inconsistent scans; the list should be defined once and imported.
| "ang_vel_norm": float(torch.linalg.norm(ang_vel).item()), | ||
| "aabb_overlap_with": overlap_partners.get(name, []), | ||
| } | ||
| status = classify_object(metrics, thresholds) |
There was a problem hiding this comment.
next(status for status in statuses if status != "stable") returns the first non-stable status in list order (the order objects were checked), not the most severe one. If objects are checked as ["slid", "fell_off"], the printed overall is "slid" even though "fell_off" should dominate. The canonical priority ordering — ["spawn_collision", "fell_off", "tipped", "slid", "unsettled"] — is already used in object_stability_repro._overall_status. The same bug appears in the _print_metrics function of run_fixed_layout_subset_viz.py.
![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: Object Placement Diagnosis
This is a thorough diagnosis PR that investigates object stability issues in multi-object placement scenarios. The analysis documented in STABILITY_FINDINGS.md is excellent and provides clear evidence for the root causes.
Summary
Strengths:
- Comprehensive diagnostic tooling with clear CLI interfaces
- Well-documented findings with reproducible commands
- Clean separation between diagnostic scripts and core changes
- The
force_convex_hullapproach is a pragmatic fix for scanned mesh instability
Areas for Attention:
1. Copyright Year Inconsistency
📍 isaaclab_arena/llm_env_gen/run_drop_height_sweep.py:1
# Copyright (c) 2026, The Isaac Lab Arena Project DevelopersSeveral files use 2026 while others use 2025-2026. Should be consistent with the repo standard.
2. Potential Silent Failure in _force_convex_hull
📍 isaaclab_arena/environments/arena_env_builder.py:431-444
The function traverses all prims but doesn't report how many were modified. For debugging purposes, consider adding a count or log:
def _force_convex_hull(env: ManagerBasedEnv) -> None:
from pxr import UsdPhysics
stage = env.unwrapped.sim.stage
count = 0
for prim in stage.Traverse():
if not prim.HasAPI(UsdPhysics.MeshCollisionAPI):
continue
mesh_col = UsdPhysics.MeshCollisionAPI(prim)
approx_attr = mesh_col.GetApproximationAttr()
if not approx_attr or not approx_attr.HasValue():
continue
if approx_attr.Get() == "convexDecomposition":
approx_attr.Set("convexHull")
count += 1
if count > 0:
print(f"[force_convex_hull] Replaced {count} convexDecomposition -> convexHull")3. Hardcoded Asset Paths
📍 isaaclab_arena/assets/background_library.py:150
usd_path = f"{ISAACLAB_NUCLEUS_DIR}/Mimic/nut_pour_task/nut_pour_assets/table.usd"This references a specific Mimic task asset. If this background is intended for general use, the path dependency should be documented or the asset relocated.
4. Breaking Change in Environment Config
📍 isaaclab_arena_environments/gr1_table_multi_object_no_collision_environment.py:76-82
Changing from office_table to office_table_background and updating the prim path is a behavioral change that affects existing users of this environment. Since this is a diagnosis PR, consider:
- Making this change opt-in via a flag, or
- Documenting the migration path clearly
5. Default Parameter Change Impact
📍 isaaclab_arena/relations/relation_solver_params.py:53-69
no_collision_xy_only: bool = True # Was effectively False
no_collision_include_anchors: bool = False # Was effectively TrueThese defaults change solver behavior for all users. The diagnosis shows this fixes multi-object instability, but:
- Existing pipelines may rely on the old behavior
- Consider keeping old defaults and requiring explicit opt-in for the fix
6. Missing Type Hints
📍 isaaclab_arena/llm_env_gen/stability_utils.py:175-177
def collect_checkable_objects(arena_env, only_name: str | None = None) -> list[str]:The arena_env parameter lacks a type hint. Consider adding:
def collect_checkable_objects(arena_env: "IsaacLabArenaEnvironment", only_name: str | None = None) -> list[str]:7. Error Handling in Subprocess Scan
📍 isaaclab_arena/llm_env_gen/run_stability_scan.py:99-110
def run_check(object_names: list[str], force_convex_hull: bool = False, timeout: int = 120) -> dict | None:
# ...
if result.returncode not in (0, 4, 5):
return NoneReturning None loses error context. Consider returning a structured error:
if result.returncode not in (0, 4, 5):
return {"error": f"Process failed with code {result.returncode}", "stderr": result.stderr[:500]}8. Diagnostic Scripts in Production Path
The isaaclab_arena/llm_env_gen/ directory contains diagnosis-specific scripts. If these are not intended for long-term maintenance:
- Consider moving to a
scripts/diagnosis/ortools/diagnosis/directory - Add a clear README noting these are diagnostic utilities
Questions for the Author
-
Integration Plan: Will the solver parameter changes (
xy_only,no_collision_include_anchors) be merged separately with proper versioning/migration? -
Test Coverage: Are there plans to add unit tests for the stability classification logic in
stability_utils.py? -
Asset Dependency: The
OfficeTableBackgroundreferences Mimic assets. Is this a temporary diagnosis setup or intended for production?
Conclusion
This is valuable diagnostic work that clearly identifies root causes of object placement instability. The proposed fixes (XY-only collision, excluding anchors, convexHull override) are well-motivated by the empirical evidence.
For a diagnosis PR, the code quality is good. Before merging any behavioral changes to main, I'd recommend:
- Separating diagnostic scripts from core changes
- Making solver default changes opt-in
- Adding migration guidance for the environment config change
Review Type: Comment (no approval/rejection since this is marked as diagnosis-only)
1 participant
Baseline Code
xy_onlyandforce_convex_hull