diff --git a/src/app/command_driver/entity_commit.rs b/src/app/command_driver/entity_commit.rs index 2ff842393..85c3dfc19 100644 --- a/src/app/command_driver/entity_commit.rs +++ b/src/app/command_driver/entity_commit.rs @@ -1049,6 +1049,15 @@ impl OpenCADStudio { } } } else { + // Attributes are placed through the same base-point convention as + // the render path, so hand the command the definition's base point. + let block_base = crate::scene::render_graph::block_base_point( + &self.tabs[i].scene.document, + &block_name, + ); + if let Some(cmd) = self.tabs[i].active_cmd.as_mut() { + cmd.attreq_set_block_base(block_base); + } let completed = self.tabs[i] .active_cmd .as_mut() diff --git a/src/app/command_driver/tests.rs b/src/app/command_driver/tests.rs index ef27ca475..da8faf9f5 100644 --- a/src/app/command_driver/tests.rs +++ b/src/app/command_driver/tests.rs @@ -1479,3 +1479,278 @@ mod bedit_characterization_tests { ); } } + +#[cfg(test)] +mod block_base_point_tests { + use super::super::*; + use crate::command::{CadCommand, CmdResult}; + use crate::modules::insert::insert_block::InsertBlockCommand; + use codec::entities::AttributeDefinition; + use codec::types::Vector3; + use codec::EntityType; + + /// base-point fix (A), host side: `AttreqNeeded` is where the host resolves the + /// block's ATTDEFs, so it also injects the definition's base point. Without + /// that the committed attribute lands on the codec transform's position. + #[test] + fn locked_attreq_injects_the_block_base_point_for_attribute_placement() { + let mut app = OpenCADStudio::new_for_test(); + let _ = app.automation_op(r#"{"op":"new"}"#); + let i = app.active_tab; + // Block "ATTREQ_BP": base (10,5,0); LINE (10,5)-(30,5); ATTDEF TAG1 at + // (12,8). Rendering convention (see scene::block_base_point_tests): the + // ATTDEF lands at (100,100) + Rot(30)*Scale(2)*((12,8) - (10,5)) + // = (100,100) + 2*(1.732051 - 1.5, 1 + 2.598076) = (100.464102, 107.196152). + app.tabs[i].scene.define_block_raw( + "ATTREQ_BP", + Vector3::new(10.0, 5.0, 0.0), + vec![ + EntityType::Line(codec::entities::Line::from_points( + Vector3::new(10.0, 5.0, 0.0), + Vector3::new(30.0, 5.0, 0.0), + )), + EntityType::AttributeDefinition(AttributeDefinition { + insertion_point: Vector3::new(12.0, 8.0, 0.0), + height: 1.0, + ..AttributeDefinition::new( + "TAG1".to_string(), + "Tag one".to_string(), + String::new(), + ) + }), + ], + ); + + let mut cmd = InsertBlockCommand::classic_for_block( + "ATTREQ_BP".to_string(), + Vec::new(), + String::new(), + ); + let _ = cmd.on_point(glam::DVec3::new(100.0, 100.0, 0.0)); + assert!(matches!(cmd.on_text_input("2"), Some(CmdResult::NeedPoint))); + assert!(matches!(cmd.on_text_input("2"), Some(CmdResult::NeedPoint))); + assert!(matches!( + cmd.on_text_input("30"), + Some(CmdResult::AttreqNeeded { .. }) + )); + app.tabs[i].active_cmd = Some(Box::new(cmd)); + + let _ = app.apply_cmd_result(CmdResult::AttreqNeeded { + block_name: "ATTREQ_BP".to_string(), + }); + let result = app.tabs[i] + .active_cmd + .as_mut() + .expect("the INSERT command waits for the attribute value") + .on_text_input("VALUE1") + .expect("the value finishes the INSERT"); + let _ = app.apply_cmd_result(result); + + let attribute = app.tabs[i] + .scene + .document + .entities() + .find_map(|entity| match entity { + EntityType::Insert(ins) if ins.block_name == "ATTREQ_BP" => { + ins.attributes.first().cloned() + } + _ => None, + }) + .expect("the committed INSERT's attribute"); + assert_eq!(attribute.tag, "TAG1"); + let point = attribute.insertion_point; + assert!( + (point.x - 100.464102).abs() < 1e-6 + && (point.y - 107.196152).abs() < 1e-6 + && point.z.abs() < 1e-9, + "attribute at ({:.6},{:.6},{:.6}), expected (100.464102,107.196152,0)", + point.x, + point.y, + point.z + ); + } + + /// base-point fix (D), host side: picking an existing block in the Block + /// Definition dialog prefills the base point from that definition. + #[test] + fn locked_block_def_name_select_prefills_the_definition_base_point() { + let mut app = OpenCADStudio::new_for_test(); + let _ = app.automation_op(r#"{"op":"new"}"#); + let i = app.active_tab; + app.tabs[i].scene.define_block_raw( + "DEF_BASE", + Vector3::new(10.0, 5.0, 0.0), + vec![EntityType::Line(codec::entities::Line::from_points( + Vector3::new(10.0, 5.0, 0.0), + Vector3::new(30.0, 5.0, 0.0), + ))], + ); + app.block_definition = Some( + crate::ui::window::block_definition::BlockDefinitionState::new( + vec!["DEF_BASE".to_string()], + Vec::new(), + 0, + ), + ); + assert_eq!( + app.block_definition + .as_ref() + .map(|s| s.base_point_y.as_str()), + Some("0.0000"), + "a new block still starts at 0.0000" + ); + + let _ = app.update(Message::BlockDefNameSelect("DEF_BASE".to_string())); + + let state = app.block_definition.as_ref().expect("the dialog is open"); + assert_eq!(state.name, "DEF_BASE"); + assert_eq!(state.base_point_x, "10.0000"); + assert_eq!(state.base_point_y, "5.0000"); + assert_eq!(state.base_point_z, "0.0000"); + assert_eq!(state.parse_base_point(), glam::DVec3::new(10.0, 5.0, 0.0)); + } + + /// A block with base (10,5,0) and LINE (10,5)-(30,5), referenced by an + /// INSERT at (100,100) with 30° rotation and uniform scale 2. Under the + /// convention the LINE renders at (100,100)-(134.641016,120). + fn block_and_instance_with_base(app: &mut OpenCADStudio, name: &str) -> codec::Handle { + let _ = app.automation_op(r#"{"op":"new"}"#); + let i = app.active_tab; + app.tabs[i].scene.define_block_raw( + name, + Vector3::new(10.0, 5.0, 0.0), + vec![EntityType::Line(codec::entities::Line::from_points( + Vector3::new(10.0, 5.0, 0.0), + Vector3::new(30.0, 5.0, 0.0), + ))], + ); + let mut insert = + codec::entities::Insert::new(name.to_string(), Vector3::new(100.0, 100.0, 0.0)); + insert.set_x_scale(2.0); + insert.set_y_scale(2.0); + insert.set_z_scale(2.0); + insert.rotation = std::f64::consts::PI / 6.0; + app.tabs[i].scene.add_entity(EntityType::Insert(insert)) + } + + /// base-point follow-up fix: the REFEDIT working copy is placed through the + /// base-point-aware INSERT transform, so it sits exactly on the rendered + /// reference, and saving folds it back to the same block-local geometry. + #[test] + fn locked_refedit_working_copy_sits_on_the_rendered_reference() { + let mut app = OpenCADStudio::new_for_test(); + let handle = block_and_instance_with_base(&mut app, "REFEDIT_BP"); + let i = app.active_tab; + + let _ = app.dispatch_command(&format!("REFEDIT_BEGIN:{}", handle.value())); + assert!( + app.tabs[i].refedit_session.is_some(), + "setup: REFEDIT_BEGIN opens a session" + ); + + // The working copy is the LINE the session moved into model space. + let temp: Vec = app.tabs[i] + .refedit_session + .as_ref() + .expect("the session") + .temp_handles + .clone(); + let line = temp + .iter() + .find_map(|h| match app.tabs[i].scene.document.get_entity(*h) { + Some(EntityType::Line(line)) => Some(line.clone()), + _ => None, + }) + .expect("the working-copy LINE"); + // (100,100) + Rot(30)*2*((10,5)-(10,5)) and ...*((30,5)-(10,5)). + assert!( + (line.start.x - 100.0).abs() < 1e-6 + && (line.start.y - 100.0).abs() < 1e-9 + && (line.end.x - 134.641016).abs() < 1e-6 + && (line.end.y - 120.0).abs() < 1e-6, + "working copy at ({:.6},{:.6})-({:.6},{:.6}), expected (100,100)-(134.641016,120)", + line.start.x, + line.start.y, + line.end.x, + line.end.y + ); + + // Saving without editing restores the definition verbatim: the + // inverse is the exact inverse of the forward transform. + let _ = app.dispatch_command("REFCLOSE_SAVE"); + assert!(app.tabs[i].refedit_session.is_none()); + let stored = app.tabs[i] + .scene + .document + .block_records + .get("REFEDIT_BP") + .expect("the block record") + .entity_handles + .iter() + .find_map(|h| match app.tabs[i].scene.document.get_entity(*h) { + Some(EntityType::Line(line)) => Some(line.clone()), + _ => None, + }) + .expect("the stored LINE"); + assert!( + (stored.start.x - 10.0).abs() < 1e-9 + && (stored.start.y - 5.0).abs() < 1e-9 + && (stored.end.x - 30.0).abs() < 1e-9 + && (stored.end.y - 5.0).abs() < 1e-9, + "stored LINE at ({:.9},{:.9})-({:.9},{:.9}), expected (10,5)-(30,5)", + stored.start.x, + stored.start.y, + stored.end.x, + stored.end.y + ); + assert_eq!( + crate::scene::render_graph::block_base_point(&app.tabs[i].scene.document, "REFEDIT_BP"), + Vector3::new(10.0, 5.0, 0.0), + "the base point survives the round trip" + ); + } + + /// base-point follow-up fix: the dynamic-block visibility grip is a + /// block-definition coordinate, so it follows the base-point convention + /// too: (100,100) + Rot(30)*2*((12,3) - (10,5)) = (105.464102, 98.535898). + #[test] + fn locked_visibility_grip_follows_the_base_point_convention() { + let mut app = OpenCADStudio::new_for_test(); + let handle = block_and_instance_with_base(&mut app, "VIS_BP"); + let i = app.active_tab; + + let def_block = app.tabs[i] + .scene + .document + .block_records + .get("VIS_BP") + .expect("the block record") + .handle; + let mut param = codec::objects::BlockVisibilityParameter::default(); + param.owner = def_block; + param.def_point = Vector3::new(12.0, 3.0, 0.0); + app.tabs[i] + .scene + .document + .block_visibility_params + .insert(codec::Handle::new(0x70), param); + app.tabs[i].selected_handle = Some(handle); + + app.refresh_visibility_grip([0.0, 0.0, 0.0]); + + let grip = app.tabs[i] + .selected_grips + .last() + .expect("the visibility lookup grip"); + assert!( + (grip.world.x - 105.464102).abs() < 1e-6 && (grip.world.y - 98.535898).abs() < 1e-6, + "grip at ({:.6},{:.6}), expected (105.464102,98.535898)", + grip.world.x, + grip.world.y + ); + assert!( + app.tabs[i].visibility_grip.is_some(), + "the selection is a dynamic-block reference" + ); + } +} diff --git a/src/app/commands/inquiry.rs b/src/app/commands/inquiry.rs index 24bfd2a80..2d1ed0a4e 100644 --- a/src/app/commands/inquiry.rs +++ b/src/app/commands/inquiry.rs @@ -809,12 +809,19 @@ impl OpenCADStudio { }; // Build the INSERT's full placement transform (OCS + rotation + - // scale, including non-uniform / mirrored) and its inverse, so - // edits round-trip back to block-local coordinates on SAVE. + // scale, including non-uniform / mirrored, minus the block base + // point) and its inverse, so edits round-trip back to + // block-local coordinates on SAVE. The base point is part of + // the placement: without it the working copy sits Rot*Scale*base + // away from the reference the user picked. let sx = insert.x_scale(); let sy = insert.y_scale(); let sz = insert.z_scale(); - let forward = insert.get_transform(); + let base = crate::scene::render_graph::block_base_point( + &self.tabs[i].scene.document, + &insert.block_name, + ); + let forward = crate::scene::render_graph::insert_transform_with_base(&insert, base); let inverse = { use codec::types::{Matrix3, Matrix4, Transform}; let ocs_t = @@ -826,8 +833,11 @@ impl OpenCADStudio { ); let r_inv = Matrix4::rotation_z(-insert.rotation); let s_inv = Matrix4::scaling(1.0 / sx, 1.0 / sy, 1.0 / sz); - // inverse(OCS·T·R·S) = S⁻¹·R⁻¹·T⁻¹·OCSᵀ - Transform::from_matrix(s_inv * r_inv * t_inv * ocs_t) + // inverse(T(-base)·OCS·T·R·S) = T(+base)·S⁻¹·R⁻¹·T⁻¹·OCSᵀ + Transform::from_matrix( + Matrix4::translation(base.x, base.y, base.z) + * (s_inv * r_inv * t_inv * ocs_t), + ) }; // Find the block record. diff --git a/src/app/commands/sheet_set.rs b/src/app/commands/sheet_set.rs index a0af7cf63..3da99868d 100644 --- a/src/app/commands/sheet_set.rs +++ b/src/app/commands/sheet_set.rs @@ -2269,8 +2269,8 @@ impl OpenCADStudio { let host = self.tabs[i].scene.document.header.insertion_units; let k = crate::app::properties::insert_unit_scale(host, units).unwrap_or(1.0); let mut insert = codec::entities::Insert::new(name.to_string(), at).with_scale(k, k, k); - let transform = insert.get_transform(); let doc = &self.tabs[i].scene.document; + let transform = crate::scene::render_graph::insert_transform(doc, &insert); let attdefs: Vec = doc .block_records .get(name) diff --git a/src/app/commands/xclip.rs b/src/app/commands/xclip.rs index f1665acbe..ee9b23ab7 100644 --- a/src/app/commands/xclip.rs +++ b/src/app/commands/xclip.rs @@ -6,13 +6,14 @@ use crate::scene::pick::xclip as clip; impl OpenCADStudio { /// A block reference's extents in WCS: its block's objects through the - /// insert transform. + /// insert transform — the base-point-aware one, because that is where the + /// renderer puts the contents the user drew the boundary around. fn insert_extents(&self, i: usize, insert: codec::Handle) -> ([f64; 2], [f64; 2]) { let document = &self.tabs[i].scene.document; let Some(codec::EntityType::Insert(ins)) = document.get_entity(insert) else { return ([0.0; 2], [0.0; 2]); }; - let xform = ins.get_transform(); + let xform = crate::scene::render_graph::insert_transform(document, ins); let (mut lo, mut hi) = ([f64::MAX; 2], [f64::MIN; 2]); for e in document.entities_in_block(&ins.block_name) { let (a, b) = crate::scene::convert::tess::entity_bounds_in(document, e); @@ -77,7 +78,14 @@ impl OpenCADStudio { else { continue; }; - let outline = clip::clip_outline_world(doc, filter, &ins.get_transform()); + // The same base-point-aware transform the spatial filter + // was stored with, so the generated polyline lands on the + // boundary the user drew (the two cancel exactly). + let outline = clip::clip_outline_world( + doc, + filter, + &crate::scene::render_graph::insert_transform(doc, ins), + ); let mut pl = codec::entities::LwPolyline::from_points( outline .iter() diff --git a/src/app/update/mod.rs b/src/app/update/mod.rs index 0ab8654a2..00ff5c49e 100644 --- a/src/app/update/mod.rs +++ b/src/app/update/mod.rs @@ -5017,9 +5017,16 @@ impl OpenCADStudio { } Message::BlockDefNameSelect(chosen) => { if let Some(state) = self.block_definition.as_mut() { - state.name = chosen; + state.name = chosen.clone(); state.error_message = None; state.confirm_redefine = None; + // Redefining an existing block starts from the base point + // that definition already has (0.0000 for a new block). + let base = crate::scene::render_graph::block_base_point( + &self.tabs[self.active_tab].scene.document, + &chosen, + ); + state.set_base_point(glam::DVec3::new(base.x, base.y, base.z)); } Task::none() } diff --git a/src/app/visibility.rs b/src/app/visibility.rs index c290b45a0..f3d8504ca 100644 --- a/src/app/visibility.rs +++ b/src/app/visibility.rs @@ -136,7 +136,10 @@ impl OpenCADStudio { return; }; - let wp = ins.get_transform().apply(Vector3::new( + // `def_point` is a block-definition coordinate (see the codec's + // BlockVisibilityParameter), so the grip follows the same base-point + // convention as the rendered reference. + let wp = crate::scene::render_graph::insert_transform(doc, ins).apply(Vector3::new( param.def_point.x, param.def_point.y, param.def_point.z, diff --git a/src/command.rs b/src/command.rs index fbe52867d..54efd1035 100644 --- a/src/command.rs +++ b/src/command.rs @@ -3035,6 +3035,11 @@ pub trait CadCommand: Send { None } + /// Supply the referenced block's insertion base point so the attributes + /// the command now owns are placed through the same base-point convention + /// as the render path. Default: no-op (no base point). + fn attreq_set_block_base(&mut self, _base: codec::types::Vector3) {} + /// After the host commits an ATTREQ insert: keep the command running /// (repeated placement) instead of ending it. fn attreq_continue(&self) -> bool { diff --git a/src/modules/draw/modify/explode.rs b/src/modules/draw/modify/explode.rs index cd2e4dfa7..ba11835a4 100644 --- a/src/modules/draw/modify/explode.rs +++ b/src/modules/draw/modify/explode.rs @@ -23,8 +23,8 @@ use codec::entities::{ }; use codec::entities::{Polyline, Polyline2D}; use codec::tables::BlockRecord; -use codec::types::Vector3; -use codec::{CadDocument, EntityType, Handle}; +use codec::types::{Transform, Vector3}; +use codec::{CadDocument, Entity, EntityType, Handle}; use crate::command::{CadCommand, CmdResult, WorkingPlane}; use crate::entities::curve::lwpolyline_world_xy; @@ -106,7 +106,24 @@ pub fn explode_entity(entity: &EntityType, document: &CadDocument) -> Vec = document + .entities_in_block(&ins.block_name) + .map(|entity| { + let mut shifted = entity.clone(); + Entity::apply_transform(shifted.as_entity_mut(), &to_block); + shifted + }) + .collect(); + ins.explode(&entities) .into_iter() .map(normalize_insert_entity) .collect() diff --git a/src/modules/insert/insert_block.rs b/src/modules/insert/insert_block.rs index 411eaefeb..b9e19c473 100644 --- a/src/modules/insert/insert_block.rs +++ b/src/modules/insert/insert_block.rs @@ -126,6 +126,10 @@ pub struct InsertBlockCommand { rotation: f64, pending_insert: Option, plane: WorkingPlane, + /// The referenced block's insertion base point. Attributes are placed + /// through the same base-point convention the render path uses, so the + /// host injects it when it resolves the block's ATTDEFs. + block_base: Vector3, } impl InsertBlockCommand { @@ -152,6 +156,7 @@ impl InsertBlockCommand { rotation: 0.0, pending_insert: None, plane: WorkingPlane::default(), + block_base: Vector3::ZERO, } } @@ -852,9 +857,18 @@ impl CadCommand for InsertBlockCommand { fn attreq_take_insert(&mut self) -> Option { self.pending_insert.take().map(EntityType::Insert) } + + fn attreq_set_block_base(&mut self, base: Vector3) { + self.set_block_base(base); + } } impl InsertBlockCommand { + /// The referenced block's insertion base point, for attribute placement. + pub fn set_block_base(&mut self, base: Vector3) { + self.block_base = base; + } + fn answer(&mut self, ask: Ask, text: Option<&str>) -> CmdResult { let default = !matches!(ask, Ask::RepeatScale | Ask::RepeatRotate); let Some(yes) = Self::yes_no(text.unwrap_or(""), default) else { @@ -955,7 +969,8 @@ impl InsertBlockCommand { _ => return None, }; let mut insert = self.pending_insert.take()?; - let transform = insert.get_transform(); + let transform = + crate::scene::render_graph::insert_transform_with_base(&insert, self.block_base); for (attdef_idx, value) in values { let Some(attdef) = attdefs.get(attdef_idx) else { continue; diff --git a/src/scene/block_base_point_tests.rs b/src/scene/block_base_point_tests.rs new file mode 100644 index 000000000..d5a501d1f --- /dev/null +++ b/src/scene/block_base_point_tests.rs @@ -0,0 +1,1038 @@ +//! Base-point reproduction. Tests only; no production code changes. +//! +//! Convention under test: +//! world = insert_point + Rot(rotation) * Scale * (P - block.base_point) +//! +//! Block "BP_TEST": base (10,5,0); LINE (10,5,0)-(30,5,0); +//! CIRCLE center (20,15,0) r=3; ATTDEF tag TAG1 at (12,8,0). +//! INSERT at (100,100,0), rotation 30 degrees, uniform scale 2. +//! +//! Hand derivation (c = cos30 = 0.8660254038, s = sin30 = 0.5, k = scale 2): +//! Rot(30)*Scale maps (x,y) to (k*(c*x - s*y), k*(s*x + c*y)). +//! LINE start (10,5)-base = (0,0) -> (100, 100) +//! LINE end (30,5)-base = (20,0) -> 100 + (2*17.320508, 2*10) +//! = (134.641016, 120) +//! CIRCLE center (20,15)-base = (10,10) -> 100 + (2*(8.660254-5), 2*(5+8.660254)) +//! = (107.320508, 127.320508) +//! CIRCLE radius 3 * scale 2 -> 6 +//! ATTDEF (12,8)-base = (2,3) -> 100 + (2*(1.732051-1.5), 2*(1+2.598076)) +//! = (100.464102, 107.196152) +//! +//! Offset between the codec's `Insert::get_transform` (no base subtraction) +//! and the app's `insert_transform` (base-subtracting), measured at the block's +//! base point, is exactly Rot(30)*Scale*base(10,5,0): +//! (2*(c*10 - s*5), 2*(s*10 + c*5)) = (12.320508, 18.660254, 0) +//! +//! Naming: `locked_*` = behaviour to keep; `documenting_*` = behaviour expected +//! to change. Tests that fail today carry +//! `#[ignore = "base-point repro: fails today"]`. + +use crate::command::{CadCommand, CmdResult}; +use crate::modules::insert::insert_block::InsertBlockCommand; +use crate::scene::render_graph::{ + block_base_point, insert_transform, BlockRoot, BlockRootRole, RenderSceneGraph, SceneRoot, +}; +use crate::scene::{CreateBlockOptions, Scene}; +use crate::ui::window::block_definition::{BlockDefinitionState, BlockObjectMode}; +use codec::entities::{AttributeDefinition, AttributeEntity, Circle, Insert, Line}; +use codec::types::{Handle, Matrix4, Transform, Vector3}; +use codec::{CadDocument, EntityType}; +use glam::DVec3; + +const BP_TEST: &str = "BP_TEST"; +const DEG30: f64 = std::f64::consts::PI / 6.0; + +/// Expected world positions, hand-derived from the convention above. +pub const EXPECTED_LINE_START: [f64; 3] = [100.0, 100.0, 0.0]; +pub const EXPECTED_LINE_END: [f64; 3] = [134.641016, 120.0, 0.0]; +pub const EXPECTED_CIRCLE_CENTER: [f64; 3] = [107.320508, 127.320508, 0.0]; +pub const EXPECTED_ATTR_POINT: [f64; 3] = [100.464102, 107.196152, 0.0]; +/// `Rot(30)*Scale(2)*base(10,5,0)` — the gap between the two transform paths. +pub const GET_TO_INSERT_OFFSET: [f64; 3] = [12.320508, 18.660254, 0.0]; +/// Step-1 geometry pushed through the codec transform (no base subtraction) — +/// the pre-fix position, kept for the picking test's negative case. +pub const CODEC_LINE_END: [f64; 3] = [146.961524, 138.660254, 0.0]; + +fn v(x: f64, y: f64, z: f64) -> Vector3 { + Vector3::new(x, y, z) +} + +fn assert_close(actual: Vector3, expected: [f64; 3], tol: f64, label: &str) { + let delta = [ + (actual.x - expected[0]).abs(), + (actual.y - expected[1]).abs(), + (actual.z - expected[2]).abs(), + ]; + assert!( + delta.iter().all(|d| *d <= tol), + "{label}: actual=({:.6},{:.6},{:.6}) expected=({:.6},{:.6},{:.6})", + actual.x, + actual.y, + actual.z, + expected[0], + expected[1], + expected[2] + ); +} + +/// Two points must agree — used where both sides are computed by the app. +fn assert_same(actual: Vector3, expected: Vector3, tol: f64, label: &str) { + assert!( + (actual - expected).length() <= tol, + "{label}: actual=({:.9},{:.9},{:.9}) expected=({:.9},{:.9},{:.9})", + actual.x, + actual.y, + actual.z, + expected.x, + expected.y, + expected.z + ); +} + +fn attdef(tag: &str) -> AttributeDefinition { + AttributeDefinition { + insertion_point: v(12.0, 8.0, 0.0), + height: 1.0, + ..AttributeDefinition::new(tag.to_string(), "Tag one".to_string(), String::new()) + } +} + +fn block_entities() -> Vec { + vec![ + EntityType::Line(Line::from_points(v(10.0, 5.0, 0.0), v(30.0, 5.0, 0.0))), + EntityType::Circle(Circle::from_coords(20.0, 15.0, 0.0, 3.0)), + EntityType::AttributeDefinition(attdef("TAG1")), + ] +} + +/// A scene holding "BP_TEST": base (10,5,0) plus the LINE/CIRCLE/ATTDEF above. +fn scene_with_bp_test() -> Scene { + let mut scene = Scene::new(); + scene.define_block_raw(BP_TEST, v(10.0, 5.0, 0.0), block_entities()); + scene +} + +/// The LINE stored in `block`, in block-local coordinates. +fn block_line(document: &CadDocument, block: &str) -> Line { + document + .block_records + .get(block) + .unwrap() + .entity_handles + .iter() + .find_map(|h| match document.get_entity(*h) { + Some(EntityType::Line(line)) => Some(line.clone()), + _ => None, + }) + .expect("the block's LINE") +} + +fn block_attdefs(document: &CadDocument, block: &str) -> Vec { + let Some(br) = document.block_records.get(block) else { + return Vec::new(); + }; + br.entity_handles + .iter() + .filter_map(|h| match document.get_entity(*h) { + Some(EntityType::AttributeDefinition(ad)) => Some(ad.clone()), + _ => None, + }) + .collect() +} + +/// The model-space INSERT referencing `block`. +fn instance_of(scene: &Scene, block: &str) -> Insert { + scene + .document + .entities() + .find_map(|e| match e { + EntityType::Insert(ins) if ins.block_name == block => Some(ins.clone()), + _ => None, + }) + .expect("the placed INSERT") +} + +/// World positions of the instance's LINE, as the scene computes them. +fn instance_line_world(scene: &Scene, block: &str) -> (Vector3, Vector3) { + let line = block_line(&scene.document, block); + let insert = instance_of(scene, block); + let transform = insert_transform(&scene.document, &insert); + (transform.apply(line.start), transform.apply(line.end)) +} + +/// Drive the real `-INSERT` command through its prompt sequence: insertion +/// point, X scale, Y scale, rotation, then the attribute fill — the same code +/// path the INSERT command uses. The host injects the definition's base point +/// when it resolves the block's ATTDEFs (`handle_attreq_needed` in +/// app/command_driver/entity_commit.rs), which is what makes attribute +/// placement follow the render path's convention. +fn insert_via_command(scene: &mut Scene) -> Insert { + let mut cmd = + InsertBlockCommand::classic_for_block(BP_TEST.to_string(), Vec::new(), String::new()); + let _ = cmd.on_point(DVec3::new(100.0, 100.0, 0.0)); + assert!(matches!(cmd.on_text_input("2"), Some(CmdResult::NeedPoint))); + assert!(matches!(cmd.on_text_input("2"), Some(CmdResult::NeedPoint))); + assert!(matches!( + cmd.on_text_input("30"), + Some(CmdResult::AttreqNeeded { .. }) + )); + let attdefs = block_attdefs(&scene.document, BP_TEST); + assert_eq!(attdefs.len(), 1, "one ATTDEF is expected in the block"); + cmd.set_block_base(block_base_point(&scene.document, BP_TEST)); + assert!(cmd.attreq_set_attdefs(attdefs).is_none()); + match cmd.on_text_input("VALUE1") { + Some(CmdResult::CommitAndExit(EntityType::Insert(insert))) + | Some(CmdResult::CommitEntity(EntityType::Insert(insert))) => { + scene.add_entity(EntityType::Insert(insert.clone())); + insert + } + _ => panic!("the command did not commit an INSERT"), + } +} + +fn placed_instance(scene: &mut Scene) -> Insert { + insert_via_command(scene) +} + +/// Walk the render graph from model space, collecting (entity, instance +/// transform) pairs — the traversal the renderer itself uses. +fn render_graph_leaves(scene: &Scene) -> Vec<(EntityType, Transform)> { + let model_space = scene + .document + .block_records + .iter() + .find(|record| record.is_model_space()) + .map(|record| record.handle) + .expect("a model space block record"); + let depths = rustc_hash::FxHashMap::default(); + let graph = RenderSceneGraph::new(&scene.document, None, None, true, &depths); + let mut leaves = Vec::new(); + graph.walk_root( + SceneRoot::Block(BlockRoot { + record: model_space, + role: BlockRootRole::ModelSpace, + }), + |_, _| true, + |entity, context| leaves.push((entity.clone(), context.transform)), + ); + leaves +} + +/// A view where world coordinates equal screen coordinates, so a click at a +/// world point can be named directly. +struct WorldAsScreen { + view_rot: glam::Mat4, + eye: DVec3, + bounds: iced::Rectangle, +} + +impl WorldAsScreen { + fn new(extent: f32) -> Self { + Self { + view_rot: glam::Mat4::from_scale(glam::Vec3::new(2.0 / extent, -2.0 / extent, 1.0)), + eye: DVec3::new(extent as f64 / 2.0, extent as f64 / 2.0, 0.0), + bounds: iced::Rectangle::new(iced::Point::ORIGIN, iced::Size::new(extent, extent)), + } + } + + fn at(&self, point: [f64; 3]) -> iced::Point { + iced::Point::new(point[0] as f32, point[1] as f32) + } +} + +fn click_at( + point: [f64; 3], + view: &WorldAsScreen, + wires: &[crate::scene::model::wire_model::WireModel], + depths: &rustc_hash::FxHashMap, +) -> Option { + crate::scene::pick::hit_test::click_hit( + view.at(point), + wires, + view.view_rot, + view.eye, + view.bounds, + false, + 8.0, + depths, + ) + .and_then(Scene::handle_from_wire_name) +} + +#[test] +fn locked_block_base_point_is_stored_on_the_definition() { + let scene = scene_with_bp_test(); + assert_close( + block_base_point(&scene.document, BP_TEST), + [10.0, 5.0, 0.0], + 1e-9, + "block base point", + ); +} + +#[test] +fn locked_insert_command_builds_the_step1_reference() { + let mut scene = scene_with_bp_test(); + let insert = insert_via_command(&mut scene); + assert_close( + insert.insert_point, + [100.0, 100.0, 0.0], + 1e-9, + "insert point", + ); + assert_eq!( + (insert.x_scale(), insert.y_scale(), insert.z_scale()), + (2.0, 2.0, 2.0) + ); + assert!((insert.rotation.to_degrees() - 30.0).abs() < 1e-9); + let attribute = insert.attributes.first().expect("TAG1 is filled"); + assert_eq!(attribute.tag, "TAG1"); +} + +#[test] +fn locked_render_path_subtracts_the_block_base_point() { + let mut scene = scene_with_bp_test(); + let insert = placed_instance(&mut scene); + let transform = insert_transform(&scene.document, &insert); + assert_close( + transform.apply(v(10.0, 5.0, 0.0)), + EXPECTED_LINE_START, + 1e-6, + "line start", + ); + assert_close( + transform.apply(v(30.0, 5.0, 0.0)), + EXPECTED_LINE_END, + 1e-6, + "line end", + ); + assert_close( + transform.apply(v(20.0, 15.0, 0.0)), + EXPECTED_CIRCLE_CENTER, + 1e-6, + "circle center", + ); +} + +#[test] +fn locked_render_graph_instances_at_convention_positions() { + let mut scene = scene_with_bp_test(); + let _ = placed_instance(&mut scene); + let leaves = render_graph_leaves(&scene); + let line = leaves + .iter() + .find_map(|(entity, _)| match entity { + EntityType::Line(line) => Some(line.clone()), + _ => None, + }) + .expect("the rendered LINE instance"); + let transform = leaves + .iter() + .find(|(entity, _)| matches!(entity, EntityType::Line(_))) + .map(|(_, transform)| *transform) + .unwrap(); + assert_close( + transform.apply(line.start), + EXPECTED_LINE_START, + 1e-6, + "render graph: line start", + ); + assert_close( + transform.apply(line.end), + EXPECTED_LINE_END, + 1e-6, + "render graph: line end", + ); +} + +#[test] +fn locked_hit_test_hits_the_base_point_adjusted_line() { + let mut scene = scene_with_bp_test(); + let _ = placed_instance(&mut scene); + let wires = scene.hit_test_wires(); + let depth_map = scene.draw_depth_map(); + let view = WorldAsScreen::new(2000.0); + let insert_handle = instance_of(&scene, BP_TEST).common.handle; + + // Mid-point of the LINE under the convention. + let convention_mid = [ + (EXPECTED_LINE_START[0] + EXPECTED_LINE_END[0]) / 2.0, + (EXPECTED_LINE_START[1] + EXPECTED_LINE_END[1]) / 2.0, + 0.0, + ]; + let hit = click_at(convention_mid, &view, wires.as_slice(), depth_map.as_ref()); + assert_eq!( + hit, + Some(insert_handle), + "clicking the convention position must select the INSERT" + ); + + // Mid-point of the LINE under the codec transform (base not subtracted). + let codec_mid = [ + (EXPECTED_LINE_END[0] + CODEC_LINE_END[0]) / 2.0, + (EXPECTED_LINE_END[1] + CODEC_LINE_END[1]) / 2.0, + 0.0, + ]; + let hit = click_at(codec_mid, &view, wires.as_slice(), depth_map.as_ref()); + assert_eq!( + hit, None, + "clicking the codec position must not hit anything" + ); +} + +#[test] +fn locked_attribute_placement_matches_the_convention() { + // base-point: the host now injects the definition's base point into the INSERT + // command, so `finish_insert` places the attribute through + // `insert_transform_with_base` — the same convention as the render path. + // Hand derivation (see the module header): ATTDEF (12,8) - base (10,5) = + // (2,3); Rot(30)*Scale(2)*(2,3) = (2*(1.732051-1.5), 2*(1+2.598076)) = + // (0.464102, 7.196152); + insert point (100,100) = (100.464102, 107.196152). + let mut scene = scene_with_bp_test(); + let insert = placed_instance(&mut scene); + let attribute = insert.attributes.first().expect("TAG1"); + assert_close( + attribute.insertion_point, + EXPECTED_ATTR_POINT, + 1e-6, + "attribute insertion point", + ); + // And it is exactly the render path's placement of the ATTDEF. + let attdef = block_attdefs(&scene.document, BP_TEST) + .into_iter() + .next() + .expect("TAG1"); + let render = insert_transform(&scene.document, &insert).apply(attdef.insertion_point); + assert_close( + attribute.insertion_point, + [render.x, render.y, render.z], + 1e-9, + "attribute vs render path", + ); +} + +#[test] +fn documenting_get_transform_offset_from_insert_transform() { + // Ran and observed: the two paths differ by exactly Rot(30)*Scale(2)*base + // for the step-1 case, measured at the block's own base point. + let mut scene = scene_with_bp_test(); + let insert = placed_instance(&mut scene); + let p = v(10.0, 5.0, 0.0); + let render = insert_transform(&scene.document, &insert).apply(p); + let codec = insert.get_transform().apply(p); + let delta = render - codec; + for (actual, expected) in [delta.x, delta.y, delta.z].iter().zip(GET_TO_INSERT_OFFSET) { + assert!((actual.abs() - expected).abs() < 1e-6, "offset {delta:?}"); + } +} + +#[test] +fn locked_creation_with_a_picked_base_bakes_the_origin_into_the_geometry() { + // base-point fix (B): the caller bakes the picked base into `world_to_block`, so + // the contents are stored relative to it and the definition's own base + // point stays ZERO — the picked base becomes the block origin. Storing the + // base in the definition as well would subtract it twice. The geometry of + // existing / imported definitions is never touched. + const NAME: &str = "BP_CREATE_BASE"; + let mut scene = Scene::new(); + let line = scene.add_entity(EntityType::Line(Line::from_points( + v(10.0, 5.0, 0.0), + v(30.0, 5.0, 0.0), + ))); + let options = CreateBlockOptions { + name: NAME.to_string(), + handles: vec![line], + base_point: DVec3::new(10.0, 5.0, 0.0), + world_to_block: Transform::from_translation(v(-10.0, -5.0, 0.0)), + block_to_world: Transform::from_translation(v(10.0, 5.0, 0.0)), + mode: BlockObjectMode::Retain, + annotative: false, + match_orientation: false, + scale_uniformly: false, + allow_exploding: true, + unit: 0, + description: String::new(), + hyperlink_url: String::new(), + hyperlink_desc: String::new(), + redefine: false, + }; + assert!(scene.create_block_with_options(options).is_ok()); + + // The stored LINE is the drawn LINE translated by -(10,5): (0,0)-(20,0). + let stored = block_line(&scene.document, NAME); + assert_close(stored.start, [0.0, 0.0, 0.0], 1e-9, "stored line start"); + assert_close(stored.end, [20.0, 0.0, 0.0], 1e-9, "stored line end"); + // The definition's base point stays ZERO, so the picked base — now the + // block origin — is what an insertion point lands on. + assert_close( + block_base_point(&scene.document, NAME), + [0.0, 0.0, 0.0], + 1e-9, + "stored base point after create_block_with_options", + ); + // Inserted at (100,100) with no rotation or scale the LINE therefore lands + // at (100,100)-(120,100): the picked base was honoured on creation. + let _ = scene.add_entity(EntityType::Insert(Insert::new( + NAME.to_string(), + v(100.0, 100.0, 0.0), + ))); + let (start, end) = instance_line_world(&scene, NAME); + assert_close(start, [100.0, 100.0, 0.0], 1e-6, "instance line start"); + assert_close(end, [120.0, 100.0, 0.0], 1e-6, "instance line end"); +} + +#[test] +fn locked_redefine_with_a_changed_base_moves_the_instance() { + // base-point fix (C). Step 3: create through the dialog's options with a + // non-zero base, insert it, then redefine it through the + // `redefine_block_raw` call dialog.rs makes, with a different base. A + // changed base is allowed to move instances — the contents stay put in + // block coordinates while the base they are measured from moves. + const NAME: &str = "BP_REDEF_MOVED"; + let mut scene = Scene::new(); + let line = scene.add_entity(EntityType::Line(Line::from_points( + v(10.0, 5.0, 0.0), + v(30.0, 5.0, 0.0), + ))); + let options = CreateBlockOptions { + name: NAME.to_string(), + handles: vec![line], + base_point: DVec3::new(10.0, 5.0, 0.0), + world_to_block: Transform::from_translation(v(-10.0, -5.0, 0.0)), + block_to_world: Transform::from_translation(v(10.0, 5.0, 0.0)), + mode: BlockObjectMode::Retain, + annotative: false, + match_orientation: false, + scale_uniformly: false, + allow_exploding: true, + unit: 0, + description: String::new(), + hyperlink_url: String::new(), + hyperlink_desc: String::new(), + redefine: false, + }; + assert!(scene.create_block_with_options(options).is_ok()); + + let mut insert = Insert::new(NAME.to_string(), v(100.0, 100.0, 0.0)); + insert.set_x_scale(2.0); + insert.set_y_scale(2.0); + insert.set_z_scale(2.0); + insert.rotation = DEG30; + let instance = scene.add_entity(EntityType::Insert(insert)); + + // Block-local LINE is world (10,5)-(30,5) shifted by -(10,5) = (0,0)-(20,0), + // and the stored base point is (0,0,0), so the instance renders at + // (100,100) and (134.641016, 120). + let (start, end) = instance_line_world(&scene, NAME); + assert_close( + start, + [100.0, 100.0, 0.0], + 1e-6, + "line start before redefine", + ); + assert_close( + end, + [134.641016, 120.0, 0.0], + 1e-6, + "line end before redefine", + ); + + // The redefine path: the same contents, but a new base point (0,20,0). + scene.redefine_block_raw( + NAME, + v(0.0, 20.0, 0.0), + vec![EntityType::Line(Line::from_points( + v(0.0, 0.0, 0.0), + v(20.0, 0.0, 0.0), + ))], + ); + assert_close( + block_base_point(&scene.document, NAME), + [0.0, 20.0, 0.0], + 1e-9, + "stored base point after redefine", + ); + + // The contents did not move, so every instance moves by exactly + // Rot(30)*Scale(2)*(newBase - oldBase) = Rot(30)*2*((0,20) - (0,0)) + // = 2*(0.866025*0 - 0.5*20, 0.5*0 + 0.866025*20) = (20, -34.641016). + let (start_after, end_after) = instance_line_world(&scene, NAME); + // start: (100,100) + Rot(30)*2*((0,0) - (0,20)) = (120, 65.358984) + assert_close( + start_after, + [120.0, 65.358984, 0.0], + 1e-6, + "line start after redefine", + ); + // end: (100,100) + Rot(30)*2*((20,0) - (0,20)) + // = (100,100) + 2*(17.320508 + 10, 10 - 17.320508) = (154.641016, 85.358984) + assert_close( + end_after, + [154.641016, 85.358984, 0.0], + 1e-6, + "line end after redefine", + ); + let jump = Vector3::new( + start_after.x - start.x, + start_after.y - start.y, + start_after.z - start.z, + ); + assert_close(jump, [20.0, -34.641016, 0.0], 1e-6, "instance movement"); + assert!( + (jump.x.abs() + jump.y.abs()) > 1.0, + "the instance moved by {jump:?}" + ); + let _ = instance; +} + +#[test] +fn locked_noop_redefine_leaves_the_instance_where_it_is() { + // base-point fix (C): redefining a block with the same contents and the same + // base point must not move any instance — the definition is replaced + // verbatim. + const NAME: &str = "BP_REDEF_NOOP"; + let mut scene = Scene::new(); + let base = v(6.0, 4.0, 0.0); + scene.define_block_raw( + NAME, + base, + vec![EntityType::Line(Line::from_points( + v(6.0, 4.0, 0.0), + v(16.0, 4.0, 0.0), + ))], + ); + let mut insert = Insert::new(NAME.to_string(), v(50.0, 20.0, 0.0)); + insert.set_x_scale(1.5); + insert.set_y_scale(1.5); + insert.set_z_scale(1.5); + insert.rotation = DEG30; + let _ = scene.add_entity(EntityType::Insert(insert)); + + // Render path before: (50,20) + Rot(30)*1.5*((6,4)-(6,4)) = (50,20) and + // (50,20) + Rot(30)*1.5*((16,4)-(6,4)) = (50,20) + 1.5*Rot(30)*(10,0) + // = (50,20) + 1.5*(8.660254, 5) = (62.990381, 27.5). + let (start, end) = instance_line_world(&scene, NAME); + assert_close(start, [50.0, 20.0, 0.0], 1e-6, "line start before"); + assert_close(end, [62.990381, 27.5, 0.0], 1e-6, "line end before"); + + // No-op redefine: identical contents, same base. + scene.redefine_block_raw( + NAME, + base, + vec![EntityType::Line(Line::from_points( + v(6.0, 4.0, 0.0), + v(16.0, 4.0, 0.0), + ))], + ); + assert_close( + block_base_point(&scene.document, NAME), + [6.0, 4.0, 0.0], + 1e-9, + "base point after no-op redefine", + ); + let (start_after, end_after) = instance_line_world(&scene, NAME); + assert_same(start_after, start, 1e-12, "line start after no-op redefine"); + assert_same(end_after, end, 1e-12, "line end after no-op redefine"); +} + +#[test] +fn locked_block_definition_dialog_prefills_the_existing_definition_base_point() { + // base-point fix (D): choosing an existing block in the Block Definition dialog + // prefills the base point from that definition, so a redefine starts from + // the base it already has. A new block still starts at 0.0000. + let scene = scene_with_bp_test(); + assert_close( + block_base_point(&scene.document, BP_TEST), + [10.0, 5.0, 0.0], + 1e-9, + "the drawing's block base point", + ); + + // A new block: the fields stay at 0.0000. + let fresh = BlockDefinitionState::new(vec![BP_TEST.to_string()], Vec::new(), 0); + assert_eq!(fresh.base_point_x, "0.0000"); + assert_eq!(fresh.base_point_y, "0.0000"); + assert_eq!(fresh.base_point_z, "0.0000"); + assert_eq!(fresh.parse_base_point(), DVec3::ZERO); + + // Picking the existing block (what `Message::BlockDefNameSelect` does). + let mut state = fresh; + let base = block_base_point(&scene.document, BP_TEST); + state.set_base_point(DVec3::new(base.x, base.y, base.z)); + assert_eq!(state.base_point_x, "10.0000"); + assert_eq!(state.base_point_y, "5.0000"); + assert_eq!(state.base_point_z, "0.0000"); + assert_eq!(state.parse_base_point(), DVec3::new(10.0, 5.0, 0.0)); +} + +#[test] +fn locked_explode_places_geometry_at_convention_positions() { + // base-point: `explode_entity` shifts the definition's contents by -base before + // handing them to the codec's explode, so the pieces land where the render + // path puts them (see the module header for the derivation). + let mut scene = scene_with_bp_test(); + let insert = placed_instance(&mut scene); + let pieces = crate::modules::draw::modify::explode::explode_entity( + &EntityType::Insert(insert), + &scene.document, + ); + let mut line_start = None; + let mut line_end = None; + let mut circle_center = None; + for piece in pieces { + match piece { + EntityType::Line(line) => { + if line_end.is_none() { + line_start = Some(line.start); + line_end = Some(line.end); + } + } + EntityType::Circle(circle) => circle_center = Some(circle.center), + EntityType::Ellipse(ellipse) => circle_center = Some(ellipse.center), + _ => {} + } + } + assert_close( + line_start.expect("the exploded LINE"), + EXPECTED_LINE_START, + 1e-6, + "exploded line start", + ); + assert_close( + line_end.expect("the exploded LINE"), + EXPECTED_LINE_END, + 1e-6, + "exploded line end", + ); + assert_close( + circle_center.expect("the exploded CIRCLE"), + EXPECTED_CIRCLE_CENTER, + 1e-6, + "exploded circle center", + ); + // Same numbers as the render path: EXPLODE and the renderer agree. + let (render_start, render_end) = instance_line_world(&scene, BP_TEST); + assert_close(render_end, EXPECTED_LINE_END, 1e-6, "render path: line end"); + assert_close( + render_start, + EXPECTED_LINE_START, + 1e-6, + "render path: line start", + ); +} + +#[test] +fn locked_xclip_boundary_round_trips_through_the_base_point_convention() { + // base-point follow-up fix: the XCLIP ring is stored as WCS points plus the + // inverse of the transform every consumer composes it back with. That + // consumer is the base-point-aware instance transform, so the stored + // inverse has to be the base-aware one; a base-ignoring inverse displaces + // the ring by exactly Rot(30)*Scale(2)*base = (12.320508, 18.660254). + const NAME: &str = "BP_XCLIP"; + let mut scene = Scene::new(); + // Same fixture as BP_TEST: base (10,5,0), LINE (10,5)-(30,5) local + // (0,0)-(20,0), INSERT at (100,100) with 30° and scale 2, so the rendered + // LINE runs (100,100)-(134.641016,120). + scene.define_block_raw( + NAME, + v(10.0, 5.0, 0.0), + vec![EntityType::Line(Line::from_points( + v(10.0, 5.0, 0.0), + v(30.0, 5.0, 0.0), + ))], + ); + let mut insert = Insert::new(NAME.to_string(), v(100.0, 100.0, 0.0)); + insert.set_x_scale(2.0); + insert.set_y_scale(2.0); + insert.set_z_scale(2.0); + insert.rotation = DEG30; + let handle = scene.add_entity(EntityType::Insert(insert)); + let insert = match scene.document.get_entity(handle) { + Some(EntityType::Insert(ins)) => ins.clone(), + _ => panic!("the INSERT was stored"), + }; + + // A rectangle drawn in WCS around the rendered LINE. + let boundary = [[98.0, 98.0], [136.0, 122.0]]; + assert!(crate::scene::pick::xclip::set_insert_clip( + &mut scene.document, + handle, + &boundary, + false, + ([100.0, 100.0], [134.641016, 120.0]), + )); + + let filter = crate::scene::pick::xclip::insert_spatial_filter(&scene.document, &insert) + .expect("the stored spatial filter"); + let world = crate::scene::pick::xclip::world_clip_polygon_for_transform( + filter, + &insert_transform(&scene.document, &insert), + ); + // Two stored points expand to the rectangle in drawn order. + let expected = [[98.0, 98.0], [136.0, 98.0], [136.0, 122.0], [98.0, 122.0]]; + assert_eq!(world.len(), expected.len()); + for (got, want) in world.iter().zip(expected) { + assert!( + (got[0] - want[0]).abs() < 1e-9 && (got[1] - want[1]).abs() < 1e-9, + "ring corner ({:.9},{:.9}), expected ({:.9},{:.9})", + got[0], + got[1], + want[0], + want[1] + ); + } + + // The same stored filter composed with the codec's base-ignoring transform + // displaces the ring by exactly Rot(30)*Scale(2)*base — the error the + // base-aware storage removes. + let base_ignoring = crate::scene::pick::xclip::world_clip_polygon_for_transform( + filter, + &insert.get_transform(), + ); + for (got, want) in base_ignoring.iter().zip(expected) { + assert!( + ((got[0] - want[0]) - GET_TO_INSERT_OFFSET[0]).abs() < 1e-6 + && ((got[1] - want[1]) - GET_TO_INSERT_OFFSET[1]).abs() < 1e-6, + "base-ignoring corner ({:.9},{:.9}) is not the drawn corner plus {:?}", + got[0], + got[1], + GET_TO_INSERT_OFFSET + ); + } +} + +#[test] +fn locked_reframe_conjugates_attributes_through_the_convention() { + // base-point follow-up fix: `reframe_block_definition` (BEDIT UCS bake) + // conjugates the block-local reframe through the INSERT transform for the + // ATTRIBs living on external instances. That transform is the + // base-point-aware one, because it is the map the attribute positions on + // the INSERT were placed through. + const NAME: &str = "BP_REFRAME"; + let mut scene = Scene::new(); + scene.define_block_raw( + NAME, + v(10.0, 5.0, 0.0), + vec![ + EntityType::Line(Line::from_points(v(10.0, 5.0, 0.0), v(30.0, 5.0, 0.0))), + EntityType::AttributeDefinition(attdef("TAG1")), + ], + ); + let mut insert = Insert::new(NAME.to_string(), v(100.0, 100.0, 0.0)); + insert.set_x_scale(2.0); + insert.set_y_scale(2.0); + insert.set_z_scale(2.0); + insert.rotation = DEG30; + // The attribute sits where the convention puts the ATTDEF at (12,8). + let mut attribute = AttributeEntity::simple("TAG1", "VALUE1"); + attribute.insertion_point = insert_transform(&scene.document, &insert).apply(v(12.0, 8.0, 0.0)); + insert.attributes.push(attribute); + let insert_handle = scene.add_entity(EntityType::Insert(insert)); + + // Reframe: a 90° turn of the block-local frame, the kind of UCS change + // `commit_active_ucs_change` bakes in. local_from_old maps (x,y) to (-y,x). + let local = Transform::from_matrix(Matrix4::rotation_z(std::f64::consts::FRAC_PI_2)); + let record = scene + .document + .block_records + .get(NAME) + .expect("the block record") + .handle; + let changed = scene.reframe_block_definition(record, &local); + assert!( + changed >= 2, + "the LINE and the ATTDEF were reframed: {changed}" + ); + + // The block-local contents turned 90°: (10,5)→(-5,10), (30,5)→(-5,30), + // ATTDEF (12,8)→(-8,12). + let stored = block_line(&scene.document, NAME); + assert_close(stored.start, [-5.0, 10.0, 0.0], 1e-9, "reframed line start"); + assert_close(stored.end, [-5.0, 30.0, 0.0], 1e-9, "reframed line end"); + let attdefs = block_attdefs(&scene.document, NAME); + assert_close( + attdefs[0].insertion_point, + [-8.0, 12.0, 0.0], + 1e-9, + "reframed attdef", + ); + + // The attribute on the instance must now sit where the convention puts the + // reframed ATTDEF: (100,100) + Rot(30)*2*((-8,12) - (10,5)) + // = (100,100) + 2*Rot(30)*(-18,7) + // Rot(30)*(-18,7) = (-18*0.8660254 - 7*0.5, -18*0.5 + 7*0.8660254) + // = (-19.088457, -2.937822) + // -> (100,100) + (-38.176914, -5.875644) = (61.823086, 94.124356). + // Conjugating through the base-ignoring transform instead lands on + // (92.803848, 100.464102): get_transform⁻¹ of the old position is the + // base-relative (2,3), which the 90° turn maps to (-3,2). + let attribute = match scene.document.get_entity(insert_handle) { + Some(EntityType::Insert(ins)) => ins.attributes.first().cloned(), + _ => None, + } + .expect("the attribute on the instance"); + assert_close( + attribute.insertion_point, + [61.823086, 94.124356, 0.0], + 1e-6, + "reframed attribute position", + ); +} + +/// Back-to-back nested reference: block `OUTER` (base (10,5,0)) holds a LINE +/// and an INSERT of block `INNER` (base (4,2,0)) at OUTER-local (5,3) with +/// 60° rotation and scale 1; `INNER` holds the LINE (4,2)-(10,2). +fn nested_scene() -> (Scene, Insert) { + const INNER: &str = "BP_NESTED_INNER"; + const OUTER: &str = "BP_NESTED_OUTER"; + let mut scene = Scene::new(); + // INNER: base (4,2,0), so its LINE is local (0,0)-(6,0). + scene.define_block_raw( + INNER, + v(4.0, 2.0, 0.0), + vec![EntityType::Line(Line::from_points( + v(4.0, 2.0, 0.0), + v(10.0, 2.0, 0.0), + ))], + ); + // OUTER: base (10,5,0), LINE (10,5)-(30,5) is local (0,0)-(20,0), plus the + // nested INSERT at OUTER-local (5,3), 60°, scale 1. + let mut nested = Insert::new(INNER.to_string(), v(5.0, 3.0, 0.0)); + nested.set_x_scale(1.0); + nested.set_y_scale(1.0); + nested.set_z_scale(1.0); + nested.rotation = std::f64::consts::PI / 3.0; + scene.define_block_raw( + OUTER, + v(10.0, 5.0, 0.0), + vec![ + EntityType::Line(Line::from_points(v(10.0, 5.0, 0.0), v(30.0, 5.0, 0.0))), + EntityType::Insert(nested), + ], + ); + // Model space: INSERT of OUTER at (100,100), 30°, scale 2. + let mut insert = Insert::new(OUTER.to_string(), v(100.0, 100.0, 0.0)); + insert.set_x_scale(2.0); + insert.set_y_scale(2.0); + insert.set_z_scale(2.0); + insert.rotation = DEG30; + let handle = scene.add_entity(EntityType::Insert(insert.clone())); + let placed = scene + .document + .get_entity(handle) + .and_then(|e| match e { + EntityType::Insert(ins) => Some(ins.clone()), + _ => None, + }) + .expect("the OUTER INSERT"); + (scene, placed) +} + +#[test] +fn locked_nested_reference_explode_matches_the_render_path() { + // base-point: exploding the outer reference one level yields the nested + // reference, and placing the inner block through that piece lands on + // exactly the position the render graph computes for it. + let (scene, outer) = nested_scene(); + + // Hand derivation (c30 = 0.8660254, s30 = 0.5; c60 = 0.5, s60 = 0.8660254): + // INNER-local (0,0) [the base point] -> OUTER-local + // (5,3) + Rot(60)*(0,0) = (5,3) + // OUTER-local (5,3) -> world + // (100,100) + 2*Rot(30)*((5,3) - (10,5)) = (100,100) + 2*Rot(30)*(-5,-2) + // Rot(30)*(-5,-2) = (-5*0.8660254 + 2*0.5, -5*0.5 - 2*0.8660254) + // = (-3.330127, -4.232051) + // -> (100,100) + (-6.660254, -8.464102) = (93.339746, 91.535898) + let mut render_start = None; + for (entity, transform) in render_graph_leaves(&scene) { + let EntityType::Line(line) = entity else { + continue; + }; + if (line.start.x - 4.0).abs() < 1e-9 && (line.start.y - 2.0).abs() < 1e-9 { + let world = transform.apply(line.start); + render_start = Some(world); + } + } + let render_start = render_start.expect("the INNER LINE on the render path"); + assert_close( + render_start, + [93.339746, 91.535898, 0.0], + 1e-6, + "render path: inner line start", + ); + + // Explode path: one level down gives back the nested reference. + let pieces = crate::modules::draw::modify::explode::explode_entity( + &EntityType::Insert(outer.clone()), + &scene.document, + ); + let nested_piece = pieces + .iter() + .find_map(|piece| match piece { + EntityType::Insert(ins) if ins.block_name == "BP_NESTED_INNER" => Some(ins.clone()), + _ => None, + }) + .expect("the nested reference among the exploded pieces"); + // The nested reference itself lands where the render path puts it: the + // codec applies Rot(30+60)=90° and scale 2*1=2 to it. + assert_close( + nested_piece.insert_point, + [93.339746, 91.535898, 0.0], + 1e-6, + "exploded nested reference insert point", + ); + assert!( + (nested_piece.rotation.to_degrees() - 90.0).abs() < 1e-6, + "nested rotation = 90, got {}", + nested_piece.rotation.to_degrees() + ); + assert!((nested_piece.x_scale() - 2.0).abs() < 1e-9); + + // And placing the inner block through that piece reproduces the render + // path: (93.339746, 91.535898) + Rot(90)*2*((10,2) - (4,2)) + // = (93.339746, 91.535898) + 2*Rot(90)*(6,0) = ... + (0,12) + // = (93.339746, 103.535898) + let inner_line = scene + .document + .block_records + .get("BP_NESTED_INNER") + .and_then(|br| { + br.entity_handles + .iter() + .find_map(|h| match scene.document.get_entity(*h) { + Some(EntityType::Line(line)) => Some(line.clone()), + _ => None, + }) + }) + .expect("the INNER LINE"); + let piece_transform = insert_transform(&scene.document, &nested_piece); + assert_close( + piece_transform.apply(inner_line.start), + [93.339746, 91.535898, 0.0], + 1e-6, + "explode path: inner line start", + ); + assert_close( + piece_transform.apply(inner_line.end), + [93.339746, 103.535898, 0.0], + 1e-6, + "explode path: inner line end", + ); + + let mut render_end = None; + for (entity, transform) in render_graph_leaves(&scene) { + let EntityType::Line(line) = entity else { + continue; + }; + if (line.start.x - 4.0).abs() < 1e-9 && (line.start.y - 2.0).abs() < 1e-9 { + let world = transform.apply(line.end); + render_end = Some(world); + } + } + assert_close( + render_end.expect("the INNER LINE end on the render path"), + [93.339746, 103.535898, 0.0], + 1e-6, + "render path: inner line end", + ); +} diff --git a/src/scene/entity.rs b/src/scene/entity.rs index 19e6210bf..5cfe113a9 100644 --- a/src/scene/entity.rs +++ b/src/scene/entity.rs @@ -947,6 +947,14 @@ impl Scene { &mut self, options: CreateBlockOptions, ) -> Result { + // Base-point contract: the contents are stored *relative to the picked + // base* because `world_to_block` already subtracts it, so the + // definition's own `Block.base_point` stays ZERO and the picked base + // becomes the block origin (inserting at a point puts that point where + // the base was). Callers must not also store the picked base in the + // definition — that would subtract it twice. Imported definitions, + // whose geometry arrives relative to their own base point, keep that + // base point and are never rewritten here. let name = options.name.trim(); if name.is_empty() { return Err("Block name cannot be empty.".into()); @@ -988,6 +996,11 @@ impl Scene { // Update existing Block entity fields if let Some(EntityType::Block(b)) = self.document.get_entity_mut(existing_blk_h) { b.description = options.description.clone(); + // The new contents are baked relative to the picked base (see + // the contract above), so the definition's base point has to + // go back to ZERO. Leaving an imported definition's non-zero + // base point here would subtract the picked base twice. + b.base_point = codec::types::Vector3::ZERO; } if options.annotative { let annotative_vals = vec![codec::xdata::XDataValue::Integer16(1)]; diff --git a/src/scene/mod.rs b/src/scene/mod.rs index a895f6dd8..6a10a9e33 100644 --- a/src/scene/mod.rs +++ b/src/scene/mod.rs @@ -26,6 +26,8 @@ pub enum BlockScalePolicy { // Topic submodules split out of this root (each contributes `impl Scene` // blocks and/or free functions). Pure text-move from the original mod.rs. +#[cfg(test)] +mod block_base_point_tests; mod boundary; mod camera_ops; pub(crate) mod centerline; diff --git a/src/scene/modify.rs b/src/scene/modify.rs index aeb936aa7..ae9e84058 100644 --- a/src/scene/modify.rs +++ b/src/scene/modify.rs @@ -506,7 +506,11 @@ impl Scene { // ATTRIBs live inline on each INSERT in that reference's owner space. // Conjugate the block-local reframe through the INSERT transform: - // owner' = M · local_from_old · M⁻¹ · owner. + // owner' = M · local_from_old · M⁻¹ · owner. M is the block-local → + // world map the render path uses (`insert_transform`), because that is + // the map the attribute positions on the INSERT were placed through; + // the codec's base-ignoring transform would conjugate them through a + // displaced frame whenever the definition has a non-zero base point. let references: Vec<(Handle, codec::types::Transform)> = self .document .entities() @@ -516,7 +520,8 @@ impl Scene { && !reference.attributes.is_empty() && !owned.contains(&reference.common.handle) => { - let insertion = reference.get_transform(); + let insertion = + crate::scene::render_graph::insert_transform(&self.document, reference); let inverse = inverse_affine(&insertion)?; Some(( reference.common.handle, diff --git a/src/scene/pick/xclip.rs b/src/scene/pick/xclip.rs index 298a277b2..be143a131 100644 --- a/src/scene/pick/xclip.rs +++ b/src/scene/pick/xclip.rs @@ -250,7 +250,12 @@ pub fn set_insert_clip( let Some(codec::EntityType::Insert(ins)) = doc.get_entity(insert) else { return false; }; - let t = ins.get_transform().matrix.m; + // The stored inverse is the inverse of the transform every consumer + // composes the boundary back with — the base-point-aware instance + // transform the render path uses. Storing the codec's base-ignoring + // inverse would displace the ring by exactly -base on the next render. + let forward = crate::scene::render_graph::insert_transform(doc, ins); + let t = forward.matrix.m; let forward = glam::DMat4::from_cols_array_2d(&[ [t[0][0], t[1][0], t[2][0], t[3][0]], [t[0][1], t[1][1], t[2][1], t[3][1]], diff --git a/src/scene/render_graph.rs b/src/scene/render_graph.rs index 9a42a1ed9..38ee100e6 100644 --- a/src/scene/render_graph.rs +++ b/src/scene/render_graph.rs @@ -688,7 +688,14 @@ pub(crate) fn clear_dim_picture_placement(entity: &mut EntityType) { } pub fn insert_transform(document: &CadDocument, insert: &Insert) -> Transform { - let base = block_base_point(document, &insert.block_name); + insert_transform_with_base(insert, block_base_point(document, &insert.block_name)) +} + +/// [`insert_transform`] for a caller that already resolved the block's base +/// point (the `-INSERT` command builds the INSERT before the drawing is in +/// scope), so the base-point convention lives in exactly one place: +/// world = insert_point + Rot(rotation) * Scale * (P - base). +pub fn insert_transform_with_base(insert: &Insert, base: Vector3) -> Transform { Transform::from_translation(Vector3::new(-base.x, -base.y, -base.z)) .then(&insert.get_transform()) } diff --git a/src/ui/window/block_definition.rs b/src/ui/window/block_definition.rs index eadb1700b..e8905a685 100644 --- a/src/ui/window/block_definition.rs +++ b/src/ui/window/block_definition.rs @@ -139,6 +139,15 @@ impl BlockDefinitionState { glam::DVec3::new(x, y, z) } + /// Prefill the base point fields from an existing definition, so + /// redefining a block starts from the base it already has (0.0000 for a + /// new block). Formatted the same way the pick point button writes them. + pub fn set_base_point(&mut self, base: glam::DVec3) { + self.base_point_x = format!("{:.4}", base.x); + self.base_point_y = format!("{:.4}", base.y); + self.base_point_z = format!("{:.4}", base.z); + } + pub fn is_existing_name(&self) -> bool { let trimmed = self.name.trim(); !trimmed.is_empty()