From d2052eb4c7bde7a66936e366e40141d02a2ef68b Mon Sep 17 00:00:00 2001 From: mkzung <103102868+mkzung@users.noreply.github.com> Date: Mon, 2 Nov 2026 21:17:55 +0000 Subject: [PATCH] DWG: isolate proxy conversion failures and normalize proxy names --- crates/dwg/src/lib.rs | 7 +- crates/dwg/src/proxy_transport.rs | 170 ++++++++++++++++++++-------- crates/io/src/dxf_write.rs | 14 ++- crates/io/tests/registered_proxy.rs | 45 ++++++++ 4 files changed, 181 insertions(+), 55 deletions(-) diff --git a/crates/dwg/src/lib.rs b/crates/dwg/src/lib.rs index 55aaa464f..d3112c449 100644 --- a/crates/dwg/src/lib.rs +++ b/crates/dwg/src/lib.rs @@ -109,7 +109,9 @@ mod native { } let mut notes = read_notes(&outcome.stats); let mut doc = outcome.document; - guarded(AssertUnwindSafe(|| super::proxy_transport::prepare(&mut doc))).map_err(|_| "the DWG proxy conversion failed".to_string())??; + notes.extend( + guarded(AssertUnwindSafe(|| super::proxy_transport::prepare(&mut doc))).map_err(|_| "the DWG proxy conversion failed".to_string())?, + ); let orphans = guarded(AssertUnwindSafe(|| post_process(&mut doc))).map_err(|_| "the DWG post-processing failed on this file".to_string())?; if orphans > 0 { let (s, were) = if orphans == 1 { ("", "was") } else { ("s", "were") }; @@ -737,7 +739,6 @@ mod native { mb(Limits::DEFAULT.max_dxf_bytes as u64) )); } - let classes = super::proxy_transport::read_classes(dxf)?; let data = dxf.to_vec(); let mut doc = std::panic::catch_unwind(move || acadrust::DxfReader::from_reader(Cursor::new(data)).and_then(|r| r.read())) .map_err(|_| "the DXF→DWG conversion failed".to_string())? @@ -750,7 +751,7 @@ mod native { None if doc.version < MIN_DWG_VERSION => doc.version = MIN_DWG_VERSION, None => {} } - guarded(AssertUnwindSafe(|| super::proxy_transport::restore_classes(&mut doc, classes))) + guarded(AssertUnwindSafe(|| super::proxy_transport::restore_classes(&mut doc, || super::proxy_transport::read_classes(dxf)))) .map_err(|_| "the DXF proxy class conversion failed".to_string())??; // The fix-ups run on the third-party reader's document, so a panic in one is an error, not a crash. let unwritten = guarded(AssertUnwindSafe(|| { diff --git a/crates/dwg/src/proxy_transport.rs b/crates/dwg/src/proxy_transport.rs index 8139f5c0d..cbbc6a210 100644 --- a/crates/dwg/src/proxy_transport.rs +++ b/crates/dwg/src/proxy_transport.rs @@ -12,16 +12,16 @@ fn class_number(index: usize) -> Result { index.checked_add(500).and_then(|n| i16::try_from(n).ok()).ok_or_else(|| "DXF: too many application classes".into()) } -/// Replace the dependency's private registered-class envelope with a standard proxy. -pub(super) fn prepare(doc: &mut CadDocument) -> Result<(), String> { +/// Convert representable application records and report only the records left out. +pub(super) fn prepare(doc: &mut CadDocument) -> Vec { let classes: Vec<_> = doc.classes.iter().cloned().collect(); let mut numbers = HashMap::new(); let mut identities = HashMap::new(); for (index, class) in classes.iter().enumerate() { - let number = class_number(index)?; - numbers.entry(i32::from(class.class_number)).and_modify(|v| *v = None).or_insert(Some(i32::from(number))); + let number = class_number(index).ok(); + numbers.entry(i32::from(class.class_number)).and_modify(|v| *v = None).or_insert(number.map(i32::from)); let identity = (class.dxf_name.as_str(), class.cpp_class_name.as_str(), class.is_an_entity); - identities.entry(identity).and_modify(|v| *v = None).or_insert(Some(number)); + identities.entry(identity).and_modify(|v| *v = None).or_insert(number); } let identity = |dxf: &str, cpp: &str, entity: bool| { identities @@ -29,7 +29,7 @@ pub(super) fn prepare(doc: &mut CadDocument) -> Result<(), String> { .copied() .flatten() .map(i32::from) - .ok_or_else(|| format!("DWG: missing or ambiguous application CLASS for {dxf} ({cpp})")) + .ok_or_else(|| format!("missing or ambiguous application CLASS for {dxf:.80} ({cpp:.80})")) }; let source = doc.dwg_source_version.unwrap_or(doc.version).version_code(); let dwg_version = match source { @@ -41,10 +41,10 @@ pub(super) fn prepare(doc: &mut CadDocument) -> Result<(), String> { 1024 => 29, 1027 => 31, 1032 => 33, - _ => return Ok(()), + _ => return Vec::new(), }; let maintenance = i32::from(doc.maintenance_version); - let remap = |id: i32| numbers.get(&id).copied().flatten().ok_or_else(|| format!("DWG: proxy names missing application class {id}")); + let remap = |id: i32| numbers.get(&id).copied().flatten().ok_or_else(|| format!("proxy names missing or ambiguous application class {id}")); // Only mutate proxies: entities_mut invalidates the decoder's verbatim records for every entity. let handles: Vec<_> = doc .entities() @@ -57,46 +57,81 @@ pub(super) fn prepare(doc: &mut CadDocument) -> Result<(), String> { _ => None, }) .collect(); + let mut notes = Vec::new(); + let mut entity_count = 0usize; + let mut object_count = 0usize; for handle in handles { - let text_payload = match doc.get_entity(handle) { - Some(EntityType::Extended(e)) if matches!(e.data, ExtendedEntityData::RegisteredClass(_)) => { - if let ExtendedEntityData::RegisteredClass(data) = &e.data - && !data.properties.is_empty() - { - return Err(format!("DWG: cannot transport semantic properties of {} as an opaque proxy", data.dxf_name)); + let converted = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let Some(EntityType::Extended(entity)) = doc.get_entity(handle) else { return Ok(None) }; + let data = match &entity.data { + ExtendedEntityData::Proxy(data) => { + let mut proxy = data.clone(); + proxy.class_id = remap(proxy.class_id)?; + ExtendedEntityData::Proxy(proxy) + } + ExtendedEntityData::RegisteredClass(data) => { + if !data.properties.is_empty() { + return Err(format!("cannot transport semantic properties of {:.80} as an opaque proxy", data.dxf_name)); + } + let class_id = identity(&data.dxf_name, &data.cpp_class_name, true)?; + let text_payload = entity.common.raw_record.as_deref().map(registered_text).transpose()?.unwrap_or_default(); + ExtendedEntityData::Proxy(ProxyEntityData { + proxy_id: 498, + class_id, + dxf_subclass: String::new(), + version: (maintenance << 16) | dwg_version, + dwg_version, + maintenance_version: maintenance, + from_dxf: false, + graphics: ProxyPayload::from_bytes(entity.common.graphic_data.as_deref().unwrap_or_default()), + payload: data.payload.clone(), + text_payload, + object_ids: data.object_ids.clone(), + }) + } + _ => return Ok(None), + }; + Ok(Some(data)) + })) + .unwrap_or_else(|_| Err("application record conversion failed".into())); + match converted { + Ok(Some(data)) => { + if let Some(EntityType::Extended(entity)) = doc.get_entity_mut(handle) { + entity.data = data; } - e.common.raw_record.as_deref().map(registered_text).transpose()?.unwrap_or_default() } - _ => ProxyPayload::default(), - }; - let Some(EntityType::Extended(entity)) = doc.get_entity_mut(handle) else { continue }; - match &mut entity.data { - ExtendedEntityData::Proxy(data) => data.class_id = remap(data.class_id)?, - ExtendedEntityData::RegisteredClass(data) => { - let class_id = identity(&data.dxf_name, &data.cpp_class_name, true)?; - entity.data = ExtendedEntityData::Proxy(ProxyEntityData { - proxy_id: 498, - class_id, - dxf_subclass: String::new(), - version: (maintenance << 16) | dwg_version, - dwg_version, - maintenance_version: maintenance, - from_dxf: false, - graphics: ProxyPayload::from_bytes(entity.common.graphic_data.as_deref().unwrap_or_default()), - payload: data.payload.clone(), - text_payload, - object_ids: data.object_ids.clone(), - }); + Ok(None) => {} + Err(why) => { + doc.remove_entity(handle); + entity_count = entity_count.saturating_add(1); + skipped_note(&mut notes, "entity", handle, &why); } - _ => {} } } - for object in doc.objects.values_mut() { + doc.objects.retain(|handle, object| { if let ObjectType::ProxyObject(proxy) = object { - proxy.class_id = remap(proxy.class_id)?; + match remap(proxy.class_id) { + Ok(id) => proxy.class_id = id, + Err(why) => { + object_count = object_count.saturating_add(1); + skipped_note(&mut notes, "object", *handle, &why); + return false; + } + } } + true + }); + if entity_count > 0 || object_count > 0 { + notes.insert(0, format!("DWG: partial read; application proxy records left out (entities: {entity_count}, objects: {object_count})")); + } + notes +} + +fn skipped_note(notes: &mut Vec, kind: &str, handle: acadrust::types::Handle, why: &str) { + if notes.len() < 6 { + let why: String = why.chars().take(256).collect(); + notes.push(format!("DWG: proxy {kind} {handle} left out: {why}")); } - Ok(()) } /// The decoder's registered-class representation omits the separate R2007+ string stream. @@ -191,7 +226,7 @@ fn read_class(tags: &[Tag], number: i16) -> Result { } /// Keep source class positions and metadata, adding missing decoder defaults only afterwards. -pub(super) fn restore_classes(doc: &mut CadDocument, classes: Vec) -> Result<(), String> { +pub(super) fn restore_classes(doc: &mut CadDocument, source: impl FnOnce() -> Result, String>) -> Result<(), String> { let mut proxies = Vec::new(); for entity in doc.entities() { if let EntityType::Extended(e) = entity @@ -208,6 +243,7 @@ pub(super) fn restore_classes(doc: &mut CadDocument, classes: Vec) -> if proxies.is_empty() { return Ok(()); } + let classes = source()?; for (number, entity) in proxies { let index = number.checked_sub(500).and_then(|n| usize::try_from(n).ok()); if !index.and_then(|i| classes.get(i)).is_some_and(|c| c.is_an_entity == entity) { @@ -286,7 +322,7 @@ mod tests { assert_eq!(classes[0].instance_count, 7); assert!(classes[0].was_zombie); let mut doc = proxy_doc(); - restore_classes(&mut doc, classes.clone()).unwrap(); + restore_classes(&mut doc, || Ok(classes.clone())).unwrap(); assert_eq!(doc.classes.iter().take(2).cloned().collect::>(), classes); } } @@ -302,11 +338,47 @@ mod tests { } let mut class = DxfClass::new_entity("ENTITY", "Entity"); class.class_number = 500; - assert!(restore_classes(&mut doc, vec![class]).is_err()); + assert!(restore_classes(&mut doc, || Ok(vec![class])).is_err()); } - assert!(restore_classes(&mut proxy_doc(), vec![DxfClass::new("A", "A"), DxfClass::new("B", "B")]).is_err()); + assert!(restore_classes(&mut proxy_doc(), || Ok(vec![DxfClass::new("A", "A"), DxfClass::new("B", "B")])).is_err()); } + #[test] + fn unrepresentable_proxies_keep_other_geometry_and_bounded_diagnostics() { + use acadrust::entities::{Line, RawRecord}; + use acadrust::objects::ProxyObject; + use acadrust::types::{Handle, Vector3}; + for count in [1, 10] { + let mut doc = proxy_doc(); + if let Some(EntityType::Extended(e)) = doc.entities_mut().next() + && let ExtendedEntityData::Proxy(p) = &mut e.data + { + p.class_id = i32::MAX; + } + let template = doc.entities().next().unwrap().clone(); + for _ in 1..count { + let mut entity = template.clone(); + entity.common_mut().handle = Handle::NULL; + doc.add_entity(entity).unwrap(); + } + let object = Handle::new(u64::MAX - 1); + assert!(!doc.objects.contains_key(&object)); + doc.objects.insert(object, ObjectType::ProxyObject(ProxyObject { handle: object, class_id: i32::MAX, ..Default::default() })); + let line = doc.add_entity(EntityType::Line(Line::from_points(Vector3::new(2.0, 3.0, 0.0), Vector3::new(12.0, 13.0, 0.0)))).unwrap(); + let raw = RawRecord { data: vec![0xAA, 0x55], handle_bits: 0, version: DxfVersion::AC1032 }; + doc.get_entity_mut(line).unwrap().common_mut().raw_record = Some(std::sync::Arc::new(raw.clone())); + let notes = prepare(&mut doc); + assert_eq!(notes.len(), (count + 1).min(6) + 1); + assert!(notes.iter().all(|n| n.starts_with("DWG:"))); + assert!(notes[0].contains(&format!("entities: {count}, objects: 1"))); + assert_eq!(doc.entities().count(), 1); + assert!(!doc.objects.contains_key(&object)); + let Some(EntityType::Line(actual)) = doc.get_entity(line) else { panic!("readable line") }; + assert_eq!(actual.start, Vector3::new(2.0, 3.0, 0.0)); + assert_eq!(actual.end, Vector3::new(12.0, 13.0, 0.0)); + assert_eq!(doc.get_entity(line).unwrap().common().raw_record.as_deref(), Some(&raw)); + } + } #[test] fn opaque_registered_class_ambiguity_is_not_guessed() { let mut doc = CadDocument::new(); @@ -323,11 +395,12 @@ mod tests { }), }))) .unwrap(); - assert!(prepare(&mut doc).unwrap_err().contains("ambiguous")); + assert!(prepare(&mut doc).iter().any(|n| n.contains("ambiguous"))); + assert_eq!(doc.entities().count(), 0); assert!(class_number(usize::MAX).is_err()); } #[test] - fn semantic_records_keep_their_verbatim_data_and_mixed_payloads_are_rejected() { + fn semantic_records_keep_verbatim_data_and_mixed_payloads_are_reported() { use acadrust::entities::RawRecord; use acadrust::objects::{SemanticProperty, SemanticPropertyValue}; for payload in [ProxyPayload::default(), ProxyPayload::from_bits(&[0x80], 1)] { @@ -352,10 +425,11 @@ mod tests { doc.get_entity_mut(handle).unwrap().common_mut().raw_record = Some(std::sync::Arc::new(raw.clone())); assert_eq!(doc.get_entity(handle).unwrap().common().raw_record.as_deref(), Some(&raw)); if payload.bit_count == 0 { - prepare(&mut doc).unwrap(); + assert!(prepare(&mut doc).is_empty()); assert_eq!(doc.get_entity(handle).unwrap().common().raw_record.as_deref(), Some(&raw)); } else { - assert!(prepare(&mut doc).unwrap_err().contains("semantic properties")); + assert!(prepare(&mut doc).iter().any(|n| n.contains("semantic properties"))); + assert!(doc.get_entity(handle).is_none()); } } } @@ -411,7 +485,7 @@ mod string_stream_tests { object_ids: vec![], }); entity.common.raw_record = Some(raw); - prepare(&mut decoded).unwrap(); + assert!(prepare(&mut decoded).is_empty()); for _ in 0..2 { let mut dxf = Vec::new(); acadrust::DxfWriter::new(&decoded).write_to_writer(&mut dxf).unwrap(); diff --git a/crates/io/src/dxf_write.rs b/crates/io/src/dxf_write.rs index 411f1c4b2..3ce6393d8 100644 --- a/crates/io/src/dxf_write.rs +++ b/crates/io/src/dxf_write.rs @@ -127,11 +127,17 @@ pub(crate) fn dwg_bridge(d: &Drawing, dwg: Version) -> String { cadcraft_dxf::write_ascii(&tags) } -/// Subclass markers the DWG bridge knows by another spelling (it matches them exactly): -/// AutoCAD writes a dynamic block lookup parameter's as `AcDbBlockLookUpParameter`, the bridge -/// reads `AcDbBlockLookupParameter`; otherwise the parameter loses its name and its lookup -/// action in the DWG, and AutoCAD's lookups stop working. +/// Proxy type names and lookup-parameter subclass names must match the bridge reader's spelling. +/// Ordinary DXF output retains source spellings. fn bridge_markers(tags: &mut [Tag]) { + for tag in tags.iter_mut().filter(|tag| tag.code == 0) { + for name in ["ACAD_PROXY_ENTITY", "ACAD_PROXY_OBJECT"] { + if tag.str().trim().eq_ignore_ascii_case(name) { + *tag = Tag::s(0, name); + break; + } + } + } for t in tags.iter_mut().filter(|t| t.code == 100) { if t.str().trim().eq_ignore_ascii_case("AcDbBlockLookupParameter") { *t = Tag::s(100, "AcDbBlockLookupParameter"); diff --git a/crates/io/tests/registered_proxy.rs b/crates/io/tests/registered_proxy.rs index d50c078ab..29559a6fb 100644 --- a/crates/io/tests/registered_proxy.rs +++ b/crates/io/tests/registered_proxy.rs @@ -187,3 +187,48 @@ fn proxies_inside_block_definitions_survive_repeated_saves() { assert!(block.entities.iter().any(|e| matches!(e.kind, EntityKind::Unknown(_)))); } } + +#[test] +fn unused_malformed_classes_do_not_block_plain_geometry() { + for (name, cpp, flags, count) in [("", "Unused", 0, 0), ("UNUSED", "", 0, 0), ("UNUSED", "Unused", 65536, 0), ("UNUSED", "Unused", 0, i64::MAX)] { + let dxf = format!( + "0\nSECTION\n2\nHEADER\n9\n$ACADVER\n1\nAC1032\n0\nENDSEC\n0\nSECTION\n2\nCLASSES\n0\nCLASS\n1\n{name}\n2\n{cpp}\n3\nSynthetic\n90\n{flags}\n91\n{count}\n281\n1\n0\nENDSEC\n0\nSECTION\n2\nENTITIES\n0\nLINE\n5\n10\n100\nAcDbEntity\n8\n0\n100\nAcDbLine\n10\n2\n20\n3\n11\n12\n21\n13\n0\nENDSEC\n0\nEOF\n" + ); + let bytes = cadcraft_dwg::dxf_to_dwg_as(dxf.as_bytes(), "AC1032").unwrap(); + let drawing = cadcraft_io::read(&bytes, "plain.dwg").unwrap(); + let lines: Vec<_> = drawing.model.iter().filter_map(|e| if let EntityKind::Line(l) = &e.kind { Some(l) } else { None }).collect(); + assert_eq!(lines.len(), 1); + assert_eq!(lines[0].a.x, 2.0); + assert_eq!(lines[0].a.y, 3.0); + assert_eq!(lines[0].b.x, 12.0); + assert_eq!(lines[0].b.y, 13.0); + } +} + +#[test] +fn proxy_spelling_is_preserved_in_dxf_and_normalized_only_for_dwg() { + let source = String::from_utf8(duplicate_class_dxf()).unwrap(); + for (entity, object) in [("acad_proxy_entity", "acad_proxy_object"), ("AcAd_PrOxY_EnTiTy", "AcAd_PrOxY_ObJeCt")] { + let dxf = source.replace("ACAD_PROXY_ENTITY", entity).replace("ACAD_PROXY_OBJECT", object); + let drawing = cadcraft_io::read(dxf.as_bytes(), "source.dxf").unwrap(); + assert_eq!(drawing.entity_count(), 3); + let out = cadcraft_io::write_with(&drawing, "copy.dxf", &SaveOptions { version: Some(Version::R2018), ..Default::default() }).unwrap(); + let tags = cadcraft_dxf::parse(&out).unwrap(); + assert!(tags.iter().any(|t| t.code == 0 && t.str() == entity)); + let bytes = cadcraft_io::write_with(&drawing, "copy.dwg", &SaveOptions { version: Some(Version::R2018), ..Default::default() }).unwrap(); + let decoded = cadcraft_dwg::dwg_to_dxf(&bytes).unwrap(); + let tags = cadcraft_dxf::parse(&decoded).unwrap(); + let proxies: Vec<_> = cadcraft_dxf::sections(&tags) + .iter() + .flat_map(|s| cadcraft_dxf::records(&s.tags)) + .filter(|(kind, _)| kind.starts_with("ACAD_PROXY_")) + .collect(); + assert_eq!(proxies.len(), 3); + for (_, tags) in proxies { + assert!(tags.iter().any(|t| t.code == 310 && t.str() == "A580")); + assert!(tags.iter().any(|t| matches!(t.code, 93 | 161) && t.i64() == 9)); + } + let recovered = cadcraft_io::read(&bytes, "copy.dwg").unwrap(); + assert_eq!(recovered.entity_count(), 3); + } +}