[OUTDATED] Make no-overlap a built-in solver behavior

[cvolkcvolk]

Summary

• No-overlap is now a built-in behavior of RelationSolver, controlled by a single clearance_m parameter and exposed through RelationSolverParams (default 1cm).
• ArenaEnvBuilder._add_pairwise_no_collision() removed from ArenaEnvBuilder -> This is solver specific and now handled within the solver (Eg just using the RelationSolver without the ArenaEnvBuilder would have not gotten NoCollisionConstraints)
• NoCollision class removed from the public API : Users should not need to manage collision avoidance manually. This is now a default behaviour.

Test plan

• All 9 no-collision tests pass (6 strategy-level, 3 solver-level)
• All 52 relation/placer tests pass
• Pre-commit checks pass

[greptile-apps]

Greptile Summary

This PR refactors no-overlap handling from an explicit user-facing NoCollision relation into a built-in behavior of RelationSolver, controlled by a single clearance_m parameter on RelationSolverParams (default 1 cm). The _add_pairwise_no_collision() helper is removed from ArenaEnvBuilder, and NoCollisionLossStrategy is marked as internal-only.

• P1 regression: When On.clearance_m + on_relation_z_tolerance_m < solver.clearance_m (e.g. On(clearance_m=0.001) with the default solver clearance_m=0.01), the built-in no-overlap loss (slope 10 000) dominates and pushes the child to solver.clearance_m above the parent, but _validate_on_relations rejects any Z position beyond On.clearance_m + 5 mm. With defaults this means any On.clearance_m < 5 mm causes every placement attempt to fail silently.

Confidence Score: 4/5

Safe to merge only after the _validate_on_relations upper-bound regression is addressed; default-clearance users are unaffected but sub-5mm On.clearance_m silently fails.

One P1 regression: On.clearance_m values smaller than (solver.clearance_m - on_relation_z_tolerance_m) = 5 mm will cause all placement attempts to fail silently because the built-in no-overlap loss pushes the child past the On validation upper bound. All tests use matching default clearances (10 mm) and therefore miss this path. The core refactor logic, gradient coupling, and margin-based no-overlap validation are otherwise sound.

isaaclab_arena/relations/object_placer.py (_validate_on_relations upper-bound) and isaaclab_arena/tests/test_no_collision_loss.py (missing coverage for On.clearance_m < solver.clearance_m)

Important Files Changed

Filename	Overview
isaaclab_arena/relations/relation_solver.py	Core solver: new _compute_no_overlap_loss applies built-in pairwise clearance with correct dual-pass gradient coupling; hardcoded slope (10000.0) is not configurable; _print_relation_debug uses stale world bbox for non-anchor parents.
isaaclab_arena/relations/object_placer.py	_validate_no_overlap margin logic is correct for same-clearance case, but _validate_on_relations upper-Z bound does not account for solver.clearance_m overriding On.clearance_m, causing always-fail for On.clearance_m < (solver.clearance_m - on_relation_z_tolerance_m).
isaaclab_arena/relations/relation_solver_params.py	Clean addition of clearance_m field with docstring and post_init non-negative assertion; default 0.01 m matches On.clearance_m default.
isaaclab_arena/relations/relation_loss_strategies.py	NoCollisionLossStrategy correctly computes clearance-expanded per-axis overlap and multiplies for volume loss; marked internal-only, not in strategies registry.
isaaclab_arena/tests/test_no_collision_loss.py	Good coverage of NoCollisionLossStrategy and solver-level no-overlap; all tests use On.clearance_m=0.01 matching solver default, leaving the On.clearance_m < solver.clearance_m failure path untested.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[ObjectPlacer.place] --> B[_generate_initial_positions]
    B --> C[RelationSolver.solve]
    C --> D[_compute_total_loss per iteration]
    D --> E[Relation losses\nOn / NextTo / AtPosition]
    D --> F[_compute_no_overlap_loss\nbuilt-in, slope=10000]
    F --> G{child in\noptimizable?}
    G -->|yes| H[compute overlap vs ALL objects\nanchor + non-anchor]
    H --> I[volume loss = slope * overlap_x * overlap_y * overlap_z]
    C --> J[_validate_placement]
    J --> K[_validate_no_overlap\nmargin = solver.clearance_m - 1e-6]
    J --> L[_validate_on_relations\nupper = On.clearance_m + 5mm]
    K --> M{gap ≥ margin?}
    L --> N{child_bottom ≤\nOn.clearance_m + 5mm?}
    M -->|yes| O[pass]
    N -->|yes| O
    M -->|no| P[retry]
    N -->|no - regression when\nOn.clearance_m < 5mm| P
    O --> Q[_apply_positions to objects]

Loading

Comments Outside Diff (1)

1. isaaclab_arena/relations/object_placer.py, line 257-295 (link)

   _validate_on_relations upper-Z bound is too tight when On.clearance_m < solver.clearance_m

   The built-in no-overlap loss (slope 10 000) will always push a child object to at least solver.clearance_m above its On-parent, even when the user sets a smaller On.clearance_m. But _validate_on_relations rejects a child whose bottom exceeds parent_top + On.clearance_m + on_relation_z_tolerance_m. The failure condition is:

   solver.clearance_m > On.clearance_m + on_relation_z_tolerance_m
   # e.g. defaults: 0.01 > On.clearance_m + 0.005 → fails when On.clearance_m < 0.005
   

   Concrete example: On(table, clearance_m=0.001) with default solver settings → solver places child at ≈10 mm, but validation allows at most 1 mm + 5 mm = 6 mm → all placement attempts fail silently, returning only "Warning: Relation solving not completed".

   The fix is to account for solver_params.clearance_m in the upper bound:

   # Current
   if child_bottom_z <= parent_top_z - eps_z or child_bottom_z > parent_top_z + clearance_m + eps_z:
   
   # Suggested: widen upper bound to accommodate the solver's built-in clearance
   solver_clearance = self.params.solver_params.clearance_m
   effective_upper = max(clearance_m, solver_clearance) + eps_z
   if child_bottom_z <= parent_top_z - eps_z or child_bottom_z > parent_top_z + effective_upper:

_{Reviews (3): Last reviewed commit: "Document gradient coupling in no-overlap..." | Re-trigger Greptile}

[kellyguo11]

Review: Make no-overlap a built-in solver behavior

Nice refactor. Moving no-overlap from a user-managed NoCollision relation to a built-in solver behavior is the right call — reduces API surface and eliminates a common footgun (forgetting to add pairwise constraints). The commit stack is clean and logical.

What works well

• Single parameter (clearance_m) on RelationSolverParams — clean, discoverable, with a sensible 1cm default and __post_init__ validation.
• Bidirectional loss for non-anchor pairs — computing loss in both directions (i→j and j→i) ensures both objects receive gradient. Good detail.
• Smart On-parent skipping in validation — _has_on_relation_with() correctly lets child-on-parent overlap pass while still checking non-On anchor pairs. The asymmetric margin (clearance for sibling pairs, 0.0 for non-On anchor pairs) is a reasonable choice.
• Test coverage — 9 tests cover the strategy API change, solver separation, clearance_m respect, negative validation, On-parent overlap acceptance, non-anchor overlap rejection, and reproducibility. Solid.
• Clean removal — NoCollision class, _add_pairwise_no_collision(), min_separation_m, and all example/env usages are fully removed. No stale references found in docs or other source files.

Issues / Suggestions

1. Solver applies clearance to non-anchor↔anchor pairs, but validation doesn't (intentional asymmetry?)

In _compute_no_overlap_loss, non-anchor vs anchor pairs use clearance_m. But in _validate_no_overlap, those same pairs use margin=0.0. The docstring says "clearance is meant for sibling objects" — but the solver loss pushes non-anchors away from anchors by clearance_m during optimization, then validation only checks hard overlap.

This is probably fine in practice (the solver already enforced the gap), but it means a scenario that barely passes optimization could technically fail validation if the solver residual ate into the margin. Consider whether the solver should also use margin=0.0 for non-anchor↔anchor pairs to keep them consistent, or document the rationale more explicitly.

2. O(n²) complexity for large object counts

The pairwise loop is O(n²) per iteration for n non-anchor objects. With the current use cases (handful of objects on a table) this is fine, but worth a brief comment noting that scaling to 50+ objects may want spatial hashing or a grid-based approach.

3. NoCollisionLossStrategy slope is hardcoded at 10000.0

self._no_collision_strategy = NoCollisionLossStrategy(slope=10000.0)

Previously this was configurable via the strategy dict. Now it's hardcoded. For most users this won't matter, but power users may want to tune it (e.g., if clearance loss dominates their On/NextTo losses). Consider exposing no_overlap_slope on RelationSolverParams for completeness.

4. get_world_bounding_box() assumption for anchors

In _compute_no_overlap_loss, anchor bboxes are retrieved via anchor.get_world_bounding_box(), while in _compute_total_loss for regular relations, the same is done with parent.get_world_bounding_box() for anchor parents. This is consistent, but it means the solver assumes anchors always have their world bbox precomputed. If that's guaranteed by RelationSolverState initialization, great — just worth noting.

5. Spec doc not included

The PR body references docs/superpowers/specs/2026-04-10-no-collision-solver-builtin-design.md but this file is not in the branch. Not blocking, but consider including it or removing the reference.

6. Minor: relation_loss_weight no longer applies to no-overlap

Previously users could set relation_loss_weight on NoCollision to increase/decrease its priority relative to other relations. The removed test test_no_collision_loss_scales_with_relation_weight confirms this is intentionally dropped. Worth a migration note if any downstream code relied on per-pair weight tuning.

Verdict

Clean, well-tested, good API simplification. The items above are mostly minor / future-proofing. 👍

[kellyguo11]

The .item() calls to build the translation tuple detach from the computation graph, which is correct here — parent_world_bbox is used as a fixed target for the loss, and gradients flow only through child_pos. Good.

One thing to keep in mind: if you ever wanted gradients to flow through both positions simultaneously (e.g., for a different optimizer), you'd need to restructure this. Current approach (compute in both directions with one detached) is fine for the Adam optimizer loop.

[zhx06]

A minor question: Do we plan to have the same clearance_m for all objects? Will we expect that different object sizes sometimes benefit from different clearance values in the future?

[cvolkcvolk]

Yes that's a good point, we might want to add this in the future. For now I think we should go with simplicity.

[alexmillane]

Looks good!

[alexmillane]

Yea good move to remove this from the EnvBuilder.

[cvolkcvolk]

Superseded by #589 (re-implemented on develop branch)