From b221251787cea1508e3f07a636ae582260b1f677 Mon Sep 17 00:00:00 2001 From: cinnaboot Date: Wed, 28 Jan 2026 12:15:18 -0500 Subject: [PATCH] Remove altitude parameter to fix spacecraft initialization bug - Remove altitude convenience parameter from body and spacecraft config parsing - Update test configs to use explicit semi_major_axis instead of altitude - Remove buggy post-processing loop that added parent radius to all spacecraft - Result: Semi-major axis now used as-is (correct behavior) - Test results: Molniya position tests now pass (was failing by 1-11M meters) - Documentation: Remove altitude references from technical reference - Planning: Document bug fix decision and implementation details Changes: - src/config_loader.cpp: Remove altitude parsing and post-processing - tests/test_maneuver_planning.toml: Use semi_major_axis = 6.771e6 - tests/test_maneuvers.toml: Use semi_major_axis = 6.771e6 - tests/test_orbit_rendering.toml: Use semi_major_axis = 6.771e6 - docs/technical_reference.md: Remove altitude documentation - docs/planning/molniya-orbit-test-plan.md: Document bug fix approach --- docs/planning/molniya-orbit-test-plan.md | 70 ++++++++++++++++++++++++ docs/technical_reference.md | 1 - src/config_loader.cpp | 42 ++++---------- tests/test_maneuver_planning.toml | 3 +- tests/test_maneuvers.toml | 3 +- tests/test_orbit_rendering.toml | 3 +- 6 files changed, 88 insertions(+), 34 deletions(-) diff --git a/docs/planning/molniya-orbit-test-plan.md b/docs/planning/molniya-orbit-test-plan.md index 44c526f..c3a91cd 100644 --- a/docs/planning/molniya-orbit-test-plan.md +++ b/docs/planning/molniya-orbit-test-plan.md @@ -246,3 +246,73 @@ Where: ## Files to Potentially Modify (future 3D implementation) - `src/orbital_mechanics.cpp` - add 3D rotation logic - `src/orbital_mechanics.h` - potentially expose rotation helper functions + +## Bug Fix Implementation + +### Decision Made +**Remove altitude parameter entirely** rather than fix it, based on: +- Minimal usage: Only 3 test configs, 0 production configs +- High complexity cost: Requires tracking parameter usage, post-processing loop +- Confusing semantics: Bodies say they'll add radius but don't +- Clean alternative: Explicit semi_major_axis is unambiguous + +### Changes Implemented + +#### 1. Updated Test Configs +Replaced `altitude` with explicit `semi_major_axis` in: +- `tests/test_maneuver_planning.toml` +- `tests/test_maneuvers.toml` +- `tests/test_orbit_rendering.toml` + +**Change pattern:** +```toml +# OLD: +orbit = { altitude = 400000.0, ... } + +# NEW: +orbit = { semi_major_axis = 6.771e6, ... } +# LEO orbit: 400 km altitude (Earth radius 6.371e6 m + 400e3 m) +``` + +#### 2. Removed Altitude from Body Parsing +**File**: `src/config_loader.cpp` (lines 70-73) +- Removed `altitude` variable declaration +- Removed altitude handling block (lines 99-101) +- Updated error messages to only mention `semi_major_axis` + +#### 3. Removed Altitude from Spacecraft Parsing +**File**: `src/config_loader.cpp` (lines 296-299) +- Removed `altitude` variable declaration +- Removed altitude handling block (lines 325-327) +- Updated error messages to only mention `semi_major_axis` + +#### 4. Removed Buggy Post-Processing Loop +**File**: `src/config_loader.cpp` (lines 249-257) +- Deleted entire loop that unconditionally added parent radius to spacecraft semi_major_axis +- This was the root cause of the bug + +#### 5. Updated Technical Reference +**File**: `docs/technical_reference.md` (line 317) +- Removed documentation of altitude convenience feature +- Simplified to only mention `semi_major_axis` as required parameter + +### Test Results After Fix + +**Molniya Tests:** +- Position verification: ✅ PASS (radius error: 0 m, was 1.7M m) +- Z-coordinate checks: ⚠️ FAIL (expected, 3D deferred) +- Orbital period: ⚠️ INCOMPLETE (separate OrbitTracker issue, not related to bug) + +**All Tests:** +- 36 tests passed +- 3 tests failed as expected (Molniya 3D tests) +- 0 regressions + +### Remaining Issues + +1. **OrbitTracker not completing orbits** for Molniya period test + - May need to increase `MAX_SIMULATION_HOURS` or fix tracking logic + - Separate from altitude parameter bug + +2. **3D orientation not implemented** (documented as deferred) + - Molniya tests will fail z-coordinate checks until implemented diff --git a/docs/technical_reference.md b/docs/technical_reference.md index ef0dea6..d773100 100644 --- a/docs/technical_reference.md +++ b/docs/technical_reference.md @@ -314,7 +314,6 @@ orbit = { } ``` - Uses `orbit` table for orbital elements (same as bodies) -- Altitude can be used instead of `semi_major_axis` for non-parabolic orbits (convenience feature, added to parent radius) - Optional 3D orbital elements (inclination, longitude_of_ascending_node, argument_of_periapsis) are defined but not yet applied (deferred) **Config format (TOML) - Maneuvers:** diff --git a/src/config_loader.cpp b/src/config_loader.cpp index 7798c0f..85f02d3 100644 --- a/src/config_loader.cpp +++ b/src/config_loader.cpp @@ -67,10 +67,9 @@ static bool parse_toml_body(toml_datum_t body_table, CelestialBody* body) { body->orbit.argument_of_periapsis = 0.0; body->orbit.true_anomaly = 0.0; - // Parse semi_major_axis (for elliptical/hyperbolic) or semi_latus_rectum (for parabolic) or altitude (convenience) + // Parse semi_major_axis (for elliptical/hyperbolic) or semi_latus_rectum (for parabolic) toml_datum_t semi_major = toml_get(orbit_table, "semi_major_axis"); toml_datum_t semi_latus = toml_get(orbit_table, "semi_latus_rectum"); - toml_datum_t altitude = toml_get(orbit_table, "altitude"); // Parse eccentricity first to determine which parameter is required toml_datum_t eccentricity = toml_get(orbit_table, "eccentricity"); @@ -85,22 +84,19 @@ static bool parse_toml_body(toml_datum_t body_table, CelestialBody* body) { if (is_parabolic) { // Parabolic orbit - requires semi_latus_rectum if (semi_latus.type != TOML_FP64) { - printf("Error: Parabolic orbit for body '%s' requires 'semi_latus_rectum' (not 'semi_major_axis' or 'altitude')\n", body->name); + printf("Error: Parabolic orbit for body '%s' requires 'semi_latus_rectum' (not 'semi_major_axis')\n", body->name); return false; } body->orbit.semi_latus_rectum = semi_latus.u.fp64; - if (semi_major.type == TOML_FP64 || altitude.type == TOML_FP64) { - printf("Warning: Body '%s' has parabolic eccentricity, 'semi_latus_rectum' used instead of 'semi_major_axis' or 'altitude'\n", body->name); + if (semi_major.type == TOML_FP64) { + printf("Warning: Body '%s' has parabolic eccentricity, 'semi_latus_rectum' used instead of 'semi_major_axis'\n", body->name); } } else { - // Elliptical or hyperbolic - requires semi_major_axis or altitude + // Elliptical or hyperbolic - requires semi_major_axis if (semi_major.type == TOML_FP64) { body->orbit.semi_major_axis = semi_major.u.fp64; - } else if (altitude.type == TOML_FP64) { - // altitude will be added to parent radius in initialization - body->orbit.semi_major_axis = altitude.u.fp64; } else { - printf("Error: Body '%s' must have either 'semi_major_axis' or 'altitude' in orbit table (non-parabolic orbits)\n", body->name); + printf("Error: Body '%s' must have 'semi_major_axis' in orbit table (non-parabolic orbits)\n", body->name); return false; } if (semi_latus.type == TOML_FP64) { @@ -250,16 +246,6 @@ static bool load_spacecraft_from_toml(SimulationState* sim, toml_result_t result } } - for (int i = 0; i < sim->craft_count; i++) { - Spacecraft* craft = &sim->spacecraft[i]; - bool is_parabolic = (fabs(craft->orbit.eccentricity - 1.0) < 0.005); - - if (!is_parabolic && craft->parent_index >= 0 && craft->parent_index < sim->body_count) { - CelestialBody* parent = &sim->bodies[craft->parent_index]; - craft->orbit.semi_major_axis += parent->radius; - } - } - printf("Loaded %d spacecraft from %s\n", craft_count, sim->config_name); return true; } @@ -297,10 +283,9 @@ static bool parse_toml_spacecraft(toml_datum_t craft_table, Spacecraft* craft) { craft->orbit.argument_of_periapsis = 0.0; craft->orbit.true_anomaly = 0.0; - // Parse semi_major_axis (for elliptical/hyperbolic) or semi_latus_rectum (for parabolic) or altitude (convenience) + // Parse semi_major_axis (for elliptical/hyperbolic) or semi_latus_rectum (for parabolic) toml_datum_t semi_major = toml_get(orbit_table, "semi_major_axis"); toml_datum_t semi_latus = toml_get(orbit_table, "semi_latus_rectum"); - toml_datum_t altitude = toml_get(orbit_table, "altitude"); // Parse eccentricity first to determine which parameter is required toml_datum_t eccentricity = toml_get(orbit_table, "eccentricity"); @@ -315,22 +300,19 @@ static bool parse_toml_spacecraft(toml_datum_t craft_table, Spacecraft* craft) { if (is_parabolic) { // Parabolic orbit - requires semi_latus_rectum if (semi_latus.type != TOML_FP64) { - printf("Error: Parabolic orbit for spacecraft '%s' requires 'semi_latus_rectum' (not 'semi_major_axis' or 'altitude')\n", craft->name); + printf("Error: Parabolic orbit for spacecraft '%s' requires 'semi_latus_rectum' (not 'semi_major_axis')\n", craft->name); return false; } craft->orbit.semi_latus_rectum = semi_latus.u.fp64; - if (semi_major.type == TOML_FP64 || altitude.type == TOML_FP64) { - printf("Warning: Spacecraft '%s' has parabolic eccentricity, 'semi_latus_rectum' used instead of 'semi_major_axis' or 'altitude'\n", craft->name); + if (semi_major.type == TOML_FP64) { + printf("Warning: Spacecraft '%s' has parabolic eccentricity, 'semi_latus_rectum' used instead of 'semi_major_axis'\n", craft->name); } } else { - // Elliptical or hyperbolic - requires semi_major_axis or altitude + // Elliptical or hyperbolic - requires semi_major_axis if (semi_major.type == TOML_FP64) { craft->orbit.semi_major_axis = semi_major.u.fp64; - } else if (altitude.type == TOML_FP64) { - // altitude will be added to parent radius in initialization - craft->orbit.semi_major_axis = altitude.u.fp64; } else { - printf("Error: Spacecraft '%s' must have either 'semi_major_axis' or 'altitude' in orbit table (non-parabolic orbits)\n", craft->name); + printf("Error: Spacecraft '%s' must have 'semi_major_axis' in orbit table (non-parabolic orbits)\n", craft->name); return false; } if (semi_latus.type == TOML_FP64) { diff --git a/tests/test_maneuver_planning.toml b/tests/test_maneuver_planning.toml index 0617d68..cdc98eb 100644 --- a/tests/test_maneuver_planning.toml +++ b/tests/test_maneuver_planning.toml @@ -31,10 +31,11 @@ name = "LEO_Satellite" mass = 1000.0 parent_index = 1 orbit = { - altitude = 400000.0, + semi_major_axis = 6.771e6, eccentricity = 0.0, true_anomaly = 0.0 } +# LEO orbit: 400 km altitude (Earth radius 6.371e6 m + 400e3 m) [[maneuvers]] name = "orbit_raise_1" diff --git a/tests/test_maneuvers.toml b/tests/test_maneuvers.toml index 5a0a543..1ff584a 100644 --- a/tests/test_maneuvers.toml +++ b/tests/test_maneuvers.toml @@ -31,7 +31,8 @@ name = "LEO_Satellite" mass = 1000.0 parent_index = 1 orbit = { - altitude = 400000.0, + semi_major_axis = 6.771e6, eccentricity = 0.0, true_anomaly = 0.0 } +# LEO orbit: 400 km altitude (Earth radius 6.371e6 m + 400e3 m) diff --git a/tests/test_orbit_rendering.toml b/tests/test_orbit_rendering.toml index 7ed0b31..8688815 100644 --- a/tests/test_orbit_rendering.toml +++ b/tests/test_orbit_rendering.toml @@ -31,7 +31,8 @@ name = "LEO_Satellite" mass = 1000.0 parent_index = 1 orbit = { - altitude = 400000.0, + semi_major_axis = 6.771e6, eccentricity = 0.0, true_anomaly = 0.0 } +# LEO orbit: 400 km altitude (Earth radius 6.371e6 m + 400e3 m)