diff --git a/crates/develop/src/lib.rs b/crates/develop/src/lib.rs index 4631e5e49..ea7ed1bd0 100644 --- a/crates/develop/src/lib.rs +++ b/crates/develop/src/lib.rs @@ -271,6 +271,54 @@ mod tests { assert_eq!(s.grading.blending, 50.0); } + /// Settings with one colour-range mask of `n` distinct samples (built in code, so not capped). + fn color_range_settings(n: usize) -> DevelopSettings { + let samples = (0..n).map(|i| [i as f64 / 1000.0, 0.1, -0.1]).collect(); + let mut s = DevelopSettings::default(); + s.masks.push(Mask { + id: 1, + components: vec![MaskComponent { name: None, op: MaskOp::Add, invert: false, shape: MaskShape::ColorRange { samples, refine: 50.0 } }], + ..Default::default() + }); + s + } + + fn color_range_samples(s: &DevelopSettings) -> &[[f64; 3]] { + match s.masks.first().and_then(|m| m.components.first()).map(|c| &c.shape) { + Some(MaskShape::ColorRange { samples, .. }) => samples, + other => panic!("expected a colour range, got {other:?}"), + } + } + + /// Issue #2556: `mask.add`/`mask.update` capped a colour range's samples, but a sidecar `lc:settings`, a + /// preset, `develop.merge`, `render --settings` or a catalog / history load took any number (render cost is + /// pixels x samples). Every load now keeps the first [`MAX_COLOR_RANGE_SAMPLES`], in order. + #[test] + fn color_range_samples_are_capped_however_settings_load() { + let big = color_range_settings(1000); + let want = &color_range_samples(&big)[..MAX_COLOR_RANGE_SAMPLES]; + let v = big.to_json(); + let loaded = DevelopSettings::from_json(&v).unwrap(); + assert_eq!(color_range_samples(&loaded).len(), MAX_COLOR_RANGE_SAMPLES); + assert_eq!(color_range_samples(&loaded), want); + let from_str: DevelopSettings = serde_json::from_str(&v.to_string()).unwrap(); + assert_eq!(color_range_samples(&from_str), want); + // `develop.merge`, `render --settings` and presets merge a partial + let merged = DevelopSettings::default().merged(&json!({"masks": v["masks"]})).unwrap(); + assert_eq!(color_range_samples(&merged), want); + let preset = + Preset { id: "p".into(), name: "P".into(), group: "G".into(), settings: json!({"masks": v["masks"]}), favorite: false, builtin: false }; + assert_eq!(color_range_samples(&preset.apply(&DevelopSettings::default(), 1.0)), want); + // a capped list round-trips, and a short one is unchanged + assert_eq!(DevelopSettings::from_json(&loaded.to_json()).unwrap(), loaded); + let few = color_range_settings(2); + assert_eq!(DevelopSettings::from_json(&few.to_json()).unwrap(), few); + // a list that isn't samples is still refused + let mut bad = v.clone(); + bad["masks"][0]["components"][0]["shape"]["samples"] = json!([[0.1, 0.2]]); + assert!(DevelopSettings::from_json(&bad).is_err()); + } + #[test] fn merge_partial() { let s = DevelopSettings::default(); diff --git a/crates/develop/src/settings.rs b/crates/develop/src/settings.rs index a40f094c2..ceef048c8 100644 --- a/crates/develop/src/settings.rs +++ b/crates/develop/src/settings.rs @@ -921,6 +921,35 @@ impl Default for BrushStroke { } } +/// Most samples a colour-range mask keeps. It compares every pixel against every sample, so render and export cost +/// is pixels x samples; the interactive sampler stops at 5, and a sidecar, preset or `mask.add` with thousands could +/// stall a render or lose the GPU device (issue #2556). +pub const MAX_COLOR_RANGE_SAMPLES: usize = 64; + +/// Deserialize a colour range's samples, keeping the first [`MAX_COLOR_RANGE_SAMPLES`] (the rest are skipped without +/// being stored), however the settings arrive: sidecar, catalog, history, preset or merge. +fn de_color_range_samples<'de, D: serde::Deserializer<'de>>(d: D) -> Result, D::Error> { + struct Capped; + impl<'de> serde::de::Visitor<'de> for Capped { + type Value = Vec<[f64; 3]>; + fn expecting(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + f.write_str("a list of OkLab colours") + } + fn visit_seq>(self, mut seq: A) -> Result, A::Error> { + let mut out = Vec::new(); + while out.len() < MAX_COLOR_RANGE_SAMPLES { + match seq.next_element::<[f64; 3]>()? { + Some(s) => out.push(s), + None => return Ok(out), + } + } + while seq.next_element::()?.is_some() {} + Ok(out) + } + } + d.deserialize_seq(Capped) +} + #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] #[serde(tag = "kind", rename_all = "camelCase")] pub enum MaskShape { @@ -941,8 +970,9 @@ pub enum MaskShape { feather: f64, invert: bool, }, - /// Colour range: sampled OkLab colours with a refine amount 0..100. + /// Colour range: sampled OkLab colours (at most [`MAX_COLOR_RANGE_SAMPLES`]) with a refine amount 0..100. ColorRange { + #[serde(deserialize_with = "de_color_range_samples")] samples: Vec<[f64; 3]>, refine: f64, }, diff --git a/crates/engine/src/cmd/masks.rs b/crates/engine/src/cmd/masks.rs index 2fd44d566..ceea76e1b 100644 --- a/crates/engine/src/cmd/masks.rs +++ b/crates/engine/src/cmd/masks.rs @@ -1,6 +1,6 @@ //! Masking and Remove (spot) commands on the active photo. -use lightcraft_develop::{BrushStroke, LocalAdjustments, Mask, MaskComponent, MaskOp, MaskShape, RedEye, Spot, SpotMode}; +use lightcraft_develop::{BrushStroke, LocalAdjustments, MAX_COLOR_RANGE_SAMPLES, Mask, MaskComponent, MaskOp, MaskShape, RedEye, Spot, SpotMode}; use lightcraft_geom::Point; use serde_json::{Value, json}; @@ -8,11 +8,6 @@ use super::{CommandSpec, bad, bool_or, cmd, f64_or, has_active, ok, point, str_p use crate::segment::{Click, MAX_CLICKS}; use crate::{Result, Session}; -/// A colour-range mask compares every pixel against every sample, so render and export cost is -/// pixels x samples. The interactive sampler stops at 5; automation (`mask.add`/`mask.update`) -/// passed its `samples` array straight through, so a few hundred samples on a large render hung. -const MAX_COLOR_RANGE_SAMPLES: usize = 64; - /// Caps a colour-range shape's sample list at [`MAX_COLOR_RANGE_SAMPLES`] (render cost is pixels x samples). fn cap_color_range_samples(shape: &mut MaskShape) { if let MaskShape::ColorRange { samples, .. } = shape { diff --git a/crates/engine/src/sidecar.rs b/crates/engine/src/sidecar.rs index d13b95125..60c180d2c 100644 --- a/crates/engine/src/sidecar.rs +++ b/crates/engine/src/sidecar.rs @@ -980,6 +980,23 @@ mod tests { assert_eq!(sc.keywords, Some(vec!["Places|Lisbon".to_string()])); } + /// Issue #2556: a sidecar's `lc:settings` colour-range mask keeps at most `MAX_COLOR_RANGE_SAMPLES` samples. + #[test] + fn sidecar_color_range_samples_are_capped() { + use lightcraft_develop::{MAX_COLOR_RANGE_SAMPLES, Mask, MaskComponent, MaskOp, MaskShape}; + let samples: Vec<[f64; 3]> = (0..1000).map(|i| [i as f64 / 1000.0, 0.1, -0.1]).collect(); + let mut d = DevelopSettings::default(); + let shape = MaskShape::ColorRange { samples: samples.clone(), refine: 50.0 }; + d.masks.push(Mask { id: 1, components: vec![MaskComponent { name: None, op: MaskOp::Add, invert: false, shape }], ..Default::default() }); + let xmp = lightcraft_meta::write_xmp(&lightcraft_meta::Metadata::default(), Some(&d.to_json().to_string())); + let sc = super::parse_sidecar(&xmp, crate::crs::Target::Rendered).unwrap(); + let Some(DevelopPatch::Full(s)) = sc.develop else { panic!("expected full settings") }; + let Some(MaskShape::ColorRange { samples: got, .. }) = s.masks.first().and_then(|m| m.components.first()).map(|c| &c.shape) else { + panic!("expected a colour range") + }; + assert_eq!(got.as_slice(), &samples[..MAX_COLOR_RANGE_SAMPLES]); + } + use super::*; use lightcraft_catalog::Meta; diff --git a/crates/gpu/src/render.rs b/crates/gpu/src/render.rs index 90aed7974..f9bbc23f6 100644 --- a/crates/gpu/src/render.rs +++ b/crates/gpu/src/render.rs @@ -1017,8 +1017,10 @@ fn masks(cx: &mut Cx<'_>, lin: &Buf, prep: &Prep, plan: &Plan<'_>, host: &mut Ho } MaskShape::ColorRange { samples, refine } => { let tol = lightcraft_pipeline::masks::color_range_tolerance(*refine); + // at most the CPU's samples (`MAX_COLOR_RANGE_SAMPLES`): cost is pixels x samples + let samples = samples.iter().take(lightcraft_develop::MAX_COLOR_RANGE_SAMPLES); p.extend([tol.to_bits(), ev.exp2().to_bits(), samples.len() as u32]); - aux.extend(samples.iter().flat_map(|s| s.map(|v| v as f32))); + aux.extend(samples.flat_map(|s| s.map(|v| v as f32))); Some(3) } MaskShape::Brush { strokes } => { diff --git a/crates/pipeline/src/masks.rs b/crates/pipeline/src/masks.rs index c45c88850..fdb84fd0d 100644 --- a/crates/pipeline/src/masks.rs +++ b/crates/pipeline/src/masks.rs @@ -251,7 +251,9 @@ pub fn shape_alpha(shape: &MaskShape, frame: &Frame, w: usize, h: usize, img: &R } MaskShape::ColorRange { samples, refine } => { let tol = color_range_tolerance(*refine); - let samples: Vec<[f32; 3]> = samples.iter().map(|s| [s[0] as f32, s[1] as f32, s[2] as f32]).collect(); + // (a settings value built in code can skip the deserializer's cap: cost is pixels x samples) + let samples: Vec<[f32; 3]> = + samples.iter().take(lightcraft_develop::MAX_COLOR_RANGE_SAMPLES).map(|s| [s[0] as f32, s[1] as f32, s[2] as f32]).collect(); for (v, c) in out.data.iter_mut().zip(&img.data) { let lab = lightcraft_color::perceptual::oklab_from_2020(tonemap_for_select(c.map(|v| v * gain))); let d = samples diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index a955061f5..9479846ff 100644 --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -299,7 +299,7 @@ differences and missing bindings are in [ui-parity.md](ui-parity.md#shortcuts). | LR-MASK-BRUSH | Brush mask | P0 | 🟡 | `cmd:tool.brush`, `cmd:mask.brushStroke` (`autoMask`), `crates/pipeline/src/masks.rs`, `crates/ui-egui/src/brush_live.rs` | size/feather/flow/density/erase; Create New Mask starts a separate brush in paint mode; the stroke shows while the button is held with the committed stroke's dab spacing, feather and flow, so fast strokes have no gaps (#517); with the mask overlay hidden (O / the eye, or after a mask slider hid it) or a colour overlay at opacity 0 it is not tinted while painting either, so dodging and burning is judged on the photo, and its effect shows once the stroke is committed (#2549); coverage builds up by passes, not hold time (holding the brush still adds nothing, live or committed, #518); oversized path segments skip intermediate dabs instead of allocating a million samples; Auto Mask: dabs weighted by similarity to the colour under the dab centre, refined by a guided filter on luminance (CPU + GPU); ⇧-click draws a straight stroke from where the last stroke ended (`connect`), tapering size, feather and flow from that stroke's values when they changed; no A/B brushes; pen pressure (Brush ▸ Pen Pressure, on by default; `pressure` per point in `mask.brushStroke`) scales each dab's size (a tenth to all of it) and flow, live and committed alike, where winit reports a pen force (Windows; not macOS / Linux tablets yet); no tilt | | LR-MASK-LINEAR | Linear gradient | P0 | ✅ | `cmd:tool.linear`, `cmd:mask.update` | | | LR-MASK-RADIAL | Radial gradient | P0 | ✅ | `cmd:tool.radial`, `cmd:mask.update`, `crates/ui-egui/src/panels/detail.rs` | drag the selected ellipse's body or centre pin to move; edge grips resize, outer grip rotates; rotated and compound radial components, hidden pins, outside hit rejection and one-step undo tested | -| LR-MASK-COLORRANGE | Colour range | P1 | ✅ | `cmd:mask.add` (`colorRange`), `cmd:mask.sampleColor`, `cmd:mask.update`, `crates/pipeline/src/lib.rs` (`color_range_sample`) | Color tile → click the photo to sample (⇧-click adds, up to 5; samples taken in the space the mask compares in, respecting section eyes and the requested HDR/SDR rendition), Pick button, Refine slider | +| LR-MASK-COLORRANGE | Colour range | P1 | ✅ | `cmd:mask.add` (`colorRange`), `cmd:mask.sampleColor`, `cmd:mask.update`, `crates/pipeline/src/lib.rs` (`color_range_sample`) | Color tile → click the photo to sample (⇧-click adds, up to 5; samples taken in the space the mask compares in, respecting section eyes and the requested HDR/SDR rendition), Pick button, Refine slider; at most 64 samples (`MAX_COLOR_RANGE_SAMPLES`, render cost is pixels × samples) however the mask arrives: `mask.add` / `mask.update`, a sidecar's `lc:settings`, a preset, `develop.merge`, `render --settings` or the catalog and history keep the first 64 (issue #2556) | | LR-MASK-LUMRANGE | Luminance range | P1 | ✅ | `cmd:mask.add` (`luminanceRange`), `cmd:mask.update`, `crates/ui-egui/src/panels/masking.rs` (`range_controls`) | range bar with two handles (the pressed endpoint stays selected through fast/crossing drags; one undo step per drag), Smoothness, Show Luminance Map (B&W photo with the range tinted) | | LR-MASK-DEPTHRANGE | Depth range | P2 | ⬜ | | shape exists, needs depth data | | LR-MASK-COMBINE | Add / subtract / intersect | P0 | ✅ | `cmd:mask.addComponent`, `cmd:tool.brush` | Add/Subtract > Brush creates a component with the chosen operation; Erase removes paint within that brush component | @@ -964,3 +964,4 @@ Latin stays upright. Advanced Japanese composition remains open. Regression cove | 2026-10-27 | patch | MENU-EDIT-SELECTNONE, MENU-EDIT-PASTESELECTED: Select Only Active Photo and Paste Settings from Previous were not in the Edit menu's layout and ended up after Settings…, near the end of the menu; they now sit with the other Select and Paste rows. On macOS the Edit menu no longer shows two separators in a row where Settings… moved to the app menu; status unchanged (✅) | | 2026-10-27 | patch | MENU-FILE-CLOSE, KEY-CLOSEWINDOW: the notes say that Close Window and the red close button of the main window quit the app, after the check Quit makes; kept on purpose; a user page says so; status unchanged (✅) | | 2026-10-29 | minor | LR-MASK-OBJECTS: an Object / Describe selection's stored grid is enlarged along the photo's edges (fast guided filter of its logits by log luminance, a 1.5-cell window), so a raised Exposure no longer draws an outline around the subject (#3272); existing AI masks are 1–2 px tighter at edges; status unchanged (🟡) | +| 2026-10-30 | patch | LR-MASK-COLORRANGE: a colour-range mask keeps its first 64 samples however it is loaded (sidecar `lc:settings`, presets, `develop.merge`, `render --settings`, catalog and history), not only through `mask.add` / `mask.update`, so a document with thousands can't stall a render or lose the GPU device (issue #2556); status unchanged (✅) | diff --git a/docs/whats-new.md b/docs/whats-new.md index 7464f430e..013220cd0 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -1,5 +1,14 @@ # What's new in LightCraft +## 2026-10-30 + +### A colour-range mask keeps at most 64 samples, however it arrives +- A colour-range mask compares every pixel with every colour it has sampled, so a mask with thousands of samples + could stall a render or export, or lose the GPU for the rest of the session. `mask.add` and `mask.update` already + kept the first 64; a sidecar's `lc:settings`, a preset, `develop.merge`, `render --settings` and the catalog and + edit history now keep the first 64 too, in order, and the extra ones are skipped as they are read. Masks made in + the app (up to 5 samples) are unchanged. + ## 2026-10-29 ### Object and Describe masks show their pins @@ -21,6 +30,7 @@ - Fielded search tokens now take inclusive comparisons: `rating:>=3`, `iso:<=400`, `f:>=2.8`, `focal:<=50`. Before, the `=` was read as part of the number, so such a search quietly showed no photos at all. + ## 2026-10-24 ### Relative-date filters and smart albums follow the clock