From bc12ec8e40617928b44764030ddb90347d25e89e Mon Sep 17 00:00:00 2001 From: Bruno-Alumn <171815746+Bruno-Alumn@users.noreply.github.com> Date: Fri, 30 Oct 2026 13:31:01 +0100 Subject: [PATCH 1/2] Never crash: a mask id at the u32 limit doesn't overflow the next mask's id Mask ids come unchecked from develop.merge, sidecars (lc:settings) and pastes. With a mask id of u32::MAX, next_mask_id() and Preset::apply added 1 to it: debug builds panicked, release builds wrapped to 0, so the second new mask got the same id as the first. next_mask_id() now falls back to the lowest id no mask uses when the highest is at the limit, and Preset::apply takes each preset mask's id from it instead of counting on its own, so mask.add, mask.brushStroke, mask.duplicate and presets with masks all get unused ids. --- crates/develop/src/lib.rs | 22 ++++++++++++++++++++-- crates/develop/src/presets.rs | 23 ++++++++++++++++++++--- crates/engine/src/tests.rs | 11 +++++++++++ docs/whats-new.md | 7 +++++++ 4 files changed, 58 insertions(+), 5 deletions(-) diff --git a/crates/develop/src/lib.rs b/crates/develop/src/lib.rs index 4631e5e49..dd059c117 100644 --- a/crates/develop/src/lib.rs +++ b/crates/develop/src/lib.rs @@ -161,9 +161,15 @@ impl DevelopSettings { } } - /// Next free mask id. + /// Next free mask id: one past the highest, or, when that is `u32::MAX` (ids come from files, + /// pastes and agents), the lowest one no mask uses. pub fn next_mask_id(&self) -> u32 { - self.masks.iter().map(|m| m.id).max().map_or(1, |m| m + 1) + let used = |id: u32| self.masks.iter().any(|m| m.id == id); + match self.masks.iter().map(|m| m.id).max() { + None => 1, + // fewer masks than ids: one of the first `masks.len() + 1` is free + Some(max) => max.checked_add(1).or_else(|| (1..=u32::MAX).find(|&id| !used(id))).unwrap_or(0), + } } } @@ -363,4 +369,16 @@ mod tests { s.reset_section(Section::Detail); assert_eq!(s.enhance.denoise_on, None); } + + #[test] + fn next_mask_id_after_the_highest_id_is_unused() { + // ids come from files, pastes and agents: one at the limit must not overflow (a panic in + // debug builds) or wrap to an id another mask has + let mut s = DevelopSettings::default(); + assert_eq!(s.next_mask_id(), 1); + s.masks = vec![Mask { id: u32::MAX, ..Default::default() }, Mask { id: 1, ..Default::default() }]; + let next = s.next_mask_id(); + assert!(!s.masks.iter().any(|m| m.id == next), "{next}"); + assert_eq!(next, 2); + } } diff --git a/crates/develop/src/presets.rs b/crates/develop/src/presets.rs index b3edc801c..d5a658f51 100644 --- a/crates/develop/src/presets.rs +++ b/crates/develop/src/presets.rs @@ -220,14 +220,12 @@ impl Preset { o.remove("masks"); } let mut out = apply_partial(s, &rest, amount); - let mut next = out.masks.iter().map(|m| m.id).max().unwrap_or(0); for m in masks { let Ok(mut m) = serde_json::from_value::(m.clone()) else { continue }; if out.masks.iter().any(|q| q.name == m.name && q.components == m.components) { continue; } - next += 1; - m.id = next; + m.id = out.next_mask_id(); m.adjust.amount = (m.adjust.amount * amount).clamp(0.0, 200.0); out.masks.push(m); } @@ -293,4 +291,23 @@ mod tests { deep_merge(&mut a, &json!({"x": {"b": 3}, "y": [2, 3]})); assert_eq!(a, json!({"x": {"a": 1, "b": 3}, "y": [2, 3]})); } + + #[test] + fn preset_masks_get_unused_ids_after_a_mask_id_at_the_limit() { + let s = DevelopSettings { masks: vec![crate::Mask { id: u32::MAX, name: "Theirs".into(), ..Default::default() }], ..Default::default() }; + let p = Preset { + id: "p".into(), + name: "P".into(), + group: "User".into(), + settings: json!({"masks": [{"id": 7, "name": "A"}, {"id": 7, "name": "B"}]}), + favorite: false, + builtin: false, + }; + let out = p.apply(&s, 1.0); + let mut ids: Vec = out.masks.iter().map(|m| m.id).collect(); + assert_eq!(ids.len(), 3); + ids.sort_unstable(); + ids.dedup(); + assert_eq!(ids.len(), 3, "{:?}", out.masks); + } } diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index ae03859d2..db77c3cba 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -412,6 +412,17 @@ fn connected_brush_strokes_start_where_the_last_one_ended() { assert_eq!(strokes(&s).len(), 2); } +#[test] +fn a_mask_id_at_the_limit_does_not_overflow_the_next_masks() { + let mut s = demo(); + s.execute("develop.merge", &json!({"settings": {"masks": [{"id": u32::MAX, "name": "Pasted"}]}})).unwrap(); + s.execute("mask.add", &json!({"kind": "radial"})).unwrap(); + s.execute("mask.duplicate", &json!({})).unwrap(); + let ids: Vec = active_dev(&s).masks.iter().map(|m| m.id).collect(); + assert_eq!(ids.len(), 3); + assert!(ids.iter().enumerate().all(|(i, id)| !ids[..i].contains(id)), "distinct ids: {ids:?}"); +} + #[test] fn brush_strokes_keep_pen_pressure_and_refuse_bad_lists() { let mut s = demo(); diff --git a/docs/whats-new.md b/docs/whats-new.md index c6c259200..934c3c47f 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -1,5 +1,12 @@ # What's new in LightCraft +## 2026-10-30 + +### Reliability +- A photo with a mask id at the 32-bit limit (from a damaged or hand-edited sidecar, a paste or an agent) no longer + breaks the masks added after it: new masks, duplicates and the masks of a preset each get an id no other mask has. + Before, the next id wrapped around to 0, so a second new mask shared the first one's id (debug builds crashed). + ## 2026-10-24 ### Relative-date filters and smart albums follow the clock From 5bcdc439f60867e914bcbc242c8578bc3c14f52c Mon Sep 17 00:00:00 2001 From: Bruno-Alumn <171815746+Bruno-Alumn@users.noreply.github.com> Date: Fri, 30 Oct 2026 15:21:57 +0100 Subject: [PATCH 2/2] Build the used mask ids once in next_mask_id's fallback Scanning every mask for every candidate id made a dense run of ids quadratic. Collect them into a set first. --- crates/develop/src/lib.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/crates/develop/src/lib.rs b/crates/develop/src/lib.rs index dd059c117..f2d1e63bf 100644 --- a/crates/develop/src/lib.rs +++ b/crates/develop/src/lib.rs @@ -164,11 +164,16 @@ impl DevelopSettings { /// Next free mask id: one past the highest, or, when that is `u32::MAX` (ids come from files, /// pastes and agents), the lowest one no mask uses. pub fn next_mask_id(&self) -> u32 { - let used = |id: u32| self.masks.iter().any(|m| m.id == id); match self.masks.iter().map(|m| m.id).max() { None => 1, // fewer masks than ids: one of the first `masks.len() + 1` is free - Some(max) => max.checked_add(1).or_else(|| (1..=u32::MAX).find(|&id| !used(id))).unwrap_or(0), + Some(max) => max + .checked_add(1) + .or_else(|| { + let used: std::collections::HashSet = self.masks.iter().map(|m| m.id).collect(); + (1..=u32::MAX).find(|id| !used.contains(id)) + }) + .unwrap_or(0), } } }