Add Collision Check for Background Objects - #835
Conversation
Greptile SummaryThis PR introduces passive background-object collision checking into the placement solver pipeline. Previously, relation-free fixed geometry (e.g. kitchen cabinets, walls) was invisible to the solver and placed objects could overlap it; now such objects are treated as immovable obstacles whose bounding boxes the solver must avoid.
Confidence Score: 5/5Safe to merge; the change is purely additive and defaults to no-op behaviour when no collision objects are discovered. The bounding-box math through both the culling path (ComputeWorldBound → scale → rotate → translate) and the ObjectReference path (local bbox → scale → rotate → translate by world position) is algebraically equivalent and confirmed by seven dedicated unit tests. The prior performance concern about per-step world-bbox recomputation is addressed by the new _fixed_obstacle_world_bboxes cache in RelationSolverState. When collision_objects is empty or omitted the code falls back to the existing behaviour exactly, so there is no regression risk for existing scenes. No files require special attention. Important Files Changed
Reviews (7): Last reviewed commit: "change filtering mechanism" | Re-trigger Greptile |
5ae33dc to
9deb1a8
Compare
There was a problem hiding this comment.
🤖 Isaac Lab-Arena Review Bot
Summary
This PR teaches the placement solver to treat relation-free background geometry as fixed collision obstacles, so placed objects no longer overlap furniture/fixtures. It threads a collision_objects list through the interface → placer → pooled placer → solver → state, caches the fixed obstacles' world bounding boxes once per solve, and adds Scene.get_collision_objects() plus a USD-fixture discovery helper. The solver-side logic (fixed-obstacle no-overlap loss, validation, disjointness assertion) is clean and well tested. My main note is a design question about two parallel discovery mechanisms, plus a couple of small style points.
Design, Boundaries & Scope
The PR ships two ways to obtain collision objects that never meet:
- the automatic builder path (
_solve_relations→Scene.get_collision_objects()), which passes every relation-free scene asset to the solver as an obstacle with no spatial culling; and - the region-culling helpers in
background_colliders.py(build_placement_region+find_background_colliders), which are invoked only from the example notebook.
background_colliders.py's own module docstring motivates itself as keeping the solver cheap by not "feeding every prim in the scene into the all-pairs no-overlap term" — yet the production path does exactly the un-culled thing (over whole assets). Is the intent for the builder to eventually use the discovery/culling helpers, or is coarse whole-asset collision the intended automatic default with fine-grained discovery reserved for manual use? Worth a short comment either way so the two paths don't silently drift. Not a blocker — both paths are individually correct.
Findings
See inline comments.
Test Coverage
Strong. Solver loss/solve, placer + pooled-placer forwarding, validation reject/accept, Scene.get_collision_objects() filtering, and both discovery helpers are covered. Pure-solver tests correctly run without the sim-app harness (the solver is sim-agnostic), and the USD/Background tests correctly use the run_simulation_app_function inner/outer pattern and land in Phase 1 (headless, no cameras/subprocess). No coverage gaps worth blocking on.
Verdict
Minor fixes needed
There was a problem hiding this comment.
🤖 Isaac Lab-Arena Review Bot
Summary
This teaches the placement solver to avoid static scene geometry that carries no relations: Scene.get_collision_objects() collects relation-free, fixed-pose assets and threads them through the placer/solver/validation as passive fixed obstacles, plus an opt-in background_colliders module for region-culled per-fixture discovery. The implementation is clean, the bbox-caching is a nice touch, and the tests cover the solver loss, the full solve, validation, and the scene filter well. My main question is about blast radius — the automatic path changes the default placement behavior for every build (details below); the code itself I couldn't fault.
Design, Boundaries & Scope
The automatic path changes default placement behavior for everyone. _solve_relations now calls get_collision_objects() on every build, so any relation-free, fixed-pose Object/ObjectReference becomes a hard no-overlap constraint by default. Two things make me want to confirm this is intended:
- Coarse whole-asset AABBs can over-reject. A large or concave relation-free background prop contributes its full axis-aligned box, so placements in empty space within that box are now rejected even though nothing physically overlaps.
- The failure mode is quiet. When the added constraints can't be satisfied,
solve_and_apply_relation_placementfalls back to best-loss layouts that failed strict validation and only prints a warning (had_fallbacks). The solver's overlap-volume loss is also flat when an object starts fully enclosed by an obstacle (the PR's own test notes this), so an object spawned inside a coarse background box may not get pushed out — it just lands in the fallback path. Net effect: scenes that placed cleanly before could now silently degrade to fallback layouts.
Given the PR's goal is to change this default, this may well be acceptable — but could it be worth gating (e.g. a builder/CLI flag defaulting the automatic path off, or on but opt-out) so existing scenes aren't surprised? At minimum, is the increased fallback frequency something you've checked against the existing environment suite?
Separately, relations/background_colliders.py (the fine-grained region-culling path) is currently wired up only by the example notebook and its own tests — nothing in core uses it. Is it intended as public API for task authors now, or could it live alongside the example until a real task consumes it? (Just questioning whether the extra core surface earns its place yet.)
Findings
See the inline comment on arena_env_builder.py.
Test Coverage
Good. Pure-solver/validation tests (test_relation_solver_background_collision.py) correctly avoid the sim harness; the ones needing USD/Scene (_test_scene_get_collision_objects_filters, test_background_colliders.py) use the inner/outer run_simulation_app_function pattern, matching the existing no-marker Phase-1 convention for in-process sim tests. Coverage spans the loss term, the full solve, ObjectPlacer/PooledObjectPlacer forwarding, and the scene filter, including negative cases. No gaps I'd block on.
Verdict
Minor fixes needed — mainly confirming the default-behavior change is intended (and ideally checked against the existing scene suite).
There was a problem hiding this comment.
🤖 Isaac Lab-Arena Review Bot
Summary
This PR makes the placement solver avoid static background geometry, not just anchors: a new collision_objects channel threads fixed obstacles through RelationSolverState → RelationSolver → ObjectPlacer/PooledObjectPlacer, Scene.get_collision_objects() auto-discovers relation-free fixed assets, and relations/background_colliders.py offers an opt-in fine-grained culling path for large scenes. The solver/state changes are clean (constant-bbox caching, disjoint-set assert, gradient only to placed objects) and well tested across the pure-Python and sim paths. My main concern is a correctness gap in the opt-in region culling, plus a question on where background_colliders belongs.
Design, Boundaries & Scope
relations/background_colliders.py is the only module in relations/ that opens USD stages and walks prims (open_stage, UsdGeom.BBoxCache) — the solver files there are pure torch. Its actual job is reading background USD geometry and emitting ObjectReferences, which reads more like scene/asset work sitting next to Scene.get_collision_objects(), and it's currently consumed only by the example notebook. Would scene/ (or an assets util) be a more natural home, keeping relations/ as pure placement math? Not a blocker — a question worth a sentence in the PR.
One heads-up (not a finding): _solve_relations now calls get_collision_objects() unconditionally, so every relation-solving scene with a relation-free, fixed-pose Object/ObjectReference gains new obstacles by default. That's the PR's intent and had_fallbacks still surfaces failures, so it's not silent — just worth confirming no existing scene relies on the old overlap-with-furniture behavior.
Findings
🟡 Warning: isaaclab_arena/relations/background_colliders.py:51 — build_placement_region pads the region only in Z; an object placed On an anchor can straddle its edge (more so under random_yaw_init), so a wall/backsplash just outside the anchor footprint is culled yet still collidable — contradicting the module's own "a nearby wall or backsplash is a real constraint". Suggest padding XY by the largest object's half-extent + clearance. (inline)
🔵 Improvement: isaaclab_arena/tests/test_background_colliders.py:195 — _test_build_placement_region is pure (MagicMock + torch), so it doesn't need a persistent SimApp; its siblings in test_relation_solver_background_collision.py run as plain functions. (inline)
Test Coverage
Strong. Pure solver/state/placer behavior is covered without a SimApp (loss present/absent, full solve pushes off obstacle, disjoint-object assert, validation accept/reject, pooled multi-env), and the USD-discovery + get_world_bounding_box paths run under the persistent-SimApp inner/outer pattern (Phase 1 — correct, since run_simulation_app_function runs in-process, not via with_subprocess). New-file copyright years (2026) are correct. The kitchen example has a real-asset smoke test asserting the sink is discovered.
Verdict
Minor fixes needed
There was a problem hiding this comment.
🤖 Isaac Lab-Arena Review Bot
Summary
This PR makes the placement solver aware of fixed background geometry so placed objects stop overlapping furniture, with a coarse whole-asset path enabled automatically inside solve_relations and an opt-in per-fixture culling module (background_colliders) for large scenes. The implementation is clean, the get_world_bounding_box fix is correct (the local bbox is already axis-aligned in the parent's frame, so applying only the parent rotation is right), and test coverage is thorough. One design question below about the default-behavior change.
Design, Boundaries & Scope
The new obstacle-avoidance is wired unconditionally into _solve_relations (arena_env_builder.py:88): whenever solve_relations is enabled, every relation-free fixed Object/ObjectReference now becomes a collision obstacle. That's a good correctness improvement, but it silently changes the solved layout for any existing scene that already had such objects — placed positions (and thus the layouts a given placement_seed reproduces) will shift, and placements that previously succeeded may now fall back if the fixed object blocks the region. For an eval library that's a reproducibility consideration worth a deliberate call. Is the always-on coarse path the intended default, or should it be gated behind a flag (default off) so existing benchmarks don't move without an explicit opt-in? Flagging as a question, not a blocker — inline at the call site.
Findings
No correctness issues found. See the design question above.
Test Coverage
Strong. test_background_colliders.py covers region construction, spatial culling (near kept, far/behind culled, enclosing shell dropped, anchor excluded), explicit and nested prim paths, and the two get_world_bounding_box frames — all via the inner/outer run_simulation_app_function pattern, matching the sibling example tests. test_relation_solver_background_collision.py covers the pure-torch solver/validation paths (loss presence, full-solve escape, the disjoint-object assertion, and the pooled multi-env production path) with substantive assertions against actual overlap. The kitchen smoke test asserts a real fixture (sink_main_group) is discovered. Nothing missing.
Verdict
Minor fixes needed — confirm the default-behavior intent; otherwise ship it.
5b87fba to
5c04c72
Compare
qianl-nv
left a comment
There was a problem hiding this comment.
Thanks Zihao, change is well structured and thorough.
A few nits, feel free to merge after fixing.
One question: is the goal for v0.3 to switch to mesh mode by default? Are we still keeping AABB mode then?
This MR currently don't provide a user interface through CLI/config to switch to the mesh mode with policy_runner/eval_runner scripts. What's your plan for it?
alexmillane
left a comment
There was a problem hiding this comment.
Review #1.
The results look great!
I have a few comments. I'll have another review after you take a look.
f0e916a to
5d62ac6
Compare
alexmillane
left a comment
There was a problem hiding this comment.
A few more small comments. Feel free to merge once addressed.
3a37c8b to
03cf56e
Compare
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
03cf56e to
67e9c0c
Compare
Summary
Add background collision checking to relation-based object placement
Detailed description
MESHmode fixtures merge into one aggregate collision mesh, with AABB fallback for meshless objects.