From 30ceaed2035745b3dbbec06b13f70c9cbff85e01 Mon Sep 17 00:00:00 2001 From: Kevin Reid Date: Mon, 21 Oct 2024 10:43:39 -0700 Subject: [PATCH] Convert `fov_y` and `view_distance` fields to use `PositiveSign`. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This doesn’t buy us a lot because both are already clamped to be positive when used, but it’s better defined. --- .../src/math/restricted_number.rs | 14 ++++++++++ all-is-cubes-gpu/src/in_wgpu/camera.rs | 2 +- all-is-cubes-gpu/src/in_wgpu/light_texture.rs | 17 +++++++---- all-is-cubes-gpu/src/in_wgpu/rerun_image.rs | 4 +-- .../src/in_wgpu/shader_testing.rs | 4 +-- all-is-cubes-mesh/src/dynamic/chunked_mesh.rs | 5 ++-- .../src/dynamic/chunked_mesh/tests.rs | 6 ++-- all-is-cubes-port/src/gltf.rs | 2 +- all-is-cubes-ui/src/ui_content/vui_manager.rs | 5 ++-- all-is-cubes/src/camera.rs | 13 +++++---- all-is-cubes/src/camera/graphics_options.rs | 28 ++++++------------- all-is-cubes/src/camera/tests.rs | 10 +++---- test-renderers/src/test_cases.rs | 14 +++++----- 13 files changed, 67 insertions(+), 57 deletions(-) diff --git a/all-is-cubes-base/src/math/restricted_number.rs b/all-is-cubes-base/src/math/restricted_number.rs index 36e86fd6e..f2e1e84c5 100644 --- a/all-is-cubes-base/src/math/restricted_number.rs +++ b/all-is-cubes-base/src/math/restricted_number.rs @@ -685,6 +685,13 @@ pub const fn ps32(value: f32) -> PositiveSign { PositiveSign::::new_strict(value) } +/// Convenient alias for [`PositiveSign::::new_strict()`], +/// to be used in tests and pseudo-literals. +#[inline] +pub const fn ps64(value: f64) -> PositiveSign { + PositiveSign::::new_strict(value) +} + /// Convenient alias for [`ZeroOne::::new_strict()`], /// to be used in tests and pseudo-literals. #[inline] @@ -692,6 +699,13 @@ pub const fn zo32(value: f32) -> ZeroOne { ZeroOne::::new_strict(value) } +/// Convenient alias for [`ZeroOne::::new_strict()`], +/// to be used in tests and pseudo-literals. +#[inline] +pub const fn zo64(value: f64) -> ZeroOne { + ZeroOne::::new_strict(value) +} + // ------------------------------------------------------------------------------------------------- #[cfg(test)] diff --git a/all-is-cubes-gpu/src/in_wgpu/camera.rs b/all-is-cubes-gpu/src/in_wgpu/camera.rs index f9eeef2d7..55a7db23f 100644 --- a/all-is-cubes-gpu/src/in_wgpu/camera.rs +++ b/all-is-cubes-gpu/src/in_wgpu/camera.rs @@ -36,7 +36,7 @@ pub(crate) struct ShaderSpaceCamera { impl ShaderSpaceCamera { pub fn new(camera: &Camera) -> Self { let options = camera.options(); - let view_distance = camera.view_distance() as f32; + let view_distance = camera.view_distance().into_inner() as f32; let (fog_mode_blend, fog_distance) = match options.fog { FogOption::Abrupt => (1.0, view_distance), FogOption::Compromise => (0.5, view_distance), diff --git a/all-is-cubes-gpu/src/in_wgpu/light_texture.rs b/all-is-cubes-gpu/src/in_wgpu/light_texture.rs index 066cfca1f..753e002a6 100644 --- a/all-is-cubes-gpu/src/in_wgpu/light_texture.rs +++ b/all-is-cubes-gpu/src/in_wgpu/light_texture.rs @@ -5,11 +5,14 @@ use rayon::{ slice::ParallelSliceMut as _, }; -use all_is_cubes::euclid::{Box3D, Vector3D}; use all_is_cubes::math::{ Aab, Axis, Cube, FaceMap, FreeCoordinate, GridAab, GridCoordinate, GridSize, GridSizeCoord, }; use all_is_cubes::space::Space; +use all_is_cubes::{ + euclid::{Box3D, Vector3D}, + math::PositiveSign, +}; use all_is_cubes_render::camera::Camera; use crate::{ @@ -36,7 +39,8 @@ const CAMERA_MARGIN_RADIUS: f64 = crate::in_wgpu::space::CHUNK_SIZE as f64 * 1.7 /// This region is bounded by view distance and by `Space` bounds. fn visible_light_volume(space_bounds: GridAab, camera: &Camera) -> GridAab { // TODO: handle NaN and overflow cases, and the texture not being big enough, for robustness. - let effective_view_radius = Vector3D::splat(camera.view_distance() + CAMERA_MARGIN_RADIUS); + let effective_view_radius = + Vector3D::splat(camera.view_distance().into_inner() + CAMERA_MARGIN_RADIUS); let visible_bounds = Aab::from_lower_upper( camera.view_position() - effective_view_radius, camera.view_position() + effective_view_radius, @@ -83,7 +87,7 @@ impl LightTexture { pub fn choose_size( limits: &wgpu::Limits, space_bounds: GridAab, - view_distance: FreeCoordinate, + view_distance: PositiveSign, ) -> GridSize { // Extra volume of 1 extra cube around all sides automatically captures sky light. let space_size = space_bounds.size() + GridSize::splat(2); @@ -92,6 +96,7 @@ impl LightTexture { // containing cubes. let camera_size = GridSize::splat( view_distance + .into_inner() .mul_add(2., CAMERA_MARGIN_RADIUS.mul_add(2., 1.)) .ceil() as GridSizeCoord, ); @@ -445,7 +450,7 @@ fn split_axis( #[cfg(test)] mod tests { use all_is_cubes::euclid::vec3; - use all_is_cubes::math::NotNan; + use all_is_cubes::math::ps64; use all_is_cubes_render::camera::{GraphicsOptions, ViewTransform, Viewport}; use super::*; @@ -465,12 +470,12 @@ mod tests { // for this. let step = 1. / 8.; // note: view distance is clamped in graphics options to be a minimum of 1.0 - for view_distance in (8..100).map(|i| f64::from(i) * step) { + for view_distance in (8..100).map(|i| ps64(f64::from(i) * step)) { let texture_size = LightTexture::choose_size(&limits, irrelevant_space_bounds, view_distance); let mut options = GraphicsOptions::default(); - options.view_distance = NotNan::new(view_distance).unwrap(); + options.view_distance = view_distance; camera.set_options(options); for position in (0..100).map(|i| f64::from(i) * step) { diff --git a/all-is-cubes-gpu/src/in_wgpu/rerun_image.rs b/all-is-cubes-gpu/src/in_wgpu/rerun_image.rs index 329ef3d24..69b2ad100 100644 --- a/all-is-cubes-gpu/src/in_wgpu/rerun_image.rs +++ b/all-is-cubes-gpu/src/in_wgpu/rerun_image.rs @@ -266,8 +266,8 @@ fn perform_image_copy( let (pinhole, transform) = rg::convert_camera_to_pinhole(camera); let depth_range = Some(rg::components::ValueRange::new( - camera.near_plane_distance(), - camera.view_distance(), + camera.near_plane_distance().into_inner(), + camera.view_distance().into_inner(), )); Box::pin(async move { diff --git a/all-is-cubes-gpu/src/in_wgpu/shader_testing.rs b/all-is-cubes-gpu/src/in_wgpu/shader_testing.rs index 25e112a3b..97d7e84ce 100644 --- a/all-is-cubes-gpu/src/in_wgpu/shader_testing.rs +++ b/all-is-cubes-gpu/src/in_wgpu/shader_testing.rs @@ -14,7 +14,7 @@ use wgpu::util::DeviceExt as _; use all_is_cubes::euclid::{point3, Rotation3D}; use all_is_cubes::listen::ListenableSource; -use all_is_cubes::math::{notnan, Face6, FreeVector, GridSize, GridVector, Rgba}; +use all_is_cubes::math::{ps64, Face6, FreeVector, GridSize, GridVector, Rgba}; use all_is_cubes::time; use all_is_cubes_mesh::{BlockVertex, Coloring}; use all_is_cubes_render::camera::{Camera, GraphicsOptions, ViewTransform, Viewport}; @@ -174,7 +174,7 @@ where }); let mut options = GraphicsOptions::default(); - options.fov_y = notnan!(90.0); + options.fov_y = ps64(90.0); let mut camera = Camera::new(options, output_viewport); camera.set_view_transform(ViewTransform { rotation: Rotation3D::identity(), diff --git a/all-is-cubes-mesh/src/dynamic/chunked_mesh.rs b/all-is-cubes-mesh/src/dynamic/chunked_mesh.rs index d178d718b..8ec62dfe8 100644 --- a/all-is-cubes-mesh/src/dynamic/chunked_mesh.rs +++ b/all-is-cubes-mesh/src/dynamic/chunked_mesh.rs @@ -305,7 +305,8 @@ where self.complete_time = None; } - self.chunk_chart.resize_if_needed(camera.view_distance()); + self.chunk_chart + .resize_if_needed(camera.view_distance().into_inner()); let prep_to_update_meshes_time = M::Instant::now(); @@ -334,7 +335,7 @@ where // Not urgently needed, though. let cache_distance = FreeCoordinate::from(CHUNK_SIZE); let retention_distance_squared = - (camera.view_distance().ceil() + cache_distance).powi(2) as i32; + (camera.view_distance().into_inner().ceil() + cache_distance).powi(2) as i32; self.chunks.retain(|pos, _| { pos.min_distance_squared_from(view_chunk) <= retention_distance_squared }); diff --git a/all-is-cubes-mesh/src/dynamic/chunked_mesh/tests.rs b/all-is-cubes-mesh/src/dynamic/chunked_mesh/tests.rs index 2d45d374f..f8cee8110 100644 --- 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::{zo32, GridPoint, NotNan}; +use all_is_cubes::math::{zo32, GridPoint}; use all_is_cubes::math::{Cube, FreePoint, GridAab, GridCoordinate}; use all_is_cubes::space::{BlockIndex, Space, SpaceChange, SpaceTransaction}; use all_is_cubes::time; @@ -167,7 +167,7 @@ impl CsmTester { let camera = Camera::new( { let mut o = GraphicsOptions::default(); - o.view_distance = NotNan::new(view_distance).unwrap(); + o.view_distance = view_distance.try_into().unwrap(); o }, Viewport::ARBITRARY, @@ -268,7 +268,7 @@ fn sort_view_every_frame_only_if_transparent() { fn graphics_options_change() { // TODO: This test is fragile because it doesn't think about multiple chunks. let mut options = GraphicsOptions::default(); - options.view_distance = NotNan::from(1); + options.view_distance = 1u8.into(); options.transparency = TransparencyOption::Volumetric; let mut space = Space::empty_positive(1, 1, 1); diff --git a/all-is-cubes-port/src/gltf.rs b/all-is-cubes-port/src/gltf.rs index 8f359b5e3..ee8f4233b 100644 --- a/all-is-cubes-port/src/gltf.rs +++ b/all-is-cubes-port/src/gltf.rs @@ -480,7 +480,7 @@ fn convert_camera(name: Option, camera: &Camera) -> gltf_json::Camera { aspect_ratio: Some(camera.viewport().nominal_aspect_ratio() as f32), yfov: camera.options().fov_y.into_inner() as f32 * (std::f32::consts::PI / 180.), zfar: Some(camera.options().view_distance.into_inner() as f32), - znear: camera.near_plane_distance() as f32, + znear: camera.near_plane_distance().into_inner() as f32, extensions: Default::default(), extras: Default::default(), }), diff --git a/all-is-cubes-ui/src/ui_content/vui_manager.rs b/all-is-cubes-ui/src/ui_content/vui_manager.rs index dd0e694ff..0480dcaa0 100644 --- a/all-is-cubes-ui/src/ui_content/vui_manager.rs +++ b/all-is-cubes-ui/src/ui_content/vui_manager.rs @@ -8,7 +8,6 @@ use all_is_cubes::arcstr::ArcStr; use all_is_cubes::character::{Character, Cursor}; use all_is_cubes::inv::{EphemeralOpaque, Tool, ToolError, ToolInput}; use all_is_cubes::listen::{DirtyFlag, ListenableCell, ListenableSource, Notifier}; -use all_is_cubes::math::NotNan; use all_is_cubes::space::Space; use all_is_cubes::time; use all_is_cubes::transaction::{self, Transaction}; @@ -277,14 +276,14 @@ impl Vui { /// Compute graphics options to render the VUI space given the user's regular options. fn graphics_options(mut options: GraphicsOptions) -> GraphicsOptions { // Set FOV to give a predictable, not-too-wide-angle perspective. - options.fov_y = NotNan::from(30); + options.fov_y = 30u8.into(); // Disable fog for maximum clarity and because we shouldn't have any far clipping to hide. options.fog = FogOption::None; // Fixed view distance for our layout. // TODO: Derive this from HudLayout and also FOV (since FOV determines eye-to-space distance). - options.view_distance = NotNan::from(100); + options.view_distance = 100u8.into(); // clutter options.debug_chunk_boxes = false; diff --git a/all-is-cubes/src/camera.rs b/all-is-cubes/src/camera.rs index 0cab9d6b1..f1dd42e85 100644 --- a/all-is-cubes/src/camera.rs +++ b/all-is-cubes/src/camera.rs @@ -1,5 +1,6 @@ //! Note: This module is hidden, and its contents re-exported as `all_is_cubes_render::camera`. +use all_is_cubes_base::math::ps64; use euclid::{ point3, vec3, Angle, Point2D, Point3D, RigidTransform3D, Rotation3D, Size2D, Transform3D, }; @@ -189,15 +190,15 @@ impl Camera { /// Returns the view distance; the far plane of the view frustum, or the distance /// at which rendering may be truncated. - pub fn view_distance(&self) -> FreeCoordinate { - self.options.view_distance.into_inner() + pub fn view_distance(&self) -> PositiveSign { + self.options.view_distance } /// Returns the position of the near plane of the view frustum. /// This is not currently configurable. - pub fn near_plane_distance(&self) -> FreeCoordinate { + pub fn near_plane_distance(&self) -> PositiveSign { // half a voxel at resolution=16 - (32.0f64).recip() + ps64((32.0f64).recip()) } /// Returns a perspective projection matrix based on the configured FOV and view distance, @@ -378,8 +379,8 @@ impl Camera { let fov_cot = (self.fov_y() / 2.).to_radians().tan().recip(); let aspect = self.viewport.nominal_aspect_ratio(); - let near = self.near_plane_distance(); - let far = self.view_distance(); + let near = self.near_plane_distance().into_inner(); + let far = self.view_distance().into_inner(); // Rationale for this particular matrix formula: "that's what `cgmath` does", // and we used to use `cgmath`. diff --git a/all-is-cubes/src/camera/graphics_options.rs b/all-is-cubes/src/camera/graphics_options.rs index 8a7ca6c18..812fc67e7 100644 --- a/all-is-cubes/src/camera/graphics_options.rs +++ b/all-is-cubes/src/camera/graphics_options.rs @@ -1,9 +1,8 @@ use core::fmt; use num_traits::ConstOne as _; -use ordered_float::NotNan; -use crate::math::{notnan, zo32, FreeCoordinate, PositiveSign, Rgb, Rgba, ZeroOne}; +use crate::math::{ps64, zo32, FreeCoordinate, PositiveSign, Rgb, Rgba, ZeroOne}; use crate::util::ShowStatus; #[cfg(doc)] @@ -18,13 +17,6 @@ use crate::{block::Block, space::Space}; // (Due to crate splitting that can't be a doc-link.) #[doc = include_str!("../save/serde-warning.md")] #[derive(Clone, Eq, PartialEq)] -#[cfg_attr( - feature = "save", - expect( - clippy::unsafe_derive_deserialize, - reason = "false positive from notnan! macro" - ) -)] #[cfg_attr(feature = "save", derive(serde::Serialize, serde::Deserialize))] #[cfg_attr(feature = "save", serde(default))] #[non_exhaustive] @@ -41,7 +33,7 @@ pub struct GraphicsOptions { pub fog: FogOption, /// Field of view, in degrees from top to bottom edge of the viewport. - pub fov_y: NotNan, + pub fov_y: PositiveSign, /// Method to use to remap colors to fit within the displayable range. pub tone_mapping: ToneMappingOperator, @@ -58,7 +50,7 @@ pub struct GraphicsOptions { /// Distance, in unit cubes, from the camera to the farthest visible point. /// /// TODO: Implement view distance limit (and fog) in raytracer. - pub view_distance: NotNan, + pub view_distance: PositiveSign, /// Style in which to draw the lighting of [`Space`](crate::space::Space)s. /// This does not affect the *computation* of lighting. @@ -130,12 +122,12 @@ impl GraphicsOptions { pub const UNALTERED_COLORS: Self = Self { render_method: RenderMethod::Preferred, fog: FogOption::None, - fov_y: notnan!(90.), + fov_y: ps64(90.), // TODO: Change tone mapping default once we have a good implementation. tone_mapping: ToneMappingOperator::Clamp, exposure: ExposureOption::Fixed(PositiveSign::::ONE), bloom_intensity: zo32(0.), - view_distance: notnan!(200.), + view_distance: ps64(200.), lighting_display: LightingOption::None, transparency: TransparencyOption::Volumetric, show_ui: true, @@ -152,10 +144,8 @@ impl GraphicsOptions { /// Constrain fields to valid/practical values. #[must_use] pub fn repair(mut self) -> Self { - self.fov_y = self.fov_y.clamp(NotNan::from(1), NotNan::from(189)); - self.view_distance = self - .view_distance - .clamp(NotNan::from(1), NotNan::from(10000)); + self.fov_y = self.fov_y.clamp(ps64(1.), ps64(189.)); + self.view_distance = self.view_distance.clamp(ps64(1.), ps64(10000.)); self } } @@ -215,12 +205,12 @@ impl Default for GraphicsOptions { Self { render_method: RenderMethod::Preferred, fog: FogOption::Abrupt, - fov_y: NotNan::from(90), + fov_y: ps64(90.), // TODO: Change tone mapping default once we have a good implementation. tone_mapping: ToneMappingOperator::Clamp, exposure: ExposureOption::default(), bloom_intensity: zo32(0.125), - view_distance: NotNan::from(200), + view_distance: ps64(200.), lighting_display: LightingOption::Smooth, transparency: TransparencyOption::Volumetric, show_ui: true, diff --git a/all-is-cubes/src/camera/tests.rs b/all-is-cubes/src/camera/tests.rs index 9e4004ce8..884e58bf2 100644 --- a/all-is-cubes/src/camera/tests.rs +++ b/all-is-cubes/src/camera/tests.rs @@ -6,7 +6,7 @@ use crate::camera::{ look_at_y_up, Camera, ExposureOption, FrustumPoints, GraphicsOptions, LightingOption, ViewTransform, Viewport, }; -use crate::math::{ps32, rgba_const, Aab, NotNan}; +use crate::math::{ps32, ps64, rgba_const, Aab}; #[test] fn camera_bad_viewport_doesnt_panic() { @@ -25,7 +25,7 @@ fn set_options_updates_matrices() { let matrix = camera.projection_matrix(); camera.set_options({ let mut g = camera.options().clone(); - g.fov_y = NotNan::from(30); + g.fov_y = ps64(30.); g }); assert_ne!(matrix, camera.projection_matrix()); @@ -54,7 +54,7 @@ fn projection_depth() { let world_depths = [camera.near_plane_distance(), camera.view_distance()]; let expected_ndc_depths = [0., 1.]; let actual_ndc_depths = world_depths.map(|z| { - let eye = point3(0., 0., -z); + let eye = point3(0., 0., -f64::from(z)); let clip = mat.transform_point3d_homogeneous(eye); // doesn't reject z=0 like euclid's to_point3d() does clip.z / clip.w @@ -71,8 +71,8 @@ fn projection_depth() { fn view_frustum() { let camera = Camera::new( GraphicsOptions { - view_distance: NotNan::from(10i32.pow(2)), - fov_y: NotNan::from(90), + view_distance: ps64(10f64.powi(2)), + fov_y: ps64(90.), ..GraphicsOptions::default() }, Viewport::with_scale(1.0, [10, 5]), diff --git a/test-renderers/src/test_cases.rs b/test-renderers/src/test_cases.rs index 6427726bd..2e4bf9e4d 100644 --- a/test-renderers/src/test_cases.rs +++ b/test-renderers/src/test_cases.rs @@ -16,7 +16,7 @@ use all_is_cubes::euclid::{point3, size2, size3, vec2, vec3, Point2D, Size2D, Si use all_is_cubes::listen::{ListenableCell, ListenableSource}; use all_is_cubes::math::{ ps32, rgb_const, rgba_const, zo32, Axis, Cube, Face6, FreeCoordinate, GridAab, GridCoordinate, - GridPoint, GridRotation, GridVector, NotNan, Rgb, Rgba, Vol, + GridPoint, GridRotation, GridVector, Rgb, Rgba, Vol, }; use all_is_cubes::space::{self, LightPhysics, Space}; use all_is_cubes::time; @@ -427,7 +427,7 @@ async fn error_character_unavailable(context: RenderTestContext) { async fn fog(mut context: RenderTestContext, fog: FogOption) { let mut options = GraphicsOptions::UNALTERED_COLORS; options.lighting_display = LightingOption::Smooth; - options.view_distance = NotNan::from(50); + options.view_distance = 50u8.into(); options.fog = fog; let scene = StandardCameras::from_constant_for_test(options, COMMON_VIEWPORT, context.universe()); @@ -498,9 +498,9 @@ async fn follow_options_change(mut context: RenderTestContext) { // Two sets of graphics options with various differences let mut options_1 = GraphicsOptions::UNALTERED_COLORS; options_1.lighting_display = LightingOption::Smooth; - options_1.fov_y = NotNan::from(90); + options_1.fov_y = 90u8.into(); let mut options_2 = options_1.clone(); - options_2.fov_y = NotNan::from(70); + options_2.fov_y = 70u8.into(); options_2.exposure = ExposureOption::Fixed(ps32(1.5)); options_2.transparency = TransparencyOption::Threshold(zo32(0.1)); @@ -698,7 +698,7 @@ async fn icons(mut context: RenderTestContext) { let mut options = GraphicsOptions::UNALTERED_COLORS; options.lighting_display = LightingOption::Flat; - options.fov_y = NotNan::from(45); + options.fov_y = 45u8.into(); context .render_comparison_test( // Fairly sloppy because this test is looking for "Does this icon look right", @@ -1238,7 +1238,7 @@ async fn light_test_universe() -> Arc { fn light_test_options() -> GraphicsOptions { let mut options = GraphicsOptions::UNALTERED_COLORS; options.lighting_display = LightingOption::Smooth; - options.fov_y = NotNan::from(45); + options.fov_y = 45u8.into(); options } @@ -1349,6 +1349,6 @@ fn tone_mapping_test_options() -> GraphicsOptions { // TODO: We want to see how bloom looks along with the tone mapping, but raytracer doesn't // support bloom yet and we can't opt out per-test of Flaws matching. // options.bloom_intensity = GraphicsOptions::default().bloom_intensity; - options.fov_y = NotNan::from(45); + options.fov_y = 45u8.into(); options }