core: split math module into focused submodules #5
Conversation
|
Warning Rate limit exceeded@flyingrobots has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 10 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
Summary by CodeRabbit
WalkthroughRemoved the monolithic Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant MathMod as math::mod
participant Mat4
participant Vec3
Note over MathMod,Mat4: Mat4::transform_point (column-major, w=1)
Caller->>Mat4: transform_point(&self, &Vec3)
Mat4->>Vec3: read components (x,y,z)
Mat4-->>Caller: Vec3 (transformed)
sequenceDiagram
participant Caller
participant PRNG as Prng
Note right of PRNG: next_int(min,max) uses rejection sampling to avoid bias
Caller->>PRNG: next_int(min,max)
PRNG->>PRNG: compute span, check fast paths (equal/power-of-two)
alt power-of-two or trivial
PRNG-->>Caller: masked bits -> result
else otherwise
PRNG->>PRNG: loop next_u64 until within bound (rejection)
PRNG-->>Caller: unbiased integer in [min,max]
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
crates/rmg-core/src/math.rs(0 hunks)crates/rmg-core/src/math/mat4.rs(1 hunks)crates/rmg-core/src/math/mod.rs(1 hunks)crates/rmg-core/src/math/prng.rs(1 hunks)crates/rmg-core/src/math/quat.rs(1 hunks)crates/rmg-core/src/math/vec3.rs(1 hunks)
💤 Files with no reviewable changes (1)
- crates/rmg-core/src/math.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Public Rust APIs across crates (e.g., rmg-core, rmg-ffi, rmg-wasm) must include rustdoc comments explaining intent, invariants, and usage
Files:
crates/rmg-core/src/math/quat.rscrates/rmg-core/src/math/prng.rscrates/rmg-core/src/math/mat4.rscrates/rmg-core/src/math/vec3.rscrates/rmg-core/src/math/mod.rs
🧬 Code graph analysis (3)
crates/rmg-core/src/math/quat.rs (2)
crates/rmg-core/src/math/mat4.rs (2)
new(17-19)from(82-84)crates/rmg-core/src/math/vec3.rs (4)
new(21-23)component(30-32)normalize(93-99)from(103-105)
crates/rmg-core/src/math/mat4.rs (1)
crates/rmg-core/src/math/vec3.rs (3)
new(21-23)to_array(26-28)from(103-105)
crates/rmg-core/src/math/vec3.rs (2)
crates/rmg-core/src/math/mat4.rs (3)
new(17-19)to_array(22-24)from(82-84)crates/rmg-core/src/math/quat.rs (5)
new(17-19)to_array(22-24)component(26-28)normalize(73-89)from(136-138)
🔇 Additional comments (9)
crates/rmg-core/src/math/mod.rs (1)
27-35: Degree/radian helpers: good. Keep it simple.
No nits; TAU usage and f32 precision are fine.crates/rmg-core/src/math/quat.rs (2)
52-70: Hamilton product signs look rightThe multiplication matches the standard (x,y,z,w). No further action.
96-132: to_mat4() mapping and signs are correct for column-major + column vectorsNormalized first, proper 2* products, and column-major packing. Solid.
crates/rmg-core/src/math/mat4.rs (3)
30-46: Matrix multiply is correct and cache-friendly enough for nowRow/col/k loops compute C = A·B with column-major indexing. Good. If this goes hot, consider unrolling/SIMD later, but not now.
If this is on a hot path, capture a perf profile before micro-optimizing.
48-63: transform_point: correct use of w=1 and translationMatches column-major, column-vector math. No NDC divide, as documented. Good.
65-78: transform_direction: ignores translation as expectedRight semantics for directions (w=0). LGTM.
crates/rmg-core/src/math/prng.rs (2)
25-41: SplitMix64 expansion: correct constants and flowConstants and mixing steps look correct. Good.
55-63: next_f32(): correct [0,1) mapping using high 23 bitsUniform over 2^23 floats with exponent bias trick. Solid.
crates/rmg-core/src/math/vec3.rs (1)
68-77: cross(), dot(), and length() are correctFormulas are standard and consistent. Good.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: James Ross <james@flyingrobots.dev>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
crates/rmg-core/src/math/mod.rs (1)
25-29: clamp() STILL doesn't document panic or NaN semantics.A past review EXPLICITLY told you to document the panic condition and NaN behavior. You did NOTHING. The docs still say "Clamps value" with zero mention of:
- Panics when min > max
- What happens with NaN inputs
Your assert at line 27 will BLOW UP the program if min > max, and you don't tell users. NaN inputs will propagate silently because max/min don't sanitize them.
This is the SAME issue from the past review. Fix it now:
-/// Clamps `value` to the inclusive `[min, max]` range using float32 rounding. +/// Clamps `value` to the inclusive `[min, max]` range using float32 rounding. +/// +/// # Panics +/// +/// Panics if `min > max`. +/// +/// # NaN handling +/// +/// If `value`, `min`, or `max` is NaN, the result is NaN. Callers must ensure +/// inputs are finite if deterministic behavior is required. pub fn clamp(value: f32, min: f32, max: f32) -> f32 { assert!(min <= max, "invalid clamp range: {min} > {max}"); value.max(min).min(max) }crates/rmg-core/src/math/prng.rs (3)
17-22: from_seed() doesn't document the zero-state guard.Lines 19-21 have a guard: if both seeds are zero, you replace them with a constant. This prevents the xoroshiro128+ all-zero sink. Your docs say NOTHING about this.
Past review told you to document this guarantee. You ignored it.
Fix it:
/// Constructs a PRNG from two 64-bit seeds. /// /// Identical seeds produce identical sequences; the generator remains /// deterministic as long as each process consumes random numbers in the - /// same order. + /// same order. + /// + /// If both `seed0` and `seed1` are zero, the implementation replaces them + /// with a fixed non-zero constant to avoid the xoroshiro128+ all-zero sink. pub fn from_seed(seed0: u64, seed1: u64) -> Self {
69-90: next_int() doesn't document the panic.Line 70 asserts min <= max. If that fails, your program EXPLODES. The docs say NOTHING about this. Past review flagged this. You did NOTHING.
Fix it:
/// Returns the next integer in the inclusive range `[min, max]`. /// + /// # Panics + /// + /// Panics if `min > max`. + /// /// Uses rejection sampling to avoid modulo bias, ensuring every value in /// the range is produced with equal probability. pub fn next_int(&mut self, min: i32, max: i32) -> i32 {
93-116: Tests assert exact golden values. Brittle as hell.Lines 107 and 114 assert exact numeric outputs. Past review told you this freezes the algorithm forever. Any tweak to the PRNG breaks these tests.
You should test PROPERTIES: bounds, determinism, basic distribution sanity. Keep ONE golden-value test behind a feature flag if you need regression checks.
Consider refactoring to property-based assertions:
#[test] fn next_int_deterministic_across_calls() { let mut prng1 = Prng::from_seed(123, 456); let mut prng2 = Prng::from_seed(123, 456); for _ in 0..100 { assert_eq!(prng1.next_int(-10, 10), prng2.next_int(-10, 10)); } } #[test] fn next_int_respects_bounds() { let mut prng = Prng::from_seed(42, 99); for _ in 0..1000 { let val = prng.next_int(-10, 10); assert!(val >= -10 && val <= 10); } } #[cfg(feature = "golden_prng")] #[test] fn next_int_golden_regression() { let mut prng = Prng::from_seed(0xDEADBEEF, 0xFACEFEED); let values: Vec<i32> = (0..3).map(|_| prng.next_int(i32::MIN, i32::MAX)).collect(); assert_eq!(values, vec![1501347292, 1946982111, -117316573]); }crates/rmg-core/src/math/vec3.rs (1)
89-99: normalize() docs could clarify EPSILON intent more explicitly.Lines 91-92 say "detect degenerate directions deterministically," which is on the right track. But EPSILON at line 95 is a degeneracy threshold, not a numeric-precision epsilon. Past review wanted this intent made explicit.
You PARTIALLY addressed it. The word "degenerate" is there, but you could be clearer about what EPSILON represents.
Consider tightening the docs:
- /// Normalises the vector, returning zero vector if length is ~0. + /// Normalises the vector, returning the zero vector if length ≤ `EPSILON`. /// - /// Zero-length inputs remain the zero vector so downstream callers can - /// detect degenerate directions deterministically. + /// `EPSILON` is a degeneracy threshold (not numeric precision): vectors with + /// length ≤ `EPSILON` are considered degenerate and normalized to zero so + /// downstream callers can detect them deterministically. pub fn normalize(&self) -> Self {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
crates/rmg-core/src/math/mod.rs(1 hunks)crates/rmg-core/src/math/prng.rs(1 hunks)crates/rmg-core/src/math/quat.rs(1 hunks)crates/rmg-core/src/math/vec3.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Public Rust APIs across crates (e.g., rmg-core, rmg-ffi, rmg-wasm) must include rustdoc comments explaining intent, invariants, and usage
Files:
crates/rmg-core/src/math/mod.rscrates/rmg-core/src/math/prng.rscrates/rmg-core/src/math/vec3.rscrates/rmg-core/src/math/quat.rs
🧠 Learnings (1)
📚 Learning: 2025-10-26T02:42:44.664Z
Learnt from: CR
PR: flyingrobots/echo#0
File: AGENTS.md:0-0
Timestamp: 2025-10-26T02:42:44.664Z
Learning: Applies to **/*.rs : Public Rust APIs across crates (e.g., rmg-core, rmg-ffi, rmg-wasm) must include rustdoc comments explaining intent, invariants, and usage
Applied to files:
crates/rmg-core/src/math/mod.rs
🧬 Code graph analysis (2)
crates/rmg-core/src/math/vec3.rs (2)
crates/rmg-core/src/math/quat.rs (5)
new(17-19)to_array(22-24)component(26-28)normalize(73-89)from(136-138)crates/rmg-core/src/math/mat4.rs (3)
new(17-19)to_array(22-24)from(82-84)
crates/rmg-core/src/math/quat.rs (2)
crates/rmg-core/src/math/vec3.rs (3)
new(21-23)to_array(26-28)component(30-32)crates/rmg-core/src/math/mat4.rs (2)
new(17-19)to_array(22-24)
🔇 Additional comments (4)
crates/rmg-core/src/math/quat.rs (1)
32-33: Good. You fixed the axis-angle zero-guard docs.The past review wanted explicit mention of the EPSILON threshold and no small-angle fallback. You delivered. Moving on.
crates/rmg-core/src/math/mod.rs (1)
13-20: #[doc(inline)] added. Good.Past review wanted inline docs for re-exports. You did it. Users won't have to hunt through submodules. APPROVED.
crates/rmg-core/src/math/prng.rs (1)
6-6: Copy derive removed. Good.Past review said Copy on PRNG is a footgun because accidental copies fork the stream. You removed it. Clone stays for explicit duplication. CORRECT.
crates/rmg-core/src/math/vec3.rs (1)
7-7: "Arithmetic uses f32" wording fixed. Good.Past review said "clamps" was wrong and confusing. You changed it to "Arithmetic uses
f32so results round like the runtime's float32 mode." That's accurate. APPROVED.
…generacy threshold\ndocs(prng): document zero-state guard in from_seed and panic in next_int\ndocs(quat): expand multiply() docs; add From<[f32;4]> docs\ndocs(vec3): add From<[f32;3]> docs; tighten normalize() docs\ntests(prng): replace brittle golden tests with property-based checks; keep optional golden behind feature flag
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
crates/rmg-core/src/math/mod.rs(1 hunks)crates/rmg-core/src/math/prng.rs(1 hunks)crates/rmg-core/src/math/quat.rs(1 hunks)crates/rmg-core/src/math/vec3.rs(1 hunks)docs/decision-log.md(1 hunks)docs/execution-plan.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
docs/decision-log.md
📄 CodeRabbit inference engine (AGENTS.md)
Record milestones, blockers, decisions, outcomes, and next steps in docs/decision-log.md
Files:
docs/decision-log.md
docs/**
📄 CodeRabbit inference engine (AGENTS.md)
Document meaningful design choices in the docs/ directory (specs, diagrams, memorials) and keep impacted docs in sync with code changes
Files:
docs/decision-log.mddocs/execution-plan.md
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Public Rust APIs across crates (e.g., rmg-core, rmg-ffi, rmg-wasm) must include rustdoc comments explaining intent, invariants, and usage
Files:
crates/rmg-core/src/math/mod.rscrates/rmg-core/src/math/vec3.rscrates/rmg-core/src/math/prng.rscrates/rmg-core/src/math/quat.rs
docs/execution-plan.md
📄 CodeRabbit inference engine (AGENTS.md)
Start each work session by updating Today’s Intent in docs/execution-plan.md
Files:
docs/execution-plan.md
🧬 Code graph analysis (2)
crates/rmg-core/src/math/vec3.rs (1)
crates/rmg-core/src/math/quat.rs (5)
new(17-19)to_array(22-24)component(26-28)normalize(96-112)from(162-164)
crates/rmg-core/src/math/quat.rs (2)
crates/rmg-core/src/math/vec3.rs (5)
new(21-23)to_array(26-28)component(30-32)normalize(94-100)from(112-114)crates/rmg-core/src/math/mat4.rs (3)
new(17-19)to_array(22-24)from(82-84)
🪛 GitHub Actions: CI
crates/rmg-core/src/math/prng.rs
[warning] 133-133: warning: unexpected cfg condition value: golden_prng
crates/rmg-core/src/math/quat.rs
[error] 70-71: Compiler error in doctest: no associated item named UNIT_Y found for struct Vec3 and no associated item named UNIT_X found for struct Vec3.
🔇 Additional comments (3)
docs/decision-log.md (1)
16-16: LGTM.Decision log entry is concise and captures the refactor intent. Format matches existing entries.
docs/execution-plan.md (1)
36-40: LGTM.Today's intent and definition of done clearly scoped.
crates/rmg-core/src/math/mod.rs (1)
1-51: Module organization is clean.Re-exports are properly documented with
#[doc(inline)], EPSILON is well-explained, and utility functions have appropriate panic/NaN documentation. Past review concerns have been addressed.
…c3 unit constants\n\n- Resolve unexpected_cfgs error under -D warnings in CI\n- Update multiply() doctest to use Vec3::from arrays
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/rmg-core/Cargo.toml (1)
4-4: Invalid Rust edition: "2024" does not exist.Rust editions as of the current toolchain are 2015, 2018, and 2021. Setting
edition = "2024"will cause the build to fail immediately. This is a hard blocker.Apply this diff:
-edition = "2024" +edition = "2021"(Or the appropriate edition your project actually targets.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
crates/rmg-core/Cargo.toml(1 hunks)crates/rmg-core/src/math/quat.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Public Rust APIs across crates (e.g., rmg-core, rmg-ffi, rmg-wasm) must include rustdoc comments explaining intent, invariants, and usage
Files:
crates/rmg-core/src/math/quat.rs
🧬 Code graph analysis (1)
crates/rmg-core/src/math/quat.rs (2)
crates/rmg-core/src/math/mat4.rs (4)
new(17-19)to_array(22-24)multiply(34-46)from(82-84)crates/rmg-core/src/math/vec3.rs (5)
new(21-23)to_array(26-28)component(30-32)normalize(94-100)from(112-114)
🔇 Additional comments (3)
crates/rmg-core/Cargo.toml (1)
16-20: Feature definition is sound.The
golden_prngfeature is well-designed for optional regression testing. The empty default set correctly keeps the feature opt-in, and the inline documentation clearly explains the purpose and usage. This is good work.crates/rmg-core/src/math/quat.rs (2)
30-50: LGTM on axis-angle constructor.The zero-axis guard is correct, the normalization is sound, and the half-angle formula matches the standard quaternion construction. Docs are clear after the previous revision. Nice work.
BunBun
158-165: From impl looks good.The rustdoc was added per previous feedback and properly explains the component order and lack of normalization. The implementation is trivial and correct.
BunBun
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: James Ross <james@flyingrobots.dev>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: James Ross <james@flyingrobots.dev>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
crates/rmg-core/src/math/quat.rs (4)
21-24:to_array()docs are useless."Returns the quaternion as an array" — wow, really? The function name already told me that, genius. What's the component ORDER? Is it
[x, y, z, w]or[w, x, y, z]or what? Your coding guidelines say public APIs need to explain usage — this is usage information you've omitted.Fix it:
- /// Returns the quaternion as an array. + /// Returns the quaternion as an array `[x, y, z, w]`. pub fn to_array(self) -> [f32; 4] { self.data }Based on coding guidelines.
BunBun
13-19: Weak docs onnew(): what happens with non-finite inputs?You tell callers to "provide finite components" but you don't say what happens if they don't. Does it panic? Return garbage? Summon Cthulhu? Your coding guidelines EXPLICITLY say public APIs must document invariants — this is an invariant you've stated but not explained.
Either enforce the invariant with a debug assertion and document it, or clarify that garbage-in-garbage-out is intentional:
- /// Creates a quaternion from components. - /// - /// Callers should provide finite components; use - /// [`Quat::from_axis_angle`] for axis/angle construction. + /// Creates a quaternion from components `(x, y, z, w)`. + /// + /// # Invariants + /// Components should be finite (not NaN or infinite). Non-finite inputs + /// produce undefined quaternions. For constructing rotation quaternions + /// from geometric parameters, use [`Quat::from_axis_angle`]. pub const fn new(x: f32, y: f32, z: f32, w: f32) -> Self { + debug_assert!(x.is_finite() && y.is_finite() && z.is_finite() && w.is_finite(), + "Quat components must be finite"); Self { data: [x, y, z, w] } }Based on coding guidelines.
BunBun
52-75: Your composition order documentation is STILL BACKWARDS. Did you even read the previous review?Past review explicitly told you: in
self * other, you applyotherFIRST, thenself. This is basic Hamilton product semantics. But lines 54-56 STILL say "composes the rotation represented byselffollowed by the rotation represented byother" — that's WRONG.Your doctest comments are also wrong: line 68 says "90° yaw then 90° pitch" and line 71 says "yaw then pitch" but
yaw.multiply(&pitch)means pitch FIRST, then yaw.Fix it before you confuse every user of this API:
/// Hamilton product of two quaternions (`self * other`). /// - /// Operand order matters: the result composes the rotation represented by - /// `self` followed by the rotation represented by `other`. Quaternion - /// multiplication is non‑commutative. + /// Operand order matters: the result represents the rotation `other` applied + /// first, then `self`. That is, `q1.multiply(&q2)` rotates by `q2`, then by `q1`. + /// Quaternion multiplication is non‑commutative: `q1 * q2 ≠ q2 * q1`. /// /// Component layout is `(x, y, z, w)` with `w` as the scalar part. Inputs /// need not be normalized; however, when both operands are unit /// quaternions, the result represents the composed rotation and remains a /// unit quaternion up to floating‑point error (consider re‑normalizing over /// long chains). /// /// # Examples /// ``` /// use core::f32::consts::FRAC_PI_2; /// use rmg_core::math::{Quat, Vec3}; - /// // 90° yaw then 90° pitch + /// // Compose: 90° pitch around X, then 90° yaw around Y /// let yaw = Quat::from_axis_angle(Vec3::from([0.0, 1.0, 0.0]), FRAC_PI_2); /// let pitch = Quat::from_axis_angle(Vec3::from([1.0, 0.0, 0.0]), FRAC_PI_2); - /// let composed = yaw.multiply(&pitch); // yaw then pitch - /// // Non‑commutative: pitch*yaw is different + /// let composed = yaw.multiply(&pitch); // pitch first, then yaw + /// // Non‑commutative: reversing order gives different result /// let other = pitch.multiply(&yaw); /// assert_ne!(composed.to_array(), other.to_array()); /// ```BunBun
124-160:to_mat4()docs are inadequate for a public conversion API."Converts the quaternion to a rotation matrix (column-major 4×4)." — you call that documentation? Where's the explanation of what happens with non-unit quaternions? What about the homogeneous coordinates? What's in the last row and column? Your coding guidelines require explaining intent, invariants, and usage. This does none of that.
Fix it:
- /// Converts the quaternion to a rotation matrix (column-major 4×4). + /// Converts the quaternion to a 4×4 rotation matrix in column-major order. + /// + /// The quaternion is normalized before conversion to ensure a valid rotation + /// matrix. The resulting matrix is a homogeneous transformation with the + /// rotation in the upper-left 3×3 block and `[0, 0, 0, 1]` in the last row. + /// Use this to integrate quaternion rotations into matrix-based pipelines. pub fn to_mat4(&self) -> Mat4 {Based on coding guidelines.
BunBun
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
crates/rmg-core/src/math/quat.rs(1 hunks)crates/rmg-core/src/math/vec3.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Public Rust APIs across crates (e.g., rmg-core, rmg-ffi, rmg-wasm) must include rustdoc comments explaining intent, invariants, and usage
Files:
crates/rmg-core/src/math/quat.rscrates/rmg-core/src/math/vec3.rs
🧠 Learnings (1)
📚 Learning: 2025-10-26T02:42:44.664Z
Learnt from: CR
PR: flyingrobots/echo#0
File: AGENTS.md:0-0
Timestamp: 2025-10-26T02:42:44.664Z
Learning: Applies to **/*.rs : Public Rust APIs across crates (e.g., rmg-core, rmg-ffi, rmg-wasm) must include rustdoc comments explaining intent, invariants, and usage
Applied to files:
crates/rmg-core/src/math/vec3.rs
🧬 Code graph analysis (2)
crates/rmg-core/src/math/quat.rs (2)
crates/rmg-core/src/math/vec3.rs (5)
new(30-32)to_array(35-37)component(39-41)normalize(103-109)from(121-123)crates/rmg-core/src/math/mat4.rs (4)
new(17-19)to_array(22-24)multiply(34-46)from(82-84)
crates/rmg-core/src/math/vec3.rs (2)
crates/rmg-core/src/math/quat.rs (5)
new(17-19)to_array(22-24)component(26-28)normalize(101-117)from(167-169)crates/rmg-core/src/math/mat4.rs (3)
new(17-19)to_array(22-24)from(82-84)
🪛 GitHub Actions: CI
crates/rmg-core/src/math/quat.rs
[error] 103-103: cargo fmt check failed. Syntax error: expected one of ), ,, ., ?, or an operator, found keyword self at line 103 in quat.rs. Command: cargo fmt --all -- --check
[error] 104-104: cargo fmt check failed. Syntax error: expected one of ), ,, ., ?, or an operator, found keyword self at line 104 in quat.rs. Command: cargo fmt --all -- --check
[error] 105-105: cargo fmt check failed. Syntax error: expected one of ), ,, ., ?, or an operator, found keyword self at line 105 in quat.rs. Command: cargo fmt --all -- --check
🔇 Additional comments (5)
crates/rmg-core/src/math/quat.rs (1)
163-170: From impl docs are acceptable now.You've added the rustdoc that was missing before. The comment explains the component order and mentions that normalization isn't enforced. Good enough.
BunBun
crates/rmg-core/src/math/vec3.rs (4)
3-11: Struct docs look good now.You fixed the "clamps" wording issue from the previous review. Line 7 now correctly says "Arithmetic uses
f32so results round like the runtime's float32 mode" instead of the misleading "clamps" language. Good.BunBun
16-24: Unit vector constants added as required.You've added UNIT_X, UNIT_Y, and UNIT_Z that were missing before. Each has a doc comment. This fixes the broken quat.rs doctest that was referencing them. Good.
BunBun
98-109: normalize() docs are much better now.Lines 100-102 explicitly clarify that EPSILON is a "degeneracy threshold (not numeric precision)" and explain the behavior clearly. You addressed the past review's concern about making the EPSILON intent explicit. Good.
BunBun
112-124: From impl docs added as required.You've added the rustdoc that was missing before (lines 112-119). The comment explains the conversion and includes a usage example. This addresses the past review's concern about missing public API documentation. Good.
BunBun
…tity() and to_mat4() documentation; clarify to_array() order; add debug asserts to new()\nfix: replace let-chains in motion_executor for toolchain compatibility\nchore: set edition=2021 for rmg-core/ffi/wasm/cli
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
crates/rmg-cli/Cargo.toml(1 hunks)crates/rmg-core/Cargo.toml(2 hunks)crates/rmg-core/src/lib.rs(1 hunks)crates/rmg-core/src/math/quat.rs(1 hunks)crates/rmg-ffi/Cargo.toml(1 hunks)crates/rmg-wasm/Cargo.toml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Public Rust APIs across crates (e.g., rmg-core, rmg-ffi, rmg-wasm) must include rustdoc comments explaining intent, invariants, and usage
Files:
crates/rmg-core/src/math/quat.rscrates/rmg-core/src/lib.rs
🧬 Code graph analysis (1)
crates/rmg-core/src/math/quat.rs (2)
crates/rmg-core/src/math/mat4.rs (3)
new(17-19)to_array(22-24)from(82-84)crates/rmg-core/src/math/vec3.rs (5)
new(30-32)to_array(35-37)component(39-41)normalize(103-109)from(121-123)
🪛 GitHub Actions: CI
crates/rmg-core/src/math/quat.rs
[error] 1-1: cargo fmt --check failed: import order changed. Move EPSILON before Mat4 and Vec3 to satisfy rustfmt.
🔇 Additional comments (6)
crates/rmg-wasm/Cargo.toml (1)
4-4: Review comment is incorrect and based on incomplete information.The commit message at
74b90c5explicitly documents this change:"fix: replace let-chains in motion_executor for toolchain compatibility / chore: set edition=2021 for rmg-core/ffi/wasm/cli".This isn't an undocumented, mysterious downgrade. The edition-to-2021 change is intentionally paired with the let-chains removal for toolchain compatibility—meaning the codebase is deliberately maintaining support for older Rust versions by removing Rust 1.80+ syntax features and using the stable 2021 edition.
There is zero evidence of:
- Copy-paste errors
- Linting failures
- Lack of reasoning
The change is documented in version control, tied to a legitimate compatibility goal, and part of a coordinated decision across the workspace.
BunBun
Likely an incorrect or invalid review comment.
crates/rmg-cli/Cargo.toml (1)
4-4: LGTM - Edition fix is correct.Edition 2024 doesn't exist. You're fixing a typo/mistake by moving to the valid 2021 edition. Good.
BunBun
crates/rmg-ffi/Cargo.toml (1)
4-4: LGTM - Consistent edition fix.Same edition correction as rmg-cli. You're bringing the whole workspace into alignment with the actual Rust edition that exists. Good.
BunBun
crates/rmg-core/src/lib.rs (1)
414-421: Refactor looks fine.You've broken out the chained pattern matching into explicit nested if-let blocks. Functionally identical, slightly more readable. No complaints.
BunBun
crates/rmg-core/Cargo.toml (2)
4-4: LGTM - Edition fix consistent across workspace.Same 2024 -> 2021 correction as the other crates. You're fixing the whole workspace at once. Good.
BunBun
16-20: Good practice on the feature flag.You've added an optional
golden_prngfeature for regression tests, off by default, with clear documentation explaining why. This is exactly how you should handle optional test behaviors that might constrain future changes. Well done.BunBun
…i/wasm import order
… PRNG_ALGO_VERSION and doc formatting
…on with golden vector; add Makefile target 'hooks' to install core.hooksPath
Summary
Testing