From ae0af8f5e314ae1c44be5b9d690e5e621189a8d6 Mon Sep 17 00:00:00 2001 From: iris <2+iris@noreply.localhost> Date: Wed, 9 Sep 2026 12:08:20 -0400 Subject: [PATCH] iris: ScrollArea measures its content instead of drawing it twice A container that probes a child's size was still doing it with a real draw, so `Painter::measure` existed and almost nothing used it. Tracing every draw of one streamed frame: 1,083 `Widget::draw` calls over 113 distinct widgets, the worst drawn 11 times at nesting depth 7-8, every one of them mode `Draw` and none of them dirty. The cache was working -- each of the 11 was offered a genuinely different region, alternating between an oversized probe box and a real one. ScrollArea::draw was one of the two sources. It drew its whole content at a box built from a stale hint, read the length back, corrected the scroll position, and drew the content again where it belonged. The first of those is now a measurement. The other half is the measure fast path. A widget's reported size is a function of its own state and the size it was offered, not of where it was offered -- so an undirtied widget already drawn at a region of this size has already answered, and `active.size` is the answer. This is the same assumption `mov` makes one branch further down (same offered size, therefore identical output, therefore a translation); it is only stated as a size here rather than acted on as a move. Without it a measurement costs a full recursive walk, which is what made the nesting compound. A measurement now also peeks at the redraw mark instead of consuming it -- it is not the redraw the mark asked for, and swallowing it would leave the widget stale until something marked it again. 453 draws from 1,083, and the streamed frame is p50 1.18ms (from 1.22ms, and 2.20ms before this run of work). The headless phone render is byte-identical to the previous commit's on the real GPU. What is deliberately NOT here: the same change to `Span::draw`'s phase 1, which is the remaining 2x and which moves the layout by a few pixels. The layout stays intact -- it is a position difference, not a broken frame -- but which of the two is correct was not established, and the suspicion (that phase 2 now takes `mov`, and `mov` accumulates deltas where a redraw recomputes) points at a bug in `mov` rather than in `Span`. Written up in docs/IRIS_TODO.md with Iris's target shape for `Span`: no probe phase at all for `abs` children, and a `rest` child forcing a reposition pass rather than a redraw. --- docs/IRIS_TODO.md | 43 +++++++++++++++++++++++++ iris/core/src/ui/render_state.rs | 39 ++++++++++++++++++---- iris/src/widget/position/scroll_area.rs | 6 +++- iris/src/widget/position/span.rs | 31 ++++++++++-------- 4 files changed, 98 insertions(+), 21 deletions(-) diff --git a/docs/IRIS_TODO.md b/docs/IRIS_TODO.md index f087aa8..24b15b4 100644 --- a/docs/IRIS_TODO.md +++ b/docs/IRIS_TODO.md @@ -12,6 +12,49 @@ and six phone-report sections went on 2026-09-08 for that reason. ## Fix +- [ ] **`Span` draws every child twice, and making the first one a + measurement moves the layout.** Found 2026-09-09; the safe half landed + and this is the part that needs a decision. + + `Span::draw`'s phase 1 draws each child at `UiRegion::FULL` purely to + learn its length along the axis, then phase 2 draws it at its real + share. The provisional slot is the whole span, so it is wrong by + construction for every child, and the doubling compounds through nested + spans: one streamed frame of the bench fixture made **1,083 + `Widget::draw` calls over 113 distinct widgets** before `ScrollArea`'s + probe became a `Painter::measure`, and **453** after, with 102 of the + 113 still at 4 draws (2 real, 2 measurements). + + Changing phase 1 to `painter.measure(child, UiRegion::FULL)` takes it + to 2, and **it renders differently**: 28,771 pixels, and the headless + phone shot shows the transcript shifted a few pixels vertically. The + layout is intact -- panels, code fences and text all draw correctly -- + so this is a position difference, not a broken frame, but which of the + two is *right* was not established and it must be before this lands. + + The mechanism to check first. With phase 1 drawing, the child's + `active.region` is the full span when phase 2 asks, so phase 2 always + finds a different size and does a real redraw. With phase 1 measuring, + `active.region` is still *last frame's* share, which usually matches, + so phase 2 takes `draw_inner`'s `mov` branch -- one `move_offsets` + write instead of a redraw, which is the intended win. But `mov` + accumulates (`entry.delta += delta`) where a redraw recomputes from + scratch, so the suspicion is float drift that phase 1's unconditional + redraw was hiding. If that is it, the fix is in `mov`, not in `Span`. + + Iris's framing, which is the target shape (2026-09-09): *"if you draw + one child in a list of fixed sized children, then you know immediately + where the second one must be and shouldn't need to probe its size + again. The only time redraws should actually be needed are if you're + using a `rest` length, where you don't know how long it's gonna be + until you draw everything else first, and so you have to move things. + Even then it should just be moving, not redrawing."* So the endpoint is + no probe phase at all for `abs` children -- draw each in turn at + `[cursor, end]`, read its length, advance the cursor -- with a `rest` + child forcing a reposition pass over what follows it rather than a + redraw. `Aligned` already has exactly this shape (one draw, then + `Painter::reposition`) and is the example to copy. + - [ ] **A row moving because the list grew should be one `move_offsets` write, and today it is a redraw.** Found 2026-09-09 by `scripts/rigs/ui-profile`'s `arena_churn` and left for whoever picks diff --git a/iris/core/src/ui/render_state.rs b/iris/core/src/ui/render_state.rs index 740008c..bd9208a 100644 --- a/iris/core/src/ui/render_state.rs +++ b/iris/core/src/ui/render_state.rs @@ -509,14 +509,39 @@ impl UiRenderState { // and, with `LazySpan` 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. - // A measurement consumes no redraw mark and takes none of the - // fast paths: it is not the redraw the mark asked for, and - // "already drawn at this region" would make it return without - // reporting a size at all. - let dirty = !mode.measuring() && rsc.widgets_mut().needs_redraw.remove(&id); - if let Some(active) = self.active.get_mut(&id) + // A measurement **peeks** at the mark rather than consuming it: it + // is not the redraw the mark asked for, and swallowing it would + // leave the widget stale until something else marked it again. + let dirty = if mode.measuring() { + rsc.widgets().needs_redraw.contains(&id) + } else { + rsc.widgets_mut().needs_redraw.remove(&id) + }; + + // What a measurement can answer without drawing at all. + // + // A widget's reported size is a function of its own state and the + // size it was offered -- not of where it was offered. So an + // undirtied widget already drawn at a region of this size has + // *already answered this question*, and `active.size` is that + // answer. This is the same assumption `mov` below already makes + // (same offered size, therefore identical output, therefore a + // translation rather than a redraw); it is only stated here as a + // size rather than acted on as a move. + // + // Without it a measurement costs a full recursive walk of the + // subtree, and since the containers that measure nest, that walk + // is what made one streamed frame 1,083 `Widget::draw` calls over + // 113 distinct widgets. + if mode.measuring() { + if let Some(active) = self.active.get(&id) + && !dirty + && active.region.size() == region.size() + { + return active.size; + } + } else if let Some(active) = self.active.get_mut(&id) && !dirty - && !mode.measuring() { // check to see if we can skip drawing first if active.region == region { diff --git a/iris/src/widget/position/scroll_area.rs b/iris/src/widget/position/scroll_area.rs index 42fbf2e..ff2bdbd 100644 --- a/iris/src/widget/position/scroll_area.rs +++ b/iris/src/widget/position/scroll_area.rs @@ -107,7 +107,11 @@ impl Widget for ScrollArea { // has been measured: a zero-length region on the first frame would // place the child's primitives against a box of no size. let hint = self.content_len.unwrap_or(container_len); - let used = painter.widget_within(&self.inner, self.child_region(hint)); + // A **measurement**: this asks how long the content is, and the + // box it asks about is built from a hint that the answer below is + // about to correct. Drawing it here painted the whole content at a + // provisional offset and then painted it again at the real one. + let used = painter.measure(&self.inner, self.child_region(hint)); // A child reporting `rel` means "this fraction of what I was // offered", and what it was offered is this scroll area -- so the diff --git a/iris/src/widget/position/span.rs b/iris/src/widget/position/span.rs index 9ecca40..a896b7a 100644 --- a/iris/src/widget/position/span.rs +++ b/iris/src/widget/position/span.rs @@ -17,14 +17,17 @@ impl Widget for Span { let axis = self.dir.axis; let gap = self.gap.apply_rest(painter.density()).abs; - // Phase 1: draw each child once, at the ambient (unmodified, full) - // region a size-only query used to see before this migration, to - // learn its length along the layout axis. This paints real - // primitives at a provisional slot; phase 2 below places each - // child for real via the normal `widget_within` dispatch, which - // only actually redraws it when that slot's *size* differs from - // this provisional one (most children: a resize, since the - // provisional slot is the whole span, not this child's share). + // Phase 1: ask each child how long it is along the layout axis, + // at the ambient (unmodified, full) region. + // + // A **measurement**, so it writes nothing: the provisional slot + // this asks about is the whole span rather than the child's own + // share, so it is almost never where the child ends up, and + // painting it there meant every child of every `Span` was drawn + // twice -- once at a slot that was wrong by construction and once + // where it belongs. With containers nested that doubling + // compounds, which is where a streamed frame's 1,083 draws over + // 113 widgets came from. let lens: Vec = self .children .iter() @@ -34,11 +37,13 @@ impl Widget for Span { let gap_total = gap * self.children.len().saturating_sub(1) as f32; let total = lens.iter().fold(Len::abs(gap_total), |s, &l| s + l); - // Phase 2: place each child for real, using the lengths just - // learned -- the same arithmetic this loop always used. The cross- - // axis length of *this* draw (used for `Span`'s own reported size - // below) falls out of each child's real, resolved-width `used` - // here for free -- this is what replaces `desired_ortho`'s former + // Phase 2: draw each child for real, using the lengths just + // learned -- the same arithmetic this loop always used. This is + // the only draw a child gets; where its size is unchanged and only + // its position moved, `draw_inner` turns it into one `move_offsets` + // write rather than a redraw. The cross-axis length of *this* draw + // (used for `Span`'s own reported size below) falls out of each + // child's real, resolved-width `used` here for free -- this is what replaces `desired_ortho`'s former // duplicate simulation of this same loop (see LAYOUT.md section 4). let mut start = UiScalar::rel_min(); let mut ortho_len = Len::ZERO;