Conversation
📝 WalkthroughWalkthroughRemoved external step parameters and consolidated step-size logic inside SolveStep; updated Solve to call the single SolveStep path unconditionally; refactored environmental computations to use typed temporaries; added documentation clarifying Builder::StepSize == 0 enables variable-step mode. (36 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Solve as Solve (caller)
participant SolveStep as SolveStep
participant Integrator as HeunStep / Trajectory
note right of SolveStep `#D6EAF8`: compute internal kStep = f(input.step_size, velocity)
Solve->>SolveStep: SolveStep(&s, &t, in)
SolveStep->>SolveStep: compute kStep (uses input.step_size, current velocity)
SolveStep->>Integrator: HeunStep(state, kStep)
Integrator-->>SolveStep: updated state/time
SolveStep-->>Solve: return updated state/time
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-04-15T13:11:22.842ZApplied to files:
📚 Learning: 2025-05-22T16:06:12.031ZApplied to files:
🧬 Code graph analysis (1)source/lob_builder.cpp (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
🔇 Additional comments (2)
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: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
include/lob/lob.hppsource/lob_builder.cppsource/lob_solve.cppsource/solve_step.cppsource/solve_step.hpp
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: joelbenway
Repo: joelbenway/lob PR: 22
File: source/lob_builder.cpp:520-552
Timestamp: 2025-05-22T16:06:12.031Z
Learning: The `SolveStep` function in `solve_step.hpp` has an overload with a default argument of `FeetT(1)` for the step parameter: `void SolveStep(TrajectoryStateT* ps, SecT* pt, const Input& input, FeetT step = FeetT(1))`
📚 Learning: 2025-06-17T12:55:26.522Z
Learnt from: joelbenway
Repo: joelbenway/lob PR: 25
File: include/lob/lob.hpp:123-130
Timestamp: 2025-06-17T12:55:26.522Z
Learning: In the lob ballistics library, when Input::step_size is set to 0 (the default), the solver automatically sets an appropriate step size internally rather than failing with an assertion error.
Applied to files:
include/lob/lob.hppsource/solve_step.cppsource/lob_solve.cppsource/lob_builder.cppsource/solve_step.hpp
📚 Learning: 2025-05-22T16:06:12.031Z
Learnt from: joelbenway
Repo: joelbenway/lob PR: 22
File: source/lob_builder.cpp:520-552
Timestamp: 2025-05-22T16:06:12.031Z
Learning: The `SolveStep` function in `solve_step.hpp` has an overload with a default argument of `FeetT(1)` for the step parameter: `void SolveStep(TrajectoryStateT* ps, SecT* pt, const Input& input, FeetT step = FeetT(1))`
Applied to files:
source/solve_step.cppsource/lob_solve.cppsource/lob_builder.cppsource/solve_step.hpp
📚 Learning: 2025-05-22T16:11:54.845Z
Learnt from: joelbenway
Repo: joelbenway/lob PR: 22
File: source/ode.hpp:66-81
Timestamp: 2025-05-22T16:11:54.845Z
Learning: The operator+ and operator* overloads in TrajectoryStateT that take a SecT parameter apply the time scalar to both position and velocity components. While this might appear to break dimensional correctness from a physics perspective, it's an intentional design for the numerical integration methods used in the codebase and is well unit tested.
Applied to files:
source/solve_step.cppsource/lob_solve.cppsource/solve_step.hpp
📚 Learning: 2025-04-15T13:11:22.842Z
Learnt from: joelbenway
Repo: joelbenway/lob PR: 15
File: source/calc.hpp:52-52
Timestamp: 2025-04-15T13:11:22.842Z
Learning: In lob, the `CalculateProjectileReferenceArea` function returns area in square inches (SqInT), but in some calculations like those in test/source/calc_test.cpp, this value is intentionally converted to square feet (SqFtT) when multiplied by air density in pounds per cubic foot (LbsPerCuFtT) to ensure proper unit cancellation.
Applied to files:
source/lob_builder.cpp
📚 Learning: 2025-05-22T16:12:16.630Z
Learnt from: joelbenway
Repo: joelbenway/lob PR: 22
File: test/source/boatright_test.cpp:58-65
Timestamp: 2025-05-22T16:12:16.630Z
Learning: In the lob codebase, std::pow and potentially other standard library functions have been overloaded to work directly with strong types (like InchT, CaliberT, etc.), allowing expressions like std::pow(kD / 2, 2) to compile correctly without needing to extract the underlying value first.
Applied to files:
source/lob_builder.cpp
🧬 Code graph analysis (3)
source/solve_step.cpp (1)
source/ode.hpp (2)
HeunStep(20-25)HeunStep(20-20)
source/lob_solve.cpp (2)
source/solve_step.cpp (2)
SolveStep(18-58)SolveStep(18-18)source/solve_step.hpp (1)
SolveStep(13-13)
source/lob_builder.cpp (2)
source/calc.hpp (10)
CalculateWaterVaporSaturationPressure(74-89)CalculateWaterVaporSaturationPressure(74-74)CalculateAirDensityRatio(92-95)CalculateAirDensityRatio(92-92)CalculateAirDensityRatioHumidityCorrection(98-104)CalculateAirDensityRatioHumidityCorrection(98-99)CalculateSpeedOfSoundInAir(67-70)CalculateSpeedOfSoundInAir(67-67)CalculateSpeedOfSoundHumidityCorrection(107-113)CalculateSpeedOfSoundHumidityCorrection(107-108)source/eng_units.hpp (14)
LbsPerCuFtT(462-466)LbsPerCuFtT(462-462)FpsT(768-770)FpsT(768-768)FpsT(786-788)FpsT(786-786)FpsT(792-794)FpsT(792-792)FpsT(798-800)FpsT(798-798)sin(192-194)sin(192-192)sin(937-939)sin(937-937)
🪛 GitHub Actions: Continuous Integration
source/solve_step.cpp
[error] 1-1: The following files are badly formatted: source/solve_step.cpp. Run again with FIX=YES to fix these files.
🔇 Additional comments (7)
source/solve_step.hpp (1)
13-13: LGTM! API simplification aligns with the PR objective.The removal of the explicit step parameter overloads consolidates step-sizing logic within
SolveStepitself. Based on learnings, this replaces the previous overload with defaultFeetT(1)step parameter.include/lob/lob.hpp (1)
433-433: Good documentation addition.This clarifies the behavior when
step_sizeis set to zero, which aligns with the existing behavior noted in previous learnings.source/lob_builder.cpp (2)
421-438: LGTM! Clean refactoring with explicit typed constants.The introduction of named constants
kWaterVaporSaturationPressureInHg,kAirDensity, andkSpeedOfSoundwith explicit types improves readability and type safety without changing functionality.
766-785: LGTM! Well-structured zero-angle computation with proper step size handling.The refactoring introduces:
- Named temporaries (
kAngle,kVelocity) for clarity- Save/restore pattern for
step_sizethat forces variable stepping during the binary search while preserving the user's selected step size for the finalSolvecallThis ensures the zero-angle calibration uses optimal stepping internally without affecting the user's configuration.
source/lob_solve.cpp (1)
89-89: LGTM! Simplified call site aligns with the refactored API.The
SolveStepcall now relies on internal step computation, removing the conditional branching that previously existed at this call site. This centralizes step-sizing logic withinSolveStepitself.source/solve_step.cpp (2)
56-57: LGTM! Internal step usage is correct.The
HeunStepinvocation now uses the internally computedkStep, and the time accumulation is consistent. This consolidates the step-sizing logic as intended by the refactor.
22-24: Edge case is handled by calling loop's exit conditions, not SolveStep itself.When
step_size == 0andV().X() <= 0,kStepbecomes zero. However, the solver loop inlob_solve.cppexits before this occurs: the loop terminates when velocity drops belowminimum_speed(lines 109-116), and when vertical velocity exceeds horizontal by more than 3x (line 118). This meansV().X()will never reach zero or negative values during normal execution.While SolveStep doesn't explicitly guard this edge case, it's implicitly protected by the caller's loop logic. The code works as designed when
step_size == 0uses adaptive stepping based on velocity, but relies on the calling context to prevent the pathological case. No change needed unless defensive programming inside SolveStep is desired.
| const SecT kStep = input.step_size == 0 && ps->V().X() > FpsT(0) | ||
| ? SecT(ps->V().X().Inverse().Value()) | ||
| : SecT(UsecT(input.step_size)); | ||
|
|
There was a problem hiding this comment.
Fix formatting issue flagged by CI.
The pipeline failure indicates a formatting issue in this file. The trailing whitespace on line 25 needs to be removed.
🔎 Proposed fix
const SecT kStep = input.step_size == 0 && ps->V().X() > FpsT(0)
? SecT(ps->V().X().Inverse().Value())
: SecT(UsecT(input.step_size));
-
+
const CartesianT<FpsT> kWind(FpsT(input.wind.x), FpsT(0.0),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const SecT kStep = input.step_size == 0 && ps->V().X() > FpsT(0) | |
| ? SecT(ps->V().X().Inverse().Value()) | |
| : SecT(UsecT(input.step_size)); | |
| const SecT kStep = input.step_size == 0 && ps->V().X() > FpsT(0) | |
| ? SecT(ps->V().X().Inverse().Value()) | |
| : SecT(UsecT(input.step_size)); | |
🤖 Prompt for AI Agents
In source/solve_step.cpp around lines 22 to 25, there is trailing whitespace at
the end of line 25 causing the CI formatting failure; remove the trailing
space(s) from that line so the line ends immediately after the semicolon and
re-run the formatter/checks.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
@coderabbitai what is the issue? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3040 @joelbenway The Codecov report indicates that 2 lines in const auto kSavedStepSize = pimpl->build.step_size;
pimpl->build.step_size = 0U; // Use variable step size ⚠️ Missing coverage
// ... SolveStep loop ...
pimpl->build.step_size = kSavedStepSize; // Restore selected step size ⚠️ Missing coverageWhy these lines lack coverage:
Recommendation:
The coverage issue doesn't indicate a functional problem—it simply means these specific code paths aren't being exercised by your current test suite. |
1 participant
Summary by CodeRabbit
Documentation
Refactor
Breaking Change
✏️ Tip: You can customize this high-level summary in your review settings.