From a3e2271e947a039e869c0e9c881f2641b0fb81ce Mon Sep 17 00:00:00 2001 From: pravdomil <2387356+pravdomil@users.noreply.github.com> Date: Mon, 12 Oct 2026 22:52:38 +0200 Subject: [PATCH] Viewer eyedropper: stay armed until there is a frame to sample A colour pick clicked in the Composition panel before the viewer had a frame (still rendering on a background thread, or a readback pending) sampled nothing but was used up, leaving the colour unchanged. The click now leaves the eyedropper armed, as the text colour eyedropper already does. timeline_colour_eyedropper_samples_the_comp clicked without waiting for the frame and hit this in about one run in four; it now waits for the frame. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/panels/effect_controls.rs | 4 +++ crates/ui-egui/tests/ui_effect_controls.rs | 37 ++++++++++++++++++++ crates/ui-egui/tests/ui_timeline_shapes.rs | 14 +++++++- 3 files changed, 54 insertions(+), 1 deletion(-) diff --git a/crates/ui-egui/src/panels/effect_controls.rs b/crates/ui-egui/src/panels/effect_controls.rs index d8de61d8..4a3f9804 100644 --- a/crates/ui-egui/src/panels/effect_controls.rs +++ b/crates/ui-egui/src/panels/effect_controls.rs @@ -1390,7 +1390,11 @@ pub fn viewer_hook( painter.rect_stroke(sw, 2.0, Stroke::new(1.0, Color32::WHITE), egui::StrokeKind::Outside); } } + // No frame to sample yet (it is still rendering, or its readback is pending): the click + // leaves the eyedropper armed instead of using it up without a colour. + let unsampled = pick.kind == "color" && keyer.is_none() && app.viewer_image.is_none(); if resp.clicked() + && !unsampled && let Some(pos) = resp.interact_pointer_pos() { let c = map.to_comp(pos); diff --git a/crates/ui-egui/tests/ui_effect_controls.rs b/crates/ui-egui/tests/ui_effect_controls.rs index 3251aed0..5e98d9a7 100644 --- a/crates/ui-egui/tests/ui_effect_controls.rs +++ b/crates/ui-egui/tests/ui_effect_controls.rs @@ -189,6 +189,43 @@ fn the_eyedropper_samples_under_the_pointer_in_a_region_of_interest() { assert!(c[0] > 0.98 && c[1] < 0.02 && c[2] < 0.02, "{c:?}"); } +/// A click before the viewer has a frame to sample (it is still rendering) leaves the eyedropper +/// armed: it used the pick up without setting a colour. +#[test] +fn the_eyedropper_stays_armed_until_the_viewer_has_a_frame() { + // A large blurred frame, so the first render outlasts the click. + let mut s = Session::default(); + s.execute("comp.new", json!({"name": "Slow", "width": 7680, "height": 4320, "frameRate": 30, "duration": 1})).unwrap(); + let back = s.execute("layer.newSolid", json!({"name": "Back", "color": "#20c040"})).unwrap()["layer"].as_u64().unwrap(); + s.execute("effect.apply", json!({"layers": [back], "effect": "Gaussian Blur"})).unwrap(); + let top = s.execute("layer.newSolid", json!({"name": "Top", "color": "#ff0000", "width": 200, "height": 100})).unwrap()["layer"].as_u64().unwrap(); + s.execute("effect.apply", json!({"layers": [top], "effect": "Color Control"})).unwrap(); + let prop = layer(&s, top).effects().unwrap().groups().find(|g| g.match_id == "ec.control.color").and_then(|g| g.get("color")).map(|p| p.uid).unwrap(); + s.execute("prop.set", json!({"layer": top, "prop": prop, "value": [0.0, 0.0, 1.0, 1.0]})).unwrap(); + let colour = |h: &Harness<'_, EffectcraftApp>| { + let v = layer(&h.state().session, top).effects().unwrap().find(prop).unwrap().value.clone(); + let KV::Color(c) = v else { panic!("{v:?}") }; + c + }; + let mut h = Harness::builder().with_size(egui::vec2(1700.0, 1100.0)).build_eframe(|_| EffectcraftApp::new(s)); + h.state_mut().ui.fx_pick = Some(FxPick { kind: "color".into(), layer: top, prop, name: "Color".into() }); + h.step(); + let pos = effectcraft_ui_egui::panels::viewer::comp_to_screen(&h.ctx, [100.0, 100.0]).unwrap(); + click(&mut h, pos); + // (A frame that made it in time was sampled: the colour below is checked either way.) + if h.state().ui.fx_pick.is_some() { + assert_eq!(colour(&h), [0.0, 0.0, 1.0, 1.0], "nothing sampled yet"); + settle(&mut h); + click(&mut h, pos); + assert!(h.state().ui.fx_pick.is_none(), "the pick is used up"); + } + let c = colour(&h); + let want = [0x20 as f64 / 255.0, 0xc0 as f64 / 255.0, 0x40 as f64 / 255.0]; + for k in 0..3 { + assert!((c[k] - want[k]).abs() < 0.02, "{c:?} vs {want:?}"); + } +} + /// Key Light's Screen Colour eyedropper picks from the effect's input: the shown frame is /// already keyed (the default screen colour takes most of a green), so sampling it would miss. #[test] diff --git a/crates/ui-egui/tests/ui_timeline_shapes.rs b/crates/ui-egui/tests/ui_timeline_shapes.rs index 411bb22a..3993a6f9 100644 --- a/crates/ui-egui/tests/ui_timeline_shapes.rs +++ b/crates/ui-egui/tests/ui_timeline_shapes.rs @@ -28,6 +28,18 @@ fn center(h: &Harness<'_, EffectcraftApp>, id: &str) -> Pos2 { pos2(e.rect[0] + e.rect[2] / 2.0, e.rect[1] + e.rect[3] / 2.0) } +/// Step until the viewer's frame has rendered (frames render on background threads). +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(10)); + } + h.run_steps(2); +} + fn click_at(h: &mut Harness<'_, EffectcraftApp>, p: Pos2) { h.input_mut().events.push(Event::PointerMoved(p)); h.step(); @@ -224,7 +236,7 @@ fn timeline_colour_eyedropper_samples_the_comp() { let mut h = harness(s); h.state_mut().ui.timeline.open_groups.extend(open); open_layer(&mut h, l); - h.run_steps(2); + settle(&mut h); click(&mut h, &format!("timeline.prop.{fill}.eyedropper")); assert_eq!(h.state().ui.fx_pick.as_ref().map(|p| (p.prop, p.kind.as_str())), Some((fill, "color")), "the eyedropper waits for a viewer click"); let pos = effectcraft_ui_egui::panels::viewer::comp_to_screen(&h.ctx, [6.0, 6.0]).unwrap();