From 08c9d5aa320690545995570f2229d1362e195b9d Mon Sep 17 00:00:00 2001 From: iris-ai <4+iris-ai@noreply.localhost> Date: Wed, 16 Sep 2026 17:03:45 -0400 Subject: [PATCH] Drop a multiply to the step below rather than rounding it Bryan's call, 2026-09-16, taken for the cycles: a share now lands a thousandth of a pixel short of its row instead of on it, which is less than an even number of pixels draws. `Fixed::mul` is a widening multiply and a shift, with the sign branch and the half-step add gone. The two short-circuits priced against the old multiply go with it: `UiSpan::within`'s test for a span that is the whole of its parent, and `Fixed::scaled`'s test for nothing scaled by something, which was the whole of `scaled` -- both cases come out of the truncating multiply unchanged, and the bodies the comparisons cost were what kept the inliner from taking `within` at all. `nm` is the check: `::within` is a symbol in the rounding head and in neither the float head nor this one. `Holds::through` inverts the multiply, so its widening is re-derived: each rounding now drops a whole step where it dropped half of one, which doubles the allowance for the two routes to a length, and the multiply on the way in drops only downward, so its own step goes at the top of the range alone. The derived allowance for one truncation either side is measurably too narrow -- it excludes boxes drawings were made in, in eleven generated cases -- because each route is a chain of multiplies rather than one. Measured on the fixed-shape fixture (`Edits::fixed_branches`), seed 1 depth 8, 500 frames of `many`, medians of 25 runs of uninstrumented release binaries with this VM's garbage `perf` readings dropped: | | instructions | cycles | IPC | | --- | ---: | ---: | ---: | | `5ed9e87`, the float head | 1,761M | 688M | 2.561 | | `60367d8`, rounding | 1,915M | 777M | 2.465 | | this | 1,800M | 715M | 2.516 | -6.0% instructions and -8.0% cycles against `60367d8`, whose twenty-five work counters are identical to this one's, so that pair is the same work at a different speed. It leaves +2.2% and +3.9% against the float head, from +8.7% and +12.9% -- but the float head draws 100 widgets to this one's 97 and writes 4,272 primitives to 3,951, so that pair is not, and the remainder is not all arithmetic. Checked: fmt, clippy, 80 suite tests and 18 core unit tests, the release oracle at 100 seeds, all fifteen shrinker cases at 400 seeds of depth 5 (seed 288 on `region-node` still failing, unchanged), and depth-6 oracle seeds 18 and 190 passing with 326 still failing. `view`, `minimal`, `text`, `random` and the tab replay render byte-identical at 1920x1200; `tabs` differs on 4,664 of 2,304,000 pixels, single-pixel-wide runs along 80 columns of one band of rounded rects, which is an antialiased edge moved less than a pixel. Three tests say what changed rather than being relaxed: a multiply drops on both sides of zero, a division cannot put back what it dropped, and an unevenly nested row's shares stay contiguous and end at its edge with each edge on the even division or one step below. Co-Authored-By: Claude Opus 5 --- core/src/fixed.rs | 53 ++++++++++++++++------------- core/src/orientation/pos.rs | 24 +++---------- core/src/ui/holds.rs | 68 +++++++++++++++++++++++-------------- core/src/ui/render_state.rs | 6 ++-- tests/cases/layout.rs | 23 +++++++++++-- 5 files changed, 101 insertions(+), 73 deletions(-) diff --git a/core/src/fixed.rs b/core/src/fixed.rs index 72380d6..9e71070 100644 --- a/core/src/fixed.rs +++ b/core/src/fixed.rs @@ -10,10 +10,10 @@ use std::{ /// chain, and the same box summed from what its children asked for -- and has /// to decide whether the two are the same place. In floats they land a few /// bits apart, which is a defect wherever the answer changes what is drawn -/// rather than where. Here adding and subtracting are exact and only a -/// multiply or a conversion rounds, back onto the same steps, so two routes -/// that come within half a step land on one number and everything downstream -/// compares for equality instead of for nearness. +/// rather than where. Here adding and subtracting are exact, a multiply +/// drops to the step below, and a conversion between grids takes the nearest +/// one, so two routes to one place land on one number and everything +/// downstream compares for equality instead of for nearness. /// /// `SHIFT` is the number of fractional bits, which is what makes the steps /// divide a whole number: a power of two also converts to `f32` without @@ -31,9 +31,9 @@ use std::{ )] pub struct Fixed(i32); -/// A length or a coordinate in pixels, to a sixty-fourth. Finer than anything -/// a display can show, and exact in `f32` up to 262,144 px, which is what lets -/// the same number reach the GPU. +/// A length or a coordinate in pixels, in steps of `1/1024`. Finer than +/// anything a display can show, and exact in `f32` up to 16,384 px, which is +/// what lets the same number reach the GPU. pub type Px = Fixed; /// How many bits of a pixel a [`Px`] keeps. One place, because [`PxVec2`] @@ -142,18 +142,16 @@ impl Fixed { /// Scaled by a number on any grid, which is how a length takes a fraction /// of itself and keeps being a length: the product is measured in the /// receiver's steps. + /// + /// Dropped to the step below rather than taken to the nearest one + /// (Bryan, 2026-09-16), which costs a share a thousandth of a pixel of + /// its row -- less than an even number of pixels draws. Toward negative + /// infinity on both sides of zero, since that is a shift and nothing + /// else: a value and its negation therefore land different distances + /// from where they came, so a flipped span can sit a step from its + /// mirror image. pub const fn mul(self, by: Fixed) -> Self { - Self(shift_round(self.0 as i64 * by.0 as i64, BY) as i32) - } - - /// A part of a span that is often nothing: no part of nothing is - /// nothing, for the cost of a comparison rather than a widening - /// multiply and a rounding. - pub const fn scaled(self, by: Fixed) -> Self { - match self.0 == 0 { - true => self, - false => self.mul(by), - } + Self(((self.0 as i64 * by.0 as i64) >> BY) as i32) } /// Repeated a whole number of times, which no grid rounds. @@ -198,7 +196,7 @@ impl Fixed { /// `from` and `to` a fraction of the way apart, the fraction being the /// receiver -- the argument order [`crate::util::LerpUtil`] already uses. pub const fn lerp(self, from: Fixed, to: Fixed) -> Fixed { - from.add(to.sub(from).scaled(self)) + from.add(to.sub(from).mul(self)) } pub const fn min(self, other: Self) -> Self { @@ -465,19 +463,28 @@ mod tests { assert_eq!(Px::from_int(100) * Rel::ZERO, Px::ZERO); } + /// Toward negative infinity on both sides of zero, which is what makes + /// it a shift rather than a shift and a sign branch -- and what makes a + /// value and its negation land different distances from where they came, + /// so a flipped span can sit a step from its mirror image. #[test] - fn halves_round_away_from_zero_either_side() { + fn a_multiply_drops_to_the_step_below_on_both_sides_of_zero() { // A step and a half of one, which has no step of its own. let step_and_a_half = Rel::from_f32(1.5).div_int(Px::ONE.raw()); - assert_eq!(Px::ONE * step_and_a_half, Px::from_raw(2)); + assert_eq!(Px::ONE * step_and_a_half, Px::from_raw(1)); assert_eq!(Px::ONE.neg() * step_and_a_half, Px::from_raw(-2)); } + /// A division rounds to the nearest step, so it cannot put back the + /// steps a truncating multiply dropped: a round trip comes back short, + /// never long, and by the few steps the two operations gave up. #[test] - fn dividing_by_a_fraction_undoes_multiplying_by_it() { + fn dividing_by_a_fraction_cannot_undo_a_truncating_multiply() { let third = Rel::ONE / Rel::from_int(3); let len = Px::from_int(300); - assert_eq!(len * third / third, len); + let back = len * third / third; + assert!(back <= len, "{back:?} is longer than {len:?}"); + assert!(len - back <= Px::from_raw(3), "{back:?} against {len:?}"); assert_eq!(Px::from_int(100) / Rel::from_f32(0.5), Px::from_int(200)); } diff --git a/core/src/orientation/pos.rs b/core/src/orientation/pos.rs index 495575b..6b7dd13 100644 --- a/core/src/orientation/pos.rs +++ b/core/src/orientation/pos.rs @@ -282,26 +282,12 @@ impl UiSpan { self.end += offset; } - /// The whole of the box it sits in: a span that composes to nothing and - /// a parent that changes nothing. - pub const fn is_full(&self) -> bool { - self.start.rel.raw() == Rel::ZERO.raw() - && self.start.px.raw() == Px::ZERO.raw() - && self.end.rel.raw() == Rel::ONE.raw() - && self.end.px.raw() == Px::ZERO.raw() - } - + /// Composing a box through the one it sits in, and the hottest line in + /// layout. It used to skip the multiplies where a span was the whole of + /// its parent or the parent the whole of its own; both come out of the + /// multiply unchanged anyway, and the body those comparisons cost was + /// what kept the inliner from taking this at all. pub const fn within(&self, parent: &Self) -> Self { - // A part that is the whole box is the box, and a box composed through - // the whole of its parent is itself. Both are exact -- multiplying by - // one rounds to what it started as -- and both are common enough to - // be worth four comparisons rather than four multiplies to find out. - if self.is_full() { - return *parent; - } - if parent.is_full() { - return *self; - } Self { start: self.start.within(parent), end: self.end.within(parent), diff --git a/core/src/ui/holds.rs b/core/src/ui/holds.rs index b30ab6e..080352c 100644 --- a/core/src/ui/holds.rs +++ b/core/src/ui/holds.rs @@ -55,25 +55,31 @@ impl Holds { if rel == 0 { return Self::ANY; } - // Half steps either side: two for the difference between a length - // composed down the chain and the same length measured against the - // window, and one more for the rounding on the way in -- which the - // whole of a box did not have, however many pixels were added to it, - // since multiplying by one is exact and taking the pixels off again - // is too. Allowing for it there anyway compounded, half a step a - // level down a chain of widgets each taking the whole of its parent, - // and a range wider than what a drawing holds for admits reusing it - // where it does not hold. Shifted by half of what a `Rel` counts in, - // to divide by the fraction: exact until the division takes it back - // to the grid. + // In half steps. The box a length was composed down the chain from + // and the box the same length is measured against the window in are + // two routes to one number, each rounding where the other does not, + // and each rounding drops a whole step since `Fixed::mul` truncates: + // two steps either side. The multiply on the way in drops a step of + // its own, and only downward, so it is one more step at the top and + // nothing at the bottom -- and the whole of a box has no multiply in + // it, however many pixels were added to it, since multiplying by one + // is exact and taking the pixels off again is too. Allowing for it + // there anyway compounded, a step a level down a chain of widgets + // each taking the whole of its parent, which is the unsound + // direction: a range wider than what a drawing holds for admits + // reusing it where it does not hold. + // + // Shifted by half of what a `Rel` counts in, to divide by the + // fraction: exact until the division takes it back to the grid. + const ROUTES: i64 = 4; let px = len.px.raw() as i64; let half_rel = REL_SHIFT - 1; - let slack = match rel == Rel::ONE.raw() as i64 { - true => 2, - false => 3, + let way_in = match rel == Rel::ONE.raw() as i64 { + true => 0, + false => 2, }; - let lo = ((self.lo.raw() as i64 - px) * 2 - slack) << half_rel; - let hi = ((self.hi.raw() as i64 - px) * 2 + slack) << half_rel; + let lo = ((self.lo.raw() as i64 - px) * 2 - ROUTES) << half_rel; + let hi = ((self.hi.raw() as i64 - px) * 2 + ROUTES + way_in) << half_rel; // Dividing by a negative turns the ends around, so which end each // bound comes from is decided before dividing rather than by taking // the min and max of four divisions. @@ -127,25 +133,35 @@ mod tests { } /// A widget handed the whole of its parent's box, with or without pixels - /// taken off it, brings no rounding of its own: only the step between a - /// length composed down the chain and the same length measured against - /// the window is left to allow for. Widening for the multiply as well - /// grew the interval a level at a time down a chain of them. + /// taken off it, brings no multiply of its own: only the two routes to + /// the same length are left to allow for, and not a rounding that did + /// not happen. Widening for it as well grew the interval a level at a + /// time down a chain of them. #[test] - fn the_whole_of_a_box_widens_by_the_one_step_it_has_to() { + fn the_whole_of_a_box_widens_by_the_routes_alone() { let at = Px::from_int(956); - let step = |len: Px| Holds { - lo: len - Px::STEP, - hi: len + Px::STEP, + let two_steps = |len: Px| Holds { + lo: len - Px::from_raw(2), + hi: len + Px::from_raw(2), }; - assert_eq!(Holds::at(at).through(Len::FULL), step(at)); + assert_eq!(Holds::at(at).through(Len::FULL), two_steps(at)); let less_eight = Len::from_parts(Rel::ONE, Px::from_int(-8)); assert_eq!( Holds::at(at).through(less_eight), - step(at + Px::from_int(8)) + two_steps(at + Px::from_int(8)) ); } + /// A truncating multiply only ever drops, so the step it needs allowing + /// for on the way in belongs at the top of the range and not the bottom. + #[test] + fn a_fraction_widens_further_up_than_down() { + let half = Len::from_parts(Rel::from_f32(0.5), Px::ZERO); + let holds = Holds::at(Px::from_int(100)).through(half); + let box_len = Px::from_int(200); + assert!(holds.hi - box_len > box_len - holds.lo, "{holds:?}"); + } + #[test] fn a_boundary_the_next_step_along_does_not_admit_it() { let boundary = Px::from_int(10); diff --git a/core/src/ui/render_state.rs b/core/src/ui/render_state.rs index 06f231c..051f21b 100644 --- a/core/src/ui/render_state.rs +++ b/core/src/ui/render_state.rs @@ -1147,9 +1147,9 @@ impl AxisRemap { true => offset, false => offset / scale.extent, }; - let from_px = scale.from_px + scale.from_px_span.scaled(fraction); - let to_rel = scale.to_rel + scale.to_rel_span.scaled(fraction); - let to_px = scale.to_px + scale.to_px_span.scaled(fraction); + let from_px = scale.from_px + scale.from_px_span.mul(fraction); + let to_rel = scale.to_rel + scale.to_rel_span.mul(fraction); + let to_px = scale.to_px + scale.to_px_span.mul(fraction); Len::from_parts(to_rel, scalar.px - from_px + to_px) } } diff --git a/tests/cases/layout.rs b/tests/cases/layout.rs index 51b69b4..5b972cf 100644 --- a/tests/cases/layout.rs +++ b/tests/cases/layout.rs @@ -266,6 +266,11 @@ fn nested_spans_divide_the_space_once_however_deep_the_nesting_is() { /// The same space, unevenly nested: weights carried up mean a share is a /// share of the whole, not of whatever branch a widget happens to sit in. +/// +/// Each edge lands on the even division or one step below it, since a share +/// is a fraction of the room and a truncating multiply gives up what that +/// fraction does not divide. What stays exact is that each share starts +/// where the last one ended and the row ends at its own edge. #[test] fn an_uneven_nesting_still_gives_every_share_the_same_length() { let mut h = Harness::new((400, 200)); @@ -279,10 +284,24 @@ fn an_uneven_nesting_still_gives_every_share_the_same_length() { let three = (b, c, d).span(Dir::RIGHT).add(&mut h.rsc); h.set_root((one, three).span(Dir::RIGHT)); + let mut start = Px::ZERO; for (i, id) in [a, b, c, d].into_iter().enumerate() { - let x = i as f32 * 100.0; - assert_corners!(h, id, (x, 0), (x + 100.0, 200)); + let got = h.region(&id).expect("widget drew nothing"); + let even = Px::from_int((i as i32 + 1) * 100); + assert_eq!(got.top_left, PxVec2::new(start, Px::ZERO), "share {i}"); + assert_eq!(got.bot_right.y, Px::from_int(200), "share {i}"); + assert!( + got.bot_right.x == even || got.bot_right.x == even.next_down(), + "share {i} ends at {:?}, not {even:?}", + got.bot_right.x + ); + start = got.bot_right.x; } + assert_eq!( + start, + Px::from_int(400), + "the row stopped short of its edge" + ); } /// However many ways a row is divided, the shares add up to the row: each