From b7b8d09e409bec381ddb705b9c21aa8db95a345c Mon Sep 17 00:00:00 2001 From: iris-ai <4+iris-ai@noreply.localhost> Date: Sun, 20 Sep 2026 02:09:41 -0400 Subject: [PATCH] Write a shared constant once, and stop a scroll placing its own content Two findings from a sweep over the WGSL prelude and the position widgets, scoped against upstream/main at ca2b4b2. `module_source` already builds each shader's preamble from iris_core's own constants, so the move-chain work's second copy of `MOVE_NONE` and `CHAIN_LIMIT` -- under "keep in step with iris_core::CHAIN_LIMIT" -- asked a reader by hand for what the mechanism beside it exists to do. Both are injected now, with `MASK_NONE` beside them replacing a bare literal, and the shader declares none of them. `Scroll`'s `content_len` is never less than its box, so `slack` and the `anchor` computed from it were always zero whatever the alignment: the framework centres short content by placing the answer in the whole box, and the comment credited arithmetic that could not have done it. The same belief guarded the fits-in-the-box contract with `align == NEG`, so at the default alignment -- the middle -- every box change redrew the scroll, measured as 1 widget against 0 at TOP_LEFT. `align` now has no reader at all. `UiSpan::translated` and `UiRegion::translated` are reachable only from each other and from nothing else. Format, clippy with and without layout-diagnostics, and the 131-test suite are clean. The cold dump over 400 depth-5 trees is byte-identical to 1096c31, and all three seed scans pass: 400 at depth 5 in 69.07s, 1,000 at depth 6 in 169.29s, 2,000 at depth 4 in 300.75s. Co-Authored-By: Claude Opus 5 --- core/src/orientation/pos.rs | 20 ------------------ core/src/primitive/text.rs | 4 ---- core/src/render/atlas.rs | 4 ---- core/src/render/mod.rs | 17 +++++++++++---- core/src/render/shader/prelude.wgsl | 16 ++++++--------- src/widget/position/scroll.rs | 32 ++++++++++++++--------------- src/widget/text/edit.rs | 10 ++++----- tests/layout_diagnostics.rs | 24 ++++++++++++++++++++++ 8 files changed, 63 insertions(+), 64 deletions(-) diff --git a/core/src/orientation/pos.rs b/core/src/orientation/pos.rs index e4a87d9..d8a516b 100644 --- a/core/src/orientation/pos.rs +++ b/core/src/orientation/pos.rs @@ -281,15 +281,6 @@ impl UiSpan { pub const fn len(&self) -> Len { self.end - self.start } - - /// Both ends by the same amount, which is what moving a box without - /// changing its length does to every part of it. - pub const fn translated(self, by: Len) -> Self { - Self { - start: self.start + by, - end: self.end + by, - } - } } #[repr(C)] @@ -300,17 +291,6 @@ pub struct UiRegion { } impl UiRegion { - /// Every part of the box by the same amount on each axis. Done to the - /// whole region rather than an end at a time, because that is what it is - /// -- and because four adds in a row are four adds, where four asked for - /// separately are four sequences. - pub const fn translated(self, x: Len, y: Len) -> Self { - Self { - x: self.x.translated(x), - y: self.y.translated(y), - } - } - pub const FULL: Self = Self { x: UiSpan::FULL, y: UiSpan::FULL, diff --git a/core/src/primitive/text.rs b/core/src/primitive/text.rs index d0034a4..b808435 100644 --- a/core/src/primitive/text.rs +++ b/core/src/primitive/text.rs @@ -133,10 +133,6 @@ impl TextBuffer { } } - pub fn new_empty() -> Self { - Self::new("") - } - pub fn text(&self) -> &str { &self.text } diff --git a/core/src/render/atlas.rs b/core/src/render/atlas.rs index 170bdeb..79c0654 100644 --- a/core/src/render/atlas.rs +++ b/core/src/render/atlas.rs @@ -167,10 +167,6 @@ impl GlyphAtlas { pub fn page_count(&self) -> u32 { self.pages.len() as u32 } - - pub fn glyph_count(&self) -> usize { - self.entries.len() - } } impl Page { diff --git a/core/src/render/mod.rs b/core/src/render/mod.rs index bdcaf1f..4dd265a 100644 --- a/core/src/render/mod.rs +++ b/core/src/render/mod.rs @@ -23,13 +23,22 @@ pub use primitive::*; const PRELUDE: &str = include_str!("./shader/prelude.wgsl"); fn module_source(wgsl: &str) -> String { - // The steps come from the same constants the CPU counts in, rather than - // a second copy of them written into the shader: a grid the two disagree - // about puts every coordinate somewhere else. + // Every number both sides count in, written once here rather than a + // second time in the shader: a grid the two disagree about puts every + // coordinate somewhere else, and a sentinel they disagree about makes one + // of them walk a chain from a slot the other says is not there. format!( - "const PX_STEP: f32 = 1.0 / {}.0;\nconst REL_STEP: f32 = 1.0 / {}.0;\n{PRELUDE}\n{wgsl}", + "const PX_STEP: f32 = 1.0 / {}.0;\n\ + const REL_STEP: f32 = 1.0 / {}.0;\n\ + const MASK_NONE: u32 = {}u;\n\ + const MOVE_NONE: u32 = {}u;\n\ + const CHAIN_LIMIT: u32 = {}u;\n\ + {PRELUDE}\n{wgsl}", 1u32 << crate::PX_SHIFT, 1u32 << crate::REL_SHIFT, + MaskIdx::NONE.idx(), + MoveIdx::NONE.idx(), + crate::CHAIN_LIMIT, ) } diff --git a/core/src/render/shader/prelude.wgsl b/core/src/render/shader/prelude.wgsl index 0c51c5e..510f1c3 100644 --- a/core/src/render/shader/prelude.wgsl +++ b/core/src/render/shader/prelude.wgsl @@ -26,9 +26,11 @@ struct MoveOffset { parent: u32, } -// `PX_STEP` and `REL_STEP` are prepended from `iris_core`'s own constants: -// what it stores is a whole count of each, both powers of two, so decoding -// is exact and the number here is the number the CPU decided. +// `PX_STEP`, `REL_STEP`, `MASK_NONE`, `MOVE_NONE` and `CHAIN_LIMIT` are +// prepended from `iris_core`'s own constants, so none of them is written +// twice. What the CPU stores is a whole count of each step, and both steps +// are powers of two, so decoding is exact and the number here is the number +// the CPU decided. // Every coordinate the CPU decided is a whole count of `PX_STEP`, so one that // composes to within half a step of a pixel boundary is on that boundary and @@ -70,12 +72,6 @@ struct Region { y: UiSpan, } -const MOVE_NONE: u32 = 4294967295u; -// Keep in step with `iris_core::CHAIN_LIMIT`. It bounds a malformed cycle -// rather than any real tree, and the CPU walk uses the same number so both -// resolve a deep one the same way. -const CHAIN_LIMIT: u32 = 64u; - // The same expression `Len::within` uses, in floats rather than on the // CPU's grid: a move is resolved here so that scrolling a subtree writes one // entry instead of walking it. What has to hold is that this agrees with @@ -171,7 +167,7 @@ fn vs_main( } fn masked(in: VertexOutput, color: vec4) -> vec4 { - if in.mask_idx == 4294967295u { + if in.mask_idx == MASK_NONE { return color; } let mask = masks[in.mask_idx]; diff --git a/src/widget/position/scroll.rs b/src/widget/position/scroll.rs index 60f14ee..788f355 100644 --- a/src/widget/position/scroll.rs +++ b/src/widget/position/scroll.rs @@ -24,36 +24,36 @@ impl Widget for Scroll { self.amt = self.content_len - self.container_len; } self.update_amt(); - let align = painter.alignment()[self.axis]; - // Content of a fixed length that fits sits at the start of any box it - // fits in -- but only anchored there. Anywhere else it is a part of - // the room left over, so it moves with every length the box takes and - // the drawing holds for that length alone. One scrolled part way sits - // where it is until the box shrinks past what is left of it. Kept to - // the end, it moves with every length. + // Reading the box in pixels above holds this drawing to that one + // length, so these two say where it holds more widely. + // + // Content of a fixed length that fits is handed the whole box below, + // and nothing here reads the box again, so every longer box gives the + // same drawing: it holds from the length the content needs upwards, + // and shrinking past that is what changes it. Where it sits in a box + // longer than itself is not this widget's to say -- placing its + // answer in the whole box is its own alignment, and that placement is + // a fraction of the box, so it holds at every length too. + // + // One scrolled part way sits where it is until the box shrinks past + // what is left of it. Kept to the end, it moves with every length. let answer_is_px = answer_len.is_px(); - if answer_is_px && self.content_len <= self.container_len && align == AxisAlign::NEG { + if answer_is_px && self.content_len <= self.container_len { painter.holds(self.axis, answer_px..=Px::MAX); } else if answer_is_px && !self.snap_end { let left = self.content_len - self.amt; painter.holds(self.axis, Px::MIN..=left); } - // Content shorter than the viewport has room to sit in, and where it - // sits is this widget's own alignment -- the same property that would - // have placed the whole scroll in a box longer than it. - let slack = (self.container_len - self.content_len).max(Px::ZERO); - let anchor = slack.mul(align.rel()); // Content that fills the viewport and has not been scrolled is the // viewport, and is handed back as it came. Writing the same box as // its own length in pixels is the same box in another form, and the // two do not round alike: a part centred in `rel 1` lands a step from // one centred in `px 900`, since halving a difference is not halving // each part of it. - let moved = anchor != Px::ZERO || self.amt != Px::ZERO; - let content = match moved || self.content_len != self.container_len { + let content = match self.amt != Px::ZERO || self.content_len != self.container_len { true => { - let start = Len::from_parts(Rel::ZERO, anchor - self.amt); + let start = Len::from_parts(Rel::ZERO, -self.amt); UiSpan::new(start, start.offset(self.content_len)).shifted_desc() } false => PlaceDescAxis::WHOLE, diff --git a/src/widget/text/edit.rs b/src/widget/text/edit.rs index 6126046..77c2fdf 100644 --- a/src/widget/text/edit.rs +++ b/src/widget/text/edit.rs @@ -321,12 +321,10 @@ impl<'a> TextEditCtx<'a> { let old = (self.text.view.buf.text().to_string(), self.text.selection); let mut undo = false; let res = self.apply_event_inner(event, modifiers, &mut undo); - if undo { - if let Some((old, selection)) = self.text.history.pop() { - self.set(&old); - self.text.selection = selection; - self.clamp_selection_to_layout(); - } + if undo && let Some((old, selection)) = self.text.history.pop() { + self.set(&old); + self.text.selection = selection; + self.clamp_selection_to_layout(); } else if self.text.view.buf.text() != old.0 { self.text.history.push(old); } diff --git a/tests/layout_diagnostics.rs b/tests/layout_diagnostics.rs index 0ad5a95..7e56d59 100644 --- a/tests/layout_diagnostics.rs +++ b/tests/layout_diagnostics.rs @@ -22,6 +22,30 @@ use std::time::Instant; const OUTPUT: (f32, f32) = (1920.0, 1200.0); +/// A scroll whose content fits is the same drawing in every box it still +/// fits in, so a longer or shorter one relays out nothing. Where the content +/// sits in that box is decided by placing its answer in the whole of it, +/// which is a fraction of the box and holds at every length -- so the +/// contract must not turn on the alignment. It did, and at the default +/// alignment, which is the middle, every box change redrew the scroll. +#[cfg(feature = "layout-diagnostics")] +#[test] +fn a_fitting_scroll_holds_for_every_box_its_content_fits_in() { + use iris::core::layout_diagnostics as diag; + + for align in [Align::TOP_LEFT, Align::CENTER, Align::BOT_RIGHT] { + let mut harness = Harness::new((400, 200)); + let inner = rect(Color::RED).height(50).add(&mut harness.rsc); + harness.set_root(inner.scrollable().align(align)); + harness.frame(); + let _ = diag::take(); + // Still far longer than the 50 the content needs. + harness.resize((400, 180)); + harness.frame(); + assert_eq!(diag::take().distinct_widgets(), 0, "{align:?}"); + } +} + #[cfg(feature = "layout-diagnostics")] #[test] fn a_selected_widget_retains_its_layout_events() {