iris: the composer scrolls on a finger -- a dp cap worth zero, a stale mask slot, a hit box moved twice
Wrapping the composer's field in .scrollable().masked() needed three layout defects fixed first, each with a headless regression test that was confirmed to fail without its fix: - MaxSize/Sized reported a caller's declared dp length unresolved, and Span places a child from the abs/rel of what it reported, so dp(168) was worth zero: the bar got a slot of nothing the moment its content passed six lines and the Scroll inside measured its container at -63px (container=-63 content=415.8 amt=478.8 on the emulator). Len::fold_dp, used on the way out, plus a debug_assert in draw_inner that a reported Size carries no dp -- the rule is about every widget, not those two. - Masked allocated a fresh mask slot per draw, and draw_inner's unchanged-region fast path does not revisit descendants, so they kept clipping against a box the bar had moved away from: four live mask entries, none of them current, and the field drew nothing. ActiveData::own_mask, allocated once and rewritten in place. - mov updates active.region and accumulates the same delta on the move slot, and resolved_region added both, so a panned widget's own hit box sat at twice the pan -- the composer's field was untappable after a drag. ActiveData::move_applied. Scroll itself measured the right number by a misleading route; it is written against painter.px_size() now and still reports its content's size, since reporting the container makes the answer a function of itself. Verified on this checkout's emulator: swipe 540 1200 -> 540 1460 moved the field's Message box 31,1041..1048,1509 -> 31,1131..1048,1651 with its height unchanged at 468px. run-bench.sh polled logcat for a prefix copy_report also logs at startup, so it printed a report that had never been run. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
1 parent
d73db97629
commit
167862ca1b
14 files changed
+567
-39
No files matched your search
@@ -147,6 +147,29 @@ impl Len {
|
||||
}
|
||||
}
|
||||
|
||||
/// The same fold as [`Self::apply_rest`] but staying a `Len`, so
|
||||
/// `rest` survives: `dp` becomes physical pixels and every other
|
||||
/// component is left alone.
|
||||
///
|
||||
/// **A `Len` a widget *reports* must have been through this.** `dp` is
|
||||
/// an input unit -- a number the widget author wrote -- and the
|
||||
/// containers that consume a reported length read `abs`/`rel`/`rest`
|
||||
/// directly (`Span::draw`'s placement arithmetic, `Pad`'s addition),
|
||||
/// so a reported `dp` is silently worth zero. That is what made the
|
||||
/// composer's bar collapse to nothing the moment its content grew past
|
||||
/// `MaxSize`'s cap: the cap was `dp(168)` and was returned unresolved,
|
||||
/// so the bar was given a slot of 0 and the field inside it was panned
|
||||
/// out of a container measured at -63px. `UiRenderState::draw_inner`
|
||||
/// debug-asserts the invariant after every `Widget::draw`.
|
||||
pub fn fold_dp(&self, density: f32) -> Self {
|
||||
Self {
|
||||
abs: self.abs + self.dp * density,
|
||||
dp: 0.0,
|
||||
rel: self.rel,
|
||||
rest: self.rest,
|
||||
}
|
||||
}
|
||||
|
||||
pub fn abs(abs: impl UiNum) -> Self {
|
||||
Self {
|
||||
abs: abs.to_f32(),
|
||||
|
||||
@@ -1,4 +1,6 @@
|
||||
use crate::{LayerId, MaskIdx, MoveIdx, PrimitiveHandle, Size, TextureHandle, UiRegion, WidgetId};
|
||||
use crate::{
|
||||
LayerId, MaskIdx, MoveIdx, PrimitiveHandle, Size, TextureHandle, UiRegion, WidgetId, util::Vec2,
|
||||
};
|
||||
|
||||
/// important non rendering data for retained drawing
|
||||
#[derive(Debug)]
|
||||
@@ -9,7 +11,22 @@ pub struct ActiveData {
|
||||
pub textures: Vec<TextureHandle>,
|
||||
pub primitives: Vec<PrimitiveHandle>,
|
||||
pub children: Vec<WidgetId>,
|
||||
/// The mask this widget was drawn **under** (its parent's), not the
|
||||
/// one it set for itself -- see `own_mask` for that.
|
||||
pub mask: MaskIdx,
|
||||
/// The mask slot this widget allocated for *itself* with
|
||||
/// `Painter::set_mask`, or `MaskIdx::NONE`. Kept across redraws and
|
||||
/// rewritten in place, the way `move_slot` is: a `Masked` that pushed
|
||||
/// a fresh slot each draw left every already-drawn descendant --
|
||||
/// which `draw_inner`'s unchanged-region fast path does not revisit --
|
||||
/// clipping to the *old* slot's region, so a composer whose bar had
|
||||
/// since been placed at the bottom of the screen was still being
|
||||
/// clipped to a box at the top of it and drew nothing (measured
|
||||
/// 2026-09-06: four mask entries live, none of them the widget's
|
||||
/// current region). Its path out is the `undraw` branch of
|
||||
/// `UiRenderState::remove`, which drops the self-ownership ref taken
|
||||
/// when the slot was allocated.
|
||||
pub own_mask: MaskIdx,
|
||||
pub layer: LayerId,
|
||||
/// What `Widget::draw` returned the last time this widget was actually
|
||||
/// drawn -- read by a parent placing this widget again without
|
||||
@@ -21,4 +38,19 @@ pub struct ActiveData {
|
||||
/// so a retained child's `parent` link never goes stale). See
|
||||
/// LAYOUT.md section 2.
|
||||
pub move_slot: MoveIdx,
|
||||
/// How much of this widget's own `move_slot` delta is already folded
|
||||
/// into `region` above, in window pixels. The two mechanisms that
|
||||
/// write that slot disagree about this and cannot be told apart from
|
||||
/// the slot alone: `UiRenderState::mov` shifts `region` and the delta
|
||||
/// together (the *offered* region genuinely moved), while
|
||||
/// `Painter::reposition` writes only the delta (`region` stays the
|
||||
/// offered box and the delta says where inside it the content was
|
||||
/// placed). So anything that wants the widget's real position --
|
||||
/// `resolved_region`, and through it every hit test -- must subtract
|
||||
/// this from the chain sum. Without it a panned widget's own hit box
|
||||
/// sits at twice the pan while its descendants' are correct, which is
|
||||
/// how it went unnoticed: the composer's field became untappable
|
||||
/// 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,
|
||||
}
|
||||
@@ -13,6 +13,10 @@ pub struct Painter<'a> {
|
||||
pub(super) region: UiRegion,
|
||||
pub(super) mask: MaskIdx,
|
||||
pub(super) move_slot: MoveIdx,
|
||||
/// This widget's own mask slot, reused across redraws -- see
|
||||
/// `ActiveData::own_mask`. `MaskIdx::NONE` until `set_mask` is called
|
||||
/// for the first time in this widget's life.
|
||||
pub(super) own_mask: MaskIdx,
|
||||
pub(super) textures: Vec<TextureHandle>,
|
||||
pub(super) primitives: Vec<PrimitiveHandle>,
|
||||
pub(super) children: Vec<WidgetId>,
|
||||
@@ -48,12 +52,32 @@ impl<'a> Painter<'a> {
|
||||
self.primitive_at(primitive, region.within(&self.region));
|
||||
}
|
||||
|
||||
/// Clip everything this widget draws, itself and its descendants, to
|
||||
/// `region`. One per widget: a second call would need the two to be
|
||||
/// intersected, which nothing here does.
|
||||
///
|
||||
/// The slot is allocated once and **rewritten in place** on every
|
||||
/// later draw rather than pushed again, because a descendant whose own
|
||||
/// region did not change is not redrawn (`draw_inner`'s fast path) and
|
||||
/// so keeps pointing at whichever slot it was drawn under. See
|
||||
/// `ActiveData::own_mask` for what pushing a fresh one cost.
|
||||
pub fn set_mask(&mut self, region: UiRegion) {
|
||||
assert!(self.mask == MaskIdx::NONE);
|
||||
self.mask = self.rsc.ui_mut().masks.push(Mask {
|
||||
let mask = Mask {
|
||||
region,
|
||||
move_idx: self.move_slot,
|
||||
});
|
||||
};
|
||||
if self.own_mask == MaskIdx::NONE {
|
||||
let slot = self.rsc.ui_mut().masks.push(mask);
|
||||
// The one ref this widget holds on its own slot, so the slot
|
||||
// outlives any single frame's primitives; released in
|
||||
// `UiRenderState::remove`'s `undraw` branch.
|
||||
self.rsc.ui_mut().masks.push_ref(slot);
|
||||
self.own_mask = slot;
|
||||
} else {
|
||||
*self.rsc.ui_mut().masks.get_mut(self.own_mask) = mask;
|
||||
}
|
||||
self.mask = self.own_mask;
|
||||
}
|
||||
|
||||
/// Draws a widget within this widget's region, returning the size it
|
||||
@@ -86,6 +110,7 @@ impl<'a> Painter<'a> {
|
||||
self.mask,
|
||||
None,
|
||||
None,
|
||||
crate::render::MaskIdx::NONE,
|
||||
self.rsc,
|
||||
);
|
||||
self.state
|
||||
|
||||
@@ -156,6 +156,7 @@ impl UiRenderState {
|
||||
MaskIdx::NONE,
|
||||
None,
|
||||
None,
|
||||
MaskIdx::NONE,
|
||||
rsc,
|
||||
);
|
||||
}
|
||||
@@ -190,10 +191,12 @@ impl UiRenderState {
|
||||
mask: MaskIdx,
|
||||
old_children: Option<Vec<WidgetId>>,
|
||||
old_move_slot: Option<MoveIdx>,
|
||||
old_own_mask: MaskIdx,
|
||||
rsc: &mut dyn UiRsc,
|
||||
) {
|
||||
let mut old_children = old_children.unwrap_or_default();
|
||||
let mut old_move_slot = old_move_slot;
|
||||
let mut own_mask = old_own_mask;
|
||||
// Consumed here, not merely read: this call *is* the redraw the mark
|
||||
// asked for, and leaving the mark set is what stranded a widget's
|
||||
// primitives. `Painter::draw_twice` calls this twice for the same id
|
||||
@@ -207,6 +210,7 @@ impl UiRenderState {
|
||||
// the doubled `Compacted:` row in docs/bench/iris-phone-v2-2026-09-06.md.
|
||||
// The same shape reaches any dirty widget an ancestor redraws first.
|
||||
let dirty = rsc.widgets_mut().needs_redraw.remove(&id);
|
||||
let output_size = self.output_size;
|
||||
if let Some(active) = self.active.get_mut(&id)
|
||||
&& !dirty
|
||||
{
|
||||
@@ -235,6 +239,13 @@ impl UiRenderState {
|
||||
*r = r.outside(&from).within(®ion);
|
||||
self.region_mut_count += 1;
|
||||
}
|
||||
// Same bookkeeping `mov` does below and for the same
|
||||
// reason: `region` moves, this widget's own slot delta
|
||||
// does not, so the part of that delta `region` accounts
|
||||
// for grows by exactly this step. See
|
||||
// `ActiveData::move_applied`.
|
||||
active.move_applied +=
|
||||
region.top_left().to_abs(output_size) - from.top_left().to_abs(output_size);
|
||||
active.region = region;
|
||||
return;
|
||||
}
|
||||
@@ -242,6 +253,7 @@ impl UiRenderState {
|
||||
let active = self.remove(id, false, rsc).unwrap();
|
||||
old_children = active.children;
|
||||
old_move_slot = Some(active.move_slot);
|
||||
own_mask = active.own_mask;
|
||||
} else if dirty && self.active.contains_key(&id) {
|
||||
// Dirty and already drawn: none of the fast paths above may be
|
||||
// taken (the widget's own content changed, so its old primitives
|
||||
@@ -250,6 +262,7 @@ impl UiRenderState {
|
||||
let active = self.remove(id, false, rsc).unwrap();
|
||||
old_children = active.children;
|
||||
old_move_slot = Some(active.move_slot);
|
||||
own_mask = active.own_mask;
|
||||
}
|
||||
|
||||
// draw widget
|
||||
@@ -302,6 +315,7 @@ impl UiRenderState {
|
||||
region,
|
||||
mask,
|
||||
move_slot,
|
||||
own_mask,
|
||||
layer,
|
||||
id,
|
||||
textures: Vec::new(),
|
||||
@@ -313,6 +327,16 @@ impl UiRenderState {
|
||||
let mut widget = painter.rsc.widgets().get_dyn_dynamic(id);
|
||||
painter.state.draw_count += 1;
|
||||
let size = widget.draw(&mut painter);
|
||||
// A reported length is consumed by containers that read `abs`,
|
||||
// `rel` and `rest` straight off it (`Span`'s placement, `Pad`'s
|
||||
// addition), so an unresolved `dp` in one is silently worth zero
|
||||
// -- see `Len::fold_dp`, which is what a widget reporting a
|
||||
// caller-declared size has to put it through.
|
||||
debug_assert!(
|
||||
size.x.dp == 0.0 && size.y.dp == 0.0,
|
||||
"widget {id:?} reported an unresolved `dp` size ({size:?}); \
|
||||
report `Len::fold_dp(painter.density())` instead"
|
||||
);
|
||||
drop(widget);
|
||||
painter.state.draw_started.remove(&id);
|
||||
|
||||
@@ -322,6 +346,7 @@ impl UiRenderState {
|
||||
region,
|
||||
mask: _,
|
||||
move_slot,
|
||||
own_mask,
|
||||
textures,
|
||||
primitives,
|
||||
children,
|
||||
@@ -341,6 +366,8 @@ impl UiRenderState {
|
||||
layer,
|
||||
size,
|
||||
move_slot,
|
||||
own_mask,
|
||||
move_applied: Vec2::ZERO,
|
||||
};
|
||||
|
||||
// remove old children that weren't kept
|
||||
@@ -368,6 +395,7 @@ 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;
|
||||
active.move_applied += delta;
|
||||
let entry = rsc.ui_mut().move_offsets.get_mut(slot);
|
||||
entry.delta[0] += delta.x;
|
||||
entry.delta[1] += delta.y;
|
||||
@@ -402,6 +430,12 @@ 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 from = active
|
||||
.size
|
||||
.to_uivec2(self.density)
|
||||
@@ -443,6 +477,11 @@ impl UiRenderState {
|
||||
// the parent's own `ActiveData` may already be gone by the
|
||||
// time a deep descendant is retired (see LAYOUT.md
|
||||
// section 2's lifecycle note).
|
||||
if active.own_mask != MaskIdx::NONE {
|
||||
// The self-ownership ref `Painter::set_mask` took when
|
||||
// it allocated this widget's own mask slot.
|
||||
rsc.ui_mut().masks.remove(active.own_mask);
|
||||
}
|
||||
let parent_slot = rsc.ui_mut().move_offsets[active.move_slot.idx()].parent;
|
||||
rsc.ui_mut().move_offsets.remove(active.move_slot);
|
||||
if parent_slot != MoveOffset::NONE_PARENT {
|
||||
@@ -628,7 +667,12 @@ impl UiRenderState {
|
||||
/// section 2b.
|
||||
pub fn resolved_region(&self, id: &impl IdLike, rsc: &dyn UiRsc) -> Option<UiRegion> {
|
||||
let active = self.active.get(&id.id())?;
|
||||
let delta = self.resolve_move_chain(active.move_slot, rsc);
|
||||
// The chain sum is what the shader adds to this widget's
|
||||
// *primitives*, which were written before any of those moves.
|
||||
// `region`, unlike them, has already been shifted by whatever
|
||||
// part of this widget's own slot `mov` put there -- see
|
||||
// `ActiveData::move_applied`, which is exactly that part.
|
||||
let delta = self.resolve_move_chain(active.move_slot, rsc) - active.move_applied;
|
||||
Some(active.region.offset(UiVec2::abs(delta)))
|
||||
}
|
||||
|
||||
@@ -691,6 +735,7 @@ impl UiRenderState {
|
||||
active.mask,
|
||||
Some(active.children),
|
||||
Some(active.move_slot),
|
||||
active.own_mask,
|
||||
rsc,
|
||||
);
|
||||
// If this widget's own reported size changed, its parent's layout
|
||||
|
||||
Reference in new issue
Block a user