026 direction bricks #171
026 direction bricks #171
Conversation
- Impulses: Instantaneous velocity changes (not per-frame acceleration) - Observability: Tracing spans via tracing crate - Event system: Observers + Trigger pattern - Validation: Physics system owns bounds, no validation by bricks - RNG: Generate magnitude directly in 5.0-15.0 range
…r velocity effects - Added observer system for direction brick effects, applying velocity impulses to balls upon destruction. - Updated specifications and plans to clarify behavior of bricks, including randomized velocity for brick 52. - Created detailed tasks for implementation, testing, and integration of direction bricks into the existing game framework. - Ensured compliance with Bevy 0.17 standards, including message-event separation and coordinate system correctness.
Changed direction brick effects from direct velocity modification to using bevy_rapier3d's ExternalImpulse component for more physically realistic behavior.
…ase 5, T026-T027)
- Updated `ball_spawn_bricks.rs` to ensure ball spawning occurs before respawn detection. - Made `despawn_balls_and_merkabas_on_life_loss` public in `merkaba.rs` for broader access. - Refined `respawn.rs` to improve system execution order and added checks for paddle shrink conditions. - Enhanced impulse tests in `direction_bricks.rs` to reflect accurate impulse application for directional bricks. - Introduced new tests in `paddle_shrink_multiball.rs` and `paddle_shrink_regression.rs` to validate paddle shrink behavior under various ball loss scenarios, ensuring it only triggers when the last ball is lost. - Ensured that paddle shrink animations do not occur when multiple balls are in play.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cleder, your pull request is larger than the review limit of 150000 diff characters
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
📝 WalkthroughWalkthroughThis PR introduces a Direction Bricks feature that applies instantaneous impulses to balls upon collision with seven new brick types (IDs 43–48, 52). The implementation includes an observer-based event system for handling velocity modifications, comprehensive specifications and documentation, accompanying test coverage, and supporting asset and configuration updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Ball
participant CollisionHandler as BrickCollision
participant Signals as DirectionBrickEffect
participant Observer as ApplyDirectionBrickEffects
participant Physics
Ball->>CollisionHandler: Collision with brick (43-48,52)
CollisionHandler->>CollisionHandler: Compute impulse (or random magnitude/angle for 52)
CollisionHandler->>Signals: Emit DirectionBrickEffect(ball, brick_type, pos, vel_before, impulse)
Observer->>Signals: On<DirectionBrickEffect> trigger
alt Cardinal / Diagonal (43-48)
Observer->>Physics: Add impulse to ExternalImpulse (X/Y)
else Random (52)
Observer->>Observer: Generate random magnitude & angle
Observer->>Physics: Apply random impulse (XZ plane)
end
Physics->>Ball: Integrate, update LinearVelocity
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to b3ad6f5 in 25 seconds. Click for details.
- Reviewed
4156lines of code in28files - Skipped
14files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_vupGzWx5LwPGWicv
You can customize 
Summary of ChangesHello @cleder, 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 delivers a significant new gameplay mechanic by introducing 'direction bricks' that dynamically alter ball velocity. The implementation leverages Bevy's observer pattern for responsive physics interactions and includes robust documentation covering design, implementation, and testing. Alongside this feature, the PR refines existing paddle behavior for multi-ball scenarios and updates development tooling, ensuring a stable and well-documented addition to the game. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
for more information, see https://pre-commit.ci
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
src/systems/brick_effects.rs
Outdated
| // Apply the impulse vector computed by collision system | ||
| external_impulse.impulse = effect.impulse; |
There was a problem hiding this comment.
Impulse Overwrite Instead of Accumulation
The code sets external_impulse.impulse = effect.impulse;, which overwrites any existing impulse value. According to the documentation (lines 63-64), impulses from multiple direction brick effects should compound additively (except for brick 52). This implementation will only apply the last impulse if multiple triggers occur for the same ball entity in a single frame.
Recommended Solution:
Accumulate the impulse instead:
external_impulse.impulse += effect.impulse;For brick 52 (random direction), ensure that it replaces the X and Y components as specified, but otherwise, additive accumulation should be the default.
src/systems/brick_effects.rs
Outdated
| // This is a non-critical error that can occur if the ball despawned | ||
| debug!( | ||
| ball = ?effect.ball_entity, | ||
| brick_type = effect.brick_type, | ||
| "Ball entity not found or missing ExternalImpulse component when applying direction brick effect" | ||
| ); |
There was a problem hiding this comment.
Error Logging Level May Be Insufficient
The error case for missing ball entity or ExternalImpulse component is logged at the debug level. If this error is rare but critical (e.g., due to unexpected despawning or system misordering), it may be missed in production logs.
Recommended Solution:
Consider logging at the warning or error level to ensure visibility:
warn!(
ball = ?effect.ball_entity,
brick_type = effect.brick_type,
"Ball entity not found or missing ExternalImpulse component when applying direction brick effect"
);This will help diagnose issues that could affect gameplay or system integrity.
| fn apply_paddle_shrink( | ||
| mut life_lost_events: MessageReader<LifeLostEvent>, | ||
| paddles: Query<(Entity, &Transform), (With<Paddle>, Without<PaddleGrowing>)>, | ||
| balls: Query<Entity, With<Ball>>, | ||
| respawn_schedule: Res<RespawnSchedule>, | ||
| mut commands: Commands, | ||
| mut shrink_applied_this_frame: Local<bool>, | ||
| ) { | ||
| // Reset the flag at the start of each frame | ||
| *shrink_applied_this_frame = false; | ||
|
|
||
| for _event in life_lost_events.read() { | ||
| // Only shrink if ALL balls are gone (no more balls in play) | ||
| // This runs BEFORE the life decrement in enqueue_respawn_requests | ||
| let remaining_balls = balls.iter().count(); | ||
|
|
||
| if remaining_balls != 0 { | ||
| // Other balls still in play, don't shrink | ||
| continue; | ||
| } | ||
|
|
||
| // Only apply the shrink once per frame, even if multiple LifeLostEvents trigger | ||
| if *shrink_applied_this_frame { | ||
| continue; | ||
| } | ||
| *shrink_applied_this_frame = true; | ||
|
|
||
| for (entity, transform) in paddles.iter() { | ||
| let shrink_duration = respawn_schedule.timer.duration(); | ||
| commands.entity(entity).insert(PaddleGrowing { | ||
| timer: Timer::from_seconds(shrink_duration.as_secs_f32(), TimerMode::Once), | ||
| target_scale: Vec3::splat(0.01), | ||
| start_scale: transform.scale, | ||
| }); | ||
| commands.entity(entity).insert(( | ||
| PaddleGrowing { | ||
| timer: Timer::from_seconds(shrink_duration.as_secs_f32(), TimerMode::Once), | ||
| target_scale: Vec3::splat(0.01), | ||
| start_scale: transform.scale, | ||
| }, | ||
| InputLocked, | ||
| )); | ||
| info!( | ||
| target: "respawn", | ||
| event = "paddle_shrink_started", | ||
| ?entity, | ||
| start_scale = ?transform.scale, | ||
| duration_secs = shrink_duration.as_secs_f32(), | ||
| "Paddle shrink animation triggered by ball loss" | ||
| "Paddle shrink animation triggered when last ball is lost" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential for Unintended Behavior with Multiple Paddles
The current implementation applies the shrink animation to all paddles without an active PaddleGrowing component when all balls are lost. While this is acceptable for a single-paddle design, it could lead to unintended behavior if the game is extended to support multiple paddles. Consider updating the logic to target only the relevant paddle(s) associated with the lost ball, or clearly enforce single-paddle constraints in the system to prevent future issues.
Recommended Solution:
- If multi-paddle support is planned, associate each ball with its controlling paddle and only apply the shrink to that paddle.
- Otherwise, add a runtime assertion or comment to ensure only one paddle exists.
| use rand::Rng; | ||
|
|
||
| // Test multiple samples to verify RNG distribution | ||
| let mut rng = rand::rng(); | ||
| for _ in 0..100 { | ||
| // Simulate the RNG logic from lib.rs brick 52 case | ||
| let magnitude = rng.random_range(5.0..15.0); | ||
| let angle: f32 = rng.random_range(0.0..std::f32::consts::TAU); | ||
| let impulse = Vec3::new(magnitude * angle.cos(), 0.0, magnitude * angle.sin()); |
There was a problem hiding this comment.
Non-standard RNG API usage in random impulse tests
The random impulse tests (T026, T027) use rand::rng() and rng.random_range, which are not part of the standard rand crate API. This will likely cause compilation errors or incorrect behavior. The standard approach is to use rand::thread_rng() and rng.gen_range(a..b):
let mut rng = rand::thread_rng();
let magnitude = rng.gen_range(5.0..15.0);
let angle: f32 = rng.gen_range(0.0..std::f32::consts::TAU);Recommended solution:
Replace all instances of rand::rng() and random_range with the standard rand::thread_rng() and gen_range methods to ensure compatibility and correct random value generation.
| fn apply_impulse_to_ball(app: &mut App, ball: Entity, impulse: Vec3) -> Vec3 { | ||
| if let Some(mut external_impulse) = app.world_mut().get_mut::<ExternalImpulse>(ball) { | ||
| external_impulse.impulse = impulse; | ||
| } | ||
|
|
||
| // Return the current velocity for assertion purposes | ||
| // (In real physics, this would be updated by the physics system) | ||
| if let Some(velocity) = app.world().get::<Velocity>(ball) { | ||
| velocity.linvel | ||
| } else { | ||
| Vec3::ZERO | ||
| } | ||
| } |
There was a problem hiding this comment.
Impulse overwriting in helper function may not match physics system behavior
The apply_impulse_to_ball function overwrites the ExternalImpulse.impulse value each time it is called. In many physics systems, impulses are accumulated rather than replaced. This could lead to misleading test results, especially in tests that expect composability or persistence of impulses.
Recommended solution:
If the production system accumulates impulses, modify the helper to add to the existing impulse:
external_impulse.impulse += impulse;Otherwise, clarify in the test documentation that impulses are expected to be overwritten, and ensure this matches the actual system behavior.
| advance_time(&mut app, 0.016); | ||
| app.update(); |
There was a problem hiding this comment.
The test advances time and calls app.update() only once after triggering a life loss. If the system under test requires multiple update cycles to process all events (e.g., respawn logic, component changes), this may result in flaky or incomplete test coverage. Recommendation: Run multiple update cycles after advancing time to ensure all systems have processed their events:
advance_time(&mut app, 0.016);
for _ in 0..2 { app.update(); }This will make the tests more robust against changes in system scheduling.
| let mut lives = app | ||
| .world_mut() | ||
| .resource_mut::<brkrs::systems::respawn::LivesState>(); | ||
| lives.lives_remaining = 1; | ||
| } |
There was a problem hiding this comment.
Directly mutating the lives_remaining field in the resource may be brittle if the underlying system changes how lives are managed (e.g., via events or deferred updates). Recommendation: Abstract lives manipulation into a helper function or use the game's public API for changing lives, if available. This will improve maintainability and reduce coupling to internal implementation details.
Codacy's Analysis Summary2 new issues (≤ 0 issue) Review Pull Request in Codacy →
|
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||
dosubot
bot
added
the
Levels 🆙
There was a problem hiding this comment.
Pull request overview
This PR implements direction bricks (types 43-48, 52) that apply instantaneous impulses to the ball when destroyed, adding dynamic velocity control for level design. The implementation uses Bevy 0.17's observer pattern for immediate physics effects, scoring integration, and comprehensive test coverage including multi-frame persistence validation.
Changes:
- Adds 7 new brick types with directional impulse mechanics (cardinal, diagonal, and random directions)
- Implements observer-based system for immediate velocity modifications via
ExternalImpulsecomponent - Includes comprehensive test suites validating impulse application, multi-ball scenarios, and paddle shrink behavior refinements
Reviewed changes
Copilot reviewed 29 out of 66 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
tests/direction_bricks.rs |
New test suite covering all 7 direction brick types with cardinal, diagonal, and random impulse validation |
tests/paddle_shrink_*.rs |
New regression tests ensuring paddle shrink only triggers when last ball is lost |
src/systems/brick_effects.rs |
New observer system applying direction brick impulses to ball ExternalImpulse component |
src/systems/respawn.rs |
Updated paddle shrink logic to check remaining balls before triggering animation |
src/signals.rs |
Added DirectionBrickEffect event type for observer pattern |
src/lib.rs |
Integrated direction brick collision detection and impulse triggering |
specs/026-direction-bricks/* |
Complete feature specification with acceptance criteria and implementation plan |
docs/direction-bricks-coordinates.md |
Documentation of coordinate system and impulse mappings |
assets/levels/level_099.ron |
Test level demonstrating all 7 direction brick types |
Comments suppressed due to low confidence (2)
tests/direction_bricks.rs:1
- The random direction brick implementation in the test and production code applies impulses incorrectly to the Y-axis instead of the Z-axis. According to the coordinate system documentation in
docs/direction-bricks-coordinates.mdandspecs/026-direction-bricks/plan.md, the XZ plane is horizontal (gameplay surface) and Y is vertical. The random brick should modify X and Z components (horizontal plane), not X and Y. The correct formula should beVec3::new(magnitude * angle.cos(), 0.0, magnitude * angle.sin())to apply random direction in the horizontal XZ plane while preserving vertical Y component.
//! Tests for direction brick effects (bricks 43-48, 52)
src/systems/brick_effects.rs:1
- The coordinate system documentation contradicts the actual implementation and test code. Lines 43-44 state X is for left/right and Z is for forward/backward, but the code in
src/lib.rslines 885-890 applies impulses to X for down/up (lines 885, 888) and Z for left/right (lines 886-887). This mismatch between documentation and implementation will cause confusion. The documentation should be updated to match the actual brick mappings: X-axis for down/up movement and Z-axis for left/right movement.
use crate::signals::DirectionBrickEffect;
| /// - Positive Y: Up (away from gravity) | ||
| /// - Negative Y: Gravity pull (down) | ||
| /// | ||
| /// **Z-Axis Invariant**: No direction brick modifies Z-component of velocity. |
There was a problem hiding this comment.
Line 41 states 'No direction brick modifies Z-component of velocity' but this directly contradicts the implementation in src/lib.rs lines 886-887 and 889-890, which modify the Z component for bricks 44, 45, 47, and 48. The comment should either be removed or corrected to state that the Y-component is never modified (as Y is vertical and unused for horizontal gameplay).
| let ball = spawn_ball( | ||
| &mut app, | ||
| Vec3::new(-3.0, 2.0, 0.0), | ||
| Vec3::new(-3.0, 2.0, 0.0), | ||
| ); |
There was a problem hiding this comment.
Test T006 (brick 43 down impulse) spawns the ball with initial velocity Vec3::new(-3.0, 2.0, 0.0) but brick 43 applies +5.0 X impulse (line 885 in src/lib.rs). The test should verify the X component changes from -3.0 to 2.0, not verify Y component changes. The current assertions on lines 87-91 check the wrong axis - they should validate X-axis changes, not Y-axis.
src/lib.rs
Outdated
| let ball_velocity = transforms | ||
| .get(triggering_ball) | ||
| .ok() | ||
| .map(|_t| Vec3::ZERO) |
There was a problem hiding this comment.
The velocity_before field is hardcoded to Vec3::ZERO instead of reading the actual ball velocity from the Velocity component. The transforms query is used but the transform is discarded (line 880 |_t|), and the result is always Vec3::ZERO. This makes the velocity_before field in DirectionBrickEffect useless for tracing and debugging. Query the ball's Velocity component instead: velocities.get(triggering_ball).map(|v| v.linvel).unwrap_or(Vec3::ZERO).
| let ball_velocity = transforms | |
| .get(triggering_ball) | |
| .ok() | |
| .map(|_t| Vec3::ZERO) | |
| let ball_velocity = velocities | |
| .get(triggering_ball) | |
| .map(|v| v.linvel) |
| let magnitude = rng.random_range(5.0..15.0); | ||
| let angle: f32 = rng.random_range(0.0..std::f32::consts::TAU); | ||
| let impulse = Vec3::new(magnitude * angle.cos(), 0.0, magnitude * angle.sin()); |
There was a problem hiding this comment.
Test T026 validates the random brick RNG logic but uses the wrong axes - it should match the production implementation in src/lib.rs line 898. However, both this test and the production code have the same axis error (Y instead of Z for the sine component). When the production bug is fixed to use Z-axis, this test will also need updating to Vec3::new(magnitude * angle.cos(), 0.0, magnitude * angle.sin()) to maintain consistency.
| for _event in life_lost_events.read() { | ||
| // Only shrink if ALL balls are gone (no more balls in play) | ||
| // This runs BEFORE the life decrement in enqueue_respawn_requests | ||
| let remaining_balls = balls.iter().count(); |
There was a problem hiding this comment.
The balls.iter().count() on line 479 iterates through all ball entities every time a LifeLostEvent occurs. For games with multiple balls in play, this could be inefficient if many LifeLostEvents fire in the same frame. Consider using balls.iter().len() if the query supports it, or cache the count. However, since this only runs when a ball is lost (infrequent event), the performance impact is likely negligible.
| let remaining_balls = balls.iter().count(); | |
| let remaining_balls = balls.iter().len(); |
| commands.entity(entity).queue_silenced( | ||
| |mut entity_commands: bevy::ecs::world::EntityWorldMut| { | ||
| entity_commands.insert(MarkedForDespawn); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
The use of queue_silenced with a closure to insert MarkedForDespawn (lines 956-960 and 1210-1214) is more verbose than the direct commands.entity(entity).insert(MarkedForDespawn) pattern. Unless there's a specific reason to silence the command or defer it beyond normal command application, consider reverting to the simpler direct insertion pattern for consistency with the rest of the codebase.
| commands.entity(entity).queue_silenced( | |
| |mut entity_commands: bevy::ecs::world::EntityWorldMut| { | |
| entity_commands.insert(MarkedForDespawn); | |
| }, | |
| ); | |
| commands.entity(entity).insert(MarkedForDespawn); |
| ), | ||
| ( | ||
| object_class: Brick, | ||
| type_id: 39, |
There was a problem hiding this comment.
Line 1386 assigns type_id: 39 to the brick/balls_3 profile, but this duplicates the type_id from lines 1379-1380 where brick/balls_2 also uses type_id: 39. Each brick type should have a unique type_id. Based on the pattern (brick/balls_1 = 37, brick/balls_2 = 39), this should likely be type_id: 40 or another unique value.
| type_id: 39, | |
| type_id: 40, |
specs/026-direction-bricks/plan.md
Outdated
| - "Randomized velocity" for brick 52: Generate random 2D direction in XZ plane (or full 3D if Z movement is allowed), magnitude 5.0-15.0 units/sec | ||
| - Direction angle generated in **radians** (0.0..TAU = 0.0..2π), NOT degrees (0.0..360.0) | ||
| - Implementation: `angle = rng.gen_range(0.0..std::f32::consts::TAU)` (direct radians); convert to cos/sin for X/Y components |
There was a problem hiding this comment.
Line 108 states 'convert to cos/sin for X/Y components' but this conflicts with line 106 which correctly identifies the XZ plane as the horizontal gameplay surface. The implementation should convert to X/Z components (X = magnitude * cos(angle), Z = magnitude * sin(angle)), not X/Y. This documentation error matches the implementation bug in src/lib.rs line 898.
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a very comprehensive pull request that adds the direction bricks feature, including implementation, extensive tests, and detailed documentation. The fix for the paddle shrink logic in multi-ball scenarios is also well-implemented and tested.
I've found a few inconsistencies and a bug that need to be addressed:
- There's a duplicated
type_idinassets/textures/manifest.ron. - The random direction brick (type 52) impulse is calculated in the wrong plane.
- There are several documentation inconsistencies across
src/signals.rsandsrc/systems/brick_effects.rsregarding the direction brick mappings and effects, which could be misleading.
Once these issues are resolved, this will be an excellent addition to the project.
| ( | ||
| object_class: Brick, | ||
| type_id: 39, | ||
| profile_id: "brick/balls_3", | ||
| emissive_color: None, | ||
| animation: None, | ||
| ), |
There was a problem hiding this comment.
There appears to be a duplicated type_id: 39, which is mapped to both brick/balls_2 and brick/balls_3. This is likely a copy-paste error. Please assign the correct type_id for the brick/balls_3 profile.
| 52 => { | ||
| // Random direction brick: RNG-based impulse in XY plane | ||
| // Magnitude: 5.0-15.0 units/sec, Direction: 0-2π radians | ||
| use rand::Rng; | ||
| let mut rng = rand::rng(); | ||
| let magnitude = rng.random_range(5.0..15.0); | ||
| let angle: f32 = rng.random_range(0.0..std::f32::consts::TAU); | ||
| Vec3::new(magnitude * angle.cos(), magnitude * angle.sin(), 0.0) | ||
| } |
There was a problem hiding this comment.
The impulse for the random direction brick (type 52) is calculated in the XY plane. Based on the game's coordinate system and documentation, it should be in the XZ plane to affect horizontal movement. The Y component should be 0.
| 52 => { | |
| // Random direction brick: RNG-based impulse in XY plane | |
| // Magnitude: 5.0-15.0 units/sec, Direction: 0-2π radians | |
| use rand::Rng; | |
| let mut rng = rand::rng(); | |
| let magnitude = rng.random_range(5.0..15.0); | |
| let angle: f32 = rng.random_range(0.0..std::f32::consts::TAU); | |
| Vec3::new(magnitude * angle.cos(), magnitude * angle.sin(), 0.0) | |
| } | |
| 52 => { | |
| // Random direction brick: RNG-based impulse in XZ plane | |
| // Magnitude: 5.0-15.0 units/sec, Direction: 0-2π radians | |
| use rand::Rng; | |
| let mut rng = rand::rng(); | |
| let magnitude = rng.random_range(5.0..15.0); | |
| let angle: f32 = rng.random_range(0.0..std::f32::consts::TAU); | |
| Vec3::new(magnitude * angle.cos(), 0.0, magnitude * angle.sin()) | |
| } |
src/signals.rs
Outdated
| /// # Per-Brick Behavior | ||
| /// | ||
| /// - **Brick 43 (Left)**: Apply impulse `(-5.0, 0, 0)` units/sec | ||
| /// - **Brick 44 (Right)**: Apply impulse `(+5.0, 0, 0)` units/sec | ||
| /// - **Brick 45 (Up)**: Apply impulse `(0, +5.0, 0)` units/sec | ||
| /// - **Brick 46 (Down)**: Apply impulse `(0, -5.0, 0)` units/sec | ||
| /// - **Brick 47 (Forward)**: Apply impulse `(0, 0, +5.0)` units/sec | ||
| /// - **Brick 48 (Backward)**: Apply impulse `(0, 0, -5.0)` units/sec | ||
| /// - **Brick 52 (Random)**: Apply impulse with magnitude ∈ [5.0, 15.0], direction ∈ [0, 2π) in XY plane only (Z=0) |
There was a problem hiding this comment.
The documentation for the per-brick behavior is inconsistent with the implementation. For example, brick 43 is documented here as 'Left' with a -X impulse, but it's implemented as 'Down' with a +X impulse. Please update the documentation to match the implementation to avoid confusion. This applies to all brick types listed here.
| /// # Per-Brick Behavior | |
| /// | |
| /// - **Brick 43 (Left)**: Apply impulse `(-5.0, 0, 0)` units/sec | |
| /// - **Brick 44 (Right)**: Apply impulse `(+5.0, 0, 0)` units/sec | |
| /// - **Brick 45 (Up)**: Apply impulse `(0, +5.0, 0)` units/sec | |
| /// - **Brick 46 (Down)**: Apply impulse `(0, -5.0, 0)` units/sec | |
| /// - **Brick 47 (Forward)**: Apply impulse `(0, 0, +5.0)` units/sec | |
| /// - **Brick 48 (Backward)**: Apply impulse `(0, 0, -5.0)` units/sec | |
| /// - **Brick 52 (Random)**: Apply impulse with magnitude ∈ [5.0, 15.0], direction ∈ [0, 2π) in XY plane only (Z=0) | |
| /// # Per-Brick Behavior | |
| /// | |
| /// - **Brick 43 (Down)**: Apply impulse `(+5.0, 0, 0)` units/sec | |
| /// - **Brick 44 (Left)**: Apply impulse `(0, 0, +5.0)` units/sec | |
| /// - **Brick 45 (Right)**: Apply impulse `(0, 0, -5.0)` units/sec | |
| /// - **Brick 46 (Up)**: Apply impulse `(-5.0, 0, 0)` units/sec | |
| /// - **Brick 47 (Up-Right)**: Apply impulse `(-5.0, 0, -5.0)` units/sec | |
| /// - **Brick 48 (Up-Left)**: Apply impulse `(-5.0, 0, +5.0)` units/sec | |
| /// - **Brick 52 (Random)**: Apply impulse with magnitude ∈ [5.0, 15.0], direction ∈ [0, 2π) in XZ plane only (Y=0) |
| /// **Cardinal Directions (Bricks 43-48)**: 5.0 units/sec additive impulse per brick type | ||
| /// - Brick 43 (Left): `velocity.x -= 5.0` | ||
| /// - Brick 44 (Right): `velocity.x += 5.0` | ||
| /// - Brick 45 (Up): `velocity.y += 5.0` | ||
| /// - Brick 46 (Down): `velocity.y -= 5.0` | ||
| /// - Brick 47 (Forward): `velocity.z += 5.0` | ||
| /// - Brick 48 (Backward): `velocity.z -= 5.0` | ||
| /// | ||
| /// **Random Direction (Brick 52)**: Magnitude 5.0-15.0 units/sec, direction 0.0..2π radians | ||
| /// - Replaces both X and Y velocities (NOT additive) | ||
| /// - Preserves Z velocity | ||
| /// - Uses seeded RNG for deterministic behavior in tests | ||
| /// |
There was a problem hiding this comment.
The documentation for impulse magnitudes is inconsistent with the implementation and other documentation. For example, brick 43 is for 'Down' movement, not 'Left'. Also, for brick 52, the effect is an additive impulse, not a replacement of velocity. Please update the documentation to be accurate.
src/systems/brick_effects.rs
Outdated
| /// Brick type ID for Left direction brick. | ||
| pub const BRICK_TYPE_DIRECTION_LEFT: u8 = 43; | ||
| /// Brick type ID for Right direction brick. | ||
| pub const BRICK_TYPE_DIRECTION_RIGHT: u8 = 44; | ||
| /// Brick type ID for Up direction brick. | ||
| pub const BRICK_TYPE_DIRECTION_UP: u8 = 45; | ||
| /// Brick type ID for Down direction brick. | ||
| pub const BRICK_TYPE_DIRECTION_DOWN: u8 = 46; | ||
| /// Brick type ID for Forward direction brick. | ||
| pub const BRICK_TYPE_DIRECTION_FORWARD: u8 = 47; | ||
| /// Brick type ID for Backward direction brick. | ||
| pub const BRICK_TYPE_DIRECTION_BACKWARD: u8 = 48; | ||
| /// Brick type ID for Random direction brick. | ||
| pub const BRICK_TYPE_DIRECTION_RANDOM: u8 = 52; |
There was a problem hiding this comment.
These constants are inconsistent with the implementation. For example, BRICK_TYPE_DIRECTION_LEFT is defined as 43, but brick 43 is for 'Down'. Please update the constant names to match their effects.
| /// Brick type ID for Left direction brick. | |
| pub const BRICK_TYPE_DIRECTION_LEFT: u8 = 43; | |
| /// Brick type ID for Right direction brick. | |
| pub const BRICK_TYPE_DIRECTION_RIGHT: u8 = 44; | |
| /// Brick type ID for Up direction brick. | |
| pub const BRICK_TYPE_DIRECTION_UP: u8 = 45; | |
| /// Brick type ID for Down direction brick. | |
| pub const BRICK_TYPE_DIRECTION_DOWN: u8 = 46; | |
| /// Brick type ID for Forward direction brick. | |
| pub const BRICK_TYPE_DIRECTION_FORWARD: u8 = 47; | |
| /// Brick type ID for Backward direction brick. | |
| pub const BRICK_TYPE_DIRECTION_BACKWARD: u8 = 48; | |
| /// Brick type ID for Random direction brick. | |
| pub const BRICK_TYPE_DIRECTION_RANDOM: u8 = 52; | |
| /// Brick type ID for Down direction brick. | |
| pub const BRICK_TYPE_DIRECTION_DOWN: u8 = 43; | |
| /// Brick type ID for Left direction brick. | |
| pub const BRICK_TYPE_DIRECTION_LEFT: u8 = 44; | |
| /// Brick type ID for Right direction brick. | |
| pub const BRICK_TYPE_DIRECTION_RIGHT: u8 = 45; | |
| /// Brick type ID for Up direction brick. | |
| pub const BRICK_TYPE_DIRECTION_UP: u8 = 46; | |
| /// Brick type ID for Up-Right direction brick. | |
| pub const BRICK_TYPE_DIRECTION_UP_RIGHT: u8 = 47; | |
| /// Brick type ID for Up-Left direction brick. | |
| pub const BRICK_TYPE_DIRECTION_UP_LEFT: u8 = 48; | |
| /// Brick type ID for Random direction brick. | |
| pub const BRICK_TYPE_DIRECTION_RANDOM: u8 = 52; |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| for _event in life_lost_events.read() { | ||
| // Only shrink if ALL balls are gone (no more balls in play) | ||
| // This runs BEFORE the life decrement in enqueue_respawn_requests | ||
| let remaining_balls = balls.iter().count(); | ||
|
|
||
| if remaining_balls != 0 { | ||
| // Other balls still in play, don't shrink | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🔴 Paddle shrink never triggers because ball entity is still counted in same frame it’s despawned
apply_paddle_shrink checks balls.iter().count() to decide whether “ALL balls are gone”, but it runs in the same Update schedule immediately after detect_ball_loss. The lost ball is despawned via commands.entity(ball_entity).despawn() which is deferred until the end of the schedule, so the ball still exists during apply_paddle_shrink.
Actual: on the last ball loss, remaining_balls is still 1, so the system continues and never applies PaddleGrowing.
Expected: shrink should trigger when the last ball is lost (as the PR description/tests intend).
Click to expand
Relevant code:
detect_ball_lossdefers despawn:commands.entity(ball_entity).despawn();src/systems/respawn.rs:408-414apply_paddle_shrinkcounts balls before that despawn is applied:let remaining_balls = balls.iter().count(); if remaining_balls != 0 { continue; }src/systems/respawn.rs:476-484
Impact:
- Paddle shrink feedback won’t happen on actual game-over / last-ball loss.
- The new/updated tests around shrink-on-last-ball should fail unless something else masks this ordering.
Recommendation: When processing each LifeLostEvent, treat event.ball as already gone (e.g., remaining = balls.iter().count() - balls.contains(event.ball) as usize) or mark the ball for despawn with a component and query Without<MarkedForDespawn>, or run shrink after command application (different schedule/ordering).
Was this helpful? React with 👍 or 👎 to provide feedback.
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Request Changes
This PR introduces direction brick impulses but has critical issues: a race condition in ball spawn scheduling and potential coordinate mapping errors that could break gameplay.
📄 Documentation Diagram
This diagram documents the new direction brick impulse workflow using the observer pattern.
sequenceDiagram
participant B as Ball
participant C as CollisionSystem
participant O as ObserverSystem
participant E as ExternalImpulse
participant P as Physics
B->>C: Collision with direction brick
C->>O: Queue DirectionBrickEffect event
note over C,O: PR #35;171 implements observer pattern for immediate impulses
O->>E: Apply impulse vector
E->>P: Update ball velocity
🌟 Strengths
- Comprehensive test suite covering direction bricks and paddle shrink edge cases.
- Well-documented specification with clear acceptance criteria.
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P1 | src/systems/respawn.rs | Bug | Fixes premature paddle shrink in multi-ball scenarios | method:apply_paddle_shrink, path:tests/paddle_shrink.rs |
| P1 | src/systems/ball_spawn_bricks.rs | Architecture | Prevents race condition between ball spawn and respawn detection | |
| P1 | src/lib.rs | Architecture | Ensures direction brick impulses match documented coordinate system | method:mark_brick_on_ball_collision, path:docs/direction-bricks-coordinates.md |
| P2 | assets/textures/manifest.ron | Bug | Duplicate type_id causes texture rendering mismatch | |
| P2 | specs/026-direction-bricks/tasks.md | Maintainability | Bundling extensive spec docs increases review overhead | path:specs/026-direction-bricks/ |
🔍 Notable Themes
- Event-driven observer pattern for direction bricks enhances modularity.
- Scheduling constraints are critical for deterministic game logic.
📈 Risk Diagram
This diagram illustrates the scheduling race condition risk in ball spawn and respawn logic.
sequenceDiagram
participant BS as BallSpawnSystem
participant RD as RespawnDetectSystem
participant PS as PaddleShrinkSystem
BS->>BS: Spawn new balls
RD->>RD: Check for ball loss
alt No balls and BS not run
RD->>PS: Trigger paddle shrink
note over BS,RD: R2(P1): Race condition if .before constraint missing
end
⚠️ **Unanchored Suggestions (Manual Review Recommended)**
The following suggestions could not be precisely anchored to a specific line in the diff. This can happen if the code is outside the changed lines, has been significantly refactored, or if the suggestion is a general observation. Please review them carefully in the context of the full file.
📁 File: src/systems/ball_spawn_bricks.rs
Adding the .before(RespawnSystems::Detect) scheduling constraint to the ball spawn system is a critical architectural fix. Without this ordering, a race condition could occur where a LifeLostEvent is processed by the apply_paddle_shrink system (part of RespawnSystems::Detect) before new balls from a spawn power-up brick are added to the world. This would cause the system's balls query to be empty, incorrectly triggering a paddle shrink even though new balls are about to spawn. The change creates a deterministic order: spawn new balls first, then detect if any balls remain for the respawn/shrink logic. This directly supports the correctness of the P1 paddle shrink fix above.
Suggestion:
// In the system registration within the plugin
app.add_systems(
Update,
(
ball_spawn_bricks_system,
apply_paddle_shrink,
)
.chain()
.run_if(in_state(GameState::Playing)),
)
// OR ensure explicit ordering if using system sets
.add_systems(
Update,
ball_spawn_bricks_system.before(RespawnSystems::Detect)
)Related Code:
// Snippet from PR description: Adds scheduling constraint
.before(RespawnSystems::Detect)📁 File: src/lib.rs
The implementation introduces a new event-driven architecture (DirectionBrickEffect event & observer) for direction bricks, which is a sound Bevy 0.17 pattern. However, the architectural risk lies in the potential for the impulse logic within mark_brick_on_ball_collision to diverge from the documented coordinate system. The evidence anchor path:docs/direction-bricks-coordinates.md provides the canonical mapping (e.g., Brick 43 = Down = Vec3::new(+5.0, 0, 0)). A mismatch between this documentation and the calculation in lib.rs would cause subtle, hard-to-debug gameplay bugs where balls move in unexpected directions. While tests exist, the P1 priority reflects the high impact of an incorrect coordinate mapping on core gameplay.
Suggestion:
// In lib.rs or a shared constants module
pub const DIRECTION_BRICK_IMPULSES: [(u8, Vec3); 7] = [
(43, Vec3::new( 5.0, 0.0, 0.0)), // Down
(44, Vec3::new( 0.0, 0.0, 5.0)), // Left
// ... etc
];Related Code:
// Snippet from PR description: Logic added to mark_brick_on_ball_collision
// Implements impulse vector calculation for all 7 brick types (43-48, 52) with RNG for randomizer📁 File: assets/textures/manifest.ron
The texture manifest contains a duplicate entry for type_id: 39 (mapped to both "brick/balls_2" and "brick/balls_3"). In a typical RON deserialization into a HashMap or similar structure, the later entry would overwrite the earlier one, leading to a loss of one intended mapping. This will cause one of the ball-indicator brick types to render with the wrong texture. This is a deterministic bug in asset configuration.
Suggestion:
( object_class: Brick, type_id: 38, profile_id: "brick/balls_1", ... ),
( object_class: Brick, type_id: 39, profile_id: "brick/balls_2", ... ),
( object_class: Brick, type_id: 40, profile_id: "brick/balls_3", ... ),
Related Code:
// From the PR diff: Adds mapping for brick type 39 twice with different profiles
(
object_class: Brick,
type_id: 39,
profile_id: "brick/balls_2",
emissive_color: None,
animation: None,
),
(
object_class: Brick,
type_id: 39,
profile_id: "brick/balls_3",
emissive_color: None,
animation: None,
),
📁 File: specs/026-direction-bricks/tasks.md
The PR includes an extensive amount of specification and planning documentation (hundreds of lines across multiple files in specs/026-direction-bricks/). While thorough documentation is valuable, bundling it with the code changes in a single PR creates significant review overhead and dilutes focus on the critical code changes (systems, events, tests). In the future, consider a more streamlined approach: 1) Merge specification docs in a separate, early PR for agreement, or 2) Place finalized specs in a docs/ directory and reference them, rather than including the full evolution and planning history in the feature PR. This improves review efficiency and maintains a cleaner commit history.
Related Code:
// This is a conceptual finding about the PR's scope. No single code snippet.💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
| fn apply_paddle_shrink( | ||
| mut life_lost_events: MessageReader<LifeLostEvent>, | ||
| paddles: Query<(Entity, &Transform), (With<Paddle>, Without<PaddleGrowing>)>, | ||
| balls: Query<Entity, With<Ball>>, | ||
| respawn_schedule: Res<RespawnSchedule>, |
There was a problem hiding this comment.
P1 | Confidence: High
The modified apply_paddle_shrink system now correctly checks for the total absence of balls before triggering the shrink animation. This fixes a critical bug where in multi-ball scenarios (e.g., after spawning power-ups), the paddle would shrink prematurely upon losing a single ball, leading to an inconsistent and overly difficult game state. The evidence anchor path:tests/paddle_shrink.rs confirms the fix's intent, as the corresponding tests have been updated to only trigger shrink when lives_remaining = 1 (implying no balls are left in play). This is a breaking change to the system's behavior, but it aligns with the expected game logic and corrects a significant gameplay flaw.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/bricks.md (1)
143-145:⚠️ Potential issue | 🟡 MinorMagnet brick indices are inconsistent with the Special Bricks table.
The Magnet System section states:
"When an enabled magnet (51) is destroyed, a random disabled magnet (52) becomes enabled"
However, the Special Bricks table (lines 109-110) defines:
- 55 = Magnet (Enabled)
- 56 = Magnet (Disabled)
Brick 51 is actually Hourglass/Slow and brick 52 is the Phone (random direction) brick per the same table.
📝 Proposed fix
### Magnet System - Only one magnet can be active at a time -- When an enabled magnet (51) is destroyed, a random disabled magnet (52) becomes enabled +- When an enabled magnet (55) is destroyed, a random disabled magnet (56) becomes enabled - The magnet creates a force field that attracts the ball
🤖 Fix all issues with AI agents
In `@assets/textures/manifest.ron`:
- Around line 1377-1388: The manifest contains two entries with object_class:
Brick and the same type_id: 39 (profile_id "brick/balls_2" and "brick/balls_3"),
which will cause one mapping to shadow the other; fix by assigning a unique
type_id to one of the entries (or merge the two profiles into a single
profile_id if they should be the same), ensuring each Brick type_id is unique
and updating any references that rely on the replaced profile_id/type_id.
- Around line 1433-1444: Variants with object_class Brick and type_id 47 and 48
reference profile_id "brick/up-right" and "brick/up-left" which don't exist;
either add matching profile entries named "brick/up-right" and "brick/up-left"
to the profiles list (ensure they include the expected texture/uv/frame data) or
change those profile_id values to existing profile IDs used by other brick
variants; update the profiles block and the variant tuples that contain
object_class: Brick, type_id: 47 and type_id: 48 accordingly so the references
resolve.
In `@specs/026-direction-bricks/checklists/acceptance.md`:
- Around line 10-15: Update the axis references in the acceptance checklist:
change Y-velocity to Z-velocity for CHK002 (brick 43, Down) and CHK005 (brick
46, Up), and update the additive impulse item CHK006 (brick 43 scenario) from
"-3.0 Y-velocity → -8.0" to "-3.0 Z-velocity → -8.0"; keep CHK003 and CHK004
(Left/Right) as X-velocity, and ensure each checklist item explicitly uses the
X/Z gameplay plane terminology for clarity.
In `@specs/026-direction-bricks/contracts/observer-event.md`:
- Around line 29-33: Update the observer contract for
apply_direction_brick_effects to match the implementation: replace the
Trigger<DirectionBrickEffect> signature with On<DirectionBrickEffect> and
replace Query<&mut LinearVelocity, With<Ball>> with Query<&mut ExternalImpulse,
With<Ball>> so the contract reflects Bevy 0.17 observer syntax and the actual
component used by the function.
- Around line 14-20: The DirectionBrickEffect event struct in the contract is
missing the impulse field; update the DirectionBrickEffect definition to include
a public impulse: Vec3 field so it matches the implementation (see
DirectionBrickEffect and the impulse used/constructed in src/lib.rs lines where
the effect is created), ensuring the struct derives Event, Clone, Debug and
contains ball_entity: Entity, brick_type: u32, brick_position: Vec3,
velocity_before: Vec3, and impulse: Vec3.
In `@specs/026-direction-bricks/data-model.md`:
- Around line 155-199: The doc example in apply_direction_brick_effects
incorrectly mutates LinearVelocity directly but the real implementation uses an
ExternalImpulse component; update the example to match by obtaining or inserting
an ExternalImpulse for the ball entity (or use a Query/Commands pattern that
mirrors the real code) and set its impulse to the intended vector instead of
writing to velocity.linvel, and adjust the tracing details to compare
velocity_before with the resulting impulse (or note that velocity_after is
produced by the physics system after applying ExternalImpulse) so the example
matches the actual ExternalImpulse-based implementation.
- Around line 113-118: The pseudocode incorrectly uses degrees for
direction_angle and zeroes the Z component; update the pseudocode to generate
direction_angle in radians (e.g., range 0.0..TAU) to match the implementation
used by cos()/sin(), and change the velocity.z assignment so it preserves the
existing Z value instead of setting it to 0.0 (i.e., keep the original z
component when computing velocity.x and velocity.y); refer to the
direction_angle variable and the velocity.z assignment to locate the lines to
fix.
In `@specs/026-direction-bricks/plan.md`:
- Around line 155-167: The plan's documented system signature is inconsistent
with the implemented function; update the docs to match the implementation by
replacing the listed signature Query<&mut LinearVelocity, With<Ball>> with the
actual signature used in brick_effects.rs: pub fn
apply_direction_brick_effects(trigger: On<DirectionBrickEffect>, mut query:
Query<&mut ExternalImpulse>), removing any mention of With<Ball> or
LinearVelocity so the plan reflects the real function parameters and types.
- Line 34: The documentation lists the bevy_rapier3d LinearVelocity component
but the implementation uses ExternalImpulse in brick_effects; update the plan to
reflect the actual approach by replacing or clarifying "LinearVelocity
component" with "ExternalImpulse (used to apply impulses/forces)" and ensure
bevy_rapier3d 0.32.0 is still cited for physics and the ExternalImpulse usage;
mention that impulses are applied via the ExternalImpulse component in the brick
effects implementation to keep docs and src/systems/brick_effects.rs consistent.
In `@specs/026-direction-bricks/quickstart.md`:
- Around line 115-166: The example currently mutates LinearVelocity directly in
apply_direction_brick_effects (and apply_randomizer), contradicting the PR which
uses ExternalImpulse; change the system to fetch the target ball entity
(DirectionBrickEffect.ball_entity) and either insert/update an ExternalImpulse
component on that entity or send an event that applies an ExternalImpulse
(rather than writing to LinearVelocity.linvel), compute impulses in the same
switch/branches (and in apply_randomizer compute an impulse vector instead of
setting linvel), and update tests to assert that ExternalImpulse was
added/updated or that the ball's velocity changed after physics integration
instead of asserting direct LinearVelocity field writes. Ensure references to
apply_direction_brick_effects, DirectionBrickEffect, apply_randomizer,
ExternalImpulse, LinearVelocity, and Ball are used to locate the changes.
- Around line 36-90: The docs/tests use the removed LinearVelocity API—update
test examples to use bevy_rapier3d v0.32.0's Velocity type and ExternalImpulse
for applying impulses: replace usages of LinearVelocity::new(...) with Velocity
{ linvel: Vec3::new(...), angvel: Vec3::ZERO } (or Velocity::linear(...)) and
change any direct velocity writes in the DirectionBrickEffect tests (e.g.,
test_brick_43_down_impulse, test_brick_44_left_impulse, etc.) to simulate the
implementation by inserting/updating ExternalImpulse on the ball entity instead
of setting LinearVelocity directly, and read results from Velocity.linvel when
asserting outcomes.
In `@specs/026-direction-bricks/tasks.md`:
- Around line 3-25: Update the prerequisites and coordinate/API terminology:
change the rand version from "rand 0.8" to "rand 0.9.2" in the prerequisites
list, replace references to the non-existent bevy_rapier3d `LinearVelocity` with
`Velocity` and mention its `linvel` field (e.g., use Velocity.linvel), and
correct the coordinate-system wording from "X and Y modification, Z preserved"
to clarify that the horizontal plane is XZ and Y is vertical (e.g., "modify X
and Z, preserve Y"). Ensure these exact symbols/phrases (rand version,
LinearVelocity -> Velocity/linvel, and XZ vs Y vertical) are updated wherever
they appear in tasks.md.
In `@src/level_loader.rs`:
- Around line 646-651: spawn_bricks_only uses an outdated condition for tagging
bricks with CountsTowardsCompletion; update the logic in spawn_bricks_only to
match spawn_level_entities_impl by checking if brick_type_id <
INDESTRUCTIBLE_BRICK || brick_type_id == BRICK_54 || brick_type_id == BRICK_50
before calling entity.insert(CountsTowardsCompletion) so bricks 50 and 54
receive the same marker during spawn transitions.
In `@src/lib.rs`:
- Around line 877-881: The computation of ball_velocity is dead code because
transforms.get(triggering_ball).map(|_t| Vec3::ZERO) always yields ZERO; either
remove ball_velocity or replace it with an actual query of the ball's Velocity
component (e.g., read the Velocity for triggering_ball before applying the
impulse) and store it in velocity_before for logging/tracing; update references
to ball_velocity to use velocity_before or drop them if unused and ensure you
use the same entity id (triggering_ball) when fetching the Velocity component
instead of reading Transforms.
- Around line 884-901: The doc/comments in src/systems/brick_effects.rs
describing brick type to axis mappings are incorrect; update the documentation
to match the actual implementation in the impulse match (where current_type
produces Vec3 impulses) so the mappings read: Brick 43 (Down) -> impulse.x +=
5.0, Brick 44 (Left) -> impulse.z += 5.0, Brick 45 (Right) -> impulse.z -= 5.0,
Brick 46 (Up) -> impulse.x -= 5.0, Brick 47 (Up-Right) -> impulse.x -= 5.0 and
impulse.z -= 5.0, Brick 48 (Up-Left) -> impulse.x -= 5.0 and impulse.z += 5.0;
ensure the doc references Vec3/X/Z axes consistently with the match arm logic
(the RNG-based brick 52 can remain as-is) and remove or correct any references
that claimed X/Y axes or different direction assignments.
In `@src/signals.rs`:
- Around line 180-193: The DirectionBrickEffect docs are inconsistent with the
code/tests: update the documentation for DirectionBrickEffect to state the
coordinate system is XZ horizontal with Y vertical (gravity), correct the
per-brick mappings so bricks 43/44 represent ±Z (left/right on XZ plane) and
bricks 45/46 represent ±Y (up/down), and change the Random brick (52)
description to apply a random impulse in the XZ plane (Z and X components, Y=0)
with magnitude in [5.0, 15.0] and direction in [0, 2π); ensure you reference the
exact brick IDs (43–48 and 52) and keep the note that no brick should modify the
vertical/gravity axis incorrectly.
In `@src/systems/respawn.rs`:
- Line 458: The doc comment above the respawn/shrink logic is wrong: it
currently says “not the last ball” but the implementation triggers the shrink
animation when all balls are gone (last ball removed). Update the documentation
in src/systems/respawn.rs to state that the shrink animation is triggered when
the last ball is removed (i.e., when no balls remain), and make the wording
around “last ball” / “all balls gone” match the actual behavior implemented by
the respawn/shrink logic.
In `@tests/direction_bricks.rs`:
- Around line 56-59: The test name test_z_axis_preserved contradicts the
implementation of apply_impulse_to_ball which currently sets
ExternalImpulse.impulse via assignment (external_impulse.impulse = impulse)
causing the Z component to be overwritten; either rename the test to reflect
overwrite semantics (e.g., test_impulse_overwrites_previous) and update the
comment near line 185, or change apply_impulse_to_ball to accumulate impulses
(e.g., add the incoming impulse to existing ExternalImpulse.impulse using
+=/addition) so the Z axis is preserved—update the test expectation accordingly
and ensure you reference the ExternalImpulse struct and the
apply_impulse_to_ball function when making the change.
- Around line 301-372: The brick 52 implementation uses the XY plane but tests
expect XZ: update the Vec3 construction in the brick 52 case in src/lib.rs to
Vec3::new(magnitude * angle.cos(), 0.0, magnitude * angle.sin()) (i.e., Y = 0)
to satisfy the tests; also replace the unseeded rand::rng() in both
test_brick_52_random_magnitude_range and
test_brick_52_random_direction_distribution with a deterministic seeded RNG (use
SeedableRng::seed_from_u64 / StdRng or similar) so the tests are not flaky and
use the same gen_range calls for floats.
In `@tests/paddle_shrink_regression.rs`:
- Around line 54-161: The test
paddle_shrinks_only_once_when_losing_multiple_balls_on_last_life has mismatched
intent: it says “last life” but sets lives.lives_remaining = 3; update the name
or comment to reflect it’s testing the “last ball” behavior (e.g., rename to
paddle_shrinks_only_once_when_losing_multiple_balls_on_last_ball) or set
lives.lives_remaining to 1 if you intended to test last-life behavior; adjust
the test doc comment accordingly so the test name, comment and the setup of
lives.lives_remaining are consistent.
🧹 Nitpick comments (6)
src/systems/ball_spawn_bricks.rs (1)
343-349: Consider expanding test coverage fory_shaped_velocity.The current test only verifies non-zero output. Consider adding tests for:
- Angle symmetry verification (left and right should be symmetric around base direction)
- Zero horizontal velocity edge case (the
min_velfallback path)- Preservation of Y component
🧪 Suggested additional test cases
#[test] fn y_shaped_velocity_returns_offsets() { let base = Vec3::new(1.0, 0.0, 0.0); let (left, right) = y_shaped_velocity(base, 37.5); assert!(left.length() > 0.0); assert!(right.length() > 0.0); } + +#[test] +fn y_shaped_velocity_preserves_y_component() { + let base = Vec3::new(1.0, 5.0, 0.0); + let (left, right) = y_shaped_velocity(base, 37.5); + assert!((left.y - 5.0).abs() < f32::EPSILON); + assert!((right.y - 5.0).abs() < f32::EPSILON); +} + +#[test] +fn y_shaped_velocity_handles_zero_horizontal() { + let base = Vec3::new(0.0, 1.0, 0.0); + let (left, right) = y_shaped_velocity(base, 37.5); + assert!(left.length() > 0.0); + assert!(right.length() > 0.0); + // Should return opposing minimal horizontal velocities + assert!((left.x + right.x).abs() < f32::EPSILON); +}specs/026-direction-bricks/plan.md (1)
105-108: Minor inconsistency in angle units.Line 106 correctly states to use radians (
0.0..TAU), but Line 24 and other places reference "0-360°" (degrees). The implementation correctly uses radians. Consider updating the degree references to radians for consistency throughout the document.src/systems/brick_effects.rs (2)
44-50: Documentation describes velocity modification but implementation uses ExternalImpulse.The doc comments describe direct velocity modification:
/// - Brick 43 (Left): `velocity.x -= 5.0` /// - Brick 44 (Right): `velocity.x += 5.0`But the actual implementation sets
ExternalImpulse.impulse, which the physics system then integrates. Consider updating the documentation to reflect the actual behavior, or note that the impulse achieves an equivalent effect.
138-141: Consider addingWith<Ball>filter for defensive querying.The query
Query<&mut ExternalImpulse>could theoretically match any entity withExternalImpulse. Whileeffect.ball_entitytargets the specific entity, addingWith<Ball>filter provides an extra safety check that only ball entities can have impulses applied:pub fn apply_direction_brick_effects( trigger: On<DirectionBrickEffect>, - mut query: Query<&mut ExternalImpulse>, + mut query: Query<&mut ExternalImpulse, With<Ball>>, )This would require importing
Ballfrom the crate root.specs/026-direction-bricks/contracts/observer-event.md (1)
143-154: Physics integration details are accurate.The description of
LinearVelocitycomponent properties is informative, though the actual implementation usesExternalImpulserather than direct velocity modification. The physics propagation description correctly explains how changes flow through the system.specs/026-direction-bricks/quickstart.md (1)
343-365: Update troubleshooting guidance to match correct implementation.The troubleshooting section at line 352 references checking for the
LinearVelocitycomponent, which should be updated to match the actual component used in the implementation (likelyVelocityorExternalImpulsebased on the PR summary).Additionally, consider adding troubleshooting guidance for:
- Physics integration timing (impulses applied vs velocity changes observable)
- Frame-by-frame behavior and when velocity changes take effect
- Differences between impulse application and direct velocity writes
| ( | ||
| object_class: Brick, | ||
| type_id: 47, | ||
| profile_id: "brick/up-right", | ||
| emissive_color: None, | ||
| animation: None, | ||
| ), | ||
| ( | ||
| object_class: Brick, | ||
| type_id: 48, | ||
| profile_id: "brick/up-left", | ||
| emissive_color: None, |
There was a problem hiding this comment.
Missing profiles for brick/up-right and brick/up-left.
Type variants for bricks 47 and 48 reference profile IDs that don’t exist in the profiles list. This will likely fail to resolve or fall back unexpectedly. Add the missing profiles or point these variants at existing ones.
🤖 Prompt for AI Agents
In `@assets/textures/manifest.ron` around lines 1433 - 1444, Variants with
object_class Brick and type_id 47 and 48 reference profile_id "brick/up-right"
and "brick/up-left" which don't exist; either add matching profile entries named
"brick/up-right" and "brick/up-left" to the profiles list (ensure they include
the expected texture/uv/frame data) or change those profile_id values to
existing profile IDs used by other brick variants; update the profiles block and
the variant tuples that contain object_class: Brick, type_id: 47 and type_id: 48
accordingly so the references resolve.
| - [x] CHK001 - Are all single-direction brick scenarios (43-46) specified with measurable initial velocity and expected final velocity values? [Clarity, Spec §User Story 1] | ||
| - [x] CHK002 - Can brick 43 (Down) scenario be tested by asserting Y-velocity decreased by exactly 5.0 units/sec? [Measurability, Spec §US1-Acceptance-1] | ||
| - [x] CHK003 - Can brick 44 (Left) scenario be tested by asserting X-velocity decreased by exactly 5.0 units/sec? [Measurability, Spec §US1-Acceptance-2] | ||
| - [x] CHK004 - Can brick 45 (Right) scenario be tested by asserting X-velocity increased by exactly 5.0 units/sec? [Measurability, Spec §US1-Acceptance-3] | ||
| - [x] CHK005 - Can brick 46 (Up) scenario be tested by asserting Y-velocity increased by exactly 5.0 units/sec? [Measurability, Spec §US1-Acceptance-4] | ||
| - [x] CHK006 - Does the additive impulse scenario (brick 43 on -3.0 Y-velocity → -8.0) provide concrete numbers for verification? [Clarity, Spec §US1-Acceptance-5] |
There was a problem hiding this comment.
Acceptance checks use X/Y axes, conflicting with XZ gameplay plane.
CHK002–CHK005 assert X/Y changes for down/left/right/up, but the coordinate system elsewhere is XZ (Y vertical). This will mis-specify tests. Please update these checks to X/Z.
Suggested correction
- - [x] CHK002 - Can brick 43 (Down) scenario be tested by asserting Y-velocity decreased by exactly 5.0 units/sec? [Measurability, Spec §US1-Acceptance-1]
- - [x] CHK003 - Can brick 44 (Left) scenario be tested by asserting X-velocity decreased by exactly 5.0 units/sec? [Measurability, Spec §US1-Acceptance-2]
- - [x] CHK004 - Can brick 45 (Right) scenario be tested by asserting X-velocity increased by exactly 5.0 units/sec? [Measurability, Spec §US1-Acceptance-3]
- - [x] CHK005 - Can brick 46 (Up) scenario be tested by asserting Y-velocity increased by exactly 5.0 units/sec? [Measurability, Spec §US1-Acceptance-4]
+ - [x] CHK002 - Can brick 43 (Down) scenario be tested by asserting X-velocity increased by exactly 5.0 units/sec? [Measurability, Spec §US1-Acceptance-1]
+ - [x] CHK003 - Can brick 44 (Left) scenario be tested by asserting Z-velocity increased by exactly 5.0 units/sec? [Measurability, Spec §US1-Acceptance-2]
+ - [x] CHK004 - Can brick 45 (Right) scenario be tested by asserting Z-velocity decreased by exactly 5.0 units/sec? [Measurability, Spec §US1-Acceptance-3]
+ - [x] CHK005 - Can brick 46 (Up) scenario be tested by asserting X-velocity decreased by exactly 5.0 units/sec? [Measurability, Spec §US1-Acceptance-4]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - [x] CHK001 - Are all single-direction brick scenarios (43-46) specified with measurable initial velocity and expected final velocity values? [Clarity, Spec §User Story 1] | |
| - [x] CHK002 - Can brick 43 (Down) scenario be tested by asserting Y-velocity decreased by exactly 5.0 units/sec? [Measurability, Spec §US1-Acceptance-1] | |
| - [x] CHK003 - Can brick 44 (Left) scenario be tested by asserting X-velocity decreased by exactly 5.0 units/sec? [Measurability, Spec §US1-Acceptance-2] | |
| - [x] CHK004 - Can brick 45 (Right) scenario be tested by asserting X-velocity increased by exactly 5.0 units/sec? [Measurability, Spec §US1-Acceptance-3] | |
| - [x] CHK005 - Can brick 46 (Up) scenario be tested by asserting Y-velocity increased by exactly 5.0 units/sec? [Measurability, Spec §US1-Acceptance-4] | |
| - [x] CHK006 - Does the additive impulse scenario (brick 43 on -3.0 Y-velocity → -8.0) provide concrete numbers for verification? [Clarity, Spec §US1-Acceptance-5] | |
| - [x] CHK001 - Are all single-direction brick scenarios (43-46) specified with measurable initial velocity and expected final velocity values? [Clarity, Spec §User Story 1] | |
| - [x] CHK002 - Can brick 43 (Down) scenario be tested by asserting X-velocity increased by exactly 5.0 units/sec? [Measurability, Spec §US1-Acceptance-1] | |
| - [x] CHK003 - Can brick 44 (Left) scenario be tested by asserting Z-velocity increased by exactly 5.0 units/sec? [Measurability, Spec §US1-Acceptance-2] | |
| - [x] CHK004 - Can brick 45 (Right) scenario be tested by asserting Z-velocity decreased by exactly 5.0 units/sec? [Measurability, Spec §US1-Acceptance-3] | |
| - [x] CHK005 - Can brick 46 (Up) scenario be tested by asserting X-velocity decreased by exactly 5.0 units/sec? [Measurability, Spec §US1-Acceptance-4] | |
| - [x] CHK006 - Does the additive impulse scenario (brick 43 on -3.0 Y-velocity → -8.0) provide concrete numbers for verification? [Clarity, Spec §US1-Acceptance-5] |
🤖 Prompt for AI Agents
In `@specs/026-direction-bricks/checklists/acceptance.md` around lines 10 - 15,
Update the axis references in the acceptance checklist: change Y-velocity to
Z-velocity for CHK002 (brick 43, Down) and CHK005 (brick 46, Up), and update the
additive impulse item CHK006 (brick 43 scenario) from "-3.0 Y-velocity → -8.0"
to "-3.0 Z-velocity → -8.0"; keep CHK003 and CHK004 (Left/Right) as X-velocity,
and ensure each checklist item explicitly uses the X/Z gameplay plane
terminology for clarity.
| #[derive(Event, Clone, Debug)] | ||
| pub struct DirectionBrickEffect { | ||
| pub ball_entity: Entity, | ||
| pub brick_type: u32, | ||
| pub brick_position: Vec3, | ||
| pub velocity_before: Vec3, | ||
| } |
There was a problem hiding this comment.
DirectionBrickEffect struct missing impulse field.
The contract shows the event struct without the impulse field, but based on the implementation in src/lib.rs (lines 903-909), the actual struct includes an impulse: Vec3 field that carries the computed impulse vector. Update the contract to include this field:
pub struct DirectionBrickEffect {
pub ball_entity: Entity,
pub brick_type: u32,
pub brick_position: Vec3,
pub velocity_before: Vec3,
+ pub impulse: Vec3,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[derive(Event, Clone, Debug)] | |
| pub struct DirectionBrickEffect { | |
| pub ball_entity: Entity, | |
| pub brick_type: u32, | |
| pub brick_position: Vec3, | |
| pub velocity_before: Vec3, | |
| } | |
| #[derive(Event, Clone, Debug)] | |
| pub struct DirectionBrickEffect { | |
| pub ball_entity: Entity, | |
| pub brick_type: u32, | |
| pub brick_position: Vec3, | |
| pub velocity_before: Vec3, | |
| pub impulse: Vec3, | |
| } |
🤖 Prompt for AI Agents
In `@specs/026-direction-bricks/contracts/observer-event.md` around lines 14 - 20,
The DirectionBrickEffect event struct in the contract is missing the impulse
field; update the DirectionBrickEffect definition to include a public impulse:
Vec3 field so it matches the implementation (see DirectionBrickEffect and the
impulse used/constructed in src/lib.rs lines where the effect is created),
ensuring the struct derives Event, Clone, Debug and contains ball_entity:
Entity, brick_type: u32, brick_position: Vec3, velocity_before: Vec3, and
impulse: Vec3.
| ```rust | ||
| pub fn apply_direction_brick_effects( | ||
| trigger: Trigger<DirectionBrickEffect>, | ||
| mut query: Query<&mut LinearVelocity, With<Ball>>, | ||
| ) |
There was a problem hiding this comment.
Observer signature doesn't match implementation.
The contract shows:
trigger: Trigger<DirectionBrickEffect>,
mut query: Query<&mut LinearVelocity, With<Ball>>,But the actual implementation uses:
trigger: On<DirectionBrickEffect>,
mut query: Query<&mut ExternalImpulse>,Update the contract to reflect the actual implementation using On<T> (Bevy 0.17 observer syntax) and ExternalImpulse.
🤖 Prompt for AI Agents
In `@specs/026-direction-bricks/contracts/observer-event.md` around lines 29 - 33,
Update the observer contract for apply_direction_brick_effects to match the
implementation: replace the Trigger<DirectionBrickEffect> signature with
On<DirectionBrickEffect> and replace Query<&mut LinearVelocity, With<Ball>> with
Query<&mut ExternalImpulse, With<Ball>> so the contract reflects Bevy 0.17
observer syntax and the actual component used by the function.
| fn apply_impulse_to_ball(app: &mut App, ball: Entity, impulse: Vec3) -> Vec3 { | ||
| if let Some(mut external_impulse) = app.world_mut().get_mut::<ExternalImpulse>(ball) { | ||
| external_impulse.impulse = impulse; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l tests/direction_bricks.rsRepository: cleder/brkrs
Length of output: 85
🏁 Script executed:
cat -n tests/direction_bricks.rsRepository: cleder/brkrs
Length of output: 16396
Test name contradicts assertion: "preserved" vs. overwrite behavior.
apply_impulse_to_ball overwrites ExternalImpulse.impulse with =, so the second call replaces the Z impulse with 0. The test test_z_axis_preserved has a misleading name—it actually verifies overwriting, not preservation. Either rename the test to reflect overwrite semantics (e.g., test_impulse_overwrites_previous) or change the implementation to use += if accumulation is intended.
The comment on line 185 acknowledges this: "The second call overwrites, so Z is zero"—but the test name suggests the opposite.
🤖 Prompt for AI Agents
In `@tests/direction_bricks.rs` around lines 56 - 59, The test name
test_z_axis_preserved contradicts the implementation of apply_impulse_to_ball
which currently sets ExternalImpulse.impulse via assignment
(external_impulse.impulse = impulse) causing the Z component to be overwritten;
either rename the test to reflect overwrite semantics (e.g.,
test_impulse_overwrites_previous) and update the comment near line 185, or
change apply_impulse_to_ball to accumulate impulses (e.g., add the incoming
impulse to existing ExternalImpulse.impulse using +=/addition) so the Z axis is
preserved—update the test expectation accordingly and ensure you reference the
ExternalImpulse struct and the apply_impulse_to_ball function when making the
change.
| /// T026: Brick 52 (Random) applies impulse with magnitude in range [5.0, 15.0] | ||
| #[test] | ||
| fn test_brick_52_random_magnitude_range() { | ||
| use rand::Rng; | ||
|
|
||
| // Test multiple samples to verify RNG distribution | ||
| let mut rng = rand::rng(); | ||
| for _ in 0..100 { | ||
| // Simulate the RNG logic from lib.rs brick 52 case | ||
| let magnitude = rng.random_range(5.0..15.0); | ||
| let angle: f32 = rng.random_range(0.0..std::f32::consts::TAU); | ||
| let impulse = Vec3::new(magnitude * angle.cos(), 0.0, magnitude * angle.sin()); | ||
| let magnitude_computed = (impulse.x * impulse.x + impulse.z * impulse.z).sqrt(); | ||
|
|
||
| assert!( | ||
| magnitude_computed >= 5.0, | ||
| "Magnitude should be >= 5.0, got {:.2}", | ||
| magnitude_computed | ||
| ); | ||
| assert!( | ||
| magnitude_computed < 15.0, | ||
| "Magnitude should be < 15.0, got {:.2}", | ||
| magnitude_computed | ||
| ); | ||
| assert_eq!( | ||
| impulse.y, 0.0, | ||
| "Y component should be zero for random brick (XZ plane only)" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| /// T027: Brick 52 (Random) produces varied impulse directions across 0-2π range | ||
| #[test] | ||
| fn test_brick_52_random_direction_distribution() { | ||
| use rand::Rng; | ||
|
|
||
| // Collect samples and verify we get varied angles | ||
| let mut rng = rand::rng(); | ||
| let mut angles = Vec::new(); | ||
| let mut magnitudes = Vec::new(); | ||
|
|
||
| for _ in 0..50 { | ||
| let magnitude = rng.random_range(5.0..15.0); | ||
| let angle: f32 = rng.random_range(0.0..std::f32::consts::TAU); | ||
| let impulse = Vec3::new(magnitude * angle.cos(), 0.0, magnitude * angle.sin()); | ||
| let computed_angle = impulse.z.atan2(impulse.x); | ||
| let computed_magnitude = (impulse.x * impulse.x + impulse.z * impulse.z).sqrt(); | ||
|
|
||
| angles.push(computed_angle); | ||
| magnitudes.push(computed_magnitude); | ||
| } | ||
|
|
||
| // Verify we have diverse angles (not all clustered in one direction) | ||
| let min_angle = angles.iter().fold(f32::INFINITY, |a, &b| a.min(b)); | ||
| let max_angle = angles.iter().fold(f32::NEG_INFINITY, |a, &b| a.max(b)); | ||
| let angle_range = (max_angle - min_angle).abs(); | ||
| assert!( | ||
| angle_range > 3.0, // At least ~π radians of spread | ||
| "Random angles should be distributed across range, got range {:.2}", | ||
| angle_range | ||
| ); | ||
|
|
||
| // Verify we have diverse magnitudes | ||
| let min_mag = magnitudes.iter().fold(f32::INFINITY, |a, &b| a.min(b)); | ||
| let max_mag = magnitudes.iter().fold(f32::NEG_INFINITY, |a, &b| a.max(b)); | ||
| let mag_range = max_mag - min_mag; | ||
| assert!( | ||
| mag_range > 5.0, | ||
| "Random magnitudes should be distributed, got range {:.2}", | ||
| mag_range | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Explore repository structure
find . -type f -name "*.rs" | head -20Repository: cleder/brkrs
Length of output: 669
🏁 Script executed:
# Look at the test file to understand the full context
wc -l tests/direction_bricks.rsRepository: cleder/brkrs
Length of output: 85
🏁 Script executed:
# Check for lib.rs location
find . -type f -name "lib.rs"Repository: cleder/brkrs
Length of output: 106
🏁 Script executed:
# Read the full test file to understand context
cat -n tests/direction_bricks.rsRepository: cleder/brkrs
Length of output: 16396
🏁 Script executed:
# Search for brick 52 implementation in lib.rs
grep -n "brick 52\|brick_52\|52 =>" src/lib.rs -i | head -20Repository: cleder/brkrs
Length of output: 95
🏁 Script executed:
# Also search for the random case in lib.rs to see the actual implementation
rg -n "case 52|brick.*(52|random)" src/lib.rs -i -A 10 | head -50Repository: cleder/brkrs
Length of output: 695
🏁 Script executed:
# Get more context around brick 52 implementation
sed -n '875,950p' src/lib.rs | cat -nRepository: cleder/brkrs
Length of output: 4627
🏁 Script executed:
# Check if there are any other tests using seeded RNG in the codebase
rg "StdRng|seed_from_u64|SeedableRng" tests/ --type rsRepository: cleder/brkrs
Length of output: 82
🏁 Script executed:
# Check rand crate documentation and typical patterns for test flakiness
rg "rand::rng\|random_range" tests/ --type rs -B 2 -A 2Repository: cleder/brkrs
Length of output: 82
🏁 Script executed:
# Check for seeded RNG usage in tests directory
rg "StdRng|seed_from_u64|SeedableRng" tests/Repository: cleder/brkrs
Length of output: 38
🏁 Script executed:
# Check other RNG patterns in tests
rg "rand::rng|random_range" tests/ -B 2 -A 2Repository: cleder/brkrs
Length of output: 5598
🏁 Script executed:
# Check Cargo.toml to see rand version
grep -A 5 "\[dependencies\]" Cargo.toml | head -20Repository: cleder/brkrs
Length of output: 193
🏁 Script executed:
# Search for any #[ignore] or #[skip] marks on these tests
grep -B 3 "fn test_brick_52_random" tests/direction_bricks.rsRepository: cleder/brkrs
Length of output: 332
🏁 Script executed:
# Get the exact impulse construction in the test to confirm XY vs XZ mismatch
sed -n '301,330p' tests/direction_bricks.rs | grep -A 5 "Vec3::new"Repository: cleder/brkrs
Length of output: 342
🏁 Script executed:
# Check the exact implementation from lib.rs again for brick 52
sed -n '891,925p' src/lib.rs | grep -A 5 "52 =>"Repository: cleder/brkrs
Length of output: 453
🏁 Script executed:
# Try running cargo test on these specific tests to see if they pass
cd . && cargo test test_brick_52 --lib 2>&1 | head -100Repository: cleder/brkrs
Length of output: 302
🏁 Script executed:
# Verify the exact coordinate discrepancy between implementation and test
echo "=== Test expectation ===" && grep -A 2 "impulse = Vec3::new" tests/direction_bricks.rs | head -3
echo ""
echo "=== Implementation ===" && sed -n '917,924p' src/lib.rsRepository: cleder/brkrs
Length of output: 629
🏁 Script executed:
# Check if there are any skip/ignore attributes or if tests are in a separate module
head -30 tests/direction_bricks.rs | grep -E "mod|#\[|ignore"Repository: cleder/brkrs
Length of output: 152
🏁 Script executed:
# Look for comment in lib.rs about XY plane to confirm the mismatch
grep -B 2 -A 2 "XY plane" src/lib.rsRepository: cleder/brkrs
Length of output: 390
🏁 Script executed:
# Get the exact line 924 context from lib.rs to confirm
sed -n '917,925p' src/lib.rs | cat -nRepository: cleder/brkrs
Length of output: 640
🏁 Script executed:
# Double-check the test impulse line
sed -n '310,315p' tests/direction_bricks.rs | cat -nRepository: cleder/brkrs
Length of output: 416
🏁 Script executed:
# Check the file header comments to see which plane should be used
head -20 tests/direction_bricks.rs | grep -i "plane\|coordinate"Repository: cleder/brkrs
Length of output: 222
🏁 Script executed:
# Search for coordinate system documentation in the codebase
rg "coordinate|XY plane|XZ plane" src/ tests/ -i | head -20Repository: cleder/brkrs
Length of output: 1918
Fix coordinate system mismatch in brick 52 implementation: tests expect XZ plane, not XY plane.
The implementation in src/lib.rs (line 924) incorrectly uses the XY plane (Vec3::new(magnitude * angle.cos(), magnitude * angle.sin(), 0.0)), but the test file header and all test assertions expect the XZ plane (Vec3::new(magnitude * angle.cos(), 0.0, magnitude * angle.sin())). The test at line 327 explicitly asserts that the Y component must be zero, which the current implementation violates.
Additionally, replace unseeded rand::rng() with a deterministic seeded RNG to prevent test flakiness:
- use rand::Rng;
+ use rand::{Rng, SeedableRng};
+ use rand::rngs::StdRng;
@@
- let mut rng = rand::rng();
+ let mut rng = StdRng::seed_from_u64(42);Apply the same seeding fix to both test_brick_52_random_magnitude_range and test_brick_52_random_direction_distribution.
🤖 Prompt for AI Agents
In `@tests/direction_bricks.rs` around lines 301 - 372, The brick 52
implementation uses the XY plane but tests expect XZ: update the Vec3
construction in the brick 52 case in src/lib.rs to Vec3::new(magnitude *
angle.cos(), 0.0, magnitude * angle.sin()) (i.e., Y = 0) to satisfy the tests;
also replace the unseeded rand::rng() in both
test_brick_52_random_magnitude_range and
test_brick_52_random_direction_distribution with a deterministic seeded RNG (use
SeedableRng::seed_from_u64 / StdRng or similar) so the tests are not flaky and
use the same gen_range calls for floats.
| fn paddle_shrinks_only_once_when_losing_multiple_balls_on_last_life() { | ||
| let mut app = test_app(); | ||
| let lower_goal = app.world_mut().spawn(LowerGoal).id(); | ||
|
|
||
| // Set lives to 1 so any ball loss that removes all balls would trigger shrink | ||
| { | ||
| let mut lives = app | ||
| .world_mut() | ||
| .resource_mut::<brkrs::systems::respawn::LivesState>(); | ||
| lives.lives_remaining = 3; | ||
| } | ||
|
|
||
| // Spawn multiple balls | ||
| let ball1 = app | ||
| .world_mut() | ||
| .spawn((Ball, ball_handle_at(Vec3::new(0.0, 2.0, 0.0)))) | ||
| .id(); | ||
| let ball2 = app | ||
| .world_mut() | ||
| .spawn((Ball, ball_handle_at(Vec3::new(2.0, 2.0, 0.0)))) | ||
| .id(); | ||
| let ball3 = app | ||
| .world_mut() | ||
| .spawn((Ball, ball_handle_at(Vec3::new(-2.0, 2.0, 0.0)))) | ||
| .id(); | ||
|
|
||
| let paddle = app | ||
| .world_mut() | ||
| .spawn(( | ||
| Paddle, | ||
| Transform::from_scale(Vec3::ONE), | ||
| Velocity::zero(), | ||
| paddle_handle_at(Vec3::new(0.0, 2.0, 0.0)), | ||
| )) | ||
| .id(); | ||
|
|
||
| // Trigger loss of first ball ONLY | ||
| app.world_mut() | ||
| .resource_mut::<Messages<CollisionEvent>>() | ||
| .write(CollisionEvent::Started( | ||
| ball1, | ||
| lower_goal, | ||
| CollisionEventFlags::SENSOR, | ||
| )); | ||
|
|
||
| advance_time(&mut app, 0.016); | ||
| app.update(); | ||
|
|
||
| // Verify paddle does NOT have PaddleGrowing because other balls still exist | ||
| // (other balls exist, so paddle should NOT shrink even though a ball was lost) | ||
| assert!( | ||
| app.world().entity(paddle).get::<PaddleGrowing>().is_none(), | ||
| "Paddle should NOT shrink when other balls still exist, even if a ball was lost" | ||
| ); | ||
|
|
||
| // Trigger loss of second ball | ||
| app.world_mut() | ||
| .resource_mut::<Messages<CollisionEvent>>() | ||
| .write(CollisionEvent::Started( | ||
| ball2, | ||
| lower_goal, | ||
| CollisionEventFlags::SENSOR, | ||
| )); | ||
|
|
||
| advance_time(&mut app, 0.016); | ||
| app.update(); | ||
|
|
||
| // Still should not shrink (ball3 still exists) | ||
| assert!( | ||
| app.world().entity(paddle).get::<PaddleGrowing>().is_none(), | ||
| "Paddle should NOT shrink when other balls still exist" | ||
| ); | ||
|
|
||
| // Trigger loss of last ball | ||
| app.world_mut() | ||
| .resource_mut::<Messages<CollisionEvent>>() | ||
| .write(CollisionEvent::Started( | ||
| ball3, | ||
| lower_goal, | ||
| CollisionEventFlags::SENSOR, | ||
| )); | ||
|
|
||
| advance_time(&mut app, 0.016); | ||
| app.update(); | ||
|
|
||
| // Now paddle SHOULD have PaddleGrowing because all balls are gone (last ball lost) | ||
| let growing = app.world().entity(paddle).get::<PaddleGrowing>(); | ||
|
|
||
| assert!( | ||
| growing.is_some(), | ||
| "Paddle should shrink when the last ball is lost (all balls gone), regardless of lives count" | ||
| ); | ||
|
|
||
| // Verify the shrink component is configured correctly | ||
| let growing_component = growing.unwrap(); | ||
|
|
||
| assert_eq!( | ||
| growing_component.start_scale, | ||
| Vec3::ONE, | ||
| "Paddle shrink should start from full size (Vec3::ONE)" | ||
| ); | ||
|
|
||
| assert_eq!( | ||
| growing_component.target_scale, | ||
| Vec3::splat(0.01), | ||
| "Paddle shrink should target very small size (0.01)" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Align test name/comment with the lives setup.
The test name/comment mention “last life,” but lives_remaining is set to 3. Consider renaming or updating the comment to reflect “last ball” behavior.
✏️ Suggested rename/comment alignment
-fn paddle_shrinks_only_once_when_losing_multiple_balls_on_last_life() {
+fn paddle_shrinks_only_once_when_losing_multiple_balls() {
@@
- // Set lives to 1 so any ball loss that removes all balls would trigger shrink
+ // Keep lives > 1 to show shrink is tied to "last ball" rather than "last life"Optional: Codacy flags this test as >50 lines; consider extracting a helper if you want to quiet that warning.
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[warning] 54-54: tests/paddle_shrink_regression.rs#L54
Method paddle_shrinks_only_once_when_losing_multiple_balls_on_last_life has 82 lines of code (limit is 50)
🤖 Prompt for AI Agents
In `@tests/paddle_shrink_regression.rs` around lines 54 - 161, The test
paddle_shrinks_only_once_when_losing_multiple_balls_on_last_life has mismatched
intent: it says “last life” but sets lives.lives_remaining = 3; update the name
or comment to reflect it’s testing the “last ball” behavior (e.g., rename to
paddle_shrinks_only_once_when_losing_multiple_balls_on_last_ball) or set
lives.lives_remaining to 1 if you intended to test last-life behavior; adjust
the test doc comment accordingly so the test name, comment and the setup of
lives.lives_remaining are consistent.
… gameplay dynamics
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/systems/brick_effects.rs`:
- Around line 43-55: The module docs for the cardinal-direction bricks are using
X/Y axis language but the implementation uses X/Z; update the comment block
describing bricks 43–48 so each mapping matches the actual Vec3 impulses in
src/lib.rs (e.g. the Vec3::new(...) entries used for Brick 43, 44, etc.) and
align wording with src/signals.rs (use X and Z axis labels and correct
directions for Left/Right/Forward/Backward/Up/Down); keep the Random Direction
(Brick 52) description but note it replaces X and Z (not X and Y) and preserves
Y or Z as per the implementation so the docs accurately reflect the code.
🧹 Nitpick comments (1)
tests/direction_bricks.rs (1)
301-330: Unseeded RNG may cause non-deterministic test behavior.Using
rand::rng()without seeding could lead to flaky tests in certain CI environments or when debugging failures. While the test checks bounds rather than exact values, seeded RNG improves reproducibility.🔧 Suggested improvement
+use rand::SeedableRng; +use rand::rngs::StdRng; + fn test_brick_52_random_magnitude_range() { - use rand::Rng; - - let mut rng = rand::rng(); + use rand::Rng; + let mut rng = StdRng::seed_from_u64(42);
| /// | ||
| /// **Cardinal Directions (Bricks 43-48)**: 5.0 units/sec additive impulse per brick type | ||
| /// - Brick 43 (Left): `velocity.x -= 5.0` | ||
| /// - Brick 44 (Right): `velocity.x += 5.0` | ||
| /// - Brick 45 (Up): `velocity.y += 5.0` | ||
| /// - Brick 46 (Down): `velocity.y -= 5.0` | ||
| /// - Brick 47 (Forward): `velocity.z += 5.0` | ||
| /// - Brick 48 (Backward): `velocity.z -= 5.0` | ||
| /// | ||
| /// **Random Direction (Brick 52)**: Magnitude 5.0-15.0 units/sec, direction 0.0..2π radians | ||
| /// - Replaces both X and Y velocities (NOT additive) | ||
| /// - Preserves Z velocity | ||
| /// - Uses seeded RNG for deterministic behavior in tests |
There was a problem hiding this comment.
Module docs describe incorrect axis mappings.
The documentation (lines 43-50) describes brick behaviors using X/Y axes:
- "Brick 43 (Left):
velocity.x -= 5.0" - "Brick 45 (Up):
velocity.y += 5.0"
But the actual implementation in src/lib.rs uses X/Z axes:
- Brick 43:
Vec3::new(5.0, 0.0, 0.0)(Forward, +X) - Brick 44:
Vec3::new(0.0, 0.0, 5.0)(Left, +Z) - etc.
Update the docs to match the implementation and align with src/signals.rs which has the correct mappings.
📝 Suggested correction
/// **Cardinal Directions (Bricks 43-48)**: 5.0 units/sec additive impulse per brick type
-/// - Brick 43 (Left): `velocity.x -= 5.0`
-/// - Brick 44 (Right): `velocity.x += 5.0`
-/// - Brick 45 (Up): `velocity.y += 5.0`
-/// - Brick 46 (Down): `velocity.y -= 5.0`
-/// - Brick 47 (Forward): `velocity.z += 5.0`
-/// - Brick 48 (Backward): `velocity.z -= 5.0`
+/// - Brick 43 (Forward): `impulse.x += 5.0` (toward far wall)
+/// - Brick 44 (Left): `impulse.z += 5.0`
+/// - Brick 45 (Right): `impulse.z -= 5.0`
+/// - Brick 46 (Backward): `impulse.x -= 5.0` (toward paddle)
+/// - Brick 47 (Backward-Right): `impulse.x -= 5.0, impulse.z -= 5.0`
+/// - Brick 48 (Backward-Left): `impulse.x -= 5.0, impulse.z += 5.0`🤖 Prompt for AI Agents
In `@src/systems/brick_effects.rs` around lines 43 - 55, The module docs for the
cardinal-direction bricks are using X/Y axis language but the implementation
uses X/Z; update the comment block describing bricks 43–48 so each mapping
matches the actual Vec3 impulses in src/lib.rs (e.g. the Vec3::new(...) entries
used for Brick 43, 44, etc.) and align wording with src/signals.rs (use X and Z
axis labels and correct directions for Left/Right/Forward/Backward/Up/Down);
keep the Random Direction (Brick 52) description but note it replaces X and Z
(not X and Y) and preserves Y or Z as per the implementation so the docs
accurately reflect the code.
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
PR Type
Enhancement, Tests, Bug fix, Documentation
Description
Implements direction brick impulse system (bricks 43-48, 52) with observer pattern for instantaneous velocity modifications
Adds
DirectionBrickEffectevent signal andapply_direction_brick_effectsobserver system to apply impulses viaExternalImpulsecomponentFixes paddle shrink logic to only trigger when ALL balls are lost, not on individual ball loss in multi-ball scenarios
Adds comprehensive test suite covering direction brick impulses, paddle shrink edge cases, and multi-ball interactions
Updates level loader to include bricks 50 and 54 in completion counting logic
Provides extensive specification documentation with TDD requirements, acceptance criteria, and implementation planning
Creates GIMP texture assets for ball indicators (1-3 balls) with emissive variants and direction brick material profiles
Adds test level 099 featuring all direction brick types and updates pre-commit hooks and typos configuration
Diagram Walkthrough
File Walkthrough
4 files
direction_bricks.rs
Direction brick impulse unit and integration teststests/direction_bricks.rs
43-48, 52)
ExternalImpulsecomponent withoutfull physics simulation
random direction (52) bricks
paddle_shrink_multiball.rs
Paddle shrink regression tests for multi-ball scenariostests/paddle_shrink_multiball.rs
paddle_shrink_regression.rs
Paddle shrink edge case and regression testingtests/paddle_shrink_regression.rs
life
level_099.ron
Direction Brick Test Level 099assets/levels/level_099.ron
grid layout
descriptive notes
mechanics
6 files
brick_effects.rs
Direction brick observer system implementationsrc/systems/brick_effects.rs
52)
apply_direction_brick_effectsobserver function triggeredby
DirectionBrickEffecteventsExternalImpulsecomponent with propertracing instrumentation
direction brick types
signals.rs
Direction brick effect event signal definitionsrc/signals.rs
DirectionBrickEffectevent type for immediate velocity impulsesvia observer pattern
and random (52) bricks
types (buffered)
lib.rs
Direction brick collision detection and observer registrationsrc/lib.rs
apply_direction_brick_effectsobserver in core observersetup
mark_brick_on_ball_collisionsystem52) with RNG for randomizer
queue_silencedfor entity despawn commands to avoid commandconflicts
mod.rs
Systems module updates for brick effectssrc/systems/mod.rs
brick_effectsmodule for direction brick observer systempublic use
level_loader.rs
Level loader brick type completion countingsrc/level_loader.rs
destructible
CountsTowardsCompletionfilter1-balls-emissive.xcf
GIMP Emissive Texture Asset for Ball Indicatorassets/textures/gimp/1-balls-emissive.xcf
#ff4660)
2 files
paddle_shrink.rs
Paddle shrink test fixes for lives state validationtests/paddle_shrink.rs
lives_remaining = 1beforetriggering ball loss
== 1 condition)
shrink
respawn.rs
Paddle shrink logic fix for multi-ball scenariossrc/systems/respawn.rs
apply_paddle_shrinkto only trigger when ALL balls are gone(not just one ball lost)
ballsquery to check remaining ball count before applying shrinkanimation
same frame
InputLockedcomponent alongsidePaddleGrowingfor consistentinput locking
2 files
merkaba.rs
Merkaba system visibility adjustmentsrc/systems/merkaba.rs
despawn_balls_and_merkabas_on_life_lossfunction public for testaccess
2-balls.xcf
GIMP texture asset fileassets/textures/gimp/2-balls.xcf
3 files
ball_spawn_bricks.rs
Ball spawn system scheduling refinementsrc/systems/ball_spawn_bricks.rs
respawn detection
.before(RespawnSystems::Detect)to establish proper systemordering
_typos.toml
Typos configuration dictionary extension_typos.toml
conststo the typos dictionary to prevent false positives on Rustconstskeywordrumdl,brkrs)manifest.ron
Direction Brick Material Profiles and Type Mappingsassets/textures/manifest.ron
brick/balls_1,brick/balls_2,brick/balls_3,brick/up,brick/down,brick/left,brick/rightright, up, up-right, up-left)
bricks
corresponding profiles
12 files
tasks.md
Direction bricks feature task specification and planningspecs/026-direction-bricks/tasks.md
implementation
persistence, integration, documentation, acceptance)
0.17 compliance checkpoints
development
bricks.md
Brick documentation implementation status updatedocs/bricks.md
checkmarks
plan.md
Comprehensive implementation plan for direction bricks featurespecs/026-direction-bricks/plan.md
(43-48, 52)
and physics constraints
pattern, coordinate system, multi-frame persistence testing)
system, level design, documentation, and testing strategy
game systems
acceptance.md
Acceptance criteria testability checklist for direction bricksspecs/026-direction-bricks/checklists/acceptance.md
four user stories
randomization behavior
tracing/observability scenarios
statistical thresholds, multi-frame scenarios)
consistency
requirements.md
Specification quality checklist for direction bricksspecs/026-direction-bricks/checklists/requirements.md
markers remain
technology-agnostic
spec.md
Direction Bricks Feature Specification with TDD Requirementsspecs/026-direction-bricks/spec.md
52) with instantaneous velocity impulses
directions, diagonals, randomization, and scoring
multi-frame persistence testing
for the feature
data-model.md
Direction Bricks Data Model and System Designspecs/026-direction-bricks/data-model.md
DirectionBrickEffectobserver event type with payloadstructure
with behavior examples
transitions
quickstart.md
Direction Bricks Development Quickstart Guidespecs/026-direction-bricks/quickstart.md
implementation (green), tracing/scoring (refactor), acceptance testing
each phase
phases
observer-event.md
Direction Bricks Observer Event API Contractspecs/026-direction-bricks/contracts/observer-event.md
DirectionBrickEffectevent contract with Rust type signatureand constraints
and physics systems
observability contract
contract compliance
direction-bricks-coordinates.md
Direction Bricks Coordinate System Documentationdocs/direction-bricks-coordinates.md
axes)
direction
LICENSE-agpl-3.0.md
License Document Metadata Updatesdocs/LICENSE-agpl-3.0.md
:orphan:directive to mark document as standalonedocumentation
contributing.md
Contributing Guide License Section Additiondocs/contributing.md
license text
inclusion
1 files
.pre-commit-config.yaml
Pre-commit hook version updates.pre-commit-config.yaml
v1tov1.43.0for bug fixes andimprovements
v0.1.0tov0.1.10for enhancedfunctionality
4 files
3-balls.xcf
Add GIMP project for 3-balls texture assetassets/textures/gimp/3-balls.xcf
1-balls.xcf
Add GIMP project for 1-balls texture assetassets/textures/gimp/1-balls.xcf
3-balls-emmissve.xcf
Add GIMP project for 3-balls emissive texture variantassets/textures/gimp/3-balls-emmissve.xcf
texture
properties
configuration
2-balls-emissve.xcf
Add GIMP project for 2-balls emissive texture variantassets/textures/gimp/2-balls-emissve.xcf
texture
properties
configuration
1 files
Summary by CodeRabbit
New Features
Bug Fixes
Assets
Documentation
Tests
Chores