From 03c3d6696fe817b89ad14a8275a666be74300751 Mon Sep 17 00:00:00 2001 From: Petrukovich Evgeny Date: Tue, 14 Jan 2025 12:22:32 +0300 Subject: [PATCH] fixes by review --- src/bvh/bvh_impl.rs | 22 ++++---- ...traverse.rs => child_distance_traverse.rs} | 8 +-- src/bvh/distance_traverse.rs | 54 +++++++++---------- src/bvh/mod.rs | 4 +- 4 files changed, 43 insertions(+), 45 deletions(-) rename src/bvh/{distance_by_child_traverse.rs => child_distance_traverse.rs} (98%) diff --git a/src/bvh/bvh_impl.rs b/src/bvh/bvh_impl.rs index 0a7fb31..2da2069 100644 --- a/src/bvh/bvh_impl.rs +++ b/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 Bvh { 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>( + pub fn nearest_child_traverse_iterator<'bvh, 'shape, Shape: Bounded>( &'bvh self, ray: &'bvh Ray, 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 Bvh { &'bvh self, ray: &'bvh Ray, 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. diff --git a/src/bvh/distance_by_child_traverse.rs b/src/bvh/child_distance_traverse.rs similarity index 98% rename from src/bvh/distance_by_child_traverse.rs rename to src/bvh/child_distance_traverse.rs index 5418874..758af57 100644 --- a/src/bvh/distance_by_child_traverse.rs +++ b/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, 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, ray: &'bvh Ray, shapes: &'shape [Shape]) -> Self { - DistanceByChildTraverseIterator { + ChildDistanceTraverseIterator { bvh, ray, shapes, @@ -177,7 +177,7 @@ where } impl<'shape, T, const D: usize, Shape: Bounded, const ASCENDING: bool> Iterator - for DistanceByChildTraverseIterator<'_, 'shape, T, D, Shape, ASCENDING> + for ChildDistanceTraverseIterator<'_, 'shape, T, D, Shape, ASCENDING> where T: BHValue, { diff --git a/src/bvh/distance_traverse.rs b/src/bvh/distance_traverse.rs index c568aa2..883d9f4 100644 --- a/src/bvh/distance_traverse.rs +++ b/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 PartialEq for DistNodePair { } } -#[allow(clippy::non_canonical_partial_ord_impl)] impl PartialOrd for DistNodePair { fn partial_cmp(&self, other: &Self) -> Option { - self.dist.partial_cmp(&other.dist) + Some(self.cmp(other)) } } impl Ord for DistNodePair { 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) { + 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), diff --git a/src/bvh/mod.rs b/src/bvh/mod.rs index a171521..ea1180f 100644 --- a/src/bvh/mod.rs +++ b/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::*;