Record Iris review fixes, validation, and consolidated checkout
This commit is contained in:
iris-ai committed
1 parent
67584cedda
commit
f3c9ab8106
3 files changed
+102
-52
No files matched your search
+35
-40
@@ -6,52 +6,47 @@ in `docs/LAYOUT_LOG.md`.
|
||||
|
||||
## The Iris layout repair is submitted
|
||||
|
||||
**Iris PR #19** (`layout/one-ask` at `cadfba0`, 123 commits over upstream
|
||||
`main` at `ca2b4b2`) is open and waiting on Bryan. It **replaces #18**, which
|
||||
is closed and whose branch `split/18-position-chain` stays as the reviewed
|
||||
baseline the newer work was measured against. Bryan decided on 2026-09-19
|
||||
that the experiment replaces it, because it is more performant where it
|
||||
matters.
|
||||
**Iris PR #19** (`layout/one-ask`) replaces closed #18. The current tip is
|
||||
`84dad21`: `add6774` fixes collapsed-share placement, retained mask ownership,
|
||||
and a redraw-on-reparent defect; `84dad21` removes repeated work in the test
|
||||
harness. `docs/LAYOUT_LOG.md` records the review and its regression coverage.
|
||||
The core design remains sound; naming and round-to-nearest are unchanged.
|
||||
|
||||
The checkout is `/home/bob/repos/iris-layout-experiment`, on branch
|
||||
`wip/hint-first`, which is the same commit as `layout/one-ask`. Both are
|
||||
pushed to `origin` (`iris-ai/iris`). What the branch does and what it leaves
|
||||
undone is in the PR description; the settled protocol is in `docs/LAYOUT.md`
|
||||
under "Frames, decided boxes and padding".
|
||||
There is one Iris Git repository, stored with this checkout's submodule:
|
||||
|
||||
Verification at `cadfba0`, all run in this checkout:
|
||||
- `/home/bob/repos/iris` is the active `layout/one-ask` worktree.
|
||||
- `/home/bob/repos/ai-app-2/iris` stays on `main` at the app's `32f6ad8` pin.
|
||||
It shares Git storage with the active worktree, rather than being a second
|
||||
development repository. Keep it until the app's integration is ready.
|
||||
- The standalone `iris-layout-experiment` clone is retired. Superseded fork
|
||||
branches are removed; unique experiment tips and the #18 measurement
|
||||
baseline are preserved under `archive/2026-09-19/` tags. A complete bundle
|
||||
is also under `~/.local/state/iris/archive/`.
|
||||
|
||||
| check | result |
|
||||
| --- | --- |
|
||||
| `cargo fmt --all --check`, clippy `-D warnings`, with and without `layout-diagnostics` | clean |
|
||||
| `cargo test --workspace` | 123 suite, 20 core, 11 generated, green |
|
||||
| `cargo test --release --test generated` | 11/11 |
|
||||
| shrinker, 400 seeds, depth 5, sixteen cases | agree, 60 s |
|
||||
| 1000 seeds at depth 6 | agree, 177 s |
|
||||
| 2000-seed depth-4 scan, sixteen cases | agree, 295 s (82,203 widgets) |
|
||||
| cold dump of 400 trees at depth 5, diffed against `f6242aa` | identical, 34,488 widget boxes |
|
||||
| counters, seeds 1 and 13 at depth 8 | unchanged from `f6242aa` (the table is in `docs/LAYOUT.md`) |
|
||||
| five renders, the `tabs` replay, a live resize against a cold render | all byte-identical to `a888717` |
|
||||
|
||||
The renderer was Vulkan on the host's RX 7900 XT: `src/default/render.rs`
|
||||
asks for `Backends::PRIMARY` and `.expect()`s the adapter, so a PNG at all
|
||||
means it was not the software fallback.
|
||||
Verification of the fixes: format and workspace clippy with `-D warnings`,
|
||||
workspace tests with and without `layout-diagnostics` (128 suite tests,
|
||||
20 core tests, 21 with diagnostics, and 11 generated tests), and the release fast oracle pass.
|
||||
The 400-seed depth-5 shrinker, 1,000-seed depth-6 run, and 2,000-seed
|
||||
breadth scan all pass. The cold dump covers 34,488 boxes; 650 change because
|
||||
of the collapsed-share correction. With only that correction withheld,
|
||||
the remaining fixes reproduce the baseline dump exactly.
|
||||
The headless `tabs` example and the exact collapsed-share repro were rendered
|
||||
and inspected; the latter places the blue tail at x=220..400. The machine's
|
||||
Vulkan device is the RX 7900 XT through RADV native context.
|
||||
|
||||
## What is next, in order
|
||||
|
||||
1. **The two defects the review of #19 found**, written up with their
|
||||
repros and fixes in `docs/LAYOUT_LOG.md`: a span misplaces the slot after
|
||||
a collapsed `leftover` child, and a reused child keeps the mask its
|
||||
parent replaced. Both are live at `cadfba0` and neither has a test.
|
||||
2. **Bryan's review of #19.** Fixes to it are unreviewed code: repeat the
|
||||
`pre-submit-review` passes over whatever each round changes, and re-run
|
||||
the three fuzzers and the dump diff for anything that touches `Span`,
|
||||
`Painter` or `render_state`.
|
||||
3. **The app's Iris pin**, once #19 lands. It is still on the old submodule
|
||||
commit, and the branch changes API the app uses: `SizeRule` beside a
|
||||
widget rather than a wrapper, alignment as a widget property, `rest`
|
||||
renamed to `leftover`, `OrthoSize` gone.
|
||||
4. **Round-to-nearest**, CPU and shader together as one verified change.
|
||||
1. **Bryan's review of #19.** Fixes are themselves unreviewed code: repeat
|
||||
`pre-submit-review` over each round's changes, and re-run the three
|
||||
fuzzers and dump diff for edits to `Span`, `Painter`, or `render_state`.
|
||||
The ordinary oracle does not replace absolute geometry and
|
||||
retained-primitive expectations.
|
||||
2. **Integrate the app's Iris capabilities before changing its pin.**
|
||||
`32f6ad8` has 45 commits not reachable from the review branch; shared UI
|
||||
ownership, richer masks, Android support, and app-side performance work
|
||||
must survive the integration. Then migrate `SizeRule` beside widgets,
|
||||
alignment properties, `rest` to `leftover`, and removal of `OrthoSize`.
|
||||
3. **Round-to-nearest**, CPU and shader together as one verified change.
|
||||
Bryan approved it on 2026-09-17 and neither half has landed; the
|
||||
derivation, the form to use and what to re-check are in `docs/LAYOUT.md`
|
||||
under "Rendering the grid (pending)".
|
||||
|
||||
+10
-3
@@ -302,6 +302,11 @@ physical pixels, unchanged.
|
||||
|
||||
## Masks
|
||||
|
||||
This section and "UI ownership and frame access" describe the app's pinned
|
||||
Iris implementation. The upstream #19 review branch still has single
|
||||
rectangular masks and separate `UiData`/`UiRenderState`; adopting it requires
|
||||
integrating the app-side capabilities as well as migrating layout calls.
|
||||
|
||||
A `Mask` references a rectangle primitive and its parent mask. Nested masks
|
||||
multiply coverage. Plain `.masked()` creates an undrawn rectangle at the
|
||||
widget's region; `.masked_by(shape)` draws the shape behind the content and
|
||||
@@ -489,8 +494,8 @@ of its parent's frame and a local redraw walked back up the parent chain in
|
||||
defensively on every use.
|
||||
- A retained drawing is reusable only when its `Holds` contains the new box
|
||||
on both axes, its parent and region-node choice match, it is on the layer it
|
||||
was drawn on, and the widget is clean. A valid ordinary subtree moves by
|
||||
recursive remap; a region node moves by one entry. A container that draws a
|
||||
was drawn on, its inherited mask matches, and the widget is clean. A valid
|
||||
ordinary subtree moves by recursive remap; a region node moves by one entry. A container that draws a
|
||||
child to learn its size uses `Painter::child_layer_at`, the layer the child
|
||||
will actually occupy.
|
||||
- An answer's validity and its final drawing's validity are independent. A
|
||||
@@ -506,7 +511,9 @@ of its parent's frame and a local redraw walked back up the parent chain in
|
||||
draw it. Dirty size dependencies settle deepest-first.
|
||||
- A widget that creates a mask clips to and reports its box. Its own mask and
|
||||
its inherited mask are distinct retained state: the former says which mask
|
||||
a move rewrites, while a local redraw receives the latter.
|
||||
a move rewrites, while a local redraw receives the latter. The owner retains
|
||||
one mask reference and reuses its slot across redraws, independently of
|
||||
primitive references; dropping the mask or undrawing the owner releases it.
|
||||
- A span's leftover/no-leftover boundary is a strict structural decision, not
|
||||
a tolerance. Derive the boundary through the inverse of the expression that
|
||||
places children. A cap may not contain `leftover`, because feeding the
|
||||
|
||||
+57
-9
@@ -6,12 +6,60 @@ nothing here is rediscovered. Each entry says who found it and when.
|
||||
it (settled design, the measurement method) belongs in `docs/LAYOUT.md`, and
|
||||
the current plan is in `docs/HANDOFF.md`.
|
||||
|
||||
## What the review of #19 found (2026-09-19)
|
||||
## Follow-up implementation review (2026-09-19)
|
||||
|
||||
Read at `cadfba0` in the `iris/` submodule of this checkout, which is the
|
||||
same commit as `layout/one-ask`. `cargo fmt --all --check`, clippy
|
||||
`-D warnings` and the 123-test suite are clean there, so both defects below
|
||||
are live behaviour rather than anything the tests say.
|
||||
The ask/place split, window-unit frames, exact validity preimages, and
|
||||
bottom-up dirty settling implement the settled design. Keep this approach.
|
||||
It does not guarantee one body call per widget: an unhinted descendant that
|
||||
reports leftover weight still needs a room ask and a slot ask. The explicit
|
||||
measurement redesign remains deferred until an app screen justifies it.
|
||||
|
||||
The fixes below are on `layout/one-ask` in `/home/bob/repos/iris`:
|
||||
|
||||
- A collapsed share advances both span cursors. The regression covers one
|
||||
and two collapsed children in all four directions.
|
||||
- A masking widget owns a mask reference and reclaims its existing slot on
|
||||
redraw. Primitives retain their own references. Removing the mask,
|
||||
undrawing its owner, freeing the widget, and replacing the root release
|
||||
ownership; a changed inherited mask rejects drawing reuse. Tests check
|
||||
actual primitive mask indices, movement with and without a region node,
|
||||
child draw counts, clip removal/addition, and empty-mask slot reuse.
|
||||
- **A further handover defect:** `draw_inner` saved the old parent only
|
||||
after a redraw replaced `ActiveData`. The old parent therefore kept the
|
||||
child in its list and could undraw the subtree after its new parent drew
|
||||
it. Capture the old parent before replacing the record. A branch-switch
|
||||
regression reproduces disappearing content when its new parent owns a
|
||||
region node, and also checks the ordinary reuse path.
|
||||
|
||||
The mask and handover tests fail on the reviewed code and pass with the
|
||||
fixes. The mask fix preserves child reuse rather than redrawing descendants
|
||||
on every mask repaint. No naming sweep or rounding-policy change is included.
|
||||
|
||||
Validation: workspace tests with and without diagnostics, the release fast
|
||||
oracle, and all three prescribed seed scans pass. Comparing 34,488 cold
|
||||
boxes against `cadfba0` finds 650 changes; withholding just the collapsed-slot
|
||||
fix reproduces the baseline exactly. This is an expected geometry correction,
|
||||
not a cost-only change whose dump should remain identical. The tabs example
|
||||
and an exact 400 px collapsed-share fixture were rendered and inspected.
|
||||
|
||||
Two test-harness savings leave the random stream and coverage unchanged:
|
||||
`generated` constructs one plan per seed for its sixteen scenarios, and the
|
||||
warm/cold comparison constructs its diagnostic ancestry lookup only after
|
||||
finding a mismatch. No overall speedup is claimed; no deep profile was run.
|
||||
|
||||
**Integration is larger than an API rename.** The app's pinned `32f6ad8`
|
||||
has 45 commits not reachable from this review branch. In particular, the
|
||||
app's nested/shape masks and shared `Ui` ownership are absent here: #19's
|
||||
mask is still a single rectangle and `set_mask` rejects nested masks. Keep
|
||||
the app pin until those existing capabilities have been integrated. The
|
||||
"Masks" and "UI ownership" sections of `LAYOUT.md` describe the app-side
|
||||
implementation, not everything already present on the upstream review branch.
|
||||
|
||||
## Original review of #19 at `cadfba0` (2026-09-19)
|
||||
|
||||
The original review read `cadfba0`, then the tip of `layout/one-ask`.
|
||||
`cargo fmt --all --check`, clippy `-D warnings` and the 123-test suite were
|
||||
clean there. These are the original failures, fixed by the follow-up above.
|
||||
|
||||
### A span misplaces the slot after a collapsed `leftover` child
|
||||
|
||||
@@ -80,11 +128,11 @@ per-widget mask slot kept across redraws, the way `UiRenderState::move_slot`
|
||||
already keeps a move entry, so the index is stable and `reposition` goes on
|
||||
updating the one the descendants name.
|
||||
|
||||
### Neither path has a test
|
||||
### Regression coverage
|
||||
|
||||
The suite passes with and without both fixes. A span case belongs in
|
||||
`tests/cases/layout.rs` and a mask-reuse case in `tests/cases/retained.rs`
|
||||
when they land.
|
||||
Both paths now have regressions in `tests/cases/layout.rs` and
|
||||
`tests/cases/retained.rs`; the follow-up above records the additional cases.
|
||||
The descriptions above preserve the original failure at `cadfba0`.
|
||||
|
||||
### Clarity, in the order worth doing
|
||||
|
||||
|
||||
Reference in new issue
Block a user