newton/geometry/broad_phase_sap.py (1)

363-366: Consider implementing adaptive sweep direction selection.

The fixed sweep direction works but could be suboptimal for certain object distributions. The TODO comment correctly identifies this optimization opportunity.

For better performance across different scenarios, consider:

• Using principal component analysis (PCA) on the bounding box centers to find the direction of maximum variance
• Caching and periodically updating the sweep direction based on object movement patterns
• Allowing users to specify a custom sweep direction for domain-specific optimizations

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

newton/tests/test_broad_phase.py (3)

153-178: Consider refactoring duplicated verbose debugging code.

The verbose debugging section is duplicated across all three test methods with identical logic. This increases maintenance burden and violates the DRY principle.

Consider extracting the verbose debugging logic into a helper method:

+    def _print_verbose_debug(self, pairs_wp, num_candidate_pair, geom_bounding_box_lower, 
+                            geom_bounding_box_upper, np_geom_cutoff, np_collision_group=None):
+        """Helper method to print verbose debugging information for collision pairs."""
+        print("Warp contact pairs:")
+        for i in range(num_candidate_pair):
+            pair = pairs_wp[i]
+            body_a, body_b = pair[0], pair[1]
+            if np_collision_group is not None:
+                group_a = np_collision_group[body_a]
+                group_b = np_collision_group[body_b]
+                print(f"  Pair {i}: bodies ({body_a}, {body_b}) with collision groups ({group_a}, {group_b})")
+            else:
+                print(f"  Pair {i}: bodies ({body_a}, {body_b})")
+
+        print("Checking if bounding boxes actually overlap:")
+        for i in range(num_candidate_pair):
+            pair = pairs_wp[i]
+            body_a, body_b = pair[0], pair[1]
+            
+            # Get bounding boxes and cutoffs for both bodies
+            box_a_lower = geom_bounding_box_lower[body_a]
+            box_a_upper = geom_bounding_box_upper[body_a]
+            box_b_lower = geom_bounding_box_lower[body_b]
+            box_b_upper = geom_bounding_box_upper[body_b]
+            cutoff_a = np_geom_cutoff[body_a]
+            cutoff_b = np_geom_cutoff[body_b]
+            
+            overlap = check_aabb_overlap_host(
+                wp.vec3(box_a_lower[0], box_a_lower[1], box_a_lower[2]),
+                wp.vec3(box_a_upper[0], box_a_upper[1], box_a_upper[2]),
+                cutoff_a,
+                wp.vec3(box_b_lower[0], box_b_lower[1], box_b_lower[2]),
+                wp.vec3(box_b_upper[0], box_b_upper[1], box_b_upper[2]),
+                cutoff_b,
+            )
+            print(f"  Pair {i}: bodies ({body_a}, {body_b}) - overlap: {overlap}")

Then replace the verbose sections in each test with:

         if verbose:
-            print("Warp contact pairs:")
-            for i in range(num_candidate_pair):
-                # ... existing verbose code ...
+            self._print_verbose_debug(pairs_wp, num_candidate_pair, geom_bounding_box_lower, 
+                                    geom_bounding_box_upper, np_geom_cutoff, np_collision_group)

---

435-436: Remove commented debug code.

The commented debug print statements should be removed to keep the code clean.

-            # print("pairs_np:", pairs_np)
-            # print("pairs_wp[:num_candidate_pair]:", pairs_wp[:num_candidate_pair])

---

79-80: Consider using constants for magic numbers.

The repeated magic numbers (30 for ngeom, 5 for num_groups) could be extracted to class constants for better maintainability.

 class TestBroadPhase(unittest.TestCase):
+    NGEOM = 30
+    NUM_GROUPS = 5
+    
     def test_nxn_broadphase(self):
         verbose = False
         
         # Create random bounding boxes in min-max format
-        ngeom = 30
+        ngeom = self.NGEOM
         
         # ... later in the method ...
-        num_groups = 5  # The zero group does not need to be counted
+        num_groups = self.NUM_GROUPS  # The zero group does not need to be counted

Also applies to: 196-197, 324-325

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro