diff --git a/crates/engine/src/cmd/analysis_cmds.rs b/crates/engine/src/cmd/analysis_cmds.rs index ee5ca0fde2d..ee51e9e2d06 100644 --- a/crates/engine/src/cmd/analysis_cmds.rs +++ b/crates/engine/src/cmd/analysis_cmds.rs @@ -266,7 +266,7 @@ pub(crate) fn compound(s: &mut Session, label: &str, f: impl FnOnce(&mut Sess Ok(v) } Err(e) => { - st.doc = before; + let failed = std::mem::replace(&mut st.doc, before); st.active_layer = prior.active; st.selected_layers = prior.selected; st.layer_anchor = anchor; @@ -279,7 +279,9 @@ pub(crate) fn compound(s: &mut Session, label: &str, f: impl FnOnce(&mut Sess if was_clean { st.saved_revision = st.revision; } - st.last_damage = None; + // This revision's damage; `Session::dispatch` reports the command's whole span (nothing, + // when the inner commands had changed pixels and the restore took them back). + st.last_damage = crate::layer_multi_cmds::step_damage(&failed, &st.doc); Err(e) } }; diff --git a/crates/engine/src/cmd/extra_cmds.rs b/crates/engine/src/cmd/extra_cmds.rs index 275e3446479..a4b0b8777f0 100644 --- a/crates/engine/src/cmd/extra_cmds.rs +++ b/crates/engine/src/cmd/extra_cmds.rs @@ -377,7 +377,6 @@ fn paste_into(s: &mut Session, p: &Value, outside: bool) -> Result { let key = step_key(s, "pasteInto"); params["coalesce"] = json!(key); params["target"] = json!("pixels"); - let before = s.active().ok_or(EngineError::NoDocument)?.doc.clone(); let r = s.execute("edit.paste", params)?; let id = layer_param(s, &Value::Null)?; s.execute("layer.layerMask.revealAll", json!({"layer": id.0, "coalesce": key}))?; @@ -388,9 +387,9 @@ fn paste_into(s: &mut Session, p: &Value, outside: bool) -> Result { Ok(()) }); s.coalesce_request = None; + // Three revisions: `Session::dispatch` reports what the whole command changed, so only where + // the masked paste draws is recomposited (#2901). r2?; - // Only where the masked paste draws needs recompositing (#2901). - crate::layer_multi_cmds::note_insert(s, &before); Ok(r) } diff --git a/crates/engine/src/cmd/layer_multi_cmds.rs b/crates/engine/src/cmd/layer_multi_cmds.rs index 5e5b7c371e5..e17a258ba4d 100644 --- a/crates/engine/src/cmd/layer_multi_cmds.rs +++ b/crates/engine/src/cmd/layer_multi_cmds.rs @@ -22,8 +22,7 @@ mod structure; use align::{align, distribute}; pub(crate) use damage::grow_hint; #[cfg(any(test, debug_assertions))] -pub(crate) use damage::hint_misses; -pub(crate) use damage::note_insert; +pub(crate) use damage::{hint_misses, small_change_pct}; pub use damage::{layer_bounds, same_pixels, selection_only_change, step_damage}; use moves::link_groups; pub(crate) use moves::shift_surface; diff --git a/crates/engine/src/cmd/layer_multi_cmds/damage.rs b/crates/engine/src/cmd/layer_multi_cmds/damage.rs index 0ac4e2ca8e3..2b77acd211f 100644 --- a/crates/engine/src/cmd/layer_multi_cmds/damage.rs +++ b/crates/engine/src/cmd/layer_multi_cmds/damage.rs @@ -1,7 +1,7 @@ use photocraft_color::{PixelFormat, SampleType}; use photocraft_compose::Ancestors; -use super::{Document, Layer, LayerContent, LayerId, Rect, Session, VideoData}; +use super::{Document, Layer, LayerContent, LayerId, Rect, VideoData}; /// Calls `f` with the rect of every tile that differs between two surfaces of the same format and /// default pixel: a tile only one side has, or a different copy-on-write tile pointer at the same @@ -238,7 +238,8 @@ fn doc_pixels_equal(a: &Document, b: &Document) -> bool { clipping_path: _, quick_mask, quick_mask_unselected: _, - patterns, + // Compared by `patterns_pixels_equal`. + patterns: _, color_table, duotone, layer_comps: _, @@ -265,11 +266,57 @@ fn doc_pixels_equal(a: &Document, b: &Document) -> bool { (Some(x), Some(y)) => alpha_channels_equal(x, y), _ => false, } - && *patterns == b.patterns + && patterns_pixels_equal(a, b) && *color_table == b.color_table && *duotone == b.duotone } +/// Whether `a`'s and `b`'s pattern libraries composite the same. Layers draw a pattern only +/// through a reference that `photocraft_doc::pattern::find` resolves (fill and shape layers, +/// Pattern Overlay, pattern strokes and glows, Bevel textures), so patterns appended after an +/// unchanged prefix change nothing unless some reference now resolves differently: to an +/// appended pattern where it resolved to an earlier one or to none. A same-named addition that +/// loses to an existing id match changes nothing. Pattern Stamp copies its pattern into the +/// document this way on first use; the pixels it paints are a raster change, diffed by tile like +/// any other. +fn patterns_pixels_equal(a: &Document, b: &Document) -> bool { + use photocraft_doc::effects::{Effect, FxPaint}; + use photocraft_doc::{Fill, Pattern, pattern::find}; + if a.patterns == b.patterns { + return true; + } + if a.patterns.len() >= b.patterns.len() || !b.patterns.starts_with(a.patterns.as_slice()) { + return false; + } + // The index a reference resolves to; the prefix is equal, so equal indices mean equal patterns. + let at = |ps: &[Pattern], id: &str, name: &str| find(ps, id, name).and_then(|p| ps.iter().position(|q| std::ptr::eq(q, p))); + let hits = |id: &str, name: &str| at(&a.patterns, id, name) != at(&b.patterns, id, name); + let fill = |f: &Fill| matches!(f, Fill::Pattern { id, name, .. } if hits(id, name)); + let paint = |p: &FxPaint| matches!(p, FxPaint::Pattern { id, name, .. } if hits(id, name)); + // Past a nesting depth no real document reaches, assume a reference (unequal), as + // `pixels_equal_with` does. + fn any_layer(layers: &[Layer], depth: usize, refers: &dyn Fn(&Layer) -> bool) -> bool { + depth >= 256 || layers.iter().any(|l| refers(l) || matches!(&l.content, LayerContent::Group(g) if any_layer(&g.children, depth + 1, refers))) + } + let refers = |l: &Layer| { + let effect = l.effects.items.iter().any(|e| match e { + Effect::OuterGlow(g) | Effect::InnerGlow(g) => paint(&g.paint), + Effect::Stroke(s) => paint(&s.paint), + Effect::PatternOverlay { id, name, .. } => hits(id, name), + Effect::BevelEmboss(b) => b.texture.as_ref().is_some_and(|t| hits(&t.id, &t.name)), + Effect::DropShadow(_) | Effect::InnerShadow(_) | Effect::ColorOverlay { .. } | Effect::GradientOverlay { .. } | Effect::Satin(_) => false, + }); + effect + || match &l.content { + LayerContent::Fill(f) => fill(f), + LayerContent::Shape(s) => s.fill.as_ref().is_some_and(fill) || s.stroke.as_ref().is_some_and(|st| fill(&st.paint)), + // A smart object draws its cache; its own document holds its own patterns. + LayerContent::Raster(_) | LayerContent::Group(_) | LayerContent::Adjustment(_) | LayerContent::Text(_) | LayerContent::Smart(_) => false, + } + }; + !any_layer(&b.layers, 0, &refers) +} + /// Whether `a` and `b` composite to the same pixels: they differ at most in fields the /// compositor never reads (selection, guides, paths, names, …), by the same classification as /// [`step_damage`] but without computing any bounds. Cheap: pixels compare by tile pointer. @@ -739,12 +786,18 @@ fn inert_at(layers: &[Layer], k: usize) -> bool { && (l.clipped || layers.get(k + 1).is_none_or(|above| !above.clipped)) } -/// Report the pixels a command made of more than one edit changed since `before` ([`step_damage`]), -/// so the canvas recomposites only where its new layers draw instead of the whole document. -/// A single [`Session::edit`] already reports its own damage: don't call this after one (#5695). -pub(crate) fn note_insert(s: &mut Session, before: &Document) { - let Some(st) = s.active_mut() else { return }; - st.last_damage = step_damage(before, &st.doc); +/// How much of the document (percent, rounded down) changed between `a` and `b`, when that is +/// some but under 25 %: what [`step_damage`] finds, which a command that reported "redraw +/// everything" (`None`) should have reported instead. `None` when nothing changed (a view-only +/// change), when it's 25 % or more, or when `step_damage` can't bound it (a canvas-size change). +#[cfg(any(test, debug_assertions))] +pub(crate) fn small_change_pct(a: &Document, b: &Document) -> Option { + let d = step_damage(a, b).filter(|d| !d.is_empty())?; + let doc = b.bounds(); + let whole = u64::from(doc.width()) * u64::from(doc.height()); + let changed = d.intersect(&doc); + let pct = u64::from(changed.width()) * u64::from(changed.height()) * 100 / whole.max(1); + (pct < 25).then_some(pct) } /// Content bounds of a layer's pixels as stored (type and shape layers: their cached render), or diff --git a/crates/engine/src/cmd/layer_multi_cmds/tile_damage_tests.rs b/crates/engine/src/cmd/layer_multi_cmds/tile_damage_tests.rs index 49a7c4c900c..5f1e880d797 100644 --- a/crates/engine/src/cmd/layer_multi_cmds/tile_damage_tests.rs +++ b/crates/engine/src/cmd/layer_multi_cmds/tile_damage_tests.rs @@ -408,3 +408,195 @@ fn painting_command_hints_cover_their_changed_tiles() { assert!(unchanged_outside(&before, &doc(&s), d), "{cmd}: pixels changed outside {d:?}"); } } + +// PR C (#9329): damage spans a command, not a revision. + +/// The damage since the command began: measured from the revision before it, bounded, and +/// covering every pixel the command changed. +fn command_damage(s: &mut Session, cmd: &str, p: serde_json::Value) -> (std::sync::Arc, Rect) { + let (before, rev) = (doc(s), s.active().unwrap().revision); + s.execute(cmd, p).unwrap_or_else(|e| panic!("{cmd}: {e}")); + let st = s.active().unwrap(); + assert!(st.revision - rev >= 2, "{cmd}: several revisions ({} → {})", rev, st.revision); + assert_eq!(st.damage_from(), rev, "{cmd}: measured from before the command"); + let d = st.last_damage.unwrap_or_else(|| panic!("{cmd}: bounded damage")); + assert!(unchanged_outside(&before, &st.doc, d), "{cmd}: pixels changed outside {d:?}"); + (before, d) +} + +#[test] +fn paste_into_reports_damage_since_the_command_began() { + let mut s = session(1024, 768); + layer_with(&mut s, &[(Rect::new(0, 0, 1024, 768), [0.2, 0.4, 0.6, 1.0]), (Rect::new(40, 40, 120, 120), [0.9, 0.1, 0.1, 1.0])]); + s.execute("select.rect", json!({"x": 40, "y": 40, "width": 100, "height": 100})).unwrap(); + s.execute("edit.copy", json!({})).unwrap(); + s.execute("select.rect", json!({"x": 700, "y": 500, "width": 120, "height": 90})).unwrap(); + let (_, d) = command_damage(&mut s, "edit.pasteSpecial.pasteInto", json!({})); + assert!(!d.intersect(&Rect::new(700, 500, 820, 590)).is_empty(), "the pasted area: {d:?}"); + assert!(u64::from(d.width()) * u64::from(d.height()) * 4 < 1024 * 768, "the pasted area, not the canvas: {d:?}"); +} + +#[test] +fn stroke_path_damage_spans_every_subpath() { + let mut s = session(1024, 768); + layer_with(&mut s, &[(Rect::new(0, 0, 1024, 768), [0.5, 0.5, 0.5, 1.0])]); + let path = json!({"subpaths": [{"knots": [[20, 20], [80, 20], [80, 80]]}, {"knots": [[800, 600], [900, 600], [900, 700]]}]}); + s.execute("path.set", json!({ "path": path })).unwrap(); + // A tool with its own `paint.*` command runs once per subpath, one revision each. + let (_, d) = command_damage(&mut s, "path.stroke", json!({"tool": "burn", "size": 16})); + assert!(d.contains(50, 20) && d.contains(850, 600), "both subpaths: {d:?}"); + assert_eq!(s.whole_redraws, 0); +} + +/// Pattern Stamp copies its pattern into the document on first use: a pattern no layer +/// refers to draws nothing, so a multi-subpath Stroke Path stays bounded. +#[test] +fn a_new_unreferenced_pattern_keeps_stroke_path_bounded() { + let mut s = session(1024, 768); + layer_with(&mut s, &[(Rect::new(0, 0, 1024, 768), [0.5, 0.5, 0.5, 1.0])]); + let path = json!({"subpaths": [{"knots": [[20, 20], [80, 20], [80, 80]]}, {"knots": [[800, 600], [900, 600], [900, 700]]}]}); + s.execute("path.set", json!({ "path": path })).unwrap(); + let n = doc(&s).patterns.len(); + let p = json!({"tool": "patternStamp", "size": 16, "hardness": 100, "pattern": "Checkerboard", "scale": 100, "aligned": true}); + let (_, d) = command_damage(&mut s, "path.stroke", p); + assert!(doc(&s).patterns.len() > n, "the stamp added its pattern"); + assert!(d.contains(50, 20) && d.contains(850, 600), "both subpaths: {d:?}"); + assert_eq!(s.whole_redraws, 0); +} + +/// A pattern that an existing layer refers to (here a fill layer whose pattern was missing) +/// changes that layer's pixels when it is added: not bounded by the tile diff. +#[test] +fn a_new_pattern_a_layer_refers_to_is_unbounded() { + let mut s = session(400, 300); + s.execute("layer.newFillLayer.pattern", json!({"pattern": "Checkerboard"})).unwrap(); + // Without its pixel cache, so both sides render the fill from the library. + let mut with = (*doc(&s)).clone(); + with.layers.iter_mut().for_each(|l| l.fill_cache = None); + let mut without = with.clone(); + without.patterns.clear(); + assert!(!same_pixels(&without, &with), "the fill layer draws only with its pattern"); + assert_eq!(step_damage(&without, &with), None); + // Unchanged libraries, and an unreferenced addition, stay pixel-equal. + assert_eq!(step_damage(&with, &with), Some(Rect::EMPTY)); + let plain = doc(&session(400, 300)); + let mut more = (*plain).clone(); + more.patterns = with.patterns.clone(); + assert_eq!(step_damage(&plain, &more), Some(Rect::EMPTY)); + assert!(unchanged_outside(&plain, &more, Rect::EMPTY)); +} + +/// An appended pattern sharing its name with one a layer already resolves by id loses to that id +/// match (`pattern::find` tries ids first), so the layer draws as before: bounded, not a whole +/// redraw. A cross-document layer copy can bring such a pattern along. +#[test] +fn a_same_named_new_pattern_a_reference_still_resolves_past_is_bounded() { + let mut s = session(400, 300); + s.execute("layer.newFillLayer.pattern", json!({"pattern": "Checkerboard"})).unwrap(); + let mut with = (*doc(&s)).clone(); + with.layers.iter_mut().for_each(|l| l.fill_cache = None); + let refers_by_id = with.layers.iter().any(|l| matches!(&l.content, LayerContent::Fill(photocraft_doc::Fill::Pattern { id, .. }) if !id.is_empty())); + assert!(refers_by_id, "the fill layer refers to its pattern by id"); + let mut twin = with.patterns[0].clone(); + twin.id = "another-id".into(); + twin.surface.fill_rect(Rect::new(0, 0, twin.width as i32, twin.height as i32), &[1.0, 0.0, 1.0, 1.0]); + let mut more = with.clone(); + more.patterns.push(twin); + assert_eq!(step_damage(&with, &more), Some(Rect::EMPTY)); + assert!(unchanged_outside(&with, &more, Rect::EMPTY)); +} + +#[test] +fn a_failed_compound_reports_no_change() { + let mut s = session(600, 400); + let id = layer_with(&mut s, &[(Rect::new(0, 0, 50, 50), [1.0, 0.0, 0.0, 1.0])]); + let (before, rev) = (doc(&s), s.active().unwrap().revision); + let r: Result<()> = crate::analysis_cmds::compound(&mut s, "Both", |s| { + paint(s, id, Rect::new(300, 200, 340, 240), [0.0, 1.0, 0.0, 1.0]); + paint(s, id, Rect::new(10, 300, 40, 340), [0.0, 0.0, 1.0, 1.0]); + Err(EngineError::Other("the second step failed".into())) + }); + assert!(r.is_err()); + // What `Session::dispatch` does after any inline command. + s.note_command_damage("test.compound", &before, rev); + let st = s.active().unwrap(); + assert!(same_pixels(&before, &st.doc), "rolled back"); + assert_eq!((st.damage_from(), st.last_damage), (rev, Some(Rect::EMPTY)), "nothing changed since it began"); + assert_eq!(s.whole_redraws, 0); +} + +#[test] +fn select_then_edit_in_one_command_is_bounded() { + let mut s = session(1024, 768); + let id = layer_with(&mut s, &[(Rect::new(0, 0, 1024, 768), [0.2, 0.4, 0.6, 1.0])]); + let steps = vec![ + ("layer.select".to_string(), json!({"layer": id.0})), + ("select.rect".to_string(), json!({"x": 300, "y": 300, "width": 50, "height": 40})), + ("edit.fill".to_string(), json!({"color": "#ff0000"})), + ("select.deselect".to_string(), json!({})), + ]; + s.actions.list.push(crate::actions_cmds::Action::new("A", steps)); + let (_, d) = command_damage(&mut s, "actions.play", json!({"action": "A"})); + assert!(d.contains(300, 300) && d.contains(349, 339), "{d:?}"); + assert!(u64::from(d.width()) * u64::from(d.height()) * 4 < 1024 * 768, "the filled tiles, not the canvas: {d:?}"); + assert_eq!(s.whole_redraws, 0); +} + +#[test] +fn whole_document_redraws_of_small_edits_are_counted() { + let mut s = session(1000, 1000); + let id = layer_with(&mut s, &[(Rect::new(0, 0, 1000, 1000), [0.2, 0.4, 0.6, 1.0])]); + // A synthetic command that paints 1 % and reports "everything". + let rev = s.active().unwrap().revision; + let (before, _) = paint(&mut s, id, Rect::new(10, 10, 110, 110), [1.0, 0.0, 0.0, 1.0]); + s.active_mut().unwrap().last_damage = None; + s.note_command_damage("test.wholeRedraw", &before, rev); + assert_eq!(s.whole_redraws, 1); + // A canvas-size change really does redraw everything: not counted. + s.execute("image.canvasSize", json!({"width": 1200, "height": 900})).unwrap(); + assert_eq!(s.active().unwrap().last_damage, None); + assert_eq!(s.whole_redraws, 1); +} + +#[test] +fn no_command_redraws_the_whole_document_for_a_small_edit() { + let mut s = session(1024, 768); + let base = layer_with(&mut s, &[(Rect::new(0, 0, 1024, 768), [0.2, 0.4, 0.6, 1.0])]); + let small = layer_with(&mut s, &[(Rect::new(100, 100, 160, 160), [0.9, 0.1, 0.1, 1.0])]); + let other = layer_with(&mut s, &[(Rect::new(130, 130, 190, 190), [0.1, 0.9, 0.1, 1.0])]); + let path = json!({"subpaths": [{"knots": [[20, 20], [80, 20]]}, {"knots": [[800, 600], [900, 600]]}]}); + let steps: Vec<(&str, serde_json::Value)> = vec![ + ("layer.select", json!({"layer": base.0})), + ("select.rect", json!({"x": 300, "y": 300, "width": 50, "height": 40})), + ("edit.fill", json!({"color": "#ff0000"})), + ("filter.blur.gaussianBlur", json!({"radius": 2})), + ("edit.copy", json!({})), + ("select.rect", json!({"x": 600, "y": 400, "width": 80, "height": 60})), + ("edit.pasteSpecial.pasteInto", json!({})), + ("edit.undo", json!({})), + ("edit.redo", json!({})), + ("layer.select", json!({"layer": base.0})), + ("paint.stroke", json!({"points": [[100, 400], [200, 420]], "size": 20, "color": "#2040ff"})), + ("edit.undo", json!({})), + ("edit.redo", json!({})), + ("path.set", json!({ "path": path })), + ("path.stroke", json!({"tool": "pencil", "size": 4, "color": "#0000ff", "opacity": 100})), + ("path.stroke", json!({"tool": "burn", "size": 16})), + ("layer.select", json!({"layer": small.0})), + ("layer.setProps", json!({"layer": small.0, "opacity": 0.5})), + ("layer.setProps", json!({"layer": small.0, "visible": false})), + ("layer.setProps", json!({"layer": small.0, "visible": true})), + ("layer.translate", json!({"layer": small.0, "dx": 20, "dy": 10})), + // Show Only hides the full-canvas base: a large change, so not counted either way. + ("layer.showOnly", json!({"layer": small.0})), + ("layer.showOnly", json!({"layer": small.0})), + ("layer.arrange.bringForward", json!({})), + ("layer.arrange.sendBackward", json!({})), + ("layer.setProps", json!({"layer": other.0, "clipped": true})), + ("layer.new.layer", json!({})), + ]; + for (cmd, p) in steps { + s.execute(cmd, p).unwrap_or_else(|e| panic!("{cmd}: {e}")); + assert_eq!(s.whole_redraws, 0, "{cmd}: redrew the whole document for a small edit"); + } +} diff --git a/crates/engine/src/cmd/vector_cmds/paths.rs b/crates/engine/src/cmd/vector_cmds/paths.rs index e54c38fd198..df9d9515204 100644 --- a/crates/engine/src/cmd/vector_cmds/paths.rs +++ b/crates/engine/src/cmd/vector_cmds/paths.rs @@ -533,9 +533,8 @@ fn stroke_with_command(s: &mut Session, p: &Value, cmd: &str, lines: &[vector::P params.remove(k); } let journal = s.journal.len(); + let before = s.active().ok_or(EngineError::NoDocument)?.doc.clone(); let r = crate::analysis_cmds::compound(s, "Stroke Path", |s| { - // The union of what each subpath's command changed; None when one of them didn't say. - let mut dmg = Some(Rect::EMPTY); for pl in lines { let pts = stroke_points(&pl.pts, pl.closed, simulate); if pts.is_empty() { @@ -544,25 +543,14 @@ fn stroke_with_command(s: &mut Session, p: &Value, cmd: &str, lines: &[vector::P let mut call = params.clone(); call.insert("points".into(), Value::Array(pts.iter().map(|q| json!([q.x, q.y, q.pressure])).collect())); s.execute(cmd, Value::Object(call))?; - let r = s.active().and_then(|d| d.last_damage); - dmg = dmg.zip(r).map(|(a, b)| { - if a.is_empty() { - b - } else if b.is_empty() { - a - } else { - a.union(&b) - } - }); } - Ok(dmg) + Ok(()) }); s.journal.truncate(journal); - let dmg = r?; - if let Some(st) = s.active_mut() { - st.last_damage = dmg; - } - let dmg = dmg.unwrap_or_else(|| s.active().map_or(Rect::EMPTY, |d| d.doc.bounds())); + r?; + // One subpath keeps its command's damage; several are one span, which `Session::dispatch` + // reports. The result says what changed either way. + let dmg = s.active().and_then(|st| crate::layer_multi_cmds::step_damage(&before, &st.doc).or(Some(st.doc.bounds()))).unwrap_or(Rect::EMPTY); Ok(json!({ "damage": [dmg.x0, dmg.y0, dmg.width(), dmg.height()] })) } diff --git a/crates/engine/src/doc_state.rs b/crates/engine/src/doc_state.rs index cb9bc5fe8a4..5137fa8bac8 100644 --- a/crates/engine/src/doc_state.rs +++ b/crates/engine/src/doc_state.rs @@ -24,8 +24,13 @@ pub struct DocState { pub pcraft_cache: photocraft_io::PcraftExportCache, /// How much of `revision` came from view-only changes (channel eyes, targeting): pixel caches can ignore those. pub view_revision: u64, - /// Area changed by the latest revision (None = assume everything changed). + /// Area changed since revision [`DocState::damage_from`] (None = assume everything changed): + /// the latest revision's, or a whole command's when it made several. pub last_damage: Option, + /// `(from, to)`: the command that moved `revision` from `from` to `to` in more than one step, + /// whose whole span `last_damage` covers. Written only by `Session::dispatch`; any later + /// revision makes it stale, so [`DocState::damage_from`] falls back to one revision. + pub(crate) damage_span: Option<(u64, u64)>, /// Coalescing key of the latest history step (see [`Session::execute`]'s `coalesce` param). pub coalesce: Option, /// Channels panel: targeted channel and eye toggles (view state, not history). @@ -108,6 +113,7 @@ impl DocState { pcraft_cache: Default::default(), view_revision: 0, last_damage: None, + damage_span: None, coalesce: None, channel_view, isolated_layers: Vec::new(), @@ -144,6 +150,15 @@ impl DocState { pub(crate) fn keeps_clean(&self, before: &Document) -> bool { self.saved_revision == self.revision && layer_multi_cmds::selection_only_change(before, &self.doc) } + /// The revision [`DocState::last_damage`] is measured from: the revision before the command + /// that made it, or the previous revision. A canvas that showed any revision from this one + /// up to (not including) `revision` needs only `last_damage` redrawn. + pub fn damage_from(&self) -> u64 { + match self.damage_span { + Some((from, to)) if to == self.revision => from, + _ => self.revision.saturating_sub(1), + } + } /// Move to the next revision; `keep_clean` (from [`Self::keeps_clean`]) moves the saved /// revision with it. pub(crate) fn bump_revision(&mut self, keep_clean: bool) { diff --git a/crates/engine/src/jobs.rs b/crates/engine/src/jobs.rs index 2197d57f910..38459ef154c 100644 --- a/crates/engine/src/jobs.rs +++ b/crates/engine/src/jobs.rs @@ -1790,7 +1790,7 @@ impl Session { // action it plays (a script step) must not flush the steps it ran so far into the action // being recorded, nor move the recording mark past the entries it is about to replace. let recording = if spec.journal && crate::actions_cmds::replayable(id) { self.actions.recording.take() } else { None }; - let before = self.active().map(|st| st.doc.clone()); + let before = self.active().map(|st| (st.doc.clone(), st.revision)); let docs_before: Vec<_> = self.docs.iter().map(|st| st.doc.id).collect(); // Last-resort guard (AGENTS.md, Never crash): a command that panics anyway fails with an // error instead of taking the app down. `edit` only commits a document after its closure @@ -1811,6 +1811,11 @@ impl Session { self.hint_before = None; } let pending = self.jobs.pending.take(); + if pending.is_none() + && let Some((doc, revision)) = &before + { + self.note_command_damage(id, doc, *revision); + } // Shape Blur by an installed shape's id or name records the geometry it ran with, as the // filter's own prepared params carry it: the journal (Last Filter, recorded actions) // replays where that shape is not installed. The run resolved it already, so it can't fail. @@ -1837,7 +1842,7 @@ impl Session { Ok(Started::Job(jid)) } (Ok(v), None) => { - if let Some(before) = &before { + if let Some((before, _)) = &before { self.add_relayout_damage(before); } if let Some((doc_id, layer_id)) = new_fx_target @@ -1866,6 +1871,29 @@ impl Session { out } + /// The one place a command's redraw area spans more than one revision. A command that moved + /// the active document's revision more than once (Paste Into, Stroke Path, a compound, a + /// script) reports what changed since it began, [`layer_multi_cmds::step_damage`] of the + /// document before and after, measured from the revision before it. A single-revision command + /// keeps the damage it reported. Debug and test builds also count a whole-document redraw of + /// a small change ([`Session::whole_redraws`]). + pub(crate) fn note_command_damage(&mut self, id: &str, before: &Arc, from: u64) { + let Some(st) = self.active_mut().filter(|st| st.doc.id == before.id && st.revision > from) else { return }; + if st.revision - from > 1 { + st.last_damage = crate::layer_multi_cmds::step_damage(before, &st.doc); + st.damage_span = Some((from, st.revision)); + } + #[cfg(any(test, debug_assertions))] + if st.last_damage.is_none() + && let Some(pct) = crate::layer_multi_cmds::small_change_pct(before, &st.doc) + { + eprintln!("warning: {id}: redraws the whole document but changed {pct}% of it"); + self.whole_redraws += 1; + } + #[cfg(not(any(test, debug_assertions)))] + let _ = id; + } + /// Grows the damage a command reported for the pixels it changed by what effects laid out /// over a layer's frame repaint when it moved ([`photocraft_compose::relayout_damage`]): a /// stroke on a layer with a Gradient Overlay reports the stroke, but the gradient is laid out diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 567b5e392bf..bce1b26aaab 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -338,6 +338,9 @@ pub struct Session { /// command reporting too narrow a redraw area (#4053). Counted in debug and test builds only; /// tests read it. pub damage_misses: usize, + /// Commands that left the whole document to redraw (`last_damage` None) while changing under + /// 25 % of it ([`Session::dispatch`]). Counted in debug and test builds only; tests read it. + pub whole_redraws: usize, } #[cfg(test)] diff --git a/crates/ui-egui/examples/perf_scenarios.rs b/crates/ui-egui/examples/perf_scenarios.rs index 95a52a1fa75..1f25292897d 100644 --- a/crates/ui-egui/examples/perf_scenarios.rs +++ b/crates/ui-egui/examples/perf_scenarios.rs @@ -1744,4 +1744,92 @@ fn redraw_area_scenarios(b: &mut Bench, sz: &Sizes) { } } } + + let paste = "app frame after Paste Into on 24 MP (CPU canvas)"; + if b.wanted(paste) { + let (w, h) = sz.big_photo; + paste_into_frame_scenario(b, paste, w, h); + } +} + +/// #9329 PR C: the whole app (kittest harness, CPU canvas) with a 24 MP photo. Each sample runs +/// Paste Into a 5 % selection (untimed, after a frame that shows the selection), then times the +/// next frame, then undoes the paste and the selection (untimed). Paste Into is a compound command (several +/// revisions), so the frame's refresh goes through the real `damage_since` path. +fn paste_into_frame_scenario(b: &mut Bench, name: &str, w: u32, h: u32) { + // 0.224² ≈ 5 % of the canvas. + let sel = json!({"x": w / 2, "y": h / 2, "width": w * 224 / 1000, "height": h * 224 / 1000}); + let built = open_photo(w, h).and_then(|mut s| { + exec(&mut s, "select.rect", json!({"x": w / 10, "y": h / 10, "width": w / 5, "height": h / 5}))?; + exec(&mut s, "edit.copy", json!({}))?; + Ok(s) + }); + let mut s = match built { + Ok(s) => s, + Err(e) => return b.errors.push((name.into(), e)), + }; + let doc = std::mem::take(&mut s); + let mut harness = egui_kittest::Harness::builder().with_size(egui::vec2(1440.0, 900.0)).with_max_steps(4).build_eframe(move |cc| { + photocraft_ui_egui::PhotocraftApp::setup_context(&cc.egui_ctx, Default::default()); + photocraft_ui_egui::PhotocraftApp::new(doc, photocraft_ui_egui::Services::default()) + }); + // Warm up: first frames build the canvas cache of the 24 MP document. + harness.run_steps(6); + b.current = Some(name.to_string()); + b.sampler.reset(); + b.px.borrow_mut().clear(); + let mut samples = Vec::new(); + let run = (|| -> Res<()> { + let layers = |h: &egui_kittest::Harness<'_, photocraft_ui_egui::PhotocraftApp>| h.state().session.active().map(|d| d.doc.layers.len()); + let base_layers = layers(&harness); + let past = |h: &egui_kittest::Harness<'_, photocraft_ui_egui::PhotocraftApp>| h.state().session.active().map(|d| d.history.past_len()); + let base_past = past(&harness); + for i in 0..=b.reps.max(1) { + let rev = |h: &egui_kittest::Harness<'_, photocraft_ui_egui::PhotocraftApp>| h.state().session.active().map(|d| d.revision); + // Untimed: Paste Into turns the selection into the pasted layer's + // mask, so each sample re-creates the same ~5 % selection (the + // clipboard is not consumed), and shows it for a frame, as the app + // does between a selection and the next command. Without that frame + // the canvas would last have shown the revision before `select.rect`, + // two commands back, and redraw everything whatever Paste Into reports. + harness.state_mut().run("select.rect", sel.clone())?; + harness.step(); + let before = rev(&harness).ok_or("no document")?; + harness.state_mut().run("edit.pasteSpecial.pasteInto", json!({}))?; + let after = rev(&harness).ok_or("no document")?; + // The path claim: a multi-revision command. + if after < before + 2 { + return Err(format!("path check: Paste Into moved revision by {}, not >= 2", after - before)); + } + harness.state_mut().perf.last_refresh_px = 0; + let t = Instant::now(); + harness.step(); + let took = ms(t); + let px = harness.state().perf.last_refresh_px; + if px == 0 { + return Err("path check: the frame after Paste Into recorded no refresh".into()); + } + // Untimed: undo the paste and then the selection (one history step each), so every + // sample pastes onto the same document rather than onto the layers earlier samples + // pasted, and starts from the same history rather than one more selection state per + // sample. The next sample's selection frame shows the undone document before its timer. + harness.state_mut().run("edit.undo", json!({}))?; + if layers(&harness) != base_layers { + return Err("path check: undoing Paste Into did not restore the layer stack".into()); + } + harness.state_mut().run("edit.undo", json!({}))?; + if past(&harness) != base_past { + return Err("path check: undoing Paste Into and the selection did not restore the history".into()); + } + // Sample 0 is the warm-up. + if i > 0 { + samples.push(took); + b.px.borrow_mut().push(px); + } + } + Ok(()) + })(); + // The rows' document handle: the harness's session. + let st = std::mem::take(&mut harness.state_mut().session); + b.record(name, &st, &samples, &[], run); } diff --git a/crates/ui-egui/src/canvas/channel_view.rs b/crates/ui-egui/src/canvas/channel_view.rs index 6a15fb9bdd7..163441252dd 100644 --- a/crates/ui-egui/src/canvas/channel_view.rs +++ b/crates/ui-egui/src/canvas/channel_view.rs @@ -489,7 +489,7 @@ pub fn ensure(app: &mut PhotocraftApp, ctx: &egui::Context, idx: usize, display: let st = app.session.documents().get(idx)?; let view = st.channel_view.clone(); let active = st.active_layer; - let (revision, last_damage) = (st.revision, st.last_damage); + let (revision, last_damage) = (st.revision, (st.damage_from(), st.last_damage)); let content = revision.wrapping_sub(st.view_revision); let id = st.doc.id.0; if view.is_plain(&st.doc) { diff --git a/crates/ui-egui/src/canvas/navigator.rs b/crates/ui-egui/src/canvas/navigator.rs index 413bf82686f..c465a638bb5 100644 --- a/crates/ui-egui/src/canvas/navigator.rs +++ b/crates/ui-egui/src/canvas/navigator.rs @@ -75,8 +75,8 @@ pub fn navigator_texture(app: &mut PhotocraftApp, ctx: &egui::Context, idx: usiz return ensure_texture(app, ctx, idx, app.session.color.main_display).map(|(t, _)| t); } // Keyed by the document snapshot, not its revision: selecting a layer changes no pixels. - let (snapshot, id, revision, last_damage) = - app.session.documents().get(idx).map(|st| (std::sync::Arc::downgrade(&st.doc), st.doc.id, st.revision, st.last_damage))?; + let (snapshot, id, revision, damage_from, last_damage) = + app.session.documents().get(idx).map(|st| (std::sync::Arc::downgrade(&st.doc), st.doc.id, st.revision, st.damage_from(), st.last_damage))?; let (doc, preview_key) = display_doc(app, idx); let (display, display_key) = canvas_display(app, &doc, None); let preview_key = preview_key ^ display_key; @@ -102,13 +102,13 @@ pub fn navigator_texture(app: &mut PhotocraftApp, ctx: &egui::Context, idx: usiz return Some(t.id()); } let t0 = crate::gpu_canvas::now_ms(); - // One edit since the cached image, with the area it changed: update just that part (nothing + // One edit (or command) since the cached image, with the area it changed: update just that part (nothing // for a rename, a selection or a new empty layer). A thumbnail composite of the whole // document on every edit cost a full-resolution CPU composite on the UI thread. let size = photocraft_compose::thumbnail_size(&doc, NAVIGATOR_SIDE); if let Some((_, p, rev, t)) = &cached && *p == preview_key - && rev.checked_add(1) == Some(revision) + && (damage_from..revision).contains(rev) && let Some(d) = last_damage && t.size() == [size.0 as usize, size.1 as usize] { diff --git a/crates/ui-egui/src/canvas/tests.rs b/crates/ui-egui/src/canvas/tests.rs index be0adf94b13..5ce68488e19 100644 --- a/crates/ui-egui/src/canvas/tests.rs +++ b/crates/ui-egui/src/canvas/tests.rs @@ -2304,7 +2304,7 @@ fn brush_drag_shows_the_real_stroke_and_commits_it() { assert_eq!(alpha(&app.session.documents()[0].doc, 40, 30), 0.0, "not committed yet"); // The canvas redraws only what the stroke touched. let dk = canvas_display(&app, &app.session.documents()[0].doc, None).1; - let d = damage_since(&app, 0, (rev, dk), (rev, key), dk, None).unwrap(); + let d = damage_since(&app, 0, (rev, dk), (rev, key), dk, (rev, None)).unwrap(); assert!(d.contains(40, 30) && !d.contains(40, 2) && d.height() < 60, "{d:?}"); tool_event(&mut app, ToolEvent::Up { x: 100.0, y: 30.0 }, m); let doc = app.session.documents()[0].doc.clone(); @@ -2312,6 +2312,60 @@ fn brush_drag_shows_the_real_stroke_and_commits_it() { assert_eq!((alpha(&doc, 40, 30), alpha(&doc, 40, 38)), (alpha(&shown, 40, 30), alpha(&shown, 40, 38)), "commit matches the preview"); } +/// #9329 PR C: Paste Into is three revisions; the canvas redraws what the whole command +/// changed (the pasted area), not the whole document as when only one revision counted. +#[test] +fn paste_into_refreshes_only_the_pasted_area() { + let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), Default::default()); + app.background_jobs = false; + let ctx = egui::Context::default(); + app.run("file.new", json!({"width": 800, "height": 600})).unwrap(); + app.run("select.rect", json!({"x": 40, "y": 40, "width": 100, "height": 100})).unwrap(); + app.run("edit.copy", json!({})).unwrap(); + app.run("select.rect", json!({"x": 500, "y": 400, "width": 90, "height": 60})).unwrap(); + ensure_texture(&mut app, &ctx, 0, None).expect("a texture"); + let rev = app.session.active().unwrap().revision; + app.perf.last_refresh_px = 0; + app.run("edit.pasteSpecial.pasteInto", json!({})).unwrap(); + assert!(app.session.active().unwrap().revision >= rev + 2, "several revisions"); + ensure_texture(&mut app, &ctx, 0, None).expect("a texture"); + let px = app.perf.last_refresh_px; + assert!(px > 0 && px < 800 * 600 / 4, "refreshed {px} px of {}", 800 * 600); +} + +/// The Move tool's Auto-Select pick is its own revision that changes no pixels: the drag +/// that follows still redraws only where the layer was and is. +#[test] +fn auto_select_move_is_still_bounded() { + let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), Default::default()); + app.background_jobs = false; + let ctx = egui::Context::default(); + app.run("file.new", json!({"width": 800, "height": 600})).unwrap(); + for (x, color) in [(100, "#ff0000"), (500, "#0000ff")] { + app.run("layer.new.layer", json!({})).unwrap(); + app.run("select.rect", json!({"x": x, "y": 100, "width": 40, "height": 40})).unwrap(); + app.run("edit.fill", json!({"color": color})).unwrap(); + } + app.run("select.deselect", json!({})).unwrap(); + app.ui.tool = Tool::Move; + app.ui.tool_options.move_auto_select = true; + app.ui.extras.snap = false; + app.ui.view.show.smart_guides = false; + let active = app.session.active().unwrap().active_layer; + ensure_texture(&mut app, &ctx, 0, None).expect("a texture"); + let rev = app.session.active().unwrap().revision; + let m = egui::Modifiers::NONE; + tool_event(&mut app, ToolEvent::Down { x: 120.0, y: 120.0, pressure: 1.0 }, m); + assert_ne!(app.session.active().unwrap().active_layer, active, "Auto-Select picked the red layer"); + assert_eq!(app.session.active().unwrap().revision, rev + 1, "the pick is one revision"); + tool_event(&mut app, ToolEvent::Move { x: 150.0, y: 130.0, pressure: 1.0 }, m); + app.perf.last_refresh_px = 0; + ensure_texture(&mut app, &ctx, 0, None).expect("a texture"); + let px = app.perf.last_refresh_px; + assert!(px > 0 && px < 800 * 600 / 4, "refreshed {px} px of {}", 800 * 600); + tool_event(&mut app, ToolEvent::Up { x: 150.0, y: 130.0 }, m); +} + #[test] fn live_stroke_uses_pen_tilt_and_updates_a_reduced_texture_partially() { // Pen tilt drives the size; the preview must use the tilt the commit gets. The document @@ -3305,14 +3359,14 @@ fn free_transform_redraws_only_the_moving_layer_when_the_box_opens_and_closes() let (_, key) = display_doc(&mut app, 0); assert_eq!(key, crate::transform_tool::preview_key(app.transform_preview.as_ref().unwrap().session)); let dk = canvas_display(&app, &app.session.documents()[0].doc, None).1; - let opened = damage_since(&app, 0, (rev, dk), (rev, key), dk, None).expect("opening the box has a damage area"); + let opened = damage_since(&app, 0, (rev, dk), (rev, key), dk, (rev, None)).expect("opening the box has a damage area"); assert!(opened.contains(20, 20) && opened.contains(59, 49), "{opened:?}"); assert!(opened.width() < 400 && opened.height() < 200, "not the whole canvas: {opened:?}"); - let canceled = damage_since(&app, 0, (rev, key ^ dk), (rev, 0), dk, None).expect("canceling has one too"); + let canceled = damage_since(&app, 0, (rev, key ^ dk), (rev, 0), dk, (rev, None)).expect("canceling has one too"); assert_eq!(canceled, opened); // A commit (one revision later) adds what it changed; anything else redraws everything. let moved = photocraft_geom::Rect::new(100, 20, 140, 50); - let committed = damage_since(&app, 0, (rev, key ^ dk), (rev + 1, 0), dk, Some(moved)).expect("committing too"); + let committed = damage_since(&app, 0, (rev, key ^ dk), (rev + 1, 0), dk, (rev, Some(moved))).expect("committing too"); assert!(committed.contains(20, 20) && committed.contains(139, 49), "{committed:?}"); - assert!(damage_since(&app, 0, (rev, key ^ dk), (rev + 2, 0), dk, Some(moved)).is_none(), "a later revision isn't ours"); + assert!(damage_since(&app, 0, (rev, key ^ dk), (rev + 2, 0), dk, (rev + 1, Some(moved))).is_none(), "a later revision isn't ours"); } diff --git a/crates/ui-egui/src/canvas/textures.rs b/crates/ui-egui/src/canvas/textures.rs index 951327d2e4f..d8bb2a12907 100644 --- a/crates/ui-egui/src/canvas/textures.rs +++ b/crates/ui-egui/src/canvas/textures.rs @@ -122,7 +122,7 @@ pub(crate) fn ensure_texture_in_view( let output = display; let (revision, last_damage, id) = { let st = app.session.documents().get(idx)?; - (st.revision, st.last_damage.map(|r| if r.is_empty() { r } else { r.inflate(effect_reach(&st.doc.layers)) }), st.doc.id) + (st.revision, (st.damage_from(), st.last_damage.map(|r| if r.is_empty() { r } else { r.inflate(effect_reach(&st.doc.layers)) })), st.doc.id) }; let (mut doc, mut preview_key) = display_doc(app, idx); // A filter dialog's live preview: the GPU canvas draws it from its own texture, but this CPU @@ -317,9 +317,17 @@ fn render_detail( } /// What changed since a canvas cache showed (`revision`, `preview key`) `seen`, when only a -/// rectangle did: the last edit's damage, or the live stroke's dabs since then. Cached keys have -/// the colour display's key folded in (`^ display_key`); `now`'s is still raw. -pub(crate) fn damage_since(app: &PhotocraftApp, idx: usize, seen: (u64, u64), now: (u64, u64), display_key: u64, last_damage: Option) -> Option { +/// rectangle did: the document's damage since revision `damage_from` (one revision, or a whole +/// command's span: [`photocraft_engine::DocState::damage_from`]), or the live stroke's dabs since +/// then. Cached keys have the colour display's key folded in (`^ display_key`); `now`'s is still raw. +pub(crate) fn damage_since( + app: &PhotocraftApp, + idx: usize, + seen: (u64, u64), + now: (u64, u64), + display_key: u64, + (damage_from, last_damage): (u64, Option), +) -> Option { // A brush stroke committed with the pixels its live preview already showed: only the steps // this cache hadn't shown yet (usually nothing), not the stroke's whole bounding box. if seen.0 + 1 == now.0 @@ -336,7 +344,9 @@ pub(crate) fn damage_since(app: &PhotocraftApp, idx: usize, seen: (u64, u64), no }; return Some(if r.is_empty() { r } else { r.inflate(effect_reach(&st.doc.layers)) }); } - if seen.1 == now.1 ^ display_key && seen.0 + 1 == now.0 { + // `seen` is within the span the document's damage covers. + let in_span = seen.0 >= damage_from && seen.0 < now.0; + if seen.1 == now.1 ^ display_key && in_span { return last_damage; } // Opening, canceling or committing a Free Transform box: the moving layer and the layers @@ -367,9 +377,9 @@ pub(crate) fn damage_since(app: &PhotocraftApp, idx: usize, seen: (u64, u64), no { return Some(r); } - // Between a Move drag's offsets: where the moving layers were and are (Auto-Select's pick - // at the press changes no pixels). - if (seen.0 == now.0 || (seen.0 + 1 == now.0 && last_damage.is_some_and(|r| r.is_empty()))) + // Between a Move drag's offsets: where the moving layers were and are. Revisions that changed + // no pixels since `seen` (Auto-Select's pick at the press) count as the same document. + if (seen.0 == now.0 || (in_span && last_damage.is_some_and(|r| r.is_empty()))) && let Some(st) = app.session.documents().get(idx) && let Some(r) = crate::move_ui::damage(app, st.doc.id, now.0, seen.1 ^ display_key, now.1) { @@ -464,11 +474,10 @@ pub(crate) fn effect_reach(layers: &[photocraft_doc::Layer]) -> i32 { /// Returns false if there is no GPU canvas. pub(super) fn ensure_gpu(app: &mut PhotocraftApp, ctx: &egui::Context, idx: usize, visible: DRect) -> bool { let Some(gpu) = app.gpu.clone() else { return false }; - let Some((revision, last_damage, id)) = app - .session - .documents() - .get(idx) - .map(|st| (st.revision, st.last_damage.map(|r| if r.is_empty() { r } else { r.inflate(effect_reach(&st.doc.layers)) }), st.doc.id)) + let Some((revision, last_damage, id)) = + app.session.documents().get(idx).map(|st| { + (st.revision, (st.damage_from(), st.last_damage.map(|r| if r.is_empty() { r } else { r.inflate(effect_reach(&st.doc.layers)) })), st.doc.id) + }) else { return false; }; diff --git a/crates/ui-egui/src/canvas/viewport_pages.rs b/crates/ui-egui/src/canvas/viewport_pages.rs index 07933ede306..ce0c119d098 100644 --- a/crates/ui-egui/src/canvas/viewport_pages.rs +++ b/crates/ui-egui/src/canvas/viewport_pages.rs @@ -609,12 +609,14 @@ pub(super) fn show(app: &mut PhotocraftApp, ctx: &egui::Context, idx: usize, xf: if app.ui.view.pattern_preview || gpu.fault().is_some() { return false; } - let Some((revision, last_damage, id, size)) = app - .session - .documents() - .get(idx) - .map(|st| (st.revision, st.last_damage.map(|r| if r.is_empty() { r } else { r.inflate(effect_reach(&st.doc.layers)) }), st.doc.id, st.doc.size)) - else { + let Some((revision, last_damage, id, size)) = app.session.documents().get(idx).map(|st| { + ( + st.revision, + (st.damage_from(), st.last_damage.map(|r| if r.is_empty() { r } else { r.inflate(effect_reach(&st.doc.layers)) })), + st.doc.id, + st.doc.size, + ) + }) else { return false; }; let size = [size.width, size.height]; diff --git a/crates/ui-egui/src/dialogs/color_picker_preview/tests.rs b/crates/ui-egui/src/dialogs/color_picker_preview/tests.rs index 47123948623..7901bbb387f 100644 --- a/crates/ui-egui/src/dialogs/color_picker_preview/tests.rs +++ b/crates/ui-egui/src/dialogs/color_picker_preview/tests.rs @@ -209,7 +209,7 @@ fn damage_between_previews_is_the_text_layer_only() { // The canvas asks through `damage_since`, with the display's key folded into what it cached. let doc = app.session.active().unwrap().doc.clone(); let dk = crate::canvas::canvas_display(&app, &doc, None).1; - let since = |seen: u64, now: u64| crate::canvas::damage_since(&app, 0, (revision, seen ^ dk), (revision, now), dk, None); + let since = |seen: u64, now: u64| crate::canvas::damage_since(&app, 0, (revision, seen ^ dk), (revision, now), dk, (revision, None)); assert_eq!(since(red, blue), Some(bounds), "the canvas refreshes only the text layer between colours"); assert_eq!(since(blue, 0), Some(bounds), "and on Cancel"); } @@ -260,7 +260,7 @@ fn ok_counts_the_previewed_canvas_as_the_document_so_only_the_layer_is_redrawn() assert_eq!(seen.1, dk, "gradient={gradient}: OK's edit is what the canvas already shows"); // So the commit's own damage is all the canvas refreshes, not the whole document. let edit = photocraft_geom::Rect::new(1, 2, 3, 4); - let damage = crate::canvas::damage_since(&app, 0, seen, (revision, 0), dk, Some(edit)); + let damage = crate::canvas::damage_since(&app, 0, seen, (revision, 0), dk, (revision - 1, Some(edit))); assert_eq!(damage, Some(edit), "gradient={gradient}"); } } diff --git a/crates/ui-egui/src/panels/histogram_panel.rs b/crates/ui-egui/src/panels/histogram_panel.rs index 5b10587286d..a3576b0df96 100644 --- a/crates/ui-egui/src/panels/histogram_panel.rs +++ b/crates/ui-egui/src/panels/histogram_panel.rs @@ -406,18 +406,19 @@ pub fn update(app: &mut PhotocraftApp, ctx: &egui::Context) { Err(std::sync::mpsc::TryRecvError::Empty) => {} } } - // A revision that changed no pixels (selecting a layer, say) keeps the histogram of the one - // before (#125), and an Uncached Refresh with it. - if let Some(st) = app.session.active() - && tone::view_only(st) - { + // Revisions that changed no pixels since the histogram's (selecting a layer, say) keep it + // (#125), and an Uncached Refresh with it. + if let Some(st) = app.session.active() { let (doc, rev) = (st.doc.id, st.revision); - if app.hist_uncached == Some((doc, rev.wrapping_sub(1))) { + if let Some((d, r)) = app.hist_uncached + && d == doc + && tone::pixels_kept_since(st, r) + { app.hist_uncached = Some((doc, rev)); } if let Some(e) = app.doc_hist.as_mut() && e.key.doc == doc - && e.key.revision.wrapping_add(1) == rev + && tone::pixels_kept_since(st, e.key.revision) { e.key.revision = rev; } diff --git a/crates/ui-egui/src/stroke_timing_tests.rs b/crates/ui-egui/src/stroke_timing_tests.rs index e50e85d4db0..b26d776a4da 100644 --- a/crates/ui-egui/src/stroke_timing_tests.rs +++ b/crates/ui-egui/src/stroke_timing_tests.rs @@ -366,7 +366,7 @@ fn a_committed_stroke_repaints_a_gradient_overlay_over_the_whole_layer() { let c = app.committed_stroke.as_ref().expect("the commit took the live stroke's pixels"); assert_eq!(c.revision, st.revision); // A cache that showed the live stroke's last step. - let missed = crate::canvas::damage_since(app, idx, (st.revision - 1, c.last_key()), (st.revision, 0), 0, st.last_damage).unwrap(); + let missed = crate::canvas::damage_since(app, idx, (st.revision - 1, c.last_key()), (st.revision, 0), 0, (st.damage_from(), st.last_damage)).unwrap(); assert!(missed.contains(25, 25), "the gradient around the first stroke is repainted: {missed:?}"); } diff --git a/crates/ui-egui/src/widgets/tone.rs b/crates/ui-egui/src/widgets/tone.rs index 51cd340a213..58f84dd9fa0 100644 --- a/crates/ui-egui/src/widgets/tone.rs +++ b/crates/ui-egui/src/widgets/tone.rs @@ -374,11 +374,10 @@ pub fn histograms(app: &mut PhotocraftApp, source: HistSource, space: ToneSpace) HistSource::BelowLayer(l) | HistSource::Layer(l) | HistSource::UpTo(l) => l, HistSource::Channel { layer, .. } | HistSource::Colors { layer, .. } => layer, }; - let pixels_kept = view_only(st); if let Some((d, l, r, h)) = app.tone_hist.as_mut() && *d == doc_id && *l == layer - && (*r == rev || (*r + 1 == rev && pixels_kept)) + && (*r == rev || pixels_kept_since(st, *r)) && h.tag == want { *r = rev; @@ -391,10 +390,10 @@ pub fn histograms(app: &mut PhotocraftApp, source: HistSource, space: ToneSpace) h } -/// Whether the latest revision changed no pixels (selecting a layer, say): cached histograms of -/// the previous revision still hold (#125). -pub(crate) fn view_only(st: &photocraft_engine::DocState) -> bool { - st.last_damage.is_some_and(|r| r.is_empty()) +/// Whether no pixels changed since revision `seen` (selecting a layer, say): cached histograms of +/// that revision still hold (#125). +pub(crate) fn pixels_kept_since(st: &photocraft_engine::DocState, seen: u64) -> bool { + seen >= st.damage_from() && seen < st.revision && st.last_damage.is_some_and(|r| r.is_empty()) } /// After an editor commits its own adjustment layer, the image below it is unchanged: keep the diff --git a/docs/scorecard.md b/docs/scorecard.md index 4a1f8dcc723..cff7e384d05 100644 --- a/docs/scorecard.md +++ b/docs/scorecard.md @@ -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 | 53 | 87 scenarios: 3 within budget, 22 over budget or failing, 9 tracked only, 49 not run, 4 not measurable yet (baseline 2026-10-05) | +| [Performance](#performance) | 3 | 22 | 54 | 88 scenarios: 3 within budget, 22 over budget or failing, 9 tracked only, 50 not run, 4 not measurable yet (baseline 2026-10-05) | | [File compatibility](#file-compatibility) | 11 | 14 | 2 | io corpus: oracle ≥ 148/170, round trip ≥ 169/170; psd-tools corpus: oracle ≥ 249/309, round trip ≥ 308/309; photoshop oracles: oracle ≥ 190/258, round trip ≥ 258/258 | | [Tools](#tools) | 36 | 9 | 0 | | | [Workspace and UI](#workspace-and-ui) | 15 | 6 | 2 | settings that do nothing: 10 of 199 (target 0) | @@ -109,6 +109,7 @@ Baseline: 2026-10-05, 1 run, commit `f5d2bcb54a`, Apple M4 Pro (48 GB), GPU Appl | P86 | Open a 36 MP JPEG (RGB 8-bit): import + session admission + first composite (#10259) | ≤ 500 ms p95 | – | – | – | – | – | not run | [#10259](https://github.com/storytold/photocraft/issues/10259) | | P87 | Toggle a masked adjustment mid-stack in the layered document + refresh | track only | – | – | – | – | – | not run | [#9329](https://github.com/storytold/photocraft/issues/9329) | | P88 | Bring forward / send backward a small layer in the many-layer document + refresh | track only | – | – | – | – | – | not run | [#9329](https://github.com/storytold/photocraft/issues/9329) | +| P89 | App frame after Paste Into on 24 MP (CPU canvas) | track only | – | – | – | – | – | not run | [#9329](https://github.com/storytold/photocraft/issues/9329) | **Not measurable yet** (the code can't do it yet; no number is recorded): diff --git a/perf/budgets.toml b/perf/budgets.toml index 7017ba84603..0d3d665e2db 100644 --- a/perf/budgets.toml +++ b/perf/budgets.toml @@ -874,3 +874,11 @@ issue = 9329 bench = "perf_scenarios" row = "bring forward / send backward a small layer in the many-layer document + refresh" enforce = false # new; no baseline yet + +[[scenario]] +id = "P89" +title = "App frame after Paste Into on 24 MP (CPU canvas)" +issue = 9329 +bench = "perf_scenarios" +row = "app frame after Paste Into on 24 MP (CPU canvas)" +enforce = false # new; no baseline yet