diff --git a/src/app/command_driver.rs b/src/app/command_driver.rs index 2eed7c236..35259107d 100644 --- a/src/app/command_driver.rs +++ b/src/app/command_driver.rs @@ -454,17 +454,6 @@ impl OpenCADStudio { } } if matches!(&input, StepInput::Point(_)) { self.refresh_command_point_pick_context(i); } - // PR2: remember which viewport (if any) each dimension point was - // picked through, before the command turns it into geometry. - if let StepInput::Point(p) = &input { - if self.tabs[i] - .active_cmd - .as_ref() - .is_some_and(|command| command.measures_through_viewports()) - { - self.record_dimension_accepted_snap(i, *p); - } - } let ctrl = self.ctrl_down; let shift = self.shift_down; let result: Option = { @@ -2117,15 +2106,17 @@ impl OpenCADStudio { acadrust::entities::Dimension::Ordinate(_) ) ); - // PR2: a dimension placed on the sheet but measuring model - // geometry through a viewport carries the compensation as a - // negative DIMLFAC override. It is deliberately *not* - // associated: its definition points are projected paper - // coordinates, so inferring a paper-space association would - // bind it to geometry that is not there. PR3 records the real - // dimension -> viewport -> entity chain. - let measured_through_viewport = - self.apply_viewport_dimension_measurement(i, &mut entity); + // A dimension placed on the sheet but measuring model geometry + // through a viewport carries the compensation as a negative + // DIMLFAC override. + self.apply_viewport_dimension_measurement(i, &mut entity); + // ...and is deliberately *not* associated in paper space: its + // definition points are projected paper coordinates, so a + // paper-space association would bind it to geometry that is + // not there. PR3 records the real dimension -> viewport -> + // entity chain instead. One gate, shared with + // `infer_dimension_sources_guarded`. + let association_allowed = self.dimension_association_allowed(i); let inherited_dimension = if preserve_base_style { match &entity { acadrust::EntityType::Dimension(dimension) => Some(( @@ -2192,7 +2183,7 @@ impl OpenCADStudio { entity, preserve_base_style, ) { - if association_mode == 2 && !measured_through_viewport { + if association_mode == 2 && association_allowed { let mut changes = vec![ (handle, crate::scene::ChangeKind::Modified), ]; @@ -2236,7 +2227,6 @@ impl OpenCADStudio { self.tabs[i].dirty = true; self.tabs[i].scene.clear_preview_wire(); self.tabs[i].snap_result = None; - self.clear_dimension_accepted_snaps(); if let Some(pd) = pending { self.commit_undo_delta(i, pd); } @@ -2254,7 +2244,6 @@ impl OpenCADStudio { } } CmdResult::CommitDimensionsAndExit(dimensions) => { - self.clear_dimension_accepted_snaps(); let association_mode = self.tabs[i] .scene .document diff --git a/src/app/dim_viewport.rs b/src/app/dim_viewport.rs index 632ed364d..fbd35cce7 100644 --- a/src/app/dim_viewport.rs +++ b/src/app/dim_viewport.rs @@ -5,29 +5,42 @@ //! *report* the model measurement. Because a viewport frame is a similarity //! (uniform scale plus in-plane twist), the model measurement of any linear, //! aligned or radial span is exactly the raw paper measurement times the -//! viewport compensation `1 / scale` — projecting the model span onto the +//! viewport compensation `1 / scale` -- projecting the model span onto the //! rotated paper axis and compensating is the same number. So no geometry is //! rewritten: the compensation is persisted once, as a negative DIMLFAC //! override on the dimension (see //! [`MeasurementScale::viewport_dimlfac_override`]), and applied once, by the //! linear formatter. //! -//! Angular dimensions are never compensated — a similarity preserves angles. +//! Angular dimensions are never compensated -- a similarity preserves angles. //! -//! ## PR1 seam -//! [`OpenCADStudio::record_dimension_accepted_snap`] is the one function PR1 -//! replaces. PR1 records a real [`AcceptedSnap`] per accepted point (with the -//! snapped source and the frame in effect at snap time); this branch derives -//! the same information from the cursor's paper position and the content -//! viewport under it. Everything downstream consumes `AcceptedSnap` only. +//! ## Where the snaps come from +//! The per-point [`AcceptedSnap`] list is PR1's +//! ([`OpenCADStudio::accepted_snaps`]): the click handler records the real +//! snap for every interactive pick, and `apply_step_input` records a free +//! snap for typed / dynamic-input / headless points. There is exactly one +//! source of truth. Two things are layered on top here: +//! +//! * the explicit object-pick path ([`Scene::dimension_pick_through_viewport`]) +//! is not a point step at all, so it records its own snaps +//! ([`OpenCADStudio::record_dimension_viewport_pick`]); +//! * a *typed* coordinate carries no snap, so PR1 records it as a free point +//! with no viewport. [`OpenCADStudio::measure_snap`] re-attaches the +//! content viewport under such a point, which is what makes +//! `DIMLINEAR` + typed coordinates inside a 1:10 viewport measure the model. +//! It is deliberately limited to snaps that are free *and* source-less, so a +//! genuine paper-sheet snap that happens to sit over a viewport is never +//! reinterpreted. use acadrust::entities::Dimension; +use acadrust::types::Handle; use acadrust::xdata::XDataValue; use acadrust::EntityType; use glam::DVec3; +use crate::command::DimensionAssociationSource; use crate::entities::dim_override; -use crate::scene::viewport_ref::{AcceptedSnap, MeasurementScale, ViewportFrame}; +use crate::scene::viewport_ref::{AcceptedSnap, MeasurementScale, SnapSourceRef, ViewportFrame}; use super::OpenCADStudio; @@ -46,101 +59,106 @@ pub(crate) enum DimensionMeasureSpace { } impl OpenCADStudio { - // ───────────────────────── PR1 SEAM: BEGIN ────────────────────────── - - /// Record the snap accepted for one collected point of the active - /// command. + /// Record the accepted snaps produced by the explicit object-pick path (a + /// dimension "select object" pick resolved through a content viewport). /// - /// PR1 replaces the body with the real snap result (which knows the - /// snapped entity, the block path and the frame at snap time). Until then - /// the viewport is resolved from the paper position of the accepted point, - /// which is correct for every point that lands inside a content viewport - /// and yields a free snap everywhere else. - pub(crate) fn record_dimension_accepted_snap(&mut self, i: usize, point: DVec3) { - self.sync_dimension_snap_owner(i); - let snap = match self.tabs[i].scene.viewport_frame_at_paper_point(point) { - Some(frame) => AcceptedSnap { - paper_point: point, - model_point: frame.paper_to_model(point), - viewport: Some(frame.viewport), - frame: Some(frame), - source: None, - }, - None => AcceptedSnap::free(point), - }; - self.dim_accepted_snaps.push(snap); - } - - // ────────────────────────── PR1 SEAM: END ─────────────────────────── - - /// Record an accepted snap produced by the explicit object-pick path (a - /// pick resolved through a content viewport). Independent of PR1. + /// An object pick is not a point step, so PR1's click handler never sees + /// it. One pick supplies *both* extension origins, so it is recorded + /// twice: that keeps the list index-parallel with the definition points + /// the command derives from the entity, and lets the "every measuring + /// point came through one viewport" classification see a complete set. pub(crate) fn record_dimension_viewport_pick( &mut self, - i: usize, frame: ViewportFrame, paper_point: DVec3, model_point: DVec3, + entity: Handle, + block_path: Vec, ) { - self.sync_dimension_snap_owner(i); - // An object pick supplies both extension origins at once; record it - // twice so the "every measuring point came through one viewport" - // test sees a complete set. for _ in 0..2 { - self.dim_accepted_snaps.push(AcceptedSnap { + self.push_accepted_snap(AcceptedSnap { paper_point, model_point, viewport: Some(frame.viewport), frame: Some(frame), - source: None, + source: Some(SnapSourceRef { + source: DimensionAssociationSource::inferred(entity), + block_path: block_path.clone(), + // An object pick acquires the whole curve, not one + // feature; the per-slot feature point is derived from the + // committed dimension's own definition points. + snap_type: crate::snap::SnapType::Nearest, + }), }); } } /// The accepted snap for the `idx`-th collected point of the active - /// command, if one was recorded. The contracted accessor PR1 and PR3 read; - /// PR2 itself classifies the whole set at commit time. + /// command, with the typed-coordinate viewport fallback applied. + /// + /// Reads PR1's list -- there is no second store. #[allow(dead_code)] - pub(crate) fn accepted_snap_for_point(&self, idx: usize) -> Option<&AcceptedSnap> { - self.dim_accepted_snaps.get(idx) + pub(crate) fn accepted_snap_for_point(&self, i: usize, idx: usize) -> Option { + self.accepted_snaps().get(idx).map(|snap| self.measure_snap(i, snap)) } - /// Drop recorded snaps when the active command changes, so one command - /// never sees another's points. - fn sync_dimension_snap_owner(&mut self, i: usize) { - let name = self.tabs[i] - .active_cmd - .as_ref() - .map(|command| command.name().to_string()); - if self.dim_accepted_snaps_owner != name { - self.dim_accepted_snaps.clear(); - self.dim_accepted_snaps_owner = name; + /// One accepted snap as *measurement* sees it. + /// + /// Identical to the recorded snap except that a point which carries no + /// snap at all (a typed coordinate, a dynamic-input point, a headless + /// script point) and lands inside a content viewport is re-attached to + /// that viewport, so typing `100,50` inside a 1:10 viewport measures the + /// model just like clicking there would. A snap that landed on real + /// paper-sheet geometry keeps `viewport: None` and is never reinterpreted. + pub(crate) fn measure_snap(&self, i: usize, snap: &AcceptedSnap) -> AcceptedSnap { + if snap.frame.is_some() || snap.source.is_some() { + return snap.clone(); + } + let scene = &self.tabs[i].scene; + if scene.current_layout == "Model" || scene.active_viewport.is_some() { + return snap.clone(); + } + match scene.viewport_frame_at_paper_point(snap.paper_point) { + Some(frame) => AcceptedSnap { + paper_point: snap.paper_point, + model_point: frame.paper_to_model(snap.paper_point), + viewport: Some(frame.viewport), + frame: Some(frame), + source: None, + }, + None => snap.clone(), } } - pub(crate) fn clear_dimension_accepted_snaps(&mut self) { - self.dim_accepted_snaps.clear(); - self.dim_accepted_snaps_owner = None; + /// The accepted snaps that say what the pending dimension *measures*, in + /// collection order. + /// + /// The final collected point of every dimension command in scope is the + /// dimension-line / text placement click, which says nothing about what is + /// being measured -- it routinely lands on bare sheet even for a viewport + /// measurement, and inside a viewport rectangle even for a plain paper + /// one. It is dropped here, which is also why association inference is + /// guarded on this set rather than on every recorded snap. + pub(crate) fn dimension_measuring_snaps(&self, i: usize) -> Vec { + let all = self.accepted_snaps(); + let measuring = match all.len() { + 0 | 1 => all, + n => &all[..n - 1], + }; + measuring.iter().map(|snap| self.measure_snap(i, snap)).collect() } /// Classify how the pending dimension should be measured from the snaps /// recorded for it. - fn dimension_measure_space(&self, i: usize) -> DimensionMeasureSpace { + pub(crate) fn dimension_measure_space(&self, i: usize) -> DimensionMeasureSpace { let scene = &self.tabs[i].scene; // Only a dimension being created *on the sheet* can measure through a // viewport. Inside MSPACE the command already works in model space. if scene.current_layout == "Model" || scene.active_viewport.is_some() { return DimensionMeasureSpace::Direct; } - // The final collected point of every dimension command in scope is the - // dimension-line / text placement click, which says nothing about what - // is being measured — it routinely lands on bare sheet even for a - // viewport measurement, and inside a viewport rectangle even for a - // plain paper one. Only the measuring points are classified. - let measuring = match self.dim_accepted_snaps.len() { - 0 | 1 => self.dim_accepted_snaps.as_slice(), - n => &self.dim_accepted_snaps[..n - 1], - }; + let measuring = self.dimension_measuring_snaps(i); + let measuring = measuring.as_slice(); if measuring.is_empty() { return DimensionMeasureSpace::Direct; } @@ -167,24 +185,23 @@ impl OpenCADStudio { /// Apply viewport compensation to a dimension about to be committed. /// - /// Returns `true` when the dimension measures through a viewport, which - /// tells the caller not to infer a paper-space association from the - /// projected definition points (PR3 creates the real viewport - /// associations). + /// Whether the dimension may also take a paper-space association is a + /// separate question with the same answer source: + /// [`OpenCADStudio::dimension_association_allowed`]. pub(crate) fn apply_viewport_dimension_measurement( &mut self, i: usize, entity: &mut EntityType, - ) -> bool { + ) { let EntityType::Dimension(dimension) = entity else { - return false; + return; }; let is_angular = matches!( dimension, Dimension::Angular2Ln(_) | Dimension::Angular3Pt(_) ); let frame = match self.dimension_measure_space(i) { - DimensionMeasureSpace::Direct => return false, + DimensionMeasureSpace::Direct => return, DimensionMeasureSpace::Mixed => { self.command_line.push_warning( crate::t!( @@ -192,16 +209,15 @@ impl OpenCADStudio { ) .as_ref(), ); - return false; + return; } DimensionMeasureSpace::Viewport(frame) => frame, }; // A similarity preserves angles, so an angular dimension reads the - // model angle already. It still measured through a viewport, so the - // caller must skip association inference. + // model angle already and needs no override. if is_angular { - return true; + return; } let compensation = frame.paper_to_model_length_factor(); @@ -225,7 +241,6 @@ impl OpenCADStudio { Some(XDataValue::Real(value)), ); } - true } /// Explicit "select object" pick for a dimension command, resolved through @@ -251,7 +266,13 @@ impl OpenCADStudio { let pick = self.tabs[i] .scene .dimension_pick_through_viewport(paper, aperture_paper)?; - self.record_dimension_viewport_pick(i, pick.frame, pick.paper_point, pick.model_point); + self.record_dimension_viewport_pick( + pick.frame, + pick.paper_point, + pick.model_point, + pick.entity_handle, + pick.block_path.clone(), + ); let command = self.tabs[i].active_cmd.as_mut()?; command.inject_picked_entity(pick.paper_entity); Some(command.on_entity_pick(pick.entity_handle, pick.paper_point)) diff --git a/src/app/mod.rs b/src/app/mod.rs index 7a204e85e..c68bdfa0d 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -9,7 +9,7 @@ pub(crate) mod config; pub use automation::{export_headless, serve}; mod command_driver; pub(crate) mod commands; -mod dim_viewport; +pub(crate) mod dim_viewport; mod document; mod drafting_settings; pub(crate) mod expr_eval; @@ -509,12 +509,6 @@ pub(super) struct OpenCADStudio { cursor_size: i32, /// Selection-box size setting (PICKBOX, 0..=50). pick_box: i32, - /// PR2: accepted snaps for the points the active dimension command has - /// collected, in collection order. Transient command state — the committed - /// result is an ordinary dimension entity, so undo/redo needs nothing here. - pub(crate) dim_accepted_snaps: Vec, - /// Name of the command `dim_accepted_snaps` belongs to. - pub(crate) dim_accepted_snaps_owner: Option, /// Use REFEDIT rather than BEDIT when double-clicking an attribute-free block. double_click_block_refedit: bool, /// Open ATTEDIT when double-clicking a block with attributes. @@ -3641,8 +3635,6 @@ impl OpenCADStudio { zoom_factor: 60, cursor_size: 5, pick_box: 3, - dim_accepted_snaps: Vec::new(), - dim_accepted_snaps_owner: None, double_click_block_refedit: false, double_click_block_attedit: true, grip_object_limit: settings::DEFAULT_GRIP_OBJECT_LIMIT, diff --git a/src/app/update/viewport_snap.rs b/src/app/update/viewport_snap.rs index 898c55570..0265b36c7 100644 --- a/src/app/update/viewport_snap.rs +++ b/src/app/update/viewport_snap.rs @@ -46,43 +46,51 @@ impl OpenCADStudio { self.accepted_snaps.last() } - /// `true` when any point the active command collected came through a - /// layout viewport. Dimension association inference (which compares a - /// dimension's own definition points against geometry in the *current* - /// space) must not run in that case: the definition points are paper - /// coordinates that correspond to no paper-sheet geometry, and the model - /// geometry they really describe lives at completely different - /// coordinates. PR3 replaces this guard with real viewport-aware - /// associations built from `AcceptedSnap::source`. - pub(crate) fn accepted_snaps_through_viewport(&self) -> bool { - self.accepted_snaps.iter().any(AcceptedSnap::through_viewport) - } - - /// `Scene::infer_dimension_sources`, suppressed when the dimension's points - /// were picked through a layout viewport (PR1). + /// `Scene::infer_dimension_sources`, suppressed when the dimension being + /// created measures through a layout viewport. /// /// Inference works by looking for geometry near the dimension's own /// definition points, in whatever space it is handed. A paper-space /// dimension measured through a viewport has PAPER definition points that /// happen to sit over projected model geometry, so unguarded inference /// would either associate it with unrelated paper-sheet geometry at those - /// coordinates, or match model geometry using paper coordinates. Neither is - /// correct, and a wrong association is worse than none. Returning an empty - /// source list makes `attach_dimension_association` a no-op. + /// coordinates, or match model geometry using paper coordinates. Neither + /// is correct, and a wrong association is worse than none. Returning an + /// empty source list makes `attach_dimension_association` a no-op. /// - /// Deliberately minimal: PR3 replaces this with a real viewport-aware - /// association built from `AcceptedSnap::source`. + /// The classification is + /// [`OpenCADStudio::dimension_measure_space`] — the *same* one that + /// decides whether to compensate the measurement, so the two can never + /// disagree. In particular it ignores the trailing dimension-line + /// placement click, which routinely lands inside a viewport rectangle for + /// an ordinary paper-space dimension and must not suppress its + /// association. pub(crate) fn infer_dimension_sources_guarded( &self, tab: usize, dimension: Handle, ) -> Vec> { - if self.accepted_snaps_through_viewport() { + if !self.dimension_association_allowed(tab) { return Vec::new(); } self.tabs[tab].scene.infer_dimension_sources(dimension) } + /// The single gate on creating a paper-space association for the + /// dimension the active command is committing. + /// + /// `false` exactly when the dimension measures through (or partly through) + /// a layout viewport, whatever supplied its sources — inference, + /// an explicit object pick, or an explicit source list. Everything in the + /// commit path that used to decide this for itself now asks here, so the + /// measurement rule and the association rule cannot drift apart. + pub(crate) fn dimension_association_allowed(&self, tab: usize) -> bool { + matches!( + self.dimension_measure_space(tab), + crate::app::dim_viewport::DimensionMeasureSpace::Direct + ) + } + /// Forget the accepted snaps of a finished / abandoned command. pub(crate) fn clear_accepted_snaps(&mut self) { self.accepted_snaps.clear(); @@ -107,6 +115,15 @@ impl OpenCADStudio { } None => AcceptedSnap::free(committed), }; + self.push_accepted_snap(accepted); + } + + /// Append an already-built accepted snap, keeping the retention cap. + /// + /// Used by paths that are not point steps (the dimension object pick + /// resolved through a viewport) and therefore never reach the click + /// handler. + pub(crate) fn push_accepted_snap(&mut self, accepted: AcceptedSnap) { if self.accepted_snaps.len() >= MAX_ACCEPTED_SNAPS { self.accepted_snaps.remove(0); } diff --git a/src/scene/viewport_dim_seam.rs b/src/scene/viewport_dim_seam.rs index a89059394..575aaf505 100644 --- a/src/scene/viewport_dim_seam.rs +++ b/src/scene/viewport_dim_seam.rs @@ -1,19 +1,20 @@ -//! PR2 seams onto the paper-space viewport world. +//! Object-pick resolution for paper-space dimensions taken through a layout +//! viewport. //! -//! Everything in this file exists so that viewport-aware dimension -//! measurement works *without* PR1 (viewport snapping) in the tree. Each seam -//! is small and self-contained; the merge notes say which ones PR1 replaces. +//! Paper-space hit testing only sees the sheet's own entities, so a dimension +//! command's explicit "select object" pick that lands inside a content +//! viewport finds nothing. [`Scene::dimension_pick_through_viewport`] maps the +//! click into model space through the viewport's +//! [`crate::scene::viewport_ref::ViewportFrame`], hit-tests the viewport's +//! resident model wires (descending into block instances and baking the +//! instance transform), and hands back a copy of the picked entity already +//! projected onto the sheet. //! -//! ## Seams -//! * [`Scene::viewport_frame`] — **replaced by PR1.** PR1 owns construction of -//! [`ViewportFrame`] from the live viewport camera. This implementation -//! derives the frame straight from the DXF viewport record, which is correct -//! for plan (non-oblique) content viewports. -//! * [`Scene::content_viewport_at_paper_point`] / -//! [`Scene::viewport_frame_at_paper_point`] — PR1 supersedes these with its -//! snap-time viewport resolution; harmless to keep. -//! * [`Scene::dimension_pick_through_viewport`] — PR2 owns this permanently. -//! It is the explicit-object-pick path and does not depend on PR1. +//! The frame itself is built by [`Scene::viewport_frame`] (see +//! `crate::scene::mspace`), which derives it from the *live viewport camera* +//! so it always agrees with the pixels the renderer drew. It returns `None` +//! for oblique / perspective viewports, and every entry point here then +//! declines the pick rather than guessing a planar mapping. use super::*; @@ -43,33 +44,6 @@ pub struct ViewportDimPick { } impl Scene { - // ───────────────────────── PR1 SEAM: BEGIN ────────────────────────── - // Replace the body of `viewport_frame` with PR1's implementation on - // merge; the signature is the shared contract and must not change. - - /// Planar paper↔model mapping for one layout viewport. - /// - /// Derived from the viewport record rather than the render camera: - /// `paper_center = vp.center`, `model_target = vp.view_target`, - /// `scale = vp_effective_scale(...)`, `twist = vp.twist_angle`. - pub fn viewport_frame(&self, viewport: Handle) -> Option { - let Some(EntityType::Viewport(vp)) = self.document.get_entity(viewport) else { - return None; - }; - let scale = vp_effective_scale(vp.custom_scale, vp.view_height, vp.height); - if !scale.is_finite() || scale.abs() < 1e-12 { - return None; - } - Some(ViewportFrame { - viewport, - paper_center: DVec2::new(vp.center.x, vp.center.y), - model_target: DVec2::new(vp.view_target.x, vp.view_target.y), - scale, - twist: vp.twist_angle, - locked: vp.status.locked, - }) - } - // ────────────────────────── PR1 SEAM: END ─────────────────────────── /// The smallest *on* content viewport of the current layout whose paper /// rectangle contains `paper`. `None` on the Model tab, while editing