iris: a widget's move slot has one owner -- move_applied + repositioned
`mov` accumulates a delta onto the slot and `reposition` overwrote it, and both legitimately land on one widget in one frame: `List::place`'s Bottom-known branch offers a row a same-size box that has moved (`mov`), then corrects the placement inside it when the row's cached height no longer matches what the row reports (`reposition`). That is what a wrapped transcript row hit, and what the `move_applied == ZERO` debug assert was standing in for -- an assert against a case that happens is not a guarantee, it is a crash. The slot means `move_applied + repositioned` now, both halves recorded on `ActiveData`, so `reposition` adds the move rather than dropping it and stays idempotent. The assert it replaces is a `debug_assert_eq!` that the slot still holds that sum on entry -- i.e. that nothing but those two ever wrote it. Test: `a_widget_moved_by_its_parent_and_then_placed_inside_it_lands_at_the_placement`, which draws the child at the offered position (-100px) rather than the placement (100px) without the fix. Verified against the `.wrap(true)` repro from docs/IRIS_TODO.md (draws correctly, no panic) and an emulator bench run with assertions live.
This commit is contained in:
1 parent
3cb18ac5c2
commit
f5b88932b4
4 files changed
+155
-18
No files matched your search
+26
-11
@@ -591,17 +591,23 @@ agent ticks it here with the evidence.
|
||||
that. Fixed in `iris/src/widget/rect.rs`; the reason is written at the
|
||||
definition. Suspect the same cause for anything else tinted with a
|
||||
background rect.
|
||||
- [ ] **A wrapped transcript row trips `reposition`'s debug assert.**
|
||||
*"widget ... is both moved by its parent's own layout (`mov`) and
|
||||
repositioned within it"*, raised from `List::place`. Repro: change
|
||||
`.wrap(!verbatim)` to `.wrap(true)` in `transcript-ui/src/row.rs`'s
|
||||
`build_block` and run `iris/run-headless.sh transcript --shot
|
||||
/tmp/x.png -- -p transcript-ui`. Survives the `Rect` fix above and is
|
||||
not specific to any block kind -- it appears once the row is tall
|
||||
enough. The shipping configuration does not reach it (verbatim blocks
|
||||
do not wrap) and the Android bench runs clean with assertions live,
|
||||
but it is a real disagreement about who owns a widget's move slot and
|
||||
should be settled before more of P1 leans on `List`.
|
||||
- [x] **A wrapped transcript row tripped `reposition`'s debug assert.**
|
||||
Settled 2026-09-06 by giving the move slot one owner instead of two.
|
||||
`mov` accumulates a delta on it, `reposition` overwrote it, and both
|
||||
legitimately land on one widget in one frame: `List::place`'s
|
||||
Bottom-known branch offers a row a same-size box that has *moved*
|
||||
(`mov`), then corrects the placement inside it when the row's cached
|
||||
height no longer matches what the row reports (`reposition`). The
|
||||
slot now always means `move_applied + repositioned`
|
||||
(`ActiveData::repositioned`, `iris/core/src/ui/render_state.rs`), so
|
||||
`reposition` adds the move rather than dropping it -- the assert is
|
||||
gone and the arithmetic is right. Test:
|
||||
`a_widget_moved_by_its_parent_and_then_placed_inside_it_lands_at_the_placement`
|
||||
in `layout_tests.rs`, which lands the child at the *offered* position
|
||||
(-100px) instead of the placement (100px) without the fix, and a
|
||||
`debug_assert_eq!` in `reposition` that nothing but those two ever
|
||||
writes the slot. Verified with the `.wrap(true)` repro (draws, no
|
||||
panic) and an emulator bench run with assertions live.
|
||||
- [ ] **Desktop colours are washed out: the winit surface is sRGB and
|
||||
the shader writes the palette's bytes as linear.** Mocha Crust
|
||||
(17,17,27) is drawn as (73,73,91), measured off
|
||||
@@ -624,6 +630,15 @@ agent ticks it here with the evidence.
|
||||
backend too -- `./run-headless.sh transcript --shot /tmp/x.png -- -p
|
||||
transcript-ui --features iris/force-gles`.
|
||||
|
||||
- [ ] **The bench report pane draws over the transcript rows instead of
|
||||
replacing them.** Visible on the emulator for the first time now that
|
||||
glyphs render there (`/tmp/emu-final.png`, 2026-09-06): after a bench
|
||||
run the report's lines and the transcript's occupy the same rows in the
|
||||
top third of the screen, both legible, neither on top. Pre-existing --
|
||||
the same overlap is in a screenshot taken before the move-slot fix -- so
|
||||
it is its own item, most likely the report pane not masking or not
|
||||
claiming its region.
|
||||
|
||||
## Build (for the port)
|
||||
|
||||
Widgets `RUST.md`'s "The port, in order (decided 2026-09-05)" needs and
|
||||
|
||||
@@ -53,4 +53,17 @@ pub struct ActiveData {
|
||||
/// after a finger pan (2026-09-06). Reset to zero whenever the widget
|
||||
/// is really redrawn, since `draw_inner` zeroes the slot then too.
|
||||
pub move_applied: Vec2,
|
||||
/// The offset the last `Painter::reposition` placed this widget's
|
||||
/// content at *within* `region`, in window pixels. The move slot has
|
||||
/// exactly one owner and one meaning:
|
||||
/// `move_offsets[move_slot] == move_applied + repositioned`. `mov`
|
||||
/// adds to the first, `reposition` overwrites the second (it
|
||||
/// recomputes `from` afresh every call, so repeating it must land on
|
||||
/// the same answer rather than drifting), and both then rewrite the
|
||||
/// slot from the sum -- which is what lets a parent both move a child
|
||||
/// with its own layout and place it inside that moved region in one
|
||||
/// frame. `List::place`'s Bottom-known branch does exactly that once a
|
||||
/// row's blocks wrap. Reset to zero on a real redraw, with
|
||||
/// `move_applied` and the slot itself.
|
||||
pub repositioned: Vec2,
|
||||
}
|
||||
@@ -380,6 +380,7 @@ impl UiRenderState {
|
||||
move_slot,
|
||||
own_mask,
|
||||
move_applied: Vec2::ZERO,
|
||||
repositioned: Vec2::ZERO,
|
||||
};
|
||||
|
||||
// remove old children that weren't kept
|
||||
@@ -442,12 +443,8 @@ impl UiRenderState {
|
||||
let Some(active) = self.active.get(&id) else {
|
||||
return;
|
||||
};
|
||||
debug_assert!(
|
||||
active.move_applied == Vec2::ZERO,
|
||||
"widget {id:?} is both moved by its parent's own layout (`mov`) and repositioned \
|
||||
within it; the two write the same slot with different conventions -- see \
|
||||
`ActiveData::move_applied`"
|
||||
);
|
||||
let move_applied = active.move_applied;
|
||||
let repositioned = active.repositioned;
|
||||
let from = active
|
||||
.size
|
||||
.to_uivec2(self.density)
|
||||
@@ -457,8 +454,27 @@ impl UiRenderState {
|
||||
let from_px = from.top_left().to_abs(self.output_size);
|
||||
let to_px = to.top_left().to_abs(self.output_size);
|
||||
let delta = to_px - from_px;
|
||||
// Not `delta` alone: a parent may have `mov`ed this widget to a
|
||||
// region that itself moved earlier in the same frame, and that
|
||||
// part of the slot is `move_applied`'s, not this call's. Writing
|
||||
// `delta` on its own dropped it and put the content back at the
|
||||
// pre-move position. `from` is computed against `active.region`,
|
||||
// which `mov` already updated, so `delta` is purely the placement
|
||||
// inside the region and the two summands never overlap.
|
||||
let entry = rsc.ui_mut().move_offsets.get_mut(slot);
|
||||
entry.delta = [delta.x, delta.y];
|
||||
debug_assert_eq!(
|
||||
entry.delta,
|
||||
[
|
||||
move_applied.x + repositioned.x,
|
||||
move_applied.y + repositioned.y
|
||||
],
|
||||
"widget {id:?}'s move slot was written by something other than `mov`/`reposition`; \
|
||||
the slot is theirs and means `move_applied + repositioned` -- see `ActiveData`"
|
||||
);
|
||||
entry.delta = [move_applied.x + delta.x, move_applied.y + delta.y];
|
||||
if let Some(active) = self.active.get_mut(&id) {
|
||||
active.repositioned = delta;
|
||||
}
|
||||
self.mov_count += 1;
|
||||
}
|
||||
|
||||
|
||||
@@ -562,3 +562,96 @@ fn a_size_independent_widget_moved_by_its_parent_has_the_hit_box_it_is_drawn_at(
|
||||
after.top_left
|
||||
);
|
||||
}
|
||||
|
||||
/// A parent that both `mov`s a child (its own layout moved the box it
|
||||
/// offers) and `reposition`s it inside that box in the same frame -- what
|
||||
/// `List::place`'s Bottom-known branch does once a row's cached height
|
||||
/// stops matching what the row reports, which is reachable as soon as a
|
||||
/// transcript row's blocks wrap (docs/IRIS_TODO.md's "Found by P1a").
|
||||
struct MoveThenPlace {
|
||||
inner: StrongWidget,
|
||||
/// Where the child is *offered* a (constant-size) box, moved between
|
||||
/// frames by the test.
|
||||
offer_top: f32,
|
||||
/// Where the child is then placed within this widget's own region.
|
||||
place_top: f32,
|
||||
}
|
||||
|
||||
impl Widget for MoveThenPlace {
|
||||
fn draw(&mut self, painter: &mut Painter) -> Size {
|
||||
let offer = UiRegion::new(
|
||||
UiSpan::FULL,
|
||||
UiSpan::new(
|
||||
UiScalar::abs(self.offer_top),
|
||||
UiScalar::abs(self.offer_top + 40.0),
|
||||
),
|
||||
);
|
||||
painter.widget_within(&self.inner, offer);
|
||||
let place = UiRegion::new(
|
||||
UiSpan::FULL,
|
||||
UiSpan::new(
|
||||
UiScalar::abs(self.place_top),
|
||||
UiScalar::abs(self.place_top + 40.0),
|
||||
),
|
||||
);
|
||||
painter.reposition(&self.inner, place);
|
||||
Size::default()
|
||||
}
|
||||
}
|
||||
|
||||
/// `mov` accumulates a delta onto a widget's move slot and `reposition`
|
||||
/// overwrites it, and both can legitimately land on one widget in one
|
||||
/// frame (see `MoveThenPlace`). `reposition` used to write its own delta
|
||||
/// alone, which dropped the move and put the child back at the position
|
||||
/// the offered box had *before* it moved; a `debug_assert!` that
|
||||
/// `move_applied` was zero hid that behind a panic instead of fixing it.
|
||||
/// The slot has one owner and one meaning now --
|
||||
/// `move_applied + repositioned` -- so the child stays where it was
|
||||
/// placed however its offered box moves. Fails at the offer's position
|
||||
/// (200) rather than the placement's (100) without that.
|
||||
#[test]
|
||||
fn a_widget_moved_by_its_parent_and_then_placed_inside_it_lands_at_the_placement() {
|
||||
let mut rsc = TestRsc {
|
||||
ui: UiData::default(),
|
||||
};
|
||||
let rect = rsc.ui.widgets.add_strong(Rect::new(UiColor::WHITE));
|
||||
let child = rsc.ui.widgets.add_strong(Sized {
|
||||
inner: rect.any(),
|
||||
x: None,
|
||||
y: Some(Len::abs(40.0)),
|
||||
});
|
||||
let child_w = child.weak();
|
||||
let parent = rsc.ui.widgets.add_strong(MoveThenPlace {
|
||||
inner: child.any(),
|
||||
offer_top: 0.0,
|
||||
place_top: 100.0,
|
||||
});
|
||||
let parent_w = parent.weak();
|
||||
let root = parent.any();
|
||||
|
||||
let mut render = UiRenderState::new();
|
||||
render.resize((200.0, 400.0));
|
||||
render.update(&root, &mut rsc);
|
||||
let before = render.window_region(&child_w, &rsc).unwrap();
|
||||
assert!(
|
||||
(before.top_left.y - 100.0).abs() < 0.01,
|
||||
"the child should be drawn where it was placed, not where it was offered: {before:?}"
|
||||
);
|
||||
|
||||
// Move the offered box without changing its size (the `mov` fast path)
|
||||
// and place the child at the same spot as before. Marking the parent
|
||||
// dirty is what a real container's own content change does; the child
|
||||
// itself is untouched, which is the case `mov` exists for.
|
||||
{
|
||||
let parent = rsc.ui.widgets.get_mut(&parent_w).unwrap();
|
||||
parent.offer_top = 200.0;
|
||||
}
|
||||
rsc.ui.widgets.needs_redraw.insert(parent_w.id());
|
||||
render.update(&root, &mut rsc);
|
||||
let after = render.window_region(&child_w, &rsc).unwrap();
|
||||
assert!(
|
||||
(after.top_left.y - 100.0).abs() < 0.01,
|
||||
"the placement did not change, so neither should the child: before={before:?} \
|
||||
after={after:?}"
|
||||
);
|
||||
}
|
||||
Reference in new issue
Block a user