diff --git a/README.md b/README.md index e308a514..b65d3e3a 100644 --- a/README.md +++ b/README.md @@ -14,9 +14,9 @@ > [!IMPORTANT] > **Darkly is in beta**! Features are being [added daily](#feature-roadmap). Please [report bugs](https://github.com/darkly-art/darkly/issues/new) so we can squash them. -Do you suffer from the _oppressive sanity_ of rulers, guides, and nondestructive workflows? Break free with [Darkly](https://darkly.art), home of happy accidents and beautiful catastrophies. Harness the **[dark arts](#dark-arts)**, and craft a chaotic masterpiece that will make your graphic design professor roll over in his grave. Madness isn't a bug, it's a feature. +Do you suffer from the _oppressive sanity_ of rulers, guides, and nondestructive workflows? Break free with [Darkly](https://darkly.art), the home of happy accidents and beautiful catastrophies. Madness isn't a bug, it's a feature. -Darkly is an open source Photoshop alternative written in Rust. Digital painters are first-class citizens. +Darkly is a Photoshop alternative where painters are first-class citizens. It has a powerful brush engine, and **[dark arts](#dark-arts)** to help you commune with your imagination. **Try the demo [here](https://demo.darkly.art).** diff --git a/crates/darkly/brushes/hair.yaml b/crates/darkly/brushes/hair.yaml index 348a902a..18486e52 100644 --- a/crates/darkly/brushes/hair.yaml +++ b/crates/darkly/brushes/hair.yaml @@ -6,28 +6,42 @@ nodes: brush_settings: type: brush_settings inputs: - size: 0.5685366988182068 - spacing: 0.009999999776482582 + size: 0.2 + spacing: 0.01 stabilize: 1.0 circle: type: circle curve: type: curve + comment: pen pressure affects the threshold of hair volume inputs: curve: - - 0.0 - 1.0 - - - 0.6688293 - - 0.49044272 + - - 0.682973 + - 0.5305826 - - 1.0 - 0.0 + divide: + type: divide levels: type: levels + multiply: + type: multiply + inputs: + b: 2.56 + multiply_2: + type: multiply + inputs: + a: 0.5 + multiply_3: + type: multiply + inputs: + b: 0.085 noise: type: noise inputs: - scale: 12.040367126464844 - scale_with_brush: false + scale: 12.0 space: 1 paint: type: paint @@ -40,14 +54,21 @@ nodes: subtract: type: subtract inputs: - a: 0.11924592405557632 + a: 0.12 connections: - add.result -> levels.input +- brush_settings.size -> divide.b +- brush_settings.size -> multiply_3.a - circle.mask -> add.b - curve.output -> levels.in_low +- divide.result -> multiply.a - levels.output -> stamp.tip +- multiply.result -> multiply_2.b +- multiply_2.result -> noise.rotation +- multiply_3.result -> noise.scale - noise.value -> subtract.b - paint_color.color -> stamp.color +- pen_input.distance -> divide.a - pen_input.position -> paint.position - pen_input.pressure -> curve.input - stamp.dab -> paint.rgba @@ -55,5 +76,13 @@ connections: exposed_ports: brush_settings.stabilize: {} brush_settings.size: {} + multiply_2.a: + label: Twirl + description: Twirliness factor + icon: tabler:galaxy paint.flow: {} paint.opacity: {} + multiply_3.b: + label: Hair Thickness + description: Controls how many and how thick are the individual strands + icon: fa6-solid:arrows-left-right diff --git a/crates/darkly/brushes/twirly_hair.yaml b/crates/darkly/brushes/sponge.yaml similarity index 55% rename from crates/darkly/brushes/twirly_hair.yaml rename to crates/darkly/brushes/sponge.yaml index 97f6b5d6..714a4c77 100644 --- a/crates/darkly/brushes/twirly_hair.yaml +++ b/crates/darkly/brushes/sponge.yaml @@ -1,4 +1,4 @@ -name: Twirly Hair +name: Sponge category: Dry Media nodes: add: @@ -6,40 +6,35 @@ nodes: brush_settings: type: brush_settings inputs: - size: 0.4 - spacing: 0.01 + size: 0.2 + spacing: 0.02 stabilize: 1.0 - circle: - type: circle curve: type: curve - comment: pen pressure affects the threshold of hair volume inputs: curve: - - 0.0 - - 1.0 - - - 0.682973 - - 0.5305826 + - 0.70523924 + - - 0.59307975 + - 0.36470732 - - 1.0 - 0.0 - divide: - type: divide - comment: we have to divide the distance factor by the brush size, to keep the 'twirl' setting consistent + curve_2: + type: curve + inputs: + curve: + - - 0.0 + - 0.0 + - - 0.6392242 + - 0.36304563 + - - 1.0 + - 1.0 levels: type: levels - multiply: - type: multiply - inputs: - b: 0.005 - multiply_2: - type: multiply - inputs: - a: 0.5 noise: type: noise inputs: - scale: 12 - scale_with_brush: false + scale: 6.0 space: 1 paint: type: paint @@ -47,6 +42,18 @@ nodes: type: paint_color pen_input: type: pen_input + polygon: + type: polygon + inputs: + points: 4 + rounding: 0.30 + softness: 0.30 + squeeze: 0.35 + squeeze_angle: -0.7853981852531433 + random: + type: random + random_2: + type: random stamp: type: stamp subtract: @@ -55,26 +62,24 @@ nodes: a: 0.11924592405557632 connections: - add.result -> levels.input -- brush_settings.size -> divide.b -- circle.mask -> add.b - curve.output -> levels.in_low -- divide.result -> multiply.a +- curve_2.output -> paint.flow - levels.output -> stamp.tip -- multiply.result -> multiply_2.b -- multiply_2.result -> noise.rotation - noise.value -> subtract.b - paint_color.color -> stamp.color -- pen_input.distance -> divide.a +- pen_input.drawing_angle -> polygon.rotation_input - pen_input.position -> paint.position - pen_input.pressure -> curve.input +- pen_input.pressure -> curve_2.input +- polygon.mask -> add.b +- random.value -> noise.rotation +- random_2.value -> noise.variation - stamp.dab -> paint.rgba - subtract.result -> add.a exposed_ports: brush_settings.stabilize: {} brush_settings.size: {} - multiply_2.a: - label: Twirl - description: Twirliness factor - icon: tabler:galaxy paint.flow: {} paint.opacity: {} + polygon.softness: {} + polygon.rounding: {} diff --git a/crates/darkly/presets/defaults.yaml b/crates/darkly/presets/defaults.yaml index 3c551625..676c0719 100644 --- a/crates/darkly/presets/defaults.yaml +++ b/crates/darkly/presets/defaults.yaml @@ -50,10 +50,9 @@ hotkeys: # Photoshop binds Edit > Preferences to $mod+K; Krita/GIMP have no # default. Darkly picks the macOS preferences convention. openSettings: $mod+Comma - # Photoshop & GIMP both have export (Photoshop: $mod+Alt+W for "Export - # As"; GIMP: $mod+Shift+E for "Export As…"). Krita's `file_export_file` - # is explicitly `none`. Darkly picks GIMP's binding. - exportImage: $mod+Shift+KeyE + # Image export is folded into Save As (the save picker's type dropdown + # offers PNG/JPEG/WebP alongside `.darkly`), so there's no separate + # export-image hotkey — Save As ($mod+Shift+KeyS) covers it. # Photoshop binds Lasso to L and Polygonal Lasso to Shift+L; Krita's # selection-tool .action files have empty ; GIMP's analog is # "Free Select" (KeyF) with no polygonal counterpart. Darkly inherits diff --git a/crates/darkly/presets/gimp.yaml b/crates/darkly/presets/gimp.yaml index f03ec4df..eb98b427 100644 --- a/crates/darkly/presets/gimp.yaml +++ b/crates/darkly/presets/gimp.yaml @@ -15,9 +15,6 @@ hotkeys: pasteInPlace: $mod+Alt+KeyV # GIMP has no native Preferences hotkey. Fall through to Darkly's pick. openSettings: $mod+Comma - # GIMP binds File > Export As… to E (see - # gimp/app/actions/file-actions.c — `file-export-as`). - exportImage: $mod+Shift+KeyE # mirrorViewH: intentionally unbound. GIMP has no native mirror-view # hotkey, and Krita's M would set no established GIMP-user expectation # — leave it for explicit user binding rather than invent a default. diff --git a/crates/darkly/presets/krita.yaml b/crates/darkly/presets/krita.yaml index d6a4a9e0..2cbc8db1 100644 --- a/crates/darkly/presets/krita.yaml +++ b/crates/darkly/presets/krita.yaml @@ -15,11 +15,6 @@ hotkeys: # Krita has no native Preferences hotkey. Fall through to Darkly's # macOS-convention pick; users can rebind via the settings UI. openSettings: $mod+Comma - # Krita's `file_export_file` is explicitly `none` (see - # kritamenu.action). GIMP binds Export As to E - # (gimp/app/actions/file-actions.c — `file-export-as`); Darkly inherits - # that key here. - exportImage: $mod+Shift+KeyE # View > Mirror View. Krita binds M (kritamenu.action `mirror_canvas`, # M). Photoshop and GIMP have no opinion; their # overlays leave mirrorViewH unbound rather than reuse M (Photoshop's diff --git a/crates/darkly/presets/photoshop.yaml b/crates/darkly/presets/photoshop.yaml index 115e2a01..9f166eb0 100644 --- a/crates/darkly/presets/photoshop.yaml +++ b/crates/darkly/presets/photoshop.yaml @@ -12,9 +12,6 @@ hotkeys: pasteInPlace: $mod+Shift+KeyV # Photoshop binds Edit > Preferences to Ctrl+K (Cmd+K on macOS). openSettings: $mod+KeyK - # Photoshop's File > Export > Export As… is Alt+Shift+Ctrl+W; Darkly - # uses the closer Alt+Ctrl+W variant. - exportImage: $mod+Alt+KeyW # mirrorViewH: intentionally unbound. Photoshop has no native mirror- # view action, and M is already taken by rectSelectTool here. Users # who want a mirror-view hotkey under the PS preset can set one via diff --git a/crates/darkly/shaders/brush/bake_source.wgsl b/crates/darkly/shaders/brush/bake_source.wgsl index 6ccfa0d3..fbb71e28 100644 --- a/crates/darkly/shaders/brush/bake_source.wgsl +++ b/crates/darkly/shaders/brush/bake_source.wgsl @@ -17,9 +17,9 @@ struct BakeParams { gain: f32, warp: f32, // Field units the tile spans across [0,1) uv — matches the node's - // sample-time divide by TILE_SPAN so baked feature size equals the live + // sample-time divide by FIELD_SPAN so baked feature size equals the live // path and the field repeats once per this many field units. - tile_span: f32, + field_span: f32, // 0 = grayscale value (single fBm at the base seed, written to R), // 1 = chromatic rgba (three fBm at seed+{0,1,2}). channels: u32, @@ -51,11 +51,11 @@ fn vs_main(@builtin(vertex_index) vi: u32) -> VsOut { @fragment fn fs_main(in: VsOut) -> @location(0) vec4 { - // The tile spans `tile_span` field units across [0,1) uv; `fbm_tile` wraps + // The tile spans `field_span` field units across [0,1) uv; `fbm_tile` wraps // its lattice at that same period, so the baked tile is seamless under the // repeat-wrapped sampler. - let coord = in.uv * params.tile_span; - let period = i32(params.tile_span); + let coord = in.uv * params.field_span; + let period = i32(params.field_span); if (params.channels == 0u) { let v = fbm_tile(coord, params.seed, params.octaves, params.gain, params.warp, period); return vec4(v, v, v, 1.0); diff --git a/crates/darkly/shaders/lib/fbm2d.wgsl b/crates/darkly/shaders/lib/fbm2d.wgsl index ccb8f976..db046408 100644 --- a/crates/darkly/shaders/lib/fbm2d.wgsl +++ b/crates/darkly/shaders/lib/fbm2d.wgsl @@ -154,6 +154,17 @@ fn fbm_seed_xform(seed: u32) -> vec3f { return vec3f(a, ox, oy); } +/// Seed → a 2D point scattered uniformly over `[0, period)²`. Two independent +/// `fbm_pcg` draws (the golden-ratio salt makes the y draw independent of the +/// x draw), so the point covers the plane rather than collapsing onto the +/// `x == y` diagonal a single scaled scalar would trace. Used to decorrelate +/// per-dab sampling of a period-`period` field. +fn fbm_offset2(seed: u32, period: f32) -> vec2f { + let ox = f32(fbm_pcg(seed)) / 4294967295.0; + let oy = f32(fbm_pcg(seed ^ 0x9e3779b9u)) / 4294967295.0; + return vec2f(ox, oy) * period; +} + /// Domain-warped, **tileable** fBm of value noise. The lattice wraps at /// `base_period` cells (and `base_period * 2^i` per octave, so every octave is /// co-periodic), giving a field that is exactly seamless with period diff --git a/crates/darkly/src/brush/eval.rs b/crates/darkly/src/brush/eval.rs index 059dd475..cdfe1dab 100644 --- a/crates/darkly/src/brush/eval.rs +++ b/crates/darkly/src/brush/eval.rs @@ -14,6 +14,7 @@ use crate::nodegraph::{ use super::curve_math::CurveLut; use super::nodes::{brush_settings, clone_source, paint_color, pen_input}; +use super::DAB_REFERENCE_SIZE; use super::gpu_context::BrushGpuContext; use super::wgsl::CompiledBrush; @@ -470,6 +471,16 @@ pub struct BrushGraphRunner { /// Pre-resolved slot index for paint_color's output. Same rationale /// as `pen_input_slots` — avoid plan traversal on the hot path. paint_color_slot: Option, + /// Pre-resolved slot index of `brush_settings.size`'s source output. + /// Seeded per dab in `seed_sensors` with the base size projected to + /// canvas pixels (`base_size * DAB_REFERENCE_SIZE` — the brush + /// *diameter*), so the graph signal is a pixel-domain quantity like + /// `pen_input.position`. This is why `brush_settings` is skipped in + /// `execute_cpu`'s generic settable-source republish (which would + /// otherwise overwrite it with the raw normalized knob value): the + /// projection lives here, at the one seeding point, keeping the CPU + /// slot and the GPU-packed dab field numerically identical. + brush_settings_size_slot: Option, /// Node id of the (first) `clone_source` node, resolved at build. /// When set alongside [`Self::clone_state`], `build_slot_outputs` /// injects the stroke's anchor uniforms under this node's keys so the @@ -606,6 +617,16 @@ impl BrushGraphRunner { .and_then(|s| s.output_slots.iter().find(|(name, _)| name == "color")) .map(|(_, slot)| *slot); + // Find brush_settings' `size` source output slot — seeded per dab in + // pixels (see the field doc). The `size` port is a settable-source, so + // the compiler assigns it an output slot even though it's an input. + let brush_settings_size_slot = plan + .steps + .iter() + .find(|s| s.type_id == brush_settings::TYPE_ID) + .and_then(|s| s.output_slots.iter().find(|(name, _)| name == "size")) + .map(|(_, slot)| *slot); + // Find the (first) clone_source node for anchor-uniform seeding. let clone_source_node = plan .steps @@ -642,6 +663,7 @@ impl BrushGraphRunner { node_data, pen_input_slots, paint_color_slot, + brush_settings_size_slot, clone_source_node, clone_state: None, dab_size_slot, @@ -786,6 +808,16 @@ impl BrushGraphRunner { if let Some(slot) = self.paint_color_slot { self.slots[slot] = Some(ScalarValue::Vec4(color)); } + + // Seed the `brush_settings.size` graph signal in canvas pixels: the + // stroke's base size projected to the brush diameter. Downstream nodes + // (noise.scale, math) then operate in the same pixel domain as their + // sinks, so a `× 1` gain is a true no-op and feature size tracks the + // brush directly. + if let Some(slot) = self.brush_settings_size_slot { + let size_px = self.base_size * DAB_REFERENCE_SIZE as f32; + self.slots[slot] = Some(ScalarValue::Scalar(size_px)); + } } /// Execute all CPU nodes in topological order. @@ -802,8 +834,13 @@ impl BrushGraphRunner { let step = &self.plan.steps[idx]; if step.type_id == pen_input::TYPE_ID || step.type_id == paint_color::TYPE_ID + || step.type_id == brush_settings::TYPE_ID || step.is_gpu { + // `brush_settings` is seeded bespoke in `seed_sensors` (its + // `size` source in pixels); its only other job is publishing + // that signal, so the generic republish below must not run and + // overwrite it with the raw normalized knob value. continue; } diff --git a/crates/darkly/src/brush/nodes/brush_settings.rs b/crates/darkly/src/brush/nodes/brush_settings.rs index 913ec542..ac5296f8 100644 --- a/crates/darkly/src/brush/nodes/brush_settings.rs +++ b/crates/darkly/src/brush/nodes/brush_settings.rs @@ -12,10 +12,14 @@ //! `size` is additionally a **settable-source** (`.source()`): any node can //! wire from `brush_settings.size` to read the stroke's base size as a graph //! signal, and the editor hides the source handle once the port is driven. -//! Because this is an ordinary CPU node (not special-cased in the runner like -//! `pen_input`), the runner's generic settable-source seeding publishes -//! `size`'s value on its slot every dab, and [`Self::compile_wgsl`] packs it -//! into a dab field when — and only when — a node consumes it. +//! The signal is published **in canvas pixels** — the brush *diameter* +//! (`base_size * DAB_REFERENCE_SIZE`), not the raw `0..4` normalized knob — so +//! it lands in the same pixel domain as its sinks (e.g. `noise.scale`) and a +//! `× 1` gain wired between them is a true no-op. Like `pen_input`, the runner +//! seeds this slot bespoke (in `seed_sensors`, where the base size and the +//! pixel projection live) rather than through the generic settable-source +//! republish; [`Self::compile_wgsl`] then packs the already-projected slot +//! value into a dab field when — and only when — a node consumes it. use std::sync::Arc; @@ -165,9 +169,9 @@ pub fn register() -> BrushNodeRegistration { ) } -/// No-op evaluator. `size` is published on its slot by the runner's generic -/// settable-source seeding; `stabilize`/`spacing`/`spacing_min_px` are read -/// out-of-band and never flow through the graph. +/// No-op evaluator. `size` is published on its slot by the runner's bespoke +/// `seed_sensors` seeding (in canvas pixels); `stabilize`/`spacing`/ +/// `spacing_min_px` are read out-of-band and never flow through the graph. pub struct BrushSettingsEvaluator; impl BrushNodeEvaluator for BrushSettingsEvaluator { @@ -177,8 +181,9 @@ impl BrushNodeEvaluator for BrushSettingsEvaluator { /// Emit the `size` settable-source as a per-dab field so it reaches a /// compiled brush's fragment shader — but only when a node consumes it, - /// so an untapped size costs nothing. The value is packed from the slot - /// the runner's generic source-seeding wrote (the ambient base size). + /// so an untapped size costs nothing. The value is packed straight from + /// the slot the runner seeded — the base size already projected to canvas + /// pixels — so the GPU dab field matches the CPU slot exactly. fn compile_wgsl(&self, cctx: &CompileWgslCtx) -> Result { let mut wgsl = NodeWgsl::default(); if cctx.consumed_outputs.contains("size") { diff --git a/crates/darkly/src/brush/nodes/image.rs b/crates/darkly/src/brush/nodes/image.rs index e66b9633..9fbc0624 100644 --- a/crates/darkly/src/brush/nodes/image.rs +++ b/crates/darkly/src/brush/nodes/image.rs @@ -84,12 +84,6 @@ pub fn register() -> BrushNodeRegistration { .with_value(InputValue::Int(1)) .with_label("Space") .with_description("Sample in canvas space (pinned) or the dab's oriented frame."), - // Dab-space only: `true` scales the picture with the brush, - // `false` keeps its texel density constant in canvas pixels. - PortDef::input("scale_with_brush", BrushWireType::Bool) - .with_value(InputValue::Bool(true)) - .with_label("Scale With Brush") - .with_description("Dab space only: scale the picture with the brush size."), PortDef::output("color", BrushWireType::Vec4) .preview_image() .with_description("RGBA value sampled from the named texture at the fragment's sample position"), @@ -130,15 +124,17 @@ impl BrushNodeEvaluator for ImageEvaluator { // expression (a literal when unwired, an upstream expr when wired). let scale_expr = cctx.input("scale").as_f32(); let space = SampleFrame::from_index(cctx.input("space").enum_index().max(0) as u32); - let scale_with_brush = cctx.input("scale_with_brush").boolean(); let rotation = cctx.input("rotation").as_f32(); let variation = cctx.input("variation").as_f32(); + // The sampled uv is `fract`-wrapped against a repeat sampler, so the + // texture is effectively periodic with period 1.0 — the per-dab + // decorrelation offset must span exactly one period. let (frame_pre, coord) = frame_sample_coord_expr( space, &scale_expr, - scale_with_brush, &rotation, &variation, + 1.0, &cctx.ident("img"), ); @@ -164,15 +160,14 @@ mod tests { let reg = register(); assert_eq!(reg.node.type_id, "image"); assert_eq!(reg.node.category, "texture"); - // rotation, variation, texture_name, scale, space, scale_with_brush - // inputs plus the color output — all unified as ports now. - assert_eq!(reg.node.ports.len(), 7); + // rotation, variation, texture_name, scale, space inputs plus the + // color output — all unified as ports now. + assert_eq!(reg.node.ports.len(), 6); assert!(reg.node.ports.iter().any(|p| p.name == "color")); assert!(reg.node.ports.iter().any(|p| p.name == "rotation")); assert!(reg.node.ports.iter().any(|p| p.name == "variation")); assert!(reg.node.ports.iter().any(|p| p.name == "texture_name")); assert!(reg.node.ports.iter().any(|p| p.name == "scale")); assert!(reg.node.ports.iter().any(|p| p.name == "space")); - assert!(reg.node.ports.iter().any(|p| p.name == "scale_with_brush")); } } diff --git a/crates/darkly/src/brush/nodes/noise.rs b/crates/darkly/src/brush/nodes/noise.rs index 7527b729..9d53a881 100644 --- a/crates/darkly/src/brush/nodes/noise.rs +++ b/crates/darkly/src/brush/nodes/noise.rs @@ -22,9 +22,9 @@ //! stroke mode, preview-mask texels in preview — so the grain is pinned to //! the canvas. **Dab** samples the stamp's oriented unit frame (`local_uv` //! rotated by the `rotation` input, offset per dab by `variation`), so the -//! grain rides the stamp instead of swimming under it. `scale_with_brush` -//! chooses whether Dab-frame grain scales with the brush or stays -//! pixel-locked. +//! grain rides the stamp instead of swimming under it. In both frames `scale` +//! is a canvas-pixel feature size; to make the grain scale with the brush, +//! drive `scale` from `brush_settings.size` (also canvas pixels). //! //! The math (`fbm_value_noise`, `fbm_seed_xform`, `fbm_tile`, hash, fade) //! lives in the shared `shaders/lib/fbm2d.wgsl`, concatenated into every @@ -123,12 +123,6 @@ pub fn register() -> BrushNodeRegistration { .with_value(InputValue::Int(0)) .with_label("Space") .with_description("Pin the grain to the canvas, or lock it to each dab."), - // Dab-space only: `true` scales the grain with the brush, - // `false` keeps grain density constant in canvas pixels. - PortDef::input("scale_with_brush", BrushWireType::Bool) - .with_value(InputValue::Bool(true)) - .with_label("Scale With Brush") - .with_description("Dab space only: scale the grain with the brush size."), PortDef::output("color", BrushWireType::Vec4) .preview_image() .with_description( @@ -174,21 +168,20 @@ impl BrushNodeEvaluator for NoiseEvaluator { return Ok(wgsl); } // The sampling frame is shared by the baked and the live path. - // `scale`/`space`/`scale_with_brush`/`rotation`/`variation` shape the - // sample *coordinate*, not the field content — so they are applied - // here at sample time and one baked tile serves Canvas and Dab, every - // scale and every per-dab variation. + // `scale`/`space`/`rotation`/`variation` shape the sample *coordinate*, + // not the field content — so they are applied here at sample time and + // one baked tile serves Canvas and Dab, every scale and every per-dab + // variation. let scale_expr = cctx.input("scale").as_f32(); let space = SampleFrame::from_index(cctx.input("space").enum_index().max(0) as u32); - let scale_with_brush = cctx.input("scale_with_brush").boolean(); let rotation = cctx.input("rotation").as_f32(); let variation = cctx.input("variation").as_f32(); let (frame_pre, coord) = frame_sample_coord_expr( space, &scale_expr, - scale_with_brush, &rotation, &variation, + BakeSpec::FIELD_SPAN, &cctx.ident("noise"), ); // `seed` is a compile-time integer in both paths (a wired `Int` @@ -228,12 +221,13 @@ impl BrushNodeEvaluator for NoiseEvaluator { }; let resolution = BakeSpec::resolution_for_octaves(octaves); - // The tile packs `TILE_SPAN` field units across its `[0,1)` uv, so - // the sample coordinate is divided by `TILE_SPAN` (and wrapped): + // The tile packs `FIELD_SPAN` field units across its `[0,1)` uv, so + // the sample coordinate is divided by `FIELD_SPAN` (and wrapped): // feature size then matches the live path, and the field repeats - // once per `TILE_SPAN` field units — the seam lever (§ plan). + // once per `FIELD_SPAN` field units — a period large enough that the + // repeat is not visible within a normal view. let mut body = format!("{frame_pre} let {var}_p = {coord};\n"); - let uv = format!("fract(({var}_p) / {:.1})", BakeSpec::TILE_SPAN); + let uv = format!("fract(({var}_p) / {:.1})", BakeSpec::FIELD_SPAN); if want_value { let slot = cctx.request_source(ResolvedSource::Baked(BakeSpec { kind, @@ -275,7 +269,7 @@ impl BrushNodeEvaluator for NoiseEvaluator { // then let each consumed output reference `{var}_p`. The live field // uses the same tileable `fbm_tile` and the same period the baked path // does, so a wired-param brush and a static one look identical. - let period = BakeSpec::TILE_SPAN as i32; + let period = BakeSpec::FIELD_SPAN as i32; let mut body = format!("{frame_pre} let {var}_p = {coord};\n"); if want_value { @@ -316,22 +310,14 @@ mod tests { let reg = register(); assert_eq!(reg.node.type_id, "noise"); assert_eq!(reg.node.category, "texture"); - // rotation, variation, scale, seed, octaves, warp, roughness, space, - // scale_with_brush inputs plus the color and value outputs — all unified. - assert_eq!(reg.node.ports.len(), 11); + // rotation, variation, scale, seed, octaves, warp, roughness, space + // inputs plus the color and value outputs — all unified. + assert_eq!(reg.node.ports.len(), 10); assert!(reg.node.ports.iter().any(|p| p.name == "color")); assert!(reg.node.ports.iter().any(|p| p.name == "value")); assert!(reg.node.ports.iter().any(|p| p.name == "rotation")); assert!(reg.node.ports.iter().any(|p| p.name == "variation")); - for name in [ - "scale", - "seed", - "octaves", - "warp", - "roughness", - "space", - "scale_with_brush", - ] { + for name in ["scale", "seed", "octaves", "warp", "roughness", "space"] { assert!( reg.node.ports.iter().any(|p| p.name == name), "missing {name}" diff --git a/crates/darkly/src/brush/nodes/watercolor.rs b/crates/darkly/src/brush/nodes/watercolor.rs index 614551e0..79ff559a 100644 --- a/crates/darkly/src/brush/nodes/watercolor.rs +++ b/crates/darkly/src/brush/nodes/watercolor.rs @@ -5,12 +5,15 @@ //! with one extra pass at the front: //! //! 1. **Pickup atlas pass.** N instances, each writes the 8×8 alpha- -//! weighted neighborhood average of `pre_stroke_texture` at the -//! dab's footprint into its cell in a 128×128 atlas. The shader -//! (`watercolor_pickup.wgsl`) is brush-agnostic in math -//! but built per-brush so its `DabRecord` struct stride matches -//! the compiled brush's. Cell layout is `(idx % atlas_w, idx / -//! atlas_w)`. +//! weighted neighborhood average of the *live canvas* at the dab's +//! footprint into its cell in a 128×128 atlas. Live canvas means +//! `pre_stroke_texture` with the stroke scratch composited over it — +//! the dry layer plus the wet paint deposited so far — which is what +//! lets a mark keep building where the brush passes more than once. +//! Reading the scratch here is legal because this pass targets the +//! atlas, not the scratch. The shader is brush-agnostic in math but +//! built per-brush so its `DabRecord` struct stride matches the +//! compiled brush's. Cell layout is `(idx % atlas_w, idx / atlas_w)`. //! 2. **Composite pass.** One instanced draw, N quads. The fragment //! shader is the framework-assembled per-brush WGSL: upstream //! nodes (`circle`, `paint_color`, etc.) compile inline; this @@ -60,6 +63,18 @@ use crate::nodegraph::{NodeRegistration, PortDef, UnitType}; const ATLAS_WIDTH: u32 = 128; const ATLAS_HEIGHT: u32 = 128; +/// The atlas holds one cell per dab, addressed by instance index as +/// `(idx % ATLAS_WIDTH, idx / ATLAS_WIDTH)`. If it ever holds fewer cells +/// than a phase can queue, dab `ATLAS_WIDTH * ATLAS_HEIGHT` silently +/// aliases onto cell 0 and picks up the wrong canvas colour — no panic, no +/// validation error, just wrong pixels. The two constants are currently +/// equal, so this is exactly load-bearing. +const _: () = assert!( + (ATLAS_WIDTH as u64) * (ATLAS_HEIGHT as u64) >= MAX_DABS_PER_PHASE as u64, + "watercolor pickup atlas has fewer cells than MAX_DABS_PER_PHASE; \ + grow the atlas or lower the cap", +); + const MAX_UNIFORM_BYTES: usize = 1024; // ── Pickup uniforms ───────────────────────────────────────────────────── @@ -169,6 +184,7 @@ impl PerBrushPipeline { Some(ctx.uniform_bgl), Some(&dabs_bgl), Some(ctx.canvas_copy_bgl), // pre_stroke texture+sampler + Some(ctx.canvas_copy_bgl), // stroke scratch texture+sampler ], immediate_size: 0, }); @@ -431,6 +447,9 @@ fn watercolor_pipeline_reg() -> BrushPipelineRegistration { /// generated per brush — the file-level shader-compile test parses /// every `.wgsl` in isolation and a placeholder-bearing template /// fails that pass. +/// +/// Depends on `source_over` from `shaders/source_over.wgsl`, which +/// [`build_pickup_shader`] prepends. const PICKUP_SHADER_TAIL: &str = r#" struct PickupUniforms { pre_stroke_origin: vec2, @@ -445,6 +464,11 @@ struct PickupUniforms { @group(1) @binding(0) var dabs: array; @group(2) @binding(0) var t_pre_stroke: texture_2d; @group(2) @binding(1) var s_pre_stroke: sampler; +// The in-flight stroke scratch — premultiplied paint deposited so far this +// stroke. Sampling it here is safe: this pass renders to the atlas, so the +// scratch is read-only for the duration and there is no read/write alias. +@group(3) @binding(0) var t_scratch: texture_2d; +@group(3) @binding(1) var s_scratch: sampler; struct VertexOutput { @builtin(position) position: vec4, @@ -513,9 +537,15 @@ fn fs_main(in: VertexOutput) -> @location(0) vec4 { if (uv.x < 0.0 || uv.x > 1.0 || uv.y < 0.0 || uv.y > 1.0) { continue; } - let s = textureSampleLevel(t_pre_stroke, s_pre_stroke, uv, 0.0); - sum_rgb = sum_rgb + s.rgb * s.a; - sum_a = sum_a + s.a; + // The canvas the brush is actually touching: wet paint from this + // stroke over the dry layer beneath it. Reading only the dry + // layer would freeze the load for the whole stroke, so a mark + // could never build past its first pass. + let dry = textureSampleLevel(t_pre_stroke, s_pre_stroke, uv, 0.0); + let wet = textureSampleLevel(t_scratch, s_scratch, uv, 0.0); + let live = source_over(wet.rgb, wet.a, dry); + sum_rgb = sum_rgb + live.rgb * live.a; + sum_a = sum_a + live.a; } } let avg_rgb = select(vec3(0.0), sum_rgb / sum_a, sum_a > 0.0001); @@ -529,7 +559,9 @@ fn fs_main(in: VertexOutput) -> @location(0) vec4 { /// must match the brush's dab layout — so each brush gets its own /// pickup pipeline with the matching struct definition prepended. fn build_pickup_shader(compiled: &CompiledBrush) -> String { - let mut out = String::with_capacity(PICKUP_SHADER_TAIL.len() + 256); + let mut out = String::with_capacity(PICKUP_SHADER_TAIL.len() + 1024); + out.push_str(include_str!("../../../shaders/source_over.wgsl")); + out.push('\n'); out.push_str("struct DabRecord {\n"); for f in &compiled.dab_layout { out.push_str(&format!(" {}: {},\n", f.name, f.ty.wgsl_name())); @@ -753,6 +785,16 @@ impl BrushNodeEvaluator for WatercolorEvaluator { // default (or the value the brush graph baked into the port). // A wired-per-dab `pickup_size` would need to flow through the // dab record; not in scope. + // The pickup shader maps both `t_pre_stroke` and `t_scratch` with one + // set of origin/size uniforms, which is only valid because the two + // share the layer frame. `StrokeBuffer` grows them together. + debug_assert_eq!( + scratch.write_dimensions(), + (pre_stroke_size[0], pre_stroke_size[1]), + "scratch and pre_stroke must share the layer frame — the pickup \ + shader derives both UVs from `pre_stroke_origin`/`pre_stroke_size`", + ); + let pickup_size = ctx.input_f32("pickup_size").clamp(0.0, 2.0); let pickup_uniforms = PickupUniforms { pre_stroke_origin, @@ -799,6 +841,7 @@ impl BrushNodeEvaluator for WatercolorEvaluator { pass.set_bind_group(0, &per_brush.pickup_uniform_bind_group, &[pickup_offset]); pass.set_bind_group(1, &per_brush.dabs_bind_group_pickup, &[]); pass.set_bind_group(2, pre_stroke_bg, &[]); + pass.set_bind_group(3, scratch.live_canvas_bind_group(), &[]); pass.draw(0..6, 0..total_dabs); } diff --git a/crates/darkly/src/brush/scratch.rs b/crates/darkly/src/brush/scratch.rs index 1806bb2e..0f93e89c 100644 --- a/crates/darkly/src/brush/scratch.rs +++ b/crates/darkly/src/brush/scratch.rs @@ -23,9 +23,21 @@ //! //! The two sides are managed atomically by this type — there is no public //! API by which a caller can resize one without going through `Scratch`. -//! The R/W hazard is internal; consumers see a single object that handles -//! the WebGPU quirk and call [`Scratch::sync_read_mirror`] with the dab -//! footprint when they need to read the in-flight scratch state. +//! +//! There are two ways to read the in-flight scratch, and which one applies +//! is decided by the reader's own render target: +//! +//! - A pass that **also writes** the scratch (its color attachment is +//! [`Scratch::write_view`]) must go through [`Scratch::sync_read_mirror`]. +//! Sampling the write side from such a pass is the R/W alias WebGPU +//! forbids; the mirror is what makes it legal. +//! - A pass that **does not** target the scratch may sample the write side +//! directly via [`Scratch::live_canvas_bind_group`] — no alias, no copy. +//! Watercolor's pickup atlas pass does this: it renders to the atlas and +//! reads the scratch to see the wet paint under each dab. +//! +//! The mirror is the more expensive of the two (a `copy_texture_to_texture` +//! per dab), so prefer the direct read whenever the target allows it. //! //! Ownership: owned by `StrokeBuffer`, allocated at stroke start, freed at //! stroke end. @@ -204,6 +216,15 @@ impl Scratch { pub fn read_mirror_bind_group(&self) -> &wgpu::BindGroup { &self.read_mirror_bind_group } + /// The write side bound for sampling, for passes that read the + /// in-flight stroke pixels **without** targeting the scratch — see the + /// module docs for which of the two read paths applies. Callers whose + /// render target *is* the scratch must use + /// [`Scratch::sync_read_mirror`] instead; sampling here from such a + /// pass is the read/write alias WebGPU forbids. + pub fn live_canvas_bind_group(&self) -> &wgpu::BindGroup { + &self.write_bind_group + } pub fn read_mirror_texture(&self) -> &wgpu::Texture { &self.read_mirror_texture } diff --git a/crates/darkly/src/brush/texture_source.rs b/crates/darkly/src/brush/texture_source.rs index a7049a75..f60d80ec 100644 --- a/crates/darkly/src/brush/texture_source.rs +++ b/crates/darkly/src/brush/texture_source.rs @@ -70,24 +70,38 @@ pub struct BakeSpec { } impl BakeSpec { - /// How many field units the baked tile spans on each axis. This is the - /// seam / decorrelation lever (§ plan): a large span relative to the - /// feature size means the repeat-wrap boundary is crossed rarely per - /// unit canvas area, so the seam reads as soft, and `variation`-offset - /// dabs land on effectively random tile phases. The bake shader maps a - /// texel `uv ∈ [0,1)` to `uv * TILE_SPAN` before calling the fBm. - pub const TILE_SPAN: f32 = 16.0; + /// How many field units the baked tile spans on each axis — equivalently, + /// the field's **repeat period**: sampled through the Repeat sampler the + /// tile wraps once per `FIELD_SPAN` field units. Made large so the field + /// does not visibly repeat within a normal view. The bake shader maps a + /// texel `uv ∈ [0,1)` to `uv * FIELD_SPAN` before calling the fBm. + /// + /// Independent of [`resolution_for_octaves`](Self::resolution_for_octaves): + /// the period sets how far the fixed texel budget is stretched across the + /// plane, not how many texels the tile holds. Enlarging it costs no memory + /// — it softens fine detail instead (the texels cover more field units). + pub const FIELD_SPAN: f32 = 128.0; + + /// Reference detail window (field units) the tile is sized to resolve at + /// Nyquist — the detail axis, held **separate** from [`FIELD_SPAN`] (the + /// period axis) so neither constant is overloaded. The tile holds enough + /// texels to resolve the finest octave across *this* window; the larger + /// real [`FIELD_SPAN`] stretches those texels further, so fine octaves + /// soften rather than the tile growing. Raise this toward `FIELD_SPAN` + /// (and the memory clamp) to trade memory for sharpness across the span. + const DETAIL_SPAN: u32 = 16; /// Tile edge resolution (texels) that resolves the finest fBm frequency - /// for `octaves`, clamped to a sane memory band. With a base cell of 1 - /// field unit and octaves doubling frequency, the finest feature is - /// `TILE_SPAN / 2^(octaves-1)` field units; at ~2 texels per finest - /// half-feature that is `TILE_SPAN * 2^(octaves-1) * 2` texels. Clamped - /// to `[512, 2048]` (1–16 MiB RGBA8; ¼ that for R8), trading fine - /// detail at high octaves for bounded memory. + /// for `octaves` across [`DETAIL_SPAN`], clamped to a sane memory band. + /// With a base cell of 1 field unit and octaves doubling frequency, the + /// finest feature is `DETAIL_SPAN / 2^(octaves-1)` field units; at ~2 + /// texels per finest half-feature that is `DETAIL_SPAN * 2^(octaves-1) * + /// 2` texels. Clamped to `[512, 2048]` (1–16 MiB RGBA8; ¼ that for R8), + /// trading fine detail at high octaves for bounded memory. Deliberately + /// does not scale with [`FIELD_SPAN`] — see there. pub fn resolution_for_octaves(octaves: i32) -> u32 { let finest = 1u32 << (octaves.clamp(1, 8) - 1) as u32; - (Self::TILE_SPAN as u32 * finest * 2).clamp(512, 2048) + (Self::DETAIL_SPAN * finest * 2).clamp(512, 2048) } } diff --git a/crates/darkly/src/brush/wgsl/sample_frame.rs b/crates/darkly/src/brush/wgsl/sample_frame.rs index cfd6066c..8eefc306 100644 --- a/crates/darkly/src/brush/wgsl/sample_frame.rs +++ b/crates/darkly/src/brush/wgsl/sample_frame.rs @@ -45,21 +45,29 @@ impl SampleFrame { /// `variation_expr` are the node's own input expressions (screen-relative /// radians and a per-dab decorrelation scalar respectively). /// -/// `scale_with_brush` is meaningful only in [`SampleFrame::Dab`]: `true` -/// samples the radius-normalized unit-disc frame so the pattern scales with -/// the brush; `false` reconstructs pixel offsets so grain density stays -/// constant in canvas pixels as the brush grows. It is ignored for Canvas. +/// In [`SampleFrame::Dab`] the field is sampled in oriented dab-pixels, so +/// `scale` is a canvas-pixel feature size exactly as in Canvas space, and grain +/// density stays constant in pixels as the brush grows. To make the grain scale +/// *with* the brush instead, drive `scale` from `brush_settings.size` (also in +/// canvas pixels): the dab radius and the scale then grow together and cancel. /// /// `scale_expr` is the caller's `scale` **input expression** — a `{:.6}` /// literal when the scale input is unwired, or an upstream WGSL expression /// when it's driven per-dab. It is interpolated parenthesized so a wired /// expression composes correctly inside the divide. +/// +/// `period` is the repeat period of the field the caller samples, in the same +/// units as `coord` (Dab space only). The per-dab decorrelation offset is a +/// 2D hash of `variation` scattered over `[0, period)²`, so it lands on a fresh +/// phase of the field per dab without resonating with its repeat. Callers pass +/// their field's period (e.g. the baked-tile span for `noise`, `1.0` for an +/// `fract`-wrapped texture). Ignored for Canvas. pub fn frame_sample_coord_expr( space: SampleFrame, scale_expr: &str, - scale_with_brush: bool, rotation_expr: &str, variation_expr: &str, + period: f32, ident: &str, ) -> (String, String) { match space { @@ -78,25 +86,26 @@ pub fn frame_sample_coord_expr( \x20 -local_uv.x * {ident}_sa + local_uv.y * {ident}_ca,\n\ \x20 );\n" ); - // `variation` walks overlapping dabs to disjoint regions of the - // continuous field; the large stride keeps adjacent values (from - // `random`) landing in uncorrelated regions. - let offset = format!("vec2(({variation_expr}) * 64.0, ({variation_expr}) * 64.0)"); - let coord = if scale_with_brush { - // Stamp-relative frequency: the unit-disc offset spans ~[-1,1] - // across the stamp at any brush size, so the same pattern maps - // across the whole stamp — the grain scales with the brush. - format!("{ident}_dab_local / ({scale_expr}) + {offset}") - } else { - // Pixel-locked frequency: multiply the unit-disc offset back to - // oriented dab-pixels so grain density stays constant in canvas - // px as the brush grows (the stamp is a bigger window onto the - // same grain). - preamble.push_str(&format!( - " let {ident}_radius_px = 1.0 / d.inv_radius_target_px;\n" - )); - format!("({ident}_dab_local * {ident}_radius_px) / ({scale_expr}) + {offset}") - }; + // Per-dab 2D decorrelation: hash `variation` into two independent + // components (via `fbm_offset2`, from the always-prepended + // `fbm2d.wgsl`) so overlapping dabs sample uncorrelated regions of + // the periodic field, bounded to one `period` so it can't resonate + // with the field's repeat. `max(.., 0.0)` keeps the `u32` cast + // well-defined for any wired input. + preamble.push_str(&format!( + " let {ident}_off = fbm_offset2(u32(max(({variation_expr}), 0.0) * 4096.0), {period:.6});\n" + )); + let offset = format!("{ident}_off"); + // Multiply the unit-disc offset back to oriented dab-pixels, so + // `scale` is a canvas-pixel feature size and grain density stays + // constant in px as the brush grows. Wiring `brush_settings.size` + // (also px) into `scale` makes the radius and scale cancel, so the + // grain scales with the brush. + preamble.push_str(&format!( + " let {ident}_radius_px = 1.0 / d.inv_radius_target_px;\n" + )); + let coord = + format!("({ident}_dab_local * {ident}_radius_px) / ({scale_expr}) + {offset}"); (preamble, coord) } } @@ -111,9 +120,9 @@ mod tests { let (pre, coord) = frame_sample_coord_expr( SampleFrame::Canvas, "32.000000", - true, "d.n1_rotation", "d.n2_variation", + 16.0, "noise_3", ); assert!(pre.is_empty(), "Canvas emits no preamble"); @@ -126,41 +135,48 @@ mod tests { } #[test] - fn dab_scale_with_brush_normalizes_unit_disc() { - let (pre, coord) = - frame_sample_coord_expr(SampleFrame::Dab, "8.000000", true, "1.5", "0.0", "noise_3"); + fn dab_rotates_local_uv_by_rotation_expr() { + let (pre, _coord) = + frame_sample_coord_expr(SampleFrame::Dab, "8.000000", "1.5", "0.0", 16.0, "noise_3"); // Oriented basis rotates local_uv by the rotation expression. assert!(pre.contains("cos(1.5)")); assert!(pre.contains("sin(1.5)")); assert!(pre.contains("noise_3_dab_local")); assert!(pre.contains("local_uv.x * noise_3_ca + local_uv.y * noise_3_sa")); - // scale_with_brush=true divides the unit-disc offset directly and - // never reconstructs pixels. - assert!(coord.contains("noise_3_dab_local / (8.000000)")); - assert!(!coord.contains("inv_radius_target_px")); - assert!(!pre.contains("inv_radius_target_px")); } #[test] - fn dab_pixel_locked_reconstructs_radius() { + fn dab_reconstructs_radius_so_scale_is_canvas_pixels() { let (pre, coord) = - frame_sample_coord_expr(SampleFrame::Dab, "8.000000", false, "0.0", "0.0", "img_5"); - // scale_with_brush=false multiplies back to oriented dab-pixels. + frame_sample_coord_expr(SampleFrame::Dab, "8.000000", "0.0", "0.0", 1.0, "img_5"); + // Dab space always multiplies the unit-disc offset back to oriented + // dab-pixels, so `scale` is a canvas-pixel feature size. assert!(pre.contains("let img_5_radius_px = 1.0 / d.inv_radius_target_px;")); assert!(coord.contains("(img_5_dab_local * img_5_radius_px) / (8.000000)")); } #[test] - fn dab_folds_variation_offset() { - let (_pre, coord) = frame_sample_coord_expr( + fn dab_variation_offset_is_2d_and_period_bounded() { + let (pre, coord) = frame_sample_coord_expr( SampleFrame::Dab, "8.000000", - true, "0.0", "d.n2_variation", + 16.0, "noise_3", ); - assert!(coord.contains("vec2((d.n2_variation) * 64.0, (d.n2_variation) * 64.0)")); + // Defects 1+2: the offset is a 2D hash of `variation` bounded to the + // caller's field period (16) — not the same scalar on both axes, and + // not a `* 64.0` stride that resonates with period 16. `fbm_offset2` + // (fbm2d.wgsl) draws x and y from two different PCG inputs by + // construction, so referencing it *is* the 2D guarantee. + assert!(pre.contains( + "let noise_3_off = fbm_offset2(u32(max((d.n2_variation), 0.0) * 4096.0), 16.000000)" + )); + assert!(coord.contains("+ noise_3_off")); + assert!(!coord.contains("* 64.0")); + // Old diagonal form must be gone. + assert!(!coord.contains("d.n2_variation) * 64.0, (d.n2_variation)")); } #[test] diff --git a/crates/darkly/src/clipboard.rs b/crates/darkly/src/clipboard.rs index b82e49cb..1829686b 100644 --- a/crates/darkly/src/clipboard.rs +++ b/crates/darkly/src/clipboard.rs @@ -49,6 +49,33 @@ impl Clipboard { _ => None, } } + + /// The clip's pixels as `(rgba, width, height, offset_x, offset_y)`, + /// regardless of variant. A flat `ImageData` clip returns its buffer + /// directly; a rich `Layer` clip decodes its base64 pixels. Used by the + /// paste-in-place floating path so it works for the `Layer` clip a normal + /// copy produces, not just flat image clips. Returns `None` if a rich + /// clip's pixels are malformed. + pub fn paste_pixels(&self) -> Option<(Vec, u32, u32, i32, i32)> { + match self { + Clipboard::ImageData(c) => { + Some((c.data.clone(), c.width, c.height, c.offset_x, c.offset_y)) + } + Clipboard::Layer(l) => { + let pixels = l.decode_pixels().ok()?; + if pixels.len() != (l.bounds.width * l.bounds.height * 4) as usize { + return None; + } + Some(( + pixels, + l.bounds.width, + l.bounds.height, + l.bounds.x, + l.bounds.y, + )) + } + } + } } // --------------------------------------------------------------------------- diff --git a/crates/darkly/src/document/mod.rs b/crates/darkly/src/document/mod.rs index b2564059..5159f34b 100644 --- a/crates/darkly/src/document/mod.rs +++ b/crates/darkly/src/document/mod.rs @@ -906,74 +906,59 @@ impl Document { .is_some_and(|m| m.common.visible) } - /// Move a node to a new position in the tree. + /// Move a tree node to a new position. Filters have no position in the + /// tree — they hang off a host — so a filter id is refused rather than + /// smuggled into some group's children. pub fn move_layer(&mut self, layer_id: LayerId, target: MoveTarget) { - if self.unlink_node(layer_id).is_none() { + if self.is_filter(layer_id) { + return; + } + if self.unlink(layer_id).is_none() { return; } self.attach_at_target(layer_id, target); } - /// Detach a node from the tree for undo purposes, leaving it parked in - /// `entities` so its id stays stable across undo/redo. Returns the id on - /// success. Reattach with [`Document::reinsert_node`]; if the undo entry - /// is later discarded, call [`Document::remove_node`] to actually free it. + /// Detach an entity — tree node or filter — from its parent, leaving it + /// parked in `entities` so its id stays stable across undo/redo. Returns + /// the id on success. Reattach with [`Document::reinsert_entity`]; if the + /// undo entry is later discarded, call [`Document::remove_entity`] to + /// actually free it. pub fn detach_for_undo(&mut self, id: LayerId) -> Option { - self.unlink_node(id) + self.unlink(id) } - /// Reinsert a previously detached node at a specific position. - pub fn reinsert_node(&mut self, id: LayerId, parent: Option, position: usize) { + /// Reattach a previously detached entity under `parent` at `position`. + /// `parent` of `None` means the root. The entity's kind decides which of + /// the parent's lists it lands in, so a filter always returns to a + /// `filters` list and a node to a `children` list. + pub fn reinsert_entity(&mut self, id: LayerId, parent: Option, position: usize) { if !self.entities.contains_key(id) { return; } - let parent_id = self.resolve_parent_group(parent); - self.link_child(id, parent_id, Some(position)); - } - - /// Detach a filter from its host for undo purposes. The filter stays - /// in `entities`, so reattach via [`Document::reinsert_filter`] preserves - /// the id. - pub fn detach_filter_for_undo(&mut self, id: LayerId) -> Option { - self.unlink_filter(id) - } - - /// Reattach a previously detached filter to a host. Append-only — the - /// model doesn't expose a position parameter today because masks use - /// "first mask" lookup rather than positional access. - pub fn reinsert_filter(&mut self, filter_id: LayerId, host_id: LayerId) { - if !self.is_filter(filter_id) { - return; - } - if let Some(host) = self.find_node_mut(host_id) { - host.modifiers_mut().push(filter_id); - self.parent.insert(filter_id, host_id); - } + let parent_id = match parent { + Some(p) if self.find_node(p).is_some() => p, + _ => self.root, + }; + self.link(id, parent_id, Some(position)); } - /// Remove a node (layer or group) and everything beneath it (descendant - /// nodes, all filters on every node in the subtree) from `entities`. - /// Permanent — call [`Document::detach_for_undo`] instead if the caller - /// wants id-stable detach for undo. - pub fn remove_node(&mut self, id: LayerId) { + /// Permanently remove an entity — tree node or filter — from `entities`, + /// along with everything beneath it (descendant nodes, and all filters on + /// every node in the subtree). Call [`Document::detach_for_undo`] instead + /// when the caller wants an id-stable detach for undo. + pub fn remove_entity(&mut self, id: LayerId) { if id == self.root { return; } - // Unlink first so the parent's children Vec is consistent if anyone - // observes mid-purge. - self.unlink_node(id); - self.purge_subtree(id); - } - - /// Permanently remove a filter from `entities` (and its host's filter - /// list, if still attached). - pub fn remove_filter(&mut self, id: LayerId) { - self.unlink_filter(id); + // Unlink first so the parent's list is consistent if anyone observes + // mid-purge. + self.unlink(id); // Selection sentinel: if this was the selection, clear the field too. if self.selection == Some(id) { self.selection = None; } - self.entities.remove(id); + self.purge_subtree(id); } // --------------------------------------------------------------- @@ -990,40 +975,38 @@ impl Document { } } - /// Insert `child` into `group`'s children Vec at `position` (or at the end - /// if `None`), and update the parent map. Caller guarantees `child` is in - /// `entities` and `group` is a group. - fn link_child(&mut self, child: LayerId, group: LayerId, position: Option) { - let Some(LayerNode::Group(g)) = self.find_node_mut(group) else { + /// Link `child` under `parent`, into the list its kind belongs in — a + /// filter joins `parent`'s modifiers, a tree node joins its children — at + /// `position` (clamped) or at the end. No-op when `parent` can't hold the + /// child, so a leaf layer never acquires tree children. + fn link(&mut self, child: LayerId, parent: LayerId, position: Option) { + let slot = if self.is_filter(child) { + ChildSlot::Filter + } else { + ChildSlot::Child + }; + let Some(node) = self.find_node_mut(parent) else { return; }; - let pos = position - .map(|p| p.min(g.children.len())) - .unwrap_or(g.children.len()); - g.children.insert(pos, child); - self.parent.insert(child, group); + if node.attach_child(child, slot, position) { + self.parent.insert(child, parent); + } } - /// Unlink a node from its parent's children Vec and from the parent map. - /// Returns the node's id if it was linked, `None` if it was the root or - /// already orphaned. - fn unlink_node(&mut self, id: LayerId) -> Option { + /// Unlink an entity from its parent, whichever of the parent's two lists + /// held it, and drop the parent-map entry. Returns the id if it was + /// linked, `None` if it was the root or already orphaned. + /// + /// Kind-agnostic on purpose: the parent knows which list a child is in, so + /// no caller has to, and a filter can't be half-detached by being unlinked + /// through the wrong path. + fn unlink(&mut self, id: LayerId) -> Option { if id == self.root { return None; } let parent_id = self.parent.remove(id)?; - if let Some(LayerNode::Group(g)) = self.find_node_mut(parent_id) { - g.children.retain(|c| *c != id); - } - Some(id) - } - - /// Unlink a filter from its host's filters Vec and from the parent - /// map. Returns the filter's id if it was linked. - fn unlink_filter(&mut self, id: LayerId) -> Option { - let host_id = self.parent.remove(id)?; - if let Some(host) = self.find_node_mut(host_id) { - host.modifiers_mut().retain(|m| *m != id); + if let Some(parent) = self.find_node_mut(parent_id) { + parent.detach_child(id); } Some(id) } @@ -1063,9 +1046,9 @@ impl Document { .iter() .position(|c| *c == ref_id) .unwrap_or(0); - self.link_child(node, parent_id, Some(pos)); + self.link(node, parent_id, Some(pos)); } else { - self.link_child(node, self.root, None); + self.link(node, self.root, None); } } MoveTarget::After(ref_id) => { @@ -1076,18 +1059,18 @@ impl Document { .position(|c| *c == ref_id) .map(|p| p + 1) .unwrap_or_else(|| self.children_of(parent_id).len()); - self.link_child(node, parent_id, Some(pos)); + self.link(node, parent_id, Some(pos)); } else { - self.link_child(node, self.root, None); + self.link(node, self.root, None); } } MoveTarget::IntoGroupTop(group_id) => { let group = self.resolve_parent_group(Some(group_id)); - self.link_child(node, group, None); + self.link(node, group, None); } MoveTarget::IntoGroupBottom(group_id) => { let group = self.resolve_parent_group(Some(group_id)); - self.link_child(node, group, Some(0)); + self.link(node, group, Some(0)); } } } @@ -1289,7 +1272,7 @@ mod tests { let g1 = doc.add_group(None); let _l1 = doc.add_raster_layer(Some(g1)); - doc.remove_node(g1); + doc.remove_entity(g1); assert!(doc.flat_layers().is_empty()); } @@ -1327,7 +1310,7 @@ mod tests { let l = doc.add_raster_layer(None); let mod_id = doc.add_mask_filter(l).unwrap(); - doc.remove_filter(mod_id); + doc.remove_entity(mod_id); assert!(doc.filters_of(l).is_empty()); assert!(!doc.is_filter(mod_id)); // Truly purged from entities (not just unlinked). @@ -1372,7 +1355,7 @@ mod tests { // layer is allocated into the same slot. let mut doc = Document::new(256, 256); let stale = doc.add_raster_layer(None); - doc.remove_node(stale); + doc.remove_entity(stale); // Allocate something else; the slot may be recycled with a bumped // generation. The stale key must still not resolve. let _other = doc.add_raster_layer(None); @@ -1419,7 +1402,7 @@ mod tests { // Not in the tree. assert!(doc.flat_layers().is_empty()); - doc.reinsert_node(l, parent, pos); + doc.reinsert_entity(l, parent, pos); assert_eq!(doc.parent_of(l), parent.or(Some(doc.root))); assert_eq!(doc.flat_layers().len(), 1); assert_eq!(doc.mask_filter_id(l), Some(m)); @@ -1437,7 +1420,7 @@ mod tests { doc.move_layer(inner_g, MoveTarget::IntoGroupTop(g)); let inner_l = doc.add_raster_layer(Some(inner_g)); - doc.remove_node(g); + doc.remove_entity(g); for id in [g, l, m, inner_g, inner_l] { assert!( doc.entities.get(id).is_none(), @@ -1507,7 +1490,7 @@ mod tests { fn add_raster_layer_with_stale_anchor_falls_back_to_root_top() { let mut doc = Document::new(256, 256); let stale = doc.add_raster_layer(None); - doc.remove_node(stale); + doc.remove_entity(stale); let _other = doc.add_raster_layer(None); let new_id = doc.add_raster_layer(Some(stale)); assert_eq!(doc.children_of(doc.root).last().copied(), Some(new_id)); @@ -1531,4 +1514,105 @@ mod tests { assert_eq!(doc.parent_of(new_g), Some(outer)); assert_eq!(doc.children_of(outer), &[inner_l, new_g]); } + + /// A filter belongs to its host's `filters` list and to no `children` + /// list, whichever detach/attach pair moved it. The two lists used to be + /// reachable from the wrong primitive, so a mask could be detached as a + /// node (leaving it in `filters`) or reattached as one (landing it in + /// `root.children`). + #[test] + fn detaching_a_filter_by_the_node_path_still_unlinks_it_from_its_host() { + let mut doc = Document::new(256, 256); + let host = doc.add_raster_layer(None); + let mask = doc.add_mask_filter(host).expect("mask"); + + doc.detach_for_undo(mask); + + assert!( + !doc.filters_of(host).contains(&mask), + "mask must leave its host's filter list, got {:?}", + doc.filters_of(host) + ); + assert_eq!(doc.parent_of(mask), None); + } + + #[test] + fn reinserting_a_filter_puts_it_back_on_its_host_not_in_root_children() { + let mut doc = Document::new(256, 256); + let host = doc.add_raster_layer(None); + let mask = doc.add_mask_filter(host).expect("mask"); + let position = doc.position_in_parent(mask).expect("filter position"); + + doc.detach_for_undo(mask); + doc.reinsert_entity(mask, Some(host), position); + + assert_eq!(doc.filters_of(host), &[mask]); + assert_eq!(doc.parent_of(mask), Some(host)); + assert!( + !doc.children_of(doc.root).contains(&mask), + "a filter must never appear in a children list, got {:?}", + doc.children_of(doc.root) + ); + } + + /// Restoring a filter puts it back at its original index rather than + /// appending, so undoing the removal of one of several modifiers doesn't + /// silently reorder the stack. + #[test] + fn reinserting_a_filter_restores_its_original_index() { + let mut doc = Document::new(256, 256); + let host = doc.add_raster_layer(None); + let first = doc.add_mask_filter(host).expect("first filter"); + let second = doc.add_mask_filter(host).expect("second filter"); + assert_eq!(doc.filters_of(host), &[first, second]); + + let position = doc.position_in_parent(first).expect("filter position"); + doc.detach_for_undo(first); + doc.reinsert_entity(first, Some(host), position); + + assert_eq!(doc.filters_of(host), &[first, second]); + } + + /// Moving a filter id through the node-move path must not smuggle it into + /// a `children` list — a mask has no position in the tree. + #[test] + fn move_layer_refuses_to_relocate_a_filter() { + let mut doc = Document::new(256, 256); + let host = doc.add_raster_layer(None); + let other = doc.add_raster_layer(None); + let mask = doc.add_mask_filter(host).expect("mask"); + + doc.move_layer(mask, MoveTarget::Before(other)); + + assert_eq!( + doc.filters_of(host), + &[mask], + "mask must stay on its host after an attempted node move" + ); + assert_eq!(doc.parent_of(mask), Some(host)); + assert!(!doc.children_of(doc.root).contains(&mask)); + } + + /// `remove_entity` frees descendants as well as the entity itself, for both + /// kinds — the node path used to purge the subtree while the filter path + /// only dropped the entity. + #[test] + fn remove_entity_purges_a_subtree_and_a_filter_alike() { + let mut doc = Document::new(256, 256); + let group = doc.add_group(None); + let child = doc.add_raster_layer(Some(group)); + let child_mask = doc.add_mask_filter(child).expect("mask"); + + doc.remove_entity(group); + assert!(doc.find_node(group).is_none()); + assert!(doc.find_node(child).is_none()); + assert!(doc.find_filter(child_mask).is_none()); + + let host = doc.add_raster_layer(None); + let mask = doc.add_mask_filter(host).expect("mask"); + doc.remove_entity(mask); + assert!(doc.find_filter(mask).is_none()); + assert!(doc.filters_of(host).is_empty()); + assert!(doc.find_node(host).is_some()); + } } diff --git a/crates/darkly/src/engine/clipboard.rs b/crates/darkly/src/engine/clipboard.rs index 4dff4871..5b49996d 100644 --- a/crates/darkly/src/engine/clipboard.rs +++ b/crates/darkly/src/engine/clipboard.rs @@ -13,7 +13,7 @@ use crate::gpu::blend_mode; use crate::gpu::paint_target::GpuPaintTarget; use crate::gpu::readback; use crate::layer::{Layer, LayerId}; -use crate::undo::{GpuRegionAction, LayerAddAction}; +use crate::undo::{EntityAddAction, GpuRegionAction}; #[handlers] impl DarklyEngine { @@ -509,21 +509,25 @@ impl DarklyEngine { let parent = self.doc.parent_of(id); let pos = self.doc.position_in_parent(id).unwrap_or(0); - self.push_undo(Box::new(LayerAddAction::new(id, parent, pos))); + self.push_undo(Box::new(EntityAddAction::new(id, parent, pos))); id } - /// Paste from the internal clipboard at its original position. - /// Returns the new layer ID, or None if clipboard is empty. + /// Paste the internal clipboard INTO the active target (layer or mask) at + /// the source's original position, committed immediately — no transform + /// session. This is the committed counterpart of `paste_in_place_floating` + /// (used when "activate transform after paste" is off); both float the clip + /// onto the active node and share the single `commit_floating` path, which + /// writes RGBA layers and R8 masks alike. Returns the target id, or `None` + /// when there's no active target or nothing to paste. pub fn paste_in_place(&mut self, active_layer_id: Option) -> Option { - let clip = self.clipboard.as_ref()?.as_image()?; - let width = clip.width; - let height = clip.height; - let offset_x = clip.offset_x; - let offset_y = clip.offset_y; - let rgba = clip.data.clone(); - Some(self.paste_image(width, height, &rgba, offset_x, offset_y, active_layer_id)) + let id = active_layer_id?; + if !self.paste_in_place_floating(id) { + return None; + } + self.commit_floating(); + Some(id) } // ----------------------------------------------------------------------- @@ -551,20 +555,30 @@ impl DarklyEngine { // builds an image clip). Voids regenerate from params, so there's // nothing to read back; cross-tab clipboard for voids would need its // own JSON path (out of scope for this change). + // + // A region copy (selection active) captures flat composited pixels and + // must NOT carry the mask — a mask belongs only to a whole-layer copy + // (which is a layer duplication in spirit). Gate the mask capture on + // provenance. + let is_region_copy = self.has_selection(); let meta = match self.doc.layer(id) { Some(Layer::Raster(layer)) => { - let mask = layer.filters.iter().find_map(|mid| { - let m = self.doc.find_filter(*mid)?; - if !m.is_mask() { - return None; - } - let bounds = m.pixels().map(|p| p.bounds).unwrap_or(layer.pixels.bounds); - Some(RichCopyMask { - name: m.common.name.clone(), - visible: m.common.visible, - bounds, + let mask = if is_region_copy { + None + } else { + layer.filters.iter().find_map(|mid| { + let m = self.doc.find_filter(*mid)?; + if !m.is_mask() { + return None; + } + let bounds = m.pixels().map(|p| p.bounds).unwrap_or(layer.pixels.bounds); + Some(RichCopyMask { + name: m.common.name.clone(), + visible: m.common.visible, + bounds, + }) }) - }); + }; Some(RichCopyMetadata { name: layer.common.name.clone(), visible: layer.common.visible, @@ -644,9 +658,13 @@ impl DarklyEngine { } } - // Restore mask presence (without pixels — v1). + // Restore mask presence (without pixels — v1). Use the unseeded attach: + // the pasted layer's `EntityAddAction` already covers the mask in its + // subtree, and seeding from the receiving document's active selection + // would wrongly turn that selection into the mask. if clip.mask.is_some() { - self.add_mask(id); + self.add_mask_unseeded(id); + self.compositor.mark_dirty(); } Some(id) diff --git a/crates/darkly/src/engine/duplicate.rs b/crates/darkly/src/engine/duplicate.rs index ccc3e8de..675d7dd0 100644 --- a/crates/darkly/src/engine/duplicate.rs +++ b/crates/darkly/src/engine/duplicate.rs @@ -348,32 +348,19 @@ impl DarklyEngine { /// supports mask filters; the loop is generic so other future pixel- /// bearing filters fall in by the same path. /// - /// Goes through `Document::add_mask_filter` + compositor allocation - /// directly instead of [`Self::add_mask`] so we don't push a spurious - /// `FilterAddAction` that the parent [`DuplicateAction`] already - /// covers (a single undo step should reverse the whole duplicate). + /// Goes through [`Self::add_mask_unseeded`] (not [`Self::add_mask`]) so we + /// don't push a spurious `FilterAddAction` that the parent + /// [`DuplicateAction`] already covers, and don't seed from the active + /// selection — the pixels come from the source mask below. fn clone_modifiers(&mut self, src_host: LayerId, dst_host: LayerId) { let src_mod_ids = self.doc.filters_of(src_host).to_vec(); for src_mod_id in src_mod_ids { if self.doc.mask_filter_id(src_host) != Some(src_mod_id) { continue; // Non-mask filters don't ship in v1. } - let Some(new_mod_id) = self.doc.add_mask_filter(dst_host) else { + let Some(new_mod_id) = self.add_mask_unseeded(dst_host) else { continue; }; - let bounds = match self.doc.find_filter(new_mod_id).and_then(|m| m.pixels()) { - Some(p) => p.bounds, - None => continue, - }; - self.compositor.ensure_node_texture( - &self.gpu.device, - &self.gpu.queue, - new_mod_id, - wgpu::TextureFormat::R8Unorm, - bounds, - ); - self.compositor - .ensure_mask_snapshot_state(&self.gpu.device, dst_host); // clone_filter_pixels marks `new_mod_id` dirty internally per // the write-site invariant. self.clone_filter_pixels(src_mod_id, new_mod_id); diff --git a/crates/darkly/src/engine/filters/mask.rs b/crates/darkly/src/engine/filters/mask.rs index ec7c007d..1bfa7d47 100644 --- a/crates/darkly/src/engine/filters/mask.rs +++ b/crates/darkly/src/engine/filters/mask.rs @@ -1,8 +1,10 @@ //! Engine-level operations on mask filters. //! -//! Replaces the old `engine/masks.rs`. Add/remove/apply now go through the -//! generic `Document::add_mask_filter` / `Document::remove_filter` helpers -//! and the unified compositor node-texture pool. The "active node = paint +//! Add/remove/apply go through the generic `Document::add_mask_filter` / +//! `Document::detach_for_undo` helpers and the unified compositor node-texture +//! pool. Structural undo reuses the kind-uniform `EntityAddAction` / +//! `EntityRemoveAction`; only the mask's *pixels* need mask-specific handling, +//! via a `GpuRegionAction` in a `CompoundAction`. The "active node = paint //! target" rule means there's no `editing_mask_layer` redirect — the active //! node id directly identifies where strokes are routed. @@ -12,7 +14,7 @@ use super::super::rendering::commit_undo_region; use super::super::DarklyEngine; use crate::layer::LayerId; use crate::undo::{ - CompoundAction, FilterAddAction, FilterRemoveAction, GpuRegionAction, MaskLinkedToHostAction, + CompoundAction, EntityAddAction, EntityRemoveAction, GpuRegionAction, MaskLinkedToHostAction, UndoAction, }; @@ -37,29 +39,9 @@ impl DarklyEngine { return; } - let mod_id = match self.doc.add_mask_filter(id) { - Some(id) => id, - None => return, - }; - - let bounds = match self.doc.find_filter(mod_id).and_then(|m| m.pixels()) { - Some(buf) => buf.bounds, - None => return, + let Some(mod_id) = self.add_mask_unseeded(id) else { + return; }; - self.compositor.ensure_node_texture( - &self.gpu.device, - &self.gpu.queue, - mod_id, - wgpu::TextureFormat::R8Unorm, - bounds, - ); - - // Per-host snapshot+lerp resource for the in-place masked-host path - // (passthrough group or filter layer). Idempotent across every host - // kind; only the in-place composite paths consume it, but the engine - // doesn't need to branch — the compositor reads it lazily. - self.compositor - .ensure_mask_snapshot_state(&self.gpu.device, id); // If a selection is active, seed the mask pixels from the selection. // Grow the mask to the union of its bounds and the selection's plane @@ -77,7 +59,40 @@ impl DarklyEngine { // dirty per the write-site invariant. self.compositor.mark_dirty(); - self.push_undo(Box::new(FilterAddAction::new(mod_id, id))); + let position = self.doc.position_in_parent(mod_id).unwrap_or(0); + self.push_undo(Box::new(EntityAddAction::new(mod_id, Some(id), position))); + } + + /// Allocate an empty (unseeded) mask filter on `id`: create the filter, + /// allocate its R8 node texture, and ensure the per-host snapshot/lerp + /// resource. Does NOT seed from the active selection and does NOT push an + /// undo entry — callers frame it. [`Self::add_mask`] adds selection + /// seeding and an `EntityAddAction`; duplicate and rich paste fold the mask + /// into the subtree undo their own action already covers, so they must not + /// seed from (nor consume) the receiving document's selection. + /// + /// Returns the new mask filter id, or `None` if the host can't take one. + pub(crate) fn add_mask_unseeded(&mut self, id: LayerId) -> Option { + let mod_id = self.doc.add_mask_filter(id)?; + let bounds = self + .doc + .find_filter(mod_id) + .and_then(|m| m.pixels())? + .bounds; + self.compositor.ensure_node_texture( + &self.gpu.device, + &self.gpu.queue, + mod_id, + wgpu::TextureFormat::R8Unorm, + bounds, + ); + // Per-host snapshot+lerp resource for the in-place masked-host path + // (passthrough group or filter layer). Idempotent across every host + // kind; only the in-place composite paths consume it, but the engine + // doesn't need to branch — the compositor reads it lazily. + self.compositor + .ensure_mask_snapshot_state(&self.gpu.device, id); + Some(mod_id) } /// Set the transform relationship state owned by a mask filter entity. @@ -106,16 +121,46 @@ impl DarklyEngine { if !self.resolve_transform_conflict() || !self.doc.is_node_editable(id) { return; } - let mask_id = match self.doc.mask_filter_id(id) { - Some(id) => id, - None => return, + let Some(mask_id) = self.doc.mask_filter_id(id) else { + return; }; + self.remove_modifier(mask_id); + } + + /// Remove a modifier addressed by its **own** id rather than its host's. + /// The layer panel lists modifiers as selectable rows, so generic + /// operations (delete, batch delete) reach them this way. + pub(crate) fn remove_modifier(&mut self, modifier_id: LayerId) { + if !self.resolve_transform_conflict() { + return; + } + if let Some(action) = self.detach_for_remove(modifier_id) { + self.push_undo(action); + } + } + + /// Detach a modifier and return the matching undo action without pushing + /// it — the modifier-kind counterpart of `detach_for_remove`, so batch + /// removal can fold modifiers into one undo step alongside layers. + /// + /// Owns the bookkeeping a modifier's pixels need and a tree node's don't: + /// the mask texture is saved into a region entry (so undo can restore the + /// bytes) and disposed eagerly, which is why the structural action carries + /// no tombstones. + pub(crate) fn detach_modifier_for_remove( + &mut self, + modifier_id: LayerId, + ) -> Option> { + let host_id = self.doc.parent_of(modifier_id)?; + if !self.doc.is_node_editable(host_id) { + return None; + } // Save mask texture pixels to RegionScratch for undo before removing. let format = wgpu::TextureFormat::R8Unorm; let gpu_region_entry = if let Some((frame, rect)) = self .compositor - .node_texture(mask_id) + .node_texture(modifier_id) .map(|t| (t.canvas_frame(), t.canvas_extent())) { let snap = self.gpu.encode_ret("remove-mask-save", |encoder| { @@ -127,7 +172,7 @@ impl DarklyEngine { &self.region_scratch, &mut self.readbacks, "remove-mask-commit", - mask_id, + modifier_id, &frame, &snap, rect, @@ -136,14 +181,13 @@ impl DarklyEngine { None }; - let detached = self.doc.detach_filter_for_undo(mask_id).is_some(); - // If the removed mask was the isolated/active node, clear the session flag. - if self.isolated_node == Some(mask_id) { - self.isolated_node = None; - } - self.compositor.dispose_node_texture(mask_id); - self.compositor.dispose_mask_snapshot_state(id); - self.compositor.dispose_projection_state(id); + // Captured before the detach severs the parent link the position is + // read from, so undo restores the mask at its original index. + let mask_position = self.doc.position_in_parent(modifier_id).unwrap_or(0); + let detached = self.doc.detach_for_undo(modifier_id).is_some(); + self.compositor.dispose_node_texture(modifier_id); + self.compositor.dispose_mask_snapshot_state(host_id); + self.compositor.dispose_projection_state(host_id); self.compositor.mark_dirty(); let mut actions: Vec> = Vec::new(); @@ -151,12 +195,17 @@ impl DarklyEngine { actions.push(Box::new(GpuRegionAction::new(entry))); } if detached { - actions.push(Box::new(FilterRemoveAction::new(mask_id, id))); + actions.push(Box::new(EntityRemoveAction::new( + modifier_id, + Some(host_id), + mask_position, + Vec::new(), + ))); } - if actions.len() == 1 { - self.push_undo(actions.pop().unwrap()); - } else if !actions.is_empty() { - self.push_undo(Box::new(CompoundAction::new(actions))); + match actions.len() { + 0 => None, + 1 => actions.pop(), + _ => Some(Box::new(CompoundAction::new(actions))), } } @@ -202,7 +251,7 @@ impl DarklyEngine { // with a fresh (all-white) mask texture and the user's painting on // the mask is lost forever. Its GpuRegionAction is bundled below into // the single CompoundAction alongside the host-alpha region and the - // FilterRemoveAction, so one undo replays them in the right order: + // EntityRemoveAction, so one undo replays them in the right order: // re-attach filter → restore mask pixels → restore host alpha. let mask_frame = self .compositor @@ -328,16 +377,24 @@ impl DarklyEngine { self.compositor.mark_node_pixels_dirty(id); // Remove the filter from the document and its GPU texture, then bundle - // the FilterRemoveAction last in the vec so `CompoundAction::undo` + // the EntityRemoveAction last in the vec so `CompoundAction::undo` // (reverse) pops it first — the re-attach happens before // sync_compositor_layers re-allocates the R8 texture, after which the // pending mask-region restore can land. - let detached = self.doc.detach_filter_for_undo(mask_id).is_some(); + // Captured before the detach severs the parent link the position is + // read from, so undo restores the mask at its original index. + let mask_position = self.doc.position_in_parent(mask_id).unwrap_or(0); + let detached = self.doc.detach_for_undo(mask_id).is_some(); self.compositor.dispose_node_texture(mask_id); self.compositor.dispose_mask_snapshot_state(id); self.compositor.dispose_projection_state(id); if detached { - actions.push(Box::new(FilterRemoveAction::new(mask_id, id))); + actions.push(Box::new(EntityRemoveAction::new( + mask_id, + Some(id), + mask_position, + Vec::new(), + ))); } // One Apply Mask = one undo step: fold the host-alpha region, mask-pixel diff --git a/crates/darkly/src/engine/flatten.rs b/crates/darkly/src/engine/flatten.rs index dc86a597..b39111df 100644 --- a/crates/darkly/src/engine/flatten.rs +++ b/crates/darkly/src/engine/flatten.rs @@ -85,7 +85,7 @@ impl DarklyEngine { // Reposition result to root position 0 (bottom of stack — flatten // makes the result the new "Background"). self.doc.detach_for_undo(result_id); - self.doc.reinsert_node(result_id, Some(root_id), 0); + self.doc.reinsert_entity(result_id, Some(root_id), 0); let result_parent = self.doc.parent_of(result_id); let result_position = self.doc.position_in_parent(result_id).unwrap_or(0); @@ -234,7 +234,7 @@ impl DarklyEngine { // Reposition the result to take the group's slot. self.doc.detach_for_undo(result_id); - self.doc.reinsert_node(result_id, parent, position); + self.doc.reinsert_entity(result_id, parent, position); let result_parent = self.doc.parent_of(result_id); let result_position = self.doc.position_in_parent(result_id).unwrap_or(0); diff --git a/crates/darkly/src/engine/floating.rs b/crates/darkly/src/engine/floating.rs index 28a66dea..6c017b89 100644 --- a/crates/darkly/src/engine/floating.rs +++ b/crates/darkly/src/engine/floating.rs @@ -79,7 +79,7 @@ pub(crate) struct TransformSession { } use crate::layer::{Layer, LayerId}; use crate::undo::{ - CompoundAction, GpuRegionAction, LayerAddAction, PixelBoundsAction, SelectionAction, + CompoundAction, EntityAddAction, GpuRegionAction, PixelBoundsAction, SelectionAction, SelectionMetadataAction, UndoAction, }; @@ -186,22 +186,24 @@ impl DarklyEngine { // Auto-commit any existing floating content first. self.auto_commit_floating(); - let clip = match self.clipboard.as_ref().and_then(|c| c.as_image()) { - Some(c) => c, - None => return false, - }; - - let source_origin = (clip.offset_x, clip.offset_y); - let source_width = clip.width; - let source_height = clip.height; + // Pull pixels from either clipboard variant — a normal copy produces a + // rich `Layer` clip, so reading only flat image clips here made + // paste-in-place silently no-op after any copy. + let (rgba, source_width, source_height, offset_x, offset_y) = + match self.clipboard.as_ref().and_then(|c| c.paste_pixels()) { + Some(v) => v, + None => return false, + }; + let source_origin = (offset_x, offset_y); // Upload flat RGBA data to GPU for preview. The target node's format // is read off `compositor.node_texture(id).format` inside the - // compositor — the engine never speaks the word "mask" here. + // compositor — the engine never speaks the word "mask" here, so this + // floats onto a raster layer or an R8 mask alike. self.compositor.set_floating_content( &self.gpu.device, &self.gpu.queue, - &clip.data, + &rgba, source_origin, source_width, source_height, @@ -232,7 +234,7 @@ impl DarklyEngine { /// Paste raw RGBA bytes as floating content on a NEW raster layer. /// The caller is expected to switch to the transform tool. On commit, the - /// pixel data is rendered into the new layer and a single LayerAddAction + /// pixel data is rendered into the new layer and a single EntityAddAction /// is pushed to undo. On cancel, the new layer is removed silently. /// /// Returns the new layer id. @@ -941,7 +943,7 @@ impl DarklyEngine { let grew = self.grow_node_to_fit(layer_id, affected_canvas).is_some(); // Path A — paste onto a layer auto-created for this paste. - // The layer is empty by construction, so a single LayerAddAction + // The layer is empty by construction, so a single EntityAddAction // captures the whole paste as one undo step (no GpuRegionAction). let FloatingMode::Paste { created_layer_id } = fc.mode; if created_layer_id.is_some() { @@ -959,7 +961,7 @@ impl DarklyEngine { let parent = self.doc.parent_of(layer_id); let pos = self.doc.position_in_parent(layer_id).unwrap_or(0); - self.push_undo(Box::new(LayerAddAction::new(layer_id, parent, pos))); + self.push_undo(Box::new(EntityAddAction::new(layer_id, parent, pos))); self.compositor.mark_node_pixels_dirty(layer_id); self.compositor.clear_floating_content(); @@ -1154,7 +1156,7 @@ impl DarklyEngine { let FloatingMode::Paste { created_layer_id } = fc.mode; if let Some(id) = created_layer_id { // Paste auto-created a target layer; drop it silently. No undo - // entry to maintain — `LayerAddAction` is only pushed on commit. + // entry to maintain — `EntityAddAction` is only pushed on commit. self.doc.detach_for_undo(id); self.compositor.dispose_layer(id); self.compositor.mark_dirty(); diff --git a/crates/darkly/src/engine/layers.rs b/crates/darkly/src/engine/layers.rs index 7af3b614..4a9d2b95 100644 --- a/crates/darkly/src/engine/layers.rs +++ b/crates/darkly/src/engine/layers.rs @@ -8,7 +8,8 @@ use crate::engine::protocol::{params_from_json, RawParams}; use crate::layer::{Layer, LayerId, LayerNode}; use crate::undo::property::Property; use crate::undo::{ - CompoundAction, LayerAddAction, LayerMoveAction, LayerRemoveAction, PropertyAction, UndoAction, + CompoundAction, EntityAddAction, EntityRemoveAction, LayerMoveAction, PropertyAction, + UndoAction, }; /// Convert Darkly's row-major `[a, b, tx, c, d, ty]` affine (point map @@ -108,7 +109,7 @@ impl DarklyEngine { let parent = self.doc.parent_of(id); let pos = self.doc.position_in_parent(id).unwrap_or(0); - self.push_undo(Box::new(LayerAddAction::new(id, parent, pos))); + self.push_undo(Box::new(EntityAddAction::new(id, parent, pos))); id } @@ -285,7 +286,7 @@ impl DarklyEngine { let parent = self.doc.parent_of(id); let pos = self.doc.position_in_parent(id).unwrap_or(0); - self.push_undo(Box::new(LayerAddAction::new(id, parent, pos))); + self.push_undo(Box::new(EntityAddAction::new(id, parent, pos))); (id, object_id) } @@ -594,7 +595,7 @@ impl DarklyEngine { let parent = self.doc.parent_of(id); let pos = self.doc.position_in_parent(id).unwrap_or(0); - self.push_undo(Box::new(LayerAddAction::new(id, parent, pos))); + self.push_undo(Box::new(EntityAddAction::new(id, parent, pos))); id } @@ -657,7 +658,7 @@ impl DarklyEngine { let group_initial_pos = self.doc.position_in_parent(group_id).unwrap_or(0); let mut actions: Vec> = Vec::with_capacity(editable.len() + 2); - actions.push(Box::new(LayerAddAction::new( + actions.push(Box::new(EntityAddAction::new( group_id, group_initial_parent, group_initial_pos, @@ -694,7 +695,7 @@ impl DarklyEngine { None => self.doc.children_of(self.doc.root_id()).len(), }); self.doc - .reinsert_node(group_id, topmost_parent, clamped_pos); + .reinsert_entity(group_id, topmost_parent, clamped_pos); let group_final_parent = self.doc.parent_of(group_id); let group_final_pos = self.doc.position_in_parent(group_id).unwrap_or(0); if (group_pre_move_parent, group_pre_move_pos) != (group_final_parent, group_final_pos) { @@ -764,7 +765,7 @@ impl DarklyEngine { let parent = self.doc.parent_of(id); let pos = self.doc.position_in_parent(id).unwrap_or(0); - self.push_undo(Box::new(LayerAddAction::new(id, parent, pos))); + self.push_undo(Box::new(EntityAddAction::new(id, parent, pos))); Some(id) } @@ -798,7 +799,7 @@ impl DarklyEngine { let parent = self.doc.parent_of(id); let pos = self.doc.position_in_parent(id).unwrap_or(0); - self.push_undo(Box::new(LayerAddAction::new(id, parent, pos))); + self.push_undo(Box::new(EntityAddAction::new(id, parent, pos))); Some(id) } @@ -1066,32 +1067,48 @@ impl DarklyEngine { if !self.doc.is_node_editable(id) { return Err("Layer is locked".into()); } - if self.doc.node_count() <= 1 { + // A modifier is a selectable row, so the delete hotkey forwards its id + // here — but it hangs off a host rather than occupying a slot in the + // tree, so it never counts toward the last-layer floor. + if !self.doc.is_filter(id) && self.doc.node_count() <= 1 { return Err("Cannot delete the last layer".into()); } - if let Some(action) = self.detach_layer_for_remove(id) { + if let Some(action) = self.detach_for_remove(id) { self.push_undo(action); } self.compositor.mark_dirty(); Ok(()) } - /// Detach a single layer for removal and return the matching undo - /// action without pushing it. Returns `None` if `layer_id` isn't in - /// the tree. The caller is responsible for any editability or - /// "last layer" checks; this is the raw mutation half shared between - /// [`Self::remove_layer`] and [`Self::remove_layers`]. - fn detach_layer_for_remove(&mut self, layer_id: LayerId) -> Option> { - let parent = self.doc.parent_of(layer_id); - let pos = self.doc.position_in_parent(layer_id).unwrap_or(0); + /// Detach a single entity for removal and return the matching undo action + /// without pushing it. Returns `None` if `id` isn't attached. The caller is + /// responsible for any editability or "last layer" checks; this is the raw + /// mutation half shared between [`Self::remove_layer`] and + /// [`Self::remove_layers`]. + /// + /// Dispatches on the entity's own kind: modifiers go to the modifier path, + /// which owns the GPU bookkeeping their pixels need, and everything else is + /// a tree node. Callers pass an id and don't ask what it is. + pub(crate) fn detach_for_remove(&mut self, id: LayerId) -> Option> { + // Session state must not outlive the entity it points at: an isolation + // target detached from the tree is unreachable from the root walk, so + // every node would test as off-path and the canvas would go blank. + if self.isolated_node == Some(id) { + self.isolated_node = None; + } + if self.doc.is_filter(id) { + return self.detach_modifier_for_remove(id); + } + let parent = self.doc.parent_of(id); + let pos = self.doc.position_in_parent(id).unwrap_or(0); // Collect tombstones before detaching — `detach_for_undo` severs // the parent links `collect_pixel_node_ids` walks to enumerate // the subtree. - let tombstones = self.collect_pixel_node_ids(layer_id); - self.doc.detach_for_undo(layer_id)?; - Some(Box::new(LayerRemoveAction::new( - layer_id, parent, pos, tombstones, + let tombstones = self.collect_pixel_node_ids(id); + self.doc.detach_for_undo(id)?; + Some(Box::new(EntityRemoveAction::new( + id, parent, pos, tombstones, ))) } @@ -1107,8 +1124,12 @@ impl DarklyEngine { } let mut editable = Vec::with_capacity(ids.len()); let mut skipped_locked = 0usize; + let mut node_removals = 0usize; for &id in &ids { - if self.doc.find_node(id).is_none() { + // Modifiers are selectable rows and so can arrive in a batch; they + // resolve through their host rather than the tree. + let is_modifier = self.doc.is_filter(id); + if !is_modifier && self.doc.find_node(id).is_none() { continue; } if !self.doc.is_node_editable(id) { @@ -1117,23 +1138,30 @@ impl DarklyEngine { } // Drop any id that's a descendant of another id already in // the batch — removing the ancestor takes the subtree with it. + // A modifier whose host is also in the batch is covered the same + // way: the host's removal takes its filters along. if ids .iter() .any(|&other| other != id && self.doc.is_ancestor_of(other, id)) { continue; } + if !is_modifier { + node_removals += 1; + } editable.push(id); } if editable.is_empty() { self.compositor.mark_dirty(); return Ok(skipped_locked); } - if self.doc.node_count().saturating_sub(editable.len()) == 0 { + // Only tree nodes count against the floor — removing every modifier in + // the document still leaves its layers behind. + if node_removals > 0 && self.doc.node_count().saturating_sub(node_removals) == 0 { return Err("Cannot delete the last layer".into()); } - self.batched_undo(&editable, |engine, id| engine.detach_layer_for_remove(id)); + self.batched_undo(&editable, |engine, id| engine.detach_for_remove(id)); self.compositor.mark_dirty(); Ok(skipped_locked) } diff --git a/crates/darkly/src/engine/merge.rs b/crates/darkly/src/engine/merge.rs index e18ff67b..f4514699 100644 --- a/crates/darkly/src/engine/merge.rs +++ b/crates/darkly/src/engine/merge.rs @@ -31,6 +31,14 @@ impl DarklyEngine { } // Resolve target: the sibling at (source_position - 1) in the same // parent. If source is at position 0 (or has no parent), fail. + // + // `position_in_parent` indexes whichever list holds the entity, so a + // modifier id would yield a *filter* index and then read the host's + // `children` with it. A modifier has no sibling below to merge into, so + // reject it before the arithmetic rather than indexing the wrong list. + if self.doc.is_filter(source_id) { + return Err("Layer not in tree".into()); + } let parent = self.doc.parent_of(source_id); let pos = self .doc @@ -124,7 +132,8 @@ impl DarklyEngine { // Reposition the result to the target's old position. Simplest: // detach then re-insert. self.doc.detach_for_undo(result_id); - self.doc.reinsert_node(result_id, parent, target_pos_before); + self.doc + .reinsert_entity(result_id, parent, target_pos_before); let result_parent = self.doc.parent_of(result_id); let result_position = self.doc.position_in_parent(result_id).unwrap_or(0); @@ -270,7 +279,7 @@ impl DarklyEngine { // reinsert is the simplest exact-slot landing. self.doc.detach_for_undo(result_id); self.doc - .reinsert_node(result_id, topmost_parent, topmost_pos); + .reinsert_entity(result_id, topmost_parent, topmost_pos); let result_parent = self.doc.parent_of(result_id); let result_position = self.doc.position_in_parent(result_id).unwrap_or(0); diff --git a/crates/darkly/src/engine/mod.rs b/crates/darkly/src/engine/mod.rs index 546222cd..8166c39a 100644 --- a/crates/darkly/src/engine/mod.rs +++ b/crates/darkly/src/engine/mod.rs @@ -570,7 +570,7 @@ pub struct DarklyEngine { /// strokes and brush / tool switches, never serialized. Kept across /// layer deletion too — `LayerId` is a generational slotmap key, so a /// stale pin can't alias a new layer, and undoing the deletion - /// reinserts the same id (`LayerRemoveAction::undo`), reviving the + /// reinserts the same id (`EntityRemoveAction::undo`), reviving the /// pin. Stroke start validates it: a dead or group id falls back to /// the painted layer. pub(crate) clone_source_layer: Option, @@ -908,6 +908,27 @@ impl DarklyEngine { self.compositor.tool_overlay().cursor_preview_mask_size() } + /// Whether the frame loop would schedule another frame right now — the + /// `needs_more` value `render` returns to JS. Test-only. + #[cfg(any(test, feature = "testing"))] + pub fn test_frame_needs_more(&self) -> bool { + self.frame_needs_more() + } + + /// Mark a present as owed, mimicking the compositor's `Lost`/`Outdated` + /// early-return that reconfigures without presenting. Test-only. + #[cfg(any(test, feature = "testing"))] + pub fn test_mark_needs_present(&mut self) { + self.compositor.mark_needs_present(); + } + + /// Clear the pending-present flag (no real present happens headlessly). + /// Test-only, for establishing a deterministic baseline. + #[cfg(any(test, feature = "testing"))] + pub fn test_clear_needs_present(&mut self) { + self.compositor.test_clear_needs_present(); + } + /// Current cursor-preview coverage scale uniform — the multiplier the /// overlay shader applies to KIND_MASKED_STAMP sampled coverage. /// Test-only. diff --git a/crates/darkly/src/engine/painting.rs b/crates/darkly/src/engine/painting.rs index 702f3308..844432e8 100644 --- a/crates/darkly/src/engine/painting.rs +++ b/crates/darkly/src/engine/painting.rs @@ -1472,10 +1472,11 @@ impl DarklyEngine { ); encoder.submit(); - // 4. Commit undo. The lazy save in `gpu_stroke_to` populated - // `scratch_snapshot` with the full layer; flood fill can change - // any pixel inside the canvas, so commit the canvas-sized - // sub-rect of that snapshot. + // 4. Commit undo. The lazy save in `gpu_stroke_to` snapshotted the + // layer texture's extent; the fill can only modify pixels inside + // that texture (the readback and the stamp are both bounded by it), + // so the layer extent is the region to commit — never the canvas + // rect, which may reach past the layer when the window is resized. let snap = match self.scratch_snapshot.take() { Some(s) => s, // No snapshot means the lazy save never ran (stroke_to was @@ -1493,7 +1494,7 @@ impl DarklyEngine { return; } }; - let rect = self.doc.canvas_rect(); + let rect = layer_frame.canvas_extent; let entry = commit_undo_region( &self.gpu, &self.region_scratch, diff --git a/crates/darkly/src/engine/rendering.rs b/crates/darkly/src/engine/rendering.rs index 9b2436a1..1c19e506 100644 --- a/crates/darkly/src/engine/rendering.rs +++ b/crates/darkly/src/engine/rendering.rs @@ -158,6 +158,16 @@ impl DarklyEngine { self.compositor.set_pixel_filter(&self.gpu.queue, mode); } + /// Compile the vector renderer's GPU pipelines ahead of first use, so the + /// first text (or path) object doesn't stall the frame that would show it on + /// Vello's one-time shader compile. The frontend fires this when the text + /// tool is selected — the compile overlaps the gap before the user commits a + /// box. Idempotent: a no-op once the renderer is warm. + #[handler] + pub fn warm_vector_renderer(&mut self) { + self.compositor.ensure_vector_renderer(&self.gpu.device); + } + /// Start an async color pick at canvas coordinates. /// /// `source` selects which surface to sample. If a `Layer` source can't be @@ -717,7 +727,18 @@ impl DarklyEngine { }; // Keep requesting frames while async operations are in flight. + self.frame_needs_more() + } + + /// Whether the frame loop must schedule another frame — the value returned + /// to JS, which reschedules while it is `true`. Covers in-flight async work + /// and, critically, a present the compositor still owes: a `Lost`/`Outdated` + /// acquire reconfigures the surface and returns without presenting, so + /// `needs_present` stays set and the reconfigured surface would otherwise + /// never get a real frame. + pub(super) fn frame_needs_more(&self) -> bool { self.compositor.needs_animation(&self.doc) + || self.compositor.needs_present() || self.readbacks.has_pending() || self.compositor.has_pending_content_bounds() || self.compositor.has_pending_histogram() diff --git a/crates/darkly/src/gpu/baked_source_cache.rs b/crates/darkly/src/gpu/baked_source_cache.rs index 5b081db4..22589a12 100644 --- a/crates/darkly/src/gpu/baked_source_cache.rs +++ b/crates/darkly/src/gpu/baked_source_cache.rs @@ -34,7 +34,7 @@ struct BakeParams { octaves: i32, gain: f32, warp: f32, - tile_span: f32, + field_span: f32, channels: u32, _pad0: u32, _pad1: u32, @@ -210,7 +210,7 @@ impl BakedSourceCache { octaves, gain: BakeKind::dequantize(roughness_q), warp: BakeKind::dequantize(warp_q), - tile_span: BakeSpec::TILE_SPAN, + field_span: BakeSpec::FIELD_SPAN, channels, _pad0: 0, _pad1: 0, diff --git a/crates/darkly/src/gpu/compositor.rs b/crates/darkly/src/gpu/compositor.rs index 3d032f38..4e5daa85 100644 --- a/crates/darkly/src/gpu/compositor.rs +++ b/crates/darkly/src/gpu/compositor.rs @@ -2033,6 +2033,23 @@ impl Compositor { self.needs_present = true; } + /// Whether a present is still owed. Set by `mark_needs_present`, cleared by + /// `finish_present` after a frame actually reaches the surface. A dropped + /// acquire (`Lost`/`Outdated`) reconfigures and returns without presenting, + /// leaving this `true` — the frame loop must keep going so the reconfigured + /// surface gets a real present on the next frame. + pub fn needs_present(&self) -> bool { + self.needs_present + } + + /// Clear the pending-present flag without a real present. Headless tests + /// never reach `finish_present` (no surface), so this gives them a + /// deterministic starting point. + #[cfg(any(test, feature = "testing"))] + pub fn test_clear_needs_present(&mut self) { + self.needs_present = false; + } + // --- Content Bounds (GPU compute) --- /// Return cached content bounds for a layer: `[x, y, w, h]`. @@ -2684,12 +2701,23 @@ impl Compositor { } } + /// Compile the vector renderer's pipelines now (if not already), so the first + /// vector layer doesn't stall on the shader-compile cost. Building it compiles + /// Vello's full compute-pipeline set (a >1s one-time cost). Called when the + /// text tool is selected — the compile then overlaps the gap before the user + /// commits a text box, rather than blocking the frame that would show it. + /// Idempotent: a no-op once the renderer exists. + pub fn ensure_vector_renderer(&mut self, device: &wgpu::Device) { + self.vector_renderer + .get_or_insert_with(|| crate::gpu::vector_renderer::VectorRenderer::new(device)); + } + /// Rasterize every dirty vector layer's scene into its storage texture. /// Runs before the composite pass (in `render_offscreen`) so the blend /// walk samples up-to-date pixels. Lazily constructs the shared - /// [`VectorRenderer`] on first use. Vello submits its own command buffer - /// per layer; those submits are ordered before the compositor's, so GPU - /// ordering is preserved. + /// [`VectorRenderer`] on first use (see [`Self::ensure_vector_renderer`]). + /// Vello submits its own command buffer per layer; those submits are ordered + /// before the compositor's, so GPU ordering is preserved. fn realize_dirty_vector_layers(&mut self, device: &wgpu::Device, queue: &wgpu::Queue) { let dirty: Vec = self .vector_scenes diff --git a/crates/darkly/src/layer.rs b/crates/darkly/src/layer.rs index 30558e5f..13f5c770 100644 --- a/crates/darkly/src/layer.rs +++ b/crates/darkly/src/layer.rs @@ -525,6 +525,18 @@ pub enum LayerNode { Group(LayerGroup), } +/// Which of a node's two child lists an id lives in. A node owns both a +/// `filters` list (modifiers such as masks) and — for groups — a `children` +/// list of tree nodes; the two are disjoint. Detach reports the slot it found +/// an id in so reattach can put it back in the same one. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ChildSlot { + /// The host's `filters` list. + Filter, + /// A group's `children` list. + Child, +} + impl LayerNode { pub fn id(&self) -> LayerId { match self { @@ -575,6 +587,64 @@ impl LayerNode { } } + /// Group children, empty for a leaf layer. Pairs with + /// [`LayerNode::children_mut`] so callers can treat "the node's children" + /// uniformly without matching on the variant. + pub fn children(&self) -> &[LayerId] { + match self { + LayerNode::Group(g) => &g.children, + LayerNode::Layer(_) => &[], + } + } + + /// Mutable group children, `None` for a leaf layer — a layer has no + /// children list to insert into, and that distinction is the node's to + /// report rather than the caller's to test for. + pub fn children_mut(&mut self) -> Option<&mut Vec> { + match self { + LayerNode::Group(g) => Some(&mut g.children), + LayerNode::Layer(_) => None, + } + } + + /// Remove `child` from whichever of this node's two lists holds it, and + /// report which one that was. Ids are unique across the document's + /// slotmap, so a child can only be in one — which is what lets detach be + /// kind-agnostic instead of asking the caller to know whether it holds a + /// filter or a tree node. + pub fn detach_child(&mut self, child: LayerId) -> Option { + let filters = self.modifiers_mut(); + if let Some(i) = filters.iter().position(|c| *c == child) { + filters.remove(i); + return Some(ChildSlot::Filter); + } + let children = self.children_mut()?; + let i = children.iter().position(|c| *c == child)?; + children.remove(i); + Some(ChildSlot::Child) + } + + /// Insert `child` into the list named by `slot`, at `position` (clamped) or + /// at the end. Returns false when the node has no such list — a leaf layer + /// asked to take a tree child. + pub fn attach_child( + &mut self, + child: LayerId, + slot: ChildSlot, + position: Option, + ) -> bool { + let list = match slot { + ChildSlot::Filter => self.modifiers_mut(), + ChildSlot::Child => match self.children_mut() { + Some(list) => list, + None => return false, + }, + }; + let at = position.map_or(list.len(), |p| p.min(list.len())); + list.insert(at, child); + true + } + pub fn pixels(&self) -> Option<&PixelBuffer> { match self { LayerNode::Layer(l) => l.pixels(), diff --git a/crates/darkly/src/undo/filter.rs b/crates/darkly/src/undo/filter.rs index d57ea5f8..a73e0a90 100644 --- a/crates/darkly/src/undo/filter.rs +++ b/crates/darkly/src/undo/filter.rs @@ -1,78 +1,20 @@ -//! Undo actions for filter-node mutations. +//! Undo actions for per-node property flags shared by every entity kind — +//! layers, groups, and filters alike. //! -//! Replaces the old `MaskPropertyAction` with generic node-add / node-remove -//! actions that work for any [`Filter`] kind. Per the Modularity Principle, -//! adding a new filter kind doesn't require new undo actions — these are -//! kind-uniform. +//! Structural add/remove of a filter needs no action of its own: a filter is an +//! entity like any other, so [`EntityAddAction`] and [`EntityRemoveAction`] +//! cover it. Pixel data for pixel-bearing filters (today: masks) is preserved +//! by wrapping a `GpuRegionAction` alongside the removal in a `CompoundAction` +//! at the call site (see `engine/filters/mask.rs`). //! -//! Pixel data for pixel-bearing filters (today: masks) is preserved by -//! wrapping a `GpuRegionAction` alongside the [`FilterRemoveAction`] in a -//! [`CompoundAction`] at the call site (see `engine/filters/mask.rs`). -//! -//! Detach/reattach uses the document's orphan-keep semantics: the filter -//! stays in the slotmap with its id intact between unlink and relink. Both -//! actions only need ids — no value handles travel through the undo stack. +//! [`EntityAddAction`]: super::EntityAddAction +//! [`EntityRemoveAction`]: super::EntityRemoveAction use super::UndoAction; use crate::document::Document; use crate::layer::LayerId; use std::collections::{HashMap, HashSet}; -/// Undo action for adding a filter to a host. -/// -/// Undo unlinks the filter from its host (it stays in the document's -/// slotmap orphaned). -/// Redo relinks it on the same host. -pub struct FilterAddAction { - filter_id: LayerId, - host_id: LayerId, -} - -impl FilterAddAction { - pub fn new(filter_id: LayerId, host_id: LayerId) -> Self { - FilterAddAction { filter_id, host_id } - } -} - -impl UndoAction for FilterAddAction { - fn undo(&mut self, doc: &mut Document) -> HashMap> { - doc.detach_filter_for_undo(self.filter_id); - HashMap::new() - } - - fn redo(&mut self, doc: &mut Document) -> HashMap> { - doc.reinsert_filter(self.filter_id, self.host_id); - HashMap::new() - } -} - -/// Undo action for removing a filter from a host. -/// -/// Undo relinks the orphaned filter to its original host. -/// Redo unlinks it again. -pub struct FilterRemoveAction { - filter_id: LayerId, - host_id: LayerId, -} - -impl FilterRemoveAction { - pub fn new(filter_id: LayerId, host_id: LayerId) -> Self { - FilterRemoveAction { filter_id, host_id } - } -} - -impl UndoAction for FilterRemoveAction { - fn undo(&mut self, doc: &mut Document) -> HashMap> { - doc.reinsert_filter(self.filter_id, self.host_id); - HashMap::new() - } - - fn redo(&mut self, doc: &mut Document) -> HashMap> { - doc.detach_filter_for_undo(self.filter_id); - HashMap::new() - } -} - /// Undo action for toggling visibility on any node — layer, group, or filter. /// Stores the current value and swaps it on undo/redo. pub struct NodeVisibleAction { diff --git a/crates/darkly/src/undo/layer.rs b/crates/darkly/src/undo/layer.rs index 6fa4f14d..a814adaf 100644 --- a/crates/darkly/src/undo/layer.rs +++ b/crates/darkly/src/undo/layer.rs @@ -5,20 +5,24 @@ use crate::gpu::compositor::Compositor; use crate::layer::LayerId; use std::collections::{HashMap, HashSet}; -/// Undo action for adding a layer/group. +/// Undo action for adding an entity — layer, group, or filter. /// -/// Undo unlinks the node from the tree (it stays in the document's slotmap +/// Undo unlinks the entity from its parent (it stays in the document's slotmap /// orphaned, so the id is preserved). -/// Redo reinserts it at the original position. -pub struct LayerAddAction { +/// Redo reinserts it under the same parent at the original position. +/// +/// Kind-uniform: `Document`'s detach/reattach pair routes by the entity's own +/// kind, so attaching a mask to a host and a layer to a group are the same +/// operation from here. Adding a new entity kind needs no new action. +pub struct EntityAddAction { layer_id: LayerId, parent: Option, position: usize, } -impl LayerAddAction { +impl EntityAddAction { pub fn new(layer_id: LayerId, parent: Option, position: usize) -> Self { - LayerAddAction { + EntityAddAction { layer_id, parent, position, @@ -26,28 +30,32 @@ impl LayerAddAction { } } -impl UndoAction for LayerAddAction { +impl UndoAction for EntityAddAction { fn undo(&mut self, doc: &mut Document) -> HashMap> { doc.detach_for_undo(self.layer_id); HashMap::new() } fn redo(&mut self, doc: &mut Document) -> HashMap> { - doc.reinsert_node(self.layer_id, self.parent, self.position); + doc.reinsert_entity(self.layer_id, self.parent, self.position); HashMap::new() } } -/// Undo action for removing a layer/group. +/// Undo action for removing an entity — layer, group, or filter. /// -/// The node stays in the document's slotmap as an orphan between detach +/// The entity stays in the document's slotmap as an orphan between detach /// and reattach — the id (and all attached filters/descendants) survives /// across undo/redo with no copy. The subtree's GPU textures are /// tombstoned so the pixels survive too; they're disposed only when this /// action is evicted from the undo stack while the deletion is still in -/// effect (i.e. the user never undid it). Undo relinks the node; redo +/// effect (i.e. the user never undid it). Undo relinks the entity; redo /// unlinks again. -pub struct LayerRemoveAction { +/// +/// Callers that manage a filter's pixels themselves — `remove_mask` saves the +/// mask texture into a `GpuRegionAction` and disposes it eagerly — pass an +/// empty tombstone set. +pub struct EntityRemoveAction { layer_id: LayerId, parent: Option, position: usize, @@ -60,14 +68,14 @@ pub struct LayerRemoveAction { applied: bool, } -impl LayerRemoveAction { +impl EntityRemoveAction { pub fn new( layer_id: LayerId, parent: Option, position: usize, tombstones: Vec, ) -> Self { - LayerRemoveAction { + EntityRemoveAction { layer_id, parent, position, @@ -77,9 +85,9 @@ impl LayerRemoveAction { } } -impl UndoAction for LayerRemoveAction { +impl UndoAction for EntityRemoveAction { fn undo(&mut self, doc: &mut Document) -> HashMap> { - doc.reinsert_node(self.layer_id, self.parent, self.position); + doc.reinsert_entity(self.layer_id, self.parent, self.position); self.applied = false; HashMap::new() } @@ -128,14 +136,14 @@ impl LayerMoveAction { impl UndoAction for LayerMoveAction { fn undo(&mut self, doc: &mut Document) -> HashMap> { if doc.detach_for_undo(self.layer_id).is_some() { - doc.reinsert_node(self.layer_id, self.old_parent, self.old_position); + doc.reinsert_entity(self.layer_id, self.old_parent, self.old_position); } HashMap::new() } fn redo(&mut self, doc: &mut Document) -> HashMap> { if doc.detach_for_undo(self.layer_id).is_some() { - doc.reinsert_node(self.layer_id, self.new_parent, self.new_position); + doc.reinsert_entity(self.layer_id, self.new_parent, self.new_position); } HashMap::new() } @@ -188,7 +196,7 @@ impl UndoAction for DuplicateAction { } fn redo(&mut self, doc: &mut Document) -> HashMap> { - doc.reinsert_node(self.root_new_id, self.parent, self.position); + doc.reinsert_entity(self.root_new_id, self.parent, self.position); self.applied = true; HashMap::new() } @@ -286,7 +294,7 @@ impl UndoAction for BakeLayersAction { let mut sources_sorted = self.sources.clone(); sources_sorted.sort_by_key(|s| s.position); for slot in sources_sorted { - doc.reinsert_node(slot.id, slot.parent, slot.position); + doc.reinsert_entity(slot.id, slot.parent, slot.position); } self.applied = false; HashMap::new() @@ -298,7 +306,7 @@ impl UndoAction for BakeLayersAction { for slot in &self.sources { doc.detach_for_undo(slot.id); } - doc.reinsert_node(self.result_id, self.result_parent, self.result_position); + doc.reinsert_entity(self.result_id, self.result_parent, self.result_position); self.applied = true; HashMap::new() } diff --git a/crates/darkly/src/undo/mod.rs b/crates/darkly/src/undo/mod.rs index 24097968..d1fee94d 100644 --- a/crates/darkly/src/undo/mod.rs +++ b/crates/darkly/src/undo/mod.rs @@ -14,11 +14,11 @@ mod tombstones; pub use canvas_geometry::CanvasGeometryAction; pub use canvas_resize::CanvasResizeAction; pub use compound::CompoundAction; -pub use filter::{FilterAddAction, FilterRemoveAction, NodeLockedAction, NodeVisibleAction}; +pub use filter::{NodeLockedAction, NodeVisibleAction}; pub use gpu_region::GpuRegionAction; pub use layer::{ - BakeLayersAction, BakeSourceSlot, DuplicateAction, LayerAddAction, LayerMoveAction, - LayerRemoveAction, + BakeLayersAction, BakeSourceSlot, DuplicateAction, EntityAddAction, EntityRemoveAction, + LayerMoveAction, }; pub use mask_property::MaskLinkedToHostAction; pub use pixel_bounds::PixelBoundsAction; @@ -302,7 +302,7 @@ mod tests { // Record the add as undoable. let parent = doc.parent_of(id); let pos = doc.position_in_parent(id).unwrap(); - let _ = undo.push(&mut doc, Box::new(LayerAddAction::new(id, parent, pos))); + let _ = undo.push(&mut doc, Box::new(EntityAddAction::new(id, parent, pos))); assert_eq!(doc.flat_layers().len(), 1); @@ -328,7 +328,7 @@ mod tests { let node = doc.detach_for_undo(id).unwrap(); let _ = undo.push( &mut doc, - Box::new(LayerRemoveAction::new(node, parent, pos, Vec::new())), + Box::new(EntityRemoveAction::new(node, parent, pos, Vec::new())), ); assert_eq!(doc.flat_layers().len(), 0); @@ -486,7 +486,7 @@ mod tests { let id = doc.add_raster_layer(None); let parent = doc.parent_of(id); let pos = doc.position_in_parent(id).unwrap(); - let _ = undo.push(&mut doc, Box::new(LayerAddAction::new(id, parent, pos))); + let _ = undo.push(&mut doc, Box::new(EntityAddAction::new(id, parent, pos))); assert!(doc.dirty, "push must flip dirty"); } @@ -531,7 +531,7 @@ mod tests { let id = doc.add_raster_layer(None); let parent = doc.parent_of(id); let pos = doc.position_in_parent(id).unwrap(); - let _ = undo.push(&mut doc, Box::new(LayerAddAction::new(id, parent, pos))); + let _ = undo.push(&mut doc, Box::new(EntityAddAction::new(id, parent, pos))); assert!(doc.dirty); undo.undo(&mut doc); @@ -556,7 +556,7 @@ mod tests { let id = doc.add_raster_layer(None); let parent = doc.parent_of(id); let pos = doc.position_in_parent(id).unwrap(); - let _ = undo.push(&mut doc, Box::new(LayerAddAction::new(id, parent, pos))); + let _ = undo.push(&mut doc, Box::new(EntityAddAction::new(id, parent, pos))); assert!(doc.revision > before, "push must bump revision"); } @@ -606,7 +606,10 @@ mod tests { let new_id = doc.add_raster_layer(Some(l1)); let parent = doc.parent_of(new_id); let pos = doc.position_in_parent(new_id).unwrap(); - let _ = undo.push(&mut doc, Box::new(LayerAddAction::new(new_id, parent, pos))); + let _ = undo.push( + &mut doc, + Box::new(EntityAddAction::new(new_id, parent, pos)), + ); let flat: Vec<_> = doc.flat_layers().iter().map(|l| l.id()).collect(); assert_eq!(flat, vec![l1, new_id, l2]); diff --git a/crates/darkly/src/undo/tombstones.rs b/crates/darkly/src/undo/tombstones.rs index 78815f90..066ba5e8 100644 --- a/crates/darkly/src/undo/tombstones.rs +++ b/crates/darkly/src/undo/tombstones.rs @@ -19,7 +19,7 @@ //! | Action | Polarity | //! |--------------------------------------|----------------------| //! | [`super::DuplicateAction`] | `false` (detached when undone) | -//! | [`super::LayerRemoveAction`] | `true` (detached when applied) | +//! | [`super::EntityRemoveAction`] | `true` (detached when applied) | //! | [`super::BakeLayersAction`] sources | `true` (detached when applied) | //! | [`super::BakeLayersAction`] result | `false` (detached when undone) | diff --git a/crates/darkly/tests/canvas_resize.rs b/crates/darkly/tests/canvas_resize.rs index 12bb0fa2..6709ff1f 100644 --- a/crates/darkly/tests/canvas_resize.rs +++ b/crates/darkly/tests/canvas_resize.rs @@ -711,3 +711,37 @@ fn successive_crops_do_not_compound_squash() { "squash compounds across crops: uncropped {uncropped:.3}, 1 crop {one:.3}, 2 crops {two:.3}" ); } + +/// REGRESSION: a present dropped by a `Lost`/`Outdated` surface acquire must +/// keep the frame loop alive. On resize, the compositor's acquire can return +/// `Outdated`; that path reconfigures the surface and returns *without* +/// presenting, leaving `needs_present` set. If `render`'s returned `needs_more` +/// ignores that pending present, JS never reschedules and the reconfigured +/// surface never gets a real frame — a stale/frozen canvas. `needs_more` must +/// therefore surface `compositor.needs_present()`. +#[test] +fn dropped_present_keeps_requesting_frames() { + let mut engine = test_engine(64, 64); + + // Drain startup async work (thumbnail readbacks, etc.), then clear the + // pending-present flag so the baseline is genuinely quiescent — headless + // renders never reach `finish_present`, so `needs_present` would otherwise + // stay stuck set from engine setup. + for _ in 0..8 { + engine.render(0.0); + } + engine.test_clear_needs_present(); + assert!( + !engine.test_frame_needs_more(), + "engine did not settle to quiescence; test baseline is unreliable" + ); + + // Mimic the compositor's `Lost`/`Outdated` early-return: a present is owed + // but was dropped without `finish_present` clearing the flag. + engine.test_mark_needs_present(); + + assert!( + engine.test_frame_needs_more(), + "a pending present must keep the frame loop alive so the dropped frame reschedules" + ); +} diff --git a/crates/darkly/tests/dab_variation_scatter.rs b/crates/darkly/tests/dab_variation_scatter.rs new file mode 100644 index 00000000..2a2a9c77 --- /dev/null +++ b/crates/darkly/tests/dab_variation_scatter.rs @@ -0,0 +1,156 @@ +//! GPU coverage test for the Dab-space `variation` decorrelation offset. +//! +//! The bug this guards against: the per-dab offset was `vec2(v*64, v*64)` — the +//! same scalar on both axes, so every dab landed on the `x == y` diagonal of the +//! field (1D), and the `*64` stride resonated with the field's period. The fix +//! hashes `variation` into two independent components via `fbm_offset2` +//! (`shaders/lib/fbm2d.wgsl`), scattered over one field period. +//! +//! An emission-string unit test (`sample_frame.rs`) pins the *shape* of the +//! emitted WGSL but cannot execute `fbm_pcg`. This test executes the real +//! shipped `fbm_offset2` on the GPU for the exact input path a wired +//! `random → variation` produces, then asserts the results actually cover 2D and +//! are uncorrelated — the numerical property the bug was about. + +use darkly::gpu::test_utils::test_device; + +/// Number of per-dab samples to evaluate — the `random` node quantizes to full +/// f32 precision per dab; 1024 evenly-spaced draws is plenty to characterize the +/// distribution. +const N: u32 = 1024; +/// Grid resolution for the 2D-occupancy check. +const GRID: usize = 8; + +/// Compute wrapper around the real, shipped `fbm2d.wgsl`. For each index it +/// reproduces the exact expression the emitter builds +/// (`fbm_offset2(u32(max(variation, 0.0) * 4096.0), period)`) with `period = 1.0` +/// so the two components land in `[0, 1)`, and the `variation` the standard +/// `random → variation` wire delivers (`random * 1024`, `random = i / N`). +const WRAPPER: &str = r#" +@group(0) @binding(0) var out_off: array>; + +@compute @workgroup_size(64) +fn main(@builtin(global_invocation_id) gid: vec3) { + let i = gid.x; + if (i >= arrayLength(&out_off)) { return; } + let random = f32(i) / f32(arrayLength(&out_off)); + let variation = random * 1024.0; // wire-boundary remap + let seed = u32(max(variation, 0.0) * 4096.0); // emitter's seed quantization + out_off[i] = fbm_offset2(seed, 1.0); // period 1.0 -> [0,1)^2 +} +"#; + +#[test] +fn dab_variation_offset_scatters_across_2d() { + let (device, queue) = test_device(); + + let mut source = String::from(include_str!("../shaders/lib/fbm2d.wgsl")); + source.push_str(WRAPPER); + let module = device.create_shader_module(wgpu::ShaderModuleDescriptor { + label: Some("fbm-offset2-coverage"), + source: wgpu::ShaderSource::Wgsl(source.into()), + }); + + let bytes = (N as u64) * std::mem::size_of::<[f32; 2]>() as u64; + let out_buf = device.create_buffer(&wgpu::BufferDescriptor { + label: Some("offsets"), + size: bytes, + usage: wgpu::BufferUsages::STORAGE | wgpu::BufferUsages::COPY_SRC, + mapped_at_creation: false, + }); + let read_buf = device.create_buffer(&wgpu::BufferDescriptor { + label: Some("offsets-read"), + size: bytes, + usage: wgpu::BufferUsages::MAP_READ | wgpu::BufferUsages::COPY_DST, + mapped_at_creation: false, + }); + + let pipeline = device.create_compute_pipeline(&wgpu::ComputePipelineDescriptor { + label: Some("fbm-offset2-coverage"), + layout: None, + module: &module, + entry_point: Some("main"), + compilation_options: Default::default(), + cache: None, + }); + let bind_group = device.create_bind_group(&wgpu::BindGroupDescriptor { + label: None, + layout: &pipeline.get_bind_group_layout(0), + entries: &[wgpu::BindGroupEntry { + binding: 0, + resource: out_buf.as_entire_binding(), + }], + }); + + let mut encoder = + device.create_command_encoder(&wgpu::CommandEncoderDescriptor { label: None }); + { + let mut pass = encoder.begin_compute_pass(&wgpu::ComputePassDescriptor { + label: None, + timestamp_writes: None, + }); + pass.set_pipeline(&pipeline); + pass.set_bind_group(0, &bind_group, &[]); + pass.dispatch_workgroups(N / 64, 1, 1); + } + encoder.copy_buffer_to_buffer(&out_buf, 0, &read_buf, 0, bytes); + queue.submit([encoder.finish()]); + + // Blocking readback is test-only (native `poll(Wait)` drives the queue). + let slice = read_buf.slice(..); + let (tx, rx) = std::sync::mpsc::sync_channel(1); + slice.map_async(wgpu::MapMode::Read, move |r| { + let _ = tx.send(r); + }); + let _ = device.poll(wgpu::PollType::Wait { + submission_index: None, + timeout: None, + }); + rx.recv().unwrap().expect("map failed"); + let data = slice.get_mapped_range(); + let offs: &[[f32; 2]] = bytemuck::cast_slice(&data); + + // 2D occupancy: bin into a GRID×GRID grid over [0,1)². A diagonal-only + // distribution (the bug) fills at most GRID cells; genuine 2D scatter fills + // the vast majority. + let mut occupied = [[false; GRID]; GRID]; + for &[ox, oy] in offs { + assert!( + (0.0..1.0).contains(&ox) && (0.0..1.0).contains(&oy), + "offset out of [0,1): ({ox}, {oy})" + ); + let cx = ((ox * GRID as f32) as usize).min(GRID - 1); + let cy = ((oy * GRID as f32) as usize).min(GRID - 1); + occupied[cy][cx] = true; + } + let filled = occupied.iter().flatten().filter(|c| **c).count(); + assert!( + filled >= 48, + "expected ≥48/{} grid cells occupied (2D scatter); got {filled} \ + — the buggy diagonal fills ≤{GRID}", + GRID * GRID, + ); + + // Low correlation: the diagonal has corr(ox, oy) ≈ 1.0; independent draws ≈ 0. + let n = offs.len() as f64; + let (mut sx, mut sy, mut sxx, mut syy, mut sxy) = (0.0, 0.0, 0.0, 0.0, 0.0); + for &[ox, oy] in offs { + let (x, y) = (ox as f64, oy as f64); + sx += x; + sy += y; + sxx += x * x; + syy += y * y; + sxy += x * y; + } + let cov = sxy / n - (sx / n) * (sy / n); + let var_x = sxx / n - (sx / n).powi(2); + let var_y = syy / n - (sy / n).powi(2); + let corr = cov / (var_x.sqrt() * var_y.sqrt()); + assert!( + corr.abs() < 0.15, + "expected |corr(ox, oy)| < 0.15 (independent axes); got {corr:.3}", + ); + + drop(data); + read_buf.unmap(); +} diff --git a/crates/darkly/tests/engine.rs b/crates/darkly/tests/engine.rs index b93249f2..70e79300 100644 --- a/crates/darkly/tests/engine.rs +++ b/crates/darkly/tests/engine.rs @@ -475,7 +475,7 @@ fn paste_floating_target_layer_matches_created() { } /// Companion: committing a floating paste keeps the layer and registers -/// exactly one undoable LayerAddAction (so a single undo removes the paste). +/// exactly one undoable EntityAddAction (so a single undo removes the paste). #[test] fn paste_floating_commit_is_one_undo() { let (w, h) = (128, 128); @@ -1536,7 +1536,7 @@ fn add_remove_cycles_dont_leak_layer_textures() { /// Regression: undoing a layer removal must restore the layer's pixel /// content, not just its tree slot. Previously `remove_layer` disposed -/// the GPU texture immediately and `LayerRemoveAction` carried no pixel +/// the GPU texture immediately and `EntityRemoveAction` carried no pixel /// state, so undo reattached the node but the compositor allocated a /// fresh blank texture. #[test] @@ -3718,12 +3718,14 @@ fn copy_selected_mask_region_populates_clipboard() { ); } -/// Pasting while a mask is the active edit target must place the new layer as -/// the host's SIBLING, not nest it under the (raster) host. The active id is -/// the mask *filter* id, and `parent_of(mask) == host`, so a naive -/// `MoveTarget::After(mask_id)` linked the pasted layer as a child of the -/// raster host — an invalid tree that left the paste off the published layer -/// tree (raster layers publish no children) and invisible on canvas. +/// Plain paste (`paste_image`) while a mask is the active edit target must +/// place the new layer as the host's SIBLING at top level, not nest it under +/// the (raster) host. The active id is the mask *filter* id, and +/// `parent_of(mask) == host`, so a naive `MoveTarget::After(mask_id)` linked +/// the pasted layer as a child of the raster host — an invalid tree that left +/// the paste off the published layer tree (raster layers publish no children) +/// and invisible on canvas. (Plain paste always makes its own layer; pasting +/// INTO a mask is the `paste_in_place` verb — see `tests/paste_mask.rs`.) #[test] fn paste_while_editing_mask_places_layer_at_top_level() { use darkly::engine::types::LayerInfo; @@ -5930,7 +5932,7 @@ fn delete_refuses_to_empty_document() { /// Selecting a group AND one of its descendants and deleting both must /// dedupe — the descendant comes out with its ancestor, so issuing a -/// second `LayerRemoveAction` would corrupt the undo stack. +/// second `EntityRemoveAction` would corrupt the undo stack. #[test] fn delete_dedupes_ancestor_descendant() { use darkly::document::MoveTarget; diff --git a/crates/darkly/tests/flood_fill_resize.rs b/crates/darkly/tests/flood_fill_resize.rs new file mode 100644 index 00000000..c85c3ff0 --- /dev/null +++ b/crates/darkly/tests/flood_fill_resize.rs @@ -0,0 +1,106 @@ +//! Regression: flood fill after an upward canvas resize must not panic in the +//! undo commit, and undo/redo must round-trip the fill. +//! +//! `complete_flood_fill` committed `doc.canvas_rect()` (the resized canvas +//! window) against a scratch snapshot saved over the layer extent, which +//! `resize_canvas` leaves put in the plane. After expanding upward the canvas +//! rect is taller and offset above the layer, so `commit_region`'s containment +//! assert fired. The fill can only touch the layer texture, so the layer extent +//! is the correct commit rect. +//! +//! Run with: cargo test -p darkly --test flood_fill_resize --features darkly/testing -- --test-threads=1 + +use darkly::coord::CanvasRect; +use darkly::engine::types::StrokeOp; +use darkly::engine::DarklyEngine; +use darkly::gpu::context::GpuContext; +use darkly::gpu::test_utils::test_device; +use darkly::layer::LayerId; + +fn test_engine(width: u32, height: u32) -> DarklyEngine { + let (device, queue) = test_device(); + let gpu = GpuContext::new_headless(device, queue); + DarklyEngine::new(gpu, width, height) +} + +fn flood_fill(engine: &mut DarklyEngine, layer: LayerId, x: f32, y: f32, color: [u8; 4]) { + engine.begin_stroke(layer); + engine.stroke_to(StrokeOp::FloodFill { + x, + y, + r: color[0], + g: color[1], + b: color[2], + a: color[3], + tolerance: 0, + }); + engine.end_stroke(); + // Flood fill is async: the readback → `complete_flood_fill` (where the + // commit lives) fires on flush. This is the line that panics against the + // unfixed code. + engine.test_flush_readbacks(); + engine.render(0.0); +} + +/// Fill a layer whose texture extent is smaller than the canvas window after an +/// upward resize. The commit must stay within the saved (layer-extent) snapshot. +#[test] +fn flood_fill_after_upward_resize_does_not_panic() { + // Small doc; a fresh raster layer's texture is sized to the canvas: (0,0,64×32). + let (w, h) = (64u32, 32u32); + let mut engine = test_engine(w, h); + let layer = engine.add_raster_layer(None); + + // Expand upward, doubling height: canvas window (0, -32, 64×64); the layer + // extent stays at (0, 0, 64×32) — resize only moves the window in the plane. + engine.resize_canvas(CanvasRect::from_xywh(0, -(h as i32), w, 2 * h)); + assert_eq!(engine.canvas_dimensions(), (w, 2 * h)); + + // Fill inside the surviving layer extent (canvas y in [0, h)). + flood_fill(&mut engine, layer, 32.0, 16.0, [0, 255, 0, 255]); + + // The seeded pixel (layer-local (32,16)) is now opaque green. + let px = engine.test_readback_layer(layer); + let idx = ((16u32 * w + 32u32) * 4) as usize; + assert!( + px[idx] < 50 && px[idx + 1] > 200 && px[idx + 3] > 200, + "seed pixel must be opaque green, got ({},{},{},{})", + px[idx], + px[idx + 1], + px[idx + 2], + px[idx + 3] + ); +} + +/// Undo restores the pre-fill pixels; redo re-applies the fill. Exercises the +/// commit's readback pipeline end to end for the smaller-than-canvas layer. +#[test] +fn flood_fill_after_upward_resize_undo_redo_round_trips() { + let (w, h) = (64u32, 32u32); + let mut engine = test_engine(w, h); + let layer = engine.add_raster_layer(None); + engine.resize_canvas(CanvasRect::from_xywh(0, -(h as i32), w, 2 * h)); + + let before = engine.test_readback_layer(layer); + flood_fill(&mut engine, layer, 32.0, 16.0, [0, 255, 0, 255]); + let filled = engine.test_readback_layer(layer); + assert_ne!(before, filled, "fill must change the layer pixels"); + + // Let the undo region entry's readback reach `Ready` before restoring. + for _ in 0..16 { + engine.test_flush_readbacks(); + } + engine.undo(); + engine.render(0.0); + let after_undo = engine.test_readback_layer(layer); + assert_eq!( + after_undo, before, + "undo must restore the pre-fill layer pixels" + ); + + engine.redo(); + engine.render(0.0); + engine.test_flush_readbacks(); + let after_redo = engine.test_readback_layer(layer); + assert_eq!(after_redo, filled, "redo must re-apply the fill"); +} diff --git a/crates/darkly/tests/modifier_removal.rs b/crates/darkly/tests/modifier_removal.rs new file mode 100644 index 00000000..68c22382 --- /dev/null +++ b/crates/darkly/tests/modifier_removal.rs @@ -0,0 +1,185 @@ +//! Regression tests for removing a **modifier** (mask) by id through the +//! generic layer-removal entry points. +//! +//! A mask is a `Filter` on a host's `filters` list, not a tree node — but it is +//! a selectable row in the layer panel, so the Delete hotkey forwards its id to +//! `remove_layer` just like a layer's. These pin that every generic entry point +//! (`remove_layer`, `remove_layers`, `merge_down`) recognises a modifier id and +//! routes it to the modifier path instead of treating it as a tree node, and +//! that undo puts the mask back on its host rather than in a `children` list. +//! +//! Run with: +//! `cargo test -p darkly --test modifier_removal --features testing -- --test-threads=1` + +use darkly::engine::DarklyEngine; +use darkly::gpu::context::GpuContext; +use darkly::gpu::test_utils::*; +use darkly::layer::LayerId; + +fn test_engine(width: u32, height: u32) -> DarklyEngine { + let (device, queue) = test_device(); + let gpu = GpuContext::new_headless(device, queue); + DarklyEngine::new(gpu, width, height) +} + +/// True when `id` is a top-level row of the serializable tree. Note that a +/// *filter* id can never appear here even if the document has wrongly parked it +/// in a children list — `node_to_layer_info` resolves nodes only — so absence is +/// not evidence about filters. The document-level inline tests cover that. +fn has_top_level_row(e: &DarklyEngine, id: LayerId) -> bool { + let want = id.to_ffi() as f64; + let tree = serde_json::to_value(e.layer_tree()).expect("layer_tree serializes"); + tree.as_array() + .map(|rows| { + rows.iter().any(|row| { + row.get("id") + .and_then(|v| v.as_f64()) + .is_some_and(|got| (got - want).abs() < 0.5) + }) + }) + .unwrap_or(false) +} + +/// B1 — `remove_layer` on a mask id must actually detach the mask. +#[test] +fn remove_layer_on_a_mask_id_detaches_the_mask() { + let mut e = test_engine(32, 32); + let host = e.add_raster_layer(None); + let _other = e.add_raster_layer(None); + e.add_mask(host); + let mask = e.host_mask_id(host).expect("mask present"); + + e.remove_layer(mask).expect("removing a mask must succeed"); + + assert!( + e.host_mask_id(host).is_none(), + "the mask must be gone from its host" + ); + assert_eq!( + e.layer_tree().len(), + 2, + "only the two raster layers remain as rows" + ); +} + +/// B2 — undo restores the mask onto its host, not into the root's children. +#[test] +fn undoing_a_mask_removal_restores_it_on_its_host() { + let mut e = test_engine(32, 32); + let host = e.add_raster_layer(None); + let _other = e.add_raster_layer(None); + e.add_mask(host); + let mask = e.host_mask_id(host).expect("mask present"); + + e.remove_layer(mask).expect("removing a mask must succeed"); + assert!( + e.host_mask_id(host).is_none(), + "precondition: the mask must actually have been removed" + ); + e.undo(); + + assert_eq!( + e.host_mask_id(host), + Some(mask), + "undo must reattach the mask to its original host" + ); + assert_eq!( + e.layer_tree().len(), + 2, + "undo must not add a row — the mask belongs to its host, not the root" + ); +} + +/// B3 — the batch path must not silently drop modifier ids. +#[test] +fn remove_layers_handles_a_modifier_id_in_the_batch() { + let mut e = test_engine(32, 32); + let host = e.add_raster_layer(None); + let victim = e.add_raster_layer(None); + e.add_mask(host); + let mask = e.host_mask_id(host).expect("mask present"); + + let skipped = e + .remove_layers(vec![mask, victim]) + .expect("batch removal must succeed"); + + assert_eq!(skipped, 0); + assert!( + e.host_mask_id(host).is_none(), + "the mask in the batch must be removed, not skipped" + ); + assert!(!has_top_level_row(&e, victim)); +} + +/// B6 — a mask and a layer removed together undo as one step. +#[test] +fn a_single_undo_restores_both_a_batched_mask_and_layer() { + let mut e = test_engine(32, 32); + let host = e.add_raster_layer(None); + let victim = e.add_raster_layer(None); + e.add_mask(host); + let mask = e.host_mask_id(host).expect("mask present"); + + e.remove_layers(vec![mask, victim]) + .expect("batch removal must succeed"); + assert!( + e.host_mask_id(host).is_none(), + "precondition: the batch must actually have removed the mask" + ); + e.undo(); + + assert_eq!( + e.host_mask_id(host), + Some(mask), + "one undo must restore the mask" + ); + assert!( + has_top_level_row(&e, victim), + "the same undo must restore the layer" + ); +} + +/// B5 — a mask is not a layer, so deleting the only mask of the only layer +/// must not trip the "cannot delete the last layer" guard. +#[test] +fn removing_the_only_mask_of_the_only_layer_is_allowed() { + let mut e = test_engine(32, 32); + let host = e.add_raster_layer(None); + e.add_mask(host); + let mask = e.host_mask_id(host).expect("mask present"); + + e.remove_layer(mask) + .expect("a mask is not the last layer — removal must be allowed"); + + assert!(e.host_mask_id(host).is_none()); + assert!(has_top_level_row(&e, host), "the host layer must survive"); +} + +/// B5 (companion) — the guard still fires for the genuine last layer. +#[test] +fn removing_the_last_layer_is_still_refused() { + let mut e = test_engine(32, 32); + let only = e.add_raster_layer(None); + assert_eq!( + e.remove_layer(only), + Err("Cannot delete the last layer".to_string()) + ); +} + +/// B7 — `merge_down` must reject a modifier id before it reaches the +/// sibling-index arithmetic, which reads the host's `children` list using a +/// position taken from its `filters` list. +#[test] +fn merge_down_rejects_a_modifier_id() { + let mut e = test_engine(32, 32); + let host = e.add_raster_layer(None); + let _below = e.add_raster_layer(None); + e.add_mask(host); + let mask = e.host_mask_id(host).expect("mask present"); + + assert_eq!( + e.merge_down(mask), + Err("Layer not in tree".to_string()), + "a mask has no sibling below it to merge into" + ); +} diff --git a/crates/darkly/tests/noise_field_nonrepeating.rs b/crates/darkly/tests/noise_field_nonrepeating.rs new file mode 100644 index 00000000..ed699045 --- /dev/null +++ b/crates/darkly/tests/noise_field_nonrepeating.rs @@ -0,0 +1,88 @@ +//! Regression: the baked `noise` field must not visibly repeat at the tiny +//! period it used to. The field is baked once and Repeat-sampled to cover the +//! plane, so its repeat period is `BakeSpec::FIELD_SPAN` field units. When that +//! period was 16 field units the grain visibly tiled; the fix makes the period +//! large (`FIELD_SPAN = 128`) while leaving the tile *resolution* (memory) +//! unchanged — the texels stretch across a longer period rather than the tile +//! growing. +//! +//! This test fails against the old 16-unit field and passes against the fixed +//! one, without asserting the constant itself: it bakes the real tile and checks +//! that a shift of *16 field units* — the entire old repeat period — no longer +//! reproduces the field. Under the old period that shift was a full wrap +//! (perfect self-correlation); under the large period it lands on unrelated +//! content, as decorrelated as a reference taken half the tile away. + +use darkly::brush::texture_source::{BakeChannels, BakeKind, BakeSpec}; +use darkly::gpu::baked_source_cache::BakedSourceCache; +use darkly::gpu::test_utils::{readback_texture, test_device}; + +/// The `noise` node's default field (octaves=4, chromatic), matching the +/// baked-source cache's other tests. +fn default_noise_spec() -> BakeSpec { + BakeSpec { + kind: BakeKind::Noise { + seed: 1, + octaves: 4, + warp_q: BakeKind::quantize(0.6), + roughness_q: BakeKind::quantize(0.5), + }, + channels: BakeChannels::Rgba, + resolution: BakeSpec::resolution_for_octaves(4), + } +} + +#[test] +fn baked_field_does_not_repeat_at_the_old_period() { + let (device, queue) = test_device(); + let cache = BakedSourceCache::new(); + let spec = default_noise_spec(); + let tile = cache.get_or_bake(&device, &queue, &spec); + let res = spec.resolution as usize; + let px = readback_texture( + &device, + &queue, + &tile.texture, + wgpu::TextureFormat::Rgba8Unorm, + res as u32, + res as u32, + ); + let r = |x: usize, y: usize| px[(y * res + x) * 4] as i32; + + // How many texels correspond to the old 16-field-unit repeat period. The + // whole tile spans FIELD_SPAN field units across `res` texels, so 16 field + // units is `res * 16 / FIELD_SPAN` texels. At FIELD_SPAN=16 this is the + // full tile width (a wrap-around → identical); at 128 it is res/8. + let old_period_units = 16.0_f32; + // At FIELD_SPAN=16 this equals `res` (a full wrap → `% res` compares each + // column with itself → Δ 0), which is exactly the repeat this test rejects. + let shift = (res as f32 * old_period_units / BakeSpec::FIELD_SPAN).round() as usize; + assert!(shift > 0, "old-period shift must be positive"); + + // Mean |Δ| between each column and the one `shift` texels away (the old + // repeat offset), and — as a self-calibrating "unrelated" reference on the + // same backend — between each column and the one half a tile away. Half a + // tile is 8 field units under the old period and 64 under the new, so it is + // guaranteed-unrelated content either way. + let far = res / 2; + let mean = |off: usize| { + let mut acc = 0i64; + for y in 0..res { + for x in 0..res { + acc += (r(x, y) - r((x + off) % res, y)).abs() as i64; + } + } + acc as f64 / (res * res) as f64 + }; + let delta_old_period = mean(shift); + let delta_unrelated = mean(far); + + // The field must change substantially over the old period — as much as it + // does over a known-unrelated distance. Under the old 16-unit period the + // shift was a full wrap, so `delta_old_period` was ~0 and this fails. + assert!( + delta_old_period > delta_unrelated * 0.5, + "field still self-similar at the old 16-unit period: Δ@old-period {delta_old_period:.2} \ + vs Δ@unrelated {delta_unrelated:.2} — the grain visibly repeats", + ); +} diff --git a/crates/darkly/tests/noise_frame.rs b/crates/darkly/tests/noise_frame.rs index 04580626..0aa40461 100644 --- a/crates/darkly/tests/noise_frame.rs +++ b/crates/darkly/tests/noise_frame.rs @@ -48,8 +48,8 @@ fn preview_target(device: &wgpu::Device) -> (wgpu::Texture, wgpu::TextureView) { } /// Dab-space noise → paint, with `pen.drawing_angle → noise.rotation`. -/// `scale_with_brush = false` and a small `scale` pack several fBm cells -/// across the stamp so rotation produces a strong, easily-probed change. +/// A small `scale` packs several fBm cells across the stamp so rotation +/// produces a strong, easily-probed change. fn build_dab_noise_graph() -> Graph { let reg = registry(); let mut graph = Graph::::new(); @@ -67,7 +67,6 @@ fn build_dab_noise_graph() -> Graph { ("warp", InputValue::Scalar(0.6)), ("roughness", InputValue::Scalar(0.5)), ("space", InputValue::Int(1)), - ("scale_with_brush", InputValue::Bool(false)), ] { graph.set_port_value(&noise, name, v).unwrap(); } diff --git a/crates/darkly/tests/paste_mask.rs b/crates/darkly/tests/paste_mask.rs new file mode 100644 index 00000000..1c10b0c8 --- /dev/null +++ b/crates/darkly/tests/paste_mask.rs @@ -0,0 +1,246 @@ +//! Regression tests for the copy/paste ↔ mask interaction. +//! +//! Model: plain paste always makes its own layer; pasting INTO the active +//! target (a layer or a mask) is the "paste in place" verb, which floats the +//! clip onto the active node and commits through the shared `commit_floating` +//! path (RGBA layers and R8 masks alike). The three defects covered: +//! 1. Paste-in-place must write into the active mask (not silently no-op, +//! and not create a new layer), and be undoable. +//! 2. Copying a *region* (selection active) of a masked layer must produce a +//! flat paste with no mask. +//! 3. Pasting must never seed the restored mask from the receiving document's +//! active selection. +//! +//! Run with: `cargo test -p darkly --test paste_mask -- --test-threads=1` + +use darkly::document::SelectionMode; +use darkly::engine::types::{LayerInfo, StrokeOp}; +use darkly::engine::DarklyEngine; +use darkly::gpu::context::GpuContext; +use darkly::gpu::test_utils::test_device; +use darkly::layer::LayerId; + +fn test_engine(width: u32, height: u32) -> DarklyEngine { + let (device, queue) = test_device(); + let gpu = GpuContext::new_headless(device, queue); + DarklyEngine::new(gpu, width, height) +} + +/// Paint a dab of the given straight-alpha colour and render one frame. +fn paint_dot(engine: &mut DarklyEngine, layer_id: LayerId, x: f32, y: f32, rgb: (f32, f32, f32)) { + engine.begin_stroke(layer_id); + engine.stroke_to(StrokeOp::BrushStroke { + x, + y, + pressure: 1.0, + x_tilt: 0.0, + y_tilt: 0.0, + rotation: 0.0, + tangential_pressure: 0.0, + time_ms: 0.0, + cr: rgb.0, + cg: rgb.1, + cb: rgb.2, + ca: 1.0, + }); + engine.end_stroke(); + engine.render(0.0); +} + +/// Drive any in-flight async readbacks to completion. +fn settle(engine: &mut DarklyEngine) { + for _ in 0..8 { + engine.test_flush_readbacks(); + engine.render(0.0); + } +} + +/// Copy `src`'s pixels into the internal clipboard (async readback drained), +/// so the paste verbs have something to paste. +fn copy_into_clipboard(engine: &mut DarklyEngine, src: LayerId) { + engine.copy(src); + settle(engine); +} + +/// Block until the rich-copy readback completes and return its JSON. +fn drain_rich_copy(engine: &mut DarklyEngine) -> String { + settle(engine); + engine + .poll_copy_rich_result() + .expect("rich copy never produced a result") +} + +/// `(name, opacity, blend_mode, modifier_count)` for a top-level raster layer. +fn raster_props(engine: &DarklyEngine, id: LayerId) -> (String, f32, String, usize) { + let id_f = id.to_ffi() as f64; + for info in engine.layer_tree() { + if let LayerInfo::Raster { + id: lid, + name, + opacity, + blend_mode, + modifiers, + .. + } = info + { + if (lid - id_f).abs() < 0.5 { + return (name, opacity, blend_mode.to_string(), modifiers.len()); + } + } + } + panic!("layer {id_f} not found in tree"); +} + +/// Bug 1 (floating verb): paste-in-place floats the clipboard onto the active +/// MASK and commits into it — no new layer, and undoable. This is the path the +/// UI takes when "activate transform after paste" is on. +#[test] +fn paste_in_place_floating_writes_into_active_mask() { + let (w, h) = (64u32, 64u32); + let mut e = test_engine(w, h); + + // A source layer to copy from, plus the masked host we paste into. + let src = e.add_raster_layer(None); + paint_dot(&mut e, src, 32.0, 32.0, (0.25, 0.78, 0.13)); + let host = e.add_raster_layer(None); + e.add_mask(host); + let mask = e.test_mask_id(host).expect("host has a mask filter"); + settle(&mut e); + + copy_into_clipboard(&mut e, src); + + // Baseline: the fresh mask is uniform. + let base = e.test_readback_mask(host); + let v0 = base[0]; + assert!(base.iter().all(|&v| v == v0), "fresh mask must be uniform"); + + let layers_before = e.layer_tree().len(); + assert!( + e.paste_in_place_floating(mask), + "paste-in-place must float the clipboard onto the mask (not no-op)" + ); + e.commit_floating(); + settle(&mut e); + + assert_eq!( + e.layer_tree().len(), + layers_before, + "pasting into a mask must not add a layer" + ); + let after = e.test_readback_mask(host); + assert!( + after.iter().any(|&v| v != v0), + "paste-in-place must write pixels into the mask" + ); + + // Undoable: the mask returns to its pre-paste (uniform) state. + e.undo(); + settle(&mut e); + let undone = e.test_readback_mask(host); + assert!( + undone.iter().all(|&v| v == v0), + "undo must restore the mask to its pre-paste pixels" + ); +} + +/// Bug 1 (committed verb): with transform-after-paste off, paste-in-place +/// commits the clipboard straight into the active mask. +#[test] +fn paste_in_place_committed_writes_into_active_mask() { + let (w, h) = (64u32, 64u32); + let mut e = test_engine(w, h); + + let src = e.add_raster_layer(None); + paint_dot(&mut e, src, 32.0, 32.0, (0.25, 0.78, 0.13)); + let host = e.add_raster_layer(None); + e.add_mask(host); + let mask = e.test_mask_id(host).expect("host has a mask filter"); + settle(&mut e); + + copy_into_clipboard(&mut e, src); + + let base = e.test_readback_mask(host); + let v0 = base[0]; + let layers_before = e.layer_tree().len(); + + let target = e + .paste_in_place(Some(mask)) + .expect("committed paste-in-place must succeed"); + assert_eq!(target, mask, "committed paste-in-place targets the mask"); + settle(&mut e); + + assert_eq!( + e.layer_tree().len(), + layers_before, + "committed paste into a mask must not add a layer" + ); + let after = e.test_readback_mask(host); + assert!( + after.iter().any(|&v| v != v0), + "committed paste-in-place must write pixels into the mask" + ); +} + +/// Bug 2: a region copy (selection active) of a masked layer pastes flat, with +/// no mask attached. +#[test] +fn region_copy_of_masked_layer_pastes_without_mask() { + let (w, h) = (32u32, 32u32); + let mut source = test_engine(w, h); + let layer = source.add_raster_layer(None); + paint_dot(&mut source, layer, 16.0, 16.0, (1.0, 0.0, 0.0)); + source.add_mask(layer); + + // Active selection ⇒ this is a region copy, not a whole-layer copy. + source.select_rect(0.0, 0.0, 16.0, 16.0, SelectionMode::Replace, false, 0.0); + settle(&mut source); + + source.copy_layer_rich(layer); + let json = drain_rich_copy(&mut source); + + let mut sink = test_engine(w, h); + let pasted = sink.paste_layer_rich(&json, None).expect("paste succeeds"); + + let (_, _, _, modifier_count) = raster_props(&sink, pasted); + assert_eq!( + modifier_count, 0, + "a region copy must not carry a mask onto the pasted layer" + ); +} + +/// Bug 3: pasting a masked layer must not seed the restored mask from the +/// receiving document's active selection. +#[test] +fn paste_does_not_seed_mask_from_active_selection() { + let (w, h) = (32u32, 32u32); + + // Whole-layer copy (no selection) — the mask travels with the layer. + let mut source = test_engine(w, h); + let layer = source.add_raster_layer(None); + paint_dot(&mut source, layer, 16.0, 16.0, (1.0, 0.0, 0.0)); + source.add_mask(layer); + source.copy_layer_rich(layer); + let json = drain_rich_copy(&mut source); + assert!( + json.contains("\"mask\":{"), + "mask metadata must be in the JSON" + ); + + // Receiving document has an active selection when the paste happens. + let mut sink = test_engine(w, h); + sink.select_rect(0.0, 0.0, 16.0, 16.0, SelectionMode::Replace, false, 0.0); + settle(&mut sink); + + let pasted = sink.paste_layer_rich(&json, None).expect("paste succeeds"); + settle(&mut sink); + + // A mask seeded from the half-canvas selection would be non-uniform + // (255 inside, 0 outside). The restored mask must be uniform (unseeded). + let m = sink.test_readback_mask(pasted); + let first = m[0]; + assert!( + m.iter().all(|&v| v == first), + "the restored mask must be uniform — paste must not seed it from the \ + active selection" + ); +} diff --git a/crates/darkly/tests/settable_source.rs b/crates/darkly/tests/settable_source.rs index 9c557a0f..ec39c892 100644 --- a/crates/darkly/tests/settable_source.rs +++ b/crates/darkly/tests/settable_source.rs @@ -6,7 +6,13 @@ //! every builtin still compiles. use darkly::brush::builtin_brushes; +use darkly::brush::eval::BrushGraphRunner; use darkly::brush::nodes::brush_settings; +use darkly::brush::paint_info::PaintInformation; +use darkly::brush::registry; +use darkly::brush::wire::ScalarValue; +use darkly::brush::DAB_REFERENCE_SIZE; +use darkly::nodegraph::{Graph, PortRef}; /// Expected base size per builtin after the migration. The four non-default /// brushes are the ones whose base didn't come from the registration default @@ -19,8 +25,8 @@ fn expected_base(name: &str) -> f32 { "Blur" => 0.2, "Charcoal" => 0.25, "Calligraphy" => 0.05, - "Hair" => 0.568_536_7, - "Twirly Hair" => 0.4, + "Hair" => 0.2, + "Sponge" => 0.2, _ => 0.1, } } @@ -63,3 +69,67 @@ fn builtins_own_base_size_on_brush_settings_and_preserve_its_value() { .unwrap_or_else(|e| panic!("{name}: brush must compile: {e:?}")); } } + +/// Wire `brush_settings.size → multiply.a`, set the base size and the `b` +/// factor, run one dab, and read `multiply.result`. +fn size_through_multiply(base_size: f32, b: f32) -> f32 { + let registry = registry(); + let mut graph = Graph::new(); + + let bs_reg = registry.get(brush_settings::TYPE_ID).unwrap(); + let bs = graph.add_node(brush_settings::TYPE_ID, bs_reg.ports.clone()); + graph.set_port_default(&bs, "size", base_size).unwrap(); + + let mul_reg = registry.get("multiply").unwrap(); + let mul = graph.add_node("multiply", mul_reg.ports.clone()); + graph.set_port_default(&mul, "b", b).unwrap(); + + graph + .connect( + PortRef { + node: bs, + port: "size".into(), + }, + PortRef { + node: mul, + port: "a".into(), + }, + ) + .unwrap(); + + let mut runner = + BrushGraphRunner::new(&graph, registry.as_map(), registry.evaluators()).unwrap(); + runner.set_base_size(base_size); + runner.seed_sensors(&PaintInformation::default(), [0.0, 0.0, 0.0, 1.0], 42, 0); + runner.execute_cpu(); + + let slot = runner.find_output_slot("multiply", "result").unwrap(); + match runner.read_slot(slot).expect("result has value") { + ScalarValue::Scalar(v) => v, + other => panic!("expected Scalar, got {other:?}"), + } +} + +/// The `brush_settings.size` graph signal is published in canvas pixels (the +/// brush *diameter* = `base_size * DAB_REFERENCE_SIZE`), so a math node routed +/// between it and a pixel sink operates in the same domain: `× 1` is a true +/// no-op and gains > 1 scale the pixel value directly. +/// +/// Regression: before this, `size` published the raw normalized `0..4` knob +/// value, so any math node (or a direct wire into a pixel field like +/// `noise.scale`) received a sub-pixel number that aliased into garbage. +#[test] +fn size_signal_is_brush_diameter_in_pixels() { + let px = DAB_REFERENCE_SIZE as f32; + + // × 1 is a no-op: the signal is exactly the diameter in pixels. + assert!((size_through_multiply(0.1, 1.0) - 0.1 * px).abs() < 1e-3); + assert!((size_through_multiply(0.4, 1.0) - 0.4 * px).abs() < 1e-3); + + // A gain > 1 scales the pixel value (the multiply cap only bounds the + // manual slider widget, never a wired/authored value). + assert!((size_through_multiply(0.1, 2.0) - 0.1 * px * 2.0).abs() < 1e-3); + + // Scaling down works too. + assert!((size_through_multiply(0.2, 0.5) - 0.2 * px * 0.5).abs() < 1e-3); +} diff --git a/crates/darkly/tests/watercolor.rs b/crates/darkly/tests/watercolor.rs index b9f46649..36141d84 100644 --- a/crates/darkly/tests/watercolor.rs +++ b/crates/darkly/tests/watercolor.rs @@ -42,6 +42,57 @@ fn solid_canvas(rgba: [u8; 4]) -> Vec { out } +/// One `flush_dabs` worth of dabs, plus whether the stroke restarts in +/// front of it. +/// +/// Flush boundaries are semantically load-bearing for watercolor: the +/// pickup atlas is rebuilt once per flush, so a mark's buildup depends on +/// how the dabs are grouped. Tests must therefore be able to say "these +/// dabs in one flush" versus "one dab per flush" — hence groups rather +/// than a flat dab list. +struct FlushGroup<'a> { + dabs: &'a [(f32, f32)], + /// Re-enter `begin_stroke` before this group — the stabilizer-rewind + /// path. `begin_stroke` always runs before the first group; this only + /// affects later ones. + restart: bool, + /// Brush colour for this group, overriding the render call's default. + color: Option<[f32; 4]>, +} + +fn group(dabs: &[(f32, f32)]) -> FlushGroup<'_> { + FlushGroup { + dabs, + restart: false, + color: None, + } +} + +fn restart_group(dabs: &[(f32, f32)]) -> FlushGroup<'_> { + FlushGroup { + dabs, + restart: true, + color: None, + } +} + +impl<'a> FlushGroup<'a> { + fn with_color(mut self, color: [f32; 4]) -> Self { + self.color = Some(color); + self + } +} + +/// `n` groups of one dab each at the same spot — the shape that exposes +/// cross-flush pigment buildup. +fn repeat_at(pos: (f32, f32), n: usize) -> Vec<[(f32, f32); 1]> { + vec![[pos]; n] +} + +fn groups_of<'a>(runs: &'a [[(f32, f32); 1]]) -> Vec> { + runs.iter().map(|r| group(r.as_slice())).collect() +} + fn render_dabs( brush_name: &str, size_override: f32, @@ -57,6 +108,29 @@ fn render_dabs_on( color: [f32; 4], dabs: &[(f32, f32)], canvas: &[u8], +) -> Vec { + render_flush_groups(brush_name, size_override, color, &[group(dabs)], canvas) +} + +/// The single rendering primitive every test in this file goes through. +/// +/// Each group gets its own `BrushGpuContext` and its own `queue.submit()`, +/// mirroring production where each render phase has its own encoder and +/// `submit_final` (`engine/painting.rs`). **The submit between groups is +/// load-bearing, not incidental:** `flush_dabs` writes the dab buffer and +/// the uniform rings through `queue.write_buffer`, which is on the queue +/// timeline rather than the encoder timeline. Two `flush_dabs` calls +/// recorded into one encoder would both read the *second* batch, and a +/// multi-flush test would silently collapse into a single flush while +/// still appearing to pass. +/// +/// `commit` runs once, in the final group's context. +fn render_flush_groups( + brush_name: &str, + size_override: f32, + color: [f32; 4], + groups: &[FlushGroup<'_>], + canvas: &[u8], ) -> Vec { let brush = darkly::brush::builtin_brushes::all() .into_iter() @@ -132,25 +206,29 @@ fn render_dabs_on( }}; } - { - let mut ctx = make_ctx!("watercolor-compiled-test-begin"); - runner.begin_stroke(&mut ctx); - queue.submit([ctx.encoder.finish()]); - } - { + let mut dab_index = 0u32; + for (gi, g) in groups.iter().enumerate() { let mut ctx = make_ctx!("watercolor-compiled-test-flush"); - for (i, (x, y)) in dabs.iter().enumerate() { + if gi == 0 || g.restart { + runner.begin_stroke(&mut ctx); + } + for (x, y) in g.dabs { let info = PaintInformation { pos: [*x, *y], pressure: 1.0, ..Default::default() }; - runner.seed_sensors(&info, color, 0xC0FFEE, i as u32); + runner.seed_sensors(&info, g.color.unwrap_or(color), 0xC0FFEE, dab_index); runner.execute_cpu(); runner.execute_gpu(&mut ctx); + dab_index += 1; } runner.flush_dabs(&mut ctx); - runner.commit(&mut ctx); + if gi + 1 == groups.len() { + runner.commit(&mut ctx); + } + // Per-group submit — see the doc comment; without this the next + // group's queue writes would land before this group's passes run. queue.submit([ctx.encoder.finish()]); } @@ -264,128 +342,21 @@ fn rough_watercolor_renders_multiple_dabs_in_one_flush() { /// `begin_stroke`. #[test] fn begin_stroke_clears_scratch_so_rewind_drops_defunct_pigment() { - let brush = darkly::brush::builtin_brushes::all() - .into_iter() - .find(|b| b.metadata.name == "Smooth Watercolor") - .expect("Smooth Watercolor brush registered"); - - let mut graph = brush.metadata.graph.clone(); - let _term_id = darkly::brush::find_terminal(&graph).expect("watercolor terminal"); // Small dab — at `size = 0.05` the dab radius is ~13 px (size * - // DAB_REFERENCE_SIZE / 2 ≈ 12.8), so the two dab positions chosen - // below (40, 64) and (88, 64) are well isolated and don't overlap. - graph - .set_port_default( - &darkly::brush::nodes::brush_settings::node_id(&graph).unwrap(), - "size", - 0.05, - ) - .unwrap(); - - let canvas = light_blue_canvas(); - let (device, queue) = shared_device(); - let (layer_texture, layer_view) = create_test_texture(&device, &queue, CANVAS, CANVAS, &canvas); - let pipelines = BrushPipelines::new( - &device, - &queue, - &darkly::gpu::selection::selection_mask_bgl(&device), - ); - let mut stroke_buffer = StrokeBuffer::new(&device, CANVAS, CANVAS, &pipelines); - - let pre_stroke = darkly::gpu::paint_target::GpuPaintTarget::from_canvas_texture( - &layer_texture, - &layer_view, - wgpu::TextureFormat::Rgba8Unorm, - darkly::coord::CanvasRect::from_xywh(0, 0, CANVAS, CANVAS), - ); - let mut enc = device.create_command_encoder(&wgpu::CommandEncoderDescriptor { - label: Some("watercolor-rewind-pre-stroke"), - }); - stroke_buffer.save_pre_stroke(&device, &mut enc, &pipelines, &pre_stroke); - queue.submit([enc.finish()]); - - let mut runner: BrushGraphRunner = compile_graph(&graph).expect("brush compiles"); - macro_rules! make_ctx { - ($label:expr) => {{ - let (scratch, pre_stroke_tex, pre_stroke_bg, source_override) = - stroke_buffer.parts_for_brush_ctx(); - BrushGpuContext { - encoder: device.create_command_encoder(&wgpu::CommandEncoderDescriptor { - label: Some($label), - }), - device: &device, - queue: &queue, - pipelines: &pipelines, - selection_bind_group: pipelines.default_selection_bind_group(), - canvas_width: CANVAS, - canvas_height: CANVAS, - canvas_origin: [0, 0], - blend_mode: 0, - view_rotation: 0.0, - perf: BrushPerfCounters::default(), - stroke: Some(StrokeResources { - scratch, - paint_target: darkly::gpu::paint_target::GpuPaintTarget::from_canvas_texture( - &layer_texture, - &layer_view, - wgpu::TextureFormat::Rgba8Unorm, - darkly::coord::CanvasRect::from_xywh(0, 0, CANVAS, CANVAS), - ), - pre_stroke_texture: pre_stroke_tex, - pre_stroke_bind_group: pre_stroke_bg, - source_override, - }), - preview: None, - dab_batch: DabBatch::default(), - } - }}; - } - - // Phase 1: begin_stroke + render a dab at (40, 64) into the scratch. - // Do NOT commit — the painting loop's rewind branch does the same - // thing: stale dabs sit in the scratch when the next begin_stroke runs. - { - let mut ctx = make_ctx!("watercolor-rewind-begin-1"); - runner.begin_stroke(&mut ctx); - let info = PaintInformation { - pos: [40.0, 64.0], - pressure: 1.0, - ..Default::default() - }; - runner.seed_sensors(&info, [1.0, 0.0, 0.0, 1.0], 0xC0FFEE, 0); - runner.execute_cpu(); - runner.execute_gpu(&mut ctx); - runner.flush_dabs(&mut ctx); - queue.submit([ctx.encoder.finish()]); - } - - // Phase 2: simulate stabilizer rewind. begin_stroke again, then a - // different dab at (88, 64), then commit. The (40, 64) pigment must - // not survive — that's exactly the defunct stroke the rewind throws - // away. - { - let mut ctx = make_ctx!("watercolor-rewind-begin-2"); - runner.begin_stroke(&mut ctx); - let info = PaintInformation { - pos: [88.0, 64.0], - pressure: 1.0, - ..Default::default() - }; - runner.seed_sensors(&info, [1.0, 0.0, 0.0, 1.0], 0xC0FFEE, 1); - runner.execute_cpu(); - runner.execute_gpu(&mut ctx); - runner.flush_dabs(&mut ctx); - runner.commit(&mut ctx); - queue.submit([ctx.encoder.finish()]); - } - - let rgba = readback_texture( - &device, - &queue, - &layer_texture, - wgpu::TextureFormat::Rgba8Unorm, - CANVAS, - CANVAS, + // DAB_REFERENCE_SIZE / 2 ≈ 12.8), so the two dab positions (40, 64) + // and (88, 64) are well isolated and don't overlap. + // + // Two flush groups, each preceded by `begin_stroke`: the first lays + // the defunct dab, the second is the rewind that must wipe it. + let rgba = render_flush_groups( + "Smooth Watercolor", + 0.05, + [1.0, 0.0, 0.0, 1.0], + &[ + restart_group(&[(40.0, 64.0)]), + restart_group(&[(88.0, 64.0)]), + ], + &light_blue_canvas(), ); // The defunct dab at (40, 64) must be wiped. Allow ±1 LSB for rounding. @@ -407,3 +378,177 @@ fn begin_stroke_clears_scratch_so_rewind_drops_defunct_pigment() { "surviving dab at (88, 64) should still show red lift, got {surviving:?}" ); } + +/// Regression: pigment must keep building where the brush passes more than +/// once. +/// +/// The watercolor pickup atlas samples the canvas under each dab to decide +/// what colour to deposit. That sample used to come from the pre-stroke +/// snapshot — a texture frozen when the stroke began — so the deposited +/// load was identical on the first pass and the twentieth. The mark +/// converged after two or three passes and then stopped changing, well +/// short of the brush colour, no matter how long the brush dwelled. +/// +/// Painting one dab per flush at a fixed spot, three flushes versus eight: +/// with the pickup frozen, both land on the same colour (measured +/// `(177, 75, 116)` vs `(177, 74, 114)` — identical red, and blue moving +/// 2 LSB the *wrong* way). Reading the live canvas instead, the eight-pass +/// mark is visibly further along toward red. +#[test] +fn watercolor_pigment_builds_up_across_flushes() { + let canvas = light_blue_canvas(); + let runs_3 = repeat_at((64.0, 64.0), 3); + let runs_8 = repeat_at((64.0, 64.0), 8); + + let after_3 = pixel( + &render_flush_groups( + "Smooth Watercolor", + 0.2, + [1.0, 0.0, 0.0, 1.0], + &groups_of(&runs_3), + &canvas, + ), + 64, + 64, + ); + let after_8 = pixel( + &render_flush_groups( + "Smooth Watercolor", + 0.2, + [1.0, 0.0, 0.0, 1.0], + &groups_of(&runs_8), + &canvas, + ), + 64, + 64, + ); + + // Margin 8 clears the 1–2 LSB of rounding headroom by ~4×. + assert!( + after_8[0] > after_3[0] + 8, + "red must keep building past three passes: 3 flushes {after_3:?}, 8 flushes {after_8:?}", + ); + assert!( + after_8[2] + 8 < after_3[2], + "blue must keep receding past three passes: 3 flushes {after_3:?}, 8 flushes {after_8:?}", + ); +} + +/// The pickup is a *neighbourhood* average, not a point sample — that is +/// the whole reason the atlas pass exists. A dab laid next to wet paint +/// must pull colour from it laterally. +/// +/// Without this, `watercolor_pigment_builds_up_across_flushes` would pass +/// for any per-flush-varying input at all; this pins the mechanism. +#[test] +fn watercolor_pickup_bleeds_neighbouring_wet_paint() { + let canvas = light_blue_canvas(); + // At size 0.2 the dab radius is 51.2 px (0.2 × DAB_REFERENCE_SIZE × 0.5) + // and `pickup_size` defaults to 1.0, so the target dab's pickup window + // is also ±51.2 px about its centre. + // + // The probe pixel is the crux. The atlas holds ONE pickup value per dab, + // sampled around that dab's centre and then applied across its whole + // footprint. So probe at a pixel the *target* covers but the neighbour + // does not: the only way the neighbour's colour can reach it is through + // the target's pickup. + // + // target (64, 64) covers x ∈ [12.8, 115.2] + // neighbour(94, 64) covers x ∈ [42.8, 145.2] + // probe (30, 64) — inside the target, 12.8 px clear of the neighbour, + // and the neighbour's paint sits inside the + // target's [12.8, 115.2] pickup window. + let neighbour = (94.0, 64.0); + let target = (64.0, 64.0); + const PROBE: (u32, u32) = (30, 64); + + const WHITE: [f32; 4] = [1.0, 1.0, 1.0, 1.0]; + const RED: [f32; 4] = [1.0, 0.0, 0.0, 1.0]; + + // A red dab in the first flush, then white over the overlap. + let with_neighbour = render_flush_groups( + "Smooth Watercolor", + 0.2, + WHITE, + &[ + group(&[neighbour]).with_color(RED), + group(&[target]).with_color(WHITE), + ], + &canvas, + ); + // The same white dab, but the first flush lays white too — identical + // flush structure and coverage, only the neighbour's colour differs. + let alone = render_flush_groups( + "Smooth Watercolor", + 0.2, + WHITE, + &[ + group(&[neighbour]).with_color(WHITE), + group(&[target]).with_color(WHITE), + ], + &canvas, + ); + + let bled = pixel(&with_neighbour, PROBE.0, PROBE.1); + let clean = pixel(&alone, PROBE.0, PROBE.1); + // Both runs paint white here with identical geometry and identical + // flush structure; only the neighbour's colour differs. A white brush + // over a red-tinted pickup lands pinker — less green and less blue — + // than the same brush over a white pickup. + assert!( + bled[1] + 4 < clean[1] && bled[2] + 4 < clean[2], + "the target dab's pickup should carry its red neighbour's wet paint to {PROBE:?}, \ + which the neighbour itself never covers: with red neighbour {bled:?}, \ + with white neighbour {clean:?}", + ); +} + +/// Buildup must also work from nothing. On an empty layer the pickup +/// alpha is zero, so the pickup branch stays disabled and watercolor +/// degenerates to plain paint on the first flush; from the second flush +/// on, the wet paint it just laid down is what it picks up. Guards +/// against an unpremultiply-by-zero drift toward black and against alpha +/// running away. +#[test] +fn watercolor_builds_up_on_transparent_canvas() { + let transparent = solid_canvas([0, 0, 0, 0]); + let runs_1 = repeat_at((64.0, 64.0), 1); + let runs_6 = repeat_at((64.0, 64.0), 6); + + let after_1 = pixel( + &render_flush_groups( + "Smooth Watercolor", + 0.2, + [1.0, 0.0, 0.0, 1.0], + &groups_of(&runs_1), + &transparent, + ), + 64, + 64, + ); + let after_6 = pixel( + &render_flush_groups( + "Smooth Watercolor", + 0.2, + [1.0, 0.0, 0.0, 1.0], + &groups_of(&runs_6), + &transparent, + ), + 64, + 64, + ); + + assert!( + after_6[3] > after_1[3], + "coverage must strengthen across flushes on an empty layer: 1 flush {after_1:?}, 6 flushes {after_6:?}", + ); + assert!( + after_6[0] >= after_1[0], + "red must not drift backwards (unpremultiply-by-zero would pull toward black): \ + 1 flush {after_1:?}, 6 flushes {after_6:?}", + ); + assert!( + after_6[1] < 40 && after_6[2] < 40, + "a pure red brush must stay red, not grey out: 6 flushes {after_6:?}", + ); +} diff --git a/crates/darkly/tests/wgsl.rs b/crates/darkly/tests/wgsl.rs index 0ab6a569..b803281c 100644 --- a/crates/darkly/tests/wgsl.rs +++ b/crates/darkly/tests/wgsl.rs @@ -1284,14 +1284,9 @@ fn static_texture_brush_preview_declares_single_graph_texture() { // unit frame so the grain rides the rotating stamp. These tests assert the // emitter picks the right arm and that both shader variants stay valid. -/// Apply the noise node's inputs in registration order, with -/// `space`/`scale_with_brush`. `space`: 0 = Canvas, 1 = Dab. -fn apply_noise_inputs( - graph: &mut Graph, - noise: &NodeId, - space: i32, - scale_with_brush: bool, -) { +/// Apply the noise node's inputs in registration order. `space`: 0 = Canvas, +/// 1 = Dab. +fn apply_noise_inputs(graph: &mut Graph, noise: &NodeId, space: i32) { for (name, v) in [ ("scale", InputValue::Scalar(32.0)), ("seed", InputValue::Int(7)), @@ -1299,7 +1294,6 @@ fn apply_noise_inputs( ("warp", InputValue::Scalar(0.6)), ("roughness", InputValue::Scalar(0.5)), ("space", InputValue::Int(space)), - ("scale_with_brush", InputValue::Bool(scale_with_brush)), ] { graph.set_port_value(noise, name, v).unwrap(); } @@ -1341,7 +1335,7 @@ fn noise_canvas_space_is_byte_identical() { let mut graph = Graph::::new(); let pen = graph.add_node("pen_input", reg.get("pen_input").unwrap().ports.clone()); let noise = graph.add_node("noise", reg.get("noise").unwrap().ports.clone()); - apply_noise_inputs(&mut graph, &noise, 0, true); + apply_noise_inputs(&mut graph, &noise, 0); let term = graph.add_node("paint", reg.get("paint").unwrap().ports.clone()); wire( &mut graph, @@ -1391,7 +1385,7 @@ fn settings_size_source_reaches_compiled_brush() { .set_port_default(&settings, "size", 0.25) .expect("brush_settings has a size input"); let noise = graph.add_node("noise", reg.get("noise").unwrap().ports.clone()); - apply_noise_inputs(&mut graph, &noise, 0, true); + apply_noise_inputs(&mut graph, &noise, 0); let term = graph.add_node("paint", reg.get("paint").unwrap().ports.clone()); wire( &mut graph, @@ -1432,7 +1426,7 @@ fn noise_scale_wired_emits_upstream_expr_and_validates() { let mut graph = Graph::::new(); let pen = graph.add_node("pen_input", reg.get("pen_input").unwrap().ports.clone()); let noise = graph.add_node("noise", reg.get("noise").unwrap().ports.clone()); - apply_noise_inputs(&mut graph, &noise, 0, true); + apply_noise_inputs(&mut graph, &noise, 0); let term = graph.add_node("paint", reg.get("paint").unwrap().ports.clone()); wire( &mut graph, @@ -1468,7 +1462,7 @@ fn noise_octaves_wired_emits_i32_clamp_and_validates() { let pen = graph.add_node("pen_input", reg.get("pen_input").unwrap().ports.clone()); let rand = graph.add_node("random", reg.get("random").unwrap().ports.clone()); let noise = graph.add_node("noise", reg.get("noise").unwrap().ports.clone()); - apply_noise_inputs(&mut graph, &noise, 0, true); + apply_noise_inputs(&mut graph, &noise, 0); let term = graph.add_node("paint", reg.get("paint").unwrap().ports.clone()); wire( &mut graph, @@ -1491,13 +1485,14 @@ fn noise_octaves_wired_emits_i32_clamp_and_validates() { #[test] fn noise_dab_space_emits_oriented_frame_and_variation() { // Dab mode must rotate the unit-disc offset by the `rotation` input and - // fold the `variation` offset. With both inputs unwired they fall to - // literal defaults (0), so the basis and offset still appear. + // fold the per-dab `variation` offset (a 2D hash of `variation` bounded to + // the field period). With both inputs unwired they fall to literal defaults + // (0), so the basis and offset still appear. let reg = registry(); let mut graph = Graph::::new(); let pen = graph.add_node("pen_input", reg.get("pen_input").unwrap().ports.clone()); let noise = graph.add_node("noise", reg.get("noise").unwrap().ports.clone()); - apply_noise_inputs(&mut graph, &noise, 1, true); + apply_noise_inputs(&mut graph, &noise, 1); let term = graph.add_node("paint", reg.get("paint").unwrap().ports.clone()); wire( &mut graph, @@ -1518,8 +1513,9 @@ fn noise_dab_space_emits_oriented_frame_and_variation() { "Dab basis must rotate by the rotation input" ); assert!( - w.contains("* 64.0"), - "Dab mode must fold the per-dab variation offset" + w.contains("fbm_offset2(u32(max(") && w.contains("* 4096.0), 128.000000)"), + "Dab mode must fold the per-dab variation offset via the 2D hash \ + bounded to the noise field period (FIELD_SPAN = 128)" ); assert!( !w.contains("target_pos / 32.000000"), @@ -1530,42 +1526,30 @@ fn noise_dab_space_emits_oriented_frame_and_variation() { } #[test] -fn noise_scale_with_brush_picks_arm_at_compile_time() { +fn noise_dab_reconstructs_pixels_from_radius() { + // Dab space always reconstructs oriented dab-pixels from the dab radius, + // so `scale` is a canvas-pixel feature size (uniform with Canvas space). let reg = registry(); - for (swb, expect_norm) in [(true, true), (false, false)] { - let mut graph = Graph::::new(); - let pen = graph.add_node("pen_input", reg.get("pen_input").unwrap().ports.clone()); - let noise = graph.add_node("noise", reg.get("noise").unwrap().ports.clone()); - apply_noise_inputs(&mut graph, &noise, 1, swb); - let term = graph.add_node("paint", reg.get("paint").unwrap().ports.clone()); - wire( - &mut graph, - &[ - (pen.clone(), "position", term.clone(), "position"), - (noise.clone(), "color", term.clone(), "rgba"), - ], - ); - let plan = compile(&graph, reg.as_map()).unwrap(); - let compiled = compile_brush_to_wgsl(&graph, &plan, &evals()).expect("compiles"); - let w = &compiled.stroke_wgsl; - if expect_norm { - assert!( - w.contains("dab_local / (32.000000)"), - "scale_with_brush=true divides the unit-disc offset" - ); - // The skeleton always defines `local_uv = local * d.inv_radius_target_px`, - // so guard against the *reconstruction* specifically, not the symbol. - assert!( - !w.contains("1.0 / d.inv_radius_target_px"), - "scale_with_brush=true must not reconstruct pixels" - ); - } else { - assert!( - w.contains("1.0 / d.inv_radius_target_px"), - "scale_with_brush=false must reconstruct dab-pixels from inv_radius", - ); - } - } + let mut graph = Graph::::new(); + let pen = graph.add_node("pen_input", reg.get("pen_input").unwrap().ports.clone()); + let noise = graph.add_node("noise", reg.get("noise").unwrap().ports.clone()); + apply_noise_inputs(&mut graph, &noise, 1); + let term = graph.add_node("paint", reg.get("paint").unwrap().ports.clone()); + wire( + &mut graph, + &[ + (pen.clone(), "position", term.clone(), "position"), + (noise.clone(), "color", term.clone(), "rgba"), + ], + ); + let plan = compile(&graph, reg.as_map()).unwrap(); + let compiled = compile_brush_to_wgsl(&graph, &plan, &evals()).expect("compiles"); + assert!( + compiled + .stroke_wgsl + .contains("1.0 / d.inv_radius_target_px"), + "Dab space must reconstruct dab-pixels from inv_radius", + ); } #[test] @@ -1577,7 +1561,7 @@ fn noise_rotation_input_wires_per_dab() { let mut graph = Graph::::new(); let pen = graph.add_node("pen_input", reg.get("pen_input").unwrap().ports.clone()); let noise = graph.add_node("noise", reg.get("noise").unwrap().ports.clone()); - apply_noise_inputs(&mut graph, &noise, 1, true); + apply_noise_inputs(&mut graph, &noise, 1); let term = graph.add_node("paint", reg.get("paint").unwrap().ports.clone()); wire( &mut graph, @@ -1615,7 +1599,6 @@ fn image_dab_tip_needs_no_shape_node() { ("texture_name", InputValue::String("paper".into())), ("scale", InputValue::Scalar(512.0)), ("space", InputValue::Int(1)), - ("scale_with_brush", InputValue::Bool(true)), ] { graph.set_port_value(&image, name, v).unwrap(); } diff --git a/frontend/src/App.svelte b/frontend/src/App.svelte index 060d8129..d74b1a10 100644 --- a/frontend/src/App.svelte +++ b/frontend/src/App.svelte @@ -4,7 +4,7 @@ import LoadErrorToast from './ui/LoadErrorToast.svelte'; import PresetPicker from './ui/PresetPicker.svelte'; import SettingsModal from './ui/settings/SettingsModal.svelte'; - import ExportImageModal from './ui/ExportImageModal.svelte'; + import SaveModal from './ui/SaveModal.svelte'; import ExportTimelapseModal from './ui/ExportTimelapseModal.svelte'; import NewDocumentModal from './ui/NewDocumentModal.svelte'; import ResizeCanvasModal from './ui/ResizeCanvasModal.svelte'; @@ -60,7 +60,7 @@ - + diff --git a/frontend/src/actions/__tests__/delete_layer.test.ts b/frontend/src/actions/__tests__/delete_layer.test.ts new file mode 100644 index 00000000..980d816b --- /dev/null +++ b/frontend/src/actions/__tests__/delete_layer.test.ts @@ -0,0 +1,99 @@ +import { describe, it, expect, beforeAll, beforeEach, vi } from 'vitest'; +import { registerActions } from '../index'; +import { actions } from '../registry'; +import { DarklyInstance, setActiveInstance } from '../../state/app.svelte'; +import type { Engine } from '../../engine/protocol'; + +// End-to-end cover for the reported symptom: deleting a layer must leave an +// appropriate row selected rather than nothing. The reconciler unit tests in +// `state/__tests__/reselection.test.ts` pin the fallback rule itself; this pins +// that the delete action lets it run. +beforeAll(() => { + registerActions(); +}); + +/** `Registry.dispatch` fires the handler without returning its promise, so + * reach for the registration to await the async work. */ +function deleteLayer() { + return actions.get('deleteLayer')!.handler({}); +} + +function layer(id: number) { + return { type: 'raster', id, name: `l${id}`, visible: true, modifiers: [] }; +} + +let inst: DarklyInstance; +let tree: any[]; +let removeLayer: ReturnType; +let removeLayers: ReturnType; + +beforeEach(async () => { + vi.stubGlobal('requestAnimationFrame', vi.fn()); + tree = [layer(3), layer(2), layer(1)]; + removeLayer = vi.fn().mockResolvedValue(undefined); + removeLayers = vi.fn().mockResolvedValue(0); + inst = new DarklyInstance(); + inst.requestFrame = vi.fn(); + inst.engine = { + api: { + layerTree: () => Promise.resolve(tree), + removeLayer, + removeLayers, + setIsolatedNode: vi.fn().mockResolvedValue(null), + setGroupCollapsed: vi.fn(), + }, + } as unknown as Engine; + setActiveInstance(inst); + // Seed the pre-delete tree shape the fallback is computed against. + await inst.refreshLayerTree(); +}); + +describe('deleteLayer action', () => { + it('selects the row below instead of clearing the selection', async () => { + inst.selectLayer(2); + removeLayer.mockImplementation(async () => { + tree = tree.filter((n) => n.id !== 2); + }); + + await deleteLayer(); + + expect(removeLayer).toHaveBeenCalledWith({ id: 2 }); + expect(inst.activeLayerId).toBe(1); + expect([...inst.selectedLayerIds]).toEqual([1]); + }); + + it('selects the row above when the bottom layer is deleted', async () => { + inst.selectLayer(1); + removeLayer.mockImplementation(async () => { + tree = tree.filter((n) => n.id !== 1); + }); + + await deleteLayer(); + + expect(inst.activeLayerId).toBe(2); + }); + + it('reselects after a multi-layer delete', async () => { + inst.selectLayer(3); + inst.toggleLayer(2); + removeLayers.mockImplementation(async () => { + tree = tree.filter((n) => n.id !== 3 && n.id !== 2); + return 0; + }); + + await deleteLayer(); + + expect(removeLayers).toHaveBeenCalledWith({ ids: [3, 2] }); + expect(inst.activeLayerId).toBe(1); + expect([...inst.selectedLayerIds]).toEqual([1]); + }); + + it('leaves the selection alone when the engine refuses the delete', async () => { + inst.selectLayer(2); + removeLayer.mockRejectedValue(new Error('Layer is locked')); + + await deleteLayer(); + + expect(inst.activeLayerId).toBe(2); + }); +}); diff --git a/frontend/src/actions/__tests__/menu_actions.test.ts b/frontend/src/actions/__tests__/menu_actions.test.ts index 22679a05..bdf03e4c 100644 --- a/frontend/src/actions/__tests__/menu_actions.test.ts +++ b/frontend/src/actions/__tests__/menu_actions.test.ts @@ -125,15 +125,15 @@ describe('menu action registrations', () => { expect(actionEnablement(crop).reason).toBe('No active selection'); }); - it("save actions' enabled() follows canSave (disabled-with-reason here)", () => { - // The test environment has no File System Access API, so canSave is - // false; enabled() returns the disabled-reason string (not `true`). - const save = actions.get('saveDocument'); - const e = save?.enabled?.(); - expect(e).not.toBe(true); - expect(typeof e).toBe('string'); - expect(actionEnablement(save!)).toMatchObject({ enabled: false }); - expect(actionEnablement(save!).reason).toBeTruthy(); + it('leaves save actions always enabled (download fallback works everywhere)', () => { + // Save no longer gates on the File System Access API — browsers without + // it (Firefox/Safari) fall back to a download, so there's no `enabled` + // gate at all. + for (const id of ['saveDocument', 'saveDocumentAs']) { + const save = actions.get(id)!; + expect(save.enabled).toBeUndefined(); + expect(actionEnablement(save)).toMatchObject({ enabled: true }); + } }); it('orders items within a menu by the menuPath order suffix, not registration sequence', () => { @@ -146,7 +146,6 @@ describe('menu action registrations', () => { 'open', 'saveDocument', 'saveDocumentAs', - 'exportImage', 'exportTimelapse', ]); }); diff --git a/frontend/src/actions/clipboard.ts b/frontend/src/actions/clipboard.ts index 7545ad59..5518bd50 100644 --- a/frontend/src/actions/clipboard.ts +++ b/frontend/src/actions/clipboard.ts @@ -158,25 +158,30 @@ export function registerClipboardActions(): void { }); actions.register({ id: 'pasteInPlace', - displayName: 'Paste in Place', + displayName: 'Paste into Active Layer', category: 'edit', - description: 'Paste from the clipboard at its original position.', + description: 'Paste the clipboard into the active layer or mask at its original position.', icon: 'fa6-solid:clipboard', menuPath: ['Edit:60'], handler: async () => { const engine = app.engine; if (!engine || app.activeLayerId == null) return; + // Paste into the active target — a raster layer or, when a mask is + // the active edit target, the mask. Both flow through the same + // floating→commit path (`pasteInPlaceFloating` / `pasteInPlace`), + // which writes RGBA layers and R8 masks alike. const activateTransform = config.get('edit.activateTransformAfterPaste') !== false; if (activateTransform) { + // Float onto the target so it can be repositioned before commit. const ok = await engine.api.pasteInPlaceFloating({ id: app.activeLayerId }); if (ok) { enterTransformTool(); app.requestFrame(); } } else { - const { id: layerId } = await engine.api.pasteInPlace({ active_layer_id: app.activeLayerId }); - if (layerId >= 0) { - app.selectLayer(layerId); + // Commit into the target immediately at the source's position. + const { id } = await engine.api.pasteInPlace({ active_layer_id: app.activeLayerId }); + if (id >= 0) { await app.refreshLayerTree(); app.requestFrame(); } diff --git a/frontend/src/actions/index.ts b/frontend/src/actions/index.ts index 8ded97ac..9e50feb0 100644 --- a/frontend/src/actions/index.ts +++ b/frontend/src/actions/index.ts @@ -8,7 +8,6 @@ import { imageRescale } from '../state/imageRescale.svelte'; import { selectionModify } from '../state/selectionModify.svelte'; import { filterModal } from '../state/filterModal.svelte'; import type { FilterParam } from '../ui/filters/filterParams'; -import { exportImage } from '../state/exportImage.svelte'; import { exportTimelapse } from '../state/exportTimelapse.svelte'; import { loadError, parseLoadErrorMessage } from '../state/loadError.svelte'; import { toast } from '../state/toast.svelte'; @@ -24,19 +23,12 @@ import { detectKind, isImageKind, type FileKind } from '../storage/detectKind'; import { saveDocument } from '../storage/saveDocument'; import { fontLibrary } from '../state/font_library.svelte'; import { processRecording } from '../recording/recorder.svelte'; -import { canSave } from '../storage/fileHandle'; import { shell } from '../multi_tab/shell.svelte'; import { about } from '../state/about.svelte'; import { commandPalette } from '../state/commandPalette.svelte'; import { openCheatsheet } from '../ui/cheatsheet'; import { links, openExternal } from '../links'; -// Tooltip explaining why Save / Save As are disabled when the browser lacks -// the File System Access API (Firefox). Returned from each save action's -// `enabled()` as the disabled-reason string, surfaced as the menu row's `title`. -const NO_SAVE_TOOLTIP = - "Filesystem save isn't supported in this browser — try Chrome, Edge, or Safari."; - /** Walk the layer tree to find a node by id. The layer tree is the * JSON shape produced by `app.refreshLayerTree`, with `children` on * groups and `modifiers` on hosts. */ @@ -263,7 +255,14 @@ export function registerActions() { description: 'Undo the last action.', icon: 'fa6-solid:rotate-left', menuPath: ['Edit:10'], - handler: async () => { app.engine?.api.undo(); await app.syncCanvasRect(); await app.refreshLayerTree(); }, + // The layer-tree refresh goes first: it diffs the tree against the + // pre-undo shape to find what the operation restored, and any await in + // between could let an unrelated refresh consume that difference. + handler: async () => { + app.engine?.api.undo(); + await app.refreshLayerTree({ adoptAppeared: true }); + await app.syncCanvasRect(); + }, }); actions.register({ id: 'redo', @@ -272,7 +271,11 @@ export function registerActions() { description: 'Redo the last undone action.', icon: 'fa6-solid:rotate-right', menuPath: ['Edit:20'], - handler: async () => { app.engine?.api.redo(); await app.syncCanvasRect(); await app.refreshLayerTree(); }, + handler: async () => { + app.engine?.api.redo(); + await app.refreshLayerTree({ adoptAppeared: true }); + await app.syncCanvasRect(); + }, }); // -- Colors -- @@ -535,11 +538,11 @@ export function registerActions() { displayName: 'Save', category: 'file', description: - 'Save the current document as a `.darkly` file. ' + - 'Re-saves to the same file after the first Save As; otherwise prompts.', + 'Save the current document. Re-saves to the same `.darkly` file after ' + + 'the first Save As; otherwise opens the Save picker (`.darkly`, or ' + + 'PNG / JPEG / WebP to export the canvas).', icon: 'fa6-solid:floppy-disk', menuPath: ['File:30'], - enabled: () => canSave || NO_SAVE_TOOLTIP, handler: () => { if (!app.engine) return; void saveDocument({ forceAs: false }); @@ -549,10 +552,11 @@ export function registerActions() { id: 'saveDocumentAs', displayName: 'Save As', category: 'file', - description: 'Save the current document to a new `.darkly` file.', + description: + 'Save the current document to a new file — `.darkly`, or PNG / JPEG / ' + + 'WebP to export the canvas.', icon: 'fa6-solid:file-export', menuPath: ['File:40'], - enabled: () => canSave || NO_SAVE_TOOLTIP, handler: () => { if (!app.engine) return; void saveDocument({ forceAs: true }); @@ -585,18 +589,6 @@ export function registerActions() { void openFlow(); }, }); - actions.register({ - id: 'exportImage', - displayName: 'Export Image…', - category: 'file', - description: 'Export the canvas composite as PNG, JPEG, or WebP.', - icon: 'fa6-solid:image', - menuPath: ['File:50'], - handler: () => { - if (!app.engine) return; - exportImage.open = true; - }, - }); actions.register({ id: 'exportTimelapse', displayName: 'Export Timelapse…', @@ -810,7 +802,6 @@ export function registerActions() { for (const id of targets) app.stopStreamSource(id); if (targets.length === 1) { await engine.api.removeLayer({ id: targets[0] }); - app.clearSelection(); } else { const skipped = await engine.api.removeLayers({ ids: targets }); if (skipped > 0) { diff --git a/frontend/src/canvas/CanvasView.svelte b/frontend/src/canvas/CanvasView.svelte index f7dd3eb0..75e6d04f 100644 --- a/frontend/src/canvas/CanvasView.svelte +++ b/frontend/src/canvas/CanvasView.svelte @@ -65,7 +65,14 @@ mirror_h: inst.mirrorH, screen_w: w, screen_h: h, }); - inst.requestFrame(); + // Drive the frame synchronously in this same task. The + // `canvas.width/height` write, the enqueued `resize` + // (→ `surface.configure`), and the present must not be split + // across browser turns: Firefox's zero-copy WebGPU present + // stalls the GPU process if a present straddles a swapchain + // reconfigure. `renderNow` drains the FIFO (applying the resize) + // and presents before yielding. + inst.renderNow(); } }); } diff --git a/frontend/src/canvas/__tests__/resize_atomic.test.ts b/frontend/src/canvas/__tests__/resize_atomic.test.ts new file mode 100644 index 00000000..a9504e16 --- /dev/null +++ b/frontend/src/canvas/__tests__/resize_atomic.test.ts @@ -0,0 +1,96 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; + +// `app.svelte.ts` imports the compiled wasm bundle at module load; stub it so +// the module graph loads in the node test env without instantiating wasm. +vi.mock('../../../wasm/pkg/darkly_wasm', () => ({ + compute_view_matrices: () => new Float32Array(16), +})); + +import { DarklyInstance } from '../../state/app.svelte'; + +/** A minimal engine whose `render` returns `busy: true` so `runFrame` bails + * immediately after the render call. That isolates the property under test — + * "is the frame driven synchronously?" — from the heavy post-render pipeline + * (tool hooks, readback polls) which this test does not exercise. */ +function fakeEngine(rec: string[]) { + return { + render: vi.fn(() => { + rec.push('render'); + return { busy: true }; + }), + api: { + resize: vi.fn(() => rec.push('resize')), + setViewTransform: vi.fn(() => rec.push('setViewTransform')), + }, + } as unknown as DarklyInstance['engine']; +} + +describe('atomic canvas resize scheduling', () => { + let rafCbs: FrameRequestCallback[]; + let cancelled: number[]; + + beforeEach(() => { + rafCbs = []; + cancelled = []; + // Capture rAF callbacks WITHOUT firing them, so a deferred render is + // observable as "render has not been called yet". + vi.stubGlobal('requestAnimationFrame', (cb: FrameRequestCallback) => { + rafCbs.push(cb); + return rafCbs.length; // handle = 1-based index + }); + vi.stubGlobal('cancelAnimationFrame', (h: number) => { + cancelled.push(h); + }); + vi.stubGlobal('performance', { now: () => 0 }); + }); + + afterEach(() => vi.unstubAllGlobals()); + + it('renderNow drives the frame synchronously, in the same task', () => { + const rec: string[] = []; + const inst = new DarklyInstance(); + inst.engine = fakeEngine(rec); + + inst.renderNow(); + + // Rendered in THIS task — not deferred to a captured rAF. This is the + // atomicity the Firefox resize fix depends on: the present runs in the + // same JS task as the canvas resize, never straddling a browser turn. + expect(rec).toEqual(['render']); + expect(rafCbs).toHaveLength(0); + }); + + it('requestFrame defers to a rAF — the split the old resize path relied on', () => { + const rec: string[] = []; + const inst = new DarklyInstance(); + inst.engine = fakeEngine(rec); + + inst.requestFrame(); + + // Nothing rendered synchronously; it waits for the browser turn. Driving + // the resize present this way is exactly what let a present straddle the + // swapchain reconfigure and freeze Firefox. + expect(rec).toEqual([]); + expect(rafCbs).toHaveLength(1); + rafCbs[0](0); + expect(rec).toEqual(['render']); + }); + + it('renderNow cancels a pending rAF and clears the pending flag', () => { + const rec: string[] = []; + const inst = new DarklyInstance(); + inst.engine = fakeEngine(rec); + + inst.requestFrame(); // schedules an rAF (handle = 1) + expect(rafCbs).toHaveLength(1); + + inst.renderNow(); // must cancel the pending rAF and render now + expect(cancelled).toEqual([1]); + expect(rec).toEqual(['render']); + + // The pending flag must not be left stuck — a later requestFrame must + // still schedule (a stuck flag would silently freeze all rendering). + inst.requestFrame(); + expect(rafCbs).toHaveLength(2); + }); +}); diff --git a/frontend/src/config/__tests__/hotkey_label.test.ts b/frontend/src/config/__tests__/hotkey_label.test.ts new file mode 100644 index 00000000..f51bbea0 --- /dev/null +++ b/frontend/src/config/__tests__/hotkey_label.test.ts @@ -0,0 +1,67 @@ +import { describe, it, expect, vi, beforeAll } from 'vitest'; + +// The config store reads its three layers out of the wasm bundle; stub the +// whole surface so the module graph loads in the node test env, with a +// fixture table standing in for the resolved `user → overlay → defaults`. +const HOTKEYS: Record = { + // Multi-binding action: the YAML list arrives joined with `|`. + 'hotkeys.commandPalette': '$mod+Shift+KeyP|$mod+KeyF', + 'hotkeys.openSettings': '$mod+Comma', + 'hotkeys.isolateLayer': '', +}; +vi.mock('../../../wasm/pkg/darkly_wasm', () => ({ + config_get: (key: string) => HOTKEYS[key], + config_set: () => {}, + config_reset: () => {}, + config_reset_all: () => {}, + config_base_names: () => [], + config_base_value: (key: string) => HOTKEYS[key], + config_schema: () => '[]', + config_version: () => 1, +})); +vi.mock('../../storage', () => ({ + storage: { remove: vi.fn() }, + readJson: vi.fn(async () => null), + writeJson: vi.fn(async () => {}), +})); + +import { config, effectiveHotkeys, formatHotkey, hotkeyLabel, tooltipForAction } from '../store.svelte'; + +beforeAll(async () => { + vi.stubGlobal('navigator', { userAgent: 'Linux x86_64' }); + await config.init(); +}); + +describe('hotkey display for multi-binding actions', () => { + it('splits the `|`-joined config value into separate bindings', () => { + expect(effectiveHotkeys('commandPalette')).toEqual([ + '$mod+Shift+KeyP', + '$mod+KeyF', + ]); + expect(effectiveHotkeys('isolateLayer')).toEqual([]); + }); + + // Regression: menus, the command palette and tooltips used to hand the raw + // `hotkeys.` value to `formatHotkey`, which formats a single chord. The + // second binding leaked through verbatim and `Key…` substitution ran on the + // spliced part, rendering `Ctrl+Shift+P|$mod+F`. + it('shows only the first binding, fully substituted', () => { + expect(hotkeyLabel('commandPalette')).toBe('Ctrl+Shift+P'); + expect(hotkeyLabel('commandPalette')).not.toContain('|'); + expect(hotkeyLabel('commandPalette')).not.toContain('$mod'); + }); + + it('carries that through to action tooltips', () => { + expect(tooltipForAction('Find', 'commandPalette')).toBe('Find (Ctrl+Shift+P)'); + }); + + it('leaves single-binding actions alone, and omits unbound ones', () => { + expect(hotkeyLabel('openSettings')).toBe('Ctrl+,'); + expect(hotkeyLabel('isolateLayer')).toBeUndefined(); + expect(tooltipForAction('Isolate', 'isolateLayer')).toBe('Isolate'); + }); + + it('formats one chord at a time', () => { + expect(formatHotkey('layerPanel:$mod+Shift+KeyP')).toBe('Ctrl+Shift+P'); + }); +}); diff --git a/frontend/src/config/hotkeys.svelte.ts b/frontend/src/config/hotkeys.svelte.ts index ff563713..86e42872 100644 --- a/frontend/src/config/hotkeys.svelte.ts +++ b/frontend/src/config/hotkeys.svelte.ts @@ -1,5 +1,5 @@ import { tinykeys } from 'tinykeys'; -import { config } from './store.svelte'; +import { effectiveHotkeys } from './store.svelte'; import { actions } from '../actions/registry'; import { app } from '../state/app.svelte'; import { activeSiteChain } from '../actions/active_site'; @@ -14,37 +14,14 @@ import { } from '../actions/hotkey_resolve'; // Re-export the pure helpers so existing import paths (cheatsheet, settings -// widgets) keep resolving. The resolution logic itself lives in -// `actions/hotkey_resolve.ts` so it can be unit-tested without DOM. +// widgets) keep resolving. Chord resolution lives in +// `actions/hotkey_resolve.ts` so it can be unit-tested without DOM; the +// config reads live on the config store. export { parseBinding, type ChordEntry }; +export { effectiveHotkeys, effectiveHotkey } from './store.svelte'; let cleanup: (() => void) | null = null; -/** - * Resolve an action's effective keyboard trigger list. The full binding - * lives in `hotkeys.` under the three-layer config — defaults.yaml + - * overlay + user override. Multi-binding actions (e.g. `isolateLayer` from - * `layerThumb:alt+click` + `maskThumb:alt+click`) are joined with `|` in - * the YAML parser; we split them back into a list here. - * - * Empty string means "no keyboard trigger" — used by overlays that - * explicitly want to disable a binding the previous layer set - * (e.g. Photoshop sets `hotkeys.isolateLayer = ""`). - */ -export function effectiveHotkeys(actionId: string): string[] { - const v = config.get(`hotkeys.${actionId}`); - if (typeof v !== 'string') return []; - if (!v) return []; - return v.split('|').filter(Boolean); -} - -/** Single-string view for callers that show one binding per action - * (settings UI row, cheatsheet). Returns the first effective binding, - * or `""` if none. */ -export function effectiveHotkey(actionId: string): string { - return effectiveHotkeys(actionId)[0] ?? ''; -} - /** * Register all hotkeys from the action registry + Rust config. * diff --git a/frontend/src/config/store.svelte.ts b/frontend/src/config/store.svelte.ts index 6df55cc5..684fb928 100644 --- a/frontend/src/config/store.svelte.ts +++ b/frontend/src/config/store.svelte.ts @@ -241,6 +241,31 @@ class ConfigStore { export const config = new ConfigStore(); +/** + * Resolve an action's effective trigger list. The full binding lives in + * `hotkeys.` under the three-layer config — defaults.yaml + overlay + + * user override. Multi-binding actions (e.g. `commandPalette` from + * `$mod+Shift+KeyP` + `$mod+KeyF`) are joined with `|` by the YAML parser; + * we split them back into a list here. + * + * Empty string means "no trigger" — used by overlays that explicitly want to + * disable a binding the previous layer set (e.g. Photoshop sets + * `hotkeys.isolateLayer = ""`). + */ +export function effectiveHotkeys(actionId: string): string[] { + const v = config.get(`hotkeys.${actionId}`); + if (typeof v !== 'string') return []; + if (!v) return []; + return v.split('|').filter(Boolean); +} + +/** Single-binding view for callers that show one trigger per action + * (menus, command palette, cheatsheet, tooltips). Returns the first + * effective binding, or `""` if none. */ +export function effectiveHotkey(actionId: string): string { + return effectiveHotkeys(actionId)[0] ?? ''; +} + /** * Format a binding (`"Shift+KeyR"`, `"$mod+KeyA"`, `"$mod+click"`, …) into * a human-readable shortcut string (e.g. `"Shift+R"`, `"Ctrl+A"` / `"Cmd+A"`, @@ -251,6 +276,9 @@ export const config = new ConfigStore(); * Handles both the keyboard chord vocabulary (`Shift`/`Alt` capitalized, key * codes like `KeyA`/`Comma`) and the mouse chord vocabulary * (`shift`/`alt`/`ctrl`/`meta` lowercase, verbs like `click`/`drag`). + * + * Takes exactly ONE binding. Raw `hotkeys.` values may hold several joined + * with `|` — go through `hotkeyLabel` for those. */ export function formatHotkey(binding: string | undefined): string | undefined { if (!binding) return undefined; @@ -287,16 +315,22 @@ export function formatHotkey(binding: string | undefined): string | undefined { }).join('+'); } +/** + * An action's shortcut as shown to the user: its first effective binding, + * formatted. This is the single entry point for anything that displays a + * hotkey for an action id — menus, the command palette, tooltips. Reactive to + * the config, so displays re-render whenever the user rebinds or switches + * editor overlays. + */ +export function hotkeyLabel(actionId: string): string | undefined { + return formatHotkey(effectiveHotkey(actionId)); +} + /** * Build a tooltip combining a label with the action's effective hotkey, if - * any. The binding comes straight from the resolved config (no - * action-registry default fallback — defaults live in YAML now). - * Reactive to the config so the tooltip re-renders whenever the user - * rebinds or switches editor overlays. + * any. */ export function tooltipForAction(label: string, actionId: string): string { - const v = config.get(`hotkeys.${actionId}`); - if (typeof v !== 'string' || !v) return label; - const hk = formatHotkey(v.split('|')[0]); + const hk = hotkeyLabel(actionId); return hk ? `${label} (${hk})` : label; } diff --git a/frontend/src/engine/protocol_gen.ts b/frontend/src/engine/protocol_gen.ts index cce08c94..daec94b6 100644 --- a/frontend/src/engine/protocol_gen.ts +++ b/frontend/src/engine/protocol_gen.ts @@ -170,12 +170,6 @@ export type BrushLoadReq = { name: string, }; export type BrushNodePreviewReq = { node_id: string, }; -export type BrushWireType = "Scalar" | "Int" | "Bool" | "Vec2" | "Vec4" | "Enum" | "String" | "Curve"; - -export type InputValue = boolean | number | number | string | Array<[number, number]> | [number, number] | [number, number, number, number]; - -export type PortDir = "Input" | "Output"; - export type PortDef = { name: string, dir: PortDir, wire_type: BrushWireType, /** * Slider min when the port is disconnected (UI metadata only). @@ -343,7 +337,7 @@ persist_in_thumbnail: boolean, /** * This output port emits a *spatial, per-fragment image* — a coverage * mask or colour field that varies across the dab — so a node carrying it - * is worth a preview thumbnail (`shape.mask`, `image.color`, + * is worth a preview thumbnail (`circle.mask`, `image.color`, * `noise.color`, `stamp.dab`). Declared per port rather than inferred * from `wire_type`, because wire type can't tell a spatial field from a * per-dab constant: `random.value` and `paint_color.color` share the @@ -368,6 +362,12 @@ preview_image: boolean, */ source: boolean, }; +export type PortDir = "Input" | "Output"; + +export type BrushWireType = "Scalar" | "Int" | "Bool" | "Vec2" | "Vec4" | "Enum" | "String" | "Curve"; + +export type InputValue = boolean | number | number | string | Array<[number, number]> | [number, number] | [number, number, number, number]; + export type NodeRegistration = { /** * Unique identifier (e.g. "pen_input", "multiply"). @@ -720,7 +720,8 @@ export type StrokeToReq = { op: StrokeOp, }; export type StrokeOp = { "op": "flood_fill", x: number, y: number, r: number, g: number, b: number, a: number, tolerance: number, } | { "op": "linear_gradient", x0: number, y0: number, x1: number, y1: number, r0: number, g0: number, b0: number, a0: number, r1: number, g1: number, b1: number, a1: number, } | { "op": "brush_stroke", x: number, y: number, pressure: number, x_tilt: number, y_tilt: number, rotation: number, tangential_pressure: number, time_ms: number, /** - * Foreground color as linear RGBA floats (0-1). + * Foreground color as raw sRGB RGBA floats (0-1), as picked — the + * compositor is display-referred, so no gamma conversion is applied. */ cr: number, cg: number, cb: number, ca: number, }; @@ -952,6 +953,7 @@ export type RequestKind = | 'veil_types' | 'void_transform_info' | 'void_types' + | 'warm_vector_renderer' ; export const REQUEST_KINDS: readonly RequestKind[] = [ @@ -1145,6 +1147,7 @@ export const REQUEST_KINDS: readonly RequestKind[] = [ 'veil_types', 'void_transform_info', 'void_types', + 'warm_vector_renderer', ] as const; /** The request boundary the generated client closes over. `request` @@ -1346,6 +1349,7 @@ export interface EngineApi { veilTypes(): Promise>; voidTransformInfo(req: VoidTransformInfoReq): Promise; voidTypes(): Promise>; + warmVectorRenderer(): void; } /** Build the typed client over a transport (in-process today, Tauri later). */ @@ -1541,5 +1545,6 @@ export function makeApi(t: Transport): EngineApi { veilTypes: () => t.request('veil_types'), voidTransformInfo: (req) => t.request('void_transform_info', req), voidTypes: () => t.request('void_types'), + warmVectorRenderer: () => t.postFF('warm_vector_renderer'), }; } diff --git a/frontend/src/multi_tab/shell.svelte.ts b/frontend/src/multi_tab/shell.svelte.ts index 9cbba86f..04221c28 100644 --- a/frontend/src/multi_tab/shell.svelte.ts +++ b/frontend/src/multi_tab/shell.svelte.ts @@ -116,10 +116,13 @@ class MultiTabShell { if (idx === -1) return; const [removed] = this.instances.splice(idx, 1); - // Free the WASM handle: drops the Rust DarklyEngine, returning all - // its GPU textures to the shared device. No effect on sibling - // instances since the device is `Arc`-shared. - removed.engine?.free(); + // Tear the instance down: stops its tool session and stream sources, + // frees the WASM handle (dropping the Rust DarklyEngine and returning + // its GPU textures to the shared device), and nulls the engine so any + // frame still queued on the instance's render loop bails instead of + // rendering on a freed handle. No effect on sibling instances since the + // device is `Arc`-shared. + removed.dispose(); if (id in this.names) { const { [id]: _removed, ...rest } = this.names; this.names = rest; diff --git a/frontend/src/state/__tests__/close_use_after_free.test.ts b/frontend/src/state/__tests__/close_use_after_free.test.ts new file mode 100644 index 00000000..285b4847 --- /dev/null +++ b/frontend/src/state/__tests__/close_use_after_free.test.ts @@ -0,0 +1,66 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import type { Engine } from '../../engine/protocol'; +import { DarklyInstance } from '../app.svelte'; + +// Regression: closing a tab (or a failed `.darkly` open, which closes the +// just-opened tab) used to free the WASM handle while leaving `instance.engine` +// non-null. The self-rescheduling `requestFrame` rAF loop would then call +// `engine.render` on the freed handle, and the wasm-bindgen wrapper throws +// "Attempt to use a moved value" — an uncaught error surfacing as +// `DarklyHandle.render → Engine.render → app.svelte.ts`'s rAF callback. +// +// `dispose()` must null the engine reference so the loop's `if (!engine) return` +// guard short-circuits a frame that was already queued when the tab closed. +describe('closing a tab mid-frame does not render on a freed handle', () => { + let rafCb: FrameRequestCallback | null; + + beforeEach(() => { + rafCb = null; + // Capture the scheduled callback instead of running it, so the test + // decides exactly when the queued frame fires (after dispose). + vi.stubGlobal('requestAnimationFrame', (cb: FrameRequestCallback) => { + rafCb = cb; + return 1; + }); + }); + + it('a rAF queued before dispose() renders nothing and does not throw', () => { + // Model the wasm-bindgen "moved value" throw: once the handle is freed, + // any later `render` throws — exactly what the real `DarklyHandle` does. + let freed = false; + const render = vi.fn(() => { + if (freed) throw new Error('Attempt to use a moved value'); + return { busy: false, needsMore: false }; + }); + const free = vi.fn(() => { + freed = true; + }); + + const inst = new DarklyInstance(); + inst.engine = { + render, + free, + api: { + pollCopyResult: vi.fn(), + pollExportResult: vi.fn(), + pollSaveResult: vi.fn(), + }, + } as unknown as Engine; + + // A frame is in flight — the open-success path calls requestFrame(), and + // the loop self-reschedules, so one is essentially always queued. + inst.requestFrame(); + expect(rafCb).toBeInstanceOf(Function); + + // The load fails / the user closes the tab: the instance is disposed, + // which frees the handle. + inst.dispose(); + expect(inst.engine).toBeNull(); + + // The already-queued rAF now fires. It must bail on the null-engine + // guard rather than render on the freed handle. + expect(() => rafCb!(16)).not.toThrow(); + expect(render).not.toHaveBeenCalled(); + expect(free).toHaveBeenCalledTimes(1); + }); +}); diff --git a/frontend/src/state/__tests__/layerTree.test.ts b/frontend/src/state/__tests__/layerTree.test.ts new file mode 100644 index 00000000..3e1567ff --- /dev/null +++ b/frontend/src/state/__tests__/layerTree.test.ts @@ -0,0 +1,190 @@ +import { describe, it, expect } from 'vitest'; +import { + appearedRoots, + collapsedAncestorsOf, + indexLayerTree, + nextActiveAfterRemoval, +} from '../layerTree'; + +function layer(id: number, extra: Record = {}) { + return { type: 'raster', id, name: `l${id}`, visible: true, modifiers: [], ...extra }; +} +function group(id: number, children: unknown[], extra: Record = {}) { + return { + type: 'group', id, name: `g${id}`, visible: true, collapsed: false, + children, modifiers: [], ...extra, + }; +} +function mask(id: number) { + return { type: 'mask', id, name: `m${id}` }; +} + +/** Sibling list order matches the panel: index 0 is the top of the stack. */ +const flat = [layer(3), layer(2), layer(1)]; + +describe('indexLayerTree', () => { + it('collects every selectable id at any depth, modifiers included', () => { + const index = indexLayerTree([ + group(100, [layer(1, { modifiers: [mask(900)] })]), + layer(2), + ]); + expect([...index.ids].sort((a, b) => a - b)).toEqual([1, 2, 100, 900]); + }); + + it('orders rows top to bottom with each host followed by its modifiers', () => { + const index = indexLayerTree([ + layer(3, { modifiers: [mask(900)] }), + group(100, [layer(1), layer(2)]), + ]); + expect(index.order).toEqual([3, 900, 100, 1, 2]); + }); + + it('descends into collapsed groups for `order` but not `visibleOrder`', () => { + const tree = [group(100, [layer(1), layer(2)], { collapsed: true }), layer(3)]; + const index = indexLayerTree(tree); + expect(index.order).toEqual([100, 1, 2, 3]); + expect(index.visibleOrder).toEqual([100, 3]); + expect([...index.collapsed]).toEqual([100]); + }); + + it('hides rows nested under any collapsed ancestor', () => { + const index = indexLayerTree([ + group(100, [group(200, [layer(1)])], { collapsed: true }), + ]); + expect(index.order).toEqual([100, 200, 1]); + expect(index.visibleOrder).toEqual([100]); + }); + + it('records a modifier’s parent as its host and its siblings as the host’s modifiers', () => { + const index = indexLayerTree([layer(1, { modifiers: [mask(900), mask(901)] })]); + expect(index.slots.get(900)).toEqual({ parent: 1, siblings: [900, 901] }); + expect(index.slots.get(1)?.parent).toBeNull(); + }); + + it('tolerates a malformed tree', () => { + const index = indexLayerTree([null, undefined, { name: 'no id' }, layer(1)] as any); + expect([...index.ids]).toEqual([1]); + expect(indexLayerTree(undefined as any).ids.size).toBe(0); + }); +}); + +describe('nextActiveAfterRemoval', () => { + const prev = indexLayerTree(flat); + + it('prefers the sibling below', () => { + expect(nextActiveAfterRemoval(prev, new Set([3, 1]), 2)).toBe(1); + }); + + it('falls back to the sibling above for the bottom-most row', () => { + expect(nextActiveAfterRemoval(prev, new Set([3, 2]), 1)).toBe(2); + }); + + it('skips dead siblings on the way down, then on the way up', () => { + const wide = indexLayerTree([layer(5), layer(4), layer(3), layer(2), layer(1)]); + expect(nextActiveAfterRemoval(wide, new Set([5, 1]), 3)).toBe(1); + expect(nextActiveAfterRemoval(wide, new Set([5, 4]), 3)).toBe(4); + }); + + it('adopts the parent group when no sibling survives', () => { + const nested = indexLayerTree([group(100, [layer(1)]), layer(2)]); + expect(nextActiveAfterRemoval(nested, new Set([100, 2]), 1)).toBe(100); + }); + + it('stays inside the group when a sibling survives', () => { + const nested = indexLayerTree([group(100, [layer(1), layer(2)]), layer(3)]); + expect(nextActiveAfterRemoval(nested, new Set([100, 2, 3]), 1)).toBe(2); + }); + + it('escalates to the parent’s own level when the parent died too', () => { + const nested = indexLayerTree([group(100, [layer(1)]), layer(2)]); + expect(nextActiveAfterRemoval(nested, new Set([2]), 1)).toBe(2); + }); + + it('escalates through two dead levels', () => { + const deep = indexLayerTree([ + group(100, [group(200, [layer(1)])]), + layer(2), + ]); + expect(nextActiveAfterRemoval(deep, new Set([2]), 1)).toBe(2); + }); + + it('adopts a sibling modifier, then the host', () => { + const withMods = indexLayerTree([layer(1, { modifiers: [mask(900), mask(901)] })]); + expect(nextActiveAfterRemoval(withMods, new Set([1, 901]), 900)).toBe(901); + expect(nextActiveAfterRemoval(withMods, new Set([1]), 900)).toBe(1); + }); + + it('gives up when nothing survives at any level', () => { + expect(nextActiveAfterRemoval(prev, new Set(), 2)).toBeNull(); + }); + + it('gives up for an id the previous tree never had', () => { + expect(nextActiveAfterRemoval(prev, new Set([3, 2, 1]), 999)).toBeNull(); + }); +}); + +describe('collapsedAncestorsOf', () => { + it('returns nothing for a visible row', () => { + const index = indexLayerTree([group(100, [layer(1)]), layer(2)]); + expect(collapsedAncestorsOf(index, 1)).toEqual([]); + expect(collapsedAncestorsOf(index, 2)).toEqual([]); + }); + + it('names the collapsed group hiding a row', () => { + const index = indexLayerTree([group(100, [layer(1)], { collapsed: true })]); + expect(collapsedAncestorsOf(index, 1)).toEqual([100]); + }); + + it('names every collapsed ancestor, outermost first', () => { + const index = indexLayerTree([ + group(100, [group(200, [layer(1)], { collapsed: true })], { collapsed: true }), + ]); + expect(collapsedAncestorsOf(index, 1)).toEqual([100, 200]); + }); + + it('skips expanded groups in the chain', () => { + const index = indexLayerTree([ + group(100, [group(200, [layer(1)])], { collapsed: true }), + ]); + expect(collapsedAncestorsOf(index, 1)).toEqual([100]); + }); + + it('returns nothing for an absent id', () => { + expect(collapsedAncestorsOf(indexLayerTree(flat), 999)).toEqual([]); + }); +}); + +describe('appearedRoots', () => { + it('finds a single new row', () => { + const prev = indexLayerTree([layer(3), layer(1)]); + const next = indexLayerTree(flat); + expect(appearedRoots(prev, next)).toEqual([2]); + }); + + it('finds several new rows in panel order, topmost first', () => { + const prev = indexLayerTree([layer(1)]); + const next = indexLayerTree(flat); + expect(appearedRoots(prev, next)).toEqual([3, 2]); + }); + + it('keeps only the subtree root when a whole group comes back', () => { + const prev = indexLayerTree([layer(9)]); + const next = indexLayerTree([ + group(100, [layer(1, { modifiers: [mask(900)] }), layer(2)]), + layer(9), + ]); + expect(appearedRoots(prev, next)).toEqual([100]); + }); + + it('keeps a restored modifier whose host survived', () => { + const prev = indexLayerTree([layer(1)]); + const next = indexLayerTree([layer(1, { modifiers: [mask(900)] })]); + expect(appearedRoots(prev, next)).toEqual([900]); + }); + + it('returns nothing when the tree only shrank or is unchanged', () => { + const prev = indexLayerTree(flat); + expect(appearedRoots(prev, indexLayerTree([layer(3), layer(1)]))).toEqual([]); + expect(appearedRoots(prev, indexLayerTree(flat))).toEqual([]); + }); +}); diff --git a/frontend/src/state/__tests__/reselection.test.ts b/frontend/src/state/__tests__/reselection.test.ts new file mode 100644 index 00000000..dca6978f --- /dev/null +++ b/frontend/src/state/__tests__/reselection.test.ts @@ -0,0 +1,367 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { DarklyInstance } from '../app.svelte'; +import type { Engine } from '../../engine/protocol'; + +// In the frontend tree, index 0 of any `children` array is the TOP of the +// stack, so a higher index is lower in the panel. "Sibling below" therefore +// means the next higher index. See docs/plans/layer-delete-reselection.md §3.1. +function layer(id: number, extra: Record = {}) { + return { type: 'raster', id, name: `l${id}`, visible: true, modifiers: [], ...extra }; +} +function group(id: number, children: unknown[], extra: Record = {}) { + return { + type: 'group', id, name: `g${id}`, visible: true, collapsed: false, + children, modifiers: [], ...extra, + }; +} + +/// A `DarklyInstance` whose engine serves `treeRef.current`, so a test can swap +/// the tree between refreshes the way a real mutation would. +function instanceServing(treeRef: { current: any[] }) { + const inst = new DarklyInstance(); + inst.requestFrame = vi.fn(); + const api = { + layerTree: () => Promise.resolve(treeRef.current), + setIsolatedNode: vi.fn().mockResolvedValue(null), + setGroupCollapsed: vi.fn(), + }; + inst.engine = { api } as unknown as Engine; + return { inst, api }; +} + +let treeRef: { current: any[] }; +let inst: DarklyInstance; +let api: { setIsolatedNode: ReturnType; setGroupCollapsed: ReturnType }; + +beforeEach(() => { + vi.stubGlobal('requestAnimationFrame', vi.fn()); + treeRef = { current: [] }; + ({ inst, api } = instanceServing(treeRef) as any); +}); + +/** Seed the pre-mutation index, then apply `mutate` and refresh again. */ +async function refreshThen(mutate: () => void) { + await inst.refreshLayerTree(); + mutate(); + await inst.refreshLayerTree(); +} + +describe('reselection after a removal', () => { + // R1 + it('adopts the sibling below when a middle layer disappears', async () => { + treeRef.current = [layer(3), layer(2), layer(1)]; + await inst.refreshLayerTree(); + inst.selectLayer(2); + + treeRef.current = [layer(3), layer(1)]; + await inst.refreshLayerTree(); + + expect(inst.activeLayerId).toBe(1); + expect([...inst.selectedLayerIds]).toEqual([1]); + }); + + // R2 + it('falls back to the sibling above when the bottom-most layer disappears', async () => { + treeRef.current = [layer(3), layer(2), layer(1)]; + await inst.refreshLayerTree(); + inst.selectLayer(1); + + treeRef.current = [layer(3), layer(2)]; + await inst.refreshLayerTree(); + + expect(inst.activeLayerId).toBe(2); + }); + + it('adopts the sibling below when the top layer disappears', async () => { + treeRef.current = [layer(3), layer(2), layer(1)]; + await inst.refreshLayerTree(); + inst.selectLayer(3); + + treeRef.current = [layer(2), layer(1)]; + await inst.refreshLayerTree(); + + expect(inst.activeLayerId).toBe(2); + }); + + // R3 + it('adopts the enclosing group when its only child disappears', async () => { + treeRef.current = [group(100, [layer(1)]), layer(2)]; + await inst.refreshLayerTree(); + inst.selectLayer(1); + + treeRef.current = [group(100, []), layer(2)]; + await inst.refreshLayerTree(); + + expect(inst.activeLayerId).toBe(100); + expect([...inst.selectedLayerIds]).toEqual([100]); + }); + + it('keeps the selection inside the group when a sibling remains', async () => { + treeRef.current = [group(100, [layer(1), layer(2)]), layer(3)]; + await inst.refreshLayerTree(); + inst.selectLayer(1); + + treeRef.current = [group(100, [layer(2)]), layer(3)]; + await inst.refreshLayerTree(); + + expect(inst.activeLayerId).toBe(2); + }); + + it('escalates to the group’s own level when the group dies too', async () => { + treeRef.current = [group(100, [layer(1)]), layer(2)]; + await inst.refreshLayerTree(); + inst.selectLayer(1); + + treeRef.current = [layer(2)]; + await inst.refreshLayerTree(); + + expect(inst.activeLayerId).toBe(2); + }); + + // R5 + it('reselects after a batch delete removes the whole selection', async () => { + treeRef.current = [layer(4), layer(3), layer(2), layer(1)]; + await inst.refreshLayerTree(); + inst.selectLayer(4); + inst.toggleLayer(3); + inst.toggleLayer(2); + + treeRef.current = [layer(1)]; + await inst.refreshLayerTree(); + + expect(inst.activeLayerId).toBe(1); + expect([...inst.selectedLayerIds]).toEqual([1]); + }); + + // R6 + it('adopts the host when its only modifier disappears', async () => { + const withMask = layer(1, { modifiers: [{ type: 'mask', id: 900, name: 'Mask' }] }); + treeRef.current = [withMask, layer(2)]; + await inst.refreshLayerTree(); + inst.selectLayer(900); + + treeRef.current = [layer(1), layer(2)]; + await inst.refreshLayerTree(); + + expect(inst.activeLayerId).toBe(1); + }); + + it('adopts a sibling modifier before falling back to the host', async () => { + const two = layer(1, { + modifiers: [ + { type: 'mask', id: 900, name: 'A' }, + { type: 'mask', id: 901, name: 'B' }, + ], + }); + treeRef.current = [two]; + await inst.refreshLayerTree(); + inst.selectLayer(900); + + treeRef.current = [layer(1, { modifiers: [{ type: 'mask', id: 901, name: 'B' }] })]; + await inst.refreshLayerTree(); + + expect(inst.activeLayerId).toBe(901); + }); + + it('leaves the selection alone when an unselected layer disappears', async () => { + treeRef.current = [layer(3), layer(2), layer(1)]; + await inst.refreshLayerTree(); + inst.selectLayer(1); + + treeRef.current = [layer(3), layer(1)]; + await inst.refreshLayerTree(); + + expect(inst.activeLayerId).toBe(1); + }); + + it('demotes to a surviving selection member before using the neighbour', async () => { + treeRef.current = [layer(3), layer(2), layer(1)]; + await inst.refreshLayerTree(); + inst.selectLayer(3); + inst.toggleLayer(1); + expect(inst.activeLayerId).toBe(1); + + treeRef.current = [layer(3), layer(2)]; + await inst.refreshLayerTree(); + + expect(inst.activeLayerId).toBe(3); + expect([...inst.selectedLayerIds]).toEqual([3]); + }); + + it('gives up when the whole tree is replaced', async () => { + treeRef.current = [layer(1)]; + await inst.refreshLayerTree(); + inst.selectLayer(1); + + treeRef.current = [layer(77)]; + await inst.refreshLayerTree(); + + expect(inst.activeLayerId).toBeNull(); + expect(inst.selectedLayerIds.size).toBe(0); + }); +}); + +describe('collapsed groups', () => { + // R4 + it('keeps a selection inside a group that gets collapsed', async () => { + treeRef.current = [group(100, [layer(1), layer(2)])]; + await inst.refreshLayerTree(); + inst.selectLayer(1); + + treeRef.current = [group(100, [layer(1), layer(2)], { collapsed: true })]; + await inst.refreshLayerTree(); + + expect(inst.activeLayerId).toBe(1); + expect([...inst.selectedLayerIds]).toEqual([1]); + }); + + // R10 + it('expands the enclosing group when the fallback lands on a hidden row', async () => { + treeRef.current = [group(100, [layer(1), layer(2)], { collapsed: true })]; + await inst.refreshLayerTree(); + inst.selectLayer(1); + + treeRef.current = [group(100, [layer(2)], { collapsed: true })]; + await inst.refreshLayerTree(); + + expect(inst.activeLayerId).toBe(2); + expect(api.setGroupCollapsed).toHaveBeenCalledWith({ id: 100, collapsed: false }); + }); + + it('does not expand anything when the new active row is already visible', async () => { + treeRef.current = [layer(2), layer(1)]; + await inst.refreshLayerTree(); + inst.selectLayer(2); + + treeRef.current = [layer(1)]; + await inst.refreshLayerTree(); + + expect(inst.activeLayerId).toBe(1); + expect(api.setGroupCollapsed).not.toHaveBeenCalled(); + }); +}); + +describe('isolation target', () => { + // R9 — the isolated node dies while a different, still-live layer is + // active, so the selection branch never runs and only the dedicated check + // can clear the stale target. + it('clears the isolation target when the isolated node leaves the tree', async () => { + treeRef.current = [layer(2), layer(1)]; + await inst.refreshLayerTree(); + inst.activeLayerId = 1; + inst.selectedLayerIds = new Set([1]); + inst.isolatedNodeId = 2; + + treeRef.current = [layer(1)]; + await inst.refreshLayerTree(); + + expect(inst.isolatedNodeId).toBeNull(); + expect(api.setIsolatedNode).toHaveBeenCalledWith({ id: null }); + expect(inst.activeLayerId).toBe(1); + }); + + it('keeps the isolation target when it survives the refresh', async () => { + treeRef.current = [layer(2), layer(1)]; + await inst.refreshLayerTree(); + inst.isolatedNodeId = 2; + + treeRef.current = [layer(2)]; + await inst.refreshLayerTree(); + + expect(inst.isolatedNodeId).toBe(2); + }); +}); + +describe('adopting restored rows (undo of a delete)', () => { + // A1 + it('selects the layer an undo brought back', async () => { + treeRef.current = [layer(3), layer(1)]; + await inst.refreshLayerTree(); + inst.selectLayer(1); + + treeRef.current = [layer(3), layer(2), layer(1)]; + await inst.refreshLayerTree({ adoptAppeared: true }); + + expect(inst.activeLayerId).toBe(2); + expect([...inst.selectedLayerIds]).toEqual([2]); + }); + + // A2 + it('restores the whole set an undone batch delete brought back', async () => { + treeRef.current = [layer(1)]; + await inst.refreshLayerTree(); + inst.selectLayer(1); + + treeRef.current = [layer(3), layer(2), layer(1)]; + await inst.refreshLayerTree({ adoptAppeared: true }); + + expect(inst.activeLayerId).toBe(3); + expect([...inst.selectedLayerIds].sort()).toEqual([2, 3]); + }); + + // A5 — only the topmost restored row, not its descendants + it('selects only the group when an undone delete restores a whole subtree', async () => { + treeRef.current = [layer(9)]; + await inst.refreshLayerTree(); + inst.selectLayer(9); + + treeRef.current = [ + group(100, [ + layer(1, { modifiers: [{ type: 'mask', id: 900, name: 'Mask' }] }), + layer(2), + ]), + layer(9), + ]; + await inst.refreshLayerTree({ adoptAppeared: true }); + + expect(inst.activeLayerId).toBe(100); + expect([...inst.selectedLayerIds]).toEqual([100]); + }); + + it('keeps a restored modifier on a surviving host', async () => { + treeRef.current = [layer(1), layer(2)]; + await inst.refreshLayerTree(); + inst.selectLayer(2); + + treeRef.current = [layer(1, { modifiers: [{ type: 'mask', id: 900, name: 'Mask' }] }), layer(2)]; + await inst.refreshLayerTree({ adoptAppeared: true }); + + expect(inst.activeLayerId).toBe(900); + }); + + // A3 — undo of an *add* removes a row, so the neighbour fallback applies + it('falls back to the neighbour when an undo removes rows instead', async () => { + treeRef.current = [layer(3), layer(2), layer(1)]; + await inst.refreshLayerTree(); + inst.selectLayer(2); + + treeRef.current = [layer(3), layer(1)]; + await inst.refreshLayerTree({ adoptAppeared: true }); + + expect(inst.activeLayerId).toBe(1); + }); + + // A4 — nothing changed, nothing to adopt + it('leaves the selection alone when the tree is unchanged', async () => { + treeRef.current = [layer(2), layer(1)]; + await inst.refreshLayerTree(); + inst.selectLayer(1); + + await inst.refreshLayerTree({ adoptAppeared: true }); + + expect(inst.activeLayerId).toBe(1); + }); + + // R15 — an ordinary refresh must never seize the selection + it('does not adopt appeared rows on an unflagged refresh', async () => { + treeRef.current = [layer(1)]; + await inst.refreshLayerTree(); + inst.selectLayer(1); + + treeRef.current = [layer(2), layer(1)]; + await inst.refreshLayerTree(); + + expect(inst.activeLayerId).toBe(1); + expect([...inst.selectedLayerIds]).toEqual([1]); + }); +}); diff --git a/frontend/src/state/app.svelte.ts b/frontend/src/state/app.svelte.ts index 43df9ff2..a34acf35 100644 --- a/frontend/src/state/app.svelte.ts +++ b/frontend/src/state/app.svelte.ts @@ -12,6 +12,13 @@ import { HttpStreamSource } from '../lib/httpStreamSource'; import type { FrameSource, CaptureKind } from '../lib/frameSource'; import { processRecording } from '../recording/recorder.svelte'; import { freshDocument } from './freshDocument'; +import { + appearedRoots, + collapsedAncestorsOf, + indexLayerTree, + nextActiveAfterRemoval, + type LayerTreeIndex, +} from './layerTree'; export interface Color { r: number; g: number; b: number; a: number; @@ -168,6 +175,23 @@ export class DarklyInstance { this.session = null; } + /** Tear this instance down when its tab closes: stop its tool session and + * stream sources, free the WASM handle, then drop the engine reference. The + * instance owns every consumer of its handle, so it owns their teardown — + * the shell just removes it from the strip. Nulling `engine` is what makes + * the render loop's `if (!engine) return` guard short-circuit an + * already-queued rAF; without it, that frame would call `render` on a freed + * handle and throw "Attempt to use a moved value". Order matters: stop the + * consumers first, then free, then null last so any synchronous `$state` + * reaction observes a fully torn-down instance. Idempotent. */ + dispose(): void { + this.killToolSession(); + for (const id of [...this.streamSources.keys()]) this.stopStreamSource(id); + this.engine?.free(); + this.engine = null; + this.engineState = null; + } + /** Monotonic counter the `CanvasView` transition effect watches to re-run a * same-tool activation (paste-into-active-transform: re-pick the floating * without deactivating). Bumped by {@link requestToolReactivation}. */ @@ -314,6 +338,12 @@ export class DarklyInstance { // `clearSelection` so the invariant with `activeLayerId` holds. selectedLayerIds = $state>(new Set()); + // The layer tree's shape as of the last `refreshLayerTree`. Plain, not + // `$state`: the reconciler consults the pre-mutation shape to work out which + // row replaced a deleted one, and reading `layerTree` there would tie the + // layer panel's `$effect` to the very state the refresh just wrote. + private treeIndex: LayerTreeIndex = indexLayerTree([]); + // Active veil. Mutually exclusive with activeLayerId — the right // sidebar's properties pane shows the props of whichever is non-null. activeVeilIndex = $state(null); @@ -428,11 +458,41 @@ export class DarklyInstance { if (this.isolatedNodeId !== null && id !== this.isolatedNodeId) { void this.setIsolatedNode(null); } + this.setSoleSelection(id); + } + + /** Make `id` the entire selection. The single mutator of the + * `activeLayerId` / `selectedLayerIds` pair; `selectLayer` layers the + * isolation-exit policy on top, which the reconciler deliberately does not + * want (it clears isolation only when the isolated node itself dies). */ + private setSoleSelection(id: number | null) { this.activeLayerId = id; this.selectedLayerIds = id === null ? new Set() : new Set([id]); this.activeVeilIndex = null; } + /** The layer-tree node with `id`, searched depth-first through the group + * hierarchy, or null if absent. The single home for "resolve a node by id" + * so consumers (tools, panels) never re-walk the tree themselves. */ + nodeById(id: number): any | null { + const walk = (nodes: any[]): any | null => { + for (const n of nodes) { + if (n.id === id) return n; + if (Array.isArray(n.children)) { + const found = walk(n.children); + if (found) return found; + } + } + return null; + }; + return walk(this.layerTree); + } + + /** The active layer's tree node, or null when nothing is selected. */ + get activeNode(): any | null { + return this.activeLayerId === null ? null : this.nodeById(this.activeLayerId); + } + /** The mask modifier id relevant to the active node, or null. Resolves * both cases: the active node is a host that owns a mask, and the * active node *is* a mask modifier — clicking the mask thumbnail makes @@ -488,7 +548,9 @@ export class DarklyInstance { this.selectLayer(id); return; } - const order = this.flattenedVisibleIds(); + // Visible order only: shift-click spans the rows the user can see, so + // it never reaches into a collapsed group. + const order = indexLayerTree(this.layerTree).visibleOrder; const anchorIdx = order.indexOf(this.activeLayerId); const targetIdx = order.indexOf(id); if (anchorIdx < 0 || targetIdx < 0) { @@ -536,82 +598,81 @@ export class DarklyInstance { else this.selectLayer(id); } - /** Walk the visible tree depth-first, returning every clickable node - * id in panel-top-to-panel-bottom order. Children of collapsed - * groups are skipped (the user can't see them, so shift-click - * shouldn't reach them). */ - private flattenedVisibleIds(): number[] { - const out: number[] = []; - const walk = (nodes: any[]) => { - for (const n of nodes) { - if (n?.id === undefined) continue; - out.push(n.id); - if (n.type === 'group' && !n.collapsed && Array.isArray(n.children)) { - walk(n.children); - } - } - }; - walk(this.layerTree); - return out; - } - - /** Pick the first id in `set` that appears in panel order. Returns - * null when the set is empty or none of its ids are still in the - * tree. Caller is the active-layer demotion path for ctrl-click. */ + /** Pick the first id in `set` that appears in panel order, skipping rows + * inside collapsed groups — the caller is the ctrl-click demotion path, so + * the answer should be a row the user can see. Returns null when the set is + * empty or none of its ids are still visible. Indexed fresh from the live + * tree, since callers may have assigned `layerTree` without a refresh. */ private firstInTreeOrder(set: Set): number | null { if (set.size === 0) return null; - for (const id of this.flattenedVisibleIds()) { + for (const id of indexLayerTree(this.layerTree).visibleOrder) { if (set.has(id)) return id; } return null; } - /** Reconcile `selectedLayerIds` and `activeLayerId` against the latest - * layer tree. Ids that no longer exist (deleted, undone, replaced by a - * bake result, etc.) drop out; if the active id disappeared, demote - * to the next remaining selected id in panel order or null. Called - * from `refreshLayerTree` so batch-delete / undo / cross-tab swap - * fallout is handled in one place. + /** Reconcile session state that references layer ids against the latest + * layer tree: the selection and the isolation target. Ids that no longer + * exist (deleted, undone, replaced by a bake result, etc.) drop out, and + * when the active row itself disappeared, the row that took its place + * becomes active — so no removal path has to know about reselection. + * Called from `refreshLayerTree`, which every tree mutation funnels + * through, so delete / batch-delete / undo fallout is handled in one place. + * + * `adoptAppeared` asks for the opposite direction: rows that just came into + * existence become the selection. Only undo/redo pass it — an ordinary + * refresh must never seize the selection just because it noticed a row for + * the first time. * * Takes the tree as a parameter (rather than reading `this.layerTree`) * so this code path stays write-only on `layerTree` — reading it here * would tie the LayerPanel's `$effect` to the very state the method * just wrote, looping Svelte's update guard. Same pattern as * `reconcileStreamSources(next)`. */ - private pruneSelectionAgainstTree(tree: any[]) { - const alive = new Set(); - const visibleOrder: number[] = []; - const walk = (nodes: any[]) => { - for (const n of nodes) { - if (n?.id === undefined) continue; - alive.add(n.id); - visibleOrder.push(n.id); - if (Array.isArray(n.modifiers)) { - for (const m of n.modifiers) { - if (m?.id !== undefined) alive.add(m.id); - } - } - if (n.type === 'group' && !n.collapsed && Array.isArray(n.children)) { - walk(n.children); - } - } - }; - walk(tree); + private reconcileSelection(tree: any[], adoptAppeared = false) { + const prev = this.treeIndex; + const next = indexLayerTree(tree); + this.treeIndex = next; + + // The isolation target is session state pointing at a document node: if + // it leaves the tree it becomes unreachable from the compositor's root + // walk, every node tests as off-path, and the canvas goes blank until + // something happens to reset it. Checked independently of the selection + // below, because the isolated node can die while a different, still-live + // layer is active. Krita ends isolation the same way when its isolation + // root is removed (`KisImage::aboutToRemoveANode`). + if (this.isolatedNodeId !== null && !next.ids.has(this.isolatedNodeId)) { + void this.setIsolatedNode(null); + } + + const survivors = [...this.selectedLayerIds].filter((id) => next.ids.has(id)); + if (survivors.length !== this.selectedLayerIds.size) { + this.selectedLayerIds = new Set(survivors); + } - let mutated = false; - const nextSelected = new Set(); - for (const id of this.selectedLayerIds) { - if (alive.has(id)) nextSelected.add(id); - else mutated = true; + const appeared = adoptAppeared ? appearedRoots(prev, next) : []; + if (appeared.length > 0) { + this.selectedLayerIds = new Set(appeared); + this.activeLayerId = appeared[0]; + this.activeVeilIndex = null; + } else { + const active = this.activeLayerId; + if (active !== null && !next.ids.has(active)) { + // Prefer a surviving member of the selection, keeping the rest + // of it intact; otherwise adopt whatever replaced the dead row. + const demoted = next.order.find((id) => this.selectedLayerIds.has(id)); + if (demoted !== undefined) this.activeLayerId = demoted; + else this.setSoleSelection(nextActiveAfterRemoval(prev, next.ids, active)); + } } - if (mutated) this.selectedLayerIds = nextSelected; - if (this.activeLayerId !== null && !alive.has(this.activeLayerId)) { - let replacement: number | null = null; - for (const id of visibleOrder) { - if (nextSelected.has(id)) { replacement = id; break; } + // A row the panel doesn't draw reads to the user as "nothing selected", + // which is the complaint reselection exists to fix. Open whatever hides + // it, as GIMP's tree view does for every newly selected item. + if (this.activeLayerId !== null) { + for (const id of collapsedAncestorsOf(next, this.activeLayerId)) { + this.engine?.api.setGroupCollapsed({ id, collapsed: false }); } - this.activeLayerId = replacement; } } @@ -985,7 +1046,13 @@ export class DarklyInstance { return [r.origin_x, r.origin_y, r.width, r.height]; } - async refreshLayerTree(): Promise { + /** Re-read the layer tree and reconcile session state against it. + * + * `adoptAppeared` makes rows that just came into existence the selection — + * passed by undo/redo so undoing a delete lands on the layer it brought + * back, matching both GIMP (which selects the restored layer) and Krita + * (which restores the pre-delete selection set). */ + async refreshLayerTree(opts?: { adoptAppeared?: boolean }): Promise { if (!this.engine) return; const parsed = await this.engine.api.layerTree(); const next: any[] = Array.isArray(parsed) ? parsed : []; @@ -997,7 +1064,7 @@ export class DarklyInstance { // keeping it out of any enclosing effect's dependency set — otherwise // the write loops back through it. this.reconcileStreamSources(next); - this.pruneSelectionAgainstTree(next); + this.reconcileSelection(next, opts?.adoptAppeared ?? false); this.layerTree = next; // Schedule a render frame: callers invoke this after layer mutations // (undo/redo, add/remove, drag/drop, etc.), and the engine may have @@ -1052,6 +1119,10 @@ export class DarklyInstance { private _framePending = false; + /** Handle of the rAF scheduled by {@link requestFrame}, so {@link renderNow} + * can cancel a still-pending frame before rendering synchronously. */ + private _frameHandle = 0; + /** * Number of active UI interactions (panel drags, slider adjustments, * etc.) that should suppress continuous animation rendering. While @@ -1131,122 +1202,149 @@ export class DarklyInstance { requestFrame() { if (this._framePending) return; this._framePending = true; - requestAnimationFrame((ts) => { + this._frameHandle = requestAnimationFrame((ts) => { this._framePending = false; - const engine = this.engine; - if (!engine) return; - // Push the latest external frames (webcam / screenshare / Blender - // stream) into their void input textures BEFORE render — render - // reads from those textures during composite, so a later upload - // would lag by a frame. - // - // The frame count we pass to `tick` is the value the compositor's - // master counter *will* hold once render increments it (inside - // `update_animations`): one past the count the *previous* render - // returned. Anticipating the increment keeps JS-side divisor gates - // phase-locked with the Rust-side veil / overlay / void divisors - // that check the post-increment value — so a camera `divisor=N` - // fires on the same rAF as a veil `divisor=N`, not one off. (We - // can't read `frame_count` directly anymore — it would be a third - // competing engine borrow; render returns it on the state mirror.) - const nextFrameCount = (this.engineState?.frameCount ?? 0) + 1; - for (const src of this.streamSources.values()) { - src.tick(nextFrameCount); - } + this.runFrame(ts / 1000.0); + }); + } - // The ONE engine borrow per frame: drains the request FIFO (which - // resolves any pending `send`/`post` promises) then composites. A - // re-entrant render reached via the event pump returns `busy` — the - // outer render handles everything, so we bail without rescheduling. - const frame = engine.render(ts / 1000.0); - if (frame.busy) return; - - // Refresh the synchronously-readable engine-state mirror from - // render's returned snapshot — no per-frame query; it's a downhill - // projection of the borrow render already held this frame. This one - // assignment updates everything the UI caches: frame/thumbnail - // counters (thumbnail `$derived`s re-run when `thumbnailVersion` - // changes) and document bools. - if (frame.state) this.engineState = frame.state; - - // Per-frame tool hook — async state sync (e.g. GPU readback - // completion). The instance's OWN tool runs against its OWN session, - // so a background tab's frame drives its own tool, never the focused - // one. Wrapped so a hook whose engine op was cancelled by a session - // change mid-await settles cleanly (see tool_session.ts). - void runHook(this.tool(this.activeToolId)?.onFrame?.()); - - // Drain completed process-recording captures to the encoder - // worker. No-op unless this tab's recorder is live. - processRecording.pollFrame(this); - - // Pointer singletons tick with the focused canvas's frame — there is - // one pointer, and these read/write the global `app` (the focused - // instance). A background tab's frame must not drive them. - if (getActiveInstance() === this) { - // Global color-pick poll — drives both the color-picker tool and - // the modifier-held `sampleColor` chord. Runs regardless of - // active tool so a Ctrl-drag started in (e.g.) the brush tool - // completes. - pollPick(); - // Refresh the color-picker cursor against the latest foreground - // committed by `pollPick`. Cheap when nothing changed. - tickColorPickerCursor(); - // Refresh the clone set-source cursor — re-queries "needs source" - // on brush change and shows/hides the crosshair. Cheap when - // nothing changed (memo guards). - tickCloneSourceCursor(); - } + /** Drive one full frame synchronously in the current task — the synchronous + * counterpart to {@link requestFrame}. The canvas-resize path needs this: + * Firefox's zero-copy WebGPU present stalls the GPU process when a present + * straddles a swapchain reconfigure, so the just-enqueued `resize` + * (→ `surface.configure`) and the present must run in the same JS task as + * the `canvas.width`/`canvas.height` write, with no browser turn between. */ + renderNow(ts: number = performance.now() / 1000) { + // A normal rAF may already be queued; running now makes it redundant. + // Cancel it so we don't render twice and — critically — so `_framePending` + // is not left set, which would make the next `requestFrame` a silent no-op. + if (this._framePending) { + cancelAnimationFrame(this._frameHandle); + this._framePending = false; + } + this.runFrame(ts); + } - // Check for completed async copy/cut readback. - if (this._copyCallback) { - engine.api.pollCopyResult().then((result) => { - if (result && this._copyCallback) { - const cb = this._copyCallback; - this._copyCallback = null; - cb(result); - } - }); - } + /** One frame's work: drain the request FIFO + composite (the single engine + * borrow), refresh the state mirror, run per-frame tool/pointer hooks, poll + * async readbacks, and self-schedule the animation loop. Shared by the + * deferred ({@link requestFrame}) and synchronous ({@link renderNow}) entry + * points; `ts` is in seconds. Must not touch `_framePending` — the rAF path + * owns clearing it at the seam it scheduled. */ + private runFrame(ts: number) { + const engine = this.engine; + if (!engine) return; + // Push the latest external frames (webcam / screenshare / Blender + // stream) into their void input textures BEFORE render — render + // reads from those textures during composite, so a later upload + // would lag by a frame. + // + // The frame count we pass to `tick` is the value the compositor's + // master counter *will* hold once render increments it (inside + // `update_animations`): one past the count the *previous* render + // returned. Anticipating the increment keeps JS-side divisor gates + // phase-locked with the Rust-side veil / overlay / void divisors + // that check the post-increment value — so a camera `divisor=N` + // fires on the same rAF as a veil `divisor=N`, not one off. (We + // can't read `frame_count` directly anymore — it would be a third + // competing engine borrow; render returns it on the state mirror.) + const nextFrameCount = (this.engineState?.frameCount ?? 0) + 1; + for (const src of this.streamSources.values()) { + src.tick(nextFrameCount); + } - // Check for completed async export readback. - if (this._exportCallback) { - engine - .api.pollExportResult() - .then((result) => { - if (result && this._exportCallback) { - const cb = this._exportCallback; - this._exportCallback = null; - cb({ width: result.width, height: result.height, rgba: result.bytes }); - } - }); - } + // The ONE engine borrow per frame: drains the request FIFO (which + // resolves any pending `send`/`post` promises) then composites. A + // re-entrant render reached via the event pump returns `busy` — the + // outer render handles everything, so we bail without rescheduling. + const frame = engine.render(ts); + if (frame.busy) return; + + // Refresh the synchronously-readable engine-state mirror from + // render's returned snapshot — no per-frame query; it's a downhill + // projection of the borrow render already held this frame. This one + // assignment updates everything the UI caches: frame/thumbnail + // counters (thumbnail `$derived`s re-run when `thumbnailVersion` + // changes) and document bools. + if (frame.state) this.engineState = frame.state; + + // Per-frame tool hook — async state sync (e.g. GPU readback + // completion). The instance's OWN tool runs against its OWN session, + // so a background tab's frame drives its own tool, never the focused + // one. Wrapped so a hook whose engine op was cancelled by a session + // change mid-await settles cleanly (see tool_session.ts). + void runHook(this.tool(this.activeToolId)?.onFrame?.()); + + // Drain completed process-recording captures to the encoder + // worker. No-op unless this tab's recorder is live. + processRecording.pollFrame(this); + + // Pointer singletons tick with the focused canvas's frame — there is + // one pointer, and these read/write the global `app` (the focused + // instance). A background tab's frame must not drive them. + if (getActiveInstance() === this) { + // Global color-pick poll — drives both the color-picker tool and + // the modifier-held `sampleColor` chord. Runs regardless of + // active tool so a Ctrl-drag started in (e.g.) the brush tool + // completes. + pollPick(); + // Refresh the color-picker cursor against the latest foreground + // committed by `pollPick`. Cheap when nothing changed. + tickColorPickerCursor(); + // Refresh the clone set-source cursor — re-queries "needs source" + // on brush change and shows/hides the crosshair. Cheap when + // nothing changed (memo guards). + tickCloneSourceCursor(); + } - // Check for completed async `.darkly` save readbacks. The bundle's - // byte blobs arrive concatenated in `bytes`; slice them back out - // into the per-blob shape `saveDocument.ts` expects. - if (this._saveCallback) { - engine.api.pollSaveResult().then((packed) => { - if (!packed || !this._saveCallback) return; - const cb = this._saveCallback; - this._saveCallback = null; - cb(unpackSaveBundle(packed)); + // Check for completed async copy/cut readback. + if (this._copyCallback) { + engine.api.pollCopyResult().then((result) => { + if (result && this._copyCallback) { + const cb = this._copyCallback; + this._copyCallback = null; + cb(result); + } + }); + } + + // Check for completed async export readback. + if (this._exportCallback) { + engine + .api.pollExportResult() + .then((result) => { + if (result && this._exportCallback) { + const cb = this._exportCallback; + this._exportCallback = null; + cb({ width: result.width, height: result.height, rgba: result.bytes }); + } }); - } + } - // Continue animation loop only when no UI interaction is - // monopolizing the main thread. One-shot renders (tool - // actions, resize, etc.) always go through — only the - // self-scheduling continuous loop is suppressed. - const shouldContinue = - frame.needsMore || - this._copyCallback || - this._exportCallback || - this._saveCallback; - if (shouldContinue && this._interactionCount === 0) { - this.requestFrame(); - } - }); + // Check for completed async `.darkly` save readbacks. The bundle's + // byte blobs arrive concatenated in `bytes`; slice them back out + // into the per-blob shape `saveDocument.ts` expects. + if (this._saveCallback) { + engine.api.pollSaveResult().then((packed) => { + if (!packed || !this._saveCallback) return; + const cb = this._saveCallback; + this._saveCallback = null; + cb(unpackSaveBundle(packed)); + }); + } + + // Continue animation loop only when no UI interaction is + // monopolizing the main thread. One-shot renders (tool + // actions, resize, etc.) always go through — only the + // self-scheduling continuous loop is suppressed. + const shouldContinue = + frame.needsMore || + this._copyCallback || + this._exportCallback || + this._saveCallback; + if (shouldContinue && this._interactionCount === 0) { + this.requestFrame(); + } } } diff --git a/frontend/src/state/brush_graph.svelte.ts b/frontend/src/state/brush_graph.svelte.ts index c3b2de24..3d899a49 100644 --- a/frontend/src/state/brush_graph.svelte.ts +++ b/frontend/src/state/brush_graph.svelte.ts @@ -13,6 +13,13 @@ import type { BrushInfo, JsonValue, ExposedValue, ExposedPortInfo } from '../eng export type { BrushInfo }; +/** Upper bound the editor's "extended range" toggle unlocks numeric sliders to, + * replacing the declared `0..1`. Purely a frontend affordance for entering + * large gains (e.g. a math node scaling a signal into the canvas-pixel domain); + * the engine never enforces slider ranges, so authored/wired values are already + * unbounded — this only relaxes the editor's own slider validation. */ +export const EXTENDED_RANGE_MAX = 1000; + // --- Types mirroring Rust's nodegraph structures --- /** The authored value on a disconnected input, mirroring Rust's @@ -139,6 +146,22 @@ export class BrushGraphState { * after every structural change; never sent to Rust. */ nodePositions = $state>({}); + /** UI-only set of node ids whose numeric-input sliders the user has + * unlocked to [`EXTENDED_RANGE_MAX`]. A pure editor affordance for + * entering large gains on math nodes — never sent to Rust, never + * persisted. The port *value* persists in the graph as normal; this only + * relaxes the editor's slider bound/validation for that node. */ + extendedRangeNodes = $state>(new Set()); + + /** Toggle the extended-range slider unlock for `nodeId`. Reassigns the set + * so Svelte re-runs dependent sliders. */ + toggleExtendedRange(nodeId: string) { + const next = new Set(this.extendedRangeNodes); + if (next.has(nodeId)) next.delete(nodeId); + else next.add(nodeId); + this.extendedRangeNodes = next; + } + /** Monotonic token identifying the current graph load. Bumped by * `beginLayoutGeneration` whenever the graph is replaced by a fresh * load/reset/import/tab-sync. Node positions and the one-shot layout diff --git a/frontend/src/state/exportImage.svelte.ts b/frontend/src/state/exportImage.svelte.ts deleted file mode 100644 index e5050b4b..00000000 --- a/frontend/src/state/exportImage.svelte.ts +++ /dev/null @@ -1,10 +0,0 @@ -/** - * Global toggle for the Export Image modal. The `export-image` action - * dispatches into this; the modal itself drives the readback + encode + - * download once the user confirms. - */ -class ExportImageState { - open = $state(false); -} - -export const exportImage = new ExportImageState(); diff --git a/frontend/src/state/layerTree.ts b/frontend/src/state/layerTree.ts new file mode 100644 index 00000000..217910fc --- /dev/null +++ b/frontend/src/state/layerTree.ts @@ -0,0 +1,174 @@ +/** + * Structural queries over a serialized layer tree, and the rule for picking a + * replacement row when the active one disappears. + * + * The reselection rule follows GIMP and Krita, which agree on it exactly: + * nearest surviving sibling **below** → nearest surviving sibling **above** → + * the **parent** → nothing, scoped to the dead row's own sibling list and + * computed against the tree as it was *before* the removal. + * + * - GIMP: `gimp_item_tree_remove_item`, `app/core/gimpitemtree.c` — captures the + * index before removal, re-reads that index in the shrunk container (= the + * sibling below), `CLAMP`s to the last child (= the sibling above, when the + * removed row was bottom-most), else selects the parent. + * - Krita: `LayerBox::slotAboutToRemoveRows`, + * `plugins/dockers/layerdocker/LayerBox.cpp` — row `end + 1`, else row + * `start - 1`, else an invalid index, which `KisNodeModel::setData` resolves + * to the captured parent. + * + * Making the reselected row visible also follows GIMP, whose tree view expands + * the parent of every newly selected item + * (`gimp_container_tree_view_selection_changed`, `app/widgets/gimpcontainertreeview.c`). + * + * Direction convention: index 0 of any `children` array is the **top** of the + * stack, so a higher index is lower in the panel and "sibling below" is the next + * higher index. See `docs/coordinate-systems.md` and the double `.rev()` in + * `crates/darkly/src/engine/veils.rs` + `engine/types.rs`. + */ + +/** + * A row's position: its parent and that parent's ordered child list. Modifiers + * fold in as children of their host, which is what makes the reselection rule + * uniform across layers, groups and masks — a mask's siblings are the host's + * other modifiers, and its parent is the host. + */ +interface Slot { + parent: number | null; + siblings: number[]; +} + +export interface LayerTreeIndex { + /** Every selectable id — nodes at any depth plus their modifiers. */ + ids: Set; + /** + * Panel order, top to bottom, each host immediately followed by its + * modifiers. Descends into collapsed groups: a row the user can't currently + * see is still a live, selectable node. + */ + order: number[]; + /** `order` minus everything inside a collapsed group. */ + visibleOrder: number[]; + /** Ids of groups whose children are hidden. */ + collapsed: Set; + slots: Map; +} + +/** + * The single walk over a layer tree. Every structural question — liveness, + * panel order, visibility, parentage — is answered from the one traversal, so + * callers never hand-roll another. + */ +export function indexLayerTree(tree: any[]): LayerTreeIndex { + const ids = new Set(); + const order: number[] = []; + const visibleOrder: number[] = []; + const collapsed = new Set(); + const slots = new Map(); + + const walk = (nodes: any[], parent: number | null, visible: boolean) => { + const siblings = nodes.filter((n) => n?.id !== undefined).map((n) => n.id as number); + for (const n of nodes) { + if (n?.id === undefined) continue; + const id: number = n.id; + ids.add(id); + order.push(id); + if (visible) visibleOrder.push(id); + slots.set(id, { parent, siblings }); + + if (Array.isArray(n.modifiers) && n.modifiers.length > 0) { + const mods = n.modifiers + .filter((m: any) => m?.id !== undefined) + .map((m: any) => m.id as number); + for (const m of n.modifiers) { + if (m?.id === undefined) continue; + ids.add(m.id); + order.push(m.id); + if (visible) visibleOrder.push(m.id); + slots.set(m.id, { parent: id, siblings: mods }); + } + } + + if (n.type === 'group') { + if (n.collapsed) collapsed.add(id); + if (Array.isArray(n.children)) { + walk(n.children, id, visible && !n.collapsed); + } + } + } + }; + walk(Array.isArray(tree) ? tree : [], null, true); + + return { ids, order, visibleOrder, collapsed, slots }; +} + +/** + * The row that takes `deadId`'s place: nearest surviving sibling below, else + * nearest surviving sibling above, else the parent — the enclosing group for a + * node, the host for a modifier — escalating to the parent's own sibling level + * when the parent died in the same batch. `null` when nothing qualifies. + * + * `prev` describes the tree as it was before the removal; `alive` is the set of + * ids that remain. + */ +export function nextActiveAfterRemoval( + prev: LayerTreeIndex, + alive: Set, + deadId: number, +): number | null { + let id = deadId; + const seen = new Set(); + for (;;) { + if (seen.has(id)) return null; + seen.add(id); + + const slot = prev.slots.get(id); + if (!slot) return null; + const i = slot.siblings.indexOf(id); + for (let k = i + 1; k < slot.siblings.length; k++) { + if (alive.has(slot.siblings[k])) return slot.siblings[k]; + } + for (let k = i - 1; k >= 0; k--) { + if (alive.has(slot.siblings[k])) return slot.siblings[k]; + } + if (slot.parent === null) return null; + if (alive.has(slot.parent)) return slot.parent; + id = slot.parent; + } +} + +/** + * The collapsed groups between `id` and the root, outermost first — the set that + * must be expanded for `id` to be a row the user can see. Empty when `id` is + * already visible, absent, or hidden by nothing. + */ +export function collapsedAncestorsOf(index: LayerTreeIndex, id: number): number[] { + if (!index.ids.has(id)) return []; + const out: number[] = []; + let cursor = index.slots.get(id)?.parent ?? null; + const seen = new Set(); + while (cursor !== null && !seen.has(cursor)) { + seen.add(cursor); + if (index.collapsed.has(cursor)) out.push(cursor); + cursor = index.slots.get(cursor)?.parent ?? null; + } + return out.reverse(); +} + +/** + * Ids present in `next` but not in `prev`, keeping only the **topmost** of each + * restored subtree. Undo of a layer removal reattaches the subtree root, but the + * tree re-serializes every descendant and modifier under it, so the raw + * difference would select a group *and* everything inside it — a selection the + * rest of the codebase treats as malformed (batch ops drop any id whose ancestor + * is also selected). Both reference editors produce single-scope selections + * here: GIMP a single item, Krita the source layers without their children. + */ +export function appearedRoots(prev: LayerTreeIndex, next: LayerTreeIndex): number[] { + const fresh = next.order.filter((id) => !prev.ids.has(id)); + if (fresh.length <= 1) return fresh; + const freshSet = new Set(fresh); + return fresh.filter((id) => { + const parent = next.slots.get(id)?.parent ?? null; + return parent === null || !freshSet.has(parent); + }); +} diff --git a/frontend/src/state/saveModal.svelte.ts b/frontend/src/state/saveModal.svelte.ts new file mode 100644 index 00000000..10006fea --- /dev/null +++ b/frontend/src/state/saveModal.svelte.ts @@ -0,0 +1,36 @@ +/** + * Fallback Save dialog state, used only in browsers without the File System + * Access API (Firefox, Safari) where there is no native save picker. + * + * The unified save flow (`saveDocument`) awaits `request()`, which resolves + * once the user confirms or cancels — so `closeGuard.save()` can await the + * whole save before deciding whether to close the tab. The modal itself drives + * produce + download via the shared `saveViaDownload`. + */ +class SaveModalState { + open = $state(false); + /** Filename (no extension) to prefill, seeded from the document name. */ + suggestedName = $state(''); + private resolver: (() => void) | null = null; + + /** Open the modal for `suggestedName` and resolve when the user confirms + * or cancels (or dismisses via Escape / backdrop). */ + request(suggestedName: string): Promise { + this.suggestedName = suggestedName; + this.open = true; + return new Promise((resolve) => { + this.resolver = resolve; + }); + } + + /** Close the modal and resolve the pending `request()`. Idempotent, so it's + * safe to call from both the buttons and the dismiss-on-close guard. */ + finish(): void { + this.open = false; + const resolve = this.resolver; + this.resolver = null; + resolve?.(); + } +} + +export const saveModal = new SaveModalState(); diff --git a/frontend/src/storage/__tests__/save_flow.test.ts b/frontend/src/storage/__tests__/save_flow.test.ts new file mode 100644 index 00000000..5d7d03c4 --- /dev/null +++ b/frontend/src/storage/__tests__/save_flow.test.ts @@ -0,0 +1,170 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; + +// jsdom isn't available in this project, so we stub the few browser globals the +// save flow touches (`showSaveFilePicker`) and mock the modules that reach for +// the GPU / OPFS (`exportComposite`, `downloadBlob`, recovery). That's enough +// to pin the behaviour that matters — including the regression that Firefox +// (no File System Access API) can now save at all. + +vi.mock('../exportComposite', () => ({ + exportComposite: vi.fn(), + rgbaToBlob: vi.fn(), +})); +vi.mock('../recovery', () => ({ + removeSnapshot: vi.fn(async () => {}), +})); +vi.mock('../index', async (importOriginal) => ({ + ...(await importOriginal()), + downloadBlob: vi.fn(), +})); + +import { + formatFromName, + saveViaDownload, + saveDocument, + type Format, +} from '../saveDocument'; +import { pickFileHandle, writeToHandle, hasFilePicker, type SaveAccept } from '../fileHandle'; +import { exportComposite } from '../exportComposite'; +import { downloadBlob } from '../index'; +import { removeSnapshot } from '../recovery'; +import { saveModal } from '../../state/saveModal.svelte'; +import { setActiveInstance } from '../../state/app.svelte'; + +afterEach(() => { + vi.clearAllMocks(); + vi.unstubAllGlobals(); + setActiveInstance(null); +}); + +describe('formatFromName', () => { + it('maps known extensions to formats, unknown/missing → darkly', () => { + expect(formatFromName('a.darkly')).toBe('darkly'); + expect(formatFromName('a.png')).toBe('png'); + expect(formatFromName('photo.JPG')).toBe('jpeg'); + expect(formatFromName('photo.jpeg')).toBe('jpeg'); + expect(formatFromName('a.webp')).toBe('webp'); + // Unknown or extensionless names fall back to a true `.darkly` save. + expect(formatFromName('a.weird')).toBe('darkly'); + expect(formatFromName('noext')).toBe('darkly'); + }); +}); + +describe('pickFileHandle', () => { + const accept: SaveAccept = { + description: 'Darkly Document', + accept: { 'application/x-darkly': ['.darkly'] }, + }; + + it('passes suggestedName / types / id to showSaveFilePicker and returns the handle', async () => { + const handle = { name: 'doc.darkly' }; + const picker = vi.fn(async () => handle); + vi.stubGlobal('showSaveFilePicker', picker); + + const result = await pickFileHandle('doc.darkly', [accept], 'darkly-file'); + + expect(result).toBe(handle); + expect(picker).toHaveBeenCalledWith({ + suggestedName: 'doc.darkly', + types: [accept], + id: 'darkly-file', + }); + }); + + it('returns null when the user cancels (AbortError)', async () => { + vi.stubGlobal( + 'showSaveFilePicker', + vi.fn(async () => { + throw Object.assign(new Error('cancelled'), { name: 'AbortError' }); + }), + ); + expect(await pickFileHandle('doc.darkly', [accept], 'darkly-file')).toBeNull(); + }); + + it('rethrows non-abort errors', async () => { + vi.stubGlobal( + 'showSaveFilePicker', + vi.fn(async () => { + throw new Error('permission denied'); + }), + ); + await expect(pickFileHandle('doc.darkly', [accept], 'darkly-file')).rejects.toThrow( + 'permission denied', + ); + }); +}); + +describe('writeToHandle', () => { + function fakeHandle() { + const write = vi.fn(async () => {}); + const close = vi.fn(async () => {}); + return { + handle: { createWritable: vi.fn(async () => ({ write, close })) } as unknown as FileSystemFileHandle, + write, + close, + }; + } + + it('writes Uint8Array bytes then closes', async () => { + const { handle, write, close } = fakeHandle(); + const bytes = new Uint8Array([1, 2, 3]); + await writeToHandle(handle, bytes); + expect(write).toHaveBeenCalledWith(bytes); + expect(close).toHaveBeenCalledOnce(); + }); + + it('writes a Blob unchanged then closes', async () => { + const { handle, write, close } = fakeHandle(); + const blob = new Blob([new Uint8Array([9])]); + await writeToHandle(handle, blob); + expect(write).toHaveBeenCalledWith(blob); + expect(close).toHaveBeenCalledOnce(); + }); +}); + +describe('saveViaDownload (Firefox/Safari download path)', () => { + it('produces bytes, hands a correctly-named Blob to downloadBlob', async () => { + const fakeBlob = new Blob([new Uint8Array([1, 2, 3])], { type: 'image/png' }); + vi.mocked(exportComposite).mockResolvedValue(fakeBlob); + + await saveViaDownload({} as never, 'png', 'My Pic'); + + expect(downloadBlob).toHaveBeenCalledTimes(1); + const [blob, name] = vi.mocked(downloadBlob).mock.calls[0]; + expect(name).toBe('My Pic.png'); + expect(blob).toBe(fakeBlob); + }); + + it('an image export does NOT drop the recovery snapshot (doc stays dirty)', async () => { + vi.mocked(exportComposite).mockResolvedValue(new Blob([new Uint8Array([1])])); + await saveViaDownload({} as never, 'webp', 'pic'); + expect(removeSnapshot).not.toHaveBeenCalled(); + }); +}); + +describe('regression: save works without the File System Access API', () => { + it('hasFilePicker is false in this (Firefox-like) environment', () => { + // The node test env has no `showSaveFilePicker` — exactly Firefox/Safari. + expect(hasFilePicker).toBe(false); + }); + + it('Ctrl+S routes to the download Save modal instead of dead-ending', async () => { + // Pre-fix, `saveDocument` hit `if (!canSave) { toast error; return }` and + // produced nothing. Now it must open the download-backed Save modal. + const instance = { + engine: { api: { documentName: async () => 'MyDoc' } }, + fileHandle: null, + }; + setActiveInstance(instance as never); + expect(saveModal.open).toBe(false); + + const pending = saveDocument({ forceAs: false }); + await vi.waitFor(() => expect(saveModal.open).toBe(true)); + expect(saveModal.suggestedName).toBe('MyDoc'); + + // Resolve the awaited modal so the flow (and this test) completes. + saveModal.finish(); + await pending; + expect(saveModal.open).toBe(false); + }); +}); diff --git a/frontend/src/storage/exportComposite.ts b/frontend/src/storage/exportComposite.ts new file mode 100644 index 00000000..e4a1f580 --- /dev/null +++ b/frontend/src/storage/exportComposite.ts @@ -0,0 +1,80 @@ +/** + * Composite export: read back the current canvas composite via the async + * export readback and encode it to an image Blob off the WASM main thread. + * + * `rgbaToBlob` (OffscreenCanvas `putImageData → convertToBlob`) is the single + * home for RGBA→image encoding — `saveDocument.ts` reuses it for the `.darkly` + * zip's internal `composite.png` too, so the encode core exists once. + */ + +import type { DarklyInstance } from '../state/app.svelte'; + +export type ImageFormat = 'png' | 'jpeg' | 'webp'; + +const MIME: Record = { + png: 'image/png', + jpeg: 'image/jpeg', + webp: 'image/webp', +}; + +// JPEG/WebP quality is fixed at 0.92 — the historical export default; PNG is +// lossless and ignores it. +const QUALITY = 0.92; + +/** Encode raw RGBA8 pixels to an image Blob via `OffscreenCanvas`. The + * browser's encoder runs off the WASM main thread. `quality` is omitted for + * PNG (lossless). */ +export async function rgbaToBlob( + rgba: Uint8Array, + width: number, + height: number, + mime: string, + quality?: number, +): Promise { + const canvas = new OffscreenCanvas(width, height); + const ctx = canvas.getContext('2d'); + if (!ctx) throw new Error('2d context unavailable'); + // ImageData rejects SharedArrayBuffer-backed Uint8ClampedArray (which the + // WASM heap can be); copy into a fresh ArrayBuffer first. + const copy = new Uint8ClampedArray(rgba.length); + copy.set(rgba); + ctx.putImageData(new ImageData(copy, width, height), 0, 0); + return quality === undefined + ? await canvas.convertToBlob({ type: mime }) + : await canvas.convertToBlob({ type: mime, quality }); +} + +/** Drive the async export readback for `instance` and encode the composite to + * an image Blob. Kicks `startExport` and awaits the one-shot `onExportResult` + * callback the render loop resolves. */ +export function exportComposite( + instance: DarklyInstance, + format: ImageFormat, +): Promise { + return new Promise((resolve, reject) => { + const engine = instance.engine; + if (!engine) { + reject(new Error('no engine handle')); + return; + } + instance.onExportResult(async (result) => { + try { + if (!result?.rgba) { + reject(new Error('export produced no pixels')); + return; + } + const quality = format === 'png' ? undefined : QUALITY; + resolve( + await rgbaToBlob(result.rgba, result.width, result.height, MIME[format], quality), + ); + } catch (e) { + reject(e instanceof Error ? e : new Error(String(e))); + } + }); + // `startExport` rejects on error; surface that instead of hanging on a + // callback that will never fire. + Promise.resolve(engine.api.startExport()).catch((e) => + reject(e instanceof Error ? e : new Error(String(e))), + ); + }); +} diff --git a/frontend/src/storage/fileHandle.ts b/frontend/src/storage/fileHandle.ts index 429b31c4..24a739a1 100644 --- a/frontend/src/storage/fileHandle.ts +++ b/frontend/src/storage/fileHandle.ts @@ -8,31 +8,31 @@ * saves through Ctrl+S, Ctrl+O, the hamburger menu, etc. * * Three backends, picked per-call: - * - **FS Access API** (Chromium, Safari 16.4+): real handle, persists - * for the session — Ctrl+S after a Save As writes back to the same - * file with no prompt. + * - **FS Access API** (Chromium only): real handle, persists for the + * session — Ctrl+S after a Save As writes back to the same file with + * no prompt, and the picker remembers the last-used directory per `id`. * - **Electron**: deferred — when the desktop bundle needs file * save/open, extend `ElectronStorageBridge` with `saveAs/open` * methods returning paths-as-handles and add a branch here. - * - **Hidden-input fallback** (Firefox, older browsers): bytes only, - * no handle returned, so subsequent Save has nowhere to write. - * Phase 5 surfaces that by disabling Save/Save As at the UI level. + * - **Download fallback** (Firefox, Safari, older browsers): no picker + * exists, so the caller writes bytes via a browser download. The save + * flow (`saveDocument.ts`) routes to an in-app Save modal there. */ -/** Whether this browser can persist file save (FS Access API present). - * Save / Save As actions are disabled at the UI level when this is - * `false` — matches draw.io's posture for Firefox. */ -export const canSave: boolean = +/** Whether this browser can show a native save picker (FS Access API + * present). Chromium only — Firefox and Safari implement the + * Origin-Private File System but not `showSaveFilePicker`, so they use + * the download fallback. */ +export const hasFilePicker: boolean = typeof globalThis !== 'undefined' && typeof (globalThis as { showSaveFilePicker?: unknown }).showSaveFilePicker === 'function'; -/** Save picker filter — only `.darkly` writes back out today. */ -const SAVE_TYPES = [ - { - description: 'Darkly Document', - accept: { 'application/x-darkly': ['.darkly'] as readonly string[] }, - }, -]; +/** A file-type filter for the save/open pickers — mirrors the FS Access + * API's `FilePickerAcceptType`. */ +export interface SaveAccept { + description: string; + accept: Record; +} /** Open picker filter — accepts any file the unified Open flow knows * how to ingest. Documents and raster images share one picker; the @@ -80,49 +80,57 @@ export interface OpenedFile { handle: FileSystemFileHandle | null; } -/** Show the Save As picker. Returns the chosen handle, or `null` if +/** Show the native Save picker. Returns the chosen handle, or `null` if * the user cancelled. Throws on permission denial / API errors so the * caller can surface a toast. * - * Must be called from a user-activation context (click / keydown); - * Chrome / Safari throw `SecurityError` otherwise. The Ctrl+S - * keydown qualifies. + * `accepts` populates the picker's "Save as type" dropdown (first entry + * is the default); the caller infers the chosen type from `handle.name`. + * `pickerId` seeds the FS Access API `id` so the browser reopens in the + * last-used directory for that id and shows a native overwrite prompt. + * + * Must be called from a user-activation context (click / keydown); Chrome + * throws `SecurityError` otherwise (the Ctrl+S keydown qualifies). Only + * call when `hasFilePicker` is true. */ -export async function pickSaveFile( +export async function pickFileHandle( suggestedName: string, + accepts: SaveAccept[], + pickerId: string, ): Promise { - if (!canSave) return null; + // The File System Access API isn't in the standard lib.dom types, so go + // through `unknown` — TS 5.7+ rejects direct casts where neither type + // sufficiently overlaps. + const api = ( + globalThis as unknown as { + showSaveFilePicker: (opts: { + suggestedName?: string; + types?: SaveAccept[]; + id?: string; + }) => Promise; + } + ).showSaveFilePicker; try { - // The File System Access API isn't in the standard lib.dom types, so - // go through `unknown` — TS 5.7+ rejects direct casts where neither - // type sufficiently overlaps. - const api = ( - globalThis as unknown as { - showSaveFilePicker: (opts: { - suggestedName?: string; - types?: typeof SAVE_TYPES; - }) => Promise; - } - ).showSaveFilePicker; - return await api({ suggestedName, types: SAVE_TYPES }); + return await api({ suggestedName, types: accepts, id: pickerId }); } catch (e) { if ((e as { name?: string })?.name === 'AbortError') return null; throw e; } } -/** Write bytes to a previously-acquired handle. The writable is - * truncated on open so partial writes can't leave stale tail bytes. */ +/** Write data to a previously-acquired handle. The writable is truncated + * on open so partial writes can't leave stale tail bytes. Accepts a + * `Blob` (export encodes) or `Uint8Array` (the `.darkly` zip). */ export async function writeToHandle( handle: FileSystemFileHandle, - bytes: Uint8Array, + data: Uint8Array | Blob, ): Promise { const writable = await handle.createWritable(); - // FileSystemWriteChunkType requires Uint8Array; TS 5.7+ - // defaults Uint8Array to . Our callers all produce - // non-shared buffers (WASM exports, fflate output, fresh - // `new Uint8Array`), so the cast is sound. - await writable.write(bytes as Uint8Array); + // FileSystemWriteChunkType accepts Blob and BufferSource. TS 5.7+ defaults + // Uint8Array to ; our byte callers all produce non-shared + // buffers (WASM exports, fflate output, fresh `new Uint8Array`), so the + // cast is sound. Blobs pass through unchanged. + await writable.write(data instanceof Blob ? data : (data as Uint8Array)); await writable.close(); } @@ -158,8 +166,8 @@ export async function pickOpenFile(): Promise { /** Fallback: build a transient `` on demand, click * it, and resolve with the chosen file. No handle is returned because - * Firefox doesn't expose one — Save / Save As stay disabled in that - * session (the UI consults `canSave`). */ + * Firefox doesn't expose one — a later Save on this document routes + * through the download fallback (`saveDocument` consults `hasFilePicker`). */ async function pickViaHiddenInput(): Promise { return await new Promise(resolve => { const input = document.createElement('input'); diff --git a/frontend/src/storage/saveDocument.ts b/frontend/src/storage/saveDocument.ts index 9a3a58c8..8184fc53 100644 --- a/frontend/src/storage/saveDocument.ts +++ b/frontend/src/storage/saveDocument.ts @@ -11,10 +11,12 @@ */ import { zip, type Zippable } from 'fflate'; -import { app, getActiveInstance, type DarklyInstance } from '../state/app.svelte'; +import { getActiveInstance, type DarklyInstance } from '../state/app.svelte'; import { toast } from '../state/toast.svelte'; -import { canSave, pickSaveFile, writeToHandle } from './fileHandle'; -import { sanitizeFilename } from './index'; +import { hasFilePicker, pickFileHandle, writeToHandle, type SaveAccept } from './fileHandle'; +import { downloadBlob, sanitizeFilename } from './index'; +import { exportComposite, rgbaToBlob } from './exportComposite'; +import { saveModal } from '../state/saveModal.svelte'; import { removeSnapshot } from './recovery'; import { sessionId } from '../state/recoverySession'; import { processRecording } from '../recording/recorder.svelte'; @@ -40,44 +42,177 @@ const COMPOSITE_PATH = 'composite.png'; const THUMBNAIL_PATH = 'thumbnail.png'; const MANIFEST_PATH = 'manifest.json'; +/** A save destination format. `.darkly` is a true document save; the image + * formats are behind-the-scenes exports of the current canvas composite. */ +export type Format = 'darkly' | 'png' | 'jpeg' | 'webp'; + +/** One row of the save format table. Each format owns everything about + * itself — consumers call `produce()` and read `isDocument`, never + * `switch(format)` (type-owned dispatch). */ +interface SaveFormat { + /** Filename extension (no dot). */ + ext: string; + mime: string; + /** File-picker filter entry. */ + accept: SaveAccept; + /** True for a real `.darkly` document save; false for an export. */ + isDocument: boolean; + /** Produce the bytes to write for this format. */ + produce(instance: DarklyInstance): Promise; +} + +/** The single source of truth for what Darkly can save/export to. */ +export const SAVE_FORMATS: Record = { + darkly: { + ext: 'darkly', + mime: 'application/x-darkly', + accept: { description: 'Darkly Document', accept: { 'application/x-darkly': ['.darkly'] } }, + isDocument: true, + produce: (i) => produceDarklyBytes(i, 'file'), + }, + png: { + ext: 'png', + mime: 'image/png', + accept: { description: 'PNG Image', accept: { 'image/png': ['.png'] } }, + isDocument: false, + produce: (i) => exportComposite(i, 'png'), + }, + jpeg: { + ext: 'jpg', + mime: 'image/jpeg', + accept: { description: 'JPEG Image', accept: { 'image/jpeg': ['.jpg', '.jpeg'] } }, + isDocument: false, + produce: (i) => exportComposite(i, 'jpeg'), + }, + webp: { + ext: 'webp', + mime: 'image/webp', + accept: { description: 'WebP Image', accept: { 'image/webp': ['.webp'] } }, + isDocument: false, + produce: (i) => exportComposite(i, 'webp'), + }, +}; + +/** Format order for the native picker's "Save as type" dropdown and the + * fallback modal — `.darkly` first so it's the default. */ +export const SAVE_FORMAT_ORDER: Format[] = ['darkly', 'png', 'jpeg', 'webp']; + +/** Picker type list, in `SAVE_FORMAT_ORDER`. */ +const SAVE_ACCEPTS: SaveAccept[] = SAVE_FORMAT_ORDER.map((f) => SAVE_FORMATS[f].accept); + +const EXT_TO_FORMAT: Record = { + darkly: 'darkly', + png: 'png', + jpg: 'jpeg', + jpeg: 'jpeg', + webp: 'webp', +}; + +/** Map a picker-chosen filename to a save format. Unknown or missing + * extensions fall back to a true `.darkly` save (the default). */ +export function formatFromName(name: string): Format { + const ext = name.slice(name.lastIndexOf('.') + 1).toLowerCase(); + return EXT_TO_FORMAT[ext] ?? 'darkly'; +} + +/** Shared `showSaveFilePicker` id — one id across save and export so the + * picker reopens in the same last-used directory. */ +const PICKER_ID = 'darkly-file'; + /** - * Save the current document. Drives: - * 1. Pick a handle (cached or via `showSaveFilePicker`). - * 2. WASM `start_save_document` → poll → `SaveBundle`. - * 3. PNG-encode composite + 256px thumbnail via `OffscreenCanvas`. - * 4. Assemble zip with `fflate`. - * 5. Write bytes via the file handle. + * Save the current document. One flow for `.darkly` saves and canvas exports: + * - Cached handle + not `forceAs` → silent re-save to the same `.darkly`. + * - Native picker present (Chromium) → multi-type picker; the chosen file + * type decides `.darkly` save vs image export; write to the handle. + * - No picker (Firefox/Safari) → the in-app Save modal drives the download. * - * `forceAs` skips the cached handle and always prompts (Ctrl+Shift+S). + * `forceAs` skips the cached handle and always prompts (Ctrl+Shift+S). The + * returned promise resolves only once the save is fully done (including the + * async fallback modal), so `closeGuard` can await it. */ export async function saveDocument({ forceAs = false }: { forceAs?: boolean } = {}): Promise { - if (!app.engine) return; - if (!canSave) { - toast.show( - 'error', - "Save isn't supported in this browser — try Chrome, Edge, or Safari.", - ); + const instance = getActiveInstance(); + if (!instance?.engine) return; + + // Silent re-save to the cached handle (Chromium, after a first Save As or + // opening a `.darkly`). + if (!forceAs && instance.fileHandle) { + try { + const bytes = await produceDarklyBytes(instance, 'file'); + await writeToHandle(instance.fileHandle, bytes); + await afterSaved(instance, 'darkly'); + } catch (e: unknown) { + toast.show('error', `Save failed: ${errorMessage(e)}`); + } return; } - const handle = await acquireHandle(forceAs); - if (!handle) return; // user cancelled + const suggested = + sanitizeFilename(await instance.engine.api.documentName()) || 'darkly-document'; - const instance = getActiveInstance(); - if (!instance?.engine) return; + // Firefox / Safari: no native picker — route to the in-app Save modal, + // which drives produce + download and resolves when the user is done. + if (!hasFilePicker) { + await saveModal.request(suggested); + return; + } + // Chromium: native multi-type picker (activation required here — bytes are + // produced *after* so the picker isn't blocked). Only the picker needs + // transient activation; `writeToHandle` on the returned handle does not. try { - const zipBytes = await produceDarklyBytes(instance, 'file'); - await writeToHandle(handle, zipBytes); - app.fileHandle = handle; - // The document is now safely on disk — drop its recovery snapshot. - await removeSnapshot(sessionId, instance.recoveryId).catch(() => {}); - toast.show('success', 'Saved'); + const handle = await pickFileHandle(`${suggested}.darkly`, SAVE_ACCEPTS, PICKER_ID); + if (!handle) return; // user cancelled + const format = formatFromName(handle.name); + const data = await SAVE_FORMATS[format].produce(instance); + await writeToHandle(handle, data); + if (SAVE_FORMATS[format].isDocument) { + // A true save adopts this file: cache the handle for silent Ctrl+S + // re-saves and reflect the chosen filename in the doc name. + instance.fileHandle = handle; + const baseName = handle.name.replace(/\.[^./]+$/, ''); + if (baseName) instance.engine.api.setDocumentName({ name: baseName }); + } + await afterSaved(instance, format); } catch (e: unknown) { toast.show('error', `Save failed: ${errorMessage(e)}`); } } +/** + * Produce `format` bytes for `instance` and save them via a browser download + * — the Firefox/Safari path, shared with the fallback Save modal so the format + * table and encode live in exactly one place. + */ +export async function saveViaDownload( + instance: DarklyInstance, + format: Format, + baseName: string, +): Promise { + const fmt = SAVE_FORMATS[format]; + const data = await fmt.produce(instance); + const blob = + data instanceof Blob + ? data + : new Blob([data as Uint8Array], { type: fmt.mime }); + downloadBlob(blob, `${sanitizeFilename(baseName) || 'darkly-document'}.${fmt.ext}`); + await afterSaved(instance, format); +} + +/** + * Post-save bookkeeping. A true `.darkly` save clears crash-recovery state and + * toasts "Saved"; an image export leaves the document dirty (its recovery + * snapshot must survive a later crash) and toasts "Exported". + */ +async function afterSaved(instance: DarklyInstance, format: Format): Promise { + if (SAVE_FORMATS[format].isDocument) { + await removeSnapshot(sessionId, instance.recoveryId).catch(() => {}); + toast.show('success', 'Saved'); + } else { + toast.show('success', 'Exported'); + } +} + /** * Drive a `.darkly` save for `instance` to completion and return the * assembled zip bytes — the destination-agnostic core shared by file-save @@ -104,26 +239,6 @@ export async function produceDarklyBytes( return assembleZip(bundle, recording); } -/** Resolve the file handle for the active save. Re-uses the cached - * handle when one exists and `forceAs` is false; otherwise prompts via - * the picker and seeds `doc.name` from the chosen filename. */ -async function acquireHandle(forceAs: boolean): Promise { - const engine = app.engine; - if (!engine) return null; - if (!forceAs && app.fileHandle) return app.fileHandle; - - const suggested = - sanitizeFilename(await engine.api.documentName()) || 'darkly-document'; - const handle = await pickSaveFile(`${suggested}.darkly`); - if (!handle) return null; - - // Reflect the chosen filename in the doc's display name so the tab - // strip and a subsequent Ctrl+S both pick it up. - const baseName = handle.name.replace(/\.darkly$/i, ''); - if (baseName) engine.api.setDocumentName({ name: baseName }); - return handle; -} - /** Kick `start_save_document` on `instance` and await the * `poll_save_result` callback. `snapshot` marks an autosave save (which * must not clear the document's dirty flag — see the Rust `SavePurpose`). @@ -185,23 +300,14 @@ async function assembleZip( }); } -/** Round-trip RGBA8 bytes through `OffscreenCanvas` to PNG. The - * browser's PNG encoder runs off the WASM main thread and reuses the - * same path `export-image` already uses. */ +/** PNG-encode RGBA8 bytes for the zip's internal `composite.png`, sharing the + * same `OffscreenCanvas` encode core as `export-image` (`rgbaToBlob`). */ async function encodeRgbaPng( rgba: Uint8Array, width: number, height: number, ): Promise { - const canvas = new OffscreenCanvas(width, height); - const ctx = canvas.getContext('2d'); - if (!ctx) throw new Error('2d context unavailable'); - // ImageData rejects SharedArrayBuffer-backed Uint8ClampedArray - // (which the WASM heap can be); copy into a fresh ArrayBuffer. - const copy = new Uint8ClampedArray(rgba.length); - copy.set(rgba); - ctx.putImageData(new ImageData(copy, width, height), 0, 0); - const blob = await canvas.convertToBlob({ type: 'image/png' }); + const blob = await rgbaToBlob(rgba, width, height, 'image/png'); return new Uint8Array(await blob.arrayBuffer()); } diff --git a/frontend/src/tools/__tests__/text_tool_create.test.ts b/frontend/src/tools/__tests__/text_tool_create.test.ts new file mode 100644 index 00000000..5397a730 --- /dev/null +++ b/frontend/src/tools/__tests__/text_tool_create.test.ts @@ -0,0 +1,121 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { withApi } from '../../engine/testApi'; + +// Mock `app` and `config` before importing the tool so we never pull in the +// real engine/wasm. The regression guard: the text tool must CREATE the object +// itself on pointer-up — not merely stash a `placement` for a properties panel +// to consume. The old design created text only inside a `$effect` in +// `TextProperties.svelte`, so when that panel was tabbed behind another (thus +// unmounted), the gesture was silently dropped. This test has NO panel mounted +// (node env, no components) and asserts an `add_text` request is issued anyway. +const { fakeApp } = vi.hoisted(() => ({ + fakeApp: { + engine: null as unknown, + // `null` so the create-band overlay draw (which needs `window`) is a + // no-op in the DOM-free test env; the tool doesn't need a canvas to + // create text. + canvasEl: null as unknown, + selectLayer: vi.fn(), + requestFrame: vi.fn(), + refreshLayerTree: vi.fn(() => Promise.resolve()), + activeLayerId: null as number | null, + activeNode: null as { id: number; type: string } | null, + foreground: { r: 0, g: 0, b: 0, a: 255 }, + toolCursor: null as string | null, + }, +})); +vi.mock('../../state/app.svelte', () => ({ app: fakeApp, getActiveInstance: () => fakeApp })); +vi.mock('../../config/store.svelte', () => ({ config: { get: () => undefined } })); + +import { textTool } from '../text.svelte'; + +type TextToolLike = { + onActivate(): Promise; + onPointerDown(e: PointerEvent, cx: number, cy: number): Promise; + onPointerMove(e: PointerEvent, cx: number, cy: number): void; + onPointerUp(e: PointerEvent): void | Promise; + editing: { layerId: number; objectId: number } | null; + focusObject: number | null; +}; +const tool = textTool.create(fakeApp as never) as unknown as TextToolLike; + +const ev = (button = 0) => ({ button }) as unknown as PointerEvent; + +function mockEngine() { + return withApi({ + send: vi.fn((kind: string) => { + if (kind === 'add_text') return Promise.resolve({ id: 7, object: 3 }); + if (kind === 'add_text_object') return Promise.resolve({ object: 9 }); + if (kind === 'text_objects') return Promise.resolve({ objects: [] }); + if (kind === 'hit_test_vector_object') return Promise.resolve({ object: -1 }); + return Promise.resolve(null); + }), + post: vi.fn(), + }); +} + +beforeEach(() => { + fakeApp.selectLayer.mockClear(); + fakeApp.requestFrame.mockClear(); + fakeApp.refreshLayerTree.mockClear(); + fakeApp.engine = mockEngine(); + fakeApp.canvasEl = null; + fakeApp.activeLayerId = null; + fakeApp.activeNode = null; + tool.editing = null; + tool.focusObject = null; +}); + +describe('text tool warms the vector renderer on activation', () => { + it('activating the tool compiles the Vello pipelines ahead of first use', async () => { + // Selecting the text tool must warm the renderer so the first box doesn't + // stall on Vello's >1s one-time shader compile. + fakeApp.canvasEl = {}; + await tool.onActivate(); + const engine = fakeApp.engine as ReturnType; + expect(engine.post).toHaveBeenCalledWith('warm_vector_renderer'); + }); +}); + +describe('text tool creates the object itself (panel-independent)', () => { + it('a drag creates a new text layer with NO properties panel mounted', async () => { + await tool.onPointerDown(ev(), 20, 20); + tool.onPointerMove(ev(), 220, 140); + await tool.onPointerUp(ev()); + const engine = fakeApp.engine as ReturnType; + // Observable outcome — not another module's internal wire shape (no + // `anchor` assertion; that's `createTextFromPending`'s new-layer detail). + expect(engine.send).toHaveBeenCalledWith( + 'add_text', + expect.objectContaining({ x: 20, y: 20, box: [200, 120] }), + ); + expect(fakeApp.selectLayer).toHaveBeenCalledWith(7); + expect(tool.editing).toEqual({ layerId: 7, objectId: 3 }); + expect(tool.focusObject).toBe(3); + }); + + it('a click on empty canvas creates point text (no box)', async () => { + await tool.onPointerDown(ev(), 100, 60); + await tool.onPointerUp(ev()); + const engine = fakeApp.engine as ReturnType; + expect(engine.send).toHaveBeenCalledWith( + 'add_text', + expect.objectContaining({ x: 100, y: 60, box: null }), + ); + }); + + it('a drag on an active vector layer adds an object, no new layer', async () => { + fakeApp.activeLayerId = 42; + fakeApp.activeNode = { id: 42, type: 'vector' }; + await tool.onPointerDown(ev(), 20, 20); + tool.onPointerMove(ev(), 220, 140); + await tool.onPointerUp(ev()); + const engine = fakeApp.engine as ReturnType; + expect(engine.send).toHaveBeenCalledWith( + 'add_text_object', + expect.objectContaining({ id: 42, x: 20, y: 20, box: [200, 120] }), + ); + // The vector layer is already active; adding an object must not reselect. + expect(fakeApp.selectLayer).not.toHaveBeenCalled(); + }); +}); diff --git a/frontend/src/tools/__tests__/text_tool_placement.test.ts b/frontend/src/tools/__tests__/text_tool_placement.test.ts deleted file mode 100644 index 33b5ea43..00000000 --- a/frontend/src/tools/__tests__/text_tool_placement.test.ts +++ /dev/null @@ -1,65 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; - -// Mock `app` and `config` before importing the tool so we never pull in the -// real engine/wasm. The regression guard: a first click on empty canvas must -// set a pending placement WITHOUT deselecting the active layer — deselecting -// changed `activeLayerId`, which tripped CanvasView's dismiss effect and wiped -// the placement on the same click (the "first click does nothing" bug). -const { fakeApp } = vi.hoisted(() => ({ - fakeApp: { - engine: null as unknown, - canvasEl: {} as unknown, - selectLayer: vi.fn(), - requestFrame: vi.fn(), - activeLayerId: 5 as number | null, - toolCursor: null as string | null, - }, -})); -vi.mock('../../state/app.svelte', () => ({ app: fakeApp })); -vi.mock('../../config/store.svelte', () => ({ config: { get: () => undefined } })); - -import { textTool, type TextPlacement } from '../text.svelte'; - -// The per-document edit state lives on the tool instance now; widen the created -// tool so the placement field is visible. -type TextToolLike = { - onPointerDown(e: PointerEvent, cx: number, cy: number): Promise; - onPointerMove(e: PointerEvent, cx: number, cy: number): void; - onPointerUp(): void; - placement: TextPlacement | null; - editing: unknown; -}; -const tool = textTool.create(fakeApp as never) as unknown as TextToolLike; - -const ev = (button = 0) => ({ button }) as unknown as PointerEvent; - -beforeEach(() => { - fakeApp.selectLayer.mockClear(); - fakeApp.engine = null; - fakeApp.activeLayerId = 5; - tool.placement = null; - tool.editing = null; -}); - -describe('text tool placement (first-click regression)', () => { - it('a click on empty canvas sets a point placement and does not deselect', async () => { - await tool.onPointerDown(ev(), 100, 60); - // No movement between down and up → a click, i.e. point text. - tool.onPointerUp(); - expect(tool.placement).toEqual({ x: 100, y: 60, anchorLayerId: 5, box: null }); - expect(fakeApp.selectLayer).not.toHaveBeenCalled(); - }); - - it('a drag commits an area-text box placement', async () => { - await tool.onPointerDown(ev(), 20, 20); - tool.onPointerMove(ev(), 220, 140); - tool.onPointerUp(); - expect(tool.placement).toEqual({ - x: 20, - y: 20, - anchorLayerId: 5, - box: [200, 120], - }); - expect(fakeApp.selectLayer).not.toHaveBeenCalled(); - }); -}); diff --git a/frontend/src/tools/text.svelte.ts b/frontend/src/tools/text.svelte.ts index bfe3f12b..6911bde0 100644 --- a/frontend/src/tools/text.svelte.ts +++ b/frontend/src/tools/text.svelte.ts @@ -1,7 +1,7 @@ import { getActiveInstance, type DarklyInstance } from '../state/app.svelte'; import { config } from '../config/store.svelte'; import { OverlayBuilder } from '../canvas/gpu_overlay'; -import { flushTextContent } from './text_editor'; +import { createTextFromPending, flushTextContent, type FullStyle, type Rgba } from './text_editor'; import { TextBoxGizmo } from './text_box_gizmo'; import { ToolBase, type ToolDescriptor } from './registry'; @@ -31,15 +31,33 @@ class TextSession { export const textSession = new TextSession(); +/** Content a new text object is seeded with — shown selected in the editor so + * the first keystroke replaces it. */ +const SEED_TEXT = 'text'; + +/** The current creation style, baked from the app-global creation defaults. */ +function currentStyle(): FullStyle { + return { + font_family: textSession.fontFamily, + size: textSession.size, + variations: { ...textSession.variations }, + letter_spacing: textSession.letterSpacing, + word_spacing: textSession.wordSpacing, + line_height: textSession.lineHeight, + italic: textSession.italic, + align: textSession.align, + }; +} + /** The object whose box gizmo is shown, keyed by layer + object. */ export interface TextEditing { layerId: number; objectId: number; } -/** A click/drag on empty canvas handed to the panel, which creates the text - * object there immediately (seeded "text", selected). `box` is set when it was - * drag-created (area text). */ +/** A click/drag on empty canvas: the origin, the anchor layer to insert above, + * and `box` (`[w, h]`) when it was drag-created (area text). Consumed + * synchronously by the tool to create the text object. */ export interface TextPlacement { x: number; y: number; @@ -54,27 +72,29 @@ const DRAG_THRESHOLD = 4; * Text tool — per-instance. Chooses what the properties panel edits and drives * the on-canvas box; the text controls live in `TextProperties.svelte`. * - * The per-document edit state lives here as reactive fields (read by the panel - * via {@link focusedTextTool}): + * Creating a text object is a session→document operation, so the tool does it + * directly on pointer-up (via {@link createTextFromPending}) — it does not depend + * on any properties panel being mounted. The per-document edit state lives here + * as reactive fields (read by the panel via {@link focusedTextTool}): * - `focusObject` asks the panel to focus a specific object's editor next * render (set when a text-tool click hits an existing object, or on create). * - `editing` is the object whose box gizmo is shown — the one just clicked or * created. The box follows the active layer (see `onFrame`). - * - `placement` is a pending create the panel consumes. */ class TextTool extends ToolBase { /** Object the panel should focus next render, or null. */ focusObject = $state(null); /** Object whose box gizmo is drawn on the canvas, or null. */ editing = $state(null); - /** Where the panel should create the next text object (consumed on create), - * or null when there's no pending placement. */ - placement = $state(null); /** The box gizmo for the object currently being edited, created on activate, * torn down on deactivate. */ private gizmo: TextBoxGizmo | null = null; + /** Reentrancy guard: true while an async create is in flight, so a second + * pointer-up mid-await can't create a duplicate object. */ + private busy = false; + /** In-flight drag-to-create-a-box: start + current canvas position. */ private creating: | { sx: number; sy: number; cx: number; cy: number; anchorLayerId: number | null } @@ -97,10 +117,50 @@ class TextTool extends ToolBase { this.inst.requestFrame(); } + private foregroundTuple(): Rgba { + const c = this.inst.foreground; + return [c.r, c.g, c.b, c.a]; + } + + /** Create a text object for `placement` and select/focus it. A vector layer + * active → the object is added to it (a vector layer owns many objects); + * otherwise a new text layer is born. Seeded with "text" so the word appears + * at once and the first keystroke replaces it. */ + private async createText(placement: TextPlacement): Promise { + if (this.busy || !this.inst.engine) return; + this.busy = true; + try { + const active = this.inst.activeNode; + const target = active?.type === 'vector' ? active.id : null; + await createTextFromPending( + this.inst, + placement, + SEED_TEXT, + currentStyle(), + this.foregroundTuple(), + () => SEED_TEXT, + target, + (layerId, objectId) => { + // Show the box gizmo on the new object (drives `onFrame`) and + // ask the panel — if/when mounted — to focus its editor. + this.editing = { layerId, objectId }; + this.focusObject = objectId; + }, + ); + } finally { + this.busy = false; + } + } + async onActivate(): Promise { const canvasEl = this.canvasEl; if (!canvasEl) return; this.gizmo = new TextBoxGizmo(canvasEl); + // Compile the vector renderer's GPU pipelines now (Vello's one-time + // shader compile is >1s). Selecting the tool is the earliest signal text + // is imminent, so the compile overlaps the gap before the first click + // instead of stalling the frame that would show the new box. + this.inst.engine?.api.warmVectorRenderer(); // Pick up the persisted default size if the user configured one. const cfgSize = config.get('tools.textSize'); if (typeof cfgSize === 'number') textSession.size = cfgSize; @@ -123,10 +183,8 @@ class TextTool extends ToolBase { onDeactivate(): void { // Flush any coalesced keystroke before leaving so the last character - // isn't dropped, then drop transient state (a never-typed placement - // discards nothing — no layer was created). + // isn't dropped, then drop transient edit/box state. flushTextContent(); - this.placement = null; this.editing = null; this.creating = null; this.gizmo?.detach(); @@ -162,7 +220,6 @@ class TextTool extends ToolBase { this.inst.selectLayer(layerId); this.focusObject = hit.object; this.editing = { layerId, objectId: hit.object }; - this.placement = null; await this.gizmo?.attach(layerId, hit.object); return; } @@ -190,7 +247,7 @@ class TextTool extends ToolBase { this.gizmo?.pointerMove(cx, cy); } - onPointerUp(): void { + async onPointerUp(): Promise { if (this.gizmo?.dragging) { this.gizmo.pointerUp(); return; @@ -201,10 +258,9 @@ class TextTool extends ToolBase { this.clearCreateBand(); const w = Math.abs(c.cx - c.sx); const h = Math.abs(c.cy - c.sy); - // Hand the placement to the panel, which creates the text object there. - // A drag past the threshold makes it an area-text box; a click makes it - // point text. - this.placement = + // Create the object where the gesture ended. A drag past the threshold + // makes it an area-text box; a click makes point text. + const placement: TextPlacement = w > DRAG_THRESHOLD && h > DRAG_THRESHOLD ? { x: Math.min(c.sx, c.cx), @@ -213,6 +269,7 @@ class TextTool extends ToolBase { box: [w, h], } : { x: c.sx, y: c.sy, anchorLayerId: c.anchorLayerId, box: null }; + await this.createText(placement); } onKeyDown(e: KeyboardEvent): boolean { @@ -222,10 +279,6 @@ class TextTool extends ToolBase { this.clearCreateBand(); return true; } - if (this.placement) { - this.placement = null; - return true; - } if (this.editing) { this.editing = null; this.gizmo?.detach(); @@ -254,12 +307,11 @@ class TextTool extends ToolBase { } dismissOverlay(): void { - // Unhandled keypress on the canvas: flush the coalesced keystroke and - // drop a pending placement. The box gizmo is left to `onFrame` (it - // detaches on a layer switch), so a freshly-created edit isn't wiped by - // the same-click layer change that selects its new layer. + // Unhandled keypress on the canvas: flush the coalesced keystroke. The + // box gizmo is left to `onFrame` (it detaches on a layer switch), so a + // freshly-created edit isn't wiped by the same-click layer change that + // selects its new layer. flushTextContent(); - this.placement = null; } } diff --git a/frontend/src/ui/ConfirmDiscardModal.svelte b/frontend/src/ui/ConfirmDiscardModal.svelte index 8488de3b..56934344 100644 --- a/frontend/src/ui/ConfirmDiscardModal.svelte +++ b/frontend/src/ui/ConfirmDiscardModal.svelte @@ -1,14 +1,10 @@ @@ -22,8 +18,6 @@ diff --git a/frontend/src/ui/ExportImageModal.svelte b/frontend/src/ui/ExportImageModal.svelte deleted file mode 100644 index 90555445..00000000 --- a/frontend/src/ui/ExportImageModal.svelte +++ /dev/null @@ -1,194 +0,0 @@ - - - -
- - - - -
- - -
-
- - - diff --git a/frontend/src/ui/ExportTimelapseModal.svelte b/frontend/src/ui/ExportTimelapseModal.svelte index 2fd8837d..9be6f9b5 100644 --- a/frontend/src/ui/ExportTimelapseModal.svelte +++ b/frontend/src/ui/ExportTimelapseModal.svelte @@ -3,6 +3,12 @@ import { exportTimelapse } from '../state/exportTimelapse.svelte'; import { getActiveInstance } from '../state/app.svelte'; import { downloadBlob, sanitizeFilename } from '../storage'; + import { + hasFilePicker, + pickFileHandle, + writeToHandle, + type SaveAccept, + } from '../storage/fileHandle'; import { processRecording } from '../recording/recorder.svelte'; import { fitToLongEdge, WIDTH_ALIGN, HEIGHT_ALIGN } from '../recording/codec'; import { @@ -28,6 +34,13 @@ { id: 'gif', label: 'GIF', ext: 'gif' }, ]; + // Save-picker filters — a timelapse is never the document, so it uses the + // shared picker but never sets `fileHandle`. + const TIMELAPSE_ACCEPT: Record = { + mp4: { description: 'MP4 Video', accept: { 'video/mp4': ['.mp4'] } }, + gif: { description: 'GIF Image', accept: { 'image/gif': ['.gif'] } }, + }; + const METHODS: Array<{ id: ConversionMethod; label: string }> = [ { id: 'fit', label: 'Fit (letterbox)' }, { id: 'fill', label: 'Fill (crop)' }, @@ -114,6 +127,25 @@ async function confirm() { const inst = getActiveInstance(); if (!inst || exporting || !info || !group) return; + + const ext = FORMATS.find((f) => f.id === format)!.ext; + const filename = `${sanitizeFilename(baseName) || 'darkly-timelapse'}.${ext}`; + + // Acquire the destination in the click's user-activation window, before + // the long async encode consumes it. Chromium → native picker (same + // wrapper as Save); elsewhere the handle stays null and we download. + let handle: FileSystemFileHandle | null = null; + if (hasFilePicker) { + try { + handle = await pickFileHandle(filename, [TIMELAPSE_ACCEPT[format]], 'darkly-file'); + } catch (e) { + console.error('[export-timelapse] picker failed', e); + alert('Timelapse export failed — see console for details.'); + return; + } + if (!handle) return; // cancelled + } + exporting = true; try { const rate = clampFps(Number(fps)); @@ -123,9 +155,8 @@ format === 'mp4' ? await exportTimelapseMp4(inst, opts) : await exportTimelapseGif(inst, opts); - const ext = FORMATS.find((f) => f.id === format)!.ext; - const filename = `${sanitizeFilename(baseName) || 'darkly-timelapse'}.${ext}`; - downloadBlob(blob, filename); + if (handle) await writeToHandle(handle, blob); + else downloadBlob(blob, filename); exportTimelapse.open = false; } catch (e) { console.error('[export-timelapse] export failed', e); diff --git a/frontend/src/ui/SaveModal.svelte b/frontend/src/ui/SaveModal.svelte new file mode 100644 index 00000000..a848cd9d --- /dev/null +++ b/frontend/src/ui/SaveModal.svelte @@ -0,0 +1,183 @@ + + + +
+ + + + +
+ + +
+
+ + + diff --git a/frontend/src/ui/brush_builder/NodeWidget.svelte b/frontend/src/ui/brush_builder/NodeWidget.svelte index 5fcbe09e..9f50fcda 100644 --- a/frontend/src/ui/brush_builder/NodeWidget.svelte +++ b/frontend/src/ui/brush_builder/NodeWidget.svelte @@ -33,6 +33,13 @@ let typeInfo = $derived(brushGraph.getNodeType(node.type_id)); let displayName = $derived(typeInfo?.display_name ?? node.type_id); + /** The scalar math nodes (add/subtract/multiply/divide) offer an editor + * toggle that unlocks their numeric-input sliders from `0–1` to the + * extended range, for entering large gains. Frontend-only — see + * `brushGraph.extendedRangeNodes`. */ + let isMathNode = $derived(typeInfo?.category === 'math'); + let extendedRange = $derived(brushGraph.extendedRangeNodes.has(node.id)); + /** Apply the port's `visible_when` rule against the referenced sibling * input's current value. Engine-side the port still works regardless — * this is purely UI. */ @@ -166,6 +173,17 @@ {/if} + {#if isMathNode} + + {/if} + {#if isPreviewable} {/if} @@ -302,4 +320,18 @@ flex-direction: column; gap: 2px; } + + .extended-range { + display: flex; + align-items: center; + gap: 6px; + padding: 4px 8px 0; + font-size: 11px; + color: var(--text-muted); + cursor: pointer; + user-select: none; + } + .extended-range input { + cursor: pointer; + } diff --git a/frontend/src/ui/brush_builder/PortWidget.svelte b/frontend/src/ui/brush_builder/PortWidget.svelte index 2c521e37..05c25a63 100644 --- a/frontend/src/ui/brush_builder/PortWidget.svelte +++ b/frontend/src/ui/brush_builder/PortWidget.svelte @@ -1,6 +1,6 @@
{#if activeVeil} - {:else if activeLayer || textPlacement} - - {#if !textPlacement} - - {#if activeLayer && activeLayer.type !== 'filter'} - - {/if} - {#if activeLayer?.type === 'group'} - - {:else if activeLayer?.type === 'void'} - - {:else if activeLayer?.type === 'filter'} - - {/if} + {:else if activeLayer} + + {#if activeLayer.type !== 'filter'} + {/if} - - {#if activeLayer?.type === 'vector' || textPlacement} - + {#if activeLayer.type === 'group'} + + {:else if activeLayer.type === 'void'} + + {:else if activeLayer.type === 'filter'} + + {:else if activeLayer.type === 'vector'} + {/if} {:else}
No selection
diff --git a/frontend/src/ui/properties/TextProperties.svelte b/frontend/src/ui/properties/TextProperties.svelte index 308a794c..579eb908 100644 --- a/frontend/src/ui/properties/TextProperties.svelte +++ b/frontend/src/ui/properties/TextProperties.svelte @@ -1,9 +1,8 @@