feat(ui): sprint 36 client — ImplantApp pattern + atlas unification + tooling #131
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#131
Reference in New Issue
Block a user
Summary
Sprint 36 client — three shipped tickets plus a foundational implant-app architecture refactor.
Tickets:
implant/mapHudGroups app per D-191. Atlas opens at the hop-ring Reach view as the entry screen, navigates down through system → planet → heightmap in one continuous session. Single key binding (M); KEY_A removed.project.yaml) and protocol version, bottom-centre.sqlite-queryandsqlite-execrespond to--help/-hwith proper usage text (previously returned empty JSON).Foundational refactor —
ImplantApppattern:The implant is now treated as a faux mobile OS, and implant apps as faux mobile apps. This is the foundational shape for future implant apps and the seam for modded content.
New classes (
client/ui/implant/):implant_app_manifest.gd—Resourceclass declaringapp_path,display_name,scene_path,default_key,default_mode,preserves_state.implant_nav_stack.gd— intra-app navigation helper (push/pop/replace/reset_to_default, synchronousscreen_changedsignal).implant_app.gd— base class all implant apps extend. Absorbs anchoring, visibility,HudGroupsregistration,app_changedfiltering,set_insert_active. Lifecycle hooks:on_install,on_open,on_close,on_insert_deactivated.implant_registry.gd— new autoload. Scansres://ui/implant/apps/*/app.treslazily on firstget_manifests()call (avoids theclass_nameautoload parse-order trap).Apps migrated to the new pattern:
client/ui/implant/apps/atlas/— manifest, shell (atlas_app.gdextendingImplantApp), four screens (reach_screen,system_screen,planet_screen,regional_screen) as plainControls withenter/leaveinterface, overlays, heightmap viewer.client/ui/implant/apps/economics/— manifest, shell (economics_app.gd), overview screen with preserved selection state.Generic key routing:
main.gd's hardcodedKEY_M/KEY_Ndispatch replaced with a registry-driven loop that readsdefault_keyfrom each app's manifest. New apps drop intoapps/<name>/and bind automatically.Cleanup:
implant/map/starchartHudGroups path deleted (star_map.gd/star_map.tscnremoved).SnapshotConsumerscallson_insert_deactivated()uniformly instead of per-panel type-specific plumbing.atlas_panel.gd(1252 lines) andeconomics_panel.gdreplaced by theapps/shells.Architecture reference:
docs/architecture/implant-app-pattern.md(commitbe9cfa2f) documents the pattern, lifecycle contract, directory layout, nav-stack edge cases, deferred work (Intents dispatcher,DataChannelsautoload, mod SDK), and a#844review checklist.Key decisions
implant/mapis the unified HudGroups path (supersedesimplant/map/atlasandimplant/map/starchart).main.gdbranch.ImplantApp. New apps require zero core-code edits: drop a directory underapps/, ship anapp.tres, restart.ImplantNavStack. No more ad-hocLevelenums.SYSTEM_PICKER/ORBITAL_DIAGRAMlevel asymmetry fixed — forward and back paths now symmetric.Test plan
gdlint client/scripts/ client/ui/— zero warnings.godot --headless --path client --quit— no script errors.make game→ press M → atlas opens at the Reach hop-ring view. Click a system → orbital diagram. Click a body → body entry. Enter → heightmap viewer. ESC back through each to close.grep -r "implant/map/starchart" client/→ no hits.grep -r "atlas_panel\|economics_panel" client/→ no hits outside comments/history.v0.1.35 · protocol 21.tooling/db/sqlite-query --helpandtooling/db/sqlite-exec --helpprint usage text.Follow-up tickets
economics_link_requestedwire with a genericintent_requested(target_app, action, params)routed through anImplantIntentssibling of the registry.DataChannelsautoload — manifest-declared snapshot subscriptions so modded apps can subscribe to simulation feeds without pokingGameStatedirectly.Ticket #852 (zombie starchart retirement) is closed-as-done-in-this-PR since the legacy HudGroups path was deleted.
Establishes the ImplantApp faux-mobile-OS architecture (D-191): Foundation: - ImplantAppManifest (Resource): @export vars for app_path, scene_path, default_key, default_mode, preserves_state - ImplantNavStack (Node): push/pop/replace/reset_to_default, synchronous screen_changed signal - ImplantApp (Control base class): absorbs HudGroups boilerplate; on_install/on_open/on_close/on_insert_deactivated lifecycle hooks - ImplantRegistry (autoload): lazy-scans res://ui/implant/apps/*/app.tres; avoids autoload parse-order trap Atlas app (replaces atlas_panel + atlas_reach_map + atlas_system_map + atlas_planet_map + root viewer files): - apps/atlas/app.tres — manifest (implant/map, FULLSCREEN, key=M) - apps/atlas/atlas_app.gd — coordinator; 4-screen nav via ImplantNavStack - apps/atlas/screens/{reach,system,planet,regional}_screen.gd — enter/leave interface - apps/atlas/{atlas_viewer,atlas_marker_overlay,atlas_overlay_bar}.gd — moved from root Economics app (replaces economics_panel): - apps/economics/app.tres — manifest (implant/economics, INSERT, key=N) - apps/economics/economics_app.gd — thin shell delegating to OverviewScreen - apps/economics/screens/overview_screen.gd — full panel logic, enter/leave interface Wiring: - project.godot: add ImplantRegistry autoload after HudGroups - main.gd: registry-driven key toggle loop; rename atlas_panel→atlas_app, economics_panel→economics_app - hud.tscn: swap to new scene paths; remove legacy StarMap node - snapshot_consumers.gd: on_insert_deactivated() uniformly; rename vars - hud_groups.gd: remove stale starchart compat comment Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>feat(ui): Sprint 36 client — atlas reach map, version display, DB help flagsto feat(ui): sprint 36 client — ImplantApp pattern + atlas unification + toolingReview:
sprint-36/client→ main (type: code)Pre-review checks:
make gameverification — process gapHoshe (QA / bugs): REQUEST_CHANGES
Solid refactor with correctly-handled autoload parse-order trap, sound nav stack, and clean key routing. But
star_map.gd/.tscnwere not deleted with the atlas unification, a sprint-30 test now fails silently, and the registry's documented "first-wins, warn on startup" key-collision policy is unimplemented.Tyre (architecture): REQUEST_CHANGES
The ImplantApp pattern is a genuinely sound abstraction, but it's partial in ways that bake in moddability-blocking assumptions: the registry only drives key routing, not app lifecycle (
scene_pathis declared but unused; apps are still direct-instanced inhud.tscn),display_name/icon_pathare dead public fields, key-collision detection isn't implemented, zero test coverage on the new primitives, and the arch doc already drifts from reality on day one.Consolidated comments
client/ui/star_map.gd,client/ui/star_map.tscnstar_map.gd:122still registers deadimplant/map/starchartHudGroups path and declaresclass_name StarMapRenderer. Delete both files per D-191.client/tests/test_sprint30.gd:568-590test_star_map_is_accessible_from_insert_uiassertshud.get_node_or_null("StarMap") != null— now returns null; test will fail on CI. Delete (D-191 supersedes) or rewrite to assertAtlasApp.client/ui/implant/implant_registry.gd:7-43scene_path. Apps are still direct-instanced inhud.tscn:4-5, 19-25. Contradicts the arch doc's moddability promise. Either consumescene_pathand lazy-instance, or documentscene_pathas reserved-for-future-use.client/ui/implant/implant_registry.gd:17-36_scan()withpush_warning; also warn on invaliddefault_mode.implant_nav_stack.gd,implant_registry.gd,implant_app.gdtest_implant_nav_stack.gd(push/pop/replace/reset/push_default/empty-pop),test_implant_registry.gd(scan + collision),test_implant_app_lifecycle.gd(on_install → on_open → on_close order,preserves_stateboth values).client/ui/implant/implant_app_manifest.gd:6-12display_nameandicon_pathdeclared but consumed nowhere — dead public API; (b)default_keyembeds input concerns in app manifest (layering violation — call out with TODO); (c) no schema_version — add@export var schema_version: int = 1before mods ship against implicit v1.apps/atlas/atlas_app.gd:195-216+apps/economics/screens/overview_screen.gd:114-136STAR_MAP_DATA := "res://data/star_map_data.json"loader + sort-by-proper-name lambda. Extractclient/ui/implant/widgets/system_index.gd(arch doc reserves that dir).client/ui/implant/implant_app.gd:46-60on_insert_deactivated()auto-closes — correct for INSERT, wrong for FULLSCREEN. Gate onHudGroups.get_active_mode() == Mode.INSERT, or document that FULLSCREEN apps must override topass.client/ui/implant/implant_app.gd:36-60on_install→on_open→on_close) ordering vs nav-stack state is undocumented. Subclass puts "grab current screen" inon_install→ gets""; inon_open→ gets post-push id. Document the contract in the base class and arch doc.apps/atlas/atlas_app.gd:68-82+apps/economics/economics_app.gd:24-37_on_screen_changedare near-identical 14-line swap blocks — cargo-culted boilerplate the base class is meant to absorb. Move intoImplantAppvia_screens: Dictionary[String, Control]+register_screen(id, control).tooling/db/sqlite-query:4-14,tooling/db/sqlite-exec:4-14--helpaccidentally gets non-JSON → silent pipeline corruption. Emit help to stderr, or wrap as{"ok": false, "error": "...", "usage": "..."}.docs/architecture/implant-app-pattern.md:292vsclient/ui/star_map.gd:122implant/map/starchartis deleted, but the registration remains. Fix one side.client/ui/implant/implant_app.gd:14signal insert_deactivateddeclared but never emitted.on_insert_deactivated()is the actual hook. Remove the dead signal, or emit it from inside the hook.client/scripts/main.gd:198HudGroups.open_app → app_changed. Rewrite the comment accordingly.client/ui/implant/implant_nav_stack.gd:23-33pop()on last item silently pushes default →while not nav.is_empty(): nav.pop()loops forever. Document "pop never empties below default", or let it truly empty.client/ui/implant/implant_nav_stack.gd:17-41screen_changedhandler callingnav.push/popsynchronously can leave_current_screen_idstale in subclasses. Add in-emit deferral, or document the hazard.client/scripts/main.gd:181-186manifest.get("default_key")/.get("app_path")as if manifest were a dictionary. Cast + validate (as String, reject empty).client/scripts/main.gd:187-193[/]keys for economics nav hardcoded with hand-wave comment — contradicts moddability. Either route viaImplantApp.handle_global_key(event), or document the exception in the arch doc.apps/economics/app.tres:11,apps/atlas/app.tres:11default_mode = 2/= 1— bare HudGroups.Mode enum ints. Mod author has no discovery path. Use a String field with registry parsing, or companion .gd constants.client/ui/loading_screen.gd:58-72project.yamland server version fromProtocol.PROTOCOL_VERSIONare independent — mismatched builds display silently. Add intent-clarifying comment, or cross-check and surface mismatch.Verdict: CHANGES REQUESTED
All 20 comments are actionable. Please address each (or push back with a concrete technical rationale per the review-disagreement protocol).
Process note: no runtime smoke test (
make game) mentioned in the PR description or commits. For a refactor of this scope, launching the game and exercising atlas + economics end-to-end should happen before push — this is the Sprint 28 lesson.Addresses 6 mechanical issues from PR review: - ImplantAppManifest: drop dead display_name and icon_path fields. default_key carries a TODO noting its future migration to a keybinds manifest (input concern in app manifest is a layering violation, tracked explicitly). - default_mode wire format is now a String ("fullscreen" / "insert") for mod-author discovery. ImplantRegistry parses via _MODE_MAP, caches the resolved int in _resolved_modes, and exposes get_resolved_mode(app_path). main.gd reads the resolved int directly instead of re-parsing. - Extract client/ui/implant/widgets/system_index.gd (class_name SystemIndex, static get_sorted_systems). Removes duplicated star_map_data.json loader + sort lambda from atlas_app and economics overview_screen. - ImplantApp.on_insert_deactivated() default auto-closes only when the app is active in INSERT mode. FULLSCREEN apps no longer spuriously close on insert state changes. - tooling/db/sqlite-query and sqlite-exec: --help output goes to stderr (exit 0). Keeps stdout reserved for JSON payloads so JSON-parsing callers can't get silently corrupted. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>Review:
sprint-36/client→ main (round 2)Round 1 recap: 4 blockers, 8 issues, 8 nits (20 comments).
Team response: 7 commits organized as 4 internal rounds + docs + tests.
Hoshe (QA): APPROVE
All 7 Hoshe-relevant items resolved.
star_map.gd+star_map.tscndeleted (637 + 18 lines), brokentest_star_map_is_accessible_from_insert_uiremoved, registry collision detection firing viapush_warning,main.gd:208-210comment causality corrected, typedImplantAppManifestiteration with validatedapp_path, version-mismatch loading screen documented,sqlite-query/sqlite-exechelp routed to stderr (JSON stdout preserved). 736 lines of new tests hit real contracts (push/pop/replace/reset/empty-pop,preserves_stateboth values, INSERT-gated deactivation vs FULLSCREEN no-op, re-entrancy guard, duplicate screen id first-wins) — not happy-path-only.Minor non-blocking observations:
default_keyto exercise the collision-warn code path (shipped manifests have distinct keys 77/78).test_real_scan_no_duplicate_keysasserts the invariant but not the warn branch. Low priority.implant_nav_stack.gd:68-72—reset_to_default()lacks the_mutatingguard thatpush/pop/replacehave. Calling it from inside ascreen_changedhandler is an anti-pattern the docs explicitly warn against, but the guard pattern elsewhere creates an expectation of protection this method doesn't share.Tyre (architecture): APPROVE
Architectural rework is sound — the abstraction now matches the doc.
hud.tscn:1-23has zero app ExtResources, only anAppsContainerplacement node;hud.gd:13callsImplantRegistry.instantiate_all($AppsContainer)as the single seam;main.gdlooks up apps viaImplantRegistry.get_app_instance(...). A modder'sapp.treswith a validscene_pathnow produces a running app with zero core-code edits — the moddability promise is real. Manifestschema_versionadded (implant_app_manifest.gd:6) with forward-tolerant warn-don't-reject policy atimplant_registry.gd:79-90. String-valueddefault_mode("fullscreen"/"insert"/"gameplay") replaces bare enum ints via_MODE_MAP. Three-phase lifecycle explicitly documented in both the class doc-comment (implant_app.gd:6-37) and the arch doc (implant-app-pattern.md:191-200), including the "on_install sees empty stack" gotcha.widgets/system_index.gdextracted, atlas_app + overview_screen both migrated (zero remainingSTAR_MAP_DATAduplication). Nav re-entrancy via_mutatingflag with escape-hatch error message pointing atcall_deferred.Minor non-blocking observations:
[/]hardcoded inmain.gd:199-204) — partially resolved with named follow-up in the arch doc for ahandle_global_keylifecycle hook. Acceptable defer, but the next app needing intra-app keys will add more hardcodes before the hook lands.implant_registry.gd:112-119—_is_valid_manifestuses duck-typed"app_path" in mcheck. Fine (forward-tolerant drop-with-warn is intended behavior), flagging for future awareness.Overall verdict: APPROVED
All 4 blockers, 8 issues, and 8 nits from round 1 are resolved (one nit partially, with a documented follow-up). No new blockers. 736 lines of contract-pinning tests. Arch doc is now an honest accounting of the branch state.
Recommend merge.
Pull request closed