From 5e5a0f6d2db11fdc62b850894abc6e96d945c110 Mon Sep 17 00:00:00 2001 From: Timothy Daly <100327160+timothydaly1@users.noreply.github.com> Date: Sun, 11 Oct 2026 17:43:30 -0400 Subject: [PATCH] Layers: clip groups follow New Layer, Arrange, Reorder and Delete (#2203) (#3090) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adding, moving or deleting layers never touched Layer::clipped, so a new layer above a clipping base split its group and became the base of the layers clipped above it, and moving or deleting a base left its clipped layers clipping to whatever ended up below them. The new clip_group module applies Photoshop's rules, measured on #2203: a layer added or moved joins the clip group it lands in; a clipped layer moved stays clipped while it still sits on a clip group; the layers clipped to a base that moves away or is deleted are released; and a clipped run left with nothing below it is released. They are applied by layer.new.layer (new clipToPrevious param, Photoshop's Use Previous Layer to Create Clipping Mask), layer.arrange.*, layer.moveTo (Layers panel drags, batches and ⌥-drag copies) and layer.delete. Co-authored-by: Claude Opus 5.5 --- crates/engine/src/clip_group.rs | 282 ++++++++++++++++++++++++++ crates/engine/src/commands.rs | 39 +++- crates/engine/src/layer_multi_cmds.rs | 20 +- crates/engine/src/lib.rs | 1 + 4 files changed, 325 insertions(+), 17 deletions(-) create mode 100644 crates/engine/src/clip_group.rs diff --git a/crates/engine/src/clip_group.rs b/crates/engine/src/clip_group.rs new file mode 100644 index 000000000..4e356efce --- /dev/null +++ b/crates/engine/src/clip_group.rs @@ -0,0 +1,282 @@ +//! Clip groups follow layers as they are added, moved and deleted, as in Photoshop (#2203). +//! +//! A clipped layer clips to the nearest unclipped layer below it (its base); a base and the run of +//! clipped layers directly above it form a clip group. Photoshop keeps those groups whole while +//! the stack changes, by these rules (measured in Photoshop 27.11, the table on #2203): +//! +//! 1. A layer added or moved joins the clip group it lands in: it is clipped when the layer +//! directly above its new slot is clipped (unless that layer is one of its own group travelling +//! with it). +//! 2. A clipped layer moved stays clipped while it still sits on a clip group: the layer below its +//! new slot is clipped, or was in its group (`B, C1*, C2*` + Bring Forward on C1 → `B, C2*, C1*`). +//! Otherwise it is released (`B, C*, U` + Bring Forward on C → `B, U, C`). +//! 3. When a base is moved away or deleted, the layers that were clipped to it are released. Rule +//! 1 reads the stack before this release, so a base moved up into its own group joins it while +//! the layers it left are released (`B, C1*, C2*` + Bring Forward on B → `C1, B*, C2`). +//! 4. A clipped layer left with nothing below it to clip to is released. + +use photocraft_doc::{Document, Layer, LayerId}; + +/// The sibling list holding the layer at `path`, and the layer's index in it. +fn siblings<'a>(doc: &'a Document, path: &[usize]) -> Option<(&'a [Layer], usize)> { + let (&idx, parent) = path.split_last()?; + let list = if parent.is_empty() { &doc.layers[..] } else { doc.layer_at(parent)?.children()? }; + Some((list, idx)) +} + +/// The layer `delta` slots above (+) or below (−) `id` among its siblings. +fn neighbour(doc: &Document, id: LayerId, delta: isize) -> Option<&Layer> { + let path = doc.path_of(id)?; + let (list, idx) = siblings(doc, &path)?; + list.get(idx.checked_add_signed(delta)?) +} + +/// The clip group `id` belongs to, base first, when it has one (a clipped layer, or a base with +/// layers clipped to it). +fn group_of(doc: &Document, id: LayerId) -> Vec { + let Some(path) = doc.path_of(id) else { return Vec::new() }; + let Some((list, idx)) = siblings(doc, &path) else { return Vec::new() }; + let base = list.iter().take(idx.saturating_add(1)).rposition(|l| !l.clipped); + let Some(base) = base else { return Vec::new() }; + let group: Vec = list.iter().skip(base).enumerate().take_while(|(i, l)| *i == 0 || l.clipped).map(|(_, l)| l.id).collect(); + if group.len() > 1 { group } else { Vec::new() } +} + +/// The layers clipped to `id`: the clipped run directly above it. Empty when `id` is itself +/// clipped (it is not a base) or nothing is clipped to it. +fn clipped_to(doc: &Document, id: LayerId) -> Vec { + match group_of(doc, id).split_first() { + Some((&base, rest)) if base == id => rest.to_vec(), + _ => Vec::new(), + } +} + +/// The clip groups of layers about to move or be deleted, read before they leave their slots. +#[derive(Default)] +pub(crate) struct Before { + moving: Vec, + /// Each moving layer that was clipped. + clipped: Vec, + /// Each moving layer's clip group (base first), when it had one. + groups: Vec<(LayerId, Vec)>, + /// The layers that lose their base: those clipped to a moving base, except the moving ones. + left: Vec, +} + +impl Before { + /// Read the clip groups of `moving` (moved or deleted together). + pub(crate) fn read(doc: &Document, moving: &[LayerId]) -> Self { + let clipped = moving.iter().copied().filter(|&id| doc.layer(id).is_some_and(|l| l.clipped)).collect(); + let groups = moving.iter().map(|&id| (id, group_of(doc, id))).filter(|(_, g)| !g.is_empty()).collect(); + let mut left: Vec = moving.iter().flat_map(|&id| clipped_to(doc, id)).filter(|id| !moving.contains(id)).collect(); + left.dedup(); + Self { moving: moving.to_vec(), clipped, groups, left } + } + + fn group(&self, id: LayerId) -> &[LayerId] { + self.groups.iter().find(|(m, _)| *m == id).map_or(&[], |(_, g)| g) + } +} + +fn set_clipped(doc: &mut Document, id: LayerId, clipped: bool) { + if let Some(l) = doc.layer_mut(id) { + l.clipped = clipped; + } +} + +/// After `placed` were added or moved (`before` read before they moved; default for new layers): +/// each placed layer joins or leaves clip groups by rules 1–2, topmost first so a run of placed +/// layers joins together, or is clipped anyway when `force` (New Layer's Use Previous Layer to +/// Create Clipping Mask). Then the layers that lost their base are released (rule 3), and any +/// clipped layer with nothing below it (rule 4). +pub(crate) fn settle(doc: &mut Document, placed: &[LayerId], before: &Before, force: bool) { + let mut order: Vec<(Vec, LayerId)> = placed.iter().filter_map(|&id| Some((doc.path_of(id)?, id))).collect(); + // Paths sort bottom to top within a sibling list: walk them from the top. + order.sort(); + for (_, id) in order.into_iter().rev() { + let group = before.group(id); + let travelling = |n: LayerId| before.moving.contains(&n) && group.contains(&n); + let joins = neighbour(doc, id, 1).is_some_and(|a| a.clipped && !travelling(a.id)); + let stays = before.clipped.contains(&id) && neighbour(doc, id, -1).is_some_and(|b| b.clipped || group.contains(&b.id)); + set_clipped(doc, id, force || joins || stays); + } + for &id in &before.left { + if !placed.contains(&id) { + set_clipped(doc, id, false); + } + } + release_orphans(doc); +} + +/// Rule 4: release the clipped layers at the bottom of a sibling list (nothing below to clip to). +/// The compositor already draws such a run unclipped, so this changes no pixels. Every list is +/// visited once from an explicit stack, so a deep document can't recurse without bound. +pub(crate) fn release_orphans(doc: &mut Document) { + let mut stack: Vec<&mut Vec> = vec![&mut doc.layers]; + while let Some(list) = stack.pop() { + for l in list.iter_mut().take_while(|l| l.clipped) { + l.clipped = false; + } + for l in list { + if let Some(ch) = l.children_mut() { + stack.push(ch); + } + } + } +} + +#[cfg(test)] +mod tests { + use serde_json::{Value, json}; + + use crate::Session; + + /// A document holding `spec` (bottom to top, `*` = clipped) above its Background. + fn stack(spec: &str) -> Session { + let mut s = Session::new(); + s.execute("file.new", json!({"width": 16, "height": 16})).unwrap(); + for name in spec.split_whitespace() { + let id = s.execute("layer.new.layer", json!({"name": name.trim_end_matches('*')})).unwrap()["layer"].clone(); + if name.ends_with('*') { + s.execute("layer.setProps", json!({"layer": id, "clipped": true})).unwrap(); + } + } + assert_eq!(shown(&s), spec); + s + } + + /// The stack above the Background, as [`stack`] writes it. + fn shown(s: &Session) -> String { + let doc = &s.active().unwrap().doc; + let names = doc.layers.iter().skip(1).map(|l| format!("{}{}", l.name, if l.clipped { "*" } else { "" })); + names.collect::>().join(" ") + } + + fn id(s: &Session, name: &str) -> Value { + let doc = &s.active().unwrap().doc; + json!(doc.walk().into_iter().find(|(_, _, l)| l.name == name).unwrap().2.id.0) + } + + fn run(s: &mut Session, cmd: &str, p: Value) { + s.execute(cmd, p).unwrap_or_else(|e| panic!("{cmd}: {e}")); + } + + fn select(s: &mut Session, name: &str) { + let l = id(s, name); + run(s, "layer.select", json!({"layer": l})); + } + + /// New Layer joins the clip group it lands in (#2203 rows 1–6). + #[test] + fn new_layer_joins_the_group_it_lands_in() { + for (spec, active, want) in [("B C*", "B", "B N* C*"), ("B C*", "C", "B C* N"), ("B C1* C2*", "C1", "B C1* N* C2*"), ("B U", "B", "B N U")] { + let mut s = stack(spec); + select(&mut s, active); + run(&mut s, "layer.new.layer", json!({"name": "N"})); + assert_eq!(shown(&s), want, "{spec}, {active} active"); + } + // Use Previous Layer to Create Clipping Mask. + let mut s = stack("B"); + run(&mut s, "layer.new.layer", json!({"name": "N", "clipToPrevious": true})); + assert_eq!(shown(&s), "B N*"); + // The report: three new layers with the base active all join its group. + let mut s = stack("tpl Cont*"); + for n in ["L1", "L2", "L3"] { + select(&mut s, "tpl"); + run(&mut s, "layer.new.layer", json!({"name": n})); + } + assert_eq!(shown(&s), "tpl L3* L2* L1* Cont*"); + } + + /// Bring Forward / Send Backward (rows 7–12). + #[test] + fn arrange_joins_and_leaves_groups() { + for (spec, layer, cmd, times, want) in [ + ("B C1* C2* U", "U", "sendBackward", 1, "B C1* U* C2*"), + ("B C1* C2* U", "U", "sendBackward", 2, "B U* C1* C2*"), + ("B C* U", "C", "bringForward", 1, "B U C"), + ("B C1* C2*", "C1", "bringForward", 1, "B C2* C1*"), + // Photoshop also moves C below X here (`C X B`); the release is the same. + ("X B C*", "C", "sendBackward", 1, "X C B"), + // The base moves up into its own group: it joins it, and the layers it left go. + ("B C1* C2*", "B", "bringForward", 1, "C1 B* C2"), + ("B C1* C2* U", "U", "sendToBack", 1, "U B C1* C2*"), + ("B C1* C2* U", "C1", "bringToFront", 1, "B C2* U C1"), + ] { + let mut s = stack(spec); + for _ in 0..times { + let l = id(&s, layer); + run(&mut s, &format!("layer.arrange.{cmd}"), json!({"layer": l})); + } + assert_eq!(shown(&s), want, "{spec}: {cmd} {layer} ×{times}"); + } + } + + /// Layers panel drags (rows 13–16), several layers at once, and ⌥-drag copies. + #[test] + fn reorder_joins_and_leaves_groups() { + for (spec, layers, target, pos, want) in [ + ("B C1* C2* U", vec!["U"], "C1", "above", "B C1* U* C2*"), + ("U B C1* C2*", vec!["U"], "C1", "above", "B C1* U* C2*"), + ("B C1* C2* U", vec!["C1"], "U", "above", "B C2* U C1"), + ("X B C1* C2*", vec!["C2"], "B", "below", "X C2 B C1*"), + ("B C1* C2* U V", vec!["U", "V"], "C1", "above", "B C1* U* V* C2*"), + ("X B C1* C2* U", vec!["B", "C1"], "U", "above", "X C2 U B C1*"), + ] { + let mut s = stack(spec); + let ids: Vec = layers.iter().map(|n| id(&s, n)).collect(); + let t = id(&s, target); + run(&mut s, "layer.moveTo", json!({"layers": ids, "target": t, "position": pos})); + assert_eq!(shown(&s), want, "{spec}: {layers:?} {pos} {target}"); + } + let mut s = stack("B C1* C2* U"); + let (u, c1) = (id(&s, "U"), id(&s, "C1")); + run(&mut s, "layer.moveTo", json!({"layer": u, "target": c1, "position": "above", "copy": true})); + assert_eq!(shown(&s), "B C1* U copy* C2* U"); + } + + /// Deleting a base releases its group (row 17); deleting a clipped layer keeps the rest. + #[test] + fn delete_releases_the_group_of_a_deleted_base() { + let mut s = stack("X B C1* C2*"); + select(&mut s, "B"); + run(&mut s, "layer.delete", json!({})); + assert_eq!(shown(&s), "X C1 C2"); + let mut s = stack("X B C1* C2*"); + let c1 = id(&s, "C1"); + run(&mut s, "layer.delete", json!({"layer": c1})); + assert_eq!(shown(&s), "X B C2*"); + // Several selected: the base and one of its layers. + let mut s = stack("X B C1* C2*"); + select(&mut s, "B"); + let c1 = id(&s, "C1"); + run(&mut s, "layer.select", json!({"layer": c1, "mode": "toggle"})); + run(&mut s, "layer.delete", json!({})); + assert_eq!(shown(&s), "X C2"); + // Each change is one history step. + run(&mut s, "edit.undo", json!({})); + assert_eq!(shown(&s), "X B C1* C2*"); + } + + /// A clipped run left at the bottom of a group has nothing to clip to: it is released (the + /// compositor already draws it unclipped), wherever the edit happened. + #[test] + fn orphaned_clipped_layers_are_released() { + let mut s = stack("A B U"); + let (a, b, u) = (id(&s, "A"), id(&s, "B"), id(&s, "U")); + run(&mut s, "layer.select", json!({"layer": a})); + run(&mut s, "layer.select", json!({"layer": b, "mode": "toggle"})); + run(&mut s, "layer.groupLayers", json!({"name": "G"})); + s.edit("orphans", |doc, _| { + for l in doc.layers.iter_mut().flat_map(|g| g.children_mut().into_iter().flatten()) { + l.clipped = true; + } + Ok(()) + }) + .unwrap(); + run(&mut s, "layer.arrange.sendBackward", json!({"layer": u})); + let doc = &s.active().unwrap().doc; + let g = doc.walk().into_iter().find(|(_, _, l)| l.name == "G").unwrap().2; + assert!(g.children().unwrap().iter().all(|l| !l.clipped)); + } +} diff --git a/crates/engine/src/commands.rs b/crates/engine/src/commands.rs index 27797a61c..f9734b57a 100644 --- a/crates/engine/src/commands.rs +++ b/crates/engine/src/commands.rs @@ -520,16 +520,26 @@ fn build() -> Vec { } ), // Layer - cmd!("layer.new.layer", "Layer…", ["Layer", "New"], Some("Cmd+Shift+N"), r##"{"name":str?}"##, has_doc, |s, p| { - let id = s.edit("New Layer", |doc, active| { - let name = p.get("name").and_then(Value::as_str).map(str::to_string).unwrap_or_else(|| doc.next_layer_name("Layer")); - let id = doc.insert_above(*active, Layer::raster(name, doc.pixel_format())); - *active = Some(id); - Ok(id) - })?; - crate::layer_multi_cmds::note_inert_insert(s, id); - Ok(json!({ "layer": id.0 })) - }), + cmd!( + "layer.new.layer", + "Layer…", + ["Layer", "New"], + Some("Cmd+Shift+N"), + r##"{"name":str?,"clipToPrevious":bool=false (Use Previous Layer to Create Clipping Mask; inside a clip group the layer joins it anyway)}"##, + has_doc, + |s, p| { + let clip = p.get("clipToPrevious").and_then(Value::as_bool).unwrap_or(false); + let id = s.edit("New Layer", |doc, active| { + let name = p.get("name").and_then(Value::as_str).map(str::to_string).unwrap_or_else(|| doc.next_layer_name("Layer")); + let id = doc.insert_above(*active, Layer::raster(name, doc.pixel_format())); + crate::clip_group::settle(doc, &[id], &Default::default(), clip); + *active = Some(id); + Ok(id) + })?; + crate::layer_multi_cmds::note_inert_insert(s, id); + Ok(json!({ "layer": id.0 })) + } + ), cmd!("layer.new.group", "Group…", ["Layer", "New"], None, r##"{"name":str?}"##, has_doc, |s, p| { let id = s.edit("New Group", |doc, active| { let name = p.get("name").and_then(Value::as_str).map(str::to_string).unwrap_or_else(|| doc.next_layer_name("Group")); @@ -583,7 +593,9 @@ fn build() -> Vec { let id = layer_param(s, p)?; s.edit("Delete Layer", |doc, active| { let neighbours = crate::layer_multi_cmds::deletion_neighbours(doc, id); + let before = crate::clip_group::Before::read(doc, &[id]); doc.remove(id).ok_or(EngineError::NoLayer(id))?; + crate::clip_group::settle(doc, &[], &before, false); if doc.layers.is_empty() { return Err(EngineError::Other("a document must keep at least one layer".into())); } @@ -1291,6 +1303,7 @@ fn arrange(s: &mut Session, p: &Value, delta: i32) -> Result { d => d, }; let dir = steps.signum(); + let before = crate::clip_group::Before::read(doc, &[id]); let mut moved = false; for _ in 0..steps.abs() { if !doc.shift(id, dir) { @@ -1302,7 +1315,11 @@ fn arrange(s: &mut Session, p: &Value, delta: i32) -> Result { } moved = true; } - if moved { Ok(()) } else { Err(EngineError::Other("layer can't move further".into())) } + if !moved { + return Err(EngineError::Other("layer can't move further".into())); + } + crate::clip_group::settle(doc, &[id], &before, false); + Ok(()) })?; Ok(Value::Null) } diff --git a/crates/engine/src/layer_multi_cmds.rs b/crates/engine/src/layer_multi_cmds.rs index 384f2deea..0149f17f2 100644 --- a/crates/engine/src/layer_multi_cmds.rs +++ b/crates/engine/src/layer_multi_cmds.rs @@ -1003,6 +1003,7 @@ pub fn move_to(s: &mut Session, p: &Value) -> Result { let original_active = st.active_layer; let background = crate::background_cmds::background_id(doc); let (count, copies) = s.edit(if copy { "Duplicate Layer" } else { "Reorder Layers" }, |doc, active| { + let before = if copy { Default::default() } else { crate::clip_group::Before::read(doc, &moved_ids) }; let mut moved = Vec::with_capacity(moved_ids.len()); for &id in &moved_ids { moved.push(if copy { crate::commands::layer_copy(doc, id)? } else { doc.remove(id).ok_or(EngineError::NoLayer(id))? }); @@ -1025,6 +1026,7 @@ pub fn move_to(s: &mut Session, p: &Value) -> Result { } check_group_depth(doc, "Reorder Layer")?; crate::background_cmds::keep_background_at_bottom(background, doc)?; + crate::clip_group::settle(doc, if copy { &copies } else { &moved_ids }, &before, false); *active = if copy { copies.last().copied() } else if batch.is_some() { @@ -1245,9 +1247,11 @@ pub fn delete_selected(s: &mut Session) -> Result { s.edit("Delete Layers", |doc, active| { let ids = top_level(doc, &sel); let neighbours = active.map(|id| deletion_neighbours(doc, id)).unwrap_or_default(); + let before = crate::clip_group::Before::read(doc, &ids); for id in &ids { doc.remove(*id).ok_or(EngineError::NoLayer(*id))?; } + crate::clip_group::settle(doc, &[], &before, false); if doc.layers.is_empty() { return Err(EngineError::Other("a document must keep at least one layer".into())); } @@ -1513,16 +1517,20 @@ mod tests { }) .unwrap(); s.execute("layer.setProps", json!({"layer": clipped.0, "clipped": true})).unwrap(); - // A new layer between a clipping base and its clipped layer becomes their base. + // New Layer between a clipping base and its clipped layer joins their clip group (#2203), + // so it draws nothing; an empty layer there that stayed unclipped would become their base. s.execute("layer.select", json!({"layer": base.0})).unwrap(); let shown = photocraft_compose::render(doc(&s), doc(&s).bounds()); + let before = doc(&s).clone(); s.execute("layer.new.layer", json!({})).unwrap(); - assert_eq!(damage(&s), None); - assert_ne!(photocraft_compose::render(doc(&s), doc(&s).bounds()), shown); + assert_eq!(damage(&s), Some(Rect::EMPTY)); + assert_eq!(photocraft_compose::render(doc(&s), doc(&s).bounds()), shown); + let mut split = before.clone(); + split.insert_above(Some(base), Layer::raster("split", split.pixel_format())); + assert_ne!(photocraft_compose::render(&split, split.bounds()), shown); + assert_eq!(step_damage(&before, &split), None); + assert_eq!(step_damage(&split, &before), None); s.execute("edit.undo", json!({})).unwrap(); - assert_eq!(damage(&s), None); - s.execute("edit.redo", json!({})).unwrap(); - assert_eq!(damage(&s), None); // A step that adds a layer with pixels (Layer via Copy, paste…) is not inert either. let before = doc(&s).clone(); let mut after = before.clone(); diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 401199f5f..a7d1e6226 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -23,6 +23,7 @@ pub mod build_info; mod canvas_geom; mod channel_clip; pub mod channel_cmds; +mod clip_group; pub mod color_cmds; pub mod color_to_alpha_cmds; pub mod commands;