From eca7759f1eaf30113ccdd3d1fa397c416fe499f8 Mon Sep 17 00:00:00 2001 From: Sachin Iyer Date: Fri, 8 May 2026 20:06:39 -0700 Subject: [PATCH] fix(satisfying_sort): remove dead source_region gate that hid 27.7% of triangle pixels The source_region != region check in pixel_style_at mixed a 1D source_x (derived from a shuffled array index) with the current pixel's 2D local_y, which has no geometric meaning and incorrectly hid ~27.7% of triangle pixels during the Idle and Sorting phases. The shuffled_idx was never actually used to color pixels (styling uses nominal_index), so the gate was effectively dead code that only removed pixels. Drop the gate, simplify pixel_style_at to map nominal_index directly to a BandStyle, and delete helpers that were only used to support it. Add a regression test that walks the full viewport during Idle and asserts every TopTriangle/BottomTriangle pixel renders as Base. Fixes #268 Co-Authored-By: Claude Opus 4.7 (1M context) --- src/commands/satisfying_sort/logo_math.rs | 53 ------------------ src/commands/satisfying_sort/render.rs | 64 +++++++++++++--------- src/commands/satisfying_sort/sort_state.rs | 47 ---------------- 3 files changed, 38 insertions(+), 126 deletions(-) diff --git a/src/commands/satisfying_sort/logo_math.rs b/src/commands/satisfying_sort/logo_math.rs index 49bbf6c..674de85 100644 --- a/src/commands/satisfying_sort/logo_math.rs +++ b/src/commands/satisfying_sort/logo_math.rs @@ -74,40 +74,6 @@ pub(super) fn triangle_nominal_index(nx: f32, n: usize, region: LogoRegion) -> u floor_f32_to_usize(t * usize_to_f32(n)).min(n - 1) } -pub(super) fn shuffled_index_for_nominal(array: &[usize], nominal_index: usize, n: usize) -> usize { - let n = n.max(1); - array - .get(nominal_index.min(n - 1)) - .copied() - .unwrap_or_else(|| nominal_index.saturating_add(1)) - .saturating_sub(1) - .min(n - 1) -} - -pub(super) fn source_local_x_for_index( - shuffled_index: usize, - n: usize, - region: LogoRegion, - viewport_width: usize, -) -> usize { - if viewport_width == 0 { - return 0; - } - let max_col = viewport_width - 1; - let unit = if n <= 1 { - 0.0 - } else { - usize_to_f32(shuffled_index.min(n - 1)) / usize_to_f32(n - 1) - }; - - let nx = match region { - LogoRegion::TopTriangle => (unit * TOP_TRIANGLE_X_SPAN).clamp(0.0, 1.0), - LogoRegion::BottomTriangle => (1.0 - unit * BOTTOM_TRIANGLE_X_SPAN).clamp(0.0, 1.0), - LogoRegion::Static | LogoRegion::Empty => unit.clamp(0.0, 1.0), - }; - round_f32_to_usize(nx * usize_to_f32(max_col)).min(max_col) -} - pub(super) fn logo_region_at( local_x: usize, local_y: usize, @@ -241,25 +207,6 @@ mod tests { ); } - #[test] - fn source_column_uses_shuffled_order() { - let array = vec![2, 1, 3, 4, 5, 6, 7, 8]; - let n = array.len(); - let viewport_width = 80; - - let nominal_left = 0_usize; - let nominal_next = 1_usize; - let shuffled_left = shuffled_index_for_nominal(&array, nominal_left, n); - let shuffled_next = shuffled_index_for_nominal(&array, nominal_next, n); - - let source_left = - source_local_x_for_index(shuffled_left, n, LogoRegion::TopTriangle, viewport_width); - let source_next = - source_local_x_for_index(shuffled_next, n, LogoRegion::TopTriangle, viewport_width); - - assert!(source_left > source_next); - } - #[test] fn mask_region_lookup_tracks_generated_mask() { let logo = generate_logo_mask(128); diff --git a/src/commands/satisfying_sort/render.rs b/src/commands/satisfying_sort/render.rs index 3930983..9117ca9 100644 --- a/src/commands/satisfying_sort/render.rs +++ b/src/commands/satisfying_sort/render.rs @@ -1,9 +1,6 @@ use ratatui::{style::Color, Frame}; -use super::logo_math::{ - logo_region_at, shuffled_index_for_nominal, source_local_x_for_index, triangle_nominal_index, - LogoRegion, -}; +use super::logo_math::{logo_region_at, triangle_nominal_index, LogoRegion}; use super::numeric::{floor_f32_to_usize, round_f32_to_usize, usize_to_f32}; use super::sort_state::{BandStyle, SortState}; @@ -203,13 +200,6 @@ fn pixel_style_at(ctx: &PixelQueryCtx<'_>, local_x: usize, local_y: usize) -> Pi if matches!(region, LogoRegion::TopTriangle | LogoRegion::BottomTriangle) { let nx = usize_to_f32(local_x) / ctx.x_den; let nominal_index = triangle_nominal_index(nx, ctx.n, region); - let shuffled_index = visual_source_index_for_nominal(ctx.state, nominal_index, ctx.n); - let source_x = source_local_x_for_index(shuffled_index, ctx.n, region, ctx.viewport_width); - let source_region = - logo_region_at(source_x, local_y, ctx.viewport_width, ctx.viewport_height); - if source_region != region { - return PixelStyle::Off; - } return match ctx .state .style_for_index_with_min_window(nominal_index, ctx.active_min_window) @@ -223,21 +213,6 @@ fn pixel_style_at(ctx: &PixelQueryCtx<'_>, local_x: usize, local_y: usize) -> Pi PixelStyle::Base } -fn visual_source_index_for_nominal(state: &SortState, nominal_index: usize, n: usize) -> usize { - let n = n.max(1); - let nominal_index = nominal_index.min(n - 1); - - if state.scan_complete() - || state - .current_scan_index() - .is_some_and(|scan| nominal_index <= scan) - { - return nominal_index; - } - - shuffled_index_for_nominal(state.source_array(), nominal_index, n) -} - fn min_active_window_for_columns(n: usize, viewport_width: usize, target_columns: usize) -> usize { let n = n.max(1); let target_columns = target_columns.max(1); @@ -345,6 +320,43 @@ mod tests { assert!((0.5..=4.0).contains(&aspect)); } + #[test] + fn idle_phase_populates_every_triangle_pixel() { + let mut rng = SmallRng::seed_from_u64(42); + let state = SortState::new(1000, &mut rng); + let viewport = compute_logo_viewport(140, 50, 1.0).expect("viewport"); + + let n = state.len().max(1); + let x_den = usize_to_f32(viewport.width.saturating_sub(1).max(1)); + let active_min_window = min_active_window_for_columns(n, viewport.width, 2); + let ctx = PixelQueryCtx { + state: &state, + n, + viewport_width: viewport.width, + viewport_height: viewport.side, + x_den, + active_min_window, + }; + + let mut triangle_pixels = 0_usize; + for local_y in 0..viewport.side { + for local_x in 0..viewport.width { + let region = + logo_region_at(local_x, local_y, ctx.viewport_width, ctx.viewport_height); + if matches!(region, LogoRegion::TopTriangle | LogoRegion::BottomTriangle) { + triangle_pixels = triangle_pixels.saturating_add(1); + let style = pixel_style_at(&ctx, local_x, local_y); + assert_eq!( + style, + PixelStyle::Base, + "triangle pixel at ({local_x}, {local_y}) should be Base during Idle" + ); + } + } + } + assert!(triangle_pixels > 0, "expected non-empty triangle region"); + } + #[test] fn min_active_window_produces_two_visible_blue_columns() { let n = 1000_usize; diff --git a/src/commands/satisfying_sort/sort_state.rs b/src/commands/satisfying_sort/sort_state.rs index 7187b05..c249879 100644 --- a/src/commands/satisfying_sort/sort_state.rs +++ b/src/commands/satisfying_sort/sort_state.rs @@ -42,21 +42,6 @@ impl SortState { self.source_array.len() } - pub(super) fn source_array(&self) -> &[usize] { - &self.source_array - } - - pub(super) const fn current_scan_index(&self) -> Option { - match self.phase { - Phase::Sorting { scan_start } => Some(scan_start), - Phase::Idle | Phase::Completion { .. } => None, - } - } - - pub(super) const fn scan_complete(&self) -> bool { - matches!(self.phase, Phase::Completion { .. }) - } - pub(super) const fn apply_sort_step(&mut self, index: usize) { self.phase = Phase::Sorting { scan_start: index }; } @@ -141,18 +126,6 @@ mod tests { assert_eq!(array, expected); } - #[test] - fn state_source_array_is_permutation_after_reset() { - let mut rng = SmallRng::seed_from_u64(13); - let mut state = SortState::new(128, &mut rng); - state.reset(&mut rng); - - let mut array = state.source_array().to_vec(); - array.sort_unstable(); - let expected: Vec = (1..=128).collect(); - assert_eq!(array, expected); - } - #[test] fn style_for_index_respects_active_window() { let mut rng = SmallRng::seed_from_u64(1); @@ -189,24 +162,4 @@ mod tests { ); assert_eq!(state.style_for_index_with_min_window(7, 0), BandStyle::Idle); } - - #[test] - fn phase_helpers_match_state_transitions() { - let mut rng = SmallRng::seed_from_u64(21); - let mut state = SortState::new(32, &mut rng); - assert_eq!(state.current_scan_index(), None); - assert!(!state.scan_complete()); - - state.apply_sort_step(4); - assert_eq!(state.current_scan_index(), Some(4)); - assert!(!state.scan_complete()); - - state.finalize_sort_pass(); - assert_eq!(state.current_scan_index(), None); - assert!(state.scan_complete()); - - state.reset(&mut rng); - assert_eq!(state.current_scan_index(), None); - assert!(!state.scan_complete()); - } }