From 6969bd4da8b6b040410c543d9c0fce29801b76f5 Mon Sep 17 00:00:00 2001 From: Minher0 Date: Thu, 29 Oct 2026 10:11:06 +0000 Subject: [PATCH] Review fixes: pass &env to add_environment, extract env_shadow_map, drop the repro test (#2436) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - add_environment takes the evaluated light (&EnvLight) instead of nine loose parameters — strict clippy (#2496) denies too_many_arguments. - The Environment light's shadow map moves into env_shadow_map so shadow_maps stays under the cognitive-complexity limit. - tests/it/ui_env_light_repro.rs deleted: it duplicated ui_env_light_gpu and the engine tests, and polled with sleeps of up to 12 s. Signed-off-by: Minher0 --- crates/render/src/three_d/adv/raster.rs | 37 +++++---- crates/render/src/three_d/adv/scene.rs | 34 +++------ crates/ui-egui/tests/it/main.rs | 1 - crates/ui-egui/tests/it/ui_env_light_repro.rs | 75 ------------------- 4 files changed, 33 insertions(+), 114 deletions(-) delete mode 100644 crates/ui-egui/tests/it/ui_env_light_repro.rs diff --git a/crates/render/src/three_d/adv/raster.rs b/crates/render/src/three_d/adv/raster.rs index 91bf45307..1c37a6267 100644 --- a/crates/render/src/three_d/adv/raster.rs +++ b/crates/render/src/three_d/adv/raster.rs @@ -13,7 +13,7 @@ use std::collections::BinaryHeap; use rayon::prelude::*; -use super::scene::{Scene, ShadowMap, Vertex}; +use super::scene::{EnvInfo, Scene, ShadowMap, Vertex}; use super::shade::{Frag, sample, shade}; /// A rendered target at raster size. @@ -500,6 +500,26 @@ pub(crate) fn shadow_sizes(width: u32, height: u32, draft: bool, kinds: &[u32]) sizes.into_iter().map(|z| (z > 0).then_some(z)).collect() } +/// The Environment light's shadow map: one orthographic map over the scene's bounding sphere +/// from its dominant direction (the light travels from the bright region into the scene), like +/// a parallel light's. +fn env_shadow_map(e: EnvInfo, centre: [f64; 3], radius: f64, size: u32) -> ShadowMap { + // After Effects' environment-light shadows are soft out of the box, and this + // map covers the whole scene bounding sphere — not the view — so its texels + // can be coarse (all the more in a draft render's flat 512 budget). A + // penumbra of at least eight output pixels, or four map texels where those + // are coarser, keeps the edge smooth at any map resolution; Shadow Diffusion + // widens it further, as for the other lights. + let texel = 2.0 * radius / size as f64; + let soft = (4.0 * texel).max(8.0) + (e.shadow_diffusion as f64 * 0.5).min(24.0); + let sun = e.sun.map(|v| v as f64); + let dir = [-sun[0], -sun[1], -sun[2]]; + let eye = [centre[0] + sun[0] * radius * 2.0, centre[1] + sun[1] * radius * 2.0, centre[2] + sun[2] * radius * 2.0]; + let hint = if dir[1].abs() < 0.9 { [0.0, 1.0, 0.0] } else { [0.0, 0.0, 1.0] }; + let focal = (size as f64 * 0.5 / radius) as f32; + ShadowMap { view: basis(eye, dir, hint), focal, ortho: true, size, offset: 0, bias: 1.5, radius: soft as f32 } +} + /// Build the shadow maps of every shadow-casting light (lights marked `shadow == 0`), and of /// the Environment light when it casts shadows and its image has a dominant direction (marked /// `env.shadow == 0`, `env.sun != [0; 3]`): one map from that direction, like a parallel light. @@ -599,20 +619,7 @@ pub(crate) fn shadow_maps(s: &mut Scene, draft: bool) { match sizes.get(s.lights.len()).copied().flatten() { Some(size) => { let first = maps.len() as i32; - // After Effects' environment-light shadows are soft out of the box, and this - // map covers the whole scene bounding sphere — not the view — so its texels - // can be coarse (all the more in a draft render's flat 512 budget). A - // penumbra of at least eight output pixels, or four map texels where those - // are coarser, keeps the edge smooth at any map resolution; Shadow Diffusion - // widens it further, as for the other lights. - let texel = 2.0 * radius / size as f64; - let soft = (4.0 * texel).max(8.0) + (e.shadow_diffusion as f64 * 0.5).min(24.0); - let sun = e.sun.map(|v| v as f64); - let dir = [-sun[0], -sun[1], -sun[2]]; - let eye = [centre[0] + sun[0] * radius * 2.0, centre[1] + sun[1] * radius * 2.0, centre[2] + sun[2] * radius * 2.0]; - let hint = if dir[1].abs() < 0.9 { [0.0, 1.0, 0.0] } else { [0.0, 0.0, 1.0] }; - let focal = (size as f64 * 0.5 / radius) as f32; - maps.push(ShadowMap { view: basis(eye, dir, hint), focal, ortho: true, size, offset: 0, bias: 1.5, radius: soft as f32 }); + maps.push(env_shadow_map(e, centre, radius, size)); if let Some(e) = &mut s.env { e.shadow = first; } diff --git a/crates/render/src/three_d/adv/scene.rs b/crates/render/src/three_d/adv/scene.rs index dfd35b048..1197e5916 100644 --- a/crates/render/src/three_d/adv/scene.rs +++ b/crates/render/src/three_d/adv/scene.rs @@ -691,7 +691,7 @@ fn card_layer(r: &Renderer, ctx: &EvalCtx, layer: &Layer, b: &mut Builder) { /// The evaluated Environment light: its image source, intensity × colour, rotation, and /// shadow settings (After Effects' Light Settings shows Cast Shadows for Environment lights). #[derive(Clone, Copy)] -struct EnvLight { +pub(crate) struct EnvLight { src: Option, color: [f32; 3], rot: f32, @@ -856,27 +856,17 @@ pub(crate) fn dominant_direction(w: u32, h: u32, d: &[[f32; 4]], tint: [f32; 3], } /// Prefilter an environment image into the scene's textures. -pub(crate) fn add_environment( - b: &mut Builder, - w: u32, - h: u32, - data: Vec<[f32; 4]>, - intensity: [f32; 3], - rotation: f32, - casts_shadows: bool, - shadow_darkness: f64, - shadow_diffusion: f64, -) { +pub(crate) fn add_environment(b: &mut Builder, env: &EnvLight, w: u32, h: u32, data: Vec<[f32; 4]>) { // Intensity × colour folded into the radiance (one scalar on the GPU): tint the texels. // A white (or off) light leaves them as they are. - let k = (intensity[0] + intensity[1] + intensity[2]) / 3.0; - let tint = if k > 0.0 && (intensity[0] != intensity[1] || intensity[1] != intensity[2]) { intensity.map(|c| c / k) } else { [1.0, 1.0, 1.0] }; + let k = (env.color[0] + env.color[1] + env.color[2]) / 3.0; + let tint = if k > 0.0 && (env.color[0] != env.color[1] || env.color[1] != env.color[2]) { env.color.map(|c| c / k) } else { [1.0, 1.0, 1.0] }; // The shadow direction and key strength from the full-resolution image, before the mip // chain averages it — only a shadow-casting light reads them, so only one pays for the // pass — and weighted by the tint, so the shadow follows the radiance the light emits: // opposing chromatic regions that cancel untinted need not cancel once the colour removes // one of them (a red light over equally bright red and blue sides shadows from the red). - let (sun, key) = if casts_shadows { dominant_direction(w, h, &data, tint, rotation) } else { ([0.0; 3], 0.0) }; + let (sun, key) = if env.casts_shadows { dominant_direction(w, h, &data, tint, env.rot) } else { ([0.0; 3], 0.0) }; // Radiance level 0 at most 1024 wide. let (mut w, mut h, mut d) = (w, h, data); while w > 1024 { @@ -914,13 +904,13 @@ pub(crate) fn add_environment( mips: levels, irradiance: ii, intensity: k, - rotation, + rotation: env.rot, sun, key, // Marked; `raster::shadow_maps` assigns the map (a directionless image casts none). - shadow: if casts_shadows { 0 } else { -1 }, - shadow_darkness: shadow_darkness as f32, - shadow_diffusion: shadow_diffusion as f32, + shadow: if env.casts_shadows { 0 } else { -1 }, + shadow_darkness: env.shadow_darkness as f32, + shadow_diffusion: env.shadow_diffusion as f32, }); } @@ -1091,16 +1081,14 @@ pub(crate) fn build_at(r: &Renderer, ctx: &EvalCtx, sub: Option<&EvalCtx>, run: b.s.ambient = amb; if let Some(env) = env { match env_pixels(r, ctx, env.src, lin) { - EnvPixels::Image((w, h, d)) => { - add_environment(&mut b, w, h, d, env.color, env.rot, env.casts_shadows, env.shadow_darkness, env.shadow_diffusion) - } + EnvPixels::Image((w, h, d)) => add_environment(&mut b, &env, w, h, d), // No Source and no Environment Layer: the default environment, a soft studio // gradient of the light's colour × intensity — After Effects "uses a default // environment map" for an Environment light — so the light always lights the // scene per its parameters, and its dominant region casts shadows (#2436). EnvPixels::Absent => { let (w, h, d) = default_environment(); - add_environment(&mut b, w, h, d, env.color, env.rot, env.casts_shadows, env.shadow_darkness, env.shadow_diffusion) + add_environment(&mut b, &env, w, h, d) } // A selected or found source that produced no pixels (deleted layer, missing // footage, failed decode): no environment at all — the light contributes diff --git a/crates/ui-egui/tests/it/main.rs b/crates/ui-egui/tests/it/main.rs index 724b64b08..a99094baf 100644 --- a/crates/ui-egui/tests/it/main.rs +++ b/crates/ui-egui/tests/it/main.rs @@ -21,7 +21,6 @@ mod ui_effect_controls; mod ui_effect_drop_highlight; mod ui_effect_names; mod ui_env_light_gpu; -mod ui_env_light_repro; mod ui_font_search; mod ui_form_fit; mod ui_frame_step; diff --git a/crates/ui-egui/tests/it/ui_env_light_repro.rs b/crates/ui-egui/tests/it/ui_env_light_repro.rs deleted file mode 100644 index 78a26160f..000000000 --- a/crates/ui-egui/tests/it/ui_env_light_repro.rs +++ /dev/null @@ -1,75 +0,0 @@ -//! Reproduction of issue #2436's report: an Environment light added through the real command -//! path (what the Light Settings dialog runs) must visibly light the scene in the viewer. - -use effectcraft_engine::Session; -use effectcraft_engine::project::Renderer; -use effectcraft_ui_egui::EffectcraftApp; -use egui_kittest::Harness; -use serde_json::json; - -fn app() -> EffectcraftApp { - let mut s = Session::default(); - s.execute("file.openDemoProject", json!({})).unwrap(); - s.execute("comp.open", json!({"comp": "3D Showcase"})).unwrap(); - s.execute("time.set", json!({"time": 5.0})).unwrap(); - EffectcraftApp::new(s) -} - -/// Step the UI until background frame renders have landed. -fn settle(h: &mut Harness<'_, EffectcraftApp>) { - for _ in 0..600 { - h.step(); - if h.state().frames.inflight() == 0 && h.state().frames.last_ms.lock().map(|v| *v > 0.0).unwrap_or(false) { - break; - } - std::thread::sleep(std::time::Duration::from_millis(20)); - } - for _ in 0..4 { - h.step(); - } -} - -fn changed_pixels(before: &[egui::Color32], after: &[egui::Color32]) -> usize { - before.iter().zip(after).filter(|(a, b)| a != b).count() -} - -/// An Environment light (red, full intensity, no source: the flat default environment) added to -/// a Classic 3D comp switches it to Advanced 3D and visibly changes the viewer's pixels. -#[test] -fn environment_light_lights_the_scene() { - let mut h = Harness::builder().with_size(egui::vec2(1600.0, 1000.0)).build_eframe(|_| app()); - h.state_mut().session.execute("comp.settings", json!({"renderer": "classic3D"})).unwrap(); - h.run_steps(3); - settle(&mut h); - let before = h.state_mut().viewer_pixels().expect("frame before adding the light").pixels.clone(); - - h.state_mut().session.execute("layer.newLight", json!({"kind": "Environment", "color": [1.0, 0.0, 0.0], "intensity": 100.0})).unwrap(); - - assert_eq!(h.state().session.active_comp().unwrap().renderer, Renderer::Advanced3D, "the comp switched to Advanced 3D"); - settle(&mut h); - let after = h.state_mut().viewer_pixels().expect("frame after adding the light").pixels.clone(); - assert_eq!(before.len(), after.len()); - let diff = changed_pixels(&before, &after); - assert!(diff > 500, "a red Environment light at 100 % changed only {diff} pixels"); -} - -/// The literal action from the issue: default white Environment light, defaults everywhere — -/// even then the lighting model changes (Advanced 3D IBL instead of unlit/Classic), so pixels -/// must change somewhere in the comp frame. -#[test] -fn default_environment_light_changes_the_scene() { - let mut h = Harness::builder().with_size(egui::vec2(1600.0, 1000.0)).build_eframe(|_| app()); - h.state_mut().session.execute("comp.settings", json!({"renderer": "classic3D"})).unwrap(); - h.run_steps(3); - settle(&mut h); - let before = h.state_mut().viewer_pixels().expect("frame before adding the light").pixels.clone(); - - h.state_mut().session.execute("layer.newLight", json!({"kind": "Environment"})).unwrap(); - - assert_eq!(h.state().session.active_comp().unwrap().renderer, Renderer::Advanced3D); - settle(&mut h); - let after = h.state_mut().viewer_pixels().expect("frame after adding the light").pixels.clone(); - assert_eq!(before.len(), after.len()); - let diff = changed_pixels(&before, &after); - assert!(diff > 500, "a default white Environment light changed only {diff} pixels"); -}