From 725750eabb1e3d2a19162fa2de61465ad08af9b8 Mon Sep 17 00:00:00 2001 From: Kristian Myram Date: Fri, 30 Oct 2026 03:23:57 +0100 Subject: [PATCH] #1328: sample the displayed frame and guard modal ownership --- crates/ui-egui/src/app/frame.rs | 2 + crates/ui-egui/src/color_picker.rs | 116 +++++++++++++++++- crates/ui-egui/src/frames.rs | 16 ++- crates/ui-egui/src/frames/tests.rs | 6 + .../src/panels/effect_controls/viewer.rs | 9 +- crates/ui-egui/src/panels/fx_widgets.rs | 13 +- crates/ui-egui/src/panels/viewer_tools.rs | 80 ++++++++++++ 7 files changed, 223 insertions(+), 19 deletions(-) diff --git a/crates/ui-egui/src/app/frame.rs b/crates/ui-egui/src/app/frame.rs index f519bcdde..01245fde3 100644 --- a/crates/ui-egui/src/app/frame.rs +++ b/crates/ui-egui/src/app/frame.rs @@ -372,6 +372,7 @@ impl EffectcraftApp { // Video Preview is a second viewport: only a real pass can update it. prefs_live::frame(self, &ctx); self.auto.begin_frame(); + panels::viewer_tools::clear_sample_source(&ctx); if let Some(rect) = crate::color_picker::take_automation(&ctx) { self.auto.add("colorPicker.eyedropper", rect, "Eyedropper"); } @@ -398,6 +399,7 @@ impl EffectcraftApp { let header = egui::Rect::from_min_size(egui::pos2(full.min.x, top), egui::vec2(full.width(), header_h)); header::show(self, ui, header); let body = egui::Rect::from_min_max(egui::pos2(full.min.x + 4.0, header.max.y + 2.0), egui::pos2(full.max.x - 4.0, full.max.y - 4.0)); + crate::color_picker::note_dialog(&ctx, self.dialog); self.dock_area(ui, body); #[cfg(not(target_arch = "wasm32"))] prefs_live::dropped_files(self, ui, full); diff --git a/crates/ui-egui/src/color_picker.rs b/crates/ui-egui/src/color_picker.rs index b4fd70544..c75cc0242 100644 --- a/crates/ui-egui/src/color_picker.rs +++ b/crates/ui-egui/src/color_picker.rs @@ -122,12 +122,27 @@ struct Armed { popup: egui::Id, pass: u64, sample: Option<[f32; 3]>, + dialog: Option, } fn armed_key() -> egui::Id { egui::Id::unique("color-picker-eyedropper") } +fn dialog_key() -> egui::Id { + egui::Id::unique("color-picker-dialog") +} + +/// Record the modal before drawing the viewer. A picker may sample behind its own dialog, but +/// a new, unrelated modal must block it from its very first pass, before its rect is known. +pub(crate) fn note_dialog(ctx: &egui::Context, dialog: Option) { + ctx.data_mut(|data| data.insert_temp(dialog_key(), dialog)); +} + +fn current_dialog(ctx: &egui::Context) -> Option { + ctx.data(|data| data.get_temp::>(dialog_key())).flatten() +} + /// The armed eyedropper, if its picker was drawn in this pass or the last: one whose picker stops /// being drawn (its dialog gone) lapses. fn armed_record(ctx: &egui::Context) -> Option { @@ -147,7 +162,8 @@ pub fn eyedropper_armed(ctx: &egui::Context) -> bool { fn arm(ctx: &egui::Context, popup: egui::Id) { let pass = ctx.cumulative_pass_nr(); - ctx.data_mut(|d| d.insert_temp(armed_key(), Armed { popup, pass, sample: None })); + let dialog = current_dialog(ctx); + ctx.data_mut(|d| d.insert_temp(armed_key(), Armed { popup, pass, sample: None, dialog })); } fn disarm(ctx: &egui::Context, popup: egui::Id) { @@ -218,6 +234,9 @@ fn overlaid(ctx: &egui::Context, pos: egui::Pos2) -> bool { /// dialog. The pointer is read rather than the viewer's responses, since a modal dialog over the /// viewer takes its clicks. pub fn eyedropper_point(ctx: &egui::Context) -> Option { + if armed_record(ctx).is_some_and(|armed| armed.dialog != current_dialog(ctx)) { + return None; + } let viewer: Option<(u64, Rect)> = ctx.data(|d| d.get_temp(viewer_key())); let viewer = viewer.filter(|(p, _)| p.saturating_add(1) >= ctx.cumulative_pass_nr())?.1; let pos = ctx.input(|i| i.pointer.interact_pos())?; @@ -646,6 +665,101 @@ pub fn hsv_to_rgb([h, s, v]: [f32; 3]) -> [f32; 3] { mod tests { use super::*; + fn sampling_viewer() -> egui_kittest::Harness<'static, crate::EffectcraftApp> { + let mut session = effectcraft_engine::Session::default(); + session.execute("comp.new", serde_json::json!({"width": 320, "height": 180, "duration": 2})).unwrap(); + session.execute("layer.newSolid", serde_json::json!({"color": "#204060"})).unwrap(); + let mut harness = egui_kittest::Harness::builder().with_size(vec2(1200.0, 800.0)).build_eframe(|_| crate::EffectcraftApp::new(session)); + settle_sampling_viewer(&mut harness); + harness + } + + fn settle_sampling_viewer(harness: &mut egui_kittest::Harness<'_, crate::EffectcraftApp>) { + for _ in 0..200 { + harness.step(); + if harness.state().frames.inflight() == 0 && harness.state().viewer_image.is_some() { + harness.run_steps(3); + return; + } + std::thread::sleep(std::time::Duration::from_millis(5)); + } + panic!("the synthetic viewer frame did not arrive"); + } + + fn press_sampling_viewer(harness: &mut egui_kittest::Harness<'_, crate::EffectcraftApp>) -> Option<[f32; 3]> { + let ctx = harness.ctx.clone(); + arm(&ctx, egui::Id::unique("test-popup")); + let point = crate::panels::viewer::comp_to_screen(&ctx, [20.0, 20.0]).unwrap(); + harness.event(egui::Event::PointerMoved(point)); + harness.event(egui::Event::PointerButton { pos: point, button: egui::PointerButton::Primary, pressed: true, modifiers: egui::Modifiers::NONE }); + harness.step(); + let sample = ctx.data(|data| data.get_temp::(armed_key())).and_then(|armed| armed.sample); + harness.event(egui::Event::PointerButton { pos: point, button: egui::PointerButton::Primary, pressed: false, modifiers: egui::Modifiers::NONE }); + harness.step(); + sample + } + + #[test] + fn popup_eyedropper_samples_the_displayed_channel_and_exposure() { + let mut harness = sampling_viewer(); + harness.state_mut().session.state.viewer.channel = effectcraft_engine::viewer::Channel::Green; + let green = press_sampling_viewer(&mut harness).unwrap(); + assert!(green.iter().all(|value| (*value - 64.0 / 255.0).abs() < 0.01), "{green:?}"); + let options = &mut harness.state_mut().session.state.viewer; + options.channel = effectcraft_engine::viewer::Channel::Rgb; + options.exposure = 1.0; + let exposed = press_sampling_viewer(&mut harness).unwrap(); + let mut expected = [[32, 64, 96, 255]]; + effectcraft_engine::viewer::display_transform(&mut expected, effectcraft_engine::viewer::Channel::Rgb, false, 1.0); + for (actual, expected) in exposed.iter().zip(expected[0]) { + assert!((*actual - expected as f32 / 255.0).abs() < 0.01, "{exposed:?}"); + } + } + + #[test] + fn popup_eyedropper_samples_snapshot_and_refuses_wireframe() { + let mut harness = sampling_viewer(); + harness.state_mut().session.execute("view.takeSnapshot", serde_json::json!({})).unwrap(); + harness.state_mut().session.execute("layer.newSolid", serde_json::json!({"color": "#a0b0c0"})).unwrap(); + settle_sampling_viewer(&mut harness); + harness.event(egui::Event::Key { key: egui::Key::F5, physical_key: None, pressed: true, repeat: false, modifiers: egui::Modifiers::NONE }); + // Let the snapshot texture's discard passes finish before arming this synthetic picker. + harness.step(); + let snapshot = press_sampling_viewer(&mut harness).unwrap(); + for (actual, expected) in snapshot.iter().zip([32, 64, 96]) { + assert!((*actual - expected as f32 / 255.0).abs() < 0.01, "{snapshot:?}"); + } + let options = &mut harness.state_mut().session.state.viewer; + options.show_snapshot = false; + options.fast_previews = effectcraft_engine::commands::viewer_cmds::FastPreviews::Wireframe; + harness.event(egui::Event::Key { key: egui::Key::F5, physical_key: None, pressed: false, repeat: false, modifiers: egui::Modifiers::NONE }); + assert_eq!(press_sampling_viewer(&mut harness), None, "wireframe must not sample an old pixel frame"); + } + + #[test] + fn a_new_modal_backdrop_cannot_deliver_a_pending_sample() { + let mut harness = sampling_viewer(); + harness.state_mut().dialog = Some(crate::Dialog::About); + assert_eq!(press_sampling_viewer(&mut harness), None, "the first modal pass must already exclude its backdrop"); + } + + #[test] + fn a_picker_can_sample_behind_its_own_dialog_but_not_a_new_modal() { + let ctx = egui::Context::default(); + let point = pos2(20.0, 20.0); + let mut output = ctx.run_ui(egui::RawInput { events: vec![egui::Event::PointerMoved(point)], ..Default::default() }, |ui| { + let ctx = ui.ctx(); + note_dialog(ctx, Some(crate::Dialog::SolidSettings)); + note_viewer(ctx, Rect::from_min_size(pos2(0.0, 0.0), vec2(320.0, 180.0))); + note_overlay(ctx, Rect::from_min_size(pos2(100.0, 100.0), vec2(120.0, 80.0))); + arm(ctx, egui::Id::unique("test-picker")); + assert_eq!(eyedropper_point(ctx), Some(point)); + note_dialog(ctx, Some(crate::Dialog::About)); + assert_eq!(eyedropper_point(ctx), None); + }); + output.textures_delta.clear(); + } + #[test] fn armed_eyedropper_takes_one_sample_then_disarms() { let ctx = egui::Context::default(); diff --git a/crates/ui-egui/src/frames.rs b/crates/ui-egui/src/frames.rs index a7023a494..53e2248c5 100644 --- a/crates/ui-egui/src/frames.rs +++ b/crates/ui-egui/src/frames.rs @@ -534,18 +534,16 @@ impl Default for Frames { /// Premultiplied f32 → egui premultiplied Color32 (alpha kept, so the viewer can show the /// transparency grid or the comp background colour underneath). pub fn to_color_image(img: &effectcraft_engine::render::Image) -> egui::ColorImage { - let px: Vec = img - .data - .par_iter() - .map(|p| { - let a = p[3].clamp(0.0, 1.0); - let c = |v: f32| (v.clamp(0.0, a) * 255.0 + 0.5) as u8; - egui::Color32::from_rgba_premultiplied(c(p[0]), c(p[1]), c(p[2]), (a * 255.0 + 0.5) as u8) - }) - .collect(); + let px: Vec = img.data.par_iter().map(|p| display_pixel(*p)).collect(); egui::ColorImage::new([img.width as usize, img.height as usize], px) } +pub(crate) fn display_pixel(pixel: [f32; 4]) -> egui::Color32 { + let alpha = if pixel[3].is_nan() { 0.0 } else { pixel[3].clamp(0.0, 1.0) }; + let component = |value: f32| (value.clamp(0.0, alpha) * 255.0 + 0.5) as u8; + egui::Color32::from_rgba_premultiplied(component(pixel[0]), component(pixel[1]), component(pixel[2]), (alpha * 255.0 + 0.5) as u8) +} + /// The whole factor an image of `size` is averaged down by for a texture: at least `min_k`, and /// the smallest that fits its sides in `max_side` (frames of comps wider than the GPU's texture /// limit, #201). diff --git a/crates/ui-egui/src/frames/tests.rs b/crates/ui-egui/src/frames/tests.rs index 6da1dc269..9a522dc72 100644 --- a/crates/ui-egui/src/frames/tests.rs +++ b/crates/ui-egui/src/frames/tests.rs @@ -1,5 +1,11 @@ use super::*; +#[test] +fn converting_a_frame_with_nan_alpha_keeps_the_viewer_alive() { + let image = effectcraft_engine::render::Image::filled(1, 1, [0.5, 0.25, 0.1, f32::NAN]); + assert_eq!(to_color_image(&image).pixels, [egui::Color32::TRANSPARENT]); +} + struct ProbeDisk { keys: Mutex>, probes: std::sync::atomic::AtomicUsize, diff --git a/crates/ui-egui/src/panels/effect_controls/viewer.rs b/crates/ui-egui/src/panels/effect_controls/viewer.rs index 7bf65d40c..6f59d8c96 100644 --- a/crates/ui-egui/src/panels/effect_controls/viewer.rs +++ b/crates/ui-egui/src/panels/effect_controls/viewer.rs @@ -125,8 +125,7 @@ fn color_pick_preview(app: &EffectcraftApp, painter: &egui::Painter, map: &crate painter.rect_filled(chip, 11.0, Color32::from_black_alpha(180)); painter.text(chip.center(), Align2::CENTER_CENTER, hint, Tokens::ui(11.5), app.tokens.text); if let Some(pos) = pointer - && let Some(img) = &app.viewer_image - && let Some(sample) = fw::sample_frame(img, crate::panels::viewer_tools::shown_region(painter.ctx(), map.comp), map.to_comp(pos)) + && let Some(sample) = crate::panels::viewer_tools::sample_displayed(app, painter.ctx(), map.to_comp(pos)) { let swatch = Rect::from_min_size(pos + vec2(14.0, 14.0), vec2(26.0, 18.0)); painter.rect_filled(swatch, 2.0, Color32::from_rgb((sample[0] * 255.0) as u8, (sample[1] * 255.0) as u8, (sample[2] * 255.0) as u8)); @@ -162,8 +161,7 @@ fn text_color_pick( } if resp.clicked() && let Some(pos) = resp.interact_pointer_pos() - && let Some(img) = &app.viewer_image - && let Some(s) = fw::sample_frame(img, crate::panels::viewer_tools::shown_region(&ctx, map.comp), map.to_comp(pos)) + && let Some(s) = crate::panels::viewer_tools::sample_displayed(app, &ctx, map.to_comp(pos)) { let mut params = json!({"layer": pick.layer}); params[&pick.field] = json!([s[0], s[1], s[2]]); @@ -196,8 +194,7 @@ fn popup_eyedropper(app: &mut EffectcraftApp, ui: &mut egui::Ui, painter: &egui: app.viewer_pixels(); color_pick_preview(app, painter, map, crate::i18n::tr("Click to sample color"), crate::color_picker::eyedropper_point(&ctx)); if let Some(pos) = crate::color_picker::sample_press(&ctx) - && let Some(img) = &app.viewer_image - && let Some(s) = fw::sample_frame(img, crate::panels::viewer_tools::shown_region(&ctx, map.comp), map.to_comp(pos)) + && let Some(s) = crate::panels::viewer_tools::sample_displayed(app, &ctx, map.to_comp(pos)) { crate::color_picker::deliver_sample(&ctx, [s[0], s[1], s[2]]); } diff --git a/crates/ui-egui/src/panels/fx_widgets.rs b/crates/ui-egui/src/panels/fx_widgets.rs index fd71461e6..b3140d394 100644 --- a/crates/ui-egui/src/panels/fx_widgets.rs +++ b/crates/ui-egui/src/panels/fx_widgets.rs @@ -196,14 +196,21 @@ pub fn layer_to_comp(l2c: &Mat3, p: [f64; 2]) -> [f64; 2] { /// `region` ([x, y, w, h] in comp pixels: the whole comp, or the region of interest or Extended /// Viewer area it was rendered for), maybe at a reduced resolution. `None` outside it. pub fn frame_pixel(img: &egui::ColorImage, region: [f64; 4], pt: [f64; 2]) -> Option { + img.pixels.get(frame_pixel_index(img.size, region, pt)?).copied() +} + +pub(crate) fn frame_pixel_index(size: [usize; 2], region: [f64; 4], pt: [f64; 2]) -> Option { + if size.contains(&0) { + return None; + } let [x, y, w, h] = region; let (fx, fy) = ((pt[0] - x) / w, (pt[1] - y) / h); if !(0.0..1.0).contains(&fx) || !(0.0..1.0).contains(&fy) { return None; } - let sx = ((fx * img.size[0] as f64).floor() as usize).min(img.size[0].saturating_sub(1)); - let sy = ((fy * img.size[1] as f64).floor() as usize).min(img.size[1].saturating_sub(1)); - img.pixels.get(sy.checked_mul(img.size[0])?.checked_add(sx)?).copied() + let sx = ((fx * size[0] as f64).floor() as usize).min(size[0].saturating_sub(1)); + let sy = ((fy * size[1] as f64).floor() as usize).min(size[1].saturating_sub(1)); + sy.checked_mul(size[0])?.checked_add(sx) } /// Straight RGBA (0..1) of the displayed frame at comp point `pt` ([`frame_pixel`]). diff --git a/crates/ui-egui/src/panels/viewer_tools.rs b/crates/ui-egui/src/panels/viewer_tools.rs index f86a7bfee..3429f5404 100644 --- a/crates/ui-egui/src/panels/viewer_tools.rs +++ b/crates/ui-egui/src/panels/viewer_tools.rs @@ -366,6 +366,7 @@ pub(crate) fn display_key(app: &EffectcraftApp, has_conversion: bool) -> String /// is set), or the snapshot while Show Snapshot is on / F5 is held, through Show Channel and the /// exposure. Fast Previews ▸ Wireframe draws layer outlines instead. pub(crate) fn draw_frame(app: &mut EffectcraftApp, ctx: &egui::Context, painter: &egui::Painter, comp_rect: Rect, cid: ItemId, ectx: &EvalCtx) { + clear_sample_source(ctx); let opts = app.session.state.viewer.clone(); let snap = showing_snapshot(app, ctx); // Settings ▸ Video ▸ Mirror on Computer Monitor off: playback goes to Video Preview only. @@ -393,6 +394,8 @@ pub(crate) fn draw_frame(app: &mut EffectcraftApp, ctx: &egui::Context, painter: let dc = vw::DisplayColor::of(&app.session); let dc_key = display_key(app, dc.is_some()); let plain = opts.channel == Channel::Rgb && opts.exposure == 0.0 && dc.is_none(); + let display = SampleDisplay { channel: opts.channel, colorized: opts.colorized, exposure: opts.exposure, conversion: dc.clone() }; + let region = shown_region(ctx, [ectx.comp.width as f32, ectx.comp.height as f32]); let uv = Rect::from_min_max(pos2(0.0, 0.0), pos2(1.0, 1.0)); if snap { let Some(s) = app.session.snapshot.clone() else { return }; @@ -415,6 +418,12 @@ pub(crate) fn draw_frame(app: &mut EffectcraftApp, ctx: &egui::Context, painter: } }; painter.image(tex.id(), comp_rect, uv, Color32::WHITE); + note_sample_source( + ctx, + SampleSource::Snapshot(s.image.clone()), + [0.0, 0.0, ectx.comp.width as f64, ectx.comp.height as f64], + (!plain).then_some(display), + ); painter.text(comp_rect.left_top() + vec2(6.0, 6.0), Align2::LEFT_TOP, crate::i18n::tr("Snapshot"), Tokens::ui(11.0), SNAP_COLOR); return; } @@ -428,6 +437,9 @@ pub(crate) fn draw_frame(app: &mut EffectcraftApp, ctx: &egui::Context, painter: ctx.request_repaint(); } painter.image(id, comp_rect, uv, Color32::WHITE); + if let Some((_, key)) = app.viewer_shown { + note_sample_source(ctx, SampleSource::Gpu(key), region, None); + } return; } comp_rect @@ -444,6 +456,9 @@ pub(crate) fn draw_frame(app: &mut EffectcraftApp, ctx: &egui::Context, painter: if plain { let uv = app.viewer_image.as_ref().map_or(uv, |src| fitted_uv(ctx, src.size, minify)); painter.image(tex.id(), rect, uv, Color32::WHITE); + if let Some(image) = app.viewer_image.clone() { + note_sample_source(ctx, SampleSource::Cpu(image), region, None); + } return; } rect @@ -470,6 +485,71 @@ pub(crate) fn draw_frame(app: &mut EffectcraftApp, ctx: &egui::Context, painter: } }; painter.image(tex.id(), rect, fitted_uv(ctx, src.size, minify), Color32::WHITE); + note_sample_source(ctx, SampleSource::Cpu(src), region, Some(display)); +} + +#[derive(Clone)] +enum SampleSource { + Cpu(std::sync::Arc), + Gpu(crate::frames::FrameKey), + Snapshot(std::sync::Arc), +} + +#[derive(Clone)] +struct SampleDisplay { + channel: Channel, + colorized: bool, + exposure: f32, + conversion: Option, +} + +#[derive(Clone)] +struct DisplayedSample { + pass: u64, + source: SampleSource, + region: [f64; 4], + display: Option, +} + +fn sample_source_id() -> egui::Id { + egui::Id::unique("viewer-displayed-sample-source") +} + +pub(crate) fn clear_sample_source(ctx: &egui::Context) { + ctx.data_mut(|data| data.remove::(sample_source_id())); +} + +fn note_sample_source(ctx: &egui::Context, source: SampleSource, region: [f64; 4], display: Option) { + let sample = DisplayedSample { pass: ctx.cumulative_pass_nr(), source, region, display }; + ctx.data_mut(|data| data.insert_temp(sample_source_id(), sample)); +} + +/// Sample only a frame actually drawn this pass, with that draw's channel, exposure and +/// display conversion. Hidden and wireframe views deliberately have no sample source. +pub(crate) fn sample_displayed(app: &EffectcraftApp, ctx: &egui::Context, point: [f64; 2]) -> Option<[f32; 4]> { + let sample = ctx.data(|data| data.get_temp::(sample_source_id()))?; + if sample.pass != ctx.cumulative_pass_nr() { + return None; + } + let pixel = match sample.source { + SampleSource::Cpu(image) => super::fx_widgets::frame_pixel(&image, sample.region, point)?, + SampleSource::Gpu(key) => { + if app.viewer_shown.map(|(_, shown)| shown) != Some(key) { + return None; + } + super::fx_widgets::frame_pixel(app.viewer_image.as_ref()?, sample.region, point)? + } + SampleSource::Snapshot(image) => { + let index = super::fx_widgets::frame_pixel_index([image.width as usize, image.height as usize], sample.region, point)?; + crate::frames::display_pixel(*image.data.get(index)?) + } + }; + let image = egui::ColorImage::new([1, 1], vec![pixel]); + let image = match sample.display { + Some(display) => transformed(&image, display.channel, display.colorized, display.exposure, display.conversion), + None => image, + }; + super::fx_widgets::sample_frame(&image, [0.0, 0.0, 1.0, 1.0], [0.5, 0.5]) } /// The texture coordinates of a frame of `size` in its texture, fitted with at least `minify`