From 17d57a67058b1fe52fd600d76564bcfb16bbb4bd Mon Sep 17 00:00:00 2001 From: tylerjenningsw Date: Wed, 21 Oct 2026 12:41:50 -0400 Subject: [PATCH 1/3] Light Settings: list Environment and keep an existing light's type (#1588) The dialog offered four hard-coded light types and reopened an Environment light as Spot, so OK turned it into one. It now lists every light type the engine has, keeps the light's own type, and shows Falloff and Shadows only for lights that have them. --- crates/ui-egui/src/panels/dialogs_3d.rs | 36 +++++++++++++++---------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/crates/ui-egui/src/panels/dialogs_3d.rs b/crates/ui-egui/src/panels/dialogs_3d.rs index a1a67bc6..01fd5b9e 100644 --- a/crates/ui-egui/src/panels/dialogs_3d.rs +++ b/crates/ui-egui/src/panels/dialogs_3d.rs @@ -29,7 +29,7 @@ pub struct CameraDraft { pub struct LightDraft { pub layer: Option, pub name: String, - pub kind: usize, + pub kind: LightKind, pub color: [f32; 3], pub intensity: f64, pub cone_angle: f64, @@ -42,9 +42,14 @@ pub struct LightDraft { pub shadow_diffusion: f64, } -const KINDS: [LightKind; 4] = [LightKind::Parallel, LightKind::Spot, LightKind::Point, LightKind::Ambient]; const FALLOFFS: [&str; 3] = ["None", "Smooth", "Inverse Square Clamped"]; +/// Whether a light type has Falloff and Shadows options. Ambient and Environment lights come +/// from no one place, so they have neither. +fn falls_off(kind: LightKind) -> bool { + !matches!(kind, LightKind::Ambient | LightKind::Environment) +} + fn comp_w(app: &EffectcraftApp) -> f64 { app.session.active_comp().map(|c| c.width as f64).unwrap_or(1920.0) } @@ -105,7 +110,7 @@ pub fn open_light(app: &mut EffectcraftApp, layer: Option) -> Result<(), St LightDraft { layer: Some(l.id.0), name: l.name.clone(), - kind: KINDS.iter().position(|k| *k == kind).unwrap_or(1), + kind, color: [c[0], c[1], c[2]], intensity: f("intensity", 100.0), cone_angle: f("coneAngle", 90.0), @@ -123,7 +128,7 @@ pub fn open_light(app: &mut EffectcraftApp, layer: Option) -> Result<(), St LightDraft { layer: None, name: format!("Light {n}"), - kind: 1, + kind: LightKind::Spot, color: [1.0, 1.0, 1.0], intensity: 100.0, cone_angle: 90.0, @@ -263,11 +268,13 @@ pub fn light(app: &mut EffectcraftApp, ctx: &egui::Context, t: &Tokens) { crate::widgets::focus_on_open(ui.ctx(), &r, &d.name); ui.end_row(); ui.label(tr("Light Type")); - egui::ComboBox::from_id_salt("light-type").selected_text(KINDS[d.kind.min(3)].label()).show_ui(ui, |ui| { - for (i, k) in KINDS.iter().enumerate() { - ui.selectable_value(&mut d.kind, i, k.label()); + let combo = egui::ComboBox::from_id_salt("light-type").selected_text(tr(d.kind.label())).show_ui(ui, |ui| { + for (i, k) in LightKind::ALL.into_iter().enumerate() { + let item = ui.selectable_value(&mut d.kind, k, tr(k.label())); + app.auto.add(&format!("dialog.light.typeItem.{i}"), item.rect, k.label()); } }); + app.auto.add_response("dialog.light.type", &combo.response, "Light Type"); ui.end_row(); ui.label(tr("Color")); crate::widgets::srgb_color_button(ui, &mut d.color); @@ -275,8 +282,7 @@ pub fn light(app: &mut EffectcraftApp, ctx: &egui::Context, t: &Tokens) { ui.label(tr("Intensity")); ui.add(crate::widgets::drag_value(egui::DragValue::new(&mut d.intensity).range(-10000.0..=10000.0).suffix(" %"))); ui.end_row(); - let kind = KINDS[d.kind.min(3)]; - if kind == LightKind::Spot { + if d.kind == LightKind::Spot { ui.label(tr("Cone Angle")); ui.add(crate::widgets::drag_value(egui::DragValue::new(&mut d.cone_angle).range(0.0..=180.0).suffix("°"))); ui.end_row(); @@ -284,7 +290,7 @@ pub fn light(app: &mut EffectcraftApp, ctx: &egui::Context, t: &Tokens) { ui.add(crate::widgets::drag_value(egui::DragValue::new(&mut d.cone_feather).range(0.0..=100.0).suffix(" %"))); ui.end_row(); } - if kind != LightKind::Ambient { + if falls_off(d.kind) { ui.label(tr("Falloff")); egui::ComboBox::from_id_salt("light-falloff").selected_text(FALLOFFS[d.falloff.min(2)]).show_ui(ui, |ui| { for (i, f) in FALLOFFS.iter().enumerate() { @@ -312,15 +318,17 @@ pub fn light(app: &mut EffectcraftApp, ctx: &egui::Context, t: &Tokens) { } }); ui.add_space(14.0); - ui.label(egui::RichText::new(tr("Note: Shadows are only cast from layers with Cast Shadows enabled.")).font(Tokens::ui(11.0)).color(t.text_dim)); - ui.add_space(8.0); + if falls_off(d.kind) { + ui.label(egui::RichText::new(tr("Note: Shadows are only cast from layers with Cast Shadows enabled.")).font(Tokens::ui(11.0)).color(t.text_dim)); + ui.add_space(8.0); + } rects = buttons(ui, t, &mut ok, &mut close); }); app.auto.add("dialog.light.ok", rects.0, "OK"); app.auto.add("dialog.light.cancel", rects.1, "Cancel"); app.dialog_state.light = d.clone(); if ok || super::dialogs::ui_enter(ctx) { - let kind = KINDS[d.kind.min(3)]; + let kind = d.kind; let mut p = json!({ "name": d.name, "kind": kind.label(), @@ -331,7 +339,7 @@ pub fn light(app: &mut EffectcraftApp, ctx: &egui::Context, t: &Tokens) { p["coneAngle"] = json!(d.cone_angle); p["coneFeather"] = json!(d.cone_feather); } - if kind != LightKind::Ambient { + if falls_off(kind) { p["falloff"] = json!(d.falloff); p["radius"] = json!(d.radius); p["falloffDistance"] = json!(d.falloff_distance); From b9929c2d7108ea30b062ba2d34460efd3cfc1120 Mon Sep 17 00:00:00 2001 From: tylerjenningsw Date: Wed, 21 Oct 2026 12:41:50 -0400 Subject: [PATCH 2/3] Light Settings: test Environment lights and the other types through the dialog (#1588) --- crates/ui-egui/tests/ui_light_settings.rs | 172 ++++++++++++++++++++++ 1 file changed, 172 insertions(+) create mode 100644 crates/ui-egui/tests/ui_light_settings.rs diff --git a/crates/ui-egui/tests/ui_light_settings.rs b/crates/ui-egui/tests/ui_light_settings.rs new file mode 100644 index 00000000..a4284423 --- /dev/null +++ b/crates/ui-egui/tests/ui_light_settings.rs @@ -0,0 +1,172 @@ +//! Light Settings dialog (Layer ▸ New ▸ Light, Layer Settings on a light): every light type the +//! engine has, Environment included, and an existing light's type kept when it's reopened (#1588). + +use effectcraft_engine::Session; +use effectcraft_engine::project::{Layer, LayerId, LayerSource, LightKind}; +use effectcraft_ui_egui::{Dialog, EffectcraftApp}; +use egui_kittest::Harness; +use serde_json::{Value, json}; + +fn harness() -> Harness<'static, EffectcraftApp> { + let mut s = Session::default(); + s.execute("comp.new", json!({"name": "Lights", "width": 640, "height": 360, "duration": 2})).unwrap(); + let mut h = Harness::builder().with_size(egui::vec2(1600.0, 1000.0)).build_eframe(|_| EffectcraftApp::new(s)); + h.step(); + h +} + +/// An automation element's rect in the last frame drawn (a dialog settles its place in its +/// first frames, so the frame before can be stale). +fn element(h: &Harness<'_, EffectcraftApp>, id: &str) -> Option { + let auto = &h.state().auto; + auto.elements.iter().chain(auto.previous.iter()).find(|e| e.id == id).cloned() +} + +fn click_id(h: &mut Harness<'_, EffectcraftApp>, id: &str) { + let [x, y, w, hh] = element(h, id).unwrap_or_else(|| panic!("{id} is registered")).rect; + let p = egui::pos2(x + w / 2.0, y + hh / 2.0); + h.input_mut().events.push(egui::Event::PointerMoved(p)); + h.step(); + h.input_mut().events.push(egui::Event::PointerButton { pos: p, button: egui::PointerButton::Primary, pressed: true, modifiers: egui::Modifiers::NONE }); + h.step(); + h.input_mut().events.push(egui::Event::PointerButton { pos: p, button: egui::PointerButton::Primary, pressed: false, modifiers: egui::Modifiers::NONE }); + h.run_steps(3); +} + +fn invoke(h: &mut Harness<'_, EffectcraftApp>, id: &str, params: Value) { + let ctx = h.ctx.clone(); + effectcraft_ui_egui::menus::invoke(h.state_mut(), &ctx, id, params).unwrap(); + h.run_steps(2); +} + +/// Picks the dialog's Light Type `kind` from its menu. +fn choose(h: &mut Harness<'_, EffectcraftApp>, kind: LightKind) { + click_id(h, "dialog.light.type"); + let i = LightKind::ALL.iter().position(|k| *k == kind).unwrap(); + click_id(h, &format!("dialog.light.typeItem.{i}")); +} + +fn ok(h: &mut Harness<'_, EffectcraftApp>) { + click_id(h, "dialog.light.ok"); + assert_eq!(h.state().dialog, None); +} + +fn layer(h: &Harness<'_, EffectcraftApp>, id: u64) -> Layer { + h.state().session.active_comp().unwrap().layer(LayerId(id)).unwrap().clone() +} + +fn kind(h: &Harness<'_, EffectcraftApp>, id: u64) -> LightKind { + match layer(h, id).source { + LayerSource::Light { kind } => kind, + other => panic!("not a light: {other:?}"), + } +} + +fn last_layer(h: &Harness<'_, EffectcraftApp>) -> u64 { + h.state().session.active_comp().unwrap().layers.iter().map(|l| l.id.0).max().unwrap() +} + +/// Opens Light Settings for an existing light, as Layer Settings does. +fn reopen(h: &mut Harness<'_, EffectcraftApp>, id: u64) { + invoke(h, "layer.settings", json!({"layer": id})); + assert_eq!(h.state().dialog, Some(Dialog::LightSettings)); +} + +/// An Environment light lit from a solid, its image turned 30°. +fn environment_light(h: &mut Harness<'_, EffectcraftApp>) -> (u64, u64) { + let s = &mut h.state_mut().session; + let sky = s.execute("layer.newSolid", json!({"width": 256, "height": 128, "name": "Sky"})).unwrap()["layer"].as_u64().unwrap(); + let env = s.execute("layer.newLight", json!({"kind": "Environment", "name": "Env", "intensity": 80})).unwrap()["layer"].as_u64().unwrap(); + s.execute("prop.set", json!({"layer": env, "path": "lightOptions/source", "value": sky})).unwrap(); + s.execute("prop.set", json!({"layer": env, "path": "lightOptions/rotation", "value": 30})).unwrap(); + h.run_steps(2); + (env, sky) +} + +#[test] +fn the_light_type_menu_lists_every_light_type() { + let mut h = harness(); + invoke(&mut h, "layer.newLight", json!({})); + click_id(&mut h, "dialog.light.type"); + for (i, k) in LightKind::ALL.iter().enumerate() { + let item = element(&h, &format!("dialog.light.typeItem.{i}")).map(|e| e.label); + assert_eq!(item.as_deref(), Some(k.label())); + } +} + +#[test] +fn new_light_creates_an_environment_light() { + let mut h = harness(); + invoke(&mut h, "layer.newLight", json!({})); + choose(&mut h, LightKind::Environment); + ok(&mut h); + let id = last_layer(&h); + assert_eq!(kind(&h, id), LightKind::Environment); + let l = layer(&h, id); + assert!(l.props.prop("lightOptions/source").is_some() && l.props.prop("lightOptions/falloff").is_none()); + // One undoable step, redone as Environment. + h.state_mut().session.execute("edit.undo", json!({})).unwrap(); + assert!(h.state().session.active_comp().unwrap().layer(LayerId(id)).is_none()); + h.state_mut().session.execute("edit.redo", json!({})).unwrap(); + assert_eq!(kind(&h, id), LightKind::Environment); +} + +#[test] +fn reopening_an_environment_light_and_pressing_ok_keeps_it() { + let mut h = harness(); + let (env, sky) = environment_light(&mut h); + let before = layer(&h, env); + reopen(&mut h, env); + ok(&mut h); + let after = layer(&h, env); + assert_eq!(kind(&h, env), LightKind::Environment, "not turned into a Spot light"); + assert_eq!(after.props.prop("lightOptions/source").unwrap().value.as_layer(), Some(sky)); + for path in ["lightOptions/rotation", "lightOptions/intensity"] { + assert_eq!(after.props.prop(path).unwrap().value, before.props.prop(path).unwrap().value, "{path}"); + } +} + +#[test] +fn cancelling_light_settings_changes_nothing() { + let mut h = harness(); + let (env, _) = environment_light(&mut h); + let before = serde_json::to_value(layer(&h, env)).unwrap(); + let steps = h.state().session.history.undo.len(); + reopen(&mut h, env); + choose(&mut h, LightKind::Spot); + click_id(&mut h, "dialog.light.cancel"); + assert_eq!(h.state().dialog, None); + assert_eq!(serde_json::to_value(layer(&h, env)).unwrap(), before); + assert_eq!(h.state().session.history.undo.len(), steps); +} + +#[test] +fn changing_a_light_to_environment_undoes_and_redoes() { + let mut h = harness(); + let id = h.state_mut().session.execute("layer.newLight", json!({"kind": "Spot", "coneAngle": 45})).unwrap()["layer"].as_u64().unwrap(); + h.run_steps(2); + reopen(&mut h, id); + choose(&mut h, LightKind::Environment); + ok(&mut h); + assert_eq!(kind(&h, id), LightKind::Environment); + h.state_mut().session.execute("edit.undo", json!({})).unwrap(); + assert_eq!(kind(&h, id), LightKind::Spot); + assert_eq!(layer(&h, id).props.prop("lightOptions/coneAngle").unwrap().value.as_f64(), 45.0); + h.state_mut().session.execute("edit.redo", json!({})).unwrap(); + assert_eq!(kind(&h, id), LightKind::Environment); +} + +#[test] +fn the_other_light_types_are_created_and_kept() { + let mut h = harness(); + for k in [LightKind::Parallel, LightKind::Spot, LightKind::Point, LightKind::Ambient] { + invoke(&mut h, "layer.newLight", json!({})); + choose(&mut h, k); + ok(&mut h); + let id = last_layer(&h); + assert_eq!(kind(&h, id), k); + reopen(&mut h, id); + ok(&mut h); + assert_eq!(kind(&h, id), k, "{} kept on reopening", k.label()); + } +} From 83cd74c7305d5b2ce14630828579e556ff9b9ec0 Mon Sep 17 00:00:00 2001 From: tylerjenningsw Date: Wed, 21 Oct 2026 13:14:46 -0400 Subject: [PATCH 3/3] Light Settings: test which options each light type shows, and undoing Environment to Spot (#1588) --- crates/ui-egui/tests/ui_light_settings.rs | 31 ++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/crates/ui-egui/tests/ui_light_settings.rs b/crates/ui-egui/tests/ui_light_settings.rs index a4284423..fe1392be 100644 --- a/crates/ui-egui/tests/ui_light_settings.rs +++ b/crates/ui-egui/tests/ui_light_settings.rs @@ -4,7 +4,7 @@ use effectcraft_engine::Session; use effectcraft_engine::project::{Layer, LayerId, LayerSource, LightKind}; use effectcraft_ui_egui::{Dialog, EffectcraftApp}; -use egui_kittest::Harness; +use egui_kittest::{Harness, kittest::Queryable as _}; use serde_json::{Value, json}; fn harness() -> Harness<'static, EffectcraftApp> { @@ -170,3 +170,32 @@ fn the_other_light_types_are_created_and_kept() { assert_eq!(kind(&h, id), k, "{} kept on reopening", k.label()); } } + +#[test] +fn falloff_and_shadow_options_show_only_for_lights_that_have_them() { + let mut h = harness(); + invoke(&mut h, "layer.newLight", json!({})); + for k in LightKind::ALL { + choose(&mut h, k); + let shown = !matches!(k, LightKind::Ambient | LightKind::Environment); + for label in ["Falloff", "Casts Shadows", "Shadow Darkness"] { + assert_eq!(h.query_by_label(label).is_some(), shown, "{label} for {}", k.label()); + } + } +} + +#[test] +fn changing_an_environment_light_to_spot_undoes_to_its_source_and_rotation() { + let mut h = harness(); + let (env, sky) = environment_light(&mut h); + reopen(&mut h, env); + choose(&mut h, LightKind::Spot); + ok(&mut h); + assert_eq!(kind(&h, env), LightKind::Spot); + assert!(layer(&h, env).props.prop("lightOptions/source").is_none()); + h.state_mut().session.execute("edit.undo", json!({})).unwrap(); + let l = layer(&h, env); + assert_eq!(kind(&h, env), LightKind::Environment); + assert_eq!(l.props.prop("lightOptions/source").unwrap().value.as_layer(), Some(sky)); + assert_eq!(l.props.prop("lightOptions/rotation").unwrap().value.as_f64(), 30.0); +}