Remove tool: an empty layer is a no-op instead of running the fill (#2435) (#2447)

* Remove tool: refuse an empty layer instead of running the fill (#2435)

* Remove tool: an empty layer is a no-op, as the other retouch tools (#2435)

crates/engine/src/retouch_cmds/remove.rs

@@ -104,8 +104,9 @@ fn hole(plan: &Plan, sel: Option<&Surface>) -> Vec<bool> {
     m
 }
 
-/// The filled window and the hole it fills, computed on `doc` (a snapshot).
-fn run_remove(doc: &mut Document, plan: &Plan, p: &Value, ctx: &crate::jobs::JobCtx, label: &str) -> Result<(Region, Vec<bool>)> {
+/// The filled window and the hole it fills, computed on `doc` (a snapshot). `None` when the
+/// layer's own window is fully transparent: nothing to remove and nothing to fill from.
+fn run_remove(doc: &mut Document, plan: &Plan, p: &Value, ctx: &crate::jobs::JobCtx, label: &str) -> Result<Option<(Region, Vec<bool>)>> {
     let hole = hole(plan, doc.selection.as_ref());
     if !hole.iter().any(|m| *m) {
         return Err(bad(CMD, "the stroke doesn't cover anything that can change (check the selection)"));
@@ -121,13 +122,17 @@ fn run_remove(doc: &mut Document, plan: &Plan, p: &Value, ctx: &crate::jobs::Job
         let img = if plan.all_layers { composite_region(doc, None, SampleLayers::All, win, fmt) } else { Region::read(surf, win) };
         (img, fmt)
     };
+    // A no-op without the fill or a history step, as the other retouch tools on an empty layer.
+    if !plan.all_layers && alpha_index(&fmt).is_some_and(|a| img.data.chunks_exact(img.ch).all(|px| px.get(a).is_none_or(|v| *v <= 0.0))) {
+        return Ok(None);
+    }
     let (w, h) = (win.width() as usize, win.height() as usize);
     let mut data = ctx
         .stage(0.0, 1.0, label, |ctl| photocraft_algo::remove::remove_with(w, h, img.ch, &img.data, &hole, SEED, ctl))
         .map_err(|_| EngineError::Cancelled)?;
     // The fill's seam blending can step slightly past the storable range.
     clamp_samples(&fmt, &mut data);
-    Ok((Region { rect: win, ch: img.ch, data }, hole))
+    Ok(Some((Region { rect: win, ch: img.ch, data }, hole)))
 }
 
 pub(super) fn remove(s: &mut Session, p: &Value) -> Result<Value> {
@@ -149,7 +154,8 @@ pub(super) fn remove(s: &mut Session, p: &Value) -> Result<Value> {
                 run_remove(&mut work, &plan, &p, ctx, label)
             }
         },
-        move |s, (out, hole)| {
+        move |s, filled| {
+            let Some((out, hole)) = filled else { return Ok(json!({"changed": false})) };
             let damage = s.edit(label, |doc, _| replace(doc, Target { id, all_layers: false }, &p, &out, &hole))?;
             if let Some(st) = s.active_mut() {
                 st.last_damage = Some(damage);

crates/engine/src/retouch_cmds/tests.rs

@@ -1430,6 +1430,19 @@ fn remove_open_strokes_and_the_selection_limit_what_changes() {
     assert!(!near(rgba(&s, 46, 30), bg, 0.05));
 }
 
+#[test]
+fn remove_on_an_empty_layer_is_a_no_op_without_a_history_step() {
+    let mut s = square_session(8, "rgb");
+    s.execute("layer.new.layer", json!({})).unwrap();
+    let past = s.active().unwrap().history.past_len();
+    let v = s.execute("paint.remove", json!({"points": [[38, 30], [54, 30]], "size": 16})).unwrap();
+    assert_eq!(v, json!({"changed": false}));
+    assert_eq!(s.active().unwrap().history.past_len(), past);
+    // With Sample All Layers on, the image below is there to remove from.
+    s.execute("paint.remove", json!({"points": [[38, 30], [54, 30]], "size": 16, "sampleAllLayers": true})).unwrap();
+    assert_eq!(s.active().unwrap().history.past_len(), past + 1);
+}
+
 #[test]
 fn remove_thin_scratches_take_their_surroundings() {
     let mut s = session(80, 40, 16, "rgb");