diff --git a/docs/IRIS_TODO.md b/docs/IRIS_TODO.md index 366e429..d3fc0f9 100644 --- a/docs/IRIS_TODO.md +++ b/docs/IRIS_TODO.md @@ -591,17 +591,23 @@ agent ticks it here with the evidence. that. Fixed in `iris/src/widget/rect.rs`; the reason is written at the definition. Suspect the same cause for anything else tinted with a background rect. -- [ ] **A wrapped transcript row trips `reposition`'s debug assert.** - *"widget ... is both moved by its parent's own layout (`mov`) and - repositioned within it"*, raised from `List::place`. Repro: change - `.wrap(!verbatim)` to `.wrap(true)` in `transcript-ui/src/row.rs`'s - `build_block` and run `iris/run-headless.sh transcript --shot - /tmp/x.png -- -p transcript-ui`. Survives the `Rect` fix above and is - not specific to any block kind -- it appears once the row is tall - enough. The shipping configuration does not reach it (verbatim blocks - do not wrap) and the Android bench runs clean with assertions live, - but it is a real disagreement about who owns a widget's move slot and - should be settled before more of P1 leans on `List`. +- [x] **A wrapped transcript row tripped `reposition`'s debug assert.** + Settled 2026-09-06 by giving the move slot one owner instead of two. + `mov` accumulates a delta on it, `reposition` overwrote it, and both + legitimately land on one widget in one frame: `List::place`'s + Bottom-known branch offers a row a same-size box that has *moved* + (`mov`), then corrects the placement inside it when the row's cached + height no longer matches what the row reports (`reposition`). The + slot now always means `move_applied + repositioned` + (`ActiveData::repositioned`, `iris/core/src/ui/render_state.rs`), so + `reposition` adds the move rather than dropping it -- the assert is + gone and the arithmetic is right. Test: + `a_widget_moved_by_its_parent_and_then_placed_inside_it_lands_at_the_placement` + in `layout_tests.rs`, which lands the child at the *offered* position + (-100px) instead of the placement (100px) without the fix, and a + `debug_assert_eq!` in `reposition` that nothing but those two ever + writes the slot. Verified with the `.wrap(true)` repro (draws, no + panic) and an emulator bench run with assertions live. - [ ] **Desktop colours are washed out: the winit surface is sRGB and the shader writes the palette's bytes as linear.** Mocha Crust (17,17,27) is drawn as (73,73,91), measured off @@ -609,17 +615,29 @@ agent ticks it here with the evidence. format rather than the palette -- but it makes the desktop build useless as a colour reference, which is exactly what P1a needed it for when the emulator could not draw glyphs. -- [ ] **The emulator cannot draw iris's glyphs.** Under `-gpu host` with - Vulkan disabled (Mesa 26.2.2 / virgl -- what `emu` does on this - machine) every character renders as a solid filled box: the atlas - sample's alpha reads 1, which is what an incomplete GL texture returns - (0,0,0,1). Not new (`20303e0` does it too) and not the platform's - (Compose draws text perfectly on the same AVD in the same minute). - Enabling host Vulkan still dies at boot in gfxstream, and - `EMU_GPU=software` gives SwiftShader Vulkan on which iris **SIGSEGVs - in `surface_changed`**. Either of the last two would restore - appearance testing on Android; today it has to be done on the desktop - backend or on Iris's phone. +- [x] **Every glyph was a solid box on the GLES backend -- iris's bug, + not the emulator's.** Fixed 2026-09-06. The atlas is one + `texture_2d_array` and `GpuTextures::new` created it with **one + layer**; wgpu-hal picks the GL target from the descriptor + (`(false, 1) => TEXTURE_2D`), so under GLES that array was a + `GL_TEXTURE_2D` bound to the shader's `sampler2DArray`, the unit was + incomplete, every `textureSample` returned (0,0,0,1), and + `draw_glyph`'s `color.a *= texel.a` filled the quad. `MIN_ARRAY_LAYERS + = 2` in `iris/core/src/render/texture.rs`, with a `debug_assert!` at + `create_array_texture`. Vulkan (the phone, the desktop's default + backend) was never affected. Reproduce the class in seconds without an + emulator: `iris`'s `force-gles` feature now switches the **desktop** + backend too -- `./run-headless.sh transcript --shot /tmp/x.png -- -p + transcript-ui --features iris/force-gles`. + +- [ ] **The bench report pane draws over the transcript rows instead of + replacing them.** Visible on the emulator for the first time now that + glyphs render there (`/tmp/emu-final.png`, 2026-09-06): after a bench + run the report's lines and the transcript's occupy the same rows in the + top third of the screen, both legible, neither on top. Pre-existing -- + the same overlap is in a screenshot taken before the move-slot fix -- so + it is its own item, most likely the report pane not masking or not + claiming its region. ## Build (for the port) diff --git a/docs/RUST.md b/docs/RUST.md index be2062c..8e215eb 100644 --- a/docs/RUST.md +++ b/docs/RUST.md @@ -559,9 +559,12 @@ column above. reconnect/backoff and cross-thread stream cancellation, which are runtime policy for whichever framework embeds this crate, not pure logic -- see `CLIENT_CORE.md`'s new section for the full account. -- **Then**: redeliver `~/host/bench/iris-bench-arm64.apk` for Iris with - its README saying what changed, and record any choice she should see in - `DECISIONS.md`. +- **Then**: redeliver the APK for Iris. **Delivery is a push to the + `~/repos/ai-app-bench` repo** (`iris/build/outputs/apk/release/ + iris-bench-arm64.apk` plus a dated README section), which Dev Updater + on the host pulls -- **not** `~/host/bench/`, which nothing reads; two + builds on 2026-09-06 went there and never reached her phone. Record any + choice she should see in `DECISIONS.md`. ## Where things stand (2026-09-05) @@ -5600,6 +5603,90 @@ device. transcript-ui -p desktop-app -p tabs-ui --all-targets` warning-free; `cargo test` 85 (iris, +4) + 13 (iris-core) + 31 (transcript-ui, +11) + 123 (client-core). + **2026-09-06, after P1a: "the emulator cannot draw iris's glyphs" + was iris's bug, not the emulator's.** The finding recorded in the + box above -- that every glyph is a solid filled box under `-gpu + host` GLES and that this is what an incomplete GL texture returns + -- had the mechanism right and the attribution wrong. It is a real + defect on **any** adapter that is GL rather than Vulkan. + - **Reproduced off the emulator entirely**, which is what made it + cheap: `default/render.rs` now honours the same `force-gles` + feature `android/render.rs` did, so + `./run-headless.sh transcript --shot /tmp/x.png -- -p + transcript-ui --features iris/force-gles` draws the boxes on this + machine's own GPU in seconds. Two shader probes then said what + the sample was: `return vec4(texel.rgb, 1.0)` drew black boxes and + `return vec4(texel.a, texel.a, texel.a, 1.0)` drew white ones, so + the atlas sample was exactly (0, 0, 0, 1) -- GL's answer for an + **incomplete texture unit**, and not the null texture (which is + zeroed, alpha 0). + - **Root cause: the glyph atlas array was created with one layer.** + `GpuTextures::new` started `array_capacity` at 1 and `grow_array` + only doubles once a page needs a layer past it, so the ordinary + case -- one atlas page -- is a one-layer array. wgpu-hal picks the + GL texture target from the descriptor alone + (`gles::Texture::get_info_from_desc`: `(false, 1) => TEXTURE_2D`), + so that array is created as a `GL_TEXTURE_2D` and then bound to + the shader's `sampler2DArray`. wgpu has a name for this + (`log_failing_target_heuristics`, its issues #1614/#1574); the + result is an incomplete unit, `texel.a == 1`, and `draw_glyph`'s + `color.a *= texel.a` paints the whole glyph quad. + - **Fix**: `MIN_ARRAY_LAYERS = 2` in + `iris/core/src/render/texture.rs` -- the array is never created + with fewer, with the account at `create_array_texture` and a + `debug_assert!` there so a future capacity arithmetic change fails + at the mistake rather than as boxes on a screen. Cost: one page of + texture memory, which the next atlas page uses anyway. + - **Not a regression from `3e72a4e..20303e0`, and the bisect was not + run.** The defect is a function of the layer count, not of any + commit in that range: it has been there since the atlas became a + `texture_2d_array` (TEXTURES.md, 2026-09-04) and it reproduces at + HEAD and disappears at HEAD with the one-line capacity change. The + claimed "visible text at `3e72a4e`" is a misreading of its own + evidence -- `/tmp/final-typing.png`, the screenshot that entry + cites, is boxes; what the agent read was the `iris text render: + chars=5 glyphs=5` log line, which reports what **parley shaped**, + not what reached the screen. The earlier genuinely-good emulator + shots (`/tmp/after3.png`, 2026-09-05 19:56) predate the APK being + built with `force-gles` at all, so they were the Vulkan path. + - **The phone build is not affected and does not need withdrawing.** + `android-app/build-apk.sh`'s default features are deliberately + without `force-gles` (`d73db97`'s comment), so a phone build takes + `Backends::PRIMARY` -> Vulkan, where a one-layer array is an + ordinary one-layer array and glyphs draw correctly -- which is + also what Iris's phone reports have shown all along. The fix + matters for any device that falls back to GLES, which is why it is + not just an emulator convenience. + - **Appearance testing on Android is back.** `build-apk.sh debug + --abi x86_64 --features "transcript-screen bench force-gles"` on + this checkout's AVD draws the transcript legibly -- + `docs/bench/p1a-2026-09-06/iris-emulator-gles-fixed.png` -- so + P1b onwards can be checked here rather than only on the desktop + backend or Iris's phone. + **2026-09-06: the move slot has one owner.** IRIS_TODO's open + "a wrapped transcript row trips `reposition`'s debug assert" is + fixed rather than suppressed. `mov` accumulates a delta on a + widget's move slot and `reposition` overwrote it, and both + legitimately land on one widget in one frame: `List::place`'s + Bottom-known branch offers a row a same-size box that has *moved* + (`mov`), then corrects the placement inside it when the row's + cached height no longer matches what the row reports + (`reposition`) -- measured with a probe on the `.wrap(true)` repro: + `h=1604.7 height=548.7`, the row's own draw having updated + `active.size` without the list's height cache. The slot now means + `move_applied + repositioned` (both on `ActiveData`), so + `reposition` adds the move instead of dropping it and stays + idempotent, and the old assert is replaced by a `debug_assert_eq!` + that nothing *but* those two ever wrote the slot. Test: + `a_widget_moved_by_its_parent_and_then_placed_inside_it_lands_at_the_placement` + (`layout_tests.rs`), which draws the child at the offered position + rather than the placement without the fix. Checks: `cargo fmt --all + --check` clean, `cargo clippy --workspace --all-targets` + warning-free, `cargo test --workspace` 86 (iris, +1) + 13 + (iris-core) + 31 (transcript-ui), the `.wrap(true)` repro drawing + correctly, and an emulator bench run with assertions live and no + abort (`2438 frames over 147.7s, p50 27.2ms`). + - [ ] **P1b — tool-call cards and grouping.** `ToolRows.kt`/ `ToolInput.kt`'s cards: a collapsed row per call with name and a one-line summary, expand to input and output, runs of diff --git a/docs/bench/p1a-2026-09-06/iris-emulator-gles-fixed.png b/docs/bench/p1a-2026-09-06/iris-emulator-gles-fixed.png new file mode 100644 index 0000000..7e50be6 Binary files /dev/null and b/docs/bench/p1a-2026-09-06/iris-emulator-gles-fixed.png differ diff --git a/iris/Cargo.toml b/iris/Cargo.toml index 061cc10..40e9c6f 100644 --- a/iris/Cargo.toml +++ b/iris/Cargo.toml @@ -66,7 +66,11 @@ send_wrapper = "0.6.0" # default) or virgl's GLES path, without a second env-var plumbing path that # nothing on this machine can hand to an already-launched Android process # (there is no `am start` environment and no system-property reader here to -# add one). Android-only; `android/render.rs` is the only reader. +# add one). Read by `android/render.rs` and, so the GLES path can be +# reproduced on a machine with a real GPU rather than only in the emulator, +# by `default/render.rs`: +# ./run-headless.sh transcript --shot /tmp/x.png -- -p transcript-ui \ +# --features iris/force-gles force-gles = [] [dev-dependencies] diff --git a/iris/core/src/render/texture.rs b/iris/core/src/render/texture.rs index 03fe4dd..9fac2e4 100644 --- a/iris/core/src/render/texture.rs +++ b/iris/core/src/render/texture.rs @@ -5,6 +5,10 @@ use crate::{PatchRect, TextureKind, TextureUpdate, Textures}; use super::atlas::PAGE; +/// The fewest layers the glyph atlas array is ever created with. Two, not +/// one, for the GLES reason written on `create_array_texture`. +const MIN_ARRAY_LAYERS: u32 = 2; + /// What one texture slot is, GPU-side. Parallel to `Textures`' own slot /// numbering (`TextureKind`'s `Image`/`Page`), so a slot's index means the /// same thing on both sides without a second map to keep in sync. @@ -360,7 +364,26 @@ impl GpuTextures { }) } + /// The atlas is sampled as a `texture_2d_array`, and **a one-layer + /// array is not one on the GLES backend**: wgpu-hal picks the GL + /// texture target from the descriptor alone + /// (`gles::Texture::get_info_from_desc`, `(false, 1) => TEXTURE_2D`), + /// so a capacity of 1 creates a `GL_TEXTURE_2D` and binds it to the + /// shader's `sampler2DArray`. GL then treats that unit as incomplete + /// and every `textureSample` returns (0, 0, 0, 1) -- which, through + /// `draw_glyph`'s `color.a *= texel.a`, draws every glyph as a solid + /// filled box. That was iris's appearance on the emulator's GLES for + /// two days (RUST.md, "the emulator cannot draw iris's glyphs"), and + /// it is a real defect on any device whose adapter is GL rather than + /// Vulkan, not an emulator artifact. So the array never has fewer than + /// `MIN_ARRAY_LAYERS` layers; the second layer costs one page of + /// texture memory and is used by the next atlas page anyway. fn create_array_texture(device: &Device, capacity: u32) -> Texture { + debug_assert!( + capacity >= MIN_ARRAY_LAYERS, + "glyph atlas array asked for {capacity} layers; fewer than {MIN_ARRAY_LAYERS} is a \ + GL_TEXTURE_2D on the GLES backend and draws every glyph as a box" + ); device.create_texture(&TextureDescriptor { label: Some("glyph atlas array"), size: Extent3d { @@ -382,7 +405,7 @@ impl GpuTextures { pub fn new(device: &Device, queue: &Queue) -> Self { let sampler = default_sampler(device); let null_view = null_texture_view(device); - let array_capacity = 1; + let array_capacity = MIN_ARRAY_LAYERS; let array_texture = Self::create_array_texture(device, array_capacity); let array_view = array_texture.create_view(&TextureViewDescriptor { dimension: Some(TextureViewDimension::D2Array), diff --git a/iris/core/src/ui/active.rs b/iris/core/src/ui/active.rs index 3b702cc..c6e0675 100644 --- a/iris/core/src/ui/active.rs +++ b/iris/core/src/ui/active.rs @@ -53,4 +53,17 @@ pub struct ActiveData { /// after a finger pan (2026-09-06). Reset to zero whenever the widget /// is really redrawn, since `draw_inner` zeroes the slot then too. pub move_applied: Vec2, + /// The offset the last `Painter::reposition` placed this widget's + /// content at *within* `region`, in window pixels. The move slot has + /// exactly one owner and one meaning: + /// `move_offsets[move_slot] == move_applied + repositioned`. `mov` + /// adds to the first, `reposition` overwrites the second (it + /// recomputes `from` afresh every call, so repeating it must land on + /// the same answer rather than drifting), and both then rewrite the + /// slot from the sum -- which is what lets a parent both move a child + /// with its own layout and place it inside that moved region in one + /// frame. `List::place`'s Bottom-known branch does exactly that once a + /// row's blocks wrap. Reset to zero on a real redraw, with + /// `move_applied` and the slot itself. + pub repositioned: Vec2, } diff --git a/iris/core/src/ui/render_state.rs b/iris/core/src/ui/render_state.rs index d237df2..089eb06 100644 --- a/iris/core/src/ui/render_state.rs +++ b/iris/core/src/ui/render_state.rs @@ -380,6 +380,7 @@ impl UiRenderState { move_slot, own_mask, move_applied: Vec2::ZERO, + repositioned: Vec2::ZERO, }; // remove old children that weren't kept @@ -453,12 +454,8 @@ impl UiRenderState { let Some(active) = self.active.get(&id) else { return; }; - debug_assert!( - active.move_applied == Vec2::ZERO, - "widget {id:?} is both moved by its parent's own layout (`mov`) and repositioned \ - within it; the two write the same slot with different conventions -- see \ - `ActiveData::move_applied`" - ); + let move_applied = active.move_applied; + let repositioned = active.repositioned; let from = active .size .to_uivec2(self.density) @@ -468,8 +465,27 @@ impl UiRenderState { let from_px = from.top_left().to_abs(self.output_size); let to_px = to.top_left().to_abs(self.output_size); let delta = to_px - from_px; + // Not `delta` alone: a parent may have `mov`ed this widget to a + // region that itself moved earlier in the same frame, and that + // part of the slot is `move_applied`'s, not this call's. Writing + // `delta` on its own dropped it and put the content back at the + // pre-move position. `from` is computed against `active.region`, + // which `mov` already updated, so `delta` is purely the placement + // inside the region and the two summands never overlap. let entry = rsc.ui_mut().move_offsets.get_mut(slot); - entry.delta = [delta.x, delta.y]; + debug_assert_eq!( + entry.delta, + [ + move_applied.x + repositioned.x, + move_applied.y + repositioned.y + ], + "widget {id:?}'s move slot was written by something other than `mov`/`reposition`; \ + the slot is theirs and means `move_applied + repositioned` -- see `ActiveData`" + ); + entry.delta = [move_applied.x + delta.x, move_applied.y + delta.y]; + if let Some(active) = self.active.get_mut(&id) { + active.repositioned = delta; + } self.mov_count += 1; } diff --git a/iris/src/default/render.rs b/iris/src/default/render.rs index 2306da1..d0cc245 100644 --- a/iris/src/default/render.rs +++ b/iris/src/default/render.rs @@ -85,7 +85,16 @@ impl UiRenderer { let size = window.inner_size(); let instance = Instance::new(&InstanceDescriptor { - backends: Backends::PRIMARY, + // `force-gles` on the desktop too, not just on Android: the + // GLES backend has behaviour of its own (a one-layer array + // texture is a `GL_TEXTURE_2D` -- see + // `GpuTextures::create_array_texture`), and a machine with a + // real GPU is where that is cheap to reproduce and screenshot. + backends: if cfg!(feature = "force-gles") { + Backends::GL + } else { + Backends::PRIMARY + }, ..Default::default() }); diff --git a/iris/src/layout_tests.rs b/iris/src/layout_tests.rs index b56ce70..be2ef9c 100644 --- a/iris/src/layout_tests.rs +++ b/iris/src/layout_tests.rs @@ -562,3 +562,96 @@ fn a_size_independent_widget_moved_by_its_parent_has_the_hit_box_it_is_drawn_at( after.top_left ); } + +/// A parent that both `mov`s a child (its own layout moved the box it +/// offers) and `reposition`s it inside that box in the same frame -- what +/// `List::place`'s Bottom-known branch does once a row's cached height +/// stops matching what the row reports, which is reachable as soon as a +/// transcript row's blocks wrap (docs/IRIS_TODO.md's "Found by P1a"). +struct MoveThenPlace { + inner: StrongWidget, + /// Where the child is *offered* a (constant-size) box, moved between + /// frames by the test. + offer_top: f32, + /// Where the child is then placed within this widget's own region. + place_top: f32, +} + +impl Widget for MoveThenPlace { + fn draw(&mut self, painter: &mut Painter) -> Size { + let offer = UiRegion::new( + UiSpan::FULL, + UiSpan::new( + UiScalar::abs(self.offer_top), + UiScalar::abs(self.offer_top + 40.0), + ), + ); + painter.widget_within(&self.inner, offer); + let place = UiRegion::new( + UiSpan::FULL, + UiSpan::new( + UiScalar::abs(self.place_top), + UiScalar::abs(self.place_top + 40.0), + ), + ); + painter.reposition(&self.inner, place); + Size::default() + } +} + +/// `mov` accumulates a delta onto a widget's move slot and `reposition` +/// overwrites it, and both can legitimately land on one widget in one +/// frame (see `MoveThenPlace`). `reposition` used to write its own delta +/// alone, which dropped the move and put the child back at the position +/// the offered box had *before* it moved; a `debug_assert!` that +/// `move_applied` was zero hid that behind a panic instead of fixing it. +/// The slot has one owner and one meaning now -- +/// `move_applied + repositioned` -- so the child stays where it was +/// placed however its offered box moves. Fails at the offer's position +/// (200) rather than the placement's (100) without that. +#[test] +fn a_widget_moved_by_its_parent_and_then_placed_inside_it_lands_at_the_placement() { + let mut rsc = TestRsc { + ui: UiData::default(), + }; + let rect = rsc.ui.widgets.add_strong(Rect::new(UiColor::WHITE)); + let child = rsc.ui.widgets.add_strong(Sized { + inner: rect.any(), + x: None, + y: Some(Len::abs(40.0)), + }); + let child_w = child.weak(); + let parent = rsc.ui.widgets.add_strong(MoveThenPlace { + inner: child.any(), + offer_top: 0.0, + place_top: 100.0, + }); + let parent_w = parent.weak(); + let root = parent.any(); + + let mut render = UiRenderState::new(); + render.resize((200.0, 400.0)); + render.update(&root, &mut rsc); + let before = render.window_region(&child_w, &rsc).unwrap(); + assert!( + (before.top_left.y - 100.0).abs() < 0.01, + "the child should be drawn where it was placed, not where it was offered: {before:?}" + ); + + // Move the offered box without changing its size (the `mov` fast path) + // and place the child at the same spot as before. Marking the parent + // dirty is what a real container's own content change does; the child + // itself is untouched, which is the case `mov` exists for. + { + let parent = rsc.ui.widgets.get_mut(&parent_w).unwrap(); + parent.offer_top = 200.0; + } + rsc.ui.widgets.needs_redraw.insert(parent_w.id()); + render.update(&root, &mut rsc); + let after = render.window_region(&child_w, &rsc).unwrap(); + assert!( + (after.top_left.y - 100.0).abs() < 0.01, + "the placement did not change, so neither should the child: before={before:?} \ + after={after:?}" + ); +}