Avoid repeated plan generation and unused diagnostics in layout fuzzers

A span measuring a child in the room kept its whole `Size`, of which the
along axis is already in `lens` and only the across one is read again when
the drawing is placed. Keep that length alone, which also retires the
rebinding of the match's result and the one in the placing loop. The
placement comment already says what becomes of a drawing made in the room,
so the measuring pass no longer says it a second time.

core/src/ui/active.rs

@@ -53,6 +53,7 @@ pub struct ActiveData {
     /// Its primitives, each keeping the box it was written in -- in this
     /// widget's extent coordinates, which is what a move recomposes from.
     pub primitives: Vec<RetainedPrimitive>,
+    /// An owned mask holds one reference independently of its primitives.
     pub mask_region: Option<UiRegion>,
     pub children: Vec<WidgetId>,
     /// The children whose size this widget read while drawing.

core/src/ui/painter.rs

@@ -35,6 +35,8 @@ pub struct Painter<'a> {
     pub(super) textures: Vec<TextureHandle>,
     pub(super) primitives: Vec<RetainedPrimitive>,
     pub(super) mask_region: Option<UiRegion>,
+    /// The previous drawing's owned mask, available for this draw to reclaim.
+    pub(super) mask_slot: Option<MaskIdx>,
     /// Only children whose answers were read constrain this widget's answer.
     pub(super) answer_under: LayoutHolds,
     pub(super) children: Vec<WidgetId>,
@@ -104,7 +106,6 @@ impl<'a> Painter<'a> {
 
     fn push_primitive(&mut self, h: RetainedPrimitive) {
         if self.mask != MaskIdx::NONE {
-            // TODO: I have no clue if this works at all :joy:
             self.rsc.ui_mut().masks.push_ref(self.mask);
         }
         self.primitives.push(h);
@@ -129,10 +130,23 @@ impl<'a> Painter<'a> {
         assert!(self.mask == MaskIdx::NONE);
         let resolved = self.resolve(region);
         let move_idx = self.move_idx;
-        self.mask = self.rsc.ui_mut().masks.push(Mask {
+        let mask = Mask {
             region: resolved,
             move_idx,
-        });
+        };
+        let masks = &mut self.rsc.ui_mut().masks;
+        self.mask = match self.mask_slot.take() {
+            Some(idx) => {
+                *masks.get_mut(idx) = mask;
+                idx
+            }
+            None => {
+                let idx = masks.push(mask);
+                // The owner keeps the slot alive even with no primitives.
+                masks.push_ref(idx);
+                idx
+            }
+        };
     }
 
     /// Draws a widget in the whole of this widget's own box, with the frame
@@ -267,36 +281,32 @@ impl<'a> Painter<'a> {
         let widgets = self.rsc.widgets();
         // A rule is the answer where there is one: it wins over whatever the
         // widget would draw, so it has to win over what the widget says too.
-        let hint = widgets
+        let hint = widgets.size_rules(id.id()).axis(axis).exact().or_else(|| {
-            .size_rules(id.id())
+            widgets
-            .axis(axis)
+                .get_dyn(id.id())
-            .exact()
+                .and_then(|widget| widget.size_hint(axis))
-            .or_else(|| {
+        });
-                widgets
+        let frame = self.frame.axis(axis);
-                    .get_dyn(id.id())
+        let resolved = hint.map(|hint| hint.within_len(frame));
-                    .and_then(|widget| widget.size_hint(axis))
-            })
-            .map(|hint| hint.within_len(self.frame.axis(axis)));
         #[cfg(feature = "layout-diagnostics")]
-        diag::hint_read(id.id(), self.id, axis, hint);
+        {
-        match hint {
+            diag::hint_read(id.id(), self.id, axis, resolved);
-            Some(hint) => {
+            diag::bump(match resolved {
-                #[cfg(feature = "layout-diagnostics")]
+                Some(_) => Counter::HintHits,
-                diag::bump(Counter::HintHits);
+                None => Counter::HintMisses,
-                self.depend_on(id);
+            });
-                // A fraction was just resolved against this frame, so what
+        }
-                // this draw does with it is a function of the frame's length.
+        if let Some(hint) = hint {
-                if hint.rel != Rel::ZERO {
+            self.depend_on(id);
-                    self.frame_own_len[axis as usize] = Some(self.frame.axis(axis));
+            // Resolving a fraction against this frame makes this draw a
-                }
+            // function of the frame's length. The fraction to ask about is
-                Some(hint)
+            // the child's own: resolved against a frame of pixels, none is
-            }
+            // left to see it by.
-            None => {
+            if hint.rel != Rel::ZERO {
-                #[cfg(feature = "layout-diagnostics")]
+                self.frame_own_len[axis as usize] = Some(frame);
-                diag::bump(Counter::HintMisses);
-                None
             }
         }
+        resolved
     }
 
     fn depend_on<W: ?Sized>(&mut self, child: &StrongWidget<W>) {

core/src/ui/render_state.rs

@@ -223,6 +223,10 @@ impl UiRenderState {
         mut old: Option<ActiveData>,
         rsc: &mut dyn UiRsc,
     ) -> (Size, LayoutHolds, LayoutHolds) {
+        let old_parent = old
+            .as_ref()
+            .or_else(|| self.active.get(&id))
+            .and_then(|a| a.parent);
         let part = info.part;
         #[cfg(feature = "layout-diagnostics")]
         {
@@ -275,12 +279,9 @@ impl UiRenderState {
         active.part = part;
         active.placed = info.placed;
         active.own_align = align;
-        // A subtree can be reused whole under a different parent -- same box,
+        // The previous parent must stop owning the subtree before it can
-        // same layer, same region node -- and nothing in the drawing says it
+        // undraw it, whether changing hands reused the drawing or replaced it.
-        // changed hands. Two things read who its parent is: a deferral, which
+        active.parent = info.parent;
-        // marks whoever has it to draw, and the old parent's list of children,
-        // which its next draw undraws whatever is missing from.
-        let old_parent = std::mem::replace(&mut active.parent, info.parent);
         if old_parent != info.parent
             && let Some(old_parent) = old_parent
             && let Some(old_parent) = self.active.get_mut(&old_parent)
@@ -312,6 +313,9 @@ impl UiRenderState {
             // Reusing its index sooner could make an old parent look current.
             false => (info.parent_move, extent, self.slots.remove(&id)),
         };
+        let mask_slot = old
+            .as_ref()
+            .and_then(|old| old.mask_region.map(|_| old.mask));
         let old_children = old.map_or_else(Vec::new, |old| old.children);
         rsc.widgets_mut().needs_redraw.remove(&id);
         let px = info.px;
@@ -330,6 +334,7 @@ impl UiRenderState {
             textures: Vec::new(),
             primitives: Vec::new(),
             mask_region: None,
+            mask_slot,
             children: Vec::new(),
             size_deps: Vec::new(),
             window_own: [Holds::ANY; 2],
@@ -363,6 +368,7 @@ impl UiRenderState {
             textures,
             primitives,
             mask_region,
+            mask_slot,
             extent_own,
             extent_len,
             answer_under,
@@ -421,6 +427,9 @@ impl UiRenderState {
                 self.undraw_rec(*c, rsc);
             }
         }
+        if let Some(idx) = mask_slot {
+            rsc.ui_mut().masks.remove(idx);
+        }
         if let Some(idx) = retired_move {
             self.moves.remove(idx);
         }
@@ -605,6 +614,9 @@ impl UiRenderState {
             }
             return None;
         }
+        if active.parent_mask != info.mask {
+            return None;
+        }
         // Drawn somewhere else in the tree: its box is in coordinates it no
         // longer sits in, and its slot names the wrong parent.
         if active.parent_move != info.parent_move {
@@ -826,6 +838,9 @@ impl UiRenderState {
                     rsc.ui_mut().masks.remove(mask);
                 }
             }
+            if undraw && active.mask_region.take().is_some() {
+                rsc.ui_mut().masks.remove(active.mask);
+            }
             active.primitives.clear();
             active.textures.clear();
             rsc.ui_mut().textures.free();
@@ -913,6 +928,7 @@ impl UiRenderState {
         self.slots.clear();
         self.moves.clear();
         self.layers.clear();
+        rsc.ui_mut().masks = Default::default();
         rsc.widgets_mut().needs_redraw.clear();
         self.free(rsc);
     }

src/widget/position/span.rs

@@ -28,16 +28,13 @@ impl Widget for Span {
         // given whatever else is in it and wherever this child sits among
         // them; what a drawn child is asked in is the room left from the
         // cursor, because a text has to wrap at the width actually there.
-        // This is the one ask a drawn fixed child gets: its slot is its
-        // answer, and the drawing is moved there once the shares are known.
-        // A hinted child is asked once, in its slot.
         let mut cursor = Len::rel_min();
         let mut lens = Vec::with_capacity(self.children.len());
-        let mut measured = Vec::with_capacity(self.children.len());
+        let mut drawn_across = Vec::with_capacity(self.children.len());
         for child in &self.children {
-            let size = match painter.size_hint(child, axis) {
+            let len = match painter.size_hint(child, axis) {
                 Some(len) => {
-                    measured.push(None);
+                    drawn_across.push(None);
                     len
                 }
                 None => {
@@ -45,11 +42,10 @@ impl Widget for Span {
                     let size = painter
                         .widget_at(child, [None; 2], axis.pair(room, across))
                         .size();
-                    measured.push(Some(size));
+                    drawn_across.push(Some(size.axis(!axis)));
                     size.axis(axis)
                 }
             };
-            let len = size;
             cursor.px += len.px + self.gap;
             cursor.rel += len.rel;
             lens.push(len);
@@ -104,8 +100,7 @@ impl Widget for Span {
         let mut taken = Weight::ZERO;
         let mut start = Len::rel_min();
         let mut ortho = LayoutLen::ZERO;
-        for ((child, len), measured) in self.children.iter().zip(&lens).zip(&measured) {
+        for ((child, &len), &across_len) in self.children.iter().zip(&lens).zip(&drawn_across) {
-            let len = *len;
             // A child asking for nothing but a part of what is left over,
             // when nothing is, is not drawn at all. One that also asked for
             // pixels or a fraction keeps those and overflows.
@@ -113,6 +108,7 @@ impl Widget for Span {
             {
                 painter.undraw(child);
                 fixed.px += self.gap;
+                start = shared(fixed, taken, total.leftover, room);
                 continue;
             }
             let from = start;
@@ -135,10 +131,10 @@ impl Widget for Span {
             if len.leftover > Weight::ZERO && shares {
                 narrow[axis as usize] = Some(slot.len());
             }
-            let used = match (measured, narrow[axis as usize]) {
+            let used = match (across_len, narrow[axis as usize]) {
-                (Some(size), None) => {
+                (Some(across_len), None) => {
                     painter.place_at(child, place);
-                    size.axis(!axis)
+                    across_len
                 }
                 _ => painter.widget_at(child, narrow, place).len(!axis),
             };

tests/cases/layout.rs

@@ -821,3 +821,40 @@ fn a_root_with_a_fraction_rule_is_that_fraction_of_the_window() {
     h.set_root(root);
     assert_eq!(h.region(&root).unwrap().size().x, Px::from_int(450));
 }
+
+#[test]
+fn a_collapsed_share_keeps_the_gaps_before_the_next_slot() {
+    for dir in [Dir::RIGHT, Dir::LEFT, Dir::DOWN, Dir::UP] {
+        for collapsed in [1, 2] {
+            let mut h = Harness::new((400, 400));
+            let head = rect(Color::RED).add(&mut h.rsc);
+            h.set_len(head, dir.axis, 200);
+            let tail = rect(Color::BLUE).add(&mut h.rsc);
+            let tail_len = 200 - 10 * (collapsed + 1);
+            h.set_len(tail, dir.axis, tail_len);
+            let mut children: Vec<StrongWidget> = vec![head.add_strong(&mut h.rsc)];
+            let mut shares = Vec::new();
+            for _ in 0..collapsed {
+                let share = rect(Color::GREEN).add(&mut h.rsc);
+                shares.push(share);
+                children.push(share.add_strong(&mut h.rsc));
+            }
+            children.push(tail.add_strong(&mut h.rsc));
+            h.set_root(Span {
+                children,
+                dir,
+                gap: Px::from_int(10),
+            });
+            for share in shares {
+                assert!(h.region(&share).is_none());
+            }
+            let region = h.region(&tail).unwrap();
+            let (from, to) = match dir.sign {
+                Sign::Pos => (400 - tail_len, 400),
+                Sign::Neg => (0, tail_len),
+            };
+            assert_eq!(region.top_left.axis(dir.axis), Px::from_int(from));
+            assert_eq!(region.bot_right.axis(dir.axis), Px::from_int(to));
+        }
+    }
+}

tests/cases/retained.rs

@@ -1342,3 +1342,154 @@ fn extent_frames_keep_fractional_reports_and_numeric_dependencies_valid() {
         }
     }
 }
+
+struct OptionalMask {
+    inner: StrongWidget,
+    enabled: bool,
+}
+
+impl Widget for OptionalMask {
+    fn draw(&mut self, painter: &mut Painter) -> Size {
+        if self.enabled {
+            painter.set_mask(UiRegion::FULL);
+        }
+        painter.widget(&self.inner);
+        Size::LEFTOVER
+    }
+}
+
+fn primitive_masks(h: &Harness, id: WidgetId) -> Vec<MaskIdx> {
+    h.render.active[&id]
+        .primitives
+        .iter()
+        .map(|primitive| {
+            let handle = &primitive.handle;
+            h.render.layers[handle.layer].primitives()[handle.kind as usize]
+                .as_ref()
+                .unwrap()
+                .instances()[handle.inst_idx]
+                .mask_idx
+        })
+        .collect()
+}
+
+#[test]
+fn a_redrawn_mask_keeps_reused_primitives_clipped_when_it_moves() {
+    for node in [false, true] {
+        let mut h = Harness::new((400, 200));
+        let first = rect(Color::RED).height(50).add(&mut h.rsc);
+        let inner = rect(Color::BLUE).add(&mut h.rsc);
+        let draws = Rc::new(Cell::new(0));
+        let child = Stretchy {
+            inner: inner.add_strong(&mut h.rsc),
+            draws: draws.clone(),
+        }
+        .add(&mut h.rsc);
+        let masked = child.masked().add(&mut h.rsc);
+        h.rsc.widgets_mut().set_region_node(masked, node);
+        h.set_root((first, masked).span(Dir::DOWN));
+        let mask = h.render.active[&masked.id()].mask;
+        let settled = draws.get();
+        h.rsc.widgets_mut().get_dyn_mut(masked.id());
+        h.frame();
+        assert_eq!(primitive_masks(&h, inner.id()), vec![mask]);
+        assert_eq!(draws.get(), settled, "a mask repaint must reuse its child");
+        assert_eq!(h.render.active[&masked.id()].mask, mask);
+        h.set_len(first, Axis::Y, 10);
+        h.frame();
+        let clip = h.rsc.ui().masks[mask.idx()];
+        let clip = h
+            .render
+            .moves
+            .resolve(clip.move_idx, clip.region)
+            .to_px(h.render.output_size());
+        assert_eq!(clip, h.region(&masked).unwrap());
+        assert_corners!(h, inner, (0, 10), (400, 200));
+    }
+}
+
+#[test]
+fn adding_and_removing_a_mask_updates_existing_primitives() {
+    let mut h = Harness::new((400, 200));
+    let inner = rect(Color::BLUE).add(&mut h.rsc);
+    let masked = OptionalMask {
+        inner: inner.add_strong(&mut h.rsc),
+        enabled: false,
+    }
+    .add(&mut h.rsc);
+    h.set_root(masked);
+    for enabled in [true, false, true, false] {
+        h.rsc[masked].enabled = enabled;
+        h.frame();
+        let mask = h.render.active[&masked.id()].mask;
+        assert_eq!(mask == MaskIdx::NONE, !enabled);
+        assert_eq!(primitive_masks(&h, inner.id()), vec![mask]);
+    }
+    assert_eq!(h.rsc.ui().masks.len(), 1, "retired slots must be reusable");
+}
+
+#[test]
+fn an_empty_masks_slot_is_released_when_the_mask_is_removed_or_undrawn() {
+    let mut h = Harness::new((400, 200));
+    let (inner, _) = counted(&mut h, Size::LEFTOVER, false);
+    let masked = OptionalMask {
+        inner: inner.add_strong(&mut h.rsc),
+        enabled: true,
+    }
+    .add(&mut h.rsc);
+    let row = (masked,).span(Dir::DOWN).add(&mut h.rsc);
+    h.set_root(row);
+    for _ in 0..3 {
+        h.rsc[masked].enabled = false;
+        h.frame();
+        h.rsc[masked].enabled = true;
+        h.frame();
+        let child = h.rsc[row].pop().unwrap();
+        h.frame();
+        h.rsc[row].push(child);
+        h.frame();
+    }
+    assert_eq!(h.rsc.ui().masks.len(), 1);
+}
+
+struct SharedChild(Rc<StrongWidget>);
+
+impl Widget for SharedChild {
+    fn draw(&mut self, painter: &mut Painter) -> Size {
+        painter.widget(self.0.as_ref()).size()
+    }
+}
+
+struct SwitchParent {
+    choices: [StrongWidget; 2],
+    choice: usize,
+}
+
+impl Widget for SwitchParent {
+    fn draw(&mut self, painter: &mut Painter) -> Size {
+        painter.widget(&self.choices[self.choice]).size()
+    }
+}
+
+#[test]
+fn a_redrawn_subtree_is_not_undrawn_by_the_parent_it_left() {
+    for node in [false, true] {
+        let mut h = Harness::new((400, 200));
+        let leaf = rect(Color::RED).width(40).add(&mut h.rsc);
+        let held: StrongWidget = leaf.add_strong(&mut h.rsc);
+        let shared = Rc::new(held);
+        let first = SharedChild(shared.clone()).add_strong(&mut h.rsc);
+        let second = SharedChild(shared).add_strong(&mut h.rsc);
+        h.rsc.widgets_mut().set_region_node(&second, node);
+        let root = SwitchParent {
+            choices: [first, second],
+            choice: 0,
+        }
+        .add(&mut h.rsc);
+        h.set_root(root);
+        let before = h.region(&leaf);
+        h.rsc[root].choice = 1;
+        h.frame();
+        assert_eq!(h.region(&leaf), before);
+    }
+}

tests/generated.rs

@@ -14,7 +14,7 @@
 #[path = "scenario/mod.rs"]
 mod scenario;
 
-use iris::random::{Edits, plan};
+use iris::random::{Edits, Plan, plan};
 use scenario::{ALL, Case, diverges, env, over_seeds};
 
 /// How deep the generator branches. The generator widens two to four ways per
@@ -32,8 +32,11 @@ fn depth() -> usize {
 const SEEDS: [u64; 10] = [1, 2, 3, 5, 8, 10, 13, 20, 86, 98];
 
 fn check(seed: u64, depth: usize, case: Case) {
-    let grown = plan(seed, depth, &Edits::default());
+    check_plan(&plan(seed, depth, &Edits::default()), seed, depth, case);
-    if let Some(how) = diverges(&grown, case, seed) {
+}
+
+fn check_plan(grown: &Plan, seed: u64, depth: usize, case: Case) {
+    if let Some(how) = diverges(grown, case, seed) {
         panic!(
             "seed {seed} at depth {depth} differs after {}: {how}\n\
              reduce it with SHRINK_SEED={seed} SHRINK_DEPTH={depth} \
@@ -119,8 +122,9 @@ fn a_long_run_of_seeds_agrees() {
         None => (1..=env("IRIS_GENERATED_SEEDS", 100_u64)).collect(),
     };
     over_seeds(seeds, |seed| {
+        let grown = plan(seed, depth, &Edits::default());
         for case in ALL {
-            check(seed, depth, case);
+            check_plan(&grown, seed, depth, case);
         }
     });
 }

tests/scenario/mod.rs

@@ -444,12 +444,6 @@ pub fn diverges(plan: &Plan, case: Case, seed: u64) -> Option<String> {
     cold.state.root = Some(root);
     cold.frame();
 
-    let places: HashMap<WidgetId, usize> = tree
-        .ids
-        .iter()
-        .enumerate()
-        .map(|(i, &id)| (id, i))
-        .collect();
     let mut drawn = 0;
     for (i, (&w, &c)) in tree.ids.iter().zip(&cold_tree.ids).enumerate() {
         let (got, want) = (warm.region(&w), cold.region(&c));
@@ -457,6 +451,12 @@ pub fn diverges(plan: &Plan, case: Case, seed: u64) -> Option<String> {
         if got == want {
             continue;
         }
+        let places: HashMap<WidgetId, usize> = tree
+            .ids
+            .iter()
+            .enumerate()
+            .map(|(i, &id)| (id, i))
+            .collect();
         // Where two trees disagree is rarely where the cause is, so the
         // ancestry comes with it, marking the widgets that own a region.
         let mut chain = Vec::new();