Conversation
Reviewer's GuideThis PR enriches the Lille Datalog model with velocity and force constants and implements dynamic acceleration/velocity rules, alongside extending the ddlog tests to verify the new relations. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. """ WalkthroughPhysics modelling capabilities were added to the logic code, introducing new functions, constants, and rules for handling entity dynamics such as velocity, force, acceleration, gravity, friction, and terminal velocity. The test suite was updated to assert the existence of these new relations and rules within the source. Additionally, the Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
🪧 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}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Docstrings generation was requested by @leynos. * #23 (comment) The following files were modified: * `tests/ddlog.rs`
|
Note Generated docstrings for this pull request at #24 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/lille.dl (1)
125-129: Time-step factor omitted
UnclampedNewVelocityadds acceleration directly to velocity, implicitly assuming Δt = 1 tick.
If the engine later supports variable-length steps, storingΔtexplicitly will avoid future rewrites.tests/ddlog.rs (1)
94-102: Token-based check is brittleSplitting on non-alphanumerics cannot detect misspellings such as
NewVelocty.
For stronger guarantees, parse the.dlonce and match relation IDs, or generate the list programmatically from the DDlog compiler output.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lille.dl(3 hunks)tests/ddlog.rs(1 hunks)
🔇 Additional comments (2)
src/lille.dl (2)
113-124: Aggregator shape may violate DDlog semanticsThe custom
agg(e) sum (…) from (…)clause combines three optional relations.
When all optional branches are absent, the aggregation still emits a tuple with uninitialised variables, which DDlog rejects.Consider emitting defaults earlier or introducing explicit
?optional aggregates to avoid runtime errors.
Example pattern:AppliedAcceleration?(…). GravitationalAcceleration?(…). FrictionalDeceleration?(…). NetAcceleration(e, sum(ax), …) :- …
10-12: Ensure externs are implemented and linked
vec_magandvec_normalizemust be supplied by the Rust side or a C shim.
Missing symbols will break compilation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/ddlog.rs (1)
8-19: Compile the regexes only once – avoid repeated recompilation in every test run
parsed_relations()andparsed_constants()rebuild theirRegexobjects on every call.
Even though this is “just a test”, the code is executed many times and the compilation step is by far the most expensive part ofregex.
Store the compiled instances instaticvariables instead (e.g.once_cell::sync::Lazyorlazy_static!) and you get an immediate, zero-cost speed-up without changing the public interface.-use regex::Regex; +use once_cell::sync::Lazy; +use regex::Regex; ... -fn parsed_relations() -> HashSet<String> { - let re = Regex::new(r"(?m)^\s*(?:input\s+(?:relation|stream)|output\s+relation|relation)\s+([A-Za-z_][A-Za-z0-9_]*)").unwrap(); - re.captures_iter(DL_SRC).map(|c| c[1].to_string()).collect() -} +static REL_RE: Lazy<Regex> = Lazy::new(|| { + Regex::new(r"(?m)^\s*(?:input\s+(?:relation|stream)|output\s+relation|relation)\s+([A-Za-z_][A-Za-z0-9_]*)") + .unwrap() +}); + +fn parsed_relations() -> HashSet<String> { + REL_RE.captures_iter(DL_SRC).map(|c| c[1].to_string()).collect() +} -fn parsed_constants() -> HashSet<String> { - let re = Regex::new(r"(?m)^\s*const\s+([A-Za-z_][A-Za-z0-9_]*)").unwrap(); - re.captures_iter(DL_SRC).map(|c| c[1].to_string()).collect() -} +static CONST_RE: Lazy<Regex> = Lazy::new(|| { + Regex::new(r"(?m)^\s*const\s+([A-Za-z_][A-Za-z0-9_]*)").unwrap() +}); + +fn parsed_constants() -> HashSet<String> { + CONST_RE.captures_iter(DL_SRC).map(|c| c[1].to_string()).collect() +}A tiny change but it makes the test suite leaner and more future-proof.
(You’ll need to addonce_cell = "1"under[dev-dependencies].)src/lille.dl (1)
131-135: Only downward speed is capped – upward launches remain unbounded
NewVelocityclampsraw_nvzto-TERMINAL_VELOCITY, but positive vertical speeds are left untouched.
If entities can receive large upward impulses (e.g. explosions or scripted jumps), they may reach arbitrarily high values.Consider symmetric clamping:
var final_nvz = clamp(raw_nvz, -TERMINAL_VELOCITY, TERMINAL_VELOCITY)(or separate branch for upward case).
Please verify whether unlimited upward velocity is intentional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Cargo.toml(1 hunks)src/lille.dl(3 hunks)tests/ddlog.rs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
🔇 Additional comments (1)
src/lille.dl (1)
94-96: Gravity sign corrected – looks goodThe earlier negation has been removed and
GRAVITY_PULL(already negative) is now used directly.
This restores the intended downward acceleration.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
Cargo.toml (1)
24-25: Consider pinning versions more tightly or documenting your upgrade policyUsing bare
"1"allows any future1.xrelease, which is semver-compatible but can still introduce breaking behaviour for macros/const-fns that tests rely on.
If deterministic CI builds are important, either:regex = "=1.10.4" # exact # or regex = "1.10" # minor-range…and likewise for
once_cell.Not urgent, just be explicit about the intended policy.
tests/ddlog.rs (3)
3-6: Import ordering is fine, butHashSetcan come fromstd::collectionswithout a globThe direct
use std::collections::HashSet;is correct; if other collection types are added later, consider grouping:use std::collections::{HashSet, HashMap};Pure style note.
9-15:REL_REalso matches commented-out declarationsBecause the pattern allows leading whitespace only, a line like
// input relation Velocity …will still satisfy the regex after the slashes are trimmed.If you want to ignore comments, add a negative look-ahead:
- r"(?m)^\s*(?:input\s+(?:relation|stream)|output\s+relation|relation)\s+([A-Za-z_][A-Za-z0-9_]*)" + r"(?m)^\s*(?!//)(?:input\s+(?:relation|stream)|output\s+relation|relation)\s+([A-Za-z_][A-Za-z0-9_]*)"Same applies to
CONST_REbelow.
23-31: Duplicate logic withCONST_RE– small helper could DRY thisBoth helper fns share the “compile regex → capture names → collect HashSet” pattern.
A genericfn capture_set(re: &Regex) -> HashSet<String>would avoid duplication and make future token kinds easier to add.Not critical, just a maintainability win.
src/lille.dl (2)
17-22: Constants look reasonable – consider documenting unitsE.g. is
TERMINAL_VELOCITYmetres / tick? Ground friction is dimensionless?
A brief comment inline prevents confusion later.
97-113: Friction rules duplicated – can be unifiedThe two
FrictionalDecelerationclauses differ only by the coefficient (GROUND_FRICTIONvsAIR_FRICTION).
You can factor this to avoid copy-paste and make future changes simpler:-relation FrictionalDeceleration(e: EntityID, ax: GCoord, ay: GCoord, az: GCoord) -FrictionalDeceleration(e, fdx, fdy, 0.0) :- - IsStanding(e), +relation FrictionalDeceleration(e: EntityID, ax: GCoord, ay: GCoord, az: GCoord) +FrictionalDeceleration(e, fdx, fdy, 0.0) :- + (IsStanding(e), var coeff = GROUND_FRICTION; + IsUnsupported(e), var coeff = AIR_FRICTION), Velocity(e, vx, vy, _), var h_mag = vec_mag(vx, vy, 0.0), h_mag > 0.0, var (nx, ny, _) = vec_normalize(vx, vy, 0.0), - var decel_mag = min(h_mag, GROUND_FRICTION), + var decel_mag = min(h_mag, coeff), fdx = -nx * decel_mag, fdy = -ny * decel_mag.(Datalog pseudo-syntax; adjust as needed.)
Reduces duplication and removes the risk of the two rules drifting apart.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Cargo.toml(1 hunks)src/lille.dl(3 hunks)tests/ddlog.rs(2 hunks)
🔇 Additional comments (4)
src/lille.dl (4)
10-12: Confirm host implementation ofvec_mag/vec_normalizehandles zero-length vectors
vec_normalize(0,0,0)is never called in current rules, but future changes could.
Ensure the extern functions either return(0,0,0)or raise a controlled error to avoid NaNs propagating through the program.
95-96: Gravity sign issue fixed – good catchSwitching from
-GRAVITY_PULLtoGRAVITY_PULLresolves the earlier upward acceleration bug.
114-125: Cartesian-product bug resolved withNetAccelRow– nice refactorFlattening to
NetAccelRowthen aggregating eliminates the duplicate-force issue highlighted earlier.
127-141: Vertical velocity is forcibly zeroed for standing entities – confirm gameplay intentSetting
vzto0.0every tick prevents small upward hops when stepping onto blocks or minor slopes.
If the design expects a short aerial phase when walking off edges, this is fine; otherwise consider preserving vertical component and clamping to 0 only whenvz < 0(downward) and the entity is on the ground.Just a heads-up – no change requested.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/lille.dl (1)
140-142:⚠️ Potential issue
NewPositionstill mirrorsPosition– simulation is frozenThe physics step never integrates velocity into position; entities will not move even though velocity is updated.
-NewPosition(e, x, y, z) :- Position(e, x, y, z). +NewPosition(e, x + nvx * DELTA_TIME, + y + nvy * DELTA_TIME, + z + nvz * DELTA_TIME) :- + Position(e, x, y, z), + NewVelocity(e, nvx, nvy, nvz).
🧹 Nitpick comments (1)
src/lille.dl (1)
102-111: Friction zero-snaps slow objects
decel_mag = min(h_mag, coeff)makes friction equal to the whole speed wheneverh_mag < coeff, bringing velocity to exactly zero in one tick.
This produces noticeable “stick-slip” behaviour when objects crawl.Consider a proportional model:
-var decel_mag = min(h_mag, coeff), +var decel_mag = coeff * DELTA_TIME,and clamp it with
min(h_mag, decel_mag)afterwards to avoid overshoot.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lille.dl(3 hunks)tests/ddlog.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/ddlog.rs
🔇 Additional comments (1)
src/lille.dl (1)
130-138: Vertical velocity is always nuked for standing entitiesWhen
IsStanding(e)holds, vertical forces (e.g. jump impulse) are ignored becausenvzis hard-coded to0.0.
If future gameplay should allow jumping or elevation while on the ground, this rule will block it.Verify that this is deliberate; otherwise gate the zeroing behind
vz < 0(prevent sinking) rather than unconditional.
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Absence of Expected Change Pattern
- 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.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/vector_math.rs (2)
3-6: Function name vs documentation spelling mismatch.Doc-comment uses American spelling “Normalizes” (line 8), whereas the rest of the codebase and this PR description follow British English. Consider switching to “Normalises” for consistency.
8-17: Prefertry_normalizeto remove duplicate length check and handle NaNs.
Vec3::normalize()is only safe for non-zero, finite vectors.glamalready offerstry_normalize()which:
- Avoids recomputing length squared.
- Safely returns
Nonefor zero or non-finite inputs.This shortens the code and eliminates a branch.
pub fn vec_normalize(x: f32, y: f32, z: f32) -> (f32, f32, f32) { let v = Vec3::new(x, y, z); - if v.length_squared() > 0.0 { - let n = v.normalize(); - (n.x, n.y, n.z) - } else { - (0.0, 0.0, 0.0) - } + if let Some(n) = v.try_normalize() { + (n.x, n.y, n.z) + } else { + (0.0, 0.0, 0.0) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib.rs(2 hunks)src/vector_math.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/lib.rs
🔇 Additional comments (1)
src/vector_math.rs (1)
1-1: Checkglamis declared as a normal (non-dev) dependency.
vector_math.rsis compiled in the main crate, soglammust live under[dependencies]inCargo.toml, not[dev-dependencies]. Please verify to avoid build failures when the crate is consumed as a library.
#24) Docstrings generation was requested by @leynos. * #23 (comment) The following files were modified: * `tests/ddlog.rs` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Absence of Expected Change Pattern
- 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.
1 participant
Summary
lille.dlddlogtestsTesting
cargo clippy --all-targets --all-features -- -D warningscargo test --all-targetshttps://chatgpt.com/codex/tasks/task_e_684cef8c2e948322897b086eb24951c9
Summary by Sourcery
Add velocity and force constants to the Datalog schema, implement dynamic acceleration and velocity rules, and update tests to verify the new relations.
New Features:
Tests:
Summary by CodeRabbit
New Features
Tests
Chores