From 62d72b3a8de5b6e16360dc6e60fe06f1c41d1e23 Mon Sep 17 00:00:00 2001 From: Hakan Seven Date: Tue, 6 Oct 2026 20:54:26 +0300 Subject: [PATCH] review(#1630): one table rebuild loop that also refreshes formulas; one cell layout - main already rebuilt an edited table's block; the PR added a second loop after it that could never fire. Formula refresh and the block cache bump now live in the one loop. - Formula refresh copies the drawing only for a table that has a field. - A field cell lays its text out once and boxes it from those cells. - A zero text height at one level falls through to the next instead of ending the lookup. - Field box constants and BACKGROUND keep their own doc comments. Co-Authored-By: Claude Opus 5.5 --- src/entities/field.rs | 14 ++++++++++++-- src/entities/table.rs | 37 +++++++++++++------------------------ src/scene/mod.rs | 24 +++++++++--------------- 3 files changed, 34 insertions(+), 41 deletions(-) diff --git a/src/entities/field.rs b/src/entities/field.rs index 6ae918f01..3be2197a4 100644 --- a/src/entities/field.rs +++ b/src/entities/field.rs @@ -137,7 +137,17 @@ pub fn update_fields( /// Recompute a table's cell formulas into its block after the table changed; /// returns the cell texts that changed. pub fn refresh_table_formulas(document: &mut CadDocument, table: Handle) -> Vec { - if document.fields.is_empty() { + // Copying the drawing is only worth it for a table with a field. + let has_field = matches!( + document.get_entity(table), + Some(codec::EntityType::Table(t)) if t + .rows + .iter() + .flat_map(|row| &row.cells) + .flat_map(|cell| &cell.contents) + .any(|content| content.field_handle.is_some()) + ); + if !has_field { return Vec::new(); } let snapshot = document.clone(); @@ -332,13 +342,13 @@ pub fn set_display(on: bool) { FIELD_DISPLAY.store(on, std::sync::atomic::Ordering::Relaxed); } -/// The gray the reference draws behind a field. /// FIELDDISPLAY box extent around a line of field text, in text heights: /// from a third of the height below the baseline to 1.19 heights above it /// (the reference's box, measured at 2.5 and 10 unit text). pub const BOX_BELOW: f64 = 0.345; pub const BOX_ABOVE: f64 = 1.19; +/// The gray the reference draws behind a field. pub const BACKGROUND: [f32; 4] = [203.0 / 255.0, 203.0 / 255.0, 203.0 / 255.0, 1.0]; /// Whether `entity` hosts a field: its extension dictionary has ACAD_FIELD. diff --git a/src/entities/table.rs b/src/entities/table.rs index e4ebca599..80805b004 100644 --- a/src/entities/table.rs +++ b/src/entities/table.rs @@ -135,11 +135,14 @@ fn cell_text_height( content: &codec::entities::table::CellContent, ) -> Option { use codec::entities::table::CellStylePropertyFlags as P; + let set = |h: f64| (h > 1e-6).then_some(h); let own = content.sets(P::TEXT_HEIGHT); - own.then_some(content.text_height) - .or_else(|| style_for_property(table, row, column, cell, P::TEXT_HEIGHT).map(|s| s.text_height)) - .or(Some(content.text_height)) - .filter(|h| *h > 1e-6) + own.then(|| set(content.text_height)) + .flatten() + .or_else(|| { + style_for_property(table, row, column, cell, P::TEXT_HEIGHT).and_then(|s| set(s.text_height)) + }) + .or_else(|| set(content.text_height)) } /// A cell's text style: the content's own, else the cell, row, column or @@ -1960,6 +1963,8 @@ pub fn tessellate_table( .map(|style| style.rotation as f32) .unwrap_or(cell.rotation as f32) }; + // FIELDDISPLAY boxes come from this same layout's cells. + let field_boxes = content.field_handle.is_some() && crate::entities::field::display(); let layout = layout_mtext(&MTextRenderOpts { columns: Default::default(), value: text, @@ -1974,31 +1979,15 @@ pub fn tessellate_table( exact_line_spacing: false, rectangle_height: 0.0, vertical_text: false, - want_glyph_boxes: false, + want_glyph_boxes: field_boxes, }); // FIELDDISPLAY: a field cell's text on a gray box, one per line, // from the same layout's character cells (screen only). - if content.field_handle.is_some() && crate::entities::field::display() { - let cells = layout_mtext(&MTextRenderOpts { - columns: Default::default(), - value: text, - insertion: [to.x as f64, to.y as f64, to.z as f64], - height: cell_h, - rect_w: (col_width - margin_left - margin_right).max(0.0), - rotation: rot, - style: &resolved, - attach_h_anchor, - v_anchor, - line_spacing_factor: 1.0, - exact_line_spacing: false, - rectangle_height: 0.0, - vertical_text: false, - want_glyph_boxes: true, - }) - .glyph_boxes; + if field_boxes { + let cells = &layout.glyph_boxes; let (sin_r, cos_r) = (rot as f64).sin_cos(); let mut lines: Vec<[f64; 4]> = Vec::new(); - for b in &cells { + for b in cells { let [l, base, r, top] = b.local.map(|v| v as f64); match lines.last_mut() { Some(c) if (c[1] - base).abs() <= 1e-3 * (top - base).max(c[3]) => { diff --git a/src/scene/mod.rs b/src/scene/mod.rs index 58dc978f9..464f65aa3 100644 --- a/src/scene/mod.rs +++ b/src/scene/mod.rs @@ -3617,7 +3617,9 @@ impl Scene { // (the *T block) redrawn from the cells. Tables keeping their picture // (moved, restyled, opened from a file) are never redrawn: the block // may hold authored content the redraw does not reproduce. + // Its cell formulas are worked out again into that block. if !appearance_only { + let mut rebuilt = false; for (handle, kind) in &changes { if !matches!(kind, ChangeKind::Removed) && matches!( @@ -3625,9 +3627,15 @@ impl Scene { Some(EntityType::Table(t)) if t.block_record_handle.is_none() ) { - self.document.refresh_table_block(*handle); + rebuilt |= self.document.refresh_table_block(*handle); + rebuilt |= !crate::entities::field::refresh_table_formulas(&mut self.document, *handle) + .is_empty(); } } + // The block's cached drawing is stale once its texts change. + if rebuilt { + self.block_epoch = GEOMETRY_EPOCH.fetch_add(1, Ordering::Relaxed); + } } // A restyle (layer on/off, colour, text or dimension style) moves no // geometry, so nothing measured from it is re-resolved: an @@ -3695,20 +3703,6 @@ impl Scene { } } } - // An edited table lets go of its block; rebuild it from the cells so - // the drawing shows and saves the cell texts and grid it now holds. - let mut rebuilt_table_block = false; - for (handle, kind) in &changes { - if !matches!(kind, ChangeKind::Removed) - && matches!(self.document.get_entity(*handle), Some(EntityType::Table(t)) if t.block_record_handle.is_none()) - { - rebuilt_table_block |= self.document.refresh_table_block(*handle); - rebuilt_table_block |= !crate::entities::field::refresh_table_formulas(&mut self.document, *handle).is_empty(); - } - } - if rebuilt_table_block { - self.block_epoch = GEOMETRY_EPOCH.fetch_add(1, Ordering::Relaxed); - } if !changes.is_empty() { self.refresh_dependency_index_for_changes(&changes); }