From e9e06c5a15b4afed43f9d3d7eb25a250c2c78de2 Mon Sep 17 00:00:00 2001 From: huzaifffa Date: Thu, 29 Oct 2026 12:49:23 +0300 Subject: [PATCH 01/15] feat(labels): callouts, show all labels, and a Move Label tool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three label options, as QGIS has them: - Show all labels (per class): every label is drawn, over the others where they crowd, rather than some left out. - Callouts (per class, with a longest line, 5 to 500 points): a label crowded off its spot moves to the nearest free spot whose line back to its feature is that short at most, searched out in rings, inside the view and off the feature; the line is drawn from the feature to the text's nearest edge in the label's colour, over its halo, in the map view and in exports. With Show all labels too, a free spot nearby comes before overlapping. Labels along lines aren't moved. - Move Label (a map tool, Labeling tab › Label Placement): drag a label to where it should be. It's kept by its feature's id in longitude and latitude (LabelClass::moved, saved with the project), drawn through the map's projection as its feature is, placed first and over what's under it, and given a callout's line however far it went when its class has callouts. Shift-click puts one back; Reset Moved puts back a layer's. Labels carry their callout and what they label (layer, feature, class) in rustgis_tiles::Collide, which both renderers draw from. Commands: labels.placement takes "showAll" and "callout"; labels.move and labels.resetMoved are new; the tool is "moveLabel". Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/labels.rs | 77 +++++++- crates/engine/src/lib.rs | 6 + crates/engine/src/tests.rs | 86 +++++++++ crates/map/src/qgis/labels.rs | 3 + crates/map/src/symbology.rs | 117 ++++++++++++ crates/render/src/lib.rs | 315 +++++++++++++++++++++++++++++++- crates/render/src/text.rs | 23 +++ crates/tiles/src/symbols.rs | 21 +++ crates/ui-egui/src/labelpane.rs | 59 ++++++ crates/ui-egui/src/mapview.rs | 93 ++++++++++ crates/ui-egui/src/ribbon.rs | 18 ++ docs/user-guide/cartography.md | 17 ++ 12 files changed, 825 insertions(+), 10 deletions(-) diff --git a/crates/engine/src/cmd/labels.rs b/crates/engine/src/cmd/labels.rs index 821bfa78a..45ab75bf1 100644 --- a/crates/engine/src/cmd/labels.rs +++ b/crates/engine/src/cmd/labels.rs @@ -132,8 +132,8 @@ pub fn commands() -> Vec { C::new("labels.placement", "Label Placement", "Labeling › Label Placement", |s, p| { let pos = match ps(p, "position") { Some(code) => Some(LabelPlacement::from_code(code).ok_or_else(|| format!("unknown placement '{code}'"))?), - None if p.get("offset").is_some() || p.get("repeat").is_some() => None, - None => return Err("position is required (or an offset or a repeat)".into()), + None if ["offset", "repeat", "showAll", "callout"].iter().any(|k| p.get(k).is_some()) => None, + None => return Err("position is required (or an offset, a repeat, showAll or a callout)".into()), }; let offset = match p.get("offset") { None => None, @@ -149,6 +149,28 @@ pub fn commands() -> Vec { _ => return Err("repeat must be a number of points, or 0 or null for none".into()), }, }; + // Every label shown, over the others where they crowd. + let show_all = match p.get("showAll") { + None => None, + Some(v) => Some(v.as_bool().ok_or("showAll must be true or false")?), + }; + // Callouts: on (true: its longest line as it was, or the default), on with this + // longest line (points), or off (false, 0 or null). + enum Callout { + Off, + On, + Reach(f32), + } + let callout = match p.get("callout") { + None => None, + Some(Value::Null | Value::Bool(false)) => Some(Callout::Off), + Some(Value::Bool(true)) => Some(Callout::On), + Some(v) => match number(v) { + Some(0.0) => Some(Callout::Off), + Some(r) if r > 0.0 => Some(rustgis_map::symbology::callout_reach(r).map_or(Callout::Off, Callout::Reach)), + _ => return Err("callout must be true or false, or its longest line in points".into()), + }, + }; with_class(s, p, |c| { if let Some(pos) = pos { c.placement = pos; @@ -159,12 +181,61 @@ pub fn commands() -> Vec { if let Some(r) = repeat { c.repeat = r; } + if let Some(a) = show_all { + c.show_all = a; + } + match callout { + Some(Callout::Off) => c.callout = None, + Some(Callout::On) => c.callout = Some(c.callout.unwrap_or(rustgis_map::symbology::DEFAULT_CALLOUT)), + Some(Callout::Reach(r)) => c.callout = Some(r), + None => {} + } }) }) .when(need_feature_layer) .params( - r#"{"layer"?, "class"?: name (none: the Default class, by whatever name it has), "position"?: "auto"|"center"|"above"|"below"|"left"|"right"|"aboveRight"|"aboveLeft"|"belowRight"|"belowLeft"|"parallel" (along a line, straight)|"curved" (along a line, following its bends) (required without offset or repeat), "offset"?: [right, down] points (-200 to 200 each) the label moves from where its position puts it ([0, 0]: none; a label along a line isn't moved), "repeat"?: points (10 to 10000) between a line's labels, spaced out from the one at its middle, at most 32 a line (0 or null: the one at its middle; parallel and curved only)}"#, + r#"{"layer"?, "class"?: name (none: the Default class, by whatever name it has), "position"?: "auto"|"center"|"above"|"below"|"left"|"right"|"aboveRight"|"aboveLeft"|"belowRight"|"belowLeft"|"parallel" (along a line, straight)|"curved" (along a line, following its bends) (required without offset or repeat), "offset"?: [right, down] points (-200 to 200 each) the label moves from where its position puts it ([0, 0]: none; a label along a line isn't moved), "repeat"?: points (10 to 10000) between a line's labels, spaced out from the one at its middle, at most 32 a line (0 or null: the one at its middle; parallel and curved only), "showAll"?: bool (every label drawn, over the others where they crowd), "callout"?: true|false|points (5 to 500: a label crowded off its spot moves to a free one nearby, a line drawn back to its feature at most this long; true keeps its length, or 40; false, 0 or null: none; not labels along a line)}"#, ), + // Labels moved by hand (the Move Label tool, as QGIS's): kept by their feature's id. + C::new("labels.move", "Move Label", "Labeling › Label Placement", |s, p| { + let feature = p.get("feature").and_then(Value::as_u64).ok_or("feature is a feature's id")?; + let to = match p.get("to") { + None => return Err("to is [x, y] (longitude, latitude), or null to put it back".into()), + Some(Value::Null) => None, + Some(v) => { + let a = v.as_array().filter(|a| a.len() == 2).ok_or("to is [x, y] (longitude, latitude)")?; + let xy = [a.first().and_then(number), a.get(1).and_then(number)]; + match xy { + [Some(x), Some(y)] if x.is_finite() && y.is_finite() && x.abs() <= 540.0 && y.abs() <= 90.0 => Some([x, y]), + _ => return Err("to is [x, y]: a longitude and a latitude".into()), + } + } + }; + with_labels(s, p, |labels| { + let c = class_in(labels, p)?; + if c.move_label(feature, to) { + Ok(()) + } else { + Err(format!("a label class keeps at most {} labels moved", rustgis_map::symbology::MAX_MOVED_LABELS)) + } + }) + }) + .when(need_feature_layer) + .params(r#"{"layer"?, "class"?: name (none: the Default class), "feature": the feature's id, "to": [x, y] (longitude, latitude: where the label's text is centred) | null (back where its class puts it)}"#), + C::new("labels.resetMoved", "Reset Moved Labels", "Labeling › Label Placement", |s, p| { + let all = b(p, "all").unwrap_or(false); + with_labels(s, p, |labels| { + if all { + labels.default.moved.clear(); + labels.classes.iter_mut().for_each(|c| c.moved.clear()); + } else { + class_in(labels, p)?.moved.clear(); + } + Ok(()) + }) + }) + .when(need_feature_layer) + .params(r#"{"layer"?, "class"?: name (none: the Default class), "all"?: bool (every class of the layer)}"#), // Label classes: each labels every feature its clause takes, as in ArcGIS Pro; the classes // listed first, Default last, are placed first (`Labeling`, #732). C::new("labels.class.add", "Add Label Class", "View › Windows › Labeling", add_class) diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index ed1a67b29..762e29579 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -290,6 +290,9 @@ pub enum Tool { CreatePolygon, /// Moves, inserts and deletes the vertices of the selected features. EditVertices, + /// Drags a layer's label to where it should be (`labels.move`), as QGIS's Move Label tool; + /// dragging anything else pans the map. + MoveLabel, /// Draws a graphic: the shape is one of [`GRAPHIC_TOOLS`]' shapes. Graphic(&'static str), } @@ -324,6 +327,7 @@ impl Tool { Tool::CreateLine => "createLine", Tool::CreatePolygon => "createPolygon", Tool::EditVertices => "editVertices", + Tool::MoveLabel => "moveLabel", Tool::Graphic(shape) => GRAPHIC_TOOLS.iter().find(|g| g.0 == shape).map_or("graphicPoint", |g| g.1), } } @@ -344,6 +348,7 @@ impl Tool { Tool::CreateLine, Tool::CreatePolygon, Tool::EditVertices, + Tool::MoveLabel, ] .into_iter() .find(|t| t.code().eq_ignore_ascii_case(s.trim())) @@ -362,6 +367,7 @@ impl Tool { Tool::CreateLine => "Create Line", Tool::CreatePolygon => "Create Polygon", Tool::EditVertices => "Edit Vertices", + Tool::MoveLabel => "Move Label", Tool::Graphic(shape) => GRAPHIC_TOOLS.iter().find(|g| g.0 == shape).map_or("Point", |g| g.2), } } diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 961f72a83..cb2bdbd18 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -2343,6 +2343,92 @@ fn labels_take_a_font_by_family_name() { /// A label class's halo takes its own colour and width, and its letters a spacing; blank and null /// go back to the automatic colour and the thin halo, and other settings leave them. +#[test] +fn labels_are_moved_by_command_and_reset() { + let mut s = world(); + let id = s.map().layer_by_name("Cities").unwrap().id; + let fid = s.map().layer(id).unwrap().dataset.features.first().unwrap().id; + let moved = |s: &Session| s.map().layer(id).unwrap().labels.default.moved.clone(); + s.run("labels.move", &json!({"layer": id, "feature": fid, "to": [12.5, 41.9]})).unwrap(); + assert_eq!(moved(&s), vec![rustgis_map::symbology::MovedLabel { id: fid, at: [12.5, 41.9] }]); + // By the class's name too (Default's); again moves it again. + let name = s.map().layer(id).unwrap().labels.default.name.clone(); + s.run("labels.move", &json!({"layer": id, "class": name, "feature": fid, "to": [13.0, 42.0]})).unwrap(); + assert_eq!(moved(&s).len(), 1); + // Bad values change nothing. + for bad in [ + json!({"feature": fid}), + json!({"feature": "one", "to": [1, 1]}), + json!({"feature": fid, "to": [1]}), + json!({"feature": fid, "to": [1, 200]}), + json!({"feature": fid, "to": ["a", 1]}), + json!({"feature": fid, "to": [1, 1], "class": "No such class"}), + ] { + let mut p = bad.clone(); + p["layer"] = json!(id); + assert!(s.run("labels.move", &p).is_err(), "{bad}"); + } + assert_eq!(moved(&s)[0].at, [13.0, 42.0]); + // Saved with the project's layer, and one undo step a move. + let saved = serde_json::to_value(&s.map().layer(id).unwrap().labels).unwrap(); + assert_eq!(saved["moved"], json!([{"id": fid, "at": [13.0, 42.0]}])); + s.run("edit.undo", &json!({})).unwrap(); + assert_eq!(moved(&s)[0].at, [12.5, 41.9]); + // Put back: one, then all. + s.run("labels.move", &json!({"layer": id, "feature": fid, "to": null})).unwrap(); + assert!(moved(&s).is_empty()); + s.run("labels.move", &json!({"layer": id, "feature": fid, "to": [1, 1]})).unwrap(); + s.run("labels.resetMoved", &json!({"layer": id, "all": true})).unwrap(); + assert!(moved(&s).is_empty()); + // The tool by its code. + s.run("tool.set", &json!({"tool": "moveLabel"})).unwrap(); + assert_eq!(s.tool, crate::Tool::MoveLabel); +} + +#[test] +fn labels_show_all_and_take_callouts() { + let mut s = world(); + let class = |s: &Session| { + let c = &s.map().layer_by_name("Cities").unwrap().labels.default; + (c.show_all, c.callout) + }; + assert_eq!(class(&s), (false, None)); + s.run("labels.placement", &json!({"layer": "Cities", "showAll": true})).unwrap(); + assert_eq!(class(&s), (true, None), "no position needed"); + // On: the default length, then one given, kept within range; on again keeps it. + s.run("labels.placement", &json!({"layer": "Cities", "callout": true})).unwrap(); + assert_eq!(class(&s).1, Some(rustgis_map::symbology::DEFAULT_CALLOUT)); + s.run("labels.placement", &json!({"layer": "Cities", "callout": 80})).unwrap(); + assert_eq!(class(&s).1, Some(80.0)); + s.run("labels.placement", &json!({"layer": "Cities", "callout": 99999})).unwrap(); + assert_eq!(class(&s).1, Some(500.0), "within its range"); + s.run("labels.placement", &json!({"layer": "Cities", "callout": true})).unwrap(); + assert_eq!(class(&s).1, Some(500.0), "on again keeps its length"); + // Saved with the layer, and read back. + let saved = serde_json::to_value(&s.map().layer_by_name("Cities").unwrap().labels).unwrap(); + assert_eq!((saved["show_all"].clone(), saved["callout"].clone()), (json!(true), json!(500.0))); + let back: rustgis_map::symbology::Labeling = serde_json::from_value(saved).unwrap(); + assert_eq!((back.default.show_all, back.default.callout), (true, Some(500.0))); + // A project file's out-of-range length is brought within range. + let mut wild = serde_json::to_value(&back).unwrap(); + wild["callout"] = json!(1e9); + let read: rustgis_map::symbology::Labeling = serde_json::from_value(wild).unwrap(); + assert_eq!(read.default.callout, Some(500.0)); + // Off, three ways; bad values change nothing. + for off in [json!(false), json!(0), json!(null)] { + s.run("labels.placement", &json!({"layer": "Cities", "callout": 40})).unwrap(); + s.run("labels.placement", &json!({"layer": "Cities", "callout": off})).unwrap(); + assert_eq!(class(&s).1, None, "{off}"); + } + for bad in [json!({"callout": "long"}), json!({"callout": -5}), json!({"showAll": "yes"})] { + let mut p = bad.clone(); + p["layer"] = json!("Cities"); + assert!(s.run("labels.placement", &p).is_err(), "{bad}"); + } + s.run("edit.undo", &json!({})).unwrap(); + assert_eq!(class(&s), (true, Some(40.0)), "undoes"); +} + #[test] fn labels_take_a_halo_colour_and_width_and_letter_spacing() { use rustgis_map::Rgba; diff --git a/crates/map/src/qgis/labels.rs b/crates/map/src/qgis/labels.rs index 431dc4907..7078d2ac3 100644 --- a/crates/map/src/qgis/labels.rs +++ b/crates/map/src/qgis/labels.rs @@ -252,6 +252,9 @@ fn class( wrap_length, line_align, repeat, + show_all: false, + callout: None, + moved: Vec::new(), }) } diff --git a/crates/map/src/symbology.rs b/crates/map/src/symbology.rs index 7b4b714ba..18d1c7710 100644 --- a/crates/map/src/symbology.rs +++ b/crates/map/src/symbology.rs @@ -1881,6 +1881,95 @@ pub struct LabelClass { /// the one at its middle, as QGIS's repeat distance; none is the one at its middle. #[serde(default, deserialize_with = "de_repeat", skip_serializing_if = "Option::is_none")] pub repeat: Option, + /// Every label drawn, over the others where they crowd, rather than some left out. + #[serde(default, skip_serializing_if = "std::ops::Not::not")] + pub show_all: bool, + /// Callouts: a label crowded off its spot moves to a free one nearby, a line drawn from its + /// feature to it at most this long (points, within [`CALLOUT_REACH`]), as QGIS's callouts; + /// none keeps every label in its spot. Labels along lines aren't moved. + #[serde(default, deserialize_with = "de_callout", skip_serializing_if = "Option::is_none")] + pub callout: Option, + /// Labels moved by hand (the Move Label tool, as QGIS's): each feature's, by its id, drawn + /// where it was put, a callout's line back to the feature when the class has callouts. At + /// most [`MAX_MOVED_LABELS`]. + #[serde(default, deserialize_with = "de_moved", skip_serializing_if = "Vec::is_empty")] + pub moved: Vec, +} + +/// A label moved by hand: its feature's id, and where its text is centred, in the layer's +/// coordinates (longitude and latitude), so it stays put on the map as the view moves. +#[derive(Clone, Copy, Debug, PartialEq, Serialize, Deserialize)] +pub struct MovedLabel { + pub id: u64, + pub at: [f64; 2], +} + +/// Most labels a class keeps moved by hand. +pub const MAX_MOVED_LABELS: usize = 10_000; + +/// A project file's moved labels: those with a place that is a number, one a feature (the last +/// wins), at most [`MAX_MOVED_LABELS`]. +fn de_moved<'de, D: serde::Deserializer<'de>>(d: D) -> Result, D::Error> { + // Read one by one, so one that isn't a moved label leaves the others (and the file) readable. + let all: Vec = lenient::>>(d)? + .unwrap_or_default() + .into_iter() + .flatten() + .filter_map(|v| serde_json::from_value(v).ok()) + .collect(); + let mut out: Vec = Vec::new(); + for m in all.into_iter().filter(|m| m.at.iter().all(|v| v.is_finite())) { + if let Some(o) = out.iter_mut().find(|o| o.id == m.id) { + *o = m; + } else if out.len() < MAX_MOVED_LABELS { + out.push(m); + } + } + Ok(out) +} + +impl LabelClass { + /// Where feature `id`'s label was moved to by hand, if it was. + pub fn moved_to(&self, id: u64) -> Option<[f64; 2]> { + self.moved.iter().find(|m| m.id == id).map(|m| m.at) + } + + /// Keeps feature `id`'s label at `at` (longitude, latitude), or where its class puts it + /// again with none. False when `at` isn't a place, or the class already keeps the most. + pub fn move_label(&mut self, id: u64, at: Option<[f64; 2]>) -> bool { + match at { + None => { + self.moved.retain(|m| m.id != id); + true + } + Some(at) if !at.iter().all(|v| v.is_finite()) => false, + Some(at) => { + if let Some(m) = self.moved.iter_mut().find(|m| m.id == id) { + m.at = at; + true + } else if self.moved.len() < MAX_MOVED_LABELS { + self.moved.push(MovedLabel { id, at }); + true + } else { + false + } + } + } + } +} + +/// How long a callout's line may be (points): from about a letter to a few centimetres of paper. +pub const CALLOUT_REACH: std::ops::RangeInclusive = 5.0..=500.0; +/// A callout's longest line when none is given (points). +pub const DEFAULT_CALLOUT: f32 = 40.0; + +/// A callout's longest line as a class keeps it, within [`CALLOUT_REACH`]; none for no number. +pub fn callout_reach(v: f64) -> Option { + v.is_finite().then(|| v.clamp(*CALLOUT_REACH.start(), *CALLOUT_REACH.end()) as f32) +} + +fn de_callout<'de, D: serde::Deserializer<'de>>(d: D) -> Result, D::Error> { + Ok(lenient::(d)?.and_then(callout_reach)) } /// The longest wrap character kept (characters). @@ -2286,6 +2375,9 @@ impl LabelClass { wrap_length: 0, line_align: crate::layout::TextAlign::Left, repeat: None, + show_all: false, + callout: None, + moved: Vec::new(), } } @@ -3210,6 +3302,31 @@ mod tests { assert_eq!(e.label, "x".repeat(MAX_LABEL - 1)); } + #[test] + fn labels_are_moved_by_hand_and_put_back() { + let ds = rustgis_data::Dataset::new( + "d", + rustgis_data::GeometryType::Point, + vec![rustgis_data::Field::new("name", rustgis_data::FieldType::Text)], + ); + let mut c = LabelClass::default_for(&ds); + assert!(c.move_label(7, Some([10.0, 20.0])) && c.move_label(8, Some([1.0, 2.0]))); + assert!(c.move_label(7, Some([11.0, 21.0])), "moved again, the same feature's"); + assert_eq!((c.moved_to(7), c.moved_to(8), c.moved.len()), (Some([11.0, 21.0]), Some([1.0, 2.0]), 2)); + assert!(!c.move_label(9, Some([f64::NAN, 0.0])), "not a place"); + assert!(c.move_label(8, None) && c.moved_to(8).is_none(), "put back"); + // Read from a file: bad entries dropped, the rest kept, a feature's last place wins. + let mut v = serde_json::to_value(&c).unwrap(); + v["moved"] = serde_json::json!([{"id": 1, "at": [1, 1]}, {"id": "x"}, 7, {"id": 1, "at": [2, 2]}, {"id": 3, "at": [3, 3]}]); + let read: LabelClass = serde_json::from_value(v.clone()).unwrap(); + assert_eq!(read.moved, vec![MovedLabel { id: 1, at: [2.0, 2.0] }, MovedLabel { id: 3, at: [3.0, 3.0] }]); + v["moved"] = serde_json::json!("nonsense"); + assert!(serde_json::from_value::(v).unwrap().moved.is_empty(), "the rest still reads"); + // None moved: none saved. + let none = serde_json::to_value(LabelClass::default_for(&ds)).unwrap(); + assert!(none.get("moved").is_none()); + } + #[test] fn class_labels_are_saved_and_older_renderers_still_load() { let g = GeometryType::Point; diff --git a/crates/render/src/lib.rs b/crates/render/src/lib.rs index 439d0d1c4..be50385af 100644 --- a/crates/render/src/lib.rs +++ b/crates/render/src/lib.rs @@ -28,6 +28,7 @@ mod stroke_paths; pub mod svg; pub mod terrain; mod text; +pub use text::CALLOUT_WIDTH; pub mod tile_mesh; mod tiles; pub use tiles::{Live, live as live_tiles}; @@ -3499,11 +3500,26 @@ fn render_frame(map: &mut Map, view: &View, full_res_rasters: bool, backend: Bac } else { places.push((0, x as f32 + dx, y as f32 + dy, None, Vec::new())); } + // Moved by hand (the Move Label tool): one label, upright, where it was + // put, its feature known by its id. + let fid = layer.dataset.features.get(row).map(|f| f.id); + // (Kept as longitude and latitude: the flat map draws its projection's + // coordinates, as its features' `geoms` are; the globe draws longitude and + // latitude.) + let moved = fid + .and_then(|id| class.moved_to(id)) + .map(|at| Coord::new(at[0], at[1])) + .and_then(|c| if globe.is_some() { Some(c) } else { flat.forward_checked(c) }) + .and_then(|c| ctx.screen(c)); + if let Some((mx, my)) = moved { + places = vec![(0, mx as f32, my as f32, None, Vec::new())]; + } // Layers above first, then the classes listed first (#732), then bigger // polygons: a class (ci ≤ MAX_LABEL_CLASSES, 1e9 each) never outweighs a // layer (1e12), nor an area (≤ 1e8) a class. A line's repeats come after - // its label at the middle, nearest first (lines have no area). - let pri = (n_layers - li) as f64 * 1e12 - ci as f64 * 1e9 + area; + // its label at the middle, nearest first (lines have no area). Labels moved + // by hand come before all of them (1e15), so the others keep off them. + let pri = (n_layers - li) as f64 * 1e12 - ci as f64 * 1e9 + area + if moved.is_some() { 1e15 } else { 0.0 }; let last = places.len().saturating_sub(1); for (n, (i, lx, ly, angle, glyphs)) in places.into_iter().enumerate() { let key = (lid, label_row(row), ci, i); @@ -3531,7 +3547,21 @@ fn render_frame(map: &mut Map, view: &View, full_res_rasters: bool, backend: Bac font, line_align: class.line_align, angle, - collide: Default::default(), + collide: rustgis_tiles::Collide { + // Shown all: over the other labels where they crowd. + text_allow_overlap: class.show_all, + // Callouts: a label set by its feature (not along a line) + // may move off a crowded spot, a line drawn back to it. + callout_reach: if angle.is_none() && glyphs.is_empty() { + class.callout.unwrap_or(0.0) + } else { + 0.0 + }, + callout_anchor: point.is_some().then_some([x as f32 - lx, y as f32 - ly]), + pinned: moved.is_some(), + source: fid.map(|id| (lid, id, ci as u32)), + ..Default::default() + }, // (Not behind characters curved one by one.) background: class .background @@ -4277,6 +4307,89 @@ struct Placement { keys: Vec, } +/// Labels a placement looks for a callout's spot for, at most (each looks at up to a few thousand +/// spots). +const MAX_CALLOUT_SEARCHES: usize = 500; + +/// A label's text's extent (points, `[x0, y0, x1, y1]`), as placement keeps it clear of the +/// others: what the Move Label tool picks it by. +pub fn label_extent(l: &PlacedLabel) -> Option<[f32; 4]> { + let (text, _) = label_boxes(l); + let e = + text.iter().fold([f32::MAX, f32::MAX, f32::MIN, f32::MIN], |e, b| [e[0].min(b[0]), e[1].min(b[1]), e[2].max(b[2]), e[3].max(b[3])]); + (e[0] <= e[2] && e[1] <= e[3]).then_some(e) +} + +/// The callout's line a label would have where it is now, as offsets from its `x`, `y`: from its +/// feature (`collide.callout_anchor`) to its text's nearest edge, when its class has callouts. +/// For showing a label being moved by hand. +pub fn callout_to(l: &PlacedLabel) -> Option<[f32; 4]> { + let [ax, ay] = l.collide.callout_anchor.filter(|_| l.collide.callout_reach > 0.0)?; + let line = callout_line(&label_boxes(l).0, [l.x + ax, l.y + ay])?; + Some([line[0] - l.x, line[1] - l.y, line[2] - l.x, line[3] - l.y]) +} + +/// A callout's line, from its feature at `anchor` to the nearest edge of the text's boxes; none +/// when the text covers the feature. +fn callout_line(text: &[[f32; 4]], anchor: [f32; 2]) -> Option<[f32; 4]> { + let e = + text.iter().fold([f32::MAX, f32::MAX, f32::MIN, f32::MIN], |e, b| [e[0].min(b[0]), e[1].min(b[1]), e[2].max(b[2]), e[3].max(b[3])]); + if e[0] > e[2] || !anchor.iter().all(|v| v.is_finite()) { + return None; + } + let to = [anchor[0].clamp(e[0], e[2]), anchor[1].clamp(e[1], e[3])]; + ((to[0] - anchor[0]).hypot(to[1] - anchor[1]) >= 1.0).then_some([anchor[0], anchor[1], to[0], to[1]]) +} + +/// Where a crowded label with a callout goes: the nearest free spot for its text's boxes, inside +/// the view, whose line from its feature at `anchor` to the nearest edge of its text is at most +/// `reach` long (points), and which doesn't cover the feature. The offset that moves it there and +/// that line (from, to), or none. +fn callout_spot( + text: &[[f32; 4]], + anchor: [f32; 2], + reach: f32, + free: impl Fn(&[f32; 4]) -> bool, + w: f32, + h: f32, +) -> Option<(f32, f32, [f32; 4])> { + let ext = + text.iter().fold([f32::MAX, f32::MAX, f32::MIN, f32::MIN], |e, b| [e[0].min(b[0]), e[1].min(b[1]), e[2].max(b[2]), e[3].max(b[3])]); + if !(anchor[0].is_finite() && anchor[1].is_finite() && reach.is_finite()) || ext[0] > ext[2] { + return None; + } + // Out in rings half the text's height apart, as far as its line may reach past its size. + let step = ((ext[3] - ext[1]) * 0.5).clamp(2.0, 20.0); + let far = reach + (ext[2] - ext[0]).max(ext[3] - ext[1]); + let rings = ((far / step).ceil() as usize).clamp(1, 60); + for ring in 1..=rings { + let d = ring as f32 * step; + // More directions farther out, so the spots stay about as close together. + let n = ((d * 0.75) as usize).clamp(8, 64); + let mut best: Option<(f32, f32, f32, [f32; 4])> = None; + for k in 0..n { + let a = k as f32 / n as f32 * std::f32::consts::TAU; + let (dx, dy) = (d * a.cos(), d * a.sin()); + let moved = |b: &[f32; 4]| [b[0] + dx, b[1] + dy, b[2] + dx, b[3] + dy]; + let e = moved(&ext); + if e[0] < 0.0 || e[1] < 0.0 || e[2] > w || e[3] > h { + continue; + } + // The line ends at the text's nearest edge; text over the feature hides it. + let to = [anchor[0].clamp(e[0], e[2]), anchor[1].clamp(e[1], e[3])]; + let len = (to[0] - anchor[0]).hypot(to[1] - anchor[1]); + if len > reach || len < 1.0 || best.is_some_and(|b| len >= b.0) || !text.iter().all(|b| free(&moved(b))) { + continue; + } + best = Some((len, dx, dy, [anchor[0], anchor[1], to[0], to[1]])); + } + if let Some((_, dx, dy, line)) = best { + return Some((dx, dy, line)); + } + } + None +} + /// Greedy collision-free placement, highest priority first. Locked labels are all placed (they /// were placed without collisions when the lock was taken). /// @@ -4297,14 +4410,47 @@ fn place_labels(mut cands: Vec, fixed: Vec, w: f32, h: f } let mut out = Placement { labels: fixed, unplaced: Vec::new(), keys: Vec::new() }; let outside = |b: &[f32; 4]| b[2] < 0.0 || b[3] < 0.0 || b[0] > w || b[1] > h; - for (l, _, key) in cands.into_iter().take(20_000) { - let (text, icon) = label_boxes(&l); + // Callout searches a placement makes at most: a crowded view stays quick. + let mut searches = MAX_CALLOUT_SEARCHES; + for (mut l, _, key) in cands.into_iter().take(20_000) { + let (mut text, icon) = label_boxes(&l); if text.iter().chain(&icon).all(outside) { continue; } + // A callout: crowded where it is, the label moves to the nearest free spot within its + // reach, a line drawn back to its feature. With no such spot it stays, and is left out, + // or shown over the others when its class shows them all. + l.collide.callout = None; + if l.collide.pinned { + // Moved by hand: where it was put, a callout's line back to its feature when its class + // has callouts (however long: it was put there). + if let Some([ax, ay]) = l.collide.callout_anchor.filter(|_| l.collide.callout_reach > 0.0) + && let Some(line) = callout_line(&text, [l.x + ax, l.y + ay]) + { + l.collide.callout = Some([line[0] - l.x, line[1] - l.y, line[2] - l.x, line[3] - l.y]); + } + } else if let Some([ax, ay]) = l.collide.callout_anchor + && l.collide.callout_reach > 0.0 + && !locked + && icon.is_empty() + && !text.is_empty() + && text.iter().any(|b| grid.hits(b)) + && searches > 0 + { + searches -= 1; + let anchor = [l.x + ax, l.y + ay]; + if let Some((dx, dy, line)) = callout_spot(&text, anchor, l.collide.callout_reach, |b| !grid.hits(b), w, h) { + l.x += dx; + l.y += dy; + for b in &mut text { + *b = [b[0] + dx, b[1] + dy, b[2] + dx, b[3] + dy]; + } + l.collide.callout = Some([line[0] - l.x, line[1] - l.y, line[2] - l.x, line[3] - l.y]); + } + } let c = l.collide; let (has_text, has_icon) = (!text.is_empty(), !icon.is_empty()); - let fits = |boxes: &[[f32; 4]], overlap: bool| locked || overlap || !boxes.iter().any(|b| grid.hits(b)); + let fits = |boxes: &[[f32; 4]], overlap: bool| locked || overlap || c.pinned || !boxes.iter().any(|b| grid.hits(b)); let mut show_text = has_text && fits(&text, c.text_allow_overlap); let mut show_icon = has_icon && fits(&icon, c.icon_allow_overlap); // Which part may show without the other (MapLibre's rules). @@ -4330,7 +4476,6 @@ fn place_labels(mut cands: Vec, fixed: Vec, w: f32, h: f if show_icon && !c.icon_ignore_placement { icon.iter().for_each(|b| grid.insert(*b)); } - let mut l = l; if !show_text { l.text.clear(); } @@ -4547,6 +4692,128 @@ mod tests { } } + /// While a label is dragged, its callout's line is worked out where it is: from its feature, + /// which stays put, to its text's nearest edge; none when its class has no callouts, or its + /// text covers its feature. Its extent is what picks it. + #[test] + fn a_dragged_label_s_callout_follows_it() { + use rustgis_tiles::Collide; + let l = PlacedLabel { + y: 50.0, + collide: Collide { callout_reach: 40.0, callout_anchor: Some([-60.0, 30.0]), ..Default::default() }, + ..label("Moved", 150.0, None, 0.0) + }; + let e = label_extent(&l).unwrap(); + assert!(e[0] < 150.0 && 150.0 < e[2] && e[1] < 50.0 && 50.0 < e[3], "{e:?}"); + let [x0, y0, x1, y1] = callout_to(&l).expect("a line"); + assert_eq!((x0, y0), (-60.0, 30.0), "from the feature"); + // To the text's bottom-left corner: the feature is below and to the left of it. + assert!(((l.x + x1) - e[0]).abs() < 1e-3 && ((l.y + y1) - e[3]).abs() < 1e-3); + let none = PlacedLabel { collide: Collide { callout_reach: 0.0, ..l.collide }, ..l.clone() }; + assert!(callout_to(&none).is_none(), "no callouts"); + let over = PlacedLabel { collide: Collide { callout_anchor: Some([0.0, 0.0]), ..l.collide }, ..l.clone() }; + assert!(callout_to(&over).is_none(), "the text over its feature"); + } + + /// A label moved by hand is placed where it was put, over another and first, so the others keep + /// off it; with callouts its line reaches back to its feature, however far it went. + #[test] + fn labels_moved_by_hand_are_placed_where_they_were_put() { + use rustgis_tiles::Collide; + // Its feature 80 points left of where it was put. + let pinned = |callout: f32| PlacedLabel { + y: 50.0, + collide: Collide { pinned: true, callout_reach: callout, callout_anchor: Some([-80.0, 0.0]), ..Default::default() }, + ..label("Moved", 150.0, None, 0.0) + }; + let other = PlacedLabel { y: 50.0, ..label("Other", 150.0, None, 0.0) }; + // The other first by priority, the moved one placed first anyway: it keeps its spot. + let p = place_labels( + vec![(other.clone(), 9.0, (u32::MAX, 0, 0, 0)), (pinned(0.0), 1e15, (u32::MAX, 1, 0, 0))], + Vec::new(), + 300.0, + 120.0, + false, + ); + assert_eq!(p.labels.first().map(|l| (l.text.as_str(), l.x, l.y)), Some(("Moved", 150.0, 50.0))); + assert_eq!(p.labels.len(), 1, "the other keeps off it"); + assert!(p.labels[0].collide.callout.is_none(), "no callouts: no line"); + // With callouts: a line from the feature to its text, past any callout's usual reach. + let p = place_labels(vec![(pinned(5.0), 1e15, (u32::MAX, 1, 0, 0))], Vec::new(), 300.0, 120.0, false); + let l = &p.labels[0]; + let [x0, y0, x1, y1] = l.collide.callout.expect("a line back to its feature"); + assert!(((l.x + x0) - 70.0).abs() < 1e-3 && ((l.y + y0) - 50.0).abs() < 1e-3); + assert!((x1 - x0).hypot(y1 - y0) > 5.0); + // And over whatever is there when it's placed after: a fixed label under it stays too. + let fixed = vec![other]; + let p = place_labels(vec![(pinned(0.0), 1e15, (u32::MAX, 1, 0, 0))], fixed, 300.0, 120.0, false); + assert_eq!(p.labels.len(), 2); + } + + /// A label crowded off its spot moves, with a callout, to the nearest free spot whose line back + /// to its feature is no longer than its reach, and keeps that line; with every label shown it + /// stays, over the other; with neither it's left out. + #[test] + fn callouts_move_crowded_labels_and_show_all_overlaps_them() { + use rustgis_tiles::Collide; + // Two labels on one spot, each by its feature 10 points below the text's centre. + let at = |text: &str, collide: Collide| PlacedLabel { + y: 50.0, + collide: Collide { callout_anchor: Some([0.0, 10.0]), ..collide }, + ..label(text, 100.0, None, 0.0) + }; + let place = |second: Collide| { + let cands = vec![(at("First", Collide::default()), 2.0, (u32::MAX, 0, 0, 0)), (at("Second", second), 1.0, (u32::MAX, 1, 0, 0))]; + place_labels(cands, Vec::new(), 300.0, 120.0, false) + }; + let overlap = |a: &[f32; 4], b: &[f32; 4]| a[0] < b[2] && b[0] < a[2] && a[1] < b[3] && b[1] < a[3]; + // Neither: the second is left out. + let p = place(Collide::default()); + assert_eq!((p.labels.len(), p.unplaced.len()), (1, 1)); + // A callout: moved clear of the first, a line from its feature to its text's edge. + let p = place(Collide { callout_reach: 40.0, ..Default::default() }); + assert_eq!(p.labels.len(), 2); + let (first, second) = (&p.labels[0], &p.labels[1]); + assert!(first.collide.callout.is_none(), "in its own spot, no line"); + let [x0, y0, x1, y1] = second.collide.callout.expect("a callout's line"); + assert!(((second.x + x0) - 100.0).abs() < 1e-3 && ((second.y + y0) - 60.0).abs() < 1e-3, "it starts at the feature"); + let len = (x1 - x0).hypot(y1 - y0); + assert!((1.0..=40.0).contains(&len), "{len} points"); + let (a, b) = (label_boxes(first).0, label_boxes(second).0); + assert!(!a.iter().any(|x| b.iter().any(|y| overlap(x, y))), "clear of the first"); + // No free spot within so short a reach: left out, as without one. + let p = place(Collide { callout_reach: 1.0, ..Default::default() }); + assert_eq!(p.labels.len(), 1); + // Every label shown: the second stays in its spot, over the first, with no line. + let p = place(Collide { text_allow_overlap: true, ..Default::default() }); + assert_eq!(p.labels.len(), 2); + assert_eq!((p.labels[1].x, p.labels[1].y), (100.0, 50.0)); + assert!(p.labels[1].collide.callout.is_none()); + // Both: a free spot nearby comes before overlapping. + let p = place(Collide { callout_reach: 40.0, text_allow_overlap: true, ..Default::default() }); + assert!(p.labels[1].collide.callout.is_some()); + } + + /// A callout's line is drawn from its feature to the text, in the label's colour. + #[test] + fn a_callout_line_is_drawn() { + use rustgis_tiles::Collide; + let mut pm = Pixmap::new(200, 100).unwrap(); + pm.fill(tiny_skia::Color::WHITE); + let l = PlacedLabel { + y: 20.0, + color: Rgba::rgb(200, 0, 0), + collide: Collide { callout: Some([-60.0, 60.0, -10.0, 10.0]), ..Default::default() }, + ..label("Callout", 150.0, None, 0.0) + }; + text::draw(&mut pm, &l, 1.0); + // Halfway along the line (x + y = 170), well clear of the text: red. (The pixel whose + // centre is on it: the line passes through (115, 55)'s corner.) + let mid = pm.pixel(115, 54).unwrap(); + assert!(mid.red() > 120 && mid.green() < 80 && mid.blue() < 80, "{mid:?}"); + assert!(pm.pixel(115, 20).is_some_and(|p| p.green() > 200), "nothing above the line"); + } + /// A halo of its own width reaches past the letters, so its label's collision box grows by it: /// another label's text never falls under it. The thin halo and none leave the box as it was. #[test] @@ -4969,6 +5236,40 @@ mod tests { /// Each label class labels every feature its clause takes, as in ArcGIS Pro (#732): a feature /// two classes take gets a label from each, the class listed first placed first. A hidden /// class labels none, and a clause that no longer reads takes none (#618). + /// A label moved by hand is drawn where it was put, on the flat map: its place is kept as + /// longitude and latitude, and drawn through the map's projection as its feature is. + /// (Taken for the projection's own coordinates, it was drawn off by Null Island instead.) + #[test] + fn a_label_moved_by_hand_is_drawn_where_it_was_put() { + let mut m = Map::new("Map"); + let mut d = Dataset::new("pts", GeometryType::Point, vec![Field::new("name", FieldType::Text)]); + d.push(Some(Geometry::Point(Coord::new(2.35, 48.85))), vec![Value::Text("Paris".into())]); + d.push(Some(Geometry::Point(Coord::new(4.83, 45.76))), vec![Value::Text("Lyon".into())]); + let id = m.add_dataset(d); + let paris = m.layer(id).unwrap().dataset.features.first().unwrap().id; + if let Some(l) = m.layer_mut(id) { + l.labels.enabled = true; + l.labels.default.callout = Some(40.0); + assert!(l.labels.default.move_label(paris, Some([5.0, 49.5]))); + } + let camera = Camera { center: lonlat_to_merc(Coord::new(3.6, 47.3)), res: 5000.0, bearing: 0.0, pitch: 0.0 }; + let view = View { width: 800.0, height: 600.0, pixels_per_point: 1.0, camera, sky: false }; + let on_screen = |m: &Map, lon: f64, lat: f64| view.to_screen(m.to_map(Coord::new(lon, lat)).unwrap()); + let labels = render(&mut m, &view).unwrap().labels; + let l = labels.iter().find(|l| l.text == "Paris").expect("Paris's label is drawn"); + let (ex, ey) = on_screen(&m, 5.0, 49.5); + assert!((f64::from(l.x) - ex).abs() < 0.5 && (f64::from(l.y) - ey).abs() < 0.5, "at ({}, {}), not ({ex}, {ey})", l.x, l.y); + assert!(l.collide.pinned); + assert_eq!(l.collide.source, Some((id, paris, 0)), "what the Move Label tool grabs it by"); + // Its callout's line starts at Paris. + let [x0, y0, ..] = l.collide.callout.expect("a line back to Paris"); + let (px, py) = on_screen(&m, 2.35, 48.85); + assert!((f64::from(l.x + x0) - px).abs() < 0.5 && (f64::from(l.y + y0) - py).abs() < 0.5); + // Lyon's, not moved, is where its class puts it, with no line. + let lyon = labels.iter().find(|l| l.text == "Lyon").unwrap(); + assert!(!lyon.collide.pinned && lyon.collide.callout.is_none()); + } + #[test] fn label_classes_label_their_features() { let mut m = Map::new("Map"); diff --git a/crates/render/src/text.rs b/crates/render/src/text.rs index ef3d89cb1..7beae7189 100644 --- a/crates/render/src/text.rs +++ b/crates/render/src/text.rs @@ -610,6 +610,7 @@ fn slanted_glyph(pm: &mut Pixmap, og: &ab_glyph::OutlinedGlyph, base: f32, color /// Draws a placed label (logical coordinates) with its halo. A label turned with the map (one /// along a street) is drawn upright into a picture of its own, which is turned into place. pub fn draw(pm: &mut Pixmap, l: &PlacedLabel, ppp: f32) { + draw_callout(pm, l, ppp); // Curved along a line: each character turned into its place, every character's halo // first, so none covers the letter before it. if !l.glyphs.is_empty() { @@ -630,6 +631,28 @@ pub fn draw(pm: &mut Pixmap, l: &PlacedLabel, ppp: f32) { } } +/// A label's callout line, from its feature to the edge of its text, under the text (and its +/// halo under the line, so it shows over the map). +fn draw_callout(pm: &mut Pixmap, l: &PlacedLabel, ppp: f32) { + let Some([x0, y0, x1, y1]) = l.collide.callout else { return }; + let mut pb = tiny_skia::PathBuilder::new(); + pb.move_to((l.x + x0) * ppp, (l.y + y0) * ppp); + pb.line_to((l.x + x1) * ppp, (l.y + y1) * ppp); + let Some(path) = pb.finish() else { return }; + let width = (CALLOUT_WIDTH * ppp).max(0.5); + let inks = l.halo.map(|h| (h, width + 2.0 * ppp)).into_iter().chain([(l.color, width)]); + for (c, w) in inks { + let mut paint = Paint::default(); + paint.set_color_rgba8(c.r(), c.g(), c.b(), c.a()); + paint.anti_alias = true; + let stroke = tiny_skia::Stroke { width: w, line_cap: tiny_skia::LineCap::Round, ..Default::default() }; + pm.stroke_path(&path, &paint, &stroke, Transform::identity(), None); + } +} + +/// How wide a callout's line is (points). +pub const CALLOUT_WIDTH: f32 = 0.9; + /// What of a label to draw: its halo, its text, or both (the halo under the text). #[derive(Clone, Copy, PartialEq)] enum Ink { diff --git a/crates/tiles/src/symbols.rs b/crates/tiles/src/symbols.rs index 82fbb63b4..1de6bd397 100644 --- a/crates/tiles/src/symbols.rs +++ b/crates/tiles/src/symbols.rs @@ -88,6 +88,21 @@ pub struct Collide { /// Others may be placed over it. pub text_ignore_placement: bool, pub icon_ignore_placement: bool, + /// With a callout (a vector layer's label class): crowded where it is, the label may move to + /// a free spot whose line back to its feature is at most this long (points); 0 stays put. + pub callout_reach: f32, + /// Its feature's place, where a callout's line starts: an offset (points) from where the + /// label is drawn (`x`, `y`), so it moves with the label until it's placed. + pub callout_anchor: Option<[f32; 2]>, + /// Once moved off its spot, its callout's line: from its feature to the edge of its text, as + /// offsets (points) from where the label is drawn (`x`, `y`), so it moves with the label. + pub callout: Option<[f32; 4]>, + /// Moved by hand (the Move Label tool): placed where it was put, whatever is under it, and + /// placed first, so the others keep off it. + pub pinned: bool, + /// What it labels, for moving it by hand: its layer's id, its feature's id and its label + /// class (in the layer's labeling order). None for labels not of a layer's features. + pub source: Option<(u32, u64, u32)>, } impl Default for Collide { @@ -101,6 +116,11 @@ impl Default for Collide { icon_allow_overlap: false, text_ignore_placement: false, icon_ignore_placement: false, + callout_reach: 0.0, + callout_anchor: None, + callout: None, + pinned: false, + source: None, } } } @@ -268,6 +288,7 @@ fn label_of(layer: &StyleLayer, ctx: &FeatureCtx, sheet: Option<&Sprite>, layer_ icon_allow_overlap: flag(layer, "icon-allow-overlap", ctx), text_ignore_placement: flag(layer, "text-ignore-placement", ctx), icon_ignore_placement: flag(layer, "icon-ignore-placement", ctx), + ..Collide::default() }; Some(Label { text, diff --git a/crates/ui-egui/src/labelpane.rs b/crates/ui-egui/src/labelpane.rs index fb64a077f..885f024a5 100644 --- a/crates/ui-egui/src/labelpane.rs +++ b/crates/ui-egui/src/labelpane.rs @@ -880,6 +880,65 @@ fn position(app: &mut RustGisApp, ui: &mut Ui, l: &Layer, c: &Class) { } ui.add_space(6.0); offset(app, ui, l, c); + ui.add_space(6.0); + crowding(app, ui, l, c); +} + +/// Where labels crowd: every one shown, over the others, rather than some left out; and +/// callouts, which move a crowded label to a free spot nearby with a line back to its feature, +/// no longer than so far. +fn crowding(app: &mut RustGisApp, ui: &mut Ui, l: &Layer, c: &Class) { + use rustgis_engine::map::symbology::{CALLOUT_REACH, callout_reach}; + let id = l.id; + grid(ui, "crowding", |ui| { + let input_label = ui.label(tl!("Show all labels")); + let mut all = c.it.show_all; + if ui + .checkbox(&mut all, "") + .labelled_by(input_label.id) + .on_hover_text(tl!("Draw every label, over the others where they crowd, rather than leaving some out.")) + .changed() + { + run(app, "labels.placement", c.params(json!({"showAll": all}))); + } + ui.end_row(); + let input_label = ui.label(tl!("Callouts")); + ui.horizontal(|ui| { + let mut on = c.it.callout.is_some(); + if ui + .checkbox(&mut on, "") + .labelled_by(input_label.id) + .on_hover_text(tl!( + "Move a label crowded off its spot to a free one nearby, with a line back to its feature. Labels along lines stay." + )) + .changed() + { + run(app, "labels.placement", c.params(json!({"callout": on}))); + } + let Some(reach) = c.it.callout else { return }; + let mut v = f64::from(reach); + let r = ui + .add(egui::DragValue::new(&mut v).range(CALLOUT_REACH).speed(1.0).max_decimals(0).prefix(tl!("at most ")).suffix(" pt")) + .labelled_by(input_label.id) + .on_hover_text(tl!("The longest a callout's line may be.")); + crate::ribbon::live_drag( + app, + ui, + &r, + f64::from(reach), + v, + |app, x| { + if let Some(k) = app.session.map_mut().layer_mut(id).and_then(|l| class_mut(&mut l.labels, c.name)) { + k.callout = callout_reach(x); + } + }, + |app, x| { + run(app, "labels.placement", c.params(json!({"callout": x}))); + }, + ); + }); + ui.end_row(); + }); } /// Along a line, a label every so many points, spaced out from the one at its middle; off, the one diff --git a/crates/ui-egui/src/mapview.rs b/crates/ui-egui/src/mapview.rs index 46f7f19a4..f8b42a77d 100644 --- a/crates/ui-egui/src/mapview.rs +++ b/crates/ui-egui/src/mapview.rs @@ -54,6 +54,15 @@ enum DragKind { row: usize, origin: Pos2, }, + /// Moving a layer's label with the Move Label tool: its layer's id, its feature's id, its + /// label class (in the layer's labeling order) and where the press was. The label as it was + /// drawn is in `MapView::moving_label`. + Label { + layer: u32, + feature: u64, + class: u32, + origin: Pos2, + }, } /// Graphic shapes drawn by dragging rather than clicking. @@ -262,6 +271,8 @@ pub struct MapView { view_px: [usize; 2], resized_at: f64, drag: Option<(Pos2, DragKind)>, + /// The label being moved with the Move Label tool, as it was drawn when grabbed. + moving_label: Option, sketch: Vec, selection_gesture: Option, radius_preview: Option<(Coord, f64, Vec>)>, @@ -366,6 +377,7 @@ impl Default for MapView { view_px: [0, 0], resized_at: -10.0, drag: None, + moving_label: None, sketch: Vec::new(), selection_gesture: None, radius_preview: None, @@ -1404,7 +1416,44 @@ fn place_galley(anchor: Align2, at: Pos2, g: &egui::Galley) -> (Pos2, Rect) { (r.min - g.rect.min.to_vec2(), r) } +/// The layer's label drawn at a point of the map view (relative to it as `rect` is), topmost first: +/// what the Move Label tool grabs. Labels not of a layer's features aren't. +fn label_at(app: &RustGisApp, rect: Rect, p: Pos2) -> Option { + let at = p - rect.min; + app.map.labels.iter().rev().filter(|l| l.collide.source.is_some() && !l.text.is_empty()).find_map(|l| { + let e = rustgis_engine::render::label_extent(l)?; + let r = Rect::from_min_max(pos2(e[0], e[1]), pos2(e[2], e[3])).expand(3.0); + r.contains(pos2(at.x, at.y)).then(|| l.clone()) + }) +} + +/// A label moved by `d` (points): its text there, its feature where it was, and its callout's line +/// from the feature to the text when its class has callouts. +fn moved_label(l: &PlacedLabel, d: egui::Vec2) -> PlacedLabel { + let mut m = l.clone(); + m.x += d.x; + m.y += d.y; + m.collide.callout_anchor = l.collide.callout_anchor.map(|[ax, ay]| [ax - d.x, ay - d.y]); + m.collide.callout = rustgis_engine::render::callout_to(&m); + m +} + +/// The name of a layer's label class by its place in the labeling order. +fn label_class_name(app: &RustGisApp, layer: u32, class: u32) -> Option { + app.session.map().layer(layer)?.labels.in_order().nth(class as usize).map(|c| c.name.clone()) +} + fn draw_label(p: &egui::Painter, l: &PlacedLabel, at: Pos2) { + // A callout's line, from its feature to the edge of the text, under the text (its halo + // under the line, so it shows over the map). + if let Some([x0, y0, x1, y1]) = l.collide.callout { + let ends = [at + vec2(x0, y0), at + vec2(x1, y1)]; + let width = rustgis_engine::render::CALLOUT_WIDTH; + if let Some(h) = l.halo { + p.line_segment(ends, Stroke::new(width + 2.0, crate::theme::c32(h))); + } + p.line_segment(ends, Stroke::new(width, crate::theme::c32(l.color))); + } // Curved along a line: each character turned into its place, every character's halo first, // so none covers the letter before it. if !l.glyphs.is_empty() { @@ -2215,8 +2264,15 @@ fn interact(app: &mut RustGisApp, ui: &mut Ui, resp: &egui::Response, rect: Rect .then(|| text_graphic_at(app, ui.ctx(), rect, origin)) .flatten() .filter(|hit| crate::graphicformat::selected(app) == Some(*hit)); + // With the Move Label tool, a layer's label grabbed by its text moves; elsewhere the map pans. + let label = (tool == Tool::MoveLabel && !middle).then(|| label_at(app, rect, origin)).flatten(); let kind = if let Some((layer, row)) = grabbed { DragKind::Text { layer, row, origin } + } else if let Some(l) = label + && let Some((layer, feature, class)) = l.collide.source + { + app.map.moving_label = Some(l); + DragKind::Label { layer, feature, class, origin } } else if middle { DragKind::Pan } else if tool == Tool::EditVertices && crate::vertexedit::drag_start(app, rect, origin) { @@ -2258,6 +2314,33 @@ fn interact(app: &mut RustGisApp, ui: &mut Ui, resp: &egui::Response, rect: Rect app.map.sketch.push(c); } } + // A label being moved shows where it will land, its callout's line following it; it is kept + // there, as one undo step, on release. + if let Some((_, DragKind::Label { layer, feature, class, origin })) = app.map.drag { + let end = resp.interact_pointer_pos().or_else(|| ui.input(|i| i.pointer.latest_pos())); + let moved = app.map.moving_label.clone().zip(end).map(|(l, end)| moved_label(&l, end - origin)); + if resp.drag_stopped() { + app.map.drag = None; + app.map.moving_label = None; + let to = moved.and_then(|l| screen_to_lonlat(app, rect, rect.min + vec2(l.x, l.y))); + if let (Some(c), Some(name)) = (to, label_class_name(app, layer, class)) { + let _ = app.run("labels.move", json!({"layer": layer, "class": name, "feature": feature, "to": [c.x, c.y]})); + } + return; + } + if let Some(l) = moved { + let t = Tokens::get(ui.ctx()); + let painter = + ui.ctx().layer_painter(egui::LayerId::new(egui::Order::Foreground, ui.scope_id().with("label_drag"))).with_clip_rect(rect); + draw_label(&painter, &l, rect.min + vec2(l.x, l.y)); + if let Some(e) = rustgis_engine::render::label_extent(&l) { + let r = Rect::from_min_max(rect.min + vec2(e[0], e[1]), rect.min + vec2(e[2], e[3])); + painter.rect_stroke(r, 2.0, egui::Stroke::new(1.0, t.accent), egui::StrokeKind::Outside); + } + ui.ctx().set_cursor_icon(egui::CursorIcon::Grabbing); + ui.ctx().request_repaint(); + } + } // A dragged text shows where it will land; it is placed there, as one undo step, on release. if let Some((_, DragKind::Text { layer, row, origin })) = app.map.drag { let end = resp.interact_pointer_pos().or_else(|| ui.input(|i| i.pointer.latest_pos())); @@ -2459,6 +2542,15 @@ fn interact(app: &mut RustGisApp, ui: &mut Ui, resp: &egui::Response, rect: Rect // Sketch vertices snap to features when snapping is on. let c = if tool.creates() || matches!(tool, Tool::Graphic(_) | Tool::Measure | Tool::MeasureArea) { snapped(app, c) } else { c }; match tool { + // Labels move by dragging them; Shift-clicking a moved one puts it back. + Tool::MoveLabel => { + if shift + && let Some((layer, feature, class)) = label_at(app, rect, p).and_then(|l| l.collide.source) + && let Some(name) = label_class_name(app, layer, class) + { + let _ = app.run("labels.move", json!({"layer": layer, "class": name, "feature": feature, "to": null})); + } + } Tool::Explore => { let r = app.session.run("identify", &json!({"lon": c.x, "lat": c.y, "tolerancePx": 5})); app.map.popup = match r { @@ -2858,6 +2950,7 @@ fn overlays(app: &mut RustGisApp, ui: &mut Ui, p: &egui::Painter, rect: Rect, t: } Tool::EditVertices => Some("Click a feature, then drag its vertices · click an edge to add one"), Tool::ZoomIn => Some("Drag a box or click to zoom in"), + Tool::MoveLabel => Some("Drag a label to where it should be · drag the map to pan · Esc to stop"), Tool::Graphic("point") => Some("Click the map to place a point"), Tool::Graphic("text") => Some("Click the map to place text"), Tool::Graphic("line" | "curve") => Some("Click to add vertices · double-click to finish"), diff --git a/crates/ui-egui/src/ribbon.rs b/crates/ui-egui/src/ribbon.rs index b2ea05073..6dafeebec 100644 --- a/crates/ui-egui/src/ribbon.rs +++ b/crates/ui-egui/src/ribbon.rs @@ -4456,6 +4456,24 @@ fn label_groups(app: &mut RustGisApp, ui: &mut Ui) { } } }); + // Labels put by hand: the Move Label tool (on and off; Esc leaves it too), and putting the + // layer's moved labels back. + col(ui, |ui| { + let moving = app.session.tool == Tool::MoveLabel; + let to = if moving { Tool::Explore } else { Tool::MoveLabel }; + let desc = "Drag a label to where it should be; its callout's line follows. Shift-click a moved label to put it back."; + small(ui, app, "label", "Move Label", desc, "tool.set", json!({"tool": to.code()}), moving); + small( + ui, + app, + "undo", + "Reset Moved", + "Put this layer's moved labels back where their classes place them.", + "labels.resetMoved", + json!({"all": true}), + false, + ); + }); }); group(ui, "Label Range", |ui| { ui.vertical(|ui| { diff --git a/docs/user-guide/cartography.md b/docs/user-guide/cartography.md index 35da79ff2..b9a69dd73 100644 --- a/docs/user-guide/cartography.md +++ b/docs/user-guide/cartography.md @@ -382,6 +382,23 @@ Turn labels on per layer (Labeling › Labels › **Label**), then: Lock keeps the ones placed. The distance grows with the labels at a reference scale (`labels.placement={"layer": "Rivers", "position": "curved", "repeat": 144}`; `null` or `0` is the one at the middle). +- **Show all labels** (Labeling pane › Position): every label is drawn, over the others where + they crowd, rather than some left out (`labels.placement={"layer": "Cities", "showAll": true}`). +- **Callouts** (Labeling pane › Position): a label crowded off its spot moves to the nearest free + spot nearby, and a line is drawn from its feature to the text, as QGIS's callouts. **at most** + sets how long the line may be (5 to 500 points, 40 to start with): a label with no free spot + that close is left out, or drawn over the others with Show all labels. The line takes the + label's colour, over its halo. Labels along lines aren't moved + (`labels.placement={"layer": "Cities", "callout": 60}`; `true` keeps the length, `false` turns + them off). +- **Move Label** (Labeling tab › Label Placement): drag a label to where it should be, as with + QGIS's Move Label tool. It stays there as the map moves (kept by its feature, in longitude and + latitude, and saved with the project), is drawn whatever is under it, and the other labels keep + off it; with Callouts on, its line follows it back to its feature, however far it went. + Shift-click a moved label to put it back, or **Reset Moved** for all of the layer's. Dragging + anything else pans the map; Esc leaves the tool + (`labels.move={"layer": "Cities", "feature": 12, "to": [2.5, 49.1]}`; `"to": null` puts it back; + `labels.resetMoved={"layer": "Cities", "all": true}`). - **Font**: the labels' font family, any installed on the computer (Labeling pane › Symbol › **Font**; type part of a name to find it), or the label font the interface uses. Bold and italic take the family's own bold and italic faces where it has them. The family is saved by From 98ea58bd39d415af0e602971e77f8a8a735a2b57 Mon Sep 17 00:00:00 2001 From: huzaifffa Date: Thu, 29 Oct 2026 16:53:50 +0300 Subject: [PATCH 02/15] fix(labels): address review comments on callouts and Move Label MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Esc, another tool, or another map or project mid-drag cancels a label move. - A label moved into view is drawn when its feature is out of view (point or along-line), with its callout line back to the feature. - Long callouts: the ring search spaces out to reach the full length. - Shift-click puts back only moved labels. - A drag keeps its class name, map and project from when it started. - labels.move and labels.resetMoved relock the labels; resetMoved refuses a non-boolean "all". - labels.convert puts a moved label's annotation where it was put and keeps no moves on the annotations' classes. - Moved places are range-checked (±540 lon, ±90 lat) in files and commands. - Moved labels kept in order of id: binary search per label, O(n log n) load. - Terrain/extrusion lift a callout's feature end at its own place. - While a globe or tilted map moves, callout lines stay on their features; labels can be grabbed while a flat map moves. - Doc comments back on their tests; shared extent helper. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/labels.rs | 29 ++++- crates/engine/src/tests.rs | 27 +++- crates/map/src/symbology.rs | 76 +++++++---- crates/render/src/lib.rs | 161 +++++++++++++++++------ crates/ui-egui/src/mapview.rs | 223 +++++++++++++++++++++++++++----- 5 files changed, 405 insertions(+), 111 deletions(-) diff --git a/crates/engine/src/cmd/labels.rs b/crates/engine/src/cmd/labels.rs index 675bf3cb5..c195a83ba 100644 --- a/crates/engine/src/cmd/labels.rs +++ b/crates/engine/src/cmd/labels.rs @@ -2,6 +2,7 @@ //! layer's label visibility range and placement, and its label classes (#618). use rustgis_data::{Dataset, Field, FieldType, Geometry, GeometryType, Value as Attr}; +use rustgis_geom::Coord; use rustgis_map::symbology::{MAX_LABEL_CLASSES, label_class_name}; use rustgis_map::{LabelBackground, LabelClass, LabelMatcher, LabelPlacement, Labeling, Renderer, Rgba, Symbol}; use serde_json::{Value, json}; @@ -206,25 +207,32 @@ pub fn commands() -> Vec { let a = v.as_array().filter(|a| a.len() == 2).ok_or("to is [x, y] (longitude, latitude)")?; let xy = [a.first().and_then(number), a.get(1).and_then(number)]; match xy { - [Some(x), Some(y)] if x.is_finite() && y.is_finite() && x.abs() <= 540.0 && y.abs() <= 90.0 => Some([x, y]), - _ => return Err("to is [x, y]: a longitude and a latitude".into()), + [Some(x), Some(y)] if rustgis_map::symbology::is_label_place([x, y]) => Some([x, y]), + _ => return Err("to is [x, y]: a longitude (within ±540) and a latitude (within ±90)".into()), } } }; - with_labels(s, p, |labels| { + let r = with_labels(s, p, |labels| { let c = class_in(labels, p)?; if c.move_label(feature, to) { Ok(()) } else { Err(format!("a label class keeps at most {} labels moved", rustgis_map::symbology::MAX_MOVED_LABELS)) } - }) + })?; + // Locked, the labels are locked again where they are placed next: the one moved may + // not have been placed when the lock was taken. + s.map_mut().label_lock = None; + Ok(r) }) .when(need_feature_layer) .params(r#"{"layer"?, "class"?: name (none: the Default class), "feature": the feature's id, "to": [x, y] (longitude, latitude: where the label's text is centred) | null (back where its class puts it)}"#), C::new("labels.resetMoved", "Reset Moved Labels", "Labeling › Label Placement", |s, p| { - let all = b(p, "all").unwrap_or(false); - with_labels(s, p, |labels| { + let all = match p.get("all") { + None | Some(Value::Null) => false, + Some(v) => v.as_bool().ok_or("all must be true or false")?, + }; + let r = with_labels(s, p, |labels| { if all { labels.default.moved.clear(); labels.classes.iter_mut().for_each(|c| c.moved.clear()); @@ -232,7 +240,10 @@ pub fn commands() -> Vec { class_in(labels, p)?.moved.clear(); } Ok(()) - }) + })?; + // As after a move: those put back are locked again where they are placed next. + s.map_mut().label_lock = None; + Ok(r) }) .when(need_feature_layer) .params(r#"{"layer"?, "class"?: name (none: the Default class), "all"?: bool (every class of the layer)}"#), @@ -519,6 +530,8 @@ fn convert(s: &mut Session, p: &Value) -> CmdResult { if classes { attrs.push(Attr::Text(class.name.clone())); } + // A label moved by hand becomes an annotation where it was put. + let at = class.moved_to(ft.id).map_or(at, |[x, y]| Coord::new(x, y)); ds.push(Some(Geometry::Point(at)), attrs); } } @@ -535,6 +548,8 @@ fn convert(s: &mut Session, p: &Value) -> CmdResult { // On its point, never round it: an annotation stays where it is (#1011). c.placement = LabelPlacement::Center; c.other_positions = false; + // The annotations' ids aren't the features': each moved label is already at its place. + c.moved.clear(); // Each class picks the points it labelled, Default too once there are others (#732): // with no clause it would label theirs as well. c.where_ = classes.then(|| format!("\"CLASS\" = '{}'", c.name.replace('\'', "''"))); diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 361b7693f..ee0e07e59 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -2345,8 +2345,7 @@ fn labels_take_a_font_by_family_name() { assert!(s.run("labels.fonts", &json!({})).unwrap()["fonts"].is_array()); } -/// A label class's halo takes its own colour and width, and its letters a spacing; blank and null -/// go back to the automatic colour and the thin halo, and other settings leave them. +/// Labels are moved by hand by feature id and put back, one class's or all; bad input is refused. #[test] fn labels_are_moved_by_command_and_reset() { let mut s = world(); @@ -2382,8 +2381,30 @@ fn labels_are_moved_by_command_and_reset() { s.run("labels.move", &json!({"layer": id, "feature": fid, "to": null})).unwrap(); assert!(moved(&s).is_empty()); s.run("labels.move", &json!({"layer": id, "feature": fid, "to": [1, 1]})).unwrap(); + // A bad "all" is refused, not taken as false. + assert!(s.run("labels.resetMoved", &json!({"layer": id, "all": "tru"})).is_err()); + assert_eq!(moved(&s).len(), 1); s.run("labels.resetMoved", &json!({"layer": id, "all": true})).unwrap(); assert!(moved(&s).is_empty()); + // Locked, a move locks the labels again where they are placed next: the one moved may not + // have been placed when the lock was taken. + s.run("labels.lock", &json!({"on": true})).unwrap(); + let v = crate::render::View { width: 800.0, height: 600.0, pixels_per_point: 1.0, camera: s.map().camera, sky: false }; + crate::render::render(s.map_mut(), &v).unwrap(); + assert!(s.map().label_lock.is_some()); + s.run("labels.move", &json!({"layer": id, "feature": fid, "to": [2, 2]})).unwrap(); + assert!(s.map().label_lock.is_none() && s.map().label_display.locked, "locked again, not unlocked"); + // Converted to annotations, a moved label's annotation is where it was put, and the + // annotations' classes keep none moved (their ids aren't the features'). + s.run("labels.lock", &json!({"on": false})).unwrap(); + let r = s.run("labels.convert", &json!({"layer": id})).unwrap(); + let a = s.map().layer(r["layer"].as_u64().unwrap() as u32).unwrap(); + assert!(a.labels.default.moved.is_empty()); + let at = |g: &Option| match g { + Some(rustgis_data::Geometry::Point(c)) => Some([c.x, c.y]), + _ => None, + }; + assert!(a.dataset.features.iter().any(|f| at(&f.geometry) == Some([2.0, 2.0])), "the moved one's annotation"); // The tool by its code. s.run("tool.set", &json!({"tool": "moveLabel"})).unwrap(); assert_eq!(s.tool, crate::Tool::MoveLabel); @@ -2433,6 +2454,8 @@ fn labels_show_all_and_take_callouts() { assert_eq!(class(&s), (true, Some(40.0)), "undoes"); } +/// A label class's halo takes its own colour and width, and its letters a spacing; blank and null +/// go back to the automatic colour and the thin halo, and other settings leave them. #[test] fn labels_take_a_halo_colour_and_width_and_letter_spacing() { use rustgis_map::Rgba; diff --git a/crates/map/src/symbology.rs b/crates/map/src/symbology.rs index 1c3d90546..201824931 100644 --- a/crates/map/src/symbology.rs +++ b/crates/map/src/symbology.rs @@ -1896,7 +1896,7 @@ pub struct LabelClass { pub callout: Option, /// Labels moved by hand (the Move Label tool, as QGIS's): each feature's, by its id, drawn /// where it was put, a callout's line back to the feature when the class has callouts. At - /// most [`MAX_MOVED_LABELS`]. + /// most [`MAX_MOVED_LABELS`], kept in order of id (so a label's is found without a scan). #[serde(default, deserialize_with = "de_moved", skip_serializing_if = "Vec::is_empty")] pub moved: Vec, } @@ -1912,53 +1912,61 @@ pub struct MovedLabel { /// Most labels a class keeps moved by hand. pub const MAX_MOVED_LABELS: usize = 10_000; -/// A project file's moved labels: those with a place that is a number, one a feature (the last -/// wins), at most [`MAX_MOVED_LABELS`]. +/// Whether `at` is a place a label can be moved to: a longitude within ±540° (a view's worth +/// past the antimeridian either way) and a latitude within ±90°. +pub fn is_label_place(at: [f64; 2]) -> bool { + let [x, y] = at; + x.is_finite() && y.is_finite() && x.abs() <= 540.0 && y.abs() <= 90.0 +} + +/// A project file's moved labels: those with a place ([`is_label_place`]), one a feature (the +/// last wins), at most [`MAX_MOVED_LABELS`], in order of id. fn de_moved<'de, D: serde::Deserializer<'de>>(d: D) -> Result, D::Error> { // Read one by one, so one that isn't a moved label leaves the others (and the file) readable. - let all: Vec = lenient::>>(d)? + let mut by_id = std::collections::BTreeMap::new(); + for m in lenient::>>(d)? .unwrap_or_default() .into_iter() .flatten() - .filter_map(|v| serde_json::from_value(v).ok()) - .collect(); - let mut out: Vec = Vec::new(); - for m in all.into_iter().filter(|m| m.at.iter().all(|v| v.is_finite())) { - if let Some(o) = out.iter_mut().find(|o| o.id == m.id) { - *o = m; - } else if out.len() < MAX_MOVED_LABELS { - out.push(m); + .filter_map(|v| serde_json::from_value::(v).ok()) + .filter(|m| is_label_place(m.at)) + { + if by_id.len() < MAX_MOVED_LABELS || by_id.contains_key(&m.id) { + by_id.insert(m.id, m.at); } } - Ok(out) + Ok(by_id.into_iter().map(|(id, at)| MovedLabel { id, at }).collect()) } impl LabelClass { /// Where feature `id`'s label was moved to by hand, if it was. pub fn moved_to(&self, id: u64) -> Option<[f64; 2]> { - self.moved.iter().find(|m| m.id == id).map(|m| m.at) + self.moved.binary_search_by_key(&id, |m| m.id).ok().and_then(|i| self.moved.get(i)).map(|m| m.at) } /// Keeps feature `id`'s label at `at` (longitude, latitude), or where its class puts it - /// again with none. False when `at` isn't a place, or the class already keeps the most. + /// again with none. False when `at` isn't a place ([`is_label_place`]), or the class already + /// keeps the most. pub fn move_label(&mut self, id: u64, at: Option<[f64; 2]>) -> bool { - match at { - None => { - self.moved.retain(|m| m.id != id); + let found = self.moved.binary_search_by_key(&id, |m| m.id); + match (at, found) { + (None, Ok(i)) => { + self.moved.remove(i); true } - Some(at) if !at.iter().all(|v| v.is_finite()) => false, - Some(at) => { - if let Some(m) = self.moved.iter_mut().find(|m| m.id == id) { + (None, Err(_)) => true, + (Some(at), _) if !is_label_place(at) => false, + (Some(at), Ok(i)) => { + if let Some(m) = self.moved.get_mut(i) { m.at = at; - true - } else if self.moved.len() < MAX_MOVED_LABELS { - self.moved.push(MovedLabel { id, at }); - true - } else { - false } + true } + (Some(at), Err(i)) if self.moved.len() < MAX_MOVED_LABELS => { + self.moved.insert(i, MovedLabel { id, at }); + true + } + (Some(_), Err(_)) => false, } } } @@ -3381,12 +3389,24 @@ mod tests { assert!(c.move_label(7, Some([11.0, 21.0])), "moved again, the same feature's"); assert_eq!((c.moved_to(7), c.moved_to(8), c.moved.len()), (Some([11.0, 21.0]), Some([1.0, 2.0]), 2)); assert!(!c.move_label(9, Some([f64::NAN, 0.0])), "not a place"); + assert!(!c.move_label(9, Some([1000.0, 0.0])) && !c.move_label(9, Some([0.0, 95.0])), "off the map"); + assert!(c.move_label(2, Some([0.0, 0.0])) && c.moved.windows(2).all(|w| w[0].id < w[1].id), "kept in order of id"); + assert!(c.move_label(2, None) && c.move_label(2, None), "putting back one not moved is fine"); assert!(c.move_label(8, None) && c.moved_to(8).is_none(), "put back"); // Read from a file: bad entries dropped, the rest kept, a feature's last place wins. let mut v = serde_json::to_value(&c).unwrap(); - v["moved"] = serde_json::json!([{"id": 1, "at": [1, 1]}, {"id": "x"}, 7, {"id": 1, "at": [2, 2]}, {"id": 3, "at": [3, 3]}]); + v["moved"] = serde_json::json!([{"id": 1, "at": [1, 1]}, {"id": "x"}, 7, {"id": 1, "at": [2, 2]}, {"id": 3, "at": [3, 3]}, {"id": 0, "at": [1000, 95]}]); let read: LabelClass = serde_json::from_value(v.clone()).unwrap(); assert_eq!(read.moved, vec![MovedLabel { id: 1, at: [2.0, 2.0] }, MovedLabel { id: 3, at: [3.0, 3.0] }]); + // Out of order in the file: put in order, so each is still found. + v["moved"] = serde_json::json!([{"id": 9, "at": [9, 9]}, {"id": 4, "at": [4, 4]}]); + let read: LabelClass = serde_json::from_value(v.clone()).unwrap(); + assert_eq!((read.moved_to(4), read.moved_to(9)), (Some([4.0, 4.0]), Some([9.0, 9.0]))); + // A huge list reads quickly, and keeps the most. + let many: Vec<_> = (0..50_000u64).rev().map(|i| serde_json::json!({"id": i, "at": [0, 0]})).collect(); + v["moved"] = serde_json::Value::Array(many); + let read: LabelClass = serde_json::from_value(v.clone()).unwrap(); + assert_eq!(read.moved.len(), MAX_MOVED_LABELS); v["moved"] = serde_json::json!("nonsense"); assert!(serde_json::from_value::(v).unwrap().moved.is_empty(), "the rest still reads"); // None moved: none saved. diff --git a/crates/render/src/lib.rs b/crates/render/src/lib.rs index bd11ea4b0..85aaafc67 100644 --- a/crates/render/src/lib.rs +++ b/crates/render/src/lib.rs @@ -2085,15 +2085,22 @@ fn affine(src: [(f64, f64); 3], dst: [(f64, f64); 3]) -> Option { fn lift_labels(view: &View, ground: &terrain::Ground, candidates: &mut [Candidate]) { let cam = view.camera; let (w, h) = (view.width, view.height); - for Candidate { label: l, alternatives, .. } in candidates.iter_mut() { - let Some(m) = cam.try_to_map(f64::from(l.x), f64::from(l.y), w, h) else { continue }; + let lift = |x: f32, y: f32| { + let m = cam.try_to_map(f64::from(x), f64::from(y), w, h)?; let (u, v) = cam.ground_px(m); - let Some((sx, sy)) = cam.project_raised(u, v, ground.up_px(m, &cam), w, h) else { continue }; - let (dx, dy) = (sx as f32 - l.x, sy as f32 - l.y); - if !(dx.is_finite() && dy.is_finite()) { - continue; - } - (l.x, l.y) = (sx as f32, sy as f32); + let (sx, sy) = cam.project_raised(u, v, ground.up_px(m, &cam), w, h)?; + let (sx, sy) = (sx as f32, sy as f32); + (sx.is_finite() && sy.is_finite()).then_some((sx, sy)) + }; + for Candidate { label: l, alternatives, .. } in candidates.iter_mut() { + let Some((sx, sy)) = lift(l.x, l.y) else { continue }; + let (dx, dy) = (sx - l.x, sy - l.y); + // A callout's feature onto the ground under it, not under the label. + if let Some([ax, ay]) = l.collide.callout_anchor { + let (fx, fy) = lift(l.x + ax, l.y + ay).unwrap_or((l.x + ax + dx, l.y + ay + dy)); + l.collide.callout_anchor = Some([fx - sx, fy - sy]); + } + (l.x, l.y) = (sx, sy); if let Some(i) = &mut l.icon { (i.x, i.y) = (i.x + dx, i.y + dy); } @@ -2300,6 +2307,18 @@ fn raise_labels(map: &Map, view: &View, candidates: &mut [Candidate]) { let (u, v) = cam.ground_px(m); let Some((sx, sy)) = cam.project_raised(u, v, up, w, h) else { continue }; let (dx, dy) = (sx as f32 - l.x, sy as f32 - l.y); + // A callout's feature onto its roof where it is, not where the label is. + if let Some([ax, ay]) = l.collide.callout_anchor { + let (fx, fy) = (l.x + ax, l.y + ay); + let roof = cam + .try_to_map(f64::from(fx), f64::from(fy), w, h) + .map(|m| cam.ground_px(m)) + .and_then(|(u, v)| cam.project_raised(u, v, up, w, h)) + .map(|(x, y)| (x as f32, y as f32)) + .filter(|(x, y)| x.is_finite() && y.is_finite()); + let (fx, fy) = roof.unwrap_or((fx + dx, fy + dy)); + l.collide.callout_anchor = Some([fx - sx as f32, fy - sy as f32]); + } (l.x, l.y) = (sx as f32, sy as f32); if let Some(i) = &mut l.icon { (i.x, i.y) = (i.x + dx, i.y + dy); @@ -3536,6 +3555,17 @@ fn render_frame(map: &mut Map, view: &View, full_res_rasters: bool, backend: Bac .fold(0.0, f32::max); let offset_reach = f64::from(offset_reach); let (vw, vh) = (ctx.view.width, ctx.view.height); + // Labels moved by hand (the Move Label tool): any class's, its feature known by + // its id. Where one was put is in view or not whatever its feature's place. + let any_moved = labels.in_order().zip(&label_fields).any(|(c, f)| f.is_some() && !c.moved.is_empty()); + // (Kept as longitude and latitude: the flat map draws its projection's coordinates, + // as its features' `geoms` are; the globe draws longitude and latitude.) + let moved_to = |class: &rustgis_map::LabelClass, fid: Option| { + fid.and_then(|id| class.moved_to(id)) + .map(|[x, y]| Coord::new(x, y)) + .and_then(|c| if globe.is_some() { Some(c) } else { flat.forward_checked(c) }) + .and_then(|c| ctx.screen(c)) + }; for (row, g) in geoms.iter().enumerate() { let Some(g) = g else { continue }; if rows.get(row) != Some(&true) { @@ -3554,7 +3584,9 @@ fn render_frame(map: &mut Map, view: &View, full_res_rasters: bool, backend: Bac None }; let line_in_view = || globe.is_some() || ctx.cull.intersects(&extents.get(row).copied().unwrap_or_else(|| g.extent())); - if point.is_none() && !(any_along && matches!(g, Geometry::Line(_)) && line_in_view()) { + let fid = layer.dataset.features.get(row).map(|f| f.id); + let moved_here = || any_moved && classes.classes_of(ds_ptr, row).any(|(_, c)| moved_to(c, fid).is_some()); + if point.is_none() && !(any_along && matches!(g, Geometry::Line(_)) && line_in_view()) && !moved_here() { continue; } let is_point = g.kind() == GeometryType::Point; @@ -3579,9 +3611,14 @@ fn render_frame(map: &mut Map, view: &View, full_res_rasters: bool, backend: Bac if locked_classes.as_ref().is_some_and(|k| !k.contains(&(lid, label_row(row), ci))) { continue; } - if point.is_none() && !along_line(class.placement, g) { + // Moved by hand: one label, upright, where it was put. + let moved = if any_moved { moved_to(class, fid) } else { None }; + if point.is_none() && moved.is_none() && !along_line(class.placement, g) { continue; } + // Its feature's place, for a callout's line back to it: off the screen too + // when the label was moved into view. + let anchor = point.or_else(|| moved.and_then(|_| ctx.screen(c))); // (Along a line, the label is put on its stretch in view below.) let (x, y) = point.unwrap_or_default(); let Some(fi) = label_fields.get(ci).copied().flatten() else { continue }; @@ -3616,7 +3653,9 @@ fn render_frame(map: &mut Map, view: &View, full_res_rasters: bool, backend: Bac // Where it goes, each with its repeat's index: by its feature (0), or along // a line at its middle and, repeated, every so far either side. let mut places: Vec<(usize, f32, f32, Option, Vec)> = Vec::new(); - if let (LabelPlacement::Parallel | LabelPlacement::Curved, Geometry::Line(parts)) = (class.placement, g) { + if let Some((mx, my)) = moved { + places.push((0, mx as f32, my as f32, None, Vec::new())); + } else if let (LabelPlacement::Parallel | LabelPlacement::Curved, Geometry::Line(parts)) = (class.placement, g) { let line = screen_line.get_or_insert_with(|| longest_on_screen(ctx, parts).and_then(|l| line_labels::Line::new(&l))); let Some(line) = line.as_ref() else { continue }; @@ -3652,20 +3691,6 @@ fn render_frame(map: &mut Map, view: &View, full_res_rasters: bool, backend: Bac let (lx, ly) = place(class.placement); places.push((0, lx, ly, None, Vec::new())); } - // Moved by hand (the Move Label tool): one label, upright, where it was - // put, its feature known by its id. - let fid = layer.dataset.features.get(row).map(|f| f.id); - // (Kept as longitude and latitude: the flat map draws its projection's - // coordinates, as its features' `geoms` are; the globe draws longitude and - // latitude.) - let moved = fid - .and_then(|id| class.moved_to(id)) - .map(|at| Coord::new(at[0], at[1])) - .and_then(|c| if globe.is_some() { Some(c) } else { flat.forward_checked(c) }) - .and_then(|c| ctx.screen(c)); - if let Some((mx, my)) = moved { - places = vec![(0, mx as f32, my as f32, None, Vec::new())]; - } // Layers above first, then the classes listed first (#732), then bigger // polygons: a class (ci ≤ MAX_LABEL_CLASSES, 1e9 each) never outweighs a // layer (1e12), nor an area (≤ 1e8) a class. A line's repeats come after @@ -3724,7 +3749,7 @@ fn render_frame(map: &mut Map, view: &View, full_res_rasters: bool, backend: Bac } else { 0.0 }, - callout_anchor: point.is_some().then_some([x as f32 - lx, y as f32 - ly]), + callout_anchor: anchor.map(|(ax, ay)| [ax as f32 - lx, ay as f32 - ly]), pinned: moved.is_some(), source: fid.map(|id| (lid, id, ci as u32)), ..Default::default() @@ -4497,9 +4522,14 @@ const MAX_CALLOUT_SEARCHES: usize = 500; /// A label's text's extent (points, `[x0, y0, x1, y1]`), as placement keeps it clear of the /// others: what the Move Label tool picks it by. pub fn label_extent(l: &PlacedLabel) -> Option<[f32; 4]> { - let (text, _) = label_boxes(l); - let e = - text.iter().fold([f32::MAX, f32::MAX, f32::MIN, f32::MIN], |e, b| [e[0].min(b[0]), e[1].min(b[1]), e[2].max(b[2]), e[3].max(b[3])]); + boxes_extent(&label_boxes(l).0) +} + +/// The extent of boxes (`[x0, y0, x1, y1]` each); none for none. +fn boxes_extent(boxes: &[[f32; 4]]) -> Option<[f32; 4]> { + let e = boxes + .iter() + .fold([f32::MAX, f32::MAX, f32::MIN, f32::MIN], |e, b| [e[0].min(b[0]), e[1].min(b[1]), e[2].max(b[2]), e[3].max(b[3])]); (e[0] <= e[2] && e[1] <= e[3]).then_some(e) } @@ -4515,9 +4545,8 @@ pub fn callout_to(l: &PlacedLabel) -> Option<[f32; 4]> { /// A callout's line, from its feature at `anchor` to the nearest edge of the text's boxes; none /// when the text covers the feature. fn callout_line(text: &[[f32; 4]], anchor: [f32; 2]) -> Option<[f32; 4]> { - let e = - text.iter().fold([f32::MAX, f32::MAX, f32::MIN, f32::MIN], |e, b| [e[0].min(b[0]), e[1].min(b[1]), e[2].max(b[2]), e[3].max(b[3])]); - if e[0] > e[2] || !anchor.iter().all(|v| v.is_finite()) { + let e = boxes_extent(text)?; + if !anchor.iter().all(|v| v.is_finite()) { return None; } let to = [anchor[0].clamp(e[0], e[2]), anchor[1].clamp(e[1], e[3])]; @@ -4536,14 +4565,14 @@ fn callout_spot( w: f32, h: f32, ) -> Option<(f32, f32, [f32; 4])> { - let ext = - text.iter().fold([f32::MAX, f32::MAX, f32::MIN, f32::MIN], |e, b| [e[0].min(b[0]), e[1].min(b[1]), e[2].max(b[2]), e[3].max(b[3])]); - if !(anchor[0].is_finite() && anchor[1].is_finite() && reach.is_finite()) || ext[0] > ext[2] { + let ext = boxes_extent(text)?; + if !(anchor[0].is_finite() && anchor[1].is_finite() && reach.is_finite()) { return None; } - // Out in rings half the text's height apart, as far as its line may reach past its size. - let step = ((ext[3] - ext[1]) * 0.5).clamp(2.0, 20.0); + // Out in rings half the text's height apart, as far as its line may reach past its size: at + // most 60 of them, farther apart for a long reach, so the last still gets that far. let far = reach + (ext[2] - ext[0]).max(ext[3] - ext[1]); + let step = ((ext[3] - ext[1]) * 0.5).clamp(2.0, 20.0).max(far / 60.0); let rings = ((far / step).ceil() as usize).clamp(1, 60); for ring in 1..=rings { let d = ring as f32 * step; @@ -5510,9 +5539,6 @@ mod tests { assert!(starfield(1e9).is_some_and(|t| t.width() == 2048)); } - /// Each label class labels every feature its clause takes, as in ArcGIS Pro (#732): a feature - /// two classes take gets a label from each, the class listed first placed first. A hidden - /// class labels none, and a clause that no longer reads takes none (#618). /// A label moved by hand is drawn where it was put, on the flat map: its place is kept as /// longitude and latitude, and drawn through the map's projection as its feature is. /// (Taken for the projection's own coordinates, it was drawn off by Null Island instead.) @@ -5547,6 +5573,61 @@ mod tests { assert!(!lyon.collide.pinned && lyon.collide.callout.is_none()); } + /// A label moved by hand into view is drawn whatever its feature's place: a point's, or a + /// line's labelled along it, out of view, its callout's line reaching back off the screen. + #[test] + fn a_label_moved_into_view_is_drawn_with_its_feature_out_of_it() { + let mut m = Map::new("Map"); + let mut d = Dataset::new("pts", GeometryType::Point, vec![Field::new("name", FieldType::Text)]); + d.push(Some(Geometry::Point(Coord::new(2.35, 48.85))), vec![Value::Text("Paris".into())]); + let pts = m.add_dataset(d); + let mut d = Dataset::new("lines", GeometryType::Polyline, vec![Field::new("name", FieldType::Text)]); + d.push(Some(Geometry::Line(vec![vec![Coord::new(1.0, 44.0), Coord::new(3.0, 44.0)]])), vec![Value::Text("Garonne".into())]); + let lines = m.add_dataset(d); + for (id, to) in [(pts, [20.0, 50.0]), (lines, [20.0, 47.0])] { + let fid = m.layer(id).unwrap().dataset.features.first().unwrap().id; + let l = m.layer_mut(id).unwrap(); + l.labels.enabled = true; + l.labels.default.callout = Some(40.0); + if id == lines { + l.labels.default.placement = LabelPlacement::Parallel; + } + assert!(l.labels.default.move_label(fid, Some(to))); + } + // Over Central Europe: France is off the left of the view. + let camera = Camera { center: lonlat_to_merc(Coord::new(20.0, 48.5)), res: 2000.0, bearing: 0.0, pitch: 0.0 }; + let view = View { width: 800.0, height: 600.0, pixels_per_point: 1.0, camera, sky: false }; + let on_screen = |m: &Map, lon: f64, lat: f64| view.to_screen(m.to_map(Coord::new(lon, lat)).unwrap()); + assert!(on_screen(&m, 2.35, 48.85).0 < 0.0 && on_screen(&m, 3.0, 44.0).0 < 0.0, "the features are out of view"); + let labels = render(&mut m, &view).unwrap().labels; + for (text, at) in [("Paris", (20.0, 50.0)), ("Garonne", (20.0, 47.0))] { + let l = labels.iter().find(|l| l.text == text).unwrap_or_else(|| panic!("{text}'s label is drawn")); + let (ex, ey) = on_screen(&m, at.0, at.1); + assert!((f64::from(l.x) - ex).abs() < 0.5 && (f64::from(l.y) - ey).abs() < 0.5, "{text}"); + assert!(l.collide.pinned && l.angle.is_none() && l.glyphs.is_empty(), "{text}: upright where it was put"); + let [x0, ..] = l.collide.callout.unwrap_or_else(|| panic!("{text}: a line back to its feature")); + assert!(l.x + x0 < 0.0, "{text}: its line starts off the screen"); + } + } + + /// A long callout is looked for as far as it may reach (the search's rings spaced out to get + /// there), not only near the label. + #[test] + fn a_long_callout_reaches_a_far_free_spot() { + // A small text by its feature at (100, 100); only spots 200 points right of it are free. + let text = [[95.0, 98.0, 105.0, 102.0]]; + let free = |b: &[f32; 4]| b[0] > 300.0; + let (dx, _, [x0, y0, x1, y1]) = callout_spot(&text, [100.0, 100.0], 300.0, free, 1000.0, 200.0).expect("a spot"); + assert!(dx > 195.0); + let len = (x1 - x0).hypot(y1 - y0); + assert!((150.0..=300.0).contains(&len), "{len} points"); + // Beyond its reach, none. + assert!(callout_spot(&text, [100.0, 100.0], 100.0, free, 1000.0, 200.0).is_none()); + } + + /// Each label class labels every feature its clause takes, as in ArcGIS Pro (#732): a feature + /// two classes take gets a label from each, the class listed first placed first. A hidden + /// class labels none, and a clause that no longer reads takes none (#618). #[test] fn label_classes_label_their_features() { let mut m = Map::new("Map"); diff --git a/crates/ui-egui/src/mapview.rs b/crates/ui-egui/src/mapview.rs index 4a0421d8a..7d26a2df0 100644 --- a/crates/ui-egui/src/mapview.rs +++ b/crates/ui-egui/src/mapview.rs @@ -54,13 +54,9 @@ enum DragKind { row: usize, origin: Pos2, }, - /// Moving a layer's label with the Move Label tool: its layer's id, its feature's id, its - /// label class (in the layer's labeling order) and where the press was. The label as it was - /// drawn is in `MapView::moving_label`. + /// Moving a layer's label with the Move Label tool: where the press was. The label, and what + /// it labels, are in `MapView::moving_label`. Label { - layer: u32, - feature: u64, - class: u32, origin: Pos2, }, } @@ -260,6 +256,10 @@ pub struct MapView { /// Where on a tilted map's ground each label is (the camera's coordinates), so it stays there /// while the map moves. label_map: Vec>, + /// Where each label's callout starts (its feature), as `label_lonlat` and `label_map` keep the + /// labels: its line stays on its feature while the map moves. + callout_lonlat: Vec>, + callout_map: Vec>, /// The camera the labels were placed for. label_camera: Camera, /// The map revision whose GPU scene had only live tile layers (#635): tiles arriving draw @@ -271,8 +271,8 @@ pub struct MapView { view_px: [usize; 2], resized_at: f64, drag: Option<(Pos2, DragKind)>, - /// The label being moved with the Move Label tool, as it was drawn when grabbed. - moving_label: Option, + /// The label being moved with the Move Label tool. + moving_label: Option, sketch: Vec, selection_gesture: Option, radius_preview: Option<(Coord, f64, Vec>)>, @@ -371,6 +371,8 @@ impl Default for MapView { motion_cost: 0.0, labels: Vec::new(), label_lonlat: Vec::new(), + callout_lonlat: Vec::new(), + callout_map: Vec::new(), label_camera: Camera::world(), live_rev: None, last_interaction: -10.0, @@ -497,6 +499,15 @@ impl MapView { self.selection_gesture = None; self.radius_preview = None; self.cancel_vertex_drag(); + self.cancel_label_drag(); + } + + /// Drops a label being moved with the Move Label tool, leaving it where it was. + pub(crate) fn cancel_label_drag(&mut self) { + self.moving_label = None; + if self.drag.is_some_and(|(_, kind)| matches!(kind, DragKind::Label { .. })) { + self.drag = None; + } } /// Cancels a vertex preview and its pending release together, leaving other map gestures @@ -834,6 +845,8 @@ fn main_view(app: &mut RustGisApp, ui: &mut Ui, t: &Tokens) { app.map.labels.clear(); app.map.label_lonlat.clear(); app.map.label_map.clear(); + app.map.callout_lonlat.clear(); + app.map.callout_map.clear(); } let settling = (tilted || globe_view || tiled_flat) && !moving && app.ws.playing.is_none() && !hold; let mut tilt_draped = tilt_draped; @@ -1190,18 +1203,22 @@ fn main_view(app: &mut RustGisApp, ui: &mut Ui, t: &Tokens) { let placed = app.map.label_camera; if globe_view && (placed.center != cam.center || placed.res != cam.res) { let g = Globe::new(&cam, w, h); - for (l, c) in app.map.labels.iter().zip(&app.map.label_lonlat) { - if let Some((x, y)) = c.and_then(|c| g.forward(c)) { - draw_label(&painter, l, rect.min + vec2(x as f32, y as f32)); + let screen = |c: &Option| c.and_then(|c| g.forward(c)).map(|(x, y)| rect.min + vec2(x as f32, y as f32)); + for (i, (l, c)) in app.map.labels.iter().zip(&app.map.label_lonlat).enumerate() { + if let Some(at) = screen(c) { + let feature = app.map.callout_lonlat.get(i).and_then(screen); + draw_label(&painter, &callout_from(l, at, feature), at); } } } else if (tilt_draped || (app.map.gpu_frame.is_some() && !app.map.label_map.is_empty())) && placed != cam { // On a moving tilted (or, on the GPU, turned) map each label stays on its ground; under // the sky it drops out. let sky = cam.sky_y(h).unwrap_or(0.0); - for (l, m) in app.map.labels.iter().zip(&app.map.label_map) { - if let Some((x, y)) = m.and_then(|m| cam.try_to_screen(m, w, h)).filter(|s| s.1 > sky) { - draw_label(&painter, l, rect.min + vec2(x as f32, y as f32)); + let screen = |m: &Option| m.and_then(|m| cam.try_to_screen(m, w, h)).map(|(x, y)| rect.min + vec2(x as f32, y as f32)); + for (i, (l, m)) in app.map.labels.iter().zip(&app.map.label_map).enumerate() { + if let Some(at) = screen(m).filter(|at| at.y - rect.min.y > sky as f32) { + let feature = app.map.callout_map.get(i).and_then(screen); + draw_label(&painter, &callout_from(l, at, feature), at); } } } else { @@ -1245,9 +1262,16 @@ fn show_gpu_frame(app: &mut RustGisApp, scene: rustgis_engine::render::gpu_scene app.map.labels = scene.labels; app.map.labels.splice(0..0, scene.unplaced); app.map.label_lonlat.clear(); - app.map.label_map = match cam.bearing != 0.0 || cam.pitch != 0.0 { - true => app.map.labels.iter().map(|l| cam.try_to_map(l.x as f64, l.y as f64, view.width, view.height)).collect(), - false => Vec::new(), + app.map.callout_lonlat.clear(); + (app.map.label_map, app.map.callout_map) = match cam.bearing != 0.0 || cam.pitch != 0.0 { + true => { + let to_map = |x: f32, y: f32| cam.try_to_map(x as f64, y as f64, view.width, view.height); + ( + app.map.labels.iter().map(|l| to_map(l.x, l.y)).collect(), + app.map.labels.iter().map(|l| l.collide.callout.and_then(|c| to_map(l.x + c[0], l.y + c[1]))).collect(), + ) + } + false => (Vec::new(), Vec::new()), }; app.map.label_camera = cam; app.map.attribution = scene.attribution; @@ -1288,16 +1312,26 @@ fn show_frame( if !(globe_view && key.draft) { app.map.labels = r.labels; app.map.labels.splice(0..0, r.unplaced); - app.map.label_lonlat = match globe_view { + (app.map.label_lonlat, app.map.callout_lonlat) = match globe_view { true => { let g = Globe::new(&cam, w, h); - app.map.labels.iter().map(|l| g.inverse(l.x as f64, l.y as f64)).collect() + let at = |x: f32, y: f32| g.inverse(x as f64, y as f64); + ( + app.map.labels.iter().map(|l| at(l.x, l.y)).collect(), + app.map.labels.iter().map(|l| l.collide.callout.and_then(|c| at(l.x + c[0], l.y + c[1]))).collect(), + ) } - false => Vec::new(), + false => (Vec::new(), Vec::new()), }; - app.map.label_map = match !globe_view && cam.pitch != 0.0 { - true => app.map.labels.iter().map(|l| cam.try_to_map(l.x as f64, l.y as f64, w, h)).collect(), - false => Vec::new(), + (app.map.label_map, app.map.callout_map) = match !globe_view && cam.pitch != 0.0 { + true => { + let to_map = |x: f32, y: f32| cam.try_to_map(x as f64, y as f64, w, h); + ( + app.map.labels.iter().map(|l| to_map(l.x, l.y)).collect(), + app.map.labels.iter().map(|l| l.collide.callout.and_then(|c| to_map(l.x + c[0], l.y + c[1]))).collect(), + ) + } + false => (Vec::new(), Vec::new()), }; app.map.label_camera = cam; } @@ -1420,17 +1454,63 @@ fn place_galley(anchor: Align2, at: Pos2, g: &egui::Galley) -> (Pos2, Rect) { (r.min - g.rect.min.to_vec2(), r) } +/// A label being moved with the Move Label tool: as it was drawn when grabbed (in the view's +/// points), what it labels, and the map and label class it is of, named then (so it lands in +/// those, whatever is reordered or shown while it moves). +struct MovingLabel { + label: PlacedLabel, + layer: u32, + feature: u64, + class: String, + project: u64, + map: usize, +} + /// The layer's label drawn at a point of the map view (relative to it as `rect` is), topmost first: -/// what the Move Label tool grabs. Labels not of a layer's features aren't. +/// what the Move Label tool grabs, as it is drawn now. Labels not of a layer's features aren't; +/// nor any while a moving globe or tilted map draws them from where they were placed. fn label_at(app: &RustGisApp, rect: Rect, p: Pos2) -> Option { + let cam = app.session.map().camera; + let placed = app.map.label_camera; + // A flat map moving draws the labels placed last with its frame: shifted, and spread apart + // or closed up as it zooms (as `main_view` does). + let (shift, s) = if placed == cam { + (vec2(0.0, 0.0), 1.0) + } else if app.session.map().projection != Projection::Globe && placed.pitch == 0.0 && cam.pitch == 0.0 && placed.bearing == cam.bearing + { + (app.map.tex_rect.min - rect.min, (app.map.tex_camera.res / cam.res) as f32) + } else { + return None; + }; + if !(s.is_finite() && s > 0.0) { + return None; + } let at = p - rect.min; app.map.labels.iter().rev().filter(|l| l.collide.source.is_some() && !l.text.is_empty()).find_map(|l| { - let e = rustgis_engine::render::label_extent(l)?; + let mut l = l.clone(); + // Where it is drawn now: its text at its own size, its callout's feature where it is. + (l.x, l.y) = (shift.x + l.x * s, shift.y + l.y * s); + l.collide.callout_anchor = l.collide.callout_anchor.map(|[ax, ay]| [ax * s, ay * s]); + l.collide.callout = l.collide.callout.map(|[x0, y0, x1, y1]| [x0 * s, y0 * s, x1, y1]); + let e = rustgis_engine::render::label_extent(&l)?; let r = Rect::from_min_max(pos2(e[0], e[1]), pos2(e[2], e[3])).expand(3.0); - r.contains(pos2(at.x, at.y)).then(|| l.clone()) + r.contains(pos2(at.x, at.y)).then_some(l) }) } +/// A label drawn at `at` while the map moves, its callout's line starting at its feature where +/// that now is (none: as it was placed). +fn callout_from(l: &PlacedLabel, at: Pos2, feature: Option) -> std::borrow::Cow<'_, PlacedLabel> { + match (l.collide.callout, feature) { + (Some([_, _, x1, y1]), Some(f)) => { + let mut l = l.clone(); + l.collide.callout = Some([f.x - at.x, f.y - at.y, x1, y1]); + std::borrow::Cow::Owned(l) + } + _ => std::borrow::Cow::Borrowed(l), + } +} + /// A label moved by `d` (points): its text there, its feature where it was, and its callout's line /// from the feature to the text when its class has callouts. fn moved_label(l: &PlacedLabel, d: egui::Vec2) -> PlacedLabel { @@ -2274,9 +2354,11 @@ fn interact(app: &mut RustGisApp, ui: &mut Ui, resp: &egui::Response, rect: Rect DragKind::Text { layer, row, origin } } else if let Some(l) = label && let Some((layer, feature, class)) = l.collide.source + && let Some(class) = label_class_name(app, layer, class) { - app.map.moving_label = Some(l); - DragKind::Label { layer, feature, class, origin } + let (project, map) = (app.session.opened, app.session.project.active_map); + app.map.moving_label = Some(MovingLabel { label: l, layer, feature, class, project, map }); + DragKind::Label { origin } } else if middle { DragKind::Pan } else if tool == Tool::EditVertices && crate::vertexedit::drag_start(app, rect, origin) { @@ -2320,16 +2402,24 @@ fn interact(app: &mut RustGisApp, ui: &mut Ui, resp: &egui::Response, rect: Rect } // A label being moved shows where it will land, its callout's line following it; it is kept // there, as one undo step, on release. - if let Some((_, DragKind::Label { layer, feature, class, origin })) = app.map.drag { + // Esc, another tool, or another map or project shown drops it where it was. + if let Some((_, DragKind::Label { origin })) = app.map.drag { + let same = app.map.moving_label.as_ref().is_some_and(|m| { + m.project == app.session.opened && m.map == app.session.project.active_map && app.session.tool == Tool::MoveLabel + }); + if !same || ui.input(|i| i.key_pressed(egui::Key::Escape)) { + app.map.cancel_label_drag(); + return; + } let end = resp.interact_pointer_pos().or_else(|| ui.input(|i| i.pointer.latest_pos())); - let moved = app.map.moving_label.clone().zip(end).map(|(l, end)| moved_label(&l, end - origin)); + let moved = app.map.moving_label.as_ref().zip(end).map(|(m, end)| moved_label(&m.label, end - origin)); if resp.drag_stopped() { app.map.drag = None; - app.map.moving_label = None; let to = moved.and_then(|l| screen_to_lonlat(app, rect, rect.min + vec2(l.x, l.y))); - if let (Some(c), Some(name)) = (to, label_class_name(app, layer, class)) { - let _ = app.run("labels.move", json!({"layer": layer, "class": name, "feature": feature, "to": [c.x, c.y]})); + if let (Some(c), Some(m)) = (to, app.map.moving_label.take()) { + let _ = app.run("labels.move", json!({"layer": m.layer, "class": m.class, "feature": m.feature, "to": [c.x, c.y]})); } + app.map.moving_label = None; return; } if let Some(l) = moved { @@ -2549,7 +2639,8 @@ fn interact(app: &mut RustGisApp, ui: &mut Ui, resp: &egui::Response, rect: Rect // Labels move by dragging them; Shift-clicking a moved one puts it back. Tool::MoveLabel => { if shift - && let Some((layer, feature, class)) = label_at(app, rect, p).and_then(|l| l.collide.source) + && let Some((layer, feature, class)) = + label_at(app, rect, p).filter(|l| l.collide.pinned).and_then(|l| l.collide.source) && let Some(name) = label_class_name(app, layer, class) { let _ = app.run("labels.move", json!({"layer": layer, "class": name, "feature": feature, "to": null})); @@ -4278,6 +4369,70 @@ mod tests { h.state().session.map().layer_by_name("Points").unwrap().selection.clone() } + /// The Move Label tool: a label dragged and let go is kept where it was put, as one undo + /// step; Esc while dragging, or another tool, drops it where it was; Shift-click puts a moved + /// label back and leaves one never moved (no undo step). + #[test] + fn move_label_drags_cancels_and_puts_back() { + let mut h = selection_harness(); + for (i, point) in [[0.5, 0.5], [3.5, 2.0], [2.5, 0.5], [1.0, 4.0]].into_iter().enumerate() { + let p = json!({"layer": "Labelled", "geometry": {"type": "Point", "coordinates": point}, "attributes": {"name": format!("Place {i}")}}); + if i == 0 { + h.state_mut().run("layer.template", json!({"template": "points", "name": "Labelled"})).unwrap(); + } + h.state_mut().run("edit.addFeature", p).unwrap(); + } + let id = h.state().session.map().layer_by_name("Labelled").unwrap().id; + if let Some(l) = h.state_mut().session.map_mut().layer_mut(id) { + l.labels.enabled = true; + l.labels.default.field = "name".into(); + } + h.state_mut().run("tool.set", json!({"tool": "moveLabel"})).unwrap(); + let end = std::time::Instant::now() + std::time::Duration::from_secs(10); + while !h.state().map.labels.iter().any(|l| l.text == "Place 3") || h.state().map.drawing() { + assert!(std::time::Instant::now() < end, "labels never drawn"); + std::thread::sleep(std::time::Duration::from_millis(2)); + h.step(); + } + let rect = h.state().map.view_rect.unwrap().0; + let grab = |h: &egui_kittest::Harness<'_, RustGisApp>| { + let l = h.state().map.labels.iter().find(|l| l.text == "Place 3").unwrap(); + rect.min + vec2(l.x, l.y) + }; + let moved = |h: &egui_kittest::Harness<'_, RustGisApp>| h.state().session.map().layer(id).unwrap().labels.default.moved.clone(); + let none = egui::Modifiers::NONE; + let drag = |h: &mut egui_kittest::Harness<'_, RustGisApp>, escape: bool| { + let at = grab(h); + selection_pointer(h, at, None, none); + selection_pointer(h, at, Some(true), none); + for i in 1..=6 { + selection_pointer(h, at + vec2(0.0, -8.0 * i as f32), None, none); + } + if escape { + h.event(egui::Event::Key { key: egui::Key::Escape, physical_key: None, pressed: true, repeat: false, modifiers: none }); + h.step(); + } + selection_pointer(h, at + vec2(0.0, -48.0), Some(false), none); + }; + // Esc mid-drag: nothing saved. + drag(&mut h, true); + assert!(moved(&h).is_empty(), "Esc cancels the move"); + assert!(h.state().map.moving_label.is_none()); + // Dragged and let go: kept. + h.state_mut().run("tool.set", json!({"tool": "moveLabel"})).unwrap(); + h.step(); + drag(&mut h, false); + assert_eq!(moved(&h).len(), 1, "the move is kept"); + // Shift-click on a label never moved: no undo step, so undo takes back the move. + let other = h.state().map.labels.iter().find(|l| l.text == "Place 1" && !l.collide.pinned).map(|l| rect.min + vec2(l.x, l.y)); + let at = other.expect("another label drawn"); + let shift = egui::Modifiers { shift: true, ..none }; + selection_pointer(&mut h, at, Some(true), shift); + selection_pointer(&mut h, at, Some(false), shift); + h.state_mut().run("edit.undo", json!({})).unwrap(); + assert!(moved(&h).is_empty(), "the Shift-click left no step of its own"); + } + #[test] fn selection_tools_polygon_menu_repeat_and_fixed_default() { use egui_kittest::kittest::Queryable; From 3e8f4f3fb1d05c0916f69cf4bb3ff2f4cad5bffa Mon Sep 17 00:00:00 2001 From: huzaifffa Date: Sat, 31 Oct 2026 00:02:19 +0300 Subject: [PATCH 03/15] fix(labels): keep a callout's move when locked, and follow up on review - A lock keeps how far a callout moved a label (`LockedPlace`), so a locked label stays there with its line. - A callout's feature end is carried into turned and tilted views, and scales with a flat map's zoom while it moves. - Converted annotations stay moved under their new ids, so a class's offset doesn't shift them. - A failed drop says why; a class at its limit drops moves of deleted features. - A project file's callout of 0 or less is off. - Callout searches share a budget of spots per placement; moved-label lookup comes before reading a class's clause. - "Reset Moved" is one name; tests for Shift-click on a moved label and Reset Moved with a named class. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/labels.rs | 26 +++++- crates/engine/src/tests.rs | 18 +++- crates/map/src/lib.rs | 16 +++- crates/map/src/symbology.rs | 5 +- crates/render/src/layout.rs | 2 +- crates/render/src/lib.rs | 141 ++++++++++++++++++++++++++---- crates/render/src/stream_tests.rs | 2 +- crates/tiles/src/symbols.rs | 4 + crates/ui-egui/src/mapview.rs | 28 +++++- 9 files changed, 209 insertions(+), 33 deletions(-) diff --git a/crates/engine/src/cmd/labels.rs b/crates/engine/src/cmd/labels.rs index c195a83ba..8e5379aff 100644 --- a/crates/engine/src/cmd/labels.rs +++ b/crates/engine/src/cmd/labels.rs @@ -212,8 +212,18 @@ pub fn commands() -> Vec { } } }; + // A class at its most drops the moves of features since deleted, to make room. + let layer = s.layer_id(p).ok().and_then(|id| s.map().layer(id)); + let live: Option> = layer + .filter(|l| l.labels.in_order().any(|c| c.moved.len() >= rustgis_map::symbology::MAX_MOVED_LABELS)) + .map(|l| l.dataset.features.iter().map(|f| f.id).collect()); let r = with_labels(s, p, |labels| { let c = class_in(labels, p)?; + if let Some(live) = &live + && c.moved.len() >= rustgis_map::symbology::MAX_MOVED_LABELS + { + c.moved.retain(|m| live.contains(&m.id)); + } if c.move_label(feature, to) { Ok(()) } else { @@ -227,7 +237,7 @@ pub fn commands() -> Vec { }) .when(need_feature_layer) .params(r#"{"layer"?, "class"?: name (none: the Default class), "feature": the feature's id, "to": [x, y] (longitude, latitude: where the label's text is centred) | null (back where its class puts it)}"#), - C::new("labels.resetMoved", "Reset Moved Labels", "Labeling › Label Placement", |s, p| { + C::new("labels.resetMoved", "Reset Moved", "Labeling › Label Placement", |s, p| { let all = match p.get("all") { None | Some(Value::Null) => false, Some(v) => v.as_bool().ok_or("all must be true or false")?, @@ -515,6 +525,7 @@ fn convert(s: &mut Session, p: &Value) -> CmdResult { GeometryType::Point, out_fields, ); + let mut moved = Vec::new(); for (row, ft) in l.dataset.features.iter().enumerate() { if rows.get(row) != Some(&true) { continue; @@ -530,9 +541,14 @@ fn convert(s: &mut Session, p: &Value) -> CmdResult { if classes { attrs.push(Attr::Text(class.name.clone())); } - // A label moved by hand becomes an annotation where it was put. - let at = class.moved_to(ft.id).map_or(at, |[x, y]| Coord::new(x, y)); + // A label moved by hand becomes an annotation where it was put, and stays moved there + // (its class's offset leaves it). + let put = class.moved_to(ft.id); + let at = put.map_or(at, |[x, y]| Coord::new(x, y)); ds.push(Some(Geometry::Point(at)), attrs); + if let (Some(put), Some(new)) = (put, ds.features.last()) { + moved.push((i, new.id, put)); + } } } if ds.features.is_empty() { @@ -554,6 +570,10 @@ fn convert(s: &mut Session, p: &Value) -> CmdResult { // with no clause it would label theirs as well. c.where_ = classes.then(|| format!("\"CLASS\" = '{}'", c.name.replace('\'', "''"))); } + // (Numbered as the labeling order is: the classes, then Default.) + for (class, id, at) in moved { + labels.classes.get_mut(class).unwrap_or(&mut labels.default).move_label(id, Some(at)); + } let (min_scale, max_scale) = (l.min_scale, l.max_scale); if let Some(src) = s.map_mut().layer_mut(id) { src.labels.enabled = false; diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index b09cc5e79..b26f7e37d 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -2454,8 +2454,12 @@ fn labels_are_moved_by_command_and_reset() { // A bad "all" is refused, not taken as false. assert!(s.run("labels.resetMoved", &json!({"layer": id, "all": "tru"})).is_err()); assert_eq!(moved(&s).len(), 1); + s.run("labels.class.add", &json!({"layer": id, "name": "Named"})).unwrap(); + s.run("labels.move", &json!({"layer": id, "class": "Named", "feature": fid, "to": [3, 3]})).unwrap(); s.run("labels.resetMoved", &json!({"layer": id, "all": true})).unwrap(); assert!(moved(&s).is_empty()); + assert!(s.map().layer(id).unwrap().labels.class("Named").unwrap().moved.is_empty(), "every class's"); + s.run("labels.class.remove", &json!({"layer": id, "class": "Named"})).unwrap(); // Locked, a move locks the labels again where they are placed next: the one moved may not // have been placed when the lock was taken. s.run("labels.lock", &json!({"on": true})).unwrap(); @@ -2469,12 +2473,13 @@ fn labels_are_moved_by_command_and_reset() { s.run("labels.lock", &json!({"on": false})).unwrap(); let r = s.run("labels.convert", &json!({"layer": id})).unwrap(); let a = s.map().layer(r["layer"].as_u64().unwrap() as u32).unwrap(); - assert!(a.labels.default.moved.is_empty()); + // It stays moved there under its new id, so its class's offset doesn't shift it. let at = |g: &Option| match g { Some(rustgis_data::Geometry::Point(c)) => Some([c.x, c.y]), _ => None, }; - assert!(a.dataset.features.iter().any(|f| at(&f.geometry) == Some([2.0, 2.0])), "the moved one's annotation"); + let put = a.dataset.features.iter().find(|f| at(&f.geometry) == Some([2.0, 2.0])).expect("the moved one's annotation"); + assert_eq!(a.labels.default.moved, vec![rustgis_map::symbology::MovedLabel { id: put.id, at: [2.0, 2.0] }]); // The tool by its code. s.run("tool.set", &json!({"tool": "moveLabel"})).unwrap(); assert_eq!(s.tool, crate::Tool::MoveLabel); @@ -2482,6 +2487,13 @@ fn labels_are_moved_by_command_and_reset() { #[test] fn labels_show_all_and_take_callouts() { + // A project file's 0 is off, as the command's is; a length is kept. + let ds = rustgis_data::Dataset::new("d", rustgis_data::GeometryType::Point, vec![]); + let mut v = serde_json::to_value(rustgis_map::LabelClass::default_for(&ds)).unwrap(); + for (file, read) in [(json!(0), None), (json!(-3), None), (json!(30), Some(30.0))] { + v["callout"] = file; + assert_eq!(serde_json::from_value::(v.clone()).unwrap().callout, read); + } let mut s = world(); let class = |s: &Session| { let c = &s.map().layer_by_name("Cities").unwrap().labels.default; @@ -16738,7 +16750,7 @@ fn ogc_group_refresh_keeps_children_across_rotated_keys_and_rejects_bad_answers( let group = added["group"].clone(); let ids: Vec = s.map().service_groups[0].children.iter().map(|c| c.layer).collect(); s.run("layer.rename", &json!({"layer":ids[0], "name":"Renamed"})).unwrap(); - s.map_mut().label_lock = Some(vec![((ids[0], 0, 0, 0), 0), ((ids[1], 0, 0, 0), 0)]); + s.map_mut().label_lock = Some(vec![((ids[0], 0, 0, 0), Default::default()), ((ids[1], 0, 0, 0), Default::default())]); // A rotated key changes every source address but not the collections: the children stay. let mut q = doc("rotated-key"); diff --git a/crates/map/src/lib.rs b/crates/map/src/lib.rs index 975e4a574..539af1e11 100644 --- a/crates/map/src/lib.rs +++ b/crates/map/src/lib.rs @@ -40,6 +40,15 @@ pub use symbology::{ }; pub use tiles::{TileKind, TileLayer}; +/// Where a locked label stays. +#[derive(Clone, Copy, Debug, Default, PartialEq)] +pub struct LockedPlace { + /// The place it took: 0 its own, else one of its alternatives (#1011), numbered from 1. + pub place: u8, + /// How far a callout moved it from there (points, right and down), if one did. + pub callout: Option<[f32; 2]>, +} + use rustgis_data::{Dataset, Geometry, GeometryType}; use rustgis_geom::{Coord, Extent}; use serde::{Deserialize, Serialize}; @@ -1159,11 +1168,10 @@ pub struct Map { /// Map-wide label display switches. #[serde(default)] pub label_display: LabelDisplay, - /// The labels placed when labels were locked, each with the place it took (0 its own, else - /// one of its alternatives, #1011): only they are drawn while the lock holds, there, wherever - /// the map moves. Filled by the renderer. + /// The labels placed when labels were locked, each with the place it took: only they are + /// drawn while the lock holds, there, wherever the map moves. Filled by the renderer. #[serde(skip)] - pub label_lock: Option>, + pub label_lock: Option>, /// Relationships, snapping, topology rules and edit settings. #[serde(default)] pub editing: design::MapEditing, diff --git a/crates/map/src/symbology.rs b/crates/map/src/symbology.rs index 201824931..d07a56818 100644 --- a/crates/map/src/symbology.rs +++ b/crates/map/src/symbology.rs @@ -1982,7 +1982,8 @@ pub fn callout_reach(v: f64) -> Option { } fn de_callout<'de, D: serde::Deserializer<'de>>(d: D) -> Result, D::Error> { - Ok(lenient::(d)?.and_then(callout_reach)) + // (0 turns callouts off, as the command has it; a negative isn't a length either.) + Ok(lenient::(d)?.filter(|v| *v > 0.0).and_then(callout_reach)) } /// The longest wrap character kept (characters). @@ -3402,7 +3403,7 @@ mod tests { v["moved"] = serde_json::json!([{"id": 9, "at": [9, 9]}, {"id": 4, "at": [4, 4]}]); let read: LabelClass = serde_json::from_value(v.clone()).unwrap(); assert_eq!((read.moved_to(4), read.moved_to(9)), (Some([4.0, 4.0]), Some([9.0, 9.0]))); - // A huge list reads quickly, and keeps the most. + // A huge list reads quickly, and keeps the first the file lists, up to the most kept. let many: Vec<_> = (0..50_000u64).rev().map(|i| serde_json::json!({"id": i, "at": [0, 0]})).collect(); v["moved"] = serde_json::Value::Array(many); let read: LabelClass = serde_json::from_value(v.clone()).unwrap(); diff --git a/crates/render/src/layout.rs b/crates/render/src/layout.rs index 405823398..e12f71531 100644 --- a/crates/render/src/layout.rs +++ b/crates/render/src/layout.rs @@ -1442,7 +1442,7 @@ pub fn frame_coverage(map: &mut Map, center: Coord, scale: f64, w_mm: f64, h_mm: basemap: rustgis_map::Basemap, tiles: Vec, graticule: bool, - lock: Option>, + lock: Option>, map: &'a mut Map, } impl Drop for Aside<'_> { diff --git a/crates/render/src/lib.rs b/crates/render/src/lib.rs index 3c527e857..932157130 100644 --- a/crates/render/src/lib.rs +++ b/crates/render/src/lib.rs @@ -3102,7 +3102,8 @@ fn render_frame(map: &mut Map, view: &View, full_res_rasters: bool, backend: Bac if !display.locked { map.label_lock = None; } - let lock: Option> = map.label_lock.as_ref().map(|v| v.iter().copied().collect()); + let lock: Option> = + map.label_lock.as_ref().map(|v| v.iter().copied().collect()); // The classes of features with a label locked (any of its repeats), to pass the rest over early. let locked_classes: Option> = lock.as_ref().map(|k| k.keys().map(|&(l, r, c, _)| (l, r, c)).collect()); @@ -3645,7 +3646,12 @@ fn render_frame(map: &mut Map, view: &View, full_res_rasters: bool, backend: Bac }; let line_in_view = || globe.is_some() || ctx.cull.intersects(&extents.get(row).copied().unwrap_or_else(|| g.extent())); let fid = layer.dataset.features.get(row).map(|f| f.id); - let moved_here = || any_moved && classes.classes_of(ds_ptr, row).any(|(_, c)| moved_to(c, fid).is_some()); + // (Its class's clause read only for a feature with a label moved.) + let moved_here = || { + any_moved + && fid.is_some_and(|id| labels.in_order().any(|c| c.moved_to(id).is_some())) + && classes.classes_of(ds_ptr, row).any(|(_, c)| moved_to(c, fid).is_some()) + }; if point.is_none() && !(any_along && matches!(g, Geometry::Line(_)) && line_in_view()) && !moved_here() { continue; } @@ -3772,7 +3778,7 @@ fn render_frame(map: &mut Map, view: &View, full_res_rasters: bool, backend: Bac }; let alternatives: Vec<(f32, f32)> = match lock.as_ref().and_then(|k| k.get(&key)) { Some(&won) => { - if let Some(at) = usize::from(won).checked_sub(1).and_then(|i| others().nth(i)) { + if let Some(at) = usize::from(won.place).checked_sub(1).and_then(|i| others().nth(i)) { (lx, ly) = at; } Vec::new() @@ -3810,6 +3816,7 @@ fn render_frame(map: &mut Map, view: &View, full_res_rasters: bool, backend: Bac 0.0 }, callout_anchor: anchor.map(|(ax, ay)| [ax as f32 - lx, ay as f32 - ly]), + callout_offset: lock.as_ref().and_then(|k| k.get(&key)).and_then(|won| won.callout), pinned: moved.is_some(), source: fid.map(|id| (lid, id, ci as u32)), ..Default::default() @@ -3931,7 +3938,10 @@ fn render_frame(map: &mut Map, view: &View, full_res_rasters: bool, backend: Bac }; for l in label_candidates.iter_mut().map(|c| &mut c.label).chain(fixed_labels.iter_mut()) { turn_with(l, &|x, y| Some(at(x, y))); + // A callout's feature to where it is in that view (not turned with the label). + let feature = l.collide.callout_anchor.map(|[ax, ay]| at(l.x + ax, l.y + ay)); (l.x, l.y) = at(l.x, l.y); + l.collide.callout_anchor = feature.map(|(fx, fy)| [fx - l.x, fy - l.y]); if let Some(i) = &mut l.icon { (i.x, i.y) = at(i.x, i.y); } @@ -4502,8 +4512,11 @@ fn tilt_candidate(mut c: Candidate, at: &impl Fn(f32, f32) -> Option<(f32, f32)> let l = &mut c.label; turn_with(l, at); let alternatives = std::mem::take(&mut c.alternatives); + // A callout's feature to where it is in that view (none: out of sight, and no line). + let feature = l.collide.callout_anchor.and_then(|[ax, ay]| at(l.x + ax, l.y + ay)); let mut spots = std::iter::once((l.x, l.y)).chain(alternatives).filter_map(|(x, y)| at(x, y)); (l.x, l.y) = spots.next()?; + l.collide.callout_anchor = feature.map(|(fx, fy)| [fx - l.x, fy - l.y]); if let Some(i) = &mut l.icon { (i.x, i.y) = at(i.x, i.y)?; } @@ -4598,15 +4611,17 @@ fn halo_for(text: Rgba) -> Rgba { struct Placement { labels: Vec, unplaced: Vec, - /// The key of each placed label, and the place it took (0 its own, else its alternative's - /// number from 1). - keys: Vec<(rustgis_map::LabelKey, u8)>, + /// The key of each placed label, and the place it took. + keys: Vec<(rustgis_map::LabelKey, rustgis_map::LockedPlace)>, } /// Labels a placement looks for a callout's spot for, at most (each looks at up to a few thousand /// spots). const MAX_CALLOUT_SEARCHES: usize = 500; +/// Spots a placement tries for its callouts at most, all its searches together. +const MAX_CALLOUT_SPOTS: usize = 200_000; + /// A label's text's extent (points, `[x0, y0, x1, y1]`), as placement keeps it clear of the /// others: what the Move Label tool picks it by. pub fn label_extent(l: &PlacedLabel) -> Option<[f32; 4]> { @@ -4652,6 +4667,7 @@ fn callout_spot( free: impl Fn(&[f32; 4]) -> bool, w: f32, h: f32, + spots: &mut usize, ) -> Option<(f32, f32, [f32; 4])> { let ext = boxes_extent(text)?; if !(anchor[0].is_finite() && anchor[1].is_finite() && reach.is_finite()) { @@ -4678,7 +4694,14 @@ fn callout_spot( // The line ends at the text's nearest edge; text over the feature hides it. let to = [anchor[0].clamp(e[0], e[2]), anchor[1].clamp(e[1], e[3])]; let len = (to[0] - anchor[0]).hypot(to[1] - anchor[1]); - if len > reach || len < 1.0 || best.is_some_and(|b| len >= b.0) || !text.iter().all(|b| free(&moved(b))) { + if len > reach || len < 1.0 || best.is_some_and(|b| len >= b.0) { + continue; + } + if *spots == 0 { + return best.map(|(_, dx, dy, line)| (dx, dy, line)); + } + *spots -= 1; + if !text.iter().all(|b| free(&moved(b))) { continue; } best = Some((len, dx, dy, [anchor[0], anchor[1], to[0], to[1]])); @@ -4715,7 +4738,7 @@ fn place_labels(cands: impl IntoIterator>, fixed: Ve let mut out = Placement { labels: fixed, unplaced: Vec::new(), keys: Vec::new() }; let outside = |b: &[f32; 4]| b[2] < 0.0 || b[3] < 0.0 || b[0] > w || b[1] > h; // Callout searches a placement makes at most: a crowded view stays quick. - let mut searches = MAX_CALLOUT_SEARCHES; + let (mut searches, mut spots) = (MAX_CALLOUT_SEARCHES, MAX_CALLOUT_SPOTS); for Candidate { label: mut l, key, alternatives, .. } in cands.into_iter().take(20_000) { let c = l.collide; let (has_text, has_icon) = (!l.text.is_empty(), l.icon.is_some()); @@ -4744,7 +4767,21 @@ fn place_labels(cands: impl IntoIterator>, fixed: Ve (l.x, l.y) = (x, y); l.collide.callout = None; let relative = |l: &PlacedLabel, line: [f32; 4]| [line[0] - l.x, line[1] - l.y, line[2] - l.x, line[3] - l.y]; - if c.pinned { + let mut moved_by = None; + if let (true, Some([dx, dy])) = (locked && !c.pinned, c.callout_offset) { + // Locked after a callout moved it: there again, with its line. + l.x += dx; + l.y += dy; + for b in &mut text { + *b = [b[0] + dx, b[1] + dy, b[2] + dx, b[3] + dy]; + } + moved_by = Some([dx, dy]); + if let Some(f) = feature + && let Some(line) = callout_line(&text, f) + { + l.collide.callout = Some(relative(&l, line)); + } + } else if c.pinned { // Moved by hand: where it was put, a callout's line back to its feature when its class // has callouts (however long: it was put there). if let Some(f) = feature.filter(|_| c.callout_reach > 0.0) @@ -4764,7 +4801,8 @@ fn place_labels(cands: impl IntoIterator>, fixed: Ve // nearest free spot within its reach, a line drawn back to its feature. With no such // spot it stays, and is left out, or shown over the others when its class shows all. searches -= 1; - if let Some((dx, dy, line)) = callout_spot(&text, f, c.callout_reach, |b| !grid.hits(b), w, h) { + if let Some((dx, dy, line)) = callout_spot(&text, f, c.callout_reach, |b| !grid.hits(b), w, h, &mut spots) { + moved_by = Some([dx, dy]); l.x += dx; l.y += dy; for b in &mut text { @@ -4793,7 +4831,7 @@ fn place_labels(cands: impl IntoIterator>, fixed: Ve l.icon = None; } out.labels.push(l); - out.keys.push((key, u8::try_from(i).unwrap_or(u8::MAX))); + out.keys.push((key, rustgis_map::LockedPlace { place: u8::try_from(i).unwrap_or(u8::MAX), callout: moved_by })); } out } @@ -5332,7 +5370,8 @@ mod tests { // The first free one, past one that is taken too. let p = place_labels(vec![blocker(), taken(vec![(104.0, 20.0), (300.0, 20.0), (300.0, 100.0)])], Vec::new(), 400.0, 200.0, false); assert_eq!(at(&p)[1], ("Taken".to_string(), 300.0, 20.0)); - assert_eq!(p.keys, [((u32::MAX, 0, 0, 0), 0), ((1, 0, 0, 0), 2)], "the place it took, for a lock"); + let place = |place| rustgis_map::LockedPlace { place, callout: None }; + assert_eq!(p.keys, [((u32::MAX, 0, 0, 0), place(0)), ((1, 0, 0, 0), place(2))], "the place it took, for a lock"); assert!(p.unplaced.is_empty()); // All taken: unplaced once, where it would have gone. let p = place_labels(vec![blocker(), taken(vec![(104.0, 20.0), (96.0, 22.0)])], Vec::new(), 400.0, 200.0, false); @@ -5717,12 +5756,84 @@ mod tests { // A small text by its feature at (100, 100); only spots 200 points right of it are free. let text = [[95.0, 98.0, 105.0, 102.0]]; let free = |b: &[f32; 4]| b[0] > 300.0; - let (dx, _, [x0, y0, x1, y1]) = callout_spot(&text, [100.0, 100.0], 300.0, free, 1000.0, 200.0).expect("a spot"); + let (dx, _, [x0, y0, x1, y1]) = callout_spot(&text, [100.0, 100.0], 300.0, free, 1000.0, 200.0, &mut 100_000).expect("a spot"); assert!(dx > 195.0); let len = (x1 - x0).hypot(y1 - y0); assert!((150.0..=300.0).contains(&len), "{len} points"); // Beyond its reach, none. - assert!(callout_spot(&text, [100.0, 100.0], 100.0, free, 1000.0, 200.0).is_none()); + assert!(callout_spot(&text, [100.0, 100.0], 100.0, free, 1000.0, 200.0, &mut 100_000).is_none()); + // The placement's spots used up: it stops looking, and says how many it tried. + let mut spots = 5; + assert!(callout_spot(&text, [100.0, 100.0], 300.0, free, 1000.0, 200.0, &mut spots).is_none()); + assert_eq!(spots, 0); + } + + /// Two points close together: the second label moves off by a callout. Locked, it keeps that + /// place and its line, as the map moves too (it went back onto the first, without its line). + #[test] + fn a_lock_keeps_a_label_where_its_callout_moved_it() { + let mut m = Map::new("Map"); + let mut d = Dataset::new("pts", GeometryType::Point, vec![Field::new("name", FieldType::Text)]); + d.push(Some(Geometry::Point(Coord::new(0.0, 0.0))), vec![Value::Text("First place".into())]); + d.push(Some(Geometry::Point(Coord::new(0.01, 0.0))), vec![Value::Text("Second place".into())]); + let id = m.add_dataset(d); + let l = m.layer_mut(id).unwrap(); + l.labels.enabled = true; + l.labels.default.callout = Some(60.0); + let at = |x: f64| { + let camera = Camera { center: lonlat_to_merc(Coord::new(x, 0.0)), res: 500.0, bearing: 0.0, pitch: 0.0 }; + View { width: 800.0, height: 600.0, pixels_per_point: 1.0, camera, sky: false } + }; + let placed = |m: &mut Map, view: &View| { + let mut labels = render(m, view).unwrap().labels; + labels.sort_by(|a, b| a.text.cmp(&b.text)); + labels + }; + let free = placed(&mut m, &at(0.0)); + assert_eq!(free.len(), 2); + let line = free.iter().filter_map(|l| l.collide.callout).next().expect("one moved off by a callout"); + // Locked: the same places and line, frame after frame. + m.label_display.locked = true; + for _ in 0..2 { + let locked = placed(&mut m, &at(0.0)); + assert_eq!(locked.len(), 2); + for (a, b) in free.iter().zip(&locked) { + assert!((a.x - b.x).abs() < 0.01 && (a.y - b.y).abs() < 0.01, "{}: ({}, {}) to ({}, {})", a.text, a.x, a.y, b.x, b.y); + } + assert_eq!(locked.iter().filter_map(|l| l.collide.callout).next(), Some(line)); + } + // The map panned: the two labels stay as far apart, the line the same. + let panned = placed(&mut m, &at(0.02)); + assert_eq!(panned.len(), 2); + let apart = |l: &[PlacedLabel]| (l[1].x - l[0].x, l[1].y - l[0].y); + let (a, b) = (apart(&free), apart(&panned)); + assert!((a.0 - b.0).abs() < 0.5 && (a.1 - b.1).abs() < 0.5, "{a:?} {b:?}"); + assert_eq!(panned.iter().filter_map(|l| l.collide.callout).next(), Some(line)); + } + + /// On a turned or tilted map a callout's line starts at its feature there too, not where the + /// north-up frame had it. + #[test] + fn a_callout_starts_at_its_feature_on_a_turned_map() { + let mut m = Map::new("Map"); + let mut d = Dataset::new("pts", GeometryType::Point, vec![Field::new("name", FieldType::Text)]); + d.push(Some(Geometry::Point(Coord::new(2.35, 48.85))), vec![Value::Text("Paris".into())]); + let id = m.add_dataset(d); + let fid = m.layer(id).unwrap().dataset.features.first().unwrap().id; + let l = m.layer_mut(id).unwrap(); + l.labels.enabled = true; + l.labels.default.callout = Some(40.0); + assert!(l.labels.default.move_label(fid, Some([3.5, 49.5]))); + for (bearing, pitch) in [(40.0, 0.0), (0.0, 40.0), (40.0, 30.0)] { + let camera = Camera { center: lonlat_to_merc(Coord::new(2.9, 49.1)), res: 1000.0, bearing, pitch }; + let view = View { width: 800.0, height: 600.0, pixels_per_point: 1.0, camera, sky: false }; + let labels = render(&mut m, &view).unwrap().labels; + let l = labels.iter().find(|l| l.text == "Paris").expect("drawn"); + let [x0, y0, ..] = l.collide.callout.expect("a line back to Paris"); + let (px, py) = view.to_screen(m.to_map(Coord::new(2.35, 48.85)).unwrap()); + let off = (f64::from(l.x + x0) - px).hypot(f64::from(l.y + y0) - py); + assert!(off < 1.0, "bearing {bearing}, pitch {pitch}: {off} points off"); + } } /// Each label class labels every feature its clause takes, as in ArcGIS Pro (#732): a feature @@ -5821,7 +5932,7 @@ mod tests { // Locked, it stays there as the map moves. m.label_display.locked = true; assert_eq!(labels(&mut m, &flat), placed); - assert!(m.label_lock.as_ref().is_some_and(|k| k.iter().any(|(_, i)| *i > 0)), "{:?}", m.label_lock); + assert!(m.label_lock.as_ref().is_some_and(|k| k.iter().any(|(_, i)| i.place > 0)), "{:?}", m.label_lock); let moved = labels(&mut m, &view(0.0, 0.0, Coord::new(2.6, 48.85))); let apart = |l: &[(f32, f32, f32)]| (l[1].1 - l[0].1, l[1].2 - l[0].2); assert_eq!(moved.len(), 2); diff --git a/crates/render/src/stream_tests.rs b/crates/render/src/stream_tests.rs index af965ffc1..d71a805d1 100644 --- a/crates/render/src/stream_tests.rs +++ b/crates/render/src/stream_tests.rs @@ -998,7 +998,7 @@ fn pitched_plain_chunks_bypass_depth_budget_alone_and_with_upright_markers() { for layer in &mut failed.layers { *layer = streamed(layer, if layer.id == ground { 0 } else { 1 }); } - let lock = Some(vec![((ground, 0, 0, 0), 0)]); + let lock = Some(vec![((ground, 0, 0, 0), rustgis_map::LockedPlace::default())]); failed.label_lock = lock.clone(); assert!(render_with_backend(&mut failed, &view, true, backend).is_none()); assert_eq!(failed.label_lock, lock); diff --git a/crates/tiles/src/symbols.rs b/crates/tiles/src/symbols.rs index 1de6bd397..00ca845b3 100644 --- a/crates/tiles/src/symbols.rs +++ b/crates/tiles/src/symbols.rs @@ -97,6 +97,9 @@ pub struct Collide { /// Once moved off its spot, its callout's line: from its feature to the edge of its text, as /// offsets (points) from where the label is drawn (`x`, `y`), so it moves with the label. pub callout: Option<[f32; 4]>, + /// Locked after a callout moved it: how far (points) it stays moved from its place, its line + /// drawn back to its feature. + pub callout_offset: Option<[f32; 2]>, /// Moved by hand (the Move Label tool): placed where it was put, whatever is under it, and /// placed first, so the others keep off it. pub pinned: bool, @@ -119,6 +122,7 @@ impl Default for Collide { callout_reach: 0.0, callout_anchor: None, callout: None, + callout_offset: None, pinned: false, source: None, } diff --git a/crates/ui-egui/src/mapview.rs b/crates/ui-egui/src/mapview.rs index b8bc294dd..4c7678d0b 100644 --- a/crates/ui-egui/src/mapview.rs +++ b/crates/ui-egui/src/mapview.rs @@ -1243,7 +1243,10 @@ fn main_view(app: &mut RustGisApp, ui: &mut Ui, t: &Tokens) { } else { let s = if globe_view { 1.0 } else { (old.res / cam.res) as f32 }; for l in &app.map.labels { - draw_label(&painter, l, tex_rect.min + vec2(l.x, l.y) * s); + let at = tex_rect.min + vec2(l.x, l.y) * s; + // A callout's feature moves with the map, its text's end with the text. + let feature = l.collide.callout.filter(|_| s != 1.0).map(|c| tex_rect.min + vec2(l.x + c[0], l.y + c[1]) * s); + draw_label(&painter, &callout_from(l, at, feature), at); } } if app.map.hide_overlays { @@ -2440,8 +2443,10 @@ fn interact(app: &mut RustGisApp, ui: &mut Ui, resp: &egui::Response, rect: Rect if resp.drag_stopped() { app.map.drag = None; let to = moved.and_then(|l| screen_to_lonlat(app, rect, rect.min + vec2(l.x, l.y))); - if let (Some(c), Some(m)) = (to, app.map.moving_label.take()) { - let _ = app.run("labels.move", json!({"layer": m.layer, "class": m.class, "feature": m.feature, "to": [c.x, c.y]})); + if let (Some(c), Some(m)) = (to, app.map.moving_label.take()) + && let Err(e) = app.run("labels.move", json!({"layer": m.layer, "class": m.class, "feature": m.feature, "to": [c.x, c.y]})) + { + app.session.notify(e, true); } app.map.moving_label = None; return; @@ -4447,10 +4452,25 @@ mod tests { h.step(); drag(&mut h, false); assert_eq!(moved(&h).len(), 1, "the move is kept"); + // Shift-click on the moved one: put back, and moved again by undo. + let pinned = h.state().map.labels.iter().find(|l| l.collide.pinned).map(|l| rect.min + vec2(l.x, l.y)); + let shift = egui::Modifiers { shift: true, ..none }; + let at = pinned.expect("the moved label drawn"); + selection_pointer(&mut h, at, None, shift); + selection_pointer(&mut h, at, Some(true), shift); + selection_pointer(&mut h, at, Some(false), shift); + assert!(moved(&h).is_empty(), "put back"); + h.state_mut().run("edit.undo", json!({})).unwrap(); + assert_eq!(moved(&h).len(), 1, "undone"); + let end = std::time::Instant::now() + std::time::Duration::from_secs(10); + while !h.state().map.labels.iter().any(|l| l.collide.pinned) || h.state().map.drawing() { + assert!(std::time::Instant::now() < end, "labels never drawn again"); + std::thread::sleep(std::time::Duration::from_millis(2)); + h.step(); + } // Shift-click on a label never moved: no undo step, so undo takes back the move. let other = h.state().map.labels.iter().find(|l| l.text == "Place 1" && !l.collide.pinned).map(|l| rect.min + vec2(l.x, l.y)); let at = other.expect("another label drawn"); - let shift = egui::Modifiers { shift: true, ..none }; selection_pointer(&mut h, at, Some(true), shift); selection_pointer(&mut h, at, Some(false), shift); h.state_mut().run("edit.undo", json!({})).unwrap(); From 8a546ca75d2946882dc56112cb00479f7ab65292 Mon Sep 17 00:00:00 2001 From: mavrick Date: Tue, 3 Nov 2026 19:02:24 -0600 Subject: [PATCH 04/15] fix(labels): keep moves tied to current features and placement --- crates/engine/src/cmd/labels.rs | 9 +- crates/engine/src/cmd/project.rs | 3 + crates/engine/src/cmd/service_group.rs | 7 ++ crates/engine/src/lib.rs | 34 +++-- crates/engine/src/project.rs | 8 ++ crates/engine/src/project_label_losses.rs | 146 ++++++++++++++++++++++ crates/engine/src/tests.rs | 131 ++++++++++++++++++- crates/ui-egui/src/labelpane.rs | 10 +- crates/ui-egui/src/lib.rs | 4 +- crates/ui-egui/src/mapview.rs | 15 ++- docs/user-guide/cartography.md | 7 +- 11 files changed, 352 insertions(+), 22 deletions(-) create mode 100644 crates/engine/src/project_label_losses.rs diff --git a/crates/engine/src/cmd/labels.rs b/crates/engine/src/cmd/labels.rs index 05c77089b..e0ac6a819 100644 --- a/crates/engine/src/cmd/labels.rs +++ b/crates/engine/src/cmd/labels.rs @@ -446,9 +446,11 @@ pub(crate) fn same_class_list(a: &Labeling, b: &Labeling) -> bool { } /// Whether a lock taken with the labels `a` still names those of `b`: the same classes, listed -/// alike and putting their point labels in the same places. +/// alike, putting their point labels in the same places, with unchanged overlap and callouts. pub(crate) fn lock_holds(a: &Labeling, b: &Labeling) -> bool { - same_class_list(a, b) && same_places(a, b) + same_class_list(a, b) + && same_places(a, b) + && a.in_order().zip(b.in_order()).all(|(x, y)| x.show_all == y.show_all && x.callout == y.callout) } /// Whether the classes of `a` and `b` (listed alike) put their point labels in the same places. @@ -501,6 +503,7 @@ fn clause(p: &Value, ds: &Dataset) -> Result>, String> { } } +/// Adds a class from existing styling and a checked clause, without inheriting feature-specific moves. fn add_class(s: &mut Session, p: &Value) -> CmdResult { let name = s_req(p, "name")?.to_string(); let from = class_param(p, "from")?.map(String::from); @@ -516,7 +519,7 @@ fn add_class(s: &mut Session, p: &Value) -> CmdResult { None => &labels.default, Some(n) => labels.class(n).ok_or_else(|| format!("no label class named '{n}'"))?, }; - let class = LabelClass { name: name.clone(), visible: true, where_: clause, ..base.clone() }; + let class = LabelClass { name: name.clone(), visible: true, where_: clause, moved: Vec::new(), ..base.clone() }; labels.classes.push(class); added = name; Ok(()) diff --git a/crates/engine/src/cmd/project.rs b/crates/engine/src/cmd/project.rs index 97c9746cf..fd918d8da 100644 --- a/crates/engine/src/cmd/project.rs +++ b/crates/engine/src/cmd/project.rs @@ -155,6 +155,9 @@ fn reset(s: &mut Session, p: Project) { // Kept until the command ends: a failure gives the project back with its history. s.put_aside(); s.project = p; + for text in std::mem::take(&mut s.project.label_read_notices) { + s.notify(text, false); + } s.opened_package = None; s.opened = s.opened.wrapping_add(1); s.active_layer = s.map().layers.first().map(|l| l.id); diff --git a/crates/engine/src/cmd/service_group.rs b/crates/engine/src/cmd/service_group.rs index bfb6edb47..df18bad82 100644 --- a/crates/engine/src/cmd/service_group.rs +++ b/crates/engine/src/cmd/service_group.rs @@ -268,6 +268,7 @@ fn refresh(s: &mut Session, p: &Value) -> CmdResult { // whose classes no longer read, drawn with a single symbol. let mut notes = Vec::new(); let mut singled = Vec::new(); + let mut moved = 0; for ((mut child, mut dataset), existing) in reads.into_iter().zip(matches) { let existing = existing.and_then(|id| s.map_mut().layer_mut(id)); if let Some(layer) = existing { @@ -278,6 +279,9 @@ fn refresh(s: &mut Session, p: &Value) -> CmdResult { dataset.name = layer.name.clone(); layer.dataset = dataset; layer.selection.clear(); + moved += layer.labels.in_order().map(|class| class.moved.len()).sum::(); + layer.labels.default.moved.clear(); + layer.labels.classes.iter_mut().for_each(|class| class.moved.clear()); let invalid_renderer = match &layer.renderer { rustgis_map::Renderer::Single(_) => false, rustgis_map::Renderer::Unique { expression: Some(e), .. } => rustgis_data::query::parse(e, &layer.dataset).is_err(), @@ -343,6 +347,9 @@ fn refresh(s: &mut Session, p: &Value) -> CmdResult { } children.push(child); } + if moved > 0 { + s.notify(format!("{moved} saved label move(s) reset in refreshed service layers"), false); + } if !notes.is_empty() { let lost: usize = notes.iter().map(|(_, n)| n).sum(); let names: Vec<&str> = notes.iter().map(|(name, _)| name.as_str()).collect(); diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index a8f8f8412..813b5cd0a 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -796,21 +796,23 @@ fn fresh_source_ids(before: &Project, after: &mut Project) { /// A lock names each label by its feature's id, and [`rustgis_data::Dataset::push`] hands the /// largest id out again once its feature is deleted: the new feature would take the lock's place -/// without having been placed (#1135). So a command that removes features removes their entries, +/// without having been placed (#1135). Saved moved-label positions have the same identity rule. +/// So a command that removes features removes their locks and saved moves, /// whichever way it removes them. Undo brings them back with the rest of the project. A streamed /// layer holds only the window in view, so its other features aren't gone. It compares ids, so an /// id a single command both removes and hands out stays locked: only restoring from an archive can. -fn forget_locks_of_removed_features(before: &Project, after: &mut Project) { +fn forget_label_state_of_removed_features(before: &Project, after: &mut Project) { for (i, m) in after.maps.iter_mut().enumerate() { let was = before.maps.get(i).filter(|o| o.name == m.name); - let Some(lock) = m.label_lock.as_mut() else { continue }; - for l in m.layers.iter().filter(|l| !l.is_streamed()) { + for l in m.layers.iter_mut().filter(|l| !l.is_streamed()) { if was.and_then(|o| o.layers.iter().find(|o| o.id == l.id)).is_some_and(|o| o.dataset.features.shares(&l.dataset.features)) { continue; } - // The lock's ids for this layer, less those it still has: sized by the lock, not the layer. - let mut gone: std::collections::HashSet = - lock.iter().filter(|((layer, ..), _)| *layer == l.id).map(|((_, id, ..), _)| *id).collect(); + // Bound the lookup by saved label state, not the feature count. Off-window ids stay put. + let mut gone: std::collections::HashSet = l.labels.in_order().flat_map(|c| c.moved.iter().map(|m| m.id)).collect(); + if let Some(lock) = &m.label_lock { + gone.extend(lock.iter().filter(|((layer, ..), _)| *layer == l.id).map(|((_, id, ..), _)| *id)); + } if gone.is_empty() { continue; } @@ -820,7 +822,12 @@ fn forget_locks_of_removed_features(before: &Project, after: &mut Project) { break; } } - lock.retain(|((layer, id, ..), _)| *layer != l.id || !gone.contains(id)); + for c in std::iter::once(&mut l.labels.default).chain(l.labels.classes.iter_mut()) { + c.moved.retain(|entry| !gone.contains(&entry.id)); + } + if let Some(lock) = &mut m.label_lock { + lock.retain(|((layer, id, ..), _)| *layer != l.id || !gone.contains(id)); + } } } } @@ -839,7 +846,7 @@ fn edits_features(id: &str) -> bool { } impl Session { - pub fn new(project: Project) -> Self { + pub fn new(mut project: Project) -> Self { // The engine's tests never add Shapefile indexes to the user's cache (#1230). #[cfg(all(test, not(target_arch = "wasm32")))] rustgis_data::source::shp::use_temporary_index_dir(); @@ -852,7 +859,8 @@ impl Session { rustgis_data::diagnostics::set_clock(|| { web_time::SystemTime::now().duration_since(web_time::UNIX_EPOCH).map(|d| d.as_millis() as u64).unwrap_or(0) }); - Session { + let label_notices = std::mem::take(&mut project.label_read_notices); + let mut session = Session { project, path: None, log: project_log::SessionLog::default(), @@ -936,7 +944,11 @@ impl Session { sql_results: Vec::new(), trend_results: Vec::new(), long_runs: None, + }; + for text in label_notices { + session.notify(text, false); } + session } /// Keeps a query's result the app ran off the frame, for `layer.addSql {"ran": id}`, with the @@ -1261,7 +1273,7 @@ impl Session { (Ok(_), Some(_)) if std::mem::take(&mut self.request_only) => {} (Ok(_), Some(mut snap)) => { fresh_source_ids(&snap, &mut self.project); - forget_locks_of_removed_features(&snap, &mut self.project); + forget_label_state_of_removed_features(&snap, &mut self.project); // An edit may change the dates the time slider's window filters on: bound layers // whose features or fields the command changed (features are shared with the // snapshot until written) take the window again. diff --git a/crates/engine/src/project.rs b/crates/engine/src/project.rs index 04fc60e6e..fcaa225c2 100644 --- a/crates/engine/src/project.rs +++ b/crates/engine/src/project.rs @@ -11,6 +11,9 @@ use serde::{Deserialize, Serialize}; use crate::GpRecord; +#[path = "project_label_losses.rs"] +mod label_losses; + /// A map export cannot keep runtime-only download reservations, even in a larger project. pub(crate) fn ensure_map_imports_complete(map: &Map) -> Result<(), String> { if map.layers.iter().any(|layer| layer.dataset.source.as_deref().is_some_and(|source| source.starts_with("geolibre-pending:"))) @@ -123,6 +126,9 @@ fn identity(text: &str) -> (usize, u64) { #[derive(Clone, Debug, Serialize, Deserialize)] pub struct Project { + /// Read-time label adjustments, consumed by the session and never saved. + #[serde(skip)] + pub(crate) label_read_notices: Vec, #[serde(default = "format")] pub format: u32, pub name: String, @@ -470,6 +476,7 @@ impl Project { pub fn new(name: impl Into) -> Self { Project { + label_read_notices: Vec::new(), format: FORMAT, name: name.into(), maps: vec![Map::new("Map")], @@ -910,6 +917,7 @@ impl Project { p.resolve_paths(d); } p.normalize(); + p.label_read_notices = label_losses::notices(text, &p)?; Ok(p) } diff --git a/crates/engine/src/project_label_losses.rs b/crates/engine/src/project_label_losses.rs new file mode 100644 index 000000000..33014034e --- /dev/null +++ b/crates/engine/src/project_label_losses.rs @@ -0,0 +1,146 @@ +//! Count label-setting losses while skipping feature data and retaining no moved-label arrays. + +use serde::Deserialize; + +use crate::Project; + +#[derive(Default)] +struct MoveCount(usize); + +impl<'de> Deserialize<'de> for MoveCount { + /// Counts raw records without retaining their values, including damaged non-array settings. + fn deserialize>(d: D) -> Result { + struct Count; + impl<'de> serde::de::Visitor<'de> for Count { + type Value = MoveCount; + /// Describes the optional project setting consumed by this visitor. + fn expecting(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str("moved-label settings") + } + /// Counts entries using IgnoredAny so even an oversized list has bounded memory. + fn visit_seq>(self, mut seq: A) -> Result { + let mut count = 0usize; + while seq.next_element::()?.is_some() { + count = count.saturating_add(1); + } + Ok(MoveCount(count)) + } + /// Skips a damaged object and counts its discarded setting once. + fn visit_map>(self, mut map: A) -> Result { + while map.next_entry::()?.is_some() {} + Ok(MoveCount(1)) + } + /// A boolean is one discarded move setting. + fn visit_bool(self, _: bool) -> Result { + Ok(MoveCount(1)) + } + /// A signed number is one discarded move setting. + fn visit_i64(self, _: i64) -> Result { + Ok(MoveCount(1)) + } + /// An unsigned number is one discarded move setting. + fn visit_u64(self, _: u64) -> Result { + Ok(MoveCount(1)) + } + /// A fractional number is one discarded move setting. + fn visit_f64(self, _: f64) -> Result { + Ok(MoveCount(1)) + } + /// Text is one discarded move setting. + fn visit_str(self, _: &str) -> Result { + Ok(MoveCount(1)) + } + /// Null expresses no moved-label records and causes no loss notice. + fn visit_unit(self) -> Result { + Ok(MoveCount(0)) + } + } + d.deserialize_any(Count) + } +} + +#[derive(Default, Deserialize)] +struct Class { + #[serde(default)] + moved: MoveCount, + #[serde(default)] + callout: Option, +} + +#[derive(Default, Deserialize)] +struct Labels { + #[serde(default)] + moved: MoveCount, + #[serde(default)] + callout: Option, + #[serde(default)] + classes: Vec, +} + +#[derive(Deserialize)] +struct Layer { + #[serde(default)] + labels: Labels, +} + +#[derive(Default, Deserialize)] +struct Map { + #[serde(default)] + layers: Vec, +} + +#[derive(Deserialize)] +struct Offline { + data: Map, +} + +#[derive(Deserialize)] +struct File { + maps: Vec, + #[serde(default)] + offline_areas: Vec, + #[serde(default)] + styles: Vec, +} + +/// Zero and null explicitly disable callouts; other lengths must fit the supported range. +fn adjusted_callout(value: Option<&serde_json::Value>) -> bool { + value.is_some_and(|v| v.as_f64().is_none_or(|n| n != 0.0 && !rustgis_map::symbology::CALLOUT_REACH.contains(&n))) +} + +/// Compare raw record counts with retained state, including offline snapshots; one notice per kind. +pub(super) fn notices(text: &str, project: &Project) -> Result, String> { + let file: File = serde_json::from_str(text).map_err(|e| format!("could not read project label settings: {e}"))?; + let (mut moves, mut callouts) = (0usize, 0usize); + for layer in file.maps.iter().chain(file.offline_areas.iter().map(|a| &a.data)).flat_map(|m| &m.layers).chain(file.styles.iter()) { + let labels = &layer.labels; + moves = moves.saturating_add(labels.moved.0); + callouts = callouts.saturating_add(usize::from(adjusted_callout(labels.callout.as_ref()))); + for class in &labels.classes { + moves = moves.saturating_add(class.moved.0); + callouts = callouts.saturating_add(usize::from(adjusted_callout(class.callout.as_ref()))); + } + } + let retained = project + .maps + .iter() + .chain(project.offline_areas.iter().map(|a| &a.data)) + .flat_map(|m| &m.layers) + .map(|l| &l.labels) + .chain(project.styles.iter().map(|s| &s.labels)) + .flat_map(|labels| labels.in_order()) + .map(|c| c.moved.len()) + .fold(0usize, usize::saturating_add); + let dropped = moves.saturating_sub(retained); + let mut messages = Vec::new(); + if dropped > 0 { + messages.push(format!( + "{}: {dropped} invalid, duplicate or excess moved-label entry/setting(s) ignored while opening the project", + project.name + )); + } + if callouts > 0 { + messages.push(format!("{}: {callouts} callout length setting(s) adjusted or disabled while opening the project", project.name)); + } + Ok(messages) +} diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 52c52d3ad..d4fe037f1 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -3597,6 +3597,85 @@ fn a_new_feature_doesnt_take_the_locked_label_of_a_deleted_one() { assert_eq!(draw(&mut s), ["Alpha 2"]); } +/// Deleting a feature removes its saved label move before its id can be reused; undo restores it. +#[test] +fn a_new_feature_doesnt_take_the_moved_label_of_a_deleted_one() { + use rustgis_data::{Dataset, Field, FieldType, Geometry, GeometryType, Value as V}; + let mut s = Session::new(Project::default()); + let mut data = Dataset::new("Sites", GeometryType::Point, vec![Field::new("name", FieldType::Text)]); + data.push(Some(Geometry::Point(rustgis_geom::Coord::new(0.0, 0.0))), vec![V::Text("Old".into())]); + let layer = s.map_mut().add_dataset(data); + s.run("labels.enable", &json!({"layer":layer,"on":true})).unwrap(); + s.run("labels.move", &json!({"layer":layer,"feature":1,"to":[1.0,1.0]})).unwrap(); + s.map_mut().layer_mut(layer).unwrap().selection = vec![0]; + s.run("edit.delete", &Value::Null).unwrap(); + assert!(s.map().layer(layer).unwrap().labels.default.moved.is_empty()); + s.run("edit.addFeature", &json!({"layer":layer,"geometry":{"type":"Point","coordinates":[0.0,0.0]},"attributes":{"name":"New"}})) + .unwrap(); + assert_eq!(s.map().layer(layer).unwrap().dataset.features[0].id, 1); + let view = crate::render::View { width: 800.0, height: 600.0, pixels_per_point: 1.0, camera: s.map().camera, sky: false }; + let labels = crate::render::render(s.map_mut(), &view).unwrap().labels; + assert!(labels.iter().any(|l| l.text == "New" && !l.collide.pinned)); + s.run("edit.undo", &Value::Null).unwrap(); + s.run("edit.undo", &Value::Null).unwrap(); + assert_eq!(s.map().layer(layer).unwrap().labels.default.moved_to(1), Some([1.0, 1.0])); + s.run("edit.redo", &Value::Null).unwrap(); + assert!(s.map().layer(layer).unwrap().labels.default.moved.is_empty()); +} + +/// Project opening reports discarded label settings once per kind; clean settings stay quiet. +#[test] +fn opening_projects_reports_moved_label_and_callout_adjustments() { + use rustgis_data::{Dataset, Field, FieldType, GeometryType}; + let mut project = Project::default(); + project.maps[0].add_dataset(Dataset::new("Sites", GeometryType::Point, vec![Field::new("name", FieldType::Text)])); + project.maps[0].layers[0].labels.default.move_label(1, Some([2.0, 3.0])); + project.styles.push(crate::project::StyleItem { + name: "Saved labels".into(), + geometry: GeometryType::Point, + renderer: project.maps[0].layers[0].renderer.clone(), + labels: project.maps[0].layers[0].labels.clone(), + transparency: 0.0, + }); + project.offline_areas.push(crate::project::OfflineArea { + name: "Field copy".into(), + map: "Map".into(), + extent: rustgis_geom::Extent::new(-1.0, -1.0, 1.0, 1.0), + created: 0.0, + synced: 0.0, + data: project.maps[0].clone(), + }); + let clean = serde_json::to_string(&project).unwrap(); + let clean_session = Session::new(Project::from_json(&clean).unwrap()); + assert!(clean_session.notices.is_empty()); + let mut damaged: Value = serde_json::from_str(&clean).unwrap(); + let labels = &mut damaged["maps"][0]["layers"][0]["labels"]; + labels["moved"] = json!([ + {"id":1,"at":[2.0,3.0]}, {"id":1,"at":[4.0,5.0]}, + {"id":-1,"at":[2.0,3.0]}, {"id":1.5,"at":[2.0,3.0]}, {"id":2,"at":[2.0,100.0]} + ]); + labels["callout"] = json!(900); + damaged["styles"][0]["labels"]["moved"] = json!("damaged"); + damaged["styles"][0]["labels"]["callout"] = json!(-1); + let text = damaged.to_string(); + let loaded = Project::from_json(&text).unwrap(); + assert_eq!(loaded.maps[0].layers[0].labels.default.moved_to(1), Some([4.0, 5.0])); + let startup = Session::new(loaded); + assert_eq!(startup.notices.len(), 2); + assert!(startup.notices.iter().any(|n| n.text.contains("5 invalid, duplicate or excess moved-label"))); + assert!(startup.notices.iter().any(|n| n.text.contains("2 callout length setting(s) adjusted"))); + let mut opened = Session::new(Project::default()); + opened.run("project.openText", &json!({"text":text})).unwrap(); + assert_eq!(opened.notices.iter().filter(|n| n.text.contains("while opening the project")).count(), 2); + assert!(!serde_json::to_string(&opened.project).unwrap().contains("label_read_notices")); + let mut excess: Value = serde_json::from_str(&clean).unwrap(); + excess["styles"][0]["labels"]["moved"] = + Value::Array((0..=rustgis_map::symbology::MAX_MOVED_LABELS).map(|id| json!({"id":id,"at":[0.0,0.0]})).collect()); + let capped = Session::new(Project::from_json(&excess.to_string()).unwrap()); + assert_eq!(capped.notices.len(), 1); + assert!(capped.notices[0].text.contains("1 invalid, duplicate or excess moved-label")); +} + /// A streamed layer holds only the window in view: a lock's entry for a feature outside it stays /// when the window changes (#1135). #[test] @@ -3609,7 +3688,7 @@ fn a_lock_holds_for_a_streamed_layers_features_outside_its_window() { let before = s.project.clone(); // The window moves off the feature: no longer shared with the copy, and without its id. s.map_mut().layer_mut(id).unwrap().dataset.features = Vec::new().into(); - crate::forget_locks_of_removed_features(&before, &mut s.project); + crate::forget_label_state_of_removed_features(&before, &mut s.project); assert_eq!(s.map().label_lock.as_ref().map(Vec::len), Some(1)); drop(s); remove_shp(&p); @@ -3647,6 +3726,56 @@ fn a_change_to_the_label_classes_locks_the_labels_again() { } } +/// Changes that affect collision placement discard the lock; a new class inherits styling, +/// but not feature-specific manual positions. +#[test] +fn label_collision_changes_refresh_locks_without_copying_moves() { + let mut s = world(); + let id = s.map().layer_by_name("Cities").unwrap().id; + let feature = s.map().layer(id).unwrap().dataset.features[0].id; + for params in [json!({"showAll":true}), json!({"callout":true}), json!({"callout":false})] { + s.map_mut().label_lock = Some(vec![((id, feature, 0, 0), Default::default())]); + let mut p = params; + p["layer"] = json!(id); + s.run("labels.placement", &p).unwrap(); + assert!(s.map().label_lock.is_none(), "{p} changes collision placement"); + } + s.run("labels.move", &json!({"layer":id,"feature":feature,"to":[1,1]})).unwrap(); + s.run("labels.class.add", &json!({"layer":id,"name":"New"})).unwrap(); + let labels = &s.map().layer(id).unwrap().labels; + assert_eq!(labels.default.moved.len(), 1); + assert!(labels.class("New").unwrap().moved.is_empty()); +} + +/// A service refresh assigns new ids: default and named-class moves must not attach to them, +/// and the discarded moves are counted. Undo restores the old features and positions. +#[test] +fn service_refresh_resets_saved_label_moves_with_a_notice() { + let server = ogc_server::Server::group_fixture(); + let mut s = Session::new(Project::default()); + s.run("service.import", &json!({"url":server.root()})).unwrap(); + let id = s.map().layer_by_name("Places").unwrap().id; + let feature = s.map().layer(id).unwrap().dataset.features[0].id; + s.run("labels.class.add", &json!({"layer":id,"name":"Named"})).unwrap(); + for class in [None, Some("Named")] { + let mut p = json!({"layer":id,"feature":feature,"to":[1,1]}); + if let Some(class) = class { + p["class"] = json!(class); + } + s.run("labels.move", &p).unwrap(); + } + let mark = s.notice_mark(); + s.run("service.refresh", &json!({"layer":id})).unwrap(); + assert!(s.map().layer(id).unwrap().labels.in_order().all(|c| c.moved.is_empty())); + assert!(s.notices_since(mark).iter().any(|n| n.text.contains("2 saved label move(s) reset"))); + s.run("edit.undo", &Value::Null).unwrap(); + assert_eq!(s.map().layer(id).unwrap().labels.in_order().map(|c| c.moved.len()).sum::(), 2); + s.run("edit.redo", &Value::Null).unwrap(); + let mark = s.notice_mark(); + s.run("service.refresh", &json!({"layer":id})).unwrap(); + assert!(!s.notices_since(mark).iter().any(|n| n.text.contains("saved label move(s) reset"))); +} + /// Label classes by command (#618): added from Default and checked before it, renamed, given /// a clause, moved, styled and removed; refusals change nothing, and Default stays last. #[test] diff --git a/crates/ui-egui/src/labelpane.rs b/crates/ui-egui/src/labelpane.rs index 8f4e16baa..5f84ceff5 100644 --- a/crates/ui-egui/src/labelpane.rs +++ b/crates/ui-egui/src/labelpane.rs @@ -982,7 +982,15 @@ fn crowding(app: &mut RustGisApp, ui: &mut Ui, l: &Layer, c: &Class) { let Some(reach) = c.it.callout else { return }; let mut v = f64::from(reach); let r = ui - .add(egui::DragValue::new(&mut v).range(CALLOUT_REACH).speed(1.0).max_decimals(0).prefix(tl!("at most ")).suffix(" pt")) + .add( + egui::DragValue::new(&mut v) + .update_while_editing(false) + .range(CALLOUT_REACH) + .speed(1.0) + .max_decimals(0) + .prefix(tl!("at most ")) + .suffix(" pt"), + ) .labelled_by(input_label.id) .on_hover_text(tl!("The longest a callout's line may be.")); crate::ribbon::live_drag( diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 55e7f9ae9..f5db8c7ea 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -3789,7 +3789,9 @@ impl RustGisApp { } // A snapshot can capture the screen while the map is on it. relay.set_screen(relayshot::map_shown(self, ctx)); - relay.poll(&mut self.session); + if relay.poll(&mut self.session) > 0 { + self.map.cancel_label_drag(); + } } relayshot::drive(self, ctx); // A script an agent wrote to the file bridge. diff --git a/crates/ui-egui/src/mapview.rs b/crates/ui-egui/src/mapview.rs index 00aaed669..93492f2f3 100644 --- a/crates/ui-egui/src/mapview.rs +++ b/crates/ui-egui/src/mapview.rs @@ -1506,7 +1506,7 @@ struct MovingLabel { /// what the Move Label tool grabs, as it is drawn now. Labels not of a layer's features aren't; /// nor any while a moving globe or tilted map draws them from where they were placed. fn label_at(app: &RustGisApp, rect: Rect, p: Pos2) -> Option { - if app.map.key.is_none_or(|key| key.rev != app.session.revision) { + if app.map.key.is_none_or(|key| key.rev != app.session.revision || key.sources != rustgis_engine::map::source::generation()) { return None; } let cam = app.session.map().camera; @@ -1514,7 +1514,7 @@ fn label_at(app: &RustGisApp, rect: Rect, p: Pos2) -> Option { // A flat map moving draws the labels placed last with its frame: shifted, and spread apart // or closed up as it zooms (as `main_view` does). let (shift, s) = if placed == cam { - (vec2(0.0, 0.0), 1.0) + (app.map.tex_rect.min - rect.min, 1.0) } else if app.session.map().projection != Projection::Globe && placed.pitch == 0.0 && cam.pitch == 0.0 && placed.bearing == cam.bearing { (app.map.tex_rect.min - rect.min, (app.map.tex_camera.res / cam.res) as f32) @@ -4516,6 +4516,17 @@ mod tests { let l = h.state().map.labels.iter().find(|l| l.text == "Place 3").unwrap(); rect.min + vec2(l.x, l.y) }; + // Hit testing follows a retained frame's origin and refuses its old source window. + let at = grab(&h); + let retained = h.state().map.tex_rect; + let shift = vec2(80.0, 80.0); + h.state_mut().map.tex_rect = retained.translate(shift); + assert_eq!(label_at(h.state(), rect, at + shift).unwrap().text, "Place 3"); + h.state_mut().map.tex_rect = retained; + let key = h.state().map.key; + h.state_mut().map.key.as_mut().unwrap().sources = rustgis_engine::map::source::generation().wrapping_add(1); + assert!(label_at(h.state(), rect, at).is_none()); + h.state_mut().map.key = key; let moved = |h: &egui_kittest::Harness<'_, RustGisApp>| h.state().session.map().layer(id).unwrap().labels.default.moved.clone(); let none = egui::Modifiers::NONE; let drag = |h: &mut egui_kittest::Harness<'_, RustGisApp>, escape: bool| { diff --git a/docs/user-guide/cartography.md b/docs/user-guide/cartography.md index e188f5f5a..36c9a1c15 100644 --- a/docs/user-guide/cartography.md +++ b/docs/user-guide/cartography.md @@ -471,6 +471,10 @@ Turn labels on per layer (Labeling › Labels › **Label**), then: anything else pans the map; Esc leaves the tool (`labels.move={"layer": "Cities", "feature": 12, "to": [2.5, 49.1]}`; `"to": null` puts it back; `labels.resetMoved={"layer": "Cities", "all": true}`). + Deleting a feature removes its saved move; Undo restores it. Service refresh and Repair Data Source reset saved + moves and old label locks because replacement data can reuse feature ids, with a counted notice + and undo restoration. Opening a project reports invalid, duplicate or excess moved-label entries + that were ignored, and callout lengths that were adjusted. Clean settings give no such notice. - **Font**: the labels' font family, any installed on the computer (Labeling pane › Symbol › **Font**; type part of a name to find it), or the label font the interface uses. Bold and italic take the family's own bold and italic faces where it has them. The family is saved by @@ -505,9 +509,6 @@ Turn labels on per layer (Labeling › Labels › **Label**), then: take the place a less important one had. The class's offset applies to every place, and Lock keeps the place each label took (`labels.placement={"layer": "Cities", "otherPositions": true}`). -Repair Data Source resets saved label moves, because replacement data can reuse feature ids. -A notice gives the count; Undo restores the old source and moves. - - **Lock** keeps the labelled features fixed while you pan and zoom; **Pause** stops drawing labels for faster navigation; **View Unplaced** draws the labels that didn't fit in red. They apply to the whole map, so they are on the ribbon (Labeling › Map Labeling), not in the pane. From 7b227d675bec0624e7a430e0e9131451d92b60a5 Mon Sep 17 00:00:00 2001 From: giswqs <5016453+giswqs@users.noreply.github.com> Date: Wed, 4 Nov 2026 14:55:49 +0000 Subject: [PATCH 05/15] fix(labels): report every dropped move, keep locks and drags in step Saved label moves that a command drops now say how many they lost: the cap pruning in labels.move, a service refresh whose service no longer offers a layer, and the label settings of a map, layer or style file read by import. lock_holds also compares moved places, so a change to them relocks the labels. A file bridge script that ran ends an in-progress label move. A relay request ends it only when it changes the project (Session::state), so read-only requests leave the move alone. Co-Authored-By: Claude Haiku 5.5 Applied by the claude-autofix workflow. --- crates/engine/src/cmd/items.rs | 13 +++++++-- crates/engine/src/cmd/labels.rs | 10 +++++-- crates/engine/src/cmd/service_group.rs | 7 +++++ crates/engine/src/tests.rs | 38 ++++++++++++++++++++++++++ crates/ui-egui/src/lib.rs | 11 ++++++-- 5 files changed, 73 insertions(+), 6 deletions(-) diff --git a/crates/engine/src/cmd/items.rs b/crates/engine/src/cmd/items.rs index 4bbd79257..ed12452ac 100644 --- a/crates/engine/src/cmd/items.rs +++ b/crates/engine/src/cmd/items.rs @@ -437,7 +437,12 @@ fn import_maps(s: &mut Session, p: &Value) -> CmdResult { vec![m] } Err(_) => match Project::from_import_json(&text, dir.as_deref()) { - Ok(project) => project.maps, + Ok(mut project) => { + for notice in std::mem::take(&mut project.label_read_notices) { + s.notify(notice, false); + } + project.maps + } Err(_) => { super::connections::restore_active(s); return Err("not a RustGIS map file, project or package".into()); @@ -495,7 +500,11 @@ fn import_styles(s: &mut Session, p: &Value) -> CmdResult { super::connections::clear_active(); let project = Project::from_import_json(&text, None); super::connections::restore_active(s); - project.map_err(|_| "not a RustGIS layer file, map file or project".to_string())?.maps.into_iter().flat_map(|m| m.layers).collect() + let mut project = project.map_err(|_| "not a RustGIS layer file, map file or project".to_string())?; + for notice in std::mem::take(&mut project.label_read_notices) { + s.notify(notice, false); + } + project.maps.into_iter().flat_map(|m| m.layers).collect() }; let mut n = 0; for l in layers.into_iter().filter(|l| !l.is_raster() && l.geometry_type() != GeometryType::None).take(1000) { diff --git a/crates/engine/src/cmd/labels.rs b/crates/engine/src/cmd/labels.rs index 780af5b86..872e759b8 100644 --- a/crates/engine/src/cmd/labels.rs +++ b/crates/engine/src/cmd/labels.rs @@ -244,12 +244,15 @@ pub fn commands() -> Vec { .filter(|l| !l.dataset.windowed) .filter(|l| l.labels.in_order().any(|c| c.moved.len() >= rustgis_map::symbology::MAX_MOVED_LABELS)) .map(|l| l.dataset.features.iter().map(|f| f.id).collect()); + let mut pruned = 0usize; let r = with_labels(s, p, |labels| { let c = class_in(labels, p)?; if let Some(live) = &live && c.moved.len() >= rustgis_map::symbology::MAX_MOVED_LABELS { + let before = c.moved.len(); c.moved.retain(|m| live.contains(&m.id)); + pruned = before - c.moved.len(); } if c.move_label(feature, to) { Ok(()) @@ -257,6 +260,9 @@ pub fn commands() -> Vec { Err(format!("a label class keeps at most {} labels moved", rustgis_map::symbology::MAX_MOVED_LABELS)) } })?; + if pruned > 0 { + s.notify(format!("{pruned} saved label move(s) of features no longer in the layer dropped to make room"), false); + } // Locked, the labels are locked again where they are placed next: the one moved may // not have been placed when the lock was taken. s.map_mut().label_lock = None; @@ -464,11 +470,11 @@ pub(crate) fn same_class_list(a: &Labeling, b: &Labeling) -> bool { } /// Whether a lock taken with the layer's labels still names those of `b`: the same classes, listed -/// alike, putting their point labels in the same places, with unchanged overlap and callouts. +/// alike, putting their point labels in the same places, with unchanged overlap, callouts and moves. pub(crate) fn lock_holds(l: &Layer, b: &Labeling) -> bool { same_class_list(&l.labels, b) && same_places(l, b) - && l.labels.in_order().zip(b.in_order()).all(|(x, y)| x.show_all == y.show_all && x.callout == y.callout) + && l.labels.in_order().zip(b.in_order()).all(|(x, y)| x.show_all == y.show_all && x.callout == y.callout && x.moved == y.moved) } /// Whether the layer's classes and those of `b` (listed alike) put their labels in the same places. diff --git a/crates/engine/src/cmd/service_group.rs b/crates/engine/src/cmd/service_group.rs index df18bad82..baf861809 100644 --- a/crates/engine/src/cmd/service_group.rs +++ b/crates/engine/src/cmd/service_group.rs @@ -374,7 +374,14 @@ fn refresh(s: &mut Session, p: &Value) -> CmdResult { let ids: Vec = children.iter().map(|c| c.layer).collect(); if child_id.is_none() { let gone: Vec = old.children.iter().map(|c| c.layer).filter(|id| !ids.contains(id)).collect(); + let mut gone_moved = 0; + for l in s.map().layers.iter().filter(|l| gone.contains(&l.id)) { + gone_moved += l.labels.in_order().map(|class| class.moved.len()).sum::(); + } crate::cmd::layer::drop_layers(s, &gone); + if gone_moved > 0 { + s.notify(format!("{gone_moved} saved label move(s) dropped with layers the refreshed service no longer offers"), false); + } if let Some(lock) = &mut s.map_mut().label_lock { lock.retain(|((id, _, _, _), _)| !gone.contains(id)); } diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 8c19ad53c..598f0cd9b 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -3325,6 +3325,25 @@ fn moved_labels_preserve_off_window_features_at_the_cap() { assert!(error.contains("not present"), "{error}"); } +/// A class at its most drops the saved moves of features no longer in the layer to make room, and +/// says how many; a class with room drops none and says nothing. +#[test] +fn moved_labels_of_removed_features_make_room_with_a_notice() { + use rustgis_map::symbology::{MAX_MOVED_LABELS, MovedLabel}; + let mut s = world(); + let id = s.map().layer_by_name("Cities").unwrap().id; + let fid = s.map().layer(id).unwrap().dataset.features.first().unwrap().id; + let layer = s.map_mut().layer_mut(id).unwrap(); + layer.labels.default.moved = (0..MAX_MOVED_LABELS).map(|i| MovedLabel { id: 1_000_000 + i as u64, at: [1.0, 1.0] }).collect(); + let mark = s.notice_mark(); + s.run("labels.move", &json!({"layer": id, "feature": fid, "to": [2, 2]})).unwrap(); + assert_eq!(s.map().layer(id).unwrap().labels.default.moved.len(), 1); + assert!(s.notices_since(mark).iter().any(|n| n.text.contains("10000 saved label move(s) of features no longer in the layer"))); + let mark = s.notice_mark(); + s.run("labels.move", &json!({"layer": id, "feature": fid, "to": [3, 3]})).unwrap(); + assert!(!s.notices_since(mark).iter().any(|n| n.text.contains("dropped to make room"))); +} + /// Labels are moved by hand by feature id and put back, one class's or all; bad input is refused. #[test] fn labels_are_moved_by_command_and_reset() { @@ -4070,6 +4089,25 @@ fn service_refresh_resets_saved_label_moves_with_a_notice() { assert!(!s.notices_since(mark).iter().any(|n| n.text.contains("saved label move(s) reset"))); } +/// A refresh whose service no longer offers a layer says how many saved moves went with it. +#[test] +fn service_refresh_says_how_many_moves_go_with_a_dropped_layer() { + let server = ogc_server::Server::group_fixture(); + let mut s = Session::new(Project::default()); + s.run("service.import", &json!({"url":server.root()})).unwrap(); + let routes = s.map().layer_by_name("Routes").unwrap().id; + let feature = s.map().layer(routes).unwrap().dataset.features[0].id; + s.run("labels.move", &json!({"layer":routes,"feature":feature,"to":[1,1]})).unwrap(); + // The service now offers only Places. + let items = json!({"rel": "items", "type": "application/geo+json", "href": "collections/places/items?key=fixture-key"}); + server.set(ogc_server::CATALOG, &json!({"collections": [{"id": "places", "title": "Places", "links": [items]}]}).to_string()); + let places = s.map().layer_by_name("Places").unwrap().id; + let mark = s.notice_mark(); + s.run("service.refresh", &json!({"layer":places})).unwrap(); + assert!(s.map().layer_by_name("Routes").is_none()); + assert!(s.notices_since(mark).iter().any(|n| n.text.contains("1 saved label move(s) dropped with layers"))); +} + /// Label classes by command (#618): added from Default and checked before it, renamed, given /// a clause, moved, styled and removed; refusals change nothing, and Default stays last. #[test] diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index fd77c3232..c49896fad 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -3894,7 +3894,9 @@ impl RustGisApp { } // A snapshot can capture the screen while the map is on it. relay.set_screen(relayshot::map_shown(self, ctx)); - if relay.poll(&mut self.session) > 0 { + // Only a request that changed the project ends a label move: a read-only one leaves it. + let state = self.session.state(); + if relay.poll(&mut self.session) > 0 && self.session.state() != state { self.map.cancel_label_drag(); } } @@ -3905,7 +3907,12 @@ impl RustGisApp { let ctx = ctx.clone(); bridge.set_wake(Box::new(move || ctx.request_repaint())); } - match bridge.poll(&mut self.session) { + // A script may replace the features a label move is for: it ends the move. + let ran = bridge.poll(&mut self.session); + if ran.is_some() { + self.map.cancel_label_drag(); + } + match ran { Some(o) if o.success => self.session.notify(tl!("File bridge: the script ran").to_string(), false), Some(o) => { let text = i18n::fmt("File bridge: {error}", &[("error", o.error.as_deref().unwrap_or(""))]); From 3e00a8bbcc93d08502600b4ecb83cc7ca041649d Mon Sep 17 00:00:00 2001 From: giswqs Date: Wed, 4 Nov 2026 10:22:04 -0500 Subject: [PATCH 06/15] Address review feedback - Fix two engine tests from main that still set label_lock with a bare place number; it now holds a LockedPlace (CI compile error). - labels.placement says when it brings a callout length outside 5 to 500 points within range, instead of clamping silently. - The label-settings audit counts Show all labels values that are not true or false, and runs for bare map files (map.import, style.import) and layer files (style.import, layer.addLayerFile), not only for projects. Tests cover clean and damaged files. --- crates/engine/src/cmd/items.rs | 9 +++ crates/engine/src/cmd/labels.rs | 15 ++++- crates/engine/src/cmd/layer.rs | 3 + crates/engine/src/project.rs | 2 +- crates/engine/src/project_label_losses.rs | 77 ++++++++++++++++------- crates/engine/src/tests.rs | 52 +++++++++++++-- docs/user-guide/cartography.md | 7 ++- 7 files changed, 135 insertions(+), 30 deletions(-) diff --git a/crates/engine/src/cmd/items.rs b/crates/engine/src/cmd/items.rs index ed12452ac..5502c16fd 100644 --- a/crates/engine/src/cmd/items.rs +++ b/crates/engine/src/cmd/items.rs @@ -434,6 +434,9 @@ fn import_maps(s: &mut Session, p: &Value) -> CmdResult { if let Some(d) = dir.as_deref().and_then(Path::to_str) { crate::project::resolve_file_paths(d, m.file_paths_mut()); } + for notice in crate::project::label_losses::map_notices(&text, &m) { + s.notify(notice, false); + } vec![m] } Err(_) => match Project::from_import_json(&text, dir.as_deref()) { @@ -492,8 +495,14 @@ fn add_style(s: &mut Session, mut item: StyleItem) { fn import_styles(s: &mut Session, p: &Value) -> CmdResult { let text = read_text(p)?; let layers: Vec = if let Ok(l) = serde_json::from_str::(&text) { + for notice in crate::project::label_losses::layer_notices(&text, &l) { + s.notify(notice, false); + } vec![l] } else if let Ok(m) = serde_json::from_str::(&text) { + for notice in crate::project::label_losses::map_notices(&text, &m) { + s.notify(notice, false); + } m.layers } else { // A project parse opens sources even though this import only takes their styles. diff --git a/crates/engine/src/cmd/labels.rs b/crates/engine/src/cmd/labels.rs index 872e759b8..8eea081c8 100644 --- a/crates/engine/src/cmd/labels.rs +++ b/crates/engine/src/cmd/labels.rs @@ -176,6 +176,13 @@ pub fn commands() -> Vec { _ => return Err("callout must be true or false, or its longest line in points".into()), }, }; + // A length outside the range is brought within it, and says so. + let callout_note = match (&callout, p.get("callout").and_then(number)) { + (Some(Callout::Reach(kept)), Some(asked)) if !rustgis_map::symbology::CALLOUT_REACH.contains(&asked) => { + Some(format!("callout length {asked} points adjusted to {kept} (callouts are 5 to 500 points long)")) + } + _ => None, + }; // A label tries other places when its own is taken: round its point (#1011), or along // its line (#1187). let others = match p.get("otherPositions") { @@ -185,7 +192,7 @@ pub fn commands() -> Vec { }; let id = s.layer_id(p)?; let (points, lines) = points_and_lines(s.map().layer(id).ok_or("no such layer")?); - with_labels(s, p, |labels| { + let done = with_labels(s, p, |labels| { let c = class_in(labels, p)?; if let Some(pos) = pos { c.placement = pos; @@ -214,7 +221,11 @@ pub fn commands() -> Vec { return Err("otherPositions is for points, and for lines placed parallel or curved without a repeat".into()); } Ok(()) - }) + })?; + if let Some(note) = callout_note { + s.notify(note, false); + } + Ok(done) }) .when(need_feature_layer) .params( diff --git a/crates/engine/src/cmd/layer.rs b/crates/engine/src/cmd/layer.rs index cdd0a6c7d..61151c4af 100644 --- a/crates/engine/src/cmd/layer.rs +++ b/crates/engine/src/cmd/layer.rs @@ -2635,6 +2635,9 @@ pub(crate) fn add_layer_text(s: &mut Session, text: &str) -> CmdResult { if layer.dataset.source.as_deref().is_some_and(|source| source.starts_with("geolibre-pending:")) { return Err("a layer file cannot contain runtime GeoLibre download reservations".into()); } + for notice in crate::project::label_losses::layer_notices(text, &layer) { + s.notify(notice, false); + } // A binding in a file is no authority to use a login here. crate::s3_connections::clear_bindings(layer.addresses_mut()); // A fresh id and a unique name in this map; the rest goes through project repair. diff --git a/crates/engine/src/project.rs b/crates/engine/src/project.rs index 3525a0bbe..c54f1eb9b 100644 --- a/crates/engine/src/project.rs +++ b/crates/engine/src/project.rs @@ -12,7 +12,7 @@ use serde::{Deserialize, Serialize}; use crate::GpRecord; #[path = "project_label_losses.rs"] -mod label_losses; +pub(crate) mod label_losses; /// A map export cannot keep runtime-only download reservations, even in a larger project. pub(crate) fn ensure_map_imports_complete(map: &Map) -> Result<(), String> { diff --git a/crates/engine/src/project_label_losses.rs b/crates/engine/src/project_label_losses.rs index 33014034e..784ff0c95 100644 --- a/crates/engine/src/project_label_losses.rs +++ b/crates/engine/src/project_label_losses.rs @@ -65,6 +65,8 @@ struct Class { moved: MoveCount, #[serde(default)] callout: Option, + #[serde(default)] + show_all: Option, } #[derive(Default, Deserialize)] @@ -74,6 +76,8 @@ struct Labels { #[serde(default)] callout: Option, #[serde(default)] + show_all: Option, + #[serde(default)] classes: Vec, } @@ -108,39 +112,70 @@ fn adjusted_callout(value: Option<&serde_json::Value>) -> bool { value.is_some_and(|v| v.as_f64().is_none_or(|n| n != 0.0 && !rustgis_map::symbology::CALLOUT_REACH.contains(&n))) } -/// Compare raw record counts with retained state, including offline snapshots; one notice per kind. -pub(super) fn notices(text: &str, project: &Project) -> Result, String> { - let file: File = serde_json::from_str(text).map_err(|e| format!("could not read project label settings: {e}"))?; - let (mut moves, mut callouts) = (0usize, 0usize); - for layer in file.maps.iter().chain(file.offline_areas.iter().map(|a| &a.data)).flat_map(|m| &m.layers).chain(file.styles.iter()) { +/// Whether a "Show all labels" setting is something other than true or false (null is none). +fn damaged_flag(value: Option<&serde_json::Value>) -> bool { + value.is_some_and(|v| !v.is_boolean() && !v.is_null()) +} + +/// Count what the raw layers hold against what the read labels kept; one notice per kind, each +/// naming the item read (`name`) and what it was (`what`: "the project", "the map file", ...). +fn count<'a>( + raw: impl Iterator, + retained: impl Iterator, + name: &str, + what: &str, +) -> Vec { + let (mut moves, mut callouts, mut flags) = (0usize, 0usize, 0usize); + for layer in raw { let labels = &layer.labels; moves = moves.saturating_add(labels.moved.0); callouts = callouts.saturating_add(usize::from(adjusted_callout(labels.callout.as_ref()))); + flags = flags.saturating_add(usize::from(damaged_flag(labels.show_all.as_ref()))); for class in &labels.classes { moves = moves.saturating_add(class.moved.0); callouts = callouts.saturating_add(usize::from(adjusted_callout(class.callout.as_ref()))); + flags = flags.saturating_add(usize::from(damaged_flag(class.show_all.as_ref()))); } } - let retained = project - .maps - .iter() - .chain(project.offline_areas.iter().map(|a| &a.data)) - .flat_map(|m| &m.layers) - .map(|l| &l.labels) - .chain(project.styles.iter().map(|s| &s.labels)) - .flat_map(|labels| labels.in_order()) - .map(|c| c.moved.len()) - .fold(0usize, usize::saturating_add); + let retained = retained.flat_map(|labels| labels.in_order()).map(|c| c.moved.len()).fold(0usize, usize::saturating_add); let dropped = moves.saturating_sub(retained); let mut messages = Vec::new(); if dropped > 0 { - messages.push(format!( - "{}: {dropped} invalid, duplicate or excess moved-label entry/setting(s) ignored while opening the project", - project.name - )); + messages.push(format!("{name}: {dropped} invalid, duplicate or excess moved-label entry/setting(s) ignored while opening {what}")); } if callouts > 0 { - messages.push(format!("{}: {callouts} callout length setting(s) adjusted or disabled while opening the project", project.name)); + messages.push(format!("{name}: {callouts} callout length setting(s) adjusted or disabled while opening {what}")); + } + if flags > 0 { + messages + .push(format!("{name}: {flags} \"show all labels\" setting(s) that were not true or false read as off while opening {what}")); } - Ok(messages) + messages +} + +/// Compare raw record counts with retained state, including offline snapshots; one notice per kind. +pub(super) fn notices(text: &str, project: &Project) -> Result, String> { + let file: File = serde_json::from_str(text).map_err(|e| format!("could not read project label settings: {e}"))?; + let raw = file.maps.iter().chain(file.offline_areas.iter().map(|a| &a.data)).flat_map(|m| &m.layers).chain(file.styles.iter()); + let retained = project + .maps + .iter() + .chain(project.offline_areas.iter().map(|a| &a.data)) + .flat_map(|m| &m.layers) + .map(|l| &l.labels) + .chain(project.styles.iter().map(|s| &s.labels)); + Ok(count(raw, retained, &project.name, "the project")) +} + +/// The same audit for a map file read on its own; unreadable label settings give no notice +/// (the map itself was read, so its labels were too). +pub(crate) fn map_notices(text: &str, map: &rustgis_map::Map) -> Vec { + let Ok(raw) = serde_json::from_str::(text) else { return Vec::new() }; + count(raw.layers.iter(), map.layers.iter().map(|l| &l.labels), &map.name, "the map file") +} + +/// The same audit for a layer file read on its own. +pub(crate) fn layer_notices(text: &str, layer: &rustgis_map::Layer) -> Vec { + let Ok(raw) = serde_json::from_str::(text) else { return Vec::new() }; + count(std::iter::once(&raw), std::iter::once(&layer.labels), &layer.name, "the layer file") } diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 598f0cd9b..1a1181654 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -3434,10 +3434,14 @@ fn labels_show_all_and_take_callouts() { // On: the default length, then one given, kept within range; on again keeps it. s.run("labels.placement", &json!({"layer": "Cities", "callout": true})).unwrap(); assert_eq!(class(&s).1, Some(rustgis_map::symbology::DEFAULT_CALLOUT)); + let adjusted = |s: &Session, mark| s.notices_since(mark).iter().filter(|n| n.text.contains("callout length")).count(); + let mark = s.notice_mark(); s.run("labels.placement", &json!({"layer": "Cities", "callout": 80})).unwrap(); assert_eq!(class(&s).1, Some(80.0)); + assert_eq!(adjusted(&s, mark), 0, "a length in range says nothing"); s.run("labels.placement", &json!({"layer": "Cities", "callout": 99999})).unwrap(); assert_eq!(class(&s).1, Some(500.0), "within its range"); + assert_eq!(adjusted(&s, mark), 1, "and says it was adjusted"); s.run("labels.placement", &json!({"layer": "Cities", "callout": true})).unwrap(); assert_eq!(class(&s).1, Some(500.0), "on again keeps its length"); // Saved with the layer, and read back. @@ -3669,7 +3673,7 @@ fn labels_take_other_positions_along_lines() { // Turned on or off, the labels are locked again, with the anchors of their places on the line. let key = (id, 0, 0, 0); for on in [false, true] { - s.map_mut().label_lock = Some(vec![(key, 1)]); + s.map_mut().label_lock = Some(vec![(key, rustgis_map::LockedPlace { place: 1, callout: None })]); s.map_mut().label_lock_line_anchors.insert(key, Default::default()); s.run("labels.placement", &json!({"layer": "Rivers", "class": "Big", "otherPositions": on})).unwrap(); assert!(s.map().label_lock.is_none() && s.map().label_lock_line_anchors.is_empty(), "{on}: locked again"); @@ -3806,7 +3810,7 @@ fn a_line_class_flag_with_no_effect_keeps_the_lock() { let id = s.map().layer_by_name("Rivers").unwrap().id; s.run("labels.placement", &json!({"layer": "Rivers", "position": "curved", "otherPositions": true})).unwrap(); let held = |s: &mut Session, layer: u32, name: &str, change: Value| { - s.map_mut().label_lock = Some(vec![((layer, 0, 0, 0), 1)]); + s.map_mut().label_lock = Some(vec![((layer, 0, 0, 0), rustgis_map::LockedPlace { place: 1, callout: None })]); let mut p = change; p["layer"] = json!(name); s.run("labels.placement", &p).unwrap(); @@ -3968,18 +3972,20 @@ fn opening_projects_reports_moved_label_and_callout_adjustments() { {"id":-1,"at":[2.0,3.0]}, {"id":1.5,"at":[2.0,3.0]}, {"id":2,"at":[2.0,100.0]} ]); labels["callout"] = json!(900); + labels["show_all"] = json!("yes"); damaged["styles"][0]["labels"]["moved"] = json!("damaged"); damaged["styles"][0]["labels"]["callout"] = json!(-1); let text = damaged.to_string(); let loaded = Project::from_json(&text).unwrap(); assert_eq!(loaded.maps[0].layers[0].labels.default.moved_to(1), Some([4.0, 5.0])); let startup = Session::new(loaded); - assert_eq!(startup.notices.len(), 2); + assert_eq!(startup.notices.len(), 3); assert!(startup.notices.iter().any(|n| n.text.contains("5 invalid, duplicate or excess moved-label"))); assert!(startup.notices.iter().any(|n| n.text.contains("2 callout length setting(s) adjusted"))); + assert!(startup.notices.iter().any(|n| n.text.contains("1 \"show all labels\" setting(s)"))); let mut opened = Session::new(Project::default()); opened.run("project.openText", &json!({"text":text})).unwrap(); - assert_eq!(opened.notices.iter().filter(|n| n.text.contains("while opening the project")).count(), 2); + assert_eq!(opened.notices.iter().filter(|n| n.text.contains("while opening the project")).count(), 3); assert!(!serde_json::to_string(&opened.project).unwrap().contains("label_read_notices")); let mut excess: Value = serde_json::from_str(&clean).unwrap(); excess["styles"][0]["labels"]["moved"] = @@ -3989,6 +3995,44 @@ fn opening_projects_reports_moved_label_and_callout_adjustments() { assert!(capped.notices[0].text.contains("1 invalid, duplicate or excess moved-label")); } +/// A map file or layer file read on its own reports discarded label settings as a project does +/// (Import map, Import Styles, Add Layer File); clean files stay quiet. +#[test] +fn map_and_layer_files_report_label_setting_losses() { + let s = world(); + let map = serde_json::to_value(s.map()).unwrap(); + let layer = serde_json::to_value(s.map().layer_by_name("Cities").unwrap()).unwrap(); + let damage = |labels: &mut Value| { + labels["moved"] = json!("damaged"); + labels["callout"] = json!(900); + labels["show_all"] = json!(3); + }; + let mut bad_map = map.clone(); + let i = bad_map["layers"].as_array().unwrap().iter().position(|l| l["name"] == "Cities").unwrap(); + damage(&mut bad_map["layers"][i]["labels"]); + let mut bad_layer = layer.clone(); + damage(&mut bad_layer["labels"]); + let said = |cmd: &str, text: &Value| { + let mut s = world(); + let mark = s.notice_mark(); + s.run(cmd, &json!({"text": text.to_string()})).unwrap(); + s.notices_since(mark).iter().filter(|n| n.text.contains("while opening the")).map(|n| n.text.clone()).collect::>() + }; + for (cmd, clean, bad) in [ + ("map.import", &map, &bad_map), + ("style.import", &map, &bad_map), + ("style.import", &layer, &bad_layer), + ("layer.addLayerFile", &layer, &bad_layer), + ] { + assert!(said(cmd, clean).is_empty(), "{cmd}: {:?}", said(cmd, clean)); + let notes = said(cmd, bad); + assert_eq!(notes.len(), 3, "{cmd}: {notes:?}"); + assert!(notes.iter().any(|n| n.contains("1 invalid, duplicate or excess moved-label")), "{cmd}: {notes:?}"); + assert!(notes.iter().any(|n| n.contains("1 callout length")), "{cmd}: {notes:?}"); + assert!(notes.iter().any(|n| n.contains("1 \"show all labels\"")), "{cmd}: {notes:?}"); + } +} + /// A streamed layer holds only the window in view: a lock's entry for a feature outside it stays /// when the window changes (#1135). #[test] diff --git a/docs/user-guide/cartography.md b/docs/user-guide/cartography.md index 69a090338..b8e2cab59 100644 --- a/docs/user-guide/cartography.md +++ b/docs/user-guide/cartography.md @@ -473,8 +473,11 @@ Turn labels on per layer (Labeling › Labels › **Label**), then: `labels.resetMoved={"layer": "Cities", "all": true}`). Deleting a feature removes its saved move; Undo restores it. Service refresh and Repair Data Source reset saved moves and old label locks because replacement data can reuse feature ids, with a counted notice - and undo restoration. Opening a project reports invalid, duplicate or excess moved-label entries - that were ignored, and callout lengths that were adjusted. Clean settings give no such notice. + and undo restoration. Opening a project, importing a map or styles, or adding a layer file + reports invalid, duplicate or excess moved-label entries that were ignored, callout lengths that + were adjusted, and Show all labels settings that were not true or false. A callout length given + to `labels.placement` outside 5 to 500 points is brought within range with a notice. Clean + settings give no such notice. - **Font**: the labels' font family, any installed on the computer (Labeling pane › Symbol › **Font**; type part of a name to find it), or the label font the interface uses. Bold and italic take the family's own bold and italic faces where it has them. The family is saved by From 95972df0a51366f212a5e108966cf61f0fc70ec8 Mon Sep 17 00:00:00 2001 From: giswqs Date: Wed, 4 Nov 2026 10:42:35 -0500 Subject: [PATCH 07/15] Address Claude review feedback - service_refresh_says_how_many_moves_go_with_a_dropped_layer refreshes the whole group: a refresh of one layer never drops its siblings, so the test failed. - style.apply no longer carries the style's saved label moves, which name another layer's features; the layer keeps its own moves, in the class of the same name, and notices count what was left out. - A project template leaves out saved label moves with its features, with a counted notice. --- crates/engine/src/cmd/items.rs | 54 ++++++++++++++++++++++++++++++---- crates/engine/src/tests.rs | 50 +++++++++++++++++++++++++++++-- 2 files changed, 96 insertions(+), 8 deletions(-) diff --git a/crates/engine/src/cmd/items.rs b/crates/engine/src/cmd/items.rs index 5502c16fd..87ab24a92 100644 --- a/crates/engine/src/cmd/items.rs +++ b/crates/engine/src/cmd/items.rs @@ -99,8 +99,12 @@ pub fn commands() -> Vec { } l.renderer = st.renderer; let field = l.labels.default.field.clone(); - let relock = !super::labels::lock_holds(l, &st.labels); - l.labels = st.labels; + // Saved moves name features by id: the style's belong to the layer it was made from, + // so this layer keeps its own, in the class of the same name (Default in Default). + let mut labels = st.labels; + let (foreign, lost) = carry_moves(&mut l.labels, &mut labels); + let relock = !super::labels::lock_holds(l, &labels); + l.labels = labels; // The label field stays if the style's isn't in this layer. if l.dataset.field_index(&l.labels.default.field).is_none() { l.labels.default.field = field; @@ -111,6 +115,12 @@ pub fn commands() -> Vec { s.map_mut().label_lock = None; s.map_mut().label_lock_line_anchors.clear(); } + if foreign > 0 { + s.notify(format!("{foreign} saved label move(s) in style {name} not applied: they name another layer's features"), false); + } + if lost > 0 { + s.notify(format!("{lost} saved label move(s) dropped with label classes style {name} doesn't have"), false); + } Ok(Value::Null) }) .when(need_feature_layer) @@ -536,11 +546,42 @@ fn import_styles(s: &mut Session, p: &Value) -> CmdResult { } /// The project without its features (layer schemas, symbology, layouts and styles stay). -fn template(p: &Project) -> Project { +/// Moves `old`'s saved label moves into the class of the same name in `new` (Default into +/// Default), after dropping `new`'s own; returns how many of `new`'s were dropped, and how many +/// of `old`'s had no class to go to. +fn carry_moves(old: &mut rustgis_map::symbology::Labeling, new: &mut rustgis_map::symbology::Labeling) -> (usize, usize) { + let mut foreign = 0usize; + for c in new.classes.iter_mut().chain(std::iter::once(&mut new.default)) { + foreign = foreign.saturating_add(c.moved.len()); + c.moved.clear(); + } + new.default.moved = std::mem::take(&mut old.default.moved); + let mut lost = 0usize; + for c in &mut old.classes { + let moved = std::mem::take(&mut c.moved); + if moved.is_empty() { + continue; + } + match new.classes.iter_mut().find(|n| n.name.eq_ignore_ascii_case(&c.name) && n.moved.is_empty()) { + Some(n) => n.moved = moved, + None => lost = lost.saturating_add(moved.len()), + } + } + (foreign, lost) +} + +/// The project without its data, and how many saved label moves went with the features. +fn template(p: &Project) -> (Project, usize) { let mut t = p.clone(); t.gp_history.clear(); + let mut moves = 0usize; for m in &mut t.maps { for l in &mut m.layers { + // Saved label moves name features, and a template has none. + for c in l.labels.classes.iter_mut().chain(std::iter::once(&mut l.labels.default)) { + moves = moves.saturating_add(c.moved.len()); + c.moved.clear(); + } l.dataset.features.clear(); l.selection.clear(); l.dataset.source = None; @@ -559,7 +600,7 @@ fn template(p: &Project) -> Project { for t in &mut t.tables { t.features.clear(); } - t + (t, moves) } /// Packages only completed imports; runtime download reservations cannot be reopened. @@ -1065,7 +1106,10 @@ pub fn item_bytes(s: &mut Session, kind: &str, p: &Value) -> Result<(String, Vec } "template" => { s.project.ensure_imports_complete()?; - let mut t = template(&s.project); + let (mut t, moves) = template(&s.project); + if moves > 0 { + s.notify(format!("{moves} saved label move(s) left out of the template, which keeps no features"), false); + } if let Some(n) = ps(p, "name").map(str::trim).filter(|n| !n.is_empty()) { t.name = n.chars().take(200).collect(); } diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 1a1181654..8b48dbae3 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -3995,6 +3995,50 @@ fn opening_projects_reports_moved_label_and_callout_adjustments() { assert!(capped.notices[0].text.contains("1 invalid, duplicate or excess moved-label")); } +/// A style's saved label moves name the features of the layer it was made from: applied, the +/// layer keeps its own moves instead, and says what it left out; a style without moves is quiet. +#[test] +fn applying_a_style_keeps_the_layers_own_label_moves() { + let mut s = world(); + let id = s.map().layer_by_name("Cities").unwrap().id; + let ids: Vec = s.map().layer(id).unwrap().dataset.features.iter().take(2).map(|f| f.id).collect(); + let said = |s: &Session, mark| s.notices_since(mark).iter().filter(|n| n.text.contains("label move(s)")).count(); + s.run("style.new", &json!({"layer": id, "name": "Plain"})).unwrap(); + s.run("labels.move", &json!({"layer": id, "feature": ids[0], "to": [1, 1]})).unwrap(); + s.run("style.new", &json!({"layer": id, "name": "Moved"})).unwrap(); + s.run("labels.move", &json!({"layer": id, "feature": ids[0], "to": null})).unwrap(); + s.run("labels.move", &json!({"layer": id, "feature": ids[1], "to": [2, 2]})).unwrap(); + let mark = s.notice_mark(); + s.run("style.apply", &json!({"layer": id, "name": "Plain"})).unwrap(); + assert_eq!(said(&s, mark), 0, "{:?}", s.notices_since(mark)); + let mark = s.notice_mark(); + s.run("style.apply", &json!({"layer": id, "name": "Moved"})).unwrap(); + let labels = &s.map().layer(id).unwrap().labels.default; + assert_eq!((labels.moved_to(ids[0]), labels.moved_to(ids[1])), (None, Some([2.0, 2.0]))); + assert!(s.notices_since(mark).iter().any(|n| n.text.contains("1 saved label move(s) in style Moved not applied"))); +} + +/// A project template keeps no features, so no saved label moves either, and says so. +#[test] +fn a_project_template_leaves_out_label_moves() { + let dir = std::env::temp_dir().join(format!("rustgis-template-moves-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("t.rgis"); + let mut s = world(); + let mark = s.notice_mark(); + s.run("project.saveTemplate", &json!({"path": path})).unwrap(); + assert!(!s.notices_since(mark).iter().any(|n| n.text.contains("label move(s)"))); + let id = s.map().layer_by_name("Cities").unwrap().id; + let feature = s.map().layer(id).unwrap().dataset.features[0].id; + s.run("labels.move", &json!({"layer": id, "feature": feature, "to": [1, 1]})).unwrap(); + let mark = s.notice_mark(); + s.run("project.saveTemplate", &json!({"path": path})).unwrap(); + assert!(s.notices_since(mark).iter().any(|n| n.text.contains("1 saved label move(s) left out of the template"))); + let saved: Value = serde_json::from_str(&std::fs::read_to_string(&path).unwrap()).unwrap(); + assert!(!saved.to_string().contains("\"moved\"")); + std::fs::remove_dir_all(&dir).ok(); +} + /// A map file or layer file read on its own reports discarded label settings as a project does /// (Import map, Import Styles, Add Layer File); clean files stay quiet. #[test] @@ -4138,16 +4182,16 @@ fn service_refresh_resets_saved_label_moves_with_a_notice() { fn service_refresh_says_how_many_moves_go_with_a_dropped_layer() { let server = ogc_server::Server::group_fixture(); let mut s = Session::new(Project::default()); - s.run("service.import", &json!({"url":server.root()})).unwrap(); + let group = s.run("service.import", &json!({"url":server.root()})).unwrap()["group"].clone(); let routes = s.map().layer_by_name("Routes").unwrap().id; let feature = s.map().layer(routes).unwrap().dataset.features[0].id; s.run("labels.move", &json!({"layer":routes,"feature":feature,"to":[1,1]})).unwrap(); // The service now offers only Places. let items = json!({"rel": "items", "type": "application/geo+json", "href": "collections/places/items?key=fixture-key"}); server.set(ogc_server::CATALOG, &json!({"collections": [{"id": "places", "title": "Places", "links": [items]}]}).to_string()); - let places = s.map().layer_by_name("Places").unwrap().id; let mark = s.notice_mark(); - s.run("service.refresh", &json!({"layer":places})).unwrap(); + // The whole group: a refresh of one layer leaves its siblings as they are. + s.run("service.refresh", &json!({"group":group})).unwrap(); assert!(s.map().layer_by_name("Routes").is_none()); assert!(s.notices_since(mark).iter().any(|n| n.text.contains("1 saved label move(s) dropped with layers"))); } From eadaf3858af7f4dc0bcb0f6c4d66d9b125530db6 Mon Sep 17 00:00:00 2001 From: giswqs Date: Wed, 4 Nov 2026 11:26:17 -0500 Subject: [PATCH 08/15] Address Claude review feedback - style.apply compares the label lock against the layer with its moves still in it: carry_moves now copies them instead of taking them, so a style that places labels as they are keeps the lock (test added). - Releasing a dragged label where there is no longitude and latitude (a tilted map's sky, past the globe's edge) says so instead of snapping back silently. --- crates/engine/src/cmd/items.rs | 19 ++++++++----------- crates/engine/src/tests.rs | 6 ++++++ crates/ui-egui/src/mapview.rs | 6 +++++- 3 files changed, 19 insertions(+), 12 deletions(-) diff --git a/crates/engine/src/cmd/items.rs b/crates/engine/src/cmd/items.rs index 87ab24a92..ba0aac7c1 100644 --- a/crates/engine/src/cmd/items.rs +++ b/crates/engine/src/cmd/items.rs @@ -102,7 +102,8 @@ pub fn commands() -> Vec { // Saved moves name features by id: the style's belong to the layer it was made from, // so this layer keeps its own, in the class of the same name (Default in Default). let mut labels = st.labels; - let (foreign, lost) = carry_moves(&mut l.labels, &mut labels); + let (foreign, lost) = carry_moves(&l.labels, &mut labels); + // Compared with the layer as it stands, its moves still in it. let relock = !super::labels::lock_holds(l, &labels); l.labels = labels; // The label field stays if the style's isn't in this layer. @@ -546,25 +547,21 @@ fn import_styles(s: &mut Session, p: &Value) -> CmdResult { } /// The project without its features (layer schemas, symbology, layouts and styles stay). -/// Moves `old`'s saved label moves into the class of the same name in `new` (Default into +/// Copies `old`'s saved label moves into the class of the same name in `new` (Default into /// Default), after dropping `new`'s own; returns how many of `new`'s were dropped, and how many /// of `old`'s had no class to go to. -fn carry_moves(old: &mut rustgis_map::symbology::Labeling, new: &mut rustgis_map::symbology::Labeling) -> (usize, usize) { +fn carry_moves(old: &rustgis_map::symbology::Labeling, new: &mut rustgis_map::symbology::Labeling) -> (usize, usize) { let mut foreign = 0usize; for c in new.classes.iter_mut().chain(std::iter::once(&mut new.default)) { foreign = foreign.saturating_add(c.moved.len()); c.moved.clear(); } - new.default.moved = std::mem::take(&mut old.default.moved); + new.default.moved.clone_from(&old.default.moved); let mut lost = 0usize; - for c in &mut old.classes { - let moved = std::mem::take(&mut c.moved); - if moved.is_empty() { - continue; - } + for c in old.classes.iter().filter(|c| !c.moved.is_empty()) { match new.classes.iter_mut().find(|n| n.name.eq_ignore_ascii_case(&c.name) && n.moved.is_empty()) { - Some(n) => n.moved = moved, - None => lost = lost.saturating_add(moved.len()), + Some(n) => n.moved.clone_from(&c.moved), + None => lost = lost.saturating_add(c.moved.len()), } } (foreign, lost) diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 8b48dbae3..c6196e0a7 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -4008,6 +4008,12 @@ fn applying_a_style_keeps_the_layers_own_label_moves() { s.run("style.new", &json!({"layer": id, "name": "Moved"})).unwrap(); s.run("labels.move", &json!({"layer": id, "feature": ids[0], "to": null})).unwrap(); s.run("labels.move", &json!({"layer": id, "feature": ids[1], "to": [2, 2]})).unwrap(); + // A style that places the labels as they are keeps them locked, the layer's moves and all. + s.run("style.new", &json!({"layer": id, "name": "Same"})).unwrap(); + s.map_mut().label_lock = Some(vec![((id, 0, 0, 0), rustgis_map::LockedPlace::default())]); + s.run("style.apply", &json!({"layer": id, "name": "Same"})).unwrap(); + assert!(s.map().label_lock.is_some(), "placement unchanged keeps the lock"); + assert_eq!(s.map().layer(id).unwrap().labels.default.moved_to(ids[1]), Some([2.0, 2.0])); let mark = s.notice_mark(); s.run("style.apply", &json!({"layer": id, "name": "Plain"})).unwrap(); assert_eq!(said(&s, mark), 0, "{:?}", s.notices_since(mark)); diff --git a/crates/ui-egui/src/mapview.rs b/crates/ui-egui/src/mapview.rs index 114d3b424..561e065bc 100644 --- a/crates/ui-egui/src/mapview.rs +++ b/crates/ui-egui/src/mapview.rs @@ -2455,7 +2455,11 @@ fn interact(app: &mut RustGisApp, ui: &mut Ui, resp: &egui::Response, rect: Rect let moved = app.map.moving_label.as_ref().zip(end).map(|(m, end)| moved_label(&m.label, end - origin)); if resp.drag_stopped() { app.map.drag = None; - let to = moved.and_then(|l| screen_to_lonlat(app, rect, rect.min + vec2(l.x, l.y))); + let to = moved.as_ref().and_then(|l| screen_to_lonlat(app, rect, rect.min + vec2(l.x, l.y))); + // Over a tilted map's sky or past the globe's edge there is no place to keep it at. + if moved.is_some() && to.is_none() { + app.session.notify(tl!("No place there to move the label to; it stays where it was."), true); + } if let (Some(c), Some(m)) = (to, app.map.moving_label.take()) && let Err(e) = app.run("labels.move", json!({"layer": m.layer, "class": m.class, "feature": m.feature, "to": [c.x, c.y]})) { From 5fef05ca765f7d0e1faee3bc29ba1a84da2c0632 Mon Sep 17 00:00:00 2001 From: giswqs Date: Wed, 4 Nov 2026 11:50:35 -0500 Subject: [PATCH 09/15] Address Claude review feedback - labels.move and labels.resetMoved no longer clear the label lock themselves: with_labels already locks again when the moves change, so a Reset Moved with nothing moved, or a move to where the label already is, keeps the lock (tests added). - The style-apply notice no longer says the style's moves belong to another layer: the style may have been made from this one. --- crates/engine/src/cmd/items.rs | 2 +- crates/engine/src/cmd/labels.rs | 9 ++++----- crates/engine/src/tests.rs | 7 +++++++ 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/crates/engine/src/cmd/items.rs b/crates/engine/src/cmd/items.rs index ba0aac7c1..db9fb1c46 100644 --- a/crates/engine/src/cmd/items.rs +++ b/crates/engine/src/cmd/items.rs @@ -117,7 +117,7 @@ pub fn commands() -> Vec { s.map_mut().label_lock_line_anchors.clear(); } if foreign > 0 { - s.notify(format!("{foreign} saved label move(s) in style {name} not applied: they name another layer's features"), false); + s.notify(format!("{foreign} saved label move(s) in style {name} not applied: they name the features of the layer the style was made from"), false); } if lost > 0 { s.notify(format!("{lost} saved label move(s) dropped with label classes style {name} doesn't have"), false); diff --git a/crates/engine/src/cmd/labels.rs b/crates/engine/src/cmd/labels.rs index 8eea081c8..5ecaca197 100644 --- a/crates/engine/src/cmd/labels.rs +++ b/crates/engine/src/cmd/labels.rs @@ -274,9 +274,8 @@ pub fn commands() -> Vec { if pruned > 0 { s.notify(format!("{pruned} saved label move(s) of features no longer in the layer dropped to make room"), false); } - // Locked, the labels are locked again where they are placed next: the one moved may - // not have been placed when the lock was taken. - s.map_mut().label_lock = None; + // Locked, the labels are locked again where they are placed next (`with_labels`: the + // moves changed), as the one moved may not have been placed when the lock was taken. Ok(r) }) .when(need_feature_layer) @@ -295,8 +294,8 @@ pub fn commands() -> Vec { } Ok(()) })?; - // As after a move: those put back are locked again where they are placed next. - s.map_mut().label_lock = None; + // As after a move, those put back are locked again where they are placed next; with + // nothing moved, nothing changes and the lock holds (`with_labels`). Ok(r) }) .when(need_feature_layer) diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 7e7bcb44b..f3f95623f 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -3395,8 +3395,15 @@ fn labels_are_moved_by_command_and_reset() { let v = crate::render::View { width: 800.0, height: 600.0, pixels_per_point: 1.0, camera: s.map().camera, sky: false }; crate::render::render(s.map_mut(), &v).unwrap(); assert!(s.map().label_lock.is_some()); + // With nothing moved, Reset Moved changes nothing and the lock holds. + s.run("labels.resetMoved", &json!({"layer": id, "all": true})).unwrap(); + assert!(s.map().label_lock.is_some(), "a reset with nothing moved keeps the lock"); s.run("labels.move", &json!({"layer": id, "feature": fid, "to": [2, 2]})).unwrap(); assert!(s.map().label_lock.is_none() && s.map().label_display.locked, "locked again, not unlocked"); + // A move to where the label already is changes nothing either. + crate::render::render(s.map_mut(), &v).unwrap(); + s.run("labels.move", &json!({"layer": id, "feature": fid, "to": [2, 2]})).unwrap(); + assert!(s.map().label_lock.is_some(), "a move that changes nothing keeps the lock"); // Converted to annotations, a moved label's annotation is where it was put, and the // annotations' classes keep none moved (their ids aren't the features'). s.run("labels.lock", &json!({"on": false})).unwrap(); From 3d3f99eef6ccd3695cb4e00f70520bcee2c15ea6 Mon Sep 17 00:00:00 2001 From: giswqs Date: Wed, 4 Nov 2026 12:12:20 -0500 Subject: [PATCH 10/15] Address Claude review feedback - A project template also leaves out the saved label moves held by its styles and offline snapshots, counted in the same notice (test extended to a style saved from a moved layer). - Put template's doc back on template (it had landed on carry_moves), and describe draw_label as drawing every label, not only previews. --- crates/engine/src/cmd/items.rs | 23 ++++++++++++++++------- crates/engine/src/tests.rs | 4 +++- crates/ui-egui/src/mapview.rs | 3 ++- 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/crates/engine/src/cmd/items.rs b/crates/engine/src/cmd/items.rs index db9fb1c46..2a574cb26 100644 --- a/crates/engine/src/cmd/items.rs +++ b/crates/engine/src/cmd/items.rs @@ -546,7 +546,6 @@ fn import_styles(s: &mut Session, p: &Value) -> CmdResult { Ok(json!({"imported": n})) } -/// The project without its features (layer schemas, symbology, layouts and styles stay). /// Copies `old`'s saved label moves into the class of the same name in `new` (Default into /// Default), after dropping `new`'s own; returns how many of `new`'s were dropped, and how many /// of `old`'s had no class to go to. @@ -567,18 +566,28 @@ fn carry_moves(old: &rustgis_map::symbology::Labeling, new: &mut rustgis_map::sy (foreign, lost) } -/// The project without its data, and how many saved label moves went with the features. +/// The project without its features (layer schemas, symbology, layouts and styles stay), and how +/// many saved label moves went with them. fn template(p: &Project) -> (Project, usize) { let mut t = p.clone(); t.gp_history.clear(); + // Saved label moves name features, and a template has none: the layers', the saved styles' + // and the offline snapshots' all go. let mut moves = 0usize; + let labels = t + .maps + .iter_mut() + .chain(t.offline_areas.iter_mut().map(|a| &mut a.data)) + .flat_map(|m| m.layers.iter_mut().map(|l| &mut l.labels)) + .chain(t.styles.iter_mut().map(|s| &mut s.labels)); + for labels in labels { + for c in labels.classes.iter_mut().chain(std::iter::once(&mut labels.default)) { + moves = moves.saturating_add(c.moved.len()); + c.moved.clear(); + } + } for m in &mut t.maps { for l in &mut m.layers { - // Saved label moves name features, and a template has none. - for c in l.labels.classes.iter_mut().chain(std::iter::once(&mut l.labels.default)) { - moves = moves.saturating_add(c.moved.len()); - c.moved.clear(); - } l.dataset.features.clear(); l.selection.clear(); l.dataset.source = None; diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index f3f95623f..9165ee41b 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -4044,9 +4044,11 @@ fn a_project_template_leaves_out_label_moves() { let id = s.map().layer_by_name("Cities").unwrap().id; let feature = s.map().layer(id).unwrap().dataset.features[0].id; s.run("labels.move", &json!({"layer": id, "feature": feature, "to": [1, 1]})).unwrap(); + // A style saved from the layer holds the move too. + s.run("style.new", &json!({"layer": id, "name": "Moved"})).unwrap(); let mark = s.notice_mark(); s.run("project.saveTemplate", &json!({"path": path})).unwrap(); - assert!(s.notices_since(mark).iter().any(|n| n.text.contains("1 saved label move(s) left out of the template"))); + assert!(s.notices_since(mark).iter().any(|n| n.text.contains("2 saved label move(s) left out of the template"))); let saved: Value = serde_json::from_str(&std::fs::read_to_string(&path).unwrap()).unwrap(); assert!(!saved.to_string().contains("\"moved\"")); std::fs::remove_dir_all(&dir).ok(); diff --git a/crates/ui-egui/src/mapview.rs b/crates/ui-egui/src/mapview.rs index 561e065bc..62ce41d93 100644 --- a/crates/ui-egui/src/mapview.rs +++ b/crates/ui-egui/src/mapview.rs @@ -1566,7 +1566,8 @@ fn label_class_name(app: &RustGisApp, layer: u32, class: u32) -> Option app.session.map().layer(layer)?.labels.in_order().nth(class as usize).map(|c| c.name.clone()) } -/// Paints a moved-label preview, keeping callouts below text and curved glyph halos below ink. +/// Paints a label (on the map, or a moved one's preview), keeping callouts below text and curved +/// glyph halos below ink. fn draw_label(p: &egui::Painter, l: &PlacedLabel, at: Pos2) { // A callout's line, from its feature to the edge of the text, under the text (its halo // under the line, so it shows over the map). From 1912138e0129b891bc706283400f26f4db256b5c Mon Sep 17 00:00:00 2001 From: giswqs Date: Wed, 4 Nov 2026 12:48:39 -0500 Subject: [PATCH 11/15] Address Claude review feedback - A placed label's callout anchor is its feature's offset from where it was placed, not from its first spot, so a drag's preview line starts at the feature (test checks callout_to matches the placed line). - callout false or null is a clean setting: no adjusted-callout notice on open or import (test added for false, null and 0). - Repair Data Source keeps clearing the whole label lock, now with a comment on why: only the labels a lock names are drawn, so dropping the layer's entries alone would hide its labels. --- crates/engine/src/cmd/repair.rs | 5 ++++- crates/engine/src/project_label_losses.rs | 8 ++++++-- crates/engine/src/tests.rs | 7 +++++++ crates/render/src/lib.rs | 9 +++++++++ 4 files changed, 26 insertions(+), 3 deletions(-) diff --git a/crates/engine/src/cmd/repair.rs b/crates/engine/src/cmd/repair.rs index aa36e8373..c5b123b31 100644 --- a/crates/engine/src/cmd/repair.rs +++ b/crates/engine/src/cmd/repair.rs @@ -100,7 +100,10 @@ fn repair_source(s: &mut Session, p: &Value) -> CmdResult { let l = s.map_mut().layer_mut(id).ok_or("the layer no longer exists")?; let moved = l.replace_source(new); let name = l.name.clone(); - // A lock's feature ids and callout offsets belong to the old source too. + // A lock's feature ids and callout offsets belong to the old source too. The whole + // lock goes, not just this layer's entries: while locked only the labels it names are + // drawn, so dropping this layer's alone would hide its labels until the lock is + // taken again. Cleared, every label is locked again where it is placed next. s.map_mut().label_lock = None; s.map_mut().label_lock_line_anchors.clear(); if moved > 0 { diff --git a/crates/engine/src/project_label_losses.rs b/crates/engine/src/project_label_losses.rs index 784ff0c95..0b91966e7 100644 --- a/crates/engine/src/project_label_losses.rs +++ b/crates/engine/src/project_label_losses.rs @@ -107,9 +107,13 @@ struct File { styles: Vec, } -/// Zero and null explicitly disable callouts; other lengths must fit the supported range. +/// Zero, false and null explicitly disable callouts; other lengths must fit the supported range. fn adjusted_callout(value: Option<&serde_json::Value>) -> bool { - value.is_some_and(|v| v.as_f64().is_none_or(|n| n != 0.0 && !rustgis_map::symbology::CALLOUT_REACH.contains(&n))) + value.is_some_and(|v| { + !v.is_null() + && v.as_bool() != Some(false) + && v.as_f64().is_none_or(|n| n != 0.0 && !rustgis_map::symbology::CALLOUT_REACH.contains(&n)) + }) } /// Whether a "Show all labels" setting is something other than true or false (null is none). diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 9165ee41b..76f215639 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -3972,6 +3972,13 @@ fn opening_projects_reports_moved_label_and_callout_adjustments() { let clean = serde_json::to_string(&project).unwrap(); let clean_session = Session::new(Project::from_json(&clean).unwrap()); assert!(clean_session.notices.is_empty()); + // Callouts written off (false, null or 0) are clean too. + for off in [json!(false), json!(null), json!(0)] { + let mut v: Value = serde_json::from_str(&clean).unwrap(); + v["maps"][0]["layers"][0]["labels"]["callout"] = off.clone(); + v["styles"][0]["labels"]["callout"] = off.clone(); + assert!(Session::new(Project::from_json(&v.to_string()).unwrap()).notices.is_empty(), "{off}"); + } let mut damaged: Value = serde_json::from_str(&clean).unwrap(); let labels = &mut damaged["maps"][0]["layers"][0]["labels"]; labels["moved"] = json!([ diff --git a/crates/render/src/lib.rs b/crates/render/src/lib.rs index c5c5ba76c..c2d1ed3b9 100644 --- a/crates/render/src/lib.rs +++ b/crates/render/src/lib.rs @@ -5381,6 +5381,11 @@ fn place_labels_with_previews( if let Some(anchor) = line.as_ref().and_then(|line| line.anchors.get(i)).copied().flatten() { out.line_anchors.push((key, anchor)); } + // Its feature stays where it is: the anchor is its offset from the label as placed + // (an alternative place, a callout or a lock may have moved it). + if let Some(f) = feature { + l.collide.callout_anchor = Some([f[0] - l.x, f[1] - l.y]); + } out.labels.push(l); out.keys.push((key, rustgis_map::LockedPlace { place: u8::try_from(i).unwrap_or(u8::MAX), callout: moved_by })); } @@ -5752,6 +5757,10 @@ mod tests { assert!(first.collide.callout.is_none(), "in its own spot, no line"); let [x0, y0, x1, y1] = second.collide.callout.expect("a callout's line"); assert!(((second.x + x0) - 100.0).abs() < 1e-3 && ((second.y + y0) - 60.0).abs() < 1e-3, "it starts at the feature"); + // Its anchor names the feature from where it was moved to, as a drag's preview reads it. + let [ax, ay] = second.collide.callout_anchor.expect("an anchor"); + assert!(((second.x + ax) - 100.0).abs() < 1e-3 && ((second.y + ay) - 60.0).abs() < 1e-3, "anchor follows the move"); + assert_eq!(callout_to(second), second.collide.callout, "the preview's line is the placed one"); let len = (x1 - x0).hypot(y1 - y0); assert!((1.0..=40.0).contains(&len), "{len} points"); let (a, b) = (label_boxes(first).0, label_boxes(second).0); From edb48e66383a5ecd30b8cedca6bc3d2d70741973 Mon Sep 17 00:00:00 2001 From: giswqs Date: Wed, 4 Nov 2026 21:31:32 -0500 Subject: [PATCH 12/15] Address Claude review feedback - Callouts: gate on the label's placement (along a line or not), not on its angle, so point labels turned by an angle expression may still move off a crowded spot; regression test added. --- crates/render/src/lib.rs | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/crates/render/src/lib.rs b/crates/render/src/lib.rs index 5913f21af..8d3b0bb48 100644 --- a/crates/render/src/lib.rs +++ b/crates/render/src/lib.rs @@ -4235,9 +4235,10 @@ fn render_frame( collide: rustgis_tiles::Collide { // Shown all: over the other labels where they crowd. text_allow_overlap: class.show_all, - // Callouts: a label set by its feature (not along a line) - // may move off a crowded spot, a line drawn back to it. - callout_reach: if angle.is_none() && glyphs.is_empty() { + // Callouts: a label set by its feature (not along a line, + // rotated or not) may move off a crowded spot, a line drawn + // back to it. + callout_reach: if !along_line(class.placement, g) && glyphs.is_empty() { class.callout.unwrap_or(0.0) } else { 0.0 @@ -6498,6 +6499,27 @@ mod tests { assert_eq!(spots, 0); } + /// A point label turned by its angle expression may still move off by a callout: only labels + /// along lines are left out of callouts (it was left out, silently, for any angle). + #[test] + fn a_rotated_point_label_moves_off_by_a_callout() { + let mut m = Map::new("Map"); + let mut d = Dataset::new("pts", GeometryType::Point, vec![Field::new("name", FieldType::Text)]); + d.push(Some(Geometry::Point(Coord::new(0.0, 0.0))), vec![Value::Text("First place".into())]); + d.push(Some(Geometry::Point(Coord::new(0.01, 0.0))), vec![Value::Text("Second place".into())]); + let id = m.add_dataset(d); + let l = m.layer_mut(id).unwrap(); + l.labels.enabled = true; + l.labels.default.callout = Some(60.0); + l.labels.default.by.angle = Some("30".into()); + let camera = Camera { center: lonlat_to_merc(Coord::new(0.0, 0.0)), res: 500.0, bearing: 0.0, pitch: 0.0 }; + let view = View { width: 800.0, height: 600.0, pixels_per_point: 1.0, camera, sky: false }; + let labels = render(&mut m, &view).unwrap().labels; + assert_eq!(labels.len(), 2, "both drawn"); + assert!(labels.iter().all(|l| l.angle.is_some()), "turned"); + assert!(labels.iter().any(|l| l.collide.callout.is_some()), "one moved off by a callout"); + } + /// Two points close together: the second label moves off by a callout. Locked, it keeps that /// place and its line, as the map moves too (it went back onto the first, without its line). #[test] From 354b06bb514375c851c451637bbdf43c0acecec3 Mon Sep 17 00:00:00 2001 From: giswqs Date: Wed, 4 Nov 2026 21:49:53 -0500 Subject: [PATCH 13/15] Address Claude review feedback - Callouts: a label moved by hand keeps its callout reach whatever its placement, so a moved line label still draws its line back to its feature, as the docs say (the placement gate in edb48e66 had dropped it; caught by a_label_moved_into_view_is_drawn_with_its_feature_out_of_it). --- crates/render/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/render/src/lib.rs b/crates/render/src/lib.rs index 8d3b0bb48..189996944 100644 --- a/crates/render/src/lib.rs +++ b/crates/render/src/lib.rs @@ -4237,8 +4237,8 @@ fn render_frame( text_allow_overlap: class.show_all, // Callouts: a label set by its feature (not along a line, // rotated or not) may move off a crowded spot, a line drawn - // back to it. - callout_reach: if !along_line(class.placement, g) && glyphs.is_empty() { + // back to it; one moved by hand, a line's too, keeps its line. + callout_reach: if moved.is_some() || (!along_line(class.placement, g) && glyphs.is_empty()) { class.callout.unwrap_or(0.0) } else { 0.0 From f776c4409d6d4b011bb87369e69fc8d3025e9bc9 Mon Sep 17 00:00:00 2001 From: giswqs Date: Wed, 4 Nov 2026 22:06:45 -0500 Subject: [PATCH 14/15] Address Claude review feedback - labels.move: a label moved where the flat map's projection can't draw it is kept (the projection may change) but says so through a notice, rather than silently not being drawn; test both ways. --- crates/engine/src/cmd/labels.rs | 14 ++++++++++++++ crates/engine/src/tests.rs | 17 +++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/crates/engine/src/cmd/labels.rs b/crates/engine/src/cmd/labels.rs index 5ecaca197..1beaebaba 100644 --- a/crates/engine/src/cmd/labels.rs +++ b/crates/engine/src/cmd/labels.rs @@ -274,6 +274,20 @@ pub fn commands() -> Vec { if pruned > 0 { s.notify(format!("{pruned} saved label move(s) of features no longer in the layer dropped to make room"), false); } + // Kept, as the map's projection may change, but said: where the flat map's projection + // can't draw it, the label isn't drawn. + if let Some([x, y]) = to + && !s.map().projection.is_globe() + && s.map().to_map(Coord::new(x, y)).is_none() + { + s.notify( + format!( + "the label moved to ({x}, {y}) is outside the area the map's projection ({}) draws, so it isn't drawn in it", + s.map().projection.label() + ), + false, + ); + } // Locked, the labels are locked again where they are placed next (`with_labels`: the // moves changed), as the one moved may not have been placed when the lock was taken. Ok(r) diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 6edc0b7cb..606d28eea 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -3352,6 +3352,23 @@ fn moved_labels_of_removed_features_make_room_with_a_notice() { assert!(!s.notices_since(mark).iter().any(|n| n.text.contains("dropped to make room"))); } +/// A label moved where the map's projection can't draw it is kept, and said not to be drawn; +/// one moved inside it gets no notice. +#[test] +fn a_label_moved_outside_the_projection_says_so() { + let mut s = world(); + let id = s.map().layer_by_name("Cities").unwrap().id; + let fid = s.map().layer(id).unwrap().dataset.features.first().unwrap().id; + s.run("map.projection", &json!({"type": "usaAlbers"})).unwrap(); + let mark = s.notice_mark(); + s.run("labels.move", &json!({"layer": id, "feature": fid, "to": [-100.0, 40.0]})).unwrap(); + assert!(!s.notices_since(mark).iter().any(|n| n.text.contains("isn't drawn")), "inside: no notice"); + let mark = s.notice_mark(); + s.run("labels.move", &json!({"layer": id, "feature": fid, "to": [12.5, 41.9]})).unwrap(); + assert!(s.notices_since(mark).iter().any(|n| n.text.contains("outside the area the map's projection"))); + assert_eq!(s.map().layer(id).unwrap().labels.default.moved.len(), 1, "kept"); +} + /// Labels are moved by hand by feature id and put back, one class's or all; bad input is refused. #[test] fn labels_are_moved_by_command_and_reset() { From 14a357941bde9cc5e07e8683b0f43ba380765963 Mon Sep 17 00:00:00 2001 From: giswqs Date: Wed, 4 Nov 2026 22:22:03 -0500 Subject: [PATCH 15/15] Address Claude review feedback - Callouts: the docs and labels.placement's parameter text say labels with an icon aren't moved by a callout, as well as labels along lines. --- crates/engine/src/cmd/labels.rs | 2 +- docs/user-guide/cartography.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/cmd/labels.rs b/crates/engine/src/cmd/labels.rs index 1beaebaba..e62d4801c 100644 --- a/crates/engine/src/cmd/labels.rs +++ b/crates/engine/src/cmd/labels.rs @@ -229,7 +229,7 @@ pub fn commands() -> Vec { }) .when(need_feature_layer) .params( - r#"{"layer"?, "class"?: name (none: the Default class, by whatever name it has), "position"?: "auto"|"center"|"above"|"below"|"left"|"right"|"aboveRight"|"aboveLeft"|"belowRight"|"belowLeft"|"parallel" (along a line, straight)|"curved" (along a line, following its bends) (required without offset, repeat, otherPositions, showAll or callout), "offset"?: [right, down] points (-200 to 200 each) the label moves from where its position puts it ([0, 0]: none; a label along a line isn't moved), "repeat"?: points (10 to 10000) between a line's labels, spaced out from the one at its middle, at most 32 a line (0 or null: the one at its middle; parallel and curved only), "otherPositions"?: bool (a label whose place is taken tries others: a point's round the point, corners first; a line's along the line, nearest its middle first, placed parallel or curved without a repeat; true is refused for any other line label and on polygons), "showAll"?: bool (every label drawn, over the others where they crowd), "callout"?: true|false|points (5 to 500: a label crowded off its spot moves to a free one nearby, a line drawn back to its feature at most this long; true keeps its length, or 40; false, 0 or null: none; not labels along a line)}"#, + r#"{"layer"?, "class"?: name (none: the Default class, by whatever name it has), "position"?: "auto"|"center"|"above"|"below"|"left"|"right"|"aboveRight"|"aboveLeft"|"belowRight"|"belowLeft"|"parallel" (along a line, straight)|"curved" (along a line, following its bends) (required without offset, repeat, otherPositions, showAll or callout), "offset"?: [right, down] points (-200 to 200 each) the label moves from where its position puts it ([0, 0]: none; a label along a line isn't moved), "repeat"?: points (10 to 10000) between a line's labels, spaced out from the one at its middle, at most 32 a line (0 or null: the one at its middle; parallel and curved only), "otherPositions"?: bool (a label whose place is taken tries others: a point's round the point, corners first; a line's along the line, nearest its middle first, placed parallel or curved without a repeat; true is refused for any other line label and on polygons), "showAll"?: bool (every label drawn, over the others where they crowd), "callout"?: true|false|points (5 to 500: a label crowded off its spot moves to a free one nearby, a line drawn back to its feature at most this long; true keeps its length, or 40; false, 0 or null: none; not labels along a line or with an icon)}"#, ), // Labels moved by hand (the Move Label tool, as QGIS's): kept by their feature's id. C::new("labels.move", "Move Label", "Labeling › Label Placement", |s, p| { diff --git a/docs/user-guide/cartography.md b/docs/user-guide/cartography.md index a35b3142d..71fbd1fa6 100644 --- a/docs/user-guide/cartography.md +++ b/docs/user-guide/cartography.md @@ -488,7 +488,7 @@ Turn labels on per layer (Labeling › Labels › **Label**), then: spot nearby, and a line is drawn from its feature to the text, as QGIS's callouts. **at most** sets how long the line may be (5 to 500 points, 40 to start with): a label with no free spot that close is left out, or drawn over the others with Show all labels. The line takes the - label's colour, over its halo. Labels along lines aren't moved + label's colour, over its halo. Labels along lines and labels with an icon aren't moved (`labels.placement={"layer": "Cities", "callout": 60}`; `true` keeps the length, `false` turns them off). - **Move Label** (Labeling tab › Label Placement): drag a label to where it should be, as with