diff --git a/iris/core/src/render/primitive.rs b/iris/core/src/render/primitive.rs index c2475c3..38a4397 100644 --- a/iris/core/src/render/primitive.rs +++ b/iris/core/src/render/primitive.rs @@ -6,6 +6,7 @@ use crate::{ ArrBuf, data::{MaskIdx, MoveIdx, PrimitiveInstance}, }, + util::HashSet, }; use bytemuck::Pod; use wgpu::*; @@ -277,6 +278,23 @@ impl Primitives { } } + /// Every instance that is still bound for the GPU, as `(inst_idx, + /// owner, is_image)` -- everything except the slots already handed to + /// [`Self::free`] and waiting for [`Self::apply_free`] to compact them + /// away. Only [`crate::UiRenderState::orphaned_primitives`] uses this, + /// to check that every drawn primitive still belongs to a live widget. + pub fn live_instances(&self) -> impl Iterator + '_ { + let free: HashSet = self.free.iter().copied().collect(); + let image_free: HashSet = self.image_free.iter().copied().collect(); + let rects = (0..self.instances.len()) + .filter(move |i| !free.contains(i)) + .map(|i| (i, self.assoc[i], false)); + let images = (0..self.images.len()) + .filter(move |i| !image_free.contains(i)) + .map(|i| (i, self.image_assoc[i], true)); + rects.chain(images) + } + pub fn data(&self) -> &PrimitiveData { &self.data } diff --git a/iris/core/src/ui/render_state.rs b/iris/core/src/ui/render_state.rs index f2bba0a..67c0e70 100644 --- a/iris/core/src/ui/render_state.rs +++ b/iris/core/src/ui/render_state.rs @@ -1,7 +1,7 @@ use crate::{ ActiveData, IdLike, MaskIdx, MoveIdx, Painter, PixelRegion, PrimitiveLayers, RegionAlign, StrongWidget, UiRegion, UiRsc, UiVec2, WidgetId, Widgets, - render::MoveOffset, + render::{IMAGE_BINDING, MoveOffset}, util::{HashMap, HashSet, Id, Vec2}, }; @@ -139,6 +139,12 @@ impl UiRenderState { } else if rsc.widgets().has_updates() { self.redraw_updates(rsc); } + #[cfg(debug_assertions)] + debug_assert!( + self.orphaned_primitives().is_empty(), + "{}", + self.orphan_report(rsc), + ); } fn redraw_all(&mut self, root: Option<&StrongWidget>, rsc: &mut dyn UiRsc) { @@ -192,8 +198,21 @@ impl UiRenderState { ) { let mut old_children = old_children.unwrap_or_default(); let mut old_move_slot = old_move_slot; + // Consumed here, not merely read: this call *is* the redraw the mark + // asked for, and leaving the mark set is what stranded a widget's + // primitives. `Painter::draw_twice` calls this twice for the same id + // in one frame (`List::place`'s measurement pass), and on the second + // call the still-set mark took the whole `if let` below -- including + // the `remove` that frees the first draw's primitives -- out of play, + // so `active.insert` at the end overwrote the only handles that could + // ever have freed them. The result is a full second copy of the row, + // drawn every frame from then on at the oversized measurement region + // and, with `List` setting no mask, outside the list's own bounds: + // the doubled `Compacted:` row in docs/bench/iris-phone-v2-2026-09-06.md. + // The same shape reaches any dirty widget an ancestor redraws first. + let dirty = rsc.widgets_mut().needs_redraw.remove(&id); if let Some(active) = self.active.get_mut(&id) - && !rsc.widgets().needs_redraw.contains(&id) + && !dirty { // check to see if we can skip drawing first if active.region == region { @@ -227,6 +246,14 @@ impl UiRenderState { let active = self.remove(id, false, rsc).unwrap(); old_children = active.children; old_move_slot = Some(active.move_slot); + } else if dirty && self.active.contains_key(&id) { + // Dirty and already drawn: none of the fast paths above may be + // taken (the widget's own content changed, so its old primitives + // say nothing about its new ones), but they are also the only + // thing that frees them. Same two lines, reached the other way. + let active = self.remove(id, false, rsc).unwrap(); + old_children = active.children; + old_move_slot = Some(active.move_slot); } // draw widget @@ -475,6 +502,66 @@ impl UiRenderState { self.active.len() } + /// Primitive instances still bound for the GPU whose owner is no + /// longer in `active`, or whose owner's `ActiveData` no longer names + /// them: a copy nothing can move, clip, resize or free, redrawn every + /// frame at whatever position it last had. `(layer, inst_idx, owner)` + /// each. + /// + /// Asserted empty at the end of every [`Self::update`], because this + /// is exactly the shape of the duplicated transcript row on Iris's + /// phone (`docs/bench/iris-phone-v2-2026-09-06.md`): counting + /// `active` alone cannot see it, since the orphan's owner is very + /// much alive -- it is the *earlier* set of primitives that got + /// stranded when the widget was drawn a second time without the first + /// draw being freed. O(primitives), debug builds only. + pub fn orphaned_primitives(&self) -> Vec<(usize, usize, WidgetId)> { + let mut orphans = Vec::new(); + for (layer, primitives) in self.layers.iter() { + for (inst_idx, owner, is_image) in primitives.live_instances() { + let owned = self.active.get(&owner).is_some_and(|a| { + a.primitives.iter().any(|h| { + h.layer == layer + && h.inst_idx == inst_idx + && (h.binding == IMAGE_BINDING) == is_image + }) + }); + if !owned { + orphans.push((layer, inst_idx, owner)); + } + } + } + orphans + } + + /// The message [`Self::update`]'s orphan assert prints -- built here + /// rather than inline so the (allocating, O(primitives)) work only + /// happens on the failing path. + #[cfg(debug_assertions)] + fn orphan_report(&self, rsc: &dyn UiRsc) -> String { + let orphans = self.orphaned_primitives(); + let mut lines: Vec = orphans + .iter() + .take(8) + .map(|(layer, idx, owner)| { + let alive = self.active.contains_key(owner); + format!( + " layer {layer} instance {idx}: owner '{}' ({owner:?}), owner still active: {alive}", + rsc.widgets().label(*owner), + ) + }) + .collect(); + if orphans.len() > lines.len() { + lines.push(format!(" ... and {} more", orphans.len() - lines.len())); + } + format!( + "{} primitive(s) are drawn but owned by nobody -- a stale copy \ + nothing will ever move or free:\n{}", + orphans.len(), + lines.join("\n"), + ) + } + /// Give `id` exclusive pointer input from the next `run_sensors` call /// on -- see `captured`'s field doc. Overwrites any previous capture /// (a gesture that starts a new one has already decided the old one diff --git a/iris/src/widget/list.rs b/iris/src/widget/list.rs index 1b21872..59af0b8 100644 --- a/iris/src/widget/list.rs +++ b/iris/src/widget/list.rs @@ -1467,6 +1467,69 @@ mod tests { ); } + /// The doubled `Compacted:` row from Iris's phone (docs/bench/ + /// iris-phone-v2-2026-09-06.md), reproduced at its mechanism. + /// + /// `replacing_the_last_row_many_times_does_not_leak_primitives` above + /// counts *widgets*, which is why it passed all along: the orphan's + /// owner is very much alive -- it is an earlier set of that same + /// widget's primitives that got stranded. What strands them is a row + /// marked dirty and then reached by its **ancestor's** redraw rather + /// than by its own: `draw_inner` only *read* the dirty mark, so the + /// whole branch that frees a redrawn widget's previous primitives was + /// skipped, and the fresh `ActiveData` overwrote the only handles that + /// could ever have freed them. `List` sets no mask, so that copy then + /// draws every frame at whatever region it last had -- including, + /// where the row was being measured at `GENEROUS_PADDING`, well below + /// the list's own box and under the composer. + /// + /// Two rows, two shapes of the same fault: row 2 has a cached height + /// (one `widget_within`), row 4 is replaced so it has none (`place`'s + /// `draw_twice`, which reaches `draw_inner` twice for one id in one + /// frame and so orphans a copy even with no ancestor involved). + #[test] + fn an_ancestor_redrawing_a_dirty_row_leaves_no_stale_copy() { + let mut rsc = TestRsc { + ui: UiData::default(), + }; + let mut list = List::new(Axis::Y); + // Rows that own a primitive *at their own id* (a background rect), + // not only through a child: an orphan is a widget's own primitive + // outliving its own redraw, so a row whose top-level widget paints + // nothing itself cannot show one however broken the path is. + let mut rows = Vec::new(); + for key in 0..5u64 { + let (bg_id, row) = background_styled_row(&mut rsc, 20.0); + rows.push((row.id(), bg_id)); + list.push_back(ListRow::new(key, row)); + } + let (list_weak, root) = add_list(&mut rsc, list); + + let mut render = UiRenderState::new(); + render.resize((100.0, 100.0)); + render.update(&root, &mut rsc); + assert!(render.orphaned_primitives().is_empty()); + + // A streamed row's content changing: the row is marked dirty (any + // `.set()` on it does this)... + let (row2, row2_bg) = rows[2]; + rsc.ui.widgets.get_dyn_mut(row2).unwrap(); + rsc.ui.widgets.get_dyn_mut(row2_bg).unwrap(); + + // Redraw the *list* by name, so the dirty row is reached by its + // ancestor's draw rather than by `redraw_updates` happening to + // pick it first -- which is the order `HashSet` iteration makes + // arbitrary, and the reason this went unnoticed. + render.redraw(list_weak.id(), &mut rsc); + + let orphans = render.orphaned_primitives(); + assert!( + orphans.is_empty(), + "{} primitive(s) survived their own widget's redraw: {orphans:?}", + orphans.len(), + ); + } + /// Enough rows, tall enough, that a fling toward the start has real /// room to travel before `at_start` clamps it -- shared by the fling /// tests below.