fixes by review

src/bvh/bvh_impl.rs

@@ -13,8 +13,8 @@ use crate::utils::joint_aabb_of_shapes;
 use std::mem::MaybeUninit;
 
 use super::{
-    BvhNode, BvhNodeBuildArgs, DistanceByChildTraverseIterator, DistanceTraverseIterator,
-    ShapeIndex, Shapes,
+    BvhNode, BvhNodeBuildArgs, ChildDistanceTraverseIterator, DistanceTraverseIterator, ShapeIndex,
+    Shapes,
 };
 
 /// The [`Bvh`] data structure. Contains the list of [`BvhNode`]s.
@@ -166,36 +166,36 @@ impl<T: BHValue, const D: usize> Bvh<T, D> {
         DistanceTraverseIterator::new(self, ray, shapes)
     }
 
-    /// Creates a [`DistanceTraverseIterator`] to traverse the [`Bvh`].
+    /// Creates a [`ChildDistanceTraverseIterator`] to traverse the [`Bvh`].
     /// Returns a subset of [`shape`], in which the [`Aabb`]s of the elements were hit by [`Ray`].
     /// Return in order from nearest to farthest for ray.
     ///
     /// This is a best-effort function that orders interior parent nodes before ordering child
     /// nodes, so the output is not necessarily perfectly sorted.
     ///
-    /// Useful if childs don't intersect each other.
+    /// So the output is not necessarily perfectly sorted if the children of any node overlap.
     ///
     /// Time complexity: for first O(log(n)), for all O(n).
     ///
     /// [`Bvh`]: struct.Bvh.html
     /// [`Aabb`]: ../aabb/struct.AABB.html
     ///
-    pub fn nearest_by_child_traverse_iterator<'bvh, 'shape, Shape: Bounded<T, D>>(
+    pub fn nearest_child_traverse_iterator<'bvh, 'shape, Shape: Bounded<T, D>>(
         &'bvh self,
         ray: &'bvh Ray<T, D>,
         shapes: &'shape [Shape],
-    ) -> DistanceByChildTraverseIterator<'bvh, 'shape, T, D, Shape, true> {
-        DistanceByChildTraverseIterator::new(self, ray, shapes)
+    ) -> ChildDistanceTraverseIterator<'bvh, 'shape, T, D, Shape, true> {
+        ChildDistanceTraverseIterator::new(self, ray, shapes)
     }
 
-    /// Creates a [`DistanceTraverseIterator`] to traverse the [`Bvh`].
+    /// Creates a [`ChildDistanceTraverseIterator`] to traverse the [`Bvh`].
     /// Returns a subset of [`Shape`], in which the [`Aabb`]s of the elements were hit by [`Ray`].
     /// Return in order from farthest to nearest for ray.
     ///
     /// This is a best-effort function that orders interior parent nodes before ordering child
     /// nodes, so the output is not necessarily perfectly sorted.
     ///
-    /// Useful if childs don't intersect each other.
+    /// So the output is not necessarily perfectly sorted if the children of any node overlap.
     ///
     /// Time complexity: for first O(log(n)), for all O(n).
     ///
@@ -206,8 +206,8 @@ impl<T: BHValue, const D: usize> Bvh<T, D> {
         &'bvh self,
         ray: &'bvh Ray<T, D>,
         shapes: &'shape [Shape],
-    ) -> DistanceByChildTraverseIterator<'bvh, 'shape, T, D, Shape, false> {
-        DistanceByChildTraverseIterator::new(self, ray, shapes)
+    ) -> ChildDistanceTraverseIterator<'bvh, 'shape, T, D, Shape, false> {
+        ChildDistanceTraverseIterator::new(self, ray, shapes)
     }
 
     /// Prints the [`Bvh`] in a tree-like visualization.

src/bvh/distance_by_child_traverse.rs → src/bvh/child_distance_traverse.rs

@@ -15,7 +15,7 @@ enum RestChild {
 ///
 /// This is a best-effort iterator that orders interior parent nodes before ordering child
 /// nodes, so the output is not necessarily perfectly sorted.
-pub struct DistanceByChildTraverseIterator<
+pub struct ChildDistanceTraverseIterator<
     'bvh,
     'shape,
     T: BHValue,
@@ -40,13 +40,13 @@ pub struct DistanceByChildTraverseIterator<
 }
 
 impl<'bvh, 'shape, T, const D: usize, Shape: Bounded<T, D>, const ASCENDING: bool>
-    DistanceByChildTraverseIterator<'bvh, 'shape, T, D, Shape, ASCENDING>
+    ChildDistanceTraverseIterator<'bvh, 'shape, T, D, Shape, ASCENDING>
 where
     T: BHValue,
 {
     /// Creates a new [`DistanceTraverseIterator`]
     pub fn new(bvh: &'bvh Bvh<T, D>, ray: &'bvh Ray<T, D>, shapes: &'shape [Shape]) -> Self {
-        DistanceByChildTraverseIterator {
+        ChildDistanceTraverseIterator {
             bvh,
             ray,
             shapes,
@@ -177,7 +177,7 @@ where
 }
 
 impl<'shape, T, const D: usize, Shape: Bounded<T, D>, const ASCENDING: bool> Iterator
-    for DistanceByChildTraverseIterator<'_, 'shape, T, D, Shape, ASCENDING>
+    for ChildDistanceTraverseIterator<'_, 'shape, T, D, Shape, ASCENDING>
 where
     T: BHValue,
 {

src/bvh/distance_traverse.rs

@@ -1,7 +1,7 @@
 use std::cmp::Ordering;
 use std::collections::BinaryHeap;
 
-use crate::aabb::Bounded;
+use crate::aabb::{Aabb, Bounded};
 use crate::bounding_hierarchy::BHValue;
 use crate::bvh::{iter_initially_has_node, Bvh, BvhNode};
 use crate::ray::Ray;
@@ -20,16 +20,15 @@ impl<T: PartialOrd> PartialEq<Self> for DistNodePair<T> {
     }
 }
 
-#[allow(clippy::non_canonical_partial_ord_impl)]
 impl<T: PartialOrd> PartialOrd<Self> for DistNodePair<T> {
     fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
-        self.dist.partial_cmp(&other.dist)
+        Some(self.cmp(other))
     }
 }
 
 impl<T: PartialOrd> Ord for DistNodePair<T> {
     fn cmp(&self, other: &Self) -> Ordering {
-        self.partial_cmp(other).unwrap()
+        self.dist.partial_cmp(&other.dist).unwrap()
     }
 }
 
@@ -89,38 +88,37 @@ where
                 ref child_r_aabb,
                 ..
             } => {
-                let left_dist_opt = self.ray.intersection_slice_for_aabb(child_l_aabb);
-                let right_dist_opt = self.ray.intersection_slice_for_aabb(child_r_aabb);
-
-                if let Some((left_dist_min, left_dist_max)) = left_dist_opt {
-                    // left child has intersection with ray
-                    let left_dist = if ASCENDING {
-                        left_dist_min
-                    } else {
-                        left_dist_max
-                    };
-                    self.add_to_heap(left_dist, child_l_index);
-                };
-                if let Some((right_dist_min, right_dist_max)) = right_dist_opt {
-                    // right child has intersection with ray
-                    let right_dist = if ASCENDING {
-                        right_dist_min
-                    } else {
-                        right_dist_max
-                    };
-                    self.add_to_heap(right_dist, child_r_index);
-                };
-
+                self.process_child_intersection(child_l_index, child_l_aabb);
+                self.process_child_intersection(child_r_index, child_r_aabb);
                 None
             }
             BvhNode::Leaf { shape_index, .. } => Some(shape_index),
         }
     }
 
+    /// Intersect child node with a ray and add it to the heap.
+    fn process_child_intersection(&mut self, child_node_index: usize, child_aabb: &Aabb<T, D>) {
+        let dists_opt = self.ray.intersection_slice_for_aabb(child_aabb);
+
+        // if there is an intersection
+        if let Some((dist_to_entry_point, dist_to_exit_point)) = dists_opt {
+            let dist_to_compare = if ASCENDING {
+                // cause our iterator from nearest to farthest shapes,
+                // we compare them by first intersection point - entry point
+                dist_to_entry_point
+            } else {
+                // cause our iterator from farthest to nearest shapes,
+                // we compare them by second intersection point - exit point
+                dist_to_exit_point
+            };
+            self.add_to_heap(dist_to_compare, child_node_index);
+        };
+    }
+
     fn add_to_heap(&mut self, dist_to_node: T, node_index: usize) {
         // cause we use max-heap, it store max value on the top
         if ASCENDING {
-            // we need the smallest distance, so we negotiate value
+            // we need the smallest distance, so we negate value
             self.heap.push(DistNodePair {
                 dist: dist_to_node.neg(),
                 node_index,
@@ -292,7 +290,7 @@ mod tests {
     }
 
     #[test]
-    fn test_incorrect_order() {
+    fn test_overlapping_child_order() {
         let mut aabbs = [
             TAabb3 {
                 min: TPoint3::new(-0.33333334, -5000.3335, -5000.3335),

src/bvh/mod.rs

@@ -5,13 +5,13 @@
 
 mod bvh_impl;
 mod bvh_node;
-mod distance_by_child_traverse;
+mod child_distance_traverse;
 mod distance_traverse;
 mod iter;
 mod optimization;
 
 pub use self::bvh_impl::*;
 pub use self::bvh_node::*;
-pub use self::distance_by_child_traverse::*;
+pub use self::child_distance_traverse::*;
 pub use self::distance_traverse::*;
 pub use self::iter::*;