Layers: a new empty layer no longer recomposites every layer (#1846)
step_damage pairs layers by id and steps over a layer only one side has when it draws nothing (an empty pixel layer or group in Normal/Pass Through, no masks, effects or Advanced Blending, not a clipping base), so New Layer, New Group and their undo/redo report no damage. Adds the P51 perf scenario (new layer in the 150-layer document). Fixes #1771. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
David Mann
GitHub
parent
21649f6030
commit
d7078f3623
|
|
@ -514,6 +514,7 @@ fn build() -> Vec<CommandSpec> {
|
|||
*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| {
|
||||
|
|
@ -523,6 +524,7 @@ fn build() -> Vec<CommandSpec> {
|
|||
*active = Some(id);
|
||||
Ok(id)
|
||||
})?;
|
||||
crate::layer_multi_cmds::note_inert_insert(s, id);
|
||||
Ok(json!({ "layer": id.0 }))
|
||||
}),
|
||||
cmd!(
|
||||
|
|
|
|||
|
|
@ -574,7 +574,8 @@ fn doc_pixels_equal(a: &Document, b: &Document) -> bool {
|
|||
/// `None` (composite everything, as before) when the step touched anything the layer walk
|
||||
/// cannot bound: document-level fields the compositor reads (canvas size, mode, depth,
|
||||
/// profile, global light, patterns, channels, palettes), the layer structure (order,
|
||||
/// additions, removals, kind changes), layer properties that reach outside their own bounds
|
||||
/// additions, removals, kind changes; adding or removing a layer that draws nothing, see
|
||||
/// [`inert_at`], is no change), layer properties that reach outside their own bounds
|
||||
/// (clipping, excluded channels — what `layer.setProps` already reports as unbounded), and
|
||||
/// video layers.
|
||||
pub fn step_damage(a: &Document, b: &Document) -> Option<Rect> {
|
||||
|
|
@ -582,11 +583,26 @@ pub fn step_damage(a: &Document, b: &Document) -> Option<Rect> {
|
|||
return None;
|
||||
}
|
||||
fn walk(xs: &[Layer], ys: &[Layer], a: &Document, b: &Document, out: &mut Rect) -> bool {
|
||||
if xs.len() != ys.len() {
|
||||
return false;
|
||||
}
|
||||
for (x, y) in xs.iter().zip(ys) {
|
||||
if x.id != y.id || x.video.is_some() || y.video.is_some() {
|
||||
// Pair the layers by id, stepping over layers only one side has when they draw nothing
|
||||
// (a new empty layer or group, #1771): adding or deleting one changes no pixels.
|
||||
let (mut i, mut j) = (0, 0);
|
||||
loop {
|
||||
let (x, y) = match (xs.get(i), ys.get(j)) {
|
||||
(None, None) => return true,
|
||||
(Some(x), Some(y)) if x.id == y.id => (x, y),
|
||||
_ if inert_at(ys, j) => {
|
||||
j += 1;
|
||||
continue;
|
||||
}
|
||||
_ if inert_at(xs, i) => {
|
||||
i += 1;
|
||||
continue;
|
||||
}
|
||||
_ => return false,
|
||||
};
|
||||
i += 1;
|
||||
j += 1;
|
||||
if x.video.is_some() || y.video.is_some() {
|
||||
return false;
|
||||
}
|
||||
// Clipping and channel exclusion change how *other* layers render, outside this
|
||||
|
|
@ -611,12 +627,46 @@ pub fn step_damage(a: &Document, b: &Document) -> Option<Rect> {
|
|||
_ => return false,
|
||||
}
|
||||
}
|
||||
true
|
||||
}
|
||||
let mut out = Rect::EMPTY;
|
||||
if walk(&a.layers, &b.layers, a, b, &mut out) { Some(out) } else { None }
|
||||
}
|
||||
|
||||
/// Whether `layers[k]` draws nothing wherever it sits, so adding or removing it leaves the
|
||||
/// composite as it was: an empty pixel layer (no tiles, transparent default) or an empty group,
|
||||
/// in a Normal or Pass Through blend with no mask, vector mask, effects or Advanced Blending.
|
||||
/// A clipping base that would gain or lose the clipped layers above it is not inert.
|
||||
fn inert_at(layers: &[Layer], k: usize) -> bool {
|
||||
let Some(l) = layers.get(k) else { return false };
|
||||
let empty = match &l.content {
|
||||
LayerContent::Raster(s) => s.tile_count() == 0 && s.format().alpha && s.default_bytes().iter().all(|b| *b == 0),
|
||||
LayerContent::Group(g) => g.children.is_empty() && g.artboard.is_none(),
|
||||
_ => false,
|
||||
};
|
||||
empty
|
||||
&& matches!(l.blend, photocraft_doc::BlendMode::Normal | photocraft_doc::BlendMode::PassThrough)
|
||||
&& l.mask.is_none()
|
||||
&& l.vector_mask.is_none()
|
||||
&& !photocraft_compose::effects::has_effects(l)
|
||||
&& l.fill_cache.is_none()
|
||||
&& l.video.is_none()
|
||||
&& l.advanced == photocraft_doc::AdvancedBlending::default()
|
||||
&& (l.clipped || layers.get(k + 1).is_none_or(|above| !above.clipped))
|
||||
}
|
||||
|
||||
/// Report that adding layer `id` to the active document changed no pixels when it draws
|
||||
/// nothing ([`inert_at`]), so the canvas doesn't recomposite every layer for it (#1771). Looks
|
||||
/// only at the new layer and its neighbour, never at other layers' tiles.
|
||||
pub(crate) fn note_inert_insert(s: &mut Session, id: LayerId) {
|
||||
let Some(st) = s.active_mut() else { return };
|
||||
let Some(path) = st.doc.path_of(id) else { return };
|
||||
let Some((&k, parent)) = path.split_last() else { return };
|
||||
let siblings = if parent.is_empty() { Some(st.doc.layers.as_slice()) } else { st.doc.layer_at(parent).and_then(Layer::children) };
|
||||
if siblings.is_some_and(|sib| inert_at(sib, k)) {
|
||||
st.last_damage = Some(Rect::EMPTY);
|
||||
}
|
||||
}
|
||||
|
||||
/// Content bounds used by Align/Distribute: the layer's pixels (type and shape layers use their
|
||||
/// rendered pixels), or the union of a group's children. Fill and adjustment layers have none.
|
||||
pub fn layer_bounds(l: &Layer) -> Option<Rect> {
|
||||
|
|
@ -1277,6 +1327,83 @@ mod tests {
|
|||
assert!(s.execute("layer.translate", json!({"layer": 999_999, "dx": 1})).is_err());
|
||||
}
|
||||
|
||||
/// #1771: a new empty layer or group changes no pixels, so it (and undoing or redoing it)
|
||||
/// must not make the canvas recomposite every layer of the document.
|
||||
#[test]
|
||||
fn new_empty_layers_and_groups_damage_nothing() {
|
||||
for depth in [8, 16, 32] {
|
||||
let mut s = session(depth);
|
||||
let a = rect_layer(&mut s, Rect::new(10, 10, 20, 20));
|
||||
let composite = |s: &Session| photocraft_compose::render(doc(s), doc(s).bounds());
|
||||
let shown = composite(&s);
|
||||
s.execute("layer.new.layer", json!({})).unwrap();
|
||||
assert_eq!(damage(&s), Some(Rect::EMPTY), "{depth}-bit new layer");
|
||||
assert_eq!(composite(&s), shown, "an empty layer changes no pixels");
|
||||
s.execute("edit.undo", json!({})).unwrap();
|
||||
assert_eq!(damage(&s), Some(Rect::EMPTY), "{depth}-bit undo New Layer");
|
||||
s.execute("edit.redo", json!({})).unwrap();
|
||||
assert_eq!(damage(&s), Some(Rect::EMPTY), "{depth}-bit redo New Layer");
|
||||
s.execute("layer.new.group", json!({})).unwrap();
|
||||
assert_eq!(damage(&s), Some(Rect::EMPTY), "{depth}-bit new group");
|
||||
// Inside a group too.
|
||||
let child = s.execute("layer.new.layer", json!({})).unwrap()["layer"].as_u64().unwrap();
|
||||
let g = s.execute("layer.groupLayers", json!({})).unwrap()["layer"].as_u64().unwrap();
|
||||
s.execute("layer.select", json!({"layer": child})).unwrap();
|
||||
s.execute("layer.new.layer", json!({})).unwrap();
|
||||
assert_eq!(damage(&s), Some(Rect::EMPTY), "{depth}-bit new layer in a group");
|
||||
assert!(doc(&s).layer(LayerId(g)).and_then(Layer::children).is_some_and(|c| c.len() == 2));
|
||||
s.execute("edit.undo", json!({})).unwrap();
|
||||
assert_eq!(damage(&s), Some(Rect::EMPTY));
|
||||
// Undoing a step that painted still reports that area, not nothing.
|
||||
s.execute("layer.select", json!({"layer": a.0})).unwrap();
|
||||
s.execute("layer.translate", json!({"dx": 5, "dy": 0})).unwrap();
|
||||
s.execute("edit.undo", json!({})).unwrap();
|
||||
assert_eq!(damage(&s), Some(Rect::new(10, 10, 25, 20)));
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn new_layers_that_change_pixels_still_recomposite_everything() {
|
||||
let mut s = session(8);
|
||||
let base = rect_layer(&mut s, Rect::new(10, 10, 20, 20));
|
||||
let clipped = rect_layer(&mut s, Rect::new(0, 0, 100, 80));
|
||||
s.edit("green", |doc, _| {
|
||||
doc.layer_mut(clipped).unwrap().surface_mut().unwrap().fill_rect(Rect::new(0, 0, 100, 80), &[0.0, 1.0, 0.0, 1.0]);
|
||||
Ok(())
|
||||
})
|
||||
.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.
|
||||
s.execute("layer.select", json!({"layer": base.0})).unwrap();
|
||||
let shown = photocraft_compose::render(doc(&s), doc(&s).bounds());
|
||||
s.execute("layer.new.layer", json!({})).unwrap();
|
||||
assert_eq!(damage(&s), None);
|
||||
assert_ne!(photocraft_compose::render(doc(&s), doc(&s).bounds()), shown);
|
||||
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();
|
||||
let mut l = Layer::raster("painted", after.pixel_format());
|
||||
l.surface_mut().unwrap().fill_rect(Rect::new(1, 1, 2, 2), &[0.0, 1.0, 0.0, 1.0]);
|
||||
after.insert_above(None, l);
|
||||
assert_eq!(step_damage(&before, &after), None);
|
||||
assert_eq!(step_damage(&after, &before), None);
|
||||
// Nor, conservatively, an empty layer in another blend mode.
|
||||
let mut styled = before.clone();
|
||||
let mut l = Layer::raster("styled", styled.pixel_format());
|
||||
l.blend = photocraft_doc::BlendMode::Multiply;
|
||||
styled.insert_above(None, l);
|
||||
assert_eq!(step_damage(&before, &styled), None);
|
||||
// An empty layer in Normal mode is.
|
||||
let mut plain = before.clone();
|
||||
plain.insert_above(None, Layer::raster("plain", plain.pixel_format()));
|
||||
assert_eq!(step_damage(&before, &plain), Some(Rect::EMPTY));
|
||||
assert_eq!(step_damage(&plain, &before), Some(Rect::EMPTY));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn the_move_preview_matches_the_move() {
|
||||
let mut s = session(8);
|
||||
|
|
|
|||
|
|
@ -669,7 +669,8 @@ fn translate_moves_pixels_and_respects_locks() {
|
|||
fn damage_is_reported_for_strokes_only() {
|
||||
let mut s = session_with_doc();
|
||||
s.execute("layer.new.layer", json!({})).unwrap();
|
||||
assert_eq!(s.active().unwrap().last_damage, None);
|
||||
// A new empty layer changes no pixels (#1771).
|
||||
assert_eq!(s.active().unwrap().last_damage, Some(photocraft_geom::Rect::EMPTY));
|
||||
s.execute("paint.stroke", json!({"points": [[10, 10], [20, 10]], "size": 4})).unwrap();
|
||||
let d = s.active().unwrap().last_damage.unwrap();
|
||||
assert!(d.contains(15, 10) && d.width() < 30);
|
||||
|
|
|
|||
|
|
@ -496,11 +496,12 @@ fn layered_scenarios(b: &mut Bench, sz: &Sizes) {
|
|||
});
|
||||
}
|
||||
|
||||
/// #209: arrow-key nudge in the 150-layer document.
|
||||
/// #209: arrow-key nudge in the 150-layer document; #1771: a new layer there.
|
||||
fn many_layer_scenarios(b: &mut Bench, sz: &Sizes) {
|
||||
// Row names stay stable across modes: the layer count is in the mode, not the key.
|
||||
let name = "nudge 1 px in the many-layer document + refresh".to_string();
|
||||
if !b.wanted(&name) {
|
||||
let new_layer = "new layer in the many-layer document + refresh";
|
||||
if !b.wanted(&name) && !b.wanted(new_layer) {
|
||||
return;
|
||||
}
|
||||
// The document's first refresh is part of this row (a crash there is reported against it).
|
||||
|
|
@ -520,6 +521,15 @@ fn many_layer_scenarios(b: &mut Bench, sz: &Sizes) {
|
|||
exec(s, "layer.translate", json!({"layer": mid, "dx": if i % 2 == 0 { 1 } else { -1 }, "dy": 0}))?;
|
||||
Ok(ms(t) + b.refresh(s, false)?)
|
||||
});
|
||||
// Layer › New › Layer: as fast at any layer count. Undone (untimed) so the count stays put.
|
||||
b.time(new_layer, &mut s, reps, true, |b, s, _| {
|
||||
let t = Instant::now();
|
||||
exec(s, "layer.new.layer", json!({}))?;
|
||||
let took = ms(t) + b.refresh(s, false)?;
|
||||
exec(s, "edit.undo", json!({}))?;
|
||||
b.refresh(s, false)?;
|
||||
Ok(took)
|
||||
});
|
||||
}
|
||||
|
||||
/// #209: edits on the 15000 × 10000 16-bit document and the 32-bit document with 20 layers.
|
||||
|
|
|
|||
|
|
@ -6,7 +6,7 @@ Generated by `cargo xtask scorecard` from `perf/budgets.toml`, `perf/baseline.js
|
|||
|
||||
| Area | Done | Partial | Missing | Key numbers |
|
||||
|---|---:|---:|---:|---|
|
||||
| [Performance](#performance) | 3 | 22 | 13 | 47 scenarios: 3 within budget, 22 over budget or failing, 9 tracked only, 6 not run, 7 not measurable yet (baseline 2026-10-05) |
|
||||
| [Performance](#performance) | 3 | 22 | 14 | 48 scenarios: 3 within budget, 22 over budget or failing, 9 tracked only, 7 not run, 7 not measurable yet (baseline 2026-10-05) |
|
||||
| [File compatibility](#file-compatibility) | 4 | 12 | 11 | io corpus: oracle ≥ 146/170, round trip ≥ 169/170; psd-tools corpus: oracle ≥ 236/309, round trip ≥ 307/309; photoshop oracles: oracle ≥ 133/256, round trip ≥ 258/256 |
|
||||
| [Tools](#tools) | 12 | 14 | 8 | |
|
||||
| [Workspace and UI](#workspace-and-ui) | 3 | 7 | 13 | settings that do nothing: 55 of 141 (target 0) |
|
||||
|
|
@ -36,6 +36,7 @@ Baseline: 2026-10-05, commit `f5d2bcb54a`, Apple M4 Pro (48 GB), GPU Apple M4 Pr
|
|||
| P10 | Paste a 12 MP layer (first) | ≤ 100 ms p95 | 1232 | 3136 | 3136 | 1.3 GB | 1.4 GB | over budget (target not met yet) | [#209](https://github.com/storytold/photocraft/issues/209) |
|
||||
| P11 | Paste a 12 MP layer (repeat) | ≤ 20 ms p95 | 445 | 1174 | 1174 | 958 MB | 1.4 GB | over budget (target not met yet) | [#209](https://github.com/storytold/photocraft/issues/209) |
|
||||
| P12 | Arrow-key nudge in a 150-layer document | ≤ 30 ms p95 | – | – | – | – | – | **missing**: panicked: wgpu error: Validation Error / Caused by: / In Queue::write_buffer / Buffer with 'pc_compose_uniforms' label is invalid | [#209](https://github.com/storytold/photocraft/issues/209) |
|
||||
| P51 | New layer in a 150-layer document | ≤ 16 ms p95 | – | – | – | – | – | not run | [#1771](https://github.com/storytold/photocraft/issues/1771) |
|
||||
| P13 | Brush dab on a 15000×10000 16-bit document | ≤ 80 ms p95 | 6.42 | 146 | 146 | 839 MB | 2.0 GB | over budget (target not met yet) | [#209](https://github.com/storytold/photocraft/issues/209) |
|
||||
| P14 | Move a layer on a 15000×10000 16-bit document | ≤ 80 ms p95 | 2698 | 4318 | 4318 | 1.5 GB | 2.0 GB | over budget (target not met yet) | [#209](https://github.com/storytold/photocraft/issues/209) |
|
||||
| P16 | Brush dab on a 32-bit document with 20 layers | ≤ 100 ms p95 | 21.5 | 161 | 161 | 793 MB | 1.6 GB | over budget (target not met yet) | [#209](https://github.com/storytold/photocraft/issues/209) |
|
||||
|
|
|
|||
|
|
@ -172,6 +172,15 @@ row = "nudge 1 px in the many-layer document + refresh"
|
|||
budget_ms = 30
|
||||
enforce = false # over budget in the 2026-10-05 baseline
|
||||
|
||||
[[scenario]]
|
||||
id = "P51"
|
||||
title = "New layer in a 150-layer document"
|
||||
issue = 1771
|
||||
bench = "perf_scenarios"
|
||||
row = "new layer in the many-layer document + refresh"
|
||||
budget_ms = 16
|
||||
enforce = false # not in the baseline yet
|
||||
|
||||
[[scenario]]
|
||||
id = "P13"
|
||||
title = "Brush dab on a 15000×10000 16-bit document"
|
||||
|
|
|
|||
Loading…
Reference in New Issue