Browse Source

Update planning doc with completed refactoring status

Mark Issues 1, 2, 5, 6 as RESOLVED. Mark Issue 3 as PARTIALLY RESOLVED.
Mark Issue 4 as UNRESOLVED (time-triggered quantization).

Update call chain diagram to show current merged structure.
Update call sites summary to show final 4 call sites (3 contexts).
Add remaining work section for interpolated time triggers and
parabolic/hyperbolic orbit support.
main
cinnaboot 3 months ago
parent
commit
e7bbd7001f
  1. 159
      docs/planning/propagation_refactor.md

159
docs/planning/propagation_refactor.md

@ -3,6 +3,9 @@
## Session Date
2026-04-20
## Objective
Audit all call sites of `propagate_orbital_elements()` for spacecraft and trace the `update_simulation()` call chain through `execute_pending_maneuvers()` and `update_spacecraft_physics()` to identify inefficiencies and confusing branching.
## Recent Changes
### 2026-04-20: Eliminated Redundant Propagation in `check_maneuver_trigger()`
@ -22,18 +25,34 @@ Replaced the per-frame propagation probe in `check_maneuver_trigger()` (true-ano
**Remaining:** See Issue 1 below — still has a TODO for parabolic/hyperbolic orbit support.
---
### 2026-04-20: Merged Maneuver Execution into Single Propagation Pass
## Objective
Audit all call sites of `propagate_orbital_elements()` for spacecraft and trace the `update_simulation()` call chain through `execute_pending_maneuvers()` and `update_spacecraft_physics()` to identify inefficiencies and confusing branching.
Consolidated `execute_pending_maneuvers()` into `update_spacecraft_physics()` so every spacecraft goes through exactly one propagation path.
**What changed:**
- Removed `spacecraft_handled_this_frame[256]` static array (magic number)
- Removed `reset_spacecraft_tracking()` function
- Removed `execute_pending_maneuvers()` function
- Check maneuver triggers before propagation in `update_spacecraft_physics`
- Propagate to burn time, execute burn, propagate remainder (same order as old code)
- Removed `execute_pending_maneuvers` declaration from `simulation.h`
**Impact:**
- Single propagation call per spacecraft per frame (was 2 separate code paths)
- No static array with hardcoded size
- Simpler call chain: no separate maneuver pass
- Enables sub-step interpolation for true-anomaly triggers
- Sets up foundation for interpolated time triggers
**Remaining:** Adds TODO in `update_spacecraft_physics` about testing interpolated burns.
## Call Chain: `update_simulation()`
---
## Call Chain: `update_simulation()` (Current)
```
update_simulation()
├── reset_spacecraft_tracking()
├── update_bodies_physics()
│ └── for each body:
│ ├── find_dominant_body()
@ -44,26 +63,23 @@ update_simulation()
├── compute_global_coordinates()
├── execute_pending_maneuvers()
│ └── for each unexecuted maneuver:
│ ├── check_maneuver_trigger()
│ │ ├── TRIGGER_TIME: sim->time >= trigger_value
│ │ └── TRIGGER_TRUE_ANOMALY:
│ │ ├── propagate_orbital_elements() ← per-frame probe
│ │ └── Kepler equation solving
│ └── if triggered:
│ ├── propagate_orbital_elements(dt_to_burn)
│ ├── execute_maneuver()
│ │ ├── apply_impulsive_burn()
│ │ └── cartesian_to_orbital_elements()
│ └── propagate_orbital_elements(remaining_dt)
├── update_spacecraft_physics()
│ └── for each craft NOT handled:
│ └── for each craft:
│ ├── orbital_elements_to_cartesian() → expected_vel
│ ├── velocity drift check
│ ├── cartesian_to_orbital_elements() (if drift)
│ └── propagate_orbital_elements()
│ ├── check_maneuver_trigger() ← inline, before propagation
│ │ ├── TRIGGER_TIME: sim->time >= trigger_value
│ │ └── TRIGGER_TRUE_ANOMALY:
│ │ └── analytical mean anomaly delta (no propagation)
│ └── if maneuver fired:
│ │ ├── propagate_orbital_elements(burn_dt)
│ │ ├── execute_maneuver()
│ │ │ ├── apply_impulsive_burn()
│ │ │ └── cartesian_to_orbital_elements()
│ │ └── propagate_orbital_elements(remaining_dt)
│ └── else:
│ └── propagate_orbital_elements(sim->dt)
├── compute_spacecraft_globals()
└── sim->time += sim->dt
@ -90,111 +106,112 @@ OrbitalElements future_elements = propagate_orbital_elements(craft->orbit, sim->
### Issue 2: Mixed Concerns in `execute_pending_maneuvers()`
**Location:** `src/simulation.cpp`, `execute_pending_maneuvers()`
**Status:** RESOLVED — merged into `update_spacecraft_physics()`.
The function performs two distinct responsibilities:
The function previously performed two distinct responsibilities:
1. **Checking** trigger conditions (calls `check_maneuver_trigger()`)
2. **Executing** the burn (propagation → burn → propagation)
```cpp
void execute_pending_maneuvers(SimulationState* sim) {
for (...) {
if (check_maneuver_trigger(maneuver, craft, sim)) { // CHECKING
// ... propagation, burn, more propagation // EXECUTING
}
}
}
```
The trigger-checking logic (especially the true-anomaly branch with its Kepler equation solving) is interleaved with the execution logic. This makes it difficult to reason about what happens in each phase and complicates testing.
**Resolution:** Trigger checking now happens inline in `update_spacecraft_physics()` before the propagation decision. The code is cleaner: check → decide propagation amount → propagate → burn (if needed) → propagate remainder.
### Issue 3: Ambiguous `scheduled_dt` Semantics
The `scheduled_dt` field in `Maneuver` has different meanings depending on trigger type:
**Status:** PARTIALLY RESOLVED — semantics are now clearer but still differ by trigger type.
| Trigger Type | `scheduled_dt` meaning | Set by |
|-------------|----------------------|--------|
| `TRIGGER_TIME` | Always 0.0 (never set) | Never |
| `TRIGGER_TRUE_ANOMALY` | Seconds until exact burn position | `check_maneuver_trigger()` |
In `execute_pending_maneuvers()`:
In `update_spacecraft_physics()`:
```cpp
double dt_to_burn = maneuver->scheduled_dt;
if (dt_to_burn > 0.0) {
craft->orbit = propagate_orbital_elements(craft->orbit, dt_to_burn, ...);
}
// ... burn ...
double remaining_dt = sim->dt - dt_to_burn;
if (maneuver_fired) {
craft->orbit = propagate_orbital_elements(craft->orbit, burn_dt, ...);
execute_maneuver(fired_maneuver, ...);
craft->orbit = propagate_orbital_elements(craft->orbit, remaining_dt, ...);
}
```
For `TRIGGER_TIME`, `scheduled_dt == 0` is a coincidence — the field is never set. The code works, but the reason is opaque. The branching (`if dt_to_burn > 0`) exists only for true-anomaly triggers, but a reader cannot tell this from the code alone.
For `TRIGGER_TIME`, `burn_dt == 0` is still a coincidence — the field is never set. The branching is now explicit via `maneuver_fired` flag, which is clearer.
### Issue 4: Time-Triggered Burns Propagate from Wrong State
**Status:** UNRESOLVED — root cause of burn timing quantization.
For time triggers, the sequence is:
```
Frame N: sim->time = 310.0 (trigger_value = 305.0)
check: 310 >= 305 → true
dt_to_burn = 0
burn_dt = 0
remaining_dt = 10
execute_maneuver() → burn applies at sim->time=310
propagate craft by 10s starting from sim->time=310
```
The craft's orbit state is at `sim->time=310`, not at the trigger time `305`. The burn fires 5s late and the post-burn propagation starts from the wrong orbital position. This is the root cause of the burn timing quantization problem documented in `docs/planning/hohmann-rendezvous-quantization-fix.md`.
**TODO in code:** `update_spacecraft_physics()` has a TODO suggesting `dt_to_burn = trigger_value - (sim->time - sim->dt)` for exact placement.
### Issue 5: Hardcoded Array Size
**Location:** `src/simulation.cpp`, static declaration
**Status:** RESOLVED — removed `spacecraft_handled_this_frame[256]` static array.
**Location:** Previously `src/simulation.cpp`, static declaration
```cpp
// REMOVED
static bool spacecraft_handled_this_frame[256];
```
A global array with a hardcoded size of 256. If more than 256 spacecraft are added, the tracking silently truncates. It should be part of `SimulationState` or dynamically sized. There is already a FIXME comment on this line.
**Resolution:** The array is no longer needed since maneuver checking is inline in the propagation loop. No separate tracking mechanism required.
### Issue 6: Duplicated Propagation Logic
The "normal" propagation path in `update_spacecraft_physics()` and the maneuver path in `execute_pending_maneuvers()` are essentially duplicates:
**Status:** RESOLVED — single propagation path.
**Normal path** (`update_spacecraft_physics`):
```cpp
propagate_orbital_elements(craft->orbit, sim->dt, ...);
orbital_elements_to_cartesian(...);
```
The "normal" propagation path and the maneuver path are now the same code path:
**Maneuver path** (`execute_pending_maneuvers`, when `dt_to_burn == 0`):
```cpp
propagate_orbital_elements(craft->orbit, 0, ...); // no-op
orbital_elements_to_cartesian(...);
if (maneuver_fired) {
craft->orbit = propagate_orbital_elements(craft->orbit, burn_dt, ...);
execute_maneuver(...);
propagate_orbital_elements(craft->orbit, sim->dt, ...);
orbital_elements_to_cartesian(...);
craft->orbit = propagate_orbital_elements(craft->orbit, remaining_dt, ...);
} else {
craft->orbit = propagate_orbital_elements(craft->orbit, sim->dt, ...);
}
```
When no sub-step is needed, the maneuver path is: propagate → convert → burn → propagate → convert. The propagation and coordinate conversion logic is duplicated across both code paths.
When `burn_dt == 0` and `remaining_dt == sim->dt`, the maneuver path is: propagate(0) → burn → propagate(sim->dt). The normal path is: propagate(sim->dt). The propagation logic is no longer duplicated.
## Call Sites Summary
### Before fix (5 call sites)
### Final State (3 call sites)
| Location | Function | Context | dt value |
|----------|----------|---------|----------|
| `simulation.cpp:287` | `update_bodies_physics()` | Normal body propagation | `sim->dt` |
| `simulation.cpp:315` | `update_spacecraft_physics()` | Normal craft propagation | `sim->dt` |
| `simulation.cpp:343` | `execute_pending_maneuvers()` | Pre-burn sub-step propagation | `dt_to_burn` (0 to `sim->dt`) |
| `simulation.cpp:350` | `execute_pending_maneuvers()` | Post-burn remaining propagation | `sim->dt - dt_to_burn` |
| `maneuver.cpp:147` | `check_maneuver_trigger()` | True-anomaly look-ahead probe | `sim->dt` |
| `simulation.cpp:338` | `update_spacecraft_physics()` | Pre-burn sub-step propagation | `burn_dt` (0 to `sim->dt`) |
| `simulation.cpp:343` | `update_spacecraft_physics()` | Post-burn remaining propagation | `sim->dt - burn_dt` |
### After fix (4 call sites)
Remaining: 4 call sites (1 for bodies, 3 for spacecraft), 2 distinct contexts for spacecraft (normal propagation, sub-step execution).
| Location | Function | Context | dt value |
|----------|----------|---------|----------|
| `simulation.cpp:287` | `update_bodies_physics()` | Normal body propagation | `sim->dt` |
| `simulation.cpp:315` | `update_spacecraft_physics()` | Normal craft propagation | `sim->dt` |
| `simulation.cpp:343` | `execute_pending_maneuvers()` | Pre-burn sub-step propagation | `dt_to_burn` (0 to `sim->dt`) |
| `simulation.cpp:350` | `execute_pending_maneuvers()` | Post-burn remaining propagation | `sim->dt - dt_to_burn` |
## Remaining Work
### Interpolated Time Triggers (TODO in code)
For `TRIGGER_TIME`, `scheduled_dt` is always 0, so the burn applies at `sim->time` (step boundary). To achieve exact placement:
1. In `check_maneuver_trigger()` for `TRIGGER_TIME`: compute `dt_to_burn = trigger_value - (sim->time - sim->dt)` when `sim->time >= trigger_value`
2. Set `scheduled_dt = dt_to_burn` (positive value, 0 to `sim->dt`)
3. The propagation loop then does: `propagate(dt_to_burn)` → burn → `propagate(remaining)`
This would place the burn at the exact trigger time, not the step boundary. See `docs/planning/hohmann-rendezvous-quantization-fix.md` for quantization analysis.
### Parabolic/Hyperbolic Orbit Support (TODO in code)
Remaining: 4 call sites, 2 distinct contexts for spacecraft (normal propagation, sub-step execution). Issue 6 (duplicated propagation) remains.
The analytical `dt_needed` calculation in `check_maneuver_trigger()` only handles elliptical orbits. TODO comment added for:
- Parabolic: Barker's equation `D + D³/3 = M`, where `D = tan(ν/2)`
- Hyperbolic: `e·sinh(H) - H = M`, using hyperbolic anomaly `H`

Loading…
Cancel
Save