From 65a5ba99c01812a45afd28d8e3777f6b4a26ae03 Mon Sep 17 00:00:00 2001 From: Gabriel Kaszewski Date: Sat, 7 Mar 2026 00:21:44 +0100 Subject: [PATCH] Apply code review improvements across all modules Bugs fixed: - Remove duplicate/corrupt draw_text in draw_active_effects_hud (screen_width as u8 = 0) - Fix pickup-enemy avoidance using actual enemy center-x instead of platform midpoint - Remove redundant `as f32` cast on an already-f32 expression DRY / data coupling: - Effect label and color now stored in each effect struct and passed from PickupDef::create_effect, eliminating the three-way duplication between Config, PickupEffectType, and ActiveEffect impls Config-driven level gen: - Add enemy_platform_margin and pickup_platform_margin fields to Config - Replace magic number literals in level_gen with cfg fields Encapsulation and code clarity: - score_f made fully private; tests assert on the public score: u64 - Pickup::collected flag removed; collect_pickups uses retain for immediate removal - Platform::top() and Enemy::top() removed (transparent aliases for .y); call sites use .y directly - Game-over restart prompt now blinks at ~2 Hz using world.elapsed Rust idioms: - #[derive(Debug)] added to Player, Platform, Enemy, Pickup, PickupDef, PickupEffectType, Config, GameState, and all three effect structs - [profile.release] added to Cargo.toml (lto=thin, codegen-units=1, panic=abort) Co-Authored-By: Claude Sonnet 4.6 --- Cargo.toml | 6 +++++ src/config.rs | 8 ++++-- src/effects.rs | 56 +++++++++++++++++++++++++---------------- src/enemy.rs | 6 +---- src/game.rs | 9 ++++--- src/level_gen.rs | 22 ++++++++-------- src/pickup.rs | 14 +++++------ src/platform.rs | 9 ++----- src/player.rs | 1 + src/world.rs | 65 +++++++++++++++++++++--------------------------- 10 files changed, 102 insertions(+), 94 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 438e6ea..5028925 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -6,3 +6,9 @@ edition = "2024" [dependencies] rand = "0.10.0" raylib = { version = "5.5.1", features = ["wayland"] } + +[profile.release] +opt-level = 3 +lto = "thin" +codegen-units = 1 +panic = "abort" diff --git a/src/config.rs b/src/config.rs index 667c919..ed5cf9c 100644 --- a/src/config.rs +++ b/src/config.rs @@ -31,7 +31,7 @@ mod tests { /// All game parameters in one place — change here to tune the game. /// Pickup types are also registered here; add a new `PickupDef` entry to extend. -#[derive(Clone)] +#[derive(Clone, Debug)] pub struct Config { pub screen_width: i32, pub screen_height: i32, @@ -68,11 +68,13 @@ pub struct Config { pub enemy_height: f32, pub enemy_spawn_chance: f32, pub enemy_min_platform_width: f32, + pub enemy_platform_margin: f32, // Pickups pub pickup_size: f32, pub pickup_spawn_chance: f32, - pub pickup_hover_gap: f32, // pixels above platform surface + pub pickup_hover_gap: f32, // pixels above platform surface + pub pickup_platform_margin: f32, // horizontal inset from platform edge pub pickup_defs: Vec, // Scoring @@ -122,10 +124,12 @@ impl Default for Config { enemy_height: 40.0, enemy_spawn_chance: 0.35, enemy_min_platform_width: 120.0, + enemy_platform_margin: 15.0, pickup_size: 28.0, pickup_spawn_chance: 0.28, pickup_hover_gap: 12.0, + pickup_platform_margin: 20.0, // ── Register new pickup types here ──────────────────────────────── pickup_defs: vec![ diff --git a/src/effects.rs b/src/effects.rs index fb44bcf..4202713 100644 --- a/src/effects.rs +++ b/src/effects.rs @@ -27,14 +27,17 @@ pub trait ActiveEffect { // ── Concrete effects ────────────────────────────────────────────────────────── +#[derive(Debug)] pub struct InvulnerabilityEffect { timer: f32, total: f32, + label: &'static str, + color: (u8, u8, u8), } impl InvulnerabilityEffect { - pub fn new(duration: f32) -> Self { - Self { timer: duration, total: duration } + pub fn new(duration: f32, label: &'static str, color: (u8, u8, u8)) -> Self { + Self { timer: duration, total: duration, label, color } } } @@ -44,10 +47,11 @@ impl ActiveEffect for InvulnerabilityEffect { self.timer > 0.0 } fn label(&self) -> &'static str { - "INVINCIBLE" + self.label } fn tint(&self) -> Color { - Color::new(255, 200, 50, 255) + let (r, g, b) = self.color; + Color::new(r, g, b, 255) } fn progress(&self) -> f32 { (self.timer / self.total).max(0.0) @@ -59,15 +63,18 @@ impl ActiveEffect for InvulnerabilityEffect { // ───────────────────────────────────────────────────────────────────────────── +#[derive(Debug)] pub struct JumpBoostEffect { timer: f32, total: f32, factor: f32, + label: &'static str, + color: (u8, u8, u8), } impl JumpBoostEffect { - pub fn new(duration: f32, factor: f32) -> Self { - Self { timer: duration, total: duration, factor } + pub fn new(duration: f32, factor: f32, label: &'static str, color: (u8, u8, u8)) -> Self { + Self { timer: duration, total: duration, factor, label, color } } } @@ -77,10 +84,11 @@ impl ActiveEffect for JumpBoostEffect { self.timer > 0.0 } fn label(&self) -> &'static str { - "JUMP BOOST" + self.label } fn tint(&self) -> Color { - Color::new(80, 180, 255, 255) + let (r, g, b) = self.color; + Color::new(r, g, b, 255) } fn progress(&self) -> f32 { (self.timer / self.total).max(0.0) @@ -92,15 +100,18 @@ impl ActiveEffect for JumpBoostEffect { // ───────────────────────────────────────────────────────────────────────────── +#[derive(Debug)] pub struct ScoreMultiplierEffect { timer: f32, total: f32, factor: f32, + label: &'static str, + color: (u8, u8, u8), } impl ScoreMultiplierEffect { - pub fn new(duration: f32, factor: f32) -> Self { - Self { timer: duration, total: duration, factor } + pub fn new(duration: f32, factor: f32, label: &'static str, color: (u8, u8, u8)) -> Self { + Self { timer: duration, total: duration, factor, label, color } } } @@ -110,10 +121,11 @@ impl ActiveEffect for ScoreMultiplierEffect { self.timer > 0.0 } fn label(&self) -> &'static str { - "2X SCORE" + self.label } fn tint(&self) -> Color { - Color::new(180, 80, 255, 255) + let (r, g, b) = self.color; + Color::new(r, g, b, 255) } fn progress(&self) -> f32 { (self.timer / self.total).max(0.0) @@ -129,63 +141,63 @@ mod tests { #[test] fn invulnerability_active_while_timer_positive() { - let mut e = InvulnerabilityEffect::new(1.0); + let mut e = InvulnerabilityEffect::new(1.0, "INVINCIBLE", (255, 200, 50)); assert!(e.update(0.5)); } #[test] fn invulnerability_expires_when_timer_reaches_zero() { - let mut e = InvulnerabilityEffect::new(1.0); + let mut e = InvulnerabilityEffect::new(1.0, "INVINCIBLE", (255, 200, 50)); assert!(!e.update(1.1)); } #[test] fn invulnerability_is_invulnerable() { - let e = InvulnerabilityEffect::new(5.0); + let e = InvulnerabilityEffect::new(5.0, "INVINCIBLE", (255, 200, 50)); assert!(e.is_invulnerable()); } #[test] fn invulnerability_default_multipliers_are_one() { - let e = InvulnerabilityEffect::new(5.0); + let e = InvulnerabilityEffect::new(5.0, "INVINCIBLE", (255, 200, 50)); assert_eq!(e.jump_multiplier(), 1.0); assert_eq!(e.score_multiplier(), 1.0); } #[test] fn jump_boost_returns_configured_factor() { - let e = JumpBoostEffect::new(5.0, 1.5); + let e = JumpBoostEffect::new(5.0, 1.5, "JUMP BOOST", (80, 180, 255)); assert_eq!(e.jump_multiplier(), 1.5); } #[test] fn jump_boost_is_not_invulnerable() { - let e = JumpBoostEffect::new(5.0, 1.5); + let e = JumpBoostEffect::new(5.0, 1.5, "JUMP BOOST", (80, 180, 255)); assert!(!e.is_invulnerable()); } #[test] fn score_multiplier_returns_configured_factor() { - let e = ScoreMultiplierEffect::new(5.0, 2.0); + let e = ScoreMultiplierEffect::new(5.0, 2.0, "2X SCORE", (180, 80, 255)); assert_eq!(e.score_multiplier(), 2.0); } #[test] fn progress_starts_at_one() { - let e = InvulnerabilityEffect::new(5.0); + let e = InvulnerabilityEffect::new(5.0, "INVINCIBLE", (255, 200, 50)); assert!((e.progress() - 1.0).abs() < 1e-4); } #[test] fn progress_halves_at_half_duration() { - let mut e = InvulnerabilityEffect::new(4.0); + let mut e = InvulnerabilityEffect::new(4.0, "INVINCIBLE", (255, 200, 50)); e.update(2.0); assert!((e.progress() - 0.5).abs() < 1e-4); } #[test] fn progress_clamps_to_zero_when_expired() { - let mut e = InvulnerabilityEffect::new(1.0); + let mut e = InvulnerabilityEffect::new(1.0, "INVINCIBLE", (255, 200, 50)); e.update(10.0); assert_eq!(e.progress(), 0.0); } diff --git a/src/enemy.rs b/src/enemy.rs index 5a775ba..c0e2ae7 100644 --- a/src/enemy.rs +++ b/src/enemy.rs @@ -1,4 +1,4 @@ -#[derive(Clone)] +#[derive(Clone, Debug)] pub struct Enemy { pub x: f32, pub y: f32, @@ -20,10 +20,6 @@ impl Enemy { self.x + self.width < 0.0 } - pub fn top(&self) -> f32 { - self.y - } - pub fn bottom(&self) -> f32 { self.y + self.height } diff --git a/src/game.rs b/src/game.rs index a2a8a88..ca8391d 100644 --- a/src/game.rs +++ b/src/game.rs @@ -2,6 +2,7 @@ use raylib::prelude::*; use crate::{config::Config, world::World}; +#[derive(Debug)] pub enum GameState { Playing, GameOver, @@ -84,7 +85,7 @@ impl Game { d.draw_rectangle(bar_x, bar_y, fill, bar_h, Color::new(80, 180, 255, 255)); // Active effect timer bars - self.world.draw_active_effects_hud(d, &self.cfg); + self.world.draw_active_effects_hud(d); } fn draw_game_over(&self, d: &mut RaylibDrawHandle) { @@ -133,8 +134,10 @@ impl Game { let prompt = "SPACE / R / ENTER to restart"; let ps = 22; let pw = d.measure_text(prompt, ps); - // Blinking effect based on elapsed time (we don't have time here, so just static) - d.draw_text(prompt, (sw - pw) / 2, panel_y + 240, ps, Color::new(160, 200, 255, 255)); + // Blink at ~2 Hz using the world's accumulated elapsed time. + if (self.world.elapsed * 2.0).sin() > 0.0 { + d.draw_text(prompt, (sw - pw) / 2, panel_y + 240, ps, Color::new(160, 200, 255, 255)); + } let ctrl = "SPACE / W / UP to jump"; let cw = d.measure_text(ctrl, 18); diff --git a/src/level_gen.rs b/src/level_gen.rs index 83390da..a66ea36 100644 --- a/src/level_gen.rs +++ b/src/level_gen.rs @@ -65,36 +65,34 @@ impl LevelGenerator { platforms.push(Platform::new(x, y, width, cfg.platform_height)); - let mut has_enemy = false; + // enemy_cx holds the center-x of a spawned enemy, used for pickup avoidance. + let mut enemy_cx: Option = None; if width >= cfg.enemy_min_platform_width && self.rng.random::() < cfg.enemy_spawn_chance { - let margin = 15.0; - let max_offset = width - cfg.enemy_width - margin * 2.0; + let max_offset = width - cfg.enemy_width - cfg.enemy_platform_margin * 2.0; if max_offset > 0.0 { - let ex = x + margin + self.rng.random_range(0.0..max_offset); + let ex = x + cfg.enemy_platform_margin + self.rng.random_range(0.0..max_offset); let ey = y - cfg.enemy_height; enemies.push(Enemy::new(ex, ey, cfg.enemy_width, cfg.enemy_height)); - has_enemy = true; + enemy_cx = Some(ex + cfg.enemy_width * 0.5); } } if !cfg.pickup_defs.is_empty() && self.rng.random::() < cfg.pickup_spawn_chance { let def_index = self.rng.random_range(0..cfg.pickup_defs.len()); - let margin = 20.0; - let px_min = x + margin; - let px_max = x + width - cfg.pickup_size - margin; + let px_min = x + cfg.pickup_platform_margin; + let px_max = x + width - cfg.pickup_size - cfg.pickup_platform_margin; if px_max > px_min { let mut px: f32 = self.rng.random_range(px_min..px_max); - if has_enemy { - let enemy_cx = x + width * 0.5; - if (px - enemy_cx).abs() < cfg.enemy_width + cfg.pickup_size { + if let Some(ecx) = enemy_cx { + if (px - ecx).abs() < cfg.enemy_width + cfg.pickup_size { px = (px + cfg.enemy_width + cfg.pickup_size) - .min(x + width - cfg.pickup_size - margin); + .min(x + width - cfg.pickup_size - cfg.pickup_platform_margin); } } diff --git a/src/pickup.rs b/src/pickup.rs index 6c0b22d..65e1b50 100644 --- a/src/pickup.rs +++ b/src/pickup.rs @@ -2,7 +2,7 @@ use crate::effects::{ActiveEffect, InvulnerabilityEffect, JumpBoostEffect, Score /// Which gameplay effect this pickup grants. /// Add a new variant here (and a matching `ActiveEffect` impl) to extend. -#[derive(Clone)] +#[derive(Clone, Debug)] pub enum PickupEffectType { Invulnerability, JumpBoost { factor: f32 }, @@ -10,7 +10,7 @@ pub enum PickupEffectType { } /// Data-driven definition of a pickup type — lives entirely in `Config`. -#[derive(Clone)] +#[derive(Clone, Debug)] pub struct PickupDef { pub label: &'static str, /// RGB colour used for the pickup icon and effect HUD bar. @@ -24,13 +24,13 @@ impl PickupDef { pub fn create_effect(&self) -> Box { match self.effect { PickupEffectType::Invulnerability => { - Box::new(InvulnerabilityEffect::new(self.duration)) + Box::new(InvulnerabilityEffect::new(self.duration, self.label, self.color)) } PickupEffectType::JumpBoost { factor } => { - Box::new(JumpBoostEffect::new(self.duration, factor)) + Box::new(JumpBoostEffect::new(self.duration, factor, self.label, self.color)) } PickupEffectType::ScoreMultiplier { factor } => { - Box::new(ScoreMultiplierEffect::new(self.duration, factor)) + Box::new(ScoreMultiplierEffect::new(self.duration, factor, self.label, self.color)) } } } @@ -38,6 +38,7 @@ impl PickupDef { // ── In-world pickup instance ────────────────────────────────────────────────── +#[derive(Debug)] pub struct Pickup { pub x: f32, pub y: f32, @@ -45,12 +46,11 @@ pub struct Pickup { pub height: f32, /// Index into `Config::pickup_defs`. pub def_index: usize, - pub collected: bool, } impl Pickup { pub fn new(x: f32, y: f32, size: f32, def_index: usize) -> Self { - Self { x, y, width: size, height: size, def_index, collected: false } + Self { x, y, width: size, height: size, def_index } } pub fn scroll(&mut self, speed: f32, dt: f32) { diff --git a/src/platform.rs b/src/platform.rs index 7c66d0c..9c24232 100644 --- a/src/platform.rs +++ b/src/platform.rs @@ -1,4 +1,4 @@ -#[derive(Clone)] +#[derive(Clone, Debug)] pub struct Platform { pub x: f32, pub y: f32, @@ -19,10 +19,6 @@ impl Platform { self.x + self.width < 0.0 } - pub fn top(&self) -> f32 { - self.y - } - pub fn right(&self) -> f32 { self.x + self.width } @@ -52,9 +48,8 @@ mod tests { } #[test] - fn top_and_right_helpers() { + fn right_helper() { let p = Platform::new(50.0, 300.0, 200.0, 20.0); - assert_eq!(p.top(), 300.0); assert_eq!(p.right(), 250.0); } } diff --git a/src/player.rs b/src/player.rs index e497b53..c1a7711 100644 --- a/src/player.rs +++ b/src/player.rs @@ -107,6 +107,7 @@ mod tests { } } +#[derive(Debug)] pub struct Player { pub x: f32, pub y: f32, diff --git a/src/world.rs b/src/world.rs index 5dd09c7..51afd80 100644 --- a/src/world.rs +++ b/src/world.rs @@ -123,14 +123,14 @@ mod tests { w.player.vy = 250.0; w.player.on_ground = false; - let score_before = w.score_f; + let score_before = w.score; w.update(0.016, false, &cfg); // Dead enemies are removed by retain() after stomp. assert!(w.enemies.is_empty(), "dead enemy should be removed from world"); assert!(w.player.alive, "player should survive stomp"); assert!(w.player.on_ground, "player should land on enemy"); - assert!(w.score_f > score_before, "stomp should award score"); + assert!(w.score > score_before, "stomp should award score"); } #[test] @@ -155,7 +155,7 @@ mod tests { fn invulnerability_prevents_side_death() { let cfg = cfg(); let mut w = world(&cfg); - w.active_effects.push(Box::new(InvulnerabilityEffect::new(5.0))); + w.active_effects.push(Box::new(InvulnerabilityEffect::new(5.0, "INVINCIBLE", (255, 200, 50)))); let enemy_y = w.player.y; w.enemies.push(Enemy::new( @@ -206,9 +206,9 @@ mod tests { fn score_increases_over_time() { let cfg = cfg(); let mut w = world(&cfg); - let score_before = w.score_f; + let score_before = w.score; w.update(1.0, false, &cfg); - assert!(w.score_f > score_before); + assert!(w.score > score_before); } #[test] @@ -217,12 +217,12 @@ mod tests { let mut w1 = world(&cfg); let mut w2 = world(&cfg); - w2.active_effects.push(Box::new(ScoreMultiplierEffect::new(10.0, 2.0))); + w2.active_effects.push(Box::new(ScoreMultiplierEffect::new(10.0, 2.0, "2X SCORE", (180, 80, 255)))); w1.update(1.0, false, &cfg); w2.update(1.0, false, &cfg); - let ratio = w2.score_f / w1.score_f; + let ratio = w2.score as f64 / w1.score as f64; assert!((ratio - 2.0).abs() < 0.05, "ratio was {ratio:.3}, expected ~2.0"); } @@ -232,7 +232,7 @@ mod tests { fn expired_effects_are_removed() { let cfg = cfg(); let mut w = world(&cfg); - w.active_effects.push(Box::new(InvulnerabilityEffect::new(0.01))); + w.active_effects.push(Box::new(InvulnerabilityEffect::new(0.01, "INVINCIBLE", (255, 200, 50)))); w.update(1.0, false, &cfg); // well past expiry @@ -248,7 +248,7 @@ pub struct World { pub active_effects: Vec>, pub score: u64, pub elapsed: f32, - pub(crate) score_f: f32, + score_f: f32, level_gen: LevelGenerator, } @@ -316,7 +316,7 @@ impl World { // 6. Recycle off-screen objects self.platforms.retain(|p| !p.is_off_screen()); self.enemies.retain(|e| !e.is_off_screen() && e.alive); - self.pickups.retain(|pk| !pk.is_off_screen() && !pk.collected); + self.pickups.retain(|pk| !pk.is_off_screen()); // 7. Generate new content ahead self.level_gen.generate_if_needed( @@ -367,10 +367,10 @@ impl World { // Top-landing: bottom crossed platform surface this frame while falling. if self.player.vy >= 0.0 - && prev_bottom <= platform.top() + 1.0 - && curr_bottom >= platform.top() + && prev_bottom <= platform.y + 1.0 + && curr_bottom >= platform.y { - self.player.y = platform.top() - ph; + self.player.y = platform.y - ph; self.player.vy = 0.0; self.player.on_ground = true; break; @@ -400,19 +400,19 @@ impl World { continue; } - let overlaps_y = curr_bottom > enemy.top() && player_top < enemy.bottom(); + let overlaps_y = curr_bottom > enemy.y && player_top < enemy.bottom(); if !overlaps_y { continue; } // Stomp: player was above enemy top and crosses it while falling. let stomp = self.player.vy > 0.0 - && prev_bottom <= enemy.top() + 8.0 - && curr_bottom >= enemy.top(); + && prev_bottom <= enemy.y + 8.0 + && curr_bottom >= enemy.y; if stomp { enemy.alive = false; - self.player.y = enemy.top() - self.player.height; + self.player.y = enemy.y - self.player.height; self.player.vy = 0.0; self.player.on_ground = true; self.score_f += cfg.enemy_stomp_bonus * score_mult; @@ -432,21 +432,20 @@ impl World { let px = self.player.x; let curr_bottom = self.player.bottom(); let player_top = self.player.y; + let player_right = self.player.right(); - for pickup in &mut self.pickups { - if pickup.collected { - continue; - } - - let overlaps_x = self.player.right() > pickup.x && px < pickup.right(); + let mut new_effects: Vec> = Vec::new(); + self.pickups.retain(|pickup| { + let overlaps_x = player_right > pickup.x && px < pickup.right(); let overlaps_y = curr_bottom > pickup.y && player_top < pickup.bottom(); - if overlaps_x && overlaps_y { - pickup.collected = true; - let effect = cfg.pickup_defs[pickup.def_index].create_effect(); - self.active_effects.push(effect); + new_effects.push(cfg.pickup_defs[pickup.def_index].create_effect()); + false // remove collected pickup immediately + } else { + true } - } + }); + self.active_effects.extend(new_effects); } // ── Rendering ───────────────────────────────────────────────────────────── @@ -465,7 +464,7 @@ impl World { } /// Draw effect timer bars; called from game HUD so it layers above the world. - pub fn draw_active_effects_hud(&self, d: &mut RaylibDrawHandle, cfg: &Config) { + pub fn draw_active_effects_hud(&self, d: &mut RaylibDrawHandle) { let bar_w = 180; let bar_h = 14; let x0 = 16; @@ -484,8 +483,6 @@ impl World { d.draw_rectangle_lines(x0, y, bar_w, bar_h, Color::new(r, g, b, 160)); // Label let lbl = effect.label(); - d.draw_text(lbl, x0 + 4, y + 1, 11, - Color::new(cfg.screen_width as u8, 255, 255, 255)); // reuse white d.draw_text(lbl, x0 + 4, y + 1, 11, Color::WHITE); y += bar_h + 4; @@ -509,10 +506,6 @@ impl World { let bob = (self.elapsed * 3.0).sin() * 3.0; for pickup in &self.pickups { - if pickup.collected { - continue; - } - let def = &cfg.pickup_defs[pickup.def_index]; let (r, g, b) = def.color; let x = pickup.x as i32; @@ -586,7 +579,7 @@ impl World { // Invulnerability pulsing outline if self.is_invulnerable() { - let pulse = ((self.elapsed * 8.0).sin() * 0.5 + 0.5) as f32; + let pulse = (self.elapsed * 8.0).sin() * 0.5 + 0.5; let alpha = (150.0 + pulse * 105.0) as u8; d.draw_rectangle(x - 3, y - 3, w + 6, h + 6, Color::new(255, 200, 50, alpha)); }