From 1a3e1dbdb7bfdbe34269c7da1cdca7fbaf710ee3 Mon Sep 17 00:00:00 2001 From: cinnaboot Date: Mon, 20 Apr 2026 11:44:01 -0400 Subject: [PATCH] add planning doc for improvements to simulation propagation --- docs/TODO | 1 + docs/planning/propagation_refactor.md | 164 ++++++++++++++++++++++++++ 2 files changed, 165 insertions(+) create mode 100644 docs/planning/propagation_refactor.md diff --git a/docs/TODO b/docs/TODO index 1d889ce..c22b62c 100644 --- a/docs/TODO +++ b/docs/TODO @@ -18,6 +18,7 @@ If you see modifications to this file in git status, IGNORE them and do not comm - work on maneuver interpolation. see docs/planning/hohmann-rendezvous-quantization-fix.md - write out/fix inline code docs with the documentation generation script, and review - can then work on a better method for commenting/detecting changes via script + - simulation propagation refactor: see docs/planning/propagation_refactor.md diff --git a/docs/planning/propagation_refactor.md b/docs/planning/propagation_refactor.md new file mode 100644 index 0000000..4103e49 --- /dev/null +++ b/docs/planning/propagation_refactor.md @@ -0,0 +1,164 @@ +# Propagation Call Chain Analysis + +## 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. + +## Call Chain: `update_simulation()` + +``` +update_simulation() +│ +├── reset_spacecraft_tracking() +│ +├── update_bodies_physics() +│ └── for each body: +│ ├── find_dominant_body() +│ ├── orbital_elements_to_cartesian() → expected_vel +│ ├── velocity drift check (every frame) +│ ├── cartesian_to_orbital_elements() (if drift > 1e-6) +│ └── propagate_orbital_elements() +│ +├── 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: +│ ├── orbital_elements_to_cartesian() → expected_vel +│ ├── velocity drift check +│ ├── cartesian_to_orbital_elements() (if drift) +│ └── propagate_orbital_elements() +│ +├── compute_spacecraft_globals() +└── sim->time += sim->dt +``` + +## Issues Found + +### Issue 1: Redundant Propagation for True-Anomaly Triggers + +**Location:** `src/maneuver.cpp`, line 147 in `check_maneuver_trigger()` + +For every frame that a true-anomaly maneuver is pending, `propagate_orbital_elements()` is called as a "look-ahead" probe to determine if the target angle is approaching. + +```cpp +// maneuver.cpp:147 +OrbitalElements future_elements = propagate_orbital_elements(craft->orbit, sim->dt, parent->mass); +``` + +**Impact:** For a Hohmann transfer with a ~244,000s wait time and DT=10s, this results in ~24,400 redundant propagations — each one solving Kepler's equation — before the maneuver even fires. The spacecraft's orbit state is not modified, but the computational cost is paid every single frame. + +### Issue 2: Mixed Concerns in `execute_pending_maneuvers()` + +**Location:** `src/simulation.cpp`, `execute_pending_maneuvers()` + +The function performs 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. + +### Issue 3: Ambiguous `scheduled_dt` Semantics + +The `scheduled_dt` field in `Maneuver` has different meanings depending on 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()`: + +```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; +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. + +### Issue 4: Time-Triggered Burns Propagate from Wrong State + +For time triggers, the sequence is: + +``` +Frame N: sim->time = 310.0 (trigger_value = 305.0) + check: 310 >= 305 → true + dt_to_burn = 0 + remaining_dt = 10 + 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`. + +### Issue 5: Hardcoded Array Size + +**Location:** `src/simulation.cpp`, static declaration + +```cpp +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. + +### Issue 6: Duplicated Propagation Logic + +The "normal" propagation path in `update_spacecraft_physics()` and the maneuver path in `execute_pending_maneuvers()` are essentially duplicates: + +**Normal path** (`update_spacecraft_physics`): +```cpp +propagate_orbital_elements(craft->orbit, sim->dt, ...); +orbital_elements_to_cartesian(...); +``` + +**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(...); +``` + +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. + +## Call Sites Summary + +| 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` | + +Total: 5 call sites, 3 distinct contexts (normal propagation, sub-step execution, probe).