From d27eb625ca288c40259245183863debb93f174b4 Mon Sep 17 00:00:00 2001 From: Felix Riestra Date: Thu, 10 Sep 2026 19:25:24 +0200 Subject: [PATCH] fix(constraints): split large XRecord Chunk data across entries for DWG The DWG writer truncates an XRecord Chunk to u8::MAX bytes, so a sketch's constraint set or named-parameter table silently lost every entry on the next DWG open once serialization passed ~255 bytes (found live: a 23-constraint demo round-tripped fine through DXF but came back empty through DWG). Split the blob across multiple same-code 310 entries on save and reassemble them on load, entirely on the OCS side. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 17 +++++++++++ src/scene/named_parameters_persist.rs | 31 +++++++++++++------ src/scene/sketch_persist.rs | 43 ++++++++++++++++++++------- 3 files changed, 72 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 06779d265..78b2298fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,23 @@ pushed. ## Unreleased (since `3d0a41b6`) +- **Fix:** a sketch's constraint set (or the named-parameter table) silently + lost *all* of its entries on the next DWG open once its serialized size + passed 255 bytes — roughly 5-6 constraints, easily reached by a real + sketch. Root cause: the vendored DWG writer packs an `XRecord` binary + `Chunk` entry's length as a single `u8` and truncates anything longer + (`cadcodec`'s `encode_xrecord_entries`), so the truncated bytes then + failed to `bincode::deserialize` on load and the loader silently dropped + the whole scope rather than erroring. DXF was unaffected (its writer has + no such cap), which is what made this findable — the exact same document + round-tripped constraint-for-constraint through DXF and came back empty + through DWG. Fixed entirely on the OCS side, no dependency patch needed: + the blob is now split across as many same-code `310` entries as it takes + (below 255 bytes each) on save, and every entry for that key is + concatenated back together on load, instead of just the first one. + ([sketch_persist.rs](src/scene/sketch_persist.rs), + [named_parameters_persist.rs](src/scene/named_parameters_persist.rs)) + - **Fix:** none of the 17 sketch-constraint commands (`HCONSTRAINT`, `VCONSTRAINT`, `PCONSTRAINT`, `QCONSTRAINT`, `ECONSTRAINT`, `TCONSTRAINT`, `NCONSTRAINT`, `NRCONSTRAINT`, `LCONSTRAINT`, `FXCONSTRAINT`, diff --git a/src/scene/named_parameters_persist.rs b/src/scene/named_parameters_persist.rs index ff09b7085..d57aa1c01 100644 --- a/src/scene/named_parameters_persist.rs +++ b/src/scene/named_parameters_persist.rs @@ -48,6 +48,12 @@ use super::Scene; const XRECORD_KEY: &str = "OCS_NAMED_PARAMETERS"; +/// See `sketch_persist::MAX_CHUNK_BYTES`'s doc comment: the vendored DWG +/// writer truncates any single `XRecordValue::Chunk` entry to `u8::MAX` +/// bytes, so a table past a handful of parameters must be split across +/// multiple same-code `310` entries on write and concatenated back on read. +const MAX_CHUNK_BYTES: usize = u8::MAX as usize; + /// Prefixed onto the serialized blob so a future schema change can be /// detected and gracefully skipped rather than silently misreading bytes — /// same call-out as `sketch_persist::FORMAT_VERSION`, same caveat that the @@ -91,11 +97,13 @@ impl Scene { self.document.ensure_xrecord(owner, XRECORD_KEY); if let Some(record) = self.document.xrecord_mut(owner, XRECORD_KEY) { // Overwrite, not append: a resave must replace the prior blob, - // not accumulate one more Chunk entry every time. + // not accumulate more Chunk entries every time. record.entries.clear(); - record - .entries - .push(acadrust::objects::XRecordEntry::new(310, acadrust::objects::XRecordValue::Chunk(bytes))); + for chunk in bytes.chunks(MAX_CHUNK_BYTES) { + record + .entries + .push(acadrust::objects::XRecordEntry::new(310, acadrust::objects::XRecordValue::Chunk(chunk.to_vec()))); + } } } @@ -114,12 +122,17 @@ impl Scene { return; } let Some(record) = self.document.xrecord(owner, XRECORD_KEY) else { return }; - let Some(bytes) = record.entries.iter().find_map(|entry| match &entry.value { - acadrust::objects::XRecordValue::Chunk(bytes) => Some(bytes.clone()), - _ => None, - }) else { + // Concatenate every Chunk entry in order, not just the first — see + // `MAX_CHUNK_BYTES`'s doc comment. + let mut bytes = Vec::new(); + for entry in &record.entries { + if let acadrust::objects::XRecordValue::Chunk(chunk) = &entry.value { + bytes.extend_from_slice(chunk); + } + } + if bytes.is_empty() { return; - }; + } if let Some(table) = decode(&bytes) { self.named_parameters = table; } diff --git a/src/scene/sketch_persist.rs b/src/scene/sketch_persist.rs index b0f224540..a51088cdf 100644 --- a/src/scene/sketch_persist.rs +++ b/src/scene/sketch_persist.rs @@ -21,6 +21,21 @@ use acadrust::{CadDocument, Handle}; const XRECORD_KEY: &str = "OCS_SKETCH_CONSTRAINTS"; +/// The vendored DWG writer's `XRecordValue::Chunk` encoder packs a single +/// `u8` length prefix per entry and silently truncates anything longer +/// (`cadcodec`'s `io/dwg/dwg_stream_writers/object_writer/objects.rs`, +/// `encode_xrecord_entries`) — unlike the DXF writer, which has no such +/// limit. A constraint set past a handful of entries serializes past 255 +/// bytes easily, so a single `XRecordEntry` silently loses the tail and +/// `bincode::deserialize` then fails on the truncated bytes, dropping the +/// *entire* scope's constraints on the next DWG load — reproduced and +/// root-caused live (not from a change log): a 5-entry set round-tripped +/// clean, a 6-entry set of the same shapes lost everything. Splitting the +/// blob across as many same-code `310` entries as needed (below) works +/// around it without touching the vendored crate — `CadDocument::xrecord`'s +/// `entries: Vec` already supports repeating a code. +const MAX_CHUNK_BYTES: usize = u8::MAX as usize; + /// Removes a named XRecord from `owner`'s extension dictionary, if present. /// The dictionary itself is left in place even if now empty — other code may /// already reference it via `xdictionary_handle`, and an empty extension @@ -107,12 +122,14 @@ impl Scene { self.document.ensure_xrecord(owner, XRECORD_KEY); if let Some(record) = self.document.xrecord_mut(owner, XRECORD_KEY) { // Overwrite, not append: a resave must replace the prior - // blob, not accumulate one more Chunk entry every time. + // blob, not accumulate more Chunk entries every time. record.entries.clear(); - record.entries.push(acadrust::objects::XRecordEntry::new( - 310, - acadrust::objects::XRecordValue::Chunk(bytes), - )); + for chunk in bytes.chunks(MAX_CHUNK_BYTES) { + record.entries.push(acadrust::objects::XRecordEntry::new( + 310, + acadrust::objects::XRecordValue::Chunk(chunk.to_vec()), + )); + } } } } @@ -132,12 +149,18 @@ impl Scene { self.document.block_records.iter().map(|record| record.handle).collect(); for owner in owners { let Some(record) = self.document.xrecord(owner, XRECORD_KEY) else { continue }; - let Some(bytes) = record.entries.iter().find_map(|entry| match &entry.value { - acadrust::objects::XRecordValue::Chunk(bytes) => Some(bytes.clone()), - _ => None, - }) else { + // Concatenate every Chunk entry in order, not just the first — + // a set materialized past `MAX_CHUNK_BYTES` spans more than one + // same-code `310` entry (see that constant's doc comment). + let mut bytes = Vec::new(); + for entry in &record.entries { + if let acadrust::objects::XRecordValue::Chunk(chunk) = &entry.value { + bytes.extend_from_slice(chunk); + } + } + if bytes.is_empty() { continue; - }; + } if let Some(set) = decode(&bytes) { self.sketch_constraints.push(set); }