Merge pull request #12174 from hectorviov/fix/sampler-pixel-centre

Color Sampler: points sit on the clicked pixel's centre (#11725)

crates/engine/src/cmd/sampler_cmds.rs

@@ -60,14 +60,16 @@ fn coord(cmd: &str, p: &Value, key: &str) -> Result<f64> {
     p.get(key).and_then(Value::as_f64).filter(|v| v.is_finite()).ok_or_else(|| bad(cmd, format!("`{key}` must be a finite number (document pixels)")))
 }
 
-/// The pixel a point at document coordinate `(x, y)` sits on, if it is inside the canvas. Whole
-/// pixels only: the click's fractional position floors to the pixel it lands on.
+/// The centre of the pixel a point at document coordinate `(x, y)` sits on, if it is inside the
+/// canvas: the click's fractional position snaps to the middle of the pixel it lands on, so a
+/// click anywhere in pixel 199,149 stores 199.5,149.5, as in Photoshop (#11725). The readout
+/// floors it back to that pixel.
 fn pixel(doc: &Document, x: f64, y: f64) -> Option<[f64; 2]> {
     let (px, py) = (x.floor(), y.floor());
     if px < f64::from(i32::MIN) || px > f64::from(i32::MAX) || py < f64::from(i32::MIN) || py > f64::from(i32::MAX) {
         return None;
     }
-    doc.bounds().contains(px as i32, py as i32).then_some([px, py])
+    doc.bounds().contains(px as i32, py as i32).then_some([px + 0.5, py + 0.5])
 }
 
 fn index_param(cmd: &str, p: &Value) -> Result<usize> {
@@ -202,3 +204,6 @@ pub fn specs() -> Vec<CommandSpec> {
 
 #[cfg(test)]
 mod tests;
+
+#[cfg(test)]
+mod pixel_centre_tests;

crates/engine/src/cmd/sampler_cmds/pixel_centre_tests.rs

@@ -0,0 +1,109 @@
+//! Color Sampler points sit on the centre of the clicked pixel (#11725), as in Photoshop: a click
+//! anywhere in pixel 199,149 stores 199.5,149.5, the readout still reads that pixel, and document
+//! operations move the centre with its pixel.
+
+use super::*;
+use photocraft_geom::Rect;
+
+/// An 800 × 600 white document, as in the issue's measurement.
+fn session() -> Session {
+    let mut s = Session::new();
+    s.execute("file.new", json!({"width": 800, "height": 600, "background": "white"})).unwrap();
+    s
+}
+
+fn positions(s: &mut Session) -> Vec<[f64; 2]> {
+    let l = s.execute("view.colorSamplers.list", json!({})).unwrap();
+    l["samplers"].as_array().unwrap().iter().map(|p| [p["position"][0].as_f64().unwrap(), p["position"][1].as_f64().unwrap()]).collect()
+}
+
+fn assert_near(got: &[[f64; 2]], want: &[[f64; 2]], what: &str) {
+    assert_eq!(got.len(), want.len(), "{what}: {got:?}");
+    for (g, w) in got.iter().zip(want) {
+        assert!((g[0] - w[0]).abs() < 1e-9 && (g[1] - w[1]).abs() < 1e-9, "{what}: got {got:?}, want {want:?}");
+    }
+}
+
+#[test]
+fn a_point_inside_a_pixel_is_stored_and_listed_at_its_centre() {
+    let mut s = session();
+    let r = s.execute("view.colorSamplers.add", json!({"x": 199.3, "y": 149.6})).unwrap();
+    assert_eq!(r["point"]["position"], json!([199.5, 149.5]));
+    for (x, y) in [(199.0, 149.0), (199.99, 149.01)] {
+        s.execute("view.colorSamplers.add", json!({"x": x, "y": y})).unwrap();
+    }
+    assert_near(&positions(&mut s), &[[199.5, 149.5]; 3], "anywhere in pixel 199,149");
+    // The last pixel of the canvas.
+    s.execute("view.colorSamplers.clear", json!({})).unwrap();
+    s.execute("view.colorSamplers.add", json!({"x": 799.7, "y": 598.2})).unwrap();
+    assert_near(&positions(&mut s), &[[799.5, 598.5]], "the last pixel");
+}
+
+#[test]
+fn moving_a_point_snaps_it_to_the_pixel_centre() {
+    let mut s = session();
+    s.execute("view.colorSamplers.add", json!({"x": 10, "y": 10})).unwrap();
+    let r = s.execute("view.colorSamplers.move", json!({"index": 0, "x": 352.9, "y": 150.2})).unwrap();
+    assert_near(&positions(&mut s), &[[352.5, 150.5]], &format!("moved: {r}"));
+    // Off the canvas is still refused and the point stays put.
+    assert!(s.execute("view.colorSamplers.move", json!({"index": 0, "x": 800.0, "y": 10.0})).is_err());
+    assert_near(&positions(&mut s), &[[352.5, 150.5]], "after a refused move");
+}
+
+#[test]
+fn the_readout_reads_the_pixel_the_centre_is_on() {
+    let mut s = session();
+    // Pixel 199,149 red, its lower-right neighbour 200,150 blue.
+    s.edit("paint", |doc, _| {
+        let surf = doc.layers[0].surface_mut().unwrap();
+        let red = photocraft_raster::from_rgba(&surf.format(), [1.0, 0.0, 0.0, 1.0]);
+        let blue = photocraft_raster::from_rgba(&surf.format(), [0.0, 0.0, 1.0, 1.0]);
+        surf.fill_rect(Rect::new(199, 149, 200, 150), &red);
+        surf.fill_rect(Rect::new(200, 150, 201, 151), &blue);
+        Ok(())
+    })
+    .unwrap();
+    let r = s.execute("view.colorSamplers.add", json!({"x": 199.3, "y": 149.6})).unwrap();
+    assert_eq!(r["point"]["rgb"], json!([255, 0, 0]), "199.5,149.5 reads pixel 199,149");
+    let l = s.execute("view.colorSamplers.list", json!({})).unwrap();
+    assert_eq!(l["samplers"][0]["rgb"], json!([255, 0, 0]));
+}
+
+/// The issue's two measured points (#1 on pixel 239,169 and #2 on pixel 352,150) after `id`.
+fn after(id: &str, p: Value) -> Vec<[f64; 2]> {
+    let mut s = session();
+    s.execute("view.colorSamplers.add", json!({"x": 239.4, "y": 169.8})).unwrap();
+    s.execute("view.colorSamplers.add", json!({"x": 352.1, "y": 150.0})).unwrap();
+    assert_near(&positions(&mut s), &[[239.5, 169.5], [352.5, 150.5]], "placed");
+    s.execute(id, p).unwrap();
+    positions(&mut s)
+}
+
+#[test]
+fn document_operations_move_the_centre_as_measured() {
+    let cases = [
+        ("image.canvasSize", json!({"width": 1000, "height": 800, "anchor": "center"}), [[339.5, 269.5], [452.5, 250.5]]),
+        ("image.imageRotation.90cw", json!({}), [[430.5, 239.5], [449.5, 352.5]]),
+        ("image.rotation.arbitrary", json!({"angle": 90, "direction": "cw"}), [[430.5, 239.5], [449.5, 352.5]]),
+        ("image.imageRotation.flipCanvasHorizontal", json!({}), [[560.5, 169.5], [447.5, 150.5]]),
+        // Scaled with the pixels, not snapped back to a pixel centre.
+        ("image.imageSize", json!({"width": 400, "height": 300}), [[119.75, 84.75], [176.25, 75.25]]),
+        ("image.crop", json!({"x": 100, "y": 100, "width": 400, "height": 300}), [[139.5, 69.5], [252.5, 50.5]]),
+    ];
+    for (id, p, want) in cases {
+        assert_near(&after(id, p), &want, id);
+    }
+}
+
+#[test]
+fn points_outside_the_new_canvas_are_still_dropped() {
+    let mut s = session();
+    s.execute("view.colorSamplers.add", json!({"x": 239, "y": 169})).unwrap();
+    s.execute("view.colorSamplers.add", json!({"x": 650, "y": 500})).unwrap();
+    // The last pixel inside the crop stays; the one just outside goes.
+    s.execute("view.colorSamplers.add", json!({"x": 499, "y": 399})).unwrap();
+    s.execute("view.colorSamplers.add", json!({"x": 500, "y": 399})).unwrap();
+    s.execute("image.crop", json!({"x": 100, "y": 100, "width": 400, "height": 300})).unwrap();
+    assert_near(&positions(&mut s), &[[139.5, 69.5], [399.5, 299.5]], "after the crop");
+    assert_eq!(s.take_samplers_removed(), 2);
+}

crates/engine/src/cmd/sampler_cmds/tests.rs

@@ -25,20 +25,20 @@ fn add_reads_the_composite_pixel_and_lists_it() {
     assert_eq!(r["index"], 0);
     assert_eq!(r["count"], 1);
     assert_eq!(r["point"]["number"], 1);
-    assert_eq!(r["point"]["position"], json!([5.0, 6.0]));
+    assert_eq!(r["point"]["position"], json!([5.5, 6.5]));
     assert_eq!(r["point"]["rgb"], json!([255, 255, 255]));
     assert_eq!(r["point"]["cmyk"], json!([0, 0, 0, 0]));
     let l = list(&mut s);
     assert_eq!(l["max"], 10);
     assert_eq!(l["samplers"].as_array().unwrap().len(), 1);
-    assert_eq!(l["samplers"][0]["position"], json!([5.0, 6.0]));
+    assert_eq!(l["samplers"][0]["position"], json!([5.5, 6.5]));
 }
 
 #[test]
-fn fractional_clicks_floor_to_the_pixel() {
+fn fractional_clicks_snap_to_the_pixel_centre() {
     let mut s = session();
     let r = s.execute("view.colorSamplers.add", json!({"x": 5.9, "y": 6.1})).unwrap();
-    assert_eq!(r["point"]["position"], json!([5.0, 6.0]));
+    assert_eq!(r["point"]["position"], json!([5.5, 6.5]));
 }
 
 #[test]
@@ -84,12 +84,12 @@ fn move_delete_and_clear() {
     s.execute("view.colorSamplers.add", json!({"x": 5, "y": 5})).unwrap();
     s.execute("view.colorSamplers.add", json!({"x": 50, "y": 50})).unwrap();
     let r = s.execute("view.colorSamplers.move", json!({"index": 0, "x": 10, "y": 20})).unwrap();
-    assert_eq!(r["point"]["position"], json!([10.0, 20.0]));
-    assert_eq!(points(&mut s)[0]["position"], json!([10.0, 20.0]));
+    assert_eq!(r["point"]["position"], json!([10.5, 20.5]));
+    assert_eq!(points(&mut s)[0]["position"], json!([10.5, 20.5]));
     s.execute("view.colorSamplers.delete", json!({"index": 1})).unwrap();
     assert_eq!(points(&mut s).len(), 1);
     // The remaining point is the moved #1.
-    assert_eq!(points(&mut s)[0]["position"], json!([10.0, 20.0]));
+    assert_eq!(points(&mut s)[0]["position"], json!([10.5, 20.5]));
     let r = s.execute("view.colorSamplers.clear", json!({})).unwrap();
     assert_eq!(r["deleted"], 1);
     assert!(points(&mut s).is_empty());
@@ -132,7 +132,7 @@ fn a_crop_drops_points_outside_the_new_canvas() {
     s.execute("image.crop", json!({"x": 0, "y": 0, "width": 10, "height": 10})).unwrap();
     let p = points(&mut s);
     assert_eq!(p.len(), 1, "the outside point was removed");
-    assert_eq!(p[0]["position"], json!([5.0, 5.0]));
+    assert_eq!(p[0]["position"], json!([5.5, 5.5]));
     assert_eq!(s.take_samplers_removed(), 1, "one removed point reported");
     assert_eq!(s.take_samplers_removed(), 0, "reported once");
 }
@@ -145,7 +145,7 @@ fn a_crop_moves_points_with_their_pixels() {
     s.execute("image.crop", json!({"x": 40, "y": 30, "width": 20, "height": 20})).unwrap();
     let p = points(&mut s);
     assert_eq!(p.len(), 1);
-    assert_eq!(p[0]["position"], json!([10.0, 10.0]), "moved by (-40, -30)");
+    assert_eq!(p[0]["position"], json!([10.5, 10.5]), "moved by (-40, -30)");
     assert_eq!(s.take_samplers_removed(), 0, "nothing removed");
 }
 
@@ -157,12 +157,13 @@ fn a_canvas_size_shift_moves_points() {
     s.execute("image.canvasSize", json!({"width": 120, "height": 100, "anchor": "bottomRight"})).unwrap();
     let p = points(&mut s);
     assert_eq!(p.len(), 1);
-    assert_eq!(p[0]["position"], json!([50.0, 50.0]), "moved by the canvas shift");
+    assert_eq!(p[0]["position"], json!([50.5, 50.5]), "moved by the canvas shift");
 }
 
 #[test]
 fn a_right_angle_rotation_moves_points() {
-    // 90° clockwise about the centre: pixel (x, y) of the 100×80 canvas goes to (79 - y, x).
+    // 90° clockwise about the centre: pixel (x, y) of the 100×80 canvas goes to (79 - y, x), and
+    // the point stays on that pixel's centre.
     let mut s = session();
     s.execute("view.colorSamplers.add", json!({"x": 50, "y": 40})).unwrap();
     s.execute("image.rotation.arbitrary", json!({"angle": 90, "direction": "cw"})).unwrap();
@@ -170,7 +171,7 @@ fn a_right_angle_rotation_moves_points() {
     assert_eq!(p.len(), 1);
     let pos = p[0]["position"].as_array().unwrap();
     let (x, y) = (pos[0].as_f64().unwrap(), pos[1].as_f64().unwrap());
-    assert!((x - 39.0).abs() < 1e-6 && (y - 50.0).abs() < 1e-6, "{pos:?}");
+    assert!((x - 39.5).abs() < 1e-6 && (y - 50.5).abs() < 1e-6, "{pos:?}");
 }
 
 #[test]
@@ -197,7 +198,7 @@ fn flips_and_right_angle_turns_keep_points_on_their_pixel() {
         let (x, y) = turn.pixel_map(100, 80)(3, 3);
         let p = points(&mut s);
         assert_eq!(p.len(), 1, "{id}");
-        assert_eq!(p[0]["position"], json!([f64::from(x), f64::from(y)]), "{id}");
+        assert_eq!(p[0]["position"], json!([f64::from(x) + 0.5, f64::from(y) + 0.5]), "{id}");
         assert_eq!(p[0]["rgb"], json!([0, 0, 0]), "{id}");
     }
 }
@@ -205,7 +206,7 @@ fn flips_and_right_angle_turns_keep_points_on_their_pixel() {
 #[test]
 fn an_arbitrary_rotation_keeps_points_on_their_pixel() {
     // The pixel at (3, 3) of the 100×80 document, and where each turn moves it.
-    for (angle, direction, at) in [(90, "cw", [76.0, 3.0]), (90, "ccw", [3.0, 96.0]), (180, "cw", [96.0, 76.0])] {
+    for (angle, direction, at) in [(90, "cw", [76.5, 3.5]), (90, "ccw", [3.5, 96.5]), (180, "cw", [96.5, 76.5])] {
         let mut s = session();
         s.edit("paint", |doc, _| {
             let surf = doc.layers[0].surface_mut().unwrap();
@@ -240,15 +241,15 @@ fn a_rotated_crop_moves_points() {
     s.execute("image.crop", json!({"x": 40, "y": 30, "width": 20, "height": 20, "angle": 90})).unwrap();
     let p = points(&mut s);
     assert_eq!(p.len(), 1);
-    assert_eq!(p[0]["position"], json!([10.0, 9.0]));
+    assert_eq!(p[0]["position"], json!([10.5, 9.5]));
     assert_eq!(p[0]["rgb"], json!([0, 0, 0]));
 }
 
 #[test]
 fn a_crop_to_a_target_size_scales_points() {
-    // The crop resamples the 20 x 20 area to 40 x 40: the point (10, 10 after the shift; 10, 9
-    // turned 90°, as in `a_rotated_crop_moves_points`) doubles.
-    for (angle, at) in [(None, [20.0, 20.0]), (Some(90), [20.0, 18.0])] {
+    // The crop resamples the 20 x 20 area to 40 x 40: the point (10.5, 10.5 after the shift;
+    // 10.5, 9.5 turned 90°, as in `a_rotated_crop_moves_points`) doubles, as Image Size scales it.
+    for (angle, at) in [(None, [21.0, 21.0]), (Some(90), [21.0, 19.0])] {
         let mut s = session();
         s.execute("view.colorSamplers.add", json!({"x": 50, "y": 40})).unwrap();
         let mut p = json!({"x": 40, "y": 30, "width": 20, "height": 20, "targetWidth": 40, "targetHeight": 40});
@@ -347,7 +348,7 @@ fn points_work_at_every_depth_and_mode() {
 fn document_helpers_are_not_disturbed() {
     let mut d = Document::new("t", Size::new(4, 4), ColorMode::Rgb, SampleType::U8);
     d.layers.push(Layer::raster("Background", d.pixel_format()));
-    assert_eq!(pixel(&d, 0.0, 0.0), Some([0.0, 0.0]));
-    assert_eq!(pixel(&d, 3.9, 3.9), Some([3.0, 3.0]));
+    assert_eq!(pixel(&d, 0.0, 0.0), Some([0.5, 0.5]));
+    assert_eq!(pixel(&d, 3.9, 3.9), Some([3.5, 3.5]));
     assert_eq!(pixel(&d, 4.0, 0.0), None);
 }

crates/engine/src/session/execute.rs

@@ -291,7 +291,9 @@ impl Session {
         // state: not a history step. A mapped edit (the four above) gives the exact map; any other
         // size change falls back to dropping whatever is now outside.
         let mut samplers_removed = 0;
-        if let Some(a) = sampler_map {
+        // The points sit on pixel centres (#11725), so they follow the geometry map, the one the
+        // symmetry gets: `sampler_map` is that map shifted for pixel indices (a flip or rotation).
+        if let Some(a) = sampler_map.and(symmetry_map) {
             let bounds = st.doc.bounds();
             let n = st.color_samplers.len();
             st.color_samplers.retain_mut(|p| {

crates/ui-egui/src/panels/info.rs

@@ -112,7 +112,8 @@ pub(super) fn info_panel(app: &mut PhotocraftApp, ui: &mut egui::Ui) {
             let (rgb, cmyk) = (&s["rgb"], &s["cmyk"]);
             ui.horizontal(|ui| {
                 ui.add_sized([24.0, 16.0], egui::Label::new(RichText::new(format!("#{n}")).color(t.text_dim).size(11.5)));
-                cell(ui, format!("X: {}  Y: {}", fx(at.0), fy(at.1)));
+                // The point sits on its pixel's centre (#11725); the row names the pixel, as the pointer row does.
+                cell(ui, format!("X: {}  Y: {}", fx(at.0.floor()), fy(at.1.floor())));
             });
             ui.horizontal(|ui| {
                 ui.add_sized([24.0, 16.0], egui::Label::new(RichText::new("").size(11.5)));

crates/ui-egui/src/tools/analysis_ui/tests.rs

@@ -203,16 +203,16 @@ fn color_sampler_tool_places_moves_and_removes_points() {
     up(&mut app, 20.0, 20.0);
     down(&mut app, 60.0, 40.0, egui::Modifiers::NONE);
     up(&mut app, 60.0, 40.0);
-    assert_eq!(pts(&app), vec![[20.0, 20.0], [60.0, 40.0]]);
+    assert_eq!(pts(&app), vec![[20.5, 20.5], [60.5, 40.5]], "each on its pixel's centre");
     // Drag the first point to a new pixel (committed on release).
     down(&mut app, 20.0, 20.0, egui::Modifiers::NONE);
     mv(&mut app, 30.0, 25.0, egui::Modifiers::NONE);
     up(&mut app, 30.0, 25.0);
-    assert_eq!(pts(&app)[0], [30.0, 25.0]);
+    assert_eq!(pts(&app)[0], [30.5, 25.5]);
     // A click on a point, with no drag, leaves it where it is.
     down(&mut app, 30.0, 25.0, egui::Modifiers::NONE);
     up(&mut app, 30.0, 25.0);
-    assert_eq!(pts(&app)[0], [30.0, 25.0]);
+    assert_eq!(pts(&app)[0], [30.5, 25.5]);
     // ⌥-click removes that one point.
     down(&mut app, 60.0, 40.0, egui::Modifiers::ALT);
     up(&mut app, 60.0, 40.0);
@@ -251,11 +251,11 @@ fn color_sampler_readout_and_crop_removal_report() {
     let l = app.run("view.colorSamplers.list", json!({})).unwrap();
     assert_eq!(l["samplers"].as_array().unwrap().len(), 2);
     assert_eq!(l["samplers"][0]["number"], 1);
-    assert_eq!(l["samplers"][1]["position"], json!([150.0, 90.0]));
+    assert_eq!(l["samplers"][1]["position"], json!([150.5, 90.5]));
     assert!(l["samplers"][0]["rgb"].as_array().is_some_and(|a| a.len() == 3));
     // Cropping to the top-left drops the outside point and reports how many went.
     app.run("image.crop", json!({"x": 0, "y": 0, "width": 10, "height": 10})).unwrap();
-    assert_eq!(app.session.active().unwrap().color_samplers, vec![[5.0, 5.0]]);
+    assert_eq!(app.session.active().unwrap().color_samplers, vec![[5.5, 5.5]]);
     assert!(app.ui.status.contains("removed"), "{}", app.ui.status);
     assert!(!app.ui.status_error);
 }
@@ -271,9 +271,9 @@ fn color_sampler_is_per_document_and_inert_without_one() {
     assert!(app.session.active().unwrap().color_samplers.is_empty());
     down(&mut app, 20.0, 20.0, egui::Modifiers::NONE);
     up(&mut app, 20.0, 20.0);
-    assert_eq!(app.session.active().unwrap().color_samplers, vec![[20.0, 20.0]]);
+    assert_eq!(app.session.active().unwrap().color_samplers, vec![[20.5, 20.5]]);
     app.run("document.activate", json!({"document": 0})).unwrap();
-    assert_eq!(app.session.active().unwrap().color_samplers, vec![[10.0, 10.0]]);
+    assert_eq!(app.session.active().unwrap().color_samplers, vec![[10.5, 10.5]]);
     // With no document the tool says so (no dialog, no panic) and the commands error.
     let mut empty = PhotocraftApp::new(photocraft_engine::Session::new(), crate::Services::default());
     empty.ui.tool = Tool::ColorSampler;