Make no-overlap a built-in solver behavior [V2 against develop]

[cvolkcvolk]

Supersedes #573 (rebased onto develop due to batch solver divergence).

Motivation

No-overlap between objects was managed via explicit NoCollision relations that ArenaEnvBuilder auto-injected. This was an implementation detail leaking into the user API, and the solver had no built-in collision avoidance.

Summary

• No-overlap is now a built-in behavior of RelationSolver, controlled by a single clearance_m parameter on RelationSolverParams (default 1cm).
• The solver computes pairwise no-overlap loss for all object pairs automatically (except anchor-anchor).
• NoCollision class removed from the public API.
• ArenaEnvBuilder._add_pairwise_no_collision() workaround removed.
• ObjectPlacerParams.min_separation_m removed; validation unified to use clearance_m.
• Stale docs and comments updated.

Test plan

• All 62 relation/placer tests pass
• Pre-commit checks pass

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This PR refactors no-overlap collision avoidance from an explicit user-facing NoCollision relation into a built-in solver behavior controlled by a single clearance_m parameter on RelationSolverParams. The design is sound — it removes a leaky abstraction and simplifies the user API. The implementation is largely correct, with a few items worth addressing.

Design Assessment

Design is sound. Moving no-overlap from a user-injected relation to built-in solver behavior is the right call. The old approach leaked an implementation detail (NoCollision + ArenaEnvBuilder._add_pairwise_no_collision) into the public API. The new approach:

• Eliminates the old deduplication bug (the removed TODO about computing loss twice for bidirectional NoCollision relations)
• Centralizes no-overlap control in one place (clearance_m)
• Correctly uses .detach() on the other object's position to prevent simultaneous gradient push on both objects in a pair, improving optimization stability

The NoCollisionLossStrategy being decoupled from RelationLossStrategy (no longer inheriting from it) is appropriate since it's now an internal solver concern, not a user-configurable strategy.

Findings

Details in inline comments below.

Test Coverage

• Coverage: Adequate
• Test quality: Good

Well-tested: NoCollisionLossStrategy unit tests cover separation, touching, overlap, slope scaling, volume formula, and multi-env batched input. Solver integration tests verify objects end up separated and results are reproducible. The removed test_no_collision_loss_scales_with_relation_weight is correctly dropped.

Gaps worth noting (non-blocking):

• No test for clearance_m validation (__post_init__ raising ValueError for negative values)
• No test for anchor-anchor skip in _validate_no_overlap
• No explicit test verifying solver computes no-overlap loss between non-anchor and anchor objects

CI Status

Pre-commit check is queued.

Verdict

COMMENT

Well-motivated refactoring that simplifies the public API. Core logic is correct. See inline comments for specific items to address.

[greptile-apps]

Greptile Summary

This PR moves no-overlap enforcement from an explicit NoCollision relation that ArenaEnvBuilder injected into the solver as a built-in, controlled by a single clearance_m parameter on RelationSolverParams. The NoCollision class, _add_pairwise_no_collision(), and ObjectPlacerParams.min_separation_m are all removed.

• RelationSolverParams.clearance_m docstring says the margin applies to "every pair of non-anchor objects", but _compute_no_overlap_loss also applies it to every non-anchor ↔ anchor pair. When solver.clearance_m > On.clearance_m, the high-slope no-overlap loss (10 000) completely overrides the On strategy (slope 100), placing On(anchor) objects at solver.clearance_m above the surface rather than the specified On.clearance_m. test_solver_respects_clearance_m exercises this exact scenario (clearance_m=0.05, On.clearance_m=0.01) but never asserts the resulting Z position.

Confidence Score: 4/5

Safe to merge for default configurations; P1 docstring/behavioral mismatch should be resolved before recommending users raise solver.clearance_m above On.clearance_m.

One P1 finding: clearance_m docstring says 'non-anchor objects' but applies to all pairs including anchor-non-anchor, silently overriding On.clearance_m when the solver clearance is larger. The default config (both = 0.01 m) is unaffected, but any user following the docs to set a larger solver clearance will get wrong Z heights for On-related objects.

isaaclab_arena/relations/relation_solver_params.py (clearance_m docstring), isaaclab_arena/relations/relation_solver.py (_compute_no_overlap_loss anchor loop), isaaclab_arena/tests/test_no_collision_loss.py (test_solver_respects_clearance_m missing Z assertion)

Important Files Changed

Filename	Overview
isaaclab_arena/relations/relation_solver_params.py	Adds clearance_m parameter; docstring claims 'non-anchor objects' only but implementation applies clearance to all pairs including anchor-non-anchor, conflicting with On.clearance_m when values differ.
isaaclab_arena/relations/relation_solver.py	Implements built-in no-overlap via _compute_no_overlap_loss; anchor-non-anchor pairs are correctly included, but the associated params docstring says 'non-anchor only', creating a subtle Z-height override for On-related objects.
isaaclab_arena/tests/test_no_collision_loss.py	Good unit coverage for NoCollisionLossStrategy; test_solver_respects_clearance_m uses clearance_m=0.05 with On.clearance_m=0.01 but never asserts Z position, missing the clearance-override bug.
isaaclab_arena/relations/object_placer.py	Validation logic correctly skips On-pairs and anchor-anchor pairs; _validate_no_overlap margin is derived from solver.clearance_m and is internally consistent for default configs.
isaaclab_arena/relations/relation_loss_strategies.py	New standalone NoCollisionLossStrategy computes 3D volume-overlap loss with clearance expansion on the parent bbox; logic is sound and the multiplicative product correctly yields zero loss when objects are Z-separated.
isaaclab_arena/relations/relations.py	NoCollision class cleanly removed; remaining relation classes (On, NextTo, IsAnchor, etc.) unchanged and correct.
isaaclab_arena/environments/arena_env_builder.py	_add_pairwise_no_collision() workaround removed cleanly; new _solve_relations() delegates entirely to ObjectPlacer with no stale imports or references.
isaaclab_arena/relations/object_placer_params.py	min_separation_m removed; ObjectPlacerParams now delegates clearance configuration to solver_params.clearance_m.
isaaclab_arena_environments/gr1_table_multi_object_no_collision_environment.py	Updated to use built-in solver no-overlap; no longer injects NoCollision relations manually. Clean.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["RelationSolver.solve()"] --> B["_compute_total_loss()"]
    B --> C["Per-object relation losses\n(On, NextTo, AtPosition …)"]
    B --> D["_compute_no_overlap_loss()"]
    D --> E["non-anchor vs anchor\nclearance_m applied\n⚠ also overrides On.clearance_m"]
    D --> F["non-anchor vs non-anchor\nforward pass: grad → child\nreverse pass: grad → other"]
    C --> G["total_loss.mean()"]
    E --> G
    F --> G
    G --> H{loss < convergence_threshold?}
    H -- yes --> I["Return solved positions"]
    H -- no --> J["optimizer.step()"]
    J --> B
    I --> K["ObjectPlacer._validate_placement()"]
    K --> L["_validate_on_relations()"]
    K --> M["_validate_no_overlap()\nmargin = clearance_m - 1e-6\nskips On-pairs and anchor-anchor"]

Loading

_{Reviews (3): Last reviewed commit: "Merge branch 'develop' into cvolk/no-col..." | Re-trigger Greptile}

[zhx06]

LGTM! Approved.

[alexmillane]

Great! Couple of nits. Thanks for the cleanup.