math: Use PositiveSign instead of NotNan in Rgb and Rgba.

It is now impossible for colors to contain negative components, which will reduce the number of edge cases color processing code must consider, and remove the the hazard of distinct negative zero (<#537>). (There are some uses for negative-valued RGB components to represent colors that are outside of the gamut of the chosen color space, but I don’t know enough about that extension to know how to handle it correctly.) We’re probably also going to want a type restricted from 0 to 1, for strict alpha and reflectance calculations, but `PositiveSign` is a straightforward improvement of the overall situation.
kpreid · Oct 19, 2024 · 20cd9d6 · 20cd9d6
1 parent 9d4b1d8
commit 20cd9d6
Show file tree

Hide file tree

Showing 29 changed files with 283 additions and 266 deletions.
diff --git a/all-is-cubes-base/src/math/color.rs b/all-is-cubes-base/src/math/color.rs
diff --git a/all-is-cubes-content/src/alg.rs b/all-is-cubes-content/src/alg.rs
@@ -11,7 +11,7 @@ use all_is_cubes::block::{Atom, Block, Primitive, Resolution, AIR};
 use all_is_cubes::euclid::vec3;
 use all_is_cubes::math::{
     Cube, CubeFace, Face6, FaceMap, FreeCoordinate, FreePoint, GridAab, GridCoordinate, GridPoint,
-    GridSizeCoord, GridVector, Gridgid, NotNan, Vol,
+    GridSizeCoord, GridVector, Gridgid, PositiveSign, Vol,
 };
 use all_is_cubes::space::{CubeTransaction, SetCubeError, Space, SpaceTransaction};
 
@@ -185,7 +185,10 @@ pub(crate) fn space_to_transaction_copy(
 /// If the computation is NaN or the block is not an atom, it is returned unchanged.
 pub(crate) fn scale_color(mut block: Block, scalar: f64, quantization: f64) -> Block {
     let scalar = (scalar / quantization).round() * quantization;
-    match (block.primitive_mut(), NotNan::new(scalar as f32)) {
+    match (
+        block.primitive_mut(),
+        PositiveSign::<f32>::try_from(scalar as f32),
+    ) {
         (Primitive::Atom(Atom { color, .. }), Ok(scalar)) => {
             *color = (color.to_rgb() * scalar).with_alpha(color.alpha());
         }

diff --git a/all-is-cubes-content/src/city/exhibits/color.rs b/all-is-cubes-content/src/city/exhibits/color.rs
@@ -26,7 +26,9 @@ fn COLORS(ctx: Context<'_>) {
         let color = Rgb::from(
             color_point
                 .to_vector()
-                .map(|s| NotNan::new(s as f32 / (gradient_resolution - 1) as f32).unwrap())
+                .map(|s| {
+                    PositiveSign::<f32>::new_strict(s as f32 / (gradient_resolution - 1) as f32)
+                })
                 .cast_unit(),
         );
         let color_srgb = color.with_alpha_one().to_srgb8();

diff --git a/all-is-cubes-content/src/city/exhibits/move_modifier.rs b/all-is-cubes-content/src/city/exhibits/move_modifier.rs
@@ -56,7 +56,7 @@ fn PROJECTILE(ctx: Context<'_>) {
     // This will require getting `Move` tick actions to cooperate with `Composite`.
     let launcher = Block::builder()
         .display_name(literal!("Launcher"))
-        .color(Rgb::UNIFORM_LUMINANCE_RED.with_alpha(notnan!(1.0)))
+        .color(Rgb::UNIFORM_LUMINANCE_RED.with_alpha(ps32(1.0)))
         .animation_hint(block::AnimationHint::replacement(
             block::AnimationChange::Shape,
         ))

diff --git a/all-is-cubes-content/src/city/exhibits/prelude.rs b/all-is-cubes-content/src/city/exhibits/prelude.rs
@@ -28,8 +28,8 @@ pub(super) use all_is_cubes::euclid::{
 pub(super) use all_is_cubes::linking::{BlockProvider, InGenError};
 pub(super) use all_is_cubes::listen::ListenableSource;
 pub(super) use all_is_cubes::math::{
-    notnan, rgb_const, rgba_const, Cube, Face6, FaceMap, FreeCoordinate, GridAab, GridCoordinate,
-    GridPoint, GridRotation, GridSize, GridVector, Gridgid, NotNan, Rgb, Rgba,
+    ps32, rgb_const, rgba_const, Cube, Face6, FaceMap, FreeCoordinate, GridAab, GridCoordinate,
+    GridPoint, GridRotation, GridSize, GridVector, Gridgid, PositiveSign, Rgb, Rgba,
 };
 pub(super) use all_is_cubes::op::Operation;
 pub(super) use all_is_cubes::space::{self, Space, SpacePhysics, SpaceTransaction};

diff --git a/all-is-cubes-content/src/city/exhibits/resolutions.rs b/all-is-cubes-content/src/city/exhibits/resolutions.rs
@@ -33,12 +33,11 @@ fn RESOLUTIONS(ctx: Context<'_>) {
                             p.lower_bounds()
                                 .to_vector()
                                 .map(|s| {
-                                    NotNan::new(
+                                    ps32(
                                         (s / GridCoordinate::from(rescale)) as f32
                                             / f32::from(u16::from(resolution) / rescale - 1)
                                                 .max(1.),
                                     )
-                                    .unwrap()
                                 })
                                 .cast_unit(),
                         );

diff --git a/all-is-cubes-content/src/city/exhibits/transparency.rs b/all-is-cubes-content/src/city/exhibits/transparency.rs
@@ -17,19 +17,15 @@ fn TRANSPARENCY_LARGE(_: Context<'_>) {
         Rgb::new(0.5, 0.5, 1.0),
         Rgb::new(0.9, 0.9, 0.9),
     ];
-    let alphas = [0.25, 0.5, 0.75, 0.95];
+    let alphas = [0.25, 0.5, 0.75, 0.95].map(ps32);
     for (rot, color) in GridRotation::CLOCKWISE.iterate().zip(&colors) {
         let windowpane =
             GridAab::from_lower_upper([-1, 0, 3], [2, alphas.len() as GridCoordinate, 4]);
         space.fill(
             windowpane
                 .transform(rot.to_positive_octant_transform(1))
                 .unwrap(),
-            |Cube { y, .. }| {
-                Some(Block::from(
-                    color.with_alpha(NotNan::new(alphas[y as usize]).unwrap()),
-                ))
-            },
+            |Cube { y, .. }| Some(Block::from(color.with_alpha(alphas[y as usize]))),
         )?;
     }
 

diff --git a/all-is-cubes-content/src/clouds.rs b/all-is-cubes-content/src/clouds.rs
@@ -1,7 +1,7 @@
 //! Cloud generation.
 
 use all_is_cubes::block::{Block, BlockCollision, AIR};
-use all_is_cubes::math::{GridAab, GridCoordinate, GridPoint, NotNan, Rgb};
+use all_is_cubes::math::{ps32, GridAab, GridCoordinate, GridPoint, Rgb};
 use all_is_cubes::space::{SetCubeError, Space};
 
 use crate::alg::NoiseFnExt as _;
@@ -24,7 +24,7 @@ pub fn clouds(region: GridAab, space: &mut Space, density: f32) -> Result<(), Se
     fn cloud_block(alpha: f32) -> Block {
         Block::builder()
             .display_name("Cloud")
-            .color(Rgb::ONE.with_alpha(NotNan::new(alpha).unwrap()))
+            .color(Rgb::ONE.with_alpha(ps32(alpha)))
             .collision(if alpha >= 1.0 {
                 BlockCollision::Hard
             } else {

diff --git a/all-is-cubes-content/src/landscape.rs b/all-is-cubes-content/src/landscape.rs
@@ -17,7 +17,7 @@ use all_is_cubes::block::{
     AIR,
 };
 use all_is_cubes::linking::{BlockModule, BlockProvider, DefaultProvision, GenError, InGenError};
-use all_is_cubes::math::{notnan, Cube, FreeCoordinate, GridAab, GridCoordinate, GridVector, Rgb};
+use all_is_cubes::math::{ps32, Cube, FreeCoordinate, GridAab, GridCoordinate, GridVector, Rgb};
 use all_is_cubes::space::Sky;
 use all_is_cubes::space::{SetCubeError, Space};
 use all_is_cubes::universe::UniverseTransaction;
@@ -109,7 +109,7 @@ impl DefaultProvision<Block> for LandscapeBlocks {
         fn blades() -> Block {
             Block::builder()
                 .display_name("Grass Blades")
-                .color(palette::GRASS.with_alpha(notnan!(0.1)))
+                .color(palette::GRASS.with_alpha(ps32(0.1)))
                 .collision(BlockCollision::None)
                 .build()
         }

diff --git a/all-is-cubes-gpu/src/in_wgpu/space.rs b/all-is-cubes-gpu/src/in_wgpu/space.rs
@@ -6,14 +6,14 @@ use std::sync::{atomic, mpsc, Arc, Mutex, Weak};
 use std::time::Duration;
 
 use itertools::Itertools as _;
+use num_traits::ConstZero as _;
 
 use all_is_cubes::chunking::ChunkPos;
 use all_is_cubes::content::palette;
-use all_is_cubes::euclid::num::Zero as _;
 use all_is_cubes::listen::{Listen as _, Listener};
 use all_is_cubes::math::{
     rgba_const, Cube, Face6, FreeCoordinate, FreePoint, GridAab, GridCoordinate, GridPoint,
-    GridSize, GridVector, NotNan, Rgb, Wireframe as _,
+    GridSize, GridVector, PositiveSign, Rgb, Wireframe as _,
 };
 use all_is_cubes::raycast::Ray;
 #[cfg(feature = "rerun")]
@@ -980,8 +980,10 @@ impl ParticleSet {
         let mut tmp: Vec<WgpuLinesVertex> = Vec::with_capacity(24); // TODO: inefficient allocation per object
         crate::wireframe_vertices::<WgpuLinesVertex, _, _>(
             &mut tmp,
-            Rgb::ONE
-                .with_alpha(NotNan::new(0.9f32.powf(self.age as f32)).unwrap_or(NotNan::zero())),
+            Rgb::ONE.with_alpha(
+                PositiveSign::<f32>::try_from(0.9f32.powf(self.age as f32))
+                    .unwrap_or(PositiveSign::ZERO),
+            ),
             &self.fluff.position.aab().expand(0.004 * (self.age as f64)),
         );
         tmp.into_iter()

diff --git a/all-is-cubes-mesh/src/dynamic/chunked_mesh/tests.rs b/all-is-cubes-mesh/src/dynamic/chunked_mesh/tests.rs
@@ -8,7 +8,7 @@ use all_is_cubes::chunking::ChunkPos;
 use all_is_cubes::color_block;
 use all_is_cubes::content::make_some_blocks;
 use all_is_cubes::listen::Listener as _;
-use all_is_cubes::math::{notnan, GridPoint, NotNan};
+use all_is_cubes::math::{ps32, GridPoint, NotNan};
 use all_is_cubes::math::{Cube, FreePoint, GridAab, GridCoordinate};
 use all_is_cubes::space::{BlockIndex, Space, SpaceChange, SpaceTransaction};
 use all_is_cubes::time;
@@ -284,7 +284,7 @@ fn graphics_options_change() {
     assert_eq!(vertices, Some(24));
 
     // Change options so that the mesh should disappear
-    options.transparency = TransparencyOption::Threshold(notnan!(0.5));
+    options.transparency = TransparencyOption::Threshold(ps32(0.5));
     tester.camera.set_options(options.clone());
 
     vertices = None;

diff --git a/all-is-cubes-mesh/src/tests.rs b/all-is-cubes-mesh/src/tests.rs
@@ -8,12 +8,11 @@ use all_is_cubes::block::{self, Block, Resolution::*, AIR};
 use all_is_cubes::color_block;
 use all_is_cubes::content::{make_some_blocks, make_some_voxel_blocks};
 use all_is_cubes::euclid::{point3, Point3D, Vector3D};
+use all_is_cubes::math::{ps32, Cube, Rgb};
 use all_is_cubes::math::{
-    notnan,
     Face6::{self, *},
     FaceMap, FreeCoordinate, GridAab, GridRotation, Rgba,
 };
-use all_is_cubes::math::{Cube, Rgb};
 use all_is_cubes::space::{Space, SpacePhysics};
 use all_is_cubes::universe::Universe;
 use all_is_cubes_render::camera::TransparencyOption;
@@ -70,7 +69,7 @@ fn test_block_mesh_threshold(block: Block) -> BlockMesh<TextureMt> {
         &block.evaluate().unwrap(),
         &Allocator::new(),
         &MeshOptions {
-            transparency: TransparencyOption::Threshold(notnan!(0.5)),
+            transparency: TransparencyOption::Threshold(ps32(0.5)),
             ..MeshOptions::dont_care_for_test()
         },
     )

diff --git a/all-is-cubes-port/src/stl.rs b/all-is-cubes-port/src/stl.rs
@@ -7,7 +7,7 @@ use stl_io::Triangle;
 
 use all_is_cubes::block;
 use all_is_cubes::euclid::Vector3D;
-use all_is_cubes::math::{notnan, Cube, FreeCoordinate};
+use all_is_cubes::math::{ps32, Cube, FreeCoordinate};
 use all_is_cubes::space::Space;
 use all_is_cubes::universe::PartialUniverse;
 use all_is_cubes::util::YieldProgress;
@@ -81,7 +81,7 @@ pub(crate) fn block_to_stl_triangles(block: &block::EvaluatedBlock) -> Vec<Trian
 
 fn mesh_options_for_stl() -> mesh::MeshOptions {
     let mut g = GraphicsOptions::default();
-    g.transparency = all_is_cubes_render::camera::TransparencyOption::Threshold(notnan!(0.01));
+    g.transparency = all_is_cubes_render::camera::TransparencyOption::Threshold(ps32(0.01));
     mesh::MeshOptions::new(&g)
 }
 

diff --git a/all-is-cubes-ui/src/apps/input.rs b/all-is-cubes-ui/src/apps/input.rs
@@ -3,15 +3,14 @@
     reason = "module is private; https://github.com/rust-lang/rust-clippy/issues/8524"
 )]
 
-
 use alloc::vec::Vec;
 use core::time::Duration;
 use std::collections::{HashMap, HashSet};
 
 use all_is_cubes::character::Character;
 use all_is_cubes::euclid::{Point2D, Vector2D};
 use all_is_cubes::listen::{ListenableCell, ListenableSource};
-use all_is_cubes::math::{notnan, FreeCoordinate, FreeVector};
+use all_is_cubes::math::{ps32, FreeCoordinate, FreeVector};
 use all_is_cubes::time::Tick;
 use all_is_cubes::universe::{Handle, Universe};
 use all_is_cubes_render::camera::{
@@ -375,7 +374,7 @@ impl InputProcessor {
                             options.transparency = match options.transparency {
                                 TransparencyOption::Surface => TransparencyOption::Volumetric,
                                 TransparencyOption::Volumetric => {
-                                    TransparencyOption::Threshold(notnan!(0.5))
+                                    TransparencyOption::Threshold(ps32(0.5))
                                 }
                                 TransparencyOption::Threshold(_) => TransparencyOption::Surface,
                                 _ => TransparencyOption::Surface, // TODO: either stop doing cycle-commands or put it on the enum so it can be exhaustive

diff --git a/all-is-cubes-ui/src/ui_content/options.rs b/all-is-cubes-ui/src/ui_content/options.rs
@@ -4,7 +4,7 @@ use alloc::vec::Vec;
 use core::fmt;
 
 use all_is_cubes::arcstr::{self, literal};
-use all_is_cubes::math::{notnan, Face6};
+use all_is_cubes::math::{ps32, Face6};
 use all_is_cubes_render::camera::{self, AntialiasingOption, GraphicsOptions};
 
 use crate::apps::ControlMessage;
@@ -71,7 +71,7 @@ pub(crate) fn graphics_options_widgets(
                 [
                     camera::TransparencyOption::Surface,
                     camera::TransparencyOption::Volumetric,
-                    camera::TransparencyOption::Threshold(notnan!(0.5)),
+                    camera::TransparencyOption::Threshold(ps32(0.5)),
                 ],
             ),
         ]);

diff --git a/all-is-cubes/src/block/eval/derived.rs b/all-is-cubes/src/block/eval/derived.rs
@@ -3,7 +3,6 @@ use core::ops;
 use itertools::Itertools;
 
 use euclid::Vector3D;
-use ordered_float::NotNan;
 
 /// Acts as polyfill for float methods
 #[cfg(not(feature = "std"))]
@@ -14,7 +13,9 @@ use crate::block::{
     self,
     Resolution::{self, R1},
 };
-use crate::math::{Cube, Face6, FaceMap, GridAab, Intensity, OpacityCategory, Rgb, Rgba, Vol};
+use crate::math::{
+    Cube, Face6, FaceMap, GridAab, Intensity, OpacityCategory, PositiveSign, Rgb, Rgba, Vol,
+};
 use crate::raytracer;
 
 #[cfg(doc)]
@@ -234,8 +235,7 @@ impl VoxSum {
                     // Note that by dividing the alpha by the full surface area, not the count,
                     // we handle the case where the voxel data doesn't cover the full block and
                     // uncounted pixels should act as if they are transparent.
-                    NotNan::new(self.alpha_sum / (surface_area))
-                        .expect("Recursive block alpha computation produced NaN"),
+                    PositiveSign::<f32>::new_clamped(self.alpha_sum / (surface_area)),
                 )
         }
     }

diff --git a/all-is-cubes/src/block/modifier/composite.rs b/all-is-cubes/src/block/modifier/composite.rs
@@ -3,15 +3,15 @@
     reason = "module is private; https://github.com/rust-lang/rust-clippy/issues/8524"
 )]
 
+use alloc::vec;
 use core::mem;
 
-use alloc::vec;
-use ordered_float::NotNan;
+use num_traits::Zero;
 
 use crate::block::{
     self, Block, BlockCollision, Evoxel, Evoxels, MinEval, Modifier, Resolution::R1, AIR,
 };
-use crate::math::{Cube, GridAab, GridCoordinate, GridRotation, GridSize, Rgb, Vol};
+use crate::math::{Cube, GridAab, GridCoordinate, GridRotation, GridSize, PositiveSign, Rgb, Vol};
 use crate::op::Operation;
 use crate::universe;
 
@@ -444,31 +444,31 @@ impl CompositeOperator {
     fn alpha_blend(
         self,
         source: Rgb,
-        sa: NotNan<f32>,
+        sa: PositiveSign<f32>,
         destination: Rgb,
-        da: NotNan<f32>,
-    ) -> (Rgb, NotNan<f32>) {
+        da: PositiveSign<f32>,
+    ) -> (Rgb, PositiveSign<f32>) {
         match self {
             Self::Over => {
                 // TODO: Surely this is not the only place we have implemented rgba blending?
                 // Note that this math would be simpler if we used premultiplied alpha.
-                let sa_complement = NotNan::new(1. - sa.into_inner()).unwrap();
+                let sa_complement = PositiveSign::<f32>::new_clamped(1. - sa.into_inner());
                 let rgb = source * sa + destination * sa_complement;
                 (rgb, sa + sa_complement * da)
             }
 
             Self::In => (source, sa * da),
             Self::Out => {
-                let da_complement = NotNan::new(1. - da.into_inner()).unwrap();
+                let da_complement = PositiveSign::<f32>::new_clamped(1. - da.into_inner());
                 (source, sa * da_complement)
             }
 
             Self::Atop => {
-                let sa_complement = NotNan::new(1. - sa.into_inner()).unwrap();
+                let sa_complement = PositiveSign::<f32>::new_clamped(1. - sa.into_inner());
                 let rgb = source * sa + destination * sa_complement;
 
                 let out_alpha = da;
-                if out_alpha == 0.0 {
+                if out_alpha.is_zero() {
                     // we wouldn't have to do this if we used premultiplied alpha :/
                     (Rgb::ZERO, out_alpha)
                 } else {
@@ -683,7 +683,7 @@ mod tests {
     use super::*;
     use crate::block::{EvKey, EvaluatedBlock, Resolution::*};
     use crate::content::{make_slab, make_some_blocks};
-    use crate::math::Rgba;
+    use crate::math::{ps32, Rgba};
     use crate::space::Space;
     use crate::time;
     use crate::universe::Universe;
@@ -720,7 +720,7 @@ mod tests {
         Evoxel {
             // color doesn't matter, except that at zero alpha it should be the canonical zero
             // for convenience of testing. (TODO: maybe `Rgba` should enforce that or be premultiplied.)
-            color: Rgb::ZERO.with_alpha(NotNan::new(alpha).unwrap()),
+            color: Rgb::ZERO.with_alpha(ps32(alpha)),
             emission,
             selectable: true,
             collision: Hard,