From a202a61ffe6079d781f54bd8b97c9f305f9bce72 Mon Sep 17 00:00:00 2001 From: Panos <25664832+mechpanos@users.noreply.github.com> Date: Thu, 10 Sep 2026 18:00:53 +0300 Subject: [PATCH] fix(display): regenerate existing solid isolines Rebuild cached solid meshes for REGEN and apply Options ISOLINES changes once when the slider is released. --- src/app/commands/display.rs | 120 +++++++++++++++++++++++++++++++++++- src/app/mod.rs | 7 +++ src/app/settings.rs | 52 ++++++++++++++++ src/app/update/file.rs | 27 +++++--- src/app/update/mod.rs | 97 +++++++++++++++++------------ src/ui/window/options.rs | 51 ++++++++------- 6 files changed, 281 insertions(+), 73 deletions(-) diff --git a/src/app/commands/display.rs b/src/app/commands/display.rs index 908b02960..4a84ccba8 100644 --- a/src/app/commands/display.rs +++ b/src/app/commands/display.rs @@ -508,7 +508,18 @@ impl OpenCADStudio { // "modified" merely because tessellation caches were invalidated (C7). // REGENALL is functionally identical (C5). "REGEN" | "REGENALL" => { - self.tabs[i].scene.bump_geometry(); + // Solid meshes carry the isoline count they were tessellated + // with: `add_entity_internal` and `update_entity` pass + // `header.isolines` into `tessellate_volume` and store the + // result. Bumping the geometry epoch clears the wire memo but + // never re-runs that, so changing ISOLINES left every existing + // solid showing the density it was born with, and REGEN — the + // command whose whole job is this — did not help. + // + // `populate_meshes_from_document` re-tessellates from the + // document and bumps the epoch itself, so it replaces the bump + // rather than adding to it. + self.tabs[i].scene.populate_meshes_from_document(); self.command_line.push_output(crate::t!("REGEN: regenerated model.").as_ref()); return Some(Task::none()); } @@ -1893,3 +1904,110 @@ mod tests { assert!(!app.tabs[i].dirty, "REGENALL must not dirty the document either"); } } + +#[cfg(test)] +mod regen_tests { + use crate::app::OpenCADStudio; + + /// A solid's mesh carries the isoline count it was tessellated with: + /// `add_entity_internal` and `update_entity` pass `header.isolines` into + /// `tessellate_volume` and store the result. REGEN used to bump the + /// geometry epoch and stop, which clears the wire memo but leaves + /// `scene.meshes` exactly as it was — reported on #1123 as "REGEN should + /// fix this, but the isolines keep visible". + /// + /// Building ACIS geometry here would be out of proportion, so the mesh map + /// is seeded with a stale entry instead: REGEN must rebuild the map from + /// the document, and an entry belonging to no entity cannot survive that. + /// Bumping alone would leave it in place. + #[test] + fn regen_rebuilds_the_mesh_map_rather_than_only_bumping_the_epoch() { + let mut app = OpenCADStudio::new_for_test(); + app.automation_op(r#"{"op":"new"}"#); + let i = app.active_tab; + + // An entry belonging to no entity. Rebuilding the map from the document + // drops it; bumping the geometry epoch leaves it exactly where it is, + // which is the whole difference this fix turns on. + let stale = acadrust::Handle::new(0xDEAD); + app.tabs[i].scene.meshes.insert(stale, stale_mesh()); + let epoch_before = app.tabs[i].scene.geometry_epoch; + + let _ = app.run_command_line("REGEN"); + + assert!( + !app.tabs[i].scene.meshes.contains_key(&stale), + "REGEN must rebuild the mesh map, not leave what was there", + ); + assert_ne!( + app.tabs[i].scene.geometry_epoch, epoch_before, + "and it still has to bump the geometry epoch", + ); + } + + /// Dragging the isolines slider must not rebuild every solid's mesh on the + /// way — a drag emits a message per pixel, and the rebuild is the same work + /// REGEN does. It waits for the release, and only runs if the value moved. + #[test] + fn the_isolines_slider_rebuilds_once_on_release_and_not_while_dragging() { + use crate::app::Message; + + let mut app = OpenCADStudio::new_for_test(); + app.automation_op(r#"{"op":"new"}"#); + let i = app.active_tab; + + let seed = |app: &mut OpenCADStudio, i: usize| { + let stale = acadrust::Handle::new(0xBEEF); + app.tabs[i].scene.meshes.insert(stale, stale_mesh()); + stale + }; + let stale = seed(&mut app, i); + + for value in [4i16, 3, 2, 1, 0] { + let _ = app.update(Message::IsolinesChanged(value)); + } + assert!( + app.tabs[i].scene.meshes.contains_key(&stale), + "the drag itself must not rebuild the meshes", + ); + assert_eq!(app.tabs[i].scene.document.header.isolines, 0); + + let _ = app.update(Message::IsolinesReleased); + assert!( + !app.tabs[i].scene.meshes.contains_key(&stale), + "releasing after a change rebuilds them", + ); + + // A release with nothing changed since the last one is a click that + // moved nothing, and must not pay for a rebuild. + let stale = seed(&mut app, i); + let _ = app.update(Message::IsolinesChanged(0)); + let _ = app.update(Message::IsolinesReleased); + assert!( + app.tabs[i].scene.meshes.contains_key(&stale), + "releasing without a change must rebuild nothing", + ); + } + + fn stale_mesh() -> crate::scene::model::mesh_model::MeshLodSet { + crate::scene::model::mesh_model::MeshLodSet { + lods: Vec::new(), + material: None, + face_materials: Default::default(), + visual_style: None, + complete: true, + edge_verts: Vec::new(), + edge_verts_low: Vec::new(), + curved_gens: Vec::new(), + metrics: Default::default(), + world_aabb: [0.0; 4], + z_aabb: [0.0; 2], + instance_source: None, + instance_transform: None, + instance_handle: None, + instance_color: None, + instance_aabb: None, + } + } +} + diff --git a/src/app/mod.rs b/src/app/mod.rs index eef45d546..3a87326f8 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -517,6 +517,10 @@ pub(super) struct OpenCADStudio { crosshair_color: Option<[u8; 3]>, /// Editable Options buffer for the crosshair colour. crosshair_color_input: String, + /// Set while the isolines slider is being dragged with a value that + /// differs from the drawing's, so the costly mesh rebuild runs once on + /// release rather than on every pixel of the drag. + isolines_awaiting_regen: bool, /// Edit buffer for the SNAPANG field on the Options Drafting page. Kept /// separate from `snap_angle_deg` so a half-typed angle is not parsed. snap_angle_input: String, @@ -2048,6 +2052,8 @@ pub enum Message { OpenFolder(String), /// Change isolines per surface in the current drawing (ISOLINES). IsolinesChanged(i16), + /// The isolines slider was released; rebuild the meshes if it moved. + IsolinesReleased, /// Toggle silhouette edges in the current drawing (DISPSILH). DispSilhChanged(bool), /// Change surface density U in the current drawing (SURFU). @@ -3460,6 +3466,7 @@ impl OpenCADStudio { cursor_type: settings::CursorType::Crosshair, crosshair_color: None, crosshair_color_input: String::new(), + isolines_awaiting_regen: false, snap_angle_input: "0".to_string(), lineweight_display_scale: 100, isometric_drafting: false, diff --git a/src/app/settings.rs b/src/app/settings.rs index 9db2d574c..dcacee388 100644 --- a/src/app/settings.rs +++ b/src/app/settings.rs @@ -145,6 +145,27 @@ pub(crate) fn snaps_from_osmode(osmode: i32) -> (Vec, bool) { (modes, osmode & OSMODE_SUPPRESS == 0) } +fn deserialize_options_tab<'de, D>( + deserializer: D, +) -> Result +where + D: serde::Deserializer<'de>, +{ + use crate::ui::window::options::OptionsTab; + use serde::Deserialize; + let name = String::deserialize(deserializer).unwrap_or_default(); + Ok(match name.as_str() { + "files" => OptionsTab::Files, + "open-and-save" => OptionsTab::OpenAndSave, + "display" => OptionsTab::Display, + "drafting" => OptionsTab::Drafting, + "modeling" => OptionsTab::Modeling, + "selection" => OptionsTab::Selection, + "user-preferences" => OptionsTab::UserPreferences, + _ => OptionsTab::General, + }) +} + /// Render a drafting angle without a trailing `.0`, so `22.5` but `30`. /// /// The polar pop-up formats its presets the same way; both are showing the @@ -183,6 +204,12 @@ pub struct UserSettings { /// all. 0 means no limit. The drawing header carries no slot for it. pub grip_object_limit: i32, /// Which Options page was showing when the dialog was last closed. + /// + /// Read leniently: `AppConfig::load` discards the *whole* file when any + /// field fails to parse, so a page name that no longer exists — a tab + /// renamed or merged away between versions — would silently reset every + /// setting the user has. An unknown name falls back to the first page. + #[serde(default, deserialize_with = "deserialize_options_tab")] pub options_tab: crate::ui::window::options::OptionsTab, /// Show the navigation cube (NAVVCUBE). pub show_viewcube: bool, @@ -427,4 +454,29 @@ mod tests { let b: std::collections::HashSet<_> = back.into_iter().collect(); assert_eq!(a, b); } + + /// `AppConfig::load` throws the whole file away when any field fails to + /// parse, so a page name that no longer exists would quietly reset every + /// preference the user has. This branch removed the Drawing page, and + /// anyone who had it open when they last closed the dialog has that name + /// on disk. + #[test] + fn a_page_name_that_no_longer_exists_does_not_cost_the_other_settings() { + let json = r#"{ + "settings": { + "options_tab": "drawing", + "pick_add": false, + "savetime_min": 42 + } + }"#; + let cfg: crate::app::config::AppConfig = + serde_json::from_str(json).expect("an unknown page name must not fail the parse"); + assert_eq!( + cfg.settings.options_tab, + crate::ui::window::options::OptionsTab::General, + ); + assert!(!cfg.settings.pick_add, "the rest of the file must survive"); + assert_eq!(cfg.settings.savetime_min, 42); + } } + diff --git a/src/app/update/file.rs b/src/app/update/file.rs index a98df8247..c8352555b 100644 --- a/src/app/update/file.rs +++ b/src/app/update/file.rs @@ -355,24 +355,37 @@ impl OpenCADStudio { } } - /// Write a tessellation-affecting header variable on the active drawing. + /// Write a header variable on the active drawing and mark it modified. /// - /// These decide how solids and surfaces are tessellated, so the geometry - /// has to be rebuilt for the change to appear — the same reason `LTSCALE` - /// bumps it. Marking the tab modified is what makes the new value reach - /// the file. - pub(in crate::app) fn set_drawing_tessellation_var( + /// Deliberately does no geometry work. The variables the Options pages + /// write differ in what they cost to apply, and treating them alike is + /// wrong in both directions: `DISPSILH` is read at render time and needs + /// nothing, `SURFU`/`SURFV`/`SURFTYPE` are inputs to PEDIT's smoothing and + /// need nothing until it runs, while `ISOLINES` is baked into solid meshes + /// at tessellation and needs more than a bump — see `regenerate_meshes`. + pub(in crate::app) fn set_drawing_var( &mut self, write: impl FnOnce(&mut acadrust::document::HeaderVariables), ) { let i = self.active_tab; if let Some(tab) = self.tabs.get_mut(i) { write(&mut tab.scene.document.header); - tab.scene.bump_geometry(); tab.dirty = true; } } + /// Re-tessellate the active drawing's solids from the document. + /// + /// What REGEN does, and the only thing that makes a changed `ISOLINES` + /// reach solids that already exist. Costly on a solid-heavy drawing, so + /// callers driven by a slider run it when the drag ends, not per tick. + pub(in crate::app) fn regenerate_meshes(&mut self) { + let i = self.active_tab; + if let Some(tab) = self.tabs.get_mut(i) { + tab.scene.populate_meshes_from_document(); + } + } + /// Rebuild text geometry in every open drawing. /// /// `TEXTFILL` is a process-global, so a change to it invalidates the text diff --git a/src/app/update/mod.rs b/src/app/update/mod.rs index 3b4844fe5..e93374c43 100644 --- a/src/app/update/mod.rs +++ b/src/app/update/mod.rs @@ -6305,54 +6305,73 @@ impl OpenCADStudio { Task::none() } - // The five below are drawing variables: they go into the header of - // the active drawing and mark it modified, exactly as SETVAR does. - // The four that feed solid tessellation also bump the geometry, or - // the change is invisible until an unrelated edit rebuilds it. + // Drawing variables: they go into the header of the active + // drawing and mark it modified, as SETVAR does. What each one + // costs to apply differs, so they are not treated alike. + // + // ISOLINES is baked into a solid's mesh when it is tessellated, so + // an existing solid keeps the density it was born with until the + // meshes are rebuilt. That is expensive, and a slider emits on + // every pixel of a drag, so the rebuild waits for the release — + // and only happens if the value actually moved. Message::IsolinesChanged(value) => { - self.set_drawing_tessellation_var(|header| header.isolines = value.max(0)); - Task::none() - } - - Message::DispSilhChanged(on) => { - self.set_drawing_tessellation_var(|header| header.display_silhouette = on); - Task::none() - } - - Message::SurfaceUChanged(value) => { - self.set_drawing_tessellation_var(|header| { - header.surface_u_density = value.clamp(0, 200) - }); - Task::none() - } - - Message::SurfaceVChanged(value) => { - self.set_drawing_tessellation_var(|header| { - header.surface_v_density = value.clamp(0, 200) - }); - Task::none() - } - - Message::SurfaceTypeChanged(value) => { - self.set_drawing_tessellation_var(|header| header.surface_type = value); - Task::none() - } - - Message::SolidHistChanged(record) => { - let i = self.active_tab; - if let Some(tab) = self.tabs.get_mut(i) { - tab.scene.document.header.record_solid_history = record; - tab.dirty = true; + let value = value.max(0); + let changed = self + .tabs + .get(self.active_tab) + .is_some_and(|tab| tab.scene.document.header.isolines != value); + if changed { + self.set_drawing_var(|header| header.isolines = value); + self.isolines_awaiting_regen = true; } Task::none() } + Message::IsolinesReleased => { + if std::mem::take(&mut self.isolines_awaiting_regen) { + self.regenerate_meshes(); + } + Task::none() + } + + // Read at render time, so nothing has to be rebuilt for it. + Message::DispSilhChanged(on) => { + self.set_drawing_var(|header| header.display_silhouette = on); + Task::none() + } + + // Inputs to PEDIT's mesh smoothing; they change nothing until it + // runs, so writing the header is the whole job. + Message::SurfaceUChanged(value) => { + self.set_drawing_var(|header| header.surface_u_density = value.clamp(0, 200)); + Task::none() + } + + Message::SurfaceVChanged(value) => { + self.set_drawing_var(|header| header.surface_v_density = value.clamp(0, 200)); + Task::none() + } + + Message::SurfaceTypeChanged(value) => { + self.set_drawing_var(|header| header.surface_type = value); + Task::none() + } + + // Whether history is recorded from now on; nothing to redraw. + Message::SolidHistChanged(record) => { + self.set_drawing_var(|header| header.record_solid_history = record); + Task::none() + } + + // Read while building geometry, so this one does bump — the same + // thing the SHOWHIST command does. Message::ShowHistChanged(mode) => { + self.set_drawing_var(|header| { + header.show_solid_history = mode.clamp(0, 2) + }); let i = self.active_tab; if let Some(tab) = self.tabs.get_mut(i) { - tab.scene.document.header.show_solid_history = mode.clamp(0, 2); tab.scene.bump_geometry(); - tab.dirty = true; } Task::none() } diff --git a/src/ui/window/options.rs b/src/ui/window/options.rs index 0923ebe21..5c8210df8 100644 --- a/src/ui/window/options.rs +++ b/src/ui/window/options.rs @@ -32,7 +32,6 @@ pub enum OptionsTab { Modeling, Selection, UserPreferences, - Drawing, } /// Application preferences the dialog reads that are plain scalars on the app. @@ -1077,29 +1076,7 @@ pub fn view_window<'a>( .spacing(0) .width(sizing.width); - let drawing = column![ - text(crate::t!("Block Edit")).size(15), - Space::new().height(10), - row![ - iced::widget::checkbox(double_click_block_refedit) - .on_toggle(Message::DoubleClickBlockRefeditChanged) - .size(15), - text(crate::t!("Double-click to edit block in-place (REFEDIT)")).size(12), - ] - .spacing(8) - .align_y(iced::Center), - Space::new().height(18), - row![ - iced::widget::checkbox(double_click_block_attedit) - .on_toggle(Message::DoubleClickBlockAtteditChanged) - .size(15), - text(crate::t!("Double-click attributed blocks to edit attributes (ATTEDIT)")).size(12), - ] - .spacing(8) - .align_y(iced::Center), - ] - .spacing(0) - .width(sizing.width); + // ANNOAUTOSCALE's magnitude decides which objects a newly added scale @@ -1225,6 +1202,26 @@ pub fn view_window<'a>( .size(11) .width(sizing.width), Space::new().height(24), + text(crate::t!("Block Edit")).size(15), + Space::new().height(10), + row![ + iced::widget::checkbox(double_click_block_refedit) + .on_toggle(Message::DoubleClickBlockRefeditChanged) + .size(15), + text(crate::t!("Double-click to edit block in-place (REFEDIT)")).size(12), + ] + .spacing(8) + .align_y(iced::Center), + Space::new().height(18), + row![ + iced::widget::checkbox(double_click_block_attedit) + .on_toggle(Message::DoubleClickBlockAtteditChanged) + .size(15), + text(crate::t!("Double-click attributed blocks to edit attributes (ATTEDIT)")).size(12), + ] + .spacing(8) + .align_y(iced::Center), + Space::new().height(24), text(crate::t!("Drawing Units")).size(15), Space::new().height(10), row![ @@ -1394,12 +1391,16 @@ pub fn view_window<'a>( .push( row![ text(crate::t!("Isolines per surface")).size(12).width(150), + // Rebuilding every solid's mesh is what makes a new + // isoline count reach solids that already exist, and it is + // far too costly to do on each pixel of a drag. slider( 0..=64, drawing_prefs.isolines.clamp(0, 64), Message::IsolinesChanged, ) .step(1i16) + .on_release(Message::IsolinesReleased) .width(Fill), text(drawing_prefs.isolines.clamp(0, 64).to_string()) .size(11) @@ -1565,7 +1566,6 @@ pub fn view_window<'a>( OptionsTab::Drafting => drafting.into(), OptionsTab::Modeling => modeling.into(), OptionsTab::UserPreferences => user_prefs.into(), - OptionsTab::Drawing => drawing.into(), }; // A vertical rail rather than a horizontal strip: the tab names are @@ -1588,7 +1588,6 @@ pub fn view_window<'a>( tab_button(crate::t!("3D Modeling"), OptionsTab::Modeling), tab_button(crate::t!("Selection"), OptionsTab::Selection), tab_button(crate::t!("User Preferences"), OptionsTab::UserPreferences), - tab_button(crate::t!("Drawing"), OptionsTab::Drawing), ] .spacing(2) .width(iced::Length::Fixed(TAB_RAIL_WIDTH));