feat(ui): Sprint 35 — Atlas of the Reach (Phase 3 planetary viewer) #128
Assignees
jpmschweitzer (Jeroen Schweitzer)
Clear assignees
jpmschweitzer (Jeroen Schweitzer)
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: jpmschweitzer/settled-reach#128
Reference in New Issue
Block a user
Summary
Sprint 35 client work — Atlas of the Reach (Phase 3 planetary viewer), all three stories in the chain.
implant/map/atlasFULLSCREEN app (z=20) with three nav levels: system picker (◄ ► cycle), orbital diagram (star-centred with bodies grouped byorbit_index, moons sub-orbiting parents, stations as clickable markers with mini data panel), and body entry (data sheet). Composed from the D-169 implant component library.tooling/generate-star-map-data.pyextended to emit per-systemcurrency_zone,orbit_bodies, andstationsarrays fromsystems.db;star_map_data.jsonregenerated deterministically. Wired intohud.tscn, toggled via the A key inmain.gd, insert-state propagated throughsnapshot_consumers.gd.AtlasViewercontrol loads a body'sterrain_referencePNG andmarkers.json, renders heightmap + markers in texture-space via anAtlasMarkerOverlayNode2D child of a transformed canvas (pan = offset, zoom = scale, cursor-centred wheel zoom). Handles emptymarkers.jsonand missingterrain_referencegracefully (themed "terrain data pending (#839)" notice). City data sidebar rebuilds on click with name, pop tier, function, currency zone, Commission presence, shadow zone, gate distance. Pressing N on a selected city emitseconomics_link_requested(system_id)—main.gdbridges this toEconomicsPanel.select_system()+HudGroups.open_app("implant/economics"), satisfying the D-191 Phase 2/3 cross-panel integration.AtlasOverlayBarHBoxContainer docked top-right of the viewer with 11 short-label buttons mapped to the D-181 signal visibility ladder: 5 always-on (TER/INF/NAM/GAT/POL), 4 toggleable (POP/PRD/SHD/CRP), 2 locked (STK/BSL — disabled + tooltipped with unlock requirements). Writes throughAtlasViewer.set_overlay_visible(), which is the single entry point into the per-overlay visibility dict consumed byAtlasMarkerOverlay._draw().Implements D-191 criteria 1, 4, 5, 6, 8.
Notes for reviewers
#839has not yet populatedterrain_referenceinsystems.db, so the viewer will show the "terrain data pending" empty state on all bodies until the server atlas pipeline runs. Rendering against real heightmaps needs a downstreamstar_map_data.jsonregen once#839lands.markers.jsonfiles for most bodies currently have emptycities/roads/rail/poisarrays — the overlay handles this without crashing. Once server#832/#833populate them, markers appear automatically with no client change.Test plan
terrain_referenceis populated: drag to pan, wheel to zoom, R to reset, click a city → sidebar, N → economics monitor opens pre-filtered to the systemAtlasOverlayBar is an HBoxContainer docked top-right of the heightmap viewer with 11 short-label buttons mapping to D-191 §7 / D-181 signal visibility: always-on (5) TER INF NAM GAT POL — terrain, infrastructure, named features, gate markers, political zones. Pinned on; clicks are swallowed so the layers can't accidentally be disabled. toggleable (4) POP PRD SHD CRP — population density, production zones, shadow economy, corporate presence. Reflect and mutate viewer overlay state. locked (2) STK BSL — stockpile_weeks, production_vs_baseline. Disabled and greyed out with unlock-requirement tooltips, per D-181 semi-private/private tiers; kept in the bar so players see that deeper data exists and is gated. Each button writes through AtlasViewer.set_overlay_visible(), which is the single entry point into the viewer's _overlay_visibility dict consumed by AtlasMarkerOverlay._draw(). Locked overlays short-circuit in that setter. Per D-191 criterion 6.PR Review — sprint-35/client → main (2-reviewer consolidated)
Reviewers: Hoshe (code quality), Tyre (architecture)
Verdict: CHANGES REQUESTED
Architecturally the PR is well-shaped: the 4-module decomposition (AtlasPanel / AtlasViewer / AtlasMarkerOverlay / AtlasOverlayBar) is clean, D-170 HUD registration is correct, the economics cross-link signal path is the right API shape, the city-data sidebar composes from the implant component library correctly, and pan/zoom math is right. Pre-flight was clean:
gdlintreported no problems on all 6 touched files, headlessgodot --path client --quitproduced no new SCRIPT ERROR lines.The blockers are concentrated in two real bugs and one contract mismatch between the overlay code and the D-191 §8 markers schema. The rest is warnings and polish.
D-191 compliance summary (Tyre)
atlas_panel.gd._draw_header()andatlas_viewer.gd._draw_header()hand-roll the top-level title/hint withdraw_string(get_theme_default_font(), …, COLOR_TEXT)— bypasses the implant theme and duplicates ImplantHeader.implant/map/atlasin FULLSCREEN mode;open_appemitsgameplay_occluded;_on_app_changedroutes mode → visibility. Correct.TODO: migrate to HudGroups.open_app("implant/map/starchart")— atlas and Reach star map are currently separate apps, not one extended chain. Not this PR's fault (star_map.gd predates D-170), but criterion 1 is technically not end-to-end until the two are linked. Acceptable for this PR; flag as follow-up.AtlasViewer.economics_link_requested(system_id)→AtlasPanel.economics_link_requested→main._on_atlas_economics_link→EconomicsPanel.select_system()+HudGroups.open_app("implant/economics", INSERT). Clean signal path.terrain,infrastructure,named_features,gate_markers,population_densityrender real data.political_zonesdraws a flat tint (flagged as MVP placeholder in comment). 3 of 4 toggleable overlays (production_zones,shadow_economy,corp_presence) readmarkers.production_zones/markers.shadow_zones/markers.corp_presence— none of these keys exist in D-191 §8's markers schema. Toggling draws nothing. See comment #3 below._station_panelImplantPanel with name, operator, function, currency, deferred hint, no drill-down. Matches spec.Blocking — fix before merge
atlas_panel.gd:240–2444.0.TAU * float(moon_idx - 1) / 4.0— if a parent body has 5 or more moons, moon 5 lands at the same angle as moon 1, moon 6 overlaps moon 2, etc. Dots render on top of each other and become unclickable. Fix: use the count of moons per parent body as the divisor, not the constant.atlas_viewer.gd:198–199(vs line 202 null check)_grid_w/_grid_hwhen a body has no heightmap after one that did._grid_wand_grid_hare set from_tex_w/_tex_hon lines 198–199 before theterrain_referencenull check at line 202. When showing a body withterrain_reference = nullafter a body that had one, the grid dimensions stay pointed at the previous body's texture.grid_to_canvas()then uses stale dimensions and markers render in the wrong place. Fix: move the grid assignment to after the null check, or reset to safe defaults whenterrain_referenceis null.atlas_marker_overlay.gd:203–220_draw_production_zones,_draw_shadow_economy,_draw_corp_presencereadmarkers.production_zones,markers.shadow_zones,markers.corp_presence— none are in D-191 §8's markers schema (cities,roads,railroads,pois,rivers,oceans,mountain_ranges). No generator will populate them. Toggling these overlays is a silent no-op. Fix options: (a) derive from existing data — e.g.corp_presencefromcities[].commission_presence; or (b) amend D-191 §8 schema and add a follow-up ticket for the server generator. Current state makes criterion 6's "present AND functional" false for 3/4 toggleable overlays.Warnings — fix or document
atlas_panel.gd:288–291,atlas_viewer.gd:301–308draw_string(get_theme_default_font(), …). Picker/body/viewer levels draw their title and hint strings directly instead of composing ImplantHeader. Bypasses the implant theme's font + semantic color roles; breaks the D-169 "Theme swap changes the implant hardware appearance" invariant. Replace with an ImplantHeader at the top of each panel.atlas_marker_overlay.gd:161, 170–179, 197viewer._city_canvas_pos(c),viewer._hovered_city,viewer._selected_city— all_-prefixed members accessed from outside. AtlasViewer already publishesget_heightmap_texture()/get_markers()/grid_to_canvas()as the public API. Promote_city_canvas_postocity_canvas_pos()and addget_hovered_city()/get_selected_city()(or inject them as args on a publicrender(...)method).vieweris untyped (var viewer = null), so renaming the private fields will silently break at runtime.atlas_viewer.gd:488–498vsmain.gd:187_unhandled_key_inputswallows KEY_N only when_selected_cityis non-empty. When no city is selected, KEY_N falls through tomain.gd:187which callseconomics_panel.toggle_visible()→HudGroups.open_app("implant/economics", INSERT), which closes the FULLSCREEN atlas as a side effect. Surprising UX. Fix: consume KEY_N unconditionally while the viewer is visible, or gate main.gd's KEY_N handler onnot HudGroups.is_app_active("implant/map/atlas").atlas_viewer.gd:81–97vsatlas_overlay_bar.gd:18–42_overlay_visibility/_overlay_lockedin AtlasViewer andOVERLAY_DEFSin AtlasOverlayBar both hardcode the same overlay id list. If they drift, the "locked" classification in the bar and the guard inset_overlay_visiblecan disagree silently. Single source of truth: move the overlay definitions to a shared const.atlas_overlay_bar.gd:53–56class_name AtlasOverlayBar+_init(viewer_ref)with no default is a Godot editor footgun. Any editor script introspection or hot-reload that tries to instantiate the class without args crashes. Either make it_init(viewer_ref = null)with lazy assignment in_ready(), or dropclass_nameand instance viaload(...)as atlas_panel already does for AtlasViewer. (Compare ImplantHeader/DataRow/TextBlock which all use the default-arg pattern.)generate-star-map-data.py:196–238terrain_referenceregen dependency is tribal knowledge. AtlasViewer's pivot from "data pending" → heightmap-ready is gated on this script having been re-run after server #839 populatesterrain_referencein systems.db. There is no Make target dependency or CI assertion for this. Addatlas-generate→ star-map-data regen as a make target chain, or a--checkassertion so the regen step becomes automatic instead of lore.Polish
atlas_panel.gd:591ImplantDataRow.new(sys.get("bodies", "no body data"))— the"no body data"fallback is dead code (generator always writesentry["bodies"]). Remove or replace with"—"to match the panel's empty-state convention.atlas_viewer.gd:293–298_draw()mutates child-node transform._draw()sets_canvas.position/_canvas.scaleevery redraw, duplicating what_apply_transform()already does. Causes extraNOTIFICATION_TRANSFORM_CHANGEDchurn. Remove the duplicate assignment from_draw().atlas_panel.gd:88,atlas_panel.gd:667–673load("res://ui/implant/atlas_viewer.gd").new()dance at 667–673 imply a parse-order risk that doesn't exist for this class.var _viewer := AtlasViewer.new()would work fine. Remove the misleading comments or convert to the direct form.atlas_marker_overlay.gd:177,178if c == selected/elif c == hoveredis O(fields) per city per redraw. Fine at 100 cities × 10 fields but balloons before Phase 4. Compare bybody_id/ index instead.atlas_panel.gd:476,484b["body_id"]/s["station_id"]click-handler indexing. If the generator ever emits a NULL id the handler crashes.var bid: String = str(b.get("body_id", ""))with anis_empty()guard matches the defensive style elsewhere in the file.atlas_viewer.gd:149–156set_overlay_visible()silently returns on unknown overlay ids. Combined with #7's drift risk, a typo in the bar would do nothing. Addpush_warning("unknown overlay id %s" % overlay_id)on the unknown branch.atlas_overlay_bar.gd:79, 93–96pressedto a handler that re-setsbutton_pressed = trueon a non-toggle Button (no visual effect after first set). Simpler:disabled = truefor always-on buttons and drop the handler.generate-star-map-data.pyhelperscompute_gdp,infer_tier_from_population,build_adjacency,parse_wiki_index. Client-side test coverage is sparse project-wide (not a blocker), butcompute_gdp's boundary at exactly 1T Tr (BTr → TTr format switch) is one display-facing test worth adding.Pre-flight results (for the record)
gdlintonatlas_panel.gd / atlas_viewer.gd / atlas_marker_overlay.gd / atlas_overlay_bar.gd / main.gd / snapshot_consumers.gd: no problems found.godot --headless --path client --quiton the sprint-35/client worktree: no new SCRIPT ERROR lines introduced. The 23 pre-existing autoload parse errors (sim_bridge, protocol, input_mapper, audio_manager) are unchanged baseline and out of scope.Re-review cadence
3 blockers, 6 warnings, 8 polish items. Once fixed,
/pr-pushand re-review. Criterion 1 (star map ↔ atlas app unification) and the helper unit tests (finding 17) can be follow-up tickets rather than blockers on this PR.4. AtlasPanel and AtlasViewer now compose their title + hint from an ImplantHeader child rather than hand-rolling them via draw_string, so the D-169 "theme swap changes the implant hardware appearance" invariant holds end-to-end. _refresh_screen_header() drives content per level and on system navigation. 5. AtlasViewer exposes city_canvas_pos(), get_hovered_city(), get_selected_city(), and get_overlay_defs() as public API — the marker overlay no longer reaches into underscore-prefixed state, which is especially important because viewer is an untyped var in the overlay. 6. KEY_N now consumes unconditionally while the viewer is visible, and main.gd's global economics-monitor toggle is gated on !HudGroups.is_app_active("implant/map/atlas"). Previously pressing N without a selected city fell through and closed the fullscreen atlas as a side effect. 7. OVERLAY_DEFS lives in AtlasViewer as the single source of truth. AtlasOverlayBar reads the list via viewer.get_overlay_defs(), and AtlasViewer derives _overlay_visibility / _overlay_locked from the same table at _ready() — no more hand-maintained parallel lists, so the bar and the guard in set_overlay_visible can't drift. 8. AtlasOverlayBar drops `class_name`: it now loads via load("res://ui/implant/atlas_overlay_bar.gd") from AtlasViewer, the same pattern AtlasPanel uses for AtlasViewer. _init(viewer_ref = null) keeps the required-arg footgun off the editor's introspection path. 9. `star-map-data` make target added to regenerate client/data/star_map_data.json from systems.db + wiki, and `check-star-map` wired into pre-pr-validate + pre-pr-client so any commit that touches the generator (or any downstream systems.db change like server #839) fails pre-pr until the JSON is regenerated. The terrain_reference data-availability dependency is no longer tribal knowledge. Also addresses review #15 (push_warning on unknown overlay id in set_overlay_visible) and #16 (disabled always-on buttons drop handler churn) as part of the same refactor.10. Dropped the "no body data" fallback string in the picker panel — the generator always writes entry["bodies"], so the fallback was dead. Use the "—" convention the rest of the panel already follows. 12. Removed the autoload parse-order comment from AtlasPanel (it is scene-instanced, not an autoload, so the rule does not apply), and collapsed _build_heightmap_viewer to a direct AtlasViewer.new() — mirroring the rest of the file rather than dancing around a risk that is not real for this class. 13. AtlasMarkerOverlay._draw_cities now compares hover/selection by a stable _city_key() (name → city_id → pos → hash) instead of Dictionary.==, which was O(fields) per city per redraw. Preps the renderer for much larger city counts without a rewrite. 14. Orbital click/draw handlers guard against missing body_id / station_id by reading through str(dict.get(..., "")) and skipping empty ids. Matches the defensive style already used for parent_body_id and keeps a NULL id from crashing _draw_bodies / _handle_orbital_click.Review response — blockers + warnings + polish all addressed
Pushed four follow-up commits in the order the review laid them out. Pre-flight on the current branch is clean:
gdlintzero,make check-star-mapOK, headless godot no new script errors.gdformatreports one pre-existing advisory onclient/ui/stance_indicator.gdwhich is out of scope for this branch.Blockers →
1328db31AtlasPanel._compute_body_positionsnow groups moons per parent and dividesTAUby the per-parent count instead of the hardcoded4.0. Gas giants with 5+ moons no longer overlap satellites._load_heightmapresets_heightmap_texture/_tex_w/_tex_hto sentinel defaults at the top so opening a body with no heightmap after one that had one can no longer inherit the previous texture's dimensions._draw_production_zones/_draw_shadow_economy/_draw_corp_presencenow read fromcities[](primary_function filter, commission_presence inverse, commission_presence positive respectively). Option (a) from the review. Follow-up ticket #846 tracks formalising per-overlay arrays in D-191 §8 with the server team.Warnings →
f8b81e1fAtlasPanelandAtlasViewernow compose top-of-screen title/hint from anImplantHeaderchild (_build_screen_header+_refresh_screen_header). Alldraw_string(get_theme_default_font(), …)calls in the atlas UI are gone; swapping the implant theme now cascades to the screen header.AtlasViewerexposescity_canvas_pos(),get_hovered_city(),get_selected_city(), andget_overlay_defs().AtlasMarkerOverlayno longer reaches into underscore-prefixed fields.AtlasViewer._unhandled_key_inputnow consumesKEY_Nunconditionally while the viewer is visible, andmain.gd's global economics-monitor toggle is gated on!HudGroups.is_app_active("implant/map/atlas"). No more atlas-closes-as-side-effect.OVERLAY_DEFSmoved toAtlasViewer;AtlasOverlayBarreadsviewer.get_overlay_defs()andAtlasViewer._overlay_visibility/_overlay_lockedderive from the same table at_ready(). Parallel lists eliminated.class_namefootgun —AtlasOverlayBardropsclass_name;AtlasViewerinstances it viaload("res://ui/implant/atlas_overlay_bar.gd").new(self)._init(viewer_ref = null)also has a default so an editor-introspection path can't crash.star-map-datatarget (regen) andcheck-star-mapis now wired intopre-pr-client+pre-pr-validate. Pre-PR will fail when the JSON is stale instead of relying on tribal knowledge. Help text updated.Polish →
489aec48"no body data"fallback replaced with"—"._draw()transform duplication — already addressed in the warnings pass (background rect only).AtlasPanel;_build_heightmap_viewernow just callsAtlasViewer.new()directly._city_key()(name → city_id → pos → lat/lon → hash). Ready for Phase 4 city counts.b["body_id"]/s["station_id"]indexing guarded viastr(dict.get(..., ""))+is_empty()in_compute_body_positions,_draw_bodies,_draw_stations, and_handle_orbital_click.set_overlay_visible()now emits apush_warningon unknown overlay ids.AtlasOverlayBararedisabled = truewith a pinnedfont_disabled_coloroverride — no handler churn.Format →
22c17f1fRan
gdformaton the three modified atlas files to clear the pre-push advisory.stance_indicator.gd's advisory is pre-existing and outside the scope of this PR.Deferred to follow-up tickets
implant/mapchain (D-191 criterion 1 end-to-end).generate-star-map-data.pyhelpers (compute_gdp boundary, etc).Ready for re-review.
PR Review Round 2 — sprint-35/client → main
Reviewers: Hoshe (code quality), Tyre (architecture)
Verdict: APPROVED (2/2)
Fix commit chain (
1328db31 fix → f8b81e1f refactor → 489aec48 polish → 22c17f1f gdformat) cleanly resolves every R1 finding. No regressions introduced; the +240 net lines are all legitimate fix work (OVERLAY_DEFS const + getters, ImplantHeader composition, schema-compliant overlay derivations, public AtlasViewer API). TheOVERLAY_DEFSconsolidation is particularly elegant — one const on AtlasViewer now feeds both the marker renderer (viais_overlay_visible) and the toggle bar (viaget_overlay_defs), eliminating the R1 drift risk and making #836 extensions a one-file change.D-decision re-compliance
draw_stringtitle/hint)atlas_viewer.gd:387+atlas_panel.gd:332composeImplantHeaderwithapply_implant_theme. On-canvas body/station labels outside D-169 scope.HudGroups.is_app_activeadded inmain.gd:191primary_function(PRODUCTION_FUNCTIONS whitelist) andcommission_presence— schema-compliant fields; doc comments acknowledge future explicit-schema promotionPer-finding resolution
4.0(5+ moons overlap)atlas_panel.gd:252–267groups moons by parent viamoons_by_parentdict, usessiblings.size()as the divisor; comment at 237–239 cites review #1_grid_w/_grid_hacross bodiesatlas_viewer.gd:260–268resets_tex_w=1024 / _tex_h=512unconditionally before any null guard;_load_markers()immediately assigns grid defaults before theterrain_referencenull check; comment at 285–287 cites review #2atlas_marker_overlay.gd:260–292derive production/shadow/corp overlays fromprimary_function+commission_presence(schema-compliant). Softly-functional (all false → shadow renders everywhere, corp renders nothing) but produces visible output; acceptable MVP until schema grows explicit zone polygonsdraw_stringtitle/hintatlas_viewer.gd:383–402andatlas_panel.gd:328–373compose ImplantHeader viaset_content+apply_implant_theme(_implant_theme), matchingimplant_header.gdAPIcity_canvas_pos,get_hovered_city,get_selected_city,get_heightmap_texture,get_markers,get_overlay_defs,is_overlay_visibleat 235–252, 499); marker overlay calls public API throughoutatlas_viewer.gd:582–597_unhandled_key_inputunconditionally consumes KEY_N viaset_input_as_handled()while visible ANDmain.gd:191gates the global handler onnot HudGroups.is_app_active("implant/map/atlas")OVERLAY_DEFSatatlas_viewer.gd:58–125;atlas_overlay_bar.gd:34iterates via_viewer.get_overlay_defs(); no dict copy in the barclass_name AtlasOverlayBar+ required_initfootgunclass_nameremoved;_init(viewer_ref = null)with default at line 25; loaded viaload().new(self)atatlas_viewer.gd:640–641generate-star-map-data.pyterrain_reference tribal knowledgeMakefile:377check-star-maprunsgenerate-star-map-data.py --check(generator exits 1 on drift at lines 329, 336); wired intopre-pr-client(307) andpre-pr-validate(265). Any PR that mutatessystems.dbwithout regeneratingstar_map_data.jsonfails the pre-PR gate."no body data"dead fallbackatlas_panel.gd:649now uses"—"matching the panel empty-state convention_draw()mutates child transformatlas_viewer.gd:379–380_draw()now only draws the background rect; canvas mutation happens only in_apply_transform()atlas_panel.gd:88directload()with no misleading autoload comment;_viewer = AtlasViewer.new()at 723 via class_name directly_draw()callatlas_marker_overlay.gd:190–198uses_city_key()string comparison;_city_key(222–238) prefersname→city_id→ positional → hashb["body_id"]/s["station_id"]atlas_panel.gd:533str(b.get("body_id", "")), 541str(s.get("station_id", ""));_compute_body_positionsskips empty idsset_overlay_visible()silent return on unknown idatlas_viewer.gd:220–222push_warning("AtlasViewer: unknown overlay id '%s'" % overlay_id)atlas_overlay_bar.gd:46–50usesdisabled = true, button_pressed = truewithfont_disabled_coloroverride; no.toggled.connect()on always groupgenerate-star-map-data.pyhelpersRegressions
None. Line growth is all legitimate fix work:
atlas_viewer.gd+102: OVERLAY_DEFS const + getters (+67) + screen header composition (+26) + public API helpersatlas_panel.gd+59: screen header composition (+45) + moon grouping logic (+14)atlas_marker_overlay.gd+74: three schema-compliant overlay draw functions +_city_keyhelperatlas_overlay_bar.gd−35: const moved upstream,class_nameremoved, always-on handler droppedPR #128 is approved for merge.
Pull request closed