Prune the docs of work already done: 18,252 -> 7,567 lines
Iris: "the documentation is also pretty crazy too. Can you go through it
and remove everything that's already done and decided? There's entire md
files iirc for projects already complete. And many with checkboxes already
ticked off that just fill up context."
docs/RUST.md 8503 -> 905 the framework bake-off (options,
recommendation, twelve closed
experiment boxes) and two superseded
"where things stand" sections, out;
what the experiments settled kept as
one line each
docs/IRIS_TODO.md 1383 -> 229 fifty closed items and six
phone-report sections whose defects
are all fixed
docs/LAYOUT.md 1116 -> 829 the pre-implementation framing: the
old trait, the checklist, the
migration list, the pass conditions
docs/TEXTURES.md 496 -> 240 the prior-art survey, the proposal
and its review, all implemented
docs/REVIEW-*.md 673 -> 0 two completed review passes; the two
findings left open on purpose (mask
hit-testing, the phone's font set)
moved into RUST.md
What survives a prune is what cannot be cheaply re-derived: measurements
(the APK-size table, the phone bench reports), dead ends, invariants and
their reasons, and the design of what exists now rather than the route to
it. AGENTS.md now says that, so the next session prunes as it goes rather
than appending; docs/IRIS_TODO.md's header says items are deleted when
they land rather than ticked.
Deleting the two review files left eighteen citations dangling in code
comments that state their reason inline and cited the file for provenance
only — those now read "(review, 2026-09-06)" and carry no dead pointer.
The emulator's measured GPU capabilities moved to the this-machine-android
skill, where machine facts belong. IRIS.md and DECISIONS.md are dated
records and were not rewritten; each gained one note that paths in older
entries predate the 2026-09-08 crate merge, pointing at the mapping.
Not touched, deliberately: docs/DECISIONS.md's entries (that file *is* the
queue of things for Iris to review, so deleting decided items would remove
what it exists for) and iris/readme.md and iris/TODO, which are hers.
Verified: ./run-tests.sh and `cd iris && cargo test` green, clippy and fmt
clean in every workspace, and every remaining docs/*.md cross-reference
resolves.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
irisandClaude Opus 5 committed
1 parent
6d5a231f5c
commit
09778346a0
23 files changed
+354
-10297
No files matched your search
@@ -102,6 +102,16 @@ Module-by-module intent is in `docs/PLAN.md`'s "Backend layout".
|
||||
public API** -- Iris, 2026-09-08), working list, decisions log,
|
||||
layout/render design, texture-atlas design, and the design of
|
||||
`app-rust`'s `client` module, respectively.
|
||||
|
||||
**These documents are pruned as the work lands, not appended to
|
||||
forever** (Iris, 2026-09-08: *"remove everything that's already done and
|
||||
decided… many with checkboxes already ticked off that just fill up
|
||||
context"*). A ticked box, a finished experiment and a completed review
|
||||
are deleted once carried out; `IRIS_TODO.md` holds only open items, and
|
||||
a finished document is removed rather than archived in place. What
|
||||
survives is what cannot be cheaply re-derived — measurements, dead ends
|
||||
and failed hypotheses, invariants and their reasons, and the design of
|
||||
what exists now rather than the route to it.
|
||||
- `docs/SCROLL.md` — how anything in iris scrolls: one
|
||||
`ScrollController` holds the position, the gesture, the fling and the
|
||||
pin, and the two widgets that scroll (`ScrollArea`, `LazySpan`) own
|
||||
|
||||
@@ -196,8 +196,8 @@ fn battery_line(samples: &[i32]) -> String {
|
||||
// up -- pairing the `Option` unwraps with the emptiness check right
|
||||
// here (rather than two statements apart, with `mean` in between
|
||||
// reading the same slice) is what keeps a future reorder from
|
||||
// separating the guard from what it protects (docs/
|
||||
// REVIEW-2026-09-06.md finding 7).
|
||||
// separating the guard from what it protects (review, 2026-09-06
|
||||
// finding 7).
|
||||
let (Some(min), Some(max)) = (samples.iter().min(), samples.iter().max()) else {
|
||||
unreachable!("samples is non-empty, checked above");
|
||||
};
|
||||
|
||||
@@ -292,7 +292,7 @@ impl TranscriptScreen {
|
||||
// rebuild with *fewer* blocks under the same key would
|
||||
// otherwise leave the extra blocks in `Selection`
|
||||
// pointing at widgets the `drop` below frees (the shape
|
||||
// docs/REVIEW-2026-09-06.md's finding 1 called out).
|
||||
// a review on 2026-09-06 called out).
|
||||
self.selection.borrow_mut().unregister(old_key);
|
||||
// Uncapped: this is the row a delta just failed to land
|
||||
// in, and the reason may be that it *is* capped
|
||||
@@ -319,7 +319,7 @@ impl TranscriptScreen {
|
||||
// gets cleared the same way `LazySpan` does, right before the
|
||||
// rows it was pointing at go with it -- `push_row` below
|
||||
// re-`register`s whatever survives as it rebuilds each
|
||||
// row (docs/REVIEW-2026-09-06.md finding 1: a key that
|
||||
// row (review, 2026-09-06 finding 1: a key that
|
||||
// `group_tool_runs` regrouped away used to stay in
|
||||
// `Selection` pointing at a widget this `clear()` had
|
||||
// just freed, panicking the next long-press anywhere).
|
||||
@@ -623,7 +623,7 @@ mod diff_tests {
|
||||
}
|
||||
|
||||
/// Exercises `TranscriptScreen::apply`'s `Rebuild` arm through a real
|
||||
/// `Selection`, the gap docs/REVIEW-2026-09-06.md finding 8 named: the
|
||||
/// `Selection`, the gap the 2026-09-06 review named: the
|
||||
/// pure `diff_rows` decision above and `selection.rs`'s own registration
|
||||
/// tests each pass in isolation, and neither alone catches finding 1 (a
|
||||
/// regrouped-away row's key surviving in `Selection` after `LazySpan::clear()`
|
||||
|
||||
@@ -105,7 +105,7 @@ impl Selection {
|
||||
/// clears the list, and what `TranscriptScreen::apply`'s `Rebuild` arm
|
||||
/// calls right before it, since a full rebuild drops every row's old
|
||||
/// widget and `push_row` re-`register`s each surviving key's new one
|
||||
/// as it goes (review docs/REVIEW-2026-09-06.md finding 1: the
|
||||
/// as it goes (review, 2026-09-06 finding 1: the
|
||||
/// `Rebuild` arm used to call only `LazySpan::clear()`, leaving any key
|
||||
/// dropped by the regroup -- present in the old rows, absent from the
|
||||
/// new ones -- pointing at a widget the list had just freed, so the
|
||||
@@ -350,8 +350,7 @@ impl Selection {
|
||||
// Only when `fling` actually took it: below Compose's
|
||||
// `|v| <= 1.0` there is nothing to tick, and registering
|
||||
// an animation for a widget that is not animating asks the
|
||||
// next frame to find that out (docs/REVIEW-2026-09-07.md's
|
||||
// second nit).
|
||||
// next frame to find that out (review, 2026-09-07).
|
||||
if let Some(scroll) = self.scroll
|
||||
&& scroll(ui).fling(v)
|
||||
{
|
||||
|
||||
@@ -2,7 +2,7 @@
|
||||
//! themselves rather than a widget: `iris::diagnostics::set_trace` gates
|
||||
//! `iris::input`/`iris::frame` (Iris's 2026-09-07 request, "add another
|
||||
//! button to copy input event info ... instrument a lot of the code with
|
||||
//! timings"), and `docs/REVIEW-2026-09-07.md`'s D1 found that the switch
|
||||
//! timings"), and the 2026-09-07 review found that the switch
|
||||
//! existed but four older per-frame `debug!` lines were not wired to it,
|
||||
//! filling the app's 2000-line log ring with frame spam before `Copy
|
||||
//! report` had a chance to include anything else. This is what a fix to
|
||||
|
||||
@@ -71,7 +71,7 @@ fn a_recorded_flick_releases_with_a_velocity_and_flings_the_list() {
|
||||
// found in the spline's own tests, and it left this one able to fail
|
||||
// in the "ran too long" direction only -- never in the "stopped dead"
|
||||
// direction, which is what Iris actually reported
|
||||
// (docs/REVIEW-2026-09-07.md's T1).
|
||||
// (review, 2026-09-07's T1).
|
||||
const REFERENCE_MS: u64 = 2071;
|
||||
const REFERENCE_PX: f32 = 11057.0;
|
||||
let end = flick.end_ms() + REFERENCE_MS * 2;
|
||||
|
||||
@@ -138,7 +138,7 @@ fn the_list_is_clipped_to_its_own_box() {
|
||||
// mask exists and sits in the right place; neither says any primitive
|
||||
// references it, so a broken `Mask::parent` chain -- what d507ae4
|
||||
// introduced -- would leave them green while a code fence inside a row
|
||||
// drew unclipped again (docs/REVIEW-2026-09-07.md's T3).
|
||||
// drew unclipped again (review, 2026-09-07's T3).
|
||||
let rows = h
|
||||
.render
|
||||
.active
|
||||
@@ -213,7 +213,7 @@ fn mask_chain(h: &Harness, mask: MaskIdx) -> Vec<MaskIdx> {
|
||||
/// every height already known -- including the second walk *back*, which
|
||||
/// is there because a regression that draws rows in the wrong place while
|
||||
/// travelling backwards would otherwise be checked only by the row count
|
||||
/// (docs/REVIEW-2026-09-07.md's T2). That is also the ordinary state of a
|
||||
/// (review, 2026-09-07's T2). That is also the ordinary state of a
|
||||
/// transcript being panned around in. The bound on how many rows are
|
||||
/// placed at once holds on all three.
|
||||
#[test]
|
||||
|
||||
@@ -5,6 +5,14 @@ they can be judged and reversed later. Detail lives in RUST.md (and IRIS.md
|
||||
for iris API changes); this file is only the summary. Newest first. Items
|
||||
marked **DEFERRED** are ones the agent chose not to decide alone.
|
||||
|
||||
**Paths in older entries are pre-2026-09-08.** The port's crates became
|
||||
one crate on that date -- `client-core` -> `app-rust`'s `src/client`,
|
||||
`transcript-ui` -> `src/ui`, `transcript-fixture` -> `src/ui/fixture.rs`
|
||||
and `tests/`, `desktop-app` -> `src/desktop`, `android-app` ->
|
||||
`src/android`, `android-shell` -> `src/shell`. Entries are left as they
|
||||
were written rather than rewritten, since each is a dated record;
|
||||
`docs/RUST.md`'s "One app crate" is the mapping.
|
||||
|
||||
## 2026-09-08 (the port is one crate, and iris is framework-only)
|
||||
|
||||
Asked for by Iris directly, so the shape rather than the fact is what is
|
||||
|
||||
+10
-2
@@ -9,6 +9,14 @@ Iris's instruction, 2026-09-08: "any major additions or design things
|
||||
should be added there, not just public API stuff"). Small and trivial
|
||||
things still stay out.
|
||||
|
||||
**Paths in older entries are pre-2026-09-08.** The port's crates became
|
||||
one crate on that date -- `client-core` -> `app-rust`'s `src/client`,
|
||||
`transcript-ui` -> `src/ui`, `transcript-fixture` -> `src/ui/fixture.rs`
|
||||
and `tests/`, `desktop-app` -> `src/desktop`, `android-app` ->
|
||||
`src/android`, `android-shell` -> `src/shell`. Entries are left as they
|
||||
were written rather than rewritten, since each is a dated record;
|
||||
`docs/RUST.md`'s "One app crate" is the mapping.
|
||||
|
||||
An entry gives the date, what changed, why, and a short before/after where
|
||||
it helps judge the change without the session that made it. Newest first.
|
||||
|
||||
@@ -914,7 +922,7 @@ block rather than all at once, which is if anything closer to what the
|
||||
old shortcut in `Selection`'s module doc was apologising for. `register`
|
||||
takes a `SelKey`; `unregister` still takes a `RowKey` and now drops every
|
||||
block of it (dropping only the first is how a freed widget gets left in
|
||||
the map -- the shape docs/REVIEW-2026-09-06.md's finding 1 called out).
|
||||
the map -- the shape a review on 2026-09-06 called out).
|
||||
|
||||
`Selection::locate(ui, render, pos_window)` is new: which block is under a
|
||||
window position, with that block's own local position and size. The
|
||||
@@ -1584,7 +1592,7 @@ streamed event" cost RUST.md's P0 box measured (20 events/second against a
|
||||
BTreeMap<RowKey, WeakWidget<TextEdit>>` is the one this crate has) must
|
||||
clear it in step with `List::clear()`** — the fallback drops every row
|
||||
`List` was holding, so any side table not cleared the same way is left
|
||||
pointing at widgets the clear just freed (docs/REVIEW-2026-09-06.md
|
||||
pointing at widgets the clear just freed (a review on 2026-09-06
|
||||
finding 1, fixed 2026-09-06 by `Selection::clear()`, called from
|
||||
`apply`'s `Rebuild` arm right before `List::clear()`). `bench_client.rs`, `transcript_client.rs`
|
||||
and `desktop-app/app.rs` all call this now instead of rebuilding on every
|
||||
|
||||
+30
-1184
File diff suppressed because it is too large.
Load diff
+13
-300
@@ -1,17 +1,6 @@
|
||||
# iris: one `draw` that reports a size
|
||||
|
||||
Preference stated by Iris, 2026-09-04, on the `rustify` branch. Recorded before
|
||||
any design or code so that it survives a cleared session. **Status: implemented
|
||||
2026-09-04, against every pass condition in §8** (measured, not assumed — see
|
||||
that section). Every widget listed in §7 was migrated in one change; none
|
||||
kept `desired_width`/`desired_height`. Five points needed correction or
|
||||
refinement beyond what this file originally specified — see "Deviations
|
||||
found during implementation" below, added right before "For IRIS.md" — read
|
||||
that section before touching `Aligned`, `Sized`, `MaxSize`, `Scroll`, or the
|
||||
move-slot lifecycle in `render_state.rs`, since each of those five is a real
|
||||
bug this file's first draft would have reproduced if implemented literally.
|
||||
|
||||
## What Iris asked for
|
||||
Iris, 2026-09-04:
|
||||
|
||||
> I don't like that widgets need both a draw and size functions. I'd much
|
||||
> rather them have a single draw that reports a size, and if it needs to be
|
||||
@@ -19,78 +8,16 @@ bug this file's first draft would have reproduced if implemented literally.
|
||||
> done after as well. This should be done efficiently like everything else
|
||||
> tries to do right now.
|
||||
|
||||
She added, a few minutes later: "single draw is not a requirement. It
|
||||
just seems more efficient from what I've heard. Feel free to override any
|
||||
decision I've made if you can find a genuinely better & still clean
|
||||
alternative." So the single-draw model is the default to design against,
|
||||
and the design below may reject it, but only with a written comparison
|
||||
showing the alternative does less work per frame and is no harder to use.
|
||||
|
||||
Standing constraints from RUST.md still apply: no DSL, plain Rust, do as
|
||||
little processing as possible per frame, but the model must cover every
|
||||
layout need a real app has (the transcript's virtualised list, wrapped
|
||||
text whose height depends on width, rows and columns that size to their
|
||||
children, overlays, masks).
|
||||
|
||||
## What exists today
|
||||
|
||||
`Widget` (`iris/core/src/widget/mod.rs`) has three methods: `draw(&mut
|
||||
self, &mut Painter)`, `desired_width(&mut self, &mut SizeCtx) -> Len` and
|
||||
`desired_height`. A parent asks `SizeCtx::width/height` for a child, which
|
||||
is memoised per widget id and axis in `Cache.size` keyed on the outer
|
||||
size, then places the child with `Painter::widget_within(region)`. So a
|
||||
child is visited twice (sized, then drawn), every widget implements sizing
|
||||
twice (one per axis), and a widget whose size depends on what it draws
|
||||
(wrapped text, a laid-out paragraph) does the layout in the size pass and
|
||||
again in the draw pass unless it caches by hand.
|
||||
|
||||
Primitives are already positioned by `UiRegion` values whose scalars have
|
||||
a `rel` and an `abs` part, resolved against the window in the vertex
|
||||
shader (`core/src/render/shader.wgsl`), and `Primitives::region_mut`
|
||||
exists to rewrite one instance's region in place. That is the mechanism a
|
||||
"move after the fact" can build on.
|
||||
|
||||
## What the design must answer
|
||||
|
||||
1. **Parent-before-child ordering.** A row has to know each child's width
|
||||
to place the next one, but under "one draw" the child's size only
|
||||
exists after it has drawn. The answer is meant to be: the child draws
|
||||
at a provisional origin, reports its size, and the parent *moves* it.
|
||||
The move must be O(1) per moved subtree, not O(primitives in the
|
||||
subtree). One way: every instance carries an index into a small
|
||||
per-widget offset buffer, so moving a widget writes one entry and the
|
||||
vertex shader adds it. Other ways may be better; the design should say
|
||||
what was considered.
|
||||
2. **Move vs resize are different costs and must be kept apart.** A move
|
||||
never re-runs `draw`. A resize re-runs `draw` for exactly the widgets
|
||||
whose size input changed, and a widget whose output does not depend on
|
||||
its size (an icon, a fixed rect) must be able to say so and be skipped.
|
||||
3. **Size-dependent content.** Wrapped text is the hard case: its height
|
||||
is a function of its width. A single `draw` receives the available
|
||||
size (what `SizeCtx.outer` is today) and reports what it used, so the
|
||||
two-pass "measure then draw" collapses into one for the common case.
|
||||
The design must say what happens when a parent wants the child's
|
||||
height *before* deciding the width it will offer (rare; say whether it
|
||||
is supported, or is done by drawing twice as an explicit, opt-in cost).
|
||||
4. **Caching.** Today's `Cache.size` memoises by (id, axis, outer). The
|
||||
replacement should memoise the whole draw result by (id, available
|
||||
size) so that an unchanged subtree costs nothing on the next frame,
|
||||
which is what makes a virtualised list cheap.
|
||||
5. **Everything currently written against `desired_width`/`desired_height`
|
||||
moves over in one change**, per the code rules: two names for one
|
||||
concept is not an intermediate state to leave behind. The widgets are
|
||||
in `iris/src/widget/` (`ptr`, `mask`, `image`, `rect`, `trait_fns`, and
|
||||
whatever else is there when the change is made).
|
||||
|
||||
## Order relative to the texture work
|
||||
|
||||
TEXTURES.md's redesign touches the render core (shader, `GpuTextures`,
|
||||
`Primitives`, `Painter`'s texture calls). This change touches the widget
|
||||
trait, `SizeCtx`, `Cache`, `Painter`'s widget calls, and any offset
|
||||
mechanism the vertex shader needs. They overlap in `Painter` and the
|
||||
shader, so they are done **in sequence, textures first**, and the layout
|
||||
design here is written (not implemented) while the texture work is in
|
||||
progress, then implemented on top of it.
|
||||
**Implemented 2026-09-04.** Every widget was migrated in one change; none
|
||||
kept `desired_width`/`desired_height`. What is kept below is the design as
|
||||
it stands, the five corrections implementation forced (read those before
|
||||
touching `Aligned`, `Sized`, `MaxSize`, `Scroll` or the move-slot lifecycle
|
||||
in `render_state.rs` -- each is a real bug the first draft would have
|
||||
reproduced), and the two later additions that build on it. The
|
||||
pre-implementation framing -- what the old trait looked like, the checklist
|
||||
the design had to answer, the migration list, the pass conditions and the
|
||||
"copy this into IRIS.md" note -- was deleted on 2026-09-08, having been
|
||||
carried out; `docs/IRIS.md`'s 2026-09-04 entry is the public-API record.
|
||||
|
||||
## Design
|
||||
|
||||
@@ -517,186 +444,7 @@ nothing left for `SizeCtx` to answer; `draw_text`/`label`/`px_size`/
|
||||
today (compare `size.rs:71-90` against `painter.rs:152-174`) and this
|
||||
deletes the `SizeCtx` copies, keeping the `Painter` ones.
|
||||
|
||||
### 7. Migration — every file and widget that changes
|
||||
|
||||
One change, in dependency order (rename-and-move-together, per the code
|
||||
rules — no intermediate state with both trait shapes):
|
||||
|
||||
- `core/src/widget/mod.rs` — the `Widget` trait (§1), delete
|
||||
`WidgetAxisFns`, update `impl Widget for ()`.
|
||||
- `core/src/ui/size.rs` — delete `SizeCtx` (the type and all its methods).
|
||||
- `core/src/ui/cache.rs` — delete `Cache` (§5).
|
||||
- `core/src/ui/painter.rs` — `widget`/`widget_within`/`widget_at` return
|
||||
`Size`; add `reposition`, `draw_twice`; delete `size_ctx`, `size`,
|
||||
`len_axis`; `primitive_at` writes `move_idx`.
|
||||
- `core/src/ui/render_state.rs` — `draw_inner` captures and stores
|
||||
`ActiveData.size`; `mov` becomes the O(1) offset write (§2); resize
|
||||
narrowing (§3a); `redraw`'s per-axis loop reads `ActiveData.size`
|
||||
instead of `Cache.size`.
|
||||
- `core/src/ui/active.rs` — `ActiveData` gains `size: Size`,
|
||||
`move_slot: MoveIdx`.
|
||||
- `core/src/ui/mod.rs` — `UiData` gains `move_offsets`.
|
||||
- `core/src/render/data.rs` — `PrimitiveInstance` gains `move_idx`;
|
||||
new `MoveOffset` struct.
|
||||
- `core/src/render/primitive.rs` — thread `move_idx` through `PrimitiveInst`
|
||||
and `Primitives::write`, matching `mask_idx`.
|
||||
- `core/src/render/mod.rs` — bind the new `move_offsets` storage buffer
|
||||
(group 2, beside `masks`) and its update path.
|
||||
- `core/src/render/shader.wgsl` — `InstanceInput` gains `move_idx`;
|
||||
`MoveOffset`/`UiScalar`-shaped storage binding; a shared `resolve_move`
|
||||
function (§2b) called from both `vs_main` (a primitive's own corners)
|
||||
and `fs_main` (its mask's corners, once `Mask` carries `move_idx`).
|
||||
- `core/src/ui/render_state.rs` — additionally, `resolved_region` (§2b)
|
||||
and `window_region` (`:264-267`) reimplemented on top of it.
|
||||
- `src/default/sense.rs` — `run_sensors`'s hit-test read (`:170`) switches
|
||||
from `self.active.get(id).unwrap().region` to `self.resolved_region(*id)`
|
||||
(§2b) — the pointer-routing fix this design requires, not an optional
|
||||
follow-up.
|
||||
- `core/src/render/data.rs` — additionally, `Mask` (`:46-49`) gains
|
||||
`move_idx: u32` (§2b).
|
||||
- `core/src/ui/painter.rs` — additionally, `set_mask` (`:49-52`) writes
|
||||
`move_idx: self.move_slot` into the `Mask` it pushes (§2b).
|
||||
- Every widget with a two-method `impl Widget`, collapsed to one `draw`
|
||||
(§1, §6), `is_size_independent` added where true: `core/src/widget/mod.rs`
|
||||
(`impl Widget for ()`), `iris/src/widget/rect.rs` (`Rect`, → true),
|
||||
`iris/src/widget/image.rs` (`Image`, → true — a decoded image's primitive
|
||||
never depends on the region it is offered, same as `Rect`),
|
||||
`iris/src/widget/mask.rs` (`Masked`), `iris/src/widget/ptr.rs`
|
||||
(`WidgetPtr`), `iris/src/widget/text/mod.rs` (`Text`, §4),
|
||||
`iris/src/widget/text/edit.rs` (`TextEdit`),
|
||||
`iris/src/widget/position/scroll.rs` (`Scroll`, keeps its `mov`-shaped
|
||||
offset, now O(1) automatically via §2), `iris/src/widget/position/align.rs`
|
||||
(`Aligned`, §6), `iris/src/widget/position/max_size.rs` (`MaxSize`),
|
||||
`iris/src/widget/position/layer.rs` (`LayerOffset`),
|
||||
`iris/src/widget/position/pad.rs` (`Pad`),
|
||||
`iris/src/widget/position/stack.rs` (`Stack`),
|
||||
`iris/src/widget/position/offset.rs` (`Offset`),
|
||||
`iris/src/widget/position/span.rs` (`Span`, §4's `draw_twice` for the
|
||||
cross-axis case, deleting `desired_ortho`'s duplicate loop),
|
||||
`iris/src/widget/position/sized.rs` (`Sized`).
|
||||
This list was produced by `grep -rn "impl Widget for\|fn desired_width\|fn desired_height"`
|
||||
across `core/` and `src/`; re-run it before starting, since it is the
|
||||
authoritative check that nothing was missed, not this paragraph.
|
||||
- `iris/examples/{minimal.rs,task.rs,view.rs,tabs/main.rs}` — no direct
|
||||
`impl Widget` found in any example (verified by the same grep); they use
|
||||
the builder DSL in `core/src/widget/trait_fns.rs` and should need no
|
||||
source change, which is itself part of the pass condition below.
|
||||
|
||||
### 8. Pass conditions
|
||||
|
||||
1. **Every example under `iris/examples` renders identically.** Run
|
||||
`iris/run-headless.sh EXAMPLE --shot PNG` for each of `minimal`, `task`,
|
||||
`view`, `tabs` before and after, and diff the PNGs pixel-for-pixel — not
|
||||
"looks right," since a subtle wrap or alignment regression is exactly
|
||||
what a diff catches and a glance does not.
|
||||
|
||||
**Result (2026-09-04): pass, all four, 0 differing bytes.** No PNG
|
||||
library is installed in this VM (no PIL, no ImageMagick, no pip), so the
|
||||
diff is a from-scratch PNG decoder (`zlib` + the five filter types) at
|
||||
`/tmp/layout-shots/pngdiff.py`, comparing decoded pixel bytes rather than
|
||||
file bytes (`cmp` alone is not conclusive across two separately-encoded
|
||||
PNGs, though it happened to agree here for `minimal`). Before-shots were
|
||||
taken with `git stash` at the pre-change commit; `tabs` needed two real
|
||||
fixes (deviations 1 and 2 below) before it stopped differing — the other
|
||||
three matched on the first try.
|
||||
2. **Unchanged-frame cost, measured, not assumed.** Add a counter beside
|
||||
the existing `debug_layers`/`active_widgets` instrumentation
|
||||
(`render_state.rs:241-262`) for (a) `Widget::draw` invocations and (b)
|
||||
`Primitives::write`/`region_mut` calls, both per `update()` call. Drive
|
||||
one example (`tabs`, since it already has multiple widgets and an
|
||||
interactive element) through one frame with nothing changed and report
|
||||
both counts — the pass condition is **0 draws and 0 primitive rewrites**
|
||||
for a frame in which nothing was marked dirty, resized, or moved.
|
||||
|
||||
**Result (2026-09-04): pass, 0 and 0.** Implemented as
|
||||
`UiRenderState::take_counters() -> (u64, u64, u64)` (draws, `region_mut`
|
||||
rewrites, `move_offsets` writes — a third counter, for condition 3
|
||||
below), reset on read. Measured in
|
||||
`iris/src/layout_tests.rs::an_unchanged_frame_draws_and_rewrites_nothing`
|
||||
against a `Scroll` over 500 fixed-height rects (not the `tabs` example —
|
||||
see the note on condition 3 for why this runs as a plain unit test
|
||||
instead).
|
||||
3. **Single-moved-child cost, measured.** Same counters, one frame in
|
||||
which exactly one widget is moved (not resized) with N primitives in its
|
||||
subtree — the pass condition is **1 write to `move_offsets`, 0 calls to
|
||||
`Widget::draw`, 0 calls to `region_mut`**, independent of N. Construct
|
||||
the case with a `tabs`-style example holding a deliberately large text
|
||||
block (hundreds of glyphs) inside a `Scroll`, so N is large enough that
|
||||
an O(N) regression would show up as a non-trivial write count rather
|
||||
than being lost in noise.
|
||||
|
||||
**Result (2026-09-04): pass — 0 draws, 0 rewrites, 1 move_offsets
|
||||
write, N = 500.** Built with rects rather than glyphs
|
||||
(`iris/src/layout_tests.rs::scrolling_moves_in_o1_without_a_redraw`):
|
||||
`iris-core`/`iris` touch no GPU or window to lay out and move a tree, so
|
||||
this runs as a plain `cargo test`, not through `run-headless.sh` — a
|
||||
`Widgets`/`UiData` pair and a bare `UiRsc` impl are enough, and it is
|
||||
faster and more precise than reading counters out of a real example's
|
||||
stderr. Getting a clean single move took two follow-up fixes beyond the
|
||||
design as written (deviation 3, the `parent_move_slot` threading; and
|
||||
the `Scroll` design decision below about offering last frame's content
|
||||
length) — without either, the count was in the thousands (every rect in
|
||||
the subtree redrawing) rather than 1.
|
||||
4. **Hit-testing follows the move, not just the render.** In the same
|
||||
scrolled-`tabs` construction as condition 3, scroll the content, then
|
||||
send a synthetic cursor position over a widget that moved and assert
|
||||
`run_sensors` (`src/default/sense.rs:154-200`) routes to that widget's
|
||||
id, not to whatever is now at its pre-scroll coordinates or to nothing.
|
||||
This is a correctness check, not a timing one — §2b's fix is required
|
||||
before §2 can ship at all, and this is what would fail silently
|
||||
(nothing on screen indicates a missed or misrouted hit) if it were
|
||||
skipped.
|
||||
|
||||
**Result (2026-09-04): pass**, but checked one level below
|
||||
`run_sensors`: `iris/src/layout_tests.rs::hit_testing_follows_a_scrolled_widget`
|
||||
scrolls a widget and asserts `UiRenderState::resolved_region` (the
|
||||
query `run_sensors`'s hit-test and `window_region` both now go through,
|
||||
per §2b) reports the moved, not the pre-scroll, position — within
|
||||
0.01px of the exact expected delta. `run_sensors` itself needs a
|
||||
`HasEvents`/window/cursor-state harness this pass did not build; the
|
||||
coverage that matters (does the position query the router uses reflect
|
||||
the move) is exercised directly instead.
|
||||
5. **A mask moves with its subtree.** Render a `Masked`-wrapped `Scroll`
|
||||
both before and after scrolling it (`iris/run-headless.sh` against a
|
||||
small purpose-built example, or an addition to `tabs`), and diff the
|
||||
two frames: the clipped edge of the content must have moved with the
|
||||
scroll while the viewport's own border (drawn by `Masked`, not moved)
|
||||
stays put — the specific case worked through in §2b. A mask rectangle
|
||||
that stayed at its pre-scroll position while its content slid past it
|
||||
is the regression this checks for, and it is visible in a single
|
||||
screenshot, not just in a counter.
|
||||
|
||||
**Result (2026-09-04): pass, checked numerically rather than by
|
||||
screenshot.** No example in this repository builds a `Masked`-wrapped
|
||||
`Scroll` (`tabs`'s "text edit scroll" tab uses `TextEdit`'s own internal
|
||||
scrolling, not this widget), so there was nothing to screenshot without
|
||||
first authoring a new example. Checked instead in
|
||||
`iris/src/layout_tests.rs::a_mask_stays_put_while_its_scrolled_content_moves`,
|
||||
on the exact data the fragment shader's `resolve_move` reads: the
|
||||
masked widget's own `move_offsets` slot delta is `[0, 0]` both before
|
||||
and after scrolling its content, because `Masked` is never itself the
|
||||
target of a move — only its child is, on a separate, deeper slot in the
|
||||
chain (§2b's "scroll-container case, checked rather than assumed"). A
|
||||
pixel-level screenshot check of this remains open; see RUST.md's next
|
||||
step.
|
||||
6. **`cargo test --workspace`, `cargo clippy --all-targets`, `cargo fmt`**
|
||||
stay clean at the defaults (iris has no tests today per I0b, so this is
|
||||
presently only clippy/fmt; add the first real widget-layer tests here if
|
||||
the move-offset chain or `draw_twice` are non-trivial enough to want
|
||||
one, per "match the codebase's testing posture" — judge that once the
|
||||
code exists rather than pre-committing to a number of tests here).
|
||||
|
||||
**Result (2026-09-04): pass.** `cargo fmt --all -- --check`,
|
||||
`cargo build --workspace --all-targets`, and `cargo clippy --all-targets`
|
||||
are all clean (one pre-existing, unrelated warning about `naga`/`wgpu`/
|
||||
`winit` future-incompatibility, from dependencies, not this change).
|
||||
`cargo test --workspace`: the 14 pre-existing `TextEdit` tests plus 4 new
|
||||
ones in `iris/src/layout_tests.rs` (conditions 2–5 above), 18 passed, 0
|
||||
failed — the move-offset chain turned out non-trivial enough (three real
|
||||
bugs found only by writing it) to clearly clear the "match the testing
|
||||
posture" bar this section left open.
|
||||
|
||||
### 9. Rejected, and why
|
||||
### 7. Rejected, and why
|
||||
|
||||
- **A flat (non-chained) per-subtree offset table**, Iris's literal
|
||||
phrasing — rejected in §2 for breaking under nested independent moves
|
||||
@@ -857,7 +605,7 @@ unspecified rather than getting them wrong:
|
||||
a widget once more than the old code would have (if the parent it
|
||||
bubbles up to ends up redrawing the same widget again as part of its own
|
||||
relayout) — `draw_inner`'s own skip/move dispatch absorbs most of that
|
||||
redundancy for free, and this path is not one of §8's measured
|
||||
redundancy for free, and this path was not one of the migration's measured
|
||||
conditions, so the remaining slack was accepted rather than chased
|
||||
further.
|
||||
|
||||
@@ -913,41 +661,6 @@ resolution-independent, a fraction of the parent). `Span::gap` and
|
||||
on them the same as any other size; a bare number is still `abs`,
|
||||
physical pixels, unchanged.
|
||||
|
||||
## For IRIS.md
|
||||
|
||||
When this lands, copy this entry into `IRIS.md` (newest first):
|
||||
|
||||
> **2026-09-04 — `Widget::draw` reports the size it used; `desired_width`/
|
||||
> `desired_height` are gone.** A widget used to implement three methods
|
||||
> (`draw`, `desired_width`, `desired_height`); it now implements one,
|
||||
> `fn draw(&mut self, painter: &mut Painter) -> Size`, which draws into
|
||||
> `painter.region()` and returns how much of it was used. Why: the two
|
||||
> extra methods routinely re-simulated what `draw` was about to do anyway
|
||||
> (`Span::desired_ortho` copied its own draw loop to get cross-axis sizing
|
||||
> right) — one visit per widget per frame instead of up to three. A
|
||||
> container that needs a child's size before placing it (alignment,
|
||||
> centering) draws the child once at a provisional region, reads the
|
||||
> returned `Size`, and calls the new `Painter::reposition` to move it into
|
||||
> its final spot — an O(1) offset write, not a second draw. A widget whose
|
||||
> drawn output never depends on the size it's given (a fixed-size `Rect`,
|
||||
> a decoded `Image`) overrides the new `fn is_size_independent(&self) ->
|
||||
> bool { false }` to `true`, which skips redrawing it when only its
|
||||
> offered region changes shape.
|
||||
>
|
||||
> ```rust
|
||||
> // before
|
||||
> fn draw(&mut self, painter: &mut Painter) { /* ... */ }
|
||||
> fn desired_width(&mut self, ctx: &mut SizeCtx) -> Len { /* ... */ }
|
||||
> fn desired_height(&mut self, ctx: &mut SizeCtx) -> Len { /* ... */ }
|
||||
>
|
||||
> // after
|
||||
> fn draw(&mut self, painter: &mut Painter) -> Size { /* ... */ }
|
||||
> ```
|
||||
>
|
||||
> `SizeCtx` and `Cache` are gone with it — see `LAYOUT.md` for the full
|
||||
> design, the move-offset mechanism this shipped alongside, and the file
|
||||
> list.
|
||||
|
||||
## Masks with a shape (decided 2026-09-07, built 2026-09-08)
|
||||
|
||||
Iris, on the code block's scrolling: "the code block scrolling currently
|
||||
|
||||
@@ -1,214 +0,0 @@
|
||||
# Review: iris changes since 0e46293
|
||||
|
||||
Scope: `git diff 0e46293..HEAD -- iris/ client-core/` (58 files, +5224/-226).
|
||||
Read-only review; no source changed. Ordered likely-bug, then invariant
|
||||
guards, then rules, then tests/docs.
|
||||
|
||||
## Likely bugs
|
||||
|
||||
1. **`iris/transcript-ui/src/lib.rs:152-160` (`RowDiff::Rebuild` arm of
|
||||
`TranscriptScreen::apply`) never unregisters the rows it drops from
|
||||
`Selection`, so a stale `WeakWidget<TextEdit>` outlives the widget it
|
||||
points to and the next touch on *any* row panics.**
|
||||
`Selection::rows: BTreeMap<RowKey, WeakWidget<TextEdit>>` documents its
|
||||
own contract at `selection.rs:69-71`: "every addition here needs its
|
||||
removal ... called when `List` evicts the row." The `ReplaceLast` arm
|
||||
above it honours this (`lib.rs:143-145`, `self.selection.borrow_mut()
|
||||
.unregister(old_key)` when the key changes). The `Rebuild` arm calls
|
||||
`(self.list)(rsc).clear()` and rebuilds every row from `new_rows`, but
|
||||
never touches `self.selection` — any key present in `old_rows` and
|
||||
*absent* from `new_rows` (exactly what `group_tool_runs` regrouping two
|
||||
separate tool-call rows into one produces — see `diff_tests::
|
||||
a_tool_run_closing_and_joining_an_earlier_call_is_a_regroup_fallback`,
|
||||
which tests the diff decision but not `apply` itself) is left in
|
||||
`self.rows` pointing at a widget `List::clear()` just freed.
|
||||
`TextEditable::edit` (`iris/src/widget/text/edit.rs:582-587`) resolves
|
||||
that handle with `ui.widgets.get_mut(self).unwrap()` — an unconditional
|
||||
panic on the freed slot. `Selection::begin` (`selection.rs:88-101`)
|
||||
iterates *every* registered row (`w.edit(ui).deselect()`) on an
|
||||
ordinary fresh press, so the crash fires on the next tap anywhere in
|
||||
the transcript after a regroup, not only on a tap targeting the
|
||||
orphaned row.
|
||||
Fix: give `Selection` a way to reconcile against the row set that
|
||||
survived a rebuild (e.g. `Selection::retain(&self, keys: &BTreeSet<RowKey>)`
|
||||
removing everything else, called from the `Rebuild` arm before
|
||||
rebuilding), or simplest — call `self.selection.borrow_mut()` cleared
|
||||
the same way `List::clear()` clears the list, then let the rebuild's
|
||||
`push_row` calls re-`register` everything as they already do.
|
||||
|
||||
## Guarded invariants missing
|
||||
|
||||
2. **`iris/src/widget/list.rs:751` (`List::place`) indexes/expects on
|
||||
`slot` with no assertion that it exists.** `slot_widget` (`:563-575`)
|
||||
panics via `.expect(...)` for a sentinel with no widget set, and does
|
||||
an unchecked `&self.items[s as usize]` for a real index — a bare
|
||||
"index out of bounds" with no context if `place` is ever reached with a
|
||||
stale slot. Every current caller happens to derive `slot` from
|
||||
`repair_anchor`/`prev_slot`/`next_slot`, which already check existence,
|
||||
but that invariant is enforced by convention across three call sites,
|
||||
not by the function that depends on it. Add
|
||||
`debug_assert!(self.slot_exists(slot), "place() called with a slot that doesn't exist: {slot:?}");`
|
||||
at the top of `place`.
|
||||
3. **`iris/src/widget/list.rs:426` (`List::fling`) and `sense.rs`'s
|
||||
`FlingCalculator::distance`/`duration`/`position_at` never check that
|
||||
the incoming velocity is finite.** A `NaN`/`inf` velocity (a
|
||||
`VelocityTracker::velocity()` divide-by-near-zero span, or a caller
|
||||
passing a raw device value straight through) propagates through
|
||||
`deceleration_for`'s `.ln()` silently — the fling either never settles
|
||||
(`settled_on_schedule` compares against a `NaN` `duration()`, which is
|
||||
always `false`) or jumps to `NaN` positions with nothing on screen
|
||||
saying why. Add `debug_assert!(velocity_px_per_s.is_finite())` in
|
||||
`List::fling` and `FlingCalculator::new`/`distance`.
|
||||
4. **`iris/src/sense.rs:592-604` (`VelocityTracker::velocity`) has no
|
||||
assertion that samples are chronological.** `add_sample` trusts its
|
||||
caller's `Instant` ordering; a caller that samples out of order (a
|
||||
restored/replayed gesture, a test) would silently produce a negative
|
||||
`span` handled only by the `span <= 0.0 => 0.0` catch-all, masking the
|
||||
bug that produced it rather than surfacing it. Add
|
||||
`debug_assert!(self.samples.back().is_none_or(|&(last, _)| at >= last))`
|
||||
in `add_sample`.
|
||||
5. **`iris/core/src/render/frame_report.rs:247-252` (`mark_phase`) has no
|
||||
assertion that phases are pushed in non-decreasing `start_index`
|
||||
order.** `phase_stats`'s slicing (`:274`, `idx >= phase.start_index &&
|
||||
idx < end_index`) silently produces an empty or nonsensical slice for
|
||||
an out-of-order phase rather than surfacing the misuse — cheap to add
|
||||
given `self.phases.last()` is already in scope:
|
||||
`debug_assert!(self.phases.last().is_none_or(|p| self.total_frames >= p.start_index));`
|
||||
|
||||
## Rules
|
||||
|
||||
6. **Two mechanisms answer "what row selection points at, still valid?"**
|
||||
`Selection` relies on callers remembering to `unregister` (finding 1);
|
||||
`List` relies on callers deriving slots only from already-checked
|
||||
sources (finding 2). Both are the same class of problem — a derived
|
||||
handle that silently outlives what it points to — solved ad hoc twice
|
||||
rather than once. Not asking for a shared abstraction here, but the two
|
||||
should at minimum cross-reference each other's doc comment so the next
|
||||
caller who adds a third handle-into-`List`-rows type (the code rules'
|
||||
"a rule that governs a set belongs to the set") finds both existing
|
||||
examples.
|
||||
7. **`iris/android-app/src/bench_client.rs:224-225` (`battery_line`)
|
||||
calls `.min().unwrap()`/`.max().unwrap()` on `samples` guarded three
|
||||
lines above by `if samples.is_empty()`, which is fine — but the guard
|
||||
and the two unwraps are two statements apart with a `let mean = ...`
|
||||
in between reading the same slice; a future edit reordering those
|
||||
lines loses the guard's protection silently.** Low severity (this is
|
||||
the bench tool, not the app), but worth a one-line comment tying the
|
||||
unwraps back to the guard, or restructuring as
|
||||
`let (Some(min), Some(max)) = (samples.iter().min(), samples.iter().max())`
|
||||
pattern so the empty case can't be separated from the check by a future
|
||||
edit.
|
||||
|
||||
## Tests
|
||||
|
||||
8. **No test exercises `TranscriptScreen::apply`'s `Rebuild` arm through
|
||||
`Selection`.** `lib.rs`'s `diff_tests` module (`:284-379`) tests only
|
||||
the pure `diff_rows` decision function, never `apply` itself wired to a
|
||||
real `Selection`; `selection.rs`'s own tests (`a_missed_press_start_
|
||||
recovers_on_the_next_pressing_frame`, `unregister_forgets_the_row_and_
|
||||
clears_a_matching_anchor`) never go through `apply`/`List::clear`
|
||||
either. This is exactly the gap that let finding 1 through: the two
|
||||
pieces (`apply`'s fallback, `Selection`'s registration contract) are
|
||||
each tested in isolation and never together. Add: build a
|
||||
`TranscriptScreen`, force a `RowDiff::Rebuild` (two adjacent tool-call
|
||||
rows regrouping, per the existing `diff_tests` case), then call
|
||||
`selected_text`/simulate a fresh press on a surviving row and assert no
|
||||
panic.
|
||||
9. **`iris/src/widget/list.rs`'s fling tests check total distance and the
|
||||
start/end clamp but not the speed profile in between.**
|
||||
`fling_moves_the_list_and_then_settles`/`fling_distance_is_positive_
|
||||
toward_the_end` only assert the fling started, moved in the right
|
||||
direction, and eventually stopped — none checks that
|
||||
`tick_fling`'s per-tick delta is *monotonically decreasing* once past
|
||||
the fling's peak (the property `fling_calculator_tests::position_at_
|
||||
is_monotonic_and_clamped_past_the_end` already checks one level down,
|
||||
for `FlingCalculator` alone, but never through `List::tick_fling`'s own
|
||||
`scroll`/`anchor.offset` accumulation). A regression that made
|
||||
`tick_fling` apply the *total* distance every tick instead of the
|
||||
incremental one, for instance, would still pass both existing tests
|
||||
(final position and direction are unaffected by how the interior ticks
|
||||
split it up) while being wildly wrong every intermediate frame.
|
||||
10. **`iris/src/widget/list.rs::replacing_the_last_row_stays_pinned_to_
|
||||
the_bottom` and its sibling test `replace_back`'s effect on the
|
||||
displayed row, never that the row it evicted is actually gone from
|
||||
`heights`/`extents`.** Both tests assert the *new* row's position;
|
||||
neither asserts `old.key` is absent from `list_ref.heights`/`extents`
|
||||
after the replace (the "stale primitive" class finding 1 is a
|
||||
production instance of). A cheap addition: assert
|
||||
`!list_ref.heights.contains_key(&old.key)` after `replace_back` in the
|
||||
existing test, since `old.key` is already returned to the test as
|
||||
`evicted`... (`lib.rs` calls it that way; the `list.rs` test would need
|
||||
to capture the key from `old` similarly.)
|
||||
|
||||
## Docs
|
||||
|
||||
No missing `IRIS.md` entry found for a *public* API change in this diff —
|
||||
`List::fling`/`VelocityTracker`/`FlingCalculator`, `List::
|
||||
anchor_position_display`, `FrameReport::mark_phase`/`phase_stats`/
|
||||
`late_at_hz`, `UiRenderNode::new`'s `Result` change, `Len::dp`, and
|
||||
`List::replace_back`/`clear`/`TranscriptScreen::apply` all have entries.
|
||||
The `List::replace_back`/`clear`/`TranscriptScreen::apply` entry
|
||||
(`docs/IRIS.md:526`) predates this review's finding 1 and does not mention
|
||||
`Selection`'s registration contract at all — once finding 1 is fixed,
|
||||
that entry should gain a line noting what the fix requires of a caller
|
||||
that keeps its own row-keyed side table (the same shape `Selection` is),
|
||||
so the next such table doesn't reproduce the same gap.
|
||||
|
||||
## Fixed, 2026-09-06
|
||||
|
||||
All ten findings addressed after the `DragGesture` merge (`selection.rs`
|
||||
was rewritten by that merge, but finding 1's shape and location were
|
||||
unchanged — `TranscriptScreen::apply`'s `Rebuild` arm, `iris/transcript-ui/
|
||||
src/lib.rs`).
|
||||
|
||||
1. **Fixed.** `Selection::clear()` (`selection.rs`) drops `rows` and
|
||||
`anchor`, called from `apply`'s `Rebuild` arm right before
|
||||
`List::clear()` — `push_row` re-`register`s whatever survives as it
|
||||
rebuilds each row, the "simplest" fix option the finding named.
|
||||
2. **Fixed.** `debug_assert!(self.slot_exists(slot), ...)` at the top of
|
||||
`List::place` (`iris/src/widget/list.rs`).
|
||||
3. **Fixed.** `debug_assert!(velocity_px_per_s.is_finite())` in
|
||||
`List::fling`, and `debug_assert!(velocity.is_finite())` in
|
||||
`FlingCalculator::distance`/`duration` (`iris/src/sense.rs`).
|
||||
`position_at` calls both, so it inherits the guard rather than needing
|
||||
its own.
|
||||
4. **Fixed.** `debug_assert!` on chronological sample order in
|
||||
`VelocityTracker::add_sample` (`iris/src/sense.rs`).
|
||||
5. **Fixed.** `debug_assert!` on non-decreasing `start_index` in
|
||||
`FrameReport::mark_phase` (`iris/core/src/render/frame_report.rs`).
|
||||
6. **Fixed (doc cross-reference only, as asked).** `Selection::register`'s
|
||||
doc now points at `List::place`'s `slot_exists` assertion and vice
|
||||
versa isn't needed since finding 2's fix already cites this file in
|
||||
its own comment; both are grep-able on "docs/REVIEW-2026-09-06.md" and
|
||||
on each other's type names.
|
||||
7. **Fixed.** `bench_client.rs::battery_line` restructured to
|
||||
`let (Some(min), Some(max)) = (samples.iter().min(), samples.iter().max())`,
|
||||
so the empty-guard and the two lookups can no longer be separated by a
|
||||
future edit.
|
||||
8. **Fixed.** `transcript-ui`'s new `apply_tests::
|
||||
a_row_dropped_by_a_regroup_does_not_outlive_itself_in_selection`
|
||||
(`lib.rs`) builds a real `TranscriptScreen`, forces the same regroup
|
||||
shape `diff_tests` already covers at the pure-diff level, calls `apply`,
|
||||
and then `Selection::begin` on a surviving row — which panicked before
|
||||
fix 1, resolving a `WeakWidget` `List::clear()` had just freed.
|
||||
9. **Fixed.** `list.rs`'s new `tick_fling_applies_shrinking_incremental_
|
||||
deltas` flings toward the end from `jump_to_start` and asserts each
|
||||
tick's `extents[&0]` delta is no larger than the previous one — would
|
||||
fail against a `tick_fling` that applied the total spline distance
|
||||
every tick instead of the incremental slice, which the two pre-existing
|
||||
fling tests cannot catch.
|
||||
10. **Fixed.** `list.rs`'s new `replace_back_forgets_the_evicted_keys_own_
|
||||
height` replaces row 4 with a row keyed `100` (the two existing
|
||||
`replace_back` tests always reuse the same key, so neither actually
|
||||
exercises the removal) and asserts `heights` no longer contains the
|
||||
evicted key.
|
||||
|
||||
Docs: `docs/IRIS.md`'s 2026-09-05 `List::replace_back`/`clear`/
|
||||
`TranscriptScreen::apply` entry now has a line on what the fix requires of
|
||||
a caller with its own row-keyed side table, naming `Selection` as the
|
||||
example and dating the fix.
|
||||
|
||||
Verification run alongside the rest of this pass's checks: `cargo fmt
|
||||
--all`, `cargo clippy --workspace --all-targets`, `cargo test --workspace`
|
||||
from `iris/` — see docs/RUST.md's plan box for the pass/fail and any
|
||||
caveats from this same session.
|
||||
@@ -1,459 +0,0 @@
|
||||
# Review, 2026-09-07 — `ba2afba..origin/rustify`
|
||||
|
||||
Read-only review of the day's 24 commits: the glyph-atlas fix, the fling
|
||||
spline and Lsq2 velocity estimator, keyboard/IME insets and `targetSdk`,
|
||||
historical touch samples and the input clock, list culling / clamp /
|
||||
anchor re-homing, nested masks and `draw_again`, the headless harness +
|
||||
`transcript-fixture` + `rig-input`, desktop density, the release profile,
|
||||
platform fonts + the Android monospace patch, and the client-core log ring
|
||||
with `POST /client-log`.
|
||||
|
||||
**Verified while reviewing** (working tree, which also carries three other
|
||||
agents' uncommitted edits — `iris/src/sense.rs`, `iris/core/src/ui/render_state.rs`,
|
||||
`iris/src/lib.rs`, `iris/core/src/orientation/axis.rs`, and an untracked
|
||||
`iris/src/diagnostics.rs`): `cargo fmt --check` clean in `iris/`,
|
||||
`client-core/` and `server/`; `cargo clippy --all-targets` clean in `iris/`
|
||||
and `client-core/`; `cargo test --lib -p iris` 101 passed, `cargo test -p
|
||||
transcript-fixture` 10 passed. The `iris` doctest target fails to link
|
||||
(`extern location for iris_core does not exist`) — a stale build artefact,
|
||||
not a code fault, but worth knowing before trusting `cargo test -p iris`
|
||||
as a whole.
|
||||
|
||||
The work is unusually well documented and the two "a test that compared
|
||||
the code with itself" findings the authors made themselves are real and
|
||||
were fixed correctly. What follows is what is left.
|
||||
|
||||
Counts: **5 defects, 7 risks, 3 tests that cannot fail in the bug's
|
||||
direction, 7 rule findings, 2 nits.**
|
||||
|
||||
## Fix pass, 2026-09-07 evening
|
||||
|
||||
Every finding below carries a **Status** line. In summary: **13 fixed**
|
||||
(D1, D4, D5, R1, R5, R7, T1, T2, T3 and four of the rule findings and both
|
||||
nits), **6 moot or deferred** (D2, D3, R3, R4 and two rule findings, all
|
||||
of them in the phone-logging route that `06b8a1f` deleted or in files the
|
||||
devlog agent held open), and **2 not done on purpose** (R2, which waits on
|
||||
docs/LAYOUT.md's mask redesign, and R6, which needs Iris's own phone).
|
||||
|
||||
The commits are `2ec0fee` (D4), `7e79ec1` (D5), `551c013` (R1), `e10582a`
|
||||
(T1-T3), `ff1d6ea` (R5, R7) and `a6a100e` (the rename and the nits). Each
|
||||
fix that the rig can express carries a test, and each of those was
|
||||
confirmed by breaking its subject on purpose -- the break is recorded
|
||||
beside the assertion, so the next reader does not have to re-derive it.
|
||||
|
||||
---
|
||||
|
||||
## Defects
|
||||
|
||||
### D1 — the app's own log ring is drowned by the same day's per-frame `debug!` lines, so the route built to get Iris's logs to her carries almost none of them
|
||||
|
||||
`iris/android-app/src/lib.rs:132` installs the ring at `LevelFilter::Debug`,
|
||||
and `client-core/src/log_ring.rs:279` (`RingLogger::enabled`) returns
|
||||
`true` unconditionally by design, so **every `log::debug!` in the process
|
||||
lands in a 2000-line / 256 KiB ring**. In the same commit range that ring
|
||||
became the only way a line reaches Iris, three ungated per-frame `debug!`
|
||||
callsites are live:
|
||||
|
||||
- `iris/src/android/view.rs:446` and `:509` — two lines *per rendered frame*.
|
||||
- `iris/src/widget/list.rs:576` — `iris fling tick:`, one line per fling tick.
|
||||
- `iris/src/widget/text/mod.rs:81` — one per text shape (many per frame while rows compose).
|
||||
|
||||
**Failure scenario.** Iris flicks the transcript on a 120 Hz phone. That is
|
||||
~240–360 debug lines a second; the ring's 2000-line bound is exhausted in
|
||||
**under ten seconds**, so by the time she presses `Copy report` every
|
||||
`log::info!` about what she was actually investigating has been evicted.
|
||||
The uploader makes it worse: it sends at most the ring per 10 s wake
|
||||
(2000 lines ≈ 200 lines/s) against ~350 lines/s produced, so it also runs
|
||||
permanently behind and pushes tens of KB/s of frame spam over the tunnel.
|
||||
|
||||
Note that another agent has already built the right mechanism — the
|
||||
untracked `iris/src/diagnostics.rs` has `set_trace`/`trace_enabled`, a
|
||||
default-off gate, and its module doc states this exact problem in as many
|
||||
words. It gates `iris::input`/`iris::frame`; it does **not** gate the four
|
||||
callsites above.
|
||||
|
||||
*Fix*: put `List::tick_fling`'s line and `view.rs`'s two `render():` lines
|
||||
behind `iris::diagnostics::trace_enabled()` (the mechanism that already
|
||||
exists for exactly this), and/or record into the ring at `Info` while
|
||||
leaving `android_logger` at `Debug`.
|
||||
|
||||
**Status:** fixed in `992c472` (verified 2026-09-07: all four callsites, plus `sense.rs`'s drag-release samples line, now sit behind `iris::diagnostics::trace_enabled`, and `input_log_roundtrip` proves both directions).
|
||||
|
||||
### D2 — `POST /client-log` can make `ai-server` write an unbounded runtime log at an authenticated client's request
|
||||
|
||||
`server/src/routes.rs:1473` bounds the **line count** (500) and nothing
|
||||
else. The route sits inside the router that applies
|
||||
`DefaultBodyLimit::max(32 * 1024 * 1024)` at `server/src/routes.rs:179`
|
||||
(raised for phone photos), so one request may carry 500 lines of ~64 KiB
|
||||
each, and each is re-emitted verbatim into `tracing`. There is no
|
||||
per-message cap on the server, no rate limit, and the runtime log
|
||||
`ai-server` writes is the file Dev Updater tails and never rotates.
|
||||
`MAX_MESSAGE_BYTES` (4096) exists only in the *client*
|
||||
(`client-core/src/log_upload.rs:33`), i.e. the server trusts a value the
|
||||
attacker controls.
|
||||
|
||||
**Failure scenario.** A buggy client (a `log::debug!` in a loop is enough —
|
||||
see D1) or one holding a leaked bearer token posts 32 MiB every 10 s; the
|
||||
host's disk fills and every other component's log goes with it.
|
||||
|
||||
*Fix*: give the route its own `DefaultBodyLimit` (the attachments route at
|
||||
`:175` is the precedent for a per-route limit) and truncate each `message`
|
||||
server-side to the same 4096 bytes rather than assuming the client did.
|
||||
|
||||
**Status:** moot -- `POST /client-log` was deleted with the whole upload route (`06b8a1f`), the app hands its log to Dev Updater through an on-device ContentProvider instead. Nothing to bound.
|
||||
|
||||
### D3 — lines the ring drops before the uploader sends them vanish with nothing saying so
|
||||
|
||||
`LogRing::since` (`client-core/src/log_ring.rs:169`) filters `seq >= cursor`
|
||||
and silently returns fewer lines when eviction has passed the cursor;
|
||||
`LogUploader::flush_once` (`:94`) then advances to whatever came back.
|
||||
`dropped` is counted (`log_ring.rs:109`) and shown in the *local*
|
||||
diagnostics pane, but it is never put in the upload body, and
|
||||
`ClientLogBody` has no field for it.
|
||||
|
||||
**Failure scenario.** The tunnel is down for two minutes; the ring wraps.
|
||||
When it comes back, the server log jumps from `#812` to `#5106` with no
|
||||
line saying anything was lost. This is precisely the "unknown state
|
||||
sharing a value with the empty state" UI_RULES asks to design first, and
|
||||
the module doc for `dropped` claims it is "reported rather than inferred"
|
||||
— it is, but only on the half of the path nobody is reading.
|
||||
|
||||
*Fix*: carry `dropped` (or `firstSeq`) in the batch and have `client_log`
|
||||
emit one `warn!` when the sequence is not contiguous with the last batch
|
||||
from that `source`.
|
||||
|
||||
**Status:** moot -- `client-core/src/log_upload.rs` was deleted with the route (`06b8a1f`). Whatever the ContentProvider does about eviction is that design's question, not this one's.
|
||||
|
||||
### D4 — the input clock anchors on the first event's *own* time, so that event's historical samples are dated before the anchor: the ordering assert fires, and release silently collapses them onto one instant
|
||||
|
||||
`iris/src/android/view.rs:628` takes the anchor as
|
||||
`(Instant::now(), event.event_time_nanos())` from the first `MotionEvent`
|
||||
the view ever sees, and `at()` computes
|
||||
`anchor_at + (sample_time - anchor_nanos).max(0)`. Historical samples of
|
||||
that same event are by definition **earlier** than its own `event_time`.
|
||||
|
||||
**Failure scenario.** The first event this view receives is an
|
||||
`ACTION_MOVE` (the `DOWN` was delivered to another view, or the view was
|
||||
attached mid-gesture). Its historical samples are, say, 12 ms before
|
||||
`anchor_nanos`; `at()` clamps all of them to `anchor_at`, so the tracker
|
||||
receives three samples with identical timestamps, the Lsq2 fit is
|
||||
degenerate, and the flick reads 0 px/s. In a debug build the
|
||||
`debug_assert!(ht >= previous)` at `:653` fires first — but `previous`
|
||||
starts at `anchor_nanos` (`:651`), which is a value from a *different*
|
||||
event, so that assert is also the wrong comparison for the first sample of
|
||||
every later event.
|
||||
|
||||
*Fix*: anchor on the earliest sample of the first event
|
||||
(`historical_event_time_nanos(0)` when `history_size() > 0`, else
|
||||
`event_time`), and seed `previous` from the previous event's last sample
|
||||
rather than from the anchor.
|
||||
|
||||
**Status:** fixed in `2ec0fee`. The arithmetic moved into `sense::PointerClock`, which anchors at `now - (event_time - oldest_sample)` and carries the last sample seen *across* events, so the ordering assert compares against the previous event's last sample rather than the anchor. It lives in `sense` because `iris::android` is `cfg`'d out everywhere but the device: `sense_tests.rs`'s `the_first_events_batched_samples_are_dated_apart` reports `[0ns, 0ns, 0ns]` against the old anchoring.
|
||||
|
||||
### D5 — the "before" velocity quoted in four places is not what the reference script prints
|
||||
|
||||
`iris/benches/velocity_reference.py`, run today, prints **12250 px/s** for
|
||||
`flick-120hz.touch`'s average and **12500 px/s** for "press and one move
|
||||
frame". Four places say 11750 for both:
|
||||
|
||||
- `docs/RUST.md:900` (`flick-120hz.touch | 11750 px/s`)
|
||||
- `docs/RUST.md:905` (`press + one move frame | 11750 px/s`)
|
||||
- `docs/IRIS_TODO.md:1026`
|
||||
- `iris/transcript-fixture/tests/phone_screen.rs:55`
|
||||
|
||||
`iris/src/sense.rs:1406` has the correct 12250, so the two halves of the
|
||||
same change disagree. The file that carries the wrong number is the one
|
||||
that says "every number below is printed by `velocity_reference.py` … do
|
||||
not 'fix' one by running the Rust and copying what it said". One of the
|
||||
two rows also being 11750 for a completely different sample set is the
|
||||
tell.
|
||||
|
||||
*Fix*: replace 11750 with the script's own 12250 / 12500 in those four
|
||||
places, or say which run produced 11750.
|
||||
|
||||
**Status:** fixed in `7e79ec1`. All four places now say 12250 / 12500, the 1.30x ratio becomes 1.24x, and RUST.md records where 11750 half came from (196 px over a 16.68 ms **60 Hz** frame rather than the recording's own 16 ms -- which explains the flick row and not the other one, so that one was copied).
|
||||
|
||||
---
|
||||
|
||||
## Risks
|
||||
|
||||
### R1 — every new invariant guard is a `debug_assert!`, and the phone runs release
|
||||
|
||||
The five guards added today —
|
||||
`iris/src/widget/list.rs:1156` (a `List` must be inside a `.masked()`),
|
||||
`:1218` (`extents` holds only on-screen rows),
|
||||
`iris/src/android/view.rs:653` (historical sample ordering),
|
||||
`iris/src/sense.rs:1076` (`poly_fit_least_squares` sample count), and
|
||||
`iris/core/src/ui/painter.rs`'s doubled-`set_mask` check — are all
|
||||
`debug_assert!`. `docs/RUST.md` records that the bench APK **must** be
|
||||
installed as `release` on the emulator (the debug `libmain.so` is 325 MB
|
||||
and will not install) and Iris's phone gets release too. So none of these
|
||||
can fire on any build anybody actually runs; in release a `List` drawn
|
||||
without a mask silently paints over its surroundings again — the exact
|
||||
fault e922b73 was written to fix.
|
||||
|
||||
*Fix*: for the two that are cheap and once-per-draw (`is_masked`, the
|
||||
extents check), consider a plain `assert!` or a one-shot `log::error!`, so
|
||||
the guard survives into the build the defect was found in.
|
||||
|
||||
**Status:** fixed in `551c013`. `is_masked`, the `extents` check, `set_mask`'s doubled-call check, `Painter::glyphs`'s atlas generation and `List::fling`'s finiteness are `assert!`/`assert_eq!` now; `List::place`'s slot precondition, `poly_fit_least_squares`'s two, and `PointerClock::sample`'s ordering stay `debug_assert!` and say in a comment why. The layer-1 suites pass in `--release` as well as debug, which is what says the promoted ones do not fire on a real replayed flick.
|
||||
|
||||
### R2 — a straddling row is now invisible above the list and still tappable through the header
|
||||
|
||||
Masks are applied in the fragment shader
|
||||
(`iris/core/src/render/shader.wgsl:203`); the CPU hit path
|
||||
(`UiRenderState::resolved_region`, `iris/core/src/ui/render_state.rs:709`)
|
||||
does not consult `masks` at all. Before today the top of a straddling row
|
||||
was drawn over the header *and* hit-testable there; now it is clipped away
|
||||
but still hit-testable, which is worse — a tap on "Run benchmark" can land
|
||||
on an invisible link in the row behind it. `docs/LAYOUT.md:1012` ("Hit-
|
||||
testing applies the shape") is design, not code.
|
||||
|
||||
*Fix*: until LAYOUT.md's mask redesign lands, intersect a widget's hit
|
||||
region with its mask chain in `resolved_region`; the chain walk already
|
||||
exists on the GPU side.
|
||||
|
||||
**Status:** not done, deliberately -- docs/LAYOUT.md's mask redesign ("masks reference a drawn primitive instead of copying a shape", `1121d7c`) is where hit-testing gets the shape, and intersecting a chain in `resolved_region` now would be a second mechanism to unpick. Pointer left here rather than a fix.
|
||||
|
||||
### R3 — three copies of one wire contract, none of them linked
|
||||
|
||||
`client-core/src/log_upload.rs:28` (`MAX_LINES_PER_BATCH = 500`) and
|
||||
`server/src/routes.rs:1418` (`CLIENT_LOG_MAX_LINES = 500`) must agree, in
|
||||
different crates, with only a comment saying so; the body itself is built
|
||||
by hand with `serde_json::json!` on one side and parsed by a
|
||||
`#[serde(deny_unknown_fields)]` struct on the other. This project already
|
||||
has the mechanism for exactly this — `event-model`, a crate both `server`
|
||||
and `client-core` depend on precisely so "the app hand-mirroring it" stops
|
||||
happening (`server/Cargo.toml:16` says so).
|
||||
|
||||
**Failure scenario.** Somebody raises the client's batch to 1000. Every
|
||||
upload now returns 400, the uploader retries the *same* batch from the same
|
||||
cursor forever, and the only sign is one line in a diagnostics pane on a
|
||||
phone.
|
||||
|
||||
*Fix*: move `ClientLogLine`/`ClientLogBody` and the batch constant into a
|
||||
shared crate.
|
||||
|
||||
**Status:** moot -- both copies went with the route (`06b8a1f`). If a client/server contract comes back, `event-model` is still the answer.
|
||||
|
||||
### R4 — `build.rs` bakes in a CA it never asks Cargo to watch, and the bench build now has no rebuild trigger at all
|
||||
|
||||
`emit_log_config` (`iris/android-app/build.rs:92`) calls `read_pinned_ca()`
|
||||
but emits only `rerun-if-env-changed` for `AI_APP_LOG_HOST/_PORT/_TOKEN` —
|
||||
no `rerun-if-changed` for the CA *file*, and (because the bench build
|
||||
returns at `:65`, before the transcript path's declarations) no
|
||||
`rerun-if-env-changed=AI_APP_CA`/`XDG_CONFIG_HOME` either. Emitting any
|
||||
`rerun-if-*` directive turns off Cargo's default "rerun when anything in
|
||||
the package changes" heuristic, so the bench build lost the only trigger it
|
||||
had.
|
||||
|
||||
**Failure scenario.** `~/.config/ai-app` is wiped (AGENTS.md calls this the
|
||||
one-way door), `ai-server` mints a new CA, the APK is rebuilt — and
|
||||
`build.rs` does not re-run, so the APK still pins the dead CA and every
|
||||
upload fails with a TLS error nobody can attribute.
|
||||
|
||||
*Fix*: `println!("cargo:rerun-if-changed={}", ca_path.display())` inside
|
||||
`read_pinned_ca`, and move the `AI_APP_CA`/`XDG_CONFIG_HOME` declarations
|
||||
above the bench early-return.
|
||||
|
||||
**Status:** moot -- `iris/android-app/build.rs` was deleted (`06b8a1f`/`d8562d9`): the destination comes from the enrolment link now, so nothing is baked in at build time and there is nothing for Cargo to watch.
|
||||
|
||||
### R5 — desktop density is read once and never updated
|
||||
|
||||
`iris/src/default/mod.rs:254` reads `content_scale(window)` at startup and
|
||||
sets it on both `rsc.ui.text.density` and `render`. `WindowEvent::
|
||||
ScaleFactorChanged` is not handled, and `UiRenderer::resize` deliberately
|
||||
no longer consults `scale_factor`. Dragging the window to a monitor with a
|
||||
different scale leaves every `dp(...)` and every rasterised glyph at the
|
||||
old density — the same class of disagreement the commit removed elsewhere.
|
||||
It is invisible here (every display on this machine is 1.0), which is why
|
||||
it needs writing down.
|
||||
|
||||
**Status:** fixed in `ff1d6ea`. `WindowEvent::ScaleFactorChanged` re-reads `content_scale` -- through that function, so `IRIS_SCALE` still pins `--phone`'s density instead of following the monitor -- and `UiRenderState::set_density` marks the tree for a full redraw when the value actually changes, since `Text::shape` keys its cache on `(attrs, width, density)`.
|
||||
|
||||
### R6 — removing the bundled fonts removed the guard for a fault that was found on the phone, and the check was run on the desktop
|
||||
|
||||
`iris/core/src/primitive/text.rs`'s `register_bundled_fonts` existed
|
||||
because "bold spans on a real phone rendered as blank gaps of the correct
|
||||
advance width" — the deleted doc says so. Its removal is Iris's own call
|
||||
and is recorded properly in `docs/DECISIONS.md`, but the verification
|
||||
recorded there is "checked with CJK + emoji **on desktop**", which is the
|
||||
half that cannot fail: the fault was Android's font enumeration resolving
|
||||
a weight/style. `iris/transcript-ui/src/tool.rs:110`'s comment is honest
|
||||
that `CLOSED_MARK`/`OPEN_MARK`/`UP_MARK` (U+25B8/BE/B4) are now "a bet"
|
||||
that the platform monospace face has them — which is UI_RULES' "don't rely
|
||||
on characters the platform might not have", stated and then accepted.
|
||||
|
||||
*Fix*: before the next phone build, look at a bold run and the three
|
||||
chevrons on Iris's device specifically; the emulator's font set is not
|
||||
evidence for hers.
|
||||
|
||||
**Status:** not done here -- it is a *look at it on Iris's phone* item, and no build in this VM is evidence about her device's font set. Carried forward as the review said: before the next phone build, look at a bold run and at `CLOSED_MARK`/`OPEN_MARK`/`UP_MARK` (U+25B8/BE/B4) on her device specifically.
|
||||
|
||||
### R7 — the least-squares fit clamps a degenerate norm instead of detecting it
|
||||
|
||||
`iris/src/sense.rs:1105`: `1.0 / dot(...).sqrt().max(1e-6)`. Compose's
|
||||
`polyFitLeastSquares` treats `norm < 1e-6` as "vectors are linearly
|
||||
dependent, no solution" and bails; clamping instead produces a `q` row of
|
||||
zeros, a zero on `r`'s diagonal, and a `0/0` that the `is_finite` check at
|
||||
`:1059` happens to catch. It works, but it works by accident and the escape
|
||||
is not the one the source it is transcribed from takes.
|
||||
|
||||
**Status:** fixed in `ff1d6ea`. `poly_fit_least_squares` returns `Option` and bails at `DEGENERATE_NORM` (Compose's `0.000001f`) instead of clamping; `velocity()` answers 0 on `None`. `a_fit_through_linearly_dependent_points_has_no_solution` reports `Some([NaN, NaN, NaN])` with the clamp back in place.
|
||||
|
||||
---
|
||||
|
||||
## Tests that cannot fail in the direction the bug would go
|
||||
|
||||
### T1 — `iris/transcript-fixture/tests/phone_screen.rs:64` computes the expected fling duration with the calculator under test, and asserts it one-sidedly
|
||||
|
||||
`let expected = FlingCalculator::new(PHONE_SCALE).duration(velocity);` then
|
||||
`assert!(ran_for <= expected + 2 frames)`. This is the same
|
||||
"calculator compared with itself" shape the fling-spline commit
|
||||
(73f956f) identified and fixed elsewhere, and the direction it can fail in
|
||||
is "the fling ran too long" — never "the fling stopped dead", which is
|
||||
literally Iris's reported symptom. The companion
|
||||
`assert_ne!(before, after)` passes on one pixel of travel. A fling that
|
||||
settles on the first tick passes this test.
|
||||
|
||||
*Fix*: add a lower bound from `velocity_reference.py`'s number (a fling at
|
||||
-15250 px/s at density 2.55 must run ≥ ~1.4 s and travel ≥ ~6000 px), not
|
||||
from `FlingCalculator`.
|
||||
|
||||
**Status:** fixed in `e10582a`. Both bounds come from `fling_spline_reference.py`, which gained this case's own line (`density=2.55 v=15250.0: distance=11057.424px duration=2.0716s`), and travel is measured in pixels from a row's own on-screen extent (10527px measured). Scaling `tick_fling`'s elapsed by 1000 reports "stopped after 8ms"; scaling its delta by 0.01 reports "travelled 111px".
|
||||
|
||||
### T2 — `top_edge.rs:150` checks a row *count* on the leg where the culling bug appeared, and the box only on the other leg
|
||||
|
||||
`rows_that_have_left_the_viewport_are_not_drawn` asserts `rows.len() <= 24`
|
||||
on the outbound leg and the per-row `inside the box` predicate only on the
|
||||
return leg. The doc explains why (an unmeasured row must be drawn to be
|
||||
measured), which is correct — but it means the test's name is only true of
|
||||
half of it, and a regression that draws 20 rows in the wrong *place* on the
|
||||
outbound leg passes.
|
||||
|
||||
**Status:** fixed in `e10582a`. The first leg still cannot assert the box (an unmeasured row has to be drawn to be measured), so there is a third leg -- back again, every height known. Widening `intersects_viewport` downwards passes all 40 forward steps and fails at "back 6".
|
||||
|
||||
### T3 — `top_edge.rs:116` checks that a mask exists and where it is, not that it reaches anything
|
||||
|
||||
`the_list_is_clipped_to_its_own_box` asserts `active.mask != MaskIdx::NONE`
|
||||
and that the mask's region lies within the list's box. It never checks the
|
||||
row primitives actually reference that mask, so a broken `Mask::parent`
|
||||
chain — the thing d507ae4 introduced — would leave this green while a code
|
||||
fence inside a row drew unclipped again.
|
||||
|
||||
*Fix*: assert that a row primitive's mask chain contains the list's mask
|
||||
slot.
|
||||
|
||||
**Status:** fixed in `e10582a`. It walks every row primitive's mask chain and requires the list's own slot on it, and rejects a chain that loops. Forcing `Painter::set_mask`'s `parent` to `NONE` fails it with "clips to [Id(1)], a chain that never reaches the list's own mask Id(0)".
|
||||
|
||||
---
|
||||
|
||||
## Rules
|
||||
|
||||
- **`iris/src/widget/list.rs:576` is a second mechanism for per-frame
|
||||
instrumentation.** `iris::diagnostics::trace_enabled` exists for exactly
|
||||
"a default-off `debug!` in a hot path" and this line does not use it.
|
||||
(Cause of D1; the gate is in the untracked `diagnostics.rs`, so at the
|
||||
reviewed commit the line is simply ungated.)
|
||||
- **`server/src/routes.rs:1518` (`client_log_time`) duplicates
|
||||
`client-core/src/log_ring.rs:76` (`clock_time`)** — the same arithmetic
|
||||
written twice in two crates, with a comment noting they must agree. Same
|
||||
shared-crate answer as R3.
|
||||
- **`client-core/src/log_ring.rs:301`'s doc claims more than the code
|
||||
delivers**: "the caller is named in the error so it is findable" —
|
||||
`log::SetLoggerError` names nobody. `iris/android-app/src/app_log.rs:44`
|
||||
repeats the claim.
|
||||
- **Stale comment: `iris/src/android/view.rs:624`** cites
|
||||
`VelocityTracker::add_sample`'s debug assert; the method was renamed to
|
||||
`add_position` in the same commit range.
|
||||
- **`MOVE_CHAIN_LIMIT` now bounds two different chains** (move offsets and
|
||||
masks) under a name that says one, in both
|
||||
`iris/core/src/ui/render_state.rs:63` and `shader.wgsl:97`. The shader's
|
||||
comment already calls it "the bound on the parent walk"; the constant
|
||||
should say that too, or masks should get their own.
|
||||
- **`iris/src/sense.rs:1434`'s stated negative control is not reproducible
|
||||
as written.** "Reverting `velocity` to `total / span` fails exactly this
|
||||
one, the flick recording, and `phone_screen.rs`" — but `samples` now
|
||||
holds *positions*, so `total / span` over them gives 2750 for the steady
|
||||
drag too, and the commit message for the same change says "exactly seven
|
||||
tests". Two numbers for one experiment.
|
||||
- **`iris/android-app/src/bench_client.rs:393`'s `ime_visible` is right and
|
||||
its sibling one line up is not.** `set_bottom_inset(rsc,
|
||||
insets.bottom.max(insets.ime_bottom))` still infers "make room" from a
|
||||
`max`, so during the slide-in the composer is padded by the system-bar
|
||||
inset while `ime_visible` already says the keyboard is up. Harmless
|
||||
today; it is the same conflation the comment beside it warns about.
|
||||
|
||||
**Status of the rule findings, 2026-09-07 evening.**
|
||||
|
||||
- `list.rs:576`'s ungated per-frame line -- **fixed in `992c472`** with
|
||||
the rest of D1.
|
||||
- `routes.rs:1518`'s `client_log_time` duplicating `log_ring.rs`'s
|
||||
`clock_time` -- **moot**: the route was deleted (`06b8a1f`).
|
||||
- `log_ring.rs:301`'s "the caller is named in the error" -- **deferred to
|
||||
the devlog agent**; `client-core/src/log_ring.rs` is its file this pass,
|
||||
and `app_log.rs` no longer repeats the claim.
|
||||
- `view.rs:624`'s stale `VelocityTracker::add_sample` -- **fixed in
|
||||
`2ec0fee`**; the paragraph was rewritten for the anchoring change and
|
||||
now names `PointerClock` rather than a method that no longer exists.
|
||||
- `MOVE_CHAIN_LIMIT` naming two chains -- **fixed in `a6a100e`**: renamed
|
||||
to `PARENT_CHAIN_LIMIT` in `render_state.rs` and `shader.wgsl` at once
|
||||
(it had no other users), with the doc naming both chains it governs.
|
||||
- `sense.rs:1434`'s unreproducible negative control -- **fixed in
|
||||
`7e79ec1`**. Rerun with `velocity` reverted to `(newest - oldest) /
|
||||
span`: seven fail in `-p iris` (the flick recording, the accelerating
|
||||
flick, the horizon, the stopped finger, the minimum sample count, both
|
||||
`drag_gesture` flick tests) plus `phone_screen.rs`'s flick. RUST.md's
|
||||
"exactly seven" was right; the doc comment's "exactly this one, the
|
||||
flick recording, and `phone_screen.rs`" was not, and now says the same
|
||||
thing RUST.md does.
|
||||
- `bench_client.rs:393`'s `set_bottom_inset(.., max(..))` -- **deferred to
|
||||
the devlog agent**; `iris/android-app/**` was open under it this pass.
|
||||
|
||||
## Nits
|
||||
|
||||
- `iris/src/sense.rs:798` computes `self.velocity.velocity()` twice on a
|
||||
release when `info` logging is on (once for the outcome, once for the
|
||||
log line) — a full Lsq2 fit each.
|
||||
- `iris/transcript-ui/src/selection.rs:303` calls `ui.ui_mut().animate(id)`
|
||||
even when `fling()` bailed (`|v| <= 1.0`, or no anchor). Harmless — the
|
||||
first `tick` unregisters — but it registers an animation that is known
|
||||
not to exist.
|
||||
|
||||
---
|
||||
|
||||
**Status of the nits, both fixed in `a6a100e`.** `DragGesture`'s release
|
||||
computes `velocity()` once into a local both the outcome and the
|
||||
`iris drag release:` line read. `selection.rs`'s `animate(id)` is behind
|
||||
`is_scrolling()`, which is the same answer `List::fling` itself reached --
|
||||
and `phone_screen.rs`'s recorded flick still flings, which is the half
|
||||
that says the guard did not turn a working release off.
|
||||
|
||||
## Commits reviewed
|
||||
|
||||
```
|
||||
7e4e26a iris: resolve fontique's Android monospace generic family ourselves
|
||||
84a13e8 iris: a fling starts at Compose's velocity, which is a curve fit and not an average
|
||||
452c442 docs/RUST.md: queue -- logging landed; iris app enrolment ...
|
||||
238057a docs: the phone-logging decision, how to use it, and two build-apk traps
|
||||
896c93a iris: drop bundled Noto Sans, match Compose's platform-font fonts
|
||||
690161e docs: the transcript's edges were three faults, and what the rig found
|
||||
e922b73 iris: a transcript row is drawn if it overlaps the viewport, and clipped to it
|
||||
d507ae4 iris-core: masks nest instead of aborting, and a widget can ask to be drawn again
|
||||
9ed01e2 docs: phone report 2026-09-07 later -- overscroll, low initial fling velocity ...
|
||||
5be9f1b iris-android-app: keep the app's own log, put it in Copy report, upload it
|
||||
977bdb9 client-core: the app's own log ring, and POST /client-log to get it off a phone
|
||||
9cd1263 docs/RUST.md: queue -- APK size done, the embedded-fonts question left for Iris
|
||||
42af780 iris android-app: strip+LTO+cgu1+opt-level=s halve libmain.so, no feature trim needed
|
||||
4274b8b Merge remote-tracking branch 'origin/rustify' into worktree-agent-ace98b0bdaf33ffff
|
||||
73f956f iris: the fling curve was the identity function, and the keyboard was a targetSdk
|
||||
038f6a3 docs: the test rig's layers 1 and 2, with their commands and their limits
|
||||
1121d7c docs/LAYOUT.md: masks reference a drawn primitive instead of copying a shape ...
|
||||
232de0e iris: a phone-shaped desktop window, driven by the same touch recordings
|
||||
e430880 docs: phone report 2026-09-07, rows at the transcript's top edge culled early ...
|
||||
a999bd1 docs: masks with a shape (LAYOUT.md, decided 2026-09-07) and the orchestrator queue
|
||||
6840edf iris-android-app: the bench's fixture half comes from transcript-fixture
|
||||
3332201 iris: a headless in-process harness, and the bench fixture as a shared crate
|
||||
7f4ea7e docs/TODO.md: Compose app crash from Iris's phone log export, reversed AnnotatedString range
|
||||
591128e AGENTS.md: the phone app and the planned desktop app share widgets and styling
|
||||
```
|
||||
+237
-7835
File diff suppressed because it is too large.
Load diff
+2
-2
@@ -242,7 +242,7 @@ asking for one.
|
||||
|
||||
## The transcript's wiring
|
||||
|
||||
`iris/transcript-ui/src/lib.rs`, `build_tree`.
|
||||
`app-rust/src/ui/mod.rs`, `build_tree`.
|
||||
|
||||
The transcript registers the wheel **by hand rather than calling
|
||||
`LazySpan::scrollable()`**, and this is not an oversight. That helper also
|
||||
@@ -295,7 +295,7 @@ In `lazy_span.rs`, all of these fail if the corresponding piece is undone:
|
||||
- `a_registered_fling_is_driven_by_tick_animations_and_then_unregisters` —
|
||||
a fling that nothing registers never moves, whatever its velocity.
|
||||
|
||||
In `iris/transcript-fixture/tests/` (layer 1, no window or GPU):
|
||||
In `app-rust/tests/` (layer 1, no window or GPU):
|
||||
|
||||
- `top_edge.rs`'s `scrolling_past_the_first_row_settles_on_it` /
|
||||
`scrolling_past_the_last_row_settles_on_it` — both ends, no settling
|
||||
|
||||
+12
-268
@@ -1,24 +1,17 @@
|
||||
# How iris should render an unbounded number of images
|
||||
# How iris renders an unbounded number of images
|
||||
|
||||
## Status (2026-09-04)
|
||||
**Built 2026-09-04**, in `iris/core` and `iris/src/default/render.rs`.
|
||||
This file is the design and the measurements behind it; the deliberation
|
||||
that produced it -- the prior-art survey, the proposal and its review --
|
||||
was deleted on 2026-09-08, having been carried out. What is kept is why
|
||||
the old approach could not stay (it is the reason the current one looks
|
||||
as it does), the numbers, and what actually landed.
|
||||
|
||||
**Implemented**, on the `rustify` branch of `ai-app-2`, in `iris/core` and
|
||||
`iris/src/default/render.rs`. See "Implemented, 2026-09-04" at the bottom for
|
||||
what landed, what differs from the proposal below and why, and what was
|
||||
verified versus merely reasoned about. The short version: the binding array
|
||||
is gone, `request_device` asks for no features and no binding-array limits,
|
||||
and that is now proven on the emulator's software Vulkan
|
||||
(`rigs/gpu-probe`), not just read from the code. `RUST.md`'s blocking item
|
||||
is resolved.
|
||||
|
||||
Iris (the person) asked whether iris's (the library's) approach to
|
||||
"draw however many images happen to be on screen" — relevant here because a
|
||||
transcript can hold an unbounded number of attached screenshots — actually
|
||||
works on mobile, her recollection being that it does not. Checked rather
|
||||
than assumed, on 2026-09-04, on the `rustify` branch of `ai-app-2`. This
|
||||
file is that investigation and the resulting recommendation, written for a
|
||||
second agent to review before anything in iris's render core changes — no
|
||||
code has been written against this yet.
|
||||
Iris (the person) asked whether iris's (the library's) approach to "draw
|
||||
however many images happen to be on screen" -- relevant here because a
|
||||
transcript can hold an unbounded number of attached screenshots -- works
|
||||
on mobile, her recollection being that it does not. It did not, and this
|
||||
is what replaced it.
|
||||
|
||||
## The problem
|
||||
|
||||
@@ -96,255 +89,6 @@ share of the Android fleet lacks the feature iris's texture pipeline asks
|
||||
for unconditionally, and neither the emulator's failure nor the current
|
||||
official hardware baseline gives any reason to expect that to change soon.
|
||||
|
||||
## What growth already costs today, before any redesign
|
||||
|
||||
Checked directly in `core/src/render/mod.rs` and `core/src/render/texture.rs`,
|
||||
because "does this redesign make things worse" needs the current baseline
|
||||
first:
|
||||
|
||||
- The `RenderPipeline` (`UiRenderNode::new`) is created **once** and never
|
||||
rebuilt for any reason related to texture count — its bind group
|
||||
*layouts* declare fixed slot counts (`limits.max_textures`,
|
||||
`limits.max_samplers`) up front and that never changes at runtime. Growth
|
||||
was never at risk of recreating the pipeline, in the current design or
|
||||
any redesign discussed below.
|
||||
- What **does** get rebuilt: `UiRenderNode::update` calls
|
||||
`self.textures.update(&mut ui.textures)`, and if that reports any change,
|
||||
rebuilds `self.rsc_group` — one `BindGroup` whose entries are
|
||||
`BindingResource::TextureViewArray(&tex_manager.views())`, collected
|
||||
fresh over **every currently-live texture**, plus the sampler array and
|
||||
the mask buffer. This happens on every texture `Push`, `Set`, or `Free`
|
||||
— an image added anywhere in the whole app rebuilds one shared structure
|
||||
referencing every other image too.
|
||||
- The one path already excluded from this, on purpose, is a `Patch` —
|
||||
writing into an existing texture's pixels without changing which
|
||||
textures exist. The code says why directly
|
||||
(`core/src/render/texture.rs`, in `GpuTextures::update`): *"A patch
|
||||
changes texture contents, not the binding array, so it must not report
|
||||
`changed` — rebuilding the bind group per glyph is the cost this exists
|
||||
to avoid."* This is exactly the mechanism I1 built for the glyph atlas:
|
||||
growing an existing atlas page costs a `write_texture` into a sub-rect,
|
||||
nothing else.
|
||||
|
||||
So today, growth that stays inside an existing texture (glyphs added to an
|
||||
atlas page) is already free. Growth that adds a *new* texture — a new atlas
|
||||
page, or any standalone image — already rebuilds the one shared array
|
||||
regardless of how the array is populated, before any change discussed
|
||||
below. That existing cost is O(live texture count) in CPU work to collect
|
||||
the view list and in however expensive the driver finds a
|
||||
descriptor-set-sized-for-N-descriptors to be.
|
||||
|
||||
## Prior art, checked rather than assumed
|
||||
|
||||
Two independent projects were checked to see whether "atlas for images"
|
||||
is actually how this is normally done, rather than a guess:
|
||||
|
||||
- **egui_wgpu** (`crates/egui-wgpu/src/renderer.rs` in emilk/egui), the
|
||||
closest prior art to iris — an immediate-mode wgpu-backed UI library that
|
||||
ships on Android. It keeps a `HashMap<TextureId, Texture>` and gives
|
||||
**each texture its own ordinary `BindGroup`** — one texture, one sampler,
|
||||
no array, no descriptor indexing of any kind. Draw calls are batched by
|
||||
texture id and the bind group is switched between batches within the
|
||||
render pass.
|
||||
- **Vello** — the renderer Masonry (E1/E2's Linebender stack) draws
|
||||
through — hit the identical problem and wrote down why in their own
|
||||
roadmap document
|
||||
([github.com/linebender/vello/blob/main/doc/roadmap_2023.md](https://github.com/linebender/vello/blob/main/doc/roadmap_2023.md)):
|
||||
*"The number of images that may appear in a scene is not bounded, which
|
||||
is not a good fit for the basic descriptor binding model... Until then,
|
||||
we'll do a workaround of having a single atlas image containing all the
|
||||
images in the scene."* Their reason is broader than Android — WebGPU 1.0
|
||||
has no descriptor indexing at all — but it reaches the same conclusion
|
||||
for the same shape of problem: atlas, not a bigger bindless array.
|
||||
|
||||
**This is also a live hazard, not a solved one.** Vello's own changelog
|
||||
(Sparse Strips v0.2.0) lists a fix titled *"WebGL image-atlas allocation
|
||||
and growth on Mali-G52 GPUs, avoiding application-not-responding errors"*
|
||||
— an actual ANR, from atlas growth, on an actual mid-range Android GPU,
|
||||
in the renderer Masonry is built on. The same release added
|
||||
`AtlasSpaceDiagnostics`/`AtlasLayerDiagnostics` (per-layer free-space,
|
||||
utilization, fragmentation) because growth needed instrumenting in
|
||||
production, not because it turned out to be free.
|
||||
|
||||
## Recommendation (not yet implemented)
|
||||
|
||||
1. **Small, plentiful textures** — glyphs (already done, I1), thumbnails,
|
||||
downscaled attachment previews, icons — go through a shared atlas, the
|
||||
same technique as `core/src/render/atlas.rs` generalized beyond glyphs.
|
||||
Adding one to an existing page is a `Patch`, already free per the
|
||||
section above.
|
||||
2. **Large or one-off images** — a photo attachment opened at full
|
||||
resolution, anything that would fragment a shared page — get their
|
||||
**own ordinary, non-array bind group**, the egui_wgpu way. Creating one
|
||||
is O(1): it references only itself, and does not touch any other
|
||||
texture's binding, unlike today's shared array where every push
|
||||
rebuilds a structure listing everything.
|
||||
3. **Opening a new atlas page** is the one case that still resembles
|
||||
today's rebuild — infrequent (bounded by how many *pages* are needed,
|
||||
not by how many images have ever been attached) but not free, and
|
||||
Vello's Mali-G52 fix says this specifically deserves care: it should
|
||||
never be allowed to block a frame, and it is worth having the
|
||||
equivalent of Vello's atlas diagnostics before trusting it under load.
|
||||
4. **Net effect**: dropping `TEXTURE_BINDING_ARRAY`,
|
||||
`SAMPLED_TEXTURE_AND_STORAGE_BUFFER_ARRAY_NON_UNIFORM_INDEXING`, and
|
||||
`PARTIALLY_BOUND_BINDING_ARRAY` from iris's device request entirely.
|
||||
Every path above is plain Vulkan 1.0 / GLES-level texture sampling.
|
||||
This is also what fixes the emulator failure measured above, regardless
|
||||
of the unresolved wgpu-hal question: a device that never asks for the
|
||||
feature cannot be refused for lacking it.
|
||||
|
||||
## What this touches, and what is still open
|
||||
|
||||
Implementing this reworks iris's rendering core: the shader's binding
|
||||
group layout (`shader.wgsl`), `Textures` and `GpuTextures`
|
||||
(`core/src/primitive/texture.rs`, `core/src/render/texture.rs`), both
|
||||
texture-sampling primitives, and `core/src/ui/painter.rs`'s draw-call
|
||||
batching (today one draw call can reference any texture by index; the
|
||||
per-texture-bind-group path needs draws grouped by which bind group they
|
||||
use). Nothing has been started.
|
||||
|
||||
Open questions a reviewer should weigh in on:
|
||||
|
||||
- **The size threshold** between "goes in an atlas page" and "gets its own
|
||||
bind group." Too low and ordinary attachment thumbnails end up as
|
||||
one-off bind groups, losing the batching benefit the atlas exists for;
|
||||
too high and a page fragments on a handful of medium images.
|
||||
- **Eviction policy** for atlas pages once the working set does not fit —
|
||||
today's `GlyphAtlas` never evicts, because a font's glyph set is small
|
||||
and bounded; images are not. An LRU at the page level, or at the
|
||||
individual-image level within a page, has not been designed.
|
||||
- **Whether iris should keep any binding array at all**, even a small
|
||||
fixed one (say, capped at a few dozen slots) for atlas pages themselves,
|
||||
or whether every atlas page should also be its own ordinary bind group
|
||||
like standalone images — the array's only remaining justification would
|
||||
be avoiding a bind-group-per-draw-call switch cost that has not been
|
||||
measured on this project's actual target hardware.
|
||||
- **How this interacts with I2/E2's virtualised list** (I3): a
|
||||
bottom-anchored transcript composes only visible rows, so the live
|
||||
texture set should already be bounded by what is on screen rather than
|
||||
by the whole conversation — worth confirming that invariant holds before
|
||||
relying on it to keep atlas/bind-group churn small.
|
||||
|
||||
## Review, 2026-09-04
|
||||
|
||||
A second pass over the file above against the code, done before anything
|
||||
is implemented. Iris's worry going in: a bind group per texture means a
|
||||
draw call per image, and she wants this as efficient as it can be.
|
||||
|
||||
### What checked out
|
||||
|
||||
Every code reference above is accurate as of this commit: the 100,000 /
|
||||
1,000 limits, the one-time pipeline, the `rsc_group` rebuild on every
|
||||
`Push`/`Set`/`Free`, and the `Patch` exclusion. The device request that
|
||||
asks for the three features is `iris/src/default/render.rs:96`, which the
|
||||
text above does not name. egui-wgpu and Vello are described correctly.
|
||||
|
||||
### The emulator refusal is a wgpu-hal gap, now located
|
||||
|
||||
The file guessed "a likely instance-version negotiation gap." It is
|
||||
narrower than that and it is in wgpu-hal, not the emulator. wgpu-hal
|
||||
28.0.0 (`src/vulkan/adapter.rs:1618`) only queries
|
||||
`PhysicalDeviceDescriptorIndexingFeaturesEXT` **when the device advertises
|
||||
the `VK_EXT_descriptor_indexing` extension string**. A Vulkan 1.2+ driver
|
||||
that has descriptor indexing as core need not list the extension, and
|
||||
lavapipe at 1.3 evidently does not, so wgpu never asks and reports the
|
||||
features absent, which is why `ash` sees seven `true`s and wgpu sees none.
|
||||
The properties query beside it (line 1486) correctly accepts
|
||||
`device_api_version >= 1.2 || extension`; the features query does not.
|
||||
wgpu-hal 30.0.1 in the local registry has the same asymmetry (lines
|
||||
1872 and 2036). Worth an upstream issue, but not a reason to keep the
|
||||
design: on real phones the gate that matters is stricter still.
|
||||
|
||||
**wgpu's `TEXTURE_BINDING_ARRAY` needs six sub-features, not one**
|
||||
(`adapter.rs:160-177`): non-uniform indexing *and* update-after-bind for
|
||||
sampled images, storage images and storage buffers, all together, because
|
||||
wgpu marks every array-bearing descriptor set update-after-bind. So Arm's
|
||||
"the extension is supported on Valhall" is necessary but not sufficient;
|
||||
a driver with sampled-image indexing and without storage-buffer
|
||||
update-after-bind is refused too. That widens the excluded set beyond
|
||||
what the Arm quote suggests and strengthens the conclusion.
|
||||
|
||||
### A live bug in the current code, found on the way
|
||||
|
||||
`GpuTextures::update` (`core/src/render/texture.rs:33`) implements
|
||||
"a patch must not report changed" as `changed = false`, unconditionally,
|
||||
which also **cancels a `Push` earlier in the same batch**. That ordering is
|
||||
exactly what opening a new atlas page produces: `GlyphAtlas::allocate`
|
||||
pushes the page and `insert` patches it in the same frame, so the bind
|
||||
group is not rebuilt and the new page's view is not bound until some
|
||||
unrelated texture change happens to rebuild it. It is hidden today only
|
||||
because the masks path also sets `changed`. The fix is one line
|
||||
(`changed |= !matches!(update, Patch)` in spirit); it should go in with
|
||||
the redesign since that code is being replaced, and it is recorded here
|
||||
so it is not rediscovered.
|
||||
|
||||
### In-layer draw order is already undefined
|
||||
|
||||
Relevant to any batching redesign: `Primitives::apply_free`
|
||||
(`core/src/render/primitive.rs:147`) uses `swap_remove`, so the instance
|
||||
order within a layer is permuted whenever anything is freed. Overlap order
|
||||
inside one layer is therefore not something the renderer promises today;
|
||||
ordering is done with layers. That means grouping a layer's draws by
|
||||
texture, or drawing a layer's images after its rects and glyphs, loses
|
||||
nothing that currently exists. It should be written down as an invariant
|
||||
when the redesign lands, because the new code will depend on it.
|
||||
|
||||
### On "a draw call per image"
|
||||
|
||||
Two corrections to the worry. First, it is a draw per *distinct texture per
|
||||
layer*, not per image primitive: every glyph quad in a layer shares the
|
||||
atlas and stays one instanced draw, and a thumbnail atlas would do the same
|
||||
for previews. Second, the count is bounded by what is on screen, which I3's
|
||||
virtualised transcript already bounds, and a mobile GPU is not draw-call
|
||||
bound at tens of draws per frame; egui ships exactly this on Android. What
|
||||
does cost is per-frame *bind group creation* and per-frame *sorting*, and
|
||||
the current code already creates a `primitive_group` bind group every time
|
||||
a layer updates (`render/mod.rs:103`), so one more per new image is not a
|
||||
regression in kind.
|
||||
|
||||
### Recommended shape (proposal, for Iris to accept or change)
|
||||
|
||||
Aimed at the fewest moving parts that need no feature beyond Vulkan 1.0:
|
||||
|
||||
1. **Atlas pages become layers of one `texture_2d_array`**, not separate
|
||||
textures. Every page is already `PAGE`x`PAGE` RGBA8, which is the one
|
||||
constraint an array texture imposes. A layer index is an ordinary
|
||||
sampling operand in WGSL and needs no indexing feature, so `GLYPH`
|
||||
(and any future atlased-image primitive) carries a layer instead of a
|
||||
`view_idx` and all of a layer's text stays **one draw**. This answers
|
||||
the open question above about keeping a small binding array: no. Cost
|
||||
of opening a page: recreate the array with one more layer and
|
||||
`copy_texture_to_texture` the old ones, GPU-side, no readback; grow
|
||||
with headroom (double) so it is rare. wgpu's default
|
||||
`max_texture_array_layers` is 256, at 4 MB each, so the cap is memory
|
||||
rather than the API.
|
||||
2. **Every standalone image is its own texture with its own bind group**,
|
||||
and its instances live in a **separate per-layer instance list**, not
|
||||
the main one. Then the main instance buffer never contains an image,
|
||||
there is nothing to sort, no handle remapping beyond what
|
||||
`apply_free` already does, and each image is `draw(0..4, k..k+1)` with
|
||||
its bind group set first. Group 2's layout becomes `{atlas array,
|
||||
one image texture, sampler, masks}`; the main draw binds a 1x1 null
|
||||
image in the image slot, each image draw binds its own. One pipeline,
|
||||
one shader, one layout.
|
||||
3. **No thumbnail atlas in the first version.** With images on their own
|
||||
textures, the threshold and eviction questions above disappear: an
|
||||
image is freed when the row that owns its `TextureHandle` scrolls out.
|
||||
Add an image atlas only if a measured screen shows enough small images
|
||||
to matter, which a transcript rarely does.
|
||||
4. **Drop the three features and the two `max_binding_array_*` limits from
|
||||
`src/default/render.rs`**, and the `UiLimits` counts with them.
|
||||
5. **Sampling is `NonFiltering` today** (`render/mod.rs:290,299`), so a
|
||||
downscaled attachment will alias. Either request a filtering sampler
|
||||
for the image slot or downscale on the CPU before upload; decide when
|
||||
the image widget is touched, not as part of this.
|
||||
|
||||
What this costs against the file's original recommendation: `Textures`
|
||||
needs to know an image from a page (two kinds of handle, or a kind on
|
||||
`TextureHandle`), and `Primitives` gets a second instance list per layer.
|
||||
What it saves: the sort, the size threshold, the eviction policy, and any
|
||||
per-page bind group switch.
|
||||
|
||||
## Implemented, 2026-09-04
|
||||
|
||||
The shape above, built as proposed with one structural addition the proposal
|
||||
|
||||
@@ -248,7 +248,7 @@ impl FrameReport {
|
||||
// `phase_stats`'s slicing (`idx >= phase.start_index && idx <
|
||||
// end_index`) silently produces an empty or nonsensical slice for
|
||||
// a phase pushed out of order rather than surfacing the misuse
|
||||
// (docs/REVIEW-2026-09-06.md finding 5).
|
||||
// (review, 2026-09-06).
|
||||
debug_assert!(
|
||||
self.phases
|
||||
.last()
|
||||
|
||||
@@ -131,7 +131,7 @@ impl<'a> Painter<'a> {
|
||||
// and the second call silently *replacing* the first is a widget
|
||||
// drawn unclipped -- which reaches the screen and nothing says so.
|
||||
// Every build anybody runs here is release
|
||||
// (docs/REVIEW-2026-09-07.md's R1).
|
||||
// (review, 2026-09-07).
|
||||
assert!(
|
||||
self.own_mask == MaskIdx::NONE || self.mask != self.own_mask,
|
||||
"set_mask called twice while drawing one widget: the second would replace the first \
|
||||
|
||||
@@ -108,7 +108,7 @@ pub struct UiRenderState {
|
||||
/// Named for the walk rather than for one of its two subjects: it bounds
|
||||
/// the move-offset chain *and* the mask chain (`Mask::parent`, walked in
|
||||
/// the fragment stage), and `MOVE_CHAIN_LIMIT` said only the first
|
||||
/// (docs/REVIEW-2026-09-07.md).
|
||||
/// (review, 2026-09-07).
|
||||
pub const PARENT_CHAIN_LIMIT: usize = 64;
|
||||
|
||||
impl UiRenderState {
|
||||
@@ -252,7 +252,7 @@ impl UiRenderState {
|
||||
/// every `Len::dp` already resolved and every glyph already shaped
|
||||
/// (`Text::shape` keys its cache on `(attrs, width, density)`) belongs
|
||||
/// to the old one, and nothing else would ask for them again
|
||||
/// (docs/REVIEW-2026-09-07.md's R5).
|
||||
/// (review, 2026-09-07).
|
||||
pub fn set_density(&mut self, density: f32) {
|
||||
if density != self.density {
|
||||
self.resized = true;
|
||||
|
||||
@@ -371,7 +371,7 @@ impl<State: DefaultAppState> AppState for DefaultApp<State> {
|
||||
// the matching `Resized` separately. Before 2026-09-07 this
|
||||
// event was unhandled, so every `dp` and every rasterised
|
||||
// glyph stayed at the density the window opened on
|
||||
// (docs/REVIEW-2026-09-07.md's R5) -- invisible on this
|
||||
// (review, 2026-09-07) -- invisible on this
|
||||
// machine, where every display is 1.0.
|
||||
WindowEvent::ScaleFactorChanged { .. } => {
|
||||
let scale = content_scale(ui_state.window.as_ref());
|
||||
|
||||
+9
-9
@@ -832,7 +832,7 @@ pub fn log_input_event(action: &str, x: f32, y: f32, t_ms: u64, historical: &[(u
|
||||
/// samples sharing a timestamp make the Lsq2 fit degenerate, so the flick
|
||||
/// that produced them reads 0 px/s -- reachable whenever the first event a
|
||||
/// view sees is a `Move` (the `Down` went to another view, or the view was
|
||||
/// attached mid-gesture). Found by review, 2026-09-07 (docs/REVIEW-2026-09-07.md's D4).
|
||||
/// attached mid-gesture). Found by review, 2026-09-07.
|
||||
#[derive(Clone, Copy)]
|
||||
pub struct PointerClock {
|
||||
anchor_at: Instant,
|
||||
@@ -1557,7 +1557,7 @@ impl VelocityTracker {
|
||||
// gesture, a test) would make `velocity`'s reverse walk compute
|
||||
// negative ages and fit a curve through a shuffled x-axis --
|
||||
// masking the bug that produced it rather than surfacing it
|
||||
// (docs/REVIEW-2026-09-06.md finding 4).
|
||||
// (review, 2026-09-06 finding 4).
|
||||
debug_assert!(self.samples.back().is_none_or(|&(last, _)| at >= last));
|
||||
self.samples.push_back((at, position));
|
||||
while self.samples.len() > HISTORY_SIZE {
|
||||
@@ -1670,7 +1670,7 @@ impl VelocityTracker {
|
||||
/// points than coefficients; [`MIN_SAMPLE_SIZE`] makes that unreachable
|
||||
/// from the only caller, so the truncation is an assert instead of a
|
||||
/// branch that could never be exercised.
|
||||
// Both guards stay `debug_assert!` under docs/REVIEW-2026-09-07.md's R1:
|
||||
// Both guards stay `debug_assert!` under the 2026-09-07 review's rule:
|
||||
// they are preconditions of a fit run on every velocity query, and the two
|
||||
// callers between them already answer 0 below `MIN_SAMPLE_SIZE` and check
|
||||
// the result with `is_finite`, so a release build has a defined outcome
|
||||
@@ -1712,7 +1712,7 @@ fn poly_fit_least_squares(x: &[f32], y: &[f32]) -> Option<[f32; FIT_COEFFICIENTS
|
||||
// treats a norm this small as "the vectors are linearly dependent,
|
||||
// so there is no solution" and returns nothing; clamping instead
|
||||
// -- which this did until 2026-09-07
|
||||
// (docs/REVIEW-2026-09-07.md's R7) -- produces a `q` row of zeros,
|
||||
// (review, 2026-09-07's R7) -- produces a `q` row of zeros,
|
||||
// a zero on `r`'s diagonal and a 0/0 that only the caller's
|
||||
// `is_finite` check happened to catch. Working by accident, and
|
||||
// not what the source it is transcribed from does.
|
||||
@@ -1921,7 +1921,7 @@ impl FlingCalculator {
|
||||
pub fn distance(&self, velocity: f32) -> f32 {
|
||||
// See `ScrollController::fling`'s matching assertion -- a non-finite velocity
|
||||
// here silently produces a NaN distance rather than surfacing the
|
||||
// bug that produced it (docs/REVIEW-2026-09-06.md finding 3).
|
||||
// bug that produced it (review, 2026-09-06 finding 3).
|
||||
debug_assert!(velocity.is_finite());
|
||||
if velocity == 0.0 {
|
||||
return 0.0;
|
||||
@@ -2056,11 +2056,11 @@ impl Flinger {
|
||||
// straight through) would propagate silently into
|
||||
// `deceleration_for`'s `.ln()` -- the fling either never settles
|
||||
// or jumps to NaN positions with nothing on screen saying why
|
||||
// (docs/REVIEW-2026-09-06.md finding 3). A plain `assert!` rather
|
||||
// (review, 2026-09-06 finding 3). A plain `assert!` rather
|
||||
// than a `debug_assert!`: it is one comparison per *gesture*, and
|
||||
// every build anybody runs -- the emulator's and Iris's phone's --
|
||||
// is release, where a debug-only guard against silently wrong
|
||||
// output is no guard at all (docs/REVIEW-2026-09-07.md's R1).
|
||||
// output is no guard at all (review, 2026-09-07's R1).
|
||||
assert!(velocity_px_per_s.is_finite());
|
||||
assert!(density.is_finite() && density > 0.0);
|
||||
let max = MAX_FLING_VELOCITY_DP_S * density;
|
||||
@@ -2216,7 +2216,7 @@ mod velocity_tracker_tests {
|
||||
/// -- this one, the flick recording, the horizon, the stopped finger,
|
||||
/// the minimum sample count and both `drag_gesture` flick tests, plus
|
||||
/// `phone_screen.rs`'s flick -- and leaves everything else green. Rerun
|
||||
/// 2026-09-07 (docs/REVIEW-2026-09-07.md): this comment used to say
|
||||
/// 2026-09-07: this comment used to say
|
||||
/// "exactly this one, the flick recording, and `phone_screen.rs`",
|
||||
/// which disagreed with docs/RUST.md's count of the same experiment.
|
||||
/// Seven is what the run prints; RUST.md was right.
|
||||
@@ -2293,7 +2293,7 @@ mod velocity_tracker_tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// docs/REVIEW-2026-09-07.md's R7. Three samples at one instant --
|
||||
/// Found in review, 2026-09-07. Three samples at one instant --
|
||||
/// which the input clock produced on its own before 2ec0fee -- leave
|
||||
/// the second basis vector all zeros, and Compose calls that "linearly
|
||||
/// dependent, no solution" and returns nothing. Clamping the norm to
|
||||
|
||||
@@ -326,8 +326,8 @@ fn a_finger_drag_over_a_scroll_area_pans_it() {
|
||||
assert_eq!(pointer_input(&mut rsc).holder(), Some(scroll.id()));
|
||||
}
|
||||
|
||||
/// docs/REVIEW-2026-09-07.md's D4. The first `MotionEvent` a view sees can
|
||||
/// be a `Move` -- the `Down` went to another view, or the view was attached
|
||||
/// A defect found in review, 2026-09-07. The first `MotionEvent` a view
|
||||
/// sees can be a `Move` -- the `Down` went to another view, or the view was attached
|
||||
/// mid-gesture -- and its batched samples are older than its own
|
||||
/// timestamp. Anchoring on that timestamp clamped every one of them onto
|
||||
/// the anchor, so the tracker saw three samples at one instant, the Lsq2
|
||||
|
||||
@@ -1053,8 +1053,8 @@ impl LazySpan {
|
||||
// `prev_slot`/`next_slot`, which already check existence -- but
|
||||
// that invariant is enforced by convention across three call
|
||||
// sites, not by this function, which would otherwise fail with a
|
||||
// bare "index out of bounds" and no context (docs/
|
||||
// REVIEW-2026-09-06.md finding 2). `slot_widget`, called from
|
||||
// bare "index out of bounds" and no context (review,
|
||||
// 2026-09-06). `slot_widget`, called from
|
||||
// here, is what actually indexes/`.expect`s on it. Stays a
|
||||
// `debug_assert!` under R1's rule: this runs once per row placed
|
||||
// per frame, and its release failure is the `.expect` below rather
|
||||
@@ -1995,9 +1995,9 @@ mod tests {
|
||||
/// its sibling below ever asserts the *evicted* key's own bookkeeping
|
||||
/// is actually gone -- both replace row 4 with another row also keyed
|
||||
/// `4`, so `heights.remove(&old.key)` removing and re-inserting the
|
||||
/// same key would pass either test even if it did nothing (docs/
|
||||
/// REVIEW-2026-09-06.md finding 10; this is `Selection`'s finding 1
|
||||
/// class of bug -- a stale handle outliving what it points to --
|
||||
/// same key would pass either test even if it did nothing (review,
|
||||
/// 2026-09-06; the same class of bug -- a stale handle outliving what
|
||||
/// it points to --
|
||||
/// production-tested from `LazySpan`'s own side). Replacing with a
|
||||
/// **different** key is what actually exercises the removal.
|
||||
#[test]
|
||||
|
||||
Reference in new issue
Block a user