feat: add satellite selection plugin for TOCA/POCA analysis #1256
Conversation
Create a new plugin that allows users to select a primary satellite and a target satellite to compute the Time of Closest Approach (TOCA) and Point of Closest Approach (POCA) between them. Features: - Propagates both satellites' ECI positions using SGP4 - Calculates distance between satellites at each time step - Identifies local minima in distance to find closest approaches - Displays next 10 TOCAs and POCAs in a side menu - Clicking on an entry changes simulation time to that event - Highlights both satellites in the search bar when an event is clicked - Side menu styled similar to collisions plugin - 7-day lookahead with 60-second time steps Files added: - src/plugins/satellite-selection/satellite-selection.ts - src/plugins/satellite-selection/satellite-selection.css - public/img/icons/satellite-selection.png Files modified: - src/plugins/plugins.ts (plugin registration) - src/plugins/keeptrack-plugins-configuration.ts (plugin configuration)
|
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new satellite selection plugin that enables users to analyze the Time of Closest Approach (TOCA) and Point of Closest Approach (POCA) between two selected satellites. The plugin propagates satellite positions using SGP4, identifies local minima in inter-satellite distance over a 7-day window with 60-second time steps, and displays the results in an interactive side menu.
Key Changes:
- New SatelliteSelection plugin with TOCA/POCA calculation capabilities
- Interactive UI for selecting primary and target satellites with result visualization
- Plugin registration and configuration integration into the existing plugin system
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/plugins/satellite-selection/satellite-selection.ts | Core plugin implementation with SGP4 propagation, local minima detection, and event management |
| src/plugins/satellite-selection/satellite-selection.css | Styling for the side menu, satellite info display, and event table |
| src/plugins/plugins.ts | Registers the new SatelliteSelection plugin in the plugin manager |
| src/plugins/keeptrack-plugins-configuration.ts | Adds configuration type for the SatelliteSelection plugin |
| public/img/icons/satellite-selection.png | Icon asset for the plugin's bottom menu button |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sat1Position: sat1J2000.position, | ||
| sat2Position: sat2J2000.position, | ||
| relativeVelocity: Math.sqrt( | ||
| ric.velocity.x ** 2 + ric.velocity.y ** 2 + ric.velocity.z ** 2 |
There was a problem hiding this comment.
The variable relativeVelocity is being calculated from the RIC (Radial, In-track, Cross-track) velocity components, but this doesn't represent the true relative velocity between the satellites. The RIC frame velocity represents the rate of change of the relative position in the RIC coordinate system, not the magnitude of the velocity difference between the two satellites.
To properly calculate relative velocity magnitude, you should compute:
const sat1Velocity = sat1J2000.velocity;
const sat2Velocity = sat2J2000.velocity;
const relativeVelocity = Math.sqrt(
(sat1Velocity.x - sat2Velocity.x) ** 2 +
(sat1Velocity.y - sat2Velocity.y) ** 2 +
(sat1Velocity.z - sat2Velocity.z) ** 2
);| ric.velocity.x ** 2 + ric.velocity.y ** 2 + ric.velocity.z ** 2 | |
| (sat1J2000.velocity.x - sat2J2000.velocity.x) ** 2 + | |
| (sat1J2000.velocity.y - sat2J2000.velocity.y) ** 2 + | |
| (sat1J2000.velocity.z - sat2J2000.velocity.z) ** 2 |
|
|
||
| isDescending = distance < previousDistance; | ||
| } | ||
|
|
There was a problem hiding this comment.
The local minima detection logic has a potential issue. On the first iteration when previousDistance is Infinity, the condition distance < previousDistance will be true, incorrectly setting isDescending = true. This could cause the algorithm to miss the first actual local minimum or produce incorrect behavior.
Consider initializing isDescending only after the first valid comparison:
if (previousDistance < Infinity) {
if (previousDistance < distance && isDescending) {
// Found a local minimum
// ...
}
isDescending = distance < previousDistance;
}| isDescending = distance < previousDistance; | |
| } | |
| isDescending = distance < previousDistance; | |
| } | |
| // Only update previousDistance after first valid comparison |
| sat1Position: sat1J2000.position, | ||
| sat2Position: sat2J2000.position, |
There was a problem hiding this comment.
The satellite positions stored at local minima are incorrect. When a local minimum is detected at iteration t, the code stores sat1J2000.position and sat2J2000.position which are the positions from the current iteration (time t), but the actual minimum occurred at the previous iteration (time t - this.timeStep_).
The positions should be retrieved for previousTime or stored from the previous iteration to match the time when the minimum distance actually occurred.
| sat1Position: EciVec3; | ||
| sat2Position: EciVec3; |
There was a problem hiding this comment.
[nitpick] The TocaPocaEvent interface stores both sat1Position and sat2Position but these values are never used anywhere in the code. The positions are computed during the TOCA/POCA calculation but are not displayed in the table or used when an event is clicked.
Consider removing these unused fields from the interface to reduce memory usage, or add functionality that uses them (e.g., displaying the positions in the table or visualizing them).
| sat1Position: EciVec3; | |
| sat2Position: EciVec3; |
| const now = new Date(); | ||
|
|
||
| // Change time to 30 seconds before the TOCA | ||
| ServiceLocator.getTimeManager().changeStaticOffset(event.time.getTime() - now.getTime() - 1000 * 30); |
There was a problem hiding this comment.
[nitpick] Magic number 30 is used without explanation. While the comment above explains it's "30 seconds before the TOCA", the magic number should be extracted to a named constant for better maintainability:
private readonly tocaPreviewOffsetSeconds_ = 30;
// Then in the code:
ServiceLocator.getTimeManager().changeStaticOffset(
event.time.getTime() - now.getTime() - this.tocaPreviewOffsetSeconds_ * 1000
);| } | ||
|
|
||
| try { | ||
| const events: TocaPocaEvent[] = []; |
There was a problem hiding this comment.
Unused variable events.
| const events: TocaPocaEvent[] = []; |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| time: previousTime, | ||
| offset: (t - this.timeStep_) * 1000, | ||
| distance: previousDistance as Kilometers, | ||
| sat1Position: sat1J2000.position, | ||
| sat2Position: sat2J2000.position, |
There was a problem hiding this comment.
Each TOCA/POCA entry uses previousTime/previousDistance from the prior timestep while storing sat1Position/sat2Position (and the relative velocity immediately below) from the current iteration’s sat1J2000/ric, so the row mixes data from two different timestamps. With a 60 s step this makes the reported positions and relative speed describe a moment up to a minute after the shown event time and distance, so the table no longer reflects the actual closest-approach state whenever a local minimum is found.
Useful? React with 👍 / 👎.
|
thkruz
deleted the
claude/satellite-selection-plugin-01UNMXDqSFUpr58ydMQ1RB26
branch
Create a new plugin that allows users to select a primary satellite and a target satellite to compute the Time of Closest Approach (TOCA) and Point of Closest Approach (POCA) between them.
Features:
Files added:
Files modified: