029 collision particle feedback#195
Conversation
…ebug logs and resolve query borrow conflict
…with implementation notes
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Reviewer's GuideImplements an observer-driven collision particle feedback system that spawns short-lived sparkly effects for ball collisions with walls, paddles, and bricks, wires it into existing collision paths, and adds tests, docs, and tooling metadata for the new feature. Sequence diagram for collision feedback spawning and lifetimesequenceDiagram
participant BrickCollisionSystem
participant WallCollisionSystem
participant PaddleCollisionSystem
participant Commands
participant CollisionFeedbackObserver as spawn_collision_feedback_effect
participant LifetimeSystem as update_feedback_effect_lifetimes
BrickCollisionSystem->>Commands: trigger(CollisionFeedbackTriggered)
WallCollisionSystem->>Commands: trigger(CollisionFeedbackTriggered)
PaddleCollisionSystem->>Commands: trigger(CollisionFeedbackTriggered)
Commands-->>CollisionFeedbackObserver: On<CollisionFeedbackTriggered>
CollisionFeedbackObserver->>CollisionFeedbackObserver: feedback_allowed_for_state_opt
alt [profile valid]
CollisionFeedbackObserver->>CollisionFeedbackObserver: resolve_contact_point
CollisionFeedbackObserver->>CollisionFeedbackObserver: offset_contact_toward_ball
CollisionFeedbackObserver->>Commands: spawn(FeedbackEffectInstance)
CollisionFeedbackObserver->>Commands: spawn(CollisionFeedbackParticle x N)
end
loop each frame
LifetimeSystem->>LifetimeSystem: update_feedback_effect_lifetimes
LifetimeSystem->>CollisionFeedbackParticle: move by velocity * dt
LifetimeSystem->>FeedbackEffectInstance: increment elapsed_seconds
alt [elapsed_seconds >= lifetime_seconds]
LifetimeSystem->>Commands: despawn(FeedbackEffectInstance)
LifetimeSystem->>Commands: despawn(CollisionFeedbackParticle with source_effect)
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
dosubot
Bot
added
documentation 📚
📝 WalkthroughWalkthroughThis PR adds a new collision particle-feedback feature: specification and plans, signal/event types, an ECS-based effect system (spawn + lifetime updater), integration into collision handlers, a comprehensive test suite, and supporting docs/config updates. ChangesCollision Particle Feedback Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| /// Collision target kind. | ||
| pub target_kind: CollisionFeedbackTargetKind, | ||
| /// Preferred collision contact point in world space. | ||
| pub contact_point: Vec3, |
There was a problem hiding this comment.
The contact_point: Vec3 field in CollisionFeedbackTriggered does not guarantee that the value is finite or valid. If upstream systems provide a NaN or infinite value, this could cause subtle bugs in physics or rendering consumers.
Recommendation:
- Ensure that the producer of this event validates
contact_pointfor finiteness before emitting the event. - Alternatively, document that consumers must check for validity and use
fallback_contact_pointifcontact_pointis invalid. - Consider providing a helper function to select a valid contact point, e.g.:
fn valid_contact_point(&self) -> Vec3 {
if self.contact_point.is_finite() {
self.contact_point
} else if let Some(fallback) = self.fallback_contact_point {
fallback
} else {
// Provide a sensible default or log a warning
Vec3::ZERO
}
}| for (particle_entity, particle) in particles.p0().iter() { | ||
| if expired.contains(&particle.source_effect) { | ||
| commands.entity(particle_entity).despawn(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential inefficiency in particle cleanup:
The loop iterates over all particles and checks if their source_effect is in the expired list. This approach may become inefficient as the number of particles grows, especially if there are many effects and particles. Consider optimizing by maintaining a direct mapping from effect entities to their particles, or by batching the despawn operations to reduce iteration overhead.
Recommended solution:
- Use a more efficient data structure (e.g., a HashSet for expired effects) to speed up the lookup.
- Consider associating particles with their effect via a parent-child relationship and using Bevy's hierarchical despawn to clean up all associated particles when the effect is removed.
| let visuals = if let Some(visuals) = visuals { | ||
| Some(visuals.clone()) | ||
| } else if let (Some(meshes), Some(materials)) = (meshes.as_mut(), materials.as_mut()) { | ||
| let created = create_collision_feedback_visuals(meshes.as_mut(), materials.as_mut()); | ||
| commands.insert_resource(created.clone()); | ||
| Some(created) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
Missing visuals handling in particle spawning:
When neither visuals nor asset resources (meshes, materials) are available, the code proceeds to spawn particles without any mesh or material, resulting in invisible or incomplete effects. This could lead to confusion or unintended behavior in the game.
Recommended solution:
- Add a check to ensure that visuals are available before spawning particles. If visuals are missing, log a warning and skip particle spawning, or provide a fallback visual.
| assert!(!effects.is_empty()); | ||
|
|
||
| for effect in effects { | ||
| assert!((0.85..=1.15).contains(&effect.style_variant)); |
There was a problem hiding this comment.
Floating-point comparisons in assertions (e.g., assert!((0.85..=1.15).contains(&effect.style_variant));) can lead to non-deterministic test failures due to rounding errors. Consider using an epsilon-based comparison, such as:
assert!((effect.style_variant - 1.0).abs() <= 0.15);This approach is more robust against minor floating-point inaccuracies.
| fn burst_collisions_spawn_one_effect_per_collision_no_cap() { | ||
| let mut app = make_test_app(); | ||
|
|
||
| for i in 0..25 { | ||
| trigger_collision( | ||
| &mut app, | ||
| CollisionFeedbackTargetKind::Brick, | ||
| Vec3::new(i as f32, 0.0, 0.0), | ||
| ); | ||
| } | ||
| app.update(); | ||
|
|
||
| assert_eq!(effect_instances(app.world_mut()).len(), 25); | ||
| } |
There was a problem hiding this comment.
The test burst_collisions_spawn_one_effect_per_collision_no_cap assumes that the system will always spawn one effect per collision, even for a burst of 25 collisions. If the implementation introduces an internal cap or resource exhaustion, this test may fail or become flaky. Consider explicitly documenting or testing for the absence of such caps, or parameterizing the test to check for the maximum supported burst size.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| BestPractice | 2 medium |
| CodeStyle | 3 minor |
| Complexity | 1 medium |
🟢 Metrics 73 complexity · 10 duplication
Metric Results Complexity 73 Duplication 10
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In
update_feedback_effect_lifetimes, the cleanup loop builds aVec<Entity>of expired effects and then doesexpired.contains(&particle.source_effect)for every particle, which is O(n²); consider using aHashSet<Entity>or tagging particles for removal in a first pass to keep this linear even under heavy burst loads. - The collision-feedback spawning logic for bricks is duplicated between the main destructible-brick branch and the later
brick_queries.p2branch inmark_brick_on_ball_collision; extracting a small helper that computes contact/offset and triggersCollisionFeedbackTriggeredwould reduce repetition and make it easier to evolve the feedback behavior in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `update_feedback_effect_lifetimes`, the cleanup loop builds a `Vec<Entity>` of expired effects and then does `expired.contains(&particle.source_effect)` for every particle, which is O(n²); consider using a `HashSet<Entity>` or tagging particles for removal in a first pass to keep this linear even under heavy burst loads.
- The collision-feedback spawning logic for bricks is duplicated between the main destructible-brick branch and the later `brick_queries.p2` branch in `mark_brick_on_ball_collision`; extracting a small helper that computes contact/offset and triggers `CollisionFeedbackTriggered` would reduce repetition and make it easier to evolve the feedback behavior in one place.
## Individual Comments
### Comment 1
<location path="src/lib.rs" line_range="828" />
<code_context>
}
- let current_type = brick_type_ro.0;
+
+ if systems::collision_feedback::feedback_allowed_for_state_opt(&game_state) {
+ let ball_pos = transforms
+ .get(triggering_ball)
</code_context>
<issue_to_address>
**suggestion:** Collision feedback emission for bricks is duplicated between the main and fallback brick queries, which makes the logic harder to maintain.
The block that computes `ball_pos`/`midpoint`, resolves and offsets the contact point, and emits `CollisionFeedbackTriggered` is duplicated in both the main destructible-brick branch and the `brick_queries.p2()` fallback. The only behavioral difference is `brick_destroyed_on_impact` (`!is_multi_hit_brick(current_type)` vs `false`). Consider extracting a helper (e.g. `emit_brick_feedback(...)`) to centralize this logic and avoid the branches diverging when you adjust contact resolution or logging later.
Suggested implementation:
```rust
use bevy::prelude::*;
/// Centralized helper for computing ball and contact positions for brick collisions.
/// This avoids duplicating the midpoint/contact resolution logic across brick branches.
fn resolve_brick_collision_contact(
transforms: &Query<&Transform>,
triggering_ball: Entity,
brick_pos: Vec3,
) -> (Vec3, Vec3) {
let ball_pos = transforms
.get(triggering_ball)
.map(|t| t.translation)
.unwrap_or(Vec3::ZERO);
let midpoint = (ball_pos + brick_pos) * 0.5;
let contact =
systems::collision_feedback::resolve_contact_point(midpoint, brick_pos);
let contact = systems::collision_feedback::offset_contact_toward_ball(
contact,
brick_pos,
ball_pos,
);
(ball_pos, contact)
}
```
```rust
if systems::collision_feedback::feedback_allowed_for_state_opt(&game_state) {
let (_ball_pos, contact) = resolve_brick_collision_contact(
&transforms,
triggering_ball,
brick_pos,
);
commands.trigger(crate::signals::CollisionFeedbackTriggered {
ball_entity: triggering_ball,
```
To fully implement the suggested refactor and remove duplication between the main destructible-brick branch and the `brick_queries.p2()` fallback:
1. Apply the same replacement to the fallback brick-handling branch:
- Locate the duplicated block in the fallback branch that computes `ball_pos`, `midpoint`, `contact`, and then calls `commands.trigger(CollisionFeedbackTriggered { ... })`.
- Replace the duplicated computation block with a call to `resolve_brick_collision_contact(&transforms, triggering_ball, brick_pos)` and reuse the `contact` return value exactly as in the main branch.
2. If the event payload explicitly needs `ball_pos` (for logging or additional fields in `CollisionFeedbackTriggered`), adjust the helper usage to:
- Destructure both values as `let (ball_pos, contact) = resolve_brick_collision_contact(...);`
- Use `ball_pos` wherever it’s currently being recomputed in both branches.
3. If you prefer to have the helper live in `systems::collision_feedback` instead of `src/lib.rs`, move `resolve_brick_collision_contact` into that module:
- Change its signature to be `pub(crate)` or `pub` as appropriate.
- Update the call sites to use `systems::collision_feedback::resolve_brick_collision_contact(...)` instead of the bare function name.
</issue_to_address>
### Comment 2
<location path="tests/collision_particle_feedback.rs" line_range="48-50" />
<code_context>
+ });
+}
+
+fn effect_instances(world: &mut World) -> Vec<FeedbackEffectInstance> {
+ let mut q = world.query::<&FeedbackEffectInstance>();
+ q.iter(world).copied().collect()
+}
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding direct unit tests for `resolve_contact_point` and `offset_contact_toward_ball` edge cases
Currently these helpers are only exercised indirectly via the observer. Please add focused tests (here or in a `collision_feedback_tests` module) that cover:
- `resolve_contact_point` with a non-finite `contact_point` and finite `fallback_contact_point`
- `resolve_contact_point` when both inputs are non-finite (fall back to `Vec3::ZERO`)
- `offset_contact_toward_ball` when the source→ball direction is zero-length or non-finite (returns original contact point)
- A normal `offset_contact_toward_ball` case asserting the 0.25-unit offset toward the ball
This will better protect these math-heavy helpers, which are reused across collision systems, during future refactors.
Suggested implementation:
```rust
use bevy::prelude::*;
use brkrs::game_state::GameState;
use brkrs::signals::{CollisionFeedbackTargetKind, CollisionFeedbackTriggered};
use brkrs::systems::collision_feedback::{
spawn_collision_feedback_effect, update_feedback_effect_lifetimes, CollisionFeedbackParticle,
FeedbackEffectInstance, FeedbackProfile, offset_contact_toward_ball, resolve_contact_point,
};
#[test]
fn resolve_contact_point_uses_fallback_when_primary_non_finite() {
let non_finite = Vec3::new(f32::NAN, 1.0, 0.0);
let fallback = Vec3::new(1.0, 2.0, 3.0);
let resolved = resolve_contact_point(non_finite, Some(fallback));
assert_eq!(resolved, fallback);
}
#[test]
fn resolve_contact_point_falls_back_to_zero_when_both_non_finite() {
let non_finite = Vec3::splat(f32::NAN);
let resolved = resolve_contact_point(non_finite, Some(non_finite));
assert_eq!(resolved, Vec3::ZERO);
}
#[test]
fn offset_contact_toward_ball_returns_original_when_direction_zero_length() {
let contact = Vec3::new(1.0, 0.0, 0.0);
let source = Vec3::new(0.0, 0.0, 0.0);
let ball = source; // zero-length source→ball direction
let offset = offset_contact_toward_ball(contact, ball, source);
assert_eq!(offset, contact);
}
#[test]
fn offset_contact_toward_ball_returns_original_when_direction_non_finite() {
let contact = Vec3::new(1.0, 0.0, 0.0);
let source = Vec3::new(f32::NAN, 0.0, 0.0);
let ball = Vec3::new(0.0, 0.0, 0.0);
let offset = offset_contact_toward_ball(contact, ball, source);
assert_eq!(offset, contact);
}
#[test]
fn offset_contact_toward_ball_offsets_toward_ball_by_quarter_unit() {
let contact = Vec3::new(0.0, 0.0, 0.0);
let source = Vec3::new(0.0, 0.0, 0.0);
let ball = Vec3::new(1.0, 0.0, 0.0);
let offset = offset_contact_toward_ball(contact, ball, source);
let expected = Vec3::new(0.25, 0.0, 0.0);
assert!((offset - expected).length() <= 1e-4);
}
fn make_test_app() -> App {
```
These tests assume the following signatures and visibility:
- `pub fn resolve_contact_point(contact_point: Vec3, fallback_contact_point: Option<Vec3>) -> Vec3`
- `pub fn offset_contact_toward_ball(contact_point: Vec3, ball_translation: Vec3, source_translation: Vec3) -> Vec3`
If the signatures differ (e.g., parameter order, `Option` usage, or module path), adjust the test calls accordingly. If these helpers are not currently `pub` from `brkrs::systems::collision_feedback`, they will need to be made public (or re-exported in a public module) so they can be used from the `tests` crate.
</issue_to_address>
### Comment 3
<location path="tests/collision_particle_feedback.rs" line_range="161-170" />
<code_context>
+}
+
+#[test]
+fn pause_suppresses_spawns_and_resume_does_not_replay() {
+ let mut app = make_test_app();
+
+ app.world_mut()
+ .resource_mut::<NextState<GameState>>()
+ .set(GameState::Paused);
+ app.update();
+
+ trigger_collision(&mut app, CollisionFeedbackTargetKind::Wall, Vec3::ZERO);
+ app.update();
+
+ assert!(effect_instances(app.world_mut()).is_empty());
+
+ app.world_mut()
+ .resource_mut::<NextState<GameState>>()
+ .set(GameState::Playing);
+ app.update();
+
+ assert!(
+ effect_instances(app.world_mut()).is_empty(),
+ "Suppressed effects must not replay after resume"
+ );
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Consider testing the behavior when the `GameState` resource is absent, since the observer explicitly supports that case
Right now we only verify behavior when `GameState` is present (`Paused` vs `Playing`). Since `feedback_allowed_for_state_opt` also treats a *missing* `State<GameState>` as "allowed", please add a test that builds a minimal `App` without `StatesPlugin`/`GameState`, triggers `spawn_collision_feedback_effect`, and asserts that an effect is spawned. That will guard the "no state == allowed" contract against regressions.
Suggested implementation:
```rust
assert!(
effect_instances(app.world_mut()).is_empty(),
"Suppressed effects must not replay after resume"
);
}
#[test]
fn collision_feedback_allowed_when_gamestate_absent() {
let mut app = make_test_app();
// Remove the `State<GameState>` resource to simulate an app without `GameState`
app.world_mut().remove_resource::<State<GameState>>();
trigger_collision(
&mut app,
CollisionFeedbackTargetKind::Wall,
Vec3::ZERO,
);
app.update();
assert!(
!effect_instances(app.world_mut()).is_empty(),
"Collision feedback effects should still spawn when GameState is absent"
);
}
```
1. Ensure `State` is imported in this test module (e.g. `use bevy::prelude::State;`) if it is not already.
2. This test assumes `make_test_app`, `CollisionFeedbackTargetKind`, `Vec3`, and `effect_instances` are already in scope, as in the other tests in this file.
</issue_to_address>
### Comment 4
<location path="tests/collision_particle_feedback.rs" line_range="186-195" />
<code_context>
+}
+
+#[test]
+fn repeated_collisions_cleanup_all_effects_and_particles() {
+ let mut app = make_test_app();
+
+ for i in 0..12 {
+ trigger_collision(
+ &mut app,
+ CollisionFeedbackTargetKind::Brick,
+ Vec3::new(i as f32, 0.0, 0.0),
+ );
+ }
+ app.update();
+
+ assert_eq!(effect_instances(app.world_mut()).len(), 12);
+
+ advance_time(&mut app, 1.0);
+ app.update();
+
+ assert!(effect_instances(app.world_mut()).is_empty());
+ let mut q = app.world_mut().query::<&CollisionFeedbackParticle>();
+ assert_eq!(q.iter(app.world()).count(), 0);
+}
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add an assertion that particles actually move over time, not just that they are later despawned
This test covers cleanup, but `update_feedback_effect_lifetimes` is also responsible for applying per-frame motion. A separate test could:
- Trigger a single collision and call `app.update()` to record initial particle translations.
- Advance time by a small `delta_secs` (e.g. 0.1), call `app.update()` again, and assert that at least one particle’s translation changes in the expected direction.
That would guard against regressions where velocities stop being applied while keeping this test focused on despawning.
Suggested implementation:
```rust
use std::collections::HashMap;
use bevy::prelude::*;
```
```rust
fn repeated_collisions_cleanup_all_effects_and_particles() {
let mut app = make_test_app();
for i in 0..12 {
trigger_collision(
&mut app,
CollisionFeedbackTargetKind::Brick,
Vec3::new(i as f32, 0.0, 0.0),
);
}
app.update();
assert_eq!(effect_instances(app.world_mut()).len(), 12);
advance_time(&mut app, 1.0);
app.update();
assert!(effect_instances(app.world_mut()).is_empty());
let mut q = app.world_mut().query::<&CollisionFeedbackParticle>();
assert_eq!(q.iter(app.world()).count(), 0);
}
#[test]
fn particles_move_over_time() {
let mut app = make_test_app();
// Trigger a single collision at the origin to spawn particles.
trigger_collision(
&mut app,
CollisionFeedbackTargetKind::Brick,
Vec3::ZERO,
);
app.update();
// Capture initial positions and velocities per particle entity.
let mut world = app.world_mut();
let mut initial: HashMap<Entity, (Vec3, Vec3)> = HashMap::new();
{
let mut q = world.query::<(Entity, &Transform, &CollisionFeedbackParticle)>();
for (entity, transform, particle) in q.iter(&world) {
initial.insert(entity, (transform.translation, particle.velocity));
}
}
assert!(
!initial.is_empty(),
"Expected at least one particle to be spawned by collision feedback"
);
// Advance time by a small delta and update the simulation.
let delta_secs = 0.1;
advance_time(&mut app, delta_secs);
app.update();
// Re-query and assert that at least one particle moved in the direction of its velocity.
world = app.world_mut();
let mut q = world.query::<(Entity, &Transform, &CollisionFeedbackParticle)>();
let mut any_moved_in_expected_direction = false;
for (entity, transform, _particle) in q.iter(&world) {
if let Some((initial_pos, velocity)) = initial.get(&entity) {
let displacement = transform.translation - *initial_pos;
if displacement.length() > 0.0 && displacement.dot(*velocity) > 0.0 {
any_moved_in_expected_direction = true;
break;
}
}
}
assert!(
any_moved_in_expected_direction,
"Expected at least one particle to move in the direction of its velocity over time"
);
}
```
This test assumes that `CollisionFeedbackParticle` has a public `velocity: Vec3` field and that particles are moved via `Transform.translation`. If your component uses different field names or a separate position component, adjust the query and the `(initial_pos, velocity)` extraction accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| fn effect_instances(world: &mut World) -> Vec<FeedbackEffectInstance> { | ||
| let mut q = world.query::<&FeedbackEffectInstance>(); | ||
| q.iter(world).copied().collect() |
There was a problem hiding this comment.
suggestion (testing): Consider adding direct unit tests for resolve_contact_point and offset_contact_toward_ball edge cases
Currently these helpers are only exercised indirectly via the observer. Please add focused tests (here or in a collision_feedback_tests module) that cover:
resolve_contact_pointwith a non-finitecontact_pointand finitefallback_contact_pointresolve_contact_pointwhen both inputs are non-finite (fall back toVec3::ZERO)offset_contact_toward_ballwhen the source→ball direction is zero-length or non-finite (returns original contact point)- A normal
offset_contact_toward_ballcase asserting the 0.25-unit offset toward the ball
This will better protect these math-heavy helpers, which are reused across collision systems, during future refactors.
Suggested implementation:
use bevy::prelude::*;
use brkrs::game_state::GameState;
use brkrs::signals::{CollisionFeedbackTargetKind, CollisionFeedbackTriggered};
use brkrs::systems::collision_feedback::{
spawn_collision_feedback_effect, update_feedback_effect_lifetimes, CollisionFeedbackParticle,
FeedbackEffectInstance, FeedbackProfile, offset_contact_toward_ball, resolve_contact_point,
};
#[test]
fn resolve_contact_point_uses_fallback_when_primary_non_finite() {
let non_finite = Vec3::new(f32::NAN, 1.0, 0.0);
let fallback = Vec3::new(1.0, 2.0, 3.0);
let resolved = resolve_contact_point(non_finite, Some(fallback));
assert_eq!(resolved, fallback);
}
#[test]
fn resolve_contact_point_falls_back_to_zero_when_both_non_finite() {
let non_finite = Vec3::splat(f32::NAN);
let resolved = resolve_contact_point(non_finite, Some(non_finite));
assert_eq!(resolved, Vec3::ZERO);
}
#[test]
fn offset_contact_toward_ball_returns_original_when_direction_zero_length() {
let contact = Vec3::new(1.0, 0.0, 0.0);
let source = Vec3::new(0.0, 0.0, 0.0);
let ball = source; // zero-length source→ball direction
let offset = offset_contact_toward_ball(contact, ball, source);
assert_eq!(offset, contact);
}
#[test]
fn offset_contact_toward_ball_returns_original_when_direction_non_finite() {
let contact = Vec3::new(1.0, 0.0, 0.0);
let source = Vec3::new(f32::NAN, 0.0, 0.0);
let ball = Vec3::new(0.0, 0.0, 0.0);
let offset = offset_contact_toward_ball(contact, ball, source);
assert_eq!(offset, contact);
}
#[test]
fn offset_contact_toward_ball_offsets_toward_ball_by_quarter_unit() {
let contact = Vec3::new(0.0, 0.0, 0.0);
let source = Vec3::new(0.0, 0.0, 0.0);
let ball = Vec3::new(1.0, 0.0, 0.0);
let offset = offset_contact_toward_ball(contact, ball, source);
let expected = Vec3::new(0.25, 0.0, 0.0);
assert!((offset - expected).length() <= 1e-4);
}
fn make_test_app() -> App {These tests assume the following signatures and visibility:
pub fn resolve_contact_point(contact_point: Vec3, fallback_contact_point: Option<Vec3>) -> Vec3pub fn offset_contact_toward_ball(contact_point: Vec3, ball_translation: Vec3, source_translation: Vec3) -> Vec3
If the signatures differ (e.g., parameter order, Option usage, or module path), adjust the test calls accordingly. If these helpers are not currently pub from brkrs::systems::collision_feedback, they will need to be made public (or re-exported in a public module) so they can be used from the tests crate.
| fn pause_suppresses_spawns_and_resume_does_not_replay() { | ||
| let mut app = make_test_app(); | ||
|
|
||
| app.world_mut() | ||
| .resource_mut::<NextState<GameState>>() | ||
| .set(GameState::Paused); | ||
| app.update(); | ||
|
|
||
| trigger_collision(&mut app, CollisionFeedbackTargetKind::Wall, Vec3::ZERO); | ||
| app.update(); |
There was a problem hiding this comment.
suggestion (testing): Consider testing the behavior when the GameState resource is absent, since the observer explicitly supports that case
Right now we only verify behavior when GameState is present (Paused vs Playing). Since feedback_allowed_for_state_opt also treats a missing State<GameState> as "allowed", please add a test that builds a minimal App without StatesPlugin/GameState, triggers spawn_collision_feedback_effect, and asserts that an effect is spawned. That will guard the "no state == allowed" contract against regressions.
Suggested implementation:
assert!(
effect_instances(app.world_mut()).is_empty(),
"Suppressed effects must not replay after resume"
);
}
#[test]
fn collision_feedback_allowed_when_gamestate_absent() {
let mut app = make_test_app();
// Remove the `State<GameState>` resource to simulate an app without `GameState`
app.world_mut().remove_resource::<State<GameState>>();
trigger_collision(
&mut app,
CollisionFeedbackTargetKind::Wall,
Vec3::ZERO,
);
app.update();
assert!(
!effect_instances(app.world_mut()).is_empty(),
"Collision feedback effects should still spawn when GameState is absent"
);
}- Ensure
Stateis imported in this test module (e.g.use bevy::prelude::State;) if it is not already. - This test assumes
make_test_app,CollisionFeedbackTargetKind,Vec3, andeffect_instancesare already in scope, as in the other tests in this file.
| fn repeated_collisions_cleanup_all_effects_and_particles() { | ||
| let mut app = make_test_app(); | ||
|
|
||
| for i in 0..12 { | ||
| trigger_collision( | ||
| &mut app, | ||
| CollisionFeedbackTargetKind::Brick, | ||
| Vec3::new(i as f32, 0.0, 0.0), | ||
| ); | ||
| } |
There was a problem hiding this comment.
suggestion (testing): Add an assertion that particles actually move over time, not just that they are later despawned
This test covers cleanup, but update_feedback_effect_lifetimes is also responsible for applying per-frame motion. A separate test could:
- Trigger a single collision and call
app.update()to record initial particle translations. - Advance time by a small
delta_secs(e.g. 0.1), callapp.update()again, and assert that at least one particle’s translation changes in the expected direction.
That would guard against regressions where velocities stop being applied while keeping this test focused on despawning.
Suggested implementation:
use std::collections::HashMap;
use bevy::prelude::*;fn repeated_collisions_cleanup_all_effects_and_particles() {
let mut app = make_test_app();
for i in 0..12 {
trigger_collision(
&mut app,
CollisionFeedbackTargetKind::Brick,
Vec3::new(i as f32, 0.0, 0.0),
);
}
app.update();
assert_eq!(effect_instances(app.world_mut()).len(), 12);
advance_time(&mut app, 1.0);
app.update();
assert!(effect_instances(app.world_mut()).is_empty());
let mut q = app.world_mut().query::<&CollisionFeedbackParticle>();
assert_eq!(q.iter(app.world()).count(), 0);
}
#[test]
fn particles_move_over_time() {
let mut app = make_test_app();
// Trigger a single collision at the origin to spawn particles.
trigger_collision(
&mut app,
CollisionFeedbackTargetKind::Brick,
Vec3::ZERO,
);
app.update();
// Capture initial positions and velocities per particle entity.
let mut world = app.world_mut();
let mut initial: HashMap<Entity, (Vec3, Vec3)> = HashMap::new();
{
let mut q = world.query::<(Entity, &Transform, &CollisionFeedbackParticle)>();
for (entity, transform, particle) in q.iter(&world) {
initial.insert(entity, (transform.translation, particle.velocity));
}
}
assert!(
!initial.is_empty(),
"Expected at least one particle to be spawned by collision feedback"
);
// Advance time by a small delta and update the simulation.
let delta_secs = 0.1;
advance_time(&mut app, delta_secs);
app.update();
// Re-query and assert that at least one particle moved in the direction of its velocity.
world = app.world_mut();
let mut q = world.query::<(Entity, &Transform, &CollisionFeedbackParticle)>();
let mut any_moved_in_expected_direction = false;
for (entity, transform, _particle) in q.iter(&world) {
if let Some((initial_pos, velocity)) = initial.get(&entity) {
let displacement = transform.translation - *initial_pos;
if displacement.length() > 0.0 && displacement.dot(*velocity) > 0.0 {
any_moved_in_expected_direction = true;
break;
}
}
}
assert!(
any_moved_in_expected_direction,
"Expected at least one particle to move in the direction of its velocity over time"
);
}This test assumes that CollisionFeedbackParticle has a public velocity: Vec3 field and that particles are moved via Transform.translation. If your component uses different field names or a separate position component, adjust the query and the (initial_pos, velocity) extraction accordingly.
![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 pull request implements immediate visual particle feedback for ball collisions with walls, paddles, and bricks using Bevy 0.17 observers. It introduces a new collision_feedback system, integrates event triggers into existing collision detection paths, and includes comprehensive specifications, documentation, and integration tests. Feedback focuses on improving the robustness and performance of the implementation: correcting resource cloning logic, relaxing restrictive validation on tunable parameters, and moving lazy asset initialization to a more idiomatic FromWorld pattern. Reviewers also recommended simplifying ParamSet usage, optimizing particle cleanup via entity hierarchies, and replacing the duplicated midpoint heuristic with accurate contact manifold data from the physics engine to satisfy specification requirements.
| } | ||
|
|
||
| let event = trigger.event(); | ||
| let profile = profile.map(|p| p.clone()).unwrap_or_default(); |
There was a problem hiding this comment.
In Bevy, Res<T> typically does not implement Clone. Calling .clone() on an Option<Res<FeedbackProfile>> might attempt to clone the resource wrapper itself rather than the underlying data, which could result in a compilation error. Could you clarify if you are using a specific Bevy version or a custom wrapper that implements Clone for Res? Otherwise, you should dereference the resource before cloning the data.
| let profile = profile.map(|p| p.clone()).unwrap_or_default(); | |
| let profile = profile.as_deref().cloned().unwrap_or_default(); |
References
- When commenting on API usage without full dependency context, avoid making absolute claims about compilation errors. Instead, phrase suggestions as possibilities and ask for clarification on crate versions or custom wrappers.
| fn is_valid(&self) -> bool { | ||
| self.min_particles <= self.max_particles | ||
| && self.min_lifetime_seconds <= self.max_lifetime_seconds | ||
| && self.min_particles >= DEFAULT_MIN_PARTICLES | ||
| && self.max_particles <= DEFAULT_MAX_PARTICLES | ||
| && self.min_lifetime_seconds >= DEFAULT_MIN_LIFETIME | ||
| && self.max_lifetime_seconds <= DEFAULT_MAX_LIFETIME | ||
| } |
There was a problem hiding this comment.
The is_valid check is overly restrictive because it enforces that the "tunable" parameters must remain within the hardcoded default ranges. If a developer attempts to tune the FeedbackProfile resource to use, for example, 20 particles or a 0.5s lifetime, the system will silently fail to spawn any effects. The validation should instead focus on logical consistency (e.g., min <= max and non-negative values).
fn is_valid(&self) -> bool {
self.min_particles <= self.max_particles
&& self.min_lifetime_seconds <= self.max_lifetime_seconds
&& self.min_particles > 0
&& self.min_lifetime_seconds >= 0.0
}| let visuals = if let Some(visuals) = visuals { | ||
| Some(visuals.clone()) | ||
| } else if let (Some(meshes), Some(materials)) = (meshes.as_mut(), materials.as_mut()) { | ||
| let created = create_collision_feedback_visuals(meshes.as_mut(), materials.as_mut()); | ||
| commands.insert_resource(created.clone()); | ||
| Some(created) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
Initializing the CollisionFeedbackVisuals resource lazily inside an observer is inefficient and prone to race conditions. Since commands.insert_resource is deferred until the end of the stage, multiple collisions occurring in the same frame will each trigger the mesh and material creation logic, resulting in redundant asset handles and multiple insertion commands. It is better to initialize this resource using app.init_resource::<CollisionFeedbackVisuals>() in the plugin setup, implementing FromWorld for CollisionFeedbackVisuals to handle the asset creation.
| mut particles: ParamSet<( | ||
| Query<(Entity, &CollisionFeedbackParticle)>, | ||
| Query<(&CollisionFeedbackParticle, &mut Transform)>, | ||
| )>, |
There was a problem hiding this comment.
The use of ParamSet here is unnecessary. In Bevy, multiple queries can read the same component (&CollisionFeedbackParticle) simultaneously without conflict. ParamSet is only required when multiple queries access the same component and at least one of them is mutable (&mut). Removing the ParamSet will simplify the system signature and the subsequent code.
| for (particle_entity, particle) in particles.p0().iter() { | ||
| if expired.contains(&particle.source_effect) { | ||
| commands.entity(particle_entity).despawn(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current cleanup logic iterates over all particles in the world and performs a lookup in the expired list for each one whenever any effect expires. This results in O(N_particles x N_expired) complexity, which can become a performance bottleneck during intense gameplay. A more efficient and idiomatic Bevy approach would be to make the particles children of the effect entity using commands.entity(effect_entity).add_child(particle_entity) and then using commands.entity(effect_entity).despawn_recursive(). This leverages Bevy's built-in hierarchy management for automatic cleanup.
| let midpoint = (ball_pos + brick_pos) * 0.5; | ||
| let contact = | ||
| systems::collision_feedback::resolve_contact_point(midpoint, brick_pos); | ||
| let contact = systems::collision_feedback::offset_contact_toward_ball( | ||
| contact, brick_pos, ball_pos, | ||
| ); |
There was a problem hiding this comment.
The logic for calculating the collision contact point using a midpoint heuristic is duplicated across mark_brick_on_ball_collision, detect_ball_wall_collisions, and read_character_controller_collisions. Furthermore, the midpoint between entity centers is an inaccurate representation of the "exact contact point" required by the specification (FR-004, FR-015), especially for collisions with large walls. Consider centralizing this logic into a helper function and, if possible, leveraging RapierContext to retrieve the actual contact manifold from the physics engine for better visual accuracy.
There was a problem hiding this comment.
Pull request overview
Adds an observer-driven collision particle feedback subsystem that spawns short-lived “sparkly” burst effects for ball impacts against walls, paddle, and bricks, with configurable bounds and integration coverage via acceptance-style tests.
Changes:
- Introduces
CollisionFeedbackTriggered(+ target-kind enum) and a newsystems::collision_feedbackmodule to spawn/advance/cleanup effect instances and particles. - Hooks collision feedback triggers into existing collision entry points in
src/lib.rs(brick, wall, paddle paths), gated byGameState::Playing. - Adds a comprehensive integration test suite and feature specification/plan docs for the new behavior.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/collision_particle_feedback.rs | New integration tests covering spawn timing, contact point, bounds, pause suppression, cleanup, and hierarchy safety. |
| src/systems/mod.rs | Exposes the new collision feedback module and re-exports key types. |
| src/systems/collision_feedback.rs | New observer + lifetime/particle systems and supporting helpers/resources for collision VFX. |
| src/signals.rs | Adds CollisionFeedbackTargetKind and CollisionFeedbackTriggered observer event contract. |
| src/lib.rs | Wires resource init + systems/observers; emits collision feedback triggers from existing collision paths. |
| docs/developer-guide.md | Documents collision feedback behavior contracts and guardrails. |
| .cargo/config.toml | Removes explicit LLD linker rustflag override. |
| .github/copilot-instructions.md | Points SPECKIT guidance at the new feature plan path. |
| .github/agents/copilot-instructions.md | Updates agent instructions with the feature’s tech stack line-items. |
| specs/029-collision-particle-feedback/* | Adds feature spec, plan, research, tasks, contracts, checklist, and quickstart docs. |
| .specify/feature.json | Registers the feature directory for tooling. |
| trigger_collision(&mut app, CollisionFeedbackTargetKind::Paddle, Vec3::ZERO); | ||
| app.update(); | ||
|
|
||
| let effects = effect_instances(app.world()); |
| let visuals = if let Some(visuals) = visuals { | ||
| Some(visuals.clone()) | ||
| } else if let (Some(meshes), Some(materials)) = (meshes.as_mut(), materials.as_mut()) { | ||
| let created = create_collision_feedback_visuals(meshes.as_mut(), materials.as_mut()); | ||
| commands.insert_resource(created.clone()); | ||
| Some(created) | ||
| } else { | ||
| None | ||
| }; |
| let mut expired = Vec::new(); | ||
| for (entity, mut effect) in &mut effects { | ||
| effect.elapsed_seconds += dt; | ||
| if effect.elapsed_seconds >= effect.lifetime_seconds { | ||
| expired.push(entity); | ||
| } | ||
| } | ||
|
|
||
| if expired.is_empty() { | ||
| return; | ||
| } | ||
|
|
||
| for effect in &expired { | ||
| commands.entity(*effect).despawn(); | ||
| } | ||
|
|
||
| for (particle_entity, particle) in particles.p0().iter() { | ||
| if expired.contains(&particle.source_effect) { | ||
| commands.entity(particle_entity).despawn(); | ||
| } |
![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 successfully adds collision particle feedback but introduces two critical issues: a potential behavioral change for indestructible bricks that may break level designs, and a heuristic contact point that violates the spec's accuracy requirement. Build failure risk exists due to a possible rand API mismatch.
📄 Documentation Diagram
This diagram documents the refactored collision particle feedback flow from collision event to effect cleanup.
sequenceDiagram
participant CollisionEvent as Collision Event
participant CollisionSys as Collision System
participant FeedbackObserver as Feedback Observer
participant EffectInstance as Effect Instance
participant Particle as Particles
participant UpdateLifecycle as Lifecycle System
CollisionEvent->>CollisionSys: CollisionEvent::Started
CollisionSys->>CollisionSys: Classify collision (Wall/Paddle/Brick)
CollisionSys->>CollisionSys: Compute contact point (heuristic)
CollisionSys->>FeedbackObserver: Trigger CollisionFeedbackTriggered
note over FeedbackObserver: PR #35;195: new observer
FeedbackObserver->>FeedbackObserver: Check game state (pause suppression)
FeedbackObserver->>EffectInstance: Spawn FeedbackEffectInstance
FeedbackObserver->>Particle: Spawn N particles (8-16) at contact point
EffectInstance->>UpdateLifecycle: Elapsed time updated
UpdateLifecycle->>EffectInstance: Check lifetime expired
UpdateLifecycle->>EffectInstance: Despawn effect
UpdateLifecycle->>Particle: Despawn child particles
🌟 Strengths
- Comprehensive test coverage validates burst, pause, and lifetime behavior.
- Clear observer-driven architecture integrates well with existing collision systems.
| Priority | File | Category | Impact Summary (≤12 words) | Anchors |
|---|---|---|---|---|
| P1 | src/lib.rs | Bug | Silent destruction rule change for bricks ≥ ID threshold | constant:INDESTRUCTIBLE_BRICK, path:src/level_format.rs |
| P1 | src/lib.rs | Bug | Contact point uses heuristic, not exact collision manifold | method:resolve_contact_point, method:offset_contact_toward_ball |
| P2 | src/systems/collision_feedback.rs | Build | rand API may not compile if project uses rand 0.8 | |
| P2 | src/lib.rs | Maintainability | ParamSet with three queries duplicates logic, increases risk |
🔍 Notable Themes
- Accuracy vs. Simplicity Trade-off: The contact point heuristic prioritizes simplicity over physics accuracy, risking visual misalignment. A dedicated utility to extract contact data from Rapier would uphold the spec but may require additional work.
- Version Dependency Risk: The PR assumes a rand version (0.9+) without updating
Cargo.toml, creating a potential build blocker. Dependencies should be explicitly pinned.
📈 Risk Diagram
This diagram illustrates the behavioral risks associated with the indestructible brick threshold change and the heuristic contact point computation.
sequenceDiagram
participant Level as Level Config
participant CollisionLogic as Collision System
participant Brick as Brick Entity
participant Feedback as Feedback Effect
Note over Level: Brick type IDs defined in level files
Level->>CollisionLogic: Read brick type (e.g., 90, 91, 92)
CollisionLogic->>Brick: Check destruction rule
note over CollisionLogic: R1(P1): Threshold `>= INDESTRUCTIBLE_BRICK` silently makes <br/>all types >=90 indestructible – breaks existing levels
alt Brick type >= INDESTRUCTIBLE_BRICK (e.g., 90)
CollisionLogic->>Brick: Skip destruction (new behavior)
else Brick type == HAZARD_BRICK_91 (old behavior)
CollisionLogic->>Brick: Mark for destruction (changed)
end
CollisionLogic->>Feedback: Compute contact point
note over CollisionLogic,Feedback: R2(P1): Contact point uses midpoint+offset heuristic,<br/>not exact physics manifold – violates FR-015
activate Feedback
Feedback->>Feedback: Spawn effect at heuristic point
deactivate Feedback
⚠️ **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/lib.rs
The mark_brick_on_ball_collision function now uses a ParamSet with three distinct queries. The third query (index 2) duplicates the first query’s structure minus the With<CountsTowardsCompletion> filter. This pattern duplicates query definitions and increases the risk of inconsistency (e.g., changing the first query but forgetting the third). The addition was necessary to support feedback for bricks that do not count towards completion (like indestructible bricks). However, the design could be simplified by using a single query with a With<Brick> filter and a separate check for CountsTowardsCompletion at runtime via a Has component check (Bevy 0.17 supports Has<C>() in queries). That would avoid the ParamSet growth and reduce cognitive load. The current implementation also accesses brick_queries.p2() in the else if branch, which introduces subtle control flow dependencies.
Suggestion:
Consolidate to a single query with a `With<Brick>` filter and use `Has<CountsTowardsCompletion>` on the entity to distinguish behavior. For example:
```rust
let brick_query = Query<(Entity, &BrickTypeId, Option<&GlobalTransform>, Option<&Transform>), (With<Brick>, Without<MarkedForDespawn>)>;
// ...
let is_counting = world.get::<CountsTowardsCompletion>(entity).is_some();
**Related Code:**
```rust
mut brick_queries: ParamSet<(
Query<...>,
Query<...>,
Query<...>,
)>,
💡 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.
| // Skip indestructible bricks (all types >= 90). | ||
| // These bricks still emit feedback on hit, but are not destroyed by the ball. | ||
| if current_type >= crate::level_format::INDESTRUCTIBLE_BRICK { |
There was a problem hiding this comment.
P1 | Confidence: Medium
The PR replaces a single-type check (current_type == HAZARD_BRICK_91) with a threshold check (current_type >= INDESTRUCTIBLE_BRICK). This changes the destruction logic for any brick type with an ID >= INDESTRUCTIBLE_BRICK. The constant INDESTRUCTIBLE_BRICK is not defined in the PR’s changed files; its definition in src/level_format.rs must be verified. If the constant value is 90 (as the comment suggests), then all brick types 90+ become indestructible by ball collision. This may be intentional to align with the spec’s “indestructible bricks should still emit feedback” requirement, but it is a silent behavioral change for any existing bricks in that range that were previously destructible. The spec does not call for mass-indestructibility; it only mentions hazard bricks. This change should be explicitly documented and its constant value confirmed. Without verification, this could break level designs that rely on destructibility of bricks with IDs >= 90.
| let midpoint = (ball_pos + brick_pos) * 0.5; | ||
| let contact = | ||
| systems::collision_feedback::resolve_contact_point(midpoint, brick_pos); | ||
| let contact = systems::collision_feedback::offset_contact_toward_ball( | ||
| contact, brick_pos, ball_pos, | ||
| ); |
There was a problem hiding this comment.
P1 | Confidence: Medium
FR-015 and the spec require the feedback effect to spawn at the exact collision contact point. The implementation computes the contact point as the midpoint between ball center and brick center, then offsets it toward the ball by a fixed 0.25 units. This is a heuristic that does not use the actual contact manifold from Rapier collisions. While the Rapier CollisionEvent does not directly expose contact points in a straightforward way, the use of this simplistic approximation means the effect may appear visibly offset from the true impact location, especially for edge collisions or thick walls. The research.md (Decision 3) acknowledges a “deterministic fallback path,” but the implemented fallback uses entity centers, not a physical contact point. This degrades the accuracy promised by the spec and could be visually jarring. The integration tests only verify that the effect is placed at the contact_point provided in the trigger, which comes from the same heuristic, so the tests pass but do not validate correctness against actual physics.
| let mut rng = rand::rng(); | ||
| let particle_count = profile.sample_particle_count(&mut rng); | ||
| let lifetime_seconds = profile.sample_lifetime_seconds(&mut rng); |
There was a problem hiding this comment.
P2 | Confidence: High
The code uses the API rand::rng(), RngExt, and random_range, which are only available in rand 0.9+. The plan.md lists “rand 0.x” as a dependency, but the PR does not update Cargo.toml to specify the required version. If the project currently uses rand 0.8 (which uses thread_rng() and gen_range()), this code will fail to compile. The same file also uses use rand::RngExt; which is a sub-crate trait (likely a version-specific import). A deterministic build failure cannot be proven without the project’s Cargo.toml, but the API mismatch is clear. The project must either upgrade to rand 0.9 or provide a version gate.
Code Suggestion:
If the project stays on rand 0.8, replace `rand::rng()` with `rand::thread_rng()`, `RngExt` with `Rng` (or remove `use rand::RngExt;` and use `Rng::gen_range`), and `random_range` with `gen_range`. If upgrading to rand 0.9, add an explicit dependency.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
docs/developer-guide.md (1)
261-281: ⚡ Quick winClarify "finite fallback" terminology.
The phrase "resolved contact point with finite fallback" (line 272) may be unclear to developers unfamiliar with the collision system. Consider specifying what the fallback behavior is (e.g., "uses ball position if contact point is unavailable").
📝 Suggested clarification
- One effect is spawned per qualifying collision (wall, paddle, brick) -- Effects spawn at resolved contact point with finite fallback +- Effects spawn at resolved contact point (falls back to ball position if unavailable) - Particle count is constrained to 8-16 and lifetime to 0.20-0.35 secondsIf the actual fallback behavior differs, please adjust the clarification accordingly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/developer-guide.md` around lines 261 - 281, Replace the vague phrase "resolved contact point with finite fallback" in the Collision Feedback Notes with an explicit description of the fallback used by the collision VFX (e.g., "spawns at the resolved contact point; if no contact point is available, falls back to the ball's position" or the actual behavior implemented), and update the sentence near the references to CollisionFeedbackTriggered, spawn_collision_feedback_effect, and update_feedback_effect_lifetimes so readers know exactly what fallback to expect (adjust the example fallback wording if the real implementation differs)..github/agents/copilot-instructions.md (1)
46-46: 💤 Low valueConsider adding blank line for markdown consistency.
The markdown linter suggests adding a blank line before this list item for better formatting consistency. While this is cosmetic, it improves readability and passes standard markdown linters.
📝 Optional formatting fix
## Recent Changes + - 029-collision-particle-feedback: Added Rust 1.81 (edition 2021) + Bevy 0.17.3, bevy_rapier3d 0.32.0, tracing 0.1, rand 0.x🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/agents/copilot-instructions.md at line 46, Insert a single blank line immediately before the list item line that reads "029-collision-particle-feedback: Added Rust 1.81 (edition 2021) + Bevy 0.17.3, bevy_rapier3d 0.32.0, tracing 0.1, rand 0.x" to satisfy the markdown linter and ensure consistent spacing between list entries; locate the line in .github/agents/copilot-instructions.md and add the blank line above it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/agents/copilot-instructions.md:
- Line 20: Update the documented dependency string in the stack line that
currently shows "rand 0.x" to match the repository's Cargo constraint "rand =
\"0\"" (or replace it with the exact version from Cargo.lock if you want a
pinned, reproducible value); find the stack line containing "Rust 1.81 (edition
2021) + Bevy 0.17.3, bevy_rapier3d 0.32.0, tracing 0.1, rand 0.x
(029-collision-particle-feedback)" and change the "rand 0.x" token to the chosen
form ("rand = \"0\"" or the exact pinned version).
In `@specs/029-collision-particle-feedback/tasks.md`:
- Line 40: Task T014 (the red-phase proof) is missing; update the feature
checklist entry for T014 in the requirements.md checklist by recording the
failing-test proof: include the failing test name(s), clear steps-to-reproduce,
the minimal failing assertion/output, the commit hash (or PR/branch) that
contains the red-phase proof, author and timestamp, and any relevant
logs/screenshots; alternatively, if the red-phase was already captured
elsewhere, add a brief cross-reference in the T014 checklist entry pointing to
the exact commit/PR/test file and mark T014 complete.
In `@src/lib.rs`:
- Around line 845-846: The field brick_destroyed_on_impact is being set too
early using !is_multi_hit_brick(current_type) and can be wrong when paddle-only
or indestructible skip logic later prevents destruction; move or recompute this
field after the paddle-only/indestructible skip checks and use the actual
destruction outcome (e.g., a boolean like was_destroyed or the final logic that
applies skips and decrements hits) when constructing the event instead of
relying solely on is_multi_hit_brick(current_type).
In `@tests/collision_particle_feedback.rs`:
- Line 149: The call to effect_instances currently passes an immutable world
with effect_instances(app.world()), but effect_instances expects a mutable
reference (&mut World); update the invocation to pass a mutable world by calling
effect_instances(app.world_mut()) so it compiles against the signature of
effect_instances(&mut World).
---
Nitpick comments:
In @.github/agents/copilot-instructions.md:
- Line 46: Insert a single blank line immediately before the list item line that
reads "029-collision-particle-feedback: Added Rust 1.81 (edition 2021) + Bevy
0.17.3, bevy_rapier3d 0.32.0, tracing 0.1, rand 0.x" to satisfy the markdown
linter and ensure consistent spacing between list entries; locate the line in
.github/agents/copilot-instructions.md and add the blank line above it.
In `@docs/developer-guide.md`:
- Around line 261-281: Replace the vague phrase "resolved contact point with
finite fallback" in the Collision Feedback Notes with an explicit description of
the fallback used by the collision VFX (e.g., "spawns at the resolved contact
point; if no contact point is available, falls back to the ball's position" or
the actual behavior implemented), and update the sentence near the references to
CollisionFeedbackTriggered, spawn_collision_feedback_effect, and
update_feedback_effect_lifetimes so readers know exactly what fallback to expect
(adjust the example fallback wording if the real implementation differs).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d274f8aa-6f95-4af0-9062-4061685245e4
📒 Files selected for processing (18)
.cargo/config.toml.github/agents/copilot-instructions.md.github/copilot-instructions.md.specify/feature.jsondocs/developer-guide.mdspecs/029-collision-particle-feedback/checklists/requirements.mdspecs/029-collision-particle-feedback/contracts/collision-feedback-observer.mdspecs/029-collision-particle-feedback/data-model.mdspecs/029-collision-particle-feedback/plan.mdspecs/029-collision-particle-feedback/quickstart.mdspecs/029-collision-particle-feedback/research.mdspecs/029-collision-particle-feedback/spec.mdspecs/029-collision-particle-feedback/tasks.mdsrc/lib.rssrc/signals.rssrc/systems/collision_feedback.rssrc/systems/mod.rstests/collision_particle_feedback.rs
💤 Files with no reviewable changes (1)
- .cargo/config.toml
| - RON files in `assets/levels/` directory; in-memory ECS state only (no persistent storage) (024-level-navigation-bricks) | ||
| - Rust 1.81 (2021 edition), Bevy 0.17.3 + bevy_rapier3d 0.32.0 (physics), serde 1.0 + ron 0.8 (level loading), tracing 0.1 (logging) (025-ball-spawn-bricks) | ||
| - In-memory ECS state only; levels persisted as RON files in `assets/levels/` (025-ball-spawn-bricks) | ||
| - Rust 1.81 (edition 2021) + Bevy 0.17.3, bevy_rapier3d 0.32.0, tracing 0.1, rand 0.x (029-collision-particle-feedback) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Extract the exact rand version from Cargo.toml
rg -n '^rand\s*=' Cargo.tomlRepository: cleder/brkrs
Length of output: 70
Update the documented rand dependency to match the repo’s actual constraint (and pin if reproducibility is required)
.github/agents/copilot-instructions.md lists rand 0.x, but the root Cargo.toml uses rand = "0" (major-only, still not exact). Update the stack line to match rand = "0" (or pin the exact rand version used via Cargo.lock) to keep the documentation reproducible.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/agents/copilot-instructions.md at line 20, Update the documented
dependency string in the stack line that currently shows "rand 0.x" to match the
repository's Cargo constraint "rand = \"0\"" (or replace it with the exact
version from Cargo.lock if you want a pinned, reproducible value); find the
stack line containing "Rust 1.81 (edition 2021) + Bevy 0.17.3, bevy_rapier3d
0.32.0, tracing 0.1, rand 0.x (029-collision-particle-feedback)" and change the
"rand 0.x" token to the chosen form ("rand = \"0\"" or the exact pinned
version).
| - [X] T011 [P] [US1] Add brick collision same-frame effect test in `tests/collision_particle_feedback.rs` | ||
| - [X] T012 [US1] Add exact contact-point spawn assertion test in `tests/collision_particle_feedback.rs` | ||
| - [X] T013 [US1] Add brick-destroyed-on-impact still-spawns-effect test in `tests/collision_particle_feedback.rs` | ||
| - [ ] T014 [US1] Record red-phase failing test proof notes in `specs/029-collision-particle-feedback/checklists/requirements.md` |
There was a problem hiding this comment.
Complete the red-phase proof documentation to satisfy TDD gate requirement.
Task T014 remains unchecked while all implementation tasks (T015-T019) are marked complete. This violates the constitution's TDD gate requirement that proof-of-failure commits must be recorded before implementation begins.
Based on learnings, the TDD workflow requires documenting the failing-test phase. Please complete T014 or confirm that the red-phase proof has been properly captured in the feature's requirement checklist.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/029-collision-particle-feedback/tasks.md` at line 40, Task T014 (the
red-phase proof) is missing; update the feature checklist entry for T014 in the
requirements.md checklist by recording the failing-test proof: include the
failing test name(s), clear steps-to-reproduce, the minimal failing
assertion/output, the commit hash (or PR/branch) that contains the red-phase
proof, author and timestamp, and any relevant logs/screenshots; alternatively,
if the red-phase was already captured elsewhere, add a brief cross-reference in
the T014 checklist entry pointing to the exact commit/PR/test file and mark T014
complete.
| brick_destroyed_on_impact: !is_multi_hit_brick(current_type), | ||
| }); |
There was a problem hiding this comment.
brick_destroyed_on_impact can be wrong for non-destroyed brick hits.
This field is set before paddle-only/indestructible skip checks, so some non-destroying ball impacts can be emitted as true.
💡 Proposed fix
- brick_destroyed_on_impact: !is_multi_hit_brick(current_type),
+ brick_destroyed_on_impact: !is_multi_hit_brick(current_type)
+ && !crate::level_format::is_paddle_destroyable_brick(current_type)
+ && current_type < crate::level_format::INDESTRUCTIBLE_BRICK,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib.rs` around lines 845 - 846, The field brick_destroyed_on_impact is
being set too early using !is_multi_hit_brick(current_type) and can be wrong
when paddle-only or indestructible skip logic later prevents destruction; move
or recompute this field after the paddle-only/indestructible skip checks and use
the actual destruction outcome (e.g., a boolean like was_destroyed or the final
logic that applies skips and decrements hits) when constructing the event
instead of relying solely on is_multi_hit_brick(current_type).
…or improved clarity and performance
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/collision_particle_feedback.rs (1)
33-36:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix nondeterministic
advance_timeintests/collision_particle_feedback.rsby usingTimeUpdateStrategy::ManualDuration
update_feedback_effect_lifetimesincrements lifetimes usingtime.delta_secs(), andmake_test_appusesMinimalPluginswith noTimeUpdateStrategyconfigured in the repo—so Bevy’s time system runs ahead ofUpdateand overrides theTime::advance_by(...)delta you set inadvance_time.💡 Fix
fn advance_time(app: &mut App, delta_secs: f32) { app.insert_resource(TimeUpdateStrategy::ManualDuration( Duration::from_secs_f32(delta_secs), )); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/collision_particle_feedback.rs` around lines 33 - 36, The test's advance_time function is nondeterministic because Bevy's time system advances on its own; instead of mutating Time::advance_by, set TimeUpdateStrategy::ManualDuration so the delta used by update_feedback_effect_lifetimes is exactly the value you want. Replace the body of advance_time to insert the resource TimeUpdateStrategy::ManualDuration(Duration::from_secs_f32(delta_secs)) into the App (references: function advance_time, test helper make_test_app, and the update_feedback_effect_lifetimes logic that reads time.delta_secs()).
🧹 Nitpick comments (1)
src/systems/collision_feedback.rs (1)
257-291: 💤 Low valueRedundant
particle_refsquery.
particlescan be iterated again for the despawn pass once the mutable borrow from the first loop ends, so the extra read-onlyparticle_refsparameter isn't needed.♻️ Drop the duplicate query
pub fn update_feedback_effect_lifetimes( time: Res<Time>, mut commands: Commands, mut effects: Query<(Entity, &mut FeedbackEffectInstance)>, mut particles: Query<(Entity, &CollisionFeedbackParticle, &mut Transform)>, - particle_refs: Query<(Entity, &CollisionFeedbackParticle)>, ) { @@ - for (particle_entity, particle) in particle_refs.iter() { + for (particle_entity, particle, _) in particles.iter() { if expired.contains(&particle.source_effect) { commands.entity(particle_entity).despawn(); } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/systems/collision_feedback.rs` around lines 257 - 291, The function update_feedback_effect_lifetimes has an unused duplicate read-only query parameter particle_refs; remove the particle_refs parameter from the function signature and all call sites, and after the first loop that updates transforms (which ends the mutable borrow), iterate the existing particles Query<(Entity, &CollisionFeedbackParticle, &mut Transform)> a second time (the mutable borrow will have ended) to despawn particles whose source_effect is in expired—i.e., replace the particle_refs-based loop with a loop over the same particles Query to call commands.entity(particle_entity).despawn() when expired.contains(&particle.source_effect); keep the existing logic that collects expired effects and despawns effects via commands.entity(*effect).despawn().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/collision_particle_feedback.rs`:
- Around line 33-36: The test's advance_time function is nondeterministic
because Bevy's time system advances on its own; instead of mutating
Time::advance_by, set TimeUpdateStrategy::ManualDuration so the delta used by
update_feedback_effect_lifetimes is exactly the value you want. Replace the body
of advance_time to insert the resource
TimeUpdateStrategy::ManualDuration(Duration::from_secs_f32(delta_secs)) into the
App (references: function advance_time, test helper make_test_app, and the
update_feedback_effect_lifetimes logic that reads time.delta_secs()).
---
Nitpick comments:
In `@src/systems/collision_feedback.rs`:
- Around line 257-291: The function update_feedback_effect_lifetimes has an
unused duplicate read-only query parameter particle_refs; remove the
particle_refs parameter from the function signature and all call sites, and
after the first loop that updates transforms (which ends the mutable borrow),
iterate the existing particles Query<(Entity, &CollisionFeedbackParticle, &mut
Transform)> a second time (the mutable borrow will have ended) to despawn
particles whose source_effect is in expired—i.e., replace the
particle_refs-based loop with a loop over the same particles Query to call
commands.entity(particle_entity).despawn() when
expired.contains(&particle.source_effect); keep the existing logic that collects
expired effects and despawns effects via commands.entity(*effect).despawn().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 285b31b5-42bd-4554-bc26-a3668efa56a3
📒 Files selected for processing (4)
src/lib.rssrc/signals.rssrc/systems/collision_feedback.rstests/collision_particle_feedback.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/signals.rs
- src/lib.rs
Summary by Sourcery
Add an observer-driven collision particle feedback system that spawns short-lived spark effects for ball collisions with walls, paddle, and bricks, wired into existing collision handlers and gated by game state.
New Features:
Enhancements:
Documentation:
Tests:
Chores:
Summary by CodeRabbit
New Features
Documentation
Tests