From 5ffbf598a90d1ddd19c6f6b37061538b389a6bde Mon Sep 17 00:00:00 2001 From: Hakan Seven Date: Thu, 10 Sep 2026 18:05:42 +0300 Subject: [PATCH] refactor(display): trim isolines follow-up comments --- src/app/commands/display.rs | 46 +++---------------------------------- src/app/mod.rs | 4 +--- src/app/settings.rs | 13 +---------- src/app/update/file.rs | 13 +---------- src/app/update/mod.rs | 16 +------------ src/ui/window/options.rs | 7 +----- 6 files changed, 8 insertions(+), 91 deletions(-) diff --git a/src/app/commands/display.rs b/src/app/commands/display.rs index 4a84ccba8..91cdaca00 100644 --- a/src/app/commands/display.rs +++ b/src/app/commands/display.rs @@ -502,23 +502,8 @@ impl OpenCADStudio { self.command_line.push_output(crate::t!("REDRAWALL: viewports refreshed.").as_ref()); return Some(Task::none()); } - // REGEN — full model regeneration (bump_geometry: geometry_epoch AND - // block_epoch; C4). No undo, no DB mutation, so do NOT touch - // self.tabs[i].dirty — a newly opened drawing must not become - // "modified" merely because tessellation caches were invalidated (C7). - // REGENALL is functionally identical (C5). + // Rebuild tessellation caches without modifying the document. "REGEN" | "REGENALL" => { - // 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()); @@ -1903,32 +1888,13 @@ mod tests { let _ = app.run_command_line("REGENALL"); 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; @@ -1937,17 +1903,14 @@ mod regen_tests { assert!( !app.tabs[i].scene.meshes.contains_key(&stale), - "REGEN must rebuild the mesh map, not leave what was there", + "REGEN left a stale mesh", ); assert_ne!( app.tabs[i].scene.geometry_epoch, epoch_before, - "and it still has to bump the geometry epoch", + "REGEN did not 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; @@ -1978,8 +1941,6 @@ mod regen_tests { "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); @@ -2010,4 +1971,3 @@ mod regen_tests { } } } - diff --git a/src/app/mod.rs b/src/app/mod.rs index 3a87326f8..0c2cd3e73 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -517,9 +517,7 @@ 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. + /// Defer the ISOLINES mesh rebuild until the slider is released. 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. diff --git a/src/app/settings.rs b/src/app/settings.rs index dcacee388..f40d41d54 100644 --- a/src/app/settings.rs +++ b/src/app/settings.rs @@ -203,12 +203,7 @@ pub struct UserSettings { /// GRIPOBJLIMIT: past this many selected objects, no grips are drawn at /// 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. + /// Last Options page; unknown saved names fall back without rejecting the config. #[serde(default, deserialize_with = "deserialize_options_tab")] pub options_tab: crate::ui::window::options::OptionsTab, /// Show the navigation cube (NAVVCUBE). @@ -455,11 +450,6 @@ mod tests { 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#"{ @@ -479,4 +469,3 @@ mod tests { assert_eq!(cfg.settings.savetime_min, 42); } } - diff --git a/src/app/update/file.rs b/src/app/update/file.rs index c8352555b..518d5c4a5 100644 --- a/src/app/update/file.rs +++ b/src/app/update/file.rs @@ -356,13 +356,6 @@ impl OpenCADStudio { } /// Write a header variable on the active drawing and mark it modified. - /// - /// 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), @@ -374,11 +367,7 @@ impl OpenCADStudio { } } - /// 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. + /// Re-tessellate active-drawing solids after an ISOLINES change. pub(in crate::app) fn regenerate_meshes(&mut self) { let i = self.active_tab; if let Some(tab) = self.tabs.get_mut(i) { diff --git a/src/app/update/mod.rs b/src/app/update/mod.rs index e93374c43..3fbe1ccf6 100644 --- a/src/app/update/mod.rs +++ b/src/app/update/mod.rs @@ -6305,15 +6305,7 @@ impl OpenCADStudio { Task::none() } - // 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. + // ISOLINES is baked into solid meshes, so rebuild once on release. Message::IsolinesChanged(value) => { let value = value.max(0); let changed = self @@ -6334,14 +6326,11 @@ impl OpenCADStudio { 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() @@ -6357,14 +6346,11 @@ impl OpenCADStudio { 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) diff --git a/src/ui/window/options.rs b/src/ui/window/options.rs index 5c8210df8..3c28d3700 100644 --- a/src/ui/window/options.rs +++ b/src/ui/window/options.rs @@ -1076,9 +1076,6 @@ pub fn view_window<'a>( .spacing(0) .width(sizing.width); - - - // ANNOAUTOSCALE's magnitude decides which objects a newly added scale // reaches — 1 skips layers that are off, frozen, locked or frozen in the // viewport, 2 keeps locked ones, 3 skips only locked, 4 takes everything. @@ -1391,9 +1388,7 @@ 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. + // Retessellate once when dragging ends. slider( 0..=64, drawing_prefs.isolines.clamp(0, 64),