Repository navigation
[TEST] [DO NOT REVIEW] Review agent on OBB - #1489
alexmillane wants to merge 1 commit into
Conversation
Signed-off-by: zhx06 <zihaox@nvidia.com>
 [Medium impact] The PR appears safe to merge; no actionable issues were found. SummaryAdds
No actionable issues found. Tests were not run. Reviews (1) · Last reviewed commit: "introduce OBB" · Reviewed by Greptile |
|
/review |
1 similar comment
|
/review |
![arena-review-bot[bot]](https://avatars.githubusercontent.com/u/195794629?s=60&v=4){kind=small}
| return normalized.amin(dim=-1).clamp_min(0) | ||
|
|
||
|
|
||
| class AxisAlignedBoundingBox(OrientedBoundingBox): |
There was a problem hiding this comment.
🟡 AABB doesn't run the OBB constructor
AxisAlignedBoundingBox.__init__ never calls OrientedBoundingBox.__init__, so an AABB has no _center, _half_extents, _rotation_xyzw or _axis_order. Every base member that reads them has to be overridden here; #1480 already had to add a second is_axis_aligned for this reason. A value typed OrientedBoundingBox can also be an empty, unbounded or later-edited AABB, so the base's finite, copy-on-read guarantees don't hold for the type.
Could both classes derive from a small abstract base that holds the shared SAT and transform code, with each class owning its own storage? Mixed queries already have to_obb(), and #1477 moves to this layout.
| depths, norms = self._sat_depths(other, margin) | ||
| return ((depths > 0) | (norms == 0)).all(dim=-1) | ||
|
|
||
| def penetration_depth(self, other: "OrientedBoundingBox", margin: float = 0.0) -> torch.Tensor: |
There was a problem hiding this comment.
🟡 Is penetration_depth needed yet?
Nothing outside the tests calls it, here or in the stacked #1480, whose solver optimizes positions only and validates with overlaps. It's also the only reason for the rotation.requires_grad branch in _axis_aligned_order (line 47) and for test_axis_aligned_rotation_gradients_use_generic_sat. #1477's description drops it because, as a solver loss, SAT depth often picks the vertical axis and lifts objects off their support.
Could we remove it together with that branch, and add it back when a caller needs it? get_bounds_along_axis raises the same question, since only tests call it. The depth checks in the SAT tests can become overlaps(..., margin=...) checks, as lines 109-110 of the test file already do.
| nonzero_count = (magnitudes != 0).sum(dim=-1) | ||
| equal_components = ((magnitudes == 0) | (magnitudes == magnitudes.amax(dim=-1, keepdim=True))).all(dim=-1) | ||
| # These quaternion patterns are exactly the 24 axis-aligned rotations; no angular tolerance is used. | ||
| aligned = equal_components & ((nonzero_count == 1) | (nonzero_count == 2) | (nonzero_count == 4)) |
There was a problem hiding this comment.
🔵 Exact match misses ordinary quarter-turns
The 3-axis path only saves work inside _sat_depths; it gives the same results as the 15-axis test. The exact check also misses common inputs. A 180° yaw from quat_from_euler_xyz is stored with w = -4.4e-8, and (0, 0, sin(pi/2), cos(pi/2)) with w = 6e-17. The X90-then-Z90 box in test_transform_composes_rotations_and_rotates_the_center comes out as (0.49999994, 0.5, 0.5, 0.5). All three silently take the slow path.
Do we need the fast path in this PR? If it stays for speed, could the check allow about 1e-7 of roundoff? That's still far from the 89.9° case, whose components differ by about 1e-3. A test with a 180° yaw from quat_from_euler_xyz would cover it.
| midpoint = (center * axis).sum(dim=-1) | ||
| return midpoint - radius, midpoint + radius | ||
|
|
||
| def get_axis_aligned_bounds(self) -> tuple[torch.Tensor, torch.Tensor]: |
There was a problem hiding this comment.
🔵 Third way to get the enclosing AABB after a rotation
transformed((0, 0, 0), q).to_aabb() now computes the same box as rotated_by_quat(q) and enclosing_after_rotation(q); test_aabb_transform_matches_enclosing_rotation checks it against rotated_by_quat. Could those two become thin wrappers over this, or could one of them go, so there's one implementation to maintain?
| """Oriented box geometry with float32 tensors, N rows and singleton broadcasting. | ||
|
|
||
| Centers are expressed in a common containing frame; each quaternion maps box-local | ||
| axes into that frame. Direct construction copies finite geometry, and its tensor | ||
| properties return copies. AxisAlignedBoundingBox additionally supports mutable, | ||
| empty or unbounded intervals. transformed(), get_bounds_along_axis() and SAT | ||
| queries require finite, nonempty geometry from either class. Fixed axis-aligned | ||
| batches use three-axis queries; other batches use the full separating-axis test. | ||
| """ |
There was a problem hiding this comment.
🔵 Docstring describes the subclass and internal code paths
Half of this docstring covers AxisAlignedBoundingBox and which SAT variant runs internally. AGENTS.md asks docstrings to cover calling syntax and semantics, not implementation. The float32 notes in overlaps and penetration_depth could be trimmed the same way. Suggestion:
| """Oriented box geometry with float32 tensors, N rows and singleton broadcasting. | |
| Centers are expressed in a common containing frame; each quaternion maps box-local | |
| axes into that frame. Direct construction copies finite geometry, and its tensor | |
| properties return copies. AxisAlignedBoundingBox additionally supports mutable, | |
| empty or unbounded intervals. transformed(), get_bounds_along_axis() and SAT | |
| queries require finite, nonempty geometry from either class. Fixed axis-aligned | |
| batches use three-axis queries; other batches use the full separating-axis test. | |
| """ | |
| """Oriented boxes as float32 tensors with N rows and singleton broadcasting. | |
| Centers are expressed in a common containing frame; each quaternion maps box-local | |
| axes into that frame. Construction copies its inputs, and tensor properties return copies. | |
| """ |
| box = OrientedBoundingBox((0.0, 0.0, 0.0), (1.0, 0.25, 0.5), rotation) | ||
| target = OrientedBoundingBox((0.375, 0.1, 0.0), (0.25, 0.25, 0.25)) | ||
| reference = OrientedBoundingBox((0.0, 0.0, 0.0), (1.0, 0.25, 0.5), rotation) | ||
| reference._axis_order = None |
There was a problem hiding this comment.
🔵 reference is built the same way as box
rotation requires grad, so _axis_aligned_order already returns None for both boxes, and this assignment changes nothing. Lines 451-452 then compare penetration_depth and its gradient with themselves; only lines 446 and 453 check anything. If the test stays, could the expected gradient come from an independent source, such as a finite difference on rotation? If penetration_depth goes, this test goes with it.
🤖 Isaac Lab-Arena Review BotSummaryThis review covers the same commit as the earlier bot review ( Design, Boundaries & ScopeThe design points from the earlier review still stand. Their inline threads have the details:
FindingsNew in this review:
Still open from the earlier review:
The line-53 thread treats quarter-turns missed by the exact check as a speed issue only. With a margin, they can also change the result (see line 267). Test Coverage and CI Cost
VerdictSignificant concerns |
|
/review post merge https://github.com/cvolkcvolk/arena-review-bot/pull/10 |
| not Euclidean clearance. | ||
| """ | ||
| depths, norms = self._sat_depths(other, margin) | ||
| return ((depths > 0) | (norms == 0)).all(dim=-1) |
There was a problem hiding this comment.
🟡 Margin result depends on quaternion rounding
Only cross axes with a norm of exactly 0 are dropped. When the quaternions of boxes with parallel edges carry float32 rounding, those axes come out with norms around 1e-7 and arbitrary directions, and with margin > 0 they can decide the result.
Example: a quarter-turn box built directly as (-0.5, -0.5, 0.5, -0.5), and the same box built with transformed() (corners within 1.8e-7). Against a box 0.1 away along X and Y, overlaps(other, margin=0.13) returns True for the first and False for the second. Two 30°-yawed boxes, one built through transformed(), disagree the same way at margin 0.11. #1480 validates placements with overlaps(..., margin=clearance_m), so the same layout could pass or fail depending on how its rotation was computed.
Could axes below a small norm count as parallel? With a 1e-6 cutoff (float32 rotation noise is about 1e-7), both examples agree and the PR's tests still pass:
| return ((depths > 0) | (norms == 0)).all(dim=-1) | |
| return ((depths > 0) | (norms <= _PARALLEL_EDGE_AXIS_NORM)).all(dim=-1) |
This needs _PARALLEL_EDGE_AXIS_NORM = 1e-6 at module level, the same cutoff in penetration_depth (line 283), and an update to the docstring at line 257. A test that builds one box directly and through transformed(), then compares their margin results, would cover it.
| from isaaclab.utils.math import quat_apply, quat_mul | ||
|
|
||
| center, half_extents, rotation = self._get_finite_geometry() | ||
| translation = _batched_tensor(t_A_B, 3, center.device) |
There was a problem hiding this comment.
🔵 Frame names dropped in transformed
t_A_B and q_A_B become translation and quaternion right away, and the box's own center and rotation carry no frame. That makes the last line hard to check against T_A_O = T_A_B * T_B_O. AGENTS.md asks for frame-named values here, and arena_world.py:277 already writes the same step as centroid_W = quat_apply(q_W_F, centroid_F) + t_W_F. Something like this for lines 158-166 (the PR's tests still pass):
# Centers are expressed in frame B; rotations map the box-local frame O into B.
center_B, half_extents, q_B_O = self._get_finite_geometry()
t_A_B = _batched_tensor(t_A_B, 3, center_B.device)
q_A_B = _unit_quaternion(_batched_tensor(q_A_B, 4, center_B.device))
center_B, half_extents, q_B_O, t_A_B, q_A_B = _broadcast_rows(center_B, half_extents, q_B_O, t_A_B, q_A_B)
center_A = quat_apply(q_A_B, center_B) + t_A_B
q_A_O = quat_mul(q_A_B, q_B_O)
return OrientedBoundingBox(center_A, half_extents, q_A_O)| New AxisAlignedBoundingBox with translated position. | ||
| """ | ||
| offset = self._to_batched_tensor(offset) | ||
| offset = _batched_tensor(offset, 3, self._min_point.device) |
There was a problem hiding this comment.
🔵 Two helpers that batch tensors
The new _batched_tensor does the same job as _to_batched_tensor, and also places tuples on the box's device and checks shapes. Only translated was switched to it. The constructor (lines 304-305) and scaled (line 438) still use the old helper. So on a CUDA box, translated((1.0, 2.0, 3.0)) now works, but scaled((2.0, 2.0, 2.0)) still builds its scale on the CPU and fails.
Could _to_batched_tensor go? The constructor would use _batched_tensor(min_point, 3) and _batched_tensor(max_point, 3), and scaled would use _batched_tensor(scale, 3, self._min_point.device). With that change, test_bounding_box.py and the new tests still pass.
Test MR to check the review agent