From 0faa5e5d22f8f7362b7a2bf3a692ca3b89f093d7 Mon Sep 17 00:00:00 2001 From: Kevin Reid Date: Thu, 8 Aug 2024 11:11:45 -0700 Subject: [PATCH] raytracer: Use premultiplied alpha to describe surface color. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Part of fixing . With this change, the raytracer now correctly renders blocks that have light emission only (at least, in volumetric mode; the surface-only mode is multiplying the emission by the number of voxels, which is not really the right outcome). It is incoherent or at least undesirable to multiply light emissions by the reflectance color’s alpha. To avoid doing that, the cleanest solution is to switch to premultiplied alpha, where the accumulator only uses the alpha to determine the effect on future (more distant) surfaces. Conveniently, we already have a type that implements exactly this representation: `ColorBuf`! Or, not exactly — the alpha is actually complemented (1 − α), making it transmittance instead, but that’s what we actually need. This commit breaks the `icons` rendering test, but we need to fix the same bug in `wgpu` rendering and also update the icon to match the new paradigm, so it's going to be broken sometime unless I were to squash all three changes into one commit, which I don't want to do. --- CHANGELOG.md | 3 + all-is-cubes-desktop/src/terminal.rs | 12 ++- all-is-cubes-render/src/raytracer/ortho.rs | 4 +- all-is-cubes-render/src/raytracer/renderer.rs | 2 +- all-is-cubes/src/raytracer.rs | 18 ++-- all-is-cubes/src/raytracer/accum.rs | 95 +++++++++++++----- all-is-cubes/src/raytracer/surface.rs | 20 ++-- all-is-cubes/src/raytracer/text.rs | 5 +- .../renderers/emission_only-surf-all.png | Bin 0 -> 332 bytes .../renderers/emission_only-vol-all.png | Bin 0 -> 1409 bytes 10 files changed, 109 insertions(+), 50 deletions(-) create mode 100644 test-renderers/expected/renderers/emission_only-surf-all.png create mode 100644 test-renderers/expected/renderers/emission_only-vol-all.png diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ef7b5fd2..18313f4dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,9 @@ - `block::EvalBlockError` is now a `struct` with an inner `ErrorKind` enum, instead of an enum, and contains more information. - `block::Move`’s means of construction have been changed to be more systematic and orthogonal. In particular, paired moves are constructed from unpaired ones. +- `all-is-cubes-render` library: + - The trait method `raytracer::Accumulate::add()` now accepts the surface color via a `ColorBuf` (which acts essentially as a form of premultiplied alpha) rather than `Rgba`. This enables more consistent handling of emissive materials. See the method documentation for details. + ### Removed ## 0.8.0 (2024-07-08) diff --git a/all-is-cubes-desktop/src/terminal.rs b/all-is-cubes-desktop/src/terminal.rs index 073935ba8..8b28acee7 100644 --- a/all-is-cubes-desktop/src/terminal.rs +++ b/all-is-cubes-desktop/src/terminal.rs @@ -363,18 +363,20 @@ impl Accumulate for ColorCharacterBuf { } #[inline] - fn add(&mut self, surface_color: Rgba, text: &Self::BlockData) { + fn add(&mut self, surface: ColorBuf, text: &Self::BlockData) { if self.override_color { return; } - self.color.add(surface_color, &()); - self.text.add(surface_color, text); + self.color.add(surface, &()); + self.text.add(surface, text); } fn hit_nothing(&mut self) { - self.text - .add(Rgba::TRANSPARENT, &CharacterRtData(literal_substr!(" "))); + self.text.add( + Rgba::TRANSPARENT.into(), + &CharacterRtData(literal_substr!(" ")), + ); self.override_color = true; } diff --git a/all-is-cubes-render/src/raytracer/ortho.rs b/all-is-cubes-render/src/raytracer/ortho.rs index aa077b7fa..0b5a4a2ee 100644 --- a/all-is-cubes-render/src/raytracer/ortho.rs +++ b/all-is-cubes-render/src/raytracer/ortho.rs @@ -296,8 +296,8 @@ impl raytracer::Accumulate for OrthoBuf { self.color.opaque() } - fn add(&mut self, surface_color: Rgba, &resolution: &Self::BlockData) { - self.color.add(surface_color, &()); + fn add(&mut self, surface: raytracer::ColorBuf, &resolution: &Self::BlockData) { + self.color.add(surface, &()); self.max_resolution = self.max_resolution.max(resolution); } diff --git a/all-is-cubes-render/src/raytracer/renderer.rs b/all-is-cubes-render/src/raytracer/renderer.rs index d1ab30e57..96e910070 100644 --- a/all-is-cubes-render/src/raytracer/renderer.rs +++ b/all-is-cubes-render/src/raytracer/renderer.rs @@ -652,7 +652,7 @@ mod tests { fn opaque(&self) -> bool { self.custom_options.is_empty() } - fn add(&mut self, _: Rgba, block_data: &Self::BlockData) { + fn add(&mut self, _: ColorBuf, block_data: &Self::BlockData) { if self.custom_options.is_empty() { *self = *block_data; } diff --git a/all-is-cubes/src/raytracer.rs b/all-is-cubes/src/raytracer.rs index 11159efad..08ed1d6ed 100644 --- a/all-is-cubes/src/raytracer.rs +++ b/all-is-cubes/src/raytracer.rs @@ -518,7 +518,7 @@ impl TracingState

{ // Abort excessively long traces. self.accumulator = Default::default(); self.accumulator - .add(Rgba::new(1.0, 1.0, 1.0, 1.0), &P::BlockData::error(options)); + .add(Rgba::WHITE.into(), &P::BlockData::error(options)); true } else { self.accumulator.opaque() @@ -532,7 +532,7 @@ impl TracingState

{ self.accumulator.hit_nothing(); } - self.accumulator.add(sky_color, sky_data); + self.accumulator.add(sky_color.into(), sky_data); // Debug visualization of number of raytracing steps. // TODO: Make this togglable and less of a kludge — we'd like to be able to mix with @@ -541,7 +541,9 @@ impl TracingState

{ if DEBUG_STEPS && self.accumulator.opaque() { self.accumulator = Default::default(); self.accumulator.add( - (rgb_const!(0.02, 0.002, 0.0) * self.cubes_traced as f32).with_alpha_one(), + (rgb_const!(0.02, 0.002, 0.0) * self.cubes_traced as f32) + .with_alpha_one() + .into(), sky_data, ); } @@ -561,8 +563,8 @@ impl TracingState

{ surface: &Surface<'_, P::BlockData>, rt: &SpaceRaytracer, ) { - if let Some(color) = surface.to_lit_color(rt) { - self.accumulator.add(color, surface.block_data); + if let Some(light) = surface.to_light(rt) { + self.accumulator.add(light, surface.block_data); } } @@ -629,8 +631,8 @@ pub(crate) fn trace_for_eval( let mut emission = Vector3D::zero(); while let Some(voxel) = voxels.get(cube) { - emission += Vector3D::from(voxel.emission) * color_buf.ray_alpha; - color_buf.add(apply_transmittance(voxel.color, thickness), &()); + emission += Vector3D::from(voxel.emission) * color_buf.transmittance; + color_buf.add(apply_transmittance(voxel.color, thickness).into(), &()); if color_buf.opaque() { break; @@ -700,7 +702,7 @@ mod tests { let modified_color = apply_transmittance(color, (count as f32).recip()); let mut color_buf = ColorBuf::default(); for _ in 0..count { - color_buf.add(modified_color, &()); + color_buf.add(modified_color.into(), &()); } let actual = Rgba::from(color_buf); let error: Vec = <[f32; 4]>::from(actual) diff --git a/all-is-cubes/src/raytracer/accum.rs b/all-is-cubes/src/raytracer/accum.rs index 8f25998b9..70783faf7 100644 --- a/all-is-cubes/src/raytracer/accum.rs +++ b/all-is-cubes/src/raytracer/accum.rs @@ -62,14 +62,17 @@ pub trait Accumulate: Default { /// be affected by future calls to [`Self::add`]. fn opaque(&self) -> bool; - /// Adds the color of a surface to the buffer. The provided color should already - /// have the effect of lighting applied. + /// Adds the light from a surface, and the opacity of that surface, to the accumulator. + /// This surface is positioned behind/beyond all previous `add()`ed surfaces. /// - /// You should probably give this method the `#[inline]` attribute. + /// The given [`ColorBuf`] represents the opacity and the camera-ward light output of the + /// encountered surface. + /// Implementations are responsible for reducing it according to the transmittance (inverse + /// opacity) of previously encountered surfaces which obscure it. /// - /// TODO: this interface might want even more information; generalize it to be + /// TODO: this interface might want even more information (e.g. depth); generalize it to be /// more future-proof. - fn add(&mut self, surface_color: Rgba, block_data: &Self::BlockData); + fn add(&mut self, surface: ColorBuf, block_data: &Self::BlockData); /// Called before the ray traverses any surfaces found in a block; that is, /// before all [`add()`](Self::add) calls pertaining to that block. @@ -96,7 +99,7 @@ pub trait Accumulate: Default { let mut result = Self::default(); // TODO: Should give RtBlockData a dedicated method for this, but we haven't // yet had a use case where it matters. - result.add(color, &Self::BlockData::sky(options)); + result.add(color.into(), &Self::BlockData::sky(options)); result } @@ -157,6 +160,9 @@ impl RtBlockData for Resolution { /// Implements [`Accumulate`] for RGB(A) color with [`f32`] components, /// and conversion to [`Rgba`]. +/// +/// In addition to its use in constructing images, this type is also used as an intermediate +/// format for surface colors presented to any other [`Accumulate`] implementation. #[derive(Clone, Copy, Debug, PartialEq)] pub struct ColorBuf { /// Color buffer. @@ -166,11 +172,23 @@ pub struct ColorBuf { /// display supposing that everything not already traced is black. /// /// Note: Not using the [`Rgb`](crate::math::Rgb) type so as to skip NaN checks. - color_accumulator: Vector3D, + /// This should never end up NaN, but we don't assert that property. + light: Vector3D, /// Fraction of the color value that is to be determined by future, rather than past, /// tracing; starts at 1.0 and decreases as surfaces are encountered. - pub(super) ray_alpha: f32, + /// + /// The range of this value is between 1.0 and 0.0, inclusive. + pub(super) transmittance: f32, +} + +impl ColorBuf { + pub(crate) fn from_light_and_transmittance(light: Rgb, transmittance: f32) -> ColorBuf { + Self { + light: light.into(), + transmittance, + } + } } impl Accumulate for ColorBuf { @@ -180,27 +198,26 @@ impl Accumulate for ColorBuf { fn opaque(&self) -> bool { // Let's suppose that we don't care about differences that can't be represented // in 8-bit color...not considering gamma. - self.ray_alpha < 1.0 / 256.0 + self.transmittance < 1.0 / 256.0 } #[inline] - fn add(&mut self, surface_color: Rgba, _block_data: &Self::BlockData) { - let color_vector: Vector3D = surface_color.to_rgb().into(); - let surface_alpha = surface_color.alpha().into_inner(); - let alpha_for_add = surface_alpha * self.ray_alpha; - self.ray_alpha *= 1.0 - surface_alpha; - self.color_accumulator += color_vector * alpha_for_add; + fn add(&mut self, surface: ColorBuf, _block_data: &Self::BlockData) { + // Note that the order of these assignments matters. + // surface.transmittance is only applied to surfaces *after* this one. + self.light += surface.light * self.transmittance; + self.transmittance *= surface.transmittance; } #[inline] fn mean(items: [Self; N]) -> Self { Self { - color_accumulator: items + light: items .iter() - .map(|cb| cb.color_accumulator) + .map(|cb| cb.light) .sum::>() / (N as f32), - ray_alpha: items.iter().map(|cb| cb.ray_alpha).sum::() / (N as f32), + transmittance: items.iter().map(|cb| cb.transmittance).sum::() / (N as f32), } } } @@ -209,8 +226,8 @@ impl Default for ColorBuf { #[inline] fn default() -> Self { Self { - color_accumulator: Vector3D::zero(), - ray_alpha: 1.0, + light: Vector3D::zero(), + transmittance: 1.0, } } } @@ -221,13 +238,18 @@ impl From for Rgba { /// Not tone mapped; consider using [`Camera::post_process_color()`] for that. /// /// [`Camera::post_process_color()`]: crate::camera::Camera::post_process_color() + //--- + // TODO: Converting arbitrary HDR+opacity colors to non-premultiplied RGBA is incorrect + // in the general case. We should allow for tone-mapping in premultiplied form, either by + // replacing this conversion with a custom method, or changing the [`Rgba`] type to be + // premultiplied. fn from(buf: ColorBuf) -> Rgba { - if buf.ray_alpha >= 1.0 { + if buf.transmittance >= 1.0 { // Special case to avoid dividing by zero Rgba::TRANSPARENT } else { - let color_alpha = 1.0 - buf.ray_alpha; - let non_premultiplied_color = buf.color_accumulator / color_alpha; + let color_alpha = 1.0 - buf.transmittance; + let non_premultiplied_color = buf.light / color_alpha; Rgb::try_from(non_premultiplied_color) .unwrap_or_else(|_| rgb_const!(1.0, 0.0, 0.0)) .with_alpha(NotNan::new(color_alpha).unwrap_or(notnan!(1.0))) @@ -235,12 +257,33 @@ impl From for Rgba { } } +impl From for ColorBuf { + /// Converts the given [`Rgba`] color value, interpreted as the reflectance and opacity of + /// a surface lit by white light with luminance 1.0, into a [`ColorBuf`]. + fn from(value: Rgba) -> Self { + let alpha = value.alpha().into_inner(); + Self { + light: Vector3D::new( + value.red().into_inner() * alpha, + value.green().into_inner() * alpha, + value.blue().into_inner() * alpha, + ), + transmittance: 1.0 - alpha, + } + } +} + #[cfg(test)] mod tests { use super::*; #[test] fn color_buf() { + // TODO: This entire test should be revisited with our new understanding about + // premultiplied alpha’s role in color accumulation. + // Maybe we can fix the failing assertions. + // + let color_1 = Rgba::new(1.0, 0.0, 0.0, 0.75); let color_2 = Rgba::new(0.0, 1.0, 0.0, 0.5); let color_3 = Rgba::new(0.0, 0.0, 1.0, 1.0); @@ -249,11 +292,11 @@ mod tests { assert_eq!(Rgba::from(buf), Rgba::TRANSPARENT); assert!(!buf.opaque()); - buf.add(color_1, &()); + buf.add(color_1.into(), &()); assert_eq!(Rgba::from(buf), color_1); assert!(!buf.opaque()); - buf.add(color_2, &()); + buf.add(color_2.into(), &()); // TODO: this is not the right assertion because it's the premultiplied form. // assert_eq!( // buf.result(), @@ -262,7 +305,7 @@ mod tests { // ); assert!(!buf.opaque()); - buf.add(color_3, &()); + buf.add(color_3.into(), &()); assert!(Rgba::from(buf).fully_opaque()); //assert_eq!( // buf.result(), diff --git a/all-is-cubes/src/raytracer/surface.rs b/all-is-cubes/src/raytracer/surface.rs index edac0674f..946b3bf38 100644 --- a/all-is-cubes/src/raytracer/surface.rs +++ b/all-is-cubes/src/raytracer/surface.rs @@ -4,7 +4,7 @@ use crate::block::Evoxel; use crate::camera::LightingOption; use crate::math::{Cube, Face7, FaceMap, FreeCoordinate, FreePoint, Rgb, Rgba, Vol}; use crate::raycast::{RayIsh as _, RaycasterIsh}; -use crate::raytracer::{RtBlockData, SpaceRaytracer, TracingBlock, TracingCubeData}; +use crate::raytracer::{ColorBuf, RtBlockData, SpaceRaytracer, TracingBlock, TracingCubeData}; /// Description of a surface the ray passes through (or from the volumetric perspective, /// a transition from one material to another). @@ -44,13 +44,17 @@ impl Surface<'_, D> { !diffuse_color.fully_transparent() || emission != Rgb::ZERO } - /// Convert the surface and its lighting to a single RGBA value as determined by - /// the given graphics options, or [`None`] if it is invisible. + /// Combine the surface properties, lighting, and graphics options to produce + /// a light intensity and a transmittance value for light arriving from behind the surface; + /// or [`None`] if it is invisible. + /// + /// Note that the result is “premultiplied alpha”; the returned color should *not* + /// be modified in any way by the returned transmittance. /// /// Note that this is completely unaware of volume/thickness; that is handled by /// `TracingState::trace_through_span()` tweaking the data before this is called. #[inline] - pub(crate) fn to_lit_color(&self, rt: &SpaceRaytracer) -> Option { + pub(crate) fn to_light(&self, rt: &SpaceRaytracer) -> Option { let diffuse_color = rt .graphics_options .transparency @@ -62,9 +66,13 @@ impl Surface<'_, D> { let illumination = self.compute_illumination(rt); // Combine reflected and emitted light to produce the outgoing light. - let outgoing_rgb = diffuse_color.to_rgb() * illumination + self.emission; + let outgoing_rgb = + diffuse_color.to_rgb() * illumination * diffuse_color.alpha() + self.emission; - Some(outgoing_rgb.with_alpha(diffuse_color.alpha())) + Some(ColorBuf::from_light_and_transmittance( + outgoing_rgb, + 1.0 - diffuse_color.alpha().into_inner(), + )) } fn compute_illumination(&self, rt: &SpaceRaytracer) -> Rgb { diff --git a/all-is-cubes/src/raytracer/text.rs b/all-is-cubes/src/raytracer/text.rs index 97368e090..4c4d29206 100644 --- a/all-is-cubes/src/raytracer/text.rs +++ b/all-is-cubes/src/raytracer/text.rs @@ -10,7 +10,7 @@ use euclid::size2; use unicode_segmentation::UnicodeSegmentation; use crate::camera::{eye_for_look_at, Camera, GraphicsOptions, Viewport}; -use crate::math::{FreeVector, Rgba}; +use crate::math::FreeVector; use crate::raytracer::{Accumulate, RtBlockData, RtOptionsRef, SpaceRaytracer}; use crate::space::{Space, SpaceBlockData}; @@ -59,7 +59,7 @@ impl Accumulate for CharacterBuf { } #[inline] - fn add(&mut self, _surface_color: Rgba, d: &Self::BlockData) { + fn add(&mut self, _surface: super::ColorBuf, d: &Self::BlockData) { if self.hit_text.is_none() { self.hit_text = Some(d.0.clone()); } @@ -148,6 +148,7 @@ mod tests { use crate::block::{Block, Resolution::R4}; use crate::color_block; use crate::content::make_some_blocks; + use crate::math::Rgba; use crate::universe::Universe; use euclid::vec3; use std::string::ToString; diff --git a/test-renderers/expected/renderers/emission_only-surf-all.png b/test-renderers/expected/renderers/emission_only-surf-all.png new file mode 100644 index 0000000000000000000000000000000000000000..ce4bb30f93f2f4353c2eb5fc7781e2f75e0846db GIT binary patch literal 332 zcmeAS@N?(olHy`uVBq!ia0vp^4M3d0!2~3uB;Lv~Ffa;xx;TbZ+GaNT@c9cvpvBY@Xur;qr$JkJR65!pTFcZr0Zq;;XZIU ze99@tplM&M7$02=Szopr080&F>Hq)$ literal 0 HcmV?d00001 diff --git a/test-renderers/expected/renderers/emission_only-vol-all.png b/test-renderers/expected/renderers/emission_only-vol-all.png new file mode 100644 index 0000000000000000000000000000000000000000..2e8f2b52288c59b05b138c846c8c8dd69b574779 GIT binary patch literal 1409 zcmV-{1%CR8P)N!WE%P2&qU&9D3ozoeS`9i;JEB1Rwx~00f{A zfB+N%5P(7e0#FD*015#JKp_ADCxwvfXVc4X8AE@&(lkmiT9i% zlbU;aa0EMb0UmEnWoO1g<<5LOfIlux_CJ7IPENh!vYlse0RJ`s_;Pv%OngZU?2O-E z7z|+#e!m3S{}ArOiZ9D$1N+AmPGHa&;F~R$p% z@?6Io1Ta$bfA7OC{4)3Ke+&=c4y?iIG=Q7*e2}Gn*doB!TP!;O$@FW$pfS(L{Od=s z4?FPl9JBujHsB7t4y*7QBqaxX&pPVQU}#=X&8rQt3%l(C*n`B%>7-;Fk6|yMNAMDM z;K#XS|5Mn2H>~X6bO3CFbCdTo>+sLESLQ9`MYth=y$U5k16QIF!^<$Tj-Y`tcm_Xo zoc+(>EtCCQCi^QEK;iH|H>uCI)S3rp&RX;*Jed(7@l67+fmdP@Jxj6~eQFsT!VCBZ zzV9sikKt{&3+q<)Z^AO%@E!gcKw7mO!b{kj5ujhb3WW`_UrmA%WdOE8wv0Z9zu~(M zvi~`}1NUGZ)~2%`1{lEzQZvZ60UX0ocL3g`_sb0uQ?b1Og*{_4opXZ|)0jSozu?jE}9}VbCc+QZfm`68WB`;8d8HrXsagQo9(ufF0O| zpIXZPC$I?{aMxM-Z2&v(xx~ceJ)5#8)|LYtz&`B3Q`l|*(2wqdDjE7d058M&bWO7> zLDmsHX|FMT1n-;9XWaqF-u{x4`fM;U*&my>#|x0pfc60@4ctHW0u&g=rh*N9?NE0B zg&DA7-trCK`eOr(;LHLTn5}()q~;_@oImcBFFtk=E?a`;2~em3?kaRXy#y5ij!oTa z4)B$8B9=6o1c4ghl{gR9fZYxN9t3N^lCJ>td^I3kgu4Q$We`&X@+HVy13CgIs{w^B zsH_I$0E6ioFeiY>6N03?2H*o2nocyE+IbP)ZWUnDD#EL#4PExtfWkr)Q-tRYP}zyb z7vVq+=m;QO0|G_3TLAB+h`LLEOcxYigckx(R)pOy=u!jp%3GkA6R|}BxY_@I0J!~9 zxD6G60BQrc=K%tk6F{LQwXcb~*GIiuY7J5|FqdHja0LLab`dU|6{jW1y<7lSsSPdE zgRTH{B35=HRsirOJ&iL+Ee7-6bt$6iQbY_ur9?Xb*YjdrEr2qK767j81=*0CaP1830w2qgDkjUW(A|>EYYcdvP#c zi9j1#1keK-fd@2tRReb%*#%Vt=nzOKeZD_>Vr|3`gc(}F_) z0uX?*AOHa<1Rwx~00f{AfB+N%5P(7e0#FD*015#JKp_ADC