fix(block-cache): address PR211 review follow-ups
- expand_defn: dedup the <1px baseline LOD branch the same way the <5px greek branch does, so colour-split MTEXT no longer stacks N overlapping baselines (one per \C segment) over the same OBB. - sysfont: memoise canonical_family_name process-wide. resolve_font calls it once per word on the MTEXT measure hot path and Face::resolve re-runs it right after; the fontdb query + linear family scans were uncached. - dimension/multileader/table: reword the 6 fill_tris_low FIXMEs. They cited a stale line and the wrong consumer — these fills render on the top-level path (panic-safe .get().unwrap_or), not the block cache, so they cannot trip emit_wire's debug_assert. State the real status: latent f32-precision debt, not a crash. Also fix the mis-indented text-fill WireModel in dimension.rs. - tests: add a host-independent colour-split test (two \C segments in a block keep ≥2 distinct wire colours — guards the per-wire colour fix) and a TTF-gated UTM fill test (paired, non-zero fill_tris_low). Note the arial-style test's LFF-fallback limitation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
parent
a84da122f4
commit
a93271c1bd
6 changed files with 174 additions and 31 deletions
|
|
@ -1261,9 +1261,11 @@ fn tessellate_dimension_inner(
|
||||||
plinegen: true,
|
plinegen: true,
|
||||||
vp_scissor: None,
|
vp_scissor: None,
|
||||||
fill_tris: geom.arrow_fill,
|
fill_tris: geom.arrow_fill,
|
||||||
// FIXME: fill_tris_low left empty — needs double-single split to match
|
// fill_tris_low intentionally empty: this fill renders on the top-level
|
||||||
// fill_tris. Any geometry from this path inside a block definition will
|
// path, where consumers (face3d_gpu, xclip) treat a short low half as
|
||||||
// trip the debug_assert_eq! in emit_wire (block_cache.rs:1397).
|
// all-zero, so it draws at f32 precision (sub-metre error at UTM scale)
|
||||||
|
// — not a crash. Follow-up: double-single-split via points_to_ds to
|
||||||
|
// match emit_wire's paired fill path.
|
||||||
fill_tris_low: Vec::new(),
|
fill_tris_low: Vec::new(),
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|
@ -1296,11 +1298,13 @@ fn tessellate_dimension_inner(
|
||||||
aabb: WireModel::UNBOUNDED_AABB,
|
aabb: WireModel::UNBOUNDED_AABB,
|
||||||
plinegen: true,
|
plinegen: true,
|
||||||
vp_scissor: None,
|
vp_scissor: None,
|
||||||
fill_tris: rect,
|
fill_tris: rect,
|
||||||
// FIXME: fill_tris_low left empty — needs double-single split to match
|
// fill_tris_low intentionally empty: this fill renders on the
|
||||||
// fill_tris. Any geometry from this path inside a block definition will
|
// top-level path, where consumers (face3d_gpu, xclip) treat a
|
||||||
// trip the debug_assert_eq! in emit_wire (block_cache.rs:1397).
|
// short low half as all-zero, so it draws at f32 precision
|
||||||
fill_tris_low: Vec::new(),
|
// (sub-metre error at UTM scale) — not a crash. Follow-up:
|
||||||
|
// double-single-split via points_to_ds to match emit_wire.
|
||||||
|
fill_tris_low: Vec::new(),
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -1304,9 +1304,11 @@ impl MultiLeaderTess for MultiLeader {
|
||||||
plinegen: true,
|
plinegen: true,
|
||||||
vp_scissor: None,
|
vp_scissor: None,
|
||||||
fill_tris: arrow_fill,
|
fill_tris: arrow_fill,
|
||||||
// FIXME: fill_tris_low left empty — needs double-single split to match
|
// fill_tris_low intentionally empty: this fill renders on the
|
||||||
// fill_tris. Any geometry from this path inside a block definition will
|
// top-level path, where consumers (face3d_gpu, xclip) treat a short
|
||||||
// trip the debug_assert_eq! in emit_wire (block_cache.rs:1397).
|
// low half as all-zero, so it draws at f32 precision (sub-metre
|
||||||
|
// error at UTM scale) — not a crash. Follow-up: double-single-split
|
||||||
|
// via points_to_ds to match emit_wire's paired fill path.
|
||||||
fill_tris_low: Vec::new(),
|
fill_tris_low: Vec::new(),
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|
@ -1596,9 +1598,11 @@ impl MultiLeaderTess for MultiLeader {
|
||||||
plinegen: true,
|
plinegen: true,
|
||||||
vp_scissor: None,
|
vp_scissor: None,
|
||||||
fill_tris: greek_tris,
|
fill_tris: greek_tris,
|
||||||
// FIXME: fill_tris_low left empty — needs double-single split to match
|
// fill_tris_low intentionally empty: this fill renders on
|
||||||
// fill_tris. Any geometry from this path inside a block definition will
|
// the top-level path, where consumers treat a short low
|
||||||
// trip the debug_assert_eq! in emit_wire (block_cache.rs:1397).
|
// half as all-zero, so it draws at f32 precision (sub-
|
||||||
|
// metre error at UTM scale) — not a crash. Follow-up:
|
||||||
|
// double-single-split via points_to_ds to match emit_wire.
|
||||||
fill_tris_low: Vec::new(),
|
fill_tris_low: Vec::new(),
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
@ -1676,9 +1680,11 @@ impl MultiLeaderTess for MultiLeader {
|
||||||
plinegen: true,
|
plinegen: true,
|
||||||
vp_scissor: None,
|
vp_scissor: None,
|
||||||
fill_tris: text_fill_tris,
|
fill_tris: text_fill_tris,
|
||||||
// FIXME: fill_tris_low left empty — needs double-single split to match
|
// fill_tris_low intentionally empty: this fill renders on
|
||||||
// fill_tris. Any geometry from this path inside a block definition will
|
// the top-level path, where consumers treat a short low
|
||||||
// trip the debug_assert_eq! in emit_wire (block_cache.rs:1397).
|
// half as all-zero, so it draws at f32 precision (sub-
|
||||||
|
// metre error at UTM scale) — not a crash. Follow-up:
|
||||||
|
// double-single-split via points_to_ds to match emit_wire.
|
||||||
fill_tris_low: Vec::new(),
|
fill_tris_low: Vec::new(),
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -755,9 +755,11 @@ pub fn tessellate_table(
|
||||||
plinegen: true,
|
plinegen: true,
|
||||||
vp_scissor: None,
|
vp_scissor: None,
|
||||||
fill_tris,
|
fill_tris,
|
||||||
// FIXME: fill_tris_low left empty — needs double-single split to match
|
// fill_tris_low intentionally empty: this fill renders on the
|
||||||
// fill_tris. Any geometry from this path inside a block definition will
|
// top-level path, where consumers (face3d_gpu, xclip) treat a
|
||||||
// trip the debug_assert_eq! in emit_wire (block_cache.rs:1397).
|
// short low half as all-zero, so it draws at f32 precision
|
||||||
|
// (sub-metre error at UTM scale) — not a crash. Follow-up:
|
||||||
|
// double-single-split via points_to_ds to match emit_wire.
|
||||||
fill_tris_low: Vec::new(),
|
fill_tris_low: Vec::new(),
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
|
|
|
||||||
12
src/scene/cache/block_cache.rs
vendored
12
src/scene/cache/block_cache.rs
vendored
|
|
@ -1138,6 +1138,17 @@ fn expand_defn(
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
if h_px < 1.0 {
|
if h_px < 1.0 {
|
||||||
|
// Colour-split MTEXT: only the first outline
|
||||||
|
// wire from a source entity emits the baseline
|
||||||
|
// substitute. Siblings share the same
|
||||||
|
// text_obb_local and would stack identical
|
||||||
|
// overlapping lines (last colour wins) — the
|
||||||
|
// same dedup the greek branch does, one tier
|
||||||
|
// down.
|
||||||
|
if lw.text_obb_local == last_lod_obb {
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
last_lod_obb = lw.text_obb_local;
|
||||||
emit_text_baseline(lw, accum_xform, ctx, out, wpp);
|
emit_text_baseline(lw, accum_xform, ctx, out, wpp);
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
@ -1431,7 +1442,6 @@ fn emit_wire(
|
||||||
entry.fill_tris.push([hx, hy, hz]);
|
entry.fill_tris.push([hx, hy, hz]);
|
||||||
entry.fill_tris_low.push([lx, ly, lz]);
|
entry.fill_tris_low.push([lx, ly, lz]);
|
||||||
}
|
}
|
||||||
|
|
||||||
}
|
}
|
||||||
|
|
||||||
fn transform_tangent(
|
fn transform_tangent(
|
||||||
|
|
|
||||||
|
|
@ -6,7 +6,8 @@
|
||||||
// TTF glyph engine). LFF stroke fonts stay separate — this is purely the
|
// TTF glyph engine). LFF stroke fonts stay separate — this is purely the
|
||||||
// TrueType side of the renderer.
|
// TrueType side of the renderer.
|
||||||
|
|
||||||
use std::sync::OnceLock;
|
use std::collections::HashMap;
|
||||||
|
use std::sync::{Mutex, OnceLock};
|
||||||
|
|
||||||
struct SysFonts {
|
struct SysFonts {
|
||||||
db: fontdb::Database,
|
db: fontdb::Database,
|
||||||
|
|
@ -38,7 +39,27 @@ pub fn families() -> &'static [String] {
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Resolve a requested family name to the canonical installed system family name (with exact case).
|
/// Resolve a requested family name to the canonical installed system family name (with exact case).
|
||||||
|
///
|
||||||
|
/// Memoised process-wide: `resolve_font` calls this once per word on the MTEXT
|
||||||
|
/// measure hot path for inline-`\f` TTF runs, and `Face::resolve` re-runs it
|
||||||
|
/// immediately after — the underlying fontdb query plus linear family scans are
|
||||||
|
/// not free. The cache keys on the raw request string; results are stable for
|
||||||
|
/// the process lifetime (the font DB is loaded once via `OnceLock`).
|
||||||
pub fn canonical_family_name(family: &str) -> Option<String> {
|
pub fn canonical_family_name(family: &str) -> Option<String> {
|
||||||
|
static CACHE: OnceLock<Mutex<HashMap<String, Option<String>>>> = OnceLock::new();
|
||||||
|
let cache = CACHE.get_or_init(|| Mutex::new(HashMap::new()));
|
||||||
|
if let Some(hit) = cache.lock().unwrap().get(family) {
|
||||||
|
return hit.clone();
|
||||||
|
}
|
||||||
|
let resolved = canonical_family_name_uncached(family);
|
||||||
|
cache
|
||||||
|
.lock()
|
||||||
|
.unwrap()
|
||||||
|
.insert(family.to_string(), resolved.clone());
|
||||||
|
resolved
|
||||||
|
}
|
||||||
|
|
||||||
|
fn canonical_family_name_uncached(family: &str) -> Option<String> {
|
||||||
let db = &fonts().db;
|
let db = &fonts().db;
|
||||||
|
|
||||||
// 1. Try exact match first
|
// 1. Try exact match first
|
||||||
|
|
|
||||||
|
|
@ -3,20 +3,28 @@ use acadrust::tables::{BlockRecord, TextStyle};
|
||||||
use acadrust::types::Vector3;
|
use acadrust::types::Vector3;
|
||||||
use acadrust::{CadDocument, EntityType, Handle};
|
use acadrust::{CadDocument, EntityType, Handle};
|
||||||
use OpenCADStudio::scene::cache::block_cache::{expand_insert, BlockCache};
|
use OpenCADStudio::scene::cache::block_cache::{expand_insert, BlockCache};
|
||||||
|
use OpenCADStudio::scene::WireModel;
|
||||||
|
|
||||||
fn drawable_point_count(wires: &[OpenCADStudio::scene::WireModel]) -> usize {
|
fn drawable_point_count(wires: &[WireModel]) -> usize {
|
||||||
wires
|
wires
|
||||||
.iter()
|
.iter()
|
||||||
.map(|w| w.points.iter().filter(|p| p[0].is_finite()).count() + w.fill_tris.len())
|
.map(|w| w.points.iter().filter(|p| p[0].is_finite()).count() + w.fill_tris.len())
|
||||||
.sum()
|
.sum()
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
/// Build a one-block document holding a single MTEXT and expand the INSERT
|
||||||
fn block_nested_mtext_uses_its_style_font() {
|
/// through the block cache, returning the finalized wires. `mtext_at` is the
|
||||||
|
/// MTEXT's position inside the block; `insert_at` is where the block is placed.
|
||||||
|
fn expand_block_mtext(
|
||||||
|
value: &str,
|
||||||
|
font_file: &str,
|
||||||
|
mtext_at: Vector3,
|
||||||
|
insert_at: Vector3,
|
||||||
|
) -> Vec<WireModel> {
|
||||||
let mut doc = CadDocument::new();
|
let mut doc = CadDocument::new();
|
||||||
|
|
||||||
let mut style = TextStyle::new("SHOP");
|
let mut style = TextStyle::new("SHOP");
|
||||||
style.font_file = "arial.ttf".to_string();
|
style.font_file = font_file.to_string();
|
||||||
doc.text_styles.add(style).unwrap();
|
doc.text_styles.add(style).unwrap();
|
||||||
|
|
||||||
let br_h = Handle::new(doc.next_handle());
|
let br_h = Handle::new(doc.next_handle());
|
||||||
|
|
@ -24,7 +32,7 @@ fn block_nested_mtext_uses_its_style_font() {
|
||||||
br.handle = br_h;
|
br.handle = br_h;
|
||||||
doc.block_records.add(br).unwrap();
|
doc.block_records.add(br).unwrap();
|
||||||
|
|
||||||
let mut mtext = MText::with_value("FERRAGAMO", Vector3::new(0.0, 0.0, 0.0));
|
let mut mtext = MText::with_value(value, mtext_at);
|
||||||
mtext.style = "SHOP".to_string();
|
mtext.style = "SHOP".to_string();
|
||||||
mtext.height = 20.0;
|
mtext.height = 20.0;
|
||||||
mtext.rectangle_width = 0.0;
|
mtext.rectangle_width = 0.0;
|
||||||
|
|
@ -32,10 +40,10 @@ fn block_nested_mtext_uses_its_style_font() {
|
||||||
sub.common_mut().owner_handle = br_h;
|
sub.common_mut().owner_handle = br_h;
|
||||||
doc.add_entity(sub).unwrap();
|
doc.add_entity(sub).unwrap();
|
||||||
|
|
||||||
let ins = Insert::new("LABEL_BLOCK", Vector3::new(100.0, 50.0, 0.0));
|
let ins = Insert::new("LABEL_BLOCK", insert_at);
|
||||||
doc.add_entity(EntityType::Insert(ins.clone())).unwrap();
|
doc.add_entity(EntityType::Insert(ins.clone())).unwrap();
|
||||||
let cache = BlockCache::build(&doc, 1.0, [0.0, 0.0, 0.0, 1.0]);
|
let cache = BlockCache::build(&doc, 1.0, [0.0, 0.0, 0.0, 1.0]);
|
||||||
let wires = expand_insert(
|
expand_insert(
|
||||||
&cache,
|
&cache,
|
||||||
&ins,
|
&ins,
|
||||||
Handle::new(999),
|
Handle::new(999),
|
||||||
|
|
@ -50,7 +58,22 @@ fn block_nested_mtext_uses_its_style_font() {
|
||||||
false,
|
false,
|
||||||
[0.0, 0.0, 0.0, 1.0],
|
[0.0, 0.0, 0.0, 1.0],
|
||||||
)
|
)
|
||||||
.expect("block defn is cached");
|
.expect("block defn is cached")
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn block_nested_mtext_uses_its_style_font() {
|
||||||
|
// NB: on a host without Arial, `arial.ttf` resolves to the LFF stroke
|
||||||
|
// fallback, so this exercises the "something renders" path rather than the
|
||||||
|
// TTF canonicalisation specifically. The font-canonicalisation logic itself
|
||||||
|
// is covered by the unit tests in `text_support`; the colour-split and
|
||||||
|
// fill-pairing tests below cover the block-cache rendering changes.
|
||||||
|
let wires = expand_block_mtext(
|
||||||
|
"FERRAGAMO",
|
||||||
|
"arial.ttf",
|
||||||
|
Vector3::new(0.0, 0.0, 0.0),
|
||||||
|
Vector3::new(100.0, 50.0, 0.0),
|
||||||
|
);
|
||||||
|
|
||||||
assert!(
|
assert!(
|
||||||
drawable_point_count(&wires) > 0,
|
drawable_point_count(&wires) > 0,
|
||||||
|
|
@ -58,7 +81,84 @@ fn block_nested_mtext_uses_its_style_font() {
|
||||||
);
|
);
|
||||||
|
|
||||||
assert!(
|
assert!(
|
||||||
wires.iter().all(|w| w.points.is_empty() || w.fill_tris.is_empty()),
|
wires
|
||||||
|
.iter()
|
||||||
|
.all(|w| w.points.is_empty() || w.fill_tris.is_empty()),
|
||||||
"outline and fill wires should be separate for correct GPU classification"
|
"outline and fill wires should be separate for correct GPU classification"
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn block_nested_colour_split_mtext_keeps_per_wire_colour() {
|
||||||
|
// `\C1;` = ACI red, `\C2;` = ACI yellow → two colour bins. The block cache
|
||||||
|
// must keep them as separate per-wire colours; the fold-to-one-colour bug
|
||||||
|
// this PR fixes silently collapsed every segment to the first colour. Uses
|
||||||
|
// the builtin stroke font so the split is observable without a system TTF.
|
||||||
|
let wires = expand_block_mtext(
|
||||||
|
"\\C1;AAA\\C2;BBB",
|
||||||
|
"txt",
|
||||||
|
Vector3::new(0.0, 0.0, 0.0),
|
||||||
|
Vector3::new(100.0, 50.0, 0.0),
|
||||||
|
);
|
||||||
|
|
||||||
|
let mut colours: Vec<[u8; 3]> = wires
|
||||||
|
.iter()
|
||||||
|
.filter(|w| !w.points.is_empty() || !w.fill_tris.is_empty())
|
||||||
|
.map(|w| {
|
||||||
|
[
|
||||||
|
(w.color[0] * 255.0).round() as u8,
|
||||||
|
(w.color[1] * 255.0).round() as u8,
|
||||||
|
(w.color[2] * 255.0).round() as u8,
|
||||||
|
]
|
||||||
|
})
|
||||||
|
.collect();
|
||||||
|
colours.sort();
|
||||||
|
colours.dedup();
|
||||||
|
|
||||||
|
assert!(
|
||||||
|
colours.len() >= 2,
|
||||||
|
"colour-split MTEXT in a block must keep ≥2 distinct wire colours, got {colours:?}"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn block_nested_mtext_fill_tris_keep_paired_low_half_at_utm() {
|
||||||
|
// TTF glyph fills are host-dependent (builtin LFF stroke fonts produce
|
||||||
|
// none). When the host resolves a TTF and fills ARE produced, every fill
|
||||||
|
// wire must carry an index-paired `fill_tris_low`: emit_wire reconstructs
|
||||||
|
// `fill_tris[i] + fill_tris_low[i]`, so an unpaired wire panics (release
|
||||||
|
// too) and a dropped low half quantizes fills to ~0.5 m at UTM scale.
|
||||||
|
// The MTEXT sits at a UTM-scale coordinate so the low half is significant.
|
||||||
|
// "DejaVu Sans" is present on most Linux hosts; where no TTF resolves the
|
||||||
|
// fill set is empty and the assertions are skipped (gate below).
|
||||||
|
let wires = expand_block_mtext(
|
||||||
|
"FERRAGAMO",
|
||||||
|
"DejaVu Sans",
|
||||||
|
Vector3::new(500_000.0, 4_000_000.0, 0.0),
|
||||||
|
Vector3::new(0.0, 0.0, 0.0),
|
||||||
|
);
|
||||||
|
|
||||||
|
let fill_wires: Vec<&WireModel> = wires.iter().filter(|w| !w.fill_tris.is_empty()).collect();
|
||||||
|
if fill_wires.is_empty() {
|
||||||
|
eprintln!("no TTF fills resolvable on this host; skipping fill-pairing assertions");
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
for w in &fill_wires {
|
||||||
|
assert_eq!(
|
||||||
|
w.fill_tris.len(),
|
||||||
|
w.fill_tris_low.len(),
|
||||||
|
"fill_tris and fill_tris_low must be index-paired for emit_wire"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
let any_nonzero_low = fill_wires
|
||||||
|
.iter()
|
||||||
|
.flat_map(|w| w.fill_tris_low.iter())
|
||||||
|
.any(|p| p.iter().any(|&c| c != 0.0));
|
||||||
|
assert!(
|
||||||
|
any_nonzero_low,
|
||||||
|
"UTM-scale fills must keep a non-zero low half — a dropped fill_tris_low \
|
||||||
|
silently quantizes fill triangles to ~0.5 m"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue