Repository navigation
Conversation
zhx06
changed the title
aaf7170 to
b1bebfc
Compare
c43326d to
78c30b3
Compare
b1bebfc to
1ff97e5
Compare
78c30b3 to
3e2045c
Compare
1ff97e5 to
69fb952
Compare
3e2045c to
e695eea
Compare
 [Medium impact] The PR appears safe to merge. SummaryThe PR preserves rotated collision boxes through candidate solving and validation.
No actionable issues were found. Author Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Assigned object geometry and candidate rotations] --> B[Build enclosing and oriented bounds]
B --> C[Optimize positions with cached box projections]
C --> D[Apply FaceTo headings and rebuild bounds]
D --> E[Batched box checks and existing mesh checks]
E --> F[Rank validated candidates per environment]
Reviews (1) · Last reviewed commit: "fix docs and tests" · Reviewed by Greptile |
![arena-review-bot[bot]](https://avatars.githubusercontent.com/u/195794629?s=60&v=4){kind=small}
| centers_W[obj] = geometry_center | ||
| subject_centers_W = torch.stack([centers_W[p.subject] for p in pairs]) | ||
| obstacle_centers_W = torch.stack([centers_W[p.obstacle].detach() for p in pairs]) | ||
| return strategy.slope * overlap_bound.compute(subject_centers_W, obstacle_centers_W, margin=clearance_m) |
There was a problem hiding this comment.
🟡 Loss can stop moving boxes that still overlap
OverlapVolumeUpperBound.compute returns torch.minimum(volume_A, volume_B), so only the smaller volume passes gradient. If that side is saturated on every axis, the loss stays positive but the subject gets exactly zero gradient, even when the other box's frame would still push it out.
Example: a 0.4 × 0.01 × 0.1 m bar, yawed 45°, with its center 5 cm from the center of a fixed 0.2 m cube. The loss is positive and the gradient is exactly zero. The old enclosing-AABB loss had a nonzero gradient here. I re-implemented the formula outside the repo and tried random yawed boxes overlapping a fixed box. In about two thirds of the zero-gradient cases, the other frame still had a gradient.
The validator rejects these layouts, so the cost is fewer valid candidates, not bad layouts.
Could the loss combine both volumes? For example, 2 * volume_A * volume_B / (volume_A + volume_B).clamp_min(eps):
- It is zero exactly when either volume is zero.
- It equals the exact volume for axis-aligned pairs, so the quarter-turn match with the old loss still holds.
- It passes gradient from both frames.
OverlapVolumeUpperBound.compute can stay a true upper bound if the loss reads the two volumes separately.
There was a problem hiding this comment.
@arena-review-bot Could you make a diagram explaining this?
There was a problem hiding this comment.
Sure, here's the example from the comment
Top view, to scale: 1 character = 1 cm across, 1 line = 2 cm up. + is the cube's center and o is the bar's center, 5 cm to the right. The bar is 40 cm long, 1 cm thick and 10 cm tall.
/
/
+-------------------+ /
| | /
| |/
| /|
| / |
| + o |
| / |
| / |
| / |
| / |
+----/--------------+
/
/
compute projects both boxes onto each box's own three axes, multiplies the three interval overlaps, and keeps the smaller of the two products. On one axis, moving the bar changes the overlap only if the two intervals partly overlap. If one interval sits fully inside the other, the overlap is just the shorter length, so a small move doesn't change it.
Bar's axes -> volume_A (cm, relative to the bar's center)
along bar [-20 ======================== 20]
cube [-17.7 -------------- 10.6] cube inside bar -> 28.3, fixed
across bar [-0.5 = 0.5]
cube [-10.6 -------------- 17.7] bar inside cube -> 1, fixed
up bar [-5, 5] inside cube [-10, 10] -> 10, fixed
volume_A = 28.3 x 1 x 10 = 283 cm^3, gradient 0
Cube's axes -> volume_B (cm, relative to the cube's center)
x cube [-10 ============ 10]
bar [-9.5 -------------------- 19.5] partial overlap -> 19.5, left end
moves with the bar
y cube [-10, 10] inside bar [-14.5, 14.5] -> 20, fixed
up bar [-5, 5] inside cube [-10, 10] -> 10, fixed
volume_B = 19.5 x 20 x 10 = 3900 cm^3, gradient pushes the bar toward +x
torch.minimum picks the 283 cm³ from volume_A, so the bar gets zero gradient even though volume_B would push it out. With 2 * volume_A * volume_B / (volume_A + volume_B), the combined volume here is about 527 cm³ and the gradient from volume_B reaches the bar.
There was a problem hiding this comment.
@zhx06 If the agent is correct, this seems like it could be an issue?
There was a problem hiding this comment.
Yes, the issue is real. We adopt the harmonic mean suggestion, and the valid rate increases a lot.
🤖 Isaac Lab-Arena Review BotSummaryThis PR switches Design, Boundaries & Scope
Findings🟡 Warning: 🔵 Improvement: 🔵 Improvement: 🔵 Improvement: 🔵 Improvement: Test Coverage and CI Cost
VerdictMinor fixes needed |
alexmillane
left a comment
alexmillane
left a comment
There was a problem hiding this comment.
LGTM.
The only functional issue is the one that the bot commented on about the loss being zero even when there's overlap in some cases.
My additional small comments are about breaking up some functions, moving some code to different places.
| state: RelationSolverState, | ||
| no_collision_strategy: NoCollisionLossStrategy, | ||
| clearance_m: float, | ||
| mesh_manager: WarpMeshAndSphereCache | None, |
There was a problem hiding this comment.
AI-drafted (Claude Code), reviewed by me.
Design: compute_no_overlap_loss_bbox does mesh routing
compute_no_overlap_loss_bbox only computes box losses, but it takes mesh_manager, default_collision_mode and skip_mesh_pairs so that prepare_no_overlap_pair_cache can decide which pairs it handles:
_fixed_pair_is_covered_by_mesh_collisionand_dynamic_pair_is_covered_by_mesh_collisiondrop pairs that the mesh loss covers.- The
mesh_objectsset sends pairs that involve a mesh object tomesh_fallback_pairs.
In BBOX-only mode, RelationSolver._compute_no_overlap_loss still passes self._mesh_manager, but without skip_mesh_pairs the argument is never used.
This makes the bbox module depend on mesh concerns. It also repeats the "is this pair covered by the mesh loss" rule from _collect_mesh_pairs in no_overlap_mesh.py. If the two copies drift apart, a pair is either penalized by both losses or by neither.
This builds on the bot's comment on relation_solver_state.py:155. Could RelationSolver.solve decide the routing once, next to prepare_mesh_collision_cache, and pass only the resulting pair lists to compute_no_overlap_loss_bbox? Ideally both losses would get their pairs from that one place, so the bbox module no longer needs mesh_manager.
There was a problem hiding this comment.
Addressed. Routing is in solve now. We leave shared router in a follow-up
| return AxisAlignedBoundingBox(min_point=rotated.amin(dim=1), max_point=rotated.amax(dim=1)) | ||
|
|
||
|
|
||
| class OverlapVolumeUpperBound: |
There was a problem hiding this comment.
AI-drafted (Claude Code), reviewed by me.
Question: does caching in OverlapVolumeUpperBound pay off?
OverlapVolumeUpperBound.__init__ caches _R_A_W, _R_B_A and the projected radii once per solve, and compute() reuses them every iteration. On main, _oriented_projected_overlap_volume recomputed these terms on each call.
Does the cache measurably speed up a solver iteration? The PR's performance numbers compare AABB with OBB placement, not cached with uncached, and they show per-iteration time and peak memory going up. The cached tensors scale with pairs × candidates × 9, so they may explain much of the memory increase.
If the gain is small, keeping the plain function would be simpler. BoundingBox.overlap_volume, which builds an instance and calls compute() once, wouldn't need the class either.
There was a problem hiding this comment.
I measured cached against uncached, recomputing the rotations and projected radii every call:
| Objects × candidates | CPU ms per iteration (main / cached / uncached) | Peak GPU memory, MiB (main / cached / uncached) |
|---|---|---|
| 6 × 640 | 3.2 / 3.5 / 3.7 | 2.4 / 6.6 / 7.9 |
| 12 × 1024 | 7.3 / 9.8 / 12.4 | 15.2 / 44.4 / 53.2 |
| 25 × 1024 | 26.5 / 37.0 / 43.8 | 65.8 / 194.6 / 236.5 |
Dropping the cache makes iterations 6–26% slower and raises peak memory by 16–22%, because autograd keeps the recomputed terms for the backward pass. The increase over main comes from storing a rotation per pair and candidate, which the uncached version needs too.
So I kept the cache and renamed it CachedOverlapVolumeUpperBound.
|
|
||
|
|
||
| class OverlapVolumeUpperBound: | ||
| """Overlap-volume upper bound for box pairs whose half-extents and rotations stay fixed while centers move. |
There was a problem hiding this comment.
AI-drafted (Claude Code), reviewed by me.
Suggestion (non-blocking): if the cache stays, name it as one and move it next to the solver
This follows from the question above. If caching does measurably speed up the solver, the class is solver acceleration rather than bounding-box geometry, and its name and location should say so.
- Name:
OverlapVolumeUpperBoundreads like a geometric quantity, but the class holds precomputed per-pair terms for repeated evaluation. A name such asCachedOverlapVolumeUpperBoundorOverlapVolumeUpperBoundCachewould make that clear to callers. - Location: its only real user is the solver loss in
no_overlap_bbox.py(prepare_no_overlap_pair_cacheand_compute_oriented_pair_losses). It could live there, or in another module underrelations/, alongsideNoOverlapPairCache.
BoundingBox.overlap_volume (line 125) would then go back to calling a plain function, as on main, since utils/ shouldn't import from relations/. The moved class would need that function, or _interval_overlap_volume, exposed as a public helper in bounding_box.py so the geometry is still defined once.
There was a problem hiding this comment.
Addressed. kept in utils/ to avoid a second copy
| def prepare_no_overlap_pair_cache( | ||
| state: RelationSolverState, | ||
| mesh_manager: WarpMeshAndSphereCache | None, | ||
| default_collision_mode: CollisionMode, | ||
| skip_mesh_pairs: bool, | ||
| ) -> NoOverlapPairCache: | ||
| """Prepare fixed pair routing and the overlap-volume upper bound after mesh geometry is available. | ||
|
|
||
| Args: | ||
| state: Solver state with fixed collision bounds and object relations. | ||
| mesh_manager: Cache supplying available collision meshes, or None. | ||
| default_collision_mode: Mode for objects without an explicit override. | ||
| skip_mesh_pairs: Exclude pairs represented by the mesh collision loss. | ||
|
|
||
| Returns: | ||
| Ordered pair groups and the prepared upper bound for this solve. | ||
| """ | ||
| movable = state.optimizable_objects | ||
| anchors = list(state.anchor_objects) | ||
| fixed = anchors + state.collision_objects | ||
| on_pairs = set() | ||
| for obj in [*movable, *anchors]: | ||
| for relation in obj.get_relations(): | ||
| if isinstance(relation, On): | ||
| on_pairs.add((id(obj), id(relation.parent))) | ||
| on_pairs.add((id(relation.parent), id(obj))) | ||
|
|
||
| pairs = [] | ||
| for child in movable: | ||
| for obstacle in fixed: | ||
| if (id(child), id(obstacle)) in on_pairs: | ||
| continue | ||
| if ( | ||
| skip_mesh_pairs | ||
| and mesh_manager is not None | ||
| and _fixed_pair_is_covered_by_mesh_collision( | ||
| state, child, obstacle, mesh_manager, default_collision_mode | ||
| ) | ||
| ): | ||
| continue | ||
| pairs.append(NoOverlapPair(child, obstacle)) | ||
| for index, child in enumerate(movable): | ||
| for other in movable[index + 1 :]: | ||
| if (id(child), id(other)) in on_pairs: | ||
| continue | ||
| if ( | ||
| skip_mesh_pairs | ||
| and mesh_manager is not None | ||
| and _dynamic_pair_is_covered_by_mesh_collision( | ||
| state, child, other, mesh_manager, default_collision_mode | ||
| ) | ||
| ): | ||
| continue | ||
| pairs.extend((NoOverlapPair(child, other), NoOverlapPair(other, child))) | ||
| if not pairs: | ||
| return NoOverlapPairCache([], [], None) | ||
|
|
||
| mesh_objects = set() | ||
| if skip_mesh_pairs and mesh_manager is not None: | ||
| for obj in [*movable, *fixed]: | ||
| if ( | ||
| object_uses_mesh_collision(obj, default_collision_mode) | ||
| and mesh_manager.get_collision_mesh(obj) is not None | ||
| ): | ||
| mesh_objects.add(obj) | ||
| box_pairs, mesh_fallback_pairs = [], [] | ||
| background_objects = set(state.collision_objects) | ||
| for pair in pairs: | ||
| uses_mesh_validation = pair.obstacle in mesh_objects or ( | ||
| pair.subject in mesh_objects and pair.obstacle not in background_objects | ||
| ) | ||
| if uses_mesh_validation: | ||
| # Mesh validation uses enclosing-box sphere proxies for these unsupported pairs. | ||
| mesh_fallback_pairs.append(pair) | ||
| else: | ||
| box_pairs.append(pair) | ||
|
|
||
| overlap_bound = None | ||
| if box_pairs and not state.collision_boxes_axis_aligned: | ||
| geometry = {} | ||
| for pair in box_pairs: | ||
| for obj in (pair.subject, pair.obstacle): | ||
| if obj not in geometry: | ||
| geometry[obj] = state.get_collision_geometry(obj) | ||
| overlap_bound = OverlapVolumeUpperBound( | ||
| torch.stack([geometry[p.subject][1] for p in box_pairs]).clamp_min(_MIN_LOSS_HALF_EXTENT_M), | ||
| torch.stack([geometry[p.subject][2] for p in box_pairs]), | ||
| torch.stack([geometry[p.obstacle][1].detach() for p in box_pairs]).clamp_min(_MIN_LOSS_HALF_EXTENT_M), | ||
| torch.stack([geometry[p.obstacle][2].detach() for p in box_pairs]), | ||
| ) | ||
| return NoOverlapPairCache(box_pairs, mesh_fallback_pairs, overlap_bound) |
There was a problem hiding this comment.
AI-drafted (Claude Code), reviewed by me.
Suggestion (non-blocking): split prepare_no_overlap_pair_cache into its steps
prepare_no_overlap_pair_cache is 74 lines and runs four separate steps one after another:
- Build the
On-linkedon_pairsset. - List directed pairs (movable vs fixed, then movable vs movable), skipping pairs the mesh loss covers.
- Split them into
box_pairsandmesh_fallback_pairsusing themesh_objectsset. - Gather, stack and clamp geometry to build
OverlapVolumeUpperBound.
Could each step be a small helper, for example _get_on_linked_pairs, _list_no_overlap_pairs, _split_mesh_fallback_pairs and _build_overlap_bound? This function would then be about eight lines that read as the outline above.
_get_on_linked_pairscould also replace the identicalon_pairsloop inRelationSolver.solve, as the bot's pair-cache comment onrelation_solver_state.py:155suggests.- Steps 1–3 are the routing from the
mesh_managercomment, so having them as separate helpers would make them easy to move intosolve.
There was a problem hiding this comment.
Done: split into three helpers, and get_on_linked_pairs is now shared with the mesh cache in solve
| collision_bboxes_with_defaults = dict(collision_bboxes or {}) | ||
| base_bounds = {} | ||
| for obj in objects: | ||
| # Anchors use their fixed world bounds, so they need no candidate collision bounds. | ||
| if obj.is_anchor or obj in collision_bboxes_with_defaults: | ||
| continue | ||
| if env_bboxes is not None and obj in env_bboxes: | ||
| if env_bboxes_include_yaw: | ||
| collision_bboxes_with_defaults[obj] = env_bboxes[obj] | ||
| else: | ||
| base_bounds[obj] = env_bboxes[obj] | ||
| else: | ||
| base_bounds[obj] = obj.get_bounding_box() | ||
| if base_bounds: | ||
| collision_bboxes_with_defaults.update( | ||
| get_candidate_collision_bboxes( | ||
| list(base_bounds), base_bounds, orientations or [{} for _ in initial_positions] | ||
| ) | ||
| ) | ||
| state = RelationSolverState( | ||
| objects, initial_positions, device=device, env_bboxes=env_bboxes, collision_objects=collision_objects | ||
| objects, | ||
| initial_positions, | ||
| device=device, | ||
| env_bboxes=env_bboxes, | ||
| collision_objects=collision_objects, | ||
| collision_bboxes=collision_bboxes_with_defaults, |
There was a problem hiding this comment.
AI-drafted (Claude Code), reviewed by me.
Suggestion (non-blocking): extract the collision-bounds defaulting from solve
RelationSolver.solve is already about 145 lines, and this block adds 20 more. It does one thing: fill in collision bounds for movable objects that have no collision_bboxes entry, either from env_bboxes (unchanged when env_bboxes_include_yaw, otherwise rotated by orientations) or from get_bounding_box().
Could it be a helper such as _get_collision_bboxes_with_defaults(objects, env_bboxes, env_bboxes_include_yaw, orientations, collision_bboxes, batch_size)? solve would then read as: get collision bounds, build state, run the loop.
Related: RelationSolverState.__init__ falls back to enclosing bounds for objects missing from collision_bboxes, so the defaulting logic is currently split between the two. A helper would be a natural single home for it.
There was a problem hiding this comment.
Done: extracted _get_collision_bboxes_with_defaults, so solve now reads as get bounds, build state, run the loop. The state keeps a small fallback only for states built outside solve, as in tests and debug_losses.
| self._enclosing_collision_bboxes = dict(self._fixed_obstacle_world_bboxes) | ||
| for obj in self._optimizable_objects: | ||
| self._enclosing_collision_bboxes[obj] = self.get_bbox(obj) | ||
| self._collision_bboxes: dict[PlaceableAsset | CollisionObject, BoundingBox] = dict( | ||
| self._fixed_obstacle_world_bboxes | ||
| ) | ||
| for obj in self._optimizable_objects: | ||
| bounds = ( | ||
| collision_bboxes[obj] | ||
| if collision_bboxes is not None and obj in collision_bboxes | ||
| else self._enclosing_collision_bboxes[obj] | ||
| ) | ||
| self._collision_bboxes[obj] = bounds.to(self._device) | ||
| self._collision_boxes_axis_aligned = all( | ||
| isinstance(box, AxisAlignedBoundingBox) for box in self._collision_bboxes.values() | ||
| ) | ||
| self._collision_geometry = {} | ||
| """Fixed center offsets (B, 3), half-lengths (B, 3), and axes (B, 3, 3), with B = batch size.""" | ||
| if not self.collision_boxes_axis_aligned: | ||
| for obj in self._collision_bboxes: | ||
| self.get_collision_geometry(obj) | ||
| self._no_overlap_pair_cache: NoOverlapPairCache | None = None | ||
| """Pair routing and the prepared upper bound for the current collision configuration.""" | ||
| self._no_overlap_pair_cache_key: tuple[WarpMeshAndSphereCache | None, CollisionMode, bool] | None = None |
There was a problem hiding this comment.
AI-drafted (Claude Code), reviewed by me.
Suggestion (non-blocking): move collision-bounds setup out of RelationSolverState.__init__
These lines make __init__ about 93 lines long. They do one self-contained job: build _enclosing_collision_bboxes and _collision_bboxes, set _collision_boxes_axis_aligned, precompute geometry, and initialize the pair-cache fields.
Could they be one method, for example _build_collision_bboxes(collision_bboxes), called from __init__?
This pairs with the bot's comment on line 124 about the boxes being stored three times. With the setup in one method, that cleanup touches a single place. If the pair cache moves into RelationSolver.solve, as the bot's line-155 comment suggests, the cache fields leave this block too.
There was a problem hiding this comment.
Done: the setup is now _build_collision_bboxes, called from __init__. With the bot's two suggestions also applied, it only stores collision boxes for movable objects: the enclosing copy and the pair-cache fields are gone, and the pair cache now lives in RelationSolver.
| centers_W[obj] = geometry_center | ||
| subject_centers_W = torch.stack([centers_W[p.subject] for p in pairs]) | ||
| obstacle_centers_W = torch.stack([centers_W[p.obstacle].detach() for p in pairs]) | ||
| return strategy.slope * overlap_bound.compute(subject_centers_W, obstacle_centers_W, margin=clearance_m) |
There was a problem hiding this comment.
@zhx06 If the agent is correct, this seems like it could be an issue?
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>
e695eea to
f2249a6
Compare
Signed-off-by: zhx06 <zihaox@nvidia.com>
Use oriented bounding boxes for placement collision loss and validation. Keep the existing interval calculation for axis-aligned boxes and batch collision checks across candidates. This builds on
OrientedBoundingBoxfrom #1477.Why this change?
Enclosing AABBs can overlap even when rotated objects are separate. Placement previously penalized and rejected those layouts, although the oriented geometry was available.
What changes?
overlap_volume_upper_boundduring optimization: the smaller of the interval-overlap volumes along either box's face axes.OverlapVolumeUpperBoundcaches the rotation-dependent terms once per solve.collision_bboxesstores oriented geometry once per batch. Existing candidate AABBs remain available for relation constraints and mesh proxies; their enclosing extents cannot recover the original rotated shape.CollisionMode.BBOXuses this automatically. Arbitrary-yaw results can change, including existing clutter examples. Support containment and clutter release initialization remain AABB-based; the existing rotated-support andOn(overlap=True)containment issues need a separate correction.Results
The placement-success benchmark (#1497) ran every RoboLab and KitchenBench spec on main and on this branch, with 50 layouts × 3 seeds per spec.
throw_away_snacks: layout success goes from 28% to 100%;marker_in_mug: valid candidates go from 7% to 65%.food_packing_1_boxesandfood_packing_1_cans(valid candidates 75% → 83%).Limitations
Tilted boxes: the loss can penalize tilted boxes that the separating-axis test finds separate.
Stuck overlaps: when one box's projections sit inside the other's on every face axis, the loss is constant and its gradient is zero, so the solver can leave deep overlaps. With the same oriented validator, sparse scenes with long objects do slightly worse with the oriented loss than with the enclosing-AABB loss:
A follow-up adds an in-plane enclosing-box term to rotated pairs only. In evaluation it raised random-yaw RoboLab valid candidates from 52.6% to 57.9%, with no task worse.
Near-quarter yaws: robot yaws such as
yaw_rad: 1.57in KitchenBench are not exact quarter turns, so those scenes take the oriented path. Positions matched main in a probe; only the cost differs.Performance
Compared with AABB placement at
aaf71703d4, using the maintained GR1 six-object tabletop geometry and relations reconstructed as benchmark assets, with random yaw and a denser variant. Both versions use the same three seeds, 64 environments, ten candidates per environment, 600 iterations and eight CPU threads; Intel Core Ultra 9 285K and RTX 6000 Ada. These numbers were measured before the final review changes.Times are means across the three seeds and cover candidate generation, geometry preparation, solving, validation and ranking. They exclude asset loading and physics.
Validation is now batched across candidates, which accounts for most of the total-time gain. Each solver iteration costs more, and the cached terms grow with directed pairs × candidates. These numbers are indicative, measured on a shared machine:
Full-suite wall time as authored went from 723 to 603 s on RoboLab and from 648 to 590 s on KitchenBench. Both arms ran in parallel on a shared machine.
These are placement measurements, not physics-acceptance rates.
Validation
843d8fb656and this branch ate695eeab3d. All 69 specs ran with no errors in both settings.ObjectPlacer, zero-thickness panels against a fixed block went from 14/64 to 36/64 valid candidates.