Review fixes: pass &env to add_environment, extract env_shadow_map, drop the repro test (#2436)

- 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 <minhero@outlook.fr>

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;
                 }

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<u64>,
     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

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;

crates/ui-egui/tests/it/ui_env_light_repro.rs

@@ -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");
-}