Merge pull request #206 from Walther/2024-06-10-improvements

2024-06-10 improvements

clovers-cli/src/colorize.rs

@@ -85,33 +85,42 @@ pub fn colorize(
             specular * attenuation
         }
         MaterialType::Diffuse => {
-            // Use a probability density function to figure out where to scatter a new ray
-            // TODO: this weighed priority sampling should be adjusted or removed - doesn't feel ideal.
+            // Multiple Importance Sampling:
+
+            // Create a new PDF object from the priority hitables of the scene, given the current hit_record position
             let light_ptr = PDF::HitablePDF(HitablePDF::new(
                 &scene.priority_hitables,
                 hit_record.position,
             ));
+
+            // Create a mixture PDF from the above + the PDF from the scatter_record
             let mixture_pdf = MixturePDF::new(light_ptr, scatter_record.pdf_ptr);
-            let direction = mixture_pdf.generate(rng);
-            let direction = Unit::new_normalize(direction);
+
+            // Generate a direction for the scattering ray to go towards, weighed by the mixture PDF
+            let direction = Unit::new_normalize(mixture_pdf.generate(rng));
+
+            // Create the ray
             let scatter_ray = Ray {
                 origin: hit_record.position,
                 direction,
                 time: ray.time,
                 wavelength: ray.wavelength,
             };
+
+            // Get the distribution value for the PDF
+            // TODO: improve correctness & optimization!
             let pdf_val = mixture_pdf.value(scatter_ray.direction, ray.wavelength, ray.time, rng);
             if pdf_val <= 0.0 {
                 // scattering impossible, prevent division by zero below
                 // for more ctx, see https://github.com/RayTracing/raytracing.github.io/issues/979#issuecomment-1034517236
                 return emitted;
             }
 
-            // Calculate the PDF weighting for the scatter // TODO: understand the literature for this, and explain
-            let Some(scattering_pdf) =
-                hit_record
-                    .material
-                    .scattering_pdf(&hit_record, &scatter_ray, rng)
+            // Calculate the PDF weighting for the scatter
+            // TODO: improve correctness & optimization!
+            let Some(scattering_pdf) = hit_record
+                .material
+                .scattering_pdf(&hit_record, &scatter_ray)
             else {
                 // No scatter, only emit
                 return emitted;

clovers/src/bvhnode.rs

@@ -9,7 +9,7 @@ use crate::{
     hitable::{Empty, HitRecord, Hitable, HitableTrait},
     ray::Ray,
     wavelength::Wavelength,
-    Box, Direction, Float, Position, Vec,
+    Box, Direction, Displacement, Float, Position, Vec,
 };
 
 /// Bounding Volume Hierarchy Node.
@@ -212,9 +212,10 @@ impl<'scene> HitableTrait for BVHNode<'scene> {
         }
     }
 
+    // TODO: improve correctness & optimization!
     /// Returns a random point on the surface of one of the children
     #[must_use]
-    fn random(&self, origin: Position, rng: &mut SmallRng) -> Position {
+    fn random(&self, origin: Position, rng: &mut SmallRng) -> Displacement {
         match (&*self.left, &*self.right) {
             (_, Hitable::Empty(_)) => self.left.random(origin, rng),
             (Hitable::Empty(_), _) => self.right.random(origin, rng),

clovers/src/camera.rs

@@ -30,7 +30,7 @@ pub struct Camera {
     pub u: Direction,
     /// V
     pub v: Direction,
-    /// W
+    /// The forward direction of the camera; the difference between `look_from` and `look_at` normalized to a unit vector.
     pub w: Direction,
 }
 

clovers/src/hitable.rs

@@ -12,7 +12,7 @@ use crate::{
     objects::{Boxy, ConstantMedium, MovingSphere, Quad, RotateY, Sphere, Translate, Triangle},
     ray::Ray,
     wavelength::Wavelength,
-    Direction, Float, Position,
+    Direction, Displacement, Float, Position,
 };
 
 use enum_dispatch::enum_dispatch;
@@ -102,10 +102,6 @@ impl HitableTrait for Empty {
     ) -> Float {
         0.0
     }
-
-    fn random(&self, _origin: Position, _rng: &mut SmallRng) -> Position {
-        panic!("Hitable::Empty::random called!")
-    }
 }
 
 #[enum_dispatch]
@@ -138,7 +134,11 @@ pub trait HitableTrait {
 
     #[must_use]
     /// Random point on the entity, used for multiple importance sampling.
-    fn random(&self, origin: Position, rng: &mut SmallRng) -> Position;
+    fn random(&self, _origin: Position, _rng: &mut SmallRng) -> Displacement {
+        unimplemented!(
+            "HitableTrait::random called for a Hitable that has no implementation for it!"
+        );
+    }
 }
 
 /// Returns a tuple of `(front_face, normal)`. Used in lieu of `set_face_normal` in the Ray Tracing for the Rest Of Your Life book.

clovers/src/lib.rs

@@ -125,8 +125,10 @@ pub type Vec3 = Vector3<Float>;
 pub type Vec4 = Vector4<Float>;
 /// Internal type alias: a nalgebra [Unit] of a [Vector3]
 pub type Direction = Unit<Vec3>;
-/// Internal type alias: a nalgebra [Vector3]
+/// Internal type alias: a nalgebra [Vector3]. Intended as a world-space coordinate.
 pub type Position = Vec3;
+/// Internal type alias: a nalgebra [Vector3]. Intended for direction and length between two points, i.e. a direction with a non-unit length
+pub type Displacement = Vec3;
 /// Internal const: epsilon used for avoiding "shadow acne". This is mostly used for the initial minimum distance for ray hits after reflecting or scattering from a surface.
 pub const EPSILON_SHADOW_ACNE: Float = 0.001;
 /// Internal const: epsilon used for having a finitely-sized thickness for the bounding box of an infinitely-thin rectangle. Shouldn't be too small.

clovers/src/materials.rs

@@ -66,12 +66,9 @@ pub trait MaterialTrait: Debug {
     ) -> Option<ScatterRecord>;
 
     /// TODO: explain
-    fn scattering_pdf(
-        &self,
-        hit_record: &HitRecord,
-        scattered: &Ray,
-        rng: &mut SmallRng,
-    ) -> Option<Float>;
+    fn scattering_pdf(&self, _hit_record: &HitRecord, _scattered: &Ray) -> Option<Float> {
+        None
+    }
 
     /// Returns the emissivity of the material at the given position. Defaults to black as most materials don't emit - override when needed.
     fn emit(

clovers/src/materials/cone_light.rs

@@ -40,17 +40,6 @@ impl MaterialTrait for ConeLight {
         None
     }
 
-    /// Scattering probability density function for the [`ConeLight`] material. Always returns 0, as diffuse light does not scatter.
-    #[must_use]
-    fn scattering_pdf(
-        &self,
-        _hit_record: &HitRecord,
-        _scattered: &Ray,
-        _rng: &mut SmallRng,
-    ) -> Option<Float> {
-        None
-    }
-
     /// Emission function for [`ConeLight`]. If the given [`HitRecord`] has been hit on the `front_face`, emit a color based on the texture and surface coordinates. Otherwise, emit pure black.
     #[must_use]
     fn emit(

clovers/src/materials/dielectric.rs

@@ -75,16 +75,7 @@ impl MaterialTrait for Dielectric {
         })
     }
 
-    /// Scattering probability density function for Dielectric material. NOTE: not implemented!
-    #[must_use]
-    fn scattering_pdf(
-        &self,
-        _hit_record: &HitRecord,
-        _scattered: &Ray,
-        _rng: &mut SmallRng,
-    ) -> Option<Float> {
-        None // TODO: should a dielectric material scatter? how much?
-    }
+    // TODO: should this material provide a `scattering_pdf` function?
 }
 
 impl Dielectric {

clovers/src/materials/diffuse_light.rs

@@ -38,17 +38,6 @@ impl MaterialTrait for DiffuseLight {
         None
     }
 
-    /// Scattering probability density function for the [`DiffuseLight`] material. Always returns 0, as diffuse light does not scatter.
-    #[must_use]
-    fn scattering_pdf(
-        &self,
-        _hit_record: &HitRecord,
-        _scattered: &Ray,
-        _rng: &mut SmallRng,
-    ) -> Option<Float> {
-        None
-    }
-
     /// Emission function for [`DiffuseLight`]. If the given [`HitRecord`] has been hit on the `front_face`, emit a color based on the texture and surface coordinates. Otherwise, emit pure black.
     #[must_use]
     fn emit(

clovers/src/materials/dispersive.rs

@@ -117,12 +117,5 @@ impl MaterialTrait for Dispersive {
         // End copied
     }
 
-    fn scattering_pdf(
-        &self,
-        _hit_record: &HitRecord,
-        _scattered: &Ray,
-        _rng: &mut SmallRng,
-    ) -> Option<Float> {
-        None // TODO: should a dispersive material scatter? how much?
-    }
+    // TODO: should this material provide a `scattering_pdf` function?
 }

clovers/src/materials/gltf.rs

@@ -126,12 +126,7 @@ impl<'scene> MaterialTrait for GLTFMaterial<'scene> {
         }
     }
 
-    fn scattering_pdf(
-        &self,
-        hit_record: &HitRecord,
-        scattered: &Ray,
-        _rng: &mut SmallRng,
-    ) -> Option<Float> {
+    fn scattering_pdf(&self, hit_record: &HitRecord, scattered: &Ray) -> Option<Float> {
         // TODO: what should this be for GLTF materials?
         // Borrowed from Lambertian
         let cosine = hit_record.normal.dot(&scattered.direction.normalize());

clovers/src/materials/isotropic.rs

@@ -41,12 +41,7 @@ impl MaterialTrait for Isotropic {
 
     /// Returns the scattering probability density function for the [Isotropic] material
     #[must_use]
-    fn scattering_pdf(
-        &self,
-        _hit_record: &HitRecord,
-        _scattered: &Ray,
-        _rng: &mut SmallRng,
-    ) -> Option<Float> {
+    fn scattering_pdf(&self, _hit_record: &HitRecord, _scattered: &Ray) -> Option<Float> {
         Some(1.0 / (4.0 * PI))
     }
 }

clovers/src/materials/lambertian.rs

@@ -37,15 +37,10 @@ impl MaterialTrait for Lambertian {
         })
     }
 
-    /// Returns the scattering probability density function for the [Lambertian] material. TODO: explain the math
-    #[must_use]
-    fn scattering_pdf(
-        &self,
-        hit_record: &HitRecord,
-        scattered: &Ray,
-        _rng: &mut SmallRng,
-    ) -> Option<Float> {
-        // TODO: explain the math
+    /// Returns the scattering probability density function for the [Lambertian] material.
+    ///
+    /// Given the `HitRecord` normal and a scattered `Ray`, computes the dot product and normalizes by `1/pi`. If the dot product is negative, returns `None`.
+    fn scattering_pdf(&self, hit_record: &HitRecord, scattered: &Ray) -> Option<Float> {
         let cosine = hit_record.normal.dot(&scattered.direction.normalize());
         if cosine < 0.0 {
             None

clovers/src/materials/metal.rs

@@ -49,16 +49,7 @@ impl MaterialTrait for Metal {
         })
     }
 
-    /// Scattering probability density function for [Metal]. Always returns zero. TODO: why?
-    #[must_use]
-    fn scattering_pdf(
-        &self,
-        _hit_record: &HitRecord,
-        _scattered: &Ray,
-        _rng: &mut SmallRng,
-    ) -> Option<Float> {
-        None // TODO: why does metal never scatter? should it scatter if fuzzy?
-    }
+    // TODO: should this material provide a `scattering_pdf` function?
 }
 
 impl Metal {

clovers/src/objects/boxy.rs

@@ -9,7 +9,7 @@ use crate::{
     wavelength::Wavelength,
     Box, Direction, Float, Position, Vec3,
 };
-use rand::{rngs::SmallRng, Rng};
+use rand::rngs::SmallRng;
 
 /// `BoxyInit` structure describes the necessary data for constructing a [Boxy]. Used with [serde] when importing [`SceneFile`](crate::scenes::SceneFile)s.
 #[derive(Debug, Clone)]
@@ -110,7 +110,8 @@ impl<'scene> HitableTrait for Boxy<'scene> {
         Some(&self.aabb)
     }
 
-    /// Returns a probability density function value? // TODO: understand & explain
+    // TODO: improve correctness & optimization!
+    /// Returns a probability density function value?
     #[must_use]
     fn pdf_value(
         &self,
@@ -128,11 +129,4 @@ impl<'scene> HitableTrait for Boxy<'scene> {
 
         sum
     }
-
-    /// Returns a random point on the box
-    #[must_use]
-    fn random(&self, origin: Position, rng: &mut SmallRng) -> Vec3 {
-        let index: usize = rng.gen_range(0..6);
-        self.sides[index].random(origin, rng)
-    }
 }

clovers/src/objects/constant_medium.rs

@@ -145,11 +145,4 @@ impl<'scene> HitableTrait for ConstantMedium<'scene> {
         self.boundary
             .pdf_value(origin, direction, wavelength, time, rng)
     }
-
-    /// Returns a random point on the surface of the boundary of the fog
-    // TODO: should this return a random point inside the volume instead?
-    #[must_use]
-    fn random(&self, origin: Position, rng: &mut SmallRng) -> Position {
-        self.boundary.random(origin, rng)
-    }
 }

clovers/src/objects/gltf.rs

@@ -6,7 +6,6 @@ use alloc::vec::Vec;
 use gltf::{image::Data, Mesh, Node};
 use nalgebra::Unit;
 use rand::rngs::SmallRng;
-use rand::Rng;
 #[cfg(feature = "traces")]
 use tracing::debug;
 
@@ -110,12 +109,6 @@ impl<'scene> HitableTrait for GLTF<'scene> {
         self.bvhnode
             .pdf_value(origin, direction, wavelength, time, rng)
     }
-
-    /// Returns a random point on the surface of the object
-    #[must_use]
-    fn random(&self, origin: Position, rng: &mut SmallRng) -> Position {
-        self.bvhnode.random(origin, rng)
-    }
 }
 
 fn parse_node<'scene>(
@@ -362,19 +355,6 @@ impl<'scene> HitableTrait for GLTFTriangle<'scene> {
             None => 0.0,
         }
     }
-
-    fn random(&self, origin: Position, rng: &mut SmallRng) -> Vec3 {
-        let mut a = rng.gen::<Float>();
-        let mut b = rng.gen::<Float>();
-        if a + b > 1.0 {
-            a = 1.0 - a;
-            b = 1.0 - b;
-        }
-
-        let point: Vec3 = self.q + (a * self.u) + (b * self.v);
-
-        point - origin
-    }
 }
 
 #[must_use]

clovers/src/objects/moving_sphere.rs

@@ -4,7 +4,6 @@ use crate::{
     aabb::AABB,
     hitable::{HitRecord, HitableTrait},
     materials::{Material, MaterialInit},
-    random::random_unit_vector,
     ray::Ray,
     wavelength::Wavelength,
     Direction, Float, Position, PI,
@@ -178,9 +177,4 @@ impl<'scene> HitableTrait for MovingSphere<'scene> {
         // TODO: fix
         0.0
     }
-
-    fn random(&self, _origin: Position, rng: &mut SmallRng) -> Position {
-        // FIXME: this is incorrect! does not take into account sphere size, moving sphere position
-        *random_unit_vector(rng)
-    }
 }