Avoid repeated plan generation and unused diagnostics in layout fuzzers

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

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,
-        // same layer, same region node -- and nothing in the drawing says it
-        // changed hands. Two things read who its parent is: a deferral, which
-        // 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);
+        // The previous parent must stop owning the subtree before it can
+        // undraw it, whether changing hands reused the drawing or replaced it.
+        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

@@ -108,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;

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());
-    if let Some(how) = diverges(&grown, case, seed) {
+    check_plan(&plan(seed, depth, &Edits::default()), seed, depth, case);
+}
+
+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();