feat: distinguish between profile target velocity and ground velocity #561
Conversation
WalkthroughA new target type, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Profile
participant TrajectoryTarget
participant Trajectory
participant Frame
User->>Profile: AlignAndConstrain(target, clockingTarget, angle)
Profile->>TrajectoryTarget: (If target is TargetSlidingGroundVelocity) Compute direction
TrajectoryTarget->>Trajectory: Get state/velocity at time
Trajectory->>Frame: Transform velocity to default profile frame
Frame-->>TrajectoryTarget: Returns transformed velocity
TrajectoryTarget-->>Profile: Returns normalized sliding ground velocity vector
Profile-->>User: Returns orientation aligning axis with computed vector
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (13)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #561 +/- ##
==========================================
- Coverage 91.81% 91.79% -0.02%
==========================================
Files 106 106
Lines 10017 10031 +14
==========================================
+ Hits 9197 9208 +11
- Misses 820 823 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Profile.cpp
Outdated
Show resolved
Hide resolved
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Profile.cpp
Outdated
Show resolved
Hide resolved
apaletta3
force-pushed
the
fix/profile-frames-and-target-velocity
branch
from
f3d464d to
988bba8
Compare
apaletta3
force-pushed
the
feat/distinguish-between-profile-target-velocity-and-ground-velocity
branch
from
6a21d31 to
42b005e
Compare
apaletta3
force-pushed
the
feat/distinguish-between-profile-target-velocity-and-ground-velocity
branch
from
104ad3f to
399bf95
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (7)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Profile.cpp (3)
61-65: New enum value exposed correctly – consider enriching the description
TargetSlidingGroundVelocityis now reachable from Python – nice!
A short note that this target compensates for body rotation (a-la yaw compensation) would make the purpose unambiguous for end-users who are not familiar with the C++ docs.
146-151: Doc-string still a bit vague about “scan direction”“Scan direction” might not ring a bell for everyone. A minimal clarification such as “… i.e. the instantaneous velocity vector of the target in its own inertial frame – no yaw-compensation applied” would remove any guesswork.
152-162: Mirror the yaw-compensation note in the new factory’s docBecause the ground velocity target is the yaw-compensated counterpart of
target_velocity, explicitly stating that relationship in the Python doc helps prevent misuse.Example tweak:
-Create a target, which produces a vector pointing along the ground velocity vector ... +Create a target that returns the **yaw-compensated** ground-velocity vector (i.e. the +scan-direction after compensating for the body's rotation) ...include/OpenSpaceToolkit/Astrodynamics/Flight/Profile.hpp (1)
148-167: Minor wording tweak for Doxygen commentsBoth velocity-related factory methods talk about “scan direction”.
ForTargetVelocityyou already note “will not be yaw compensated”; it would be useful to mirror the wording for the new method (“will be yaw compensated”) for readers skimming only the header.src/OpenSpaceToolkit/Astrodynamics/Flight/Profile.cpp (1)
34-34: Constant default frame – double-check static-initialisation order
Frame::GCRF()indirectly touches the frame manager singleton. Creating the shared
pointer in a namespace-scopestaticis usually fine, but if another TU’s static
needsDEFAULT_PROFILE_FRAMEduring its own initialisation the “static initialisation
order fiasco” could bite. One-liner fix is to wrap it in a function:static const Shared<const Frame>& DefaultProfileFrame() { static const auto kFrame = Frame::GCRF(); return kFrame; }Then replace uses with
DefaultProfileFrame().test/OpenSpaceToolkit/Astrodynamics/Flight/Profile.test.cpp (2)
1200-1253: Remove unreachablebreak;statements insidegetExpectedDirectionVector_Every
returnalready exits the lambda; the subsequentbreak;is never executed and may trigger compiler warnings (especially with-Werror).
Cleaning these out improves readability and eliminates dead code.- case Profile::TargetType::GeocentricNadir: - return -aState.getPosition().getCoordinates().normalized(); - break; + case Profile::TargetType::GeocentricNadir: + return -aState.getPosition().getCoordinates().normalized(); … - return (sun.getPositionIn(Frame::GCRF(), aState.accessInstant()).getCoordinates() - - aState.getPosition().getCoordinates()) - .normalized(); - break; + return (sun.getPositionIn(Frame::GCRF(), aState.accessInstant()).getCoordinates() - + aState.getPosition().getCoordinates()) + .normalized();Apply to all cases (Moon, default) in this switch.
1353-1359: Derive parameter-name strings without relying on enum underlying valuesCasting enums to
intis fragile if the underlying type changes or values are reordered. Preferstd::to_string(static_cast<std::underlying_type_t<Enum>>(…))or stringify the enum via a helper.-return "Axis" + std::to_string(static_cast<int>(std::get<0>(paramInfo.param))) + … +constexpr auto toIndex = [](auto e) +{ + return std::to_string(static_cast<std::underlying_type_t<std::decay_t<decltype(e)>>>(e)); +}; +return "Axis" + toIndex(std::get<0>(paramInfo.param)) + …This keeps the code valid even if the enum’s underlying type is changed to
std::uint8_t, etc.
🛑 Comments failed to post (2)
include/OpenSpaceToolkit/Astrodynamics/Flight/Profile.hpp (1)
93-107: 🛠️ Refactor suggestion
⚠️ Potential issueAdding a middle enum entry silently shifts underlying integral values
TargetSlidingGroundVelocityis inserted beforeSun,Moon, …
If any serialized data, hashing, or switch-statements outside this library rely on the
previous integer mapping (e.g.Sunused to be5, now it’s6) this becomes an
ABI / data-format breaking change even though the PR is labelled non-breaking.Two typical mitigations:
enum class TargetType { - GeodeticNadir = 1, - ... - TargetVelocity = 4, - TargetSlidingGroundVelocity = 5, - Sun = 6, + GeodeticNadir = 1, + ... + TargetVelocity = 4, + TargetSlidingGroundVelocity = 5, // NEW but explicit + Sun = 6, // keeps previous valueor append the new value at the end of the enum list.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In include/OpenSpaceToolkit/Astrodynamics/Flight/Profile.hpp around lines 93 to 107, the insertion of TargetSlidingGroundVelocity before Sun shifts the integral values of subsequent enum entries, causing potential ABI and data-format breaks. To fix this, move TargetSlidingGroundVelocity to the end of the enum list after Custom to preserve the original integer mappings of existing entries and avoid breaking changes.src/OpenSpaceToolkit/Astrodynamics/Flight/Profile.cpp (1)
498-503: 🛠️ Refactor suggestion
Guard against zero-magnitude target velocity
ComputeTargetVelocityVectorwillnormalize()any vector – but if the target is
momentarily stationary the call will throw deep inside Eigen. A consistency check
mirroring the one you added for sliding ground velocity would make the behaviour
explicit and symmetrical.+if (targetVelocityCoordinates.isZero()) +{ + throw ostk::core::error::RuntimeError( + "Cannot compute Target Velocity Vector: target speed is zero." + ); +} return targetVelocityCoordinates.normalized();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const Vector3d targetVelocityCoordinates = aTrajectory.getStateAt(aState.accessInstant()).inFrame(DEFAULT_PROFILE_FRAME).getVelocity().accessCoordinates(); if (targetVelocityCoordinates.isZero()) { throw ostk::core::error::RuntimeError( "Cannot compute Target Velocity Vector: target speed is zero." ); } return targetVelocityCoordinates.normalized(); }🤖 Prompt for AI Agents
In src/OpenSpaceToolkit/Astrodynamics/Flight/Profile.cpp around lines 498 to 503, the code normalizes the target velocity vector without checking if its magnitude is zero, which can cause a runtime error. Add a guard to check if the target velocity vector's magnitude is zero before normalization; if it is zero, handle this case explicitly (e.g., return a zero vector or handle as appropriate) to prevent exceptions and ensure consistent behavior with the sliding ground velocity check.
apaletta3
deleted the
feat/distinguish-between-profile-target-velocity-and-ground-velocity
branch
Non-breaking change that moves the yaw compensation functionality over to TargetGroundVelocity, while the leaving the TargetVelocity to be used for non-yaw compensated profiles. This more clearly indicates the intent of each target type, since yaw compensation doesn't align the clocking axis with the target's general velocity, it aligns it with the target's ground velocity.
It also adds more thorough tests for simple geometric cases of target trajectory position, velocity, and sliding target ground velocity for a simple polar orbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests