DXF: SAT text is groups 1 and 3 of the modeler subclass; only ASM_Data records keep geometry (#2334)

sat_text_acis now looks for groups 1 and 3 inside AcDbModelerGeometry only (all
groups when a file has no subclass markers), and the data store counts an
entity as stored only when its ACDSRECORD holds ASM_Data. The writer and the
reader share one record splitter (data_store_records).

crates/doc/src/foreign.rs

@@ -89,9 +89,17 @@ impl Foreign {
             && self.data_store.is_empty()
     }
 
-    /// The entities the data store has records for (their 320 handles).
+    /// The entities with a geometry record in the data store: an `ACDSRECORD` naming the entity
+    /// (320) that holds its `ASM_Data`. A 320 without one doesn't keep an entity's geometry.
     pub fn data_store_entities(&self) -> std::collections::BTreeSet<Handle> {
-        self.data_store.iter().filter(|t| t.code == 320).filter_map(|t| Handle::parse_hex(t.value.trim())).collect()
+        let mut out = std::collections::BTreeSet::new();
+        for rec in data_store_records(&self.data_store) {
+            let is_record = rec.first().is_some_and(|t| t.value.trim().eq_ignore_ascii_case("ACDSRECORD"));
+            if is_record && rec.iter().any(|t| t.code == 2 && t.value.trim().eq_ignore_ascii_case("ASM_Data")) {
+                out.extend(rec.iter().filter(|t| t.code == 320).filter_map(|t| Handle::parse_hex(t.value.trim())));
+            }
+        }
+        out
     }
 }
 
@@ -99,9 +107,7 @@ impl Foreign {
 /// (groups 1 and 3), which DXF 2013 and later no longer has: those files keep it in the data
 /// store section, so an entity with SAT text and no record there can't be written to one (#2334).
 pub fn sat_text_acis(u: &crate::Unknown, handle: Handle, stored: &std::collections::BTreeSet<Handle>) -> bool {
-    ["3DSOLID", "REGION", "BODY"].iter().any(|t| u.dxf_type.eq_ignore_ascii_case(t))
-        && !stored.contains(&handle)
-        && u.tags.iter().take_while(|r| r.code < 1000).any(|r| r.code == 1 || r.code == 3)
+    ["3DSOLID", "REGION", "BODY"].iter().any(|t| u.dxf_type.eq_ignore_ascii_case(t)) && !stored.contains(&handle) && has_sat_text(&u.tags)
 }
 
 /// The entities a DXF of 2013 or later leaves out because they hold SAT-text ACIS data
@@ -128,3 +134,33 @@ pub fn sat_text_dropped(d: &crate::Drawing) -> BTreeMap<String, usize> {
 pub fn xref_dependent(name: &str) -> bool {
     name.contains('|')
 }
+
+/// The records of a data store section's groups: each starts at a `0` group and runs to the next
+/// (the section's own groups before the first record are not part of one).
+pub fn data_store_records(tags: &[RawTag]) -> Vec<&[RawTag]> {
+    let mut out = Vec::new();
+    let mut start = tags.iter().position(|t| t.code == 0).unwrap_or(tags.len());
+    while start < tags.len() {
+        let end = tags.iter().skip(start + 1).position(|t| t.code == 0).map_or(tags.len(), |k| start + 1 + k);
+        out.push(tags.get(start..end).unwrap_or(&[]));
+        start = end;
+    }
+    out
+}
+
+/// Whether an entity's groups hold SAT text: groups 1 and 3 inside its `AcDbModelerGeometry`
+/// subclass. A file of a version before the subclass markers has no `100` groups: all of its
+/// groups (before the extended data) are the entity's.
+fn has_sat_text(tags: &[RawTag]) -> bool {
+    let own = tags.iter().take_while(|r| r.code < 1000);
+    let marked = tags.iter().take_while(|r| r.code < 1000).any(|r| r.code == 100);
+    let mut in_modeler = !marked;
+    for r in own {
+        if r.code == 100 {
+            in_modeler = r.value.trim().eq_ignore_ascii_case("AcDbModelerGeometry");
+        } else if in_modeler && (r.code == 1 || r.code == 3) {
+            return true;
+        }
+    }
+    false
+}

crates/io/src/dxf_raw.rs

@@ -601,17 +601,13 @@ pub(crate) fn data_store(tags: &mut Vec<Tag>, d: &Drawing, modern: bool) {
     let first = ds.iter().position(|t| t.code == 0).unwrap_or(ds.len());
     out.extend(ds.iter().take(first).map(tag));
     let mut records = 0usize;
-    let mut i = first;
-    while i < ds.len() {
-        let end = ds.iter().skip(i + 1).position(|t| t.code == 0).map_or(ds.len(), |k| i + 1 + k);
-        let rec = ds.get(i..end).unwrap_or(&[]);
+    for rec in cadcraft_doc::foreign::data_store_records(ds) {
         let is_record = rec.first().is_some_and(|t| t.value.trim().eq_ignore_ascii_case("ACDSRECORD"));
         let owner = rec.iter().find(|t| t.code == 320).map(|t| norm(&t.value));
         if !is_record || owner.as_ref().is_some_and(|h| written.contains(h)) {
             records += usize::from(is_record);
             out.extend(rec.iter().map(tag));
         }
-        i = end;
     }
     if records == 0 {
         return;

crates/io/tests/it/acis_data_store.rs

@@ -156,3 +156,32 @@ fn sat_text_solids_stay_in_dwg_2018_and_xref_contents_are_not_counted() {
     x.blocks.insert("REF|PART".into(), std::sync::Arc::new(dep));
     assert_eq!(cadcraft_doc::foreign::sat_text_dropped(&x), [("REGION".to_string(), 1)].into(), "only the model-space region");
 }
+
+/// Only groups 1 and 3 of the modeler geometry subclass are SAT text, and only a record holding
+/// the entity's ASM_Data keeps its geometry in the data store (#2334).
+#[test]
+fn sat_text_is_told_by_its_subclass_and_a_geometry_record() {
+    use cadcraft_doc::foreign::{Foreign, sat_text_acis};
+    use cadcraft_doc::{Handle, RawTag, Unknown};
+    let tag = |code: i32, value: &str| RawTag { code, value: value.into() };
+    let solid = |tags: Vec<RawTag>| Unknown { dxf_type: "3DSOLID".into(), tags };
+    let none = std::collections::BTreeSet::new();
+    let h = Handle(0x2B);
+    // A stray 1 or 3 outside AcDbModelerGeometry is not SAT text.
+    let stray =
+        solid(vec![tag(100, "AcDbEntity"), tag(1, "name"), tag(100, "AcDbModelerGeometry"), tag(70, "1"), tag(100, "AcDb3dSolid"), tag(3, "x")]);
+    assert!(!sat_text_acis(&stray, h, &none));
+    let real = solid(vec![tag(100, "AcDbEntity"), tag(100, "AcDbModelerGeometry"), tag(70, "1"), tag(1, "400 1 1 0"), tag(100, "AcDb3dSolid")]);
+    assert!(sat_text_acis(&real, h, &none));
+    // Without subclass markers (an old file) the groups are the entity's own.
+    assert!(sat_text_acis(&solid(vec![tag(8, "0"), tag(1, "400 1 1 0")]), h, &none));
+
+    // A 320 in the data store without an ASM_Data record doesn't keep the geometry.
+    let store = |asm: bool| {
+        let mut t = vec![tag(0, "ACDSRECORD"), tag(90, "1"), tag(320, "2B")];
+        t.push(tag(2, if asm { "ASM_Data" } else { "Other" }));
+        Foreign { data_store: t, ..Foreign::default() }
+    };
+    assert!(store(true).data_store_entities().contains(&h));
+    assert!(store(false).data_store_entities().is_empty());
+}