From e7bbd7001f117af8317e30e0f6dd1e0b4b55c076 Mon Sep 17 00:00:00 2001 From: cinnaboot Date: Mon, 20 Apr 2026 13:08:17 -0400 Subject: [PATCH] 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. --- docs/planning/propagation_refactor.md | 161 ++++++++++++++------------ 1 file changed, 89 insertions(+), 72 deletions(-) diff --git a/docs/planning/propagation_refactor.md b/docs/planning/propagation_refactor.md index 2234e0b..7480116 100644 --- a/docs/planning/propagation_refactor.md +++ b/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 -## Call Chain: `update_simulation()` +**Remaining:** Adds TODO in `update_spacecraft_physics` about testing interpolated burns. + +--- + +## 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, ...); +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, ...); } -// ... burn ... -double remaining_dt = sim->dt - dt_to_burn; -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(...); -execute_maneuver(...); -propagate_orbital_elements(craft->orbit, sim->dt, ...); -orbital_elements_to_cartesian(...); +if (maneuver_fired) { + craft->orbit = propagate_orbital_elements(craft->orbit, burn_dt, ...); + execute_maneuver(...); + 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`