From d005cc4ab172a57541d1671b545e5951bfec7dc6 Mon Sep 17 00:00:00 2001 From: Hakan Seven Date: Mon, 13 Jul 2026 17:31:09 +0300 Subject: [PATCH] feat(props): make dim-line colour editable for leaders and dimensions The "Dim line color" row on Leader and Dimension entities was read-only: an earlier attempt wired it to Leader.override_color, which acadrust never serialises, so the pick was lost on save. Store it instead as a standard ACAD_DSTYLE per-object dimension-style override (DXF code 176, an ACI index) through the existing dim_override codec, so it round-trips through both DWG and DXF like the other dim overrides. RGB picks collapse to the nearest ACI, matching the rest of the dim-colour stack (dimension styles are index-only through the file layer). The renderer prefers the override over the style's DIMCLRD. The write branch is guarded to leaders and dimensions so a mixed selection cannot stamp the override onto other entity types. Bumps acadrust to c9fe982, which carries parsed XDATA onto dimensions on DXF read (it was dropping common.extended_data), so the dimension override persists on DXF save as well as DWG. Co-Authored-By: Claude Opus 4.8 --- Cargo.lock | 2 +- src/app/properties.rs | 40 +++++----- src/app/update/mod.rs | 36 +++++++++ src/entities/dim_override.rs | 9 +++ src/entities/dimension.rs | 11 ++- src/entities/leader.rs | 21 ++++- src/ui/properties.rs | 24 ++++-- tests/dim_line_color_roundtrip.rs | 127 ++++++++++++++++++++++++++++++ 8 files changed, 241 insertions(+), 29 deletions(-) create mode 100644 tests/dim_line_color_roundtrip.rs diff --git a/Cargo.lock b/Cargo.lock index d78ba2fa..dab55031 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -73,7 +73,7 @@ checksum = "366ffbaa4442f4684d91e2cd7c5ea7c4ed8add41959a31447066e279e432b618" [[package]] name = "acadrust" version = "0.4.0" -source = "git+https://github.com/HakanSeven12/acadrust?branch=main#c8e63eb6b5e6d23faeebc504d57accc091b4cae4" +source = "git+https://github.com/HakanSeven12/acadrust?branch=main#c9fe9828583c6fe704191c08b942e959601ad4c0" dependencies = [ "ahash 0.8.12", "anyhow", diff --git a/src/app/properties.rs b/src/app/properties.rs index fa849cab..234cb671 100644 --- a/src/app/properties.rs +++ b/src/app/properties.rs @@ -414,6 +414,20 @@ impl OpenCADStudio { }) { sections.extend(crate::entities::dimension::style_sections(style)); + + // The style groups are a read-only mirror, but the + // dim-line colour is an editable per-object override: + // prefer the ACAD_DSTYLE code-176 (ACI) override, else + // the style's DIMCLRD. + use crate::entities::dim_override as dov; + use crate::scene::model::object::PropValue; + let dim_c = dov::color(&d.base().common.extended_data, dov::DIMCLRD) + .unwrap_or_else(|| acadrust::types::Color::from_index(style.dimclrd)); + set_row_value( + &mut sections, + "dim_line_color", + PropValue::ColorChoice(dim_c), + ); } } @@ -561,14 +575,16 @@ impl OpenCADStudio { }, ); - // Dim-line colour stays read-only: it lives in the - // leader's override_color, which the file format - // layer does not yet serialise, so making it - // editable would silently lose the pick on save. - set_row( + // Dim-line colour: a per-object ACAD_DSTYLE + // override (code 176, an ACI index) wins over the + // style's DIMCLRD. Editable — the picked colour is + // written back as that override so it round-trips. + let dim_c = dov::color(xd, dov::DIMCLRD) + .unwrap_or_else(|| acadrust::types::Color::from_index(ds.dimclrd)); + set_row_value( &mut sections, "dim_line_color", - dim_color_label(ds.dimclrd, &ld.override_color), + PropValue::ColorChoice(dim_c), ); let gap = dov::real(xd, dov::DIMGAP).unwrap_or(ds.dimgap); @@ -1578,18 +1594,6 @@ fn dim_lineweight_label(dimlwd: i16) -> String { } } -/// DIMCLRD color (ACI) → label; ByBlock falls back to the leader's override. -fn dim_color_label(dimclrd: i16, override_color: &acadrust::types::Color) -> String { - match dimclrd { - 0 => match override_color.rgb() { - Some((r, g, b)) => format!("RGB({r},{g},{b})"), - None => "ByBlock".to_string(), - }, - 256 => "ByLayer".to_string(), - n => format!("Color {n}"), - } -} - /// Human-readable INSUNITS name (DXF group 70 unit codes). fn insunits_name(code: i16) -> &'static str { match code { diff --git a/src/app/update/mod.rs b/src/app/update/mod.rs index 1d32c896..7a0129ca 100644 --- a/src/app/update/mod.rs +++ b/src/app/update/mod.rs @@ -2620,6 +2620,42 @@ impl OpenCADStudio { Message::PropColorFieldChanged { field, color } => { let i = self.active_tab; let handles = self.property_target_handles(i); + // Dim-line colour override (Leader / Dimension): write it as an + // ACAD_DSTYLE code-176 override (an ACI index) so it round-trips + // through DWG and DXF. RGB picks collapse to the nearest ACI, in + // line with the rest of the dim-colour stack (index-only through + // the file layer). Guarded to leaders / dimensions so a mixed + // selection can't stamp the override onto other entities. + if field == "dim_line_color" { + let aci = color.approximate_index(); + let targets: Vec = handles + .iter() + .copied() + .filter(|&h| { + matches!( + self.tabs[i].scene.document.get_entity(h), + Some(acadrust::EntityType::Leader(_)) + | Some(acadrust::EntityType::Dimension(_)) + ) + }) + .collect(); + if !targets.is_empty() { + self.push_undo_snapshot(i, "CHPROP"); + for &handle in &targets { + crate::entities::dim_override::set( + &mut self.tabs[i].scene.document, + handle, + crate::entities::dim_override::DIMCLRD, + Some(acadrust::xdata::XDataValue::Integer16(aci)), + ); + } + self.invalidate_property_targets(i, &targets); + self.tabs[i].properties.open_color_field = None; + self.tabs[i].dirty = true; + self.refresh_properties(); + } + return Task::none(); + } if !handles.is_empty() { self.push_undo_snapshot(i, "CHPROP"); let idx = if field == "gradient_color_2" { 1 } else { 0 }; diff --git a/src/entities/dim_override.rs b/src/entities/dim_override.rs index a727ada5..0b5d66be 100644 --- a/src/entities/dim_override.rs +++ b/src/entities/dim_override.rs @@ -6,6 +6,7 @@ //! renderer and the properties panel prefer an override over the style default, //! so editing one of these rows writes here and the change round-trips to file. +use acadrust::types::Color; use acadrust::xdata::{ExtendedData, XDataValue}; use acadrust::{CadDocument, Handle}; @@ -13,6 +14,7 @@ use acadrust::{CadDocument, Handle}; pub const DIMSCALE: i16 = 40; // overall scale (real) pub const DIMASZ: i16 = 41; // arrow size (real) pub const DIMTAD: i16 = 77; // text vertical pos (int16) +pub const DIMCLRD: i16 = 176; // dim line colour (int16 = ACI index) pub const DIMGAP: i16 = 147; // text offset / gap (real) pub const DIMLWD: i16 = 371; // dim line lineweight (int16) pub const DIMLDRBLK: i16 = 341; // leader arrow block (handle) @@ -58,6 +60,13 @@ pub fn int(xd: &ExtendedData, code: i16) -> Option { }) } +/// The colour override for `code`, if present. Dim-colour overrides are stored +/// as an ACI index (the same `int16` slot the dimension style uses), so this +/// decodes it back into a `Color` (0 = ByBlock, 256 = ByLayer, else indexed). +pub fn color(xd: &ExtendedData, code: i16) -> Option { + int(xd, code).map(Color::from_index) +} + /// The handle-valued override for `code`, if present. pub fn handle(xd: &ExtendedData, code: i16) -> Option { pairs(xd) diff --git a/src/entities/dimension.rs b/src/entities/dimension.rs index 46c62328..7ccaff5c 100644 --- a/src/entities/dimension.rs +++ b/src/entities/dimension.rs @@ -1290,11 +1290,18 @@ fn tessellate_dimension_inner( ); // Per-spec colours: DIMCLRD (dim/arrows), DIMCLRE (ext), DIMCLRT (text). - // 0=ByBlock and 256=ByLayer fall through to entity_color. + // 0=ByBlock and 256=ByLayer fall through to entity_color. DIMCLRD also + // honours a per-object ACAD_DSTYLE override (code 176) so an edited + // dim-line colour renders even without touching the style. let dim_color = if selected { WireModel::SELECTED } else { - resolve_dim_color(style.map(|s| s.dimclrd).unwrap_or(0), entity_color) + let dim_clr = crate::entities::dim_override::int( + &dim.base().common.extended_data, + crate::entities::dim_override::DIMCLRD, + ) + .unwrap_or_else(|| style.map(|s| s.dimclrd).unwrap_or(0)); + resolve_dim_color(dim_clr, entity_color) }; let ext_color = if selected { WireModel::SELECTED diff --git a/src/entities/leader.rs b/src/entities/leader.rs index 1eaa61de..c3e68448 100644 --- a/src/entities/leader.rs +++ b/src/entities/leader.rs @@ -547,10 +547,29 @@ impl LeaderTess for Leader { use crate::entities::dim_override as dov; use crate::scene::model::wire_model::WireModel; let xd = &self.common.extended_data; + // Dim-line colour: a per-object ACAD_DSTYLE override (code 176, an ACI + // index) wins over the assigned dim style's DIMCLRD; ByLayer / ByBlock + // (0 / 256) and no setting fall through to the entity colour. let color = if selected { WireModel::SELECTED } else { - entity_color + let dim_clr = dov::int(xd, dov::DIMCLRD).or_else(|| { + document + .dim_styles + .iter() + .find(|s| { + s.name.eq_ignore_ascii_case(&self.dimension_style) + || (self.dimension_style.trim().is_empty() + && s.name.eq_ignore_ascii_case("Standard")) + }) + .map(|s| s.dimclrd) + }); + match dim_clr { + Some(idx) if idx != 0 && idx != 256 => crate::scene::convert::tess_util::aci_to_rgba( + &acadrust::types::Color::from_index(idx), + ), + _ => entity_color, + } }; // A concrete DIMLWD override sets the leader line's weight; ByLayer / // ByBlock / Default and no override keep the resolved weight passed in. diff --git a/src/ui/properties.rs b/src/ui/properties.rs index a372bb30..c32dd03f 100644 --- a/src/ui/properties.rs +++ b/src/ui/properties.rs @@ -434,18 +434,28 @@ impl PropertiesPanel { ); return prop_row_widget(label, selector); } - // Generic per-field colour picker (hatch gradient colours) — routes to - // the field, not the entity's main colour. - if field == "gradient_color_1" || field == "gradient_color_2" { + // Generic per-field colour picker — routes to the named field, not the + // entity's main colour. Used by hatch gradient colours and the dim-line + // colour override (Leader / Dimension). Dim colours legitimately take + // ByLayer / ByBlock; gradient colours do not. + if field == "gradient_color_1" || field == "gradient_color_2" || field == "dim_line_color" { let open = self.open_color_field.as_deref() == Some(field); let fsel = field.to_string(); - let selector = crate::ui::color_select::color_selector( - color, - open, + let extras = if field == "dim_line_color" { + crate::ui::color_select::ColorExtras { + by_layer: true, + by_block: true, + } + } else { crate::ui::color_select::ColorExtras { by_layer: false, by_block: false, - }, + } + }; + let selector = crate::ui::color_select::color_selector( + color, + open, + extras, move |c| Message::PropColorFieldChanged { field: fsel.clone(), color: c, diff --git a/tests/dim_line_color_roundtrip.rs b/tests/dim_line_color_roundtrip.rs new file mode 100644 index 00000000..643321c0 --- /dev/null +++ b/tests/dim_line_color_roundtrip.rs @@ -0,0 +1,127 @@ +// The editable "Dim line color" property writes an ACAD_DSTYLE dimension-style +// override (code 176, an ACI index) on the leader / dimension. That override +// must survive a full DWG *and* DXF save/reload — otherwise the picked colour +// is silently lost, which is exactly why the row was kept read-only before. +// This is the regression guard for that persistence. + +use acadrust::entities::{Dimension, DimensionLinear, Leader, Line}; +use acadrust::tables::BlockRecord; +use acadrust::types::{Color, Vector3}; +use acadrust::xdata::XDataValue; +use acadrust::{CadDocument, EntityType, Handle}; +use OpenCADStudio::entities::dim_override as dov; +use OpenCADStudio::scene::Scene; + +fn roundtrip(doc: &CadDocument, ext: &str) -> CadDocument { + let bytes = OpenCADStudio::io::save_to_bytes(doc, ext, doc.version) + .unwrap_or_else(|e| panic!("save to {ext}: {e}")); + OpenCADStudio::io::load_bytes(&format!("rt.{ext}"), bytes) + .unwrap_or_else(|e| panic!("reload {ext}: {e}")) +} + +fn leader_color(doc: &CadDocument) -> Option { + doc.entities() + .find_map(|e| match e { + EntityType::Leader(l) => Some(dov::color(&l.common.extended_data, dov::DIMCLRD)), + _ => None, + }) + .flatten() +} + +fn dim_color(doc: &CadDocument) -> Option { + doc.entities() + .find_map(|e| match e { + EntityType::Dimension(d) => Some(dov::color(&d.base().common.extended_data, dov::DIMCLRD)), + _ => None, + }) + .flatten() +} + +fn leader_scene(aci: i16) -> Scene { + let mut scene = Scene::new(); + let mut ld = Leader::new(); + ld.vertices = vec![Vector3::new(0.0, 0.0, 0.0), Vector3::new(10.0, 5.0, 0.0)]; + let h = scene.add_entity(EntityType::Leader(ld)); + dov::set( + &mut scene.document, + h, + dov::DIMCLRD, + Some(XDataValue::Integer16(aci)), + ); + scene +} + +fn dimension_scene(aci: i16) -> Scene { + let mut scene = Scene::new(); + + // A baked *D0 block holding one line, so the dimension writes cleanly. + let br_h = Handle::new(scene.document.next_handle()); + let mut br = BlockRecord::new("*D0"); + br.handle = br_h; + scene.document.block_records.add(br).unwrap(); + let mut sub = Line::new(); + sub.start = Vector3::new(0.0, 0.0, 0.0); + sub.end = Vector3::new(10.0, 0.0, 0.0); + let mut sub_e = EntityType::Line(sub); + sub_e.common_mut().owner_handle = br_h; + scene.document.add_entity(sub_e).unwrap(); + + let mut dim = + DimensionLinear::new(Vector3::new(0.0, 0.0, 0.0), Vector3::new(10.0, 0.0, 0.0)); + dim.base.block_name = "*D0".to_string(); + let h = scene.add_entity(EntityType::Dimension(Dimension::Linear(dim))); + dov::set( + &mut scene.document, + h, + dov::DIMCLRD, + Some(XDataValue::Integer16(aci)), + ); + scene +} + +#[test] +fn leader_dim_line_color_survives_dwg_and_dxf() { + for ext in ["dwg", "dxf"] { + // ACI 1 (red): an indexed override. + let scene = leader_scene(1); + assert_eq!( + leader_color(&scene.document), + Some(Color::Index(1)), + "override not applied pre-save ({ext})" + ); + let re = roundtrip(&scene.document, ext); + assert_eq!( + leader_color(&re), + Some(Color::Index(1)), + "leader dim-line colour (ACI 1) lost across {ext} round-trip" + ); + + // ByLayer (256): an explicit override that must persist as ByLayer, not + // collapse to "no override". + let scene = leader_scene(256); + let re = roundtrip(&scene.document, ext); + assert_eq!( + leader_color(&re), + Some(Color::ByLayer), + "leader dim-line colour (ByLayer) lost across {ext} round-trip" + ); + } +} + +#[test] +fn dimension_dim_line_color_survives_dwg_and_dxf() { + for ext in ["dwg", "dxf"] { + let scene = dimension_scene(3); + assert_eq!( + dim_color(&scene.document), + Some(Color::Index(3)), + "override not applied pre-save ({ext})" + ); + let re = roundtrip(&scene.document, ext); + assert_eq!( + dim_color(&re), + Some(Color::Index(3)), + "dimension dim-line colour (ACI 3) lost across {ext} round-trip" + ); + } +}