feat: add constant-local-orbital-frame-direction Maneuvering Segments #591
Conversation
phc1990
changed the title
WalkthroughAdds LOF (Local Orbital Frame) maneuver analysis and conversion to constant-LOF-direction maneuvers, a HeterogeneousGuidanceLaw to sequence guidance laws over intervals, a ConstantThrust.from_maneuver factory, Segment support for constant-LOF-direction maneuvers and dynamic substitution with HeterogeneousGuidanceLaw, numerous C++/Python bindings, and expanded tests across modules. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Segment
participant Extract as ManeuverExtractor
participant Maneuver
participant LOF as LocalOrbitalFrameFactory
participant ConstThrust
participant HGuidance as HeterogeneousGuidanceLaw
participant Propagator
User->>Segment: ConstantLocalOrbitalFrameDirectionManeuver(...)
Segment->>Maneuver: toConstantLocalOrbitalFrameDirectionManeuver(LOF, max_offset?)
Maneuver->>LOF: transform thrust accelerations -> LOF
Maneuver-->>Maneuver: compute mean LOF direction & max angular offset
Maneuver->>Segment: return constant-direction Maneuver
Segment->>Extract: extract maneuvers from solution
Extract->>ConstThrust: ConstantThrust::FromManeuver(maneuver, LOF, max_offset?)
ConstThrust-->>Segment: ConstantThrust guidance law
Segment->>HGuidance: assemble HeterogeneousGuidanceLaw(constant laws)
Segment->>Propagator: Solve_(dynamics_with_HGuidance)
Propagator->>HGuidance: calculateThrustAccelerationAt(instant,...)
HGuidance->>ConstThrust: delegate to active guidance law
ConstThrust-->>Propagator: thrust vector
Propagator-->>Segment: propagation result
Segment-->>User: final solution
sequenceDiagram
participant HeterogeneousGuidanceLaw
participant GL1 as GuidanceLaw1
participant GL2 as GuidanceLaw2
participant Interval1
participant Interval2
HeterogeneousGuidanceLaw->>HeterogeneousGuidanceLaw: addGuidanceLaw(GL1, Interval1)
Note over HeterogeneousGuidanceLaw: store GL1@[Interval1]
HeterogeneousGuidanceLaw->>HeterogeneousGuidanceLaw: addGuidanceLaw(GL2, Interval2)
Note over HeterogeneousGuidanceLaw: validate Interval2 non-overlap
HeterogeneousGuidanceLaw->>HeterogeneousGuidanceLaw: calculateThrustAccelerationAt(instant)
alt instant in Interval1
HeterogeneousGuidanceLaw->>GL1: calculateThrustAccelerationAt(instant)
GL1-->>HeterogeneousGuidanceLaw: vector
else instant in Interval2
HeterogeneousGuidanceLaw->>GL2: calculateThrustAccelerationAt(instant)
GL2-->>HeterogeneousGuidanceLaw: vector
else
HeterogeneousGuidanceLaw-->>HeterogeneousGuidanceLaw: return zero vector
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
![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: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Maneuver.cpp(1 hunks)bindings/python/test/flight/test_maneuver.py(2 hunks)include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp(3 hunks)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp(4 hunks)test/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.test.cpp(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Maneuver.cpp (1)
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (2)
toConstantLocalOrbitalFrameDirectionManeuver(274-323)toConstantLocalOrbitalFrameDirectionManeuver(274-276)
bindings/python/test/flight/test_maneuver.py (3)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.hpp (1)
LocalOrbitalFrameFactory(144-148)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (3)
LocalOrbitalFrameFactory(166-175)TNW(128-131)TNW(128-128)bindings/python/test/trajectory/test_local_orbital_frame_factory.py (1)
local_orbital_frame_factory(36-37)
test/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.test.cpp (2)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Propagator.test.cpp (2)
anInstant(132-141)anInstant(132-134)src/OpenSpaceToolkit/Astrodynamics/Trajectory/State/CoordinateSubset/CartesianAcceleration.cpp (2)
ThrustAcceleration(62-69)ThrustAcceleration(62-62)
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/State/CoordinateSubset/CartesianAcceleration.cpp (2)
ThrustAcceleration(62-69)ThrustAcceleration(62-62)
⏰ 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). (1)
- GitHub Check: Build Development Image / Merge Image Manifests
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #591 +/- ##
==========================================
+ Coverage 91.83% 91.88% +0.05%
==========================================
Files 109 110 +1
Lines 10374 10517 +143
==========================================
+ Hits 9527 9664 +137
- Misses 847 853 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
phc1990
force-pushed
the
feat/solve-sequences-with-constant-attitude-bias-maneuvers
branch
from
afb46bb to
5e83dea
Compare
phc1990
changed the title
phc1990
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (1)
553-567: Optional: preserve dynamics ordering and avoid duplicate tabulated insertionsIf newManeuvers overlap or if solve is re-entered, consider guarding against duplicate tabulated dynamics and preserving relative order of non-thruster dynamics for determinism.
- Track and skip already-inserted tabulated dynamics (by pointer address or equality).
- Keep original dynamics order intact around the replaced thruster.
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (2)
310-311: Return type constness of shared_ptr is unusualReturning const Shared constrains the pointer, not the pointee, and is atypical. Prefer returning Shared or Shared.
Change signature to one of:
- static Shared identifyThrusterDynamics(...);
- static Shared identifyThrusterDynamics(...);
Ensure implementation and call sites are updated accordingly.
269-286: Docs: fix typo and clarifyMinor typos in public API doc.
- “trasnformed” → “transformed”.
- Consider: “Create a maneuvering segment where raw maneuvers are transformed into constant Local Orbital Frame direction maneuvers during solving.”
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Maneuver.cpp (1)
133-150: Binding looks correct; minor doc nit optionalAPI and argument mapping are correct. Optional: “Local Orbital Frame” capitalization is consistent; consider shortening the doc slightly for brevity.
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp (2)
424-447: Binding is correct; fix typos in docstringLooks good. Please fix:
- “trasnformed” → “transformed”
- Optionally, clarify that transformation occurs during solve.
275-283: Optional: fix minor doc typo“assocated” → “associated”.
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
3-4: Duplicate license header line (cosmetic)Two consecutive “Apache License 2.0” lines.
-/// Apache License 2.0 - /// Apache License 2.0bindings/python/test/trajectory/test_segment.py (1)
450-457: Strengthen the Python solve assertion.Right now we only assert the solve returns something. Consider also checking that a maneuver is extracted (and maybe that its frame-direction has been normalized) so the binding can’t regress silently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Maneuver.cpp(1 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp(2 hunks)bindings/python/test/flight/test_maneuver.py(2 hunks)bindings/python/test/trajectory/test_segment.py(6 hunks)include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp(3 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp(4 hunks)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp(4 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp(6 hunks)test/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.test.cpp(3 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bindings/python/test/flight/test_maneuver.py
🧰 Additional context used
🧬 Code graph analysis (7)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp (1)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (2)
ConstantLocalOrbitalFrameDirectionManeuver(661-678)ConstantLocalOrbitalFrameDirectionManeuver(661-668)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Maneuver.cpp (1)
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (2)
toConstantLocalOrbitalFrameDirectionManeuver(274-330)toConstantLocalOrbitalFrameDirectionManeuver(274-276)
test/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.test.cpp (2)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Propagator.test.cpp (2)
anInstant(132-141)anInstant(132-134)src/OpenSpaceToolkit/Astrodynamics/Trajectory/State/CoordinateSubset/CartesianAcceleration.cpp (2)
ThrustAcceleration(62-69)ThrustAcceleration(62-62)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (2)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.hpp (2)
LocalOrbitalFrameFactory(144-148)aType(116-118)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameDirection.cpp (2)
Undefined(100-103)Undefined(100-100)
bindings/python/test/trajectory/test_segment.py (4)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (3)
LocalOrbitalFrameFactory(166-175)VNC(133-136)VNC(133-133)bindings/python/test/trajectory/test_local_orbital_frame_factory.py (2)
local_orbital_frame_factory(36-37)state(58-63)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (2)
Segment(295-303)Solution(77-83)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (4)
Segment(423-452)Solution(35-48)solve(501-597)solve(501-501)
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (2)
include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (2)
aLocalOrbitalFrameFactorySPtr(197-199)Maneuver(75-75)src/OpenSpaceToolkit/Astrodynamics/Trajectory/State/CoordinateSubset/CartesianAcceleration.cpp (2)
ThrustAcceleration(62-69)ThrustAcceleration(62-62)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp (6)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Propagator.cpp (3)
Propagator(49-54)Propagator(56-92)Propagator(94-99)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Propagator.hpp (3)
Propagator(63-66)Propagator(78-83)Propagator(88-88)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (2)
VNC(133-136)VNC(133-133)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameDirection.cpp (1)
LocalOrbitalFrameDirection(18-28)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (4)
Maneuver(644-659)Maneuver(644-650)ConstantLocalOrbitalFrameDirectionManeuver(661-678)ConstantLocalOrbitalFrameDirectionManeuver(661-668)
⏰ 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). (1)
- GitHub Check: Build Development Image / Merge Image Manifests
🔇 Additional comments (4)
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
274-330: LOF-direction maneuver conversion logic LGTMValidation, frame generation, accumulation, zero-norm guard, and state rebuild are correct and robust.
include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (1)
181-199: Clear documentation for LOF conversion.The comment nails the behavior—constant LOF direction while preserving the original profile—making the API intent obvious.
test/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.test.cpp (1)
542-636: Excellent LOF direction regression coverage.Validating the transformed thrust vectors in the LOF at multiple instants gives strong confidence the mean-direction logic holds.
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp (1)
1026-1093: Great cross-validation against Propagator.Comparing the constant-LOF segment outcome to both a reconstructed Propagator and on-demand state calculation is a solid guardrail for future changes.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (2)
129-133: Critical: Cache is still not frame-aware.The past review comment correctly identified that the cache ignores the
aFrameSPtrargument. Returning cached maneuvers built in one frame for subsequent calls with another frame is incorrect and will produce invalid results.Apply the fix from the previous review:
- if (this->maneuversAreCached_) + if (this->maneuversAreCached_ && (this->maneuversCachedFrame_ != nullptr) && + (this->maneuversCachedFrame_.get() == aFrameSPtr.get())) { return this->maneuvers_; }And after computing maneuvers at Line 230:
this->maneuvers_ = extractedManeuvers; this->maneuversAreCached_ = true; + this->maneuversCachedFrame_ = aFrameSPtr;Also add
mutable Shared<const Frame> maneuversCachedFrame_ = nullptr;to theSegment::Solutionclass in the header.
531-534: Critical: Null pointer dereference.The past review comment correctly identified that
constantManeuverDirectionLocalOrbitalFrameFactory_can be null, but Line 531 dereferences it without a null check, causing undefined behavior.Apply the fix from the previous review:
- if (!this->constantManeuverDirectionLocalOrbitalFrameFactory_->isDefined()) + if ((this->constantManeuverDirectionLocalOrbitalFrameFactory_ == nullptr) || + !this->constantManeuverDirectionLocalOrbitalFrameFactory_->isDefined()) { return rawSolution; }
🧹 Nitpick comments (3)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (3)
553-561: Redundant thruster identification.Line 554 calls
identifyThrusterDynamics(dynamics_), but this was already called duringextractManeuversat Line 144. Consider caching the thruster dynamics reference in the raw solution to avoid duplicate validation.Example approach:
// Store thruster when extracting maneuvers const Shared<Thruster> thrusterDynamics = identifyThrusterDynamics(this->dynamics); // ... later in solve method, reuse the reference
536-596: Consider extracting constant LOF transformation to a separate method.The solve method now contains ~65 lines of constant LOF direction transformation logic, with a happy-path exit in the middle (Line 533). As alex-liang3 noted, this affects readability.
Consider refactoring to:
Segment::Solution Segment::solve(const State& aState, const Duration& maximumPropagationDuration) const { const Segment::Solution rawSolution = solveRaw(aState, maximumPropagationDuration); if ((this->constantManeuverDirectionLocalOrbitalFrameFactory_ == nullptr) || !this->constantManeuverDirectionLocalOrbitalFrameFactory_->isDefined()) { return rawSolution; } return applyConstantLocalOrbitalFrameDirection(rawSolution, aState, maximumPropagationDuration); }This separates concerns: raw propagation, conditional check, and LOF transformation.
553-554: Consider reserving capacity for newDynamics array.As alex-liang3 noted, since there's exactly one thruster dynamics and you're replacing it with
newManeuvers.getSize()tabulated dynamics, you can reserve the appropriate capacity upfront to avoid reallocations.Array<Shared<Dynamics>> newDynamics = Array<Shared<Dynamics>>::Empty(); +newDynamics.reserve(dynamics_.getSize() - 1 + newManeuvers.getSize()); const Shared<Dynamics> thrusterDynamics = identifyThrusterDynamics(dynamics_);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (2)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Propagator.hpp (5)
aNumericalSolver(234-234)aDynamicsArray(147-147)aState(180-180)aState(191-193)aState(203-203)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (5)
aName(246-251)aName(261-267)aName(278-285)aDynamicsArray(310-310)aState(231-231)
⏰ 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). (4)
- GitHub Check: Test / Check Python Format
- GitHub Check: Test / Build Python bindings
- GitHub Check: Test / Run C++ Unit Tests
- GitHub Check: Test / Check C++ Format
🔇 Additional comments (4)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (4)
144-144: Good refactoring.Using the centralized
identifyThrusterDynamicshelper improves maintainability and consistency.
661-678: Factory implementation looks correct.The
ConstantLocalOrbitalFrameDirectionManeuverfactory follows the same pattern as the existingManeuverfactory and correctly passes theLocalOrbitalFrameFactoryto the constructor.
680-707: Multiple-thruster detection is now correct.The past review comment identified a broken detection logic where the inner check was unreachable. The current implementation has fixed this issue by removing the
&& !thrusterDynamicsFoundcondition from the outer check, making the multiple-thruster detection reachable and functional.
428-436: No null-check needed forconstantManeuverDirectionLocalOrbitalFrameFactoryThe constructor’s default argument usesLocalOrbitalFrameFactory::Undefined(), and the implementation guards withisDefined()before dereferencing.
phc1990
force-pushed
the
feat/solve-sequences-with-constant-attitude-bias-maneuvers
branch
2 times, most recently
from
e65cb21 to
24595e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Maneuver.cpp(1 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp(2 hunks)bindings/python/test/flight/test_maneuver.py(2 hunks)bindings/python/test/trajectory/test_segment.py(6 hunks)include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp(3 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp(4 hunks)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp(3 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp(5 hunks)test/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.test.cpp(3 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bindings/python/test/flight/test_maneuver.py
🧰 Additional context used
🧬 Code graph analysis (7)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Maneuver.cpp (1)
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (2)
toConstantLocalOrbitalFrameDirectionManeuver(272-328)toConstantLocalOrbitalFrameDirectionManeuver(272-274)
test/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.test.cpp (1)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/State/CoordinateSubset/CartesianAcceleration.cpp (2)
ThrustAcceleration(62-69)ThrustAcceleration(62-62)
bindings/python/test/trajectory/test_segment.py (6)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.hpp (1)
LocalOrbitalFrameFactory(144-148)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (3)
LocalOrbitalFrameFactory(166-175)VNC(133-136)VNC(133-133)bindings/python/test/trajectory/test_local_orbital_frame_factory.py (1)
local_orbital_frame_factory(36-37)bindings/python/test/trajectory/test_local_orbital_frame_direction.py (1)
local_orbital_frame_factory(12-13)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (2)
Segment(304-312)Solution(77-83)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (4)
Segment(428-457)Solution(35-48)solve(506-559)solve(506-506)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (4)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.hpp (2)
LocalOrbitalFrameFactory(144-148)aType(116-118)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameDirection.cpp (2)
Undefined(100-103)Undefined(100-100)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp (4)
anEventConditionSPtr(198-198)anEventConditionSPtr(204-204)aState(211-211)aState(219-223)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.hpp (4)
aType(93-95)aType(102-104)aType(111-111)aType(122-122)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp (1)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (2)
ConstantLocalOrbitalFrameDirectionManeuver(623-640)ConstantLocalOrbitalFrameDirectionManeuver(623-630)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp (5)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Propagator.cpp (5)
Propagator(49-54)Propagator(56-92)Propagator(94-99)Undefined(393-399)Undefined(393-393)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Propagator.hpp (3)
Propagator(63-66)Propagator(78-83)Propagator(88-88)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (2)
VNC(133-136)VNC(133-133)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameDirection.cpp (3)
LocalOrbitalFrameDirection(18-28)Undefined(100-103)Undefined(100-100)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (5)
Solution(35-48)Maneuver(606-621)Maneuver(606-612)ConstantLocalOrbitalFrameDirectionManeuver(623-640)ConstantLocalOrbitalFrameDirectionManeuver(623-630)
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (2)
include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (2)
aLocalOrbitalFrameFactorySPtr(197-199)Maneuver(75-75)src/OpenSpaceToolkit/Astrodynamics/Trajectory/State/CoordinateSubset/CartesianAcceleration.cpp (2)
ThrustAcceleration(62-69)ThrustAcceleration(62-62)
🔇 Additional comments (37)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (7)
23-23: LGTM!The LocalOrbitalFrameFactory include and using declaration are properly added to support the new constant LOF direction maneuver functionality.
Also applies to: 51-51
179-184: LGTM!The Solution now includes frame-aware maneuver caching with
cachedManeuversFrameSPtr_andcachedManeuvers_, plus a friend declaration allowing Segment to access these private members. This addresses the caching concerns from previous reviews.
269-285: LGTM!The new
ConstantLocalOrbitalFrameDirectionManeuverfactory is well-documented, clearly explaining that raw maneuvers are transformed to constant local orbital frame direction maneuvers when solving the segment.
293-293: LGTM!The new member
constantManeuverDirectionLocalOrbitalFrameFactory_appropriately stores the LocalOrbitalFrameFactory for enforcing constant maneuver direction.
314-326: LGTM!The new
solveUsingDynamicsmethod provides a clean separation for solving with specific dynamics and event conditions, supporting the LOF-direction maneuver transformation workflow.
328-333: LGTM!The
identifyThrusterDynamicshelper clearly documents its purpose: locating exactly one Thruster dynamics from the array and throwing an error if zero or multiple are found.
295-312: Default parameter is correct.LocalOrbitalFrameFactory::Undefined()returns a nullShared<const LocalOrbitalFrameFactory>, so no change needed.bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Maneuver.cpp (1)
133-150: LGTM!The Python binding for
to_constant_local_orbital_frame_direction_maneuveris well-documented with a clear docstring explaining the transformation. The binding correctly accepts alocal_orbital_frame_factoryargument and maps to the C++ implementation.bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp (2)
17-17: LGTM!The LocalOrbitalFrameFactory using declaration properly exposes the type for Python bindings.
424-447: LGTM!The Python binding for
constant_local_orbital_frame_direction_maneuveris comprehensive, with all necessary parameters and a clear docstring explaining the segment's behavior.src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (2)
11-11: LGTM!The necessary includes and using declarations for LocalOrbitalFrameFactory and StateBuilder are properly added to support the new LOF-direction transformation functionality.
Also applies to: 15-15, 29-29
272-328: LGTM!The
toConstantLocalOrbitalFrameDirectionManeuverimplementation is well-structured:
- Properly validates the LocalOrbitalFrameFactory input
- Generates LOF frames for each state and accumulates thrust directions
- Guards against zero mean thrust direction (addressing previous review concerns)
- Correctly computes the mean direction and scales by original magnitudes
- Uses StateBuilder to expand partial states
include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (2)
23-23: LGTM!The LocalOrbitalFrameFactory include and using declaration properly expose the dependency in the header.
Also applies to: 52-52
181-199: LGTM!The
toConstantLocalOrbitalFrameDirectionManeuvermethod declaration is thoroughly documented with:
- Clear description of the transformation behavior
- Explanation that thrust direction becomes constant in LOF while magnitude profile is preserved
- Code example demonstrating usage
bindings/python/test/trajectory/test_segment.py (5)
20-20: LGTM!The LocalOrbitalFrameFactory import and fixture properly support testing the new constant LOF direction maneuver functionality.
Also applies to: 79-81
131-147: LGTM!The
constant_local_orbital_frame_direction_maneuver_segmentfixture correctly instantiates a segment using the new factory method with all required parameters including thelocal_orbital_frame_factory.
321-340: LGTM!The
test_constant_local_orbital_frame_direction_maneuvertest verifies that the new factory method can create a segment successfully.
359-359: LGTM!Renaming the test to
test_solve_maneuver_segmentimproves clarity and distinguishes it from the new constant LOF direction maneuver test.
450-458: LGTM!The
test_solve_constant_local_orbital_frame_direction_maneuver_segmenttest validates that solving a constant LOF direction maneuver segment produces a non-None solution.test/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.test.cpp (3)
54-54: LGTM!The LocalOrbitalFrameFactory using declaration enables testing the new LOF-direction maneuver functionality.
64-78: LGTM!The enhanced
stateGeneratornow accepts optional position, velocity, and frame parameters, enabling more flexible test state construction for LOF-direction maneuver tests.
494-637: LGTM!The
toConstantLocalOrbitalFrameDirectionManeuvertests are comprehensive:
- Validates basic functionality and interval preservation
- Verifies error handling for undefined LocalOrbitalFrameFactory
- Tests zero thrust acceleration edge case
- Confirms constant LOF direction with varying magnitudes
- Validates transformation from non-constant to constant LOF direction
The numerical assertions use appropriate tolerances (1e-12) to verify thrust acceleration transformations.
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp (9)
46-46: LGTM!The Propagator include and using declaration enable comprehensive validation of LOF-direction maneuver transformations by comparing segment solutions against manual propagation.
Also applies to: 100-100
159-160: Verify the rationale for reducing numerical solver tolerances.The tolerances were reduced from 1e-12 to 1e-8. The comment indicates this is "so that the solving doesn't take forever," but this 10,000× reduction could mask numerical issues or introduce test flakiness.
Please confirm:
- Are these tolerances still sufficient for the tests' assertions?
- Were specific tests timing out with the stricter tolerances?
- Do the downstream assertions account for this reduced precision?
167-171: LGTM!The
defaultLocalOrbitalFrameFactorySPtr_using VNC and its integration intoconstantThrustSPtr_properly set up the test fixture for LOF-based maneuver testing.
188-234: LGTM!The QLaw-related test scaffolding (default COEs, parameters, QLaw instance, and thruster dynamics) provides a robust foundation for testing multi-maneuver scenarios and LOF-direction transformations.
335-431: LGTM!The expanded
extractManeuverstest coverage now includes:
- Validation of undefined frame handling
- Empty state array error case
- Missing thruster dynamics error case
- Multiple thruster dynamics error case
- Coast segment returning empty maneuvers
These additions strengthen error handling verification.
434-504: LGTM!The maneuver extraction test validates that extracted maneuvers preserve timing, metrics (delta-v, delta-mass), and thrust properties within acceptable tolerances.
508-596: LGTM!The multiple maneuvers extraction test using QLaw verifies:
- Correct count and timing of extracted maneuvers
- Proper handling of zero-thrust scenarios
- State indices alignment between segment solution and extracted maneuvers
947-959: LGTM!The
ConstantLocalOrbitalFrameDirectionManeuvertest verifies the new factory method can construct a segment without throwing exceptions.
1040-1156: LGTM!The comprehensive test comparing raw vs. constant LOF direction maneuvering:
- Creates both segment types with QLaw thruster dynamics
- Verifies timing consistency (start/end instants within 1ms)
- Validates maneuver count and intervals match
- Constructs an expected Propagator with transformed maneuvers
- Compares final states across three approaches:
- Segment solution's final state
- Calculated state using segment's calculateStatesAt
- Propagated state using segment's dynamics
The position tolerance (1e-2 m) and velocity tolerance (1e-6 m/s) are reasonable for orbit propagation comparisons. This test effectively validates that the LOF-direction transformation produces dynamically equivalent results.
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (6)
149-239: LGTM!The maneuver extraction logic correctly identifies thruster dynamics, processes contributions, handles thrusting blocks, and caches results with the frame. The caching implementation properly stores both the frame and maneuvers for subsequent lookups.
508-558: LGTM!The solve method correctly implements a two-pass approach:
- Solves with raw dynamics to get initial maneuvers
- If constant LOF direction is required, converts maneuvers and resolves with tabulated dynamics
The early-return logic properly handles cases where constant LOF is not needed or no maneuvers exist, and the final solution correctly caches the constant-direction maneuvers with their frame.
623-640: LGTM!The static factory method correctly constructs a constant-LOF-direction maneuver segment, following the same pattern as the existing
Maneuverfactory while adding the LocalOrbitalFrameFactory parameter.
642-676: LGTM!The helper method cleanly extracts the common propagation logic, enabling reuse in both the raw solve and the constant-LOF-direction solve paths. The implementation correctly propagates to condition, collects observed states, and constructs the solution.
678-705: LGTM!The thruster identification logic correctly detects exactly one Thruster dynamics in the array. The fixed logic properly throws when multiple thrusters are found and when no thruster exists, with clear error messages.
433-434: Ignore default parameter suggestion. The constructor intentionally defaults toLocalOrbitalFrameFactory::Undefined()as a sentinel value rather thannullptr.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Great stuff! This is a really nice solution I like it, much better than what I was proposing previously (editing it at the sequence level). I would advocate testing this locally and comparing it in a sequence and plotting out the constant and non constant versions (similar to the notebook I'd shared before).
Also worth adding an example to the notebooks in OSTk (main repo)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp
Outdated
Show resolved
Hide resolved
phc1990
force-pushed
the
feat/solve-sequences-with-constant-attitude-bias-maneuvers
branch
from
e790b6f to
8591860
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (1)
179-185: Maneuver cache is now frame-aware — goodAdding cachedManeuversFrameSPtr_ alongside cachedManeuvers_ addresses the cache correctness concern.
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (1)
512-515: Null pointer dereference when factory is not provided.Line 512 dereferences
constantManeuverDirectionLocalOrbitalFrameFactory_with->isDefined()without checking if the pointer is null first. Coast segments and any segment created without a LOF factory will cause a segmentation fault.Apply this guard:
- if (!this->constantManeuverDirectionLocalOrbitalFrameFactory_->isDefined()) + if ((this->constantManeuverDirectionLocalOrbitalFrameFactory_ == nullptr) || + !this->constantManeuverDirectionLocalOrbitalFrameFactory_->isDefined()) { return solution; }
🧹 Nitpick comments (7)
include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (1)
181-199: Document error cases and consider optional angular tolerance
- Please note in the doc that this throws if the factory is null/undefined or if the thrust directions in LOF cancel to ~zero (as implemented in the .cpp).
- Consider an optional angular-tolerance parameter to validate “constancy” and fail when the input maneuver deviates too much (default: disabled).
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
274-329: Mean-direction computation: clarify/optionally change weighting; parameterize epsilon
- Current mean direction uses the raw sum of LOF accelerations, implicitly weighting by instantaneous magnitude and by number of samples (not by time). This can bias the direction when sampling is uneven. Consider:
- Summing normalized LOF directions (skip zero vectors) to avoid magnitude bias, and/or
- Time-step weighting (e.g., multiply by Δt) to avoid sampling-rate bias.
- Replace hardcoded 1e-12 with Real::Epsilon() (or expose epsilon as an optional parameter) for consistency.
Example alternative (equal weighting by direction, skipping zeros):
- const Vector3d thrustAccelerationInLof = transform.applyToVector(thrustAcceleration); - sumInLof += thrustAccelerationInLof; + const Vector3d thrustAccelerationInLof = transform.applyToVector(thrustAcceleration); + if (thrustAccelerationInLof.squaredNorm() > 0.0) + { + sumInLof += thrustAccelerationInLof.normalized(); + }test/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.test.cpp (1)
494-637: Add assertions for mass-flow and magnitude preservationTo tighten coverage of toConstantLocalOrbitalFrameDirectionManeuver:
- Assert that mass flow rates are unchanged per state.
- Assert that thrust acceleration magnitudes are preserved per state (in any frame), in addition to direction checks.
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp (1)
1040-1156: Strengthen validation: also check mass and caching behavior
- After comparing position/velocity, also compare final mass between the segment solution and the expected propagation.
- Consider exercising Segment::Solution::extractManeuvers twice with the same frame to validate the new frame-aware cache (no behavior change expected).
Also note that the hardcoded QLaw indices/counts can be fragile; if QLaw changes, prefer verifying intervals/monotonicity over exact indices.
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (2)
269-285: Document error cases in new factoryPlease add that creation will throw if the Thruster dynamics is missing/duplicated or if the provided LocalOrbitalFrameFactory is undefined/null (as enforced in implementation).
328-334: Return type nitConsider returning Shared (instead of const Shared) to signal immutability of the underlying object and align with typical OSTk patterns.
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (1)
556-557: Consider encapsulating cache manipulation.Directly setting cache fields (
cachedManeuversFrameSPtr_andcachedManeuvers_) bypasses the normal cache-setting logic inextractManeuvers. While this works, it creates tight coupling betweensolve()and the Solution's internal cache structure. If the cache implementation changes, this code must be manually updated.Consider adding a
Solution::setCachedManeuvers(frame, maneuvers)method to encapsulate this logic:// In Solution class void setCachedManeuvers( const Shared<const Frame>& aFrameSPtr, const Array<flightManeuver>& aManeuvers ) { this->cachedManeuversFrameSPtr_ = aFrameSPtr; this->cachedManeuvers_ = aManeuvers; }Then use it here:
- solutionWithConstantLofDirectionManeuvers.cachedManeuversFrameSPtr_ = aState.accessFrame(); - solutionWithConstantLofDirectionManeuvers.cachedManeuvers_ = constantLofDirectionManeuvers; + solutionWithConstantLofDirectionManeuvers.setCachedManeuvers( + aState.accessFrame(), constantLofDirectionManeuvers + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Maneuver.cpp(1 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp(2 hunks)bindings/python/test/flight/test_maneuver.py(2 hunks)bindings/python/test/trajectory/test_segment.py(6 hunks)include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp(3 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp(4 hunks)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp(3 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp(5 hunks)test/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.test.cpp(3 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Maneuver.cpp
- bindings/python/test/trajectory/test_segment.py
- bindings/python/test/flight/test_maneuver.py
- bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp
🧰 Additional context used
🧬 Code graph analysis (5)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp (6)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Propagator.cpp (5)
Propagator(49-54)Propagator(56-92)Propagator(94-99)Undefined(393-399)Undefined(393-393)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Propagator.hpp (3)
Propagator(63-66)Propagator(78-83)Propagator(88-88)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (2)
VNC(133-136)VNC(133-133)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameDirection.cpp (3)
LocalOrbitalFrameDirection(18-28)Undefined(100-103)Undefined(100-100)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (4)
Maneuver(607-622)Maneuver(607-613)ConstantLocalOrbitalFrameDirectionManeuver(624-641)ConstantLocalOrbitalFrameDirectionManeuver(624-631)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)
test/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.test.cpp (2)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Propagator.test.cpp (2)
anInstant(132-141)anInstant(132-134)src/OpenSpaceToolkit/Astrodynamics/Trajectory/State/CoordinateSubset/CartesianAcceleration.cpp (2)
ThrustAcceleration(62-69)ThrustAcceleration(62-62)
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (2)
aLocalOrbitalFrameFactorySPtr(197-199)Maneuver(75-75)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (5)
include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (2)
aFrameSPtr(176-179)Maneuver(75-75)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (8)
aFrameSPtr(123-123)aFrameSPtr(159-159)aName(246-251)aName(261-267)aName(278-285)aDynamicsArray(333-333)aState(231-231)aState(321-326)include/OpenSpaceToolkit/Astrodynamics/Trajectory/State.hpp (1)
aFrameSPtr(240-240)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Propagator.hpp (5)
aNumericalSolver(234-234)aDynamicsArray(147-147)aState(180-180)aState(191-193)aState(203-203)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (3)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.hpp (2)
LocalOrbitalFrameFactory(144-148)aType(116-118)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameDirection.cpp (2)
Undefined(100-103)Undefined(100-100)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.hpp (4)
aType(93-95)aType(102-104)aType(111-111)aType(122-122)
⏰ 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). (1)
- GitHub Check: Build Development Image / Merge Image Manifests
🔇 Additional comments (12)
include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (1)
23-23: LGTM: LOF factory exposureIncluding LocalOrbitalFrameFactory and the using alias are appropriate for the new API surface.
Also applies to: 52-52
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
8-17: LGTM: dependenciesNew includes are minimal and appropriate for the added functionality.
test/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.test.cpp (1)
55-55: LGTM: test scaffoldingThe LocalOrbitalFrameFactory using alias and the generalized stateGenerator improve readability and flexibility.
Also applies to: 65-78
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp (3)
46-46: LGTM: Propagator usage in testsBringing Propagator into scope for reference propagation is appropriate here.
Also applies to: 100-100
155-161: Solver tolerances relaxedReasonable for test runtime. Ensure any assertions sensitive to integrator error margins are updated accordingly (looks consistent with current 1e-2/1e-6 checks).
947-959: LGTM: constructor for ConstantLocalOrbitalFrameDirectionManeuverBasic construction path is covered.
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (2)
23-23: LGTM: LOF factory exposureHeader correctly surfaces LocalOrbitalFrameFactory where needed.
Also applies to: 51-51
293-312: RetainLocalOrbitalFrameFactory::Undefined()as the default argument
LocalOrbitalFrameFactory::Undefined()returns aShared<const LocalOrbitalFrameFactory>, binding correctly to theconstreference. The codebase (and existing tests) relies on this sentinel-pattern, so switching to a null default isn’t compatible.Likely an incorrect or invalid review comment.
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (4)
129-138: LGTM: Cache is now frame-aware with proper null-safety.The cache check correctly validates that
cachedManeuversFrameSPtr_is non-null before dereferencing and ensures the cached maneuvers match the requested frame. The cache is properly populated after extraction (lines 236-238).Also applies to: 236-238
150-151: LGTM: Good refactor to use helper function.Replacing manual thruster identification with
IdentifyThrusterDynamicsimproves maintainability and reduces code duplication.
643-677: LGTM: Clean extraction of propagation logic.
SolveWithDynamicsencapsulates the core propagation workflow, reducing duplication between the initial solve and the re-solve with constant LOF direction maneuvers. The state expansion logic correctly preserves the original state's structure.
679-706: LGTM: Thruster detection logic is now correct.The multiple-thruster detection correctly throws when a second thruster is found. The previous issue where the throw was unreachable has been fixed.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp (1)
435-455: Polish docstring; fix typo and tighten wordingAdjust phrasing per prior suggestion and fix "segement" typo.
- Create a maneuvering segment that produces maneuvers with a constant direction in the local orbital frame. + Create a maneuver segment where potentially variable‑attitude maneuvers are transformed + into constant local‑orbital‑frame (LOF) direction maneuvers when solving the segment. @@ - The provided thruster dynamics are used to solve the segment at first. The maneuvers produced by this segement solution - are then used to create a new thruster dynamics with a constant direction in the local orbital frame. This new thruster dynamics - is then used to actually solve the segment. + First, the provided thruster dynamics solve the segment. The resulting maneuvers are then + converted into a constant‑LOF‑direction thruster, which is used to perform the final solve.src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
300-313: Guard against zero/near-zero mean thrust direction before normalizingCurrently normalizes
sumInLofwithout a magnitude check; if directions cancel, this yields NaNs and a later generic failure. Fail fast with a clear message.- const Vector3d meanThrustDirectionInLof = sumInLof.normalized(); // This will fail if the norm is too small + if (sumInLof.isZero(1e-12)) + { + throw ostk::core::error::RuntimeError( + "Cannot determine mean thrust direction in Local Orbital Frame: accumulated thrust is zero." + ); + } + const Vector3d meanThrustDirectionInLof = sumInLof.normalized();include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (1)
325-327: Fix type mismatch in default argument for LocalOrbitalFrameFactory parameter.The parameter type is
const Shared<const LocalOrbitalFrameFactory>&but the default valueLocalOrbitalFrameFactory::Undefined()returns a value, not a shared pointer. This will not compile.Apply this diff:
const Shared<const LocalOrbitalFrameFactory>& aConstantManeuverDirectionLocalOrbitalFrameFactory = - LocalOrbitalFrameFactory::Undefined(), + nullptr,Also ensure
Segment.cppline 516 checks for nullptr before callingisDefined(). Based on learningssrc/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (1)
516-519: Add nullptr check before calling isDefined().Calling
->isDefined()onconstantManeuverDirectionLocalOrbitalFrameFactory_without first checking for nullptr will cause undefined behavior if the factory was not provided (defaulted to nullptr after fixing the header).Apply this diff:
- if (!this->constantManeuverDirectionLocalOrbitalFrameFactory_->isDefined()) + if ((this->constantManeuverDirectionLocalOrbitalFrameFactory_ == nullptr) || + !this->constantManeuverDirectionLocalOrbitalFrameFactory_->isDefined()) { return solution; }Based on learnings
🧹 Nitpick comments (14)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.hpp (1)
103-126: Solid API; clarify precondition for LOF factory in docs.FromManeuver’s signature and semantics look good and align with the implementation. Consider explicitly documenting that aLocalOrbitalFrameFactorySPtr must be defined (undefined will throw), to match the behavior under test. Also, note in the docs that the returned thrust uses the mean LOF direction and preserves only direction (magnitude remains caller-provided via Thruster).
src/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.cpp (2)
38-49: New overload with applicability interval is well‑formed.Consider also exposing the interval in print() for easier diagnostics.
72-80: Getter throws when undefined; consider adding a predicate.Providing an isApplicabilityIntervalDefined() would let callers avoid exceptions when probing.
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (1)
106-123: Angle comparison should not use modulo; simplify and clarify error messageOffsets are magnitudes; normalizing to [0, 360) is unnecessary and can mask issues. Compare in radians (or plain degrees) without wrap; also use lower-camel for local variable and clarify message.
- const Maneuver::MeanDirectionAndMaximumAngularOffset MeanDirectionAndMaximumAngularOffset = + const Maneuver::MeanDirectionAndMaximumAngularOffset meanDirectionAndMaximumAngularOffset = aManeuver.calculateMeanThrustDirectionAndMaximumAngularOffset(aLocalOrbitalFrameFactorySPtr); if (aMaximumAngularOffset.isDefined()) { - if (MeanDirectionAndMaximumAngularOffset.second.inDegrees(0.0, 360.0) > - aMaximumAngularOffset.inDegrees(0.0, 360.0)) + const auto computedOffsetRad = meanDirectionAndMaximumAngularOffset.second.inRadians(); + const auto maxAllowedRad = aMaximumAngularOffset.inRadians(); + if (computedOffsetRad > maxAllowedRad) { throw ostk::core::error::RuntimeError(String::Format( - "Maximum angular offset ({:.6f} deg) is greater than the maximum allowed ({:.6f} deg).", - MeanDirectionAndMaximumAngularOffset.second.inDegrees(0.0, 360.0), - aMaximumAngularOffset.inDegrees(0.0, 360.0) + "Computed angular offset ({:.6f} deg) exceeds allowed maximum ({:.6f} deg).", + ostk::mathematics::object::Angle::Radians(computedOffsetRad).inDegrees(), + aMaximumAngularOffset.inDegrees() )); } } - return {MeanDirectionAndMaximumAngularOffset.first}; + return {meanDirectionAndMaximumAngularOffset.first};bindings/python/test/dynamics/test_thruster.py (1)
53-54: Initialize inertia tensor deterministicallynp.ndarray(shape=(3, 3)) is uninitialized. Use zeros to avoid random values.
- inertia_tensor = np.ndarray(shape=(3, 3)) + inertia_tensor = np.zeros((3, 3))include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp (1)
75-79: Document exception behavior for undefined intervalGetter throws if interval is undefined (per implementation). Note this in the comment.
- /// @return Applicability interval + /// @return Applicability interval + /// @throws ostk::core::error::runtime::Undefined if the interval is undefinedbindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Dynamics/Thruster.cpp (1)
131-140: Mention exception when interval is undefinedMirror C++ behavior: getter raises if undefined.
- Returns: - Interval: The applicability interval. + Returns: + Interval: The applicability interval. + Raises: + RuntimeError: If the thruster has no defined applicability interval.bindings/python/test/guidance_law/test_constant_thrust.py (2)
86-89: Remove duplicateframefixture (F811)Second definition shadows the first. Drop it.
-@pytest.fixture -def frame() -> Frame: - return Frame.GCRF() -
115-117: Use zip(..., strict=True) for safer fixture construction (B905)Prevents silent length mismatches across profiles.
Ensure CI uses Python >= 3.10 (PEP 618).
- for instant, acceleration, mass_flow_rate in zip( - instants, acceleration_profile, mass_flow_rate_profile - ): + for instant, acceleration, mass_flow_rate in zip( + instants, acceleration_profile, mass_flow_rate_profile, strict=True + ):test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp (1)
1132-1135: Prefer tolerant state comparison to reduce flakinessDirect equality on floating states can be brittle across platforms. If available, use an
isNear/tolerant compare.test/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.test.cpp (1)
85-86: Remove unused fixture member
secondFrameSPtr_is defined but unused; consider removing to avoid warnings.include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (1)
71-72: Minor style: preferusingovertypedefFor modern C++, consider:
using MeanDirectionAndMaximumAngularOffset = Pair<LocalOrbitalFrameDirection, Angle>;src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
349-353: Hoist StateBuilder construction out of the loopAvoid repeated allocations by constructing
StateBuilderonce before the loop.- Array<State> newStates = Array<State>::Empty(); + Array<State> newStates = Array<State>::Empty(); newStates.reserve(states_.getSize()); + const StateBuilder fullStateBuilder = StateBuilder(states_.accessFirst().accessFrame(), RequiredCoordinateSubsets); @@ - const StateBuilder fullStateBuilder = StateBuilder(stateFrame, RequiredCoordinateSubsets); const State partialState = State(state.accessInstant(), newThrustAcceleration, stateFrame, {DefaultAccelerationCoordinateSubsetSPtr}); newStates.add(fullStateBuilder.expand(partialState, state));Note: If states can have differing frames, keep the builder per-frame; otherwise hoist as above.
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Maneuver.cpp (1)
137-145: Enhance docstring with more detail on the maximum angular offset.The docstring for
calculate_mean_thrust_direction_and_maximum_angular_offsetcould be more informative. Consider explaining what "maximum angular offset" represents (e.g., the largest angle between any individual thrust vector and the mean direction).Apply this diff to improve clarity:
R"doc( - Calculate the mean thrust direction in the Local Orbital Frame and its maximum angular offset w.r.t. the maneuver's thrust acceleration directions. + Calculate the mean thrust direction in the Local Orbital Frame and the maximum angular offset + between any individual thrust acceleration direction and the mean direction. Args: local_orbital_frame_factory (LocalOrbitalFrameFactory): The local orbital frame factory.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Dynamics/Thruster.cpp(3 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Maneuver.cpp(1 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/ConstantThrust.cpp(1 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp(2 hunks)bindings/python/test/dynamics/test_thruster.py(3 hunks)bindings/python/test/flight/test_maneuver.py(2 hunks)bindings/python/test/guidance_law/test_constant_thrust.py(3 hunks)include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp(5 hunks)include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp(4 hunks)include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.hpp(3 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp(4 hunks)src/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.cpp(3 hunks)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp(3 hunks)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp(7 hunks)test/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.test.cpp(6 hunks)test/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.test.cpp(4 hunks)test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.test.cpp(5 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (17)
bindings/python/test/flight/test_maneuver.py (3)
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (3)
LocalOrbitalFrameFactory(166-175)TNW(128-131)TNW(128-128)bindings/python/test/guidance_law/test_constant_thrust.py (1)
maneuver(107-126)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.hpp (3)
include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (3)
Maneuver(81-81)aLocalOrbitalFrameFactorySPtr(193-195)aLocalOrbitalFrameFactorySPtr(217-220)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameDirection.hpp (1)
LocalOrbitalFrameDirection(39-41)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.hpp (1)
LocalOrbitalFrameFactory(144-148)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/ConstantThrust.cpp (1)
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (2)
FromManeuver(100-123)FromManeuver(100-104)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Dynamics/Thruster.cpp (1)
src/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.cpp (2)
getApplicabilityInterval(72-80)getApplicabilityInterval(72-72)
bindings/python/test/dynamics/test_thruster.py (3)
include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp (3)
Thruster(44-48)Thruster(56-61)Thruster(63-63)src/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.cpp (3)
Thruster(31-36)Thruster(38-53)Thruster(55-55)test/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.test.cpp (2)
thruster(76-76)thruster(79-79)
test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.test.cpp (5)
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (4)
ConstantThrust(26-30)ConstantThrust(32-32)FromManeuver(100-123)FromManeuver(100-104)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameDirection.cpp (3)
LocalOrbitalFrameDirection(18-28)Undefined(100-103)Undefined(100-100)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (5)
LocalOrbitalFrameFactory(166-175)VNC(133-136)VNC(133-133)Undefined(138-141)Undefined(138-138)include/OpenSpaceToolkit/Astrodynamics/Trajectory/StateBuilder.hpp (3)
StateBuilder(52-54)StateBuilder(61-61)StateBuilder(66-66)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Maneuver.cpp (1)
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (4)
calculateMeanThrustDirectionAndMaximumAngularOffset(274-314)calculateMeanThrustDirectionAndMaximumAngularOffset(274-276)toConstantLocalOrbitalFrameDirectionManeuver(316-356)toConstantLocalOrbitalFrameDirectionManeuver(316-318)
src/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.cpp (4)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (3)
aName(248-253)aName(263-269)aName(289-297)include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp (4)
Thruster(44-48)Thruster(56-61)Thruster(63-63)anInstant(104-106)include/OpenSpaceToolkit/Astrodynamics/Dynamics.hpp (1)
anInstant(119-121)include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw.hpp (1)
anInstant(75-81)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp (5)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (2)
VNC(133-136)VNC(133-133)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameDirection.cpp (1)
LocalOrbitalFrameDirection(18-28)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (5)
Solution(37-50)Maneuver(611-626)Maneuver(611-617)ConstantLocalOrbitalFrameDirectionManeuver(628-647)ConstantLocalOrbitalFrameDirectionManeuver(628-636)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (2)
FromManeuver(100-123)FromManeuver(100-104)
test/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.test.cpp (2)
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)src/OpenSpaceToolkit/Astrodynamics/Trajectory/State/CoordinateSubset/CartesianAcceleration.cpp (2)
ThrustAcceleration(62-69)ThrustAcceleration(62-62)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp (1)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (2)
ConstantLocalOrbitalFrameDirectionManeuver(628-647)ConstantLocalOrbitalFrameDirectionManeuver(628-636)
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (2)
include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (3)
aLocalOrbitalFrameFactorySPtr(193-195)aLocalOrbitalFrameFactorySPtr(217-220)Maneuver(81-81)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameDirection.cpp (3)
Undefined(100-103)Undefined(100-100)LocalOrbitalFrameDirection(18-28)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (3)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.hpp (2)
ConstantThrust(49-49)ConstantThrust(52-52)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (4)
ConstantThrust(26-30)ConstantThrust(32-32)FromManeuver(100-123)FromManeuver(100-104)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (8)
aFrameSPtr(125-125)aFrameSPtr(161-161)aName(248-253)aName(263-269)aName(289-297)aDynamicsArray(349-349)aState(233-233)aState(337-342)
include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp (1)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (3)
aName(248-253)aName(263-269)aName(289-297)
bindings/python/test/guidance_law/test_constant_thrust.py (3)
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)src/OpenSpaceToolkit/Astrodynamics/Trajectory/State.cpp (5)
State(32-47)State(49-60)State(62-97)State(99-155)State(157-163)bindings/python/test/flight/test_maneuver.py (7)
instants(26-32)acceleration_profile(36-42)frame(46-47)mass_flow_rate_profile(51-52)coordinate_subsets(56-62)maneuver(89-92)states(66-85)
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (2)
include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (4)
aManeuver(87-87)aManeuver(93-93)aLocalOrbitalFrameFactorySPtr(193-195)aLocalOrbitalFrameFactorySPtr(217-220)include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.hpp (1)
aManeuver(121-125)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (3)
include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp (3)
Thruster(44-48)Thruster(56-61)Thruster(63-63)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.hpp (2)
LocalOrbitalFrameFactory(144-148)aType(116-118)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.hpp (4)
aType(93-95)aType(102-104)aType(111-111)aType(122-122)
🪛 Ruff (0.14.0)
bindings/python/test/guidance_law/test_constant_thrust.py
87-87: Redefinition of unused frame from line 62
(F811)
115-117: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
⏰ 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). (1)
- GitHub Check: Build Development Image / Merge Image Manifests
🔇 Additional comments (21)
bindings/python/test/flight/test_maneuver.py (4)
12-18: Imports for LOF and Angle are correct.
189-203: Type/shape checks for mean direction and max angular offset look good.
204-212: Conversion to constant LOF-direction maneuver is exercised appropriately.
218-218: Type-annotated reassignment is fine.Keeps intent clear without changing behavior.
test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.test.cpp (1)
315-386: Great coverage for FromManeuver, including undefined factory and offset constraints.Assertions against mean direction value and factory, plus error-path message checks, are on point.
src/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.cpp (2)
31-37: Delegating constructor is correct.
108-112: Short‑circuit outside applicability interval is correct.Returning a zero 4‑vector matches the write subsets (velocity, mass) and avoids fuel checks when inactive.
test/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.test.cpp (2)
114-138: Applicability interval API test looks solidCovers both defined and undefined interval paths with clear message checks. Good coverage.
188-200: Thrust gating by applicability interval is well validatedAsserts before/during/after behaviors and sign of mass-flow component; excellent scenario coverage.
Also applies to: 202-243
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp (1)
947-959: API creation path is exercised correctlyConstructs ConstantLocalOrbitalFrameDirectionManeuver with expected args; good sanity test.
test/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.test.cpp (2)
498-697: Thorough coverage of mean-direction computationValidates defined/undefined inputs, zero-thrust, and multiple direction scenarios with clear expectations. Nicely done.
698-887: Constant-LOF-direction conversion tests are comprehensiveChecks identity properties (instants/frames/pos/vel), magnitude preservation, LOF direction consistency, and angular offset gating. Good end-to-end validation.
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (1)
271-297: LGTM! Clear documentation for the constant LOF direction maneuver.The documentation clearly explains the two-phase solving approach and the purpose of the maximum angular offset parameter. This is well-structured and helpful for users of this API.
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (8)
131-134: LGTM! Proper frame validation.The frame validation ensures that an undefined frame is caught early with a clear error message.
136-140: Verify caching logic works correctly with nullptr initialization.The cache check includes a nullptr guard, which is correct. However, ensure the header initialization is fixed (see header file comment) to use
nullptrinstead ofFrame::Undefined()so this logic works as intended.This logic assumes
cachedManeuversFrameSPtr_is initialized tonullptrin the header. If the header initialization issue is fixed, this caching logic will work correctly.
152-152: LGTM! Clean use of helper function.Using
FindThrusterDynamicscentralizes the thruster discovery logic and provides clear error messages.
522-535: LGTM! Appropriate validation for single-maneuver constraint.The code correctly validates that only one maneuver is present when enforcing constant LOF direction, providing a clear error message if this constraint is violated.
539-549: LGTM! Correct construction of constant thrust dynamics.The code properly extracts the original thruster, creates a
ConstantThrustguidance law from the maneuver with the LOF factory and angular offset, and constructs a new thruster with the maneuver's interval. The name suffix "(Constant Thrust)" clearly distinguishes the transformed dynamics.
551-563: LGTM! Efficient dynamics array reassembly.The code efficiently rebuilds the dynamics array by copying non-thruster dynamics and appending the new constant thrust dynamics. Using
reserve()optimizes allocation.
649-683: LGTM! Clean extraction of propagation logic.The
SolveWithDynamicsmethod cleanly encapsulates the propagation workflow, making the two-phase solving approach insolve()more maintainable. The state expansion logic correctly preserves the input state's coordinate subsets.
685-712: LGTM! Correct thruster identification logic.The function correctly identifies exactly one thruster from the dynamics array, with proper error handling for zero or multiple thrusters. The logic was fixed from earlier reviews to properly detect multiple thrusters.
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/ConstantThrust.cpp
Show resolved
Hide resolved
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp
Outdated
Show resolved
Hide resolved
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Dynamics/Thruster.cpp
Outdated
Show resolved
Hide resolved
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Dynamics/Thruster.cpp
Outdated
Show resolved
Hide resolved
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Dynamics/Thruster.cpp
Outdated
Show resolved
Hide resolved
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Maneuver.cpp
Outdated
Show resolved
Hide resolved
phc1990
force-pushed
the
feat/solve-sequences-with-constant-attitude-bias-maneuvers
branch
from
7d576d7 to
9f23614
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp (1)
10-10: Remove unused Interval dependency from Thruster.Thruster shouldn't depend on Interval; this leaks guidance/interval concerns into dynamics and adds needless coupling. The include and using are also unused here. Please drop them. Based on prior review discussion.
-#include <OpenSpaceToolkit/Physics/Time/Interval.hpp> ... -using ostk::physics::time::Interval;Also applies to: 28-29
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
300-305: Guard normalization of zero vector (critical NaN risk).sumInLof.normalized() on a near-zero vector yields NaNs; try/catch won’t help (Eigen doesn’t throw). Fail fast before normalizing.
- Vector3d meanThrustDirectionInLof; - try - { - meanThrustDirectionInLof = sumInLof.normalized(); - } - catch (const std::exception& e) - { - throw ostk::core::error::RuntimeError( - "Error computing Maneuver's normalized mean thrust direction: [{}].", e.what() - ); - } + if (sumInLof.isZero(1e-12)) + { + throw ostk::core::error::RuntimeError( + "Cannot determine mean thrust direction in Local Orbital Frame: accumulated thrust is zero." + ); + } + const Vector3d meanThrustDirectionInLof = sumInLof.normalized();Echoing prior review request; code still needs this guard.
🧹 Nitpick comments (17)
test/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.test.cpp (1)
45-55: LGTM; minor naming nit.Returning a non-zero thrust acceleration vector is fine for exercising write subsets. Consider renaming parameter
aThrustDirectiontoaThrustAccelerationto mirror the base API and avoid confusion.include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.hpp (2)
103-127: Clarify angle semantics in docs.Please state the expected range for
aMaximumAllowedAngularOffset(e.g., 0°–180°) and that exceeding it throws. This avoids ambiguity around wrap-around vs. principal angles.
11-17: Optional: slim header dependencies.If possible, forward-declare
flight::Maneuverandtrajectory::LocalOrbitalFrameFactoryhere and move their includes to the .cpp. This keeps the header light and reduces rebuilds.bindings/python/test/guidance_law/test_sequential_guidance_law.py (3)
33-41: Silence unused-arg warnings and tighten typing in test shim.Rename unused params (underscore-prefix) and use np.ndarray for annotations to satisfy Ruff and improve clarity.
Based on static analysis hints.
- def calculate_thrust_acceleration_at( - self, - instant: Instant, - position_coordinates: np.array, - velocity_coordinates: np.array, - thrust_acceleration: float, - output_frame: Frame, - ) -> np.array: + def calculate_thrust_acceleration_at( + self, + _instant: Instant, + _position_coordinates: np.ndarray, + _velocity_coordinates: np.ndarray, + _thrust_acceleration: float, + _output_frame: Frame, + ) -> np.ndarray: return np.array([1.0, 2.0, 3.0])
56-64: Apply the same unused-arg/typing cleanup to GuidanceLaw2.Mirror the change made for GuidanceLaw1 to avoid ARG002.
- def calculate_thrust_acceleration_at( - self, - instant: Instant, - position_coordinates: np.array, - velocity_coordinates: np.array, - thrust_acceleration: float, - output_frame: Frame, - ) -> np.array: + def calculate_thrust_acceleration_at( + self, + _instant: Instant, + _position_coordinates: np.ndarray, + _velocity_coordinates: np.ndarray, + _thrust_acceleration: float, + _output_frame: Frame, + ) -> np.ndarray: return np.array([4.0, 5.0, 6.0])
134-146: Add endpoint cases to lock down interval inclusivity.Include instants exactly at 100 s, 200 s, and 300 s to verify closed-interval semantics.
[ (Instant.J2000() - Duration.seconds(50.0), np.array([0.0, 0.0, 0.0])), (Instant.J2000() + Duration.seconds(50.0), np.array([1.0, 2.0, 3.0])), + (Instant.J2000() + Duration.seconds(100.0), np.array([1.0, 2.0, 3.0])), (Instant.J2000() + Duration.seconds(150.0), np.array([0.0, 0.0, 0.0])), + (Instant.J2000() + Duration.seconds(200.0), np.array([4.0, 5.0, 6.0])), (Instant.J2000() + Duration.seconds(250.0), np.array([4.0, 5.0, 6.0])), + (Instant.J2000() + Duration.seconds(300.0), np.array([4.0, 5.0, 6.0])), (Instant.J2000() + Duration.seconds(350.0), np.array([0.0, 0.0, 0.0])), ],bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp (1)
425-457: Doc polish + default string consistency + typo fixes.
- Keep Python default rendering consistent (Angle.undefined()).
- Fix typos: “segement” → “segment”.
arg("local_orbital_frame_factory"), - arg_v("maximum_allowed_angular_offset", Angle::Undefined(), "Angle.Undefined()"), + arg_v("maximum_allowed_angular_offset", Angle::Undefined(), "Angle.undefined()"), R"doc( Create a maneuvering segment that produces maneuvers with a constant direction in the local orbital frame. - The provided thruster dynamics are used to solve the segment at first. The maneuvers produced by this segement solution + The provided thruster dynamics are used to solve the segment at first. The maneuvers produced by this segment solution are then used to create a new thruster dynamics with a constant direction in the local orbital frame. This new thruster dynamics is then used to actually solve the segment.test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.test.cpp (2)
38-47: Parameter name nit: use aThrustAcceleration for clarity.Minor readability: rename aThrustDirection → aThrustAcceleration to match semantics and base signature.
- [[maybe_unused]] const Real& aThrustDirection, + [[maybe_unused]] const Real& aThrustAcceleration,Also applies to: 58-67
247-254: Add boundary instants to assert closed-interval behavior.Include tests exactly at 100 s, 200 s, and 300 s to pin down inclusivity.
const Array<TestCase> testCases = { {Instant::J2000() - Duration::Seconds(50.0), Vector3d(0.0, 0.0, 0.0)}, {Instant::J2000() + Duration::Seconds(50.0), Vector3d(1.0, 2.0, 3.0)}, + {Instant::J2000() + Duration::Seconds(100.0), Vector3d(1.0, 2.0, 3.0)}, {Instant::J2000() + Duration::Seconds(150.0), Vector3d(0.0, 0.0, 0.0)}, + {Instant::J2000() + Duration::Seconds(200.0), Vector3d(4.0, 5.0, 6.0)}, {Instant::J2000() + Duration::Seconds(250.0), Vector3d(4.0, 5.0, 6.0)}, + {Instant::J2000() + Duration::Seconds(300.0), Vector3d(4.0, 5.0, 6.0)}, {Instant::J2000() + Duration::Seconds(350.0), Vector3d(0.0, 0.0, 0.0)} };Also applies to: 255-263
test/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.test.cpp (1)
17-18: Prefer including Angle.hpp (not Derived/Angle.hpp).Angle lives under Physics/Unit; using Derived/Angle.hpp is brittle across versions.
-#include <OpenSpaceToolkit/Physics/Unit/Derived/Angle.hpp> +#include <OpenSpaceToolkit/Physics/Unit/Angle.hpp>bindings/python/test/guidance_law/test_constant_thrust.py (1)
116-127: zip without strict — add safety or assert equal lengths (ruff B905).If running on Python < 3.10, prefer explicit asserts; otherwise pass strict=True.
Option A (version-agnostic, safe):
- states = [] - for instant, acceleration, mass_flow_rate in zip( + assert len(instants) == len(acceleration_profile) == len(mass_flow_rate_profile) + states = [] + for instant, acceleration, mass_flow_rate in zip( instants, acceleration_profile, mass_flow_rate_profile ):Option B (Py>=3.10):
- for instant, acceleration, mass_flow_rate in zip( - instants, acceleration_profile, mass_flow_rate_profile - ): + for instant, acceleration, mass_flow_rate in zip( + instants, acceleration_profile, mass_flow_rate_profile, strict=True + ):Please confirm supported Python versions in CI to pick the correct option.
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp (2)
69-77: Overlapping-interval check also forbids touching endpoints — confirm intent.interval.intersects(anInterval) will likely be true for back-to-back closed intervals sharing an endpoint. If adjacent intervals should be allowed, refine the check to disallow only positive-duration overlap.
If adjacency is allowed, one approach:
- if (interval.intersects(anInterval)) + if (interval.intersection(anInterval).getDuration() > Duration::Zero()) { std::stringstream ss; ss << "Interval intersects: " << interval.toString() << " and " << anInterval.toString(); throw ostk::core::error::RuntimeError(ss.str()); }(Adjust to actual Interval API.)
91-103: Linear scan is fine; consider sorting + binary search later if N grows.Current O(N) lookup is acceptable for small N. If many intervals are expected, store intervals sorted and use binary search.
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/SequentialGuidanceLaw.cpp (1)
21-24: Binding TU dependencies — ensure pybind and utilities are included by the including TU.This file relies on pybind11 types and shiftToString but doesn’t include those headers itself. If this file is included by an aggregator that already includes them, fine; otherwise add the includes.
If needed:
+#include <pybind11/pybind11.h> +#include <OpenSpaceToolkitAstrodynamicsPy/Utilities.hpp>src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (2)
288-298: Consider time-weighting the mean direction.Summing raw thrust vectors treats each sample equally, which biases the mean when state sampling is non-uniform. Weight by Δt (or equivalently integrate acceleration) before normalization.
Sketch:
- In the accumulation loop, add thrustAccelerationInLof * dt_k.
- Use trapezoidal weights if desired: 0.5*(dt_k_prev + dt_k_next) per sample.
Request confirm whether dt-weighting aligns with mission intent.Also applies to: 334-346
355-364: Use builder once per loop; style nit only.Construction/expand pattern looks good. Minor: fullStateBuilder can be created once outside the loop.
- Array<State> newStates = Array<State>::Empty(); - newStates.reserve(states_.getSize()); + Array<State> newStates = Array<State>::Empty(); + newStates.reserve(states_.getSize()); + const StateBuilder fullStateBuilder = StateBuilder(stateFrame /* any */, RequiredCoordinateSubsets); // adjust to correct frame scopingNote: only adopt if StateBuilder does not depend on per-state frame; otherwise keep as-is.
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp (1)
77-84: Virtual specifiers already implied by override.virtual is redundant with override; optional cleanup.
- virtual Vector3d calculateThrustAccelerationAt( + Vector3d calculateThrustAccelerationAt(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Dynamics/Thruster.cpp(1 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Maneuver.cpp(1 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw.cpp(2 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/ConstantThrust.cpp(1 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/SequentialGuidanceLaw.cpp(1 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp(2 hunks)bindings/python/test/dynamics/test_thruster.py(3 hunks)bindings/python/test/flight/test_maneuver.py(2 hunks)bindings/python/test/guidance_law/test_constant_thrust.py(3 hunks)bindings/python/test/guidance_law/test_sequential_guidance_law.py(1 hunks)bindings/python/test/trajectory/test_segment.py(6 hunks)include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp(2 hunks)include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp(4 hunks)include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.hpp(3 hunks)include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp(1 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp(3 hunks)src/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.cpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp(3 hunks)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp(2 hunks)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp(7 hunks)test/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.test.cpp(2 hunks)test/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.test.cpp(4 hunks)test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.test.cpp(5 hunks)test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.test.cpp(1 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/ConstantThrust.cpp
- bindings/python/test/flight/test_maneuver.py
- bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Dynamics/Thruster.cpp
- src/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.cpp
🧰 Additional context used
🧬 Code graph analysis (20)
include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (3)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/State/CoordinateSubset.cpp (2)
Mass(110-114)Mass(110-110)include/OpenSpaceToolkit/Astrodynamics/Dynamics/Tabulated.hpp (1)
Tabulated(56-62)src/OpenSpaceToolkit/Astrodynamics/Dynamics/Tabulated.cpp (1)
Tabulated(26-81)
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (4)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw.hpp (2)
GuidanceLaw(38-38)GuidanceLaw(41-41)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw.cpp (2)
GuidanceLaw(11-14)GuidanceLaw(16-16)include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (4)
aManeuver(87-87)aManeuver(93-93)aLocalOrbitalFrameFactorySPtr(193-195)aLocalOrbitalFrameFactorySPtr(218-221)include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.hpp (1)
aManeuver(122-126)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp (1)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (4)
Maneuver(598-613)Maneuver(598-604)ConstantLocalOrbitalFrameDirectionManeuver(615-637)ConstantLocalOrbitalFrameDirectionManeuver(615-623)
bindings/python/test/guidance_law/test_sequential_guidance_law.py (2)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp (1)
SequentialGuidanceLaw(47-50)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp (1)
SequentialGuidanceLaw(29-45)
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (2)
include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (3)
aLocalOrbitalFrameFactorySPtr(193-195)aLocalOrbitalFrameFactorySPtr(218-221)Maneuver(81-81)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameDirection.cpp (3)
Undefined(100-103)Undefined(100-100)LocalOrbitalFrameDirection(18-28)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (4)
include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp (2)
Thruster(44-48)Thruster(50-50)src/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.cpp (2)
Thruster(31-42)Thruster(44-44)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.hpp (2)
LocalOrbitalFrameFactory(144-148)aType(116-118)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.hpp (4)
aType(93-95)aType(102-104)aType(111-111)aType(122-122)
test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.test.cpp (2)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp (2)
SequentialGuidanceLaw(47-50)anInstant(77-83)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp (1)
SequentialGuidanceLaw(29-45)
bindings/python/test/guidance_law/test_constant_thrust.py (4)
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)src/OpenSpaceToolkit/Astrodynamics/Trajectory/State.cpp (5)
State(32-47)State(49-60)State(62-97)State(99-155)State(157-163)include/OpenSpaceToolkit/Astrodynamics/Trajectory/State.hpp (5)
State(55-60)State(71-76)State(88-95)State(102-102)State(107-107)bindings/python/test/flight/test_maneuver.py (7)
instants(26-32)acceleration_profile(36-42)frame(46-47)mass_flow_rate_profile(51-52)coordinate_subsets(56-62)maneuver(89-92)states(66-85)
test/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.test.cpp (2)
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)src/OpenSpaceToolkit/Astrodynamics/Trajectory/State/CoordinateSubset/CartesianAcceleration.cpp (2)
ThrustAcceleration(62-69)ThrustAcceleration(62-62)
bindings/python/test/trajectory/test_segment.py (3)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.hpp (1)
LocalOrbitalFrameFactory(144-148)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (3)
LocalOrbitalFrameFactory(166-175)VNC(133-136)VNC(133-133)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (4)
Segment(422-451)Solution(39-52)solve(500-551)solve(500-500)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp (8)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp (1)
SequentialGuidanceLaw(47-50)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp (1)
SequentialGuidanceLaw(29-45)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameDirection.cpp (1)
LocalOrbitalFrameDirection(18-28)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (3)
LocalOrbitalFrameFactory(166-175)VNC(133-136)VNC(133-133)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Propagator.cpp (3)
Propagator(49-54)Propagator(56-92)Propagator(94-99)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (4)
Maneuver(598-613)Maneuver(598-604)ConstantLocalOrbitalFrameDirectionManeuver(615-637)ConstantLocalOrbitalFrameDirectionManeuver(615-623)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (2)
FromManeuver(102-125)FromManeuver(102-106)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Maneuver.cpp (1)
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (4)
calculateMeanThrustDirectionAndMaximumAngularOffset(274-324)calculateMeanThrustDirectionAndMaximumAngularOffset(274-276)toConstantLocalOrbitalFrameDirectionManeuver(326-367)toConstantLocalOrbitalFrameDirectionManeuver(326-329)
bindings/python/test/dynamics/test_thruster.py (2)
include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp (2)
Thruster(44-48)Thruster(50-50)src/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.cpp (2)
Thruster(31-42)Thruster(44-44)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp (2)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw.cpp (2)
anInstant(35-54)anInstant(35-41)include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.hpp (1)
anInstant(80-86)
test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.test.cpp (6)
include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (1)
Maneuver(81-81)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (4)
ConstantThrust(28-32)ConstantThrust(34-34)FromManeuver(102-125)FromManeuver(102-106)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameDirection.cpp (1)
LocalOrbitalFrameDirection(18-28)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (3)
LocalOrbitalFrameFactory(166-175)VNC(133-136)VNC(133-133)include/OpenSpaceToolkit/Astrodynamics/Trajectory/StateBuilder.hpp (3)
StateBuilder(52-54)StateBuilder(61-61)StateBuilder(66-66)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.hpp (3)
include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (3)
Maneuver(81-81)aLocalOrbitalFrameFactorySPtr(193-195)aLocalOrbitalFrameFactorySPtr(218-221)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameDirection.hpp (1)
LocalOrbitalFrameDirection(39-41)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.hpp (1)
LocalOrbitalFrameFactory(144-148)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/SequentialGuidanceLaw.cpp (2)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp (1)
SequentialGuidanceLaw(47-50)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp (9)
SequentialGuidanceLaw(29-45)getIntervals(47-50)getIntervals(47-47)getGuidanceLaws(52-55)getGuidanceLaws(52-52)addGuidanceLaw(57-81)addGuidanceLaw(57-57)calculateThrustAccelerationAt(83-103)calculateThrustAccelerationAt(83-89)
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp (2)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp (3)
SequentialGuidanceLaw(47-50)anInterval(66-66)anInstant(77-83)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (2)
calculateThrustAccelerationAt(53-79)calculateThrustAccelerationAt(53-59)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw.cpp (1)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/SequentialGuidanceLaw.cpp (2)
OpenSpaceToolkitAstrodynamicsPy_GuidanceLaw_SequentialGuidanceLaw(21-119)OpenSpaceToolkitAstrodynamicsPy_GuidanceLaw_SequentialGuidanceLaw(21-21)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (6)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.hpp (2)
ConstantThrust(49-49)ConstantThrust(52-52)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (4)
ConstantThrust(28-32)ConstantThrust(34-34)FromManeuver(102-125)FromManeuver(102-106)include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp (1)
SequentialGuidanceLaw(47-50)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp (1)
SequentialGuidanceLaw(29-45)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (8)
aFrameSPtr(125-125)aFrameSPtr(161-161)aState(227-227)aState(324-329)aName(242-247)aName(257-263)aName(283-291)aDynamicsArray(336-336)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)
🪛 Clang (14.0.6)
test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.test.cpp
[error] 3-3: 'OpenSpaceToolkit/Mathematics/Object/Vector.hpp' file not found
(clang-diagnostic-error)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp
[error] 6-6: 'OpenSpaceToolkit/Core/Container/Array.hpp' file not found
(clang-diagnostic-error)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/SequentialGuidanceLaw.cpp
[error] 3-3: 'OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp' file not found
(clang-diagnostic-error)
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp
[error] 3-3: 'OpenSpaceToolkit/Core/Error.hpp' file not found
(clang-diagnostic-error)
🪛 Ruff (0.14.0)
bindings/python/test/guidance_law/test_sequential_guidance_law.py
35-35: Unused method argument: instant
(ARG002)
36-36: Unused method argument: position_coordinates
(ARG002)
37-37: Unused method argument: velocity_coordinates
(ARG002)
38-38: Unused method argument: thrust_acceleration
(ARG002)
39-39: Unused method argument: output_frame
(ARG002)
58-58: Unused method argument: instant
(ARG002)
59-59: Unused method argument: position_coordinates
(ARG002)
60-60: Unused method argument: velocity_coordinates
(ARG002)
61-61: Unused method argument: thrust_acceleration
(ARG002)
62-62: Unused method argument: output_frame
(ARG002)
bindings/python/test/guidance_law/test_constant_thrust.py
88-88: Redefinition of unused frame from line 63
(F811)
116-118: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
⏰ 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). (4)
- GitHub Check: Test / Build Python bindings
- GitHub Check: Test / Check Python Format
- GitHub Check: Test / Check C++ Format
- GitHub Check: Test / Run C++ Unit Tests
🔇 Additional comments (31)
bindings/python/test/dynamics/test_thruster.py (3)
13-13: LGTM! Import is correctly added.The
Intervalimport is necessary for the newapplicability_intervalfixture and test cases.
67-73: LGTM! Fixture is well-structured.The 1-second interval is appropriate for testing and aligns with the instant used in the
statefixture.
107-139: Removeapplicability_intervalparameter from Thruster constructor calls—it is not supported by the C++ implementation.The C++
Thrusterclass does not have anapplicability_intervalfield, constructor parameter, or getter method. The Python bindings (lines 107-139) pass this parameter to the constructor, but it is silently ignored since the C++ binding only acceptssatellite_system,guidance_law, andname.Either:
- Remove
applicability_intervalfrom lines 126 and 131 if the feature is not yet implemented in C++, or- Implement
applicability_intervalsupport in the C++Thrusterclass and expose it in the bindings before using it in tests.Likely an incorrect or invalid review comment.
test/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.test.cpp (1)
158-162: Good assertions for acceleration and mass flow.Checks for exact accel components and negative mass derivative are appropriate here.
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw.cpp (1)
7-8: SequentialGuidanceLaw binding hookup looks correct.Include and registration into the
guidance_lawsubmodule are consistent with existing bindings.Also applies to: 121-125
test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.test.cpp (1)
315-386: FromManeuver tests look solid; cover undefined LOF, pass, and fail paths.Good construction via StateBuilder, mean direction reuse, and message checks. No issues.
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp (1)
1-5: The suggested header path in the review comment is incorrect and inconsistent with the codebase structure.The review comment suggests adding
#include <OpenSpaceToolkit/Physics/Unit/Angle.hpp>, but the actual Angle header in the codebase is located at<OpenSpaceToolkit/Physics/Unit/Derived/Angle.hpp>(as verified in Segment.hpp line 17).Additionally, Segment.cpp already includes Segment.hpp, which transitively provides Angle visibility. The
using ostk::physics::unit::Angle;statement at line 14 resolves correctly through the transitive include chain, andAngle::Undefined()at line 434 compiles without issues.While explicit inclusion can be a defensive programming practice, the suggested diff uses an incorrect header path that does not match the repository structure.
Likely an incorrect or invalid review comment.
bindings/python/test/guidance_law/test_constant_thrust.py (1)
166-182: LGTM: from_maneuver happy path covered.Type assertions exercise both default and constrained angular offset paths.
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp (7)
43-43: LGTM! New includes support SequentialGuidanceLaw and Propagator testing.These additions enable the new constant-LOF-direction maneuver tests and SequentialGuidanceLaw-based equivalence checks.
Also applies to: 47-47, 98-98, 102-102
156-163: LGTM! Tolerance relaxation improves test performance.Reducing from 1e-12 to 1e-8 is a reasonable trade-off for test speed while maintaining sufficient accuracy for integration tests.
169-174: LGTM! Factory-based approach aligns with new LOF patterns.The new
defaultLocalOrbitalFrameFactorySPtr_enables consistent factory-based LOF construction across tests.
190-236: LGTM! QLaw fixtures enable comprehensive sequential guidance testing.The new fixtures provide a well-configured QLaw setup with reasonable parameters for testing constant-LOF-direction maneuvers with multiple burns.
337-433: LGTM! Comprehensive edge-case coverage for extractManeuvers.The refactored tests validate error handling for undefined frames, missing states, missing/multiple thruster dynamics, and coast segments. Error messages are properly verified.
949-961: LGTM! Basic construction test for new segment factory.Validates that
ConstantLocalOrbitalFrameDirectionManeuvercan be created without a maximum angular offset.
1042-1253: LGTM! Thorough validation of angular offset behavior and equivalence.The tests comprehensively validate:
- No maximum offset provided (success)
- Maximum offset provided but not violated (success)
- Maximum offset violated (RuntimeError with correct message)
- Equivalence between constant-LOF maneuvers and SequentialGuidanceLaw-based reconstruction
The equivalence test validates that the new maneuver construction produces results consistent with the expected SequentialGuidanceLaw path.
include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (3)
20-20: LGTM! New includes expose LOF and Angle types.These additions enable LOF-based maneuver operations and expose the necessary types in the
ostk::astrodynamics::flightnamespace.Also applies to: 24-25, 51-51, 55-56
71-71: LGTM! Clear typedef for mean direction and offset.The
MeanDirectionAndMaximumAngularOffsettypedef provides a self-documenting return type for the new LOF-based calculations.
187-221: LGTM! Well-documented LOF-based maneuver methods.The new methods provide:
calculateMeanThrustDirectionAndMaximumAngularOffset: Computes mean LOF direction and its maximum angular offsettoConstantLocalOrbitalFrameDirectionManeuver: Creates a new maneuver with constant LOF direction, with optional maximum angular offset validationDocumentation clearly explains the behavior, parameters, and error conditions.
bindings/python/test/trajectory/test_segment.py (3)
13-13: LGTM! Python fixtures align with new LOF functionality.The new imports and fixtures properly set up test scenarios for constant-LOF-direction maneuvers, including both with and without maximum angular offset validation.
Also applies to: 21-21, 80-83, 132-174
348-381: LGTM! Basic construction test for Python binding.Validates that
constant_local_orbital_frame_direction_maneuvercan be created with and without a maximum angular offset parameter.
400-507: LGTM! Python tests validate constant LOF maneuver solving.The renamed
test_solve_maneuver_segmentclarifies scope, and the newtest_solve_constant_local_orbital_frame_direction_maneuver_segmentvalidates solving with and without maximum angular offset.include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (3)
17-17: LGTM! New includes support LOF-based segment construction.These additions enable the new
ConstantLocalOrbitalFrameDirectionManeuverfactory method.Also applies to: 24-24, 46-46, 53-53
265-291: LGTM! Well-documented constant-LOF-direction maneuver factory.The documentation clearly explains the two-pass solving approach:
- Solve with original thruster dynamics
- Extract maneuvers and compute mean LOF direction
- Re-solve with constant-LOF thruster dynamics
Error behavior for violated maximum offset is documented.
299-336: LGTM! Private members and methods support constant-LOF maneuver solving.The new private members store LOF-related configuration, and the new private methods (
Solve_andFindThrusterDynamics) encapsulate solving logic and thruster identification, respectively.bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Maneuver.cpp (1)
133-169: LGTM! Python bindings properly expose new LOF-based methods.The bindings correctly expose:
calculate_mean_thrust_direction_and_maximum_angular_offset: ReturnsTuple[LocalOrbitalFrameDirection, Angle]to_constant_local_orbital_frame_direction_maneuver: Accepts optionalmaximum_allowed_angular_offsetwith proper defaultDocumentation is comprehensive and follows Python conventions.
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (6)
8-9: LGTM! New includes support constant-LOF maneuver implementation.The
ConstantThrustandSequentialGuidanceLawincludes enable the two-pass solving approach for constant-LOF-direction maneuvers.Also applies to: 29-30
133-136: LGTM! Frame validation and centralized thruster identification.The new frame validation prevents undefined frame errors, and the use of
FindThrusterDynamicscentralizes thruster identification logic.Also applies to: 148-148
433-435: LGTM! Proper initialization of new members.The new members are correctly initialized to
nullptr(no constant-LOF behavior by default) andAngle::Undefined()(optional parameter).
502-550: LGTM! Two-pass solve correctly implements constant-LOF maneuver logic.The implementation:
- Solves with original dynamics
- Extracts maneuvers and computes mean LOF direction for each
- Builds a
SequentialGuidanceLawwithConstantThrustfor each maneuver interval- Rebuilds dynamics, replacing the original thruster with the sequential thruster
- Re-solves with the new dynamics
The null check on line 505 before calling
isDefined()is correct and prevents null dereference.
615-637: LGTM! Factory method correctly constructs constant-LOF maneuver segment.The factory creates a standard
Maneuversegment and then sets the LOF-related private members to enable constant-LOF behavior during solving.
639-702: LGTM! Private methods encapsulate solving and thruster identification.
Solve_encapsulates the core propagation logic, andFindThrusterDynamicscorrectly identifies a single Thruster dynamics, throwing clear errors for zero or multiple thrusters.
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp
Outdated
Show resolved
Hide resolved
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.cpp
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
test/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.test.cpp (1)
24-25: LGTM! Past review feedback addressed.The explicit include for
LocalOrbitalFrameFactory.hpphas been added, resolving the previous review comment about avoiding transitive dependencies.include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp (1)
39-39: Clarify interval intersection semantics per previous review.The documentation should explicitly state that intervals must not intersect including at endpoints (i.e., adjacent intervals touching at a single point are not permitted), as this matches the implementation behavior using
interval.intersects().Apply this diff to clarify:
-/// of the intervals, the thrust acceleration is zero. The guidance law intervals must not intersect each other. +/// of the intervals, the thrust acceleration is zero. The guidance law intervals must not intersect (including at +/// endpoints); adjacent intervals are not permitted.src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (1)
104-128: Fix error message clarity and use correct angular offset range (0–180°).Progress since the last review: variable naming and degree-conversion DRY have been addressed. However, two issues remain:
Error message wording: Line 119–123 still says "Maximum angular offset … is greater than the maximum allowed …", making it unclear which value is measured vs. allowed. Change to "Measured maximum angular offset … exceeds the allowed maximum …".
Incorrect angle range: Lines 115–116 convert angular offsets using
inDegrees(0.0, 360.0). Angular offsets are inherently limited to [0°, 180°] because any angle >180° is equivalent to a smaller offset in the opposite direction (e.g., 181° ≡ 179° opposite). Comparing in the 0–360° range can cause incorrect validation.Apply this diff:
if (aMaximumAllowedAngularOffset.isDefined()) { - const Real maximumAllowedAngularOffsetDegrees = aMaximumAllowedAngularOffset.inDegrees(0.0, 360.0); - const Real maximumAngularOffsetDegrees = meanDirectionAndMaximumAngularOffset.second.inDegrees(0.0, 360.0); + const Real maximumAllowedAngularOffsetDegrees = aMaximumAllowedAngularOffset.inDegrees(0.0, 180.0); + const Real maximumAngularOffsetDegrees = meanDirectionAndMaximumAngularOffset.second.inDegrees(0.0, 180.0); if (maximumAngularOffsetDegrees > maximumAllowedAngularOffsetDegrees) { throw ostk::core::error::RuntimeError(String::Format( - "Maximum angular offset ({:.6f} deg) is greater than the maximum allowed ({:.6f} deg).", + "Measured maximum angular offset ({:.6f} deg) exceeds the allowed maximum ({:.6f} deg).", maximumAngularOffsetDegrees, maximumAllowedAngularOffsetDegrees )); } }
🧹 Nitpick comments (4)
bindings/python/test/dynamics/test_thruster.py (1)
103-118: Consider verifying parameter assignment.The test could be more comprehensive by asserting that the constructor parameters were correctly assigned. Since
get_satellite_system()andget_guidance_law()are available (used in line 121-122), you could verify:
- The satellite system and guidance law were stored correctly
- The name parameter worked as expected (if a
get_name()method exists)Example enhancement:
for thruster in thrusters: assert thruster is not None assert isinstance(thruster, Thruster) assert thruster.is_defined() assert thruster.get_satellite_system() is not None assert thruster.get_guidance_law() is not Nonesrc/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp (1)
83-103: LGTM with optional refinement.The implementation correctly iterates through intervals to find the active guidance law and appropriately returns zero thrust when no interval contains the instant. The delegation pattern is clean.
Optional: If
Vector3dprovides a staticZero()method, consider using it for clarity:- return Vector3d {0.0, 0.0, 0.0}; + return Vector3d::Zero();bindings/python/test/guidance_law/test_constant_thrust.py (2)
67-122: Consider extracting shared fixtures to reduce duplication.The fixtures
instants,acceleration_profile,mass_flow_rate_profile,coordinate_subsets, and the state-building logic closely mirror those intest_maneuver.py(lines 25–91). Extracting these to a sharedconftest.pywould improve maintainability and reduce duplication.
161-177: Strengthen test assertions to verify behavior, not just type.The test only checks
isinstance(constant_thrust, ConstantThrust), which confirms the call succeeds but doesn't validate correctness. Consider adding assertions that verify:
- The resulting thrust direction matches the expected mean LOF direction.
- Edge cases, such as when
maximum_allowed_angular_offsetis violated (should raise an exception).- The guidance law produces expected acceleration vectors when applied.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
bindings/python/test/dynamics/test_thruster.py(1 hunks)bindings/python/test/guidance_law/test_constant_thrust.py(3 hunks)include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp(0 hunks)include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp(2 hunks)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp(1 hunks)test/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.test.cpp(4 hunks)
💤 Files with no reviewable changes (1)
- include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp
🧰 Additional context used
🧬 Code graph analysis (6)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp (3)
include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp (1)
anInstant(82-84)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw.cpp (2)
anInstant(35-54)anInstant(35-41)include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.hpp (1)
anInstant(80-86)
bindings/python/test/guidance_law/test_constant_thrust.py (3)
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)src/OpenSpaceToolkit/Astrodynamics/Trajectory/State.cpp (5)
State(32-47)State(49-60)State(62-97)State(99-155)State(157-163)bindings/python/test/flight/test_maneuver.py (7)
instants(26-32)acceleration_profile(36-42)mass_flow_rate_profile(51-52)coordinate_subsets(56-62)maneuver(89-92)frame(46-47)states(66-85)
test/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.test.cpp (2)
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)src/OpenSpaceToolkit/Astrodynamics/Trajectory/State/CoordinateSubset/CartesianAcceleration.cpp (2)
ThrustAcceleration(62-69)ThrustAcceleration(62-62)
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (2)
include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (4)
aManeuver(87-87)aManeuver(93-93)aLocalOrbitalFrameFactorySPtr(193-195)aLocalOrbitalFrameFactorySPtr(218-221)include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.hpp (1)
aManeuver(122-126)
bindings/python/test/dynamics/test_thruster.py (4)
bindings/python/test/guidance_law/test_constant_thrust.py (1)
guidance_law(36-39)bindings/python/test/trajectory/test_propagator.py (1)
satellite_system(55-75)include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp (2)
Thruster(40-44)Thruster(46-46)src/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.cpp (2)
Thruster(31-42)Thruster(44-44)
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp (2)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp (3)
SequentialGuidanceLaw(47-50)anInterval(66-66)anInstant(77-83)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (2)
calculateThrustAccelerationAt(55-81)calculateThrustAccelerationAt(55-61)
🪛 Clang (14.0.6)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp
[error] 6-6: 'OpenSpaceToolkit/Core/Container/Array.hpp' file not found
(clang-diagnostic-error)
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp
[error] 3-3: 'OpenSpaceToolkit/Core/Error.hpp' file not found
(clang-diagnostic-error)
🪛 Ruff (0.14.0)
bindings/python/test/guidance_law/test_constant_thrust.py
111-113: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
⏰ 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). (4)
- GitHub Check: Test / Check C++ Format
- GitHub Check: Test / Check Python Format
- GitHub Check: Test / Build Python bindings
- GitHub Check: Test / Run C++ Unit Tests
🔇 Additional comments (11)
bindings/python/test/dynamics/test_thruster.py (1)
98-118: Constructor test updated correctly.The test now directly instantiates Thruster objects with different name parameters, properly exercising both the default name and explicit name paths. The basic assertions verify successful instantiation and validity.
test/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.test.cpp (3)
69-83: LGTM! Well-designed helper extension.The addition of position, velocity, and frame parameters with sensible defaults enables the LOF-related test scenarios while maintaining full backward compatibility with existing test code.
499-687: LGTM! Comprehensive test coverage.The test thoroughly validates
calculateMeanThrustDirectionAndMaximumAngularOffsetacross multiple scenarios: error cases (null/undefined factory, zero thrust), constant LOF direction with varying magnitudes, varying LOF directions, and combinations. The geometric configurations are well-chosen to exercise the calculation logic.
689-871: LGTM! Thorough transformation testing.The test validates
toConstantLocalOrbitalFrameDirectionManeuverwith excellent coverage: constraint enforcement (satisfied/violated max offset), error cases, and correctness of the LOF-direction transformation. The verification approach using frame transformations to validate thrust acceleration in LOF coordinates is robust and clearly demonstrates the expected behavior.include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp (1)
47-88: LGTM!The class interface is well-designed with clear separation between construction, access, mutation, and computation. The constructor provides sensible defaults, and the method signatures are consistent with the codebase patterns.
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp (4)
3-8: Past review comment about<sstream>is not applicable.The previous review suggested adding
#include <sstream>forstd::stringstream, but the current implementation usesostk::core::type::String::Format(line 73-75) instead, which doesn't require the<sstream>header.
29-45: LGTM!The constructor implementation is well-structured with proper validation and delegates to
addGuidanceLawto ensure consistent validation logic for all intervals and guidance laws.
47-55: LGTM!The accessor implementations correctly return copies of the internal arrays, preventing external modification of internal state.
57-81: LGTM!The validation logic is thorough and defensive:
- Checks for undefined intervals
- Validates non-null guidance laws
- Prevents interval intersection (including endpoint touching) with clear error messages
The linear search for intersection checking is appropriate for the expected small number of intervals.
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (1)
18-18: LGTM! Using declarations support the new FromManeuver method.The added using declarations for
Real,Angle, andGuidanceLaware appropriate for the new static factory method.Also applies to: 24-24, 26-26
bindings/python/test/guidance_law/test_constant_thrust.py (1)
5-5: LGTM! Imports support the new test fixtures and test.The added imports (numpy, time types, Maneuver, State, CoordinateSubset types) are appropriate for constructing test data and exercising the
from_maneuverpathway.Also applies to: 8-11, 18-24
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp
Outdated
Show resolved
Hide resolved
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Maneuver.cpp
Outdated
Show resolved
Hide resolved
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/ConstantThrust.cpp
Outdated
Show resolved
Hide resolved
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/SequentialGuidanceLaw.cpp
Outdated
Show resolved
Hide resolved
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/SequentialGuidanceLaw.cpp
Outdated
Show resolved
Hide resolved
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/HeterogeneousGuidanceLaw.cpp
Show resolved
Hide resolved
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp
Outdated
Show resolved
Hide resolved
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.hpp
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/ConstantThrust.cpp (1)
112-115: Build break: Angle not in scope for default arg_v.Angle::Undefined() is unqualified in this TU. Add a using or fully qualify the symbol.
@@ - using ostk::core::type::String; + using ostk::core::type::String; + using ostk::physics::unit::Angle; @@ - arg_v("maximum_allowed_angular_offset", Angle::Undefined(), "Angle.Undefined()") + arg_v("maximum_allowed_angular_offset", Angle::Undefined(), "Angle.Undefined()")(or fully-qualify the call if you prefer not to add a using.)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp (1)
36-40: Clarify interval rules; explicitly disallow endpoint adjacency.Docs should match
Interval::intersectsbehavior (touching endpoints counts as intersection). Replace “contiguous” phrasing and state adjacency is not allowed.-/// Guidance laws don't need to be contiguous, and can be added in any order. If the instant does not fall within any -/// of the intervals, the thrust acceleration is zero. The guidance law intervals must not intersect each other. +/// Guidance laws can be added in any order and intervals may be disjoint (gaps allowed). If the instant does not +/// fall within any interval, the thrust acceleration is zero. Intervals must not intersect, including touching at +/// endpoints (adjacent intervals are not permitted).bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/SequentialGuidanceLaw.cpp (1)
27-33: Docstring: specify adjacency is forbidden; remove “contiguous”.Mirror C++ docs so Python users know that touching intervals are invalid.
- A guidance law that sequences multiple guidance laws over specific time intervals. - - At each point in time, the applicable guidance law is selected and used to calculate - the thrust acceleration. Guidance laws don't need to be contiguous, and can be added - in any order. If the instant does not fall within any of the intervals, the thrust - acceleration is zero. The guidance law intervals must not intersect each other. + A guidance law that sequences multiple guidance laws over specific time intervals. + + At each point in time, the applicable guidance law is selected to compute thrust. + Intervals may be disjoint (gaps allowed) and can be added in any order. If the instant + does not fall within any interval, thrust acceleration is zero. Intervals must not + intersect, including touching at endpoints (adjacent intervals are not permitted).
🧹 Nitpick comments (10)
include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (1)
71-72: Prefer modern alias syntax for clarity.Use a type alias instead of typedef for readability and consistency with C++11+.
- typedef Pair<LocalOrbitalFrameDirection, Angle> MeanDirectionAndMaximumAngularOffset; + using MeanDirectionAndMaximumAngularOffset = Pair<LocalOrbitalFrameDirection, Angle>;include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.hpp (1)
14-17: Reduce header coupling with forward declarations.ConstantThrust only needs Maneuver and LocalOrbitalFrameFactory as parameter types; forward‑declare them and include their headers in the .cpp to speed builds and cut dependencies.
-#include <OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp> -#include <OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.hpp> +// fwd decls to avoid heavy includes in header +namespace ostk { namespace astrodynamics { +namespace flight { class Maneuver; } +namespace trajectory { class LocalOrbitalFrameFactory; } +}}Keep LocalOrbitalFrameDirection include since it’s a data member.
bindings/python/test/guidance_law/test_sequential_guidance_law.py (1)
33-41: Silence Ruff ARG002 in mock guidance laws.Prefix unused parameters with “_” to avoid linter noise.
- def calculate_thrust_acceleration_at( - self, - instant: Instant, - position_coordinates: np.array, - velocity_coordinates: np.array, - thrust_acceleration: float, - output_frame: Frame, - ) -> np.array: + def calculate_thrust_acceleration_at( + self, + _instant: Instant, + _position_coordinates: np.array, + _velocity_coordinates: np.array, + _thrust_acceleration: float, + _output_frame: Frame, + ) -> np.array: return np.array([1.0, 2.0, 3.0])Apply similarly to GuidanceLaw2.
Also applies to: 64-72
bindings/python/test/guidance_law/test_constant_thrust.py (1)
161-177: Add a negative test for angular-offset violation (optional).Consider asserting that from_maneuver raises when maximum_allowed_angular_offset is too small.
with pytest.raises(RuntimeError): ConstantThrust.from_maneuver( maneuver=maneuver, local_orbital_frame_factory=LocalOrbitalFrameFactory.TNW(Frame.GCRF()), maximum_allowed_angular_offset=Angle.degrees(0.1), )include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp (1)
16-35: Avoid transitive includes: forward‑declare Frame used in the API.Header uses
Framein the public signature but does not include or forward‑declare it. Forward‑declare to keep the header self‑contained without pulling the heavy header.#include <OpenSpaceToolkit/Physics/Time/Instant.hpp> #include <OpenSpaceToolkit/Physics/Time/Interval.hpp> #include <OpenSpaceToolkit/Astrodynamics/GuidanceLaw.hpp> +// Forward declaration to avoid relying on transitive includes of Frame.hpp +namespace ostk { namespace physics { namespace coordinate { class Frame; } } } + using ostk::core::container::Array; using ostk::core::container::Tuple; using ostk::core::type::Real; using ostk::core::type::Shared; using ostk::mathematics::object::Vector3d; using ostk::physics::coordinate::Frame;test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.test.cpp (1)
356-367: Avoid repeated degree conversions; factor once for readability.Store
maxOffsetDegin a local to reduce duplication and improve clarity.- ConstantThrust constantThrust = ConstantThrust::FromManeuver( - maneuver, - localOrbitalFrameFactorySPtr_, - Angle::Degrees(1.1 * meanDirectionAndMaximumAngularOffset.second.inDegrees(0.0, 360.0)) - ); + const double maxOffsetDeg = + meanDirectionAndMaximumAngularOffset.second.inDegrees(0.0, 360.0); + ConstantThrust constantThrust = ConstantThrust::FromManeuver( + maneuver, localOrbitalFrameFactorySPtr_, Angle::Degrees(1.1 * maxOffsetDeg) + ); ... - ConstantThrust::FromManeuver( - maneuver, - localOrbitalFrameFactorySPtr_, - Angle::Degrees(0.9 * meanDirectionAndMaximumAngularOffset.second.inDegrees(0.0, 360.0)) - ); + const double maxOffsetDeg = + meanDirectionAndMaximumAngularOffset.second.inDegrees(0.0, 360.0); + ConstantThrust::FromManeuver( + maneuver, localOrbitalFrameFactorySPtr_, Angle::Degrees(0.9 * maxOffsetDeg) + );Also applies to: 371-384
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp (1)
1202-1213: Prefer default construction with make_shared.Avoid an unnecessary copy by constructing directly.
- const Shared<SequentialGuidanceLaw> sequentialGuidanceLaw = - std::make_shared<SequentialGuidanceLaw>(SequentialGuidanceLaw()); + const Shared<SequentialGuidanceLaw> sequentialGuidanceLaw = std::make_shared<SequentialGuidanceLaw>();src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp (2)
30-41: Pre‑allocate storage when constructing from an array.Reserve to avoid reallocations when many intervals are provided.
SequentialGuidanceLaw::SequentialGuidanceLaw( const Array<Tuple<Shared<GuidanceLaw>, Interval>>& aGuidanceLawWithIntervalArray ) : GuidanceLaw("Sequential Guidance Law"), intervals_(Array<Interval>::Empty()), guidanceLaws_(Array<Shared<GuidanceLaw>>::Empty()) { + intervals_.reserve(aGuidanceLawWithIntervalArray.getSize()); + guidanceLaws_.reserve(aGuidanceLawWithIntervalArray.getSize()); for (const Tuple<Shared<GuidanceLaw>, Interval>& guidanceLawWithInterval : aGuidanceLawWithIntervalArray) { this->addGuidanceLaw(std::get<0>(guidanceLawWithInterval), std::get<1>(guidanceLawWithInterval)); } }
76-78: Data layout: consider storing pairs instead of parallel arrays.A single
Array<Tuple<Shared<GuidanceLaw>, Interval>>(or a small struct) prevents index skew and simplifies getters.Also applies to: 43-51, 54-75
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (1)
500-551: Consider adding workflow documentation.The two-solve approach is clever but could benefit from a comment block explaining: (1) first solve with original dynamics obtains draft maneuvers and their intervals; (2) those intervals drive constant-LOF thrust guidance laws; (3) second solve re-computes states with the constant-LOF constraint enforced. This would help future maintainers quickly understand the rationale.
Otherwise, the logic is sound:
- Early returns handle the non-constant-LOF case and empty maneuvers
- The dynamics array is correctly rebuilt with
reserve()and pointer comparison to exclude the original thruster- Exception from
ConstantThrust::FromManeuverwhen exceeding max angular offset is intentional per PR objectives
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Maneuver.cpp(1 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/ConstantThrust.cpp(2 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/SequentialGuidanceLaw.cpp(1 hunks)bindings/python/test/guidance_law/test_constant_thrust.py(3 hunks)bindings/python/test/guidance_law/test_sequential_guidance_law.py(1 hunks)include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp(0 hunks)include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp(4 hunks)include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.hpp(4 hunks)include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.cpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp(7 hunks)test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.test.cpp(5 hunks)test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.test.cpp(1 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp(13 hunks)
💤 Files with no reviewable changes (1)
- include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp
🧰 Additional context used
🧬 Code graph analysis (14)
include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (2)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/State/CoordinateSubset.cpp (2)
Mass(110-114)Mass(110-110)include/OpenSpaceToolkit/Astrodynamics/Dynamics/Tabulated.hpp (1)
Tabulated(56-62)
bindings/python/test/guidance_law/test_constant_thrust.py (4)
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)src/OpenSpaceToolkit/Astrodynamics/Trajectory/State.cpp (5)
State(32-47)State(49-60)State(62-97)State(99-155)State(157-163)include/OpenSpaceToolkit/Astrodynamics/Trajectory/State.hpp (5)
State(55-60)State(71-76)State(88-95)State(102-102)State(107-107)bindings/python/test/flight/test_maneuver.py (7)
instants(26-32)acceleration_profile(36-42)mass_flow_rate_profile(51-52)coordinate_subsets(56-62)maneuver(89-92)frame(46-47)states(66-85)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp (3)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.hpp (1)
anInstant(80-86)include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp (1)
anInstant(82-84)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw.cpp (2)
anInstant(35-54)anInstant(35-41)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp (9)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp (1)
SequentialGuidanceLaw(48-51)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp (1)
SequentialGuidanceLaw(30-41)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameDirection.cpp (3)
LocalOrbitalFrameDirection(18-28)Undefined(100-103)Undefined(100-100)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (3)
LocalOrbitalFrameFactory(166-175)VNC(133-136)VNC(133-133)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Propagator.cpp (5)
Propagator(49-54)Propagator(56-92)Propagator(94-99)Undefined(393-399)Undefined(393-393)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Propagator.hpp (3)
Propagator(63-66)Propagator(78-83)Propagator(88-88)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (4)
Maneuver(598-613)Maneuver(598-604)ConstantLocalOrbitalFrameDirectionManeuver(615-637)ConstantLocalOrbitalFrameDirectionManeuver(615-623)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (2)
FromManeuver(104-128)FromManeuver(104-108)
test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.test.cpp (2)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp (2)
SequentialGuidanceLaw(48-51)anInstant(73-79)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp (1)
SequentialGuidanceLaw(30-41)
test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.test.cpp (5)
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (4)
ConstantThrust(30-34)ConstantThrust(36-36)FromManeuver(104-128)FromManeuver(104-108)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameDirection.cpp (3)
LocalOrbitalFrameDirection(18-28)Undefined(100-103)Undefined(100-100)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (5)
LocalOrbitalFrameFactory(166-175)VNC(133-136)VNC(133-133)Undefined(138-141)Undefined(138-138)include/OpenSpaceToolkit/Astrodynamics/Trajectory/StateBuilder.hpp (3)
StateBuilder(52-54)StateBuilder(61-61)StateBuilder(66-66)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Maneuver.cpp (1)
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (4)
calculateMeanThrustDirectionAndMaximumAngularOffset(274-324)calculateMeanThrustDirectionAndMaximumAngularOffset(274-276)toConstantLocalOrbitalFrameDirectionManeuver(326-367)toConstantLocalOrbitalFrameDirectionManeuver(326-329)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.hpp (3)
include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (3)
Maneuver(81-81)aLocalOrbitalFrameFactorySPtr(193-195)aLocalOrbitalFrameFactorySPtr(218-221)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameDirection.hpp (1)
LocalOrbitalFrameDirection(39-41)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.hpp (1)
LocalOrbitalFrameFactory(144-148)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/ConstantThrust.cpp (1)
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (2)
FromManeuver(104-128)FromManeuver(104-108)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/SequentialGuidanceLaw.cpp (3)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp (1)
SequentialGuidanceLaw(48-51)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp (7)
SequentialGuidanceLaw(30-41)getGuidanceLawsWithIntervals(43-52)getGuidanceLawsWithIntervals(43-43)addGuidanceLaw(54-78)addGuidanceLaw(54-54)calculateThrustAccelerationAt(80-100)calculateThrustAccelerationAt(80-86)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (2)
calculateThrustAccelerationAt(55-81)calculateThrustAccelerationAt(55-61)
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp (2)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp (3)
SequentialGuidanceLaw(48-51)aGuidanceLawSPtr(62-62)anInstant(73-79)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw.cpp (2)
anInstant(35-54)anInstant(35-41)
src/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.cpp (1)
include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp (1)
anOutputStream(90-90)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (4)
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (4)
ConstantThrust(30-34)ConstantThrust(36-36)FromManeuver(104-128)FromManeuver(104-108)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp (1)
SequentialGuidanceLaw(30-41)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (8)
aFrameSPtr(125-125)aFrameSPtr(161-161)aState(227-227)aState(324-329)aName(242-247)aName(257-263)aName(283-291)aDynamicsArray(336-336)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)
bindings/python/test/guidance_law/test_sequential_guidance_law.py (2)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp (1)
SequentialGuidanceLaw(48-51)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp (1)
SequentialGuidanceLaw(30-41)
🪛 Clang (14.0.6)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp
[error] 6-6: 'OpenSpaceToolkit/Core/Container/Array.hpp' file not found
(clang-diagnostic-error)
test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.test.cpp
[error] 3-3: 'OpenSpaceToolkit/Mathematics/Object/Vector.hpp' file not found
(clang-diagnostic-error)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/SequentialGuidanceLaw.cpp
[error] 3-3: 'OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp' file not found
(clang-diagnostic-error)
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp
[error] 3-3: 'OpenSpaceToolkit/Core/Error.hpp' file not found
(clang-diagnostic-error)
🪛 Ruff (0.14.1)
bindings/python/test/guidance_law/test_sequential_guidance_law.py
35-35: Unused method argument: instant
(ARG002)
36-36: Unused method argument: position_coordinates
(ARG002)
37-37: Unused method argument: velocity_coordinates
(ARG002)
38-38: Unused method argument: thrust_acceleration
(ARG002)
39-39: Unused method argument: output_frame
(ARG002)
66-66: Unused method argument: instant
(ARG002)
67-67: Unused method argument: position_coordinates
(ARG002)
68-68: Unused method argument: velocity_coordinates
(ARG002)
69-69: Unused method argument: thrust_acceleration
(ARG002)
70-70: Unused method argument: output_frame
(ARG002)
⏰ 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). (1)
- GitHub Check: Build Development Image / Merge Image Manifests
🔇 Additional comments (16)
src/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.cpp (1)
115-115: LGTM!The change correctly uses the public accessor
getName()to align with the removal of the privatename_member from the header file. This follows proper encapsulation practices.include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (2)
187-196: Clarify how “mean direction” is computed (time/magnitude weighting).Docs should state whether the mean is:
- sample-averaged over states,
- magnitude-weighted by thrust acceleration, or
- time-weighted (impulse-weighted, a⋅Δt).
If time-weighted is intended (physically closest to net Δv direction), please note that and ensure the implementation multiplies by per-interval Δt. Else, explicitly document the chosen convention.
205-221: Docs read well; behavior is clear.The “maximum allowed angular offset” semantics and error behavior are unambiguous.
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Maneuver.cpp (1)
133-146: Verify Python return type conversion for Pair→Tuple.The C++ return type is ostk::core::container::Pair<LocalOrbitalFrameDirection, Angle>. Confirm there’s a pybind11 caster mapping this Pair to a Python Tuple; otherwise this will not marshal correctly. If absent, consider returning std::pair on the C++ side or adding a custom type_caster.
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.hpp (1)
103-126: FromManeuver API and docs look solid.Semantics and error behavior are clear and consistent with Maneuver.
bindings/python/test/guidance_law/test_constant_thrust.py (1)
67-123: Fixtures and Maneuver construction look good.Profiles align with coordinate subsets; strict zip is a nice safety.
test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.test.cpp (2)
89-149: Comprehensive coverage; scenario checks are solid.Constructor/getters/add/overlap and dispatch paths are well exercised.
1-13: Verify include path for Vector.hpp if CI complains.A static analyzer flagged 'OpenSpaceToolkit/Mathematics/Object/Vector.hpp' not found. Likely environment-specific, but if it surfaces in CI, ensure the target includes the mathematics include dirs for tests.
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/ConstantThrust.cpp (1)
77-90: Docs/signature for intrack look good.Defaulted velocity_direction and return type are clear.
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp (1)
81-100: Behavior note: returns zero outside all intervals.LGTM. Consider documenting this explicitly in the header (already added) so callers don’t misinterpret the zero vector as a failure.
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (6)
8-9: LGTM!The new includes for
ConstantThrustandSequentialGuidanceLaware appropriately placed and necessary for the constant-LOF maneuver functionality.Also applies to: 29-30
133-136: LGTM!Good defensive check to ensure the frame is defined before proceeding with maneuver extraction.
433-435: LGTM!Appropriate initialization of the constant-LOF members to nullptr and Undefined for optional functionality.
615-637: LGTM!The factory method correctly constructs a Maneuver segment and configures the constant-LOF-specific members before returning.
639-673: LGTM!Good refactoring to extract the core propagation logic into
Solve_(), enabling the two-solve workflow for constant-LOF maneuvers while keeping the code modular.
675-702: LGTM!The
FindThrusterDynamicslogic correctly identifies a single Thruster dynamics, throwing appropriate errors for zero or multiple thrusters. The multiple-thruster detection issue noted in past reviews has been properly addressed.
phc1990
force-pushed
the
feat/solve-sequences-with-constant-attitude-bias-maneuvers
branch
from
978a624 to
59a735b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (1)
113-125: Clarify error message; compare on principal angle.
- Message is awkward; prefer “Measured … exceeds allowed …”.
- Consider comparing on [0°, 180°] principal interval to avoid wrap-around surprises.
- if (aMaximumAllowedAngularOffset.isDefined()) + if (aMaximumAllowedAngularOffset.isDefined()) { - const Real maximumAllowedAngularOffsetDegrees = aMaximumAllowedAngularOffset.inDegrees(0.0, 360.0); - const Real maximumAngularOffsetDegrees = meanDirectionAndMaximumAngularOffset.second.inDegrees(0.0, 360.0); - if (maximumAngularOffsetDegrees > maximumAllowedAngularOffsetDegrees) + // Compare on principal interval + const Real allowedOffsetDeg = std::min(aMaximumAllowedAngularOffset.inDegrees(0.0, 180.0), 180.0); + const Real measuredOffsetDeg = + meanDirectionAndMaximumAngularAngularOffset.second.inDegrees(0.0, 180.0); + if (measuredOffsetDeg > allowedOffsetDeg) { - throw ostk::core::error::RuntimeError(String::Format( - "Maximum angular offset ({:.6f} deg) is greater than the maximum allowed ({:.6f} deg).", - maximumAngularOffsetDegrees, - maximumAllowedAngularOffsetDegrees - )); + throw ostk::core::error::RuntimeError( + String::Format( + "Measured maximum angular offset ({:.6f} deg) exceeds the allowed maximum ({:.6f} deg).", + measuredOffsetDeg, + allowedOffsetDeg + ) + ); } }Optionally validate the input threshold and reject values > 180° early.
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
312-319: Guard against zero vectors and normalize before computing angles.Lines 314 computes
Angle::Between(thrustAccelerationInLof, meanThrustDirectionInLof)wheremeanThrustDirectionInLofis normalized butthrustAccelerationInLofmay not be. Additionally, zero-magnitude thrust accelerations will causeAngle::Betweento fail or return undefined results.Apply this diff to skip zero vectors and ensure consistent normalization:
for (const auto& thrustAccelerationInLof : thrustAccelerationsInLof) { + if (thrustAccelerationInLof.isZero(1e-12)) + { + continue; + } - const Angle offset = Angle::Between(thrustAccelerationInLof, meanThrustDirectionInLof); + const Angle offset = Angle::Between(thrustAccelerationInLof.normalized(), meanThrustDirectionInLof); if (offset.inDegrees(0.0, 360.0) > maximumAngularOffset.inDegrees(0.0, 360.0)) { maximumAngularOffset = offset; } }
🧹 Nitpick comments (9)
bindings/python/test/dynamics/test_thruster.py (2)
116-116: Consider removing redundant None check.The
assert thruster is not Nonecheck is redundant. If the constructor succeeds, it will return an object; if it fails, pytest will catch the exception.Apply this diff to remove the redundant check:
for thruster in thrusters: - assert thruster is not None assert isinstance(thruster, Thruster) assert thruster.is_defined()
103-118: Consider verifying the name parameter is correctly set.While the test verifies construction succeeds, it doesn't confirm the name parameter is actually stored. You could enhance the test by checking
thruster.get_name()(if available) to verify the default name "Thruster" is set for the first instance and "A name" for the second.include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp (2)
36-40: Doc: define disjointness precisely (adjacency).Spell out that intervals must be pairwise disjoint, including endpoints, or explicitly allow adjacency. Current “must not intersect” is ambiguous for closed intervals.
-/// Guidance laws don't need to be contiguous, and can be added in any order. If the instant does not fall within any -/// of the intervals, the thrust acceleration is zero. The guidance law intervals must not intersect each other. +/// Guidance laws need not be contiguous and can be added in any order. If the instant does not fall within any +/// interval, the thrust acceleration is zero. Intervals must be pairwise disjoint (must not intersect, including +/// at endpoints). If adjacency at endpoints is intended to be allowed, state it explicitly here and in the checks.
49-51: Prefer const-correct storage and API for guidance laws.Store and expose Shared to prevent unintended mutation and clarify intent.
-SequentialGuidanceLaw( - const Array<Tuple<Shared<GuidanceLaw>, Interval>>& aGuidanceLawWithIntervalArray = - Array<Tuple<Shared<GuidanceLaw>, Interval>>::Empty() -); +SequentialGuidanceLaw( + const Array<Tuple<Shared<const GuidanceLaw>, Interval>>& aGuidanceLawWithIntervalArray = + Array<Tuple<Shared<const GuidanceLaw>, Interval>>::Empty() +); -Array<Tuple<Shared<GuidanceLaw>, Interval>> getGuidanceLawsWithIntervals() const; +Array<Tuple<Shared<const GuidanceLaw>, Interval>> getGuidanceLawsWithIntervals() const; -void addGuidanceLaw(const Shared<GuidanceLaw>& aGuidanceLawSPtr, const Interval& anInterval); +void addGuidanceLaw(const Shared<const GuidanceLaw>& aGuidanceLawSPtr, const Interval& anInterval); -Array<Shared<GuidanceLaw>> guidanceLaws_; +Array<Shared<const GuidanceLaw>> guidanceLaws_;Add/ensure an invariant check in addGuidanceLaw to reject any intersection per the doc.
Also applies to: 56-56, 62-62, 83-83
include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (2)
187-196: Doc: specify return ordering and angle domain.Clarify that the Pair is (mean_direction, maximum_offset) and that maximum_offset is in [0°, 180°]. Mention thrown conditions for invalid inputs (e.g., undefined factory).
-/// @return The mean thrust direction and its maximum angular offset +/// @return A Pair where first = mean thrust direction in the Local Orbital Frame, second = maximum angular offset +/// between each thrust direction sample and the mean direction. The offset is reported on [0°, 180°]. +/// @throws ostk::core::error::RuntimeError if the LocalOrbitalFrameFactory is undefined or inputs are invalid.
205-221: Doc: define threshold semantics and angle range; consider nodiscard.
- State explicitly that the check compares measured offset (in [0°, 180°]) to the provided maximum_allowed_angular_offset (clamped/validated to ≤ 180°).
- Optionally add [[nodiscard]] to highlight that the returned Maneuver should be used.
-/// @param aMaximumAllowedAngularOffset A maximum allowed angular offset to consider (if any). Defaults to -/// Undefined. +/// @param aMaximumAllowedAngularOffset If defined, the measured maximum angular offset (in [0°, 180°]) +/// must not exceed this threshold; values > 180° are invalid and should be rejected. Defaults to Undefined. +/// @throws ostk::core::error::RuntimeError if the threshold is exceeded or the factory is undefined.include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.hpp (1)
103-126: Doc: spell out throw behavior and angle convention for FromManeuver.Be explicit about:
- The angle domain ([0°, 180°]) used for the comparison.
- The exception thrown when the threshold is exceeded or when inputs are undefined.
-/// If defined, a runtime error will be thrown if the maximum allowed angular offset between the original thrust -/// acceleration direction and the mean thrust direction is violated. +/// If defined, a runtime error will be thrown if the measured maximum angular offset (computed on [0°, 180°]) +/// between the original thrust directions and the mean direction exceeds the provided threshold. +/// @throws ostk::core::error::RuntimeError if inputs are undefined or threshold is exceeded.Minor nit:
virtual ~ConstantThrust() override;—virtualis redundant withoverride. Optional cleanup.bindings/python/test/flight/test_maneuver.py (2)
189-203: Strengthen assertions: check value ranges, not only types.Also assert the offset is within [0°, 180°].
- assert isinstance(mean_thrust_direction_and_maximum_angular_offset[1], Angle) + assert isinstance(mean_thrust_direction_and_maximum_angular_offset[1], Angle) + # Offset should be in [0°, 180°] + assert 0.0 <= mean_thrust_direction_and_maximum_angular_offset[1].in_degrees(0.0, 180.0) <= 180.0
204-220: Verify the conversion actually enforces constant LOF direction.After conversion, the maximum angular offset should be ~0°.
maneuver: Maneuver = maneuver.to_constant_local_orbital_frame_direction_maneuver( local_orbital_frame_factory=LocalOrbitalFrameFactory.TNW(Frame.GCRF()), ) assert maneuver.is_defined() + mean_dir_and_max_offset = maneuver.calculate_mean_thrust_direction_and_maximum_angular_offset( + local_orbital_frame_factory=LocalOrbitalFrameFactory.TNW(Frame.GCRF()), + ) + assert mean_dir_and_max_offset[1].in_degrees(0.0, 180.0) == pytest.approx(0.0, abs=1e-6)Once behavior is finalized, add a negative test asserting that a small maximum_allowed_angular_offset raises RuntimeError.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Dynamics/Thruster.cpp(1 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Maneuver.cpp(2 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw.cpp(2 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/ConstantThrust.cpp(2 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/SequentialGuidanceLaw.cpp(1 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp(2 hunks)bindings/python/test/dynamics/test_thruster.py(1 hunks)bindings/python/test/flight/test_maneuver.py(2 hunks)bindings/python/test/guidance_law/test_constant_thrust.py(3 hunks)bindings/python/test/guidance_law/test_sequential_guidance_law.py(1 hunks)bindings/python/test/trajectory/test_segment.py(6 hunks)include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp(0 hunks)include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp(4 hunks)include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.hpp(4 hunks)include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp(1 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp(3 hunks)src/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.cpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp(3 hunks)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp(2 hunks)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp(7 hunks)test/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.test.cpp(2 hunks)test/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.test.cpp(4 hunks)test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.test.cpp(5 hunks)test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.test.cpp(1 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp(13 hunks)
💤 Files with no reviewable changes (1)
- include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp
🚧 Files skipped from review as they are similar to previous changes (6)
- bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Dynamics/Thruster.cpp
- bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw.cpp
- test/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.test.cpp
- bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/ConstantThrust.cpp
- bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Maneuver.cpp
- src/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.cpp
🧰 Additional context used
🧬 Code graph analysis (18)
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (2)
include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (4)
aManeuver(87-87)aManeuver(93-93)aLocalOrbitalFrameFactorySPtr(193-195)aLocalOrbitalFrameFactorySPtr(218-221)include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.hpp (1)
aManeuver(122-126)
test/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.test.cpp (3)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/State/CoordinateSubset.cpp (2)
Mass(110-114)Mass(110-110)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)src/OpenSpaceToolkit/Astrodynamics/Trajectory/State/CoordinateSubset/CartesianAcceleration.cpp (2)
ThrustAcceleration(62-69)ThrustAcceleration(62-62)
bindings/python/test/guidance_law/test_constant_thrust.py (3)
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)src/OpenSpaceToolkit/Astrodynamics/Trajectory/State.cpp (5)
State(32-47)State(49-60)State(62-97)State(99-155)State(157-163)bindings/python/test/flight/test_maneuver.py (7)
instants(26-32)acceleration_profile(36-42)mass_flow_rate_profile(51-52)coordinate_subsets(56-62)maneuver(89-92)frame(46-47)states(66-85)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp (8)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp (1)
SequentialGuidanceLaw(48-51)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp (1)
SequentialGuidanceLaw(30-41)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameDirection.cpp (1)
LocalOrbitalFrameDirection(18-28)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (3)
LocalOrbitalFrameFactory(166-175)VNC(133-136)VNC(133-133)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Propagator.cpp (3)
Propagator(49-54)Propagator(56-92)Propagator(94-99)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Propagator.hpp (3)
Propagator(63-66)Propagator(78-83)Propagator(88-88)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (4)
Maneuver(598-613)Maneuver(598-604)ConstantLocalOrbitalFrameDirectionManeuver(615-637)ConstantLocalOrbitalFrameDirectionManeuver(615-623)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (2)
FromManeuver(104-128)FromManeuver(104-108)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/SequentialGuidanceLaw.cpp (2)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp (1)
SequentialGuidanceLaw(48-51)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp (7)
SequentialGuidanceLaw(30-41)getGuidanceLawsWithIntervals(43-52)getGuidanceLawsWithIntervals(43-43)addGuidanceLaw(54-78)addGuidanceLaw(54-54)calculateThrustAccelerationAt(80-100)calculateThrustAccelerationAt(80-86)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp (2)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw.cpp (2)
anInstant(35-54)anInstant(35-41)include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.hpp (1)
anInstant(80-86)
bindings/python/test/trajectory/test_segment.py (4)
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (2)
ConstantThrust(30-34)ConstantThrust(36-36)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (3)
LocalOrbitalFrameFactory(166-175)VNC(133-136)VNC(133-133)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (2)
Segment(309-315)Solution(79-85)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (4)
Segment(422-451)Solution(39-52)solve(500-551)solve(500-500)
bindings/python/test/flight/test_maneuver.py (5)
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.hpp (1)
LocalOrbitalFrameFactory(144-148)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (3)
LocalOrbitalFrameFactory(166-175)TNW(128-131)TNW(128-128)bindings/python/test/guidance_law/test_constant_thrust.py (1)
maneuver(103-122)src/OpenSpaceToolkit/Astrodynamics/Trajectory/State.cpp (5)
State(32-47)State(49-60)State(62-97)State(99-155)State(157-163)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (5)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.hpp (2)
ConstantThrust(49-49)ConstantThrust(52-52)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (4)
ConstantThrust(30-34)ConstantThrust(36-36)FromManeuver(104-128)FromManeuver(104-108)include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp (1)
SequentialGuidanceLaw(48-51)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp (1)
SequentialGuidanceLaw(30-41)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (6)
aState(227-227)aState(324-329)aName(242-247)aName(257-263)aName(283-291)aDynamicsArray(336-336)
bindings/python/test/dynamics/test_thruster.py (5)
bindings/python/test/guidance_law/test_constant_thrust.py (1)
guidance_law(36-39)bindings/python/test/trajectory/test_propagator.py (1)
satellite_system(55-75)src/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.cpp (2)
Thruster(31-41)Thruster(43-43)include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp (2)
Thruster(40-44)Thruster(46-46)bindings/python/test/dynamics/test_dynamics.py (1)
is_defined(24-25)
test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.test.cpp (6)
include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (1)
Maneuver(81-81)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (4)
ConstantThrust(30-34)ConstantThrust(36-36)FromManeuver(104-128)FromManeuver(104-108)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameDirection.cpp (3)
LocalOrbitalFrameDirection(18-28)Undefined(100-103)Undefined(100-100)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (5)
LocalOrbitalFrameFactory(166-175)VNC(133-136)VNC(133-133)Undefined(138-141)Undefined(138-138)include/OpenSpaceToolkit/Astrodynamics/Trajectory/StateBuilder.hpp (3)
StateBuilder(52-54)StateBuilder(61-61)StateBuilder(66-66)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (5)
include/OpenSpaceToolkit/Astrodynamics/Dynamics.hpp (2)
Dynamics(68-68)Dynamics(71-71)src/OpenSpaceToolkit/Astrodynamics/Dynamics.cpp (2)
Dynamics(45-48)Dynamics(50-50)src/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.cpp (2)
Thruster(31-41)Thruster(43-43)include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp (2)
Thruster(40-44)Thruster(46-46)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.hpp (2)
LocalOrbitalFrameFactory(144-148)aType(116-118)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.hpp (4)
include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (3)
Maneuver(81-81)aLocalOrbitalFrameFactorySPtr(193-195)aLocalOrbitalFrameFactorySPtr(218-221)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameDirection.hpp (1)
LocalOrbitalFrameDirection(39-41)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.hpp (1)
LocalOrbitalFrameFactory(144-148)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp (1)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (4)
Maneuver(598-613)Maneuver(598-604)ConstantLocalOrbitalFrameDirectionManeuver(615-637)ConstantLocalOrbitalFrameDirectionManeuver(615-623)
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (2)
include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (3)
aLocalOrbitalFrameFactorySPtr(193-195)aLocalOrbitalFrameFactorySPtr(218-221)Maneuver(81-81)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameDirection.cpp (3)
Undefined(100-103)Undefined(100-100)LocalOrbitalFrameDirection(18-28)
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp (2)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp (3)
SequentialGuidanceLaw(48-51)aGuidanceLawSPtr(62-62)anInstant(73-79)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (2)
calculateThrustAccelerationAt(55-81)calculateThrustAccelerationAt(55-61)
bindings/python/test/guidance_law/test_sequential_guidance_law.py (2)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp (1)
SequentialGuidanceLaw(48-51)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp (1)
SequentialGuidanceLaw(30-41)
test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.test.cpp (2)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp (2)
SequentialGuidanceLaw(48-51)anInstant(73-79)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp (1)
SequentialGuidanceLaw(30-41)
🪛 Clang (14.0.6)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/SequentialGuidanceLaw.cpp
[error] 3-3: 'OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp' file not found
(clang-diagnostic-error)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp
[error] 6-6: 'OpenSpaceToolkit/Core/Container/Array.hpp' file not found
(clang-diagnostic-error)
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp
[error] 3-3: 'OpenSpaceToolkit/Core/Error.hpp' file not found
(clang-diagnostic-error)
test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.test.cpp
[error] 3-3: 'OpenSpaceToolkit/Mathematics/Object/Vector.hpp' file not found
(clang-diagnostic-error)
🪛 Ruff (0.14.1)
bindings/python/test/guidance_law/test_constant_thrust.py
111-113: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
bindings/python/test/guidance_law/test_sequential_guidance_law.py
35-35: Unused method argument: instant
(ARG002)
36-36: Unused method argument: position_coordinates
(ARG002)
37-37: Unused method argument: velocity_coordinates
(ARG002)
38-38: Unused method argument: thrust_acceleration
(ARG002)
39-39: Unused method argument: output_frame
(ARG002)
66-66: Unused method argument: instant
(ARG002)
67-67: Unused method argument: position_coordinates
(ARG002)
68-68: Unused method argument: velocity_coordinates
(ARG002)
69-69: Unused method argument: thrust_acceleration
(ARG002)
70-70: Unused method argument: output_frame
(ARG002)
🔇 Additional comments (37)
bindings/python/test/dynamics/test_thruster.py (1)
103-113: LGTM! Good coverage of constructor variants.The test correctly verifies both constructor signatures: with and without the optional name parameter.
bindings/python/test/guidance_law/test_sequential_guidance_law.py (2)
25-43: LGTM: Unused parameters in test mocks are intentional.The static analyzer flags unused method arguments in the mock GuidanceLaw implementations. These are false positives—the mock classes implement the GuidanceLaw interface for testing and intentionally return constant vectors without using the parameters. This is a standard testing pattern.
Also applies to: 56-74
92-164: LGTM: Comprehensive test coverage for SequentialGuidanceLaw.The test suite effectively validates:
- Constructor and getters with multiple guidance law/interval pairs
- Incremental addition via
add_guidance_law- Piecewise thrust computation across time intervals (including boundary cases and gaps)
The parametrized test cleanly verifies that zero thrust is returned outside intervals and correct thrust vectors are returned within active intervals.
test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.test.cpp (4)
31-69: LGTM: Well-structured mock guidance laws for testing.The MockGuidanceLaw1 and MockGuidanceLaw2 classes properly implement the GuidanceLaw interface with
[[maybe_unused]]attributes for parameters, returning constant vectors for deterministic test behavior.
89-149: LGTM: Thorough constructor error handling.The constructor tests comprehensively validate:
- No-argument and with-arguments construction
- Undefined interval error (with exact error message verification)
- Null guidance law error
- Intersecting intervals error
Error message validation ensures the API contract is maintained.
176-253: LGTM: Complete addGuidanceLaw validation.The tests verify both success and error paths:
- Incremental addition of guidance laws
- Validation that the internal state matches expectations
- Error handling for undefined intervals, null guidance laws, and intersecting intervals
255-281: LGTM: CalculateThrustAccelerationAt behavior validated.The parametrized test cases verify:
- Zero thrust before the first interval
- Correct thrust from the first guidance law within interval1
- Zero thrust in the gap between intervals
- Correct thrust from the second guidance law within interval2
- Zero thrust after the last interval
This confirms the piecewise behavior of SequentialGuidanceLaw.
bindings/python/test/guidance_law/test_constant_thrust.py (2)
67-122: LGTM: Well-constructed maneuver fixture for testing.The fixtures create a realistic maneuver from:
- Four time instants with varying acceleration profiles
- Negative mass flow rates (correctly representing propellant consumption)
- Full state construction with position, velocity, acceleration, and mass flow rate
This provides a solid foundation for testing
ConstantThrust.from_maneuver.
161-177: LGTM: Comprehensive from_maneuver test coverage.The test validates:
- Basic construction from a maneuver without angular offset constraint
- Construction with
maximum_allowed_angular_offsetparameter- Proper typing and instance validation
This covers the key use cases for the new
from_maneuverfactory method.test/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.test.cpp (6)
69-83: LGTM: Enhanced stateGenerator for frame-aware testing.The updated signature with position, velocity, and frame parameters enables more flexible test state construction, which is essential for testing Local Orbital Frame transformations.
499-567: LGTM: Robust error handling tests for calculateMeanThrustDirectionAndMaximumAngularOffset.The tests validate:
- Proper error for null LocalOrbitalFrameFactory
- Proper error for undefined LocalOrbitalFrameFactory
- RuntimeError for zero thrust acceleration (which would have undefined direction)
Error messages are validated for clarity.
568-644: LGTM: Validation of constant LoF direction with varying magnitudes.This test case verifies that when thrust direction is constant in the TNW frame (
{2, -1, 3}) but magnitudes differ (0.8 and 1.5), the algorithm:
- Correctly computes the normalized mean direction
- Returns zero angular offset (confirming the direction is constant)
The test uses different positions and velocities to generate distinct GCRF representations, ensuring the LoF transformation is properly exercised.
604-686: LGTM: Comprehensive testing of non-constant directions.These test cases validate:
- Case 1: Non-constant direction in TNW but equal magnitudes → mean direction is the average, and maximum angular offset is computed correctly
- Case 2: Neither direction nor magnitudes are constant → mean direction accounts for magnitude weighting, and maximum angular offset reflects the worst-case deviation
The explicit verification of
Angle::Betweenfor maximum offset ensures the algorithm correctly identifies the largest deviation.
689-772: LGTM: toConstantLocalOrbitalFrameDirectionManeuver error handling.The tests validate:
- Successful transformation without angular offset constraint
- Transformation with satisfied angular offset constraint
- RuntimeError when angular offset constraint is violated
- Undefined LocalOrbitalFrameFactory error
- Zero thrust acceleration error
The error message validation ensures clear diagnostics.
774-871: LGTM: Verification of thrust direction preservation in LoF frame.These tests confirm that
toConstantLocalOrbitalFrameDirectionManeuver:
- Case 1: Preserves constant LoF direction and original magnitudes when direction is already constant
- Case 2: Transforms to constant LoF direction while preserving magnitude when original direction varies
The explicit frame transformation checks (
getTransformTo+applyToVector) ensure the thrust vectors are correctly expressed in the TNW frame and match expected values.test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.test.cpp (2)
107-112: LGTM: Improved test fixture organization.Moving
localOrbitalFrameFactorySPtr_to the fixture level eliminates duplication and makes it available for the newFromManeuvertest. The initialization usesLocalOrbitalFrameFactory::VNC(gcrfSPtr_)consistently.
315-386: LGTM: Comprehensive FromManeuver test coverage.The test validates:
- Error handling for undefined
LocalOrbitalFrameFactory- Basic construction from a maneuver (verifying direction and factory match expected values)
- Construction with a satisfied angular offset constraint
- RuntimeError when angular offset is violated (with error message validation)
The test constructs a realistic maneuver using
StateBuilderand exercises all FromManeuver code paths.bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp (2)
14-18: LGTM: Proper using declarations for new types.The additions of
AngleandLocalOrbitalFrameFactoryto the using block make these types available for the new binding.
425-457: LGTM: Well-documented constant_local_orbital_frame_direction_maneuver binding.The binding:
- Correctly maps to
Segment::ConstantLocalOrbitalFrameDirectionManeuver- Documents all parameters clearly
- Properly handles the optional
maximum_allowed_angular_offsetwithAngle::Undefined()default- Explains the workflow: solve with original dynamics, compute mean direction, create constant-direction thruster, re-solve
- Notes the RuntimeError behavior when angular offset is violated
The documentation is clear about the two-phase solve process, which is important for users to understand.
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/SequentialGuidanceLaw.cpp (2)
23-48: LGTM: Proper class binding with comprehensive documentation.The binding:
- Correctly specifies inheritance from
GuidanceLawandSharedpointer semantics- Documents the piecewise behavior clearly: guidance laws can be non-contiguous, added in any order, and return zero thrust outside intervals
- Constructor accepts
List[Tuple[GuidanceLaw, Interval]]with proper error handling documentationThe class-level documentation provides essential context for users.
50-99: LGTM: Complete method bindings with clear documentation.All public methods are properly bound:
get_guidance_laws_with_intervals: Returns the internal listadd_guidance_law: Incremental addition with error handling documentedcalculate_thrust_acceleration_at: Piecewise thrust computationEach method's documentation clearly states parameters, returns, and error conditions.
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp (8)
43-47: LGTM: Proper includes and using declarations for new types.The additions of
SequentialGuidanceLawandPropagatorheaders and using declarations enable testing of the new constant LOF direction maneuver functionality.Also applies to: 98-102
161-162: Pragmatic tolerance adjustment for test performance.Reducing tolerances from
1e-12to1e-8is reasonable for integration tests involving numerical propagation, especially given the comment notes this prevents tests from "taking forever." The relaxed tolerance is still sufficiently tight for validation purposes.
169-173: LGTM: Enhanced test fixture with QLaw and LOF factory.The additions:
defaultLocalOrbitalFrameFactorySPtr_: Shared LOF factory for constant-direction maneuversdefaultQLawParameters_,defaultQLaw_,defaultQLawThrusterDynamicsSPtr_: Enable testing of sequential guidance and multi-maneuver scenariosThese fixtures support comprehensive testing of the new constant LOF direction functionality.
Also applies to: 190-236
337-433: LGTM: Comprehensive extractManeuvers error handling.The extended tests validate:
- Undefined frame error
- No states error
- No thruster dynamics error
- Multiple thruster dynamics error
- Coast segments return zero maneuvers
Each error case validates the exact error message, ensuring clear diagnostics.
435-507: LGTM: Thorough maneuver extraction validation.The tests verify:
- Single maneuver: Extraction from a maneuvering segment, with metrics (ΔV, Δmass, average thrust, specific impulse) matching segment solution
- Multiple maneuvers: Extraction of two maneuvers from a QLaw segment with correct instant ranges
- No maneuvers: Zero maneuvers when QLaw determines no thrust is needed (target already achieved)
The hardcoded expected values for QLaw maneuvers are pragmatic but brittle (noted in comments). The relative error checks (scaled by
1e-10or5e-6) appropriately account for numerical integration and trapezoidal rule approximations.Also applies to: 510-599
949-961: LGTM: Constructor test for ConstantLocalOrbitalFrameDirectionManeuver.The test validates successful construction of a constant LOF direction maneuver segment with all required parameters.
1042-1135: LGTM: Maximum angular offset behavior validation.The tests comprehensively verify:
- No offset provided: Segment solves successfully without constraint
- Offset satisfied: Segment solves successfully when actual offset is below the limit
- Offset violated: RuntimeError thrown with clear error message when actual offset exceeds the limit
This validates the intended safety mechanism for constant LOF direction maneuvers.
1137-1253: LGTM: Equivalence validation between constant LOF segment and sequential guidance law.This sophisticated test validates that:
- Constant LOF maneuver segment produces two maneuvers with intervals close to the original QLaw maneuvers
- Manually constructing a
SequentialGuidanceLawfrom the original maneuvers usingConstantThrust::FromManeuverproduces equivalent results- The final states match exactly
This confirms that the constant LOF segment internally implements the expected transformation workflow: solve → extract maneuvers → create constant-direction guidance laws → re-solve. The test provides strong confidence in the correctness of the implementation.
src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
326-367: Implementation looks correct.The logic correctly computes the mean direction, validates the maximum allowed angular offset, and transforms each state to use a constant LOF direction while preserving magnitude. The use of StateBuilder ensures proper state expansion.
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp (1)
30-100: Clean implementation of sequential guidance law dispatch.The implementation correctly validates inputs (null checks, overlap detection) and dispatches to the appropriate guidance law by interval. The fallback to a zero vector when no interval matches (line 99) is reasonable for this use case.
bindings/python/test/trajectory/test_segment.py (1)
80-83: Good test coverage for new constant LOF direction maneuvers.The new fixtures and test methods appropriately exercise both the construction and solving of constant local orbital frame direction maneuvers, including cases with and without the maximum angular offset parameter.
Also applies to: 133-175, 348-381, 491-507
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (1)
265-291: Well-designed API for constant LOF direction maneuvers.The new
ConstantLocalOrbitalFrameDirectionManeuverstatic factory is clearly documented, explaining the two-phase solve workflow and the optional runtime error on exceedingaMaximumAllowedAngularOffset. The private helper methods and members support this functionality cleanly.Also applies to: 299-300, 302-336
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (4)
133-136: Good defensive frame validation.Adding the frame validation before extracting maneuvers prevents potential issues downstream.
500-551: Two-phase solve workflow correctly implements constant LOF direction.The logic properly:
- Solves with original dynamics to obtain draft maneuvers
- Transforms maneuvers to constant LOF direction via
ConstantThrust::FromManeuver- Assembles a
SequentialGuidanceLawcovering maneuver intervals- Replaces original thruster dynamics with sequential thrust dynamics
- Re-solves with the updated dynamics
615-637: Factory method correctly initializes constant LOF maneuver segment.The implementation properly constructs a base maneuver segment and sets the private LOF-specific members that drive the two-phase solve in the
solve()method.
675-702: Thruster discovery logic correctly handles all cases.The helper properly detects zero, one, or multiple
Thrusterdynamics in the array, throwing appropriate errors for invalid configurations.
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/SequentialGuidanceLaw.cpp
Outdated
Show resolved
Hide resolved
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.hpp
Outdated
Show resolved
Hide resolved
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/SequentialGuidanceLaw.cpp
Outdated
Show resolved
Hide resolved
test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.test.cpp
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (1)
615-637: Factory: document why fields are set post-construction (ABI/back-compat).Add a brief comment explaining why the LOF config is applied after constructing
Segment(e.g., to keep constructor ABI stable). Matches earlier review discussion.
🧹 Nitpick comments (23)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.hpp (6)
32-35: Add an explicit forward declaration (or include) for Frame in this header.Relying on transitive includes is brittle. Forward‑declare Frame before the using statements, or include <OpenSpaceToolkit/Physics/Coordinate/Frame.hpp>.
+#include <OpenSpaceToolkit/Physics/Time/Interval.hpp> + +// Forward declaration to avoid heavy include here +namespace ostk { namespace physics { namespace coordinate { class Frame; } } } - -using ostk::physics::coordinate::Frame; +using ostk::physics::coordinate::Frame;
36-41: Clarify interval boundary semantics.Doc says intervals “must not intersect” but doesn’t say if touching (e.g., [t0,t1] and [t1,t2]) is allowed. State whether intervals are closed, half‑open, and whether endpoint adjacency is valid. This prevents ambiguity in both usage and tests.
48-51: Prefer const‑correctness for stored laws.Consider accepting/holding Shared to prevent mutation through this container and match typical design for strategy objects.
- HeterogeneousGuidanceLaw( - const Array<Pair<Shared<GuidanceLaw>, Interval>>& aGuidanceLawWithIntervalArray = - Array<Pair<Shared<GuidanceLaw>, Interval>>::Empty() + HeterogeneousGuidanceLaw( + const Array<Pair<Shared<const GuidanceLaw>, Interval>>& aGuidanceLawWithIntervalArray = + Array<Pair<Shared<const GuidanceLaw>, Interval>>::Empty() ) ;Impact: updates to addGuidanceLaw(), getGuidanceLawsWithIntervals(), and private storage.
58-63: Tighten addGuidanceLaw signature and validate upfront.
- Take Shared& to enforce immutability.
- Document/validate that anInterval is defined and non‑overlapping (tests already expect this), but call it out here too.
-void addGuidanceLaw(const Shared<GuidanceLaw>& aGuidanceLawSPtr, const Interval& anInterval); +void addGuidanceLaw(const Shared<const GuidanceLaw>& aGuidanceLawSPtr, const Interval& anInterval);
82-84: Avoid parallel arrays; store pairs together.Keeping intervals_ and guidanceLaws_ in sync is error‑prone. Prefer a single Array<Pair<Shared, Interval>> to preserve invariants and simplify getters.
-Array<Interval> intervals_; -Array<Shared<GuidanceLaw>> guidanceLaws_; +Array<Pair<Shared<const GuidanceLaw>, Interval>> lawsWithIntervals_;Note: requires small .cpp adjustments.
44-51: Tiny doc polish.Use “pairs” consistently (not “tuples”) to match the C++ type (Pair) and Python docstrings. Also mention returned list preserves insertion order.
Also applies to: 53-57
bindings/python/test/guidance_law/test_heterogeneous_guidance_law.py (3)
33-41: Silence Ruff ARG002 by prefixing unused params with underscores.This keeps signatures intact while avoiding linter noise.
- def calculate_thrust_acceleration_at( - self, - instant: Instant, - position_coordinates: np.array, - velocity_coordinates: np.array, - thrust_acceleration: float, - output_frame: Frame, - ) -> np.array: + def calculate_thrust_acceleration_at( + self, + _instant: Instant, + _position_coordinates: np.ndarray, + _velocity_coordinates: np.ndarray, + _thrust_acceleration: float, + _output_frame: Frame, + ) -> np.ndarray: return np.array([1.0, 2.0, 3.0])
64-72: Apply the same Ruff ARG002 fix to GuidanceLaw2.- def calculate_thrust_acceleration_at( - self, - instant: Instant, - position_coordinates: np.array, - velocity_coordinates: np.array, - thrust_acceleration: float, - output_frame: Frame, - ) -> np.array: + def calculate_thrust_acceleration_at( + self, + _instant: Instant, + _position_coordinates: np.ndarray, + _velocity_coordinates: np.ndarray, + _thrust_acceleration: float, + _output_frame: Frame, + ) -> np.ndarray: return np.array([4.0, 5.0, 6.0])
155-164: Prefer allclose for float arrays.np.array_equal is exact; allclose is safer if implementations change to non‑integers.
- assert np.array_equal( - heterogeneous_guidance_law.calculate_thrust_acceleration_at( - instant=instant, - position_coordinates=np.array([0.0, 0.0, 0.0]), - velocity_coordinates=np.array([0.0, 0.0, 0.0]), - thrust_acceleration=1.0, - output_frame=Frame.GCRF(), - ), - expected_thrust_acceleration, - ) + np.testing.assert_allclose( + heterogeneous_guidance_law.calculate_thrust_acceleration_at( + instant=instant, + position_coordinates=np.array([0.0, 0.0, 0.0]), + velocity_coordinates=np.array([0.0, 0.0, 0.0]), + thrust_acceleration=1.0, + output_frame=Frame.GCRF(), + ), + expected_thrust_acceleration, + rtol=0, atol=0, + )test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.test.cpp (2)
134-149: Add boundary adjacency and order‑agnostic tests.
- Verify behavior when intervals touch at endpoints (e.g., [t0,t1] and [t1,t2]) based on the class’s chosen semantics.
- Add a test where intervals are added out of chronological order to ensure selection logic is order‑independent.
202-204: Minor ctor nit: construct directly.No need to pass a temporary into make_shared.
-const Shared<HeterogeneousGuidanceLaw> heterogeneousGuidanceLaw = - std::make_shared<HeterogeneousGuidanceLaw>(HeterogeneousGuidanceLaw()); +const Shared<HeterogeneousGuidanceLaw> heterogeneousGuidanceLaw = + std::make_shared<HeterogeneousGuidanceLaw>();test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp (2)
47-48: Remove unused Propagator include/using.These appear unused in this test TU.
-#include <OpenSpaceToolkit/Astrodynamics/Trajectory/Propagator.hpp> ... -using ostk::astrodynamics::trajectory::Propagator;Also applies to: 102-103
1250-1253: Avoid exact state equality if small numerical differences are acceptable.If State supports an isNear method, prefer a tolerance‑based check to reduce flakiness across platforms/solvers. Otherwise, consider decomposing and asserting per‑component with tolerances.
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/HeterogeneousGuidanceLaw.cpp (2)
35-47: Constructor input type: verify Python list[tuple] acceptance and document interval adjacency.
- Please confirm that passing a Python
list[tuple[GuidanceLaw, Interval]]to the constructor works end-to-end (relies on Array/Pair casters). If not, add an overload takingpy::listand converting toArray<Pair<...>>.- Also clarify in the docstring whether adjacent closed intervals that share an endpoint are allowed or considered “intersecting.” Match runtime behavior in C++ (addGuidanceLaw uses
intersects).
77-98: Docstring: specify return shape and dtype.Add “Returns a (3,) numpy array of dtype float64.” Keeps parity with other bindings.
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.cpp (3)
30-41: Reserve capacity for parallel arrays.Minor perf: reserve to avoid reallocations when constructing from an input array.
HeterogeneousGuidanceLaw::HeterogeneousGuidanceLaw( const Array<Pair<Shared<GuidanceLaw>, Interval>>& aGuidanceLawWithIntervalArray ) : GuidanceLaw("Heterogeneous Guidance Law"), intervals_(Array<Interval>::Empty()), guidanceLaws_(Array<Shared<GuidanceLaw>>::Empty()) { + intervals_.reserve(aGuidanceLawWithIntervalArray.getSize()); + guidanceLaws_.reserve(aGuidanceLawWithIntervalArray.getSize()); for (const Pair<Shared<GuidanceLaw>, Interval>& guidanceLawWithInterval : aGuidanceLawWithIntervalArray) { this->addGuidanceLaw(guidanceLawWithInterval.first, guidanceLawWithInterval.second); } }
66-74: Interval intersection policy: clarify adjacency.Current check treats shared-endpoint closed intervals as “intersecting” in many interval implementations. If adjacency should be allowed, consider an “overlaps (non-zero measure)” check or explicitly document/verify behavior in tests.
80-100: Lookup complexity: optional sort + binary search.If number of intervals grows, consider storing intervals sorted and using binary search to select the active law. Keep parallel arrays in the same order.
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (5)
160-177: Thrusting detection should ignore mass-flow column and use tolerance.Using
row(i).norm() != 0.0is brittle (float noise) and includes mass-flow; classify thrust by accel magnitude only with epsilon.- for (Size i = 0; i < numberOfStates; i++) + constexpr double kAccelEps = 1e-12; + for (Size i = 0; i < numberOfStates; ++i) { - if (fullSegmentContributions.row(i).norm() != 0.0) // If thrusting + // Consider thrusting when acceleration magnitude > eps (ignore mass flow column) + if (fullSegmentContributions.block<1, 3>(i, 0).norm() > kAccelEps) { // If a new block hasn't started yet, mark its start if (!maneuverStart.isDefined()) { maneuverStart = i; } // If end of segment is thrusting, close last block if (i == numberOfStates - 1) { // Store stop index as i + 1 because you don't get a chance to "close this // block" by seeing the thrust go to zero on the next iteration, since the loop ends on this iteration maneuverBlockStartStopIndices.add(Pair<Size, Size>(maneuverStart, i + 1)); } }
220-229: Use the maneuver block for mass-flow too (minor consistency).Read column 3 from
maneuverContributionBlockrather thanfullSegmentContributions, to keep both taken from the same block.- coordinates(9) = fullSegmentContributions(startStopPair.first + i, 3); + coordinates(9) = maneuverContributionBlock(i, 3);Also applies to: 226-226
504-515: Behavior on angular-offset violation: confirm exception policy and add test.
ConstantThrust::FromManeuverthrows whenmaximumAllowedAngularOffsetis exceeded. Confirm this is the intended user-facing behavior (per PR note) and add a focused unit test that asserts the throw with a deliberately high-offset maneuver.
517-529: Pre-allocate HGL internal storage when number of maneuvers is known.Micro-optimization: reserve space before adding laws.
- Shared<HeterogeneousGuidanceLaw> heterogeneousGuidanceLaw = - std::make_shared<HeterogeneousGuidanceLaw>(HeterogeneousGuidanceLaw()); + Shared<HeterogeneousGuidanceLaw> heterogeneousGuidanceLaw = + std::make_shared<HeterogeneousGuidanceLaw>(); + // Optional: if HGL exposes reserve-equivalent, use it here.
639-673: Solve_: lightweight check for event condition pointer.
anEventConditionSPtrcomes fromeventCondition_(validated), but adding a defensive null check here can prevent misuse in future call sites.+ if (anEventConditionSPtr == nullptr) + { + throw ostk::core::error::runtime::Undefined("Event condition"); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Profile.cpp(1 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw.cpp(2 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/HeterogeneousGuidanceLaw.cpp(1 hunks)bindings/python/test/guidance_law/test_heterogeneous_guidance_law.py(1 hunks)include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.hpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.cpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp(7 hunks)test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.test.cpp(1 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp(13 hunks)
✅ Files skipped from review due to trivial changes (1)
- bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Profile.cpp
🧰 Additional context used
🧬 Code graph analysis (8)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw.cpp (1)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/HeterogeneousGuidanceLaw.cpp (2)
OpenSpaceToolkitAstrodynamicsPy_GuidanceLaw_HeterogeneousGuidanceLaw(20-104)OpenSpaceToolkitAstrodynamicsPy_GuidanceLaw_HeterogeneousGuidanceLaw(20-20)
bindings/python/test/guidance_law/test_heterogeneous_guidance_law.py (2)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.hpp (1)
HeterogeneousGuidanceLaw(48-51)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.cpp (1)
HeterogeneousGuidanceLaw(30-41)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.hpp (1)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw.cpp (2)
anInstant(35-54)anInstant(35-41)
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.cpp (1)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.hpp (3)
HeterogeneousGuidanceLaw(48-51)aGuidanceLawSPtr(62-62)anInstant(73-79)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (5)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.hpp (2)
ConstantThrust(49-49)ConstantThrust(52-52)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (4)
ConstantThrust(30-34)ConstantThrust(36-36)FromManeuver(104-128)FromManeuver(104-108)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.cpp (1)
HeterogeneousGuidanceLaw(30-41)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (6)
aState(227-227)aState(324-329)aName(242-247)aName(257-263)aName(283-291)aDynamicsArray(336-336)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp (8)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.hpp (1)
HeterogeneousGuidanceLaw(48-51)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.cpp (1)
HeterogeneousGuidanceLaw(30-41)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/QLaw.cpp (2)
QLaw(104-121)QLaw(123-123)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameDirection.cpp (1)
LocalOrbitalFrameDirection(18-28)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (3)
LocalOrbitalFrameFactory(166-175)VNC(133-136)VNC(133-133)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (4)
Maneuver(598-613)Maneuver(598-604)ConstantLocalOrbitalFrameDirectionManeuver(615-637)ConstantLocalOrbitalFrameDirectionManeuver(615-623)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (2)
FromManeuver(104-128)FromManeuver(104-108)
test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.test.cpp (3)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.hpp (2)
HeterogeneousGuidanceLaw(48-51)anInstant(73-79)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.cpp (1)
HeterogeneousGuidanceLaw(30-41)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw.cpp (2)
anInstant(35-54)anInstant(35-41)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/HeterogeneousGuidanceLaw.cpp (2)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.hpp (1)
HeterogeneousGuidanceLaw(48-51)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.cpp (7)
HeterogeneousGuidanceLaw(30-41)getGuidanceLawsWithIntervals(43-52)getGuidanceLawsWithIntervals(43-43)addGuidanceLaw(54-78)addGuidanceLaw(54-54)calculateThrustAccelerationAt(80-100)calculateThrustAccelerationAt(80-86)
🪛 Clang (14.0.6)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.hpp
[error] 6-6: 'OpenSpaceToolkit/Core/Container/Array.hpp' file not found
(clang-diagnostic-error)
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.cpp
[error] 3-3: 'OpenSpaceToolkit/Core/Error.hpp' file not found
(clang-diagnostic-error)
test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.test.cpp
[error] 3-3: 'OpenSpaceToolkit/Mathematics/Object/Vector.hpp' file not found
(clang-diagnostic-error)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/HeterogeneousGuidanceLaw.cpp
[error] 3-3: 'OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.hpp' file not found
(clang-diagnostic-error)
🪛 Ruff (0.14.1)
bindings/python/test/guidance_law/test_heterogeneous_guidance_law.py
35-35: Unused method argument: instant
(ARG002)
36-36: Unused method argument: position_coordinates
(ARG002)
37-37: Unused method argument: velocity_coordinates
(ARG002)
38-38: Unused method argument: thrust_acceleration
(ARG002)
39-39: Unused method argument: output_frame
(ARG002)
66-66: Unused method argument: instant
(ARG002)
67-67: Unused method argument: position_coordinates
(ARG002)
68-68: Unused method argument: velocity_coordinates
(ARG002)
69-69: Unused method argument: thrust_acceleration
(ARG002)
70-70: Unused method argument: output_frame
(ARG002)
⏰ 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). (4)
- GitHub Check: Test / Check Python Format
- GitHub Check: Test / Run C++ Unit Tests
- GitHub Check: Test / Build Python bindings
- GitHub Check: Test / Check C++ Format
🔇 Additional comments (2)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw.cpp (1)
6-6: LGTM — binding is registered and included in the submodule.No further changes needed here.
Also applies to: 124-124
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp (1)
161-163: Acknowledged tolerance change.Looser tolerances speed tests; ensure this still bounds solver error acceptably for delta‑V/mass assertions elsewhere.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp (1)
161-162: Clarify the comment wording.The comment says "Reducing tolerances" but the values increased from
1e-12to1e-8, which means you're increasing the tolerance (making it less strict) to speed up solving. Consider rewording for clarity.Apply this diff:
- 1.0e-8, // Reducing tolerances so that the solving doesn't take forever - 1.0e-8, // Reducing tolerances so that the solving doesn't take forever + 1.0e-8, // Relaxed tolerance (less strict) to speed up test execution + 1.0e-8, // Relaxed tolerance (less strict) to speed up test execution
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp(7 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp (9)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.hpp (1)
HeterogeneousGuidanceLaw(48-51)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.cpp (1)
HeterogeneousGuidanceLaw(30-41)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/QLaw.cpp (2)
QLaw(104-121)QLaw(123-123)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameDirection.cpp (1)
LocalOrbitalFrameDirection(18-28)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (3)
LocalOrbitalFrameFactory(166-175)VNC(133-136)VNC(133-133)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Propagator.cpp (3)
Propagator(49-54)Propagator(56-92)Propagator(94-99)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Propagator.hpp (3)
Propagator(63-66)Propagator(78-83)Propagator(88-88)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (4)
Maneuver(598-613)Maneuver(598-604)ConstantLocalOrbitalFrameDirectionManeuver(615-637)ConstantLocalOrbitalFrameDirectionManeuver(615-623)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (2)
FromManeuver(104-128)FromManeuver(104-108)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (6)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.hpp (2)
ConstantThrust(49-49)ConstantThrust(52-52)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (4)
ConstantThrust(30-34)ConstantThrust(36-36)FromManeuver(104-128)FromManeuver(104-108)include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.hpp (1)
HeterogeneousGuidanceLaw(48-51)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/HeterogeneousGuidanceLaw.cpp (1)
HeterogeneousGuidanceLaw(30-41)include/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.hpp (2)
aFrameSPtr(182-185)Maneuver(81-81)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (8)
aFrameSPtr(125-125)aFrameSPtr(161-161)aState(227-227)aState(324-329)aName(242-247)aName(257-263)aName(283-291)aDynamicsArray(336-336)
🔇 Additional comments (14)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp (6)
42-42: LGTM!The includes and using declarations for
HeterogeneousGuidanceLawandPropagatorare necessary for the new constant-LOF-direction maneuver tests.Also applies to: 47-47, 97-97, 102-102
169-174: LGTM!The factory-based
LocalOrbitalFrameFactorysetup and comprehensive QLaw configuration provide a solid foundation for testing constant-LOF-direction maneuvers. The approach aligns with the API design shown in the relevant code snippets.Also applies to: 190-237
337-614: Excellent test coverage!The expanded
extractManeuverstests comprehensively cover error cases (null/undefined frame, empty states, missing/multiple thrusters), edge cases (coast segments, no-thrust scenarios), and integration scenarios (single and multiple maneuvers). The defensive testing approach reduces the risk of runtime errors.
964-976: LGTM!The basic constructor test for
ConstantLocalOrbitalFrameDirectionManeuveris appropriate. Detailed behavior testing is covered in theSolvetest suite.
1057-1150: LGTM!The maximum allowed angular offset tests comprehensively cover all branches: no offset provided, offset provided but not violated, and offset provided and violated. The error message verification at lines 1143-1144 ensures proper user feedback.
1152-1268: Thorough equivalence testing!This test validates the constant-LOF-direction implementation by comparing three approaches:
- Regular QLaw-driven maneuver segment
- Constant-LOF-direction maneuver segment
- Manually-constructed equivalent using
HeterogeneousGuidanceLawThe assertions verify that intervals, final states, and extracted maneuvers align across implementations, providing strong confidence in correctness. While this test is quite long, the thoroughness is valuable for a core feature.
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (8)
8-9: LGTM!The includes and using declarations for
ConstantThrustandHeterogeneousGuidanceLaware necessary for the constant-LOF-direction maneuver implementation.Also applies to: 29-30
133-136: LGTM!The frame validation correctly checks for
nullptrbefore callingisDefined(), preventing potential null pointer dereference. This defensive check improves robustness.
148-148: LGTM!Using the
FindThrusterDynamicsutility instead of inline discovery improves maintainability and reduces duplication.
433-435: LGTM!The new private members are properly initialized with safe defaults (
nullptrandAngle::Undefined()), making the constant-LOF-direction feature opt-in.
502-551: Well-structured two-pass solve implementation!The refactored
solve()method uses a clean two-pass approach:
- Solve with original dynamics to produce draft maneuvers
- Convert maneuvers to constant-thrust guidance via
HeterogeneousGuidanceLawand re-solveThe early returns at lines 505-515 optimize for the common case (no constant-LOF-direction configured), and the dynamic reassembly at lines 538-548 correctly replaces the original thruster with the heterogeneous-guidance-driven thruster while preserving other dynamics.
615-637: LGTM!The factory method uses a two-step construction approach (create standard segment, then configure constant-LOF-direction members) to maintain backwards compatibility and avoid ABI breakage, as noted in past review comments.
639-673: LGTM!Extracting the core solving logic into
Solve_()improves testability and enables the two-pass constant-LOF-direction approach insolve(). The implementation is straightforward and correct.
675-702: LGTM!
FindThrusterDynamicscorrectly identifies exactly oneThrusterin the dynamics array, throwing on both zero and multiple thrusters. Past review comments confirm the multiple-thruster detection logic was fixed and is now correct.
phc1990
deleted the
feat/solve-sequences-with-constant-attitude-bias-maneuvers
branch
Main feature
We currently have no way to "enforce" the Maneuvers that are constant in Local Orbital Frame (LoF) direction when solving a Sequence, which is often a limitation in satellite operations.
✅ This MR allows to add Segments to a Sequence that will produce Maneuvers that are constant in LoF direction.
👉 How it works:
The original Thruster Dynamics are first used to solve the segment and produced "draft" Maneuvers, then, Sequential Guidance Law is created and populated with a Constant Thrust Guidance Law for each Maneuver, using its mean direction from and considering the maximum allowed angular offset if defined.
Additional features
Side-effect features from the main one:
Summary by CodeRabbit
New Features
Tests
Documentation