From 7e12dab89e0f3ce5891d7b026d513ce0fd3c1981 Mon Sep 17 00:00:00 2001 From: Panos <25664832+mechpanos@users.noreply.github.com> Date: Thu, 1 Oct 2026 17:42:38 +0300 Subject: [PATCH] fix(image): erasing a broken raster reference clears its placeholder text A raster reference that cannot be resolved is the only framed entity whose to_render emits a RenderObject::Text: the frame and its diagonals are lines, but the saved file path becomes a real glyph run in the SDF text buffer. gather_text_verts reuses the whole previous vertex buffer when text_unchanged(epoch) says no text-bearing entity moved, and that asks entity_affects_text_cache -- which enumerated Text, MText, Dimension, MultiLeader, Leader, Table, Tolerance, attributes and Insert, but not RasterImage. So an erase never raised CACHE_CATEGORY_TEXT, the stale buffer was taken as valid, and the path kept drawing over empty space. The reuse branch then re-stamps that buffer with the current epoch, which froze it until a full rebuild. CACHE_CATEGORY_IMAGE did not cover the gap: it is set from images.contains_key(handle), and a missing reference has no ImageModel at all. The same gap broke three more flows from the Added/Modified side: ATTACH of a missing reference showed no text, MOVE and grip-drag left the text behind while the frame followed, and UNDO after an erase did not bring it back. entity_affects_text_cache now counts a RasterImage whose reference does not resolve, reusing the converter's memoised resolve_image probe so the two cannot disagree. Kept a probe rather than a blanket type match so a healthy image does not rebuild every glyph in the drawing on each mouse move of a grip-drag. Four regression tests, each confirmed to fail without the fix. The main one is end-to-end on the buffer the GPU draws: before, the erased placeholder left 300 glyph vertices on screen (50 chars x 6). Also: to_render ran the same resolve probe twice; now once, shared by the outline and placeholder branches. Moved text_unchanged's doc comment off draw_ranks_stable, where it had drifted, and recorded the rule that every entity able to emit a GlyphRun must be declared there. Co-Authored-By: Claude Opus 5 --- src/entities/raster_image.rs | 27 +++---- src/scene/mod.rs | 136 +++++++++++++++++++++++++++++++++-- 2 files changed, 144 insertions(+), 19 deletions(-) diff --git a/src/entities/raster_image.rs b/src/entities/raster_image.rs index 87a51dab4..2de0fc5ff 100644 --- a/src/entities/raster_image.rs +++ b/src/entities/raster_image.rs @@ -142,9 +142,11 @@ impl RenderConvertible for RasterImage { // Diagonals are the BROKEN-reference placeholder; a resolvable image // draws its pixels inside the frame, so the X would scribble over it. - let path_probe = self.file_path.trim(); - let resolvable = path_probe.is_empty() - || crate::scene::model::image_model::resolve_image(path_probe).is_some(); + // One probe for the whole conversion: the outline below and the + // placeholder branch further down must agree on a single answer. + let path = self.file_path.trim(); + let resolvable = + path.is_empty() || crate::scene::model::image_model::resolve_image(path).is_some(); // Outline: the clip boundary, or — when it hides its inside — the // frame with the boundary as a hole. Must match the raster's own clip // triangulation in `ImageModel` so outline and pixels align. @@ -166,16 +168,15 @@ impl RenderConvertible for RasterImage { // A raster OCS can display renders its pixels (built separately) inside // this frame — just draw the frame/clip outline. A reference it cannot - // resolve (an offline/broken URL, a missing or renamed file) gets - // AutoCAD's broken-reference treatment: the frame plus the saved path - // drawn as text, so the user sees WHICH reference is unresolved instead - // of an empty box. `resolve_image` is memoised and shared with the - // raster loader, so a URL that fetches online is treated as resolvable - // (no placeholder — the image shows) while an offline one falls back to - // the path text, and neither is fetched twice. - let path = self.file_path.trim(); - let resolvable = - path.is_empty() || crate::scene::model::image_model::resolve_image(path).is_some(); + // resolve (an offline/broken URL, a missing or renamed file) gets the + // usual broken-reference treatment: the frame plus the saved path drawn + // as text, so the user sees WHICH reference is unresolved instead of an + // empty box. `resolve_image` is memoised and shared with the raster + // loader, so a URL that fetches online is treated as resolvable (no + // placeholder — the image shows) while an offline one falls back to the + // path text, and neither is fetched twice. The scene's text-cache + // classifier runs the same probe, so erasing this placeholder drops its + // glyphs from the text buffer instead of leaving them on screen. if resolvable { return Some(RenderEntity { // Picked on its frame only (shown or hidden), like the other diff --git a/src/scene/mod.rs b/src/scene/mod.rs index 151a808bf..222a288c0 100644 --- a/src/scene/mod.rs +++ b/src/scene/mod.rs @@ -3008,6 +3008,10 @@ impl Scene { Some(state.into_iter().collect()) } + /// Whether this entity contributes glyphs to the SDF text buffer, so a + /// change to it must invalidate the cached text vertices. Every entity whose + /// `to_render` can emit a `GlyphRun` belongs here — miss one and its glyphs + /// linger after an erase and fail to appear after an add. fn entity_affects_text_cache(&self, entity: &EntityType) -> bool { if matches!( entity, @@ -3024,6 +3028,21 @@ impl Scene { ) { return true; } + // A raster reference it cannot resolve draws the saved path as a glyph + // run in place of the picture (`RasterImage::to_render`), so its + // broken-reference placeholder lives in the text buffer; a resolvable + // one contributes only its frame. Same memoised probe the converter + // uses, so the two cannot disagree — and keeping it a probe rather than + // a blanket type match keeps a working image's grip-drag from + // rebuilding every glyph in the drawing on each mouse move. + if let EntityType::RasterImage(image) = entity { + let path = image.file_path.trim(); + if !path.is_empty() + && crate::scene::model::image_model::resolve_image(path).is_none() + { + return true; + } + } // Complex-linetype glyphs ride the host entity's wire. let lt = crate::scene::view::render::linetype_name_for(&self.document, entity); crate::io::linetypes::resolve_complex_lt(&self.document, <).is_some() @@ -3117,12 +3136,6 @@ impl Scene { true } - /// True when no text-bearing entity changed since `last_epoch`, so cached SDF - /// glyphs stay valid. Text comes from Text / MText / Dimension / MultiLeader / - /// Leader / Table / Tolerance / attributes (incl. ATTDEF) and from block - /// references (their baked text moves with the instance) — an edit to any of - /// those, or any removal, invalidates it; a plain line / arc / polyline edit - /// does not. /// Whether the per-entity draw-order labels can be replayed since /// `last_epoch`. Add/Remove keep every existing label stable; only a full /// structural delta (DRAWORDER, file/layout/block rebuild, journal overflow) @@ -3131,6 +3144,15 @@ impl Scene { self.replay_since(last_epoch).is_some() } + /// True when no text-bearing entity changed since `last_epoch`, so cached SDF + /// glyphs stay valid. Text comes from Text / MText / Dimension / MultiLeader / + /// Leader / Table / Tolerance / attributes (incl. ATTDEF), from block + /// references (their baked text moves with the instance), and from a raster + /// image whose reference cannot be resolved (it draws the saved path in place + /// of the picture) — an edit or removal of any of those invalidates it; a + /// plain line / arc / polyline edit does not. See + /// [`Scene::entity_affects_text_cache`], which classifies both directions: + /// live entities for an add/edit, the pre-erase category mask for a removal. fn text_unchanged(&self, last_epoch: u64) -> bool { self.category_cache_valid(last_epoch, CACHE_CATEGORY_TEXT, |handle| { self.document @@ -13060,6 +13082,108 @@ mod journal_tests { ); assert!(s.text_unchanged(cached_epoch)); } + + /// A raster image whose file cannot be resolved draws the saved path as a + /// glyph run instead of the picture, so erasing it MUST invalidate the SDF + /// text buffer — otherwise `gather_text_verts` reuses the previous vertex + /// list and the path text keeps drawing over the now-empty frame. + #[test] + fn erasing_broken_raster_reference_invalidates_text_cache() { + let mut s = Scene::new(); + let image = s.add_entity(EntityType::RasterImage(broken_raster("erase"))); + let cached_epoch = s.geometry_epoch; + s.erase_entities(&[image]); + + assert!( + !s.text_unchanged(cached_epoch), + "erasing a broken raster reference must drop its placeholder glyphs" + ); + } + + /// The same classification in the other direction: attaching (and, through + /// the identical Added/Modified path, moving or undoing the erase of) a + /// broken reference must invalidate the text buffer so its path text appears. + #[test] + fn attaching_broken_raster_reference_invalidates_text_cache() { + let mut s = Scene::new(); + let cached_epoch = s.geometry_epoch; + s.add_entity(EntityType::RasterImage(broken_raster("attach"))); + + assert!( + !s.text_unchanged(cached_epoch), + "attaching a broken raster reference must build its placeholder glyphs" + ); + } + + /// The converse perf property: a raster that resolves contributes no glyphs, + /// so editing or erasing it must leave the text buffer warm — a grip-drag of + /// a working image cannot re-walk every glyph in the drawing per mouse move. + /// An empty path counts as resolvable to both the converter and the + /// classifier, so this needs no picture on disk. + #[test] + fn erasing_resolvable_raster_keeps_text_cache_warm() { + use codec::entities::RasterImage; + use codec::types::Vector3; + + let mut s = Scene::new(); + let image = s.add_entity(EntityType::RasterImage(RasterImage::with_size( + "", + Vector3::new(0.0, 0.0, 0.0), + 16.0, + 16.0, + 10.0, + 10.0, + ))); + let cached_epoch = s.geometry_epoch; + s.erase_entities(&[image]); + + assert!(s.text_unchanged(cached_epoch)); + } + + /// End-to-end on the buffer the GPU actually draws: the placeholder's path + /// glyphs must be gone from the gathered SDF vertices after the erase. This + /// is the shape of the reported bug — `gather_text_verts` misses its content- + /// id cache on every geometry edit and then reuses the previous vertex list + /// whenever `text_unchanged` says the text is untouched. + #[test] + fn erasing_broken_raster_reference_drops_its_glyphs_from_the_text_buffer() { + let depth_map = rustc_hash::FxHashMap::default(); + let mut s = Scene::new(); + let image = s.add_entity(EntityType::RasterImage(broken_raster("gather"))); + + let wires = s.entity_wires(); + let before = s.gather_text_verts(&wires, 1, 7, &depth_map); + assert!( + !before.is_empty(), + "a broken raster reference must contribute its path glyphs" + ); + + s.erase_entities(&[image]); + let wires = s.entity_wires(); + let after = s.gather_text_verts(&wires, 2, 7, &depth_map); + assert!( + after.is_empty(), + "the erased placeholder left {} glyph vertices on screen", + after.len() + ); + } + + /// A raster pointing at a path that cannot exist. `resolve_image` memoises + /// per path for the life of the process, so each test uses its own path and + /// cannot inherit another's cached answer. + fn broken_raster(tag: &str) -> codec::entities::RasterImage { + use codec::entities::RasterImage; + use codec::types::Vector3; + + RasterImage::with_size( + &format!("/nonexistent-ocs-test/{tag}/missing-reference.png"), + Vector3::new(0.0, 0.0, 0.0), + 16.0, + 16.0, + 10.0, + 10.0, + ) + } } /// Delta-undo round-trips: a recorded entity-only edit must be exactly