forked from 1b-agent/opencadstudio-upstream
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 <noreply@anthropic.com>
This commit is contained in:
1 parent
350742f2b0
commit
7e12dab89e
2 files changed
+144
-19
No files matched your search
@@ -142,9 +142,11 @@ impl RenderConvertible for RasterImage {
|
|||||||
|
|
||||||
// Diagonals are the BROKEN-reference placeholder; a resolvable image
|
// Diagonals are the BROKEN-reference placeholder; a resolvable image
|
||||||
// draws its pixels inside the frame, so the X would scribble over it.
|
// draws its pixels inside the frame, so the X would scribble over it.
|
||||||
let path_probe = self.file_path.trim();
|
// One probe for the whole conversion: the outline below and the
|
||||||
let resolvable = path_probe.is_empty()
|
// placeholder branch further down must agree on a single answer.
|
||||||
|| crate::scene::model::image_model::resolve_image(path_probe).is_some();
|
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
|
// 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
|
// frame with the boundary as a hole. Must match the raster's own clip
|
||||||
// triangulation in `ImageModel` so outline and pixels align.
|
// 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
|
// A raster OCS can display renders its pixels (built separately) inside
|
||||||
// this frame — just draw the frame/clip outline. A reference it cannot
|
// this frame — just draw the frame/clip outline. A reference it cannot
|
||||||
// resolve (an offline/broken URL, a missing or renamed file) gets
|
// resolve (an offline/broken URL, a missing or renamed file) gets the
|
||||||
// AutoCAD's broken-reference treatment: the frame plus the saved path
|
// usual broken-reference treatment: the frame plus the saved path drawn
|
||||||
// drawn as text, so the user sees WHICH reference is unresolved instead
|
// as text, so the user sees WHICH reference is unresolved instead of an
|
||||||
// of an empty box. `resolve_image` is memoised and shared with the
|
// empty box. `resolve_image` is memoised and shared with the raster
|
||||||
// raster loader, so a URL that fetches online is treated as resolvable
|
// loader, so a URL that fetches online is treated as resolvable (no
|
||||||
// (no placeholder — the image shows) while an offline one falls back to
|
// placeholder — the image shows) while an offline one falls back to the
|
||||||
// the path text, and neither is fetched twice.
|
// path text, and neither is fetched twice. The scene's text-cache
|
||||||
let path = self.file_path.trim();
|
// classifier runs the same probe, so erasing this placeholder drops its
|
||||||
let resolvable =
|
// glyphs from the text buffer instead of leaving them on screen.
|
||||||
path.is_empty() || crate::scene::model::image_model::resolve_image(path).is_some();
|
|
||||||
if resolvable {
|
if resolvable {
|
||||||
return Some(RenderEntity {
|
return Some(RenderEntity {
|
||||||
// Picked on its frame only (shown or hidden), like the other
|
// Picked on its frame only (shown or hidden), like the other
|
||||||
|
|||||||
+130
-6
@@ -3008,6 +3008,10 @@ impl Scene {
|
|||||||
Some(state.into_iter().collect())
|
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 {
|
fn entity_affects_text_cache(&self, entity: &EntityType) -> bool {
|
||||||
if matches!(
|
if matches!(
|
||||||
entity,
|
entity,
|
||||||
@@ -3024,6 +3028,21 @@ impl Scene {
|
|||||||
) {
|
) {
|
||||||
return true;
|
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.
|
// Complex-linetype glyphs ride the host entity's wire.
|
||||||
let lt = crate::scene::view::render::linetype_name_for(&self.document, entity);
|
let lt = crate::scene::view::render::linetype_name_for(&self.document, entity);
|
||||||
crate::io::linetypes::resolve_complex_lt(&self.document, <).is_some()
|
crate::io::linetypes::resolve_complex_lt(&self.document, <).is_some()
|
||||||
@@ -3117,12 +3136,6 @@ impl Scene {
|
|||||||
true
|
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
|
/// Whether the per-entity draw-order labels can be replayed since
|
||||||
/// `last_epoch`. Add/Remove keep every existing label stable; only a full
|
/// `last_epoch`. Add/Remove keep every existing label stable; only a full
|
||||||
/// structural delta (DRAWORDER, file/layout/block rebuild, journal overflow)
|
/// structural delta (DRAWORDER, file/layout/block rebuild, journal overflow)
|
||||||
@@ -3131,6 +3144,15 @@ impl Scene {
|
|||||||
self.replay_since(last_epoch).is_some()
|
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 {
|
fn text_unchanged(&self, last_epoch: u64) -> bool {
|
||||||
self.category_cache_valid(last_epoch, CACHE_CATEGORY_TEXT, |handle| {
|
self.category_cache_valid(last_epoch, CACHE_CATEGORY_TEXT, |handle| {
|
||||||
self.document
|
self.document
|
||||||
@@ -13060,6 +13082,108 @@ mod journal_tests {
|
|||||||
);
|
);
|
||||||
assert!(s.text_unchanged(cached_epoch));
|
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
|
/// Delta-undo round-trips: a recorded entity-only edit must be exactly
|
||||||
|
|||||||
Reference in new issue
Block a user