Conversation
Reviewer's GuideThis PR implements full velocity-based physics by adding a Velocity component, extending DdlogHandle to track forces and emit velocity deltas, enhancing the physics pipeline to compute acceleration (from external forces, gravity, friction, and terminal velocity), and updating sync/spawn systems accordingly, with new BDD tests validating force application and ground friction behavior. Sequence diagram for ECS-DDlog velocity synchronizationsequenceDiagram
participant ECS as ECS Entity
participant Sync as push_state_to_ddlog_system
participant DDlog as DdlogHandle
participant Apply as apply_ddlog_deltas_system
ECS->>Sync: Entity state (Transform, Velocity, etc.)
Sync->>DDlog: Update DdlogEntity (position, velocity)
DDlog->>DDlog: step() (physics update)
DDlog-->>Apply: velocity_deltas, deltas
Apply->>ECS: Update Transform, Velocity component
Class diagram for new and updated physics componentsclassDiagram
class DdlogEntity {
Vec3 position
Vec3 velocity
UnitType unit
i32 health
Option<Vec2> target
}
class NewVelocity {
i64 entity
f32 vx
f32 vy
f32 vz
}
class DdlogHandle {
Vec<Block> blocks
HashMap<i64, BlockSlope> slopes
HashMap<i64, DdlogEntity> entities
Vec<NewPosition> deltas
Vec<NewVelocity> velocity_deltas
HashMap<i64, Vec3> forces
apply_force(id: i64, force: Vec3)
step()
}
class Velocity {
Vec3 0
}
DdlogEntity --> NewVelocity : emits
DdlogHandle "1" o-- "*" DdlogEntity : manages
DdlogHandle --> NewVelocity : emits
DdlogEntity --> Velocity : ECS sync
Velocity <|-- NewVelocity : data transfer
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant ECS
participant DDlogHandle
participant DDlog
participant Commands
Note over ECS, DDlogHandle: Push ECS state to DDlog
ECS->>DDlogHandle: Query (DdlogId, Transform, ..., Velocity)
DDlogHandle->>DDlog: Update DdlogEntity with velocity
Note over DDlog, ECS: Apply DDlog deltas to ECS
DDlog->>DDlogHandle: Provide position & velocity deltas
DDlogHandle->>ECS: Query (Entity, DdlogId, Transform, Velocity)
alt Velocity exists
DDlogHandle->>ECS: Update Velocity component
else Velocity missing
DDlogHandle->>Commands: Insert Velocity component
end
DDlogHandle->>ECS: Update Transform (position)
sequenceDiagram
participant GameLogic
participant DdlogHandle
GameLogic->>DdlogHandle: apply_force(entity_id, force)
DdlogHandle->>DdlogHandle: Store force for entity
Note over DdlogHandle: On tick/update
DdlogHandle->>DdlogHandle: Compute acceleration, gravity, friction
DdlogHandle->>DdlogHandle: Update velocity and position
DdlogHandle->>DDlog: Push velocity and position deltas
Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate Unit Tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Absence of Expected Change Pattern
- lille/src/spawn_world.rs is usually changed with: lille/tests/spawn.rs
- lille/src/lib.rs is usually changed with: lille/src/main.rs
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- The
apply_forcemethod currently overwrites any existing force for the same entity—if you ever need to combine multiple force sources in a single tick, you may want to accumulate forces (e.g. via entry API) instead of replacing them. - The
compute_entity_updatemethod mixes gravity, friction, input forces, walking logic, velocity clamping, and position updates; consider splitting it into smaller helper functions (e.g. apply_gravity, apply_friction, integrate_motion) to improve readability and facilitate targeted unit testing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `apply_force` method currently overwrites any existing force for the same entity—if you ever need to combine multiple force sources in a single tick, you may want to accumulate forces (e.g. via entry API) instead of replacing them.
- The `compute_entity_update` method mixes gravity, friction, input forces, walking logic, velocity clamping, and position updates; consider splitting it into smaller helper functions (e.g. apply_gravity, apply_friction, integrate_motion) to improve readability and facilitate targeted unit testing.
## Individual Comments
### Comment 1
<location> `src/ddlog_handle.rs:151` </location>
<code_context>
+ let unsupported = ent.position.z > floor + GRACE_DISTANCE_F32;
+
+ let mut acceleration = Vec3::ZERO;
+ if let Some(force) = self.forces.get(&id) {
+ acceleration += *force / DEFAULT_MASS_F32;
+ }
+
</code_context>
<issue_to_address>
Force application overwrites previous force rather than accumulating.
Since `apply_force` overwrites previous values, only the last force per entity is used each tick. If multiple forces should be combined, consider accumulating them instead.
Suggested implementation:
```rust
// In the struct definition (likely above, not shown here):
// forces: HashMap<i64, Vec3>,
// In the apply_force method:
pub fn apply_force(&mut self, id: i64, force: Vec3) {
self.forces
.entry(id)
.and_modify(|f| *f += force)
.or_insert(force);
}
```
If there are any places in the codebase that rely on the old "overwrite" behavior of `apply_force`, they may need to be updated to clear or reset the force before applying a new one, if that is still desired in those contexts.
</issue_to_address>
### Comment 2
<location> `src/ddlog_handle.rs:232` </location>
<code_context>
}
}
}
+ self.forces.clear();
}
}
</code_context>
<issue_to_address>
Forces are cleared at the end of each step, which may cause missed forces if step is not called every tick.
This approach relies on `step` being called exactly once per tick; otherwise, force application may be inconsistent. Please confirm that this scheduling is enforced.
</issue_to_address>
### Comment 3
<location> `src/ddlog_handle.rs:21` </location>
<code_context>
+const AIR_FRICTION_F32: f32 = AIR_FRICTION as f32;
+const DEFAULT_MASS_F32: f32 = DEFAULT_MASS as f32;
#[derive(Clone, Serialize)]
pub struct DdlogEntity {
pub position: Vec3,
</code_context>
<issue_to_address>
Missing module-level item comment (`//!`) at the top of the file.
Please add a module-level item comment at the top of this file using `//!` to describe the purpose of the module.
</issue_to_address>
### Comment 4
<location> `src/ddlog_sync.rs:6` </location>
<code_context>
use hashbrown::HashMap;
-use crate::components::{Block, BlockSlope, DdlogId, Health, Target, UnitType};
+use crate::components::{Block, BlockSlope, DdlogId, Health, Target, UnitType, Velocity};
use crate::ddlog_handle::{DdlogEntity, DdlogHandle};
</code_context>
<issue_to_address>
Missing module-level item comment (`//!`) at the top of the file.
Please add a module-level item comment at the top of this file using `//!` to describe the purpose of the module.
</issue_to_address>
### Comment 5
<location> `src/components.rs:36` </location>
<code_context>
pub grad_y: f32,
}
+
+#[derive(Component, Debug, Default, Serialize)]
+pub struct Velocity(pub Vec3);
--- a/src/lib.rs
</code_context>
<issue_to_address>
Missing module-level item comment (`//!`) at the top of the file.
Please add a module-level item comment at the top of this file using `//!` to describe the purpose of the module.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if let Some(force) = self.forces.get(&id) { | ||
| acceleration += *force / DEFAULT_MASS_F32; |
There was a problem hiding this comment.
suggestion: Force application overwrites previous force rather than accumulating.
Since apply_force overwrites previous values, only the last force per entity is used each tick. If multiple forces should be combined, consider accumulating them instead.
Suggested implementation:
// In the struct definition (likely above, not shown here):
// forces: HashMap<i64, Vec3>,
// In the apply_force method:
pub fn apply_force(&mut self, id: i64, force: Vec3) {
self.forces
.entry(id)
.and_modify(|f| *f += force)
.or_insert(force);
}If there are any places in the codebase that rely on the old "overwrite" behavior of apply_force, they may need to be updated to clear or reset the force before applying a new one, if that is still desired in those contexts.
| } | ||
| } | ||
| } | ||
| self.forces.clear(); |
There was a problem hiding this comment.
question (bug_risk): Forces are cleared at the end of each step, which may cause missed forces if step is not called every tick.
This approach relies on step being called exactly once per tick; otherwise, force application may be inconsistent. Please confirm that this scheduling is enforced.
| const AIR_FRICTION_F32: f32 = AIR_FRICTION as f32; | ||
| const DEFAULT_MASS_F32: f32 = DEFAULT_MASS as f32; | ||
|
|
||
| #[derive(Clone, Serialize)] |
There was a problem hiding this comment.
issue (review_instructions): Missing module-level item comment (//!) at the top of the file.
Please add a module-level item comment at the top of this file using //! to describe the purpose of the module.
Review instructions:
Path patterns: **/*.rs
Instructions:
All modules MUST have a containing item comment (//!)
| use hashbrown::HashMap; | ||
|
|
||
| use crate::components::{Block, BlockSlope, DdlogId, Health, Target, UnitType}; | ||
| use crate::components::{Block, BlockSlope, DdlogId, Health, Target, UnitType, Velocity}; |
There was a problem hiding this comment.
issue (review_instructions): Missing module-level item comment (//!) at the top of the file.
Please add a module-level item comment at the top of this file using //! to describe the purpose of the module.
Review instructions:
Path patterns: **/*.rs
Instructions:
All modules MUST have a containing item comment (//!)
| pub grad_y: f32, | ||
| } | ||
|
|
||
| #[derive(Component, Debug, Default, Serialize)] |
There was a problem hiding this comment.
issue (review_instructions): Missing module-level item comment (//!) at the top of the file.
Please add a module-level item comment at the top of this file using //! to describe the purpose of the module.
Review instructions:
Path patterns: **/*.rs
Instructions:
All modules MUST have a containing item comment (//!)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
src/components.rs (1)
36-38: Derive Copy/Clone & Deref forVelocityfor ergonomic use
Vec3isCopy, so the wrapper can also beCopy + Clone.
AddingDeref/DerefMut, as done forTarget, keeps the API consistent and lets systems treatVelocitytransparently as aVec3.-#[derive(Component, Debug, Default, Serialize)] -pub struct Velocity(pub Vec3); +#[derive(Component, Debug, Default, Copy, Clone, Deref, DerefMut, Serialize)] +pub struct Velocity(pub Vec3);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (4)
tests/snapshots/physics_bdd__force_position.snapis excluded by!**/*.snaptests/snapshots/physics_bdd__force_velocity.snapis excluded by!**/*.snaptests/snapshots/physics_bdd__friction_position.snapis excluded by!**/*.snaptests/snapshots/physics_bdd__friction_velocity.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
src/components.rs(1 hunks)src/ddlog_handle.rs(6 hunks)src/ddlog_sync.rs(4 hunks)src/lib.rs(1 hunks)src/spawn_world.rs(4 hunks)tests/ddlog.rs(3 hunks)tests/physics_bdd.rs(3 hunks)
🔇 Additional comments (5)
tests/ddlog.rs (1)
53-56: Velocity initialisation aligns tests with the new componentAll three entities now start with
Vec3::ZERO; this keeps the tests deterministic and reflects the updated schema.
Nothing else to flag.Also applies to: 85-88, 95-98
src/lib.rs (1)
15-15: Re-export completes the public surfaceThis makes
Velocityavailable to crates that depend onlille; good addition.src/ddlog_sync.rs (1)
50-50: Well-implemented velocity synchronization.The velocity handling correctly defaults to
Vec3::ZEROwhen absent and properly synchronizes between ECS and DDlog. The dynamic component insertion ensures backward compatibility.Also applies to: 74-79
src/ddlog_handle.rs (2)
159-171: Robust friction implementation.The friction calculation correctly prevents velocity reversal by clamping deceleration magnitude to current velocity magnitude, and properly handles the zero velocity case.
146-192: Well-structured physics simulation.The physics implementation properly handles:
- Force-based acceleration with configurable mass
- Gravity application for unsupported entities
- Separate air and ground friction coefficients
- Terminal velocity clamping
- Combined velocity and AI-driven movement for ground entities
| use bevy::prelude::*; | ||
|
|
||
| use crate::components::{DdlogId, Health, Target, UnitType}; | ||
| use crate::components::{DdlogId, Health, Target, UnitType, Velocity}; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider skipping Velocity on truly static entities
Assigning Velocity::default() to the landmark is harmless, but also unused and slightly noisy.
If the landmark will never move, omitting the component avoids an unnecessary sync to DDlog every frame.
No action required if you plan on animating landmarks later.
Also applies to: 54-55, 65-66, 76-77
🤖 Prompt for AI Agents
In src/spawn_world.rs around lines 54-55, 65-66, and 76-77, the Velocity
component is assigned to static entities like landmarks where it is unused. To
reduce unnecessary updates and syncing to DDlog, remove the Velocity::default()
assignment for these truly static entities. Only add Velocity if the entity is
expected to move or animate later.
| #[allow(clippy::type_complexity)] | ||
| pub fn push_state_to_ddlog_system( | ||
| mut ddlog: ResMut<DdlogHandle>, | ||
| entity_query: Query<(&DdlogId, &Transform, &Health, &UnitType, Option<&Target>)>, | ||
| entity_query: Query<( | ||
| &DdlogId, | ||
| &Transform, | ||
| &Health, | ||
| &UnitType, | ||
| Option<&Target>, | ||
| Option<&Velocity>, | ||
| )>, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider refactoring the complex query tuple.
The query tuple has grown to 6 components, necessitating the clippy::type_complexity suppression. This could indicate a design issue that would benefit from refactoring.
Consider bundling related components into a single component or using a query filter pattern to reduce complexity:
-#[allow(clippy::type_complexity)]
pub fn push_state_to_ddlog_system(
mut ddlog: ResMut<DdlogHandle>,
- entity_query: Query<(
- &DdlogId,
- &Transform,
- &Health,
- &UnitType,
- Option<&Target>,
- Option<&Velocity>,
- )>,
+ entity_query: Query<(&DdlogId, &Transform, &Health, &UnitType), With<DdlogId>>,
+ target_query: Query<&Target>,
+ velocity_query: Query<&Velocity>,
block_query: Query<(&Block, Option<&BlockSlope>)>,
) {Then query the optional components separately within the loop.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/ddlog_sync.rs around lines 11 to 21, the Query tuple has six components
causing high type complexity and requiring a clippy suppression. Refactor by
grouping related components into a single struct component or by splitting the
query into multiple queries, one for mandatory components and separate queries
or filters for optional components. Then, inside the loop, query the optional
components separately to reduce the tuple size and eliminate the need for the
clippy suppression.
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Absence of Expected Change Pattern
- lille/src/spawn_world.rs is usually changed with: lille/tests/spawn.rs
- lille/src/lib.rs is usually changed with: lille/src/main.rs
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
tests/snapshots/physics_bdd__friction_position.snapis excluded by!**/*.snaptests/snapshots/physics_bdd__friction_velocity.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
src/ddlog_handle.rs(6 hunks)src/spawn_world.rs(3 hunks)tests/physics_bdd.rs(3 hunks)
🔇 Additional comments (2)
src/spawn_world.rs (1)
65-66: Good use ofVelocityon dynamic entitiesAdding
Velocity::default()only to the units that will actually move keeps the component set minimal and avoids needless DDlog sync noise.
Looks clean.Also applies to: 76-77
src/ddlog_handle.rs (1)
190-195: ```shell
#!/bin/bash
set -e
echo "Extracting full civvy_move function body"
rg -n "fn civvy_move" -C50 src/ddlog_handle.rs</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| let rounded: Vec<NewPosition> = ddlog | ||
| .deltas | ||
| .iter() | ||
| .map(|d| NewPosition { | ||
| entity: d.entity, | ||
| x: (d.x * 1e4).round() / 1e4, | ||
| y: (d.y * 1e4).round() / 1e4, | ||
| z: (d.z * 1e4).round() / 1e4, | ||
| }) | ||
| .collect(); | ||
| assert_ron_snapshot!("falling_delta", &rounded); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Factor-out duplicate rounding/snapshot helper
Each test re-implements the same “round to 1e-4 then snapshot” mapping for NewPosition / NewVelocity.
Extract a small helper (or use approx assertions) to keep tests concise and reduce copy-paste risk.
Also applies to: 104-127, 147-169
🤖 Prompt for AI Agents
In tests/physics_bdd.rs around lines 69 to 79, the rounding and snapshot logic
for NewPosition is duplicated across multiple tests. To fix this, create a
helper function that takes a slice of NewPosition or NewVelocity and returns a
Vec with each element rounded to 1e-4 precision. Replace the inline mapping in
each test with calls to this helper, then perform the snapshot assertion on the
helper's output. Apply the same refactoring to the code in lines 104-127 and
147-169 to eliminate duplication and improve test clarity.
| let vel_xy = ent.velocity.truncate(); | ||
| let h_mag = vel_xy.length(); | ||
| if h_mag > 0.0 { | ||
| let coeff = if unsupported { | ||
| AIR_FRICTION_F32 | ||
| } else { | ||
| GROUND_FRICTION_F32 | ||
| }; | ||
| let decel_mag = h_mag.min(coeff); | ||
| let dir = vel_xy / h_mag; | ||
| acceleration.x -= dir.x * decel_mag; | ||
| acceleration.y -= dir.y * decel_mag; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Friction formula may over-decelerate slow movers
decel_mag = h_mag.min(coeff) applies the full coeff even when velocity is tiny (e.g. 0.01 with coeff 0.1 → decel 0.1, reversing direction).
Consider scaling by velocity magnitude or using a clamp that can’t exceed h_mag after the * DELTA_TIME step.
🤖 Prompt for AI Agents
In src/ddlog_handle.rs around lines 165 to 177, the friction deceleration
calculation uses decel_mag = h_mag.min(coeff), which can cause over-deceleration
and reverse velocity direction for very small speeds. To fix this, modify the
deceleration magnitude to scale with velocity and delta time, ensuring it never
exceeds the current velocity magnitude after applying the time step. This
prevents reversing direction by clamping deceleration appropriately.
| let updates: Vec<(i64, Vec3, Vec3)> = self | ||
| .entities | ||
| .iter() | ||
| .map(|(&id, ent)| (id, self.compute_entity_update(id, ent))) | ||
| .map(|(&id, ent)| { | ||
| let (pos, vel) = self.compute_entity_update(id, ent); | ||
| (id, pos, vel) | ||
| }) | ||
| .collect(); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid temporary allocation in step
Collecting updates into a Vec then iterating creates an unnecessary allocation each tick.
Iterate mutably in place:
for (&id, ent) in self.entities.iter_mut() {
let (pos, vel) = self.compute_entity_update(id, ent);
...
}This removes the heap hit and a copy of each Vec3.
🤖 Prompt for AI Agents
In src/ddlog_handle.rs around lines 206 to 214, the current code collects entity
updates into a Vec, causing unnecessary heap allocation and copying each tick.
To fix this, replace the collection step with a mutable iteration over
self.entities using iter_mut(), compute updates in place, and apply changes
directly within the loop to avoid temporary allocation and copying of Vec3
values.
1 participant
Summary
NewVelocityback into Bevy componentsDdlogHandlewith velocity tracking and force inputVelocitycomponentsTesting
cargo clippy -- -D warningscargo testcargo test --test physics_bddhttps://chatgpt.com/codex/tasks/task_e_6852a8754ea88322b8ac6a5497fff470
Summary by Sourcery
Implement per-entity velocity tracking and physics forces in the DDlog backend, and add BDD tests for gravity, force application, and friction behavior.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Tests