collective
Browse / github.com/storytold/cadcraft / patch
fix(engine): refresh attribute baselines after value changes
fix(engine): refresh attribute baselines after value changes Changing a justified attribute's value can leave its saved baseline at the old string's position when the width change falls within the DXF writer's deliberate font-substitution tolerance. The alignment anchor renders correctly in CADCraft, but the exported baseline can remain stale. Refresh the derived XY baseline when ATTEDIT or -ATTEDIT actually changes the value. Preserve the alignment anchor, both Z values, no-op imported font placement, and the independent points used by Left/Baseline, Aligned and Fit text. The existing writer then recomputes the baseline; its tolerance and file synta
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- smacdmac101 (imported from pull request #2214, open)
- Base
d864a08b013353143086bd8bb568c26cb6256796- Size
- +233 −9 in 7 files
- Review
- pending
- Shared
- 2026-10-28
- Id
cc2dc5a0df1fe0b8e4bd52c3
How it went
No other operator has reported using it yet.
Diff
raw · crates/engine/src/cmd/blocks/attedit.rs crates/engine/tests/it/attribute_value_baseline.rs crates/engine/tests/it/main.rs crates/io/src/dxf_write/common.rs docs/file-format-parity.md xtask/src/parity.rs xtask/src/stats.rs
@@ -3,7 +3,7 @@ //! time or by replacing a string in all of them. Attribute tags match in any case, as in AutoCAD. use cadcraft_doc::color::Color;-use cadcraft_doc::{AttribProps, Common, EntityKind, Handle};+use cadcraft_doc::{AttribProps, Common, EntityKind, HAlign, Handle, Text, VAlign}; use serde_json::{Map, Value, json}; use crate::cmd::curves::KW;@@ -67,7 +67,7 @@ fn run(s: &mut Session, p: &Value) -> Result<Value> { if let EntityKind::Insert(i) = &mut e.kind { for a in &mut i.attribs { if let Some(v) = value_for(&vals, &a.tag) {- a.text.value = v;+ set_value(&mut a.text, v); n += 1; } }@@ -130,13 +130,29 @@ fn edit(s: &mut Session, (h, k): (Handle, usize), f: impl FnOnce(&str) -> Option && let Some(a) = ins.attribs.get_mut(k) && let Some(v) = f(&a.text.value).filter(|v| *v != a.text.value) {- a.text.value = v;+ set_value(&mut a.text, v); changed = true; } })?; Ok(changed) } +/// Refresh the derived baseline after a value change, even one within the writer's+/// imported-font tolerance. Equal XY points request recomputation on export; retain+/// both elevations and the independent endpoints used by Aligned/Fit text.+fn set_value(text: &mut Text, value: String) {+ if text.value == value {+ return;+ }+ text.value = value;+ if !matches!((text.halign, text.valign), (HAlign::Left, VAlign::Baseline) | (HAlign::Aligned | HAlign::Fit, _))+ && let Some(anchor) = text.align_pt+ {+ text.insert.x = anchor.x;+ text.insert.y = anchor.y;+ }+}+ /// Change the properties set on attribute `k` of block reference `h`. fn set_props(s: &mut Session, (h, k): (Handle, usize), f: impl FnOnce(&mut AttribProps)) -> Result<()> { s.doc_mut()?.modify_entity(h, |e| {new file mode 100644@@ -0,0 +1,203 @@+//! Small value changes must not export an imported justified attribute's old baseline.+use cadcraft_doc::{Attrib, Drawing, EntityKind, HAlign, Handle, Text, VAlign};+use cadcraft_engine::{Input, Session};+use cadcraft_geom::Vec3;+use serde_json::json;++const BEFORE: &str = "POWER ADAPTER";+const AFTER: &str = "POWER ADAPTER.";++fn fixture() -> (Session, Handle, Handle) {+ let mut s = Session::new();+ for (tag, value, y) in [("T", BEFORE, 15), ("U", "ADPT-01", 0)] {+ s.execute("attdef", &json!({"tag":tag,"default":value,"at":[0,y],"height":3})).unwrap();+ }+ s.execute("line", &json!({"points":[[0,-5],[40,-5]]})).unwrap();+ let handles: Vec<_> = s.doc().unwrap().model.iter().map(|e| e.handle.hex()).collect();+ s.execute("block", &json!({"name":"Equipment","handles":handles,"base":[0,0],"keep":"delete"})).unwrap();+ let a = s.execute("insert", &json!({"name":"Equipment","at":[100,200],"scale":2,"rotation":30})).unwrap();+ let b = s.execute("insert", &json!({"name":"Equipment","at":[300,200],"attribs":{"T":"Other"}})).unwrap();+ let a = Handle::parse_hex(a["handle"].as_str().unwrap()).unwrap();+ let b = Handle::parse_hex(b["handle"].as_str().unwrap()).unwrap();+ // Set existing document fields directly: this regression does not require Text Options.+ s.doc_mut()+ .unwrap()+ .modify_entity(a, |e| {+ let EntityKind::Insert(i) = &mut e.kind else { panic!("insert") };+ let t = &mut i.attribs[0].text;+ t.insert.z = 7.0;+ t.align_pt = Some(t.insert);+ t.halign = HAlign::Center;+ t.valign = VAlign::Top;+ })+ .unwrap();+ let imported = cadcraft_io::read_dxf(cadcraft_io::write_dxf(s.doc().unwrap()).as_bytes()).unwrap();+ let mut s = Session::empty();+ s.open_drawing(imported, "Imported", None);+ (s, a, b)+}++fn attributes(d: &Drawing, h: Handle) -> Vec<Attrib> {+ let EntityKind::Insert(i) = &d.entity(h).unwrap().kind else { panic!("insert") };+ i.attribs.clone()+}++fn baseline(d: &Drawing, t: &Text) -> cadcraft_geom::Vec2 {+ let style = d.text_style(&t.style).unwrap();+ let (h, v) = cadcraft_render::text_align(t.halign, t.valign);+ cadcraft_fonts::baseline_start(+ &cadcraft_fonts::TextFont::resolve(&style.font),+ &t.value,+ &cadcraft_fonts::TextParams {+ insert: t.insert.xy(),+ align_pt: t.align_pt.map(|p| p.xy()),+ height: t.height,+ rotation: t.rotation,+ width_factor: t.width_factor,+ oblique: t.oblique,+ h,+ v,+ backwards: style.backwards,+ upside_down: style.upside_down,+ },+ )+}++#[test]+fn all_attribute_value_paths_refresh_small_justified_export_changes() {+ for path in ["json", "interactive", "global-json", "global-interactive"] {+ let (mut s, a, b) = fixture();+ let before = attributes(s.doc().unwrap(), a);+ let other = attributes(s.doc().unwrap(), b);+ let definition = s.doc().unwrap().block("Equipment").unwrap().clone();+ match path {+ "json" => {+ s.execute("attedit", &json!({"handle":a.hex(),"values":{"t":AFTER}})).unwrap();+ }+ "interactive" => {+ s.start("attedit").unwrap();+ s.input(Input::Pick(vec![a])).unwrap();+ s.input(Input::Text(AFTER.into())).unwrap();+ s.input(Input::Enter).unwrap();+ assert!(s.running.is_none());+ }+ "global-json" => {+ s.execute("-attedit", &json!({"handles":[a.hex()],"tag":"T","find":"ADAPTER","replace":"ADAPTER."})).unwrap();+ }+ "global-interactive" => {+ s.script("-attedit\nn\nn\nEquipment\nT\n\nADAPTER\nADAPTER.\n").unwrap();+ assert!(s.running.is_none(), "{}", s.prompt_text());+ }+ _ => panic!("path"),+ }+ let edited = attributes(s.doc().unwrap(), a);+ assert_eq!(edited[0].text.value, AFTER, "{path}");+ assert_eq!(edited[0].text.align_pt, before[0].text.align_pt);+ assert_eq!(edited[0].text.insert.z, 7.0);+ assert_eq!(edited[1], before[1]);+ assert_eq!(attributes(s.doc().unwrap(), b), other);+ assert_eq!(s.doc().unwrap().block("Equipment").unwrap(), &definition);+ let expected = baseline(s.doc().unwrap(), &edited[0].text);+ assert!(expected.dist(before[0].text.insert.xy()) > 0.01, "fixture must change width");+ for name in ["edited.dxf", "edited.dwg"] {+ let reopened = cadcraft_io::read(&cadcraft_io::write(s.doc().unwrap(), name).unwrap(), name).unwrap();+ let restored = attributes(&reopened, a);+ assert!(+ restored[0].text.insert.xy().dist(expected) < 1e-8,+ "{path} {name}: stale {:?}, expected {expected:?}",+ restored[0].text.insert.xy()+ );+ assert_eq!(restored[0].text.align_pt, before[0].text.align_pt);+ assert_eq!(restored[0].text.value, AFTER);+ assert_eq!(restored[0].text.insert.z, 7.0);+ }+ s.undo().unwrap();+ assert_eq!(attributes(s.doc().unwrap(), a), before, "{path} undo");+ s.redo().unwrap();+ assert_eq!(attributes(s.doc().unwrap(), a), edited, "{path} redo");+ }+}++#[test]+fn equal_attribute_values_preserve_an_imported_font_baseline() {+ let (mut s, a, _) = fixture();+ // A small imported-font offset must survive a no-op edit (writer tolerance is deliberate).+ s.doc_mut()+ .unwrap()+ .modify_entity(a, |e| {+ let EntityKind::Insert(i) = &mut e.kind else { panic!("insert") };+ i.attribs[0].text.insert.x += 0.1;+ })+ .unwrap();+ let before = attributes(s.doc().unwrap(), a);+ s.execute("attedit", &json!({"handle":a.hex(),"values":{"T":BEFORE}})).unwrap();+ assert_eq!(attributes(s.doc().unwrap(), a), before);+ s.start("attedit").unwrap();+ s.input(Input::Pick(vec![a])).unwrap();+ s.input(Input::Enter).unwrap();+ s.input(Input::Enter).unwrap();+ assert_eq!(attributes(s.doc().unwrap(), a), before);+ s.execute("-attedit", &json!({"handles":[a.hex()],"tag":"T","find":"ADAPTER","replace":"ADAPTER"})).unwrap();+ assert_eq!(attributes(s.doc().unwrap(), a), before, "global JSON no-op");+ s.script("-attedit\nn\nn\nEquipment\nT\n\nADAPTER\nADAPTER\n").unwrap();+ assert!(s.running.is_none(), "{}", s.prompt_text());+ assert_eq!(attributes(s.doc().unwrap(), a), before, "global interactive no-op");+}++/// Recomputing a justified baseline must not replace its independent insertion elevation.+#[test]+fn value_edits_preserve_distinct_insertion_and_alignment_elevations_on_export() {+ let (mut s, a, _) = fixture();+ s.doc_mut()+ .unwrap()+ .modify_entity(a, |e| {+ let EntityKind::Insert(i) = &mut e.kind else { panic!("insert") };+ i.attribs[0].text.insert.z = 9.0;+ })+ .unwrap();+ let anchor = attributes(s.doc().unwrap(), a)[0].text.align_pt;+ s.execute("attedit", &json!({"handle":a.hex(),"values":{"T":AFTER}})).unwrap();+ for name in ["elevations.dxf", "elevations.dwg"] {+ let reopened = cadcraft_io::read(&cadcraft_io::write(s.doc().unwrap(), name).unwrap(), name).unwrap();+ let restored = attributes(&reopened, a);+ assert_eq!(restored[0].text.insert.z, 9.0, "{name}: insertion elevation");+ assert_eq!(restored[0].text.align_pt.map(|p| p.xy()), anchor.map(|p| p.xy()), "{name}: alignment XY");+ if name.ends_with(".dxf") {+ assert_eq!(restored[0].text.align_pt, anchor, "DXF retains independent alignment elevation");+ } else {+ // The DWG bridge uses one text elevation for both points.+ assert_eq!(restored[0].text.align_pt.unwrap().z, 9.0);+ }+ }+}++#[test]+fn value_edits_preserve_left_baseline_and_two_point_placements() {+ for horizontal in [HAlign::Left, HAlign::Aligned, HAlign::Fit] {+ let (mut s, a, _) = fixture();+ s.doc_mut()+ .unwrap()+ .modify_entity(a, |e| {+ let EntityKind::Insert(i) = &mut e.kind else { panic!("insert") };+ let t = &mut i.attribs[0].text;+ t.halign = horizontal;+ t.valign = VAlign::Baseline;+ t.insert = Vec3::new(10.0, 20.0, 7.0);+ t.align_pt = (horizontal != HAlign::Left).then_some(Vec3::new(40.0, 30.0, 9.0));+ })+ .unwrap();+ let before = attributes(s.doc().unwrap(), a);+ s.execute("attedit", &json!({"handle":a.hex(),"values":{"T":AFTER}})).unwrap();+ let mut expected = before;+ expected[0].text.value = AFTER.into();+ assert_eq!(attributes(s.doc().unwrap(), a), expected);+ if horizontal != HAlign::Left {+ let reopened = cadcraft_io::read_dxf(cadcraft_io::write_dxf(s.doc().unwrap()).as_bytes()).unwrap();+ let restored = attributes(&reopened, a);+ assert_eq!(restored[0].text.insert, expected[0].text.insert);+ assert_eq!(restored[0].text.align_pt, expected[0].text.align_pt);+ assert_eq!(restored[0].text.halign, horizontal);+ assert_eq!(restored[0].text.value, AFTER);+ }+ }+}@@ -10,6 +10,7 @@ mod attedit_prompts; mod attrib_modes; mod attrib_props; mod attribute_tools;+mod attribute_value_baseline; mod block_keep; mod block_redefine; mod blockreplace;@@ -234,7 +234,8 @@ fn computed_text_insert(d: &Drawing, t: &Text) -> Vec3 { upside_down: ts.upside_down, }; let q = cadcraft_fonts::baseline_start(&cadcraft_fonts::TextFont::resolve(&ts.font), &t.value, &p);- if q.is_finite() { Vec3::new(q.x, q.y, a.z) } else { t.insert }+ // Justification recalculates the baseline in XY; group 10 keeps its own elevation.+ if q.is_finite() { Vec3::new(q.x, q.y, t.insert.z) } else { t.insert } } pub(super) fn text_tags(w: &mut W, d: &Drawing, t: &Text, attrib: Option<(&Attrib, Version)>, attdef_prompt: Option<&str>) {@@ -1,6 +1,6 @@ # File-format parity -> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-22 · **Change:** minor (DIMASSOC objects for angular, diametric and radial dimensions) · **Target:** Autodesk AutoCAD 2027+> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-26 · **Change:** minor (attribute value edits refresh exported baselines) · **Target:** Autodesk AutoCAD 2027 Every format AutoCAD 2027 reads or writes, with CADCraft's support, fidelity and tests. Work items are in [gaps.md](gaps.md#file-formats); the weighted score feeds@@ -75,7 +75,7 @@ on read with a note, so such files no longer exceed the DXF limit (#1482). **DWG |---|---|---|---| | LINE, POINT, CIRCLE, ARC, ELLIPSE, LWPOLYLINE (widths, bulges, elevation), SPLINE, RAY, XLINE | yes | yes | round-trip tests | | POLYLINE 2D/3D | yes | yes (as LWPOLYLINE / 3D POLYLINE) | polyface and polygon meshes flattened: lossy |-| TEXT, MTEXT, ATTDEF, INSERT + ATTRIB | yes; multiline attributes (embedded MTEXT) read and drawn | yes; multiline attributes written as embedded MTEXT (DXF 2007 and later; older versions: one line) | ATTDEF prompt kept (#137) |+| TEXT, MTEXT, ATTDEF, INSERT + ATTRIB | yes; multiline attributes (embedded MTEXT) read and drawn | yes; multiline attributes written as embedded MTEXT (DXF 2007 and later; older versions: one line); changed attribute values refresh justified baselines, including small width changes | ATTDEF prompt kept (#137); unchanged values retain imported-font placement | | DIMENSION (all 7 kinds) | yes | yes, with anonymous `*D` blocks | a reopened dimension's `*D` block is regenerated under its name, unreferenced ones dropped (#495); DIMASSOC objects written for every associated kind — linear/aligned, angular, radial/diametric, ordinate and arc length — and read back without CADCraft's xdata (#1651) | | DIMENSION (all 7 kinds) | yes | yes, with anonymous `*D` blocks | a reopened dimension's `*D` block is regenerated under its name, unreferenced ones dropped (#495); DIMASSOC objects written; angular block text honors DIMDSEP, including per-dimension overrides (#1646) | | DIMENSION (all 7 kinds) | yes | yes, with anonymous `*D` blocks; their text keeps the diameter sign as `%%c`, so SHX styles plot it (#1645) | a reopened dimension's `*D` block is regenerated under its name, unreferenced ones dropped (#495); DIMASSOC objects written |@@ -140,6 +140,7 @@ our output in AutoCAD black-box. | Date | Change | Summary | |---|---|---|+| 2026-10-26 | minor | CC-046: ATTEDIT and -ATTEDIT value changes invalidate derived justified baselines before DXF/DWG export; preserve alignment anchors, unchanged values and Aligned/Fit endpoints. Review follow-up: baseline recomputation preserves insertion elevation independently of alignment elevation. The shared writer correction also applies to justified TEXT. Independent of attribute editor UI; reverting the follow-up restores elevation loss on recomputation, while reverting the value setter restores stale XY baselines. | | 2026-10-22 | minor | DXF/DWG save: angular, diametric and radial dimensions get standard DIMASSOC objects (one centre reference to the circle for radial/diametric, the definition points for angular), so AutoCAD follows their geometry; the standard objects of those kinds are read back too, without CADCraft's xdata (#1651) | | 2026-10-19 | minor | DXF/DWG save: angular dimension block text honors DIMDSEP, including per-dimension overrides (#1646) | | 2026-10-19 | minor | A file's own arrowhead blocks with standard names (AutoCAD's `_Dot`, `_DotSmall`), named only by a dimension style, are kept and saved as they were instead of being replaced by CADCraft's solids; PURGE keeps arrowhead blocks dimension styles name |@@ -136,7 +136,9 @@ mod tests { assert_eq!(day(20_736), "2026-10-10"); assert_eq!(date(std::time::UNIX_EPOCH - std::time::Duration::from_secs(86_400)), "1970-01-01"); // Past chrono's range keeps the proleptic Gregorian date, not the epoch (old `civil_from_days`).- assert_eq!(day(115_740_740), "318857-05-20");+ // Checked on the day count: Windows' SystemTime ends in the year 30828, so building that+ // SystemTime panics there.+ assert_eq!(civil_from_days(115_740_740), (318_857, 5, 20)); // Windows' SystemTime can't hold u64::MAX / 2 seconds, so it is only checked where it exists. if let Some(far) = std::time::UNIX_EPOCH.checked_add(std::time::Duration::from_secs(u64::MAX / 2)) { assert_eq!(date(far), "292277026596-12-04");@@ -134,7 +134,7 @@ pub fn run(_root: &Path, exact: bool) -> Result<(), String> { #[cfg(test)] mod tests {- use super::{Counts, count_tests, walk};+ use super::{Counts, count_tests}; #[test] fn counts_attributes_and_proptest_blocks() {@@ -155,7 +155,7 @@ mod tests { // A directory symlink pointing back at `real` would be re-scanned if followed. symlink(&real, base.join("alias")).unwrap(); let mut c = Counts::default();- walk(&base, &mut c).unwrap();+ super::walk(&base, &mut c).unwrap(); assert_eq!(c.files, 1, "the aliased directory was followed and counted twice"); std::fs::remove_dir_all(&base).ok(); }