Game Mechanics#55
Conversation
- Introduced the `EnemyAIPlugin` to manage enemy behavior, including detection and attack mechanics. - Updated enemy attributes in the `EnemyAI` struct to include `detection_range`, `last_attack_time`, and `attack_cooldown` for improved gameplay dynamics. - Enhanced enemy movement logic to allow for attacking when within range and to stop moving when close to the player, improving responsiveness. - Added enemy spawning logic in the gameplay scene, ensuring enemies are positioned correctly and integrated with the new AI system.
- Added new properties to the `EnemyAI` struct for path memory, maximum path memory, current target index, stuck time, and stuck detection thresholds. - Implemented a `record_player_path` system to track player positions for improved enemy navigation. - Updated enemy movement logic to utilize path memory for following the player intelligently and to handle stuck detection, allowing enemies to recalculate paths when necessary. - Enhanced collision handling during movement to avoid obstacles and improve overall enemy behavior.
- Introduced `NavigationData`, `NavPoint`, and `NavStatistics` structs to manage navigation points and statistics. - Implemented pathfinding methods in `NavigationData` for finding the nearest navigation point and calculating paths between points. - Updated `enemy_ai` system to utilize the new navigation data for improved enemy movement and pathfinding capabilities. - Enhanced enemy AI logic to recalculate paths based on player movement, improving responsiveness and navigation accuracy.
- Introduced `NavigationData`, `NavPoint`, and `NavStatistics` structs to manage navigation points and statistics for improved pathfinding. - Implemented methods in `NavigationData` for finding the nearest navigation point and calculating paths between points. - Updated the `enemy_ai` system to utilize the new navigation data for enhanced enemy movement and pathfinding capabilities. - Integrated navigation loading system to initialize navigation data at startup, improving overall gameplay dynamics.
- Updated the `move_speed` of the `EnemyAI` struct to 4.5 for better balance with player speed. - Enhanced close-range behavior logic to include smooth transitions when approaching the player, allowing enemies to back up slightly if too close. - Introduced a distance factor to control enemy movement speed based on proximity to the player, improving overall responsiveness and gameplay dynamics.
- Implemented height difference checks to clear recorded player positions when the player is significantly above or below the enemy. - Updated position recording logic to consider both horizontal distance and height difference, improving enemy tracking accuracy. - Adjusted targeting behavior to prioritize direct player targeting when height differences exceed a threshold, enhancing enemy responsiveness in varied terrain.
- Removed unused properties from the `EnemyAI` struct to streamline the code. - Enhanced position recording logic to clear recorded positions when the enemy is close enough to chase the player directly. - Improved path following behavior by adjusting how the enemy targets recorded player positions based on distance. - Updated movement calculations to ensure smoother transitions and better responsiveness when approaching the player. - Implemented boundary checks to prevent enemies from moving outside defined limits, enhancing gameplay dynamics.
- Updated enemy spawning logic to position enemies behind the player for strategic gameplay. - Renamed `EnemyAI` to `PathFollower` and streamlined its properties for better clarity and efficiency. - Enhanced player position recording to allow enemies to follow the player's path more effectively, with improved memory management. - Adjusted movement logic to ensure enemies smoothly transition towards recorded positions while maintaining responsiveness to player proximity. - Updated animation handling to reflect movement states accurately, enhancing overall enemy behavior.
- Updated the spawning functions for coins to use `Sensor` instead of `RigidBody::Kinematic`, allowing coins to detect collisions without being solid. - Enabled collision events for coins to enhance interaction with other game elements. - Cleaned up redundant code in the spawning functions for better readability and maintainability.
- Introduced `GameTimer` and `PlayerProgression` resources to manage game time and player levels. - Updated the gameplay system to initialize these resources and integrate them into the HUD. - Refactored the player HUD widget to display time left and player level dynamically. - Enhanced the fight scene to utilize the new HUD structure for improved gameplay experience.
There was a problem hiding this comment.
Summary of Changes
Hello @supreme2580, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces foundational game mechanics by implementing a game timer, player progression tracking, and an advanced enemy AI system that follows player movement paths. It also significantly refactors the player HUD to dynamically display these new game states and integrates navigation data loading for potential future use in pathfinding.
Highlights
- Core Game Resources: Implemented
GameTimerfor time-based gameplay andPlayerProgressionfor tracking player level and experience, laying the groundwork for core game loops. - Dynamic HUD: The player HUD is now dynamic, displaying real-time game timer and player progression, with corresponding progress bars that update automatically.
- Advanced Enemy AI: Introduced a
PathFollowerAI system that records and traces the player's movement path, allowing enemies to follow the player's exact steps rather than simple direct pursuit. - Navigation Data Integration: Added structures (
NavigationData,NavPoint,NavStatistics) and a loader to manage and utilize navigation path data, enabling more complex AI behaviors and level design. - Physics Refinement: Collectible items now use
Sensorphysics, allowing players to pass through them while still triggering collection events, improving gameplay fluidity.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces several new game mechanics, including a game timer, player progression, and a path-following enemy AI. It also refactors the player HUD to be dynamic and data-driven. The changes are extensive and touch UI, game state, and systems logic. My review focuses on improving robustness, fixing potential bugs like memory leaks, and addressing code duplication and performance considerations. Key areas for improvement include handling of floating-point comparisons, using more appropriate data structures, and ensuring all UI elements are properly cleaned up.
| #[derive(Resource, Default)] | ||
| pub struct MainTrack; No newline at end of file | ||
| pub struct NavigationData { | ||
| pub nav_points: Vec<NavPoint>, | ||
| pub statistics: NavStatistics, | ||
| } | ||
|
|
||
| #[derive(Debug, Serialize, Deserialize, Clone)] | ||
| pub struct NavPoint { | ||
| pub timestamp: f32, | ||
| pub position: Vec3, | ||
| pub session_time: f32, | ||
| } | ||
|
|
||
| #[derive(Debug, Serialize, Deserialize, Clone, Default)] | ||
| pub struct NavStatistics { | ||
| pub total_points: usize, | ||
| pub session_duration: f32, | ||
| pub min_bounds: Vec3, | ||
| pub max_bounds: Vec3, | ||
| pub average_position: Vec3, | ||
| } | ||
|
|
||
| impl NavigationData { | ||
| pub fn find_nearest_point(&self, position: Vec3) -> Option<&NavPoint> { | ||
| self.nav_points.iter().min_by(|a, b| { | ||
| let dist_a = (a.position - position).length_squared(); | ||
| let dist_b = (b.position - position).length_squared(); | ||
| dist_a.partial_cmp(&dist_b).unwrap() | ||
| }) | ||
| } | ||
|
|
||
| pub fn find_path_to_target(&self, start: Vec3, target: Vec3, max_points: usize) -> Vec<Vec3> { | ||
| let mut path = Vec::new(); | ||
|
|
||
| // Find nearest nav points to start and target | ||
| let start_point = self.find_nearest_point(start); | ||
| let target_point = self.find_nearest_point(target); | ||
|
|
||
| if let (Some(start), Some(target)) = (start_point, target_point) { | ||
| // Find nav points between start and target | ||
| let start_idx = self.nav_points.iter().position(|p| p.position == start.position).unwrap_or(0); | ||
| let target_idx = self.nav_points.iter().position(|p| p.position == target.position).unwrap_or(0); | ||
|
|
||
| // Determine direction of path | ||
| let (from_idx, to_idx) = if start_idx <= target_idx { | ||
| (start_idx, target_idx) | ||
| } else { | ||
| (target_idx, start_idx) | ||
| }; | ||
|
|
||
| // Get subset of points | ||
| let points_between: Vec<_> = self.nav_points[from_idx..=to_idx] | ||
| .iter() | ||
| .map(|p| p.position) | ||
| .collect(); | ||
|
|
||
| // Sample points to match max_points | ||
| let step = (points_between.len() as f32 / max_points as f32).max(1.0) as usize; | ||
| path = points_between.iter() | ||
| .step_by(step) | ||
| .cloned() | ||
| .collect(); | ||
| } | ||
|
|
||
| // Always include target position | ||
| path.push(target); | ||
| path | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
NavigationData and its related structs (NavPoint, NavStatistics) and implementations are duplicated in this file and also in client/src/resources/navigation.rs. This can lead to confusion, maintenance issues, and bugs. The version in this file appears to be an older, less robust implementation (e.g., find_path_to_target doesn't handle reversed paths correctly).
Please remove the NavigationData related code from this file and use the version from client/src/resources/navigation.rs exclusively.
| self.nav_points.iter().min_by(|a, b| { | ||
| let dist_a = (a.position - position).length_squared(); | ||
| let dist_b = (b.position - position).length_squared(); | ||
| dist_a.partial_cmp(&dist_b).unwrap() |
There was a problem hiding this comment.
The use of .unwrap() on the result of partial_cmp can cause a panic if any of the calculated squared distances are NaN. While unlikely with position vectors, floating-point arithmetic can have surprising edge cases. For more robust code, you should handle the None case from partial_cmp to prevent panics.
| dist_a.partial_cmp(&dist_b).unwrap() | |
| dist_a.partial_cmp(&dist_b).unwrap_or(std::cmp::Ordering::Equal) |
| let start_idx = self.nav_points.iter().position(|p| p.position == start.position).unwrap_or(0); | ||
| let target_idx = self.nav_points.iter().position(|p| p.position == target.position).unwrap_or(0); |
There was a problem hiding this comment.
Comparing floating-point vectors using == is unreliable due to precision issues. A point might not be found even if it's extremely close, leading to incorrect pathfinding. It's better to check if the distance between points is within a small epsilon.
| let start_idx = self.nav_points.iter().position(|p| p.position == start.position).unwrap_or(0); | |
| let target_idx = self.nav_points.iter().position(|p| p.position == target.position).unwrap_or(0); | |
| let start_idx = self.nav_points.iter().position(|p| (p.position - start.position).length_squared() < 1e-9).unwrap_or(0); | |
| let target_idx = self.nav_points.iter().position(|p| (p.position - target.position).length_squared() < 1e-9).unwrap_or(0); |
| self.nav_points.iter().min_by(|a, b| { | ||
| let dist_a = (a.position - position).length_squared(); | ||
| let dist_b = (b.position - position).length_squared(); | ||
| dist_a.partial_cmp(&dist_b).unwrap() |
There was a problem hiding this comment.
The use of .unwrap() on the result of partial_cmp can cause a panic if any of the calculated squared distances are NaN. While unlikely with position vectors, floating-point arithmetic can have surprising edge cases. For more robust code, you should handle the None case from partial_cmp to prevent panics.
| dist_a.partial_cmp(&dist_b).unwrap() | |
| dist_a.partial_cmp(&dist_b).unwrap_or(std::cmp::Ordering::Equal) |
| let start_idx = self.nav_points.iter().position(|p| p.position == start.position).unwrap_or(0); | ||
| let target_idx = self.nav_points.iter().position(|p| p.position == target.position).unwrap_or(0); |
There was a problem hiding this comment.
Comparing floating-point vectors using == is unreliable due to precision issues. A point might not be found even if it's extremely close, leading to incorrect pathfinding. It's better to check if the distance between points is within a small epsilon.
| let start_idx = self.nav_points.iter().position(|p| p.position == start.position).unwrap_or(0); | |
| let target_idx = self.nav_points.iter().position(|p| p.position == target.position).unwrap_or(0); | |
| let start_idx = self.nav_points.iter().position(|p| (p.position - start.position).length_squared() < 1e-9).unwrap_or(0); | |
| let target_idx = self.nav_points.iter().position(|p| (p.position - target.position).length_squared() < 1e-9).unwrap_or(0); |
| } | ||
|
|
||
| fn despawn_gameplay_hud(mut commands: Commands, query: Query<Entity, With<GameplayHud>>) { | ||
| fn despawn_gameplay_hud(mut commands: Commands, query: Query<Entity, With<crate::ui::dynamic_hud::DynamicPlayerHud>>) { |
There was a problem hiding this comment.
This function was changed to only despawn DynamicPlayerHud. However, the objectives UI is spawned with the GameplayHud component (which was previously used to despawn it here) and is not being cleaned up on exiting Screen::GamePlay. This will result in a memory leak, as the objectives UI will persist across scenes. You should ensure all gameplay-specific UI is despawned.
fn despawn_gameplay_hud(mut commands: Commands, query: Query<Entity, Or<(With<crate::ui::dynamic_hud::DynamicPlayerHud>, With<crate::systems::objectives::ObjectiveUI>)>>)| pub struct PathFollower { | ||
| pub move_speed: f32, | ||
| pub is_moving: bool, | ||
| pub player_path: Vec<Vec3>, // Queue of player positions to follow |
There was a problem hiding this comment.
player_path is used as a queue, with elements being removed from the front using remove(0) (lines 103 and 160). For a Vec, this is an O(n) operation which can be inefficient. Using a VecDeque would be more idiomatic and performant for queue-like behavior, as pop_front() is an O(1) operation. You would also need to change .push() to .push_back() when adding elements.
| pub player_path: Vec<Vec3>, // Queue of player positions to follow | |
| pub player_path: std::collections::VecDeque<Vec3>, // Queue of player positions to follow |
| move_speed: 3.0, | ||
| move_speed: 4.0, // Slightly slower than player | ||
| is_moving: false, | ||
| player_path: Vec::new(), |
| pub fn load_navigation_data(mut commands: Commands) { | ||
| // Load nav.json file | ||
| let nav_file = include_str!("../../nav.json"); | ||
| let nav_json: Value = serde_json::from_str(nav_file).expect("Failed to parse nav.json"); | ||
|
|
||
| // Parse positions | ||
| let positions = nav_json["positions"].as_array().unwrap(); | ||
| let nav_points: Vec<NavPoint> = positions.iter().map(|pos| { | ||
| NavPoint { | ||
| timestamp: pos["timestamp"].as_f64().unwrap() as f32, | ||
| position: Vec3::new( | ||
| pos["position"][0].as_f64().unwrap() as f32, | ||
| pos["position"][1].as_f64().unwrap() as f32, | ||
| pos["position"][2].as_f64().unwrap() as f32, | ||
| ), | ||
| session_time: pos["session_time"].as_f64().unwrap() as f32, | ||
| } | ||
| }).collect(); | ||
|
|
||
| // Parse statistics | ||
| let stats = &nav_json["statistics"]; | ||
| let statistics = NavStatistics { | ||
| total_points: stats["total_points"].as_u64().unwrap() as usize, | ||
| session_duration: stats["session_duration"].as_f64().unwrap() as f32, | ||
| min_bounds: Vec3::new( | ||
| stats["min_bounds"][0].as_f64().unwrap() as f32, | ||
| stats["min_bounds"][1].as_f64().unwrap() as f32, | ||
| stats["min_bounds"][2].as_f64().unwrap() as f32, | ||
| ), | ||
| max_bounds: Vec3::new( | ||
| stats["max_bounds"][0].as_f64().unwrap() as f32, | ||
| stats["max_bounds"][1].as_f64().unwrap() as f32, | ||
| stats["max_bounds"][2].as_f64().unwrap() as f32, | ||
| ), | ||
| average_position: Vec3::new( | ||
| stats["average_position"][0].as_f64().unwrap() as f32, | ||
| stats["average_position"][1].as_f64().unwrap() as f32, | ||
| stats["average_position"][2].as_f64().unwrap() as f32, | ||
| ), | ||
| }; | ||
|
|
||
| // Create navigation data resource | ||
| let nav_data = NavigationData { | ||
| nav_points, | ||
| statistics, | ||
| }; | ||
|
|
||
| commands.insert_resource(nav_data); | ||
| } |
There was a problem hiding this comment.
This function uses a lot of .unwrap() and .expect() calls to parse the JSON file. This is very brittle and will panic if the nav.json file has any structural differences from what's expected. A more robust approach would be to derive Deserialize on your NavigationData, NavPoint, and NavStatistics structs and then use serde_json::from_str to parse the whole file in one go. This would provide better error messages and be more maintainable.
| node.width = Val::Px(417.0 * time_percent); | ||
| } | ||
|
|
||
| // Update XP progress bar width | ||
| let (current, max) = progression.get_xp_for_display(); | ||
| let xp_percent = if max > 0 { current as f32 / max as f32 } else { 0.0 }; | ||
| for mut node in xp_bar_query.iter_mut() { | ||
| node.width = Val::Px(417.0 * xp_percent); |
There was a problem hiding this comment.
The value 417.0 is a magic number representing the width of the progress bar. This value is also used in client/src/ui/dynamic_hud.rs. It would be better to define this as a shared constant (e.g., in a ui::constants module) to improve readability and make it easier to update the UI layout consistently.
closes: #54