feat: solve Sequences considering maneuvering constraints #595
Conversation
WalkthroughAdds explicit separation of coast and thruster dynamics in Segment (constructors, accessors, conversions, and solveNextManeuver), extends Sequence with maneuver-duration/separation constraints and a MaximumManeuverDurationStrategy enum, introduces maneuver-aware solving and Solution::extractManeuvers, and updates Python bindings and tests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Sequence
participant solveSegment_
participant Segment
participant GuidanceLaw
participant EventCondition
User->>Sequence: solve(state, maxPropagationDuration)
loop per segment
Sequence->>solveSegment_: solveSegment_(state, segment, maxProp, lastManeuverInterval)
alt segment has thruster (maneuver)
solveSegment_->>Sequence: buildThrusterDynamicsWithManeuverIntervals_(...)
Sequence-->>solveSegment_: constrained thruster dynamics
solveSegment_->>Segment: solve(with assembled dynamics)
Segment->>GuidanceLaw: calculateThrustAccelerationAt(instant)
GuidanceLaw-->>Segment: non-zero thrust during maneuver intervals
else coast-only
solveSegment_->>Segment: solve(with coast dynamics)
end
Segment->>EventCondition: evaluate()
EventCondition-->>Segment: condition result
Segment-->>solveSegment_: Solution + maneuver interval (if any)
solveSegment_->>Sequence: Solution + updated lastManeuverInterval
end
Sequence-->>User: aggregated Solution (with extractManeuvers())
Note over Sequence,Segment: Maneuver-aware solving, interval constraints, and extraction
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #595 +/- ##
==========================================
- Coverage 91.98% 91.98% -0.01%
==========================================
Files 113 113
Lines 10686 10943 +257
==========================================
+ Hits 9830 10066 +236
- Misses 856 877 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.test.cpp (1)
3-36: Build fix: replace the non-existent Tuple header withstd::tuple.
#include <OpenSpaceToolkit/Core/Container/Tuple.hpp>does not exist, so the test suite fails to compile. Since the code only needs tuple semantics for grouping durations and flags, switch tostd::tuple(with<tuple>) and update theusing/Arrayaliases accordingly.Apply this diff:
-#include <OpenSpaceToolkit/Core/Container/Tuple.hpp> +#include <tuple> ... -using ostk::core::container::Tuple; +using DurationInterval = std::tuple<Duration, Duration>; +using ExpectedManeuverInterval = std::tuple<Duration, Duration, bool>;Then replace each
Array<Tuple<Duration, Duration>>withArray<DurationInterval>and eachArray<Tuple<Duration, Duration, bool>>withArray<ExpectedManeuverInterval>, and keep usingstd::getas you already do.
🧹 Nitpick comments (1)
bindings/python/test/trajectory/test_segment.py (1)
547-553: Exercisesolve_next_maneuverhere.This test never calls the new
solve_next_maneuverbinding, so we’re not actually covering the code path the test name promises. Please invokemaneuver_segment.solve_next_maneuver(...)(and keep the assertions) to validate the new API.Apply this diff:
- solution: Segment.Solution = maneuver_segment.solve(state) + solution: Segment.Solution = maneuver_segment.solve_next_maneuver(state)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp(2 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Sequence.cpp(4 hunks)bindings/python/test/trajectory/test_segment.py(2 hunks)bindings/python/test/trajectory/test_sequence.py(3 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp(5 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp(9 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp(11 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp(11 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp(8 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.test.cpp(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp (4)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (2)
Maneuver(661-677)Maneuver(661-667)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (6)
aFrameSPtr(126-126)aFrameSPtr(162-162)aState(245-245)aState(253-254)aState(357-359)aState(369-375)include/OpenSpaceToolkit/Astrodynamics/Trajectory/State.hpp (1)
aFrameSPtr(240-240)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp (3)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.test.cpp (2)
anInstant(95-112)anInstant(95-101)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw.cpp (2)
anInstant(35-54)anInstant(35-41)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (4)
Coast(644-659)Coast(644-649)Maneuver(661-677)Maneuver(661-667)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp (2)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (10)
getThrusterDynamics(549-556)getThrusterDynamics(549-549)toCoastSegment(573-585)toCoastSegment(573-573)toManeuverSegment(587-599)toManeuverSegment(587-587)solve(601-604)solve(601-601)solveNextManeuver(606-609)solveNextManeuver(606-606)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp (2)
solve(395-441)solve(395-395)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.test.cpp (6)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (2)
Maneuver(661-677)Maneuver(661-667)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp (1)
anInstant(82-84)include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw.hpp (1)
anInstant(75-81)src/OpenSpaceToolkit/Astrodynamics/Flight/System/SatelliteSystem.cpp (2)
Default(148-162)Default(148-148)src/OpenSpaceToolkit/Astrodynamics/EventCondition/RealCondition.cpp (2)
DurationCondition(101-112)DurationCondition(101-101)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp (3)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (7)
Maneuver(661-677)Maneuver(661-667)extractManeuvers(141-263)extractManeuvers(141-141)Coast(644-659)Coast(644-649)Solution(49-62)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (7)
aFrameSPtr(126-126)aFrameSPtr(162-162)aState(245-245)aState(253-254)aState(357-359)aState(369-375)Solution(80-86)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp (10)
aFrameSPtr(114-114)aMinimumManeuverDuration(239-239)aMaximumManeuverDuration(224-224)aMinimumManeuverSeparation(234-234)aMaximumManeuverDurationStrategy(229-229)aState(267-267)aState(275-279)aState(300-305)aSegment(315-317)Solution(70-70)
bindings/python/test/trajectory/test_segment.py (5)
bindings/python/test/trajectory/test_sequence.py (3)
thruster_dynamics(208-214)coast_duration_segment(235-245)state(140-162)include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp (2)
Thruster(40-44)Thruster(46-46)src/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.cpp (2)
Thruster(31-41)Thruster(43-43)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (2)
Segment(340-349)Solution(80-86)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (6)
Segment(450-510)Coast(644-659)Coast(644-649)Solution(49-62)solve(601-604)solve(601-601)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (2)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp (5)
aState(267-267)aState(275-279)aState(300-305)anEventConditionSPtr(254-254)anEventConditionSPtr(260-260)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.hpp (1)
aType(116-118)
bindings/python/test/trajectory/test_sequence.py (3)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.test.cpp (10)
sequence(521-521)sequence(525-525)sequence(529-529)sequence(533-535)sequence(539-545)sequence(553-559)sequence(600-606)sequence(626-632)sequence(675-682)sequence(688-695)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp (1)
Sequence(165-172)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp (1)
Sequence(199-258)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Sequence.cpp (3)
bindings/python/test/trajectory/test_sequence.py (1)
sequence(312-332)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (2)
extractManeuvers(141-263)extractManeuvers(141-141)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp (16)
extractManeuvers(100-111)extractManeuvers(100-100)getMaximumManeuverDuration(282-285)getMaximumManeuverDuration(282-282)getMaximumManeuverDurationStrategy(302-305)getMaximumManeuverDurationStrategy(302-302)getMinimumManeuverSeparation(297-300)getMinimumManeuverSeparation(297-297)setMaximumManeuverDuration(307-328)setMaximumManeuverDuration(307-307)setMaximumManeuverDurationStrategy(368-373)setMaximumManeuverDurationStrategy(368-370)setMinimumManeuverDuration(330-351)setMinimumManeuverDuration(330-330)setMinimumManeuverSeparation(353-366)setMinimumManeuverSeparation(353-353)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (2)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (13)
aThrusterDynamics(238-238)aName(231-231)aName(269-274)aName(284-290)aName(310-318)aState(245-245)aState(253-254)aState(357-359)aState(369-375)anOutputStream(168-168)anOutputStream(175-175)anOutputStream(189-189)anOutputStream(260-260)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Propagator.hpp (5)
aNumericalSolver(234-234)aState(180-180)aState(191-193)aState(203-203)aDynamicsArray(147-147)
🪛 Clang (14.0.6)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.test.cpp
[error] 3-3: 'OpenSpaceToolkit/Core/Container/Tuple.hpp' file not found
(clang-diagnostic-error)
🪛 Ruff (0.14.3)
bindings/python/test/trajectory/test_segment.py
338-338: Unused method argument: name
(ARG002)
340-340: Unused method argument: maneuver_segment
(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
phc1990
force-pushed
the
feat/add-sequence-solving-capabilities
branch
from
9a86ac7 to
782f625
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp (1)
7-40: Resolve missing Tuple header
OpenSpaceToolkit/Core/Container/Tuple.hppisn’t shipped with the SDK, and Clang now errors with'OpenSpaceToolkit/Core/Container/Tuple.hpp' file not found. Switch the alias over tostd::tuple(or another available container) so this header and every consumer builds again.Apply this diff:
@@ -#include <OpenSpaceToolkit/Core/Container/Tuple.hpp> +#include <tuple> @@ -using ostk::core::container::Tuple; +template <typename... Args> +using Tuple = std::tuple<Args...>;test/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.test.cpp (1)
1-35: Use std::tuple instead of non-existent core TupleThis test also tries to include
OpenSpaceToolkit/Core/Container/Tuple.hpp, which isn’t available, so the test target fails to compile. Mirror the production fix by switching tostd::tuple(with a template alias if you want to keep theTuple<...>spelling).Apply this diff:
@@ -#include <OpenSpaceToolkit/Core/Container/Tuple.hpp> +#include <tuple> @@ -using ostk::core::container::Tuple; +template <typename... Args> +using Tuple = std::tuple<Args...>;
♻️ Duplicate comments (2)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp (2)
541-559: Critical: Coast propagation not clamped to segment maximum instant.This issue was flagged in a previous review and remains unresolved. When falling into the coast fallback (line 544-545),
coastToInstantis used directly without clamping againstmaximumInstant. If the required minimum-separation coast pushes past the segment's maximum propagation duration, the code will propagate beyond the caller-specified cap, potentially causing runtime explosion or incorrect behavior.Apply this diff to clamp the coast target and validate the duration:
- Segment::Solution coastSegmentSolution = - segmentToSolve.solve(states.accessLast(), coastToInstant - states.accessLast().accessInstant()); + const Instant targetCoastInstant = std::min(coastToInstant, maximumInstant); + const Duration coastDuration = targetCoastInstant - states.accessLast().accessInstant(); + + if (!coastDuration.isDefined() || !coastDuration.isStrictlyPositive()) + { + break; + } + + Segment::Solution coastSegmentSolution = segmentToSolve.solve(states.accessLast(), coastDuration);
686-692: Clamp coastToInstant against maximumInstant to prevent over-propagation.After accepting a maneuver, the code computes
coastToInstantfor the minimum separation constraint (line 690) but does not clamp it againstmaximumInstant. This can cause the next coast iteration to exceed the segment's maximum propagation duration.Apply this diff:
segmentToSolve = aSegment.toCoastSegment(aSegment.getName() + " (Coast - Minimum Maneuver Separation Constraint)"); coastToInstant = coastToInstant.isDefined() ? std::max(states.accessLast().accessInstant() + minimumManeuverSeparation_, coastToInstant) : states.accessLast().accessInstant() + minimumManeuverSeparation_; + coastToInstant = std::min(coastToInstant, maximumInstant);
🧹 Nitpick comments (1)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp (1)
211-214: Document the hardcoded 30-second default for minimum maneuver separation.The default value of 30 seconds for
minimumManeuverSeparation_is hardcoded without clear justification or documentation. Consider adding a comment explaining why this specific value was chosen, or making it a named constant.Apply this diff to improve clarity:
+ // Default minimum separation of 30 seconds between maneuvers minimumManeuverSeparation_(Duration::Seconds(30.0)),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp(4 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Sequence.cpp(4 hunks)bindings/python/test/trajectory/test_segment.py(3 hunks)bindings/python/test/trajectory/test_sequence.py(3 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp(6 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp(9 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp(12 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp(11 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp(12 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.test.cpp(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Sequence.cpp
🧰 Additional context used
🧬 Code graph analysis (9)
bindings/python/test/trajectory/test_sequence.py (2)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp (1)
Sequence(165-172)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp (1)
Sequence(199-258)
bindings/python/test/trajectory/test_segment.py (4)
bindings/python/test/trajectory/test_sequence.py (3)
segment_solution(336-348)dynamics(166-167)state(140-162)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (6)
Segment(464-524)Solution(49-62)Coast(663-678)Coast(663-668)solve(620-623)solve(620-620)include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp (2)
Thruster(40-44)Thruster(46-46)src/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.cpp (2)
Thruster(31-41)Thruster(43-43)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp (3)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.test.cpp (2)
anInstant(95-112)anInstant(95-101)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (5)
Solution(49-62)Coast(663-678)Coast(663-668)Maneuver(680-696)Maneuver(680-686)src/OpenSpaceToolkit/Astrodynamics/Trajectory/State/CoordinateSubset.cpp (2)
Mass(110-114)Mass(110-110)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp (3)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (2)
Maneuver(680-696)Maneuver(680-686)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (6)
aFrameSPtr(130-130)aFrameSPtr(166-166)aState(253-253)aState(261-262)aState(365-367)aState(377-383)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.test.cpp (8)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (2)
Maneuver(680-696)Maneuver(680-686)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp (2)
anInstant(122-139)anInstant(122-128)include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp (1)
anInstant(82-84)include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw.hpp (1)
anInstant(75-81)src/OpenSpaceToolkit/Astrodynamics/Flight/System/PropulsionSystem.cpp (2)
Default(122-128)Default(122-122)src/OpenSpaceToolkit/Astrodynamics/Flight/System/SatelliteSystem.cpp (2)
Default(148-162)Default(148-148)src/OpenSpaceToolkit/Astrodynamics/EventCondition/RealCondition.cpp (2)
DurationCondition(101-112)DurationCondition(101-101)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp (1)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (14)
getThrusterDynamics(119-149)getThrusterDynamics(119-119)getThrusterDynamics(568-575)getThrusterDynamics(568-568)getCoastDynamics(553-556)getCoastDynamics(553-553)toCoastSegment(592-604)toCoastSegment(592-592)toManeuverSegment(606-618)toManeuverSegment(606-606)solve(620-623)solve(620-620)solveNextManeuver(625-628)solveNextManeuver(625-625)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (3)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (9)
aThrusterDynamics(246-246)aName(239-239)aName(277-282)aName(292-298)aName(318-326)aState(253-253)aState(261-262)aState(365-367)aState(377-383)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Propagator.hpp (5)
aNumericalSolver(234-234)aState(180-180)aState(191-193)aState(203-203)aDynamicsArray(147-147)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (2)
FromManeuver(104-128)FromManeuver(104-108)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp (3)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (7)
Maneuver(680-696)Maneuver(680-686)extractManeuvers(173-277)extractManeuvers(173-173)Coast(663-678)Coast(663-668)Solution(49-62)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (7)
aFrameSPtr(130-130)aFrameSPtr(166-166)aState(253-253)aState(261-262)aState(365-367)aState(377-383)Solution(80-86)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp (6)
aFrameSPtr(114-114)aState(267-267)aState(275-279)aState(300-305)aSegment(315-317)Solution(70-70)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (4)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp (5)
aState(267-267)aState(275-279)aState(300-305)anEventConditionSPtr(254-254)anEventConditionSPtr(260-260)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Propagator.hpp (4)
aState(180-180)aState(191-193)aState(203-203)aNumericalSolver(234-234)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.hpp (1)
aType(116-118)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.hpp (4)
aType(93-95)aType(102-104)aType(111-111)aType(122-122)
🪛 Clang (14.0.6)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.test.cpp
[error] 3-3: 'OpenSpaceToolkit/Core/Container/Tuple.hpp' file not found
(clang-diagnostic-error)
🪛 Ruff (0.14.3)
bindings/python/test/trajectory/test_segment.py
241-241: Unused method argument: dynamics
(ARG002)
242-242: Unused method argument: state
(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 / Build Development Image (linux/arm64, ubuntu-24.04-arm)
🔇 Additional comments (15)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp (7)
100-111: LGTM! Straightforward delegation to segment solutions.The method correctly iterates through segment solutions and aggregates their extracted maneuvers.
216-226: LGTM! Proper validation with clear error messages.The validation checks ensure the maximum propagation duration is both defined and strictly positive, using consistent error types.
307-373: LGTM! Thorough validation in setters.All setter methods properly validate inputs (defined, positive, and consistency checks between min/max constraints) with clear error messages.
562-577: LGTM! Proper duration clamping for maneuver solving.The call to
solveNextManeuvercorrectly clamps the duration usingmaximumInstant - states.accessLast().accessInstant(), preventing propagation beyond the segment cap.
588-600: LGTM! Correct handling of minimum maneuver duration constraint.When a candidate maneuver is below the minimum duration threshold, the code correctly skips it by converting to a coast segment and continuing the loop.
602-671: LGTM! Comprehensive maximum maneuver duration strategy handling.All four strategies (Fail, Skip, Slice, Center) are correctly implemented with appropriate logging and error handling. The use of
ToBeImplementedfor the default case ensures future strategies are not silently ignored.
711-729: LGTM! Clean helper for building constrained thruster dynamics.The method correctly constructs a heterogeneous guidance law from the provided maneuver intervals and wraps it in a new Thruster instance.
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (8)
119-149: LGTM! Robust thruster dynamics extraction with proper validation.The method correctly validates segment type, searches for exactly one Thruster in the dynamics array, and throws clear errors for multiple or missing thrusters.
464-524: LGTM! Excellent separation of coast and thruster dynamics with thorough validation.The constructor properly:
- Separates coast dynamics from thruster dynamics
- Validates that Coast segments have no thruster
- Validates that Maneuver segments have a defined thruster
- Prevents Thruster dynamics from appearing in the coast dynamics array (lines 514-523)
This clear separation simplifies the API and prevents misconfiguration.
543-575: LGTM! Clear accessors for dynamics separation.The three methods provide well-defined access patterns:
getDynamics()returns the combined set (coast + optional thruster)getCoastDynamics()returns only coast dynamicsgetThrusterDynamics()returns thruster (with type check)
592-618: LGTM! Convenient conversion helpers preserve all segment properties.Both
toCoastSegmentandtoManeuverSegmentcorrectly copy all relevant properties (event condition, coast dynamics, numerical solver, frame factory, angular offset) while adjusting only the segment type and thruster dynamics.
620-628: LGTM! Clean delegation to internal solve method.The public
solveandsolveNextManeuvermethods correctly delegate tosolve_with appropriateallowMultipleManeuversflags.
720-764: LGTM! Proper constant-direction maneuver enforcement.The method correctly:
- Solves with the original dynamics
- If constant local orbital frame direction is required, extracts maneuvers
- Computes constant thrust directions via
ConstantThrust::FromManeuver- Re-solves with heterogeneous guidance law containing the constant directions
This ensures maneuvers maintain a constant direction in the specified frame.
774-779: LGTM! Clean dynamics assembly.The code correctly combines coast and thruster dynamics into a single array for propagation.
766-863: Verify edge case handling in the reevaluation logic with explicit test coverage.The reevaluation logic (lines 835–854) includes defensive guards for edge cases but lacks specific test coverage:
Empty propagation result: Correctly handled by the
if (!propagatedStates.isEmpty())guard (line 850). If propagation fails,finalConditionIsSatisfiedretains its value from the initial condition solution, which is safe but may not reflect intended behavior in all scenarios.Thrust off vs. event condition: The
LogicalCondition::Orcombines both conditions, then reevaluation checks the original condition on the propagated state. This logic appears sound, but no tests exercise this path.State references: Using
secondToLastStateas the previous state inisSatisfied()(line 852) is semantically correct, as event conditions typically require a previous state to detect condition crossings.Gap: No specific tests exist for the reevaluation path or these edge cases. The parametrized tests at lines 1391 and 1920 verify basic properties (
solution.conditionIsSatisfied == true) but do not exercise the reevaluation logic. Recommend adding dedicated tests for:
- Scenarios where propagation returns empty states
- Cases where thrust turns off exactly when the event condition is satisfied
- Validation that
finalConditionIsSatisfiedcorrectly reflects the original event condition after reevaluation
|
|
||
| /// @brief Get thruster dynamics | ||
| /// @return Thruster dynamics | ||
| Shared<Thruster> getThrusterDynamics() const; |
There was a problem hiding this comment.
Nit: Perhaps? Only if it we don't already have getThrusterDynamics() anywhere else in public which i think we might
| Shared<Thruster> getThrusterDynamics() const; | |
| Shared<Thruster> getThrusterDynamic() const; |
There was a problem hiding this comment.
I see the rationale, and I had the same doubt, but it looks like we're using Dynamics form for both singular or plural.
(the actual class is already called Dynamics as opposed to Dynamic)
| /// @param aName Optional name for the new segment. If not provided, uses the current segment's name | ||
| /// @return A new coast segment | ||
| Segment toCoastSegment(const String& aName = String::Empty()) const; | ||
|
|
||
| /// @brief Build a maneuver segment from the current instance. | ||
| /// | ||
| /// @param aThrusterDynamics The thruster dynamics for the new maneuver segment | ||
| /// @param aName Optional name for the new segment. If not provided, uses the current segment's name | ||
| /// @return A new maneuver segment |
There was a problem hiding this comment.
Nit
| /// @param aName Optional name for the new segment. If not provided, uses the current segment's name | |
| /// @return A new coast segment | |
| Segment toCoastSegment(const String& aName = String::Empty()) const; | |
| /// @brief Build a maneuver segment from the current instance. | |
| /// | |
| /// @param aThrusterDynamics The thruster dynamics for the new maneuver segment | |
| /// @param aName Optional name for the new segment. If not provided, uses the current segment's name | |
| /// @return A new maneuver segment | |
| /// @param aName (optional) name for the new segment. If not provided, uses the current segment's name | |
| /// @return A new coast segment | |
| Segment toCoastSegment(const String& aName = String::Empty()) const; | |
| /// @brief Build a maneuver segment from the current instance. | |
| /// | |
| /// @param aThrusterDynamics The thruster dynamics for the new maneuver segment | |
| /// @param aName (optional) name for the new segment. If not provided, uses the current segment's name | |
| /// @return A new maneuver segment |
| /// @brief Build a maneuver segment from the current instance. | ||
| /// | ||
| /// @param aThrusterDynamics The thruster dynamics for the new maneuver segment | ||
| /// @param aName Optional name for the new segment. If not provided, uses the current segment's name |
There was a problem hiding this comment.
Are there any issues with uniqueness if it uses the current segments name? Do we kind of use the name as an "id" for the segment or not really?
There was a problem hiding this comment.
AFAIK it's purely decorative
| /// @param aState Initial state for the segment | ||
| /// @param maximumPropagationDuration Maximum duration for propagation. Defaults to 30 days | ||
| /// @return A Solution representing the result of the solve | ||
| Solution solveNextManeuver(const State& aState, const Duration& maximumPropagationDuration = Duration::Days(30.0)) |
There was a problem hiding this comment.
| Solution solveNextManeuver(const State& aState, const Duration& maximumPropagationDuration = Duration::Days(30.0)) | |
| Solution solveToNextManeuver(const State& aState, const Duration& maximumPropagationDuration = Duration::Days(30.0)) |
There was a problem hiding this comment.
Likely to be amended so that this function (if public) does not "break" the contract to return a solution that has either reached its condition or its max. propagation time.
| /// @param maximumPropagationDuration The maximum propagation duration | ||
| /// @param aDynamicsArray The dynamics array | ||
| /// @param anEventConditionSPtr The event condition | ||
| /// @param allowMultipleManeuvers True if multiple maneuvers are allowed |
There was a problem hiding this comment.
| /// @param allowMultipleManeuvers True if multiple maneuvers are allowed | |
| /// @param allowMultipleManeuvers True if multiple maneuvers are allowed per Segment Solution |
| break; | ||
| } | ||
|
|
||
| segmentToSolve = aSegment; |
There was a problem hiding this comment.
Wait counldn't you end up in an infinite loop of coast segments here because you're always resetting the segmentToSolve to the original aSegment? Or its fine cause you always start from a newly added state and so should always make progress towards the end of the segment?
Also perhaps make the naming of the variables clearer so that we know we are trying to define the next segment to solve (in the next loop iteration) and not the current one? Below line 563
There was a problem hiding this comment.
Variables renamed ✅
Or its fine cause you always start from a newly added state and so should always make progress towards the end of the segment?
Correct, you always make progress as the initial state used it's always the last one
| } | ||
| } | ||
|
|
||
| continue; |
There was a problem hiding this comment.
I think this statement is pointless if each of your 4 cases above have a break/continue statement
| continue; |
There was a problem hiding this comment.
the breaks are for the switch clause, which land here, so this continue is necessary to tell the outer while loop to jump to the next segmentToSolve (which has been set in the switch based on the max maneuver duration strategy)
| shortenedManeuverThruster, aSegment.getName() + " (Sliced Maneuver)" | ||
| ); | ||
| coastToInstant = candidateManeuver.getInterval().getStart() + maximumManeuverDuration_; | ||
| break; |
There was a problem hiding this comment.
Why are you breaking here instead of continue? The segment condition still isn't satisfied at this point right? So you would be stopping the solving early instead of trying to finish solving the segment but with a sliced maneuver
There was a problem hiding this comment.
this break is at Switch level, not outer while loop
| shortenedManeuverThruster, aSegment.getName() + " (Centered Maneuver)" | ||
| ); | ||
| coastToInstant = candidateManeuver.getInterval().getEnd(); | ||
| break; |
There was a problem hiding this comment.
Why are you breaking here instead of continue? The segment condition still isn't satisfied at this point right? So you would be stopping the solving early instead of trying to finish solving the segment but with a centered maneuver
There was a problem hiding this comment.
this break is at Switch level, not outer while loop
| if (conditionIsSatisfied) | ||
| { | ||
| break; | ||
| } |
There was a problem hiding this comment.
It seems that this statement is placed incorrectly in the code? It should go right after somewhere that changes its value, like on line 565
There was a problem hiding this comment.
The subsegment is solved, and this variable is updated.
However, that, it being ture, is not sufficient to exit.
The condition coudl have been satisfied, but the maneuver could be breaking the operational constraints, so there is more solving to do (shortening, skipping, etc.)
So it is placed after the maneuver acceptance check.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
bindings/python/test/trajectory/test_segment.py (1)
206-221: Consider using spread syntax for list concatenation.The fixture is functionally correct. However, line 214 can use more idiomatic Python syntax.
As per static analysis hints:
- dynamics=dynamics + [thruster_dynamics], + dynamics=[*dynamics, thruster_dynamics],src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp (2)
609-609: Document or make configurable thesubsegmentMargin_constant.The code uses
subsegmentMargin_(defined as 10 seconds in the header) as a safety margin when scheduling coast intervals. This magic number is neither documented in the API nor configurable by users. Consider:
- Adding documentation explaining why this margin is needed and how the value was chosen, or
- Making it a configurable parameter if different use cases might require different margins.
736-738: Verify const_pointer_cast is necessary.The code uses
std::const_pointer_castto remove const qualification from the original guidance law. This suggests thatHeterogeneousGuidanceLaw::addGuidanceLawdoesn't accept const-qualified guidance laws, even though it doesn't need to modify the original.Consider whether the API of
HeterogeneousGuidanceLawcould be improved to acceptconst Shared<const GuidanceLaw>to avoid this cast, which weakens type safety.include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp (1)
327-327: Consider makingsubsegmentMargin_configurable or better documented.The hardcoded 10-second margin is used internally but lacks:
- Public documentation explaining its purpose (spacing buffer between constraint checks)
- Getter/setter access for users who might need different values
- Justification for the chosen value
Additionally, as suggested in past comments, consider encapsulating all constraint-related members (
minimumManeuverDuration_,minimumManeuverSeparation_,maximumManeuverDuration_,maximumManeuverDurationStrategy_,subsegmentMargin_) into a dedicatedManeuverConstraintsstruct to improve code organization and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bindings/python/test/trajectory/test_segment.py(5 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp(9 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
bindings/python/test/trajectory/test_segment.py (6)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (2)
Segment(348-357)Solution(80-86)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (8)
Segment(464-524)Solution(49-62)Maneuver(680-696)Maneuver(680-686)Coast(663-678)Coast(663-668)solve(620-623)solve(620-620)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp (3)
Solution(27-31)solve(395-441)solve(395-395)include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp (2)
Thruster(40-44)Thruster(46-46)src/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.cpp (2)
Thruster(31-41)Thruster(43-43)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp (3)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (7)
Maneuver(680-696)Maneuver(680-686)extractManeuvers(173-277)extractManeuvers(173-173)Coast(663-678)Coast(663-668)Solution(49-62)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp (6)
aFrameSPtr(114-114)aState(267-267)aState(275-279)aState(300-305)aSegment(315-317)Solution(70-70)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (7)
aFrameSPtr(130-130)aFrameSPtr(166-166)aState(253-253)aState(261-262)aState(365-367)aState(377-383)Solution(80-86)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp (3)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (2)
Maneuver(680-696)Maneuver(680-686)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (6)
aFrameSPtr(130-130)aFrameSPtr(166-166)aState(253-253)aState(261-262)aState(365-367)aState(377-383)
🪛 Ruff (0.14.3)
bindings/python/test/trajectory/test_segment.py
214-214: Consider [*dynamics, thruster_dynamics] instead of concatenation
Replace with [*dynamics, thruster_dynamics]
(RUF005)
⏰ 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 (11)
bindings/python/test/trajectory/test_segment.py (6)
191-203: LGTM!The fixture correctly constructs a Coast-type Segment.Solution with appropriate type hints and a descriptive name.
258-264: LGTM!The test now correctly uses the
maneuver_segment_solutionfixture, which contains thruster dynamics. The assertion is appropriate.
332-340: LGTM!The test correctly verifies that both coast and maneuver segments maintain their coast dynamics, which aligns with the new separation of coast and thruster dynamics in the Segment implementation.
348-354: LGTM!The test appropriately verifies that the maneuver segment returns the expected thruster dynamics.
361-371: LGTM!The test adequately covers the segment-to-coast conversion functionality, including both with and without the optional name parameter.
372-389: LGTM!The test correctly covers segment-to-maneuver conversion. The previously flagged unused fixtures have been appropriately removed.
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp (4)
100-111: LGTM! Maneuver extraction correctly flattens results.The implementation properly collects maneuvers from all segment solutions into a single flat array, addressing the concern about nested arrays.
406-419: LGTM! Proper integration of maneuver interval tracking.The solve method correctly initializes and threads
lastManeuverIntervalthrough segment solving iterations to enforce separation constraints.
451-471: LGTM! Consistent maneuver interval tracking.The implementation mirrors the pattern in
solve(), correctly propagating maneuver intervals across segment boundaries.
555-561: Code change verified: Coast propagation clamping is correctly implemented.The verification confirms the review comment's claim. The code at lines 557-560 properly clamps the coast propagation duration using
std::min()to ensure it never exceeds the remaining time tomaximumInstant. This addresses the prior concern by guaranteeing propagation will not extend beyond the segment cap.The implementation is correct:
maximumInstantis defined at line 529- The while loop guards against propagation beyond this bound (line 550)
- The
std::min()ensures the solver receives a duration that respects both the desired coast instant and the maximum time limitinclude/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp (1)
49-58: LGTM! Clear enum definition for maximum duration strategies.The
MaximumManeuverDurationStrategyenum provides well-documented options. The choice of "Slice" over "Split" appropriately conveys that the maneuver is truncated from the start rather than divided into equal parts.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp (1)
211-214: Address the inconsistent handling of undefined durations.This issue was raised in previous reviews but remains unresolved.
minimumManeuverSeparation_is initialized toDuration::Undefined()(line 212), butsetMinimumManeuverSeparation(lines 355-357) rejects undefined values. This creates an inconsistency withsetMinimumManeuverDurationandsetMaximumManeuverDuration, which both accept undefined durations.Either allow
setMinimumManeuverSeparationto accept undefined values for consistency, or initializeminimumManeuverSeparation_to a defined default value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp(11 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.test.cpp(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp (2)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (5)
Maneuver(680-696)Maneuver(680-686)extractManeuvers(173-277)extractManeuvers(173-173)Solution(49-62)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp (10)
aFrameSPtr(114-114)aMinimumManeuverDuration(239-239)aMaximumManeuverDuration(224-224)aMinimumManeuverSeparation(234-234)aMaximumManeuverDurationStrategy(229-229)aState(267-267)aState(275-279)aState(300-305)aSegment(315-317)Solution(70-70)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.test.cpp (6)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (2)
Maneuver(680-696)Maneuver(680-686)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp (1)
anInstant(82-84)include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw.hpp (1)
anInstant(75-81)src/OpenSpaceToolkit/Astrodynamics/Flight/System/SatelliteSystem.cpp (2)
Default(148-162)Default(148-148)src/OpenSpaceToolkit/Astrodynamics/EventCondition/RealCondition.cpp (2)
DurationCondition(101-112)DurationCondition(101-101)
🪛 Clang (14.0.6)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.test.cpp
[error] 3-3: 'OpenSpaceToolkit/Core/Container/Tuple.hpp' file not found
(clang-diagnostic-error)
⏰ 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)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp (6)
7-7: LGTM! Includes are appropriate for new functionality.The new includes for
ToBeImplementederror handling andHeterogeneousGuidanceLawsupport the maneuver-aware solving capabilities introduced in this PR.Also applies to: 12-12, 22-23
100-111: LGTM! Maneuver extraction correctly aggregates across segments.The implementation properly delegates to each segment's
extractManeuversand aggregates the results into a flat array.
307-351: LGTM! Setter validation correctly enforces constraints.The validation logic properly:
- Allows undefined values to be set
- Enforces strictly positive durations when defined
- Cross-validates minimum and maximum maneuver durations to ensure consistency
545-567: LGTM! Coast propagation is now properly clamped.The coast duration is correctly clamped to prevent propagation beyond
maximumInstant(lines 552-555), addressing the concern raised in previous reviews. The implementation properly handles the coast segment solving and state aggregation.
594-678: LGTM! Maneuver constraint handling is well-structured.The implementation correctly:
- Filters maneuvers based on minimum duration requirements
- Handles maximum duration violations with configurable strategies (Skip, Slice, Center)
- Updates the next subsegment to solve and continues the loop appropriately
- Uses
breakto exit the switch statement, followed bycontinueto return to the while loop
721-739: Same const_cast concern applies here.Similar to line 687, this method uses
std::const_pointer_caston line 732 to removeconstqualification. This should be reviewed alongside the earlier instance to ensure const-correctness is maintained.test/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.test.cpp (6)
86-116: LGTM! Test helper provides precise control over maneuver timing.The
CustomGuidanceLawimplementation is clean and well-suited for testing maneuver constraints. It enables deterministic thrust behavior within specified intervals.
339-485: LGTM! Comprehensive test coverage for maneuver extraction.The test validates:
- Empty results when no maneuvers occur
- Correct aggregation across segments and repetitions
- Consistency between segment-level and sequence-level extraction
733-959: LGTM! Thorough validation of constraint setters.The tests comprehensively validate:
- Valid and invalid duration values
- Cross-constraints between minimum and maximum durations
- Proper error messages for validation failures
- Strategy enumeration
1335-1593: LGTM! Excellent parameterized test design.The test suite systematically validates all constraint scenarios:
- Minimum maneuver duration filtering
- Minimum maneuver separation enforcement
- Maximum maneuver duration strategies (Skip, Slice, Center)
The use of configurable tolerance thresholds appropriately handles numerical integration uncertainty. Testing both
solve()andsolveToCondition()ensures API consistency.
1595-1865: LGTM! Comprehensive edge case coverage.The tests validate critical scenarios:
- Proper exception handling when constraints are violated with the Fail strategy
- Minimum separation enforcement across segment boundaries
- Minimum separation enforcement across sequence repetitions
The timing validations with appropriate tolerances account for numerical integration behavior.
3-3: Static analysis false positive - ignore.The missing header error is a false positive. The include path is correct for the OpenSpaceToolkit project structure, and the tests compile successfully. The static analyzer likely lacks proper include path configuration.
| // Maneuver has been accepted | ||
| BOOST_LOG_TRIVIAL(debug) << "Maneuver accepted." << std::endl; | ||
| unifiedStates.add( | ||
| Array<State>(maneuverSubsegmentSolution.states.begin() + 1, maneuverSubsegmentSolution.states.end()) | ||
| ); | ||
| unifiedManeuverIntervals.add(nextManeuverCandidate.getInterval()); | ||
| unifiedGuidanceLaw->addGuidanceLaw( | ||
| std::const_pointer_cast<GuidanceLaw>(maneuverSubsegmentSolution.getThrusterDynamics()->getGuidanceLaw()), | ||
| nextManeuverCandidate.getInterval() | ||
| ); | ||
|
|
||
| if (subsegmentConditionIsSatisfied) | ||
| { | ||
| break; | ||
| } | ||
|
|
||
| // Account for minimum maneuver separation | ||
| BOOST_LOG_TRIVIAL(debug) << "Accounting for minimum maneuver separation." << std::endl; | ||
| nextSubsegmentToSolve = | ||
| aSegment.toCoastSegment(aSegment.getName() + " (Coast - Minimum Maneuver Separation Constraint)"); | ||
| if (minimumManeuverSeparation_.isDefined()) | ||
| { | ||
| updateNextCoastInstant(unifiedManeuverIntervals.accessLast().getEnd() + minimumManeuverSeparation_); | ||
| } | ||
| } | ||
|
|
||
| Array<Shared<Dynamics>> unifiedDynamics = aSegment.getCoastDynamics(); | ||
| unifiedDynamics.add(std::make_shared<Thruster>( | ||
| aSegment.getThrusterDynamics()->getSatelliteSystem(), | ||
| unifiedGuidanceLaw, | ||
| aSegment.getName() + " (Maneuvering Constraints)" | ||
| )); | ||
|
|
||
| return { | ||
| Segment::Solution( | ||
| aSegment.getName(), unifiedDynamics, unifiedStates, subsegmentConditionIsSatisfied, Segment::Type::Maneuver | ||
| ), | ||
| unifiedManeuverIntervals.isEmpty() ? aLastManeuverInterval : unifiedManeuverIntervals.accessLast() | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Review the const_cast usage.
The std::const_pointer_cast on line 687 removes const qualification, which could be problematic if the original GuidanceLaw is truly immutable. Verify that HeterogeneousGuidanceLaw::addGuidanceLaw requires a mutable pointer and that this cast doesn't violate const-correctness assumptions elsewhere in the codebase.
Otherwise, the maneuver acceptance logic and state/interval tracking are correctly implemented.
🏁 Script executed:
#!/bin/bash
# Check the signature of HeterogeneousGuidanceLaw::addGuidanceLaw to verify if it requires a non-const pointer
ast-grep --pattern 'class HeterogeneousGuidanceLaw {
$$$
addGuidanceLaw($$$) $$$
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for HeterogeneousGuidanceLaw class definition
rg -n "class HeterogeneousGuidanceLaw" -A 10 -B 2Length of output: 1764
🏁 Script executed:
#!/bin/bash
# Search for addGuidanceLaw method signature
rg -n "addGuidanceLaw" -B 2 -A 5Length of output: 13201
🏁 Script executed:
#!/bin/bash
# Search for where const_pointer_cast is used in the context of GuidanceLaw
rg -n "const_pointer_cast.*GuidanceLaw" -B 3 -A 3Length of output: 1418
🏁 Script executed:
#!/bin/bash
# Verify getThrusterDynamics and getGuidanceLaw return types
rg -n "getGuidanceLaw\(\)" -B 2 -A 2 --type=cppLength of output: 4855
🏁 Script executed:
#!/bin/bash
# Check Thruster constructor signature to see what it expects
rg -n "Thruster\(" -B 2 -A 3 --type=hppLength of output: 27
🏁 Script executed:
#!/bin/bash
# More specific search for addGuidanceLaw implementation
rg -n "void.*addGuidanceLaw|addGuidanceLaw.*Shared" -B 1 -A 4Length of output: 1514
const_cast violates const-correctness guarantees from getGuidanceLaw().
The getter getGuidanceLaw() intentionally returns Shared<const GuidanceLaw>, signaling the guidance law object should not be modified. The std::const_pointer_cast on line 687 circumvents this safety guarantee by casting to a mutable pointer, allowing potential modification of an object the designer intended to be immutable. This pattern also appears on line 732 and should be resolved consistently.
Consider why the const qualifier exists in the return type and whether addGuidanceLaw should accept const Shared<const GuidanceLaw>& instead of requiring a mutable pointer.
phc1990
force-pushed
the
feat/add-sequence-solving-capabilities
branch
from
faf0df6 to
61e9629
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
bindings/python/test/trajectory/test_segment.py (1)
581-586: Exercise the solve_next_maneuver binding in the test.This test is still invoking
solve(), so it never exercises (or guards) the newsolve_next_maneuverbinding. Ifsolve_next_maneuverregressed, the test would continue to pass. Please call the dedicated API here (optionally providing a finite propagation cap) so the test covers the intended path.- solution: Segment.Solution = maneuver_segment.solve(state) + solution: Segment.Solution = maneuver_segment.solve_next_maneuver( + state, Duration.hours(1.0) + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/QLaw.cpp(2 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp(4 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Sequence.cpp(4 hunks)bindings/python/test/guidance_law/test_qlaw.py(1 hunks)bindings/python/test/trajectory/test_segment.py(5 hunks)bindings/python/test/trajectory/test_sequence.py(3 hunks)include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/QLaw.hpp(3 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp(6 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp(9 hunks)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/QLaw.cpp(5 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp(12 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp(11 hunks)test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/QLaw.test.cpp(1 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp(12 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.test.cpp(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (13)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp (3)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (2)
Maneuver(680-696)Maneuver(680-686)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (6)
aFrameSPtr(130-130)aFrameSPtr(166-166)aState(253-253)aState(261-262)aState(365-367)aState(377-383)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Sequence.cpp (2)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (2)
extractManeuvers(173-277)extractManeuvers(173-173)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp (16)
extractManeuvers(100-111)extractManeuvers(100-100)getMaximumManeuverDuration(282-285)getMaximumManeuverDuration(282-282)getMaximumManeuverDurationStrategy(302-305)getMaximumManeuverDurationStrategy(302-302)getMinimumManeuverSeparation(297-300)getMinimumManeuverSeparation(297-297)setMaximumManeuverDuration(307-328)setMaximumManeuverDuration(307-307)setMaximumManeuverDurationStrategy(363-368)setMaximumManeuverDurationStrategy(363-365)setMinimumManeuverDuration(330-351)setMinimumManeuverDuration(330-330)setMinimumManeuverSeparation(353-361)setMinimumManeuverSeparation(353-353)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp (2)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.test.cpp (2)
anInstant(95-112)anInstant(95-101)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (5)
Solution(49-62)Coast(663-678)Coast(663-668)Maneuver(680-696)Maneuver(680-686)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.test.cpp (8)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (2)
Maneuver(680-696)Maneuver(680-686)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp (2)
anInstant(122-139)anInstant(122-128)include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp (1)
anInstant(82-84)include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw.hpp (1)
anInstant(75-81)src/OpenSpaceToolkit/Astrodynamics/Trajectory/State/NumericalSolver.cpp (2)
Default(283-294)Default(283-283)src/OpenSpaceToolkit/Astrodynamics/Flight/System/SatelliteSystem.cpp (2)
Default(148-162)Default(148-148)src/OpenSpaceToolkit/Astrodynamics/EventCondition/RealCondition.cpp (2)
DurationCondition(101-112)DurationCondition(101-101)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp (1)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (14)
getThrusterDynamics(119-149)getThrusterDynamics(119-119)getThrusterDynamics(568-575)getThrusterDynamics(568-568)getCoastDynamics(553-556)getCoastDynamics(553-553)toCoastSegment(592-604)toCoastSegment(592-592)toManeuverSegment(606-618)toManeuverSegment(606-606)solve(620-623)solve(620-620)solveNextManeuver(625-628)solveNextManeuver(625-625)
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/QLaw.cpp (2)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/QLaw.hpp (1)
aCOEDomain(170-170)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Orbit/Model/Kepler/COE.cpp (2)
Cartesian(444-641)Cartesian(444-444)
bindings/python/test/trajectory/test_segment.py (4)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (2)
Segment(348-357)Solution(80-86)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (8)
Segment(464-524)Solution(49-62)Maneuver(680-696)Maneuver(680-686)Coast(663-678)Coast(663-668)solve(620-623)solve(620-620)include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp (2)
Thruster(40-44)Thruster(46-46)src/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.cpp (2)
Thruster(31-41)Thruster(43-43)
bindings/python/test/guidance_law/test_qlaw.py (2)
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/QLaw.hpp (3)
QLaw(126-131)QLaw(134-134)Parameters(66-77)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/QLaw.cpp (3)
QLaw(108-126)QLaw(128-128)Parameters(33-96)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp (3)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (7)
Maneuver(680-696)Maneuver(680-686)extractManeuvers(173-277)extractManeuvers(173-173)Coast(663-678)Coast(663-668)Solution(49-62)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (7)
aFrameSPtr(130-130)aFrameSPtr(166-166)aState(253-253)aState(261-262)aState(365-367)aState(377-383)Solution(80-86)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp (10)
aFrameSPtr(114-114)aMinimumManeuverDuration(239-239)aMaximumManeuverDuration(224-224)aMinimumManeuverSeparation(234-234)aMaximumManeuverDurationStrategy(229-229)aState(267-267)aState(275-279)aState(300-305)aSegment(315-317)Solution(70-70)
bindings/python/test/trajectory/test_sequence.py (2)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp (1)
Sequence(165-172)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp (1)
Sequence(199-258)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/QLaw.cpp (1)
src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/QLaw.cpp (4)
getCOEDomain(158-161)getCOEDomain(158-158)setCOEDomain(163-166)setCOEDomain(163-163)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (4)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp (5)
aState(267-267)aState(275-279)aState(300-305)anEventConditionSPtr(254-254)anEventConditionSPtr(260-260)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Propagator.hpp (4)
aState(180-180)aState(191-193)aState(203-203)aNumericalSolver(234-234)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.hpp (1)
aType(116-118)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.hpp (4)
aType(93-95)aType(102-104)aType(111-111)aType(122-122)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (3)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (9)
aThrusterDynamics(246-246)aName(239-239)aName(277-282)aName(292-298)aName(318-326)aState(253-253)aState(261-262)aState(365-367)aState(377-383)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Propagator.hpp (5)
aNumericalSolver(234-234)aState(180-180)aState(191-193)aState(203-203)aDynamicsArray(147-147)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (2)
FromManeuver(104-128)FromManeuver(104-108)
🪛 Clang (14.0.6)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.test.cpp
[error] 3-3: 'OpenSpaceToolkit/Core/Container/Tuple.hpp' file not found
(clang-diagnostic-error)
🪛 Ruff (0.14.3)
bindings/python/test/trajectory/test_segment.py
214-214: Consider [*dynamics, thruster_dynamics] instead of concatenation
Replace with [*dynamics, thruster_dynamics]
(RUF005)
⏰ 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 (7)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (1)
207-217: Nice accessor split for coast and thruster dynamics.This makes it much easier for higher-level sequencing logic to reason about maneuver metadata without re-parsing the generic dynamics array.
include/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/QLaw.hpp (1)
112-171: Appreciate the explicit COE domain control.The scoped enum with dedicated accessors gives callers a clear, type-safe way to select their averaging regime.
bindings/python/test/guidance_law/test_qlaw.py (1)
120-139: Thanks for covering the new domain knob.These assertions lock in both the default and setter pathways, so the Python surface stays aligned with the C++ contract.
test/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/QLaw.test.cpp (1)
199-225: Great exhaustive exercise of the COE domain API.Hitting the default plus each mutation path in C++ keeps the core implementation honest across future changes.
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/GuidanceLaw/QLaw.cpp (1)
210-297: Python bindings match the new C++ surface cleanly.Exporting the enum alongside get/set keeps parity for scripting users, and the docstrings make the intent obvious.
bindings/python/test/trajectory/test_sequence.py (1)
431-469: Good coverage of the new setters.
These tests exercise each maneuver-duration constraint end-to-end through the Python API, which keeps the bindings honest with the C++ contract.src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/QLaw.cpp (1)
183-209: Switch-based COE domain handling looks solid.
The explicit branching for each supported domain plus the defensive default keeps thrust computation predictable while paving the way for future expansions.
| // Account for minimum maneuver separation | ||
| BOOST_LOG_TRIVIAL(debug) << "Accounting for minimum maneuver separation." << std::endl; | ||
| nextSubsegmentToSolve = | ||
| aSegment.toCoastSegment(aSegment.getName() + " (Coast - Minimum Maneuver Separation Constraint)"); | ||
| if (minimumManeuverSeparation_.isDefined()) | ||
| { | ||
| updateNextCoastInstant(unifiedManeuverIntervals.accessLast().getEnd() + minimumManeuverSeparation_); | ||
| } | ||
| } | ||
|
|
||
| Array<Shared<Dynamics>> unifiedDynamics = aSegment.getCoastDynamics(); | ||
| unifiedDynamics.add(std::make_shared<Thruster>( |
There was a problem hiding this comment.
Coast fallback uses an undefined target when no separation is configured.
When the segment condition is still unmet, we always flip to a coast subsegment. If minimumManeuverSeparation_ is not defined, nextInstantToCoastTo stays undefined, so the very next loop iteration tries to propagate with an undefined duration (nextInstantToCoastTo - unifiedStates.accessLast().accessInstant()). This bubbles into Segment::solve, triggering an undefined-termination instant and blowing up as soon as a second maneuver is required in a segment with no separation constraint. Please make sure we seed nextInstantToCoastTo in this branch (e.g., with the maneuver end plus the existing subsegmentMargin_, and then layer the separation requirement on top if it’s defined) before calling the coast solver.
- nextSubsegmentToSolve =
- aSegment.toCoastSegment(aSegment.getName() + " (Coast - Minimum Maneuver Separation Constraint)");
- if (minimumManeuverSeparation_.isDefined())
- {
- updateNextCoastInstant(unifiedManeuverIntervals.accessLast().getEnd() + minimumManeuverSeparation_);
- }
+ nextSubsegmentToSolve =
+ aSegment.toCoastSegment(aSegment.getName() + " (Coast - Minimum Maneuver Separation Constraint)");
+ updateNextCoastInstant(unifiedManeuverIntervals.accessLast().getEnd() + subsegmentMargin_);
+ if (minimumManeuverSeparation_.isDefined())
+ {
+ updateNextCoastInstant(unifiedManeuverIntervals.accessLast().getEnd() + minimumManeuverSeparation_);
+ }🤖 Prompt for AI Agents
In src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp around lines 694
to 705, a coast fallback is created but nextInstantToCoastTo is left undefined
when minimumManeuverSeparation_ is not set; seed nextInstantToCoastTo before
calling the coast solver by initializing it to the last maneuver end plus
subsegmentMargin_ (e.g., unifiedManeuverIntervals.accessLast().getEnd() +
subsegmentMargin_) and then, if minimumManeuverSeparation_.isDefined(), add the
separation on top (or call updateNextCoastInstant with the combined value) so
the coast propagation uses a defined termination instant.
What
Add Sequence-level capabilities:
Additionally:
extractManeuverssolveNextManeuverHow
The Sequence solves maneuver-type segments by solving their maneuvers iteratively, ensuring that the maneuver-related constraints above are satisfied.
Summary by CodeRabbit
New Features
API Changes
Tests