feat: add operational settings to maneuvering segments [WIP] #594
Conversation
phc1990
changed the title
WalkthroughThis PR enhances trajectory segment solving by introducing maneuver constraints (minimum duration, gap, maximum count) to control maneuver segmentation. It adds a caching mechanism for maneuver extraction in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp (1)
328-361: Potential infinite loop when all segments are skipped (no progress).If every segment is skipped by solve_ (e.g., all maneuvers < minimum), neither propagationDuration nor initialState advance; the while loop repeats forever.
Add a “progress guard” to break/return when no segment solution was added in a full pass:
while (!eventConditionIsSatisfied && propagationDuration <= aMaximumPropagationDuration) { - for (const Segment& segment : segments_) + const Size solutionsAtLoopStart = segmentSolutions.getSize(); + bool progressed = false; + for (const Segment& segment : segments_) { const Duration segmentPropagationDurationLimit = std::min(segmentPropagationDurationLimit_, aMaximumPropagationDuration - propagationDuration); const Size segmentSolutionSizeBefore = segmentSolutions.getSize(); if (!solve_(segment, segmentSolutions, initialState, segmentPropagationDurationLimit, "")) { return {segmentSolutions, false}; } // Check if a segment solution was added (it might have been skipped due to minimum maneuver duration) if (segmentSolutions.getSize() > segmentSolutionSizeBefore) { + progressed = true; const Segment::Solution& segmentSolution = segmentSolutions.accessLast(); eventConditionIsSatisfied = sequenceCondition->isSatisfied(segmentSolution.states.accessLast(), initialState); // Terminate Sequence successfully if the provided event condition was satisfied if (eventConditionIsSatisfied) { return {segmentSolutions, true}; } propagationDuration += segmentSolution.getPropagationDuration(); } } + if (!progressed && segmentSolutions.getSize() == solutionsAtLoopStart) + { + BOOST_LOG_TRIVIAL(warning) << "No progress made in solveToCondition loop; aborting to avoid infinite loop."; + return {segmentSolutions, false}; + } }
🧹 Nitpick comments (6)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (2)
192-194: Mutable cache without synchronization can race under concurrency.Segment::Solution caches maneuvers in a mutable member with no locking. If a Solution is accessed from multiple threads, this can data race.
- Document non-thread-safety or guard updates with a lightweight mutex.
- Alternatively, use std::optional cached value returned by value semantics to avoid shared mutation.
269-281: Maneuver-constraint parameters: clarify precedence and intent.Good additions. Please ensure docs specify:
- Undefined ⇒ constraint disabled.
- Precedence between Sequence-level minimumManeuverDuration vs per-segment fields (prefer per-segment only to avoid duplicate policy).
Consider removing Sequence-level duration checks and relying solely on per-segment constraints for consistency.
Also applies to: 303-316, 343-353
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp (1)
456-459: Docstring casing inconsistency.Use Angle.undefined() for consistency with other defaults (Duration.undefined(), Integer.undefined()).
Update the docstring default example string from "Angle.Undefined()" to "Angle.undefined()".
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp (1)
422-438: Duplicate “minimum maneuver duration” enforcement across Sequence and Segment.Sequence::solve_ skips entire maneuver segments if any interval < minimumManeuverDuration_. Segment::Solve_ already enforces per‑segment constraints and truncates states. Two policies can conflict.
- Drop the Sequence-level duration check to a single source of truth (Segment). If retained, update log to “Skipping maneuver segment” and document precedence.
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (2)
166-194: Exact zero test for “thrusting” is brittle; use a tolerance and ignore mass column.Using full row norm != 0.0 mixes velocity contribution and mass flow; and exact zero comparisons are numerically unstable.
- Use a small epsilon on the 3× velocity columns only, e.g.:
- if (fullSegmentContributions.row(i).norm() != 0.0) + const double thrustNorm = fullSegmentContributions.block<1,3>(i, 0).norm(); + if (thrustNorm > 1e-12) // Tolerance to account for numerical noiseAdjust epsilon based on solver precision.
150-153: Frame comparison in cache: consider pointer equality for speed.Dereferencing and comparing Frames may be costlier than pointer equality if frames are interned (e.g., Frame::GCRF()). If semantic equality is required, keep as-is; if identity suffices, prefer cachedManeuvers_.first == aFrameSPtr.
Replace with pointer equality or use aFrameSPtr->isSame(cachedManeuvers_.first) if available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp(6 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp(9 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp(9 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp (2)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp (1)
aSegment(246-252)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (2)
Maneuver(639-662)Maneuver(639-648)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp (1)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (2)
getManeuverIntervals(247-263)getManeuverIntervals(247-247)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (2)
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 (8)
aFrameSPtr(130-130)aFrameSPtr(171-171)aName(255-260)aName(273-282)aName(305-316)aDynamicsArray(374-374)aState(240-240)aState(362-367)src/OpenSpaceToolkit/Astrodynamics/Trajectory/State.cpp (11)
Undefined(530-533)Undefined(530-530)aState(165-175)aState(165-165)aState(177-213)aState(177-177)aState(215-218)aState(215-215)aState(220-263)aState(220-220)aState(265-308)src/OpenSpaceToolkit/Astrodynamics/Flight/Maneuver.cpp (1)
Maneuver(44-115)
🔇 Additional comments (2)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp (1)
220-233: Nice addition: get_maneuver_intervals.Binding mirrors C++ API and documents cache fallback to GCRF. Looks good.
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (1)
247-263: getManeuverIntervals defaulting to GCRF: good; ensure invariant in docs.Intervals are frame-invariant; defaulting to GCRF to populate cache is reasonable. Looks good.
| bool solve_( | ||
| const Segment& aSegment, | ||
| Array<Segment::Solution>& aSegmentSolutions, | ||
| State& anInitialState, | ||
| const Duration& aSegmentPropagationLimit, | ||
| const String& aSegmentNameSuffix | ||
| ) const; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing include for String (compile-time break).
solve_ takes const String& aSegmentNameSuffix but this header doesn’t include OpenSpaceToolkit/Core/Type/String.hpp. Add it to avoid relying on transitive includes.
Apply this diff near the top includes:
#include <OpenSpaceToolkit/Core/Container/Array.hpp>
#include <OpenSpaceToolkit/Core/Type/Size.hpp>
+#include <OpenSpaceToolkit/Core/Type/String.hpp>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp around lines
246 to 252, the declaration of solve_ uses const String& aSegmentNameSuffix but
this header doesn't include OpenSpaceToolkit/Core/Type/String.hpp; add the
include for OpenSpaceToolkit/Core/Type/String.hpp near the top of the include
block so the String type is declared explicitly (avoid relying on transitive
includes).
| // Check maneuver constraints if applicable | ||
| if (type_ == Segment::Type::Maneuver && | ||
| (minimumManeuverDuration_.isDefined() || minimumManeuverGap_.isDefined() || maximumManeuverCount_.isDefined())) | ||
| { | ||
| // Create preliminary solution to extract maneuvers | ||
| const Segment::Solution preliminarySolution = { | ||
| name_, | ||
| aDynamicsArray, | ||
| states, | ||
| conditionSolution.conditionIsSatisfied, | ||
| type_, | ||
| }; | ||
|
|
||
| const Array<flightManeuver> maneuvers = preliminarySolution.extractManeuvers(aState.accessFrame()); | ||
|
|
||
| // Check if we need to truncate based on constraints | ||
| Size maneuverCountLimit = maneuvers.getSize(); | ||
|
|
||
| for (Size i = 0; i < maneuvers.getSize(); ++i) | ||
| { | ||
| const flightManeuver& currentManeuver = maneuvers[i]; | ||
|
|
||
| // Check minimum maneuver duration | ||
| if (minimumManeuverDuration_.isDefined()) | ||
| { | ||
| const Duration maneuverDuration = currentManeuver.getInterval().getDuration(); | ||
| if (maneuverDuration < minimumManeuverDuration_) | ||
| { | ||
| maneuverCountLimit = i; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // Check minimum maneuver gap (if not the first maneuver) | ||
| if (minimumManeuverGap_.isDefined() && i > 0) | ||
| { | ||
| const Instant previousManeuverEnd = maneuvers[i - 1].getInterval().accessEnd(); | ||
| const Instant currentManeuverStart = currentManeuver.getInterval().accessStart(); | ||
| const Duration gap = currentManeuverStart - previousManeuverEnd; | ||
|
|
||
| if (gap < minimumManeuverGap_) | ||
| { | ||
| maneuverCountLimit = i; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // Check maximum maneuver count | ||
| if (maximumManeuverCount_.isDefined() && (i + 1) > static_cast<Size>(maximumManeuverCount_)) | ||
| { | ||
| maneuverCountLimit = i; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // Truncate states if necessary | ||
| if (maneuverCountLimit < maneuvers.getSize()) | ||
| { | ||
| // Find the first instant of the first excluded maneuver | ||
| const Instant firstExcludedManeuverStart = maneuvers[maneuverCountLimit].getInterval().accessStart(); | ||
|
|
||
| // Find the first state at or after the excluded maneuver start | ||
| Size truncationIndex = 0; | ||
| for (Size j = 0; j < states.getSize(); ++j) | ||
| { | ||
| if (states[j].accessInstant() >= firstExcludedManeuverStart) | ||
| { | ||
| truncationIndex = j; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // Truncate the states array to exclude the first excluded maneuver and everything after | ||
| Array<State> truncatedStates = Array<State>::Empty(); | ||
| if (truncationIndex > 0) | ||
| { | ||
| truncatedStates.reserve(truncationIndex); | ||
| for (Size j = 0; j < truncationIndex; ++j) | ||
| { | ||
| truncatedStates.add(states[j]); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| // If the first excluded maneuver starts at or before the first state, return only the initial state | ||
| truncatedStates.add(states.accessFirst()); | ||
| } | ||
| states = truncatedStates; | ||
| } | ||
| } |
There was a problem hiding this comment.
conditionIsSatisfied becomes stale after truncation.
States are truncated when constraints hit, but the returned flag uses the pre‑truncation conditionSolution.conditionIsSatisfied, which may be true even though the truncated timeline never reaches the event.
Track and return a corrected flag:
- const NumericalSolver::ConditionSolution conditionSolution = propagator.calculateStateToCondition(
+ const NumericalSolver::ConditionSolution conditionSolution = propagator.calculateStateToCondition(
aState, aState.accessInstant() + maximumPropagationDuration, *anEventConditionSPtr
);
+ bool conditionReached = conditionSolution.conditionIsSatisfied;
@@
- if (maneuverCountLimit < maneuvers.getSize())
+ if (maneuverCountLimit < maneuvers.getSize())
{
@@
- states = truncatedStates;
+ states = truncatedStates;
+ // Truncation prevents reaching the original event condition
+ conditionReached = false;
}
}
@@
- return {
+ return {
name_,
aDynamicsArray,
states,
- conditionSolution.conditionIsSatisfied,
+ conditionReached,
type_,
};Also applies to: 809-816
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #594 +/- ##
==========================================
- Coverage 91.98% 91.63% -0.36%
==========================================
Files 113 113
Lines 10686 10743 +57
==========================================
+ Hits 9830 9844 +14
- Misses 856 899 +43 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WIP
Summary by CodeRabbit
Release Notes
New Features
Performance