diff --git a/core/src/ui/active.rs b/core/src/ui/active.rs index f8fb0e5..7b84148 100644 --- a/core/src/ui/active.rs +++ b/core/src/ui/active.rs @@ -13,6 +13,10 @@ pub struct ActiveData { /// it is a fraction of a slot's box, and the same fraction of a box that /// has since changed is a different number of pixels. pub px: Vec2, + /// The pixel size of the box its parent first asked about it in, before + /// knowing what it came to. `px` may be a box derived from that answer, + /// and a size measured there is only the same answer asked again. + pub offered_px: Vec2, pub parent: Option, /// How far down the tree it was drawn, the root being 1. Carried down a /// draw rather than worked out by walking up, so it is right for every diff --git a/core/src/ui/painter.rs b/core/src/ui/painter.rs index e3e9c1b..afdaaa2 100644 --- a/core/src/ui/painter.rs +++ b/core/src/ui/painter.rs @@ -21,6 +21,9 @@ pub struct Painter<'a> { pub(super) textures: Vec, pub(super) primitives: Vec, pub(super) children: Vec, + /// The children asked about so far, so the first box each was asked + /// about is the one recorded as its offer. + pub(super) offered: Vec, /// The children whose size this widget read while drawing. pub(super) size_deps: Vec, /// Offered pixel axes which can affect the size this draw reports. @@ -141,6 +144,7 @@ impl<'a> Painter<'a> { None, self.rsc, ); + self.offer(id.id(), region); DrawResult { child: id, painter: self, @@ -182,6 +186,8 @@ impl<'a> Painter<'a> { axis: Axis, region: UiRegion, ) -> Option { + let region = region.within(&self.region); + self.offer(child.id(), region); if let Some(hint) = self.size_hint(child, axis) { return Some(hint); } @@ -189,12 +195,12 @@ impl<'a> Painter<'a> { .map(|size| size.axis(axis)) } + /// `region` in this widget's own coordinates. fn retained_size( &mut self, child: &StrongWidget, region: UiRegion, ) -> Option { - let region = region.within(&self.region); let (size, box_inputs, output_inputs) = self.state .retained_size(child.id(), region, self.move_idx, self.rsc.widgets())?; @@ -204,6 +210,20 @@ impl<'a> Painter<'a> { Some(size) } + /// Records the box a child was first asked about in this draw. Any later + /// box this draw gives it was decided knowing its answer, so a size the + /// child measures there is not an answer to this widget's question. + fn offer(&mut self, child: WidgetId, region: UiRegion) { + if self.offered.contains(&child) { + return; + } + self.offered.push(child); + let px = self.state.px_of(self.move_idx, region); + if let Some(active) = self.state.active.get_mut(&child) { + active.offered_px = px; + } + } + /// Depends on a length the child gave without being drawn. A hint is /// context-free, so this depends on the child but on no pixel axis. fn depend_on_hint(&mut self, child: &StrongWidget) { @@ -309,6 +329,13 @@ impl<'a> Painter<'a> { /// [`Self::px_size`] when the other axis cannot affect the reported size. pub fn px_len(&mut self, axis: Axis) -> f32 { self.size_box_inputs[axis as usize] = true; + self.px_len_for_draw(axis) + } + + /// One axis of this widget's box in pixels, for a draw whose reported + /// size does not follow from it -- a clamp or a position. Nothing records + /// the read, so a size that does depend on it would go stale. + pub fn px_len_for_draw(&self, axis: Axis) -> f32 { let region = self.state.moves.resolve(self.move_idx, self.region); region .size() diff --git a/core/src/ui/render_state.rs b/core/src/ui/render_state.rs index 630b82c..faa5094 100644 --- a/core/src/ui/render_state.rs +++ b/core/src/ui/render_state.rs @@ -203,7 +203,7 @@ impl UiRenderState { parent_move: MoveIdx, slotted: bool, mask: MaskIdx, - old_children: Option>, + mut old: Option, rsc: &mut dyn UiRsc, ) -> Size { #[cfg(feature = "layout-diagnostics")] @@ -211,14 +211,12 @@ impl UiRenderState { diag::bump(Counter::DrawRequests); diag::draw_request(id, parent, region, self.px_of(parent_move, region), slotted); } - let mut old_children = old_children.unwrap_or_default(); if self.active.contains_key(&id) { if let Some(size) = self.try_reuse(id, region, depth, parent_move, rsc) { return size; } // if not, then maintain resize and track old children to remove unneeded - let active = self.remove(id, false, rsc).unwrap(); - old_children = active.children; + old = self.remove(id, false, rsc); } // draw widget @@ -232,6 +230,12 @@ impl UiRenderState { } }; let px = self.px_of(move_idx, local); + // Drawn again in a box its parent already decided: the offer is the + // one recorded when the parent first asked, not this box. + let (old_children, offered_px) = match old { + Some(old) => (old.children, old.offered_px), + None => (Vec::new(), px), + }; rsc.widgets_mut().needs_redraw.remove(&id); self.draw_started.insert(id); @@ -244,6 +248,7 @@ impl UiRenderState { textures: Vec::new(), primitives: Vec::new(), children: Vec::new(), + offered: Vec::new(), size_deps: Vec::new(), depth, size_box_inputs: [false; 2], @@ -271,6 +276,7 @@ impl UiRenderState { textures, primitives, children, + offered: _, size_deps, size_box_inputs, size_output_inputs, @@ -292,6 +298,7 @@ impl UiRenderState { region, size, px, + offered_px, parent, depth, textures, @@ -347,7 +354,7 @@ impl UiRenderState { } /// The pixel size of a region held in `slot`'s coordinates. - fn px_of(&self, slot: MoveIdx, region: UiRegion) -> Vec2 { + pub(super) fn px_of(&self, slot: MoveIdx, region: UiRegion) -> Vec2 { self.moves .resolve(slot, region) .size() @@ -736,16 +743,25 @@ impl UiRenderState { // A widget can only answer whether its size changed by drawing in the // box its parent chose. If that box changed in pixels, its retained // placement is stale and the highest size reader must choose the new - // box first. Otherwise the widget can draw locally, and its readers - // only matter if the returned size actually changed. + // box first. The same holds when the box was decided from the + // widget's own answer: measuring there again can only repeat it, + // whatever the content now says. Otherwise the widget can draw + // locally, and its readers only matter if the returned size actually + // changed. let box_changed = self.active.get(&id).is_some_and(|active| { let px = self.px_of(active.parent_move, active.region); AXES.into_iter() .any(|axis| pixel_len_changed(active.px.axis(axis), px.axis(axis))) }); - if box_changed && let Some(top) = self.mark_readers(id, rsc) { + let top = match box_changed { + true => self.top_reader(id), + false => None, + } + .or_else(|| self.derived_box_reader(id)); + if let Some(top) = top { #[cfg(feature = "layout-diagnostics")] diag::bump(Counter::EagerReaderRedraws); + self.mark_below(id, top, rsc); self.redraw(top, rsc); rsc.widgets_mut().needs_redraw.remove(&id); return; @@ -772,7 +788,7 @@ impl UiRenderState { active.parent_move, active.move_idx != active.parent_move, active.mask, - Some(active.children), + Some(active), rsc, ); @@ -795,24 +811,56 @@ impl UiRenderState { } } - /// The furthest ancestor that read this widget's size, directly or through - /// widgets that did the same, marking everything below it on the way. - fn mark_readers(&self, id: WidgetId, rsc: &mut dyn UiRsc) -> Option { + /// The highest reader up the chain that gave what it read a box other + /// than the one it asked in, on an axis this widget's size reads. Above + /// it every box is a constraint rather than an answer. It is the highest + /// and not the nearest because a pass-through hands a derived box down + /// unchanged. + fn derived_box_reader(&self, id: WidgetId) -> Option { + let reads = self.active.get(&id)?.size_box_inputs; let mut top = None; - let mut at = id; - while let Some(active) = self.active.get(&at) - && let Some(parent) = active.parent - && self - .active - .get(&parent) - .is_some_and(|p| p.size_deps.contains(&at)) - { - rsc.widgets_mut().needs_redraw.insert(at); - top = Some(parent); - at = parent; + for (active, parent) in self.reader_chain(id) { + let px = self.px_of(active.parent_move, active.region); + if AXES.into_iter().zip(reads).any(|(axis, r)| { + r && pixel_len_changed(active.offered_px.axis(axis), px.axis(axis)) + }) { + top = Some(parent); + } } top } + + /// The furthest ancestor that read this widget's size, directly or through + /// widgets that did the same. + fn top_reader(&self, id: WidgetId) -> Option { + self.reader_chain(id).last().map(|(_, parent)| parent) + } + + /// Each widget from `id` upward whose parent read its size, with that + /// parent. + fn reader_chain(&self, id: WidgetId) -> impl Iterator { + let mut at = Some(id); + std::iter::from_fn(move || { + let active = self.active.get(&at?)?; + let parent = active.parent?; + let read = self.active.get(&parent)?.size_deps.contains(&active.id); + at = read.then_some(parent); + read.then_some((active, parent)) + }) + } + + /// Marks everything from `id` up to, and not including, `top`, so that + /// drawing `top` draws each of them rather than reusing it. + fn mark_below(&self, id: WidgetId, top: WidgetId, rsc: &mut dyn UiRsc) { + let mut at = id; + while at != top { + rsc.widgets_mut().needs_redraw.insert(at); + let Some(parent) = self.active.get(&at).and_then(|active| active.parent) else { + return; + }; + at = parent; + } + } } impl Default for UiRenderState { diff --git a/src/widget/position/scroll.rs b/src/widget/position/scroll.rs index 22c6e71..8404d29 100644 --- a/src/widget/position/scroll.rs +++ b/src/widget/position/scroll.rs @@ -12,7 +12,8 @@ pub struct Scroll { impl Widget for Scroll { fn draw(&mut self, painter: &mut Painter) -> Size { let output_len = painter.output_len(self.axis); - let container_len = UiScalar::px(painter.px_len(self.axis)); + // Its size is its content's, whatever box that is scrolled within. + let container_len = UiScalar::px(painter.px_len_for_draw(self.axis)); // Draw in the whole container only when its scrolling-axis length is // not already known, then place it at the scrolled offset. let known_len = painter.known_len(&self.inner, self.axis, UiRegion::FULL); diff --git a/tests/generated.rs b/tests/generated.rs index df0cb6f..a2d63a0 100644 --- a/tests/generated.rs +++ b/tests/generated.rs @@ -442,25 +442,11 @@ fn adding_and_removing_span_children_lands_where_growing_it_that_way_would() { } } -/// Reproduces a divergence that predates the position chain: laying a tree out -/// again does not always land where growing it cold does. -/// -/// Every one seen so far is a wrapping text on a span's *own* axis, where the -/// two draws do not agree. The span measures the child in the whole box, the -/// child shapes to that width and reports the width it used, the span then -/// places it in exactly that width -- which is a length change, so the child -/// shapes again, and its longest line is shorter than the box it was just -/// given. Each pass narrows it, so where the tree ends up depends on how many -/// passes it has had, and a warm tree has had a different number from a cold -/// one. Layout is supposed to be a function of the state alone. -/// -/// A span whose axis is not the wrap axis is stable, which is every real -/// column of text, and why nothing else has run into this. -/// -/// 7 of these 90 diverge on `db1751f`, before the chain; 30 do with it, since -/// a placed child reaches the second shaping more often. Both numbers are the -/// same defect, and it wants fixing where the two draws meet -- LAYOUT.md ยง4 -- -/// rather than anywhere in the chain. +/// The same property over a hundred seeds and every scenario. What it has +/// found so far was never where the trees disagreed: a text measured in a box +/// it was not going to get, and a widget re-measured in a box its own answer +/// had decided. `tests/shrink.rs` is how a seed from here becomes a tree +/// small enough to read. #[test] #[ignore = "a hundred seeds, rather than the seven the others check"] fn a_long_run_of_seeds_agrees() { diff --git a/tests/unsettled.rs b/tests/unsettled.rs index 3971e2f..08bcd88 100644 --- a/tests/unsettled.rs +++ b/tests/unsettled.rs @@ -1,10 +1,9 @@ -//! The smallest tree that lays out differently on a second frame, shrunk from -//! a 402-widget one `tests/shrink.rs` grew. Both of these fail: a cold frame -//! leaves a wrapping text shaped at a width it was measured in rather than the -//! one it was given, and a repaint is what puts it right. So the warm-against- -//! cold oracle in `generated.rs` has been comparing against a tree that had -//! not settled, and some of what it called a warm defect is the cold side -//! being wrong. +//! The smallest trees that laid out differently warm than cold, each shrunk +//! by `tests/shrink.rs` from hundreds of widgets. The first two are a cold +//! frame that had not settled: a wrapping text shaped at a width it was +//! measured in rather than the one it was given. The rest are a widget +//! measured again in a box its own answer had decided, where the old answer +//! is a fixed point whatever the content now says. use iris::harness::Harness; use iris::prelude::*; @@ -213,3 +212,87 @@ fn swapping_two_children_lands_where_growing_them_that_way_does() { } assert!(wrong.is_empty(), "{}", wrong.join("\n")); } + +/// Eight widgets, shrunk from 80. The scroll decides how wide to make its +/// content from what the content says, and hands that box down through a +/// pass-through; the span under it was placed once, in that box, so nothing +/// at its own edge says the box was its own answer. +fn plant_scrolled(h: &mut Harness, swapped: bool) -> (Vec, [WeakWidget; 2]) { + let words = "Wrapping shapes one source into as many lines as the box leaves room for,"; + let text = wtext(words).size(16).wrap(true).add(&mut h.rsc); + let filler = rect(Color::RED).add(&mut h.rsc); + let mut inner_children: Vec = + vec![text.add_strong(&mut h.rsc), filler.add_strong(&mut h.rsc)]; + if swapped { + inner_children.rotate_left(1); + } + let inner = Span { + children: inner_children, + dir: Dir::RIGHT, + gap: 0.0, + } + .add(&mut h.rsc); + let block = rect(Color::RED).add(&mut h.rsc); + let fixed = SetSize { + inner: block.add_strong(&mut h.rsc), + x: Some(Len::px(87.0)), + y: None, + } + .add(&mut h.rsc); + let mut outer_children: Vec = + vec![fixed.add_strong(&mut h.rsc), inner.add_strong(&mut h.rsc)]; + if swapped { + outer_children.rotate_left(1); + } + let outer = Span { + children: outer_children, + dir: Dir::RIGHT, + gap: 0.0, + } + .add(&mut h.rsc); + let through = SetSize { + inner: outer.add_strong(&mut h.rsc), + x: None, + y: None, + } + .add(&mut h.rsc); + let scroll = Scroll::new(through.add_strong(&mut h.rsc), Axis::X).add(&mut h.rsc); + h.state.root = Some(scroll.add_strong(&mut h.rsc)); + ( + vec![ + text.id(), + filler.id(), + inner.id(), + block.id(), + fixed.id(), + outer.id(), + through.id(), + scroll.id(), + ], + [inner, outer], + ) +} + +#[test] +fn a_span_placed_once_in_a_box_its_answer_decided() { + let mut warm = Harness::new((640, 900)); + let (ids, spans) = plant_scrolled(&mut warm, false); + warm.frame(); + for span in spans { + warm.rsc[span].children.rotate_left(1); + } + warm.frame(); + + let mut cold = Harness::new((640, 900)); + let (cold_ids, _) = plant_scrolled(&mut cold, true); + cold.frame(); + + let mut wrong = Vec::new(); + for (i, (&w, &c)) in ids.iter().zip(&cold_ids).enumerate() { + let (got, want) = (warm.region(&w), cold.region(&c)); + if got != want { + wrong.push(format!("widget {i}: warm {got:?} cold {want:?}")); + } + } + assert!(wrong.is_empty(), "{}", wrong.join("\n")); +}