diff --git a/Cargo.lock b/Cargo.lock index a7fa1b483f..6744dcccb1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4961,7 +4961,7 @@ dependencies = [ [[package]] name = "photocraft" -version = "0.16.0" +version = "0.17.0" dependencies = [ "arboard", "async-io", @@ -5011,7 +5011,7 @@ dependencies = [ [[package]] name = "photocraft-affinity" -version = "0.16.0" +version = "0.17.0" dependencies = [ "crc32fast", "flate2", @@ -5021,7 +5021,7 @@ dependencies = [ [[package]] name = "photocraft-algo" -version = "0.16.0" +version = "0.17.0" dependencies = [ "criterion", "photocraft-color", @@ -5036,7 +5036,7 @@ dependencies = [ [[package]] name = "photocraft-automation" -version = "0.16.0" +version = "0.17.0" dependencies = [ "base64 0.23.1", "cap-std", @@ -5061,7 +5061,7 @@ dependencies = [ [[package]] name = "photocraft-cli" -version = "0.16.0" +version = "0.17.0" dependencies = [ "clap", "clap_complete", @@ -5080,7 +5080,7 @@ dependencies = [ [[package]] name = "photocraft-cms" -version = "0.16.0" +version = "0.17.0" dependencies = [ "moxcms", "rayon", @@ -5088,7 +5088,7 @@ dependencies = [ [[package]] name = "photocraft-codecs" -version = "0.16.0" +version = "0.17.0" dependencies = [ "craft-jpeg", "exr", @@ -5110,7 +5110,7 @@ dependencies = [ [[package]] name = "photocraft-color" -version = "0.16.0" +version = "0.17.0" dependencies = [ "photocraft-cms", "serde", @@ -5118,7 +5118,7 @@ dependencies = [ [[package]] name = "photocraft-compose" -version = "0.16.0" +version = "0.17.0" dependencies = [ "criterion", "photocraft-cms", @@ -5134,7 +5134,7 @@ dependencies = [ [[package]] name = "photocraft-doc" -version = "0.16.0" +version = "0.17.0" dependencies = [ "photocraft-color", "photocraft-geom", @@ -5145,7 +5145,7 @@ dependencies = [ [[package]] name = "photocraft-engine" -version = "0.16.0" +version = "0.17.0" dependencies = [ "blake3", "chrono", @@ -5186,7 +5186,7 @@ dependencies = [ [[package]] name = "photocraft-format" -version = "0.16.0" +version = "0.17.0" dependencies = [ "blake3", "crc32fast", @@ -5209,14 +5209,14 @@ dependencies = [ [[package]] name = "photocraft-geom" -version = "0.16.0" +version = "0.17.0" dependencies = [ "serde", ] [[package]] name = "photocraft-gpu" -version = "0.16.0" +version = "0.17.0" dependencies = [ "bytemuck", "half", @@ -5236,14 +5236,14 @@ dependencies = [ [[package]] name = "photocraft-heif" -version = "0.16.0" +version = "0.17.0" dependencies = [ "heic-rs", ] [[package]] name = "photocraft-io" -version = "0.16.0" +version = "0.17.0" dependencies = [ "blake3", "craft-docpath", @@ -5278,18 +5278,18 @@ dependencies = [ [[package]] name = "photocraft-ipp" -version = "0.16.0" +version = "0.17.0" [[package]] name = "photocraft-ops" -version = "0.16.0" +version = "0.17.0" dependencies = [ "photocraft-doc", ] [[package]] name = "photocraft-paint" -version = "0.16.0" +version = "0.17.0" dependencies = [ "photocraft-cms", "photocraft-color", @@ -5302,7 +5302,7 @@ dependencies = [ [[package]] name = "photocraft-plugins" -version = "0.16.0" +version = "0.17.0" dependencies = [ "photocraft-color", "photocraft-geom", @@ -5318,7 +5318,7 @@ dependencies = [ [[package]] name = "photocraft-psd" -version = "0.16.0" +version = "0.17.0" dependencies = [ "flate2", "photocraft-psd", @@ -5329,7 +5329,7 @@ dependencies = [ [[package]] name = "photocraft-raster" -version = "0.16.0" +version = "0.17.0" dependencies = [ "photocraft-color", "photocraft-geom", @@ -5340,7 +5340,7 @@ dependencies = [ [[package]] name = "photocraft-raw" -version = "0.16.0" +version = "0.17.0" dependencies = [ "flate2", "image", @@ -5355,7 +5355,7 @@ dependencies = [ [[package]] name = "photocraft-tablet" -version = "0.16.0" +version = "0.17.0" dependencies = [ "block2 0.6.2", "objc2 0.6.5", @@ -5367,7 +5367,7 @@ dependencies = [ [[package]] name = "photocraft-testkit" -version = "0.16.0" +version = "0.17.0" dependencies = [ "memory-stats", "serde_json", @@ -5375,7 +5375,7 @@ dependencies = [ [[package]] name = "photocraft-text" -version = "0.16.0" +version = "0.17.0" dependencies = [ "flate2", "parley", @@ -5390,7 +5390,7 @@ dependencies = [ [[package]] name = "photocraft-ui-egui" -version = "0.16.0" +version = "0.17.0" dependencies = [ "base64 0.23.1", "craft-print", @@ -5435,7 +5435,7 @@ dependencies = [ [[package]] name = "photocraft-vector" -version = "0.16.0" +version = "0.17.0" dependencies = [ "photocraft-color", "photocraft-doc", @@ -5446,7 +5446,7 @@ dependencies = [ [[package]] name = "photocraft-web" -version = "0.16.0" +version = "0.17.0" dependencies = [ "eframe", "egui", @@ -5468,7 +5468,7 @@ dependencies = [ [[package]] name = "photocraft-web-boot" -version = "0.16.0" +version = "0.17.0" dependencies = [ "flate2", "js-sys", @@ -9184,7 +9184,7 @@ checksum = "ec7a2a501ed189703dba8b08142f057e887dfc4b2cc4db2d343ac6376ba3e0b9" [[package]] name = "xtask" -version = "0.16.0" +version = "0.17.0" dependencies = [ "chrono", "serde", diff --git a/Cargo.toml b/Cargo.toml index e7a70d55b3..720b16776b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -4,7 +4,7 @@ members = ["crates/*", "apps/*", "xtask"] default-members = ["crates/*", "apps/photocraft", "apps/photocraft-cli", "xtask"] [workspace.package] -version = "0.16.0" +version = "0.17.0" edition = "2024" license = "MIT OR Apache-2.0" rust-version = "1.95" diff --git a/apps/photocraft/src/services.rs b/apps/photocraft/src/services.rs index 48d78aa922..c4350141fb 100644 --- a/apps/photocraft/src/services.rs +++ b/apps/photocraft/src/services.rs @@ -634,8 +634,8 @@ fn shell_word(s: &str) -> String { if s.bytes().all(|b| b.is_ascii_alphanumeric() || b"/._-+:,@".contains(&b)) { s.to_string() } else { format!("'{}'", s.replace('\'', r"'\''")) } } -/// The release-check transport, or `None` inside Flatpak: the sandbox has no network (see the -/// manifest's finish-args) and `flatpak update` owns updates there. +/// The release-check transport, or `None` inside Flatpak: `flatpak update` owns updates there. +/// The sandbox has network (for the `--control` server), which does not change this. fn update_transport(flatpak_id: Option<&std::ffi::OsStr>) -> Option { flatpak_id.is_none().then(crate::updates::service) } diff --git a/crates/engine/src/cmd.rs b/crates/engine/src/cmd.rs index 1d0f41bf3e..aeb41bdad3 100644 --- a/crates/engine/src/cmd.rs +++ b/crates/engine/src/cmd.rs @@ -464,7 +464,6 @@ fn build() -> Vec { *active = Some(id); Ok(id) })?; - crate::layer_multi_cmds::note_inert_insert(s, id); Ok(json!({ "layer": id.0 })) } ), @@ -606,9 +605,6 @@ fn build() -> Vec { ["name"] => "Rename Layer", _ => "Layer Properties", }; - let before = s.active().ok_or(EngineError::NoDocument)?.doc.clone(); - // Clipping and channel changes reach the layers around it: recomposite everything. - let local = p.get("clipped").is_none() && p.get("channels").is_none(); // The eye: a history state only with History Options › Make Layer Visibility // Changes Undoable. let only_visibility = changed.as_slice() == ["visible"]; @@ -664,9 +660,6 @@ fn build() -> Vec { } Ok(()) })?; - if local { - crate::layer_multi_cmds::note_damage(s, &before, &ids); - } Ok(Value::Null) } ), diff --git a/crates/engine/src/cmd/brush_cmds.rs b/crates/engine/src/cmd/brush_cmds.rs index 479e33790e..e0f165afcd 100644 --- a/crates/engine/src/cmd/brush_cmds.rs +++ b/crates/engine/src/cmd/brush_cmds.rs @@ -243,9 +243,7 @@ fn layer_id(s: &Session, p: &Value) -> Result { } fn damage_json(s: &mut Session, dmg: Rect) -> Value { - if let Some(st) = s.active_mut() { - st.last_damage = Some(dmg); - } + s.hint_damage(dmg); json!({ "damage": [dmg.x0, dmg.y0, dmg.width(), dmg.height()] }) } diff --git a/crates/engine/src/cmd/eraser_cmds.rs b/crates/engine/src/cmd/eraser_cmds.rs index 129753ea86..a506ecd403 100644 --- a/crates/engine/src/cmd/eraser_cmds.rs +++ b/crates/engine/src/cmd/eraser_cmds.rs @@ -67,10 +67,8 @@ fn pixels_locked(doc: &Document, l: &Layer) -> Option { } fn damage(s: &mut Session, dmg: Rect, erased: bool) -> Value { - if let Some(st) = s.active_mut() - && !dmg.is_empty() - { - st.last_damage = Some(dmg); + if !dmg.is_empty() { + s.hint_damage(dmg); } json!({ "erased": erased, "damage": [dmg.x0, dmg.y0, dmg.width(), dmg.height()] }) } diff --git a/crates/engine/src/cmd/layer_multi_cmds.rs b/crates/engine/src/cmd/layer_multi_cmds.rs index 52f36b9ffa..79abb250a3 100644 --- a/crates/engine/src/cmd/layer_multi_cmds.rs +++ b/crates/engine/src/cmd/layer_multi_cmds.rs @@ -20,8 +20,10 @@ mod selection; mod structure; use align::{align, distribute}; -pub use damage::{layer_bounds, layers_damage, same_pixels, selection_only_change, step_damage}; -pub(crate) use damage::{note_damage, note_inert_insert, note_insert}; +#[cfg(any(test, debug_assertions))] +pub(crate) use damage::hint_misses; +pub(crate) use damage::note_insert; +pub use damage::{layer_bounds, same_pixels, selection_only_change, step_damage}; use moves::link_groups; pub(crate) use moves::shift_surface; #[cfg(test)] @@ -164,3 +166,6 @@ mod tests; #[cfg(test)] mod layers_panel_parity_tests; + +#[cfg(test)] +mod tile_damage_tests; diff --git a/crates/engine/src/cmd/layer_multi_cmds/damage.rs b/crates/engine/src/cmd/layer_multi_cmds/damage.rs index b76c7fd059..8440fab75b 100644 --- a/crates/engine/src/cmd/layer_multi_cmds/damage.rs +++ b/crates/engine/src/cmd/layer_multi_cmds/damage.rs @@ -1,57 +1,60 @@ use photocraft_color::{PixelFormat, SampleType}; -use super::{Document, Layer, LayerContent, LayerId, Rect, Session, VideoData}; +use super::{Document, Layer, LayerContent, Rect, Session, VideoData}; -/// The document pixels an edit of the layers `ids` (moved, shown, hidden or restyled; contents -/// otherwise as they were) can have changed from `before` to `after`, or `None` for anywhere. -pub fn layers_damage(before: &Document, after: &Document, ids: &[LayerId]) -> Option { - let mut out = Rect::EMPTY; - for d in [before, after] { - let canvas = d.bounds(); - for id in ids { - // A layer gone from one side (never for a move) changes who knows what. - let (siblings, k, outer) = siblings_of(d, *id)?; - let b = photocraft_compose::change_bounds_in(siblings, k, outer, canvas)?; - if !b.is_empty() { - out = if out.is_empty() { b } else { out.union(&b) }; +/// 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 +/// coordinate. A merge walk of the two sorted tile maps: pointer compares, no pixel reads. Both +/// documents stay alive during the comparison, so tile addresses cannot be reused under it. +/// `false` (and no calls) when the format or default pixel differs: every pixel may have changed. +fn each_changed_tile(a: &photocraft_raster::Surface, b: &photocraft_raster::Surface, mut f: impl FnMut(Rect)) -> bool { + if a.format() != b.format() || a.default_bytes() != b.default_bytes() { + return false; + } + let (mut xs, mut ys) = (a.tiles().peekable(), b.tiles().peekable()); + loop { + match (xs.peek(), ys.peek()) { + (None, None) => return true, + (Some((ca, ta)), Some((cb, tb))) if ca == cb => { + if !std::sync::Arc::ptr_eq(ta, tb) { + f(ca.rect()); + } + xs.next(); + ys.next(); + } + (Some((ca, _)), Some((cb, _))) if ca < cb => { + f(ca.rect()); + xs.next(); + } + (Some((ca, _)), None) => { + f(ca.rect()); + xs.next(); + } + (_, Some((cb, _))) => { + f(cb.rect()); + ys.next(); } } } - Some(out) } -/// The sibling list holding layer `id` in `doc` (bottom→top), its index there, and the list -/// holding its parent group with the group's index (`None` at the top level). -type Siblings<'d> = (&'d [Layer], usize, Option<(&'d [Layer], usize)>); - -fn siblings_of(doc: &Document, id: LayerId) -> Option> { - let path = doc.path_of(id)?; - let (&k, parent) = path.split_last()?; - let Some((&g, grand)) = parent.split_last() else { return Some((doc.layers.as_slice(), k, None)) }; - let outer = if grand.is_empty() { doc.layers.as_slice() } else { doc.layer_at(grand)?.children()? }; - Some((outer.get(g)?.children()?, k, Some((outer, g)))) +/// The pixels that differ between two surfaces, to tile precision: the bounding box of the tiles +/// [`each_changed_tile`] finds (empty when the pixels are identical), or `None` when the format +/// or default pixel differs. +fn surface_damage(a: &photocraft_raster::Surface, b: &photocraft_raster::Surface) -> Option { + let mut out = Rect::EMPTY; + each_changed_tile(a, b, |r| out = out.union(&r)).then_some(out) } -/// Report [`layers_damage`] as the active document's last change, so the canvas recomposites -/// only that (grown by the effect reach around it) instead of everything. -pub(crate) fn note_damage(s: &mut Session, before: &Document, ids: &[LayerId]) { - let Some(st) = s.active_mut() else { return }; - if before.size != st.doc.size { - return; - } - st.last_damage = layers_damage(before, &st.doc, ids); -} - -/// Whether two surfaces hold identical pixels: same format and default pixel, and the same -/// copy-on-write tile pointers at the same coordinates. Both documents stay alive during the -/// comparison, so tile addresses cannot be reused under it. +/// Whether two surfaces hold identical pixels (see [`surface_damage`]). fn surfaces_equal(a: &photocraft_raster::Surface, b: &photocraft_raster::Surface) -> bool { - a.format() == b.format() - && a.default_pixel() == b.default_pixel() - && a.tile_count() == b.tile_count() - && a.tiles().zip(b.tiles()).all(|((ca, ta), (cb, tb))| ca == cb && std::sync::Arc::ptr_eq(ta, tb)) + surface_damage(a, b) == Some(Rect::EMPTY) } +/// How [`layer_pixels_cmp`] compares a pair of surfaces a layer draws from, given the feather (in +/// pixels) the compositor blurs it by: `true` when the pair counts as equal for the caller. +type SurfaceCmp<'f> = &'f mut dyn FnMut(&photocraft_raster::Surface, &photocraft_raster::Surface, f32) -> bool; + /// Byte blobs kept in `Arc`s (PSD raw blocks, embedded smart-object sources): pointer-identical /// or content-equal (they are small compared to pixels, and undo steps rarely re-parse them). fn blob_equal(x: &std::sync::Arc>, y: &std::sync::Arc>) -> bool { @@ -66,18 +69,18 @@ fn blobs_equal(a: &Option>>, b: &Option, b: &Option) -> bool { +/// Layer masks (also the smart-filter mask): scalars plus the surface, compared by `surf`. +fn masks_cmp(a: &Option, b: &Option, surf: SurfaceCmp) -> bool { match (a, b) { (None, None) => true, (Some(x), Some(y)) => { - x.enabled == y.enabled && x.linked == y.linked && x.density == y.density && x.feather == y.feather && surfaces_equal(&x.surface, &y.surface) + x.enabled == y.enabled && x.linked == y.linked && x.density == y.density && x.feather == y.feather && surf(&x.surface, &y.surface, x.feather) } _ => false, } } -fn texts_equal(a: &photocraft_doc::TextLayer, b: &photocraft_doc::TextLayer) -> bool { +fn texts_cmp(a: &photocraft_doc::TextLayer, b: &photocraft_doc::TextLayer, surf: SurfaceCmp) -> bool { a.text == b.text && a.font_family == b.font_family && a.size_pt == b.size_pt @@ -89,15 +92,15 @@ fn texts_equal(a: &photocraft_doc::TextLayer, b: &photocraft_doc::TextLayer) -> && a.orientation == b.orientation && a.antialias == b.antialias && a.warp == b.warp + && blobs_equal(&a.psd_raw, &b.psd_raw) && match (&a.cache, &b.cache) { (None, None) => true, - (Some(x), Some(y)) => surfaces_equal(x, y), + (Some(x), Some(y)) => surf(x, y, 0.0), _ => false, } - && blobs_equal(&a.psd_raw, &b.psd_raw) } -fn smarts_equal(a: &photocraft_doc::SmartObject, b: &photocraft_doc::SmartObject) -> bool { +fn smarts_cmp(a: &photocraft_doc::SmartObject, b: &photocraft_doc::SmartObject, surf: SurfaceCmp) -> bool { a.filters_enabled == b.filters_enabled && a.transform == b.transform && a.smart_filters == b.smart_filters @@ -111,13 +114,14 @@ fn smarts_equal(a: &photocraft_doc::SmartObject, b: &photocraft_doc::SmartObject (photocraft_doc::SmartSource::Linked { path: x }, photocraft_doc::SmartSource::Linked { path: y }) => x == y, _ => false, } + && blobs_equal(&a.psd_raw, &b.psd_raw) + // The filter mask shapes the filtered result the cache holds: compared exactly. + && masks_cmp(&a.filter_mask, &b.filter_mask, &mut |x, y, _| surfaces_equal(x, y)) && match (&a.cache, &b.cache) { (None, None) => true, - (Some(x), Some(y)) => surfaces_equal(x, y), + (Some(x), Some(y)) => surf(x, y, 0.0), _ => false, } - && blobs_equal(&a.psd_raw, &b.psd_raw) - && masks_equal(&a.filter_mask, &b.filter_mask) } /// Extra alpha/spot channels (the image itself in Multichannel mode). @@ -134,6 +138,13 @@ fn alpha_channels_equal(a: &photocraft_doc::AlphaChannel, b: &photocraft_doc::Al /// (name, locks, label, `psd_blocks`, `psd_id`, link group, XMP — none of them composite; video /// bails out earlier in the walk). fn layer_pixels_equal(a: &Layer, b: &Layer) -> bool { + layer_pixels_cmp(a, b, &mut |x, y, _| surfaces_equal(x, y)) +} + +/// [`layer_pixels_equal`] with the surfaces the layer draws from (its pixels, mask, type and +/// smart-object caches, fill cache) compared by `surf`; everything else compares exactly, before +/// any surface, so `surf` sees a pair only when every scalar the compositor reads is equal. +fn layer_pixels_cmp(a: &Layer, b: &Layer, surf: SurfaceCmp) -> bool { let Layer { id, name: _, @@ -164,27 +175,38 @@ fn layer_pixels_equal(a: &Layer, b: &Layer) -> bool { && *opacity == b.opacity && *fill_opacity == b.fill_opacity && *clipped == b.clipped - && masks_equal(mask, &b.mask) && *vector_mask == b.vector_mask && *effects == b.effects && *excluded_channels == b.excluded_channels && *blend_if == b.blend_if && *advanced == b.advanced + && match (fill_cache, &b.fill_cache) { + (None, None) => true, + (Some(x), Some(y)) => x.fill == y.fill, + _ => false, + } && match (content, &b.content) { - (LayerContent::Raster(x), LayerContent::Raster(y)) => surfaces_equal(x, y), + (LayerContent::Raster(_), LayerContent::Raster(_)) => true, // Children are walked separately; only the group's own clipping frame counts. (LayerContent::Group(x), LayerContent::Group(y)) => x.artboard == y.artboard, (LayerContent::Fill(x), LayerContent::Fill(y)) => x == y, (LayerContent::Adjustment(x), LayerContent::Adjustment(y)) => x == y, - (LayerContent::Text(x), LayerContent::Text(y)) => texts_equal(x, y), + (LayerContent::Text(x), LayerContent::Text(y)) => texts_cmp(x, y, &mut |_, _, _| true), (LayerContent::Shape(x), LayerContent::Shape(y)) => x == y, - (LayerContent::Smart(x), LayerContent::Smart(y)) => smarts_equal(x, y), + (LayerContent::Smart(x), LayerContent::Smart(y)) => smarts_cmp(x, y, &mut |_, _, _| true), _ => false, } + // Every scalar is equal: now the surfaces. + && masks_cmp(mask, &b.mask, surf) + && match (content, &b.content) { + (LayerContent::Raster(x), LayerContent::Raster(y)) => surf(x, y, 0.0), + (LayerContent::Text(x), LayerContent::Text(y)) => texts_cmp(x, y, surf), + (LayerContent::Smart(x), LayerContent::Smart(y)) => smarts_cmp(x, y, surf), + _ => true, + } && match (fill_cache, &b.fill_cache) { - (None, None) => true, - (Some(x), Some(y)) => x.fill == y.fill && surfaces_equal(&x.surface, &y.surface), - _ => false, + (Some(x), Some(y)) => surf(&x.surface, &y.surface, 0.0), + _ => true, } } @@ -365,13 +387,24 @@ pub fn selection_only_change(a: &Document, b: &Document) -> bool { /// what `layer.setProps` already reports as unbounded), and video layers. Inserting a copy of a /// 12 MP layer used to recomposite every layer of the document (#2883, P8/P9 in /// `perf/budgets.toml`). +/// +/// A layer whose only change is in the pixels of its surfaces (raster content, mask, type or +/// smart-object cache, fill cache) damages only the tiles that changed ([`tile_damage`]), so +/// undoing a brush dab on a full-canvas layer redraws the dab's tiles, not the canvas (#9329). pub fn step_damage(a: &Document, b: &Document) -> Option { if !doc_pixels_equal(a, b) { return None; } /// The list holding a sibling list's parent group, and the group's index there. type Outer<'d> = Option<(&'d [Layer], usize)>; - fn walk(xs: &[Layer], ys: &[Layer], (ox, oy): (Outer, Outer), a: &Document, b: &Document, out: &mut Rect) -> bool { + /// What the groups around a sibling list add to a change inside it: the summed reach of + /// their effects, and whether any lays effects out over its frame. + #[derive(Clone, Copy)] + struct Around { + reach: i32, + spans_frame: bool, + } + fn walk(xs: &[Layer], ys: &[Layer], (ox, oy): (Outer, Outer), around: Around, a: &Document, b: &Document, out: &mut Rect) -> bool { // Pair the layers by id, stepping over layers only one side has: one that draws nothing // changes no pixels (a new empty layer or group, #1771), any other only where it draws. let one_sided = |layers: &[Layer], k: usize, outer: Outer, canvas: Rect, out: &mut Rect| { @@ -422,7 +455,8 @@ pub fn step_damage(a: &Document, b: &Document) -> Option { if x.clipped != y.clipped || x.excluded_channels != y.excluded_channels { return false; } - if !layer_pixels_equal(x, y) { + let tiles = tile_damage(x, y); + if tiles != Some(Rect::EMPTY) { // Both sides' extents: a move changes pixels where it left and where it landed. // A clipped adjustment reaches as far as its base (#6535). let (Some(bx), Some(by)) = @@ -430,11 +464,27 @@ pub fn step_damage(a: &Document, b: &Document) -> Option { else { return false; }; - *out = if out.is_empty() { bx.union(&by) } else { out.union(&bx).union(&by) }; + let whole = bx.union(&by); + // Only pixels changed: the changed tiles, grown by what the groups around reach, + // never more than the whole extents. Effects laid out over a frame repaint it + // everywhere when it moves (#4053), so then the whole extents. + let frame_moved = around.spans_frame + || (photocraft_compose::effects::spans_frame(y) + && photocraft_compose::effects_frame(x, a.bounds()) != photocraft_compose::effects_frame(y, b.bounds())); + let d = match tiles { + Some(t) if !frame_moved => t.inflate(around.reach).intersect(&whole.inflate(around.reach)).intersect(&b.bounds()), + _ => whole, + }; + *out = out.union(&d); } match (x.children(), y.children()) { (Some(xc), Some(yc)) => { - if !walk(xc, yc, (Some((xs, xk)), Some((ys, yk))), a, b, out) { + let reach = |l: &Layer| if photocraft_compose::effects::has_effects(l) { photocraft_compose::effects::margin(l) } else { 0 }; + let inner = Around { + reach: around.reach.saturating_add(reach(x).max(reach(y))), + spans_frame: around.spans_frame || photocraft_compose::effects::spans_frame(x) || photocraft_compose::effects::spans_frame(y), + }; + if !walk(xc, yc, (Some((xs, xk)), Some((ys, yk))), inner, a, b, out) { return false; } } @@ -444,7 +494,44 @@ pub fn step_damage(a: &Document, b: &Document) -> Option { } } let mut out = Rect::EMPTY; - if walk(&a.layers, &b.layers, (None, None), a, b, &mut out) { Some(out) } else { None } + if walk(&a.layers, &b.layers, (None, None), Around { reach: 0, spans_frame: false }, a, b, &mut out) { Some(out) } else { None } +} + +/// How many tiles that differ between `before` and `after` (in the surfaces of layers both have) +/// `hint` misses: a command's own damage rect that misses a changed tile is too narrow (#4053). +#[cfg(any(test, debug_assertions))] +pub(crate) fn hint_misses(before: &Document, after: &Document, hint: Rect) -> usize { + let mut misses = 0; + for (_, _, y) in after.walk() { + let Some(x) = before.layer(y.id) else { continue }; + layer_pixels_cmp(x, y, &mut |p, q, _| { + each_changed_tile(p, q, |t| misses += usize::from(t.intersect(&hint).is_empty())); + true + }); + } + misses +} + +/// The pixels a change from layer `a` to layer `b` (the same layer on either side of a step) can +/// have changed when only the pixels of its surfaces changed: the tiles that differ +/// ([`surface_damage`]), a mask's grown by its feather, all grown by the layer's own effects. +/// Empty when the layers render identically. `None` when anything else changed (a property, the +/// content kind, a surface's format or default pixel): the caller then takes the layer's whole +/// extents. Children of groups are not compared (the caller walks them). +fn tile_damage(a: &Layer, b: &Layer) -> Option { + let mut tiles = Rect::EMPTY; + let only_pixels = layer_pixels_cmp(a, b, &mut |x, y, feather| { + let Some(r) = surface_damage(x, y) else { return false }; + let r = if feather > 0.0 { photocraft_vector::feather::grow(r, photocraft_vector::feather::feather_sigma(feather)) } else { r }; + tiles = tiles.union(&r); + true + }); + if !only_pixels { + return None; + } + // Effects are equal on both sides here (they are compared before any surface). + let margin = if photocraft_compose::effects::has_effects(b) { photocraft_compose::effects::margin(b) } else { 0 }; + Some(if tiles.is_empty() || margin == 0 { tiles } else { tiles.inflate(margin) }) } /// The pixels adding or removing `layers[k]` can change, when that is bounded: its change bounds @@ -487,19 +574,6 @@ fn inert_at(layers: &[Layer], k: usize) -> bool { && (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); - } -} - /// 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). diff --git a/crates/engine/src/cmd/layer_multi_cmds/moves.rs b/crates/engine/src/cmd/layer_multi_cmds/moves.rs index 822e0f6ad1..9be731e0c8 100644 --- a/crates/engine/src/cmd/layer_multi_cmds/moves.rs +++ b/crates/engine/src/cmd/layer_multi_cmds/moves.rs @@ -1,4 +1,3 @@ -use super::damage::note_damage; use super::selection::{reselect, same_named_targets, selected, top_level}; use super::structure::check_group_depth; use super::{Document, EngineError, Layer, LayerContent, LayerId, Result, Session, Value, bad, int_i32, json}; @@ -151,7 +150,7 @@ pub fn translate(s: &mut Session, p: &Value) -> Result { } let mask_target = p.get("target").and_then(Value::as_str) == Some("mask"); let before = s.active().ok_or(EngineError::NoDocument)?.doc.clone(); - let ids = s.edit("Move", |doc, _| { + s.edit("Move", |doc, _| { let (alone, rest): (Vec, Vec) = roots.iter().partition(|id| mask_target && doc.layer(**id).is_some_and(moves_mask_alone)); let mut ids = move_targets(doc, &rest); if ids.is_empty() && alone.is_empty() { @@ -181,7 +180,6 @@ pub fn translate(s: &mut Session, p: &Value) -> Result { } Ok(ids) })?; - note_damage(s, &before, &ids); Ok(Value::Null) } diff --git a/crates/engine/src/cmd/layer_multi_cmds/tests.rs b/crates/engine/src/cmd/layer_multi_cmds/tests.rs index 689a9856b4..a3c055cf7b 100644 --- a/crates/engine/src/cmd/layer_multi_cmds/tests.rs +++ b/crates/engine/src/cmd/layer_multi_cmds/tests.rs @@ -389,9 +389,6 @@ fn clipped_adjustment_in_pass_through_clip_base_damages_its_group() { let LayerContent::Group(g) = &mut deleted.layers[2].content else { panic!("group") }; g.children.remove(1); assert_eq!(step_damage(&before, &deleted), group); - let LayerContent::Group(g) = &before.layers[2].content else { panic!("group") }; - let ids = [g.children[1].id]; - assert_eq!(layers_damage(&before, &after, &ids), group); // Without the layer clipped to the group it stays pass-through: just A's base. before.layers.truncate(3); after.layers.truncate(3); 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 new file mode 100644 index 0000000000..c7acbff4a0 --- /dev/null +++ b/crates/engine/src/cmd/layer_multi_cmds/tile_damage_tests.rs @@ -0,0 +1,321 @@ +//! #9329 PR A: a pixel change damages only the tiles that changed, and command rects are hints +//! checked against them. + +use super::*; +use photocraft_geom::TILE_SIZE; + +fn session(w: u32, h: u32) -> Session { + let mut s = Session::new(); + s.execute("file.new", json!({"width": w, "height": h})).unwrap(); + s +} + +fn doc(s: &Session) -> std::sync::Arc { + s.active().unwrap().doc.clone() +} + +fn damage(s: &Session) -> Option { + s.active().unwrap().last_damage +} + +/// The composite of `after` equals `before`'s outside `damage`. +fn unchanged_outside(before: &Document, after: &Document, damage: Rect) -> bool { + let (x, y) = (photocraft_compose::render(before, before.bounds()), photocraft_compose::render(after, after.bounds())); + let w = before.bounds().width() as i32; + x.px.iter().zip(&y.px).enumerate().all(|(i, (p, q))| p == q || damage.contains(i as i32 % w, i as i32 / w)) +} + +/// The tiles `r` overlaps, as one rect. +fn tiles_of(r: Rect) -> Rect { + let down = |v: i32| v.div_euclid(TILE_SIZE) * TILE_SIZE; + Rect::new(down(r.x0), down(r.y0), down(r.x1 - 1) + TILE_SIZE, down(r.y1 - 1) + TILE_SIZE) +} + +/// A new pixel layer with `rects` filled opaque, through one plain edit. +fn layer_with(s: &mut Session, rects: &[(Rect, [f32; 4])]) -> LayerId { + let id = LayerId(s.execute("layer.new.layer", json!({})).unwrap()["layer"].as_u64().unwrap()); + s.edit("paint", |doc, _| { + let surf = doc.layer_mut(id).unwrap().surface_mut().unwrap(); + for (r, px) in rects { + surf.fill_rect(*r, px); + } + Ok(()) + }) + .unwrap(); + id +} + +fn full_layer(s: &mut Session) -> LayerId { + let canvas = doc(s).bounds(); + layer_with(s, &[(canvas, [0.4, 0.5, 0.6, 1.0])]) +} + +/// One plain edit of layer `id`'s pixels, returning the documents on either side. +fn paint(s: &mut Session, id: LayerId, r: Rect, px: [f32; 4]) -> (std::sync::Arc, std::sync::Arc) { + let before = doc(s); + s.edit("paint", |doc, _| { + doc.layer_mut(id).unwrap().surface_mut().unwrap().fill_rect(r, &px); + Ok(()) + }) + .unwrap(); + (before, doc(s)) +} + +#[test] +fn a_fill_on_a_selection_damages_only_its_tiles() { + let mut s = session(1024, 768); + full_layer(&mut s); + let sel = Rect::new(300, 300, 440, 410); + s.execute("select.rect", json!({"x": sel.x0, "y": sel.y0, "width": sel.width(), "height": sel.height()})).unwrap(); + let before = doc(&s); + s.execute("edit.fill", json!({"color": "#ff0000"})).unwrap(); + let d = damage(&s).expect("a bounded fill"); + assert!(!d.is_empty() && tiles_of(sel).contains_rect(&d), "{d:?}"); + assert!(d.size().area() < doc(&s).bounds().size().area() / 4, "{d:?}"); + assert!(unchanged_outside(&before, &doc(&s), d)); +} + +#[test] +fn undo_of_a_brush_dab_on_a_full_layer_redraws_only_its_tiles() { + let mut s = session(1024, 768); + full_layer(&mut s); + let four_tiles = 4 * (TILE_SIZE as u64).pow(2); + let before = doc(&s); + s.execute("paint.stroke", json!({"points": [[500, 400]], "size": 60, "color": "#ff0000"})).unwrap(); + let painted = doc(&s); + let d = damage(&s).expect("the dab"); + assert!(!d.is_empty() && d.size().area() <= four_tiles, "dab {d:?}"); + assert!(unchanged_outside(&before, &painted, d)); + assert!(s.undo()); + let d = damage(&s).expect("undo is bounded"); + assert!(!d.is_empty() && d.size().area() <= four_tiles, "undo {d:?}"); + assert!(unchanged_outside(&painted, &doc(&s), d)); + assert!(s.redo()); + let d = damage(&s).expect("redo is bounded"); + assert!(!d.is_empty() && d.size().area() <= four_tiles, "redo {d:?}"); + assert!(unchanged_outside(&before, &doc(&s), d)); +} + +/// The damage `note_damage` used to report after a move: both extents of the moved layer. +#[test] +fn a_move_still_damages_both_extents() { + let mut s = session(1024, 768); + let r = Rect::new(100, 100, 200, 200); + let id = layer_with(&mut s, &[(r, [1.0, 0.0, 0.0, 1.0])]); + let before = doc(&s); + s.execute("layer.translate", json!({"layer": id.0, "dx": 300, "dy": 50})).unwrap(); + let after = doc(&s); + let canvas = after.bounds(); + let extents = |d: &Document| photocraft_compose::change_bounds(d.layer(id).unwrap(), canvas).unwrap(); + let both = extents(&before).union(&extents(&after)); + assert_eq!(both, r.union(&r.translate(300, 50))); + assert_eq!(damage(&s), Some(both)); + assert!(unchanged_outside(&before, &after, both)); +} + +#[test] +fn set_props_damage_is_unchanged_without_note_damage() { + let r = Rect::new(100, 100, 200, 200); + for props in [json!({"opacity": 0.5}), json!({"fill": 0.5}), json!({"blend": "multiply"}), json!({"visible": false})] { + let mut s = session(1024, 768); + let id = layer_with(&mut s, &[(r, [1.0, 0.0, 0.0, 0.8])]); + let before = doc(&s); + let mut p = props.clone(); + p["layer"] = json!(id.0); + s.execute("layer.setProps", p).unwrap(); + assert_eq!(damage(&s), Some(r), "{props}"); + assert!(unchanged_outside(&before, &doc(&s), r), "{props}"); + } +} + +#[test] +fn a_mask_edit_damages_its_tiles_grown_by_the_feather() { + let mut s = session(1024, 768); + let id = full_layer(&mut s); + let feather = 10.0; + s.edit("mask", |doc, _| { + doc.layer_mut(id).unwrap().mask = Some(photocraft_doc::LayerMask { feather, ..photocraft_doc::LayerMask::reveal_all() }); + Ok(()) + }) + .unwrap(); + let before = doc(&s); + let hole = Rect::new(600, 400, 610, 410); + s.edit("mask paint", |doc, _| { + doc.layer_mut(id).unwrap().mask.as_mut().unwrap().surface.fill_rect(hole, &[0.0]); + Ok(()) + }) + .unwrap(); + let after = doc(&s); + let d = step_damage(&before, &after).unwrap(); + let grown = photocraft_vector::feather::grow(tiles_of(hole), photocraft_vector::feather::feather_sigma(feather)); + assert_eq!(d, grown.intersect(&after.bounds())); + assert!(d.contains_rect(&tiles_of(hole)) && d != after.bounds(), "{d:?}"); + assert!(unchanged_outside(&before, &after, d)); +} + +#[test] +fn tile_damage_grows_by_the_layer_effect_margin() { + let mut s = session(1024, 768); + let (a, b) = (Rect::new(50, 50, 150, 150), Rect::new(600, 400, 700, 500)); + let id = layer_with(&mut s, &[(a, [1.0, 0.0, 0.0, 1.0]), (b, [1.0, 0.0, 0.0, 1.0])]); + s.edit("fx", |doc, _| { + doc.layer_mut(id).unwrap().effects.items.push(photocraft_doc::Effect::default_drop_shadow()); + Ok(()) + }) + .unwrap(); + let m = photocraft_compose::effects::margin(doc(&s).layer(id).unwrap()); + assert!(m > 0); + let (before, after) = paint(&mut s, id, Rect::new(640, 440, 660, 460), [0.0, 0.0, 0.0, 0.0]); + let d = step_damage(&before, &after).unwrap(); + let whole = photocraft_compose::change_bounds(after.layer(id).unwrap(), after.bounds()).unwrap(); + assert_eq!(d, tiles_of(b).inflate(m).intersect(&whole)); + assert!(d.x0 < tiles_of(b).x0 && d.y0 < tiles_of(b).y0, "the shadow reaches past the tile: {d:?}"); + assert!(unchanged_outside(&before, &after, d)); +} + +#[test] +fn a_dab_inside_a_group_with_a_drop_shadow_damages_the_shadow() { + let mut s = session(1024, 768); + let child = layer_with(&mut s, &[(Rect::new(600, 400, 700, 500), [1.0, 0.0, 0.0, 1.0])]); + s.execute("layer.select", json!({"layer": child.0})).unwrap(); + let g = LayerId(s.execute("layer.groupLayers", json!({})).unwrap()["layer"].as_u64().unwrap()); + s.edit("fx", |doc, _| { + doc.layer_mut(g).unwrap().effects.items.push(photocraft_doc::Effect::default_drop_shadow()); + Ok(()) + }) + .unwrap(); + assert!(photocraft_compose::effects::margin(doc(&s).layer(g).unwrap()) > 0); + let dab = Rect::new(640, 440, 660, 460); + let (before, after) = paint(&mut s, child, dab, [0.0, 0.0, 1.0, 1.0]); + let d = damage(&s).expect("a bounded dab"); + assert!(d.contains_rect(&dab), "{d:?}"); + assert!(unchanged_outside(&before, &after, d), "the group's shadow changed outside {d:?}"); +} + +#[test] +fn painting_a_clip_base_under_a_styled_clipped_layer_is_covered() { + let mut s = session(1024, 768); + let base = layer_with(&mut s, &[(Rect::new(500, 300, 800, 600), [0.0, 1.0, 0.0, 1.0])]); + let top = full_layer(&mut s); + s.edit("clip", |doc, _| { + let l = doc.layer_mut(top).unwrap(); + l.clipped = true; + l.effects.items.push(photocraft_doc::Effect::default_drop_shadow()); + Ok(()) + }) + .unwrap(); + let (before, after) = paint(&mut s, base, Rect::new(300, 200, 340, 240), [0.0, 0.0, 1.0, 1.0]); + let d = damage(&s).expect("a bounded base edit"); + assert!(unchanged_outside(&before, &after, d), "the clipped layer changed outside {d:?}"); + let (before, after) = paint(&mut s, base, Rect::new(640, 440, 660, 460), [0.0, 0.0, 0.0, 0.0]); + let d = damage(&s).expect("a bounded base edit"); + assert!(unchanged_outside(&before, &after, d), "the clipped layer changed outside {d:?}"); +} + +/// #4053: effects laid out over the layer's frame repaint it everywhere when it grows. +#[test] +fn a_frame_spanning_effect_still_redraws_its_whole_frame() { + use photocraft_doc::{Effect, FxCommon, Gradient}; + let mut s = session(1024, 768); + let id = layer_with(&mut s, &[(Rect::new(100, 100, 700, 200), [1.0, 0.0, 0.0, 1.0])]); + s.edit("fx", |doc, _| { + doc.layer_mut(id).unwrap().effects.items.push(Effect::GradientOverlay { + common: FxCommon::new(photocraft_doc::BlendMode::Normal, 1.0), + gradient: Gradient::default(), + dither: false, + interpolation: None, + }); + Ok(()) + }) + .unwrap(); + let (before, after) = paint(&mut s, id, Rect::new(700, 100, 800, 200), [0.0, 1.0, 0.0, 1.0]); + let d = step_damage(&before, &after).unwrap(); + let extents = |d: &Document| photocraft_compose::change_bounds(d.layer(id).unwrap(), d.bounds()).unwrap(); + let whole = extents(&before).union(&extents(&after)); + assert!(whole.contains_rect(&Rect::new(100, 100, 800, 200)), "{whole:?}"); + assert_eq!(d, whole); + assert!(unchanged_outside(&before, &after, d)); +} + +#[test] +fn refreshing_type_layers_damages_only_those_layers() { + let mut s = session(1500, 1000); + let one = LayerId(s.execute("type.create", json!({"x": 100, "y": 100, "text": "One", "size": 20})).unwrap()["layer"].as_u64().unwrap()); + let two = LayerId(s.execute("type.create", json!({"x": 1000, "y": 800, "text": "Two", "size": 20})).unwrap()["layer"].as_u64().unwrap()); + let before = doc(&s); + let busy = s.refresh_type_layers(&[(before.id, one)]); + assert!(busy.is_empty()); + let after = doc(&s); + let d = damage(&s).expect("a bounded refresh"); + let bounds = |d: &Document, id: LayerId| photocraft_compose::change_bounds(d.layer(id).unwrap(), d.bounds()).unwrap(); + assert!(bounds(&before, one).union(&bounds(&after, one)).contains_rect(&d), "{d:?}"); + assert!(d.intersect(&bounds(&after, two)).is_empty(), "{d:?}"); + assert!(unchanged_outside(&before, &after, d)); +} + +#[test] +fn a_hint_that_misses_a_changed_tile_is_counted() { + let mut s = session(1024, 768); + let id = full_layer(&mut s); + paint(&mut s, id, Rect::new(600, 400, 620, 420), [1.0, 0.0, 0.0, 1.0]); + assert_eq!(s.damage_misses, 0); + s.hint_damage(Rect::new(0, 0, 10, 10)); + assert_eq!(s.damage_misses, 1); + assert_eq!(damage(&s), Some(Rect::EMPTY)); + // A hint covering the change is not a miss. + paint(&mut s, id, Rect::new(600, 400, 620, 420), [0.0, 1.0, 0.0, 1.0]); + s.hint_damage(Rect::new(600, 400, 620, 420)); + assert_eq!(s.damage_misses, 1); + assert_eq!(damage(&s), Some(Rect::new(600, 400, 620, 420))); +} + +#[test] +fn painting_command_hints_cover_their_changed_tiles() { + let mut s = session(600, 400); + let mut rects = Vec::new(); + for by in 0..20 { + for bx in 0..30 { + let v = ((bx * 7 + by * 13) % 17) as f32 / 17.0; + rects.push((Rect::from_xywh(bx * 20, by * 20, 20, 20), [v, 1.0 - v, 0.5 * v + 0.2, 1.0])); + } + } + rects.push((Rect::new(430, 280, 470, 320), [0.1, 0.1, 0.1, 1.0])); + for y in 292..309 { + for x in 442..459 { + if (x - 450) * (x - 450) + (y - 300) * (y - 300) <= 64 { + rects.push((Rect::from_xywh(x, y, 1, 1), [0.9, 0.05, 0.05, 1.0])); + } + } + } + layer_with(&mut s, &rects); + let ty = s.execute("type.create", json!({"x": 40, "y": 380, "text": "Hi", "size": 20})).unwrap()["layer"].as_u64().unwrap(); + let mut steps: Vec<(&str, serde_json::Value, Option)> = vec![ + ("paint.stroke", json!({"points": [[100, 100], [300, 120]], "size": 20, "color": "#2040ff"}), None), + ("paint.backgroundEraser", json!({"points": [[120, 300], [200, 320]], "size": 20}), None), + ("paint.cloneStamp", json!({"points": [[400, 100]], "offset": [-100, 0], "size": 30, "hardness": 100}), None), + ("paint.healingBrush", json!({"points": [[420, 200]], "source": [300, 200], "size": 30, "hardness": 100}), None), + ("paint.redEye", json!({"x": 450, "y": 300}), None), + ("paint.patch", json!({"offset": [60, 0]}), Some(Rect::new(60, 250, 100, 290))), + ("paint.remove", json!({"points": [[500, 100], [540, 140]], "size": 24}), None), + ("paint.contentAwareMove", json!({"offset": [100, -50], "structure": 3, "color": 4}), Some(Rect::new(300, 300, 340, 340))), + ("paint.patternStamp", json!({"points": [[200, 200]], "size": 16, "hardness": 100, "pattern": "Checkerboard", "scale": 100, "aligned": true}), None), + ]; + steps.push(("type.setStyle", json!({"layer": ty, "size": 40}), None)); + for (cmd, p, sel) in steps { + if let Some(r) = sel { + s.execute("select.rect", json!({"x": r.x0, "y": r.y0, "width": r.width(), "height": r.height()})).unwrap(); + } else if doc(&s).selection.is_some() { + s.execute("select.deselect", json!({})).unwrap(); + } + if cmd != "type.setStyle" { + let layer = doc(&s).layers.iter().find(|l| l.surface().is_some() && !doc(&s).is_background(l.id)).unwrap().id; + s.select_layer(layer).unwrap(); + } + let before = doc(&s); + s.execute(cmd, p).unwrap_or_else(|e| panic!("{cmd}: {e}")); + assert_eq!(s.damage_misses, 0, "{cmd}: its hint missed a changed tile"); + assert!(!same_pixels(&before, &doc(&s)), "{cmd} changed nothing"); + let d = damage(&s).unwrap_or_else(|| doc(&s).bounds()); + assert!(unchanged_outside(&before, &doc(&s), d), "{cmd}: pixels changed outside {d:?}"); + } +} diff --git a/crates/engine/src/cmd/redeye_cmds.rs b/crates/engine/src/cmd/redeye_cmds.rs index eefd1a37ba..02067d130b 100644 --- a/crates/engine/src/cmd/redeye_cmds.rs +++ b/crates/engine/src/cmd/redeye_cmds.rs @@ -243,9 +243,7 @@ fn red_eye(s: &mut Session, p: &Value) -> Result { } Ok((damage, total)) })?; - if let Some(st) = s.active_mut() { - st.last_damage = Some(damage); - } + s.hint_damage(damage); Ok(json!({ "damage": [damage.x0, damage.y0, damage.width(), damage.height()], "pixels": total })) } diff --git a/crates/engine/src/cmd/retouch_cmds.rs b/crates/engine/src/cmd/retouch_cmds.rs index da9183169f..bb754e13a0 100644 --- a/crates/engine/src/cmd/retouch_cmds.rs +++ b/crates/engine/src/cmd/retouch_cmds.rs @@ -193,9 +193,7 @@ fn run_stroke( // large layer would dominate the cost of a dab. f(&pre, surf, pre.selection.as_ref(), lock) })?; - if let Some(st) = s.active_mut() { - st.last_damage = Some(dmg); - } + s.hint_damage(dmg); Ok(dmg) } diff --git a/crates/engine/src/cmd/retouch_cmds/content_aware_move.rs b/crates/engine/src/cmd/retouch_cmds/content_aware_move.rs index 6e01926e3f..dbc73d9388 100644 --- a/crates/engine/src/cmd/retouch_cmds/content_aware_move.rs +++ b/crates/engine/src/cmd/retouch_cmds/content_aware_move.rs @@ -529,9 +529,7 @@ pub(super) fn content_aware_move(s: &mut Session, p: &Value) -> Result { } Ok(()) })?; - if let Some(st) = s.active_mut() { - st.last_damage = Some(damage); - } + s.hint_damage(damage); let mode = if plan.mode == Mode::Move { "move" } else { "extend" }; let mut out = json!({ "damage": damage_json(damage), "offset": [dx, dy], "mode": mode }); if plan.map.is_some() { diff --git a/crates/engine/src/cmd/retouch_cmds/patch.rs b/crates/engine/src/cmd/retouch_cmds/patch.rs index 557c173b4b..b84de04052 100644 --- a/crates/engine/src/cmd/retouch_cmds/patch.rs +++ b/crates/engine/src/cmd/retouch_cmds/patch.rs @@ -296,9 +296,7 @@ fn patch_content_aware(s: &mut Session, p: &Value) -> Result { *crate::channel_cmds::target_surface(doc, target.id, &p)?.0 = surf; Ok(()) })?; - if let Some(st) = s.active_mut() { - st.last_damage = Some(damage); - } + s.hint_damage(damage); Ok(json!({ "damage": damage_json(damage), "offset": [dx, dy], "contentAware": true })) }, ) diff --git a/crates/engine/src/cmd/retouch_cmds/pattern_stamp.rs b/crates/engine/src/cmd/retouch_cmds/pattern_stamp.rs index f13afc112e..4499715307 100644 --- a/crates/engine/src/cmd/retouch_cmds/pattern_stamp.rs +++ b/crates/engine/src/cmd/retouch_cmds/pattern_stamp.rs @@ -96,9 +96,7 @@ pub(super) fn run(s: &mut Session, p: &Value) -> Result { paint_aligned(surf, &st.stroke, sel, lock, &st.tile, st.placement(), st.mode, CMD) } })?; - if let Some(state) = s.active_mut() { - state.last_damage = Some(dmg); - } + s.hint_damage(dmg); Ok(json!({ "damage": damage_json(dmg), "phase": [st.phase.0, st.phase.1], diff --git a/crates/engine/src/cmd/retouch_cmds/remove.rs b/crates/engine/src/cmd/retouch_cmds/remove.rs index a7283d390b..777b1131f9 100644 --- a/crates/engine/src/cmd/retouch_cmds/remove.rs +++ b/crates/engine/src/cmd/retouch_cmds/remove.rs @@ -252,15 +252,11 @@ pub(super) fn remove(s: &mut Session, p: &Value) -> Result { *active = Some(r.1); Ok(r) })?; - if let Some(st) = s.active_mut() { - st.last_damage = Some(damage); - } + s.hint_damage(damage); return Ok(json!({ "damage": damage_json(damage), "layer": nid.0 })); } let damage = s.edit(label, |doc, _| replace(doc, Target { id, all_layers: false }, &p, &out, &hole))?; - if let Some(st) = s.active_mut() { - st.last_damage = Some(damage); - } + s.hint_damage(damage); Ok(json!({ "damage": damage_json(damage) })) }, ) diff --git a/crates/engine/src/cmd/type_cmds.rs b/crates/engine/src/cmd/type_cmds.rs index 1e103cf89f..58a2145409 100644 --- a/crates/engine/src/cmd/type_cmds.rs +++ b/crates/engine/src/cmd/type_cmds.rs @@ -694,8 +694,8 @@ fn with_text_layer(s: &mut Session, p: &Value, label: &str, f: impl FnOnce(&m } Ok((r, damage)) })?; - if let Some(st) = s.active_mut() { - st.last_damage = damage; + if let Some(d) = damage { + s.hint_damage(d); } Ok(r) } @@ -764,8 +764,8 @@ pub(crate) fn with_text_targets( } Ok(damage) })?; - if let Some(st) = s.active_mut() { - st.last_damage = damage; + if let Some(d) = damage { + s.hint_damage(d); } Ok(ids) } diff --git a/crates/engine/src/jobs.rs b/crates/engine/src/jobs.rs index bf3560eb9f..7a01d6786d 100644 --- a/crates/engine/src/jobs.rs +++ b/crates/engine/src/jobs.rs @@ -860,6 +860,10 @@ impl Session { self.coalesce_request = None; self.color_restrict = None; self.filter_mask_layer = None; + #[cfg(any(test, debug_assertions))] + { + self.hint_before = None; + } let pending = self.jobs.pending.take(); match (r, pending) { (Err(e), Some(job)) => { diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index c88ae3bfaf..17dbed3e43 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -277,6 +277,15 @@ pub struct Session { /// document, source read-only)`. The shell takes them with [`Session::take_held_open`]. held_opens: Vec<(u64, Document, bool)>, next_held_open: u64, + /// The document before the latest [`Session::edit`] of the running command, which + /// [`Session::hint_damage`] checks a hint against. Set by `edit`, taken by `hint_damage`, + /// dropped when the command ends. Debug and test builds only. + #[cfg(any(test, debug_assertions))] + hint_before: Option>, + /// Damage hints that missed a tile their edit changed ([`Session::hint_damage`]): each one a + /// command reporting too narrow a redraw area (#4053). Counted in debug and test builds only; + /// tests read it. + pub damage_misses: usize, } #[cfg(test)] diff --git a/crates/engine/src/new_layer.rs b/crates/engine/src/new_layer.rs index 4cba7f0c52..b102907ccc 100644 --- a/crates/engine/src/new_layer.rs +++ b/crates/engine/src/new_layer.rs @@ -58,7 +58,6 @@ pub fn run(s: &mut Session, p: &Value) -> Result { *active = Some(id); Ok(id) })?; - crate::layer_multi_cmds::note_inert_insert(s, id); Ok(json!({ "layer": id.0 })) } diff --git a/crates/engine/src/session/execute.rs b/crates/engine/src/session/execute.rs index 69c389f0c6..bc70ed2739 100644 --- a/crates/engine/src/session/execute.rs +++ b/crates/engine/src/session/execute.rs @@ -248,6 +248,8 @@ impl Session { // report its own, tighter damage after this. let damage = layer_multi_cmds::step_damage(&before, &st.doc); let keep_clean = st.keeps_clean(&before); + #[cfg(any(test, debug_assertions))] + let hint_before = before.clone(); let key = self.coalesce_request.clone(); let st = self.active_mut().ok_or(EngineError::NoDocument)?; let layers = st.layer_target(); @@ -268,6 +270,10 @@ impl Session { st.bump_revision(keep_clean); st.last_damage = damage; self.samplers_removed += samplers_removed; + #[cfg(any(test, debug_assertions))] + { + self.hint_before = Some(hint_before); + } if damage.is_none_or(|d| !d.is_empty()) { // Free the mask early; a stale entry would only miss (it is keyed by pixels). self.subject_cache = Default::default(); @@ -275,4 +281,26 @@ impl Session { } Ok(r) } + + /// A command's own rect for the pixels its latest [`Session::edit`] changed: sub-tile precise + /// for a brush dab, where the edit's own damage ([`layer_multi_cmds::step_damage`]) has tile + /// precision. The redraw area becomes the intersection of the two, so a hint can only narrow + /// it (or bound a change `step_damage` can't). In debug and test builds, a hint that misses a + /// tile the edit changed is counted in [`Session::damage_misses`] and logged; it never panics. + pub fn hint_damage(&mut self, rect: photocraft_geom::Rect) { + #[cfg(any(test, debug_assertions))] + if let Some(before) = self.hint_before.take() + && let Some(st) = self.active() + && st.doc.id == before.id + { + let misses = layer_multi_cmds::hint_misses(&before, &st.doc, rect); + if misses > 0 { + let step = st.history.undo_label().unwrap_or("?"); + eprintln!("warning: `{step}` hinted damage {rect:?} that misses {misses} changed tile(s); the canvas may not redraw them"); + self.damage_misses += misses; + } + } + let Some(st) = self.active_mut() else { return }; + st.last_damage = Some(st.last_damage.map_or(rect, |d| d.intersect(&rect))); + } } diff --git a/crates/engine/src/session/layers.rs b/crates/engine/src/session/layers.rs index b8ea5ea89d..2e6009d16f 100644 --- a/crates/engine/src/session/layers.rs +++ b/crates/engine/src/session/layers.rs @@ -1,5 +1,5 @@ use super::history::fix_selection; -use super::{Arc, EngineError, LayerId, Result, Session, channel_cmds, type_cmds}; +use super::{Arc, EngineError, LayerId, Result, Session, channel_cmds, layer_multi_cmds, type_cmds}; impl Session { /// Replace the active document's active layer without creating a history step. @@ -83,7 +83,8 @@ impl Session { if clean { st.saved_revision = st.revision; } - st.last_damage = None; + // Only the refreshed layers' caches changed: their changed tiles. + st.last_damage = layer_multi_cmds::step_damage(&snapshot, &st.doc); } } busy diff --git a/crates/engine/tests/it/type_format_targets.rs b/crates/engine/tests/it/type_format_targets.rs index 54e7d3f8af..e5f13ffd48 100644 --- a/crates/engine/tests/it/type_format_targets.rs +++ b/crates/engine/tests/it/type_format_targets.rs @@ -87,8 +87,10 @@ fn selected_type_properties_preserve_each_layers_other_attributes_and_one_histor if indices.contains(&i) { assert_ne!(actual.psd_raw, before[i].psd_raw, "editable PSD data refreshed"); let damage = s.active().unwrap().last_damage.unwrap(); + // The pixels the layer draws, before and after (the cache's tiles, narrowed to the + // layer's extents by the edit's own damage, #9329). for t in [&before[i], &actual] { - let bounds = t.cache.as_ref().unwrap().tile_bounds(); + let bounds = t.cache.as_ref().unwrap().content_bounds(); assert_eq!(damage.intersect(&bounds), bounds); } } diff --git a/crates/ui-egui/examples/perf_scenarios.rs b/crates/ui-egui/examples/perf_scenarios.rs index 4ea1c1e87b..9572453c75 100644 --- a/crates/ui-egui/examples/perf_scenarios.rs +++ b/crates/ui-egui/examples/perf_scenarios.rs @@ -102,6 +102,10 @@ struct Bench { errors: Vec<(String, String)>, /// The row being timed, so a panic inside it is reported against that row. current: Option, + /// Pixels the last `refresh` redrew (`Refresh.px`; the rect's area on the CPU). + last_px: std::cell::Cell, + /// Refreshed pixels per sample, for rows that report them (#9329); `record` prints the p50. + px: std::cell::RefCell>, } impl Bench { @@ -138,11 +142,12 @@ impl Bench { let t = Instant::now(); match &self.gpu { Some((g, rs)) => { - g.refresh(doc.id.0, doc, damage, None); + self.last_px.set(g.refresh(doc.id.0, doc, damage, None).px); let _ = rs.device.poll(eframe::wgpu::PollType::Wait { submission_index: None, timeout: None }); } None => { let r = damage.unwrap_or(doc.bounds()).intersect(&doc.bounds()); + self.last_px.set(if r.is_empty() { 0 } else { r.size().area() }); if !r.is_empty() { std::hint::black_box(photocraft_compose::render(doc, r)); } @@ -190,10 +195,13 @@ impl Bench { } self.current = Some(name.to_string()); self.sampler.reset(); + self.px.borrow_mut().clear(); let mut samples = Vec::new(); let mut run = || -> Res<()> { if warm { f(self, s, 0)?; + // `refresh px` describes only the timed runs. + self.px.borrow_mut().clear(); } for i in 0..reps.max(1) { samples.push(f(self, s, i + 1)?); @@ -216,6 +224,11 @@ impl Bench { samples.len(), r["peak_rss_bytes"].as_u64().unwrap_or(0) >> 20, ); + let mut px = std::mem::take(&mut *self.px.borrow_mut()); + if !px.is_empty() { + px.sort_unstable(); + println!("{name:<52} refresh px {}", px[px.len() / 2]); + } self.rows.push(r); } Err(e) => { @@ -368,6 +381,8 @@ fn main() { rows: Vec::new(), errors: Vec::new(), current: None, + last_px: std::cell::Cell::new(0), + px: std::cell::RefCell::new(Vec::new()), }; println!("perf_scenarios: {} mode, {} reps, {}", if quick { "quick" } else { "full" }, reps, adapter.as_deref().unwrap_or("CPU canvas")); let t_all = Instant::now(); @@ -375,7 +390,7 @@ fn main() { // A panic in one group (a crash the scenario found) is reported against the row that was // running, and the other groups still run. type Group = fn(&mut Bench, &Sizes); - let groups: [(&str, Group); 7] = [ + let groups: [(&str, Group); 8] = [ ("layered", layered_scenarios), ("many layers", many_layer_scenarios), ("full-canvas layers", full_canvas_layer_scenarios), @@ -383,6 +398,7 @@ fn main() { ("long operations", job_scenarios), ("algorithms", algorithm_scenarios), ("huge document", huge_document_scenarios), + ("redraw area", redraw_area_scenarios), ]; let use_gpu = b.gpu.is_some(); if let Some(wanted) = arg(&args, "--group") @@ -952,6 +968,70 @@ fn background_job_scenarios(b: &mut Bench, sz: &Sizes) { if b.wanted(frame) { frame_scenario(b, frame, w, h); } + let full = "app frame: full CPU refresh of a 24 MP document"; + if b.wanted(full) { + full_refresh_frame_scenario(b, full, w, h); + } +} + +/// #9328: the whole app (kittest harness, CPU canvas) with a 24 MP document; each sample toggles +/// a layer's visibility and steps one frame, and every frame recomposites all 24 MP on the CPU. +/// Always 10 samples, whatever `--reps` says. +/// +/// The stack is the photo plus three full-canvas copies in blend modes, and the top copy toggles. +/// Toggling the photo's only layer (the first version) measured 3.4 ms: with it hidden no layer +/// is visible, so the reduced composite's one-colour shortcut (`compose::uniform`) draws the whole +/// canvas as one pixel. Here the photo stays visible in every frame, its noisy tiles are never one +/// colour, and the toggled layer's damage is the whole canvas. Each frame checks that the canvas +/// refresh it recorded was a CPU one covering the whole document (within the reduction's rounding). +fn full_refresh_frame_scenario(b: &mut Bench, name: &str, w: u32, h: u32) { + let mut s = match open_photo(w, h) { + Ok(s) => s, + Err(e) => return b.errors.push((name.into(), e)), + }; + let built = (|| -> Res { + let photo = active_layer(&s)?; + for (blend, opacity) in [("Multiply", 0.7), ("Screen", 0.5), ("Overlay", 0.6)] { + exec(&mut s, "layer.duplicate", json!({"layer": photo}))?; + let copy = active_layer(&s)?; + if copy == photo { + return Err("layer.duplicate did not select the copy".into()); + } + exec(&mut s, "layer.setProps", json!({"layer": copy, "blend": blend, "opacity": opacity}))?; + } + active_layer(&s) + })(); + let top = match built { + Ok(l) => l, + Err(e) => return b.errors.push((name.into(), e)), + }; + 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(s, photocraft_ui_egui::Services::default()) + }); + harness.run_steps(6); + b.current = Some(name.to_string()); + b.sampler.reset(); + let mut samples = Vec::new(); + let pixels = u64::from(w) * u64::from(h); + let run = (|| -> Res<()> { + for i in 0..10 { + harness.state_mut().run("layer.setProps", json!({"layer": top, "visible": i % 2 == 1}))?; + harness.state_mut().perf.last_refresh_px = 0; + let t = Instant::now(); + harness.step(); + samples.push(ms(t)); + // A damage refresh counts its reduced texels times the factor squared, which can fall + // a few rows and columns short of the document when the factor doesn't divide it. + let perf = &harness.state().perf; + if !matches!(perf.last_refresh, "full" | "rect") || perf.last_refresh_px.saturating_mul(100) < pixels.saturating_mul(95) { + return Err(format!("frame {i} refreshed {} px ({}), not the whole {pixels} px on the CPU", perf.last_refresh_px, perf.last_refresh)); + } + } + Ok(()) + })(); + let st = std::mem::take(&mut harness.state_mut().session); + b.record(name, &st, &samples, run); } /// The whole app (kittest harness, CPU canvas) with a 24 MP document: frame times while a @@ -1199,3 +1279,99 @@ fn liquify_preview_scenario(b: &mut Bench) { } b.record(name, &Session::new(), &samples, Ok(())); } + +/// The share of the canvas `r` covers. +fn coverage(doc: &photocraft_doc::Document, r: Rect) -> f64 { + r.intersect(&doc.bounds()).size().area() as f64 / doc.bounds().size().area().max(1) as f64 +} + +/// The content bounds of `layer`'s pixels. +fn layer_bounds(s: &Session, layer: u64) -> Res { + let doc = &s.active().ok_or("no document")?.doc; + doc.layer(photocraft_doc::LayerId(layer)).and_then(|l| l.surface()).map(|p| p.content_bounds()).ok_or_else(|| "layer has no pixels".to_string()) +} + +/// A document with one painted layer covering the whole canvas; returns it and the layer. +fn full_layer_doc(w: u32, h: u32, depth: u32) -> Res<(Session, u64)> { + let mut s = Session::new(); + exec(&mut s, "file.new", json!({"width": w, "height": h, "depth": depth, "background": "white"}))?; + exec(&mut s, "layer.new.layer", json!({"name": "full"}))?; + exec(&mut s, "edit.fill", json!({"color": "#6080a0"}))?; + let layer = active_layer(&s)?; + let doc = s.active().ok_or("no document")?.doc.clone(); + let cover = coverage(&doc, layer_bounds(&s, layer)?); + if cover < 0.95 { + return Err(format!("path check: the painted layer covers {:.1} % of the canvas, not >= 95 %", cover * 100.0)); + } + Ok((s, layer)) +} + +/// #9329 PR A: the redraw area of edits on a full-canvas layer, where redrawing the whole +/// document is the expensive cliff. Each row checks it hits that path and prints `refresh px`. +fn redraw_area_scenarios(b: &mut Bench, sz: &Sizes) { + let undo = "undo a brush dab on a full-canvas 16-bit 15000x10000 layer + refresh"; + if b.wanted(undo) { + b.current = Some(undo.to_string()); + let (w, h) = sz.huge16; + match full_layer_doc(w, h, 16) { + Err(e) => b.errors.push((undo.to_string(), e)), + Ok((mut s, _)) => { + let _ = b.refresh(&s, true); + // Two 1.2 GB layers and, before the fix, a 150 MP refresh per sample. + let reps = b.reps.min(5); + b.time(undo, &mut s, reps, true, |b, s, i| { + let (x, y) = (200 + (i as u32 * 97) % (w - 400), 200 + (i as u32 * 61) % (h - 400)); + exec(s, "paint.stroke", json!({"points": [[x, y]], "size": 60, "hardness": 0.8, "color": "#c03020"}))?; + b.refresh(s, false)?; + let before = s.active().ok_or("no document")?.doc.clone(); + let t = Instant::now(); + exec(s, "edit.undo", json!({}))?; + let took = ms(t) + b.refresh(s, false)?; + b.px.borrow_mut().push(b.last_px.get()); + let after = s.active().ok_or("no document")?.doc.clone(); + if photocraft_engine::layer_multi_cmds::step_damage(&before, &after).is_some_and(|r| r.is_empty()) { + return Err("path check: the undo changed no pixels".into()); + } + Ok(took) + }); + } + } + } + + let fill = "fill a 2 % selection on a full-canvas 24 MP layer + refresh"; + if b.wanted(fill) { + b.current = Some(fill.to_string()); + let (w, h) = sz.big_photo; + let built = full_layer_doc(w, h, 8).and_then(|(mut s, layer)| { + exec(&mut s, "select.rect", json!({"x": w / 3, "y": h / 3, "width": w / 8, "height": h / 8}))?; + let doc = s.active().ok_or("no document")?.doc.clone(); + let sel = doc.selection.as_ref().map(|m| m.content_bounds()).ok_or("no selection")?; + let share = coverage(&doc, sel); + if share > 0.02 { + return Err(format!("path check: the selection covers {:.2} % of the canvas, not <= 2 %", share * 100.0)); + } + Ok((s, layer)) + }); + match built { + Err(e) => b.errors.push((fill.to_string(), e)), + Ok((mut s, _)) => { + let _ = b.refresh(&s, true); + let reps = b.reps; + b.time(fill, &mut s, reps, true, |b, s, i| { + // The colour alternates, so every sample changes pixels. + let color = if i % 2 == 0 { "#20c040" } else { "#c02040" }; + let before = s.active().ok_or("no document")?.doc.clone(); + let t = Instant::now(); + exec(s, "edit.fill", json!({"color": color}))?; + let took = ms(t) + b.refresh(s, false)?; + b.px.borrow_mut().push(b.last_px.get()); + let after = s.active().ok_or("no document")?.doc.clone(); + if photocraft_engine::layer_multi_cmds::step_damage(&before, &after).is_some_and(|r| r.is_empty()) { + return Err("path check: the fill changed no pixels".into()); + } + Ok(took) + }); + } + } + } +} diff --git a/crates/ui-egui/src/app.rs b/crates/ui-egui/src/app.rs index e5f987c71d..d12c85d005 100644 --- a/crates/ui-egui/src/app.rs +++ b/crates/ui-egui/src/app.rs @@ -122,6 +122,7 @@ impl PhotocraftApp { synthetic_reinject: false, synthetic_snapshot: None, automation_input: false, + run_depth: 0, paused_playbacks: Default::default(), channel_thumbs: None, channel_views: HashMap::new(), @@ -174,6 +175,7 @@ impl PhotocraftApp { perf: Default::default(), stroke_trace: stroke_trace::StrokeTrace::from_env(), render_path: Default::default(), + slow_frames: Default::default(), prefs_rt: Default::default(), updates_rt: Default::default(), discard: None, @@ -256,11 +258,9 @@ impl eframe::App for PhotocraftApp { if self.custom_titlebar { titlebar::release_after_os_resize(ctx, raw_input); } - if self.stroke_trace.enabled() { - // The OS's moves and the control channel's (injected just above). - let moves = raw_input.events.iter().filter(|e| matches!(e, egui::Event::PointerMoved(_))).count(); - self.stroke_trace.begin_frame(frame_start, moves); - } + // The OS's moves and the control channel's (injected just above). + let moves = raw_input.events.iter().filter(|e| matches!(e, egui::Event::PointerMoved(_))).count(); + self.perf.stage.start(frame_start, moves); } fn ui(&mut self, ui: &mut egui::Ui, frame: &mut eframe::Frame) { diff --git a/crates/ui-egui/src/app/commands.rs b/crates/ui-egui/src/app/commands.rs index 9e7e986d69..14cce82bb8 100644 --- a/crates/ui-egui/src/app/commands.rs +++ b/crates/ui-egui/src/app/commands.rs @@ -35,6 +35,13 @@ impl PhotocraftApp { } pub(crate) fn run_command(&mut self, id: &str, params: Value) -> Result { + self.run_depth += 1; + let r = self.run_command_at_depth(id, params); + self.run_depth -= 1; + r + } + + fn run_command_at_depth(&mut self, id: &str, params: Value) -> Result { if !vector_ui::cancel_stale(self) { return Err(self.ui.status.clone()); } @@ -121,6 +128,9 @@ impl PhotocraftApp { self.sync_command_side_effects(id, &r, creates_active_mask, creates_active_vector_mask, creates_adjustment_or_fill, path_mask); self.perf.last_command = id.to_string(); self.perf.command_ms = gpu_canvas::now_ms() - t0; + if self.run_depth == 1 { + self.perf.stage.command(id, self.perf.command_ms); + } match &r { Ok(_) => { if self.ui.status_error { diff --git a/crates/ui-egui/src/app/control.rs b/crates/ui-egui/src/app/control.rs index 425742d519..1032873c56 100644 --- a/crates/ui-egui/src/app/control.rs +++ b/crates/ui-egui/src/app/control.rs @@ -1,4 +1,4 @@ -use super::{ControlRequest, PhotocraftApp, Receiver}; +use super::{ControlRequest, PhotocraftApp, Receiver, gpu_canvas}; impl PhotocraftApp { /// Attach a control channel (requests arrive from a transport thread: TCP, stdin, tests). @@ -7,6 +7,19 @@ impl PhotocraftApp { self } + /// Close this frame's stage record (`frame_stages`): keep it if it was slow, and hand it to + /// the stroke trace. `ui_ms` is the UI build's time. + pub(crate) fn finish_frame(&mut self, ui_ms: f64) { + let mut stages = std::mem::take(&mut self.perf.stage); + stages.ui_ms = ui_ms; + let stages = self.slow_frames.finish(stages, gpu_canvas::now_ms(), self.perf.gpu, self.perf.gpu_ms); + let info = &self.perf.gpu_info; + self.stroke_trace.frame(&stages, self.live_stroke.is_some(), || { + let gpu = format!("{} {}", info.backend, info.adapter).trim().to_string(); + if info.present.is_empty() { gpu } else { format!("{gpu}, {}", info.present) } + }); + } + pub(crate) fn drain_control(&mut self, ctx: &egui::Context) { let Some(rx) = self.control_rx.take() else { return }; while let Ok(req) = rx.try_recv() { diff --git a/crates/ui-egui/src/app/frame.rs b/crates/ui-egui/src/app/frame.rs index d19eb31f7a..df0de6ff0b 100644 --- a/crates/ui-egui/src/app/frame.rs +++ b/crates/ui-egui/src/app/frame.rs @@ -167,13 +167,67 @@ impl PhotocraftApp { self.automation_input = false; // No editor was drawn this pass: end its IME context rather than keep a stale owner. type_tool::sync_ime(self, &ctx); + // The frame's stage record was started with its input: close it here too. + self.finish_frame(0.0); return; } let t0 = gpu_canvas::now_ms(); - self.perf.frame_refresh_ms = 0.0; + self.perf.stage.ui_start(t0, frame.info().cpu_usage); // A software adapter or CPU mode chosen at startup is every document's reason. crate::render_path::frame(self); - self.stroke_trace.cpu_prev(frame.info().cpu_usage); + // Every frame that started its stage record closes it, whichever way `ui_body` returns. + let drawn = self.ui_body(ui); + let ui_ms = gpu_canvas::now_ms() - t0; + // A GPU measurement lands a frame or two after the refresh it belongs to. Refreshes + // carry them out, but the last refresh before the app settles has none following it, so + // drain the timer here too or its number is never reported. Drain it either way: a + // measurement left sitting would surface later against an unrelated refresh. Report it + // only while the canvas is still on the GPU, since a CPU refresh clears `gpu_ms` + // precisely so a GPU number is never shown as that refresh's. + if let Some(gpu) = &self.gpu + && let Some(t) = gpu.take_gpu_timing() + && self.perf.last_refresh.starts_with("gpu") + { + self.perf.record_gpu(t); + } + self.perf.frame(ui_ms); + self.finish_frame(ui_ms); + if !drawn { + return; + } + // This pass handled its input: the in-flight synthetic batch is delivered once and the + // next step may be injected on the next frame. + self.finish_synthetic_pass(); + // Synthetic input is injected one press/release step per frame: keep frames coming until + // the queue is empty, then release control replies waiting on it. + // Only a pass that reached the end of the UI and delivered the in-flight batch counts as + // settled input; an early-returning pass keeps the batch (and the waiting replies). + if !self.synthetic.is_empty() || self.synthetic_batch.is_some() { + ctx.request_repaint(); + } else if !self.input_waiters.is_empty() { + for w in self.input_waiters.drain(..) { + let _ = w.send(serde_json::json!({"ok": true, "result": null})); + } + } + // A menu or dialog can start a filter after `jobs_ui::tick` ran in this frame. Keep + // polling it even when the platform would otherwise put the event loop to sleep. + jobs_ui::keep_polling(self, &ctx); + // Menus and buttons in this frame may have asked for a file dialog. + self.poll_file_dialog(&ctx, Some(frame)); + // The Type tool's IME descriptor is the last writer of this pass: every panel and the + // canvas have finished mutating the model, and an absent/focus-moved editor ends here. + type_tool::sync_ime(self, &ctx); + // Issue any due `ui.screenshot` now that this pass's state is settled: a queued request + // that has waited its passes and animation delay captures exactly this frame. + self.issue_captures(&ctx); + // Last, after everything drawn this frame: a lost window device can't draw it (#3438). + gpu_status::check_window(self, &ctx); + } + + /// The frame's UI, between `ui_start` and `finish_frame`. False when the screen picker's + /// wait modal took the frame (its input, IME and window check already handled). + fn ui_body(&mut self, ui: &mut egui::Ui) -> bool { + let ctx = ui.ctx().clone(); // A text field's right-click menu, before any field is drawn (text_field_menu.rs). text_field_menu::show(&ctx); // The wheel goes to a focused value field before the canvas or a panel sees it (#5473). @@ -204,7 +258,7 @@ impl PhotocraftApp { self.automation_input = false; type_tool::sync_ime(self, &ctx); gpu_status::check_window(self, &ctx); - return; + return false; } if screen_picker::busy(&ctx) { ui.disable(); @@ -299,54 +353,6 @@ impl PhotocraftApp { egui::Modal::new(egui::Id::unique("screen-color-wait")).frame(egui::Frame::NONE).backdrop_color(egui::Color32::TRANSPARENT).show(&ctx, |_| {}); ctx.set_cursor_icon(egui::CursorIcon::Wait); } - let ui_ms = gpu_canvas::now_ms() - t0; - // A GPU measurement lands a frame or two after the refresh it belongs to. Refreshes - // carry them out, but the last refresh before the app settles has none following it, so - // drain the timer here too or its number is never reported. Drain it either way: a - // measurement left sitting would surface later against an unrelated refresh. Report it - // only while the canvas is still on the GPU, since a CPU refresh clears `gpu_ms` - // precisely so a GPU number is never shown as that refresh's. - if let Some(gpu) = &self.gpu - && let Some(t) = gpu.take_gpu_timing() - && self.perf.last_refresh.starts_with("gpu") - { - self.perf.record_gpu(t); - } - self.perf.frame(ui_ms); - if self.stroke_trace.enabled() { - self.stroke_trace.canvas(self.perf.frame_refresh_ms); - let info = &self.perf.gpu_info; - self.stroke_trace.end_frame(ui_ms, self.live_stroke.is_some(), || { - let gpu = format!("{} {}", info.backend, info.adapter).trim().to_string(); - if info.present.is_empty() { gpu } else { format!("{gpu}, {}", info.present) } - }); - } - // This pass handled its input: the in-flight synthetic batch is delivered once and the - // next step may be injected on the next frame. - self.finish_synthetic_pass(); - // Synthetic input is injected one press/release step per frame: keep frames coming until - // the queue is empty, then release control replies waiting on it. - // Only a pass that reached the end of the UI and delivered the in-flight batch counts as - // settled input; an early-returning pass keeps the batch (and the waiting replies). - if !self.synthetic.is_empty() || self.synthetic_batch.is_some() { - ctx.request_repaint(); - } else if !self.input_waiters.is_empty() { - for w in self.input_waiters.drain(..) { - let _ = w.send(serde_json::json!({"ok": true, "result": null})); - } - } - // A menu or dialog can start a filter after `jobs_ui::tick` ran in this frame. Keep - // polling it even when the platform would otherwise put the event loop to sleep. - jobs_ui::keep_polling(self, &ctx); - // Menus and buttons in this frame may have asked for a file dialog. - self.poll_file_dialog(&ctx, Some(frame)); - // The Type tool's IME descriptor is the last writer of this pass: every panel and the - // canvas have finished mutating the model, and an absent/focus-moved editor ends here. - type_tool::sync_ime(self, &ctx); - // Issue any due `ui.screenshot` now that this pass's state is settled: a queued request - // that has waited its passes and animation delay captures exactly this frame. - self.issue_captures(&ctx); - // Last, after everything drawn this frame: a lost window device can't draw it (#3438). - gpu_status::check_window(self, &ctx); + true } } diff --git a/crates/ui-egui/src/canvas/background_redraw/tests.rs b/crates/ui-egui/src/canvas/background_redraw/tests.rs index 55afe9ac6b..b13692d83c 100644 --- a/crates/ui-egui/src/canvas/background_redraw/tests.rs +++ b/crates/ui-egui/src/canvas/background_redraw/tests.rs @@ -249,8 +249,9 @@ fn an_edit_damaging_most_of_a_large_document_is_drawn_in_the_background_too() { let Some(mut g) = gpu_app() else { return }; let doc = cpu_only_document(g.harness.state_mut(), 2600, 1700); settle(&mut g); - // Filling part of the Blend If layer can change the composite anywhere: the whole canvas. - g.harness.state_mut().run("select.rect", json!({"x": 100, "y": 100, "width": 300, "height": 300})).expect("select"); + // Filling most of the Blend If layer touches tiles all over it: the whole canvas (a fill of a + // small part damages only its own tiles, #9329). + g.harness.state_mut().run("select.rect", json!({"x": 100, "y": 100, "width": 2500, "height": 1500})).expect("select"); g.harness.step(); g.harness.state_mut().run("edit.fill", json!({"color": "#22ee22"})).expect("fill"); assert_eq!(g.harness.state().session.active().expect("document").last_damage, Some(Rect::new(0, 0, 2600, 1700))); diff --git a/crates/ui-egui/src/canvas/gpu_canvas/perf.rs b/crates/ui-egui/src/canvas/gpu_canvas/perf.rs index b1fedc6a62..58b12774b2 100644 --- a/crates/ui-egui/src/canvas/gpu_canvas/perf.rs +++ b/crates/ui-egui/src/canvas/gpu_canvas/perf.rs @@ -68,9 +68,9 @@ pub struct Perf { pub spans: std::collections::BTreeMap<&'static str, f64>, #[serde(skip)] max_decay: f64, - /// Composite and upload time of this frame's canvas refreshes, in ms (the stroke trace). + /// This frame's stages, gathered as it runs (see `frame_stages`). #[serde(skip)] - pub(crate) frame_refresh_ms: f64, + pub(crate) stage: crate::frame_stages::FrameStages, } /// What the app renders with and why (Help › System Info, Preferences › Performance, the `perf` @@ -214,7 +214,7 @@ pub fn backend_name(b: wgpu::Backend) -> &'static str { impl Perf { pub fn record(&mut self, kind: &'static str, px: u64, composite_ms: f64, upload_ms: f64) { - self.frame_refresh_ms += composite_ms + upload_ms; + self.stage.refresh(kind, px, composite_ms, upload_ms); self.last_refresh = kind; self.last_refresh_px = px; self.composite_ms = composite_ms; diff --git a/crates/ui-egui/src/canvas/strokes.rs b/crates/ui-egui/src/canvas/strokes.rs index 560c4e5aef..9388c478c1 100644 --- a/crates/ui-egui/src/canvas/strokes.rs +++ b/crates/ui-egui/src/canvas/strokes.rs @@ -466,7 +466,7 @@ pub(super) fn feed_live_stroke(app: &mut PhotocraftApp) { } } } - app.stroke_trace.stroke(crate::gpu_canvas::now_ms() - started); + app.perf.stage.stroke_ms += crate::gpu_canvas::now_ms() - started; } /// The points the live retouching stroke has rendered. @@ -663,7 +663,7 @@ pub(super) fn commit_brush_stroke(app: &mut PhotocraftApp, d: &Drag) { } let started = crate::gpu_canvas::now_ms(); let result = app.run(stroke_command(app, d.tool), p); - app.stroke_trace.stroke(crate::gpu_canvas::now_ms() - started); + app.perf.stage.stroke_ms += crate::gpu_canvas::now_ms() - started; app.session.clear_live_stroke(); let Ok(r) = result else { return }; let after = app.session.documents().iter().find(|st| st.doc.id == doc).map(|st| (st.revision, st.doc.clone())); diff --git a/crates/ui-egui/src/canvas/tests.rs b/crates/ui-egui/src/canvas/tests.rs index f8e7a74660..80b9c502a9 100644 --- a/crates/ui-egui/src/canvas/tests.rs +++ b/crates/ui-egui/src/canvas/tests.rs @@ -175,15 +175,15 @@ fn inline_filter_preview_is_composited_and_recorded_once() { app.background_jobs = false; app.run("file.new", json!({"width": 320, "height": 240})).unwrap(); crate::filter_dialog::open(&mut app, "filter.stylize.solarize").unwrap(); - app.perf.frame_refresh_ms = 0.0; + app.perf.stage = Default::default(); let (_, _, size) = ensure_filter_preview(&mut app, 0, &egui::Context::default()).expect("a preview"); assert_eq!(size, [320, 240]); assert_eq!(app.perf.last_refresh, "filter-preview"); assert_eq!(app.perf.last_refresh_px, 320 * 240); - assert_eq!(app.perf.frame_refresh_ms, app.perf.composite_ms + app.perf.upload_ms, "one record per preview"); + assert_eq!(app.perf.stage.composite_ms + app.perf.stage.upload_ms, app.perf.composite_ms + app.perf.upload_ms, "one record per preview"); // Unchanged request: nothing is recomputed or recorded again. ensure_filter_preview(&mut app, 0, &egui::Context::default()).expect("the same preview"); - assert_eq!(app.perf.frame_refresh_ms, app.perf.composite_ms + app.perf.upload_ms); + assert_eq!(app.perf.stage.composite_ms + app.perf.stage.upload_ms, app.perf.composite_ms + app.perf.upload_ms); } /// #6326: a filter's live preview is never coarser than the screen. On a document big enough @@ -1735,12 +1735,20 @@ fn mask_view_uploads_each_live_stroke_before_release() { assert_eq!(app.session.documents()[0].doc.layer(layer).unwrap(), &original); crate::channel_view::ensure(&mut app, &ctx, 0); let mut delta = ctx.tex_manager().write().take_delta(); - let egui::ImageData::Color(image) = &delta.set.get(&tex).unwrap()[0].image; + let upload = &delta.set.get(&tex).unwrap()[0]; + let egui::ImageData::Color(image) = &upload.image; let st = &app.session.documents()[0]; + // Undo redraws only the tiles the stroke changed (#9329): a partial upload of them. + let f = factor as i32; + let pos = upload.pos.expect("undo uploads only the changed tiles, not the whole mask"); + let r = DRect::new(pos[0] as i32 * f, pos[1] as i32 * f, (pos[0] + image.size[0]) as i32 * f, (pos[1] + image.size[1]) as i32 * f) + .intersect(&st.doc.bounds()); + // It covers the stroke's span (x 10..70 along y 30, size 12), so it + // proves coverage and not only values. + assert!(r.contains_rect(&DRect::new(10, 26, 70, 34)), "undo covers the stroke: {r:?}"); assert_eq!( image.pixels, - crate::channel_view::render(&st.doc, &st.channel_view, st.doc.bounds(), factor, crate::channel_view::Look::default(), st.active_layer) - .unwrap(), + crate::channel_view::render(&st.doc, &st.channel_view, r, factor, crate::channel_view::Look::default(), st.active_layer).unwrap(), "undo restores the shown mask" ); delta.clear(); diff --git a/crates/ui-egui/src/canvas/textures.rs b/crates/ui-egui/src/canvas/textures.rs index 99f9e77096..f11c4338e9 100644 --- a/crates/ui-egui/src/canvas/textures.rs +++ b/crates/ui-egui/src/canvas/textures.rs @@ -466,10 +466,12 @@ pub(super) fn ensure_gpu(app: &mut PhotocraftApp, ctx: &egui::Context, idx: usiz // The texture is shared by every display: only what it stores counts, not the monitor. let display_key = texture_key(display.as_deref()); let seen = app.canvases.get(&(id, GPU_OUTPUT)).map(|c| (c.revision, c.preview_key)); + let damage_start = crate::gpu_canvas::now_ms(); let mut damage = seen.and_then(|seen| damage_since(app, idx, seen, (revision, raw_key), display_key, last_damage)); if let Some(region) = damage { damage = Some(crate::blend_preview::clip_damage(app, id, revision, raw_key, region, blend_visible)); } + app.perf.stage.damage_ms += crate::gpu_canvas::now_ms() - damage_start; // To an adjustment dialog's preview: only what the view shows now (the rest as it moves there). if damage.is_some() && let Some(r) = seen.filter(|s| s.0 == revision).and_then(|s| crate::adjust_preview::switch_region(app, id, revision, s.1 ^ display_key, raw_key)) @@ -551,7 +553,9 @@ pub(super) fn ensure_gpu(app: &mut PhotocraftApp, ctx: &egui::Context, idx: usiz return false; } let partial = present && damage.is_some(); + let budget_start = crate::gpu_canvas::now_ms(); gpu_budget(app, &gpu, idx, partial, visible); + app.perf.stage.damage_ms += crate::gpu_canvas::now_ms() - budget_start; let defer = crate::background_redraw::enabled(app); let r = gpu.refresh_with(id.0, &doc, if partial { damage } else { None }, display.as_deref(), defer); app.perf.gpu_layer_fit = gpu.layer_fit(); diff --git a/crates/ui-egui/src/control/inspect.rs b/crates/ui-egui/src/control/inspect.rs index 86b7d0d734..12b34034b4 100644 --- a/crates/ui-egui/src/control/inspect.rs +++ b/crates/ui-egui/src/control/inspect.rs @@ -88,7 +88,13 @@ pub fn inspect(app: &PhotocraftApp, ctx: &egui::Context) -> Value { "frame": app.frame, "session": photocraft_engine::inspect::session(&app.session), "document": app.session.active().map(photocraft_engine::inspect::document), - "perf": {"fps": app.fps, "timings": app.perf, "strokeTrace": app.stroke_trace.last, "renderPath": crate::render_path::inspect(app)}, + "perf": { + "fps": app.fps, + "timings": app.perf, + "strokeTrace": app.stroke_trace.last, + "renderPath": crate::render_path::inspect(app), + "slowFrames": app.slow_frames, + }, "brush": {"size": app.session.tools.brush.size, "hardness": app.session.tools.brush.hardness, "opacity": app.session.tools.brush.opacity}, "distort": app.distort.describe(), "jobs": crate::jobs_ui::inspect(app), diff --git a/crates/ui-egui/src/control/tests.rs b/crates/ui-egui/src/control/tests.rs index c7b9896e2f..1db2ec45ad 100644 --- a/crates/ui-egui/src/control/tests.rs +++ b/crates/ui-egui/src/control/tests.rs @@ -28,6 +28,39 @@ fn ui_set_turns_the_stroke_trace_on_and_off_at_runtime() { assert!(!app.stroke_trace.enabled() && app.ui.tool != crate::state::Tool::Pencil, "a rejected call applies nothing"); } +#[test] +fn perf_output_includes_slow_frames() { + let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), crate::Services::default()); + let ctx = egui::Context::default(); + assert_eq!(call(&mut app, &ctx, "ui.inspect", json!({}))["result"]["perf"]["slowFrames"], json!([])); + app.slow_frames.set_threshold_ms(0.0); + app.run("file.new", json!({"width": 64, "height": 32})).unwrap(); + app.finish_frame(1.0); + let frames = call(&mut app, &ctx, "ui.inspect", json!({}))["result"]["perf"]["slowFrames"].clone(); + let last = frames.as_array().and_then(|f| f.last()).expect("a slow frame").clone(); + assert_eq!(last["command"], "file.new", "{last}"); + for key in ["t", "frameMs", "inputMs", "commandMs", "damageMs", "compositeMs", "uploadMs", "gpuMs", "presentMs", "refresh", "refreshPx", "path"] { + assert!(last.get(key).is_some(), "{key} in {last}"); + } + // A nested run (`actions.play` → its view steps through `App::run`) is inside the outer + // call's time: the frame counts the outer command once, not the steps again. + app.session.actions.list.push(photocraft_engine::actions_cmds::Action { + name: "Zoom".into(), + steps: vec![("view.zoomIn".into(), json!({})), ("view.zoomOut".into(), json!({}))], + disabled: Default::default(), + ..Default::default() + }); + let played = app.run("actions.play", json!({"action": "Zoom"})).unwrap(); + assert_eq!(played["ran"], 2, "{played}"); + let outer_ms = app.perf.command_ms; + app.finish_frame(1.0); + let frames = call(&mut app, &ctx, "ui.inspect", json!({}))["result"]["perf"]["slowFrames"].clone(); + let last = frames.as_array().and_then(|f| f.last()).expect("a slow frame").clone(); + assert_eq!(last["command"], "actions.play", "{last}"); + let command_ms = last["commandMs"].as_f64().unwrap(); + assert!((command_ms - outer_ms).abs() < 1e-9, "the outer run's {outer_ms} ms, once: {last}"); +} + #[test] fn camera_raw_control_updates_and_commits_or_cancels_an_open_raw() { use photocraft_raw::testgen::{DngSpec, mosaic, scene}; diff --git a/crates/ui-egui/src/frame_stages.rs b/crates/ui-egui/src/frame_stages.rs new file mode 100644 index 0000000000..c2b4fc5fb9 --- /dev/null +++ b/crates/ui-egui/src/frame_stages.rs @@ -0,0 +1,323 @@ +//! Where each frame's time goes (#9328), gathered every frame, and the slow ones kept. +//! +//! [`FrameStages`] is one frame's record. Each stage has one writer: the raw input hook starts +//! the frame, `App::run` adds commands, the GPU canvas path adds damage tracking and budgeting, +//! [`crate::gpu_canvas::Perf::record`] adds composites and uploads, and the canvas adds stroke +//! rendering. `App::ui` closes it ([`SlowFrames::finish`]). A frame over [`SLOW_FRAME_MS`] goes +//! into [`SlowFrames`], the last [`SLOW_FRAMES`] of them, which the control channel's `perf` +//! output lists as `slowFrames`; one `info` log line per second names them. The stroke trace +//! (`stroke_trace`) reads the same record, so the app times a frame in one place. +//! +//! On the web `now_ms()` is 0, so no frame is ever slow and the ring stays empty. + +use std::collections::VecDeque; + +/// A frame slower than this (ms) is kept and logged. +pub const SLOW_FRAME_MS: f64 = 50.0; +/// How many slow frames are kept. +pub const SLOW_FRAMES: usize = 64; +/// At most one slow-frame log line per this many ms. +const LOG_EVERY_MS: f64 = 1000.0; + +/// One frame's stages. Times in ms. +#[derive(Clone, Debug, Default, PartialEq, serde::Serialize)] +#[serde(rename_all = "camelCase")] +pub struct FrameStages { + /// Frame start (the raw input hook), from [`crate::gpu_canvas::now_ms`]. + pub t: f64, + /// The whole frame: from its start until its UI is built. Never less than the stages below + /// (a command run between frames counts toward the next one). + pub frame_ms: f64, + /// From the raw input hook to the start of the UI build (input handling, minus commands). + pub input_ms: f64, + /// Commands run through the app since the previous frame ended. + pub command_ms: f64, + /// The last of those commands ("" for none). + pub command: String, + /// Damage tracking and GPU memory budgeting on the GPU canvas path. + pub damage_ms: f64, + /// Canvas composites, CPU side (on the GPU path: encoding and submitting them). + pub composite_ms: f64, + /// Canvas conversions and uploads. + pub upload_ms: f64, + /// The GPU's own time for a recent composite, when the canvas is on the GPU and it reports one. + pub gpu_ms: Option, + /// eframe's CPU time for the previous frame minus that frame's UI build: tessellation and + /// GPU submission (the previous frame's, so it isn't part of `frame_ms`). + pub present_ms: Option, + /// The last canvas refresh's kind ("full", "rect", "gpu-full", …; "" for none). + pub refresh: &'static str, + /// Pixels the frame's canvas refreshes covered. + pub refresh_px: u64, + /// "gpu" or "cpu": who drew the canvas. + pub path: &'static str, + /// Building the UI (`App::ui`), the canvas refresh included. + #[serde(skip)] + pub ui_ms: f64, + /// Rendering stroke points (the stroke trace's `stroke`). + #[serde(skip)] + pub stroke_ms: f64, + /// Pointer moves the frame delivered. + #[serde(skip)] + pub moves: usize, + /// eframe's CPU time for the previous frame. + #[serde(skip)] + pub cpu_prev_ms: Option, + /// The raw input hook ran for this frame. + #[serde(skip)] + started: bool, +} + +impl FrameStages { + /// The raw input hook: the frame starts at `now` with `moves` pointer moves. + pub fn start(&mut self, now: f64, moves: usize) { + self.t = now; + self.moves = moves; + self.started = true; + } + + /// The UI build starts at `now`; `cpu_prev` is eframe's CPU time for the previous frame, in s. + pub fn ui_start(&mut self, now: f64, cpu_prev: Option) { + if !self.started { + self.start(now, 0); + } + self.input_ms = (now - self.t - self.command_ms).max(0.0); + self.cpu_prev_ms = cpu_prev.map(|s| f64::from(s) * 1000.0); + } + + /// A command `id` ran for `ms`. + pub fn command(&mut self, id: &str, ms: f64) { + self.command_ms += ms; + id.clone_into(&mut self.command); + } + + /// A canvas refresh of kind `kind` and `px` pixels. + pub fn refresh(&mut self, kind: &'static str, px: u64, composite_ms: f64, upload_ms: f64) { + self.composite_ms += composite_ms; + self.upload_ms += upload_ms; + self.refresh = kind; + self.refresh_px += px; + } + + /// The stages that make up `frame_ms` (`present_ms` belongs to the previous frame). + pub fn stage_ms(&self) -> f64 { + self.input_ms + self.command_ms + self.damage_ms + self.composite_ms + self.upload_ms + } + + /// The frame ends at `now`. + fn end(&mut self, now: f64) { + if !self.started { + self.t = now; + } + self.frame_ms = (now - self.t).max(self.stage_ms()); + } + + /// One line for the log. + pub fn line(&self) -> String { + let opt = |v: Option| v.map_or("-".to_string(), |v| format!("{v:.1}")); + let command = if self.command.is_empty() { String::new() } else { format!(" ({})", self.command) }; + format!( + "slow frame: {:.1} ms on the {} path: input {:.1}, command {:.1}{command}, damage {:.1}, composite {:.1}, upload {:.1}, gpu {}, present {}; refresh {} {} px", + self.frame_ms, + self.path, + self.input_ms, + self.command_ms, + self.damage_ms, + self.composite_ms, + self.upload_ms, + opt(self.gpu_ms), + opt(self.present_ms), + if self.refresh.is_empty() { "none" } else { self.refresh }, + self.refresh_px, + ) + } +} + +/// The last [`SLOW_FRAMES`] frames over the threshold. +#[derive(Debug)] +pub struct SlowFrames { + frames: VecDeque, + threshold_ms: f64, + /// The previous frame's UI build (for its `present_ms`). + prev_ui_ms: Option, + /// When the last log line went out, and the slow frames since that weren't logged. + logged_at: Option, + unlogged: usize, +} + +impl Default for SlowFrames { + fn default() -> Self { + Self { frames: VecDeque::with_capacity(SLOW_FRAMES), threshold_ms: SLOW_FRAME_MS, prev_ui_ms: None, logged_at: None, unlogged: 0 } + } +} + +impl SlowFrames { + /// The kept frames, oldest first. + pub fn frames(&self) -> impl DoubleEndedIterator { + self.frames.iter() + } + + /// Keep frames slower than `ms` instead of [`SLOW_FRAME_MS`] (tests). + #[cfg(test)] + pub(crate) fn set_threshold_ms(&mut self, ms: f64) { + self.threshold_ms = ms; + } + + /// Close frame `s` at `now`: `gpu` says the canvas is on the GPU, `gpu_ms` is the GPU's + /// latest composite time. A slow frame is kept, and logged at most once a second. + pub fn finish(&mut self, mut s: FrameStages, now: f64, gpu: bool, gpu_ms: Option) -> FrameStages { + s.end(now); + let on_gpu = if s.refresh.is_empty() { gpu } else { s.refresh.starts_with("gpu") }; + s.path = if on_gpu { "gpu" } else { "cpu" }; + s.gpu_ms = gpu_ms.filter(|_| on_gpu); + s.present_ms = s.cpu_prev_ms.zip(self.prev_ui_ms).map(|(cpu, ui)| (cpu - ui).max(0.0)); + self.prev_ui_ms = Some(s.ui_ms); + if let Some(line) = self.push(&s) { + log::info!("{line}"); + } + s + } + + /// Keep `s` if it's slow; the log line to write, if one is due. + fn push(&mut self, s: &FrameStages) -> Option { + if s.frame_ms.is_nan() || s.frame_ms <= self.threshold_ms { + return None; + } + if self.frames.len() == SLOW_FRAMES { + self.frames.pop_front(); + } + self.frames.push_back(s.clone()); + if self.logged_at.is_some_and(|at| s.t - at < LOG_EVERY_MS) { + self.unlogged += 1; + return None; + } + let mut line = s.line(); + if self.unlogged > 0 { + line.push_str(&format!(" ({} more slow frames)", self.unlogged)); + } + self.logged_at = Some(s.t); + self.unlogged = 0; + Some(line) + } +} + +impl serde::Serialize for SlowFrames { + fn serialize(&self, ser: S) -> Result { + ser.collect_seq(self.frames.iter()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn frame(t: f64, ms: f64) -> FrameStages { + let mut s = FrameStages::default(); + s.start(t, 0); + s.ui_start(t + 1.0, None); + s.refresh("full", 100, ms / 2.0, 1.0); + s + } + + #[test] + fn slow_frames_keep_the_last_64_over_the_threshold() { + let mut r = SlowFrames::default(); + for k in 0..100 { + let t = f64::from(k) * 100.0; + r.finish(frame(t, 0.0), t + 60.0 + f64::from(k), false, None); + } + assert_eq!(r.frames().count(), SLOW_FRAMES); + let first = r.frames().next().unwrap(); + assert_eq!(first.t, 3600.0, "the oldest are dropped"); + assert_eq!(r.frames().last().unwrap().frame_ms, 159.0); + assert!(r.frames().all(|f| f.path == "cpu" && f.refresh_px == 100)); + let json = serde_json::to_value(&r).unwrap(); + assert_eq!(json.as_array().map(Vec::len), Some(SLOW_FRAMES)); + assert_eq!(json[0]["frameMs"], 96.0); + } + + #[test] + fn a_fast_frame_is_not_recorded() { + let mut r = SlowFrames::default(); + let s = r.finish(frame(0.0, 0.0), 49.0, true, Some(3.0)); + assert_eq!(s.frame_ms, 49.0); + assert_eq!(s.path, "cpu", "the refresh says who drew it"); + assert_eq!(s.gpu_ms, None, "no GPU time for a CPU refresh"); + r.finish(frame(100.0, 0.0), 150.0, false, None); + assert_eq!(r.frames().count(), 0, "exactly 50 ms isn't over"); + } + + #[test] + fn stages_never_exceed_the_frame() { + let mut r = SlowFrames::default(); + // A command run between frames (before the hook) makes the stages longer than the clock. + let mut s = FrameStages::default(); + s.command("layer.setProps", 80.0); + s.start(1000.0, 2); + s.ui_start(1005.0, Some(0.004)); + s.damage_ms = 2.0; + s.refresh("gpu-full", 12, 30.0, 10.0); + s.ui_ms = 40.0; + let s = r.finish(s, 1050.0, true, Some(7.0)); + assert!(s.stage_ms() <= s.frame_ms, "{s:?}"); + assert_eq!((s.input_ms, s.frame_ms, s.path, s.gpu_ms), (0.0, 122.0, "gpu", Some(7.0))); + assert_eq!(s.present_ms, None, "no previous frame"); + let mut next = frame(1100.0, 4.0); + next.cpu_prev_ms = Some(45.0); + let next = r.finish(next, 1110.0, true, None); + assert_eq!(next.present_ms, Some(5.0), "the previous frame's CPU time minus its UI build"); + assert!(next.stage_ms() <= next.frame_ms); + } + + #[test] + fn slow_frame_log_is_rate_limited() { + let mut r = SlowFrames::default(); + let lines: Vec = (0..10) + .filter_map(|k| { + let mut s = frame(f64::from(k) * 100.0, 0.0); + s.end(s.t + 60.0); + r.push(&s) + }) + .collect(); + assert_eq!(lines.len(), 1, "{lines:?}"); + assert!(lines[0].starts_with("slow frame: 60.0 ms") && !lines[0].contains("more"), "{}", lines[0]); + let mut s = frame(1000.0, 0.0); + s.end(1070.0); + let line = r.push(&s).expect("a second later, the next line"); + assert!(line.ends_with("(9 more slow frames)"), "{line}"); + assert_eq!(r.frames().count(), 11); + } +} + +#[cfg(all(test, not(target_arch = "wasm32")))] +mod app_tests { + use serde_json::json; + + use crate::PhotocraftApp; + + #[test] + fn an_app_frame_with_a_full_refresh_records_its_stages() { + let (w, h) = (4000u64, 3000u64); + let mut s = photocraft_engine::Session::new(); + s.execute("file.new", json!({"width": w, "height": h, "background": "white"})).unwrap(); + let layer = s.active().and_then(|d| d.active_layer).expect("the background").0; + let mut harness = egui_kittest::Harness::builder().with_size(egui::vec2(1440.0, 900.0)).with_max_steps(4).build_eframe(move |cc| { + PhotocraftApp::setup_context(&cc.egui_ctx, Default::default()); + PhotocraftApp::new(s, crate::Services::default()) + }); + harness.run_steps(6); + // Every frame counts as slow, so the test doesn't depend on the machine's speed. + harness.state_mut().slow_frames.set_threshold_ms(0.0); + harness.state_mut().run("layer.setProps", json!({"layer": layer, "visible": false})).unwrap(); + harness.step(); + let app = harness.state(); + let f = app.slow_frames.frames().rev().find(|f| f.command == "layer.setProps").expect("the toggle's frame is kept").clone(); + // The CPU canvas (kittest has no wgpu) redraws the whole document. A damage refresh counts + // reduced texels times the factor squared, which can fall a few rows short (see P80). + assert_eq!(f.path, "cpu", "{f:?}"); + assert!(matches!(f.refresh, "full" | "rect"), "{f:?}"); + assert!(f.refresh_px * 100 >= w * h * 95 && f.refresh_px <= w * h, "{f:?}"); + assert!(f.composite_ms > 0.0 && f.command_ms > 0.0, "{f:?}"); + assert!(f.stage_ms() <= f.frame_ms, "{f:?}"); + } +} diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 67b54ef73e..22d3565ffa 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -19,6 +19,7 @@ pub mod dialogs; pub mod dock; pub mod file_dialog; pub mod file_open; +pub mod frame_stages; pub mod gpu_status; pub mod i18n; mod icon_data; @@ -504,6 +505,10 @@ pub struct PhotocraftApp { /// makes shortcut- and dialog-triggered commands pass the same policy as /// direct control calls. pub(crate) automation_input: bool, + /// How many `run_command` calls are on the stack. Only the outermost one writes the frame's + /// command stage: a nested run (`file.save` → `layer.smartObjects.saveContents`, + /// `actions.play` → its steps) is already inside the outer call's time. + run_depth: u32, /// Action playbacks paused at a step dialog, by the token in that dialog's fields /// ([`actions::RESUME`]): what to play next and the automation gate it paused under. pub(crate) paused_playbacks: actions::PausedPlaybacks, @@ -618,6 +623,8 @@ pub struct PhotocraftApp { pub stroke_trace: stroke_trace::StrokeTrace, /// Why each document's view is drawn on the CPU, and the history of changes (`render_path`). pub render_path: render_path::RenderPath, + /// The last slow frames and their stages (`perf.slowFrames`, see `frame_stages`). + pub slow_frames: frame_stages::SlowFrames, /// Preferences, autosave and snapping runtime state (see `prefs_ui`, `snap_ui`). pub(crate) prefs_rt: prefs_ui::Runtime, pub(crate) updates_rt: updates_ui::Runtime, diff --git a/crates/ui-egui/src/stroke_timing_tests.rs b/crates/ui-egui/src/stroke_timing_tests.rs index ac88a6c899..ed59943fae 100644 --- a/crates/ui-egui/src/stroke_timing_tests.rs +++ b/crates/ui-egui/src/stroke_timing_tests.rs @@ -387,11 +387,10 @@ fn the_stroke_trace_times_each_frame_of_a_stroke_and_ends_on_release() { } let t0 = crate::gpu_canvas::now_ms(); let moves = ctx.input(|i| i.raw.events.iter().filter(|e| matches!(e, egui::Event::PointerMoved(_))).count()); - app.stroke_trace.begin_frame(t0, moves); - app.stroke_trace.cpu_prev(Some(0.004)); + app.perf.stage.start(t0, moves); + app.perf.stage.ui_start(t0, Some(0.004)); egui::CentralPanel::default().show(ui, |ui| crate::canvas::document_area(app, ui)); - let stroking = app.live_stroke.is_some(); - app.stroke_trace.end_frame(crate::gpu_canvas::now_ms() - t0, stroking, || "test".into()); + app.finish_frame(crate::gpu_canvas::now_ms() - t0); }, app, ); @@ -409,7 +408,7 @@ fn the_stroke_trace_times_each_frame_of_a_stroke_and_ends_on_release() { assert!(s.frames >= 6, "{s:?}"); assert!(s.stroke[1] > 0.0 && s.ui[0] >= s.stroke[0], "{s:?}"); assert_eq!(s.moves[1], 1.0, "one move per test frame: {s:?}"); - assert_eq!(s.backend, "test"); + assert_eq!(s.backend, "", "the app's adapter (none in a test)"); } /// A Clone Stamp stroke through the canvas renders the points its commit gets (position and diff --git a/crates/ui-egui/src/tools/stroke_trace.rs b/crates/ui-egui/src/tools/stroke_trace.rs index 809414ed49..8bbd1c887a 100644 --- a/crates/ui-egui/src/tools/stroke_trace.rs +++ b/crates/ui-egui/src/tools/stroke_trace.rs @@ -11,9 +11,14 @@ //! submission, without the vsync wait). What the app can't see (the OS input queue before a frame //! starts, frames queued for the display, the compositor) is the rest of the input-to-photon time; //! the summary names the GPU backend because GL ignores the frame-latency setting. +//! +//! The trace keeps no clock of its own: each row is the frame's `frame_stages::FrameStages`, +//! which the app gathers every frame (the slow-frame ring reads the same record). use std::fmt::Write as _; +use crate::frame_stages::FrameStages; + /// One frame of a stroke. Times in ms. #[derive(Clone, Debug, Default, PartialEq)] pub(crate) struct Row { @@ -33,6 +38,13 @@ pub(crate) struct Row { pub cpu_prev: Option, } +impl Row { + /// The trace's row for a frame's stages. + pub(crate) fn from_stages(s: &FrameStages, interval: f64) -> Self { + Self { t: s.t, interval, moves: s.moves, stroke: s.stroke_ms, canvas: s.composite_ms + s.upload_ms, ui: s.ui_ms, cpu_prev: s.cpu_prev_ms } + } +} + /// Percentiles of a stroke's frames, for the summary line and the control channel. #[derive(Clone, Debug, Default, PartialEq, serde::Serialize)] #[serde(rename_all = "camelCase")] @@ -86,8 +98,6 @@ pub(crate) enum Output { #[derive(Debug, Default)] pub struct StrokeTrace { pub(crate) output: Output, - /// The frame being built (started by `begin_frame`). - frame: Option, last_start: Option, /// The current stroke's frames. rows: Vec, @@ -143,49 +153,20 @@ impl StrokeTrace { self.output != Output::Off } - /// A frame starts (the raw input hook) with `moves` pointer moves. - pub fn begin_frame(&mut self, now: f64, moves: usize) { + /// A frame ended, with the stages the app gathered for it (`frame_stages`; the trace keeps + /// no clock of its own). `stroking`: a stroke is being drawn (a frame that rendered stroke + /// points counts too, such as the release's commit). A stroke's frames are kept until the + /// first frame without one, which ends it. + pub fn frame(&mut self, s: &FrameStages, stroking: bool, backend: impl FnOnce() -> String) { if !self.enabled() { return; } - let interval = self.last_start.map_or(0.0, |t| now - t); - self.last_start = Some(now); - self.frame = Some(Row { t: now, interval, moves, ..Default::default() }); - } - - /// eframe's CPU time for the previous frame, in seconds (`Frame::info().cpu_usage`). - pub fn cpu_prev(&mut self, secs: Option) { - if let Some(f) = self.frame.as_mut() { - f.cpu_prev = secs.map(|s| f64::from(s) * 1000.0); - } - } - - /// Time spent rendering stroke points this frame. - pub fn stroke(&mut self, ms: f64) { - if let Some(f) = self.frame.as_mut() { - f.stroke += ms; - } - } - - /// Time spent refreshing the canvas this frame. - pub fn canvas(&mut self, ms: f64) { - if let Some(f) = self.frame.as_mut() { - f.canvas += ms; - } - } - - /// The frame's UI is built (`ui` ms). `stroking`: a stroke is being drawn (a frame that - /// rendered stroke points counts too, such as the release's commit). A stroke's frames are - /// kept until the first frame without one, which ends it. - pub fn end_frame(&mut self, ui: f64, stroking: bool, backend: impl FnOnce() -> String) { - let Some(mut f) = self.frame.take() else { return }; - if stroking || f.stroke > 0.0 { - f.ui = ui; + let interval = self.last_start.map_or(0.0, |t| s.t - t); + self.last_start = Some(s.t); + if stroking || s.stroke_ms > 0.0 { // The first frame of a stroke doesn't follow a stroke frame: its interval is idle time. - if self.rows.is_empty() { - f.interval = 0.0; - } - self.rows.push(f); + let interval = if self.rows.is_empty() { 0.0 } else { interval }; + self.rows.push(Row::from_stages(s, interval)); } else if !self.rows.is_empty() { self.finish(backend()); } @@ -251,14 +232,38 @@ mod tests { StrokeTrace { output: Output::Stderr, ..Default::default() } } - fn frame(t: &mut StrokeTrace, now: f64, cpu: f32, stroking: bool) { - t.begin_frame(now, 3); - t.cpu_prev(Some(cpu / 1000.0)); + fn stages(now: f64, cpu: f32, stroking: bool) -> FrameStages { + let mut s = FrameStages::default(); + s.start(now, 3); + s.ui_start(now, Some(cpu / 1000.0)); if stroking { - t.stroke(1.0); - t.canvas(2.0); + s.stroke_ms = 1.0; + s.refresh("rect", 10, 1.5, 0.5); } - t.end_frame(4.0, stroking, || "vulkan Test GPU".into()); + s.ui_ms = 4.0; + s + } + + fn frame(t: &mut StrokeTrace, now: f64, cpu: f32, stroking: bool) { + t.frame(&stages(now, cpu, stroking), stroking, || "vulkan Test GPU".into()); + } + + #[test] + fn stroke_trace_reads_the_shared_frame_stages() { + let mut t = on(); + let mut ring = crate::frame_stages::SlowFrames::default(); + let mut last = None; + for (k, now) in [100.0, 116.0, 132.0].into_iter().enumerate() { + let s = ring.finish(stages(now, 6.0, true), now + 5.0, false, None); + t.frame(&s, true, || "test".into()); + let interval = if k == 0 { 0.0 } else { 16.0 }; + let row = t.rows.last().expect("a stroke frame").clone(); + assert_eq!((row.t, row.interval, row.moves, row.stroke, row.canvas, row.ui), (s.t, interval, 3, 1.0, 2.0, 4.0)); + assert_eq!(row.cpu_prev, s.cpu_prev_ms); + assert_eq!(t.rows.last(), Some(&Row::from_stages(&s, interval)), "the row is the frame's stages"); + last = Some(s); + } + assert_eq!(last.map(|s| s.composite_ms + s.upload_ms), Some(2.0)); } #[test] diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 0c286693c2..e13f64fedc 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -46,7 +46,7 @@ The transport is `apps/photocraft/src/control_server.rs`, and the handlers are i - `engine.execute {command, params}`: run any engine or UI command by id. Engine commands run directly with their default params and never open a dialog. Use `ui.menu.invoke` for menu-click behaviour, which opens a command's dialog when no params are given. `params` must be a JSON object (omit it or pass `null` for none); an array, string, number or boolean is an error naming the command, in every transport (control channel, `serve`, MCP and the CLI) - `engine.commands`: list commands with enablement; the `result` is a JSON array of command objects (see [Command list replies](#command-list-replies)) -- `ui.inspect`: full UI state (tool, panels, views, dialogs, windows, window size). The menu tree is not included; use `ui.menu.list`. `brushPicker` is `null` while closed and `{pos: [x, y], size: [w, h]?, list: {collapsed, filter, renaming, showName, showStroke, showTip, scale}}` while the Brush Preset picker is open (coordinates in screen points; `size` is the picker's content size once its corner grip was dragged, `null` before that, and the drag remembers both dimensions; the three `show*` flags pick the parts of each preset's card — at least one is always on; `scale` is the footer slider, 0.15–2 with 1 the standard, multiplying the cards' width and height — the heights stop at half size and the name and tip-size text hold their 1.0 size — and dropping the tips' size numbers at 0.40 and below; the parts and the scale are remembered across restarts, while the search, collapsed groups and a rename in progress are not). `view` holds the View/Window/Type preferences: `screen_mode`, `extras`, `show` and `snap_to` flags, `flip_horizontal`, `arrange` (Window › Arrange layout), pixel aspect, font preview size, language options. `perf.timings.gpuInfo` holds the graphics adapter, backend, driver, the backend chosen at launch and why, the canvas renderer (`gpu`/`cpu`) and, after a device loss, `lost` (`help.systemInfo` returns the same as `info`). `perf.renderPath` says why the active document is drawn on the CPU: `current` is `null` on the GPU, else `{doc, kind, detail}` (`kind` is one of `overBudget`, `canvasTooLarge`, `compositorBuild`, `deviceLost`, `gpuError`, `unsupported`, `adjustPreview`, `viewTransform`, `softwareAdapter`, `cpuMode`; `detail` is the English reason, with the working set and budget for `overBudget`), and `history` holds the last 32 changes, oldest first (`{t, doc, kind, detail}`, `kind` `null` for back on the GPU, `doc` the changed document's id, or 0 for a change to every document). An adjustment dialog's CPU proxy preview has its own slot: `current` reports it (`adjustPreview`) only when neither the window nor the canvas has a reason, it isn't recorded while the whole window is off the GPU, and its history entries always have `kind` `adjustPreview`, with `detail` `null` when the preview stops using the proxy (never read as back on the GPU). `perf.timings.gpuFallback` is `current`'s `detail` as text +- `ui.inspect`: full UI state (tool, panels, views, dialogs, windows, window size). The menu tree is not included; use `ui.menu.list`. `brushPicker` is `null` while closed and `{pos: [x, y], size: [w, h]?, list: {collapsed, filter, renaming, showName, showStroke, showTip, scale}}` while the Brush Preset picker is open (coordinates in screen points; `size` is the picker's content size once its corner grip was dragged, `null` before that, and the drag remembers both dimensions; the three `show*` flags pick the parts of each preset's card — at least one is always on; `scale` is the footer slider, 0.15–2 with 1 the standard, multiplying the cards' width and height — the heights stop at half size and the name and tip-size text hold their 1.0 size — and dropping the tips' size numbers at 0.40 and below; the parts and the scale are remembered across restarts, while the search, collapsed groups and a rename in progress are not). `view` holds the View/Window/Type preferences: `screen_mode`, `extras`, `show` and `snap_to` flags, `flip_horizontal`, `arrange` (Window › Arrange layout), pixel aspect, font preview size, language options. `perf.timings.gpuInfo` holds the graphics adapter, backend, driver, the backend chosen at launch and why, the canvas renderer (`gpu`/`cpu`) and, after a device loss, `lost` (`help.systemInfo` returns the same as `info`). `perf.renderPath` says why the active document is drawn on the CPU: `current` is `null` on the GPU, else `{doc, kind, detail}` (`kind` is one of `overBudget`, `canvasTooLarge`, `compositorBuild`, `deviceLost`, `gpuError`, `unsupported`, `adjustPreview`, `viewTransform`, `softwareAdapter`, `cpuMode`; `detail` is the English reason, with the working set and budget for `overBudget`), and `history` holds the last 32 changes, oldest first (`{t, doc, kind, detail}`, `kind` `null` for back on the GPU, `doc` the changed document's id, or 0 for a change to every document). An adjustment dialog's CPU proxy preview has its own slot: `current` reports it (`adjustPreview`) only when neither the window nor the canvas has a reason, it isn't recorded while the whole window is off the GPU, and its history entries always have `kind` `adjustPreview`, with `detail` `null` when the preview stops using the proxy (never read as back on the GPU). `perf.timings.gpuFallback` is `current`'s `detail` as text. `perf.slowFrames` lists the last 64 frames over 50 ms, oldest first, each with its stages in ms: `t` (start), `frameMs`, `inputMs` (raw input to the UI build, commands excluded), `commandMs` and `command` (the last command run since the previous frame), `damageMs` (damage tracking and GPU memory budgeting), `compositeMs`, `uploadMs`, `gpuMs` (the GPU's own composite time, or `null`), `presentMs` (the previous frame's tessellation and submission, or `null`), `refresh` and `refreshPx` (the canvas redraw's kind and pixels) and `path` (`gpu` or `cpu`). It stays empty on the web, which has no clock - `ui.set {tool?, panels?, dockTabs?, dock?, dockWidth?, colorPanel?, maskTarget?, vectorMaskTarget?, filterMaskTarget?, selectionMode?, zoom?, center?, rotation?, fit?, theme?, brushSize?, brushSection?, brushTab?, brushesView?, brushPicker?, brushPickerName?, brushPickerStroke?, brushPickerTip?, brushPickerScale?, gradientBlendMode?, gradientClassic?, shapeStroke?}`: change UI state; any other field is an error, checked before anything changes (`zoom` is the canvas zoom where 1 = 100%, one document pixel per physical display pixel, so a scaled display doesn't magnify the image; `theme` is `pro`, `proMedium`, `studio`, `studioLight` or `classic`; `selectionMode` is the selection tools' options-bar mode, 0 New, 1 Add, 2 Subtract, 3 Intersect; `brushSection` indexes the Brush Settings sections, `brushTab` 0 = Brush Settings, 1 = Brushes; `brushesView` is `list` or `grid` for the Brushes panel; `brushPickerName`, `brushPickerStroke` and `brushPickerTip` are the Brush Preset picker's card parts (booleans, at least one on), and `brushPickerScale` its footer card-scale slider (0.15 to 2, 1 the standard); `brushPicker` is `[x, y]` in screen points to open the Brush Preset picker there (as a right-click with a painting tool does) or `null` to close it, and picking a preset in it is `tools.setBrush {"preset": name}`; `dock` is `{order: ["layers", …], heights: {"properties": 180}, collapsed: ["color"], tabOrder: {"layers": ["Paths", "Layers", "Channels"]}}`, the right-dock groups top to bottom, their heights in points, the groups collapsed to their tab strip and each group's tab order left to right by panel name (a group left out keeps its default order, unknown names are ignored and tabs not listed follow in their default order; `dockTabs` keeps indexing tabs in their default order); `dockWidth` sets the right dock width in points, clamped to 250..520; `colorPanel` is `{background?, mode?, ramp?, webSafeRamp?}`, merged into the Color panel's state: `background` is whether it edits the background colour, `mode` its panel-menu display (`hueCube`, `brightnessCube`, `colorWheel`, `grayscaleSlider`, `rgbSliders`, `hsbSliders`, `cmykSliders`, `labSliders` or `webColorSliders`), `ramp` the colour ramp under the sliders (`rgbSpectrum`, `cmykSpectrum`, `grayscaleRamp` or `currentColors`) and `webSafeRamp` Make Ramp Web Safe; `maskTarget` targets the active layer's mask, which, like Photoshop, swaps in the foreground/background pair kept for masks (white/black until one is picked) and swaps the pixels' pair back when the pixels are targeted again; `filterMaskTarget` targets the active smart object's filter mask (an error without smart filters), so painting, fills, filters and adjustments edit it (`"target":"filterMask"`) while Move and Edit › Transform still act on the smart object) - `typeTransform` is `null` outside a temporary Ctrl/Cmd Type-tool drag. During a drag it reports its source document/revision, original affine, oriented `frame` (layer, local bounds, document-space corners and pivot), and captured `gesture`. Drive it with `ui.pointer` and `command: true` at `down`; `move`/`up` may release the modifier. The document/history remain unchanged during preview, and `up` applies one `type.edit` within `textEdit`'s session. Pressing Escape during the drag cancels only the preview. This pointer state is never restored from saved UI state. - `ui.menu.invoke {id, params?, wait?}` / `ui.menu.list`: activate a menu item by id; list the menu tree. A menu item that starts a background job (a filter without a dialog, such as Blur More) replies with the job's result once it has been applied; with `"wait": false` the reply is `{job, pending: true}` at once. Window ends with the open documents as `window.document.` (tab order, the active one checked, #2991); invoking one brings that document to the front and replies `{document: index}`. Camera Raw and the full-window workspaces (Liquify, Content-Aware Fill, Filter Gallery, Adaptive Wide Angle) are modal: while one is open, `ui.menu.invoke` refuses every other menu item ("menu command is unavailable while a modal dialog is open") and accepts only that workspace's own `{"ui": {...}}` requests (`filter.liquify`, `edit.contentAwareFill`, `filter.filterGallery`, `filter.adaptiveWideAngle`, `filter.cameraRaw`; `params` must hold only the `ui` object), so finish it with `{"ui":{"commit":true}}` or `{"ui":{"cancel":true}}` first. Those requests are refused too while a dialog, an unsaved-changes prompt or a crop that blocks the command is open: resolve that first, then commit or cancel the workspace. `ui.menu.list` reports the same gate: an item the modal state refuses is listed with `enabled: false`. `engine.execute` is not a menu click and is not gated diff --git a/docs/development.md b/docs/development.md index 41d74bae67..a9e9fc486e 100644 --- a/docs/development.md +++ b/docs/development.md @@ -200,6 +200,8 @@ stroke trace: 62 frames on vulkan NVIDIA GeForce RTX 5090 (p50/p95 ms): interval What the app can't time is the rest of input-to-photon: the OS input queue before a frame starts (up to one `interval`), frames queued for the display (the surface keeps up to two; the GL backend ignores that setting), and the compositor. So the first line names the backend and how frames are presented. To compare presentation settings without rebuilding, launch with `PHOTOCRAFT_PRESENT_MODE` and `PHOTOCRAFT_FRAME_LATENCY` (see [Environment variables](#environment-variables)): with `novsync` the app no longer waits on vsync (`wait` near 0): each refresh shows the newest frame, and where the driver only offers Immediate, the screen may tear. The last summary is also in the control channel's `perf.strokeTrace` (`ui.inspect`). The code is `crates/ui-egui/src/stroke_trace.rs`. +The trace has no clock of its own: it reads the per-frame stage record the app gathers every frame (`crates/ui-egui/src/frame_stages.rs`). The same record feeds the slow-frame ring: any frame over 50 ms is kept (the last 64, in `perf.slowFrames`) and logged at `info`, at most one line a second with a count of the slow frames in between, naming its input, command, damage, composite, upload, GPU and present times and the canvas redraw's kind and pixels. + ## Driving the app programmatically Start the app with a private token file. It then accepts authenticated JSON lines on `127.0.0.1:7878`: diff --git a/docs/scorecard.md b/docs/scorecard.md index b9f9bb0a1e..4cef89bfaa 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 | 42 | 76 scenarios: 3 within budget, 22 over budget or failing, 9 tracked only, 38 not run, 4 not measurable yet (baseline 2026-10-05) | +| [Performance](#performance) | 3 | 22 | 45 | 79 scenarios: 3 within budget, 22 over budget or failing, 9 tracked only, 41 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: 12 of 181 (target 0) | @@ -98,6 +98,9 @@ Baseline: 2026-10-05, 1 run, commit `f5d2bcb54a`, Apple M4 Pro (48 GB), GPU Appl | P77 | Toggle an adjustment layer's visibility | ≤ 10 ms p95 | – | – | – | – | – | not run | [#6535](https://github.com/storytold/photocraft/issues/6535) | | P78 | Hide the top adjustment layer among 150 full-canvas layers, 6 MP, GPU budget forced to 512 MB (CPU fallback) | ≤ 30 ms p95 | – | – | – | – | – | not run | [#6533](https://github.com/storytold/photocraft/issues/6533) | | P79 | Zoomed-out reduced composite of a 30000x30000 document with one painted layer (#9330 target: 60000x60000) | track only | – | – | – | – | – | not run | [#1015](https://github.com/storytold/photocraft/issues/1015) | +| P80 | App frame: full CPU refresh of a 24 MP document | ≤ 50 ms p95 | – | – | – | – | – | not run | [#9328](https://github.com/storytold/photocraft/issues/9328) | +| P81 | Undo a brush dab on a full-canvas 16-bit 15000 x 10000 layer + refresh | track only | – | – | – | – | – | not run | [#9329](https://github.com/storytold/photocraft/issues/9329) | +| P82 | Fill a 2 % selection on a full-canvas 24 MP layer + refresh | 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/packaging/freebsd/package.sh b/packaging/freebsd/package.sh index 2d94ef57fb..5b7e0a3646 100755 --- a/packaging/freebsd/package.sh +++ b/packaging/freebsd/package.sh @@ -1,8 +1,8 @@ #!/usr/bin/env bash # Build and package PhotoCraft for FreeBSD: # -# $DIST/photocraft--freebsd-x86_64.tar.gz a /usr/local-style tree: -# photocraft--freebsd-x86_64/{bin, man/man1, share/applications, share/icons, +# $DIST/photocraft--freebsd-.tar.gz a /usr/local-style tree (arch x86_64 or aarch64): +# photocraft--freebsd-/{bin, man/man1, share/applications, share/icons, # share/mime, share/metainfo, share/doc/photocraft, share/{bash-completion,zsh,fish}} # # Install by copying the tree's contents into /usr/local: @@ -30,12 +30,32 @@ while [[ $# -gt 0 ]]; do done # FreeBSD's uname says amd64; release file names use x86_64 like the Linux and Windows ones. +# CARGO_BUILD_TARGET is set when the release VM cross-compiles aarch64 from an x86_64 guest. MACHINE="$(uname -m)" case "$MACHINE" in - amd64 | x86_64) ARCH=x86_64 ;; - arm64 | aarch64) ARCH=aarch64 ;; + amd64 | x86_64) HOST_ARCH=x86_64 ;; + arm64 | aarch64) HOST_ARCH=aarch64 ;; *) echo "unsupported architecture $MACHINE" >&2; exit 2 ;; esac +ARCH="$HOST_ARCH" +if [[ -n "${CARGO_BUILD_TARGET:-}" ]]; then + # cargo's host-tuple alias means the host triple; resolve it so the binary dir matches cargo's output. + if [[ "$CARGO_BUILD_TARGET" = host-tuple ]]; then + CARGO_BUILD_TARGET="$(rustc -vV | sed -n 's/^host: //p')" + export CARGO_BUILD_TARGET + fi + case "$CARGO_BUILD_TARGET" in + aarch64-unknown-freebsd) ARCH=aarch64 ;; + x86_64-unknown-freebsd) ARCH=x86_64 ;; + *) + echo "error: unsupported CARGO_BUILD_TARGET $CARGO_BUILD_TARGET (use x86_64-unknown-freebsd or aarch64-unknown-freebsd)" >&2 + exit 2 + ;; + esac +fi +# A cross-built binary cannot run on this host, so the --version check and the man-page example are skipped. +CROSS=0 +if [[ "$ARCH" != "$HOST_ARCH" ]]; then CROSS=1; fi if [[ "$DRY_RUN" = 0 ]] && [[ "$(uname -s)" != FreeBSD ]]; then echo "error: build on FreeBSD (or pass --dry-run to check the tree layout)" >&2 exit 2 @@ -48,10 +68,12 @@ echo "==> PhotoCraft $VERSION for FreeBSD $ARCH" export CARGO_BUILD_JOBS="${CARGO_BUILD_JOBS:-4}" # The man-and-completions example writes photocraft-cli's man pages and shell completions below. It # is built in its own cargo run: built with the binaries, its dev-dependencies' features would be -# unified into them. +# unified into them. cargo reads CARGO_BUILD_TARGET itself, so cross builds need no --target flag. if [[ "$SKIP_BUILD" = 0 ]]; then (cd "$ROOT" && cargo build --release --locked -p photocraft -p photocraft-cli --features heif --bins) - (cd "$ROOT" && cargo build --release --locked -p photocraft-cli --features heif --example man-and-completions) + if [[ "$CROSS" = 0 ]]; then + (cd "$ROOT" && cargo build --release --locked -p photocraft-cli --features heif --example man-and-completions) + fi fi WORK="$CARGO_TARGET_DIR/freebsd-package" STAGE="$WORK/$BASENAME" @@ -59,6 +81,7 @@ rm -rf "$WORK" mkdir -p "$WORK" BIN="$CARGO_TARGET_DIR/release" +if [[ -n "${CARGO_BUILD_TARGET:-}" ]]; then BIN="$CARGO_TARGET_DIR/$CARGO_BUILD_TARGET/release"; fi OUT_DIR="$DIST" if [[ "$DRY_RUN" = 1 ]]; then # Stub binaries, and the tarball stays in the work dir so dist/ only ever holds real builds. @@ -91,7 +114,9 @@ done # example built above. A dry run builds nothing, so it leaves them out, as a --skip-build run whose # prebuilt binaries came without the example does. CLI_DOCS="$BIN/examples/man-and-completions" -if [[ "$DRY_RUN" = 0 ]] && [[ ! -x "$CLI_DOCS" ]]; then +if [[ "$DRY_RUN" = 0 ]] && [[ "$CROSS" = 1 ]]; then + warn "cross-building $ARCH, so photocraft-cli's man pages and completions are left out" +elif [[ "$DRY_RUN" = 0 ]] && [[ ! -x "$CLI_DOCS" ]]; then warn "$CLI_DOCS is not built, so photocraft-cli's man pages and completions are left out" elif [[ "$DRY_RUN" = 0 ]]; then "$CLI_DOCS" "$WORK/cli-docs" @@ -114,7 +139,11 @@ mkdir -p "$OUT_DIR" tar -C "$WORK" -czf "$OUT_DIR/$BASENAME.tar.gz" "$BASENAME" echo "wrote $OUT_DIR/$BASENAME.tar.gz" -"$STAGE/bin/photocraft-cli" --version +if [[ -n "${CARGO_BUILD_TARGET:-}" ]]; then + echo "==> skipping photocraft-cli --version: CARGO_BUILD_TARGET is set, so the binary is not run on this host" +else + "$STAGE/bin/photocraft-cli" --version +fi if [[ "$DRY_RUN" = 1 ]]; then echo "==> dry run: tarball contents" tar -tzvf "$OUT_DIR/$BASENAME.tar.gz" diff --git a/packaging/linux/flatpak/ai.storyteller.photocraft.bundle.yml b/packaging/linux/flatpak/ai.storyteller.photocraft.bundle.yml index c7d30ac7d3..c716cf310f 100644 --- a/packaging/linux/flatpak/ai.storyteller.photocraft.bundle.yml +++ b/packaging/linux/flatpak/ai.storyteller.photocraft.bundle.yml @@ -22,6 +22,10 @@ finish-args: - --socket=fallback-x11 # X11 shared memory (MIT-SHM). Without it the X11 fallback is slow or fails. - --share=ipc + # Network for the optional --control server: it listens on 127.0.0.1 in the sandbox, which with + # network shared is also reachable from the host's network namespace. The in-app update check + # is disabled under Flatpak (flatpak update owns updates), so this does not enable it. + - --share=network # GPU access for the wgpu compositor (Vulkan, or GL as a fallback). - --device=dri # Printing: the host's CUPS socket, so File › Print reaches the print server directly (IPP, @@ -33,9 +37,7 @@ finish-args: # line, Open Recent, "Save" next to the original) work in the usual places. - --filesystem=xdg-pictures - --filesystem=xdg-documents - # Deliberately not granted: network (the optional --control server listens on 127.0.0.1 in - # the sandbox; `flatpak override --user --share=network ai.storyteller.photocraft` exposes it to - # host agents) and home/host filesystem access. + # Deliberately not granted: home/host filesystem access. modules: - name: photocraft buildsystem: simple diff --git a/packaging/linux/flatpak/ai.storyteller.photocraft.yml b/packaging/linux/flatpak/ai.storyteller.photocraft.yml index 4738c9bda6..7f333cf914 100644 --- a/packaging/linux/flatpak/ai.storyteller.photocraft.yml +++ b/packaging/linux/flatpak/ai.storyteller.photocraft.yml @@ -23,6 +23,10 @@ finish-args: - --socket=fallback-x11 # X11 shared memory (MIT-SHM). Without it the X11 fallback is slow or fails. - --share=ipc + # Network for the optional --control server: it listens on 127.0.0.1 in the sandbox, which with + # network shared is also reachable from the host's network namespace. The in-app update check + # is disabled under Flatpak (flatpak update owns updates), so this does not enable it. + - --share=network # GPU access for the wgpu compositor (Vulkan, or GL as a fallback). - --device=dri # Printing: the host's CUPS socket, so File › Print reaches the print server directly (IPP, @@ -34,9 +38,7 @@ finish-args: # line, Open Recent, "Save" next to the original) work in the usual places. - --filesystem=xdg-pictures - --filesystem=xdg-documents - # Deliberately not granted: network (the optional --control server listens on 127.0.0.1 in - # the sandbox; `flatpak override --user --share=network ai.storyteller.photocraft` exposes it to - # host agents) and home/host filesystem access. + # Deliberately not granted: home/host filesystem access. build-options: append-path: /usr/lib/sdk/rust-stable/bin modules: diff --git a/perf/budgets.toml b/perf/budgets.toml index b538808553..1e553fd99f 100644 --- a/perf/budgets.toml +++ b/perf/budgets.toml @@ -751,3 +751,33 @@ issue = 1015 bench = "perf_scenarios" row = "zoomed-out reduced composite of the huge document with one painted layer" enforce = false # tracked only + +[[scenario]] +id = "P80" +title = "App frame: full CPU refresh of a 24 MP document" +issue = 9328 +bench = "perf_scenarios" +row = "app frame: full CPU refresh of a 24 MP document" +budget_ms = 50 +enforce = false # track only: the worst case for the slow-frame ring (#9328 PR B) + +# ---- #9329: redraw area of edits on full-canvas layers (track only) --------------------------- +# +# Each row checks it hits the path it claims (a full-canvas layer; a non-empty edit; a small +# selection) and prints `refresh px ` after its timing line. + +[[scenario]] +id = "P81" +title = "Undo a brush dab on a full-canvas 16-bit 15000 x 10000 layer + refresh" +issue = 9329 +bench = "perf_scenarios" +row = "undo a brush dab on a full-canvas 16-bit 15000x10000 layer + refresh" +enforce = false # new; no baseline yet + +[[scenario]] +id = "P82" +title = "Fill a 2 % selection on a full-canvas 24 MP layer + refresh" +issue = 9329 +bench = "perf_scenarios" +row = "fill a 2 % selection on a full-canvas 24 MP layer + refresh" +enforce = false # new; no baseline yet