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
|
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
|
definition. Suspect the same cause for anything else tinted with a
|
||||||
background rect.
|
background rect.
|
||||||
- [ ] **A wrapped transcript row trips `reposition`'s debug assert.**
|
- [x] **A wrapped transcript row tripped `reposition`'s debug assert.**
|
||||||
*"widget ... is both moved by its parent's own layout (`mov`) and
|
Settled 2026-09-06 by giving the move slot one owner instead of two.
|
||||||
repositioned within it"*, raised from `List::place`. Repro: change
|
`mov` accumulates a delta on it, `reposition` overwrote it, and both
|
||||||
`.wrap(!verbatim)` to `.wrap(true)` in `transcript-ui/src/row.rs`'s
|
legitimately land on one widget in one frame: `List::place`'s
|
||||||
`build_block` and run `iris/run-headless.sh transcript --shot
|
Bottom-known branch offers a row a same-size box that has *moved*
|
||||||
/tmp/x.png -- -p transcript-ui`. Survives the `Rect` fix above and is
|
(`mov`), then corrects the placement inside it when the row's cached
|
||||||
not specific to any block kind -- it appears once the row is tall
|
height no longer matches what the row reports (`reposition`). The
|
||||||
enough. The shipping configuration does not reach it (verbatim blocks
|
slot now always means `move_applied + repositioned`
|
||||||
do not wrap) and the Android bench runs clean with assertions live,
|
(`ActiveData::repositioned`, `iris/core/src/ui/render_state.rs`), so
|
||||||
but it is a real disagreement about who owns a widget's move slot and
|
`reposition` adds the move rather than dropping it -- the assert is
|
||||||
should be settled before more of P1 leans on `List`.
|
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
|
- [ ] **Desktop colours are washed out: the winit surface is sRGB and
|
||||||
the shader writes the palette's bytes as linear.** Mocha Crust
|
the shader writes the palette's bytes as linear.** Mocha Crust
|
||||||
(17,17,27) is drawn as (73,73,91), measured off
|
(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
|
backend too -- `./run-headless.sh transcript --shot /tmp/x.png -- -p
|
||||||
transcript-ui --features iris/force-gles`.
|
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)
|
## Build (for the port)
|
||||||
|
|
||||||
Widgets `RUST.md`'s "The port, in order (decided 2026-09-05)" needs and
|
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
|
/// after a finger pan (2026-09-06). Reset to zero whenever the widget
|
||||||
/// is really redrawn, since `draw_inner` zeroes the slot then too.
|
/// is really redrawn, since `draw_inner` zeroes the slot then too.
|
||||||
pub move_applied: Vec2,
|
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,
|
move_slot,
|
||||||
own_mask,
|
own_mask,
|
||||||
move_applied: Vec2::ZERO,
|
move_applied: Vec2::ZERO,
|
||||||
|
repositioned: Vec2::ZERO,
|
||||||
};
|
};
|
||||||
|
|
||||||
// remove old children that weren't kept
|
// remove old children that weren't kept
|
||||||
@@ -442,12 +443,8 @@ impl UiRenderState {
|
|||||||
let Some(active) = self.active.get(&id) else {
|
let Some(active) = self.active.get(&id) else {
|
||||||
return;
|
return;
|
||||||
};
|
};
|
||||||
debug_assert!(
|
let move_applied = active.move_applied;
|
||||||
active.move_applied == Vec2::ZERO,
|
let repositioned = active.repositioned;
|
||||||
"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 from = active
|
let from = active
|
||||||
.size
|
.size
|
||||||
.to_uivec2(self.density)
|
.to_uivec2(self.density)
|
||||||
@@ -457,8 +454,27 @@ impl UiRenderState {
|
|||||||
let from_px = from.top_left().to_abs(self.output_size);
|
let from_px = from.top_left().to_abs(self.output_size);
|
||||||
let to_px = to.top_left().to_abs(self.output_size);
|
let to_px = to.top_left().to_abs(self.output_size);
|
||||||
let delta = to_px - from_px;
|
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);
|
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;
|
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
|
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