feat: run input_system less, rework profiling system to allow for conditional ticks, prepopulate and simplify locking mechanisms, drop RwLock

src/game.rs

@@ -29,8 +29,9 @@ use bevy_ecs::event::EventRegistry;
 use bevy_ecs::observer::Trigger;
 use bevy_ecs::schedule::common_conditions::resource_changed;
 use bevy_ecs::schedule::{Condition, IntoScheduleConfigs, Schedule, SystemSet};
-use bevy_ecs::system::ResMut;
+use bevy_ecs::system::{Local, ResMut};
 use bevy_ecs::world::World;
+use glam::UVec2;
 use sdl2::event::EventType;
 use sdl2::image::LoadTexture;
 use sdl2::render::{BlendMode, Canvas, ScaleMode, TextureCreator};
@@ -42,7 +43,7 @@ use crate::{
     asset::{get_asset_bytes, Asset},
     events::GameCommand,
     map::render::MapRenderer,
-    systems::debug::TtfAtlasResource,
+    systems::debug::{BatchedLinesResource, TtfAtlasResource},
     systems::input::{Bindings, CursorPosition},
     texture::sprite::{AtlasMapper, SpriteAtlas},
 };
@@ -299,6 +300,10 @@ impl Game {
         EventRegistry::register_event::<GameEvent>(&mut world);
         EventRegistry::register_event::<AudioEvent>(&mut world);
 
+        let scale =
+            (UVec2::from(canvas.output_size().unwrap()).as_vec2() / UVec2::from(canvas.logical_size()).as_vec2()).min_element();
+
+        world.insert_resource(BatchedLinesResource::new(&map, scale));
         world.insert_resource(Self::create_ghost_animations(&atlas)?);
         world.insert_resource(map);
         world.insert_resource(GlobalState { exit: false });
@@ -360,7 +365,11 @@ impl Game {
         schedule.add_systems((
             forced_dirty_system.run_if(resource_changed::<ScoreResource>.or(resource_changed::<StartupSequence>)),
             (
-                input_system,
+                input_system.run_if(|mut local: Local<u8>| {
+                    *local = local.wrapping_add(1u8);
+                    // run every nth frame
+                    *local % 2 == 0
+                }),
                 player_control_system,
                 player_movement_system,
                 startup_stage_system,

src/systems/debug.rs

@@ -13,6 +13,7 @@ use sdl2::rect::{Point, Rect};
 use sdl2::render::{Canvas, Texture};
 use sdl2::video::Window;
 use smallvec::SmallVec;
+use std::collections::{HashMap, HashSet};
 use tracing::warn;
 
 #[derive(Resource, Default, Debug, Copy, Clone)]
@@ -30,6 +31,118 @@ pub struct DebugTextureResource(pub Texture);
 /// Resource to hold the TTF text atlas
 pub struct TtfAtlasResource(pub TtfAtlas);
 
+/// Resource to hold pre-computed batched line segments
+#[derive(Resource, Default, Debug, Clone)]
+pub struct BatchedLinesResource {
+    horizontal_lines: Vec<(i32, i32, i32)>, // (y, x_start, x_end)
+    vertical_lines: Vec<(i32, i32, i32)>,   // (x, y_start, y_end)
+}
+
+impl BatchedLinesResource {
+    /// Computes and caches batched line segments for the map graph
+    pub fn new(map: &Map, scale: f32) -> Self {
+        let mut horizontal_segments: HashMap<i32, Vec<(i32, i32)>> = HashMap::new();
+        let mut vertical_segments: HashMap<i32, Vec<(i32, i32)>> = HashMap::new();
+        let mut processed_edges: HashSet<(u16, u16)> = HashSet::new();
+
+        // Process all edges and group them by axis
+        for (start_node_id, edge) in map.graph.edges() {
+            // Acquire a stable key for the edge (from < to)
+            let edge_key = (start_node_id.min(edge.target), start_node_id.max(edge.target));
+
+            // Skip if we've already processed this edge in the reverse direction
+            if processed_edges.contains(&edge_key) {
+                continue;
+            }
+            processed_edges.insert(edge_key);
+
+            let start_pos = map.graph.get_node(start_node_id).unwrap().position;
+            let end_pos = map.graph.get_node(edge.target).unwrap().position;
+
+            let start = transform_position_with_offset(start_pos, scale);
+            let end = transform_position_with_offset(end_pos, scale);
+
+            // Determine if this is a horizontal or vertical line
+            if (start.y - end.y).abs() < 2 {
+                // Horizontal line (allowing for slight vertical variance)
+                let y = start.y;
+                let x_min = start.x.min(end.x);
+                let x_max = start.x.max(end.x);
+                horizontal_segments.entry(y).or_default().push((x_min, x_max));
+            } else if (start.x - end.x).abs() < 2 {
+                // Vertical line (allowing for slight horizontal variance)
+                let x = start.x;
+                let y_min = start.y.min(end.y);
+                let y_max = start.y.max(end.y);
+                vertical_segments.entry(x).or_default().push((y_min, y_max));
+            }
+        }
+
+        /// Merges overlapping or adjacent segments into continuous lines
+        fn merge_segments(segments: Vec<(i32, i32)>) -> Vec<(i32, i32)> {
+            if segments.is_empty() {
+                return Vec::new();
+            }
+
+            let mut merged = Vec::new();
+            let mut current_start = segments[0].0;
+            let mut current_end = segments[0].1;
+
+            for &(start, end) in segments.iter().skip(1) {
+                if start <= current_end + 1 {
+                    // Adjacent or overlapping
+                    current_end = current_end.max(end);
+                } else {
+                    merged.push((current_start, current_end));
+                    current_start = start;
+                    current_end = end;
+                }
+            }
+
+            merged.push((current_start, current_end));
+            merged
+        }
+
+        // Convert to flat vectors for fast iteration during rendering
+        let horizontal_lines = horizontal_segments
+            .into_iter()
+            .flat_map(|(y, mut segments)| {
+                segments.sort_unstable_by_key(|(start, _)| *start);
+                let merged = merge_segments(segments);
+                merged.into_iter().map(move |(x_start, x_end)| (y, x_start, x_end))
+            })
+            .collect::<Vec<_>>();
+
+        let vertical_lines = vertical_segments
+            .into_iter()
+            .flat_map(|(x, mut segments)| {
+                segments.sort_unstable_by_key(|(start, _)| *start);
+                let merged = merge_segments(segments);
+                merged.into_iter().map(move |(y_start, y_end)| (x, y_start, y_end))
+            })
+            .collect::<Vec<_>>();
+
+        Self {
+            horizontal_lines,
+            vertical_lines,
+        }
+    }
+
+    pub fn render(&self, canvas: &mut Canvas<Window>) {
+        // Render horizontal lines
+        for &(y, x_start, x_end) in &self.horizontal_lines {
+            let points = [Point::new(x_start, y), Point::new(x_end, y)];
+            let _ = canvas.draw_lines(&points[..]);
+        }
+
+        // Render vertical lines
+        for &(x, y_start, y_end) in &self.vertical_lines {
+            let points = [Point::new(x, y_start), Point::new(x, y_end)];
+            let _ = canvas.draw_lines(&points[..]);
+        }
+    }
+}
+
 /// Transforms a position from logical canvas coordinates to output canvas coordinates (with board offset)
 fn transform_position_with_offset(pos: Vec2, scale: f32) -> IVec2 {
     ((pos + BOARD_PIXEL_OFFSET.as_vec2()) * scale).as_ivec2()
@@ -93,6 +206,7 @@ pub fn debug_render_system(
     mut canvas: NonSendMut<&mut Canvas<Window>>,
     mut debug_texture: NonSendMut<DebugTextureResource>,
     mut ttf_atlas: NonSendMut<TtfAtlasResource>,
+    batched_lines: Res<BatchedLinesResource>,
     debug_state: Res<DebugState>,
     timings: Res<SystemTimings>,
     map: Res<Map>,
@@ -157,22 +271,13 @@ pub fn debug_render_system(
             }
 
             debug_canvas.set_draw_color(Color {
-                a: f32_to_u8(0.4),
+                a: f32_to_u8(0.6),
                 ..Color::RED
             });
             debug_canvas.set_blend_mode(sdl2::render::BlendMode::Blend);
-            for (start_node, end_node) in map.graph.edges() {
-                let start_node_model = map.graph.get_node(start_node).unwrap();
-                let end_node = map.graph.get_node(end_node.target).unwrap().position;
 
-                // Transform positions using common method
+            // Use cached batched line segments
-                let start = transform_position_with_offset(start_node_model.position, scale);
+            batched_lines.render(debug_canvas);
-                let end = transform_position_with_offset(end_node, scale);
-
-                debug_canvas
-                    .draw_line(Point::from((start.x, start.y)), Point::from((end.x, end.y)))
-                    .unwrap();
-            }
 
             {
                 let rects: Vec<_> = map

src/systems/profiling.rs

@@ -3,7 +3,7 @@ use bevy_ecs::{resource::Resource, system::System};
 use circular_buffer::CircularBuffer;
 use micromap::Map;
 use num_width::NumberWidth;
-use parking_lot::{Mutex, RwLock};
+use parking_lot::Mutex;
 use smallvec::SmallVec;
 use std::fmt::Display;
 use std::time::Duration;
@@ -46,7 +46,7 @@ impl Display for SystemId {
     }
 }
 
-#[derive(Resource, Default, Debug)]
+#[derive(Resource, Debug)]
 pub struct SystemTimings {
     /// Map of system names to a queue of durations, using a circular buffer.
     ///
@@ -55,42 +55,54 @@ pub struct SystemTimings {
     ///
     /// Also, we use a micromap::Map as the number of systems is generally quite small.
     /// Just make sure to set the capacity appropriately, or it will panic.
-    pub timings: RwLock<Map<SystemId, Mutex<CircularBuffer<TIMING_WINDOW_SIZE, Duration>>, MAX_SYSTEMS>>,
+    ///
+    /// Pre-populated with all SystemId variants during initialization to avoid runtime allocations
+    /// and allow systems to have default zero timings when they don't submit data.
+    pub timings: Map<SystemId, Mutex<CircularBuffer<TIMING_WINDOW_SIZE, Duration>>, MAX_SYSTEMS>,
+}
+
+impl Default for SystemTimings {
+    fn default() -> Self {
+        let mut timings = Map::new();
+
+        // Pre-populate with all SystemId variants to avoid runtime allocations
+        // and provide default zero timings for systems that don't submit data
+        for id in SystemId::iter() {
+            timings.insert(id, Mutex::new(CircularBuffer::new()));
+        }
+
+        Self { timings }
+    }
 }
 
 impl SystemTimings {
     pub fn add_timing(&self, id: SystemId, duration: Duration) {
-        // acquire a upgradable read lock
+        // Since all SystemId variants are pre-populated, we can use a simple read lock
-        let mut timings = self.timings.upgradable_read();
+        let queue = self
-
+            .timings
-        // happy path, the name is already in the map (no need to mutate the hashmap)
+            .get(&id)
-        if timings.contains_key(&id) {
+            .expect("SystemId not found in pre-populated map - this is a bug");
-            let queue = timings
+        queue.lock().push_back(duration);
-                .get(&id)
-                .expect("System name not found in map after contains_key check");
-            let mut queue = queue.lock();
-
-            queue.push_back(duration);
-            return;
-        }
-
-        // otherwise, acquire a write lock and insert a new queue
-        timings.with_upgraded(|timings| {
-            let queue = timings.entry(id).or_insert_with(|| Mutex::new(CircularBuffer::new()));
-            queue.lock().push_back(duration);
-        });
     }
 
     pub fn get_stats(&self) -> Map<SystemId, (Duration, Duration), MAX_SYSTEMS> {
-        let timings = self.timings.read();
         let mut stats = Map::new();
 
-        for (id, queue) in timings.iter() {
+        // Iterate over all SystemId variants to ensure every system has an entry
-            if queue.lock().is_empty() {
+        for id in SystemId::iter() {
+            let queue = self
+                .timings
+                .get(&id)
+                .expect("SystemId not found in pre-populated map - this is a bug");
+
+            let queue_guard = queue.lock();
+            if queue_guard.is_empty() {
+                // Return zero timing for systems that haven't submitted any data
+                stats.insert(id, (Duration::ZERO, Duration::ZERO));
                 continue;
             }
 
-            let durations: Vec<f64> = queue.lock().iter().map(|d| d.as_secs_f64() * 1000.0).collect();
+            let durations: Vec<f64> = queue_guard.iter().map(|d| d.as_secs_f64() * 1000.0).collect();
             let count = durations.len() as f64;
 
             let sum: f64 = durations.iter().sum();
@@ -100,7 +112,7 @@ impl SystemTimings {
             let std_dev = variance.sqrt();
 
             stats.insert(
-                *id,
+                id,
                 (
                     Duration::from_secs_f64(mean / 1000.0),
                     Duration::from_secs_f64(std_dev / 1000.0),
@@ -113,8 +125,7 @@ impl SystemTimings {
 
     pub fn get_total_stats(&self) -> (Duration, Duration) {
         let duration_sums = {
-            let timings = self.timings.read();
+            self.timings
-            timings
                 .iter()
                 .map(|(_, queue)| queue.lock().iter().sum::<Duration>())
                 .collect::<Vec<_>>()

tests/profiling.rs

@@ -1,5 +1,22 @@
 use pacman::systems::profiling::{SystemId, SystemTimings};
 use std::time::Duration;
+use strum::IntoEnumIterator;
+
+macro_rules! assert_close {
+    ($actual:expr, $expected:expr, $concern:expr) => {
+        let tolerance = Duration::from_micros(500);
+        let diff = $actual.abs_diff($expected);
+        assert!(
+            diff < tolerance,
+            "Expected {expected:?} ± {tolerance:.0?}, got {actual:?}, off by {diff:?} ({concern})",
+            concern = $concern,
+            expected = $expected,
+            actual = $actual,
+            tolerance = tolerance,
+            diff = diff
+        );
+    };
+}
 
 #[test]
 fn test_timing_statistics() {
@@ -15,30 +32,79 @@ fn test_timing_statistics() {
     timings.add_timing(SystemId::Blinking, Duration::from_millis(2));
     timings.add_timing(SystemId::Blinking, Duration::from_millis(1));
 
-    fn close_enough(a: Duration, b: Duration) -> bool {
+    {
-        if a > b {
+        let stats = timings.get_stats();
-            a - b < Duration::from_micros(500) // 0.1ms
+        let (avg, std_dev) = stats.get(&SystemId::PlayerControls).unwrap();
-        } else {
+
-            b - a < Duration::from_micros(500)
+        assert_close!(*avg, Duration::from_millis(10), "PlayerControls average timing");
-        }
+        assert_close!(*std_dev, Duration::from_millis(2), "PlayerControls standard deviation timing");
     }
 
-    let stats = timings.get_stats();
+    {
-    let (avg, std_dev) = stats.get(&SystemId::PlayerControls).unwrap();
+        let (total_avg, total_std) = timings.get_total_stats();
-
+        assert_close!(total_avg, Duration::from_millis(2), "Total average timing across all systems");
-    // Average should be 10ms, standard deviation should be small
+        assert_close!(
-    assert!(close_enough(*avg, Duration::from_millis(10)), "avg: {:?}", avg);
+            total_std,
-    assert!(close_enough(*std_dev, Duration::from_millis(2)), "std_dev: {:?}", std_dev);
+            Duration::from_millis(7),
-
+            "Total standard deviation timing across all systems"
-    let (total_avg, total_std) = timings.get_total_stats();
+        );
-    assert!(
+    }
-        close_enough(total_avg, Duration::from_millis(18)),
+}
-        "total_avg: {:?}",
+
-        total_avg
+#[test]
-    );
+fn test_default_zero_timing_for_unused_systems() {
-    assert!(
+    let timings = SystemTimings::default();
-        close_enough(total_std, Duration::from_millis(17)),
+
-        "total_std: {:?}",
+    // Add timing data for only one system
-        total_std
+    timings.add_timing(SystemId::PlayerControls, Duration::from_millis(5));
-    );
+
+    let stats = timings.get_stats();
+
+    // Verify all SystemId variants are present in the stats
+    let expected_count = SystemId::iter().count();
+    assert_eq!(stats.len(), expected_count, "All SystemId variants should be in stats");
+
+    // Verify that the system with data has non-zero timing
+    let (avg, std_dev) = stats.get(&SystemId::PlayerControls).unwrap();
+    assert_close!(*avg, Duration::from_millis(5), "System with data should have correct timing");
+    assert_close!(*std_dev, Duration::ZERO, "Single measurement should have zero std dev");
+
+    // Verify that all other systems have zero timing
+    for id in SystemId::iter() {
+        if id != SystemId::PlayerControls {
+            let (avg, std_dev) = stats.get(&id).unwrap();
+            assert_close!(
+                *avg,
+                Duration::ZERO,
+                format!("Unused system {:?} should have zero avg timing", id)
+            );
+            assert_close!(
+                *std_dev,
+                Duration::ZERO,
+                format!("Unused system {:?} should have zero std dev", id)
+            );
+        }
+    }
+}
+
+#[test]
+fn test_pre_populated_timing_entries() {
+    let timings = SystemTimings::default();
+
+    // Verify that we can add timing to any SystemId without panicking
+    // (this would fail with the old implementation if the entry didn't exist)
+    for id in SystemId::iter() {
+        timings.add_timing(id, Duration::from_nanos(1));
+    }
+
+    // Verify all systems now have non-zero timing
+    let stats = timings.get_stats();
+    for id in SystemId::iter() {
+        let (avg, _) = stats.get(&id).unwrap();
+        assert!(
+            *avg > Duration::ZERO,
+            "System {:?} should have non-zero timing after add_timing",
+            id
+        );
+    }
 }