Skip to content

Mesh-based Non-collision Constraints #771

Open
zhx06 wants to merge 3 commits into
mainfrom
zxiao/feature/mesh_support
Open

Mesh-based Non-collision Constraints #771
zhx06 wants to merge 3 commits into
mainfrom
zxiao/feature/mesh_support

Conversation

@zhx06

@zhx06 zhx06 commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

Add mesh-based non-collision constraints via sphere-to-SDF, unified with the existing AABB path

Detailed description

  • Introduces CollisionMode.MESH as an alternative to AABB for no-overlap constraints, using greedy sphere decomposition + differentiable Warp SDF queries against actual collision geometry.
  • Solver falls back to AABB for pairs where either object lacks a mesh. Validator mirrors this.
  • Unifies naming and computational flow between AABB and mesh modes: both use subject/obstacle terminology, forward/reverse directed pairs, and broadphase gating.
  • MeshPairCache dataclass and MeshPairEntry NamedTuple give the mesh path the same collect-then-batch structure as the AABB vectorized path.
  • Environment-level placer_params is the primary configuration source; CLI --collision_mode mesh is a fallback.

Core files

  • relations/warp_sdf_kernels.py — differentiable SDF queries on Warp meshes
  • relations/warp_mesh_manager.py — sphere decomposition and mesh caching
  • relations/relation_solver.py — vectorized mesh collision loss during optimization
  • relations/mesh_pair_cache.py — typed dataclass for precomputed per-pair collision data
  • relations/object_placer.py — mesh collision validation at placement time

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Isaac Lab Review Bot — PR #771

Mesh-based Non-collision Constraints

Summary

This PR adds sphere-to-SDF mesh collision support as an alternative to AABB overlap detection. The architecture is clean — CollisionMode.MESH integrates well into the existing NoCollisionLossStrategy dispatch, and the greedy sphere decomposition + Warp SDF kernel approach is sound. The test suite is thorough (542 lines!) with good coverage of dispatch routing, gradient flow, and integration.

Findings

# Severity Finding
1 🟡 Warning Validator creates fresh WarpMeshManager per call — cache never reused
2 🟡 Warning Scale applied post-transform in extract_trimesh_from_usd may be incorrect for nested prims
3 🔵 Suggestion object_base.py abstract method has no explicit return None
4 🔵 Suggestion Sentinel warning pattern on function object is not thread-safe
5 🔵 Suggestion Consider documenting the rotated-anchor limitation more prominently

See inline comments for details.


Update (5e86ed0a): Reviewed incremental changes since 655ac73.

Addressed Findings

  • Finding #1 resolved_get_cpu_mesh_manager() now lazily creates and caches the WarpMeshManager on the instance, eliminating redundant allocations per validation call. Good fix.
  • Finding #2 resolved — Removed erroneous .T transpose on ComputeLocalToWorldTransform in usd_helpers.py. USD returns row-major matrices; the transpose was producing incorrect vertex transforms for nested prims.

Other Changes

  • Validation logic refactored (_validate_placement): Mesh mode now skips AABB validation entirely (else branch). Previously both checks ran in mesh mode — the AABB check was redundant and could produce false negatives for non-convex shapes. Clean improvement.
  • Test suite trimmed: Removed test_sphere_count_respects_budget, test_cache_key_differs_for_different_meshes, test_dispatch_falls_back_when_obj_is_none, and test_mesh_zero_loss_separated_cylinders. These removals look intentional (simplified scope / covered elsewhere), though removing cache-key differentiation test reduces regression coverage on the caching layer.

Remaining Observations

  • Findings #3#5 from original review remain unaddressed (low priority, suggestions only).
  • The new _get_cpu_mesh_manager uses hasattr check — works fine but Optional attribute initialized in __init__ would be more explicit.

Overall: Good incremental improvement. The two main warnings from the initial review are resolved. No new concerns.


Update (729d892c): Reviewed incremental changes since 5e86ed0a.

Changes in this push (2 files)

  1. relation_loss_strategies.py — Added parent_pos_resolved.expand(batch_size, -1) before the per-batch loop. This fixes a shape mismatch when parent_pos_resolved is not already batch-expanded (e.g., single parent broadcast to multiple children). Correct fix.

  2. warp_mesh_manager.py — Wrapped getattr(obj, "scale", ...) in tuple() for cache key computation. This prevents unhashable types (e.g., numpy arrays or torch tensors returned by .scale) from breaking the dict lookup. Necessary bugfix.

Assessment

Both changes are small, targeted bugfixes. No new concerns introduced. All previous suggestions (#3#5) remain low-priority and unaddressed.

Comment thread isaaclab_arena/relations/object_placer.py Outdated
Comment thread isaaclab_arena/utils/usd_helpers.py Outdated
Comment thread isaaclab_arena/assets/object_base.py
Comment thread isaaclab_arena/relations/warp_sdf_kernels.py Outdated
Comment thread isaaclab_arena/relations/relation_loss_strategies.py Outdated
@greptile-apps

greptile-apps Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds mesh-based non-collision constraints as an alternative to AABB, using greedy sphere decomposition and differentiable Warp SDF queries (sphere-to-SDF). When CollisionMode.MESH is selected, both the solver and validator dispatch mesh-capable pairs to the new sphere-to-SDF path and fall back to AABB for objects without a collision mesh.

  • New core modules: warp_sdf_kernels.py (differentiable single- and multi-mesh SDF kernels with correct autograd bridge), warp_mesh_manager.py (BVH and sphere-decomposition cache), mesh_pair_cache.py (typed precomputed-pair container), and collision_mode.py (mode enum).
  • Solver extension: relation_solver.py gains _prepare_mesh_collision_cache, _compute_no_overlap_loss_mesh, and a yaw-aware AABB broadphase that correctly applies R(src_yaw − tgt_yaw) to sphere centers and R(−tgt_yaw) to offsets before each SDF query.
  • Validator extension: object_placer.py adds _validate_no_overlap_mesh with per-pair mesh-or-AABB dispatch and a lazily-initialized CPU mesh manager reused across placement attempts.

Confidence Score: 4/5

The collision-detection logic is substantially correct — yaw transforms, broadphase, gradient flow, and AABB fallback all look sound. The greedy sphere decomposition contains a double-selection bug that can quietly waste sphere budget, but it does not crash and does not break the AABB safety net.

The yaw-transform math in both the solver and the validator is consistently correct. The skip_mesh_pairs flag properly prevents AABB from double-gating mesh-capable pairs. The one confirmed defect — duplicate sphere selection in greedy_sphere_decomposition — silently reduces coverage quality under specific inter-sphere overlap patterns, potentially allowing marginal penetrations to go unpenalized without the caller knowing.

isaaclab_arena/relations/warp_mesh_manager.py — the greedy set-cover loop in greedy_sphere_decomposition can re-select already-selected spheres.

Important Files Changed

Filename Overview
isaaclab_arena/relations/warp_mesh_manager.py New file: BVH/sphere cache manager. Contains a double-selection bug in greedy_sphere_decompositionoutgoing[idx] is never cleared after selection, so spheres can be selected multiple times, wasting budget.
isaaclab_arena/relations/relation_solver.py Large extension adding mesh-mode AABB broadphase, per-env sphere-to-SDF loss, and MeshPairCache precomputation; yaw rotation logic and gradient flow for anchor/non-anchor pairs looks correct.
isaaclab_arena/relations/object_placer.py Adds mesh-mode validation path (_validate_no_overlap_mesh), lazy CPU mesh manager, and correct _centers_in_target_frame yaw transform; skip_mesh_pairs correctly avoids double-checking mesh pairs in AABB gate.
isaaclab_arena/relations/warp_sdf_kernels.py New file: differentiable Warp SDF kernels (single-mesh and multi-mesh), with correct autograd bridge, sentinel handling, and gradient computation.
isaaclab_arena/utils/usd_helpers.py Adds extract_trimesh_from_usd: uses USD row-vector convention correctly (verts_h @ world_tf, no transpose), with fan-triangulation and positive-scale assertion.
isaaclab_arena/relations/mesh_pair_cache.py New typed dataclass container for precomputed mesh-collision pair data; well-guarded __post_init__ assertions for shape consistency.
isaaclab_arena/relations/collision_mode.py New enum CollisionMode with BBOX and MESH variants; straightforward and correct.
isaaclab_arena/relations/relation_loss_strategies.py Minor cleanup: forward-reference strings replaced with direct imports; NoCollisionLossStrategy docstring simplified. No functional changes to loss computation.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[ObjectPlacer.place] --> B[RelationSolver.solve]
    B --> C{collision_mode?}
    C -->|MESH| D[_prepare_mesh_collision_cache]
    D --> E[WarpMeshAndSphereCache]
    E --> F[get_collision_mesh per object]
    E --> G[greedy_sphere_decomposition]
    E --> H[get_warp_mesh: build BVH]
    D --> I[_finalize_mesh_cache → MeshPairCache]
    C -->|MESH| J[_compute_no_overlap_loss_mesh]
    J --> K[AABB broadphase per env]
    K --> L[multi_mesh_sdf kernel]
    L --> M[_MultiMeshSDFFunction.backward]
    C -->|BBOX / fallback| N[_compute_no_overlap_loss_aabb]
    B --> O[Solved positions]
    O --> P[_validate_placement]
    P --> Q{collision_mode?}
    Q -->|MESH| R[_validate_no_overlap skip_mesh_pairs=True]
    R --> S[_validate_no_overlap_mesh]
    S --> T{has mesh?}
    T -->|Yes| U[_spheres_penetrate_mesh: mesh_sdf on CPU]
    T -->|No| V[_pair_aabb_overlaps fallback]
    Q -->|BBOX| W[_validate_no_overlap AABB only]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[ObjectPlacer.place] --> B[RelationSolver.solve]
    B --> C{collision_mode?}
    C -->|MESH| D[_prepare_mesh_collision_cache]
    D --> E[WarpMeshAndSphereCache]
    E --> F[get_collision_mesh per object]
    E --> G[greedy_sphere_decomposition]
    E --> H[get_warp_mesh: build BVH]
    D --> I[_finalize_mesh_cache → MeshPairCache]
    C -->|MESH| J[_compute_no_overlap_loss_mesh]
    J --> K[AABB broadphase per env]
    K --> L[multi_mesh_sdf kernel]
    L --> M[_MultiMeshSDFFunction.backward]
    C -->|BBOX / fallback| N[_compute_no_overlap_loss_aabb]
    B --> O[Solved positions]
    O --> P[_validate_placement]
    P --> Q{collision_mode?}
    Q -->|MESH| R[_validate_no_overlap skip_mesh_pairs=True]
    R --> S[_validate_no_overlap_mesh]
    S --> T{has mesh?}
    T -->|Yes| U[_spheres_penetrate_mesh: mesh_sdf on CPU]
    T -->|No| V[_pair_aabb_overlaps fallback]
    Q -->|BBOX| W[_validate_no_overlap AABB only]
Loading

Reviews (21): Last reviewed commit: "unify AABB and mesh" | Re-trigger Greptile

Comment thread isaaclab_arena/utils/usd_helpers.py Outdated
Comment thread isaaclab_arena/relations/object_placer.py
Comment thread isaaclab_arena/relations/object_placer.py Outdated
Comment thread isaaclab_arena/relations/warp_sdf_kernels.py Outdated
Comment thread isaaclab_arena/relations/relation_loss_strategies.py Outdated
Comment thread isaaclab_arena/relations/warp_mesh_manager.py Outdated
Comment thread isaaclab_arena/relations/relation_loss_strategies.py Outdated
@zhx06 zhx06 force-pushed the zxiao/feature/mesh_support branch from ef73a02 to 7c46283 Compare June 11, 2026 17:56

@alexmillane alexmillane left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First partial review.

Looks good. I haven't got to the warp mesh based stuff.

Comment thread isaaclab_arena/assets/object.py Outdated
Comment thread isaaclab_arena/cli/isaaclab_arena_cli.py
Comment thread isaaclab_arena/environments/relation_solver_interface.py Outdated
Comment thread isaaclab_arena/relations/object_placer.py Outdated
Comment thread isaaclab_arena/relations/object_placer.py
Comment thread isaaclab_arena/relations/relation_loss_strategies.py Outdated
Comment on lines +604 to +626
# Rotate child sphere centers by net_yaw = child_yaw - parent_yaw.
net_yaw = child_yaw - parent_yaw
if net_yaw != 0.0:
cos_n = math.cos(net_yaw)
sin_n = math.sin(net_yaw)
rx = centers_local[:, 0] * cos_n - centers_local[:, 1] * sin_n
ry = centers_local[:, 0] * sin_n + centers_local[:, 1] * cos_n
centers_local = torch.stack([rx, ry, centers_local[:, 2]], dim=-1)

batch_size = child_pos.shape[0]
parent_pos_resolved = parent_pos_resolved.expand(batch_size, -1)
total_loss = torch.zeros(batch_size, device=device, dtype=child_pos.dtype)

for b in range(batch_size):
offset = child_pos[b] - parent_pos_resolved[b]
# Rotate offset into the parent's local frame.
if parent_yaw != 0.0:
cos_p = math.cos(-parent_yaw)
sin_p = math.sin(-parent_yaw)
ox = offset[0] * cos_p - offset[1] * sin_p
oy = offset[0] * sin_p + offset[1] * cos_p
offset = torch.stack([ox, oy, offset[2]])
centers_in_parent = centers_local + offset

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two suggestions:

  • Part of this code represents transforming vectors in one frame to another. Under a transformation (that happens to only have a yaw component). I'm wondering if we can use more general tools for this. For example, can we use: isaaclab.utils.math for example quat_apply for applying rotations, rather than recoding the code for rotations here.
  • It is useful if you use structured notation. For example q_B_A represents the rotation from the frame A to the frame B.

I just wrote a good example of doing this type of this here

Comment thread isaaclab_arena/relations/warp_mesh_manager.py Outdated
n_candidates = max(num_spheres, n_candidates)
n_surface = max(n_candidates, n_surface)

rng = np.random.default_rng(seed)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might destroy the seeding we do elsewhere.

I'm wondering if we should avoid setting the seed anywhere in our codebase, except for the one place we set it in Isaac Lab.

@xyao-nv @peterd-NV What do you think? Should we outlaw setting seeds in our code?

Comment thread isaaclab_arena/relations/warp_mesh_manager.py Outdated
@zhx06 zhx06 force-pushed the zxiao/feature/mesh_support branch 2 times, most recently from af4e742 to bc78db6 Compare June 16, 2026 14:27

@alexmillane alexmillane left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another partial review

Comment thread isaaclab_arena/cli/isaaclab_arena_cli.py
Comment thread isaaclab_arena/relations/relation_solver.py
Comment thread isaaclab_arena/relations/warp_sdf_kernels.py Outdated
Comment thread isaaclab_arena/assets/object.py Outdated
Comment thread isaaclab_arena/relations/relation_solver.py Outdated
Comment thread isaaclab_arena/relations/relation_solver.py Outdated
Comment thread isaaclab_arena/relations/relation_solver.py
Comment thread isaaclab_arena/relations/relation_solver.py Outdated
Comment thread isaaclab_arena/relations/warp_mesh_manager.py Outdated
Comment thread isaaclab_arena/relations/warp_mesh_manager.py Outdated
@zhx06 zhx06 force-pushed the zxiao/feature/mesh_support branch 2 times, most recently from e844ccc to e307fcd Compare June 24, 2026 00:04
Comment on lines 216 to 225
parent_world_bbox=anchor_world_bbox,
child_obj=child,
parent_obj=anchor,
parent_pos=None,
)
if debug:
print(f" [NoOverlap] {child.name} vs {anchor.name}: loss={loss.mean().item():.6f}")
total_loss = total_loss + loss

# Against other non-anchors (unique pairs, both directions)
for j in range(i + 1, len(non_anchor_objects)):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Null-dereference on _mesh_manager when skip_mesh_pairs=True

_compute_no_overlap_loss_aabb(skip_mesh_pairs=True) is only reached when collision_mode == MESH and calls self._mesh_manager.get_collision_mesh(child) directly. After __deepcopy__ the custom __deepcopy__ explicitly sets _mesh_manager = None. If the copied solver ever enters _compute_total_loss without first calling solve() (which is the only method that calls _prepare_mesh_collision_cache), this raises AttributeError: 'NoneType' object has no attribute 'get_collision_mesh'. The method should assert self._mesh_manager is not None or add a short-circuit early return before dereferencing it when skip_mesh_pairs=True.

@zhx06 zhx06 force-pushed the zxiao/feature/mesh_support branch from e307fcd to 2b8adcd Compare June 24, 2026 17:31
Comment on lines +706 to +716
source,
source_mesh,
source_pos,
target,
target_mesh,
target_pos,
mesh_manager,
tolerance,
orientations,
) -> bool:
"""True if source's spheres penetrate target's mesh."""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 AssertionError crash for roll/pitch anchor with no mesh in MESH mode

When MESH mode is active and an anchor object has (a) no collision mesh and (b) a non-pure-Z rotation (roll or pitch), _pair_aabb_overlaps is reached via the AABB fallback in _validate_no_overlap_mesh. The assert on qx/qy then propagates as an unhandled AssertionError, crashing the entire place() call instead of returning False or raising a descriptive ValueError.

The solver-side guard in _prepare_mesh_collision_cache only fires for anchor pairs that have a mesh; anchors without meshes silently bypass it, so this is the only gate. Replacing the assert with an explicit ValueError (or a return True for conservative rejection) would make the failure mode predictable.

Comment on lines 204 to +234
continue
if (
skip_mesh_pairs
and self._mesh_manager.get_collision_mesh(child) is not None
and self._mesh_manager.get_collision_mesh(anchor) is not None
):
continue
anchor_world_bbox = anchor.get_world_bounding_box().to(device)
loss = self._no_collision_strategy.compute_loss(
clearance_m=self.params.clearance_m,
child_pos=child_pos,
child_bbox=child_bbox,
parent_world_bbox=anchor_world_bbox,
child_obj=child,
parent_obj=anchor,
parent_pos=None,
)
if debug:
print(f" [NoOverlap] {child.name} vs {anchor.name}: loss={loss.mean().item():.6f}")
total_loss = total_loss + loss

# Against other non-anchors (unique pairs, both directions)
for j in range(i + 1, len(non_anchor_objects)):
other = non_anchor_objects[j]
if (id(child), id(other)) in on_pairs:
continue
if (
skip_mesh_pairs
and self._mesh_manager.get_collision_mesh(child) is not None
and self._mesh_manager.get_collision_mesh(other) is not None
):
continue

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 _mesh_manager null-dereference when skip_mesh_pairs=True after __deepcopy__

_compute_no_overlap_loss_aabb accesses self._mesh_manager.get_collision_mesh(child) unconditionally when skip_mesh_pairs=True (lines 207 and 231). After __deepcopy__, the custom implementation explicitly sets _mesh_manager = None. Any path that calls _compute_total_loss on a copied solver without first invoking solve() will raise AttributeError: 'NoneType' object has no attribute 'get_collision_mesh'.

A short-circuit guard — if self._mesh_manager is None: skip_mesh_pairs = False or an early-return of zeros — would make post-copy solver state safe.

@zhx06 zhx06 force-pushed the zxiao/feature/mesh_support branch 2 times, most recently from ca965ca to 923bc46 Compare June 26, 2026 00:00
zhx06 added 3 commits June 26, 2026 09:53
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
@zhx06 zhx06 force-pushed the zxiao/feature/mesh_support branch from 923bc46 to 0e57d62 Compare June 26, 2026 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants